Re: [PATCH v5 02/10] PCI: Deprecate iomap-table functions
Hello,
On Thu, 2024-04-04 at 09:29 +0300, Dan Carpenter wrote:
> Hi Philipp,
>
> kernel test robot noticed the following build warnings:
>
> https://git-scm.com/docs/git-format-patch#_base_tree_information]
>
> url:
> https://github.com/intel-lab-lkp/linux/commits/Philipp-Stanner/PCI-Add-new-set-of-devres-functions/20240403-160932
> base:
> https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git next
> patch link:
> https://lore.kernel.org/r/20240403080712.13986-5-pstanner%40redhat.com
> patch subject: [PATCH v5 02/10] PCI: Deprecate iomap-table functions
> config: i386-randconfig-141-20240404
> (https://download.01.org/0day-ci/archive/20240404/202404040920.QIxhNe
> mu-...@intel.com/config)
> compiler: gcc-7 (Ubuntu 7.5.0-6ubuntu2) 7.5.0
>
> If you fix the issue in a separate patch/commit (i.e. not just a new
> version of
> the same patch/commit), kindly add following tags
> > Reported-by: kernel test robot
> > Reported-by: Dan Carpenter
> > Closes:
> > https://lore.kernel.org/r/202404040920.qixhnemu-...@intel.com/
>
> smatch warnings:
> drivers/pci/devres.c:897 pcim_iomap_regions_request_all() error: we
> previously assumed 'legacy_iomap_table' could be null (see line 894)
>
> vim +/legacy_iomap_table +897 drivers/pci/devres.c
>
> acc2364fe66106 Philipp Stanner 2024-01-31 865 int
> pcim_iomap_regions_request_all(struct pci_dev *pdev, int mask,
> acc2364fe66106 Philipp Stanner 2024-01-31
> 866 const char *name)
> acc2364fe66106 Philipp Stanner 2024-01-31 867 {
> 34e90b966504f3 Philipp Stanner 2024-04-03 868 short bar;
> 34e90b966504f3 Philipp Stanner 2024-04-03 869 int ret;
> 34e90b966504f3 Philipp Stanner 2024-04-03 870 void __iomem
> **legacy_iomap_table;
> acc2364fe66106 Philipp Stanner 2024-01-31 871
> 34e90b966504f3 Philipp Stanner 2024-04-03 872 ret =
> pcim_request_all_regions(pdev, name);
> 34e90b966504f3 Philipp Stanner 2024-04-03 873 if (ret != 0)
> 34e90b966504f3 Philipp Stanner 2024-04-03
> 874 return ret;
> acc2364fe66106 Philipp Stanner 2024-01-31 875
> 34e90b966504f3 Philipp Stanner 2024-04-03 876 for (bar = 0;
> bar < PCI_STD_NUM_BARS; bar++) {
> 34e90b966504f3 Philipp Stanner 2024-04-03 877 if
> (!mask_contains_bar(mask, bar))
> 34e90b966504f3 Philipp Stanner 2024-04-03
> 878 continue;
> 34e90b966504f3 Philipp Stanner 2024-04-03 879 if
> (!pcim_iomap(pdev, bar, 0))
> 34e90b966504f3 Philipp Stanner 2024-04-03
> 880 goto err;
> 34e90b966504f3 Philipp Stanner 2024-04-03 881 }
> 34e90b966504f3 Philipp Stanner 2024-04-03 882
> 34e90b966504f3 Philipp Stanner 2024-04-03 883 return 0;
> 34e90b966504f3 Philipp Stanner 2024-04-03 884
> 34e90b966504f3 Philipp Stanner 2024-04-03 885 err:
> 34e90b966504f3 Philipp Stanner 2024-04-03 886 /*
> 34e90b966504f3 Philipp Stanner 2024-04-03 887 * Here it
> gets tricky: pcim_iomap() above has most likely
> 34e90b966504f3 Philipp Stanner 2024-04-03 888 * failed
> because it got an OOM when trying to create the
> 34e90b966504f3 Philipp Stanner 2024-04-03 889 * legacy-
> table.
> 34e90b966504f3 Philipp Stanner 2024-04-03 890 * We check
> here if that has happened. If not, pcim_iomap()
> 34e90b966504f3 Philipp Stanner 2024-04-03 891 * must have
> failed because of EINVAL.
> 34e90b966504f3 Philipp Stanner 2024-04-03 892 */
> 34e90b966504f3 Philipp Stanner 2024-04-03
> 893 legacy_iomap_table = (void __iomem
> **)pcim_iomap_table(pdev);
> 34e90b966504f3 Philipp Stanner 2024-04-03 @894 ret =
> legacy_iomap_table ? -EINVAL : -ENOMEM;
>
> ^^
> Check for NULL
>
> 34e90b966504f3 Philipp Stanner 2024-04-03 895
> 34e90b966504f3 Philipp Stanner 2024-04-03 896 while (--bar
> >= 0)
> 34e90b966504f3 Philipp Stanner 2024-04-03
> @897 pcim_iounmap(pdev, legacy_iomap_table[bar]);
>
> ^^
> Unchecked dereference
I think this is fine because `bar` can only be larger 0 if at least one
mapping has been created, thus, when it was possible to create
legacy_iomap_table, which is then valid.
So the second for-loop only executes when it's not NULL.
I guess we could silence the warning by doing
ret = bar > 0 ? -EINVAL : -ENOMEM;
Would even save one line of code
P.
>
> 34e90b966504f3 Philipp Stanner 2024-04-03 898
> 34e90b966504f3 Philipp Stanner 2024-04-03
> 899 pcim_release_all_regions(pdev);
> 34e90b966504f3 Philipp Stanner 2024-04-03 900
> 34e90b966504f3 Philipp Stanner 2024-04-03 901 return ret;
> acc2364fe66106 Philipp Stanner 2024-01-31 902 }
>
Re: [PATCH v5 02/10] PCI: Deprecate iomap-table functions
Hi Philipp,
kernel test robot noticed the following build warnings:
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url:
https://github.com/intel-lab-lkp/linux/commits/Philipp-Stanner/PCI-Add-new-set-of-devres-functions/20240403-160932
base: https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git next
patch link:
https://lore.kernel.org/r/20240403080712.13986-5-pstanner%40redhat.com
patch subject: [PATCH v5 02/10] PCI: Deprecate iomap-table functions
config: i386-randconfig-141-20240404
(https://download.01.org/0day-ci/archive/20240404/202404040920.qixhnemu-...@intel.com/config)
compiler: gcc-7 (Ubuntu 7.5.0-6ubuntu2) 7.5.0
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot
| Reported-by: Dan Carpenter
| Closes: https://lore.kernel.org/r/202404040920.qixhnemu-...@intel.com/
smatch warnings:
drivers/pci/devres.c:897 pcim_iomap_regions_request_all() error: we previously
assumed 'legacy_iomap_table' could be null (see line 894)
vim +/legacy_iomap_table +897 drivers/pci/devres.c
acc2364fe66106 Philipp Stanner 2024-01-31 865 int
pcim_iomap_regions_request_all(struct pci_dev *pdev, int mask,
acc2364fe66106 Philipp Stanner 2024-01-31 866
const char *name)
acc2364fe66106 Philipp Stanner 2024-01-31 867 {
34e90b966504f3 Philipp Stanner 2024-04-03 868 short bar;
34e90b966504f3 Philipp Stanner 2024-04-03 869 int ret;
34e90b966504f3 Philipp Stanner 2024-04-03 870 void __iomem
**legacy_iomap_table;
acc2364fe66106 Philipp Stanner 2024-01-31 871
34e90b966504f3 Philipp Stanner 2024-04-03 872 ret =
pcim_request_all_regions(pdev, name);
34e90b966504f3 Philipp Stanner 2024-04-03 873 if (ret != 0)
34e90b966504f3 Philipp Stanner 2024-04-03 874 return ret;
acc2364fe66106 Philipp Stanner 2024-01-31 875
34e90b966504f3 Philipp Stanner 2024-04-03 876 for (bar = 0; bar <
PCI_STD_NUM_BARS; bar++) {
34e90b966504f3 Philipp Stanner 2024-04-03 877 if
(!mask_contains_bar(mask, bar))
34e90b966504f3 Philipp Stanner 2024-04-03 878
continue;
34e90b966504f3 Philipp Stanner 2024-04-03 879 if
(!pcim_iomap(pdev, bar, 0))
34e90b966504f3 Philipp Stanner 2024-04-03 880 goto
err;
34e90b966504f3 Philipp Stanner 2024-04-03 881 }
34e90b966504f3 Philipp Stanner 2024-04-03 882
34e90b966504f3 Philipp Stanner 2024-04-03 883 return 0;
34e90b966504f3 Philipp Stanner 2024-04-03 884
34e90b966504f3 Philipp Stanner 2024-04-03 885 err:
34e90b966504f3 Philipp Stanner 2024-04-03 886 /*
34e90b966504f3 Philipp Stanner 2024-04-03 887 * Here it gets tricky:
pcim_iomap() above has most likely
34e90b966504f3 Philipp Stanner 2024-04-03 888 * failed because it
got an OOM when trying to create the
34e90b966504f3 Philipp Stanner 2024-04-03 889 * legacy-table.
34e90b966504f3 Philipp Stanner 2024-04-03 890 * We check here if
that has happened. If not, pcim_iomap()
34e90b966504f3 Philipp Stanner 2024-04-03 891 * must have failed
because of EINVAL.
34e90b966504f3 Philipp Stanner 2024-04-03 892 */
34e90b966504f3 Philipp Stanner 2024-04-03 893 legacy_iomap_table =
(void __iomem **)pcim_iomap_table(pdev);
34e90b966504f3 Philipp Stanner 2024-04-03 @894 ret =
legacy_iomap_table ? -EINVAL : -ENOMEM;
^^
Check for NULL
34e90b966504f3 Philipp Stanner 2024-04-03 895
34e90b966504f3 Philipp Stanner 2024-04-03 896 while (--bar >= 0)
34e90b966504f3 Philipp Stanner 2024-04-03 @897
pcim_iounmap(pdev, legacy_iomap_table[bar]);
^^
Unchecked dereference
34e90b966504f3 Philipp Stanner 2024-04-03 898
34e90b966504f3 Philipp Stanner 2024-04-03 899
pcim_release_all_regions(pdev);
34e90b966504f3 Philipp Stanner 2024-04-03 900
34e90b966504f3 Philipp Stanner 2024-04-03 901 return ret;
acc2364fe66106 Philipp Stanner 2024-01-31 902 }
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
[PATCH v5 02/10] PCI: Deprecate iomap-table functions
The old plural devres functions tie the PCI devres API to the
iomap-table mechanism which unfortunately is not extensible.
As the plural functions are almost never used with more than one bit set
in their bit-mask, deprecating them and encouraging users to use the new
singular functions instead is reasonable.
Furthermore, to make the implementation more consistent and extensible,
the plural functions should use the singular functions.
Add new wrapper to request / release all BARs.
Make the plural functions call into the singular functions.
Mark the plural functions as deprecated.
Remove as much of the iomap-table-mechanism as possible.
Add comments describing the path towards a cleaned-up API.
Signed-off-by: Philipp Stanner
---
drivers/pci/devres.c | 374 +--
drivers/pci/pci.c| 20 +++
drivers/pci/pci.h| 5 +
include/linux/pci.h | 2 +
4 files changed, 318 insertions(+), 83 deletions(-)
diff --git a/drivers/pci/devres.c b/drivers/pci/devres.c
index 6a653e1a1acb..bc31e3a8cc04 100644
--- a/drivers/pci/devres.c
+++ b/drivers/pci/devres.c
@@ -4,15 +4,43 @@
#include "pci.h"
/*
- * PCI iomap devres
+ * On the state of PCI's devres implementation:
+ *
+ * The older devres API for PCI has two significant problems:
+ *
+ * 1. It is very strongly tied to the statically allocated mapping table in
+ *struct pcim_iomap_devres below. This is mostly solved in the sense of the
+ *pcim_ functions in this file providing things like ranged mapping by
+ *bypassing this table, wheras the functions that were present in the old
+ *API still enter the mapping addresses into the table for users of the old
+ *API.
+ * 2. The region-request-functions in pci.c do become managed IF the device has
+ *been enabled with pcim_enable_device() instead of pci_enable_device().
+ *This resulted in the API becoming inconsistent: Some functions have an
+ *obviously managed counter-part (e.g., pci_iomap() <-> pcim_iomap()),
+ *whereas some don't and are never managed, while others don't and are
+ *_sometimes_ managed (e.g. pci_request_region()).
+ *Consequently, in the new API, region requests performed by the pcim_
+ *functions are automatically cleaned up through the devres callback
+ *pcim_addr_resource_release(), while requests performed by
+ *pcim_enable_device() + pci_*region*() are automatically cleaned up
+ *through the for-loop in pcim_release().
+ *
+ * TODO 1:
+ * Remove the legacy table entirely once all calls to pcim_iomap_table() in
+ * the kernel have been removed.
+ *
+ * TODO 2:
+ * Port everyone calling pcim_enable_device() + pci_*region*() to using the
+ * pcim_ functions. Then, remove all devres functionality from pci_*region*()
+ * functions and remove the associated cleanups described above in point #2.
*/
-#define PCIM_IOMAP_MAX PCI_STD_NUM_BARS
/*
* Legacy struct storing addresses to whole mapped BARs.
*/
struct pcim_iomap_devres {
- void __iomem *table[PCIM_IOMAP_MAX];
+ void __iomem *table[PCI_STD_NUM_BARS];
};
enum pcim_addr_devres_type {
@@ -373,6 +401,16 @@ static void pcim_release(struct device *gendev, void *res)
struct pci_devres *this = res;
int i;
+ /*
+* This is legacy code.
+* All regions requested by a pcim_ function do get released through
+* pcim_addr_resource_release(). Thanks to the hybrid nature of the pci_
+* region-request functions, this for-loop has to release the regions
+* if they have been requested by such a function.
+*
+* TODO: Remove this once all users of pcim_enable_device() PLUS
+* pci-region-request-functions have been ported to pcim_ functions.
+*/
for (i = 0; i < DEVICE_COUNT_RESOURCE; i++)
if (this->region_mask & (1 << i))
pci_release_region(dev, i);
@@ -459,19 +497,21 @@ EXPORT_SYMBOL(pcim_pin_device);
static void pcim_iomap_release(struct device *gendev, void *res)
{
- struct pci_dev *dev = to_pci_dev(gendev);
- struct pcim_iomap_devres *this = res;
- int i;
-
- for (i = 0; i < PCIM_IOMAP_MAX; i++)
- if (this->table[i])
- pci_iounmap(dev, this->table[i]);
+ /*
+* Do nothing. This is legacy code.
+*
+* Cleanup of the mappings is now done directly through the callbacks
+* registered when creating them.
+*/
}
/**
- * pcim_iomap_table - access iomap allocation table
+ * pcim_iomap_table - access iomap allocation table (DEPRECATED)
* @pdev: PCI device to access iomap table for
*
+ * Returns:
+ * Const pointer to array of __iomem pointers on success NULL on failure.
+ *
* Access iomap allocation table for @dev. If iomap table doesn't
* exist and @pdev is managed, it will be allocated. All iomaps
* recorded in the iomap table are automatically unmapped on driver
@@ -480,6
