[PATCH 0/2] Allow deferred execution of iomem_get_mapping()

[PATCH 0/2] Allow deferred execution of iomem_get_mapping()

[Krzysztof Wilczyński]

Hello,

At the moment, the dependency on iomem_get_mapping() that is currently
used in the pci_create_resource_files() and pci_create_legacy_files()
stops us from completely retiring the late_initcall() that is used to
invoke pci_sysfs_init() when creating sysfs object for PCI devices.

This dependency on iomem_get_mapping() stops us from retiring the
late_initcall at the moment as when we convert dynamically added sysfs
objects, that are primarily added in the pci_create_resource_files() and
pci_create_legacy_files(), as these attributes are added before the VFS
completes its initialisation, and since most of the PCI devices are
typically enumerated in subsys_initcall this leads to a failure and an
Oops related to iomem_get_mapping() access.

See relevant conversations:
  https://lore.kernel.org/linux-pci/20210204165831.2703772-1-daniel.vetter@ffwll.ch/
  https://lore.kernel.org/linux-pci/20210313215747.GA2394467@bjorn-Precision-5520/
  
After some deliberation about the problem at hand, Dan Williams
suggested a solution to the problem, see:
  https://lore.kernel.org/linux-pci/CAPcyv4i0y_4cMGEpNVShLUyUk3nyWH203Ry3S87BqnDJE0Rmxg@mail.gmail.com/

The idea is to defer execution of the iomem_get_mapping() to only when
the sysfs open callback is run, and thus removing the reliance of
fs_initcalls to complete before any other sub-system that uses the
iomem_get_mapping().

Currently, the PCI sub-system will benefit the most from this change
allowing for it to complete the transition from dynamically created to
static sysfs objects.

This series aims to take Dan Williams' idea through the finish line.

Related to:
  https://lore.kernel.org/linux-pci/20210527205845.GA1421476@bjorn-Precision-5520/
  https://lore.kernel.org/linux-pci/20210507102706.7658-1-danijel.slivka@amd.com/
  https://lore.kernel.org/linux-pci/20200716110423.xtfyb3n6tn5ixedh@pali/

	Krzysztof

Krzysztof Wilczyński (2):
  sysfs: Invoke iomem_get_mapping() from the sysfs open callback
  PCI/sysfs: Pass iomem_get_mapping() as a function pointer

 drivers/pci/pci-sysfs.c | 6 +++---
 fs/sysfs/file.c         | 2 +-
 include/linux/sysfs.h   | 2 +-
 3 files changed, 5 insertions(+), 5 deletions(-)

-- 
2.32.0

[PATCH 1/2] sysfs: Invoke iomem_get_mapping() from the sysfs open callback

[Krzysztof Wilczyński]

Defer invocation of the iomem_get_mapping() to the sysfs open callback
so that it can be executed as needed when the binary sysfs object has
been accessed.

To do that, convert the "mapping" member of the struct bin_attribute
from a pointer to the struct address_space into a function pointer with
a signature that requires the same return type, and then updates the
sysfs_kf_bin_open() to invoke provided function should the function
pointer be valid.

Thus, this change removes the need for the fs_initcalls to complete
before any other sub-system that uses the iomem_get_mapping() would be
able to invoke it safely without leading to a failure and an Oops
related to an invalid iomem_get_mapping() access.

Co-authored-by: Dan Williams <dan.j.williams@intel.com>
Suggested-by: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Krzysztof Wilczyński <kw@linux.com>
---
 fs/sysfs/file.c       | 2 +-
 include/linux/sysfs.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
index 9aefa7779b29..a3ee4c32a264 100644
--- a/fs/sysfs/file.c
+++ b/fs/sysfs/file.c
@@ -175,7 +175,7 @@ static int sysfs_kf_bin_open(struct kernfs_open_file *of)
 	struct bin_attribute *battr = of->kn->priv;
 
 	if (battr->mapping)
-		of->file->f_mapping = battr->mapping;
+		of->file->f_mapping = battr->mapping();
 
 	return 0;
 }
diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
index d76a1ddf83a3..fbb7c7df545c 100644
--- a/include/linux/sysfs.h
+++ b/include/linux/sysfs.h
@@ -170,7 +170,7 @@ struct bin_attribute {
 	struct attribute	attr;
 	size_t			size;
 	void			*private;
-	struct address_space	*mapping;
+	struct address_space *(*mapping)(void);
 	ssize_t (*read)(struct file *, struct kobject *, struct bin_attribute *,
 			char *, loff_t, size_t);
 	ssize_t (*write)(struct file *, struct kobject *, struct bin_attribute *,
-- 
2.32.0

Re: [PATCH 1/2] sysfs: Invoke iomem_get_mapping() from the sysfs open callback

[Dan Williams]

On Fri, Jun 25, 2021 at 4:31 PM Krzysztof Wilczyński <kw@linux.com> wrote:
>
> Defer invocation of the iomem_get_mapping() to the sysfs open callback
> so that it can be executed as needed when the binary sysfs object has
> been accessed.
>
> To do that, convert the "mapping" member of the struct bin_attribute
> from a pointer to the struct address_space into a function pointer with
> a signature that requires the same return type, and then updates the
> sysfs_kf_bin_open() to invoke provided function should the function
> pointer be valid.
>
> Thus, this change removes the need for the fs_initcalls to complete
> before any other sub-system that uses the iomem_get_mapping() would be
> able to invoke it safely without leading to a failure and an Oops
> related to an invalid iomem_get_mapping() access.
>
> Co-authored-by: Dan Williams <dan.j.williams@intel.com>

Go ahead and replace this with:

Signed-off-by: Dan Williams <dan.j.williams@intel.com>

Re: [PATCH 1/2] sysfs: Invoke iomem_get_mapping() from the sysfs open callback

[Christoph Hellwig]

On Fri, Jun 25, 2021 at 11:31:17PM +0000, Krzysztof Wilczy??ski wrote:
>  	if (battr->mapping)
> -		of->file->f_mapping = battr->mapping;
> +		of->file->f_mapping = battr->mapping();

I think get_mapping() is a better name now.  That being said this
whole programming model looks a little weird.

Also, does this patch imply the mapping field of the sysfs bin
attributes wasn't used before at all?

Re: [PATCH 1/2] sysfs: Invoke iomem_get_mapping() from the sysfs open callback

[Krzysztof Wilczy??ski]

Hi Christoph,

> >  	if (battr->mapping)
> > -		of->file->f_mapping = battr->mapping;
> > +		of->file->f_mapping = battr->mapping();
> 
> I think get_mapping() is a better name now.  That being said this
> whole programming model looks a little weird.

I would have to lean on Daniel and Dan here as they might have more
context than I do, especially since in the PCI world we are only
consumers of this new API.  The related patches are:

  commit 3234ac664a87 ("/dev/mem: Revoke mappings when a driver claims the region")
  commit 636b21b50152 ("PCI: Revoke mappings like devmem") 

> Also, does this patch imply the mapping field of the sysfs bin
> attributes wasn't used before at all?

No, everything worked as intended, thankfully.  Changes here are meant
to help us transition to use static sysfs objects when we add various
PCI-related attributes a particular device.  This in turn will allow us
to remove the need for late_initcall() in the drivers/pci/pci-sysfs.c,
and thus fix the race condition people noticed on some platforms when
sysfs objects are being added while PCI sub-system and devices are
initialised.

More details are captured in the following conversations:
  https://lore.kernel.org/linux-pci/20200716110423.xtfyb3n6tn5ixedh@pali/
  https://lore.kernel.org/linux-pci/20210527205845.GA1421476@bjorn-Precision-5520/
  https://lore.kernel.org/linux-pci/20210313215747.GA2394467@bjorn-Precision-5520/

Dan's original patch:
  https://lore.kernel.org/linux-pci/CAPcyv4i0y_4cMGEpNVShLUyUk3nyWH203Ry3S87BqnDJE0Rmxg@mail.gmail.com/

	Krzysztof

Re: [PATCH 1/2] sysfs: Invoke iomem_get_mapping() from the sysfs open callback

[Dan Williams]

On Mon, Jun 28, 2021 at 3:12 AM Christoph Hellwig <hch@infradead.org> wrote:
>
> On Fri, Jun 25, 2021 at 11:31:17PM +0000, Krzysztof Wilczy??ski wrote:
> >       if (battr->mapping)
> > -             of->file->f_mapping = battr->mapping;
> > +             of->file->f_mapping = battr->mapping();
>
> I think get_mapping() is a better name now.  That being said this
> whole programming model looks a little weird.

I think both those points are fair.

> Also, does this patch imply the mapping field of the sysfs bin
> attributes wasn't used before at all?

It defaulted to an address_space per file rather than a shared address
space across all files that map physical addresses as file offsets.

Re: [PATCH 1/2] sysfs: Invoke iomem_get_mapping() from the sysfs open callback

[Krzysztof Wilczyński]

Hi Dan,

On 21-06-28 10:36:13, Dan Williams wrote:
> On Mon, Jun 28, 2021 at 3:12 AM Christoph Hellwig <hch@infradead.org> wrote:
> >
> > On Fri, Jun 25, 2021 at 11:31:17PM +0000, Krzysztof Wilczy??ski wrote:
> > >       if (battr->mapping)
> > > -             of->file->f_mapping = battr->mapping;
> > > +             of->file->f_mapping = battr->mapping();
> >
> > I think get_mapping() is a better name now.  That being said this
> > whole programming model looks a little weird.
> 
> I think both those points are fair.

Anything for us to do?

> > Also, does this patch imply the mapping field of the sysfs bin
> > attributes wasn't used before at all?
> 
> It defaulted to an address_space per file rather than a shared address
> space across all files that map physical addresses as file offsets.

I will include this in the commit message for v3 of the patch series, if
you don't mind.  Also, would this shared address space be a potential
issue?  Security, functional, etc.

	Krzysztof

Re: [PATCH 1/2] sysfs: Invoke iomem_get_mapping() from the sysfs open callback

[Dan Williams]

On Mon, Jun 28, 2021 at 11:07 AM Krzysztof Wilczyński <kw@linux.com> wrote:
>
> Hi Dan,
>
> On 21-06-28 10:36:13, Dan Williams wrote:
> > On Mon, Jun 28, 2021 at 3:12 AM Christoph Hellwig <hch@infradead.org> wrote:
> > >
> > > On Fri, Jun 25, 2021 at 11:31:17PM +0000, Krzysztof Wilczy??ski wrote:
> > > >       if (battr->mapping)
> > > > -             of->file->f_mapping = battr->mapping;
> > > > +             of->file->f_mapping = battr->mapping();
> > >
> > > I think get_mapping() is a better name now.  That being said this
> > > whole programming model looks a little weird.
> >
> > I think both those points are fair.
>
> Anything for us to do?
>
> > > Also, does this patch imply the mapping field of the sysfs bin
> > > attributes wasn't used before at all?
> >
> > It defaulted to an address_space per file rather than a shared address
> > space across all files that map physical addresses as file offsets.
>
> I will include this in the commit message for v3 of the patch series, if
> you don't mind.  Also, would this shared address space be a potential
> issue?  Security, functional, etc.

The shared address_space arrangement is what allows for a "revoke"
mechanism for /dev/mem and pci-resource-sysfs mappings. Without a
shared address space there would need to be tracking for each 'inode'
instance to run revoke_iomem(). Note that this shared address_space
scheme is also deployed for block-device special files, see
blkdev_open(). It's done for similar reasons of allowing all
address_space operations to act on a common representation of the
single block-device.

[PATCH 2/2] PCI/sysfs: Pass iomem_get_mapping() as a function pointer

[Krzysztof Wilczyński]

The struct bin_attribute requires the "mapping" member to be a function
pointer with a signature requiring the return type to be a pointer to
the struct address_space.

Thus, convert every invocation of iomem_get_mapping() into a function
pointer assignment, therefore allowing for the iomem_get_mapping()
invocation to be deferred to when the sysfs open callback runs.

Co-authored-by: Dan Williams <dan.j.williams@intel.com>
Suggested-by: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Krzysztof Wilczyński <kw@linux.com>
---
 drivers/pci/pci-sysfs.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index beb8d1f4fafe..cff1c121eb08 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -965,7 +965,7 @@ void pci_create_legacy_files(struct pci_bus *b)
 	b->legacy_io->read = pci_read_legacy_io;
 	b->legacy_io->write = pci_write_legacy_io;
 	b->legacy_io->mmap = pci_mmap_legacy_io;
-	b->legacy_io->mapping = iomem_get_mapping();
+	b->legacy_io->mapping = iomem_get_mapping;
 	pci_adjust_legacy_attr(b, pci_mmap_io);
 	error = device_create_bin_file(&b->dev, b->legacy_io);
 	if (error)
@@ -978,7 +978,7 @@ void pci_create_legacy_files(struct pci_bus *b)
 	b->legacy_mem->size = 1024*1024;
 	b->legacy_mem->attr.mode = 0600;
 	b->legacy_mem->mmap = pci_mmap_legacy_mem;
-	b->legacy_io->mapping = iomem_get_mapping();
+	b->legacy_io->mapping = iomem_get_mapping;
 	pci_adjust_legacy_attr(b, pci_mmap_mem);
 	error = device_create_bin_file(&b->dev, b->legacy_mem);
 	if (error)
@@ -1195,7 +1195,7 @@ static int pci_create_attr(struct pci_dev *pdev, int num, int write_combine)
 		}
 	}
 	if (res_attr->mmap)
-		res_attr->mapping = iomem_get_mapping();
+		res_attr->mapping = iomem_get_mapping;
 	res_attr->attr.name = res_attr_name;
 	res_attr->attr.mode = 0600;
 	res_attr->size = pci_resource_len(pdev, num);
-- 
2.32.0

Re: [PATCH 2/2] PCI/sysfs: Pass iomem_get_mapping() as a function pointer

[Dan Williams]

On Fri, Jun 25, 2021 at 4:31 PM Krzysztof Wilczyński <kw@linux.com> wrote:
>
> The struct bin_attribute requires the "mapping" member to be a function
> pointer with a signature requiring the return type to be a pointer to
> the struct address_space.
>
> Thus, convert every invocation of iomem_get_mapping() into a function
> pointer assignment, therefore allowing for the iomem_get_mapping()
> invocation to be deferred to when the sysfs open callback runs.
>

Looks good. Since I did not write any of the below go ahead and change
the "Co-authored-by"  to:

Reviewed-by: Dan Williams <dan.j.williams@intel.com>

Thanks for picking this up and carrying it through.

Re: [PATCH 2/2] PCI/sysfs: Pass iomem_get_mapping() as a function pointer

[Krzysztof Wilczyński]

Hi Dan,

[...]
> Looks good. Since I did not write any of the below go ahead and change
> the "Co-authored-by"  to:
> 
> Reviewed-by: Dan Williams <dan.j.williams@intel.com>

Thank you!

> Thanks for picking this up and carrying it through.

Of course, any time!  Also, thank you for suggesting how to solve the
problem we've had and for sending the initial patch!

	Krzysztof

Re: [PATCH 2/2] PCI/sysfs: Pass iomem_get_mapping() as a function pointer

[Christoph Hellwig]

Doesn't this need to be merged into the previous patch to prevent
a compile failure after just the previous patch is applied?

Re: [PATCH 2/2] PCI/sysfs: Pass iomem_get_mapping() as a function pointer

[Krzysztof Wilczy??ski]

Hi Christoph,

> Doesn't this need to be merged into the previous patch to prevent
> a compile failure after just the previous patch is applied?

Yes, it does.  I kept it separate for the sake of review, since we have
sysfs and PCI involved.  I wasn't sure if Bjorn would prefer to have
this done as separate patches or not, to be honest.

Bjorn said that he can squash this when applying, thus I left it as-is
for now - which means that the entire series has to be applied for
everything to build cleanly.

I will send v3 that merges first two patches.  Sorry for troubles!

	Krzysztof

Re: [PATCH 2/2] PCI/sysfs: Pass iomem_get_mapping() as a function pointer

[Greg Kroah-Hartman]

On Mon, Jun 28, 2021 at 12:24:53PM +0200, Krzysztof Wilczy??ski wrote:
> Hi Christoph,
> 
> > Doesn't this need to be merged into the previous patch to prevent
> > a compile failure after just the previous patch is applied?
> 
> Yes, it does.  I kept it separate for the sake of review, since we have
> sysfs and PCI involved.  I wasn't sure if Bjorn would prefer to have
> this done as separate patches or not, to be honest.
> 
> Bjorn said that he can squash this when applying, thus I left it as-is
> for now - which means that the entire series has to be applied for
> everything to build cleanly.
> 
> I will send v3 that merges first two patches.  Sorry for troubles!

This all needs to wait until after 5.14-rc1 is out anyway, so no rush.

greg k-h