[PATCH] evh_bytechan: fix out of bounds accesses
ev_byte_channel_send() assumes that its third argument is a 16 byte array. Some places where it is called it may not be (or we can't easily tell if it is). Newer compilers have started producing warnings about this, so make sure we actually pass a 16 byte array. There may be more elegant solutions to this, but the driver is quite old and hasn't been updated in many years. The warnings (from a powerpc allyesconfig build) are: In file included from include/linux/byteorder/big_endian.h:5, from arch/powerpc/include/uapi/asm/byteorder.h:14, from include/asm-generic/bitops/le.h:6, from arch/powerpc/include/asm/bitops.h:250, from include/linux/bitops.h:29, from include/linux/kernel.h:12, from include/asm-generic/bug.h:19, from arch/powerpc/include/asm/bug.h:109, from include/linux/bug.h:5, from include/linux/mmdebug.h:5, from include/linux/gfp.h:5, from include/linux/slab.h:15, from drivers/tty/ehv_bytechan.c:24: drivers/tty/ehv_bytechan.c: In function ‘ehv_bc_udbg_putc’: arch/powerpc/include/asm/epapr_hcalls.h:298:20: warning: array subscript 1 is outside array bounds of ‘const char[1]’ [-Warray-bounds] 298 | r6 = be32_to_cpu(p[1]); include/uapi/linux/byteorder/big_endian.h:40:51: note: in definition of macro ‘__be32_to_cpu’ 40 | #define __be32_to_cpu(x) ((__force __u32)(__be32)(x)) | ^ arch/powerpc/include/asm/epapr_hcalls.h:298:7: note: in expansion of macro ‘be32_to_cpu’ 298 | r6 = be32_to_cpu(p[1]); | ^~~ drivers/tty/ehv_bytechan.c:166:13: note: while referencing ‘data’ 166 | static void ehv_bc_udbg_putc(char c) | ^~~~ In file included from include/linux/byteorder/big_endian.h:5, from arch/powerpc/include/uapi/asm/byteorder.h:14, from include/asm-generic/bitops/le.h:6, from arch/powerpc/include/asm/bitops.h:250, from include/linux/bitops.h:29, from include/linux/kernel.h:12, from include/asm-generic/bug.h:19, from arch/powerpc/include/asm/bug.h:109, from include/linux/bug.h:5, from include/linux/mmdebug.h:5, from include/linux/gfp.h:5, from include/linux/slab.h:15, from drivers/tty/ehv_bytechan.c:24: arch/powerpc/include/asm/epapr_hcalls.h:299:20: warning: array subscript 2 is outside array bounds of ‘const char[1]’ [-Warray-bounds] 299 | r7 = be32_to_cpu(p[2]); include/uapi/linux/byteorder/big_endian.h:40:51: note: in definition of macro ‘__be32_to_cpu’ 40 | #define __be32_to_cpu(x) ((__force __u32)(__be32)(x)) | ^ arch/powerpc/include/asm/epapr_hcalls.h:299:7: note: in expansion of macro ‘be32_to_cpu’ 299 | r7 = be32_to_cpu(p[2]); | ^~~ drivers/tty/ehv_bytechan.c:166:13: note: while referencing ‘data’ 166 | static void ehv_bc_udbg_putc(char c) | ^~~~ In file included from include/linux/byteorder/big_endian.h:5, from arch/powerpc/include/uapi/asm/byteorder.h:14, from include/asm-generic/bitops/le.h:6, from arch/powerpc/include/asm/bitops.h:250, from include/linux/bitops.h:29, from include/linux/kernel.h:12, from include/asm-generic/bug.h:19, from arch/powerpc/include/asm/bug.h:109, from include/linux/bug.h:5, from include/linux/mmdebug.h:5, from include/linux/gfp.h:5, from include/linux/slab.h:15, from drivers/tty/ehv_bytechan.c:24: arch/powerpc/include/asm/epapr_hcalls.h:300:20: warning: array subscript 3 is outside array bounds of ‘const char[1]’ [-Warray-bounds] 300 | r8 = be32_to_cpu(p[3]); include/uapi/linux/byteorder/big_endian.h:40:51: note: in definition of macro ‘__be32_to_cpu’ 40 | #define __be32_to_cpu(x) ((__force __u32)(__be32)(x)) | ^ arch/powerpc/include/asm/epapr_hcalls.h:300:7: note: in expansion of macro ‘be32_to_cpu’ 300 | r8 = be32_to_cpu(p[3]); | ^~~ drivers/tty/ehv_bytechan.c:166:13: note: while referencing ‘data’ 166 | static void ehv_bc_udbg_putc(char c) | ^~~~ In file included from include/linux/byteorder/big_endian.h:5, from arch/powerpc/include/uapi/asm/byteorder.h:14, from include/asm-generic/bitops/le.h:6, from arch/powerpc/include/asm/bitops.h:250, from include/linux/bitops.h:29, from include/linux/kernel.h:12, from
[PATCH v5 4/4] powerpc: Book3S 64-bit "heavyweight" KASAN support
KASAN support on Book3S is a bit tricky to get right: - It would be good to support inline instrumentation so as to be able to catch stack issues that cannot be caught with outline mode. - Inline instrumentation requires a fixed offset. - Book3S runs code in real mode after booting. Most notably a lot of KVM runs in real mode, and it would be good to be able to instrument it. - Because code runs in real mode after boot, the offset has to point to valid memory both in and out of real mode. [ppc64 mm note: The kernel installs a linear mapping at effective address c000... onward. This is a one-to-one mapping with physical memory from ... onward. Because of how memory accesses work on powerpc 64-bit Book3S, a kernel pointer in the linear map accesses the same memory both with translations on (accessing as an 'effective address'), and with translations off (accessing as a 'real address'). This works in both guests and the hypervisor. For more details, see s5.7 of Book III of version 3 of the ISA, in particular the Storage Control Overview, s5.7.3, and s5.7.5 - noting that this KASAN implementation currently only supports Radix.] One approach is just to give up on inline instrumentation. This way all checks can be delayed until after everything set is up correctly, and the address-to-shadow calculations can be overridden. However, the features and speed boost provided by inline instrumentation are worth trying to do better. If _at compile time_ it is known how much contiguous physical memory a system has, the top 1/8th of the first block of physical memory can be set aside for the shadow. This is a big hammer and comes with 3 big consequences: - there's no nice way to handle physically discontiguous memory, so only the first physical memory block can be used. - kernels will simply fail to boot on machines with less memory than specified when compiling. - kernels running on machines with more memory than specified when compiling will simply ignore the extra memory. Implement and document KASAN this way. The current implementation is Radix only. Despite the limitations, it can still find bugs, e.g. http://patchwork.ozlabs.org/patch/1103775/ At the moment, this physical memory limit must be set _even for outline mode_. This may be changed in a later series - a different implementation could be added for outline mode that dynamically allocates shadow at a fixed offset. For example, see https://patchwork.ozlabs.org/patch/795211/ Suggested-by: Michael Ellerman Cc: Balbir Singh # ppc64 out-of-line radix version Cc: Christophe Leroy # ppc32 version Signed-off-by: Daniel Axtens --- Changes since v4: - fix some ppc32 build issues - support ptdump - clean up the header file. It turns out we don't need or use KASAN_SHADOW_SIZE, so just dump it, and make KASAN_SHADOW_END the thing that varies between 32 and 64 bit. As part of this, make sure KASAN_SHADOW_OFFSET is only configured for 32 bit - it is calculated in the Makefile for ppc64. - various cleanups Changes since v3: - Address further feedback from Christophe. - Drop changes to stack walking, it looks like the issue I observed is related to that particular stack, not stack-walking generally. Changes since v2: - Address feedback from Christophe around cleanups and docs. - Address feedback from Balbir: at this point I don't have a good solution for the issues you identify around the limitations of the inline implementation but I think that it's worth trying to get the stack instrumentation support. I'm happy to have an alternative and more flexible outline mode - I had envisoned this would be called 'lightweight' mode as it imposes fewer restrictions. I've linked to your implementation. I think it's best to add it in a follow-up series. - Made the default PHYS_MEM_SIZE_FOR_KASAN value 1024MB. I think most people have guests with at least that much memory in the Radix 64s case so it's a much saner default - it means that if you just turn on KASAN without reading the docs you're much more likely to have a bootable kernel, which you will never have if the value is set to zero! I'm happy to bikeshed the value if we want. Changes since v1: - Landed kasan vmalloc support upstream - Lots of feedback from Christophe. Changes since the rfc: - Boots real and virtual hardware, kvm works. - disabled reporting when we're checking the stack for exception frames. The behaviour isn't wrong, just incompatible with KASAN. - Documentation! - Dropped old module stuff in favour of KASAN_VMALLOC. The bugs with ftrace and kuap were due to kernel bloat pushing prom_init calls to be done via the plt. Because we did not have a relocatable kernel, and they are done very early, this caused everything to explode. Compile with CONFIG_RELOCATABLE! --- Documentation/dev-tools/kasan.rst| 8 +- Documentation/powerpc/kasan.txt
[PATCH v5 3/4] powerpc/mm/kasan: rename kasan_init_32.c to init_32.c
kasan is already implied by the directory name, we don't need to
repeat it.
Suggested-by: Christophe Leroy
Signed-off-by: Daniel Axtens
---
arch/powerpc/mm/kasan/Makefile | 2 +-
arch/powerpc/mm/kasan/{kasan_init_32.c => init_32.c} | 0
2 files changed, 1 insertion(+), 1 deletion(-)
rename arch/powerpc/mm/kasan/{kasan_init_32.c => init_32.c} (100%)
diff --git a/arch/powerpc/mm/kasan/Makefile b/arch/powerpc/mm/kasan/Makefile
index 6577897673dd..36a4e1b10b2d 100644
--- a/arch/powerpc/mm/kasan/Makefile
+++ b/arch/powerpc/mm/kasan/Makefile
@@ -2,4 +2,4 @@
KASAN_SANITIZE := n
-obj-$(CONFIG_PPC32) += kasan_init_32.o
+obj-$(CONFIG_PPC32) += init_32.o
diff --git a/arch/powerpc/mm/kasan/kasan_init_32.c
b/arch/powerpc/mm/kasan/init_32.c
similarity index 100%
rename from arch/powerpc/mm/kasan/kasan_init_32.c
rename to arch/powerpc/mm/kasan/init_32.c
--
2.20.1
[PATCH v5 2/4] kasan: Document support on 32-bit powerpc
KASAN is supported on 32-bit powerpc and the docs should reflect this. Suggested-by: Christophe Leroy Reviewed-by: Christophe Leroy Signed-off-by: Daniel Axtens --- Documentation/dev-tools/kasan.rst | 3 ++- Documentation/powerpc/kasan.txt | 12 2 files changed, 14 insertions(+), 1 deletion(-) create mode 100644 Documentation/powerpc/kasan.txt diff --git a/Documentation/dev-tools/kasan.rst b/Documentation/dev-tools/kasan.rst index e4d66e7c50de..4af2b5d2c9b4 100644 --- a/Documentation/dev-tools/kasan.rst +++ b/Documentation/dev-tools/kasan.rst @@ -22,7 +22,8 @@ global variables yet. Tag-based KASAN is only supported in Clang and requires version 7.0.0 or later. Currently generic KASAN is supported for the x86_64, arm64, xtensa and s390 -architectures, and tag-based KASAN is supported only for arm64. +architectures. It is also supported on 32-bit powerpc kernels. Tag-based KASAN +is supported only on arm64. Usage - diff --git a/Documentation/powerpc/kasan.txt b/Documentation/powerpc/kasan.txt new file mode 100644 index ..a85ce2ff8244 --- /dev/null +++ b/Documentation/powerpc/kasan.txt @@ -0,0 +1,12 @@ +KASAN is supported on powerpc on 32-bit only. + +32 bit support +== + +KASAN is supported on both hash and nohash MMUs on 32-bit. + +The shadow area sits at the top of the kernel virtual memory space above the +fixmap area and occupies one eighth of the total kernel virtual memory space. + +Instrumentation of the vmalloc area is not currently supported, but modules +are. -- 2.20.1
[PATCH v5 1/4] kasan: define and use MAX_PTRS_PER_* for early shadow tables
powerpc has a variable number of PTRS_PER_*, set at runtime based
on the MMU that the kernel is booted under.
This means the PTRS_PER_* are no longer constants, and therefore
breaks the build.
Define default MAX_PTRS_PER_*s in the same style as MAX_PTRS_PER_P4D.
As KASAN is the only user at the moment, just define them in the kasan
header, and have them default to PTRS_PER_* unless overridden in arch
code.
Suggested-by: Christophe Leroy
Suggested-by: Balbir Singh
Reviewed-by: Christophe Leroy
Reviewed-by: Balbir Singh
Signed-off-by: Daniel Axtens
---
include/linux/kasan.h | 18 +++---
mm/kasan/init.c | 6 +++---
2 files changed, 18 insertions(+), 6 deletions(-)
diff --git a/include/linux/kasan.h b/include/linux/kasan.h
index e18fe54969e9..70865810d0e7 100644
--- a/include/linux/kasan.h
+++ b/include/linux/kasan.h
@@ -14,10 +14,22 @@ struct task_struct;
#include
#include
+#ifndef MAX_PTRS_PER_PTE
+#define MAX_PTRS_PER_PTE PTRS_PER_PTE
+#endif
+
+#ifndef MAX_PTRS_PER_PMD
+#define MAX_PTRS_PER_PMD PTRS_PER_PMD
+#endif
+
+#ifndef MAX_PTRS_PER_PUD
+#define MAX_PTRS_PER_PUD PTRS_PER_PUD
+#endif
+
extern unsigned char kasan_early_shadow_page[PAGE_SIZE];
-extern pte_t kasan_early_shadow_pte[PTRS_PER_PTE];
-extern pmd_t kasan_early_shadow_pmd[PTRS_PER_PMD];
-extern pud_t kasan_early_shadow_pud[PTRS_PER_PUD];
+extern pte_t kasan_early_shadow_pte[MAX_PTRS_PER_PTE];
+extern pmd_t kasan_early_shadow_pmd[MAX_PTRS_PER_PMD];
+extern pud_t kasan_early_shadow_pud[MAX_PTRS_PER_PUD];
extern p4d_t kasan_early_shadow_p4d[MAX_PTRS_PER_P4D];
int kasan_populate_early_shadow(const void *shadow_start,
diff --git a/mm/kasan/init.c b/mm/kasan/init.c
index ce45c491ebcd..8b54a96d3b3e 100644
--- a/mm/kasan/init.c
+++ b/mm/kasan/init.c
@@ -46,7 +46,7 @@ static inline bool kasan_p4d_table(pgd_t pgd)
}
#endif
#if CONFIG_PGTABLE_LEVELS > 3
-pud_t kasan_early_shadow_pud[PTRS_PER_PUD] __page_aligned_bss;
+pud_t kasan_early_shadow_pud[MAX_PTRS_PER_PUD] __page_aligned_bss;
static inline bool kasan_pud_table(p4d_t p4d)
{
return p4d_page(p4d) == virt_to_page(lm_alias(kasan_early_shadow_pud));
@@ -58,7 +58,7 @@ static inline bool kasan_pud_table(p4d_t p4d)
}
#endif
#if CONFIG_PGTABLE_LEVELS > 2
-pmd_t kasan_early_shadow_pmd[PTRS_PER_PMD] __page_aligned_bss;
+pmd_t kasan_early_shadow_pmd[MAX_PTRS_PER_PMD] __page_aligned_bss;
static inline bool kasan_pmd_table(pud_t pud)
{
return pud_page(pud) == virt_to_page(lm_alias(kasan_early_shadow_pmd));
@@ -69,7 +69,7 @@ static inline bool kasan_pmd_table(pud_t pud)
return false;
}
#endif
-pte_t kasan_early_shadow_pte[PTRS_PER_PTE] __page_aligned_bss;
+pte_t kasan_early_shadow_pte[MAX_PTRS_PER_PTE] __page_aligned_bss;
static inline bool kasan_pte_table(pmd_t pmd)
{
--
2.20.1
[PATCH v5 0/4] KASAN for powerpc64 radix
Building on the work of Christophe, Aneesh and Balbir, I've ported
KASAN to 64-bit Book3S kernels running on the Radix MMU.
This provides full inline instrumentation on radix, but does require
that you be able to specify the amount of physically contiguous memory
on the system at compile time. More details in patch 4.
v5: ptdump support. More cleanups, tweaks and fixes, thanks
Christophe. Details in patch 4.
I have seen another stack walk splat, but I don't think it's
related to the patch set, I think there's a bug somewhere else,
probably in stack frame manipulation in the kernel or (more
unlikely) in the compiler.
v4: More cleanups, split renaming out, clarify bits and bobs.
Drop the stack walk disablement, that isn't needed. No other
functional change.
v3: Reduce the overly ambitious scope of the MAX_PTRS change.
Document more things, including around why some of the
restrictions apply.
Clean up the code more, thanks Christophe.
v2: The big change is the introduction of tree-wide(ish)
MAX_PTRS_PER_{PTE,PMD,PUD} macros in preference to the previous
approach, which was for the arch to override the page table array
definitions with their own. (And I squashed the annoying
intermittent crash!)
Apart from that there's just a lot of cleanup. Christophe, I've
addressed most of what you asked for and I will reply to your v1
emails to clarify what remains unchanged.
Daniel Axtens (4):
kasan: define and use MAX_PTRS_PER_* for early shadow tables
kasan: Document support on 32-bit powerpc
powerpc/mm/kasan: rename kasan_init_32.c to init_32.c
powerpc: Book3S 64-bit "heavyweight" KASAN support
Documentation/dev-tools/kasan.rst | 7 +-
Documentation/powerpc/kasan.txt | 122 ++
arch/powerpc/Kconfig | 2 +
arch/powerpc/Kconfig.debug| 23 +++-
arch/powerpc/Makefile | 11 ++
arch/powerpc/include/asm/book3s/64/hash.h | 4 +
arch/powerpc/include/asm/book3s/64/pgtable.h | 7 +
arch/powerpc/include/asm/book3s/64/radix.h| 5 +
arch/powerpc/include/asm/kasan.h | 15 ++-
arch/powerpc/kernel/prom.c| 61 -
arch/powerpc/mm/kasan/Makefile| 3 +-
.../mm/kasan/{kasan_init_32.c => init_32.c} | 0
arch/powerpc/mm/kasan/init_book3s_64.c| 71 ++
arch/powerpc/mm/ptdump/ptdump.c | 10 +-
arch/powerpc/platforms/Kconfig.cputype| 1 +
include/linux/kasan.h | 18 ++-
mm/kasan/init.c | 6 +-
17 files changed, 350 insertions(+), 16 deletions(-)
create mode 100644 Documentation/powerpc/kasan.txt
rename arch/powerpc/mm/kasan/{kasan_init_32.c => init_32.c} (100%)
create mode 100644 arch/powerpc/mm/kasan/init_book3s_64.c
--
2.20.1
RE: [PATCH v2 5/9] arc: Constify ioreadX() iomem argument (as in generic implementation)
Hi Krzysztof, > The ioreadX() helpers have inconsistent interface. On some architectures > void *__iomem address argument is a pointer to const, on some not. > > Implementations of ioreadX() do not modify the memory under the > address so they can be converted to a "const" version for const-safety > and consistency among architectures. Thanks for this clean-up. Looks good to me, so ... Acked-by: Alexey Brodkin
Re: [PATCH V6 3/4] ASoC: pcm_dmaengine: Extract snd_dmaengine_pcm_refine_runtime_hwparams
On Thu, Sep 26, 2019 at 6:50 PM Shengjiu Wang wrote:
>
> When set the runtime hardware parameters, we may need to query
> the capability of DMA to complete the parameters.
>
> This patch is to Extract this operation from
> dmaengine_pcm_set_runtime_hwparams function to a separate function
> snd_dmaengine_pcm_refine_runtime_hwparams, that other components
> which need this feature can call this function.
>
> Signed-off-by: Shengjiu Wang
> Reviewed-by: Nicolin Chen
As a heads up, this patch seems to be causing a regression on the HiKey board.
On boot up I'm seeing:
[ 17.721424] hi6210_i2s f7118000.i2s: ASoC: can't open component
f7118000.i2s: -6
And HDMI audio isn't working. With this patch reverted, audio works again.
> diff --git a/sound/core/pcm_dmaengine.c b/sound/core/pcm_dmaengine.c
> index 89a05926ac73..5749a8a49784 100644
> --- a/sound/core/pcm_dmaengine.c
> +++ b/sound/core/pcm_dmaengine.c
> @@ -369,4 +369,87 @@ int snd_dmaengine_pcm_close_release_chan(struct
> snd_pcm_substream *substream)
...
> + ret = dma_get_slave_caps(chan, _caps);
> + if (ret == 0) {
> + if (dma_caps.cmd_pause && dma_caps.cmd_resume)
> + hw->info |= SNDRV_PCM_INFO_PAUSE |
> SNDRV_PCM_INFO_RESUME;
> + if (dma_caps.residue_granularity <=
> DMA_RESIDUE_GRANULARITY_SEGMENT)
> + hw->info |= SNDRV_PCM_INFO_BATCH;
> +
> + if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
> + addr_widths = dma_caps.dst_addr_widths;
> + else
> + addr_widths = dma_caps.src_addr_widths;
> + }
It seems a failing ret from dma_get_slave_caps() here is being returned...
> +
> + /*
> +* If SND_DMAENGINE_PCM_DAI_FLAG_PACK is set keep
> +* hw.formats set to 0, meaning no restrictions are in place.
> +* In this case it's the responsibility of the DAI driver to
> +* provide the supported format information.
> +*/
> + if (!(dma_data->flags & SND_DMAENGINE_PCM_DAI_FLAG_PACK))
> + /*
> +* Prepare formats mask for valid/allowed sample types. If the
> +* dma does not have support for the given physical word size,
> +* it needs to be masked out so user space can not use the
> +* format which produces corrupted audio.
> +* In case the dma driver does not implement the slave_caps
> the
> +* default assumption is that it supports 1, 2 and 4 bytes
> +* widths.
> +*/
> + for (i = SNDRV_PCM_FORMAT_FIRST; i <= SNDRV_PCM_FORMAT_LAST;
> i++) {
> + int bits = snd_pcm_format_physical_width(i);
> +
> + /*
> +* Enable only samples with DMA supported physical
> +* widths
> +*/
> + switch (bits) {
> + case 8:
> + case 16:
> + case 24:
> + case 32:
> + case 64:
> + if (addr_widths & (1 << (bits / 8)))
> + hw->formats |= pcm_format_to_bits(i);
> + break;
> + default:
> + /* Unsupported types */
> + break;
> + }
> + }
> +
> + return ret;
... down here.
Where as in the old code...
> diff --git a/sound/soc/soc-generic-dmaengine-pcm.c
> b/sound/soc/soc-generic-dmaengine-pcm.c
> index 748f5f641002..b9f147eaf7c4 100644
> --- a/sound/soc/soc-generic-dmaengine-pcm.c
> +++ b/sound/soc/soc-generic-dmaengine-pcm.c
> @@ -145,56 +140,12 @@ static int dmaengine_pcm_set_runtime_hwparams(struct
> snd_pcm_substream *substrea
> if (pcm->flags & SND_DMAENGINE_PCM_FLAG_NO_RESIDUE)
> hw.info |= SNDRV_PCM_INFO_BATCH;
>
> - ret = dma_get_slave_caps(chan, _caps);
> - if (ret == 0) {
> - if (dma_caps.cmd_pause && dma_caps.cmd_resume)
> - hw.info |= SNDRV_PCM_INFO_PAUSE |
> SNDRV_PCM_INFO_RESUME;
> - if (dma_caps.residue_granularity <=
> DMA_RESIDUE_GRANULARITY_SEGMENT)
> - hw.info |= SNDRV_PCM_INFO_BATCH;
> -
> - if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
> - addr_widths = dma_caps.dst_addr_widths;
> - else
> - addr_widths = dma_caps.src_addr_widths;
> - }
...the ret from dma_get_slave_caps() checked above, but is not
actually returned.
Suggestions on how to sort this out?
thanks
-john
[Bug 206049] alg: skcipher: p8_aes_xts encryption unexpectedly succeeded on test vector "random: len=0 klen=64"; expected_error=-22, cfg="random: inplace may_sleep use_finup src_divs=[66.99%@+1
https://bugzilla.kernel.org/show_bug.cgi?id=206049 Michael Ellerman (mich...@ellerman.id.au) changed: What|Removed |Added Status|NEEDINFO|RESOLVED Resolution|--- |CODE_FIX -- You are receiving this mail because: You are watching the assignee of the bug.
RE: Freescale network device not activated on mpc8360 (kmeter1 board)
Hi Heiner, thank you for the quick answer. > > Hi all, > > > > With the introduction of the following patch, we are facing an issue with > the activation of the Freescale network device (ucc_geth driver) on our > kmeter1 board based on a MPC8360: > > +Li as ucc_geth maintainer > > Can you describe the symptoms of the issue? I am trying to boot in NFS, but as soon as the boot process is finished there is no network connections between the board and the host. > > > > commit 124eee3f6955f7aa19b9e6ff5c9b6d37cb3d1e2c > > Author: Heiner Kallweit > > Date: Tue Sep 18 21:55:36 2018 +0200 > > > > net: linkwatch: add check for netdevice being present to > > linkwatch_do_dev > > > > Based on my observations, just before trying to activate the device through > linkwatch_event, the controller wants to adjust the MAC configuration and in > order to achieve this it detaches the device. This avoids the activation of > the > net device. > > > It sounds a little bit odd to rely on an asynchronous linkwatch event here. > Can you give a call trace? Here is a call trace form the adjust_link function in the if condition at line 1644 (ucc_geth.c file): CPU: 0 PID: 35 Comm: kworker/0:1 Not tainted 5.4.8-dirty #19 Workqueue: events_power_efficient phy_state_machine Call Trace: [cf88bde8] [c02ddca8] adjust_link+0x304/0x320 (unreliable) [cf88be28] [c02cbf3c] phy_check_link_status+0xe4/0xfc [cf88be48] [c02cccdc] phy_state_machine+0x44/0x170 [cf88be78] [c00361a0] process_one_work+0x264/0x408 [cf88bea8] [c00370f8] worker_thread+0x140/0x53c [cf88bef8] [c003d818] kthread+0xdc/0x108 [cf88bf38] [c0010274] ret_from_kernel_thread+0x14/0x1c Here the call trace from the netif_carrier_on function just before the call to the linkwatch_fire_event function (line 498, sch_generic.c file): CPU: 0 PID: 35 Comm: kworker/0:1 Not tainted 5.4.8-dirty #20 Workqueue: events_power_efficient phy_state_machine Call Trace: [cf88bde8] [c0352064] netif_carrier_on+0xc4/0xc8 (unreliable) [cf88be08] [c02cf4ec] phy_link_change+0x84/0xb4 [cf88be28] [c02cbf3c] phy_check_link_status+0xe4/0xfc [cf88be48] [c02cccdc] phy_state_machine+0x44/0x170 [cf88be78] [c00361a0] process_one_work+0x264/0x408 [cf88bea8] [c00370f8] worker_thread+0x140/0x53c [cf88bef8] [c003d818] kthread+0xdc/0x108 [cf88bf38] [c0010274] ret_from_kernel_thread+0x14/0x1c Moreover, I noticed that by adding the dump directly in the linkwatch_do_dev function (link_watch.c) the interface comes up correctly, because of the delay introduced by the dump_stack function. Here another log with some prints that maybe can help to understand the situation. The prints are placed just before calling the function mentioned in the second part of the message (hopefully this will not bring more confusion): <...> ubi0: available PEBs: 235, total reserved PEBs: 269, PEBs reserved for bad PEB handling: 0 ubi0: background thread "ubi_bgt0d" started, PID 45 # [phy_device.c] phy_link_change - calling netif_carrier_on (eth2) # [sched_generic.c] netif_carrier_on - calling linkwatch_fire_event (eth2) # [phy_device.c] phy_link_change - calling adjust_link (eth2) # [ucc_geth.c] adjust_link - calling ugeth_quiesce (detaching device) (eth2) # [link_watch.c] linkwatch_do_dev - checking for netif_device_present(eth2) => 0 IP-Config: Guessing netmask 255.255.255.0 IP-Config: Complete: device=eth2, hwaddr=00:e0:df:56:54:07, ipaddr=192.168.1.20, mask=255.255.255.0, gw=255.255.255.255 host=kmeter1, domain=, nis-domain=(none) bootserver=192.168.1.100, rootserver=192.168.1.100, rootpath= # [ucc_geth.c] adjust_link - calling ugeth_activate (attaching device) (eth2) ucc_geth e0103200.ucc eth2: Link is Up - 100Mbps/Full - flow control off rpcbind: server 192.168.1.100 not responding, timed out rpcbind: server 192.168.1.100 not responding, timed out As mentioned, just before that the linkwatch checks for the net_device presence, this one is detached by the ucc_geth driver and reattached later. > The driver is quite old and maybe some parts need to be improved. The > referenced change is more than a year old and I'm not aware of any other > problem with it. So it seems the change isn't wrong. I agree. I pointed out the commit by bisecting. This gave me the direction to where the problem could be. > > This is already happening with older versions (I checked with the v4.14.162) > and also there the situation is the same, but without the additional check in > the if condition the device is activated. > > > > I am currently working with the v5.4.8 kernel version, but the behavior > remains the same also with the latest v5.5-rc4. > > > > Any idea how to solve this? Any help is appreciated. > > > > Regards, > > Matteo > > > Heiner Matteo
Re: [PATCH 2/3] powerpc sstep: add support for divde[.] and divdeu[.] instructions
On Tue, Dec 10, 2019 at 12:49:03PM +0530, Balamuruhan S wrote:
> This patch adds emulation support for divde, divdeu instructions,
> * Divide Doubleword Extended (divde[.])
> * Divide Doubleword Extended Unsigned (divdeu[.])
>
> Signed-off-by: Balamuruhan S
> ---
> arch/powerpc/lib/sstep.c | 27 ++-
> 1 file changed, 26 insertions(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/lib/sstep.c b/arch/powerpc/lib/sstep.c
> index c077acb983a1..4b4119729e59 100644
> --- a/arch/powerpc/lib/sstep.c
> +++ b/arch/powerpc/lib/sstep.c
> @@ -1736,7 +1736,32 @@ int analyse_instr(struct instruction_op *op, const
> struct pt_regs *regs,
> op->val = (int) regs->gpr[ra] /
> (int) regs->gpr[rb];
> goto arith_done;
> -
> +#ifdef __powerpc64__
> + case 425: /* divde[.] */
> + if (instr & 1) {
> + asm volatile(PPC_DIVDE_DOT(%0, %1, %2) :
> + "=r" (op->val) : "r" (regs->gpr[ra]),
> + "r" (regs->gpr[rb]));
> + set_cr0(regs, op);
This seems unneccesarily complicated. You take the trouble to do a
"divde." instruction rather than a "divde" instruction but then don't
use the CR0 setting that the instruction did, but instead go and work
out what happens to CR0 manually in set_cr0(). Also you don't tell
the compiler that CR0 has been modified, which could lead to problems.
This case could be done much more simply like this:
case 425: /* divde[.] */
asm volatile(PPC_DIVDE(%0, %1, %2) :
"=r" (op->val) : "r" (regs->gpr[ra]),
"r" (regs->gpr[rb]));
goto arith_done;
(note, goto arith_done rather than compute_done) and similarly for the
divdeu case.
Paul.
Re: [PATCH -next] soc: fsl: qe: remove set but not used variable 'mm_gc'
On Wed, Jan 8, 2020 at 7:12 AM YueHaibing wrote:
>
> drivers/soc/fsl/qe/gpio.c: In function qe_pin_request:
> drivers/soc/fsl/qe/gpio.c:163:26: warning: variable mm_gc set but not used
> [-Wunused-but-set-variable]
>
> commit 1e714e54b5ca ("powerpc: qe_lib-gpio: use gpiochip data pointer")
> left behind this unused variable.
>
> Reported-by: Hulk Robot
> Signed-off-by: YueHaibing
Applied for next. Thanks.
Btw, I find another patch from Chen Zhou fixing the same problem sent
earlier. I will add his signed-off-by to the commit for credit too.
Regards,
Leo
> ---
> drivers/soc/fsl/qe/gpio.c | 2 --
> 1 file changed, 2 deletions(-)
>
> diff --git a/drivers/soc/fsl/qe/gpio.c b/drivers/soc/fsl/qe/gpio.c
> index 12bdfd9..ed75198 100644
> --- a/drivers/soc/fsl/qe/gpio.c
> +++ b/drivers/soc/fsl/qe/gpio.c
> @@ -160,7 +160,6 @@ struct qe_pin *qe_pin_request(struct device_node *np, int
> index)
> {
> struct qe_pin *qe_pin;
> struct gpio_chip *gc;
> - struct of_mm_gpio_chip *mm_gc;
> struct qe_gpio_chip *qe_gc;
> int err;
> unsigned long flags;
> @@ -186,7 +185,6 @@ struct qe_pin *qe_pin_request(struct device_node *np, int
> index)
> goto err0;
> }
>
> - mm_gc = to_of_mm_gpio_chip(gc);
> qe_gc = gpiochip_get_data(gc);
>
> spin_lock_irqsave(_gc->lock, flags);
> --
> 2.7.4
>
>
RE: [RFT 00/13] iomap: Constify ioreadX() iomem argument
From: Christophe Leroy > Sent: 08 January 2020 08:49 ... > And as pointed by Arnd, the volatile is really only necessary for the > dereference itself, should the arch use dereferencing. I've had trouble with some versions of gcc and reading of 'volatile unsigned char *'. It tended to follow the memory read with an extra mask with 0xff. (I suspect that internally the value landed into a temporary 'int' variable.) I got better code using memory barriers. So putting an asm barrier for the exact location of the memory read either side of the read should have the desired effect without adding extra instructions. (You might think 'volatile' would mean that - but it doesn't.) David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
Re: [PATCH v2 1/9] iomap: Constify ioreadX() iomem argument (as in generic implementation)
On Wed, Jan 8, 2020 at 9:05 PM Krzysztof Kozlowski wrote: > > The ioreadX() and ioreadX_rep() helpers have inconsistent interface. On > some architectures void *__iomem address argument is a pointer to const, > on some not. > > Implementations of ioreadX() do not modify the memory under the address > so they can be converted to a "const" version for const-safety and > consistency among architectures. > > Suggested-by: Geert Uytterhoeven > Signed-off-by: Krzysztof Kozlowski > Reviewed-by: Geert Uytterhoeven Thanks for getting this done! Reviewed-by: Arnd Bergmann > Changes since v1: > 1. Constify also ioreadX_rep() and mmio_insX(), > 2. Squash lib+alpha+powerpc+parisc+sh into one patch for bisectability, The alpha and parisc versions should be independent, I was thinking you leave them as separate patches, but this work for me too. I have no real opinion on the other 8 patches, I would have left them out completely, but they don't hurt either. Arnd
Re: [PATCH v2 3/9] ntb: intel: Constify ioreadX() iomem argument (as in generic implementation)
On 1/8/20 1:05 PM, Krzysztof Kozlowski wrote:
The ioreadX() helpers have inconsistent interface. On some architectures
void *__iomem address argument is a pointer to const, on some not.
Implementations of ioreadX() do not modify the memory under the address
so they can be converted to a "const" version for const-safety and
consistency among architectures.
Signed-off-by: Krzysztof Kozlowski
Reviewed-by: Geert Uytterhoeven
Acked-by: Dave Jiang
---
Changes since v1:
1. Add Geert's review.
---
drivers/ntb/hw/intel/ntb_hw_gen1.c | 2 +-
drivers/ntb/hw/intel/ntb_hw_gen3.h | 2 +-
drivers/ntb/hw/intel/ntb_hw_intel.h | 2 +-
3 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/ntb/hw/intel/ntb_hw_gen1.c
b/drivers/ntb/hw/intel/ntb_hw_gen1.c
index bb57ec239029..9202502a9787 100644
--- a/drivers/ntb/hw/intel/ntb_hw_gen1.c
+++ b/drivers/ntb/hw/intel/ntb_hw_gen1.c
@@ -1202,7 +1202,7 @@ int intel_ntb_peer_spad_write(struct ntb_dev *ntb, int
pidx, int sidx,
ndev->peer_reg->spad);
}
-static u64 xeon_db_ioread(void __iomem *mmio)
+static u64 xeon_db_ioread(const void __iomem *mmio)
{
return (u64)ioread16(mmio);
}
diff --git a/drivers/ntb/hw/intel/ntb_hw_gen3.h
b/drivers/ntb/hw/intel/ntb_hw_gen3.h
index 75fb86ca27bb..d1455f24ec99 100644
--- a/drivers/ntb/hw/intel/ntb_hw_gen3.h
+++ b/drivers/ntb/hw/intel/ntb_hw_gen3.h
@@ -91,7 +91,7 @@
#define GEN3_DB_TOTAL_SHIFT 33
#define GEN3_SPAD_COUNT 16
-static inline u64 gen3_db_ioread(void __iomem *mmio)
+static inline u64 gen3_db_ioread(const void __iomem *mmio)
{
return ioread64(mmio);
}
diff --git a/drivers/ntb/hw/intel/ntb_hw_intel.h
b/drivers/ntb/hw/intel/ntb_hw_intel.h
index e071e28bca3f..3c0a5a2da241 100644
--- a/drivers/ntb/hw/intel/ntb_hw_intel.h
+++ b/drivers/ntb/hw/intel/ntb_hw_intel.h
@@ -102,7 +102,7 @@ struct intel_ntb_dev;
struct intel_ntb_reg {
int (*poll_link)(struct intel_ntb_dev *ndev);
int (*link_is_up)(struct intel_ntb_dev *ndev);
- u64 (*db_ioread)(void __iomem *mmio);
+ u64 (*db_ioread)(const void __iomem *mmio);
void (*db_iowrite)(u64 db_bits, void __iomem *mmio);
unsigned long ntb_ctl;
resource_size_t db_size;
Re: [PATCH v2 01/11] powerpc/powernv/ioda: Fix ref count for devices with their own PE
On 9/1/20 1:11 am, Frederic Barrat wrote: It took me a while to parse exactly what you're doing here. - In pnv_ioda_setup_dev_PE(), we take a reference on the pci_dev, this is to protect a pointer in the pci_dn structure, but not to protect the pointer in the pci_dev structure (which doesn't need to be protected by taking a reference, because the lifetime of the pnv_ioda_pe is the same as the pci_dev). - The pointer in the pci_dn structure has now been removed, so we should remove the pci_dev_get() accordingly. Correct. Did I do such a bad job explaining it in the commit message that I need to rephrase? I might just be a bit slow :) -- Andrew Donnellan OzLabs, ADL Canberra a...@linux.ibm.com IBM Australia Limited
[PATCH v2 9/9] net: wireless: ath5k: Constify ioreadX() iomem argument (as in generic implementation)
The ioreadX() helpers have inconsistent interface. On some architectures
void *__iomem address argument is a pointer to const, on some not.
Implementations of ioreadX() do not modify the memory under the address
so they can be converted to a "const" version for const-safety and
consistency among architectures.
Signed-off-by: Krzysztof Kozlowski
---
drivers/net/wireless/ath/ath5k/ahb.c | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/net/wireless/ath/ath5k/ahb.c
b/drivers/net/wireless/ath/ath5k/ahb.c
index 2c9cec8b53d9..8bd01df369fb 100644
--- a/drivers/net/wireless/ath/ath5k/ahb.c
+++ b/drivers/net/wireless/ath/ath5k/ahb.c
@@ -138,18 +138,18 @@ static int ath_ahb_probe(struct platform_device *pdev)
if (bcfg->devid >= AR5K_SREV_AR2315_R6) {
/* Enable WMAC AHB arbitration */
- reg = ioread32((void __iomem *) AR5K_AR2315_AHB_ARB_CTL);
+ reg = ioread32((const void __iomem *) AR5K_AR2315_AHB_ARB_CTL);
reg |= AR5K_AR2315_AHB_ARB_CTL_WLAN;
iowrite32(reg, (void __iomem *) AR5K_AR2315_AHB_ARB_CTL);
/* Enable global WMAC swapping */
- reg = ioread32((void __iomem *) AR5K_AR2315_BYTESWAP);
+ reg = ioread32((const void __iomem *) AR5K_AR2315_BYTESWAP);
reg |= AR5K_AR2315_BYTESWAP_WMAC;
iowrite32(reg, (void __iomem *) AR5K_AR2315_BYTESWAP);
} else {
/* Enable WMAC DMA access (assuming 5312 or 231x*/
/* TODO: check other platforms */
- reg = ioread32((void __iomem *) AR5K_AR5312_ENABLE);
+ reg = ioread32((const void __iomem *) AR5K_AR5312_ENABLE);
if (to_platform_device(ah->dev)->id == 0)
reg |= AR5K_AR5312_ENABLE_WLAN0;
else
@@ -202,12 +202,12 @@ static int ath_ahb_remove(struct platform_device *pdev)
if (bcfg->devid >= AR5K_SREV_AR2315_R6) {
/* Disable WMAC AHB arbitration */
- reg = ioread32((void __iomem *) AR5K_AR2315_AHB_ARB_CTL);
+ reg = ioread32((const void __iomem *) AR5K_AR2315_AHB_ARB_CTL);
reg &= ~AR5K_AR2315_AHB_ARB_CTL_WLAN;
iowrite32(reg, (void __iomem *) AR5K_AR2315_AHB_ARB_CTL);
} else {
/*Stop DMA access */
- reg = ioread32((void __iomem *) AR5K_AR5312_ENABLE);
+ reg = ioread32((const void __iomem *) AR5K_AR5312_ENABLE);
if (to_platform_device(ah->dev)->id == 0)
reg &= ~AR5K_AR5312_ENABLE_WLAN0;
else
--
2.17.1
[PATCH v2 8/9] media: fsl-viu: Constify ioreadX() iomem argument (as in generic implementation)
The ioreadX() helpers have inconsistent interface. On some architectures void *__iomem address argument is a pointer to const, on some not. Implementations of ioreadX() do not modify the memory under the address so they can be converted to a "const" version for const-safety and consistency among architectures. Signed-off-by: Krzysztof Kozlowski --- drivers/media/platform/fsl-viu.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/media/platform/fsl-viu.c b/drivers/media/platform/fsl-viu.c index 81a8faedbba6..991d9dc82749 100644 --- a/drivers/media/platform/fsl-viu.c +++ b/drivers/media/platform/fsl-viu.c @@ -34,7 +34,7 @@ /* Allow building this driver with COMPILE_TEST */ #if !defined(CONFIG_PPC) && !defined(CONFIG_MICROBLAZE) #define out_be32(v, a) iowrite32be(a, (void __iomem *)v) -#define in_be32(a) ioread32be((void __iomem *)a) +#define in_be32(a) ioread32be((const void __iomem *)a) #endif #define BUFFER_TIMEOUT msecs_to_jiffies(500) /* 0.5 seconds */ -- 2.17.1
[PATCH v2 7/9] drm/nouveau: Constify ioreadX() iomem argument (as in generic implementation)
The ioreadX() helpers have inconsistent interface. On some architectures void *__iomem address argument is a pointer to const, on some not. Implementations of ioreadX() do not modify the memory under the address so they can be converted to a "const" version for const-safety and consistency among architectures. Signed-off-by: Krzysztof Kozlowski --- drivers/gpu/drm/nouveau/nouveau_bo.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/nouveau/nouveau_bo.c b/drivers/gpu/drm/nouveau/nouveau_bo.c index f8015e0318d7..5120d062c2df 100644 --- a/drivers/gpu/drm/nouveau/nouveau_bo.c +++ b/drivers/gpu/drm/nouveau/nouveau_bo.c @@ -613,7 +613,7 @@ nouveau_bo_rd32(struct nouveau_bo *nvbo, unsigned index) mem += index; if (is_iomem) - return ioread32_native((void __force __iomem *)mem); + return ioread32_native((const void __force __iomem *)mem); else return *mem; } -- 2.17.1
[PATCH v2 6/9] drm/mgag200: Constify ioreadX() iomem argument (as in generic implementation)
The ioreadX() helpers have inconsistent interface. On some architectures void *__iomem address argument is a pointer to const, on some not. Implementations of ioreadX() do not modify the memory under the address so they can be converted to a "const" version for const-safety and consistency among architectures. Signed-off-by: Krzysztof Kozlowski --- drivers/gpu/drm/mgag200/mgag200_drv.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h b/drivers/gpu/drm/mgag200/mgag200_drv.h index aa32aad222c2..6512b3af4fb7 100644 --- a/drivers/gpu/drm/mgag200/mgag200_drv.h +++ b/drivers/gpu/drm/mgag200/mgag200_drv.h @@ -34,9 +34,9 @@ #define MGAG200FB_CONN_LIMIT 1 -#define RREG8(reg) ioread8(((void __iomem *)mdev->rmmio) + (reg)) +#define RREG8(reg) ioread8(((const void __iomem *)mdev->rmmio) + (reg)) #define WREG8(reg, v) iowrite8(v, ((void __iomem *)mdev->rmmio) + (reg)) -#define RREG32(reg) ioread32(((void __iomem *)mdev->rmmio) + (reg)) +#define RREG32(reg) ioread32(((const void __iomem *)mdev->rmmio) + (reg)) #define WREG32(reg, v) iowrite32(v, ((void __iomem *)mdev->rmmio) + (reg)) #define ATTR_INDEX 0x1fc0 -- 2.17.1
[PATCH v2 5/9] arc: Constify ioreadX() iomem argument (as in generic implementation)
The ioreadX() helpers have inconsistent interface. On some architectures
void *__iomem address argument is a pointer to const, on some not.
Implementations of ioreadX() do not modify the memory under the
address so they can be converted to a "const" version for const-safety
and consistency among architectures.
Signed-off-by: Krzysztof Kozlowski
---
arch/arc/plat-axs10x/axs10x.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/arc/plat-axs10x/axs10x.c b/arch/arc/plat-axs10x/axs10x.c
index 63ea5a606ecd..180c260a8221 100644
--- a/arch/arc/plat-axs10x/axs10x.c
+++ b/arch/arc/plat-axs10x/axs10x.c
@@ -84,7 +84,7 @@ static void __init axs10x_print_board_ver(unsigned int creg,
const char *str)
unsigned int val;
} board;
- board.val = ioread32((void __iomem *)creg);
+ board.val = ioread32((const void __iomem *)creg);
pr_info("AXS: %s FPGA Date: %u-%u-%u\n", str, board.d, board.m,
board.y);
}
@@ -95,7 +95,7 @@ static void __init axs10x_early_init(void)
char mb[32];
/* Determine motherboard version */
- if (ioread32((void __iomem *) CREG_MB_CONFIG) & (1 << 28))
+ if (ioread32((const void __iomem *) CREG_MB_CONFIG) & (1 << 28))
mb_rev = 3; /* HT-3 (rev3.0) */
else
mb_rev = 2; /* HT-2 (rev2.0) */
--
2.17.1
[PATCH v2 4/9] virtio: pci: Constify ioreadX() iomem argument (as in generic implementation)
The ioreadX() helpers have inconsistent interface. On some architectures
void *__iomem address argument is a pointer to const, on some not.
Implementations of ioreadX() do not modify the memory under the address
so they can be converted to a "const" version for const-safety and
consistency among architectures.
Signed-off-by: Krzysztof Kozlowski
Reviewed-by: Geert Uytterhoeven
---
Changes since v1:
1. Add Geert's review.
---
drivers/virtio/virtio_pci_modern.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/virtio/virtio_pci_modern.c
b/drivers/virtio/virtio_pci_modern.c
index 7abcc50838b8..fc58db4ab6c3 100644
--- a/drivers/virtio/virtio_pci_modern.c
+++ b/drivers/virtio/virtio_pci_modern.c
@@ -26,16 +26,16 @@
* method, i.e. 32-bit accesses for 32-bit fields, 16-bit accesses
* for 16-bit fields and 8-bit accesses for 8-bit fields.
*/
-static inline u8 vp_ioread8(u8 __iomem *addr)
+static inline u8 vp_ioread8(const u8 __iomem *addr)
{
return ioread8(addr);
}
-static inline u16 vp_ioread16 (__le16 __iomem *addr)
+static inline u16 vp_ioread16 (const __le16 __iomem *addr)
{
return ioread16(addr);
}
-static inline u32 vp_ioread32(__le32 __iomem *addr)
+static inline u32 vp_ioread32(const __le32 __iomem *addr)
{
return ioread32(addr);
}
--
2.17.1
[PATCH v2 3/9] ntb: intel: Constify ioreadX() iomem argument (as in generic implementation)
The ioreadX() helpers have inconsistent interface. On some architectures
void *__iomem address argument is a pointer to const, on some not.
Implementations of ioreadX() do not modify the memory under the address
so they can be converted to a "const" version for const-safety and
consistency among architectures.
Signed-off-by: Krzysztof Kozlowski
Reviewed-by: Geert Uytterhoeven
---
Changes since v1:
1. Add Geert's review.
---
drivers/ntb/hw/intel/ntb_hw_gen1.c | 2 +-
drivers/ntb/hw/intel/ntb_hw_gen3.h | 2 +-
drivers/ntb/hw/intel/ntb_hw_intel.h | 2 +-
3 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/ntb/hw/intel/ntb_hw_gen1.c
b/drivers/ntb/hw/intel/ntb_hw_gen1.c
index bb57ec239029..9202502a9787 100644
--- a/drivers/ntb/hw/intel/ntb_hw_gen1.c
+++ b/drivers/ntb/hw/intel/ntb_hw_gen1.c
@@ -1202,7 +1202,7 @@ int intel_ntb_peer_spad_write(struct ntb_dev *ntb, int
pidx, int sidx,
ndev->peer_reg->spad);
}
-static u64 xeon_db_ioread(void __iomem *mmio)
+static u64 xeon_db_ioread(const void __iomem *mmio)
{
return (u64)ioread16(mmio);
}
diff --git a/drivers/ntb/hw/intel/ntb_hw_gen3.h
b/drivers/ntb/hw/intel/ntb_hw_gen3.h
index 75fb86ca27bb..d1455f24ec99 100644
--- a/drivers/ntb/hw/intel/ntb_hw_gen3.h
+++ b/drivers/ntb/hw/intel/ntb_hw_gen3.h
@@ -91,7 +91,7 @@
#define GEN3_DB_TOTAL_SHIFT33
#define GEN3_SPAD_COUNT16
-static inline u64 gen3_db_ioread(void __iomem *mmio)
+static inline u64 gen3_db_ioread(const void __iomem *mmio)
{
return ioread64(mmio);
}
diff --git a/drivers/ntb/hw/intel/ntb_hw_intel.h
b/drivers/ntb/hw/intel/ntb_hw_intel.h
index e071e28bca3f..3c0a5a2da241 100644
--- a/drivers/ntb/hw/intel/ntb_hw_intel.h
+++ b/drivers/ntb/hw/intel/ntb_hw_intel.h
@@ -102,7 +102,7 @@ struct intel_ntb_dev;
struct intel_ntb_reg {
int (*poll_link)(struct intel_ntb_dev *ndev);
int (*link_is_up)(struct intel_ntb_dev *ndev);
- u64 (*db_ioread)(void __iomem *mmio);
+ u64 (*db_ioread)(const void __iomem *mmio);
void (*db_iowrite)(u64 db_bits, void __iomem *mmio);
unsigned long ntb_ctl;
resource_size_t db_size;
--
2.17.1
[PATCH v2 2/9] net: wireless: rtl818x: Constify ioreadX() iomem argument (as in generic implementation)
The ioreadX() helpers have inconsistent interface. On some architectures
void *__iomem address argument is a pointer to const, on some not.
Implementations of ioreadX() do not modify the memory under the address
so they can be converted to a "const" version for const-safety and
consistency among architectures.
Signed-off-by: Krzysztof Kozlowski
Reviewed-by: Geert Uytterhoeven
---
Changes since v1:
1. Add Geert's review.
---
drivers/net/wireless/realtek/rtl818x/rtl8180/rtl8180.h | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/net/wireless/realtek/rtl818x/rtl8180/rtl8180.h
b/drivers/net/wireless/realtek/rtl818x/rtl8180/rtl8180.h
index 7948a2da195a..2ff00800d45b 100644
--- a/drivers/net/wireless/realtek/rtl818x/rtl8180/rtl8180.h
+++ b/drivers/net/wireless/realtek/rtl818x/rtl8180/rtl8180.h
@@ -150,17 +150,17 @@ void rtl8180_write_phy(struct ieee80211_hw *dev, u8 addr,
u32 data);
void rtl8180_set_anaparam(struct rtl8180_priv *priv, u32 anaparam);
void rtl8180_set_anaparam2(struct rtl8180_priv *priv, u32 anaparam2);
-static inline u8 rtl818x_ioread8(struct rtl8180_priv *priv, u8 __iomem *addr)
+static inline u8 rtl818x_ioread8(struct rtl8180_priv *priv, const u8 __iomem
*addr)
{
return ioread8(addr);
}
-static inline u16 rtl818x_ioread16(struct rtl8180_priv *priv, __le16 __iomem
*addr)
+static inline u16 rtl818x_ioread16(struct rtl8180_priv *priv, const __le16
__iomem *addr)
{
return ioread16(addr);
}
-static inline u32 rtl818x_ioread32(struct rtl8180_priv *priv, __le32 __iomem
*addr)
+static inline u32 rtl818x_ioread32(struct rtl8180_priv *priv, const __le32
__iomem *addr)
{
return ioread32(addr);
}
--
2.17.1
[PATCH v2 1/9] iomap: Constify ioreadX() iomem argument (as in generic implementation)
The ioreadX() and ioreadX_rep() helpers have inconsistent interface. On
some architectures void *__iomem address argument is a pointer to const,
on some not.
Implementations of ioreadX() do not modify the memory under the address
so they can be converted to a "const" version for const-safety and
consistency among architectures.
Suggested-by: Geert Uytterhoeven
Signed-off-by: Krzysztof Kozlowski
Reviewed-by: Geert Uytterhoeven
---
Changes since v1:
1. Constify also ioreadX_rep() and mmio_insX(),
2. Squash lib+alpha+powerpc+parisc+sh into one patch for bisectability,
3. Add Geert's review.
---
arch/alpha/include/asm/core_apecs.h | 6 +--
arch/alpha/include/asm/core_cia.h | 6 +--
arch/alpha/include/asm/core_lca.h | 6 +--
arch/alpha/include/asm/core_marvel.h | 4 +-
arch/alpha/include/asm/core_mcpcia.h | 6 +--
arch/alpha/include/asm/core_t2.h | 2 +-
arch/alpha/include/asm/io.h | 12 ++---
arch/alpha/include/asm/io_trivial.h | 16 +++---
arch/alpha/include/asm/jensen.h | 2 +-
arch/alpha/include/asm/machvec.h | 6 +--
arch/alpha/kernel/core_marvel.c | 2 +-
arch/alpha/kernel/io.c| 12 ++---
arch/parisc/include/asm/io.h | 4 +-
arch/parisc/lib/iomap.c | 72 +--
arch/powerpc/kernel/iomap.c | 28 +--
arch/sh/kernel/iomap.c| 22
include/asm-generic/iomap.h | 28 +--
include/linux/io-64-nonatomic-hi-lo.h | 4 +-
include/linux/io-64-nonatomic-lo-hi.h | 4 +-
lib/iomap.c | 30 +--
20 files changed, 136 insertions(+), 136 deletions(-)
diff --git a/arch/alpha/include/asm/core_apecs.h
b/arch/alpha/include/asm/core_apecs.h
index 0a07055bc0fe..2d9726fc02ef 100644
--- a/arch/alpha/include/asm/core_apecs.h
+++ b/arch/alpha/include/asm/core_apecs.h
@@ -384,7 +384,7 @@ struct el_apecs_procdata
} \
} while (0)
-__EXTERN_INLINE unsigned int apecs_ioread8(void __iomem *xaddr)
+__EXTERN_INLINE unsigned int apecs_ioread8(const void __iomem *xaddr)
{
unsigned long addr = (unsigned long) xaddr;
unsigned long result, base_and_type;
@@ -420,7 +420,7 @@ __EXTERN_INLINE void apecs_iowrite8(u8 b, void __iomem
*xaddr)
*(vuip) ((addr << 5) + base_and_type) = w;
}
-__EXTERN_INLINE unsigned int apecs_ioread16(void __iomem *xaddr)
+__EXTERN_INLINE unsigned int apecs_ioread16(const void __iomem *xaddr)
{
unsigned long addr = (unsigned long) xaddr;
unsigned long result, base_and_type;
@@ -456,7 +456,7 @@ __EXTERN_INLINE void apecs_iowrite16(u16 b, void __iomem
*xaddr)
*(vuip) ((addr << 5) + base_and_type) = w;
}
-__EXTERN_INLINE unsigned int apecs_ioread32(void __iomem *xaddr)
+__EXTERN_INLINE unsigned int apecs_ioread32(const void __iomem *xaddr)
{
unsigned long addr = (unsigned long) xaddr;
if (addr < APECS_DENSE_MEM)
diff --git a/arch/alpha/include/asm/core_cia.h
b/arch/alpha/include/asm/core_cia.h
index c706a7f2b061..cb22991f6761 100644
--- a/arch/alpha/include/asm/core_cia.h
+++ b/arch/alpha/include/asm/core_cia.h
@@ -342,7 +342,7 @@ struct el_CIA_sysdata_mcheck {
#define vuip volatile unsigned int __force *
#define vulp volatile unsigned long __force *
-__EXTERN_INLINE unsigned int cia_ioread8(void __iomem *xaddr)
+__EXTERN_INLINE unsigned int cia_ioread8(const void __iomem *xaddr)
{
unsigned long addr = (unsigned long) xaddr;
unsigned long result, base_and_type;
@@ -374,7 +374,7 @@ __EXTERN_INLINE void cia_iowrite8(u8 b, void __iomem *xaddr)
*(vuip) ((addr << 5) + base_and_type) = w;
}
-__EXTERN_INLINE unsigned int cia_ioread16(void __iomem *xaddr)
+__EXTERN_INLINE unsigned int cia_ioread16(const void __iomem *xaddr)
{
unsigned long addr = (unsigned long) xaddr;
unsigned long result, base_and_type;
@@ -404,7 +404,7 @@ __EXTERN_INLINE void cia_iowrite16(u16 b, void __iomem
*xaddr)
*(vuip) ((addr << 5) + base_and_type) = w;
}
-__EXTERN_INLINE unsigned int cia_ioread32(void __iomem *xaddr)
+__EXTERN_INLINE unsigned int cia_ioread32(const void __iomem *xaddr)
{
unsigned long addr = (unsigned long) xaddr;
if (addr < CIA_DENSE_MEM)
diff --git a/arch/alpha/include/asm/core_lca.h
b/arch/alpha/include/asm/core_lca.h
index 84d5e5b84f4f..ec86314418cb 100644
--- a/arch/alpha/include/asm/core_lca.h
+++ b/arch/alpha/include/asm/core_lca.h
@@ -230,7 +230,7 @@ union el_lca {
} while (0)
-__EXTERN_INLINE unsigned int lca_ioread8(void __iomem *xaddr)
+__EXTERN_INLINE unsigned int lca_ioread8(const void __iomem *xaddr)
{
unsigned long addr = (unsigned long) xaddr;
unsigned long result, base_and_type;
@@ -266,7 +266,7 @@ __EXTERN_INLINE void lca_iowrite8(u8 b, void __iomem *xaddr)
*(vuip) ((addr << 5) + base_and_type) = w;
}
-__EXTERN_INLINE unsigned int
[PATCH v2 0/9] iomap: Constify ioreadX() iomem argument
Hi, Changes since v1 https://lore.kernel.org/lkml/1578415992-24054-1-git-send-email-k...@kernel.org/ 1. Constify also ioreadX_rep() and mmio_insX(), 2. Squash lib+alpha+powerpc+parisc+sh into one patch for bisectability, 3. Add Geert's review, 4. Re-order patches so all optional driver changes are at the end. Description === The ioread8/16/32() and others have inconsistent interface among the architectures: some taking address as const, some not. It seems there is nothing really stopping all of them to take pointer to const. Patchset was not really tested on all affected architectures. Build testing is in progress - I hope auto-builders will point any issues. volatile There is still interface inconsistency between architectures around "volatile" qualifier: - include/asm-generic/io.h:static inline u32 ioread32(const volatile void __iomem *addr) - include/asm-generic/iomap.h:extern unsigned int ioread32(const void __iomem *); This is still discussed and out of scope of this patchset. Merging === Multiple architectures are affected in first patch so acks are welcomed. Patches 2-4 depend on first patch. The rest is optional cleanup, without actual impact. Best regards, Krzysztof Krzysztof Kozlowski (9): iomap: Constify ioreadX() iomem argument (as in generic implementation) net: wireless: rtl818x: Constify ioreadX() iomem argument (as in generic implementation) ntb: intel: Constify ioreadX() iomem argument (as in generic implementation) virtio: pci: Constify ioreadX() iomem argument (as in generic implementation) arc: Constify ioreadX() iomem argument (as in generic implementation) drm/mgag200: Constify ioreadX() iomem argument (as in generic implementation) drm/nouveau: Constify ioreadX() iomem argument (as in generic implementation) media: fsl-viu: Constify ioreadX() iomem argument (as in generic implementation) net: wireless: ath5k: Constify ioreadX() iomem argument (as in generic implementation) arch/alpha/include/asm/core_apecs.h | 6 +- arch/alpha/include/asm/core_cia.h | 6 +- arch/alpha/include/asm/core_lca.h | 6 +- arch/alpha/include/asm/core_marvel.h | 4 +- arch/alpha/include/asm/core_mcpcia.h | 6 +- arch/alpha/include/asm/core_t2.h | 2 +- arch/alpha/include/asm/io.h | 12 ++-- arch/alpha/include/asm/io_trivial.h | 16 ++--- arch/alpha/include/asm/jensen.h | 2 +- arch/alpha/include/asm/machvec.h | 6 +- arch/alpha/kernel/core_marvel.c | 2 +- arch/alpha/kernel/io.c| 12 ++-- arch/arc/plat-axs10x/axs10x.c | 4 +- arch/parisc/include/asm/io.h | 4 +- arch/parisc/lib/iomap.c | 72 +-- arch/powerpc/kernel/iomap.c | 28 arch/sh/kernel/iomap.c| 22 +++--- drivers/gpu/drm/mgag200/mgag200_drv.h | 4 +- drivers/gpu/drm/nouveau/nouveau_bo.c | 2 +- drivers/media/platform/fsl-viu.c | 2 +- drivers/net/wireless/ath/ath5k/ahb.c | 10 +-- .../realtek/rtl818x/rtl8180/rtl8180.h | 6 +- drivers/ntb/hw/intel/ntb_hw_gen1.c| 2 +- drivers/ntb/hw/intel/ntb_hw_gen3.h| 2 +- drivers/ntb/hw/intel/ntb_hw_intel.h | 2 +- drivers/virtio/virtio_pci_modern.c| 6 +- include/asm-generic/iomap.h | 28 include/linux/io-64-nonatomic-hi-lo.h | 4 +- include/linux/io-64-nonatomic-lo-hi.h | 4 +- lib/iomap.c | 30 30 files changed, 156 insertions(+), 156 deletions(-) -- 2.17.1
Re: [PATCH v2 2/8] mm/memory_hotplug: Rename mhp_restrictions to mhp_modifiers
On 2020-01-08 12:13 p.m., Dan Williams wrote: > On Wed, Jan 8, 2020 at 11:08 AM David Hildenbrand wrote: >> >> >> >>> Am 08.01.2020 um 20:00 schrieb Dan Williams : >>> >>> On Wed, Jan 8, 2020 at 9:17 AM Logan Gunthorpe wrote: > On 2020-01-08 5:28 a.m., David Hildenbrand wrote: > On 07.01.20 21:59, Logan Gunthorpe wrote: >> The mhp_restrictions struct really doesn't specify anything resembling >> a restriction anymore so rename it to be mhp_modifiers. > > I wonder if something like "mhp_params" would be even better. It's > essentially just a way to avoid changing call chains rough-out all archs > whenever we want to add a new parameter. Sure, that does sound a bit nicer to me. I can change it for v3. >>> >>> Oh, I was just about to chime in to support "modifiers" because I >>> would expect all parameters to folded into a "params" struct. The >>> modifiers seem to be limited to the set of items that are only >>> considered in a non-default / expert memory hotplug use cases. >> >> It‘s a set of extended parameters I‘d say. > Sure, we can call them "mhp_params" and just clarify that they are > optional / extended in the kernel-doc. Well pgprot isn't going to be optional... But I'll add something to the kernel_doc. Logan
Re: Freescale network device not activated on mpc8360 (kmeter1 board)
On 08.01.2020 12:53, Matteo Ghidoni wrote:
> Hi Heiner, thank you for the quick answer.
>
>>> Hi all,
>>>
>>> With the introduction of the following patch, we are facing an issue with
>> the activation of the Freescale network device (ucc_geth driver) on our
>> kmeter1 board based on a MPC8360:
>>
>> +Li as ucc_geth maintainer
>>
>> Can you describe the symptoms of the issue?
>
> I am trying to boot in NFS, but as soon as the boot process is finished there
> is no network connections between the board and the host.
>
>>>
>>> commit 124eee3f6955f7aa19b9e6ff5c9b6d37cb3d1e2c
>>> Author: Heiner Kallweit
>>> Date: Tue Sep 18 21:55:36 2018 +0200
>>>
>>> net: linkwatch: add check for netdevice being present to
>>> linkwatch_do_dev
>>>
>>> Based on my observations, just before trying to activate the device through
>> linkwatch_event, the controller wants to adjust the MAC configuration and in
>> order to achieve this it detaches the device. This avoids the activation of
>> the
>> net device.
>>>
>> It sounds a little bit odd to rely on an asynchronous linkwatch event here.
>> Can you give a call trace?
>
> Here is a call trace form the adjust_link function in the if condition at
> line 1644 (ucc_geth.c file):
>
> CPU: 0 PID: 35 Comm: kworker/0:1 Not tainted 5.4.8-dirty #19
> Workqueue: events_power_efficient phy_state_machine
> Call Trace:
> [cf88bde8] [c02ddca8] adjust_link+0x304/0x320 (unreliable)
> [cf88be28] [c02cbf3c] phy_check_link_status+0xe4/0xfc
> [cf88be48] [c02cccdc] phy_state_machine+0x44/0x170
> [cf88be78] [c00361a0] process_one_work+0x264/0x408
> [cf88bea8] [c00370f8] worker_thread+0x140/0x53c
> [cf88bef8] [c003d818] kthread+0xdc/0x108
> [cf88bf38] [c0010274] ret_from_kernel_thread+0x14/0x1c
>
> Here the call trace from the netif_carrier_on function just before the call
> to the linkwatch_fire_event function (line 498, sch_generic.c file):
>
> CPU: 0 PID: 35 Comm: kworker/0:1 Not tainted 5.4.8-dirty #20
> Workqueue: events_power_efficient phy_state_machine
> Call Trace:
> [cf88bde8] [c0352064] netif_carrier_on+0xc4/0xc8 (unreliable)
> [cf88be08] [c02cf4ec] phy_link_change+0x84/0xb4
> [cf88be28] [c02cbf3c] phy_check_link_status+0xe4/0xfc
> [cf88be48] [c02cccdc] phy_state_machine+0x44/0x170
> [cf88be78] [c00361a0] process_one_work+0x264/0x408
> [cf88bea8] [c00370f8] worker_thread+0x140/0x53c
> [cf88bef8] [c003d818] kthread+0xdc/0x108
> [cf88bf38] [c0010274] ret_from_kernel_thread+0x14/0x1c
>
> Moreover, I noticed that by adding the dump directly in the linkwatch_do_dev
> function (link_watch.c) the interface comes up correctly, because of the
> delay introduced by the dump_stack function.
>
> Here another log with some prints that maybe can help to understand the
> situation. The prints are placed just before calling the function mentioned
> in the second part of the message (hopefully this will not bring more
> confusion):
>
> <...>
> ubi0: available PEBs: 235, total reserved PEBs: 269, PEBs reserved for bad
> PEB handling: 0
> ubi0: background thread "ubi_bgt0d" started, PID 45
> # [phy_device.c] phy_link_change - calling netif_carrier_on
> (eth2)
> # [sched_generic.c] netif_carrier_on - calling
> linkwatch_fire_event (eth2)
> # [phy_device.c] phy_link_change - calling adjust_link (eth2)
> # [ucc_geth.c] adjust_link - calling ugeth_quiesce (detaching
> device) (eth2)
> # [link_watch.c] linkwatch_do_dev - checking for
> netif_device_present(eth2) => 0
> IP-Config: Guessing netmask 255.255.255.0
> IP-Config: Complete:
> device=eth2, hwaddr=00:e0:df:56:54:07, ipaddr=192.168.1.20,
> mask=255.255.255.0, gw=255.255.255.255
> host=kmeter1, domain=, nis-domain=(none)
> bootserver=192.168.1.100, rootserver=192.168.1.100, rootpath=
> # [ucc_geth.c] adjust_link - calling ugeth_activate
> (attaching device) (eth2)
> ucc_geth e0103200.ucc eth2: Link is Up - 100Mbps/Full - flow control off
> rpcbind: server 192.168.1.100 not responding, timed out
> rpcbind: server 192.168.1.100 not responding, timed out
>
> As mentioned, just before that the linkwatch checks for the net_device
> presence, this one is detached by the ucc_geth driver and reattached later.
>
Detaching the netdev was introduced with 08b5e1c91ce9
("ucc_geth: Fix netdev watchdog triggering on link changes").
Most likely detaching the netdev isn't the best way to fix the original issue.
If it's just about switching the watchdog off temporarily, then maybe
calling dev_watchdog_down() is sufficient.
Relying on an asynchronous linkwatch event to active a netdev that is
marked as not present is at least questionable.
>> The driver is quite old and maybe some parts need to be improved. The
>> referenced change is more than a year old and I'm not aware of any other
>> problem with it. So it seems the change isn't wrong.
>
> I agree. I pointed out the commit by bisecting. This gave me the direction to
> where
Re: [PATCH v2 2/8] mm/memory_hotplug: Rename mhp_restrictions to mhp_modifiers
On Wed, Jan 8, 2020 at 11:08 AM David Hildenbrand wrote: > > > > > Am 08.01.2020 um 20:00 schrieb Dan Williams : > > > > On Wed, Jan 8, 2020 at 9:17 AM Logan Gunthorpe wrote: > >> > >> > >> > >>> On 2020-01-08 5:28 a.m., David Hildenbrand wrote: > >>> On 07.01.20 21:59, Logan Gunthorpe wrote: > The mhp_restrictions struct really doesn't specify anything resembling > a restriction anymore so rename it to be mhp_modifiers. > >>> > >>> I wonder if something like "mhp_params" would be even better. It's > >>> essentially just a way to avoid changing call chains rough-out all archs > >>> whenever we want to add a new parameter. > >> > >> Sure, that does sound a bit nicer to me. I can change it for v3. > > > > Oh, I was just about to chime in to support "modifiers" because I > > would expect all parameters to folded into a "params" struct. The > > modifiers seem to be limited to the set of items that are only > > considered in a non-default / expert memory hotplug use cases. > > > > It‘s a set of extended parameters I‘d say. Sure, we can call them "mhp_params" and just clarify that they are optional / extended in the kernel-doc. >
Re: [PATCH v2 2/8] mm/memory_hotplug: Rename mhp_restrictions to mhp_modifiers
> Am 08.01.2020 um 20:00 schrieb Dan Williams : > > On Wed, Jan 8, 2020 at 9:17 AM Logan Gunthorpe wrote: >> >> >> >>> On 2020-01-08 5:28 a.m., David Hildenbrand wrote: >>> On 07.01.20 21:59, Logan Gunthorpe wrote: The mhp_restrictions struct really doesn't specify anything resembling a restriction anymore so rename it to be mhp_modifiers. >>> >>> I wonder if something like "mhp_params" would be even better. It's >>> essentially just a way to avoid changing call chains rough-out all archs >>> whenever we want to add a new parameter. >> >> Sure, that does sound a bit nicer to me. I can change it for v3. > > Oh, I was just about to chime in to support "modifiers" because I > would expect all parameters to folded into a "params" struct. The > modifiers seem to be limited to the set of items that are only > considered in a non-default / expert memory hotplug use cases. > It‘s a set of extended parameters I‘d say.
Re: [PATCH v2 2/8] mm/memory_hotplug: Rename mhp_restrictions to mhp_modifiers
On Wed, Jan 8, 2020 at 9:17 AM Logan Gunthorpe wrote: > > > > On 2020-01-08 5:28 a.m., David Hildenbrand wrote: > > On 07.01.20 21:59, Logan Gunthorpe wrote: > >> The mhp_restrictions struct really doesn't specify anything resembling > >> a restriction anymore so rename it to be mhp_modifiers. > > > > I wonder if something like "mhp_params" would be even better. It's > > essentially just a way to avoid changing call chains rough-out all archs > > whenever we want to add a new parameter. > > Sure, that does sound a bit nicer to me. I can change it for v3. Oh, I was just about to chime in to support "modifiers" because I would expect all parameters to folded into a "params" struct. The modifiers seem to be limited to the set of items that are only considered in a non-default / expert memory hotplug use cases.
Re: [PATCH v2 6/8] s390/mm: Thread pgprot_t through vmem_add_mapping()
On 2020-01-08 5:43 a.m., David Hildenbrand wrote:
> On 07.01.20 21:59, Logan Gunthorpe wrote:
>> In prepartion to support a pgprot_t argument for arch_add_memory().
>>
>> Cc: Heiko Carstens
>> Cc: Vasily Gorbik
>> Cc: Christian Borntraeger
>> Signed-off-by: Logan Gunthorpe
>> ---
>> arch/s390/include/asm/pgtable.h | 3 ++-
>> arch/s390/mm/extmem.c | 3 ++-
>> arch/s390/mm/init.c | 2 +-
>> arch/s390/mm/vmem.c | 10 +-
>> 4 files changed, 10 insertions(+), 8 deletions(-)
>>
>> diff --git a/arch/s390/include/asm/pgtable.h
>> b/arch/s390/include/asm/pgtable.h
>> index 7b03037a8475..e667a1a96879 100644
>> --- a/arch/s390/include/asm/pgtable.h
>> +++ b/arch/s390/include/asm/pgtable.h
>> @@ -1640,7 +1640,8 @@ static inline swp_entry_t __swp_entry(unsigned long
>> type, unsigned long offset)
>>
>> #define kern_addr_valid(addr) (1)
>>
>> -extern int vmem_add_mapping(unsigned long start, unsigned long size);
>> +extern int vmem_add_mapping(unsigned long start, unsigned long size,
>> +pgprot_t prot);
>> extern int vmem_remove_mapping(unsigned long start, unsigned long size);
>> extern int s390_enable_sie(void);
>> extern int s390_enable_skey(void);
>> diff --git a/arch/s390/mm/extmem.c b/arch/s390/mm/extmem.c
>> index fd0dae9d10f4..6cf7029a7b35 100644
>> --- a/arch/s390/mm/extmem.c
>> +++ b/arch/s390/mm/extmem.c
>> @@ -313,7 +313,8 @@ __segment_load (char *name, int do_nonshared, unsigned
>> long *addr, unsigned long
>> goto out_free;
>> }
>>
>> -rc = vmem_add_mapping(seg->start_addr, seg->end - seg->start_addr + 1);
>> +rc = vmem_add_mapping(seg->start_addr, seg->end - seg->start_addr + 1,
>> + PAGE_KERNEL);
>>
>> if (rc)
>> goto out_free;
>> diff --git a/arch/s390/mm/init.c b/arch/s390/mm/init.c
>> index a0c88c1c9ad0..ef19522ddad2 100644
>> --- a/arch/s390/mm/init.c
>> +++ b/arch/s390/mm/init.c
>> @@ -277,7 +277,7 @@ int arch_add_memory(int nid, u64 start, u64 size,
>> if (WARN_ON_ONCE(modifiers->altmap))
>> return -EINVAL;
>>
>> -rc = vmem_add_mapping(start, size);
>> +rc = vmem_add_mapping(start, size, PAGE_KERNEL);
>> if (rc)
>> return rc;
>>
>> diff --git a/arch/s390/mm/vmem.c b/arch/s390/mm/vmem.c
>> index b403fa14847d..8a5e95f184a2 100644
>> --- a/arch/s390/mm/vmem.c
>> +++ b/arch/s390/mm/vmem.c
>> @@ -66,7 +66,7 @@ pte_t __ref *vmem_pte_alloc(void)
>> /*
>> * Add a physical memory range to the 1:1 mapping.
>> */
>> -static int vmem_add_mem(unsigned long start, unsigned long size)
>> +static int vmem_add_mem(unsigned long start, unsigned long size, pgprot_t
>> prot)
>> {
>> unsigned long pgt_prot, sgt_prot, r3_prot;
>> unsigned long pages4k, pages1m, pages2g;
>> @@ -79,7 +79,7 @@ static int vmem_add_mem(unsigned long start, unsigned long
>> size)
>> pte_t *pt_dir;
>> int ret = -ENOMEM;
>>
>> -pgt_prot = pgprot_val(PAGE_KERNEL);
>> +pgt_prot = pgprot_val(prot);
>> sgt_prot = pgprot_val(SEGMENT_KERNEL);
>> r3_prot = pgprot_val(REGION3_KERNEL);
>
> So, if we map as huge/gigantic pages, the protection would be discarded?
> That looks wrong.
>
> s390x does not support ZONE_DEVICE yet. Maybe simply bail out for s390x
> as you do for sh to make your life easier?
Yeah, ok, makes sense to me; I'll change it for v3.
Logan
Re: [PATCH v2 7/8] mm/memory_hotplug: Add pgprot_t to mhp_modifiers
On 2020-01-08 5:42 a.m., Michal Hocko wrote:
> On Tue 07-01-20 13:59:58, Logan Gunthorpe wrote:
>> devm_memremap_pages() is currently used by the PCI P2PDMA code to create
>> struct page mappings for IO memory. At present, these mappings are created
>> with PAGE_KERNEL which implies setting the PAT bits to be WB. However, on
>> x86, an mtrr register will typically override this and force the cache
>> type to be UC-. In the case firmware doesn't set this register it is
>> effectively WB and will typically result in a machine check exception
>> when it's accessed.
>>
>> Other arches are not currently likely to function correctly seeing they
>> don't have any MTRR registers to fall back on.
>>
>> To solve this, add an argument to arch_add_memory() to explicitly
>> set the pgprot value to a specific value.
>>
>> Of the arches that support MEMORY_HOTPLUG: x86_64, s390 and arm64 is a
>> simple change to pass the pgprot_t down to their respective functions
>> which set up the page tables. For x86_32, set the page tables explicitly
>> using _set_memory_prot() (seeing they are already mapped). For sh, reject
>> anything but PAGE_KERNEL settings -- this should be fine, for now, seeing
>> sh doesn't support ZONE_DEVICE anyway.
>>
>> Cc: Dan Williams
>> Cc: David Hildenbrand
>> Cc: Michal Hocko
>> Signed-off-by: Logan Gunthorpe
>
> OK, this is less code churn than I expected. Having pgprot as an implcit
> parameter de-facto is a bit fragile though. Should we add a WARN_ON_ONCE
> (e.g. into the add_pages to catch all arches) for value 0?
Sure, I can add that for v3.
Logan
> Other than that
> Acked-by: Michal Hocko
>
>> ---
>> arch/arm64/mm/mmu.c| 3 ++-
>> arch/ia64/mm/init.c| 4
>> arch/powerpc/mm/mem.c | 3 ++-
>> arch/s390/mm/init.c| 2 +-
>> arch/sh/mm/init.c | 3 +++
>> arch/x86/mm/init_32.c | 5 +
>> arch/x86/mm/init_64.c | 2 +-
>> include/linux/memory_hotplug.h | 2 ++
>> mm/memory_hotplug.c| 2 +-
>> mm/memremap.c | 6 +++---
>> 10 files changed, 24 insertions(+), 8 deletions(-)
>>
>> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
>> index 3320406579c3..9b214b0d268f 100644
>> --- a/arch/arm64/mm/mmu.c
>> +++ b/arch/arm64/mm/mmu.c
>> @@ -1058,7 +1058,8 @@ int arch_add_memory(int nid, u64 start, u64 size,
>> flags = NO_BLOCK_MAPPINGS | NO_CONT_MAPPINGS;
>>
>> __create_pgd_mapping(swapper_pg_dir, start, __phys_to_virt(start),
>> - size, PAGE_KERNEL, __pgd_pgtable_alloc, flags);
>> + size, modifiers->pgprot, __pgd_pgtable_alloc,
>> + flags);
>>
>> memblock_clear_nomap(start, size);
>>
>> diff --git a/arch/ia64/mm/init.c b/arch/ia64/mm/init.c
>> index daf438e08b96..5fd6ae4929c9 100644
>> --- a/arch/ia64/mm/init.c
>> +++ b/arch/ia64/mm/init.c
>> @@ -677,6 +677,10 @@ int arch_add_memory(int nid, u64 start, u64 size,
>> int ret;
>>
>> ret = __add_pages(nid, start_pfn, nr_pages, modifiers);
>> +if (modifiers->pgprot != PAGE_KERNEL)
>> +return -EINVAL;
>> +
>> +ret = __add_pages(nid, start_pfn, nr_pages, restrictions);
>> if (ret)
>> printk("%s: Problem encountered in __add_pages() as ret=%d\n",
>> __func__, ret);
>> diff --git a/arch/powerpc/mm/mem.c b/arch/powerpc/mm/mem.c
>> index 631ee684721f..fddeaee53198 100644
>> --- a/arch/powerpc/mm/mem.c
>> +++ b/arch/powerpc/mm/mem.c
>> @@ -137,7 +137,8 @@ int __ref arch_add_memory(int nid, u64 start, u64 size,
>> resize_hpt_for_hotplug(memblock_phys_mem_size());
>>
>> start = (unsigned long)__va(start);
>> -rc = create_section_mapping(start, start + size, nid, PAGE_KERNEL);
>> +rc = create_section_mapping(start, start + size, nid,
>> +modifiers->pgprot);
>> if (rc) {
>> pr_warn("Unable to create mapping for hot added memory
>> 0x%llx..0x%llx: %d\n",
>> start, start + size, rc);
>> diff --git a/arch/s390/mm/init.c b/arch/s390/mm/init.c
>> index ef19522ddad2..c65fb33f6a89 100644
>> --- a/arch/s390/mm/init.c
>> +++ b/arch/s390/mm/init.c
>> @@ -277,7 +277,7 @@ int arch_add_memory(int nid, u64 start, u64 size,
>> if (WARN_ON_ONCE(modifiers->altmap))
>> return -EINVAL;
>>
>> -rc = vmem_add_mapping(start, size, PAGE_KERNEL);
>> +rc = vmem_add_mapping(start, size, modifiers->pgprot);
>> if (rc)
>> return rc;
>>
>> diff --git a/arch/sh/mm/init.c b/arch/sh/mm/init.c
>> index 7e64f42fb570..7071dc5bd2e4 100644
>> --- a/arch/sh/mm/init.c
>> +++ b/arch/sh/mm/init.c
>> @@ -412,6 +412,9 @@ int arch_add_memory(int nid, u64 start, u64 size,
>> unsigned long nr_pages = size >> PAGE_SHIFT;
>> int ret;
>>
>> +if (modifiers->pgprot != PAGE_KERNEL)
>> +return -EINVAL;
>> +
>> /* We only have ZONE_NORMAL, so this is easy.. */
Re: [PATCH v2 7/8] mm/memory_hotplug: Add pgprot_t to mhp_modifiers
On 2020-01-08 5:39 a.m., David Hildenbrand wrote:
> On 07.01.20 21:59, Logan Gunthorpe wrote:
>> devm_memremap_pages() is currently used by the PCI P2PDMA code to create
>> struct page mappings for IO memory. At present, these mappings are created
>> with PAGE_KERNEL which implies setting the PAT bits to be WB. However, on
>> x86, an mtrr register will typically override this and force the cache
>> type to be UC-. In the case firmware doesn't set this register it is
>> effectively WB and will typically result in a machine check exception
>> when it's accessed.
>>
>> Other arches are not currently likely to function correctly seeing they
>> don't have any MTRR registers to fall back on.
>>
>> To solve this, add an argument to arch_add_memory() to explicitly
>> set the pgprot value to a specific value.
>
> You're adding a parameter indirectly by adding it to the structure.
> Maybe "provide a way to specify the pgprot value explicitly to
> arch_add_memory()"
>
>>
>> Of the arches that support MEMORY_HOTPLUG: x86_64, s390 and arm64 is a
>
> s/is/need/
>
>> simple change to pass the pgprot_t down to their respective functions
>> which set up the page tables. For x86_32, set the page tables explicitly
>
> "page table protection" ?
>
>> using _set_memory_prot() (seeing they are already mapped). For sh, reject
>> anything but PAGE_KERNEL settings -- this should be fine, for now, seeing
>> sh doesn't support ZONE_DEVICE anyway.
>>
>> Cc: Dan Williams
>> Cc: David Hildenbrand
>> Cc: Michal Hocko
>> Signed-off-by: Logan Gunthorpe
>> ---
>> arch/arm64/mm/mmu.c| 3 ++-
>> arch/ia64/mm/init.c| 4
>> arch/powerpc/mm/mem.c | 3 ++-
>> arch/s390/mm/init.c| 2 +-
>> arch/sh/mm/init.c | 3 +++
>> arch/x86/mm/init_32.c | 5 +
>> arch/x86/mm/init_64.c | 2 +-
>> include/linux/memory_hotplug.h | 2 ++
>> mm/memory_hotplug.c| 2 +-
>> mm/memremap.c | 6 +++---
>> 10 files changed, 24 insertions(+), 8 deletions(-)
>>
>> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
>> index 3320406579c3..9b214b0d268f 100644
>> --- a/arch/arm64/mm/mmu.c
>> +++ b/arch/arm64/mm/mmu.c
>> @@ -1058,7 +1058,8 @@ int arch_add_memory(int nid, u64 start, u64 size,
>> flags = NO_BLOCK_MAPPINGS | NO_CONT_MAPPINGS;
>>
>> __create_pgd_mapping(swapper_pg_dir, start, __phys_to_virt(start),
>> - size, PAGE_KERNEL, __pgd_pgtable_alloc, flags);
>> + size, modifiers->pgprot, __pgd_pgtable_alloc,
>> + flags);
>>
>> memblock_clear_nomap(start, size);
>>
>> diff --git a/arch/ia64/mm/init.c b/arch/ia64/mm/init.c
>> index daf438e08b96..5fd6ae4929c9 100644
>> --- a/arch/ia64/mm/init.c
>> +++ b/arch/ia64/mm/init.c
>> @@ -677,6 +677,10 @@ int arch_add_memory(int nid, u64 start, u64 size,
>> int ret;
>>
>> ret = __add_pages(nid, start_pfn, nr_pages, modifiers);
>> +if (modifiers->pgprot != PAGE_KERNEL)
>> +return -EINVAL;
>
> ... maybe better "if (WARN_ON_ONCE(...))"
> [...]
>
>> --- a/include/linux/memory_hotplug.h
>> +++ b/include/linux/memory_hotplug.h
>> @@ -56,9 +56,11 @@ enum {
>> /*
>> * Restrictions for the memory hotplug:
>> * altmap: alternative allocator for memmap array
>> + * pgprot: page protection flags to apply to newly added page tables
>> */
>> struct mhp_modifiers {
>> struct vmem_altmap *altmap;
>> +pgprot_t pgprot;
>> };
>>
>> /*
>> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
>> index 1bb3f92e087d..0888f821af06 100644
>> --- a/mm/memory_hotplug.c
>> +++ b/mm/memory_hotplug.c
>> @@ -1027,7 +1027,7 @@ static int online_memory_block(struct memory_block
>> *mem, void *arg)
>> */
>> int __ref add_memory_resource(int nid, struct resource *res)
>> {
>> -struct mhp_modifiers modifiers = {};
>> +struct mhp_modifiers modifiers = {.pgprot = PAGE_KERNEL};
>
> I think we usually use spaces like
>
> = { .pgprot = PAGE_KERNEL };
>
> t480s: ~/git/linux virtio-mem-v1 $ git grep "= {\." | wc -l
> 978
> t480s: ~/git/linux virtio-mem-v1 $ git grep "= { " | wc -l
> 35447
>
>> u64 start, size;
>> bool new_node = false;
>> int ret;
>> diff --git a/mm/memremap.c b/mm/memremap.c
>> index e30be8ba706b..45ab4ef0643d 100644
>> --- a/mm/memremap.c
>> +++ b/mm/memremap.c
>> @@ -163,8 +163,8 @@ void *memremap_pages(struct dev_pagemap *pgmap, int nid)
>> * We do not want any optional features only our own memmap
>> */
>> .altmap = pgmap_altmap(pgmap),
>> +.pgprot = PAGE_KERNEL,
>> };
>> -pgprot_t pgprot = PAGE_KERNEL;
>> int error, is_ram;
>> bool need_devmap_managed = true;
>>
>> @@ -252,8 +252,8 @@ void *memremap_pages(struct dev_pagemap *pgmap, int nid)
>> if (nid < 0)
>> nid = numa_mem_id();
>>
>> -error = track_pfn_remap(NULL, , PHYS_PFN(res->start), 0,
>> -
Re: [PATCH v2 2/8] mm/memory_hotplug: Rename mhp_restrictions to mhp_modifiers
On 2020-01-08 5:28 a.m., David Hildenbrand wrote: > On 07.01.20 21:59, Logan Gunthorpe wrote: >> The mhp_restrictions struct really doesn't specify anything resembling >> a restriction anymore so rename it to be mhp_modifiers. > > I wonder if something like "mhp_params" would be even better. It's > essentially just a way to avoid changing call chains rough-out all archs > whenever we want to add a new parameter. Sure, that does sound a bit nicer to me. I can change it for v3. Logan
Re: [PATCH v6 2/5] powerpc/kprobes: Mark newly allocated probes as RO
Le 24/12/2019 à 06:55, Russell Currey a écrit :
With CONFIG_STRICT_KERNEL_RWX=y and CONFIG_KPROBES=y, there will be one
W+X page at boot by default. This can be tested with
CONFIG_PPC_PTDUMP=y and CONFIG_PPC_DEBUG_WX=y set, and checking the
kernel log during boot.
powerpc doesn't implement its own alloc() for kprobes like other
architectures do, but we couldn't immediately mark RO anyway since we do
a memcpy to the page we allocate later. After that, nothing should be
allowed to modify the page, and write permissions are removed well
before the kprobe is armed.
The memcpy() would fail if >1 probes were allocated, so use
patch_instruction() instead which is safe for RO.
Reviewed-by: Daniel Axtens
Signed-off-by: Russell Currey
---
arch/powerpc/kernel/kprobes.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
index 2d27ec4feee4..b72761f0c9e3 100644
--- a/arch/powerpc/kernel/kprobes.c
+++ b/arch/powerpc/kernel/kprobes.c
@@ -24,6 +24,7 @@
#include
#include
#include
+#include
DEFINE_PER_CPU(struct kprobe *, current_kprobe) = NULL;
DEFINE_PER_CPU(struct kprobe_ctlblk, kprobe_ctlblk);
@@ -124,13 +125,14 @@ int arch_prepare_kprobe(struct kprobe *p)
}
if (!ret) {
- memcpy(p->ainsn.insn, p->addr,
- MAX_INSN_SIZE * sizeof(kprobe_opcode_t));
+ patch_instruction(p->ainsn.insn, *p->addr);
p->opcode = *p->addr;
flush_icache_range((unsigned long)p->ainsn.insn,
(unsigned long)p->ainsn.insn + sizeof(kprobe_opcode_t));
patch_instruction() already does the flush, no need to flush again with
flush_icache_range()
}
+ set_memory_ro((unsigned long)p->ainsn.insn, 1);
+
I don't really understand, why do you need to set this ro ? Or why do
you need to change the memcpy() to patch_instruction() if the area is
not already ro ?
If I understand correctly, p->ainsn.insn is within a special executable
page allocated via module_alloc(). Wouldn't it be more correct to modify
kprobe get_insn_slot() logic so that allocated page is ROX instead of RWX ?
p->ainsn.boostable = 0;
return ret;
}
Christophe
Re: [PATCH v4 2/9] perf/core: open access for CAP_SYS_PERFMON privileged process
On Wed, Dec 18, 2019 at 12:25:35PM +0300, Alexey Budankov wrote:
>
> Open access to perf_events monitoring for CAP_SYS_PERFMON privileged
> processes. For backward compatibility reasons access to perf_events
> subsystem remains open for CAP_SYS_ADMIN privileged processes but
> CAP_SYS_ADMIN usage for secure perf_events monitoring is discouraged
> with respect to CAP_SYS_PERFMON capability.
>
> Signed-off-by: Alexey Budankov
> ---
> include/linux/perf_event.h | 6 +++---
> kernel/events/core.c | 6 +++---
> 2 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
> index 34c7c6910026..f46acd69425f 100644
> --- a/include/linux/perf_event.h
> +++ b/include/linux/perf_event.h
> @@ -1285,7 +1285,7 @@ static inline int perf_is_paranoid(void)
>
> static inline int perf_allow_kernel(struct perf_event_attr *attr)
> {
> - if (sysctl_perf_event_paranoid > 1 && !capable(CAP_SYS_ADMIN))
> + if (sysctl_perf_event_paranoid > 1 && !perfmon_capable())
> return -EACCES;
>
> return security_perf_event_open(attr, PERF_SECURITY_KERNEL);
> @@ -1293,7 +1293,7 @@ static inline int perf_allow_kernel(struct
> perf_event_attr *attr)
>
> static inline int perf_allow_cpu(struct perf_event_attr *attr)
> {
> - if (sysctl_perf_event_paranoid > 0 && !capable(CAP_SYS_ADMIN))
> + if (sysctl_perf_event_paranoid > 0 && !perfmon_capable())
> return -EACCES;
>
> return security_perf_event_open(attr, PERF_SECURITY_CPU);
> @@ -1301,7 +1301,7 @@ static inline int perf_allow_cpu(struct perf_event_attr
> *attr)
>
> static inline int perf_allow_tracepoint(struct perf_event_attr *attr)
> {
> - if (sysctl_perf_event_paranoid > -1 && !capable(CAP_SYS_ADMIN))
> + if (sysctl_perf_event_paranoid > -1 && !perfmon_capable())
> return -EPERM;
>
> return security_perf_event_open(attr, PERF_SECURITY_TRACEPOINT);
These are OK I suppose.
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index 059ee7116008..d9db414f2197 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -9056,7 +9056,7 @@ static int perf_kprobe_event_init(struct perf_event
> *event)
> if (event->attr.type != perf_kprobe.type)
> return -ENOENT;
>
> - if (!capable(CAP_SYS_ADMIN))
> + if (!perfmon_capable())
> return -EACCES;
>
> /*
This one only allows attaching to already extant kprobes, right? It does
not allow creation of kprobes.
> @@ -9116,7 +9116,7 @@ static int perf_uprobe_event_init(struct perf_event
> *event)
> if (event->attr.type != perf_uprobe.type)
> return -ENOENT;
>
> - if (!capable(CAP_SYS_ADMIN))
> + if (!perfmon_capable())
> return -EACCES;
>
> /*
Idem, I presume.
> @@ -11157,7 +11157,7 @@ SYSCALL_DEFINE5(perf_event_open,
> }
>
> if (attr.namespaces) {
> - if (!capable(CAP_SYS_ADMIN))
> + if (!perfmon_capable())
> return -EACCES;
> }
And given we basically make the entire kernel observable with this CAP,
busting namespaces shoulnd't be a problem either.
So yeah, I suppose that works.
Re: [PATCH v2 01/11] powerpc/powernv/ioda: Fix ref count for devices with their own PE
Le 08/01/2020 à 08:33, Andrew Donnellan a écrit :
On 22/11/19 12:49 am, Frederic Barrat wrote:
The pci_dn structure used to store a pointer to the struct pci_dev, so
taking a reference on the device was required. However, the pci_dev
pointer was later removed from the pci_dn structure, but the reference
was kept for the npu device.
See commit 902bdc57451c ("powerpc/powernv/idoa: Remove unnecessary
pcidev from pci_dn").
We don't need to take a reference on the device when assigning the PE
as the struct pnv_ioda_pe is cleaned up at the same time as
the (physical) device is released. Doing so prevents the device from
being released, which is a problem for opencapi devices, since we want
to be able to remove them through PCI hotplug.
Now the ugly part: nvlink npu devices are not meant to be
released. Because of the above, we've always leaked a reference and
simply removing it now is dangerous and would likely require more
work. There's currently no release device callback for nvlink devices
for example. So to be safe, this patch leaks a reference on the npu
device, but only for nvlink and not opencapi.
CC: a...@ozlabs.ru
CC: ooh...@gmail.com
Signed-off-by: Frederic Barrat
It took me a while to parse exactly what you're doing here.
- In pnv_ioda_setup_dev_PE(), we take a reference on the pci_dev, this
is to protect a pointer in the pci_dn structure, but not to protect the
pointer in the pci_dev structure (which doesn't need to be protected by
taking a reference, because the lifetime of the pnv_ioda_pe is the same
as the pci_dev).
- The pointer in the pci_dn structure has now been removed, so we should
remove the pci_dev_get() accordingly.
Correct. Did I do such a bad job explaining it in the commit message
that I need to rephrase?
Fred
This seems okay to me, though anything PE-related is irritatingly hairy
so one can never be truly certain...
Reviewed-by: Andrew Donnellan
[linux-next/mainline][bisected 3acac06][ppc] Oops when unloading mpt3sas driver
Greeting's
Kernel Oops on my powerpc system when unloading driver mpt3sas.
Thanks Oliver for bisecting it to commit 3acac06 ("dma-mapping: merge
the generic remapping helpers into dma-direct")
Christoph, could you please have a look
Kernel version : latest mainline and next kernel
System : powerpc bare-metal
config: attached kernel config
test: rmmod mpt3sas
trace:
kernel: mpt3sas_cm0: enclosure logical id(0x500304801f080d3f), slot(12)
kernel: mpt3sas_cm0: enclosure level(0x), connector name( )
kernel: mpt3sas_cm0: expander_remove: handle(0x0009),
sas_addr(0x500304801f080d3f)
kernel: mpt3sas_cm0: sending diag reset !!
kernel: mpt3sas_cm0: diag reset: SUCCESS
kernel: BUG: Unable to handle kernel data access on write at
0xc04a00017c34
kernel: Faulting instruction address: 0xc02f9c70
kernel: Oops: Kernel access of bad area, sig: 11 [#1]
kernel: LE PAGE_SIZE=64K MMU=Hash SMP NR_CPUS=2048 NUMA PowerNV
kernel: Dumping ftrace buffer:
kernel: (ftrace buffer empty)
kernel: Modules linked in: ixgbe i40e iptable_mangle xt_MASQUERADE
iptable_nat nf_nat xt_conntrack nf_conntrack nf_defrag_ipv4 ipt_REJECT
nf_reject_ipv4 xt_tcpudp tun bridge stp llc iptable_filter btrfs
blake2b_generic xor zstd_decompress zstd_compress lzo_compress
vmx_crypto gf128mul raid6_pq powernv_rng rng_core kvm_hv kvm nfsd
binfmt_misc ip_tables x_tables autofs4 xfs libcrc32c qla2xxx nvme_fc
nvme_fabrics mdio nvme_core mpt3sas(-) raid_class scsi_transport_sas
[last unloaded: ixgbe]
kernel: CPU: 61 PID: 138496 Comm: rmmod Not tainted 5.5.0-rc3-autotest-autotest
#1
kernel: NIP: c02f9c70 LR: c01a9b44 CTR: c0049ef0
kernel: REGS: c03f225af5e0 TRAP: 0380 Not tainted
(5.5.0-rc3-autotest-autotest)
kernel: MSR: 90009033 CR: 24002424 XER:
2000
kernel: CFAR: c01a9b40 IRQMASK: 0 #012GPR00: c0049f88
c03f225af870 c12fc900 c04a00017c00 #012GPR04:
c03fbbe7 003e00017c00
#012GPR08: c13ad000 c04a00017c34
c0080fbbe9e0 #012GPR12: c0049ef0 c03caa80
#012GPR16:
010029f601e0 10020098 #012GPR20:
10020050 10020038 10020078 100200b0
#012GPR24: c0d4c2f8 05f0
#012GPR28: c126e038 c03fbbe7
0001 c03fdd22d0a8
kernel: NIP [c02f9c70] __free_pages+0x10/0x50
kernel: LR [c01a9b44] dma_direct_free_pages+0x54/0x90
kernel: Call Trace:
kernel: [c03f225af870] [c01a9b44] dma_direct_free_pages+0x54/0x90
(unreliable)
kernel: [c03f225af890] [c0049f88] dma_iommu_free_coherent+0x98/0xd0
kernel: [c03f225af8e0] [c01a8b78] dma_free_attrs+0xf8/0x100
kernel: [c03f225af930] [c0310af4] dma_pool_destroy+0x174/0x200
kernel: [c03f225af9d0] [c0080fb917b8]
_base_release_memory_pools+0x1d8/0x620 [mpt3sas]
kernel: [c03f225afa60] [c0080fb9b3b0] mpt3sas_base_detach+0x40/0x150
[mpt3sas]
kernel: [c03f225afad0] [c0080fbabdfc]
_scsih_flush_running_cmds+0x5bc/0x1140 [mpt3sas]
kernel: [c03f225afb90] [c060eda4] pci_device_remove+0x64/0x110
kernel: [c03f225afbd0] [c06c4c44]
device_release_driver_internal+0x154/0x260
kernel: [c03f225afc10] [c06c4e1c] driver_detach+0x8c/0x140
kernel: [c03f225afc50] [c06c2f28] bus_remove_driver+0x78/0x100
kernel: [c03f225afc80] [c06c5b30] driver_unregister+0x40/0x90
kernel: [c03f225afcf0] [c060e4c8] pci_unregister_driver+0x38/0x110
kernel: [c03f225afd40] [c0080fbbe338] cleanup_module+0x50/0x3fd8
[mpt3sas]
kernel: [c03f225afda0] [c01d866c] sys_delete_module+0x1dc/0x2a0
kernel: [c03f225afe20] [c000b9d0] system_call+0x5c/0x68
kernel: Instruction dump:
kernel: 88830051 2fa4 41de0008 4bffe86c 7d234b78 4bfffe94 6000 6042
kernel: 3c4c0100 38422ca0 39430034 7c0004ac <7d005028> 3108 7d00512d
40c2fff4
kernel: ---[ end trace ef72317ef11520bc ]---
kernel:
kernel: qla2xxx [0020:04:00.1]-b079:20: Removing driver
kernel: qla2xxx [0020:04:00.1]-00af:20: Performing ISP error recovery -
ha=ec46524c.
kernel: qla2xxx [0020:04:00.0]-b079:19: Removing driver
kernel: qla2xxx [0020:04:00.0]-00af:19: Performing ISP error recovery -
ha=af37b975.
kernel: qla2xxx [0020:03:00.1]-b079:18: Removing driver
--
Regard's
Abdul Haleem
IBM Linux Technology Centre
#
# Automatically generated file; DO NOT EDIT.
# Linux/powerpc 4.11.0-rc4 Kernel Configuration
#
CONFIG_PPC64=y
#
# Processor support
#
CONFIG_PPC_BOOK3S_64=y
# CONFIG_PPC_BOOK3E_64 is not set
# CONFIG_POWER7_CPU is not set
CONFIG_POWER8_CPU=y
CONFIG_PPC_BOOK3S=y
CONFIG_PPC_FPU=y
CONFIG_ALTIVEC=y
CONFIG_VSX=y
# CONFIG_PPC_ICSWX is not set
CONFIG_PPC_STD_MMU=y
CONFIG_PPC_STD_MMU_64=y
CONFIG_PPC_RADIX_MMU=y
CONFIG_PPC_MM_SLICES=y
[PATCH -next] soc: fsl: qe: remove set but not used variable 'mm_gc'
drivers/soc/fsl/qe/gpio.c: In function qe_pin_request:
drivers/soc/fsl/qe/gpio.c:163:26: warning: variable mm_gc set but not used
[-Wunused-but-set-variable]
commit 1e714e54b5ca ("powerpc: qe_lib-gpio: use gpiochip data pointer")
left behind this unused variable.
Reported-by: Hulk Robot
Signed-off-by: YueHaibing
---
drivers/soc/fsl/qe/gpio.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/soc/fsl/qe/gpio.c b/drivers/soc/fsl/qe/gpio.c
index 12bdfd9..ed75198 100644
--- a/drivers/soc/fsl/qe/gpio.c
+++ b/drivers/soc/fsl/qe/gpio.c
@@ -160,7 +160,6 @@ struct qe_pin *qe_pin_request(struct device_node *np, int
index)
{
struct qe_pin *qe_pin;
struct gpio_chip *gc;
- struct of_mm_gpio_chip *mm_gc;
struct qe_gpio_chip *qe_gc;
int err;
unsigned long flags;
@@ -186,7 +185,6 @@ struct qe_pin *qe_pin_request(struct device_node *np, int
index)
goto err0;
}
- mm_gc = to_of_mm_gpio_chip(gc);
qe_gc = gpiochip_get_data(gc);
spin_lock_irqsave(_gc->lock, flags);
--
2.7.4
Re: [PATCH v6 1/5] powerpc/mm: Implement set_memory() routines
Le 24/12/2019 à 06:55, Russell Currey a écrit :
The set_memory_{ro/rw/nx/x}() functions are required for STRICT_MODULE_RWX,
and are generally useful primitives to have. This implementation is
designed to be completely generic across powerpc's many MMUs.
It's possible that this could be optimised to be faster for specific
MMUs, but the focus is on having a generic and safe implementation for
now.
This implementation does not handle cases where the caller is attempting
to change the mapping of the page it is executing from, or if another
CPU is concurrently using the page being altered. These cases likely
shouldn't happen, but a more complex implementation with MMU-specific code
could safely handle them, so that is left as a TODO for now.
Signed-off-by: Russell Currey
---
arch/powerpc/Kconfig | 1 +
arch/powerpc/include/asm/set_memory.h | 32 +++
arch/powerpc/mm/Makefile | 1 +
arch/powerpc/mm/pageattr.c| 83 +++
4 files changed, 117 insertions(+)
create mode 100644 arch/powerpc/include/asm/set_memory.h
create mode 100644 arch/powerpc/mm/pageattr.c
diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 1ec34e16ed65..f0b9b47b5353 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -133,6 +133,7 @@ config PPC
select ARCH_HAS_PTE_SPECIAL
select ARCH_HAS_MEMBARRIER_CALLBACKS
select ARCH_HAS_SCALED_CPUTIME if VIRT_CPU_ACCOUNTING_NATIVE
&& PPC_BOOK3S_64
+ select ARCH_HAS_SET_MEMORY
select ARCH_HAS_STRICT_KERNEL_RWX if ((PPC_BOOK3S_64 || PPC32) &&
!RELOCATABLE && !HIBERNATION)
select ARCH_HAS_TICK_BROADCAST if GENERIC_CLOCKEVENTS_BROADCAST
select ARCH_HAS_UACCESS_FLUSHCACHE
diff --git a/arch/powerpc/include/asm/set_memory.h
b/arch/powerpc/include/asm/set_memory.h
new file mode 100644
index ..5230ddb2fefd
--- /dev/null
+++ b/arch/powerpc/include/asm/set_memory.h
@@ -0,0 +1,32 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _ASM_POWERPC_SET_MEMORY_H
+#define _ASM_POWERPC_SET_MEMORY_H
+
+#define SET_MEMORY_RO 1
+#define SET_MEMORY_RW 2
+#define SET_MEMORY_NX 3
+#define SET_MEMORY_X 4
Maybe going from 0 to 3 would be better than 1 to 4
+
+int change_memory_attr(unsigned long addr, int numpages, int action);
action could be unsigned.
+
+static inline int set_memory_ro(unsigned long addr, int numpages)
+{
+ return change_memory_attr(addr, numpages, SET_MEMORY_RO);
+}
+
+static inline int set_memory_rw(unsigned long addr, int numpages)
+{
+ return change_memory_attr(addr, numpages, SET_MEMORY_RW);
+}
+
+static inline int set_memory_nx(unsigned long addr, int numpages)
+{
+ return change_memory_attr(addr, numpages, SET_MEMORY_NX);
+}
+
+static inline int set_memory_x(unsigned long addr, int numpages)
+{
+ return change_memory_attr(addr, numpages, SET_MEMORY_X);
+}
+
+#endif
diff --git a/arch/powerpc/mm/Makefile b/arch/powerpc/mm/Makefile
index 5e147986400d..d0a0bcbc9289 100644
--- a/arch/powerpc/mm/Makefile
+++ b/arch/powerpc/mm/Makefile
@@ -20,3 +20,4 @@ obj-$(CONFIG_HIGHMEM) += highmem.o
obj-$(CONFIG_PPC_COPRO_BASE) += copro_fault.o
obj-$(CONFIG_PPC_PTDUMP) += ptdump/
obj-$(CONFIG_KASAN) += kasan/
+obj-$(CONFIG_ARCH_HAS_SET_MEMORY) += pageattr.o
CONFIG_ARCH_HAS_SET_MEMORY is set inconditionnally, I think you should
add pageattr.o to obj-y instead. CONFIG_ARCH_HAS_XXX are almost never
used in Makefiles
diff --git a/arch/powerpc/mm/pageattr.c b/arch/powerpc/mm/pageattr.c
new file mode 100644
index ..15d5fb04f531
--- /dev/null
+++ b/arch/powerpc/mm/pageattr.c
@@ -0,0 +1,83 @@
+// SPDX-License-Identifier: GPL-2.0
+
+/*
+ * MMU-generic set_memory implementation for powerpc
+ *
+ * Copyright 2019, IBM Corporation.
+ */
+
+#include
+#include
+
+#include
+#include
+#include
+
+
+/*
+ * Updates the attributes of a page in three steps:
+ *
+ * 1. invalidate the page table entry
+ * 2. flush the TLB
+ * 3. install the new entry with the updated attributes
+ *
+ * This is unsafe if the caller is attempting to change the mapping of the
+ * page it is executing from, or if another CPU is concurrently using the
+ * page being altered.
+ *
+ * TODO make the implementation resistant to this.
+ */
+static int __change_page_attr(pte_t *ptep, unsigned long addr, void *data)
+{
+ int action = *((int *)data);
Don't use pointers for so simple things, pointers forces the compiler to
setup a stack frame and save the data into stack. Instead do:
int action = (int)data;
+ pte_t pte_val;
+
+ // invalidate the PTE so it's safe to modify
+ pte_val = ptep_get_and_clear(_mm, addr, ptep);
+ flush_tlb_kernel_range(addr, addr + PAGE_SIZE);
Why flush a range for a single page ? On most targets this will do a
tlbia which is heavy, while a tlbie would suffice.
I think flush_tlb_kernel_range() should be
Re: [PATCH v2 6/8] s390/mm: Thread pgprot_t through vmem_add_mapping()
On 07.01.20 21:59, Logan Gunthorpe wrote:
> In prepartion to support a pgprot_t argument for arch_add_memory().
>
> Cc: Heiko Carstens
> Cc: Vasily Gorbik
> Cc: Christian Borntraeger
> Signed-off-by: Logan Gunthorpe
> ---
> arch/s390/include/asm/pgtable.h | 3 ++-
> arch/s390/mm/extmem.c | 3 ++-
> arch/s390/mm/init.c | 2 +-
> arch/s390/mm/vmem.c | 10 +-
> 4 files changed, 10 insertions(+), 8 deletions(-)
>
> diff --git a/arch/s390/include/asm/pgtable.h b/arch/s390/include/asm/pgtable.h
> index 7b03037a8475..e667a1a96879 100644
> --- a/arch/s390/include/asm/pgtable.h
> +++ b/arch/s390/include/asm/pgtable.h
> @@ -1640,7 +1640,8 @@ static inline swp_entry_t __swp_entry(unsigned long
> type, unsigned long offset)
>
> #define kern_addr_valid(addr) (1)
>
> -extern int vmem_add_mapping(unsigned long start, unsigned long size);
> +extern int vmem_add_mapping(unsigned long start, unsigned long size,
> + pgprot_t prot);
> extern int vmem_remove_mapping(unsigned long start, unsigned long size);
> extern int s390_enable_sie(void);
> extern int s390_enable_skey(void);
> diff --git a/arch/s390/mm/extmem.c b/arch/s390/mm/extmem.c
> index fd0dae9d10f4..6cf7029a7b35 100644
> --- a/arch/s390/mm/extmem.c
> +++ b/arch/s390/mm/extmem.c
> @@ -313,7 +313,8 @@ __segment_load (char *name, int do_nonshared, unsigned
> long *addr, unsigned long
> goto out_free;
> }
>
> - rc = vmem_add_mapping(seg->start_addr, seg->end - seg->start_addr + 1);
> + rc = vmem_add_mapping(seg->start_addr, seg->end - seg->start_addr + 1,
> + PAGE_KERNEL);
>
> if (rc)
> goto out_free;
> diff --git a/arch/s390/mm/init.c b/arch/s390/mm/init.c
> index a0c88c1c9ad0..ef19522ddad2 100644
> --- a/arch/s390/mm/init.c
> +++ b/arch/s390/mm/init.c
> @@ -277,7 +277,7 @@ int arch_add_memory(int nid, u64 start, u64 size,
> if (WARN_ON_ONCE(modifiers->altmap))
> return -EINVAL;
>
> - rc = vmem_add_mapping(start, size);
> + rc = vmem_add_mapping(start, size, PAGE_KERNEL);
> if (rc)
> return rc;
>
> diff --git a/arch/s390/mm/vmem.c b/arch/s390/mm/vmem.c
> index b403fa14847d..8a5e95f184a2 100644
> --- a/arch/s390/mm/vmem.c
> +++ b/arch/s390/mm/vmem.c
> @@ -66,7 +66,7 @@ pte_t __ref *vmem_pte_alloc(void)
> /*
> * Add a physical memory range to the 1:1 mapping.
> */
> -static int vmem_add_mem(unsigned long start, unsigned long size)
> +static int vmem_add_mem(unsigned long start, unsigned long size, pgprot_t
> prot)
> {
> unsigned long pgt_prot, sgt_prot, r3_prot;
> unsigned long pages4k, pages1m, pages2g;
> @@ -79,7 +79,7 @@ static int vmem_add_mem(unsigned long start, unsigned long
> size)
> pte_t *pt_dir;
> int ret = -ENOMEM;
>
> - pgt_prot = pgprot_val(PAGE_KERNEL);
> + pgt_prot = pgprot_val(prot);
> sgt_prot = pgprot_val(SEGMENT_KERNEL);
> r3_prot = pgprot_val(REGION3_KERNEL);
So, if we map as huge/gigantic pages, the protection would be discarded?
That looks wrong.
s390x does not support ZONE_DEVICE yet. Maybe simply bail out for s390x
as you do for sh to make your life easier?
[...]
--
Thanks,
David / dhildenb
Re: [PATCH v2 7/8] mm/memory_hotplug: Add pgprot_t to mhp_modifiers
On Tue 07-01-20 13:59:58, Logan Gunthorpe wrote:
> devm_memremap_pages() is currently used by the PCI P2PDMA code to create
> struct page mappings for IO memory. At present, these mappings are created
> with PAGE_KERNEL which implies setting the PAT bits to be WB. However, on
> x86, an mtrr register will typically override this and force the cache
> type to be UC-. In the case firmware doesn't set this register it is
> effectively WB and will typically result in a machine check exception
> when it's accessed.
>
> Other arches are not currently likely to function correctly seeing they
> don't have any MTRR registers to fall back on.
>
> To solve this, add an argument to arch_add_memory() to explicitly
> set the pgprot value to a specific value.
>
> Of the arches that support MEMORY_HOTPLUG: x86_64, s390 and arm64 is a
> simple change to pass the pgprot_t down to their respective functions
> which set up the page tables. For x86_32, set the page tables explicitly
> using _set_memory_prot() (seeing they are already mapped). For sh, reject
> anything but PAGE_KERNEL settings -- this should be fine, for now, seeing
> sh doesn't support ZONE_DEVICE anyway.
>
> Cc: Dan Williams
> Cc: David Hildenbrand
> Cc: Michal Hocko
> Signed-off-by: Logan Gunthorpe
OK, this is less code churn than I expected. Having pgprot as an implcit
parameter de-facto is a bit fragile though. Should we add a WARN_ON_ONCE
(e.g. into the add_pages to catch all arches) for value 0?
Other than that
Acked-by: Michal Hocko
> ---
> arch/arm64/mm/mmu.c| 3 ++-
> arch/ia64/mm/init.c| 4
> arch/powerpc/mm/mem.c | 3 ++-
> arch/s390/mm/init.c| 2 +-
> arch/sh/mm/init.c | 3 +++
> arch/x86/mm/init_32.c | 5 +
> arch/x86/mm/init_64.c | 2 +-
> include/linux/memory_hotplug.h | 2 ++
> mm/memory_hotplug.c| 2 +-
> mm/memremap.c | 6 +++---
> 10 files changed, 24 insertions(+), 8 deletions(-)
>
> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
> index 3320406579c3..9b214b0d268f 100644
> --- a/arch/arm64/mm/mmu.c
> +++ b/arch/arm64/mm/mmu.c
> @@ -1058,7 +1058,8 @@ int arch_add_memory(int nid, u64 start, u64 size,
> flags = NO_BLOCK_MAPPINGS | NO_CONT_MAPPINGS;
>
> __create_pgd_mapping(swapper_pg_dir, start, __phys_to_virt(start),
> - size, PAGE_KERNEL, __pgd_pgtable_alloc, flags);
> + size, modifiers->pgprot, __pgd_pgtable_alloc,
> + flags);
>
> memblock_clear_nomap(start, size);
>
> diff --git a/arch/ia64/mm/init.c b/arch/ia64/mm/init.c
> index daf438e08b96..5fd6ae4929c9 100644
> --- a/arch/ia64/mm/init.c
> +++ b/arch/ia64/mm/init.c
> @@ -677,6 +677,10 @@ int arch_add_memory(int nid, u64 start, u64 size,
> int ret;
>
> ret = __add_pages(nid, start_pfn, nr_pages, modifiers);
> + if (modifiers->pgprot != PAGE_KERNEL)
> + return -EINVAL;
> +
> + ret = __add_pages(nid, start_pfn, nr_pages, restrictions);
> if (ret)
> printk("%s: Problem encountered in __add_pages() as ret=%d\n",
> __func__, ret);
> diff --git a/arch/powerpc/mm/mem.c b/arch/powerpc/mm/mem.c
> index 631ee684721f..fddeaee53198 100644
> --- a/arch/powerpc/mm/mem.c
> +++ b/arch/powerpc/mm/mem.c
> @@ -137,7 +137,8 @@ int __ref arch_add_memory(int nid, u64 start, u64 size,
> resize_hpt_for_hotplug(memblock_phys_mem_size());
>
> start = (unsigned long)__va(start);
> - rc = create_section_mapping(start, start + size, nid, PAGE_KERNEL);
> + rc = create_section_mapping(start, start + size, nid,
> + modifiers->pgprot);
> if (rc) {
> pr_warn("Unable to create mapping for hot added memory
> 0x%llx..0x%llx: %d\n",
> start, start + size, rc);
> diff --git a/arch/s390/mm/init.c b/arch/s390/mm/init.c
> index ef19522ddad2..c65fb33f6a89 100644
> --- a/arch/s390/mm/init.c
> +++ b/arch/s390/mm/init.c
> @@ -277,7 +277,7 @@ int arch_add_memory(int nid, u64 start, u64 size,
> if (WARN_ON_ONCE(modifiers->altmap))
> return -EINVAL;
>
> - rc = vmem_add_mapping(start, size, PAGE_KERNEL);
> + rc = vmem_add_mapping(start, size, modifiers->pgprot);
> if (rc)
> return rc;
>
> diff --git a/arch/sh/mm/init.c b/arch/sh/mm/init.c
> index 7e64f42fb570..7071dc5bd2e4 100644
> --- a/arch/sh/mm/init.c
> +++ b/arch/sh/mm/init.c
> @@ -412,6 +412,9 @@ int arch_add_memory(int nid, u64 start, u64 size,
> unsigned long nr_pages = size >> PAGE_SHIFT;
> int ret;
>
> + if (modifiers->pgprot != PAGE_KERNEL)
> + return -EINVAL;
> +
> /* We only have ZONE_NORMAL, so this is easy.. */
> ret = __add_pages(nid, start_pfn, nr_pages, modifiers);
> if (unlikely(ret))
> diff --git a/arch/x86/mm/init_32.c b/arch/x86/mm/init_32.c
> index
Re: [PATCH v2 7/8] mm/memory_hotplug: Add pgprot_t to mhp_modifiers
On 07.01.20 21:59, Logan Gunthorpe wrote:
> devm_memremap_pages() is currently used by the PCI P2PDMA code to create
> struct page mappings for IO memory. At present, these mappings are created
> with PAGE_KERNEL which implies setting the PAT bits to be WB. However, on
> x86, an mtrr register will typically override this and force the cache
> type to be UC-. In the case firmware doesn't set this register it is
> effectively WB and will typically result in a machine check exception
> when it's accessed.
>
> Other arches are not currently likely to function correctly seeing they
> don't have any MTRR registers to fall back on.
>
> To solve this, add an argument to arch_add_memory() to explicitly
> set the pgprot value to a specific value.
You're adding a parameter indirectly by adding it to the structure.
Maybe "provide a way to specify the pgprot value explicitly to
arch_add_memory()"
>
> Of the arches that support MEMORY_HOTPLUG: x86_64, s390 and arm64 is a
s/is/need/
> simple change to pass the pgprot_t down to their respective functions
> which set up the page tables. For x86_32, set the page tables explicitly
"page table protection" ?
> using _set_memory_prot() (seeing they are already mapped). For sh, reject
> anything but PAGE_KERNEL settings -- this should be fine, for now, seeing
> sh doesn't support ZONE_DEVICE anyway.
>
> Cc: Dan Williams
> Cc: David Hildenbrand
> Cc: Michal Hocko
> Signed-off-by: Logan Gunthorpe
> ---
> arch/arm64/mm/mmu.c| 3 ++-
> arch/ia64/mm/init.c| 4
> arch/powerpc/mm/mem.c | 3 ++-
> arch/s390/mm/init.c| 2 +-
> arch/sh/mm/init.c | 3 +++
> arch/x86/mm/init_32.c | 5 +
> arch/x86/mm/init_64.c | 2 +-
> include/linux/memory_hotplug.h | 2 ++
> mm/memory_hotplug.c| 2 +-
> mm/memremap.c | 6 +++---
> 10 files changed, 24 insertions(+), 8 deletions(-)
>
> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
> index 3320406579c3..9b214b0d268f 100644
> --- a/arch/arm64/mm/mmu.c
> +++ b/arch/arm64/mm/mmu.c
> @@ -1058,7 +1058,8 @@ int arch_add_memory(int nid, u64 start, u64 size,
> flags = NO_BLOCK_MAPPINGS | NO_CONT_MAPPINGS;
>
> __create_pgd_mapping(swapper_pg_dir, start, __phys_to_virt(start),
> - size, PAGE_KERNEL, __pgd_pgtable_alloc, flags);
> + size, modifiers->pgprot, __pgd_pgtable_alloc,
> + flags);
>
> memblock_clear_nomap(start, size);
>
> diff --git a/arch/ia64/mm/init.c b/arch/ia64/mm/init.c
> index daf438e08b96..5fd6ae4929c9 100644
> --- a/arch/ia64/mm/init.c
> +++ b/arch/ia64/mm/init.c
> @@ -677,6 +677,10 @@ int arch_add_memory(int nid, u64 start, u64 size,
> int ret;
>
> ret = __add_pages(nid, start_pfn, nr_pages, modifiers);
> + if (modifiers->pgprot != PAGE_KERNEL)
> + return -EINVAL;
... maybe better "if (WARN_ON_ONCE(...))"
[...]
> --- a/include/linux/memory_hotplug.h
> +++ b/include/linux/memory_hotplug.h
> @@ -56,9 +56,11 @@ enum {
> /*
> * Restrictions for the memory hotplug:
> * altmap: alternative allocator for memmap array
> + * pgprot: page protection flags to apply to newly added page tables
> */
> struct mhp_modifiers {
> struct vmem_altmap *altmap;
> + pgprot_t pgprot;
> };
>
> /*
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index 1bb3f92e087d..0888f821af06 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -1027,7 +1027,7 @@ static int online_memory_block(struct memory_block
> *mem, void *arg)
> */
> int __ref add_memory_resource(int nid, struct resource *res)
> {
> - struct mhp_modifiers modifiers = {};
> + struct mhp_modifiers modifiers = {.pgprot = PAGE_KERNEL};
I think we usually use spaces like
= { .pgprot = PAGE_KERNEL };
t480s: ~/git/linux virtio-mem-v1 $ git grep "= {\." | wc -l
978
t480s: ~/git/linux virtio-mem-v1 $ git grep "= { " | wc -l
35447
> u64 start, size;
> bool new_node = false;
> int ret;
> diff --git a/mm/memremap.c b/mm/memremap.c
> index e30be8ba706b..45ab4ef0643d 100644
> --- a/mm/memremap.c
> +++ b/mm/memremap.c
> @@ -163,8 +163,8 @@ void *memremap_pages(struct dev_pagemap *pgmap, int nid)
>* We do not want any optional features only our own memmap
>*/
> .altmap = pgmap_altmap(pgmap),
> + .pgprot = PAGE_KERNEL,
> };
> - pgprot_t pgprot = PAGE_KERNEL;
> int error, is_ram;
> bool need_devmap_managed = true;
>
> @@ -252,8 +252,8 @@ void *memremap_pages(struct dev_pagemap *pgmap, int nid)
> if (nid < 0)
> nid = numa_mem_id();
>
> - error = track_pfn_remap(NULL, , PHYS_PFN(res->start), 0,
> - resource_size(res));
> + error = track_pfn_remap(NULL, , PHYS_PFN(res->start),
> + 0, resource_size(res));
> if (error)
>
Re: [PATCH v2 2/8] mm/memory_hotplug: Rename mhp_restrictions to mhp_modifiers
On 07.01.20 21:59, Logan Gunthorpe wrote: > The mhp_restrictions struct really doesn't specify anything resembling > a restriction anymore so rename it to be mhp_modifiers. I wonder if something like "mhp_params" would be even better. It's essentially just a way to avoid changing call chains rough-out all archs whenever we want to add a new parameter. -- Thanks, David / dhildenb
Re: [PATCH v2 1/8] mm/memory_hotplug: Drop the flags field from struct mhp_restrictions
On 07.01.20 21:59, Logan Gunthorpe wrote:
> This variable is not used anywhere and should therefore be removed
> from the structure.
>
> Signed-off-by: Logan Gunthorpe
> ---
> include/linux/memory_hotplug.h | 2 --
> 1 file changed, 2 deletions(-)
>
> diff --git a/include/linux/memory_hotplug.h b/include/linux/memory_hotplug.h
> index ba0dca6aac6e..e47a29761088 100644
> --- a/include/linux/memory_hotplug.h
> +++ b/include/linux/memory_hotplug.h
> @@ -55,11 +55,9 @@ enum {
>
> /*
> * Restrictions for the memory hotplug:
> - * flags: MHP_ flags
> * altmap: alternative allocator for memmap array
> */
> struct mhp_restrictions {
> - unsigned long flags;
> struct vmem_altmap *altmap;
> };
We wanted to use that for the "vmemmap on added memory" config knob. But
that might still take some time and we might actually realize it using
the altmap instead (hopefully :) ).
Reviewed-by: David Hildenbrand
--
Thanks,
David / dhildenb
Re: [RFT 00/13] iomap: Constify ioreadX() iomem argument
On Wed, Jan 8, 2020 at 10:15 AM Krzysztof Kozlowski wrote: > > The __force-cast that removes the __iomem here also means that > > the 'volatile' keyword could be dropped from the argument list, > > as it has no real effect any more, but then there are a few drivers > > that mark their iomem pointers as either 'volatile void __iomem*' or > > (worse) 'volatile void *', so we keep it in the argument list to not > > add warnings for those drivers. > > > > It may be time to change these drivers to not use volatile for __iomem > > pointers, but that seems out of scope for what Krzysztof is trying > > to do. Ideally we would be consistent here though, either using volatile > > all the time or never. > > Indeed. I guess there are no objections around const so let me send v2 > for const only. Ok, sounds good. Maybe mention in the changelog then that the 'volatile' in the interface is intentionally left out, and that only users of readl/writel still have it to deal with existing drivers. Arnd
Re: [PATCH v6 3/6] powerpc/fadump: reorganize /sys/kernel/fadump_* sysfs files
On Wed, Dec 11, 2019 at 09:39:07PM +0530, Sourabh Jain wrote: > As the number of FADump sysfs files increases it is hard to manage all of > them inside /sys/kernel directory. It's better to have all the FADump > related sysfs files in a dedicated directory /sys/kernel/fadump. But in > order to maintain backward compatibility a symlink has been added for every > sysfs that has moved to new location. Patched this series into my test kernel and the sysfs sfiles look OK. Tested-by: Michal Suchanek Thanks Michal
Re: [PATCH 0/3] Add support for divde[.] and divdeu[.] instruction emulation
On 10/12/19 12:49 pm, Balamuruhan S wrote: > Hi All, > > This patchset adds support to emulate divde, divde., divdeu and divdeu. > instructions and testcases for it. > LGTM. Reviewed-by: Sandipan Das
[PATCH v4] powerpc/kernel/sysfs: Add new config option PMU_SYSFS to enable PMU SPRs sysfs file creation
Many of the performance moniroting unit (PMU) SPRs are
exposed in the sysfs. This may not be a desirable since
"perf" API is the primary interface to program PMU and
collect counter data in the system. But that said, we
cant remove these sysfs files since we dont whether
anyone/anything is using them.
So the patch adds a new CONFIG option 'CONFIG_PMU_SYSFS'
(user selectable) to be used in sysfs file creation for
PMU SPRs. New option by default is disabled, but can be
enabled if user needs it.
Tested this patch behaviour in powernv and pseries machines.
Also did compilation testing for different architecture include:
x86, mips, mips64, alpha, arm. Patch is also compile tested for
pmac32_defconfig.
Signed-off-by: Kajol Jain
---
arch/powerpc/kernel/sysfs.c| 22 +-
arch/powerpc/platforms/Kconfig.cputype | 6 ++
2 files changed, 19 insertions(+), 9 deletions(-)
---
Changelog:
v3 -> v4
- Make 'PMU_SYSFS' config option user selectable
v2 -> v3
- Remove 'PMU_SYSFS' config option dependency on
'PERF_EVENTS'.
- Add PMU_SYSFS config check at time of register/unregister
PMU SPRs.
- Replace #ifdefs with IS_ENABLE while registering/unregistering
PMU SPRs.
Resend v2
Added 'Reviewed-by' and 'Tested-by' tag along with test scenarios.
v1 -> v2
- Added new config option 'PMU_SYSFS' for PMU SPR's creation
rather than using PERF_EVENTS config option directly and make
sure SPR's file creation only if 'CONFIG_PERF_EVENTS' disabled.
---
diff --git a/arch/powerpc/kernel/sysfs.c b/arch/powerpc/kernel/sysfs.c
index 80a676da11cb..d4faa60f1d27 100644
--- a/arch/powerpc/kernel/sysfs.c
+++ b/arch/powerpc/kernel/sysfs.c
@@ -606,12 +606,14 @@ static void sysfs_create_dscr_default(void)
#endif /* CONFIG_PPC64 */
#ifdef HAS_PPC_PMC_PA6T
+#ifdef CONFIG_PMU_SYSFS
SYSFS_PMCSETUP(pa6t_pmc0, SPRN_PA6T_PMC0);
SYSFS_PMCSETUP(pa6t_pmc1, SPRN_PA6T_PMC1);
SYSFS_PMCSETUP(pa6t_pmc2, SPRN_PA6T_PMC2);
SYSFS_PMCSETUP(pa6t_pmc3, SPRN_PA6T_PMC3);
SYSFS_PMCSETUP(pa6t_pmc4, SPRN_PA6T_PMC4);
SYSFS_PMCSETUP(pa6t_pmc5, SPRN_PA6T_PMC5);
+#endif /* CONFIG_PMU_SYSFS */
#ifdef CONFIG_DEBUG_MISC
SYSFS_SPRSETUP(hid0, SPRN_HID0);
SYSFS_SPRSETUP(hid1, SPRN_HID1);
@@ -645,21 +647,21 @@ SYSFS_SPRSETUP(tsr3, SPRN_PA6T_TSR3);
#endif /* HAS_PPC_PMC_PA6T */
#ifdef HAS_PPC_PMC_IBM
-static struct device_attribute ibm_common_attrs[] = {
+static __maybe_unused struct device_attribute ibm_common_attrs[] = {
__ATTR(mmcr0, 0600, show_mmcr0, store_mmcr0),
__ATTR(mmcr1, 0600, show_mmcr1, store_mmcr1),
};
#endif /* HAS_PPC_PMC_G4 */
#ifdef HAS_PPC_PMC_G4
-static struct device_attribute g4_common_attrs[] = {
+static __maybe_unused struct device_attribute g4_common_attrs[] = {
__ATTR(mmcr0, 0600, show_mmcr0, store_mmcr0),
__ATTR(mmcr1, 0600, show_mmcr1, store_mmcr1),
__ATTR(mmcr2, 0600, show_mmcr2, store_mmcr2),
};
#endif /* HAS_PPC_PMC_G4 */
-static struct device_attribute classic_pmc_attrs[] = {
+static __maybe_unused struct device_attribute classic_pmc_attrs[] = {
__ATTR(pmc1, 0600, show_pmc1, store_pmc1),
__ATTR(pmc2, 0600, show_pmc2, store_pmc2),
__ATTR(pmc3, 0600, show_pmc3, store_pmc3),
@@ -674,6 +676,7 @@ static struct device_attribute classic_pmc_attrs[] = {
#ifdef HAS_PPC_PMC_PA6T
static struct device_attribute pa6t_attrs[] = {
+#ifdef CONFIG_PMU_SYSFS
__ATTR(mmcr0, 0600, show_mmcr0, store_mmcr0),
__ATTR(mmcr1, 0600, show_mmcr1, store_mmcr1),
__ATTR(pmc0, 0600, show_pa6t_pmc0, store_pa6t_pmc0),
@@ -682,6 +685,7 @@ static struct device_attribute pa6t_attrs[] = {
__ATTR(pmc3, 0600, show_pa6t_pmc3, store_pa6t_pmc3),
__ATTR(pmc4, 0600, show_pa6t_pmc4, store_pa6t_pmc4),
__ATTR(pmc5, 0600, show_pa6t_pmc5, store_pa6t_pmc5),
+#endif /* CONFIG_PMU_SYSFS */
#ifdef CONFIG_DEBUG_MISC
__ATTR(hid0, 0600, show_hid0, store_hid0),
__ATTR(hid1, 0600, show_hid1, store_hid1),
@@ -751,13 +755,12 @@ static int register_cpu_online(unsigned int cpu)
/* PMC stuff */
switch (cur_cpu_spec->pmc_type) {
-#ifdef HAS_PPC_PMC_IBM
+#ifdef CONFIG_PMU_SYSFS
case PPC_PMC_IBM:
attrs = ibm_common_attrs;
nattrs = sizeof(ibm_common_attrs) / sizeof(struct
device_attribute);
pmc_attrs = classic_pmc_attrs;
break;
-#endif /* HAS_PPC_PMC_IBM */
#ifdef HAS_PPC_PMC_G4
case PPC_PMC_G4:
attrs = g4_common_attrs;
@@ -765,6 +768,7 @@ static int register_cpu_online(unsigned int cpu)
pmc_attrs = classic_pmc_attrs;
break;
#endif /* HAS_PPC_PMC_G4 */
+#endif /* CONFIG_PMU_SYSFS */
#ifdef HAS_PPC_PMC_PA6T
case PPC_PMC_PA6T:
/* PA Semi starts counting at PMC0 */
@@ -787,7 +791,7 @@ static int register_cpu_online(unsigned int cpu)
device_create_file(s, _attrs[i]);
#ifdef CONFIG_PPC64
- if
Re: [RFT 00/13] iomap: Constify ioreadX() iomem argument
On Wed, Jan 08, 2020 at 09:44:36AM +0100, Arnd Bergmann wrote:
> On Wed, Jan 8, 2020 at 9:36 AM Christophe Leroy
> wrote:
> > Le 08/01/2020 à 09:18, Krzysztof Kozlowski a écrit :
> > > On Wed, 8 Jan 2020 at 09:13, Geert Uytterhoeven
> > > wrote:
> > > I'll add to this one also changes to ioreadX_rep() and add another
> > > patch for volatile for reads and writes. I guess your review will be
> > > appreciated once more because of ioreadX_rep()
> > >
> >
> > volatile should really only be used where deemed necessary:
> >
> > https://www.kernel.org/doc/html/latest/process/volatile-considered-harmful.html
> >
> > It is said: " ... accessor functions might use volatile on
> > architectures where direct I/O memory access does work. Essentially,
> > each accessor call becomes a little critical section on its own and
> > ensures that the access happens as expected by the programmer."
>
> The I/O accessors are one of the few places in which 'volatile' generally
> makes sense, at least for the implementations that do a plain pointer
> dereference (probably none of the ones in question here).
>
> In case of readl/writel, this is what we do in asm-generic:
>
> static inline u32 __raw_readl(const volatile void __iomem *addr)
> {
> return *(const volatile u32 __force *)addr;
> }
SuperH is another example:
1. ioread8_rep(void __iomem *addr, void *dst, unsigned long count)
calls mmio_insb()
2. static inline void mmio_insb(void __iomem *addr, u8 *dst, int count)
calls __raw_readb()
3. #define __raw_readb(a) (__chk_io_ptr(a), *(volatile u8 __force
*)(a))
Even if interface was not marked as volatile, in fact its implementation
was casting to volatile.
> The __force-cast that removes the __iomem here also means that
> the 'volatile' keyword could be dropped from the argument list,
> as it has no real effect any more, but then there are a few drivers
> that mark their iomem pointers as either 'volatile void __iomem*' or
> (worse) 'volatile void *', so we keep it in the argument list to not
> add warnings for those drivers.
>
> It may be time to change these drivers to not use volatile for __iomem
> pointers, but that seems out of scope for what Krzysztof is trying
> to do. Ideally we would be consistent here though, either using volatile
> all the time or never.
Indeed. I guess there are no objections around const so let me send v2
for const only.
Best regards,
Krzysztof
Re: [RFT 02/13] alpha: Constify ioreadX() iomem argument (as in generic implementation)
On Wed, Jan 08, 2020 at 09:10:06AM +0100, Geert Uytterhoeven wrote: > Hi Krzysztof, > > On Tue, Jan 7, 2020 at 5:53 PM Krzysztof Kozlowski wrote: > > The ioreadX() helpers have inconsistent interface. On some architectures > > void *__iomem address argument is a pointer to const, on some not. > > > > Implementations of ioreadX() do not modify the memory under the address > > so they can be converted to a "const" version for const-safety and > > consistency among architectures. > > > > Signed-off-by: Krzysztof Kozlowski > > > --- a/arch/alpha/include/asm/io.h > > +++ b/arch/alpha/include/asm/io.h > > @@ -151,9 +151,9 @@ static inline void generic_##NAME(TYPE b, QUAL void > > __iomem *addr) \ > > alpha_mv.mv_##NAME(b, addr);\ > > } > > > > -REMAP1(unsigned int, ioread8, /**/) > > -REMAP1(unsigned int, ioread16, /**/) > > -REMAP1(unsigned int, ioread32, /**/) > > +REMAP1(unsigned int, ioread8, const) > > +REMAP1(unsigned int, ioread16, const) > > +REMAP1(unsigned int, ioread32, const) > > If these would become "const volatile", there would no longer be a need > for the last parameter of the REMAP1() macro. > > > REMAP1(u8, readb, const volatile) > > REMAP1(u16, readw, const volatile) > > REMAP1(u32, readl, const volatile) > > Same for REMAP2() macro below, for iowrite*(). Good point, thanks! Best regards, Krzysztof
[PATCH v2] powerpc/32: refactor pmd_offset(pud_offset(pgd_offset...
At several places pmd pointer is retrieved through the same action:
pmd = pmd_offset(pud_offset(pgd_offset(mm, addr), addr), addr);
or
pmd = pmd_offset(pud_offset(pgd_offset_k(addr), addr), addr);
Refactor this by implementing two helpers pmd_ptr() and pmd_ptr_k()
This will help when adding the p4d level.
Signed-off-by: Christophe Leroy
---
v2: fixed missing arg in mm/mem.c in call to pte_offset_kernel()
---
arch/powerpc/include/asm/pgtable.h| 12
arch/powerpc/mm/book3s32/mmu.c| 2 +-
arch/powerpc/mm/book3s32/tlb.c| 4 ++--
arch/powerpc/mm/kasan/kasan_init_32.c | 8
arch/powerpc/mm/mem.c | 3 +--
arch/powerpc/mm/nohash/40x.c | 4 ++--
arch/powerpc/mm/pgtable_32.c | 2 +-
7 files changed, 23 insertions(+), 12 deletions(-)
diff --git a/arch/powerpc/include/asm/pgtable.h
b/arch/powerpc/include/asm/pgtable.h
index 0e4ec8cc37b7..b5e358c0ea7e 100644
--- a/arch/powerpc/include/asm/pgtable.h
+++ b/arch/powerpc/include/asm/pgtable.h
@@ -41,6 +41,18 @@ struct mm_struct;
#ifndef __ASSEMBLY__
+#ifdef CONFIG_PPC32
+static inline pmd_t *pmd_ptr(struct mm_struct *mm, unsigned long va)
+{
+ return pmd_offset(pud_offset(pgd_offset(mm, va), va), va);
+}
+
+static inline pmd_t *pmd_ptr_k(unsigned long va)
+{
+ return pmd_offset(pud_offset(pgd_offset_k(va), va), va);
+}
+#endif
+
#include
/* Keep these as a macros to avoid include dependency mess */
diff --git a/arch/powerpc/mm/book3s32/mmu.c b/arch/powerpc/mm/book3s32/mmu.c
index 69b2419accef..91553e1ff4b9 100644
--- a/arch/powerpc/mm/book3s32/mmu.c
+++ b/arch/powerpc/mm/book3s32/mmu.c
@@ -312,7 +312,7 @@ void hash_preload(struct mm_struct *mm, unsigned long ea)
if (!Hash)
return;
- pmd = pmd_offset(pud_offset(pgd_offset(mm, ea), ea), ea);
+ pmd = pmd_ptr(mm, ea);
if (!pmd_none(*pmd))
add_hash_page(mm->context.id, ea, pmd_val(*pmd));
}
diff --git a/arch/powerpc/mm/book3s32/tlb.c b/arch/powerpc/mm/book3s32/tlb.c
index 2fcd321040ff..b08f0ec7f409 100644
--- a/arch/powerpc/mm/book3s32/tlb.c
+++ b/arch/powerpc/mm/book3s32/tlb.c
@@ -87,7 +87,7 @@ static void flush_range(struct mm_struct *mm, unsigned long
start,
if (start >= end)
return;
end = (end - 1) | ~PAGE_MASK;
- pmd = pmd_offset(pud_offset(pgd_offset(mm, start), start), start);
+ pmd = pmd_ptr(mm, start);
for (;;) {
pmd_end = ((start + PGDIR_SIZE) & PGDIR_MASK) - 1;
if (pmd_end > end)
@@ -145,7 +145,7 @@ void flush_tlb_page(struct vm_area_struct *vma, unsigned
long vmaddr)
return;
}
mm = (vmaddr < TASK_SIZE)? vma->vm_mm: _mm;
- pmd = pmd_offset(pud_offset(pgd_offset(mm, vmaddr), vmaddr), vmaddr);
+ pmd = pmd_ptr(mm, vmaddr);
if (!pmd_none(*pmd))
flush_hash_pages(mm->context.id, vmaddr, pmd_val(*pmd), 1);
}
diff --git a/arch/powerpc/mm/kasan/kasan_init_32.c
b/arch/powerpc/mm/kasan/kasan_init_32.c
index 0e6ed4413eea..4b505ff0ff44 100644
--- a/arch/powerpc/mm/kasan/kasan_init_32.c
+++ b/arch/powerpc/mm/kasan/kasan_init_32.c
@@ -36,7 +36,7 @@ static int __ref kasan_init_shadow_page_tables(unsigned long
k_start, unsigned l
unsigned long k_cur, k_next;
pgprot_t prot = slab_is_available() ? kasan_prot_ro() : PAGE_KERNEL;
- pmd = pmd_offset(pud_offset(pgd_offset_k(k_start), k_start), k_start);
+ pmd = pmd_ptr_k(k_start);
for (k_cur = k_start; k_cur != k_end; k_cur = k_next, pmd++) {
pte_t *new;
@@ -94,7 +94,7 @@ static int __ref kasan_init_region(void *start, size_t size)
block = memblock_alloc(k_end - k_start, PAGE_SIZE);
for (k_cur = k_start & PAGE_MASK; k_cur < k_end; k_cur += PAGE_SIZE) {
- pmd_t *pmd = pmd_offset(pud_offset(pgd_offset_k(k_cur), k_cur),
k_cur);
+ pmd_t *pmd = pmd_ptr_k(k_cur);
void *va = block ? block + k_cur - k_start :
kasan_get_one_page();
pte_t pte = pfn_pte(PHYS_PFN(__pa(va)), PAGE_KERNEL);
@@ -118,7 +118,7 @@ static void __init kasan_remap_early_shadow_ro(void)
kasan_populate_pte(kasan_early_shadow_pte, prot);
for (k_cur = k_start & PAGE_MASK; k_cur < k_end; k_cur += PAGE_SIZE) {
- pmd_t *pmd = pmd_offset(pud_offset(pgd_offset_k(k_cur), k_cur),
k_cur);
+ pmd_t *pmd = pmd_ptr_k(k_cur);
pte_t *ptep = pte_offset_kernel(pmd, k_cur);
if ((pte_val(*ptep) & PTE_RPN_MASK) != pa)
@@ -205,7 +205,7 @@ void __init kasan_early_init(void)
unsigned long addr = KASAN_SHADOW_START;
unsigned long end = KASAN_SHADOW_END;
unsigned long next;
- pmd_t *pmd = pmd_offset(pud_offset(pgd_offset_k(addr), addr), addr);
+ pmd_t *pmd = pmd_ptr_k(addr);
BUILD_BUG_ON(KASAN_SHADOW_START & ~PGDIR_MASK);
diff --git
Re: [RFT 00/13] iomap: Constify ioreadX() iomem argument
Hi Geert, Le 08/01/2020 à 09:43, Geert Uytterhoeven a écrit : Hi Christophe, On Wed, Jan 8, 2020 at 9:35 AM Christophe Leroy wrote: Le 08/01/2020 à 09:18, Krzysztof Kozlowski a écrit : On Wed, 8 Jan 2020 at 09:13, Geert Uytterhoeven wrote: On Wed, Jan 8, 2020 at 9:07 AM Geert Uytterhoeven wrote: On Tue, Jan 7, 2020 at 5:53 PM Krzysztof Kozlowski wrote: The ioread8/16/32() and others have inconsistent interface among the architectures: some taking address as const, some not. It seems there is nothing really stopping all of them to take pointer to const. Shouldn't all of them take const volatile __iomem pointers? It seems the "volatile" is missing from all but the implementations in include/asm-generic/io.h. As my "volatile" comment applies to iowrite*(), too, probably that should be done in a separate patch. Hence with patches 1-5 squashed, and for patches 11-13: Reviewed-by: Geert Uytterhoeven I'll add to this one also changes to ioreadX_rep() and add another patch for volatile for reads and writes. I guess your review will be appreciated once more because of ioreadX_rep() volatile should really only be used where deemed necessary: https://www.kernel.org/doc/html/latest/process/volatile-considered-harmful.html It is said: " ... accessor functions might use volatile on architectures where direct I/O memory access does work. Essentially, each accessor call becomes a little critical section on its own and ensures that the access happens as expected by the programmer." That is exactly the use case here: all above are accessor functions. Why would ioreadX() not need volatile, while readY() does? My point was: it might be necessary for some arches and not for others. And as pointed by Arnd, the volatile is really only necessary for the dereference itself, should the arch use dereferencing. So I guess the best would be to go in the other direction: remove volatile keyword wherever possible instead of adding it where it is not needed. Christophe
Re: [RFT 00/13] iomap: Constify ioreadX() iomem argument
On Wed, Jan 8, 2020 at 9:36 AM Christophe Leroy wrote:
> Le 08/01/2020 à 09:18, Krzysztof Kozlowski a écrit :
> > On Wed, 8 Jan 2020 at 09:13, Geert Uytterhoeven
> > wrote:
> > I'll add to this one also changes to ioreadX_rep() and add another
> > patch for volatile for reads and writes. I guess your review will be
> > appreciated once more because of ioreadX_rep()
> >
>
> volatile should really only be used where deemed necessary:
>
> https://www.kernel.org/doc/html/latest/process/volatile-considered-harmful.html
>
> It is said: " ... accessor functions might use volatile on
> architectures where direct I/O memory access does work. Essentially,
> each accessor call becomes a little critical section on its own and
> ensures that the access happens as expected by the programmer."
The I/O accessors are one of the few places in which 'volatile' generally
makes sense, at least for the implementations that do a plain pointer
dereference (probably none of the ones in question here).
In case of readl/writel, this is what we do in asm-generic:
static inline u32 __raw_readl(const volatile void __iomem *addr)
{
return *(const volatile u32 __force *)addr;
}
The __force-cast that removes the __iomem here also means that
the 'volatile' keyword could be dropped from the argument list,
as it has no real effect any more, but then there are a few drivers
that mark their iomem pointers as either 'volatile void __iomem*' or
(worse) 'volatile void *', so we keep it in the argument list to not
add warnings for those drivers.
It may be time to change these drivers to not use volatile for __iomem
pointers, but that seems out of scope for what Krzysztof is trying
to do. Ideally we would be consistent here though, either using volatile
all the time or never.
Arnd
Re: [RFT 00/13] iomap: Constify ioreadX() iomem argument
Hi Christophe,
On Wed, Jan 8, 2020 at 9:35 AM Christophe Leroy wrote:
> Le 08/01/2020 à 09:18, Krzysztof Kozlowski a écrit :
> > On Wed, 8 Jan 2020 at 09:13, Geert Uytterhoeven
> > wrote:
> >> On Wed, Jan 8, 2020 at 9:07 AM Geert Uytterhoeven
> >> wrote:
> >>> On Tue, Jan 7, 2020 at 5:53 PM Krzysztof Kozlowski
> >>> wrote:
> The ioread8/16/32() and others have inconsistent interface among the
> architectures: some taking address as const, some not.
>
> It seems there is nothing really stopping all of them to take
> pointer to const.
> >>>
> >>> Shouldn't all of them take const volatile __iomem pointers?
> >>> It seems the "volatile" is missing from all but the implementations in
> >>> include/asm-generic/io.h.
> >>
> >> As my "volatile" comment applies to iowrite*(), too, probably that should
> >> be
> >> done in a separate patch.
> >>
> >> Hence with patches 1-5 squashed, and for patches 11-13:
> >> Reviewed-by: Geert Uytterhoeven
> >
> > I'll add to this one also changes to ioreadX_rep() and add another
> > patch for volatile for reads and writes. I guess your review will be
> > appreciated once more because of ioreadX_rep()
>
> volatile should really only be used where deemed necessary:
>
> https://www.kernel.org/doc/html/latest/process/volatile-considered-harmful.html
>
> It is said: " ... accessor functions might use volatile on
> architectures where direct I/O memory access does work. Essentially,
> each accessor call becomes a little critical section on its own and
> ensures that the access happens as expected by the programmer."
That is exactly the use case here: all above are accessor functions.
Why would ioreadX() not need volatile, while readY() does?
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Re: [PATCH] powerpc/32: refactor pmd_offset(pud_offset(pgd_offset...
Hi Christophe,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on powerpc/next]
[also build test ERROR on v5.5-rc5 next-20200106]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url:
https://github.com/0day-ci/linux/commits/Christophe-Leroy/powerpc-32-refactor-pmd_offset-pud_offset-pgd_offset/20200107-210342
base: https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next
config: powerpc-pmac32_defconfig (attached as .config)
compiler: powerpc-linux-gcc (GCC) 7.5.0
reproduce:
wget
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O
~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=7.5.0 make.cross ARCH=powerpc
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot
All errors (new ones prefixed by >>):
arch/powerpc/mm/mem.c: In function 'virt_to_kpte':
>> arch/powerpc/mm/mem.c:71:43: error: macro "pte_offset_kernel" requires 2
>> arguments, but only 1 given
return pte_offset_kernel(pmd_ptr_k(vaddr));
^
>> arch/powerpc/mm/mem.c:71:9: error: 'pte_offset_kernel' undeclared (first use
>> in this function); did you mean 'pte_free_kernel'?
return pte_offset_kernel(pmd_ptr_k(vaddr));
^
pte_free_kernel
arch/powerpc/mm/mem.c:71:9: note: each undeclared identifier is reported
only once for each function it appears in
arch/powerpc/mm/mem.c:72:1: error: control reaches end of non-void function
[-Werror=return-type]
}
^
cc1: all warnings being treated as errors
vim +/pte_offset_kernel +71 arch/powerpc/mm/mem.c
68
69 static inline pte_t *virt_to_kpte(unsigned long vaddr)
70 {
> 71 return pte_offset_kernel(pmd_ptr_k(vaddr));
72 }
73 #endif
74
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/hyperkitty/list/kbuild-...@lists.01.org Intel Corporation
.config.gz
Description: application/gzip
Re: [RFT 00/13] iomap: Constify ioreadX() iomem argument
Le 08/01/2020 à 09:18, Krzysztof Kozlowski a écrit : On Wed, 8 Jan 2020 at 09:13, Geert Uytterhoeven wrote: Hi Krzysztof, On Wed, Jan 8, 2020 at 9:07 AM Geert Uytterhoeven wrote: On Tue, Jan 7, 2020 at 5:53 PM Krzysztof Kozlowski wrote: The ioread8/16/32() and others have inconsistent interface among the architectures: some taking address as const, some not. It seems there is nothing really stopping all of them to take pointer to const. Shouldn't all of them take const volatile __iomem pointers? It seems the "volatile" is missing from all but the implementations in include/asm-generic/io.h. As my "volatile" comment applies to iowrite*(), too, probably that should be done in a separate patch. Hence with patches 1-5 squashed, and for patches 11-13: Reviewed-by: Geert Uytterhoeven I'll add to this one also changes to ioreadX_rep() and add another patch for volatile for reads and writes. I guess your review will be appreciated once more because of ioreadX_rep() volatile should really only be used where deemed necessary: https://www.kernel.org/doc/html/latest/process/volatile-considered-harmful.html It is said: " ... accessor functions might use volatile on architectures where direct I/O memory access does work. Essentially, each accessor call becomes a little critical section on its own and ensures that the access happens as expected by the programmer." Christophe
Re: [RFT 00/13] iomap: Constify ioreadX() iomem argument
On Wed, 8 Jan 2020 at 09:13, Geert Uytterhoeven wrote: > > Hi Krzysztof, > > On Wed, Jan 8, 2020 at 9:07 AM Geert Uytterhoeven > wrote: > > On Tue, Jan 7, 2020 at 5:53 PM Krzysztof Kozlowski wrote: > > > The ioread8/16/32() and others have inconsistent interface among the > > > architectures: some taking address as const, some not. > > > > > > It seems there is nothing really stopping all of them to take > > > pointer to const. > > > > Shouldn't all of them take const volatile __iomem pointers? > > It seems the "volatile" is missing from all but the implementations in > > include/asm-generic/io.h. > > As my "volatile" comment applies to iowrite*(), too, probably that should be > done in a separate patch. > > Hence with patches 1-5 squashed, and for patches 11-13: > Reviewed-by: Geert Uytterhoeven I'll add to this one also changes to ioreadX_rep() and add another patch for volatile for reads and writes. I guess your review will be appreciated once more because of ioreadX_rep() Thanks, Krzysztof
Re: [RFT 00/13] iomap: Constify ioreadX() iomem argument
On Wed, 8 Jan 2020 at 09:08, Geert Uytterhoeven wrote: > > Hi Krzysztof, > > On Tue, Jan 7, 2020 at 5:53 PM Krzysztof Kozlowski wrote: > > The ioread8/16/32() and others have inconsistent interface among the > > architectures: some taking address as const, some not. > > > > It seems there is nothing really stopping all of them to take > > pointer to const. > > Shouldn't all of them take const volatile __iomem pointers? > It seems the "volatile" is missing from all but the implementations in > include/asm-generic/io.h. It's kind of separate issue although I could squash it to limit redundant changes. > > Patchset was really tested on all affected architectures. I just spot an error in my first message. I wanted to say: "Patchset was NOT really tested on all affected architectures." Obviously. > > Build testing is in progress - I hope auto-builders will point any issues. > > > > > > Todo > > > > Convert also string versions (ioread16_rep() etc) if this aproach looks OK. > > > > > > Merging > > === > > The first 5 patches - iomap, alpha, sh, parisc and powerpc - should > > probably go > > via one tree, or even squashed into one. > > Yes, they should be squashed, cfr. Arnd's comment. > I also wouldn't bother doing the updates in patches 6-10. Indeed, thanks for comments. Best regards, Krzysztof
Re: [RFT 00/13] iomap: Constify ioreadX() iomem argument
Hi Krzysztof,
On Wed, Jan 8, 2020 at 9:07 AM Geert Uytterhoeven wrote:
> On Tue, Jan 7, 2020 at 5:53 PM Krzysztof Kozlowski wrote:
> > The ioread8/16/32() and others have inconsistent interface among the
> > architectures: some taking address as const, some not.
> >
> > It seems there is nothing really stopping all of them to take
> > pointer to const.
>
> Shouldn't all of them take const volatile __iomem pointers?
> It seems the "volatile" is missing from all but the implementations in
> include/asm-generic/io.h.
As my "volatile" comment applies to iowrite*(), too, probably that should be
done in a separate patch.
Hence with patches 1-5 squashed, and for patches 11-13:
Reviewed-by: Geert Uytterhoeven
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Re: [RFT 02/13] alpha: Constify ioreadX() iomem argument (as in generic implementation)
Hi Krzysztof,
On Tue, Jan 7, 2020 at 5:53 PM Krzysztof Kozlowski wrote:
> The ioreadX() helpers have inconsistent interface. On some architectures
> void *__iomem address argument is a pointer to const, on some not.
>
> Implementations of ioreadX() do not modify the memory under the address
> so they can be converted to a "const" version for const-safety and
> consistency among architectures.
>
> Signed-off-by: Krzysztof Kozlowski
> --- a/arch/alpha/include/asm/io.h
> +++ b/arch/alpha/include/asm/io.h
> @@ -151,9 +151,9 @@ static inline void generic_##NAME(TYPE b, QUAL void
> __iomem *addr) \
> alpha_mv.mv_##NAME(b, addr);\
> }
>
> -REMAP1(unsigned int, ioread8, /**/)
> -REMAP1(unsigned int, ioread16, /**/)
> -REMAP1(unsigned int, ioread32, /**/)
> +REMAP1(unsigned int, ioread8, const)
> +REMAP1(unsigned int, ioread16, const)
> +REMAP1(unsigned int, ioread32, const)
If these would become "const volatile", there would no longer be a need
for the last parameter of the REMAP1() macro.
> REMAP1(u8, readb, const volatile)
> REMAP1(u16, readw, const volatile)
> REMAP1(u32, readl, const volatile)
Same for REMAP2() macro below, for iowrite*().
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Re: [RFT 00/13] iomap: Constify ioreadX() iomem argument
Hi Krzysztof,
On Tue, Jan 7, 2020 at 5:53 PM Krzysztof Kozlowski wrote:
> The ioread8/16/32() and others have inconsistent interface among the
> architectures: some taking address as const, some not.
>
> It seems there is nothing really stopping all of them to take
> pointer to const.
Shouldn't all of them take const volatile __iomem pointers?
It seems the "volatile" is missing from all but the implementations in
include/asm-generic/io.h.
> Patchset was really tested on all affected architectures.
> Build testing is in progress - I hope auto-builders will point any issues.
>
>
> Todo
>
> Convert also string versions (ioread16_rep() etc) if this aproach looks OK.
>
>
> Merging
> ===
> The first 5 patches - iomap, alpha, sh, parisc and powerpc - should probably
> go
> via one tree, or even squashed into one.
Yes, they should be squashed, cfr. Arnd's comment.
I also wouldn't bother doing the updates in patches 6-10.
The rest looks good to me.
Thanks a lot!
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
