| Message ID | 20210713102436.304693-1-kw@linux.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | PCI: Refactor pci_ioremap_bar() and pci_ioremap_wc_bar() | expand |
On Tue, Jul 13, 2021 at 10:24:36AM +0000, Krzysztof Wilczyński wrote: > Currently, functions pci_ioremap_bar() and pci_ioremap_wc_bar() share > similar implementation details as both functions were almost identical > in the past, especially when the latter was initially introduced in the > commit c43996f4001d ("PCI: Add pci_ioremap_wc_bar()") as somewhat exact > copy of the function pci_ioremap_bar(). > > However, function pci_ioremap_bar() received several updates that were > never introduced to the function pci_ioremap_wc_bar(). > > Thus, to align implementation of both functions and reduce the need to > duplicate code between them, introduce a new internal function called > __pci_ioremap_resource() as a helper with a shared codebase intended to > be called from functions pci_ioremap_bar() and pci_ioremap_wc_bar(). > > The __pci_ioremap_resource() function will therefore include a check > for the IORESOURCE_UNSET flag that has previously been added to the > function pci_ioremap_bar() in the commit 646c0282df04 ("PCI: Fail > pci_ioremap_bar() on unassigned resources") and otherwise has been > missing from function pci_ioremap_wc_bar(). > > Additionally, function __pci_ioremap_resource() will retire the usage of > the WARN_ON() macro and replace it with pci_err() to show information > such as the driver name, the BAR number and resource details in case of > a failure, instead of printing a complete backtrace. The WARN_ON() has > already been replaced with pci_warn() in the commit 1f7bf3bfb5d6 ("PCI: > Show driver, BAR#, and resource on pci_ioremap_bar() failure") which > sadly didn't include an update to the function pci_ioremap_wc_bar() at > that time. > > Finally, a direct use of functions ioremap() and ioremap_wc() in the > function __pci_ioremap_resource() will be replaced with calls to the > pci_iomap_range() and pci_iomap_wc_range() functions respectively. > > Signed-off-by: Krzysztof Wilczyński <kw@linux.com> Nice cleanup, thanks! Applied to pci/resource for v5.15. I reverted to using plain ioremap() since pci_iomap_range() doesn't seem to add anything except overhead. > --- > drivers/pci/pci.c | 26 ++++++++++++++------------ > 1 file changed, 14 insertions(+), 12 deletions(-) > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > index e3bb0d073352..4bae55f0700b 100644 > --- a/drivers/pci/pci.c > +++ b/drivers/pci/pci.c > @@ -206,7 +206,8 @@ int pci_status_get_and_clear_errors(struct pci_dev *pdev) > EXPORT_SYMBOL_GPL(pci_status_get_and_clear_errors); > > #ifdef CONFIG_HAS_IOMEM > -void __iomem *pci_ioremap_bar(struct pci_dev *pdev, int bar) > +static void __iomem *__pci_ioremap_resource(struct pci_dev *pdev, int bar, > + bool write_combine) > { > struct resource *res = &pdev->resource[bar]; > > @@ -214,24 +215,25 @@ void __iomem *pci_ioremap_bar(struct pci_dev *pdev, int bar) > * Make sure the BAR is actually a memory resource, not an IO resource > */ > if (res->flags & IORESOURCE_UNSET || !(res->flags & IORESOURCE_MEM)) { > - pci_warn(pdev, "can't ioremap BAR %d: %pR\n", bar, res); > + pci_err(pdev, "can't ioremap BAR %d: %pR\n", bar, res); > return NULL; > } > - return ioremap(res->start, resource_size(res)); > + > + if (write_combine) > + return pci_iomap_wc_range(pdev, bar, 0, 0); > + > + return pci_iomap_range(pdev, bar, 0, 0); > +} > + > +void __iomem *pci_ioremap_bar(struct pci_dev *pdev, int bar) > +{ > + return __pci_ioremap_resource(pdev, bar, false); > } > EXPORT_SYMBOL_GPL(pci_ioremap_bar); > > void __iomem *pci_ioremap_wc_bar(struct pci_dev *pdev, int bar) > { > - /* > - * Make sure the BAR is actually a memory resource, not an IO resource > - */ > - if (!(pci_resource_flags(pdev, bar) & IORESOURCE_MEM)) { > - WARN_ON(1); > - return NULL; > - } > - return ioremap_wc(pci_resource_start(pdev, bar), > - pci_resource_len(pdev, bar)); > + return __pci_ioremap_resource(pdev, bar, true); > } > EXPORT_SYMBOL_GPL(pci_ioremap_wc_bar); > #endif > -- > 2.32.0 >
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index e3bb0d073352..4bae55f0700b 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -206,7 +206,8 @@ int pci_status_get_and_clear_errors(struct pci_dev *pdev) EXPORT_SYMBOL_GPL(pci_status_get_and_clear_errors); #ifdef CONFIG_HAS_IOMEM -void __iomem *pci_ioremap_bar(struct pci_dev *pdev, int bar) +static void __iomem *__pci_ioremap_resource(struct pci_dev *pdev, int bar, + bool write_combine) { struct resource *res = &pdev->resource[bar]; @@ -214,24 +215,25 @@ void __iomem *pci_ioremap_bar(struct pci_dev *pdev, int bar) * Make sure the BAR is actually a memory resource, not an IO resource */ if (res->flags & IORESOURCE_UNSET || !(res->flags & IORESOURCE_MEM)) { - pci_warn(pdev, "can't ioremap BAR %d: %pR\n", bar, res); + pci_err(pdev, "can't ioremap BAR %d: %pR\n", bar, res); return NULL; } - return ioremap(res->start, resource_size(res)); + + if (write_combine) + return pci_iomap_wc_range(pdev, bar, 0, 0); + + return pci_iomap_range(pdev, bar, 0, 0); +} + +void __iomem *pci_ioremap_bar(struct pci_dev *pdev, int bar) +{ + return __pci_ioremap_resource(pdev, bar, false); } EXPORT_SYMBOL_GPL(pci_ioremap_bar); void __iomem *pci_ioremap_wc_bar(struct pci_dev *pdev, int bar) { - /* - * Make sure the BAR is actually a memory resource, not an IO resource - */ - if (!(pci_resource_flags(pdev, bar) & IORESOURCE_MEM)) { - WARN_ON(1); - return NULL; - } - return ioremap_wc(pci_resource_start(pdev, bar), - pci_resource_len(pdev, bar)); + return __pci_ioremap_resource(pdev, bar, true); } EXPORT_SYMBOL_GPL(pci_ioremap_wc_bar); #endif
Currently, functions pci_ioremap_bar() and pci_ioremap_wc_bar() share similar implementation details as both functions were almost identical in the past, especially when the latter was initially introduced in the commit c43996f4001d ("PCI: Add pci_ioremap_wc_bar()") as somewhat exact copy of the function pci_ioremap_bar(). However, function pci_ioremap_bar() received several updates that were never introduced to the function pci_ioremap_wc_bar(). Thus, to align implementation of both functions and reduce the need to duplicate code between them, introduce a new internal function called __pci_ioremap_resource() as a helper with a shared codebase intended to be called from functions pci_ioremap_bar() and pci_ioremap_wc_bar(). The __pci_ioremap_resource() function will therefore include a check for the IORESOURCE_UNSET flag that has previously been added to the function pci_ioremap_bar() in the commit 646c0282df04 ("PCI: Fail pci_ioremap_bar() on unassigned resources") and otherwise has been missing from function pci_ioremap_wc_bar(). Additionally, function __pci_ioremap_resource() will retire the usage of the WARN_ON() macro and replace it with pci_err() to show information such as the driver name, the BAR number and resource details in case of a failure, instead of printing a complete backtrace. The WARN_ON() has already been replaced with pci_warn() in the commit 1f7bf3bfb5d6 ("PCI: Show driver, BAR#, and resource on pci_ioremap_bar() failure") which sadly didn't include an update to the function pci_ioremap_wc_bar() at that time. Finally, a direct use of functions ioremap() and ioremap_wc() in the function __pci_ioremap_resource() will be replaced with calls to the pci_iomap_range() and pci_iomap_wc_range() functions respectively. Signed-off-by: Krzysztof Wilczyński <kw@linux.com> --- drivers/pci/pci.c | 26 ++++++++++++++------------ 1 file changed, 14 insertions(+), 12 deletions(-)