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

[PATCH v2 0/3] 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, 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 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 (3):
  sysfs: Invoke iomem_get_mapping() from the sysfs open callback
  PCI/sysfs: Pass iomem_get_mapping() as a function pointer
  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(-)

-- 
2.32.0

[PATCH v2 1/3] 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.

Suggested-by: Dan Williams <dan.j.williams@intel.com>
Signed-off-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

[PATCH v2 2/3] 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.

Suggested-by: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Krzysztof Wilczyński <kw@linux.com>
Reviewed-by: Dan Williams <dan.j.williams@intel.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

[PATCH v2 3/3] sysfs: Rename struct bin_attribute member to f_mapping

[Krzysztof Wilczyński]

There are two users of iomem_get_mapping(), the struct file and struct
bin_attribute.  The former has a member called "f_mapping" and the
latter has a member called "mapping", and both are poniters to struct
address_space.

Rename struct bin_attribute member to "f_mapping" to keep both meaning
and the usage consistent with other users of iomem_get_mapping().

Signed-off-by: Krzysztof Wilczyński <kw@linux.com>
---
 drivers/pci/pci-sysfs.c | 6 +++---
 fs/sysfs/file.c         | 4 ++--
 include/linux/sysfs.h   | 2 +-
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index cff1c121eb08..26cef98d8352 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->f_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->f_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->f_mapping = iomem_get_mapping;
 	res_attr->attr.name = res_attr_name;
 	res_attr->attr.mode = 0600;
 	res_attr->size = pci_resource_len(pdev, num);
diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
index a3ee4c32a264..d019d6ac6ad0 100644
--- a/fs/sysfs/file.c
+++ b/fs/sysfs/file.c
@@ -174,8 +174,8 @@ 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();
+	if (battr->f_mapping)
+		of->file->f_mapping = battr->f_mapping();
 
 	return 0;
 }
diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
index fbb7c7df545c..fa5de9b1fa4a 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)(void);
+	struct address_space *(*f_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 v2 1/3] sysfs: Invoke iomem_get_mapping() from the sysfs open callback

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 6136 bytes --]

Hi "Krzysztof,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on pci/next]
[also build test ERROR on driver-core/driver-core-testing linus/master v5.13-rc7 next-20210625]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Krzysztof-Wilczy-ski/Allow-deferred-execution-of-iomem_get_mapping/20210626-205836
base:   https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git next
config: i386-allyesconfig (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce (this is a W=1 build):
        # https://github.com/0day-ci/linux/commit/e2a3b2d0e981d30bc58d044387dc17e55ab6ee03
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Krzysztof-Wilczy-ski/Allow-deferred-execution-of-iomem_get_mapping/20210626-205836
        git checkout e2a3b2d0e981d30bc58d044387dc17e55ab6ee03
        # save the attached .config to linux build tree
        make W=1 ARCH=i386 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

Note: the linux-review/Krzysztof-Wilczy-ski/Allow-deferred-execution-of-iomem_get_mapping/20210626-205836 HEAD eef095b16d6c52c82c68e26f94558e39dc49f4ae builds fine.
      It only hurts bisectibility.

All errors (new ones prefixed by >>):

   drivers/pci/pci-sysfs.c: In function 'pci_create_attr':
>> drivers/pci/pci-sysfs.c:1198:21: error: assignment to 'struct address_space * (*)(void)' from incompatible pointer type 'struct address_space *' [-Werror=incompatible-pointer-types]
    1198 |   res_attr->mapping = iomem_get_mapping();
         |                     ^
   cc1: some warnings being treated as errors


vim +1198 drivers/pci/pci-sysfs.c

45aec1ae72fc59 venkatesh.pallipadi@intel.com 2008-03-18  1165  
45aec1ae72fc59 venkatesh.pallipadi@intel.com 2008-03-18  1166  static int pci_create_attr(struct pci_dev *pdev, int num, int write_combine)
45aec1ae72fc59 venkatesh.pallipadi@intel.com 2008-03-18  1167  {
45aec1ae72fc59 venkatesh.pallipadi@intel.com 2008-03-18  1168  	/* allocate attribute structure, piggyback attribute name */
45aec1ae72fc59 venkatesh.pallipadi@intel.com 2008-03-18  1169  	int name_len = write_combine ? 13 : 10;
45aec1ae72fc59 venkatesh.pallipadi@intel.com 2008-03-18  1170  	struct bin_attribute *res_attr;
bd5174dfb6f171 Bjorn Helgaas                 2016-03-10  1171  	char *res_attr_name;
45aec1ae72fc59 venkatesh.pallipadi@intel.com 2008-03-18  1172  	int retval;
45aec1ae72fc59 venkatesh.pallipadi@intel.com 2008-03-18  1173  
45aec1ae72fc59 venkatesh.pallipadi@intel.com 2008-03-18  1174  	res_attr = kzalloc(sizeof(*res_attr) + name_len, GFP_ATOMIC);
bd5174dfb6f171 Bjorn Helgaas                 2016-03-10  1175  	if (!res_attr)
bd5174dfb6f171 Bjorn Helgaas                 2016-03-10  1176  		return -ENOMEM;
bd5174dfb6f171 Bjorn Helgaas                 2016-03-10  1177  
bd5174dfb6f171 Bjorn Helgaas                 2016-03-10  1178  	res_attr_name = (char *)(res_attr + 1);
45aec1ae72fc59 venkatesh.pallipadi@intel.com 2008-03-18  1179  
a07e4156a2ee63 Eric W. Biederman             2010-02-11  1180  	sysfs_bin_attr_init(res_attr);
45aec1ae72fc59 venkatesh.pallipadi@intel.com 2008-03-18  1181  	if (write_combine) {
45aec1ae72fc59 venkatesh.pallipadi@intel.com 2008-03-18  1182  		pdev->res_attr_wc[num] = res_attr;
45aec1ae72fc59 venkatesh.pallipadi@intel.com 2008-03-18  1183  		sprintf(res_attr_name, "resource%d_wc", num);
45aec1ae72fc59 venkatesh.pallipadi@intel.com 2008-03-18  1184  		res_attr->mmap = pci_mmap_resource_wc;
45aec1ae72fc59 venkatesh.pallipadi@intel.com 2008-03-18  1185  	} else {
45aec1ae72fc59 venkatesh.pallipadi@intel.com 2008-03-18  1186  		pdev->res_attr[num] = res_attr;
45aec1ae72fc59 venkatesh.pallipadi@intel.com 2008-03-18  1187  		sprintf(res_attr_name, "resource%d", num);
8633328be24267 Alex Williamson               2010-07-19  1188  		if (pci_resource_flags(pdev, num) & IORESOURCE_IO) {
8633328be24267 Alex Williamson               2010-07-19  1189  			res_attr->read = pci_read_resource_io;
8633328be24267 Alex Williamson               2010-07-19  1190  			res_attr->write = pci_write_resource_io;
e854d8b2a82ef7 David Woodhouse               2017-04-12  1191  			if (arch_can_pci_mmap_io())
e854d8b2a82ef7 David Woodhouse               2017-04-12  1192  				res_attr->mmap = pci_mmap_resource_uc;
e854d8b2a82ef7 David Woodhouse               2017-04-12  1193  		} else {
e854d8b2a82ef7 David Woodhouse               2017-04-12  1194  			res_attr->mmap = pci_mmap_resource_uc;
e854d8b2a82ef7 David Woodhouse               2017-04-12  1195  		}
8633328be24267 Alex Williamson               2010-07-19  1196  	}
636b21b50152d4 Daniel Vetter                 2021-02-04  1197  	if (res_attr->mmap)
636b21b50152d4 Daniel Vetter                 2021-02-04 @1198  		res_attr->mapping = iomem_get_mapping();
45aec1ae72fc59 venkatesh.pallipadi@intel.com 2008-03-18  1199  	res_attr->attr.name = res_attr_name;
e2154044dd4168 Kelsey Skunberg               2019-08-13  1200  	res_attr->attr.mode = 0600;
45aec1ae72fc59 venkatesh.pallipadi@intel.com 2008-03-18  1201  	res_attr->size = pci_resource_len(pdev, num);
dca40b186b757c David Woodhouse               2017-04-12  1202  	res_attr->private = (void *)(unsigned long)num;
45aec1ae72fc59 venkatesh.pallipadi@intel.com 2008-03-18  1203  	retval = sysfs_create_bin_file(&pdev->dev.kobj, res_attr);
b562ec8f74e4ed Bjorn Helgaas                 2016-03-10  1204  	if (retval)
b562ec8f74e4ed Bjorn Helgaas                 2016-03-10  1205  		kfree(res_attr);
45aec1ae72fc59 venkatesh.pallipadi@intel.com 2008-03-18  1206  
45aec1ae72fc59 venkatesh.pallipadi@intel.com 2008-03-18  1207  	return retval;
b19441af185559 Greg Kroah-Hartman            2006-08-28  1208  }
b19441af185559 Greg Kroah-Hartman            2006-08-28  1209  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 65239 bytes --]