mbox series

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

Message ID 20210729233235.1508920-1-kw@linux.com (mailing list archive)
Headers show
Series Allow deferred execution of iomem_get_mapping() | expand

Message

Krzysztof Wilczyński July 29, 2021, 11:32 p.m. UTC
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, as per:
  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

---
Changes in v3:
  Squashed first two patches from v2 into a single patch to resolve
  compile time issues.
  Carried over Dan Williams' review from v2 as there are no code or
  functional changes.

Changes in v2:
  Added "Signed-off-by" and "Reviewed-by" from Dan Williams as per his
  feedback.
  Added third patch that renames struct bin_attribute member "mapping"
  to "f_mapping" to keep the naming consistent with struct file.  N.B.
  this patch can definitely be dropped as the rename is not required for
  anything else to work.

Krzysztof Wilczyński (2):
  sysfs: Invoke iomem_get_mapping() from the sysfs open callback
  sysfs: Rename struct bin_attribute member to f_mapping

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

Comments

Greg KH July 30, 2021, 5:06 a.m. UTC | #1
On Thu, Jul 29, 2021 at 11:32:33PM +0000, Krzysztof Wilczyński wrote:
> 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, as per:
>   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
> 

No objection from me on these, Bjorn, mind if I take them through my
driver core tree?

thanks,

greg k-h
Bjorn Helgaas July 30, 2021, 7:50 p.m. UTC | #2
On Fri, Jul 30, 2021 at 07:06:26AM +0200, Greg Kroah-Hartman wrote:
> On Thu, Jul 29, 2021 at 11:32:33PM +0000, Krzysztof Wilczyński wrote:
> > 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, as per:
> >   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
> > 
> 
> No objection from me on these, Bjorn, mind if I take them through my
> driver core tree?

That'd be great!

Is your tree immutable?  I'm hoping we can take advantage of this to
get rid of pci_sysfs_init(), which will fix a race that people seem to
be hitting frequently now [1].  I think Krzysztof is getting pretty
close.

[1] https://lore.kernel.org/r/m3eebg9puj.fsf@t19.piap.pl
Greg KH Aug. 5, 2021, 12:43 p.m. UTC | #3
On Fri, Jul 30, 2021 at 02:50:13PM -0500, Bjorn Helgaas wrote:
> On Fri, Jul 30, 2021 at 07:06:26AM +0200, Greg Kroah-Hartman wrote:
> > On Thu, Jul 29, 2021 at 11:32:33PM +0000, Krzysztof Wilczyński wrote:
> > > 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, as per:
> > >   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
> > > 
> > 
> > No objection from me on these, Bjorn, mind if I take them through my
> > driver core tree?
> 
> That'd be great!
> 
> Is your tree immutable?  I'm hoping we can take advantage of this to
> get rid of pci_sysfs_init(), which will fix a race that people seem to
> be hitting frequently now [1].  I think Krzysztof is getting pretty
> close.
> 
> [1] https://lore.kernel.org/r/m3eebg9puj.fsf@t19.piap.pl

Yes, my tree is immutable.  Let me create a branch just for this that
you can pull from after it passes 0-day...

thanks,

greg k-h
Greg KH Aug. 6, 2021, 11:05 a.m. UTC | #4
On Thu, Aug 05, 2021 at 02:43:46PM +0200, Greg Kroah-Hartman wrote:
> On Fri, Jul 30, 2021 at 02:50:13PM -0500, Bjorn Helgaas wrote:
> > On Fri, Jul 30, 2021 at 07:06:26AM +0200, Greg Kroah-Hartman wrote:
> > > On Thu, Jul 29, 2021 at 11:32:33PM +0000, Krzysztof Wilczyński wrote:
> > > > 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, as per:
> > > >   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
> > > > 
> > > 
> > > No objection from me on these, Bjorn, mind if I take them through my
> > > driver core tree?
> > 
> > That'd be great!
> > 
> > Is your tree immutable?  I'm hoping we can take advantage of this to
> > get rid of pci_sysfs_init(), which will fix a race that people seem to
> > be hitting frequently now [1].  I think Krzysztof is getting pretty
> > close.
> > 
> > [1] https://lore.kernel.org/r/m3eebg9puj.fsf@t19.piap.pl
> 
> Yes, my tree is immutable.  Let me create a branch just for this that
> you can pull from after it passes 0-day...

Ok, here you go, a signed tag for you to pull from:


The following changes since commit ff1176468d368232b684f75e82563369208bc371:

  Linux 5.14-rc3 (2021-07-25 15:35:14 -0700)

are available in the Git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core.git tags/sysfs_defferred_iomem_get_mapping-5.15

for you to fetch changes up to f06aff924f975881a6abf91d2af0078fc8cd37bf:

  sysfs: Rename struct bin_attribute member to f_mapping (2021-08-05 14:47:31 +0200)

----------------------------------------------------------------
sysfs: Allow deferred execution of iomem_get_mapping()

Tag for toerh trees/branches to pull from in order to have a stable base
to build off of for the "Allow deferred execution of
iomem_get_mapping()" set of sysfs changes

Link: https://lore.kernel.org/r/20210729233235.1508920-1-kw@linux.com
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

----------------------------------------------------------------
Krzysztof Wilczyński (2):
      sysfs: Invoke iomem_get_mapping() from the sysfs open callback
      sysfs: Rename struct bin_attribute member to f_mapping

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