tile: switch to generic pci_iomap
Subsystems (3)
cover tile: switch to generic pci_iomap
Here's a completely untested patch to switch tile
over to the new pci_iomap (in -mm).
This is on top of pci: use generic pci_iomap in most architectures, and converts just the tile architecture.
Seems straightforward enough but completely untested - could someone with access to appropriate systems respond and say whether the first patch in the series is OK?
The second one should then be trivial.
Signature · 1 line
1.7.5.53.gc233e
PATCH 1/2 tile: don't panic on iomap
I think panic on iomap is there just for debugging. If we return NULL instead, the generic pci_iomap will DTRT so we don't need to roll our own.
--- arch/tile/include/asm/io.h | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)
arch/tile/include/asm/io.h +2 -1
@@ -204,7 +204,8 @@ static inline long ioport_panic(void)static inline void __iomem *ioport_map (unsigned long port,unsigned int len) { -return (void __iomem *)ioport_panic (); +pr_info ("Trying to map an IO resource - it does not exit on tile.\n" ); +return NULL ; }static inline void ioport_unmap (void __iomem *addr)
Signature · 1 line
1.7.5.53.gc233e
PATCH 2/2 tile: switch to GENERIC_PCI_IOMAP
In reply to Michael S. Tsirkintile now has working stubs for ioport_map and ioremap such that the generic pci_iomap will DTRT: cast to pointer on memory and return NULL and log message on IO map.
Switch it over to GENERIC_PCI_IOMAP.
--- arch/tile/Kconfig | 1 + arch/tile/include/asm/pci.h | 2 +- arch/tile/kernel/pci.c | 21 --------------------- 3 files changed, 2 insertions(+), 22 deletions(-)
arch/tile/Kconfig +1 -0
@@ -321,6 +321,7 @@ config PCI bool "PCI support" default y select PCI_DOMAINS + select GENERIC_PCI_IOMAP ---help--- Enable PCI root complex support, so PCIe endpoint devices can be attached to the Tile chip. Many, but not all, PCI devices
arch/tile/include/asm/pci.h +1 -1
@@ -16,6 +16,7 @@#define _ASM_TILE_PCI_H#include <linux/pci.h> +#include <asm-generic/pci_iomap.h> /* * Structure of a PCI controller (host bridge)
@@ -49,7 +50,6 @@ struct pci_controller {int __devinit tile_pci_init (void );int __devinit pcibios_init (void ); -void __iomem *pci_iomap (struct pci_dev *dev,int bar,unsigned long max);static inline void pci_iounmap (struct pci_dev *dev,void __iomem *addr) {}void __devinit pcibios_fixup_bus (struct pci_bus *bus);
arch/tile/kernel/pci.c +0 -21
@@ -465,27 +465,6 @@ int pcibios_enable_device(struct pci_dev *dev, int mask)return 0 ; } -void __iomem *pci_iomap (struct pci_dev *dev,int bar,unsigned long max) -{ -unsigned long start =pci_resource_start (dev, bar); -unsigned long len =pci_resource_len (dev, bar); -unsigned long flags =pci_resource_flags (dev, bar); - -if (!len) -return NULL ; -if (max && len > max) - len = max; - -if (!(flags &IORESOURCE_MEM )) { -pr_info ("PCI: Trying to map invalid resource %#lx\n" , flags); - start =0 ; - } - -return (void __iomem *)start; -} -EXPORT_SYMBOL (pci_iomap); - -/**************************************************************** * * Tile PCI config space read/write routines
Signature · 1 line
1.7.5.53.gc233e
Bjorn Helgaas
maintainerRe: [PATCH-RFC 1/2] tile: don't panic on iomap
In reply to Michael S. TsirkinQuoted 3 lines · Michael S. Tsirkin
I think panic on iomap is there just for debugging. If we return NULL instead, the generic
pci_iomapwill DTRT so we don't need to roll our own.
Just to be explicit about what "doing the right thing" means, here's what I think is changing (I think the new behavior is OK, but it *is* different):
Old behavior: Caller calls pci_iomap(), which panics in ioport_map().
New behavior: Caller calls pci_iomap(), ioport_map() returns NULL, pci_iomap() returns NULL (failure), caller may check for failure. If caller does not check for failure and passes the NULL to ioread()/iowrite(), we WARN in bad_io_access().
Quoted 5 lines
static inline void
__iomem*ioport_map(unsigned long port, unsigned int len) {
- return (void__iomem*)ioport_panic();
+pr_info("Trying to map an IO resource - it does not exit on tile.\n");
+ return NULL;
s/exit/exist/
Since we only expect to see this message during debugging, maybe it could be more informative, e.g., use dump_stack() to identify the offending driver? I don't think either the "Trying to map" message or the "Bad IO access" message is enough to actually make progress in debugging.
Bjorn
Arnd Bergmann
maintainerRe: [PATCH-RFC 1/2] tile: don't panic on iomap
In reply to Michael S. Tsirkinarch/tile/include/asm/io.h static inline long ioport_panic(void) · line 209
Quoted 12 lines · Michael S. Tsirkin
arch/tile/include/asm/io.h +2
-1@@ -204,7 +204,8 @@ static inline long ioport_panic(void)static inline void __iomem *ioport_map (unsigned long port,unsigned int len) { -return (void __iomem *)ioport_panic (); +pr_info ("Trying to map an IO resource - it does not exit on tile.\n" ); +return NULL ; }
Why not just set CONFIG_NO_IOPORT and make this unavailable at compile time?
Arnd
Re: [PATCH-RFC 1/2] tile: don't panic on iomap
In reply to Bjorn HelgaasQuoted 8 lines · Bjorn Helgaas
Quoted 3 lines · Michael S. Tsirkin
I think panic on iomap is there just for debugging. If we return NULL instead, the generic
pci_iomapwill DTRT so we don't need to roll our own.Just to be explicit about what "doing the right thing" means, here's what I think is changing (I think the new behavior is OK, but it *is* different):
I think the change is that anyone calling ioport_map *directly* will fail. pci_iomap callers are mostly unaffected.
Quoted 2 lines
Old behavior: Caller calls
pci_iomap(), which panics inioport_map().
Not really, the old pci_iomap simply returned NULL in this case, it did not call ioport_map.
Quoted 13 lines
New behavior: Caller calls
pci_iomap(),ioport_map()returns NULL,pci_iomap()returns NULL (failure), caller may check for failure. If caller does not check for failure and passes the NULL to ioread()/iowrite(), we WARN inbad_io_access().Quoted 5 lines
static inline void
__iomem*ioport_map(unsigned long port, unsigned int len) {
- return (void__iomem*)ioport_panic();
+pr_info("Trying to map an IO resource - it does not exit on tile.\n");
+ return NULL;s/exit/exist/
Quoted 7 lines
Since we only expect to see this message during debugging, maybe it could be more informative, e.g., use
dump_stack()to identify the offending driver? I don't think either the "Trying to map" message or the "Bad IO access" message is enough to actually make progress in debugging.Bjorn
As explained above, only direct callers of ioport_map get a changed behaviour. If we start dumping stack there we will hurt users of pci_iomap which used to get a graceful failure and will start getting scary messages. Is does not seem to be worth doing to simplify debugging, right? How about sticking the function name in the pr_info message?
A simple grep for ioport_map will then get you the culprit ...
Like this:
+ pr_info("ioport_map: mapping IO resources is unsupported on tile.\n"); ?
Signature · 1 line
MST
Re: [PATCH-RFC 1/2] tile: don't panic on iomap
In reply to Arnd BergmannQuoted 16 lines · Arnd Bergmann
arch/tile/include/asm/io.h static inline long ioport_panic(void) · line 209
Quoted 12 lines · Michael S. Tsirkin
arch/tile/include/asm/io.h +2
-1@@ -204,7 +204,8 @@ static inline long ioport_panic(void)static inline void __iomem *ioport_map (unsigned long port,unsigned int len) { -return (void __iomem *)ioport_panic (); +pr_info ("Trying to map an IO resource - it does not exit on tile.\n" ); +return NULL ; }Why not just set
CONFIG_NO_IOPORTand make this unavailable at compile time?
Quoted 1 line
Arnd
Well I think CONFIG_NO_IOPORT only has effect if you pull in asm-generic/io.h. This might work but I have no idea. Presumably whoever wrote that architecture considered and discarded setting GENERIC_IOMAP.
Signature · 1 line
MST
PATCH v2 1/2 tile: don't panic on iomap
In reply to Michael S. TsirkinI think panic on iomap is there just for debugging. If we return NULL instead, the generic pci_iomap will do the same thing as the custom one that tile currently has (that is, return NULL on an IO BAR)
so tile won't need to roll its own anymore.
---
Note: the other patch in the series is unchanged. Changes from v1:
- tweaked pr_info message
arch/tile/include/asm/io.h | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)
arch/tile/include/asm/io.h +2 -1
@@ -204,7 +204,8 @@ static inline long ioport_panic(void)static inline void __iomem *ioport_map (unsigned long port,unsigned int len) { -return (void __iomem *)ioport_panic (); +pr_info ("ioport_map: mapping IO resources is unsupported on tile.\n" ); +return NULL ; }static inline void ioport_unmap (void __iomem *addr)
Signature · 1 line
1.7.5.53.gc233e
Arnd Bergmann
maintainerRe: [PATCH-RFC 1/2] tile: don't panic on iomap
In reply to Michael S. TsirkinQuoted 18 lines · Michael S. Tsirkin
Quoted 16 lines · Arnd Bergmann
arch/tile/include/asm/io.h static inline long ioport_panic(void) · line 209
Quoted 12 lines · Michael S. Tsirkin
arch/tile/include/asm/io.h +2
-1@@ -204,7 +204,8 @@ static inline long ioport_panic(void)static inline void __iomem *ioport_map (unsigned long port,unsigned int len) { -return (void __iomem *)ioport_panic (); +pr_info ("Trying to map an IO resource - it does not exit on tile.\n" ); +return NULL ; }Why not just set
CONFIG_NO_IOPORTand make this unavailable at compile time?
Quoted 4 lines
Quoted 1 line
Arnd
Well I think
CONFIG_NO_IOPORTonly has effect if you pull inasm-generic/io.h. This might work but I have no idea.
NO_IOPORT ha nothing to do with asm-generic, it just makes lib/iomap.c leave out the calls to ioport_map, and prevent all drivers that call ioport_map from being built.
Quoted 1 line
Presumably whoever wrote that architecture considered and discarded setting
GENERIC_IOMAP.
The tile PCI controller is apparently special in that it does not map the PIO region into MMIO space, or make it available in any other way, so generic iomap with IOCOND is pointless. Setting NO_IPORT will still mean that drivers using ioport_map will not be built. This mostly impacts the legacy (non-AHCI) ATA controllers, which I assume are not used on tile anyway.
Arnd
Re: [PATCH-RFC 1/2] tile: don't panic on iomap
Quoted 19 lines · Arnd Bergmann
Quoted 18 lines · Michael S. Tsirkin
Quoted 16 lines · Arnd Bergmann
arch/tile/include/asm/io.h static inline long ioport_panic(void) · line 209
Quoted 12 lines · Michael S. Tsirkin
arch/tile/include/asm/io.h +2
-1@@ -204,7 +204,8 @@ static inline long ioport_panic(void)static inline void __iomem *ioport_map (unsigned long port,unsigned int len) { -return (void __iomem *)ioport_panic (); +pr_info ("Trying to map an IO resource - it does not exit on tile.\n" ); +return NULL ; }Why not just set
CONFIG_NO_IOPORTand make this unavailable at compile time?
Quoted 8 lines
Quoted 4 lines
Quoted 1 line
Arnd
Well I think
CONFIG_NO_IOPORTonly has effect if you pull inasm-generic/io.h. This might work but I have no idea.
NO_IOPORTha nothing to do with asm-generic, it just makeslib/iomap.cleave out the calls toioport_map, and prevent all drivers that callioport_mapfrom being built.
Yes. But lib/iomap.c isn't even linked unless GENERIC_IOMAP is set.
Quoted 9 lines
Quoted 1 line
Presumably whoever wrote that architecture considered and discarded setting
GENERIC_IOMAP.The tile PCI controller is apparently special in that it does not map the PIO region into MMIO space, or make it available in any other way, so generic iomap with IOCOND is pointless. Setting
NO_IPORTwill still mean that drivers usingioport_mapwill not be built. This mostly impacts the legacy (non-AHCI) ATA controllers, which I assume are not used on tile anyway.Arnd
I don't object to setting NO_IPORT at all, but this won't help address the problem this patch is fixing, which is to use a generic pci_iomap on tile.
Signature · 1 line
MST
Arnd Bergmann
maintainerRe: [PATCH-RFC 1/2] tile: don't panic on iomap
Quoted 11 lines · Michael S. Tsirkin
Quoted 7 lines
Quoted 1 line
Presumably whoever wrote that architecture considered and discarded setting
GENERIC_IOMAP.The tile PCI controller is apparently special in that it does not map the PIO region into MMIO space, or make it available in any other way, so generic iomap with IOCOND is pointless. Setting
NO_IPORTwill still mean that drivers usingioport_mapwill not be built. This mostly impacts the legacy (non-AHCI) ATA controllers, which I assume are not used on tile anyway.I don't object to setting
NO_IPORTat all, but this won't help address the problem this patch is fixing, which is to use a genericpci_iomapon tile.
Ah, right. I didn't realize that the generic pci_iomap still attempts to call ioport_map(). It would probably make sense to enclose the ioport_map() call in pci_iomap() inside of #ifdef CONFIG_HAS_IOPORT. It's not exactly beautiful, but probably the most correct solution so that we can make any call to ioport_map() a build-time error on architectures that set CONFIG_NO_IOPORT.
Arnd
Re: [PATCH-RFC 1/2] tile: don't panic on iomap
Quoted 13 lines · Arnd Bergmann
Quoted 11 lines · Michael S. Tsirkin
Quoted 7 lines
Quoted 1 line
Presumably whoever wrote that architecture considered and discarded setting
GENERIC_IOMAP.The tile PCI controller is apparently special in that it does not map the PIO region into MMIO space, or make it available in any other way, so generic iomap with IOCOND is pointless. Setting
NO_IPORTwill still mean that drivers usingioport_mapwill not be built. This mostly impacts the legacy (non-AHCI) ATA controllers, which I assume are not used on tile anyway.I don't object to setting
NO_IPORTat all, but this won't help address the problem this patch is fixing, which is to use a genericpci_iomapon tile.
Quoted 8 lines
Ah, right. I didn't realize that the generic
pci_iomapstill attempts to callioport_map(). It would probably make sense to enclose theioport_map()call inpci_iomap()inside of #ifdefCONFIG_HAS_IOPORT. It's not exactly beautiful, but probably the most correct solution so that we can make any call toioport_map()a build-time error on architectures that setCONFIG_NO_IOPORT.Arnd
I'm not sure why do you want to do that.
Signature · 1 line
MST
Arnd Bergmann
maintainerRe: [PATCH-RFC 1/2] tile: don't panic on iomap
Quoted 11 lines · Michael S. Tsirkin
Quoted 6 lines · Arnd Bergmann
Ah, right. I didn't realize that the generic
pci_iomapstill attempts to callioport_map(). It would probably make sense to enclose theioport_map()call inpci_iomap()inside of #ifdefCONFIG_HAS_IOPORT. It's not exactly beautiful, but probably the most correct solution so that we can make any call toioport_map()a build-time error on architectures that setCONFIG_NO_IOPORT.I'm not sure why do you want to do that.
The problem is that any definition of ioport_map on architectures that can't do it is potentially harmful. Calling panic() is bad style as you pointed out, but simply returning NULL can also be harmful because it's likely that some drivers are written under the (false) assumption that ioport_map can never fail.
Getting a build-time error would be more helpful here IMHO.
Arnd
Re: [PATCH-RFC 1/2] tile: don't panic on iomap
Quoted 13 lines · Arnd Bergmann
Quoted 11 lines · Michael S. Tsirkin
Quoted 6 lines · Arnd Bergmann
Ah, right. I didn't realize that the generic
pci_iomapstill attempts to callioport_map(). It would probably make sense to enclose theioport_map()call inpci_iomap()inside of #ifdefCONFIG_HAS_IOPORT. It's not exactly beautiful, but probably the most correct solution so that we can make any call toioport_map()a build-time error on architectures that setCONFIG_NO_IOPORT.I'm not sure why do you want to do that.
Quoted 8 lines
The problem is that any definition of
ioport_mapon architectures that can't do it is potentially harmful. Callingpanic()is bad style as you pointed out, but simply returning NULL can also be harmful because it's likely that some drivers are written under the (false) assumption thatioport_mapcan never fail. Getting a build-time error would be more helpful here IMHO.Arnd
Yes but uglifying these users is also bad, ifdefs in code are incredibly fragile. Isn't it enough to declare ioport_map __must_check?
Signature · 1 line
MST
Arnd Bergmann
maintainerRe: [PATCH-RFC 1/2] tile: don't panic on iomap
Quoted 21 lines · Michael S. Tsirkin
Quoted 13 lines · Arnd Bergmann
Quoted 11 lines · Michael S. Tsirkin
Quoted 6 lines · Arnd Bergmann
Ah, right. I didn't realize that the generic
pci_iomapstill attempts to callioport_map(). It would probably make sense to enclose theioport_map()call inpci_iomap()inside of #ifdefCONFIG_HAS_IOPORT. It's not exactly beautiful, but probably the most correct solution so that we can make any call toioport_map()a build-time error on architectures that setCONFIG_NO_IOPORT.I'm not sure why do you want to do that.
Quoted 6 lines
The problem is that any definition of
ioport_mapon architectures that can't do it is potentially harmful. Callingpanic()is bad style as you pointed out, but simply returning NULL can also be harmful because it's likely that some drivers are written under the (false) assumption thatioport_mapcan never fail. Getting a build-time error would be more helpful here IMHO.
Quoted 2 lines
Yes but uglifying these users is also bad, ifdefs in code are incredibly fragile. Isn't it enough to declare
ioport_map__must_check?
I guess we already wasted too many electrons over this triviality, I don't actually care all that much, and will probably have to touch the same code again when I get to submit my patch to make inb/outb optional for architectures. Just pick any solution (including your original one). I'll revisit this when I'm bothered by the presence of the ioport_map function and will keep you in the loop on those patches.
Arnd
Re: [PATCH-RFC 2/2] tile: switch to GENERIC_PCI_IOMAP
In reply to Michael S. TsirkinQuoted 4 lines · Michael S. Tsirkin
tile now has working stubs for
ioport_mapand ioremap such that the genericpci_iomapwill DTRT: cast to pointer on memory and return NULL and log message on IO map.
Quoted 8 lines
Switch it over to
GENERIC_PCI_IOMAP.Signed-off-by Michael S. Tsirkin <[email protected]>--- arch/tile/Kconfig | 1 + arch/tile/include/asm/pci.h | 2 +- arch/tile/kernel/pci.c | 21 --------------------- 3 files changed, 2 insertions(+), 22 deletions(-)
Signature · 2 lines
Chris Metcalf, Tilera Corp. http://www.tilera.com
PATCH 1/1 lib/devres.c: allow specifying NO_IOPORT while using PCI
In reply to Arnd BergmannThe tile architecture does not support IOPORT, but it does support PCI and IOMEM. We were not specifying NO_IOPORT, though, because it was causing a couple of functions (pcim_iomap_regions and pcim_iomap_table) that are needed to not get provided. Moving the #ifdef fixes this so that those functions and some related ones are now always provided if CONFIG_PCI is true.
---
This commit was inspired by the recent linux-arch discussion about what to do with ioport_map on architectures that can't do it, so I went back to figure out why I hadn't set NO_IOPORT in the first place. I'm willing to take this into the tile tree if the PCI folks are OK with it, or if someone else can pick it up, that's fine too.
arch/tile/Kconfig | 2 +- lib/devres.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
arch/tile/Kconfig +1 -1
@@ -333,7 +333,7 @@ config NO_IOMEM def_bool !PCI config NO_IOPORT - def_bool !PCI + def_bool y source "drivers/pci/Kconfig"
lib/devres.c +1 -1
@@ -144,6 +144,7 @@ void devm_ioport_unmap(struct device *dev, void __iomem *addr) devm_ioport_map_match, (void *)addr)); }EXPORT_SYMBOL (devm_ioport_unmap); +#endif #ifdef CONFIG_PCI /*
@@ -349,4 +350,3 @@ void pcim_iounmap_regions(struct pci_dev *pdev, u16 mask) }EXPORT_SYMBOL (pcim_iounmap_regions);#endif -#endif
Signature · 1 line
1.6.5.2
Re: [PATCH] lib/devres.c: allow specifying NO_IOPORT while using PCI
Hello,
Quoted 8 lines · Chris Metcalf
The tile architecture does not support IOPORT, but it does support PCI and IOMEM. We were not specifying
NO_IOPORT, though, because it was causing a couple of functions (pcim_iomap_regionsandpcim_iomap_table) that are needed to not get provided. Moving the #ifdef fixes this so that those functions and some related ones are now always provided ifCONFIG_PCIis true.Signed-off-by Chris Metcalf <[email protected]>
Please feel free to route the change through tile. If not, Andrew, can you please pick this up?
Thanks.
Signature · 1 line
tejun
Re: [PATCH] lib/devres.c: allow specifying NO_IOPORT while using PCI
Quoted 10 lines · Tejun Heo
Hello,
Quoted 7 lines · Chris Metcalf
The tile architecture does not support IOPORT, but it does support PCI and IOMEM. We were not specifying
NO_IOPORT, though, because it was causing a couple of functions (pcim_iomap_regionsandpcim_iomap_table) that are needed to not get provided. Moving the #ifdef fixes this so that those functions and some related ones are now always provided ifCONFIG_PCIis true.
Quoted 5 lines
Quoted 1 line
Signed-off-by Chris Metcalf <[email protected]>Acked-by Tejun Heo <[email protected]>Please feel free to route the change through tile. If not, Andrew, can you please pick this up?
Thanks. I'll plan to push this to Linus through the tile tree with your Acked-by.
Signature · 2 lines
Chris Metcalf, Tilera Corp. http://www.tilera.com
Arnd Bergmann
maintainerRe: [PATCH] lib/devres.c: allow specifying NO_IOPORT while using PCI
In reply to Chris MetcalfQuoted 8 lines · On Thursday 01 December 2011 12:59:19 Chris Metcalf wrote:
The tile architecture does not support IOPORT, but it does support PCI and IOMEM. We were not specifying
NO_IOPORT, though, because it was causing a couple of functions (pcim_iomap_regionsandpcim_iomap_table) that are needed to not get provided. Moving the #ifdef fixes this so that those functions and some related ones are now always provided ifCONFIG_PCIis true.Signed-off-by Chris Metcalf <[email protected]>
Hmm, I'm sure that Al Viro intentionally did it the way it is, but I'm not sure exactly why that would be.
Any driver using the functions you now expose for NO_IOPORT is probably prepared to handle both MMIO and PIO BARs on PCI, which you can consider broken in the case where a card actually requires PIO.
On tile, it would be just as broken as other drivers that directly use inb/outb to do the port access, but on platforms that actually have PIO but are not able to map them (like arm/mach-rpc), you would now enable people to write silently broken drivers.
However, assuming they check the return value of the *iomap, the drivers could still easily detect this case at runtime. Also, at least on tile, you already provide a pci_iomap function, which has the exact same problem as the devres functions.
One area where your patch might actually break existing code is architectures that allow the devres functions to be built *and* have PCI support but don't provide a pci_iomap function, because they do not support mapping the PIO range into addresses. I'm not sure if that's actually still the case anywhere after Michael's recent patches, but it used to be the case on RPC and probably others.
Arnd
Re: [PATCH] lib/devres.c: allow specifying NO_IOPORT while using PCI
Quoted 26 lines · Arnd Bergmann
Quoted 8 lines · On Thursday 01 December 2011 12:59:19 Chris Metcalf wrote:
The tile architecture does not support IOPORT, but it does support PCI and IOMEM. We were not specifying
NO_IOPORT, though, because it was causing a couple of functions (pcim_iomap_regionsandpcim_iomap_table) that are needed to not get provided. Moving the #ifdef fixes this so that those functions and some related ones are now always provided ifCONFIG_PCIis true.Signed-off-by Chris Metcalf <[email protected]>Hmm, I'm sure that Al Viro intentionally did it the way it is, but I'm not sure exactly why that would be.
Any driver using the functions you now expose for
NO_IOPORTis probably prepared to handle both MMIO and PIO BARs on PCI, which you can consider broken in the case where a card actually requires PIO.On tile, it would be just as broken as other drivers that directly use inb/outb to do the port access, but on platforms that actually have PIO but are not able to map them (like arm/mach-rpc), you would now enable people to write silently broken drivers.
However, assuming they check the return value of the *iomap, the drivers could still easily detect this case at runtime. Also, at least on tile, you already provide a
pci_iomapfunction, which has the exact same problem as the devres functions.
Quoted 6 lines
One area where your patch might actually break existing code is architectures that allow the devres functions to be built *and* have PCI support but don't provide a
pci_iomapfunction, because they do not support mapping the PIO range into addresses. I'm not sure if that's actually still the case anywhere after Michael's recent patches, but it used to be the case on RPC and probably others.
Sounds sufficiently subtle that I'll plan to drop the patch unless someone else (maybe Al Viro, now cc'ed) wants to weigh in. For tile this just means we'll continue not to set NO_IOPORT, at the cost of a few hundred bytes of dead code; no big deal.
The specific issue I saw was calls to pcim_iomap_{regions,table} from drivers/ata/libata-sff.c and drivers/ata/sata_sil24.c; we use the sata_sil24 in one of our platforms (TILEmpower).
Signature · 2 lines
Chris Metcalf, Tilera Corp. http://www.tilera.com
Arnd Bergmann
maintainerRe: [PATCH] lib/devres.c: allow specifying NO_IOPORT while using PCI
Quoted 3 lines · Chris Metcalf
The specific issue I saw was calls to pcim_iomap_{regions,table} from
drivers/ata/libata-sff.canddrivers/ata/sata_sil24.c; we use thesata_sil24in one of our platforms (TILEmpower).
Hmm, so sata_sil24 does not actually use PIO, so we can probably find a way to make that work. Why do you have to build libata-sff? Shouldn't that only be needed if you actually have an sff-type controller?
Arnd
Re: [PATCH] lib/devres.c: allow specifying NO_IOPORT while using PCI
Quoted 6 lines · Arnd Bergmann
Quoted 3 lines · Chris Metcalf
The specific issue I saw was calls to pcim_iomap_{regions,table} from
drivers/ata/libata-sff.canddrivers/ata/sata_sil24.c; we use thesata_sil24in one of our platforms (TILEmpower).Hmm, so
sata_sil24does not actually use PIO, so we can probably find a way to make that work.
OK, that would be good. I'm not familiar enough with the driver (or indeed the devres stuff) to really take an effective swing at this myself, though.
Quoted 2 lines
Why do you have to build libata-sff? Shouldn't
that only be needed if you actually have an sff-type controller?
Yes, I think this is an accidental legacy of the original development work for PCI root complex three years ago. I disabled ATA_SFF and booted up a number of our inhouse platforms, and all the devices appear to work fine without it. I've now turned it off in our standard configuration.
Signature · 2 lines
Chris Metcalf, Tilera Corp. http://www.tilera.com