* Re: [PATCH 2/9] fs: add an argument-less alloc_anon_inode
From: Christian Brauner @ 2021-03-10 9:35 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Jason Gunthorpe, Michael S. Tsirkin, VMware, Inc.,
David Hildenbrand, linux-kernel, dri-devel, virtualization,
linux-mm, Minchan Kim, Alex Williamson, Nadav Amit, Al Viro,
Daniel Vetter, linux-fsdevel, Andrew Morton, linuxppc-dev,
Nitin Gupta
In-Reply-To: <20210309155348.974875-3-hch@lst.de>
On Tue, Mar 09, 2021 at 04:53:41PM +0100, Christoph Hellwig wrote:
> Add a new alloc_anon_inode helper that allocates an inode on
> the anon_inode file system.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
Looks good!
Reviewed-by: Christian Brauner <christian.brauner@ubuntu.com>
^ permalink raw reply
* Re: PowerPC64 future proof kernel toc, revised for lld
From: Alexey Kardashevskiy @ 2021-03-10 9:33 UTC (permalink / raw)
To: Alan Modra; +Cc: alexey, linuxppc-dev, ellerman
In-Reply-To: <20210310050722.GN6042@bubble.grove.modra.org>
One more question - the older version had a construct "DEFINED (.TOC.) ?
.TOC. : ..." in case .TOC. is not defined (too old ld? too old gcc?) but
the newer patch seems assuming it is always defined, when was it added?
I have the same check in SLOF, for example, do I still need it?
On 10/03/2021 16:07, Alan Modra wrote:
> On Wed, Mar 10, 2021 at 03:44:44PM +1100, Alexey Kardashevskiy wrote:
>> For my own education, is .got for prom_init.o still generated by ld or gcc?
>
> .got is generated by ld.
>
>> In other words, should "objdump -D -s -j .got" ever dump .got for any .o
>> file, like below?
>
> No. "objdump -r prom_init.o | grep GOT" will tell you whether
> prom_init.o *may* cause ld to generate .got entries. (Linker
> optimisations or --gc-sections might remove the need for those .got
> entries.)
>
>> objdump: section '.got' mentioned in a -j option, but not found in any input
>> file
>
> Right, expected.
>
--
Alexey Kardashevskiy
IBM OzLabs, LTC Team
e-mail: aik@linux.ibm.com
^ permalink raw reply
* Re: [PATCH 1/9] fs: rename alloc_anon_inode to alloc_anon_inode_sb
From: Christian Brauner @ 2021-03-10 9:33 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Jason Gunthorpe, Michael S. Tsirkin, VMware, Inc.,
David Hildenbrand, linux-kernel, dri-devel, virtualization,
linux-mm, Minchan Kim, Alex Williamson, Nadav Amit, Al Viro,
Daniel Vetter, linux-fsdevel, Andrew Morton, linuxppc-dev,
Nitin Gupta
In-Reply-To: <20210309155348.974875-2-hch@lst.de>
On Tue, Mar 09, 2021 at 04:53:40PM +0100, Christoph Hellwig wrote:
> Rename alloc_inode to free the name for a new variant that does not
> need boilerplate to create a super_block first.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
Looks good (with the metioned fix in
https://lore.kernel.org/lkml/20210310083040.GA5217@lst.de)
Reviewed-by: Christian Brauner <christian.brauner@ubuntu.com>
> arch/powerpc/platforms/pseries/cmm.c | 2 +-
> drivers/dma-buf/dma-buf.c | 2 +-
> drivers/gpu/drm/drm_drv.c | 2 +-
> drivers/misc/cxl/api.c | 2 +-
> drivers/misc/vmw_balloon.c | 2 +-
> drivers/scsi/cxlflash/ocxl_hw.c | 2 +-
> drivers/virtio/virtio_balloon.c | 2 +-
> fs/aio.c | 2 +-
> fs/anon_inodes.c | 4 ++--
> fs/libfs.c | 2 +-
> include/linux/fs.h | 2 +-
> kernel/resource.c | 2 +-
> mm/z3fold.c | 2 +-
> mm/zsmalloc.c | 2 +-
> 14 files changed, 15 insertions(+), 15 deletions(-)
>
> diff --git a/arch/powerpc/platforms/pseries/cmm.c b/arch/powerpc/platforms/pseries/cmm.c
> index 45a3a3022a85c9..6d36b858b14df1 100644
> --- a/arch/powerpc/platforms/pseries/cmm.c
> +++ b/arch/powerpc/platforms/pseries/cmm.c
> @@ -580,7 +580,7 @@ static int cmm_balloon_compaction_init(void)
> return rc;
> }
>
> - b_dev_info.inode = alloc_anon_inode(balloon_mnt->mnt_sb);
> + b_dev_info.inode = alloc_anon_inode_sb(balloon_mnt->mnt_sb);
> if (IS_ERR(b_dev_info.inode)) {
> rc = PTR_ERR(b_dev_info.inode);
> b_dev_info.inode = NULL;
> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
> index f264b70c383eb4..dedcc9483352dc 100644
> --- a/drivers/dma-buf/dma-buf.c
> +++ b/drivers/dma-buf/dma-buf.c
> @@ -445,7 +445,7 @@ static inline int is_dma_buf_file(struct file *file)
> static struct file *dma_buf_getfile(struct dma_buf *dmabuf, int flags)
> {
> struct file *file;
> - struct inode *inode = alloc_anon_inode(dma_buf_mnt->mnt_sb);
> + struct inode *inode = alloc_anon_inode_sb(dma_buf_mnt->mnt_sb);
>
> if (IS_ERR(inode))
> return ERR_CAST(inode);
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index 20d22e41d7ce74..87e7214a8e3565 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -519,7 +519,7 @@ static struct inode *drm_fs_inode_new(void)
> return ERR_PTR(r);
> }
>
> - inode = alloc_anon_inode(drm_fs_mnt->mnt_sb);
> + inode = alloc_anon_inode_sb(drm_fs_mnt->mnt_sb);
> if (IS_ERR(inode))
> simple_release_fs(&drm_fs_mnt, &drm_fs_cnt);
>
> diff --git a/drivers/misc/cxl/api.c b/drivers/misc/cxl/api.c
> index b493de962153ba..2efbf6c98028ef 100644
> --- a/drivers/misc/cxl/api.c
> +++ b/drivers/misc/cxl/api.c
> @@ -73,7 +73,7 @@ static struct file *cxl_getfile(const char *name,
> goto err_module;
> }
>
> - inode = alloc_anon_inode(cxl_vfs_mount->mnt_sb);
> + inode = alloc_anon_inode_sb(cxl_vfs_mount->mnt_sb);
> if (IS_ERR(inode)) {
> file = ERR_CAST(inode);
> goto err_fs;
> diff --git a/drivers/misc/vmw_balloon.c b/drivers/misc/vmw_balloon.c
> index b837e7eba5f7dc..5d057a05ddbee8 100644
> --- a/drivers/misc/vmw_balloon.c
> +++ b/drivers/misc/vmw_balloon.c
> @@ -1900,7 +1900,7 @@ static __init int vmballoon_compaction_init(struct vmballoon *b)
> return PTR_ERR(vmballoon_mnt);
>
> b->b_dev_info.migratepage = vmballoon_migratepage;
> - b->b_dev_info.inode = alloc_anon_inode(vmballoon_mnt->mnt_sb);
> + b->b_dev_info.inode = alloc_anon_inode_sb(vmballoon_mnt->mnt_sb);
>
> if (IS_ERR(b->b_dev_info.inode))
> return PTR_ERR(b->b_dev_info.inode);
> diff --git a/drivers/scsi/cxlflash/ocxl_hw.c b/drivers/scsi/cxlflash/ocxl_hw.c
> index 244fc27215dc79..40184ed926b557 100644
> --- a/drivers/scsi/cxlflash/ocxl_hw.c
> +++ b/drivers/scsi/cxlflash/ocxl_hw.c
> @@ -88,7 +88,7 @@ static struct file *ocxlflash_getfile(struct device *dev, const char *name,
> goto err2;
> }
>
> - inode = alloc_anon_inode(ocxlflash_vfs_mount->mnt_sb);
> + inode = alloc_anon_inode_sb(ocxlflash_vfs_mount->mnt_sb);
> if (IS_ERR(inode)) {
> rc = PTR_ERR(inode);
> dev_err(dev, "%s: alloc_anon_inode failed rc=%d\n",
> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> index 8985fc2cea8615..cae76ee5bdd688 100644
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -916,7 +916,7 @@ static int virtballoon_probe(struct virtio_device *vdev)
> }
>
> vb->vb_dev_info.migratepage = virtballoon_migratepage;
> - vb->vb_dev_info.inode = alloc_anon_inode(balloon_mnt->mnt_sb);
> + vb->vb_dev_info.inode = alloc_anon_inode_sb(balloon_mnt->mnt_sb);
> if (IS_ERR(vb->vb_dev_info.inode)) {
> err = PTR_ERR(vb->vb_dev_info.inode);
> goto out_kern_unmount;
> diff --git a/fs/aio.c b/fs/aio.c
> index 1f32da13d39ee6..d1c2aa7fd6de7c 100644
> --- a/fs/aio.c
> +++ b/fs/aio.c
> @@ -234,7 +234,7 @@ static const struct address_space_operations aio_ctx_aops;
> static struct file *aio_private_file(struct kioctx *ctx, loff_t nr_pages)
> {
> struct file *file;
> - struct inode *inode = alloc_anon_inode(aio_mnt->mnt_sb);
> + struct inode *inode = alloc_anon_inode_sb(aio_mnt->mnt_sb);
> if (IS_ERR(inode))
> return ERR_CAST(inode);
>
> diff --git a/fs/anon_inodes.c b/fs/anon_inodes.c
> index a280156138ed89..4745fc37014332 100644
> --- a/fs/anon_inodes.c
> +++ b/fs/anon_inodes.c
> @@ -63,7 +63,7 @@ static struct inode *anon_inode_make_secure_inode(
> const struct qstr qname = QSTR_INIT(name, strlen(name));
> int error;
>
> - inode = alloc_anon_inode(anon_inode_mnt->mnt_sb);
> + inode = alloc_anon_inode_sb(anon_inode_mnt->mnt_sb);
> if (IS_ERR(inode))
> return inode;
> inode->i_flags &= ~S_PRIVATE;
> @@ -231,7 +231,7 @@ static int __init anon_inode_init(void)
> if (IS_ERR(anon_inode_mnt))
> panic("anon_inode_init() kernel mount failed (%ld)\n", PTR_ERR(anon_inode_mnt));
>
> - anon_inode_inode = alloc_anon_inode(anon_inode_mnt->mnt_sb);
> + anon_inode_inode = alloc_anon_inode_sb(anon_inode_mnt->mnt_sb);
> if (IS_ERR(anon_inode_inode))
> panic("anon_inode_init() inode allocation failed (%ld)\n", PTR_ERR(anon_inode_inode));
>
> diff --git a/fs/libfs.c b/fs/libfs.c
> index e2de5401abca5a..600bebc1cd847f 100644
> --- a/fs/libfs.c
> +++ b/fs/libfs.c
> @@ -1216,7 +1216,7 @@ static int anon_set_page_dirty(struct page *page)
> return 0;
> };
>
> -struct inode *alloc_anon_inode(struct super_block *s)
> +struct inode *alloc_anon_inode_sb(struct super_block *s)
> {
> static const struct address_space_operations anon_aops = {
> .set_page_dirty = anon_set_page_dirty,
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index ec8f3ddf4a6aa8..52387368af3c00 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -3286,7 +3286,7 @@ extern int simple_write_end(struct file *file, struct address_space *mapping,
> loff_t pos, unsigned len, unsigned copied,
> struct page *page, void *fsdata);
> extern int always_delete_dentry(const struct dentry *);
> -extern struct inode *alloc_anon_inode(struct super_block *);
> +extern struct inode *alloc_anon_inode_sb(struct super_block *);
> extern int simple_nosetlease(struct file *, long, struct file_lock **, void **);
> extern const struct dentry_operations simple_dentry_operations;
>
> diff --git a/kernel/resource.c b/kernel/resource.c
> index 627e61b0c12418..0fd091a3f2fc66 100644
> --- a/kernel/resource.c
> +++ b/kernel/resource.c
> @@ -1863,7 +1863,7 @@ static int __init iomem_init_inode(void)
> return rc;
> }
>
> - inode = alloc_anon_inode(iomem_vfs_mount->mnt_sb);
> + inode = alloc_anon_inode_sb(iomem_vfs_mount->mnt_sb);
> if (IS_ERR(inode)) {
> rc = PTR_ERR(inode);
> pr_err("Cannot allocate inode for iomem: %d\n", rc);
> diff --git a/mm/z3fold.c b/mm/z3fold.c
> index b5dafa7e44e429..e7cd9298b221f5 100644
> --- a/mm/z3fold.c
> +++ b/mm/z3fold.c
> @@ -376,7 +376,7 @@ static void z3fold_unmount(void)
> static const struct address_space_operations z3fold_aops;
> static int z3fold_register_migration(struct z3fold_pool *pool)
> {
> - pool->inode = alloc_anon_inode(z3fold_mnt->mnt_sb);
> + pool->inode = alloc_anon_inode_sb(z3fold_mnt->mnt_sb);
> if (IS_ERR(pool->inode)) {
> pool->inode = NULL;
> return 1;
> diff --git a/mm/zsmalloc.c b/mm/zsmalloc.c
> index 30c358b7202510..a6449a2ad861de 100644
> --- a/mm/zsmalloc.c
> +++ b/mm/zsmalloc.c
> @@ -2086,7 +2086,7 @@ static const struct address_space_operations zsmalloc_aops = {
>
> static int zs_register_migration(struct zs_pool *pool)
> {
> - pool->inode = alloc_anon_inode(zsmalloc_mnt->mnt_sb);
> + pool->inode = alloc_anon_inode_sb(zsmalloc_mnt->mnt_sb);
> if (IS_ERR(pool->inode)) {
> pool->inode = NULL;
> return 1;
> --
> 2.30.1
>
^ permalink raw reply
* Re: PowerPC64 future proof kernel toc, revised for lld
From: Alexey Kardashevskiy @ 2021-03-10 4:44 UTC (permalink / raw)
To: Alan Modra, linuxppc-dev; +Cc: alexey, ellerman
In-Reply-To: <20210310034813.GM6042@bubble.grove.modra.org>
On 10/03/2021 14:48, Alan Modra wrote:
> This patch future-proofs the kernel against linker changes that might
> put the toc pointer at some location other than .got+0x8000, by
> replacing __toc_start+0x8000 with .TOC. throughout. If the kernel's
> idea of the toc pointer doesn't agree with the linker, bad things
> happen.
Works great with gcc (v8, v10), ld (2.23), clang-11, lld-11.
>
> prom_init.c code relocating its toc is also changed so that a symbolic
> __prom_init_toc_start toc-pointer relative address is calculated
> rather than assuming that it is always at toc-pointer - 0x8000. The
> length calculations loading values from the toc are also avoided.
> It's a little incestuous to do that with unreloc_toc picking up
> adjusted values (which is fine in practice, they both adjust by the
> same amount if all goes well).
>
> I've also changed the way .got is aligned in vmlinux.lds and
> zImage.lds, mostly so that dumping out section info by objdump or
> readelf plainly shows the alignment is 256. This linker script
> feature was added 2005-09-27, available in FSF binutils releases from
> 2.17 onwards. Should be safe to use in the kernel, I think.
>
> Finally, put *(.got) before the prom_init.o entry which only needs
> *(.toc), so that the GOT header goes in the correct place. I don't
> believe this makes any difference for the kernel as it would for
> dynamic objects being loaded by ld.so. That change is just to stop
> lusers who blindly copy kernel scripts being led astray. Of course,
> this change needs the prom_init.c changes.
>
> Some notes on .toc and .got.
>
> .toc is a compiler generated section of addresses. .got is a linker
> generated section of addresses, generally built when the linker sees
> R_*_*GOT* relocations. In the case of powerpc64 ld.bfd, there are
> multiple generated .got sections, one per input object file. So you
> can somewhat reasonably write in a linker script an input section
> statement like *prom_init.o(.got .toc) to mean "the .got and .toc
> section for files matching *prom_init.o".
For my own education, is .got for prom_init.o still generated by ld or gcc?
In other words, should "objdump -D -s -j .got" ever dump .got for any .o
file, like below?
===
objdump -D -s -j .got
~/pbuild/kernel-llvm-ld/arch/powerpc/kernel/prom_init.o
/home/aik/pbuild/kernel-llvm-ld/arch/powerpc/kernel/prom_init.o:
file format elf64-powerpcle
objdump: section '.got' mentioned in a -j option, but not found in any
input file
===
--
Alexey Kardashevskiy
IBM OzLabs, LTC Team
e-mail: aik@linux.ibm.com
^ permalink raw reply
* Re: [PATCH 14/17] iommu: remove DOMAIN_ATTR_DMA_USE_FLUSH_QUEUE
From: Christoph Hellwig @ 2021-03-10 9:25 UTC (permalink / raw)
To: Robin Murphy
Cc: kvm, Will Deacon, Joerg Roedel, linuxppc-dev, dri-devel, Li Yang,
iommu, netdev, David Woodhouse, linux-arm-kernel, virtualization,
freedreno, Christoph Hellwig, linux-arm-msm
In-Reply-To: <20210310091501.GC5928@lst.de>
On Wed, Mar 10, 2021 at 10:15:01AM +0100, Christoph Hellwig wrote:
> On Thu, Mar 04, 2021 at 03:25:27PM +0000, Robin Murphy wrote:
> > On 2021-03-01 08:42, Christoph Hellwig wrote:
> >> Use explicit methods for setting and querying the information instead.
> >
> > Now that everyone's using iommu-dma, is there any point in bouncing this
> > through the drivers at all? Seems like it would make more sense for the x86
> > drivers to reflect their private options back to iommu_dma_strict (and
> > allow Intel's caching mode to override it as well), then have
> > iommu_dma_init_domain just test !iommu_dma_strict &&
> > domain->ops->flush_iotlb_all.
>
> Hmm. I looked at this, and kill off ->dma_enable_flush_queue for
> the ARM drivers and just looking at iommu_dma_strict seems like a
> very clear win.
>
> OTOH x86 is a little more complicated. AMD and intel defaul to lazy
> mode, so we'd have to change the global iommu_dma_strict if they are
> initialized. Also Intel has not only a "static" option to disable
> lazy mode, but also a "dynamic" one where it iterates structure. So
> I think on the get side we're stuck with the method, but it still
> simplifies the whole thing.
Actually... Just mirroring the iommu_dma_strict value into
struct iommu_domain should solve all of that with very little
boilerplate code.
^ permalink raw reply
* Re: [PATCH] KVM: PPC: Book3S HV: Do not expose HFSCR sanitisation to nested hypervisor
From: Paul Mackerras @ 2021-03-10 9:23 UTC (permalink / raw)
To: Nicholas Piggin; +Cc: linuxppc-dev, kvm-ppc, Fabiano Rosas
In-Reply-To: <1615191200.1pjltfhe7o.astroid@bobo.none>
On Mon, Mar 08, 2021 at 06:18:47PM +1000, Nicholas Piggin wrote:
> Excerpts from Fabiano Rosas's message of March 6, 2021 9:10 am:
> > As one of the arguments of the H_ENTER_NESTED hypercall, the nested
> > hypervisor (L1) prepares a structure containing the values of various
> > hypervisor-privileged registers with which it wants the nested guest
> > (L2) to run. Since the nested HV runs in supervisor mode it needs the
> > host to write to these registers.
> >
> > To stop a nested HV manipulating this mechanism and using a nested
> > guest as a proxy to access a facility that has been made unavailable
> > to it, we have a routine that sanitises the values of the HV registers
> > before copying them into the nested guest's vcpu struct.
> >
> > However, when coming out of the guest the values are copied as they
> > were back into L1 memory, which means that any sanitisation we did
> > during guest entry will be exposed to L1 after H_ENTER_NESTED returns.
> >
> > This is not a problem by itself, but in the case of the Hypervisor
> > Facility Status and Control Register (HFSCR), we use the intersection
> > between L2 hfscr bits and L1 hfscr bits. That means that L1 could use
> > this to indirectly read the (hv-privileged) value from its vcpu
> > struct.
> >
> > This patch fixes this by making sure that L1 only gets back the bits
> > that are necessary for regular functioning.
>
> The general idea of restricting exposure of HV privileged bits, but
> for the case of HFSCR a guest can probe the HFCR anyway by testing which
> facilities are available (and presumably an HV may need some way to know
> what features are available for it to advertise to its own guests), so
> is this necessary? Perhaps a comment would be sufficient.
I would see it a bit differently. From L1's point of view, L0 is the
hardware. The situation we have now is akin to writing a value to the
real HFSCR, then reading HFSCR and finding that some of the facility
enable bits have magically got set to zero. That's not the way real
hardware works, so L0 shouldn't behave that way either, or at least
not without some strong justification.
Paul.
^ permalink raw reply
* Re: [PATCH] powerpc/mm: Add cond_resched() while removing hpte mappings
From: Michael Ellerman @ 2021-03-10 9:17 UTC (permalink / raw)
To: Vaibhav Jain, linuxppc-dev; +Cc: Vaibhav Jain, Aneesh Kumar K . V
In-Reply-To: <20210310075938.361656-1-vaibhav@linux.ibm.com>
Vaibhav Jain <vaibhav@linux.ibm.com> writes:
> While removing large number of mappings from hash page tables for
> large memory systems as soft-lockup is reported because of the time
> spent inside htap_remove_mapping() like one below:
>
> watchdog: BUG: soft lockup - CPU#8 stuck for 23s!
> <snip>
> NIP plpar_hcall+0x38/0x58
> LR pSeries_lpar_hpte_invalidate+0x68/0xb0
> Call Trace:
> 0x1fffffffffff000 (unreliable)
> pSeries_lpar_hpte_removebolted+0x9c/0x230
> hash__remove_section_mapping+0xec/0x1c0
> remove_section_mapping+0x28/0x3c
> arch_remove_memory+0xfc/0x150
> devm_memremap_pages_release+0x180/0x2f0
> devm_action_release+0x30/0x50
> release_nodes+0x28c/0x300
> device_release_driver_internal+0x16c/0x280
> unbind_store+0x124/0x170
> drv_attr_store+0x44/0x60
> sysfs_kf_write+0x64/0x90
> kernfs_fop_write+0x1b0/0x290
> __vfs_write+0x3c/0x70
> vfs_write+0xd4/0x270
> ksys_write+0xdc/0x130
> system_call+0x5c/0x70
>
> Fix this by adding a cond_resched() to the loop in
> htap_remove_mapping() that issues hcall to remove hpte mapping. This
> should prevent the soft-lockup from being reported.
Can/should we also/instead be using H_BLOCK_REMOVE?
cheers
^ permalink raw reply
* Re: [PATCH 14/17] iommu: remove DOMAIN_ATTR_DMA_USE_FLUSH_QUEUE
From: Christoph Hellwig @ 2021-03-10 9:15 UTC (permalink / raw)
To: Robin Murphy
Cc: kvm, Will Deacon, Joerg Roedel, linuxppc-dev, dri-devel, Li Yang,
iommu, netdev, David Woodhouse, linux-arm-kernel, virtualization,
freedreno, Christoph Hellwig, linux-arm-msm
In-Reply-To: <1658805c-ed28-b650-7385-a56fab3383e3@arm.com>
On Thu, Mar 04, 2021 at 03:25:27PM +0000, Robin Murphy wrote:
> On 2021-03-01 08:42, Christoph Hellwig wrote:
>> Use explicit methods for setting and querying the information instead.
>
> Now that everyone's using iommu-dma, is there any point in bouncing this
> through the drivers at all? Seems like it would make more sense for the x86
> drivers to reflect their private options back to iommu_dma_strict (and
> allow Intel's caching mode to override it as well), then have
> iommu_dma_init_domain just test !iommu_dma_strict &&
> domain->ops->flush_iotlb_all.
Hmm. I looked at this, and kill off ->dma_enable_flush_queue for
the ARM drivers and just looking at iommu_dma_strict seems like a
very clear win.
OTOH x86 is a little more complicated. AMD and intel defaul to lazy
mode, so we'd have to change the global iommu_dma_strict if they are
initialized. Also Intel has not only a "static" option to disable
lazy mode, but also a "dynamic" one where it iterates structure. So
I think on the get side we're stuck with the method, but it still
simplifies the whole thing.
^ permalink raw reply
* Re: [PATCH] powerpc/mm: Add cond_resched() while removing hpte mappings
From: Christophe Leroy @ 2021-03-10 9:14 UTC (permalink / raw)
To: Vaibhav Jain, linuxppc-dev; +Cc: Aneesh Kumar K . V
In-Reply-To: <20210310075938.361656-1-vaibhav@linux.ibm.com>
Le 10/03/2021 à 08:59, Vaibhav Jain a écrit :
> While removing large number of mappings from hash page tables for
> large memory systems as soft-lockup is reported because of the time
> spent inside htap_remove_mapping() like one below:
>
> watchdog: BUG: soft lockup - CPU#8 stuck for 23s!
> <snip>
> NIP plpar_hcall+0x38/0x58
> LR pSeries_lpar_hpte_invalidate+0x68/0xb0
> Call Trace:
> 0x1fffffffffff000 (unreliable)
> pSeries_lpar_hpte_removebolted+0x9c/0x230
> hash__remove_section_mapping+0xec/0x1c0
> remove_section_mapping+0x28/0x3c
> arch_remove_memory+0xfc/0x150
> devm_memremap_pages_release+0x180/0x2f0
> devm_action_release+0x30/0x50
> release_nodes+0x28c/0x300
> device_release_driver_internal+0x16c/0x280
> unbind_store+0x124/0x170
> drv_attr_store+0x44/0x60
> sysfs_kf_write+0x64/0x90
> kernfs_fop_write+0x1b0/0x290
> __vfs_write+0x3c/0x70
> vfs_write+0xd4/0x270
> ksys_write+0xdc/0x130
> system_call+0x5c/0x70
>
> Fix this by adding a cond_resched() to the loop in
> htap_remove_mapping() that issues hcall to remove hpte mapping. This
> should prevent the soft-lockup from being reported.
Isn't it overkill to call is at each iteration ?
Looking at a few other places, there is some mitigation. For instance fadump_free_reserved_memory()
does it based on elapsed time. Another exemple is drmem_lmb_next() doing it every 16 iteration.
>
> Suggested-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
> Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com>
> ---
> arch/powerpc/mm/book3s64/hash_utils.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/arch/powerpc/mm/book3s64/hash_utils.c b/arch/powerpc/mm/book3s64/hash_utils.c
> index 581b20a2feaf..ea3945c70b18 100644
> --- a/arch/powerpc/mm/book3s64/hash_utils.c
> +++ b/arch/powerpc/mm/book3s64/hash_utils.c
> @@ -359,6 +359,8 @@ int htab_remove_mapping(unsigned long vstart, unsigned long vend,
> }
> if (rc < 0)
> return rc;
> +
> + cond_resched();
> }
>
> return ret;
>
Christophe
^ permalink raw reply
* Re: [Freedreno] [PATCH 16/17] iommu: remove DOMAIN_ATTR_IO_PGTABLE_CFG
From: Christoph Hellwig @ 2021-03-10 8:58 UTC (permalink / raw)
To: Will Deacon
Cc: freedreno, Sai Prakash Ranjan, kvm, Joerg Roedel, linuxppc-dev,
dri-devel, Li Yang, Rob Clark,
list@263.net:IOMMU DRIVERS <iommu@lists.linux-foundation.org>, Joerg Roedel <joro@8bytes.org>, ,
David Woodhouse, linux-arm-msm, netdev, virtualization,
Robin Murphy, Christoph Hellwig,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE
In-Reply-To: <20210305100012.GB22536@willie-the-truck>
On Fri, Mar 05, 2021 at 10:00:12AM +0000, Will Deacon wrote:
> > But one thing I'm not sure about is whether
> > IO_PGTABLE_QUIRK_ARM_OUTER_WBWA is something that other devices
> > *should* be using as well, but just haven't gotten around to yet.
>
> The intention is certainly that this would be a place to collate per-domain
> pgtable quirks, so I'd prefer not to tie that to the GPU.
So the overall consensus is to just keep this as-is for now?
^ permalink raw reply
* Re: [PATCH 14/17] iommu: remove DOMAIN_ATTR_DMA_USE_FLUSH_QUEUE
From: Christoph Hellwig @ 2021-03-10 8:54 UTC (permalink / raw)
To: Robin Murphy
Cc: kvm, Will Deacon, Joerg Roedel, linuxppc-dev, dri-devel, Li Yang,
iommu, netdev, David Woodhouse, linux-arm-kernel, virtualization,
freedreno, Christoph Hellwig, linux-arm-msm
In-Reply-To: <1658805c-ed28-b650-7385-a56fab3383e3@arm.com>
On Thu, Mar 04, 2021 at 03:25:27PM +0000, Robin Murphy wrote:
> On 2021-03-01 08:42, Christoph Hellwig wrote:
>> Use explicit methods for setting and querying the information instead.
>
> Now that everyone's using iommu-dma, is there any point in bouncing this
> through the drivers at all? Seems like it would make more sense for the x86
> drivers to reflect their private options back to iommu_dma_strict (and
> allow Intel's caching mode to override it as well), then have
> iommu_dma_init_domain just test !iommu_dma_strict &&
> domain->ops->flush_iotlb_all.
Indeed. I switch to that.
^ permalink raw reply
* Re: make alloc_anon_inode more useful
From: Christoph Hellwig @ 2021-03-10 8:37 UTC (permalink / raw)
To: Matthew Wilcox
Cc: Jason Gunthorpe, Michael S. Tsirkin, VMware, Inc.,
David Hildenbrand, linux-kernel, dri-devel, virtualization,
linux-mm, Minchan Kim, Alex Williamson, Nadav Amit, Al Viro,
Daniel Vetter, linux-fsdevel, Andrew Morton, linuxppc-dev,
Christoph Hellwig, Nitin Gupta
In-Reply-To: <20210310040545.GM3479805@casper.infradead.org>
On Wed, Mar 10, 2021 at 04:05:45AM +0000, Matthew Wilcox wrote:
> On Tue, Mar 09, 2021 at 04:53:39PM +0100, Christoph Hellwig wrote:
> > this series first renames the existing alloc_anon_inode to
> > alloc_anon_inode_sb to clearly mark it as requiring a superblock.
> >
> > It then adds a new alloc_anon_inode that works on the anon_inode
> > file system super block, thus removing tons of boilerplate code.
> >
> > The few remainig callers of alloc_anon_inode_sb all use alloc_file_pseudo
> > later, but might also be ripe for some cleanup.
>
> On a somewhat related note, could I get you to look at
> drivers/video/fbdev/core/fb_defio.c?
>
> As far as I can tell, there's no need for fb_deferred_io_aops to exist.
> We could just set file->f_mapping->a_ops to NULL, and set_page_dirty()
> would do the exact same thing this code does (except it would get the
> return value correct).
> But maybe that would make something else go wrong that distinguishes
> between page->mapping being NULL and page->mapping->a_ops->foo being NULL?
I can't find any place in the kernel that treats a NULL aops different
from not having the method it is looking for.
> Completely untested patch ...
the patch looks mostly good to me.
> }
> -#ifdef CONFIG_FB_DEFERRED_IO
> - if (info->fbdefio)
> - fb_deferred_io_open(info, inode, file);
> -#endif
> + file->f_mapping->a_ops = NULL;
But I'd also skip this. Drivers generally do not set aops, but if they
do a funtion like this really should not override it. This will require
an audit of the callers, though.
^ permalink raw reply
* Re: make alloc_anon_inode more useful
From: Christoph Hellwig @ 2021-03-10 8:32 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Michael S. Tsirkin, VMware, Inc., David Hildenbrand, linux-kernel,
dri-devel, virtualization, linux-mm, Minchan Kim, Alex Williamson,
Nadav Amit, Al Viro, Daniel Vetter, linux-fsdevel, Andrew Morton,
linuxppc-dev, Christoph Hellwig, Nitin Gupta
In-Reply-To: <20210309165452.GL2356281@nvidia.com>
On Tue, Mar 09, 2021 at 12:54:52PM -0400, Jason Gunthorpe wrote:
> On Tue, Mar 09, 2021 at 04:53:39PM +0100, Christoph Hellwig wrote:
> > Hi all,
> >
> > this series first renames the existing alloc_anon_inode to
> > alloc_anon_inode_sb to clearly mark it as requiring a superblock.
> >
> > It then adds a new alloc_anon_inode that works on the anon_inode
> > file system super block, thus removing tons of boilerplate code.
> >
> > The few remainig callers of alloc_anon_inode_sb all use alloc_file_pseudo
> > later, but might also be ripe for some cleanup.
>
> I like it
>
> For a submission plan can we have this on a git branch please? I will
> need a copy for RDMA and Alex will need one for vfio..
anon_inode.c stuff seems to mostly go through Al's tree, but also various
others. So best would be if Al has a branch, but I could also set one up.
^ permalink raw reply
* Re: [PATCH 1/9] fs: rename alloc_anon_inode to alloc_anon_inode_sb
From: Christoph Hellwig @ 2021-03-10 8:30 UTC (permalink / raw)
To: Minchan Kim
Cc: Jason Gunthorpe, Michael S. Tsirkin, VMware, Inc.,
David Hildenbrand, linux-kernel, dri-devel, virtualization,
linux-mm, Alex Williamson, Nadav Amit, Al Viro, Daniel Vetter,
linux-fsdevel, Andrew Morton, linuxppc-dev, Christoph Hellwig,
Nitin Gupta
In-Reply-To: <YEhpiRCOUll6Ri6J@google.com>
On Tue, Mar 09, 2021 at 10:39:05PM -0800, Minchan Kim wrote:
> > -struct inode *alloc_anon_inode(struct super_block *s)
> > +struct inode *alloc_anon_inode_sb(struct super_block *s)
> > {
> > static const struct address_space_operations anon_aops = {
> > .set_page_dirty = anon_set_page_dirty,
>
> EXPORT_SYMBOL(alloc_anon_inode_sb) ?
Yes, I actually fixed this up shortly after sending the series.
^ permalink raw reply
* Re: [PATCH v1 01/15] powerpc/uaccess: Remove __get_user_allowed() and unsafe_op_wrap()
From: Christophe Leroy @ 2021-03-10 8:14 UTC (permalink / raw)
To: Daniel Axtens, Benjamin Herrenschmidt, Paul Mackerras,
Michael Ellerman
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <87im6ao7ld.fsf@dja-thinkpad.axtens.net>
Le 01/03/2021 à 23:02, Daniel Axtens a écrit :
>
>
> Christophe Leroy <christophe.leroy@csgroup.eu> writes:
>
>> Those two macros have only one user which is unsafe_get_user().
>>
>> Put everything in one place and remove them.
>>
>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>> ---
>> arch/powerpc/include/asm/uaccess.h | 10 +++++-----
>> 1 file changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h
>> index 78e2a3990eab..8cbf3e3874f1 100644
>> --- a/arch/powerpc/include/asm/uaccess.h
>> +++ b/arch/powerpc/include/asm/uaccess.h
>> @@ -53,9 +53,6 @@ static inline bool __access_ok(unsigned long addr, unsigned long size)
>> #define __put_user(x, ptr) \
>> __put_user_nocheck((__typeof__(*(ptr)))(x), (ptr), sizeof(*(ptr)))
>>
>> -#define __get_user_allowed(x, ptr) \
>> - __get_user_nocheck((x), (ptr), sizeof(*(ptr)), false)
>> -
>> #define __get_user_inatomic(x, ptr) \
>> __get_user_nosleep((x), (ptr), sizeof(*(ptr)))
>> #define __put_user_inatomic(x, ptr) \
>> @@ -482,8 +479,11 @@ user_write_access_begin(const void __user *ptr, size_t len)
>> #define user_write_access_begin user_write_access_begin
>> #define user_write_access_end prevent_current_write_to_user
>>
>> -#define unsafe_op_wrap(op, err) do { if (unlikely(op)) goto err; } while (0)
>> -#define unsafe_get_user(x, p, e) unsafe_op_wrap(__get_user_allowed(x, p), e)
>> +#define unsafe_get_user(x, p, e) do { \
>> + if (unlikely(__get_user_nocheck((x), (p), sizeof(*(p)), false)))\
>> + goto e; \
>> +} while (0)
>> +
>
> This seems correct to me.
>
> Checkpatch does have one check that is relevant:
>
> CHECK: Macro argument reuse 'p' - possible side-effects?
> #36: FILE: arch/powerpc/include/asm/uaccess.h:482:
> +#define unsafe_get_user(x, p, e) do { \
> + if (unlikely(__get_user_nocheck((x), (p), sizeof(*(p)), false)))\
> + goto e; \
> +} while (0)
>
> Given that we are already creating a new block, should we do something
> like this (completely untested):
>
> #define unsafe_get_user(x, p, e) do { \
> __typeof__(p) __p = (p);
> if (unlikely(__get_user_nocheck((x), (__p), sizeof(*(__p)), false)))\
> goto e; \
> } while (0)
>
As mentioned by Segher, this is not needed, sizeof(p) doesn't evaluate (p) so (p) is only evaluated
once in the macro, so no risk of side-effects with that.
Christophe
^ permalink raw reply
* Re: [PATCH v1 03/15] powerpc/uaccess: Remove __get/put_user_inatomic()
From: Christophe Leroy @ 2021-03-10 8:03 UTC (permalink / raw)
To: Daniel Axtens, Benjamin Herrenschmidt, Paul Mackerras,
Michael Ellerman
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <87czwio5qz.fsf@dja-thinkpad.axtens.net>
Le 01/03/2021 à 23:42, Daniel Axtens a écrit :
> Christophe Leroy <christophe.leroy@csgroup.eu> writes:
>
>> Since commit 662bbcb2747c ("mm, sched: Allow uaccess in atomic with
>> pagefault_disable()"), __get/put_user() can be used in atomic parts
>> of the code, therefore the __get/put_user_inatomic() introduced
>> by commit e68c825bb016 ("[POWERPC] Add inatomic versions of __get_user
>> and __put_user") have become useless.
>
> I spent some time chasing these macro definitions.
Ok, I'll try to make it more explicit.
>
> Let me see if I understand you.
>
> __get_user(x, ptr) becomes __get_user_nocheck(..., true)
> __get_user_inatomic() become __get_user_nosleep()
>
> The difference between how __get_user_nosleep() and
> __get_user_nocheck(..., true) operate is that __get_user_nocheck calls
> might_fault() and __get_user_nosleep() does not.
>
> If I understand the commit you reference and mm/memory.c, you're saying
> that we can indeed call might_fault() when page faults are disabled,
> because __might_fault() checks if page faults are disabled and does not
> fire a warning if it is called with page faults disabled.
>
> Therefore, it is safe to remove our _inatomic version that does not call
> might_fault and just to call might_fault unconditionally.
>
> Is that right?
Yes that's exactly that.
>
> I haven't checked changes you made to the various .c files in fine
> detail but they appear to be entirely mechanical.
>
>> powerpc is the only one having such functions. There is a real
>> intention not to have to provide such _inatomic() helpers, see the
>> comment in might_fault() in mm/memory.c introduced by
>> commit 3ee1afa308f2 ("x86: some lock annotations for user
>> copy paths, v2"):
>>
>> /*
>> * it would be nicer only to annotate paths which are not under
>> * pagefault_disable, however that requires a larger audit and
>> * providing helpers like get_user_atomic.
>> */
>>
>
> I'm not fully sure I understand what you're saying in this part of the
> commit message.
The idea was to say, based of that unrelated comment I found, that really nobody wanted to have to
use atomic dedicated helpers like.
As it brings more confusion than clarify, I'll remove it and only say that powperpc was the only
architecture with such helpers.
Christophe
>
> Kind regards,
> Daniel
>
>>
>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>> ---
>> arch/powerpc/include/asm/uaccess.h | 37 -------------------
>> arch/powerpc/kernel/align.c | 32 ++++++++--------
>> .../kernel/hw_breakpoint_constraints.c | 2 +-
>> arch/powerpc/kernel/traps.c | 2 +-
>> 4 files changed, 18 insertions(+), 55 deletions(-)
>>
>> diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h
>> index a08c482b1315..01aea0df4dd0 100644
>> --- a/arch/powerpc/include/asm/uaccess.h
>> +++ b/arch/powerpc/include/asm/uaccess.h
>> @@ -53,11 +53,6 @@ static inline bool __access_ok(unsigned long addr, unsigned long size)
>> #define __put_user(x, ptr) \
>> __put_user_nocheck((__typeof__(*(ptr)))(x), (ptr), sizeof(*(ptr)))
>>
>> -#define __get_user_inatomic(x, ptr) \
>> - __get_user_nosleep((x), (ptr), sizeof(*(ptr)))
>> -#define __put_user_inatomic(x, ptr) \
>> - __put_user_nosleep((__typeof__(*(ptr)))(x), (ptr), sizeof(*(ptr)))
>> -
>> #ifdef CONFIG_PPC64
>>
>> #define ___get_user_instr(gu_op, dest, ptr) \
>> @@ -92,9 +87,6 @@ static inline bool __access_ok(unsigned long addr, unsigned long size)
>> #define __get_user_instr(x, ptr) \
>> ___get_user_instr(__get_user, x, ptr)
>>
>> -#define __get_user_instr_inatomic(x, ptr) \
>> - ___get_user_instr(__get_user_inatomic, x, ptr)
>> -
>> extern long __put_user_bad(void);
>>
>> #define __put_user_size(x, ptr, size, retval) \
>> @@ -141,20 +133,6 @@ __pu_failed: \
>> __pu_err; \
>> })
>>
>> -#define __put_user_nosleep(x, ptr, size) \
>> -({ \
>> - long __pu_err; \
>> - __typeof__(*(ptr)) __user *__pu_addr = (ptr); \
>> - __typeof__(*(ptr)) __pu_val = (x); \
>> - __typeof__(size) __pu_size = (size); \
>> - \
>> - __chk_user_ptr(__pu_addr); \
>> - __put_user_size(__pu_val, __pu_addr, __pu_size, __pu_err); \
>> - \
>> - __pu_err; \
>> -})
>> -
>> -
>> /*
>> * We don't tell gcc that we are accessing memory, but this is OK
>> * because we do not write to any memory gcc knows about, so there
>> @@ -320,21 +298,6 @@ do { \
>> __gu_err; \
>> })
>>
>> -#define __get_user_nosleep(x, ptr, size) \
>> -({ \
>> - long __gu_err; \
>> - __long_type(*(ptr)) __gu_val; \
>> - __typeof__(*(ptr)) __user *__gu_addr = (ptr); \
>> - __typeof__(size) __gu_size = (size); \
>> - \
>> - __chk_user_ptr(__gu_addr); \
>> - __get_user_size(__gu_val, __gu_addr, __gu_size, __gu_err); \
>> - (x) = (__force __typeof__(*(ptr)))__gu_val; \
>> - \
>> - __gu_err; \
>> -})
>> -
>> -
>> /* more complex routines */
>>
>> extern unsigned long __copy_tofrom_user(void __user *to,
>> diff --git a/arch/powerpc/kernel/align.c b/arch/powerpc/kernel/align.c
>> index c7797eb958c7..83b199026a1e 100644
>> --- a/arch/powerpc/kernel/align.c
>> +++ b/arch/powerpc/kernel/align.c
>> @@ -174,18 +174,18 @@ static int emulate_spe(struct pt_regs *regs, unsigned int reg,
>>
>> switch (nb) {
>> case 8:
>> - ret |= __get_user_inatomic(temp.v[0], p++);
>> - ret |= __get_user_inatomic(temp.v[1], p++);
>> - ret |= __get_user_inatomic(temp.v[2], p++);
>> - ret |= __get_user_inatomic(temp.v[3], p++);
>> + ret |= __get_user(temp.v[0], p++);
>> + ret |= __get_user(temp.v[1], p++);
>> + ret |= __get_user(temp.v[2], p++);
>> + ret |= __get_user(temp.v[3], p++);
>> fallthrough;
>> case 4:
>> - ret |= __get_user_inatomic(temp.v[4], p++);
>> - ret |= __get_user_inatomic(temp.v[5], p++);
>> + ret |= __get_user(temp.v[4], p++);
>> + ret |= __get_user(temp.v[5], p++);
>> fallthrough;
>> case 2:
>> - ret |= __get_user_inatomic(temp.v[6], p++);
>> - ret |= __get_user_inatomic(temp.v[7], p++);
>> + ret |= __get_user(temp.v[6], p++);
>> + ret |= __get_user(temp.v[7], p++);
>> if (unlikely(ret))
>> return -EFAULT;
>> }
>> @@ -259,18 +259,18 @@ static int emulate_spe(struct pt_regs *regs, unsigned int reg,
>> p = addr;
>> switch (nb) {
>> case 8:
>> - ret |= __put_user_inatomic(data.v[0], p++);
>> - ret |= __put_user_inatomic(data.v[1], p++);
>> - ret |= __put_user_inatomic(data.v[2], p++);
>> - ret |= __put_user_inatomic(data.v[3], p++);
>> + ret |= __put_user(data.v[0], p++);
>> + ret |= __put_user(data.v[1], p++);
>> + ret |= __put_user(data.v[2], p++);
>> + ret |= __put_user(data.v[3], p++);
>> fallthrough;
>> case 4:
>> - ret |= __put_user_inatomic(data.v[4], p++);
>> - ret |= __put_user_inatomic(data.v[5], p++);
>> + ret |= __put_user(data.v[4], p++);
>> + ret |= __put_user(data.v[5], p++);
>> fallthrough;
>> case 2:
>> - ret |= __put_user_inatomic(data.v[6], p++);
>> - ret |= __put_user_inatomic(data.v[7], p++);
>> + ret |= __put_user(data.v[6], p++);
>> + ret |= __put_user(data.v[7], p++);
>> }
>> if (unlikely(ret))
>> return -EFAULT;
>> diff --git a/arch/powerpc/kernel/hw_breakpoint_constraints.c b/arch/powerpc/kernel/hw_breakpoint_constraints.c
>> index 867ee4aa026a..675d1f66ab72 100644
>> --- a/arch/powerpc/kernel/hw_breakpoint_constraints.c
>> +++ b/arch/powerpc/kernel/hw_breakpoint_constraints.c
>> @@ -141,7 +141,7 @@ void wp_get_instr_detail(struct pt_regs *regs, struct ppc_inst *instr,
>> {
>> struct instruction_op op;
>>
>> - if (__get_user_instr_inatomic(*instr, (void __user *)regs->nip))
>> + if (__get_user_instr(*instr, (void __user *)regs->nip))
>> return;
>>
>> analyse_instr(&op, regs, *instr);
>> diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
>> index 1583fd1c6010..1fa36bd08efe 100644
>> --- a/arch/powerpc/kernel/traps.c
>> +++ b/arch/powerpc/kernel/traps.c
>> @@ -864,7 +864,7 @@ static void p9_hmi_special_emu(struct pt_regs *regs)
>> unsigned long ea, msr, msr_mask;
>> bool swap;
>>
>> - if (__get_user_inatomic(instr, (unsigned int __user *)regs->nip))
>> + if (__get_user(instr, (unsigned int __user *)regs->nip))
>> return;
>>
>> /*
>> --
>> 2.25.0
^ permalink raw reply
* [PATCH] powerpc/mm: Add cond_resched() while removing hpte mappings
From: Vaibhav Jain @ 2021-03-10 7:59 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Vaibhav Jain, Aneesh Kumar K . V
While removing large number of mappings from hash page tables for
large memory systems as soft-lockup is reported because of the time
spent inside htap_remove_mapping() like one below:
watchdog: BUG: soft lockup - CPU#8 stuck for 23s!
<snip>
NIP plpar_hcall+0x38/0x58
LR pSeries_lpar_hpte_invalidate+0x68/0xb0
Call Trace:
0x1fffffffffff000 (unreliable)
pSeries_lpar_hpte_removebolted+0x9c/0x230
hash__remove_section_mapping+0xec/0x1c0
remove_section_mapping+0x28/0x3c
arch_remove_memory+0xfc/0x150
devm_memremap_pages_release+0x180/0x2f0
devm_action_release+0x30/0x50
release_nodes+0x28c/0x300
device_release_driver_internal+0x16c/0x280
unbind_store+0x124/0x170
drv_attr_store+0x44/0x60
sysfs_kf_write+0x64/0x90
kernfs_fop_write+0x1b0/0x290
__vfs_write+0x3c/0x70
vfs_write+0xd4/0x270
ksys_write+0xdc/0x130
system_call+0x5c/0x70
Fix this by adding a cond_resched() to the loop in
htap_remove_mapping() that issues hcall to remove hpte mapping. This
should prevent the soft-lockup from being reported.
Suggested-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com>
---
arch/powerpc/mm/book3s64/hash_utils.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/arch/powerpc/mm/book3s64/hash_utils.c b/arch/powerpc/mm/book3s64/hash_utils.c
index 581b20a2feaf..ea3945c70b18 100644
--- a/arch/powerpc/mm/book3s64/hash_utils.c
+++ b/arch/powerpc/mm/book3s64/hash_utils.c
@@ -359,6 +359,8 @@ int htab_remove_mapping(unsigned long vstart, unsigned long vend,
}
if (rc < 0)
return rc;
+
+ cond_resched();
}
return ret;
--
2.29.2
^ permalink raw reply related
* Re: [PATCH 9/9] zsmalloc: remove the zsmalloc file system
From: Minchan Kim @ 2021-03-10 6:44 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Jason Gunthorpe, Michael S. Tsirkin, VMware, Inc.,
David Hildenbrand, linux-kernel, dri-devel, virtualization,
linux-mm, Alex Williamson, Nadav Amit, Al Viro, Daniel Vetter,
linux-fsdevel, Andrew Morton, linuxppc-dev, Nitin Gupta
In-Reply-To: <20210309155348.974875-10-hch@lst.de>
On Tue, Mar 09, 2021 at 04:53:48PM +0100, Christoph Hellwig wrote:
> Just use the generic anon_inode file system.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
Acked-by: Minchan Kim <minchan@kernel.org>
^ permalink raw reply
* [PATCH] powerpc: fix warning comparing pointer to 0
From: Jiapeng Chong @ 2021-03-10 6:42 UTC (permalink / raw)
To: mpe; +Cc: linuxppc-dev, paulus, linux-kernel, Jiapeng Chong
Fix the following coccicheck warning:
./arch/powerpc/platforms/powermac/pfunc_core.c:688:40-41: WARNING
comparing pointer to 0.
Reported-by: Abaci Robot <abaci@linux.alibaba.com>
Signed-off-by: Jiapeng Chong <jiapeng.chong@linux.alibaba.com>
---
arch/powerpc/platforms/powermac/pfunc_core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/powerpc/platforms/powermac/pfunc_core.c b/arch/powerpc/platforms/powermac/pfunc_core.c
index 94df0a9..a5aa40f 100644
--- a/arch/powerpc/platforms/powermac/pfunc_core.c
+++ b/arch/powerpc/platforms/powermac/pfunc_core.c
@@ -685,7 +685,7 @@ static int pmf_add_functions(struct pmf_device *dev, void *driverdata)
const int plen = strlen(PP_PREFIX);
int count = 0;
- for (pp = dev->node->properties; pp != 0; pp = pp->next) {
+ for (pp = dev->node->properties; pp; pp = pp->next) {
const char *name;
if (strncmp(pp->name, PP_PREFIX, plen) != 0)
continue;
--
1.8.3.1
^ permalink raw reply related
* Re: [PATCH 1/9] fs: rename alloc_anon_inode to alloc_anon_inode_sb
From: Minchan Kim @ 2021-03-10 6:39 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Jason Gunthorpe, Michael S. Tsirkin, VMware, Inc.,
David Hildenbrand, linux-kernel, dri-devel, virtualization,
linux-mm, Alex Williamson, Nadav Amit, Al Viro, Daniel Vetter,
linux-fsdevel, Andrew Morton, linuxppc-dev, Nitin Gupta
In-Reply-To: <20210309155348.974875-2-hch@lst.de>
On Tue, Mar 09, 2021 at 04:53:40PM +0100, Christoph Hellwig wrote:
> Rename alloc_inode to free the name for a new variant that does not
> need boilerplate to create a super_block first.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
> arch/powerpc/platforms/pseries/cmm.c | 2 +-
> drivers/dma-buf/dma-buf.c | 2 +-
> drivers/gpu/drm/drm_drv.c | 2 +-
> drivers/misc/cxl/api.c | 2 +-
> drivers/misc/vmw_balloon.c | 2 +-
> drivers/scsi/cxlflash/ocxl_hw.c | 2 +-
> drivers/virtio/virtio_balloon.c | 2 +-
> fs/aio.c | 2 +-
> fs/anon_inodes.c | 4 ++--
> fs/libfs.c | 2 +-
> include/linux/fs.h | 2 +-
> kernel/resource.c | 2 +-
> mm/z3fold.c | 2 +-
> mm/zsmalloc.c | 2 +-
> 14 files changed, 15 insertions(+), 15 deletions(-)
>
> diff --git a/arch/powerpc/platforms/pseries/cmm.c b/arch/powerpc/platforms/pseries/cmm.c
> index 45a3a3022a85c9..6d36b858b14df1 100644
> --- a/arch/powerpc/platforms/pseries/cmm.c
> +++ b/arch/powerpc/platforms/pseries/cmm.c
> @@ -580,7 +580,7 @@ static int cmm_balloon_compaction_init(void)
> return rc;
> }
>
> - b_dev_info.inode = alloc_anon_inode(balloon_mnt->mnt_sb);
> + b_dev_info.inode = alloc_anon_inode_sb(balloon_mnt->mnt_sb);
> if (IS_ERR(b_dev_info.inode)) {
> rc = PTR_ERR(b_dev_info.inode);
> b_dev_info.inode = NULL;
> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
> index f264b70c383eb4..dedcc9483352dc 100644
> --- a/drivers/dma-buf/dma-buf.c
> +++ b/drivers/dma-buf/dma-buf.c
> @@ -445,7 +445,7 @@ static inline int is_dma_buf_file(struct file *file)
> static struct file *dma_buf_getfile(struct dma_buf *dmabuf, int flags)
> {
> struct file *file;
> - struct inode *inode = alloc_anon_inode(dma_buf_mnt->mnt_sb);
> + struct inode *inode = alloc_anon_inode_sb(dma_buf_mnt->mnt_sb);
>
> if (IS_ERR(inode))
> return ERR_CAST(inode);
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index 20d22e41d7ce74..87e7214a8e3565 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -519,7 +519,7 @@ static struct inode *drm_fs_inode_new(void)
> return ERR_PTR(r);
> }
>
> - inode = alloc_anon_inode(drm_fs_mnt->mnt_sb);
> + inode = alloc_anon_inode_sb(drm_fs_mnt->mnt_sb);
> if (IS_ERR(inode))
> simple_release_fs(&drm_fs_mnt, &drm_fs_cnt);
>
> diff --git a/drivers/misc/cxl/api.c b/drivers/misc/cxl/api.c
> index b493de962153ba..2efbf6c98028ef 100644
> --- a/drivers/misc/cxl/api.c
> +++ b/drivers/misc/cxl/api.c
> @@ -73,7 +73,7 @@ static struct file *cxl_getfile(const char *name,
> goto err_module;
> }
>
> - inode = alloc_anon_inode(cxl_vfs_mount->mnt_sb);
> + inode = alloc_anon_inode_sb(cxl_vfs_mount->mnt_sb);
> if (IS_ERR(inode)) {
> file = ERR_CAST(inode);
> goto err_fs;
> diff --git a/drivers/misc/vmw_balloon.c b/drivers/misc/vmw_balloon.c
> index b837e7eba5f7dc..5d057a05ddbee8 100644
> --- a/drivers/misc/vmw_balloon.c
> +++ b/drivers/misc/vmw_balloon.c
> @@ -1900,7 +1900,7 @@ static __init int vmballoon_compaction_init(struct vmballoon *b)
> return PTR_ERR(vmballoon_mnt);
>
> b->b_dev_info.migratepage = vmballoon_migratepage;
> - b->b_dev_info.inode = alloc_anon_inode(vmballoon_mnt->mnt_sb);
> + b->b_dev_info.inode = alloc_anon_inode_sb(vmballoon_mnt->mnt_sb);
>
> if (IS_ERR(b->b_dev_info.inode))
> return PTR_ERR(b->b_dev_info.inode);
> diff --git a/drivers/scsi/cxlflash/ocxl_hw.c b/drivers/scsi/cxlflash/ocxl_hw.c
> index 244fc27215dc79..40184ed926b557 100644
> --- a/drivers/scsi/cxlflash/ocxl_hw.c
> +++ b/drivers/scsi/cxlflash/ocxl_hw.c
> @@ -88,7 +88,7 @@ static struct file *ocxlflash_getfile(struct device *dev, const char *name,
> goto err2;
> }
>
> - inode = alloc_anon_inode(ocxlflash_vfs_mount->mnt_sb);
> + inode = alloc_anon_inode_sb(ocxlflash_vfs_mount->mnt_sb);
> if (IS_ERR(inode)) {
> rc = PTR_ERR(inode);
> dev_err(dev, "%s: alloc_anon_inode failed rc=%d\n",
> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> index 8985fc2cea8615..cae76ee5bdd688 100644
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -916,7 +916,7 @@ static int virtballoon_probe(struct virtio_device *vdev)
> }
>
> vb->vb_dev_info.migratepage = virtballoon_migratepage;
> - vb->vb_dev_info.inode = alloc_anon_inode(balloon_mnt->mnt_sb);
> + vb->vb_dev_info.inode = alloc_anon_inode_sb(balloon_mnt->mnt_sb);
> if (IS_ERR(vb->vb_dev_info.inode)) {
> err = PTR_ERR(vb->vb_dev_info.inode);
> goto out_kern_unmount;
> diff --git a/fs/aio.c b/fs/aio.c
> index 1f32da13d39ee6..d1c2aa7fd6de7c 100644
> --- a/fs/aio.c
> +++ b/fs/aio.c
> @@ -234,7 +234,7 @@ static const struct address_space_operations aio_ctx_aops;
> static struct file *aio_private_file(struct kioctx *ctx, loff_t nr_pages)
> {
> struct file *file;
> - struct inode *inode = alloc_anon_inode(aio_mnt->mnt_sb);
> + struct inode *inode = alloc_anon_inode_sb(aio_mnt->mnt_sb);
> if (IS_ERR(inode))
> return ERR_CAST(inode);
>
> diff --git a/fs/anon_inodes.c b/fs/anon_inodes.c
> index a280156138ed89..4745fc37014332 100644
> --- a/fs/anon_inodes.c
> +++ b/fs/anon_inodes.c
> @@ -63,7 +63,7 @@ static struct inode *anon_inode_make_secure_inode(
> const struct qstr qname = QSTR_INIT(name, strlen(name));
> int error;
>
> - inode = alloc_anon_inode(anon_inode_mnt->mnt_sb);
> + inode = alloc_anon_inode_sb(anon_inode_mnt->mnt_sb);
> if (IS_ERR(inode))
> return inode;
> inode->i_flags &= ~S_PRIVATE;
> @@ -231,7 +231,7 @@ static int __init anon_inode_init(void)
> if (IS_ERR(anon_inode_mnt))
> panic("anon_inode_init() kernel mount failed (%ld)\n", PTR_ERR(anon_inode_mnt));
>
> - anon_inode_inode = alloc_anon_inode(anon_inode_mnt->mnt_sb);
> + anon_inode_inode = alloc_anon_inode_sb(anon_inode_mnt->mnt_sb);
> if (IS_ERR(anon_inode_inode))
> panic("anon_inode_init() inode allocation failed (%ld)\n", PTR_ERR(anon_inode_inode));
>
> diff --git a/fs/libfs.c b/fs/libfs.c
> index e2de5401abca5a..600bebc1cd847f 100644
> --- a/fs/libfs.c
> +++ b/fs/libfs.c
> @@ -1216,7 +1216,7 @@ static int anon_set_page_dirty(struct page *page)
> return 0;
> };
>
> -struct inode *alloc_anon_inode(struct super_block *s)
> +struct inode *alloc_anon_inode_sb(struct super_block *s)
> {
> static const struct address_space_operations anon_aops = {
> .set_page_dirty = anon_set_page_dirty,
EXPORT_SYMBOL(alloc_anon_inode_sb) ?
^ permalink raw reply
* Re: [PATCH V2] mm/memtest: Add ARCH_USE_MEMTEST
From: Anshuman Khandual @ 2021-03-10 6:03 UTC (permalink / raw)
To: linux-mm
Cc: Chris Zankel, Thomas Bogendoerfer, linux-kernel, linux-xtensa,
Catalin Marinas, linuxppc-dev, Russell King, linux-mips,
Max Filippov, Ingo Molnar, Paul Mackerras, Thomas Gleixner,
Will Deacon, linux-arm-kernel
In-Reply-To: <1614573126-7740-1-git-send-email-anshuman.khandual@arm.com>
On 3/1/21 10:02 AM, Anshuman Khandual wrote:
> early_memtest() does not get called from all architectures. Hence enabling
> CONFIG_MEMTEST and providing a valid memtest=[1..N] kernel command line
> option might not trigger the memory pattern tests as would be expected in
> normal circumstances. This situation is misleading.
>
> The change here prevents the above mentioned problem after introducing a
> new config option ARCH_USE_MEMTEST that should be subscribed on platforms
> that call early_memtest(), in order to enable the config CONFIG_MEMTEST.
> Conversely CONFIG_MEMTEST cannot be enabled on platforms where it would
> not be tested anyway.
>
> Cc: Russell King <linux@armlinux.org.uk>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will@kernel.org>
> Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
> Cc: Michael Ellerman <mpe@ellerman.id.au>
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Cc: Paul Mackerras <paulus@samba.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Chris Zankel <chris@zankel.net>
> Cc: Max Filippov <jcmvbkbc@gmail.com>
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-mips@vger.kernel.org
> Cc: linuxppc-dev@lists.ozlabs.org
> Cc: linux-xtensa@linux-xtensa.org
> Cc: linux-mm@kvack.org
> Cc: linux-kernel@vger.kernel.org
> Reviewed-by: Max Filippov <jcmvbkbc@gmail.com>
> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
> ---
> This patch applies on v5.12-rc1 and has been tested on arm64 platform.
> But it has been just build tested on all other platforms.
>
> Changes in V2:
>
> - Added ARCH_USE_MEMTEST in the sorted alphabetical order on platforms
Gentle ping, any updates or objections ?
^ permalink raw reply
* Re: [PATCH 2/6] mm: Generalize SYS_SUPPORTS_HUGETLBFS (rename as ARCH_SUPPORTS_HUGETLBFS)
From: Michael Ellerman @ 2021-03-10 5:20 UTC (permalink / raw)
To: Anshuman Khandual, linux-mm
Cc: Rich Felker, linux-ia64, linux-sh, Catalin Marinas, linux-kernel,
James E.J. Bottomley, Paul Mackerras, linux-riscv, Will Deacon,
linux-s390, Yoshinori Sato, Helge Deller, x86, Russell King,
linux-snps-arc, Albert Ou, Anshuman Khandual, Alexander Viro,
Paul Walmsley, linux-arm-kernel, Thomas Bogendoerfer,
linux-parisc, linux-mips, Palmer Dabbelt, linux-fsdevel,
linuxppc-dev
In-Reply-To: <1615185706-24342-3-git-send-email-anshuman.khandual@arm.com>
Anshuman Khandual <anshuman.khandual@arm.com> writes:
> SYS_SUPPORTS_HUGETLBFS config has duplicate definitions on platforms that
> subscribe it. Instead, just make it a generic option which can be selected
> on applicable platforms. Also rename it as ARCH_SUPPORTS_HUGETLBFS instead.
> This reduces code duplication and makes it cleaner.
>
> Cc: Russell King <linux@armlinux.org.uk>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will@kernel.org>
> Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
> Cc: "James E.J. Bottomley" <James.Bottomley@HansenPartnership.com>
> Cc: Helge Deller <deller@gmx.de>
> Cc: Michael Ellerman <mpe@ellerman.id.au>
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Cc: Paul Mackerras <paulus@samba.org>
> Cc: Paul Walmsley <paul.walmsley@sifive.com>
> Cc: Palmer Dabbelt <palmer@dabbelt.com>
> Cc: Albert Ou <aou@eecs.berkeley.edu>
> Cc: Yoshinori Sato <ysato@users.sourceforge.jp>
> Cc: Rich Felker <dalias@libc.org>
> Cc: Alexander Viro <viro@zeniv.linux.org.uk>
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-mips@vger.kernel.org
> Cc: linux-parisc@vger.kernel.org
> Cc: linuxppc-dev@lists.ozlabs.org
> Cc: linux-riscv@lists.infradead.org
> Cc: linux-sh@vger.kernel.org
> Cc: linux-fsdevel@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
> ---
> arch/arm/Kconfig | 5 +----
> arch/arm64/Kconfig | 4 +---
> arch/mips/Kconfig | 6 +-----
> arch/parisc/Kconfig | 5 +----
> arch/powerpc/Kconfig | 3 ---
> arch/powerpc/platforms/Kconfig.cputype | 6 +++---
Acked-by: Michael Ellerman <mpe@ellerman.id.au> (powerpc)
cheers
^ permalink raw reply
* Re: [PATCH v4] powerpc/uprobes: Validation for prefixed instruction
From: Michael Ellerman @ 2021-03-10 5:13 UTC (permalink / raw)
To: Ravi Bangoria, Naveen N. Rao
Cc: Ravi Bangoria, jniethe5, oleg, rostedt, linux-kernel, paulus,
sandipan, naveen.n.rao, linuxppc-dev
In-Reply-To: <1a080cb5-af98-6b6e-352d-772a90cfa902@linux.ibm.com>
Ravi Bangoria <ravi.bangoria@linux.ibm.com> writes:
> On 3/9/21 4:51 PM, Naveen N. Rao wrote:
>> On 2021/03/09 08:54PM, Michael Ellerman wrote:
>>> Ravi Bangoria <ravi.bangoria@linux.ibm.com> writes:
>>>> As per ISA 3.1, prefixed instruction should not cross 64-byte
>>>> boundary. So don't allow Uprobe on such prefixed instruction.
>>>>
>>>> There are two ways probed instruction is changed in mapped pages.
>>>> First, when Uprobe is activated, it searches for all the relevant
>>>> pages and replace instruction in them. In this case, if that probe
>>>> is on the 64-byte unaligned prefixed instruction, error out
>>>> directly. Second, when Uprobe is already active and user maps a
>>>> relevant page via mmap(), instruction is replaced via mmap() code
>>>> path. But because Uprobe is invalid, entire mmap() operation can
>>>> not be stopped. In this case just print an error and continue.
>>>>
>>>> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
>>>> Acked-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
>>>
>>> Do we have a Fixes: tag for this?
>>
>> Since this is an additional check we are adding, I don't think we should
>> add a Fixes: tag. Nothing is broken per-se -- we're just adding more
>> checks to catch simple mistakes. Also, like Oleg pointed out, there are
>> still many other ways for users to shoot themselves in the foot with
>> uprobes and prefixed instructions, if they so desire.
>>
>> However, if you still think we should add a Fixes: tag, we can perhaps
>> use the below commit since I didn't see any specific commit adding
>> support for prefixed instructions for uprobes:
>>
>> Fixes: 650b55b707fdfa ("powerpc: Add prefixed instructions to
>> instruction data type")
>
> True. IMO, It doesn't really need any Fixes tag.
Yep OK, I'm happy without a Fixes tag based on that explanation.
>>>> diff --git a/arch/powerpc/kernel/uprobes.c b/arch/powerpc/kernel/uprobes.c
>>>> index e8a63713e655..4cbfff6e94a3 100644
>>>> --- a/arch/powerpc/kernel/uprobes.c
>>>> +++ b/arch/powerpc/kernel/uprobes.c
>>>> @@ -41,6 +41,13 @@ int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe,
>>>> if (addr & 0x03)
>>>> return -EINVAL;
>>>>
>>>> + if (cpu_has_feature(CPU_FTR_ARCH_31) &&
>>>> + ppc_inst_prefixed(auprobe->insn) &&
>>>> + (addr & (SZ_64 - 4)) == SZ_64 - 4) {
>>>> + pr_info_ratelimited("Cannot register a uprobe on 64 byte unaligned prefixed instruction\n");
>>>> + return -EINVAL;
>>>
>>> I realise we already did the 0x03 check above, but I still think this
>>> would be clearer simply as:
>>>
>>> (addr & 0x3f == 60)
>>
>> Indeed, I like the use of `60' there -- hex is overrated ;)
>
> Sure. Will resend.
Thanks.
cheers
^ permalink raw reply
* Re: PowerPC64 future proof kernel toc, revised for lld
From: Alan Modra @ 2021-03-10 5:07 UTC (permalink / raw)
To: Alexey Kardashevskiy; +Cc: alexey, linuxppc-dev, ellerman
In-Reply-To: <3c92968f-7c61-8d36-4001-91f8630de4b1@linux.ibm.com>
On Wed, Mar 10, 2021 at 03:44:44PM +1100, Alexey Kardashevskiy wrote:
> For my own education, is .got for prom_init.o still generated by ld or gcc?
.got is generated by ld.
> In other words, should "objdump -D -s -j .got" ever dump .got for any .o
> file, like below?
No. "objdump -r prom_init.o | grep GOT" will tell you whether
prom_init.o *may* cause ld to generate .got entries. (Linker
optimisations or --gc-sections might remove the need for those .got
entries.)
> objdump: section '.got' mentioned in a -j option, but not found in any input
> file
Right, expected.
--
Alan Modra
Australia Development Lab, IBM
^ permalink raw reply
* Re: Errant readings on LM81 with T2080 SoC
From: Guenter Roeck @ 2021-03-10 5:06 UTC (permalink / raw)
To: Chris Packham, jdelvare@suse.com
Cc: linux-hwmon@vger.kernel.org, linuxppc-dev@lists.ozlabs.org,
linux-kernel@vger.kernel.org, linux-i2c@vger.kernel.org
In-Reply-To: <1aa0dc23-0706-5902-2f46-0767de0e3ad6@alliedtelesis.co.nz>
On 3/9/21 6:19 PM, Chris Packham wrote:
> On 9/03/21 9:27 am, Chris Packham wrote:
>> On 8/03/21 5:59 pm, Guenter Roeck wrote:
>>> Other than that, the only other real idea I have would be to monitor
>>> the i2c bus.
>> I am in the fortunate position of being able to go into the office and
>> even happen to have the expensive scope at the moment. Now I just need
>> to find a tame HW engineer so I don't burn myself trying to attach the
>> probes.
> One thing I see on the scope is that when there is a CPU load there
> appears to be some clock stretching going on (SCL is held low some
> times). I don't see it without the CPU load. It's hard to correlate a
> clock stretching event with a bad read or error but it is one area where
> the SMBUS spec has a maximum that might cause the device to give up waiting.
>
Do you have CONFIG_PREEMPT enabled in your kernel ? But even without
that it is possible that the hot loops at the beginning and end of
each operation mess up the driver and cause it to sleep longer
than intended. Did you try usleep_range() ?
On a side note, can you send me a register dump for the lm81 ?
It would be useful for my module test code.
Thanks,
Guenter
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox