| Message ID | 20240408084423.6697-1-pstanner@redhat.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Make PCI's devres API more consistent | expand |
Yo, we know reached -rc5. Is this fine for v6.10? Regards, P. On Mon, 2024-04-08 at 10:44 +0200, Philipp Stanner wrote: > Changes in v6: > - Restructure the cleanup in pcim_iomap_regions_request_all() so > that > it doesn't trigger a (false positive) test robot warning. No > behavior change intended. (Dan Carpenter) > > Changes in v5: > - Add Hans's Reviewed-by to vboxvideo patch (Hans de Goede) > - Remove stable-kernel from CC in vboxvideo patch (Hans de Goede) > > Changes in v4: > - Rebase against linux-next > > Changes in v3: > - Use the term "PCI devres API" at some forgotten places. > - Fix more grammar errors in patch #3. > - Remove the comment advising to call (the outdated) pcim_intx() in > pci.c > - Rename __pcim_request_region_range() flags-field "exclusive" to > "req_flags", since this is what the int actually represents. > - Remove the call to pcim_region_request() from patch #10. (Hans) > > Changes in v2: > - Make commit head lines congruent with PCI's style (Bjorn) > - Add missing error checks for devm_add_action(). (Andy) > - Repair the "Returns: " marks for docu generation (Andy) > - Initialize the addr_devres struct with memset(). (Andy) > - Make pcim_intx() a PCI-internal function so that new drivers > won't > be encouraged to use the outdated pci_intx() mechanism. > (Andy / Philipp) > - Fix grammar and spelling (Bjorn) > - Be more precise on why pcim_iomap_table() is problematic (Bjorn) > - Provide the actual structs' and functions' names in the commit > messages (Bjorn) > - Remove redundant variable initializers (Andy) > - Regroup PM bitfield members in struct pci_dev (Andy) > - Make pcim_intx() visible only for the PCI subsystem so that > new > drivers won't use this outdated API (Andy, Myself) > - Add a NOTE to pcim_iomap() to warn about this function being > the onee > xception that does just return NULL. > - Consistently use the term "PCI devres API"; also in Patch #10 > (Bjorn) > > > ¡Hola! > > PCI's devres API suffers several weaknesses: > > 1. There are functions prefixed with pcim_. Those are always managed > counterparts to never-managed functions prefixed with pci_ – or so > one > would like to think. There are some apparently unmanaged functions > (all region-request / release functions, and pci_intx()) which > suddenly become managed once the user has initialized the device > with > pcim_enable_device() instead of pci_enable_device(). This > "sometimes > yes, sometimes no" nature of those functions is confusing and > therefore bug-provoking. In fact, it has already caused a bug in > DRM. > The last patch in this series fixes that bug. > 2. iomappings: Instead of giving each mapping its own callback, the > existing API uses a statically allocated struct tracking one > mapping > per bar. This is not extensible. Especially, you can't create > _ranged_ managed mappings that way, which many drivers want. > 3. Managed request functions only exist as "plural versions" with a > bit-mask as a parameter. That's quite over-engineered considering > that each user only ever mapps one, maybe two bars. > > This series: > - add a set of new "singular" devres functions that use devres the > way > its intended, with one callback per resource. > - deprecates the existing iomap-table mechanism. > - deprecates the hybrid nature of pci_ functions. > - preserves backwards compatibility so that drivers using the > existing > API won't notice any changes. > - adds documentation, especially some warning users about the > complicated nature of PCI's devres. > > > Note that this series is based on my "unify pci_iounmap"-series from > a > few weeks ago. [1] > > I tested this on a x86 VM with a simple pci test-device with two > regions. Operates and reserves resources as intended on my system. > Kasan and kmemleak didn't find any problems. > > I believe this series cleans the API up as much as possible without > having to port all existing drivers to the new API. Especially, I > think > that this implementation is easy to extend if the need for new > managed > functions arises :) > > Greetings, > P. > > Philipp Stanner (10): > PCI: Add new set of devres functions > PCI: Deprecate iomap-table functions > PCI: Warn users about complicated devres nature > PCI: Make devres region requests consistent > PCI: Move dev-enabled status bit to struct pci_dev > PCI: Move pinned status bit to struct pci_dev > PCI: Give pcim_set_mwi() its own devres callback > PCI: Give pci(m)_intx its own devres callback > PCI: Remove legacy pcim_release() > drm/vboxvideo: fix mapping leaks > > drivers/gpu/drm/vboxvideo/vbox_main.c | 20 +- > drivers/pci/devres.c | 1011 +++++++++++++++++++++-- > -- > drivers/pci/iomap.c | 18 + > drivers/pci/pci.c | 123 ++- > drivers/pci/pci.h | 21 +- > include/linux/pci.h | 18 +- > 6 files changed, 999 insertions(+), 212 deletions(-) >
On Mon, Apr 08, 2024 at 10:44:12AM +0200, Philipp Stanner wrote: > ... > PCI's devres API suffers several weaknesses: > > 1. There are functions prefixed with pcim_. Those are always managed > counterparts to never-managed functions prefixed with pci_ – or so one > would like to think. There are some apparently unmanaged functions > (all region-request / release functions, and pci_intx()) which > suddenly become managed once the user has initialized the device with > pcim_enable_device() instead of pci_enable_device(). This "sometimes > yes, sometimes no" nature of those functions is confusing and > therefore bug-provoking. In fact, it has already caused a bug in DRM. > The last patch in this series fixes that bug. > 2. iomappings: Instead of giving each mapping its own callback, the > existing API uses a statically allocated struct tracking one mapping > per bar. This is not extensible. Especially, you can't create > _ranged_ managed mappings that way, which many drivers want. > 3. Managed request functions only exist as "plural versions" with a > bit-mask as a parameter. That's quite over-engineered considering > that each user only ever mapps one, maybe two bars. > > This series: > - add a set of new "singular" devres functions that use devres the way > its intended, with one callback per resource. > - deprecates the existing iomap-table mechanism. > - deprecates the hybrid nature of pci_ functions. > - preserves backwards compatibility so that drivers using the existing > API won't notice any changes. > - adds documentation, especially some warning users about the > complicated nature of PCI's devres. There's a lot of good work here; thanks for working on it. > Philipp Stanner (10): > PCI: Add new set of devres functions This first patch adds some infrastructure and several new exported interfaces: void __iomem *pcim_iomap_region(struct pci_dev *pdev, int bar, const char *name) void pcim_iounmap_region(struct pci_dev *pdev, int bar) int pcim_request_region(struct pci_dev *pdev, int bar, const char *name) void pcim_release_region(struct pci_dev *pdev, int bar) void __iomem *pcim_iomap_range(struct pci_dev *pdev, int bar, void __iomem *pcim_iomap_region_range(struct pci_dev *pdev, int bar, void pcim_iounmap_region_range(struct pci_dev *pdev, int bar, > PCI: Deprecate iomap-table functions This adds a little bit of infrastructure (add/remove to legacy_table), reimplements these existing interfaces: void __iomem *pcim_iomap(struct pci_dev *pdev, int bar, unsigned long maxlen) void pcim_iounmap(struct pci_dev *pdev, void __iomem *addr) int pcim_iomap_regions(struct pci_dev *pdev, int mask, const char *name) int pcim_iomap_regions_request_all(struct pci_dev *pdev, int mask, void pcim_iounmap_regions(struct pci_dev *pdev, int mask) and adds a couple new exported interfaces: void pcim_release_all_regions(struct pci_dev *pdev) int pcim_request_all_regions(struct pci_dev *pdev, const char *name) There's a lot going on in these two patches, so they're hard to review. I think it would be easier if you could do the fixes to existing interfaces first, followed by adding new things, maybe something like separate patches that: - Add pcim_addr_devres_alloc(), pcim_addr_devres_free(), pcim_addr_devres_clear(). - Add pcim_add_mapping_to_legacy_table(), pcim_remove_mapping_from_legacy_table(), pcim_remove_bar_from_legacy_table(). - Reimplement pcim_iomap(), pcim_iomap_regions(), pcim_iounmap(). - Add new interfaces like pcim_iomap_region(), pcim_request_region(), etc. AFAICS, except for pcim_iomap_range() (used by vbox), these new interfaces have no users outside drivers/pci, so ... we might defer adding them, or at least defer exposing them via include/linux/pci.h, until we have users for them. > PCI: Warn users about complicated devres nature > PCI: Make devres region requests consistent > PCI: Move dev-enabled status bit to struct pci_dev > PCI: Move pinned status bit to struct pci_dev > PCI: Give pcim_set_mwi() its own devres callback > PCI: Give pci(m)_intx its own devres callback > PCI: Remove legacy pcim_release() > drm/vboxvideo: fix mapping leaks > > drivers/gpu/drm/vboxvideo/vbox_main.c | 20 +- > drivers/pci/devres.c | 1011 +++++++++++++++++++++---- > drivers/pci/iomap.c | 18 + > drivers/pci/pci.c | 123 ++- > drivers/pci/pci.h | 21 +- > include/linux/pci.h | 18 +- > 6 files changed, 999 insertions(+), 212 deletions(-) > > -- > 2.44.0 >
On Wed, 2024-04-24 at 15:12 -0500, Bjorn Helgaas wrote: > On Mon, Apr 08, 2024 at 10:44:12AM +0200, Philipp Stanner wrote: > > ... > > PCI's devres API suffers several weaknesses: > > > > 1. There are functions prefixed with pcim_. Those are always > > managed > > counterparts to never-managed functions prefixed with pci_ – or > > so one > > would like to think. There are some apparently unmanaged > > functions > > (all region-request / release functions, and pci_intx()) which > > suddenly become managed once the user has initialized the device > > with > > pcim_enable_device() instead of pci_enable_device(). This > > "sometimes > > yes, sometimes no" nature of those functions is confusing and > > therefore bug-provoking. In fact, it has already caused a bug in > > DRM. > > The last patch in this series fixes that bug. > > 2. iomappings: Instead of giving each mapping its own callback, the > > existing API uses a statically allocated struct tracking one > > mapping > > per bar. This is not extensible. Especially, you can't create > > _ranged_ managed mappings that way, which many drivers want. > > 3. Managed request functions only exist as "plural versions" with a > > bit-mask as a parameter. That's quite over-engineered > > considering > > that each user only ever mapps one, maybe two bars. > > > > This series: > > - add a set of new "singular" devres functions that use devres the > > way > > its intended, with one callback per resource. > > - deprecates the existing iomap-table mechanism. > > - deprecates the hybrid nature of pci_ functions. > > - preserves backwards compatibility so that drivers using the > > existing > > API won't notice any changes. > > - adds documentation, especially some warning users about the > > complicated nature of PCI's devres. > > There's a lot of good work here; thanks for working on it. Thanks! Good to get some more feedback from you > > > Philipp Stanner (10): > > PCI: Add new set of devres functions > > This first patch adds some infrastructure and several new exported > interfaces: > > void __iomem *pcim_iomap_region(struct pci_dev *pdev, int bar, > const char *name) > void pcim_iounmap_region(struct pci_dev *pdev, int bar) > int pcim_request_region(struct pci_dev *pdev, int bar, const char > *name) > void pcim_release_region(struct pci_dev *pdev, int bar) > void __iomem *pcim_iomap_range(struct pci_dev *pdev, int bar, > void __iomem *pcim_iomap_region_range(struct pci_dev *pdev, int > bar, > void pcim_iounmap_region_range(struct pci_dev *pdev, int bar, > > > PCI: Deprecate iomap-table functions > > This adds a little bit of infrastructure (add/remove to > legacy_table), > reimplements these existing interfaces: > > void __iomem *pcim_iomap(struct pci_dev *pdev, int bar, unsigned > long maxlen) > void pcim_iounmap(struct pci_dev *pdev, void __iomem *addr) > int pcim_iomap_regions(struct pci_dev *pdev, int mask, const char > *name) > int pcim_iomap_regions_request_all(struct pci_dev *pdev, int mask, > void pcim_iounmap_regions(struct pci_dev *pdev, int mask) > > and adds a couple new exported interfaces: > > void pcim_release_all_regions(struct pci_dev *pdev) > int pcim_request_all_regions(struct pci_dev *pdev, const char > *name) > > There's a lot going on in these two patches, so they're hard to > review. I think it would be easier if you could do the fixes to > existing interfaces first, I agree that the patches can be further split into smaller chunks to make them more atomic and easier to review. I can do that. BUT I'd need some more details about what you mean by "do the fixes first" – which fixes? The later patches at least in part rely on the new better functions being available. > followed by adding new things, maybe > something like separate patches that: > > - Add pcim_addr_devres_alloc(), pcim_addr_devres_free(), > pcim_addr_devres_clear(). > > - Add pcim_add_mapping_to_legacy_table(), > pcim_remove_mapping_from_legacy_table(), > pcim_remove_bar_from_legacy_table(). > > - Reimplement pcim_iomap(), pcim_iomap_regions(), pcim_iounmap(). > > - Add new interfaces like pcim_iomap_region(), > pcim_request_region(), etc. > > AFAICS, except for pcim_iomap_range() (used by vbox), these new > interfaces have no users outside drivers/pci, so ... we might > defer adding them, or at least defer exposing them via > include/linux/pci.h, until we have users for them. Dropping (the export of) functions like pcim_request_region_range() or pcim_request_all_regions() is not a problem. What I quite fundamentally have to disagree with, however, is not to export the functions * pcim_request_region() * pcim_iomap_region() the main point of this series is to deprecate that hybrid nature of those existing pci_* functions. You can only deprecate something when you provide users with new, better alternatives. Not exporting them would inevitably tempt driver programmers into using pcim_enable_device() + pci_*() as they did so far, which caused at least that leak in vboxvideo and another one that my plan was to address after we got this merged. Once we have those new pcim_ functions exported, we could successively port the older drivers which use the aforementioned combination with pcim_enable_device(). Then we could drop the hybrid nature of pci_ functions once and for all and would end up with a consistent, clean API. I intended to do that over the months after we merged this. So I'd suggest let me cancel the export of the "luxury functions" and let's keep the two listed above exported P. > > > PCI: Warn users about complicated devres nature > > PCI: Make devres region requests consistent > > PCI: Move dev-enabled status bit to struct pci_dev > > PCI: Move pinned status bit to struct pci_dev > > PCI: Give pcim_set_mwi() its own devres callback > > PCI: Give pci(m)_intx its own devres callback > > PCI: Remove legacy pcim_release() > > drm/vboxvideo: fix mapping leaks > > > > drivers/gpu/drm/vboxvideo/vbox_main.c | 20 +- > > drivers/pci/devres.c | 1011 > > +++++++++++++++++++++---- > > drivers/pci/iomap.c | 18 + > > drivers/pci/pci.c | 123 ++- > > drivers/pci/pci.h | 21 +- > > include/linux/pci.h | 18 +- > > 6 files changed, 999 insertions(+), 212 deletions(-) > > > > -- > > 2.44.0 > > >
On Fri, Apr 26, 2024 at 10:07:02AM +0200, Philipp Stanner wrote: > On Wed, 2024-04-24 at 15:12 -0500, Bjorn Helgaas wrote: > > On Mon, Apr 08, 2024 at 10:44:12AM +0200, Philipp Stanner wrote: > > > ... > > > PCI's devres API suffers several weaknesses: > > > > > > 1. There are functions prefixed with pcim_. Those are always > > > managed > > > counterparts to never-managed functions prefixed with pci_ – or > > > so one > > > would like to think. There are some apparently unmanaged > > > functions > > > (all region-request / release functions, and pci_intx()) which > > > suddenly become managed once the user has initialized the device > > > with > > > pcim_enable_device() instead of pci_enable_device(). This > > > "sometimes > > > yes, sometimes no" nature of those functions is confusing and > > > therefore bug-provoking. In fact, it has already caused a bug in > > > DRM. > > > The last patch in this series fixes that bug. > > > 2. iomappings: Instead of giving each mapping its own callback, the > > > existing API uses a statically allocated struct tracking one > > > mapping > > > per bar. This is not extensible. Especially, you can't create > > > _ranged_ managed mappings that way, which many drivers want. > > > 3. Managed request functions only exist as "plural versions" with a > > > bit-mask as a parameter. That's quite over-engineered > > > considering > > > that each user only ever mapps one, maybe two bars. > > > > > > This series: > > > - add a set of new "singular" devres functions that use devres the > > > way > > > its intended, with one callback per resource. > > > - deprecates the existing iomap-table mechanism. > > > - deprecates the hybrid nature of pci_ functions. > > > - preserves backwards compatibility so that drivers using the > > > existing > > > API won't notice any changes. > > > - adds documentation, especially some warning users about the > > > complicated nature of PCI's devres. > > > > There's a lot of good work here; thanks for working on it. > > Thanks! > Good to get some more feedback from you > > > > > > Philipp Stanner (10): > > > PCI: Add new set of devres functions > > > > This first patch adds some infrastructure and several new exported > > interfaces: > > > > void __iomem *pcim_iomap_region(struct pci_dev *pdev, int bar, > > const char *name) > > void pcim_iounmap_region(struct pci_dev *pdev, int bar) > > int pcim_request_region(struct pci_dev *pdev, int bar, const char > > *name) > > void pcim_release_region(struct pci_dev *pdev, int bar) > > void __iomem *pcim_iomap_range(struct pci_dev *pdev, int bar, > > void __iomem *pcim_iomap_region_range(struct pci_dev *pdev, int > > bar, > > void pcim_iounmap_region_range(struct pci_dev *pdev, int bar, > > > > > PCI: Deprecate iomap-table functions > > > > This adds a little bit of infrastructure (add/remove to > > legacy_table), > > reimplements these existing interfaces: > > > > void __iomem *pcim_iomap(struct pci_dev *pdev, int bar, unsigned > > long maxlen) > > void pcim_iounmap(struct pci_dev *pdev, void __iomem *addr) > > int pcim_iomap_regions(struct pci_dev *pdev, int mask, const char > > *name) > > int pcim_iomap_regions_request_all(struct pci_dev *pdev, int mask, > > void pcim_iounmap_regions(struct pci_dev *pdev, int mask) > > > > and adds a couple new exported interfaces: > > > > void pcim_release_all_regions(struct pci_dev *pdev) > > int pcim_request_all_regions(struct pci_dev *pdev, const char > > *name) > > > > There's a lot going on in these two patches, so they're hard to > > review. I think it would be easier if you could do the fixes to > > existing interfaces first, > > I agree that the patches can be further split into smaller chunks to > make them more atomic and easier to review. I can do that. > > BUT I'd need some more details about what you mean by "do the fixes > first" – which fixes? > The later patches at least in part rely on the new better functions > being available. > > > followed by adding new things, maybe > > something like separate patches that: > > > > - Add pcim_addr_devres_alloc(), pcim_addr_devres_free(), > > pcim_addr_devres_clear(). > > > > - Add pcim_add_mapping_to_legacy_table(), > > pcim_remove_mapping_from_legacy_table(), > > pcim_remove_bar_from_legacy_table(). > > > > - Reimplement pcim_iomap(), pcim_iomap_regions(), pcim_iounmap(). This is the part I meant by "fixes", but maybe it's not so much a fix as it is reimplementing based on different infrastructure. The diffs in "PCI: Deprecate iomap-table functions" for pcim_iomap() and pcim_iounmap() are fairly straightforward and only depend on the above. pcim_iomap_regions() is a bit more complicated and probably needs pcim_iomap_region() but not necessarily __pcim_request_region() and __pcim_request_region_range(). This would be a pretty small patch and defer making them deprecated until replacements are added. > > - Add new interfaces like pcim_iomap_region(), > > pcim_request_region(), etc. > > > > AFAICS, except for pcim_iomap_range() (used by vbox), these new > > interfaces have no users outside drivers/pci, so ... we might > > defer adding them, or at least defer exposing them via > > include/linux/pci.h, until we have users for them. > > Dropping (the export of) functions like pcim_request_region_range() or > pcim_request_all_regions() is not a problem. > > What I quite fundamentally have to disagree with, however, is not to > export the functions > > * pcim_request_region() > * pcim_iomap_region() > > the main point of this series is to deprecate that hybrid nature of > those existing pci_* functions. You can only deprecate something when > you provide users with new, better alternatives. Right. But the new alternatives are only better when there are actual examples in the tree for people to look at. If there are no users, more interfaces are at best confusing and at worst dead code. Bjorn
Hey Bjorn, so I have spent a few hours seeing how we could simplify this series. Here are my thoughts. On Fri, 2024-04-26 at 17:01 -0500, Bjorn Helgaas wrote: > On Fri, Apr 26, 2024 at 10:07:02AM +0200, Philipp Stanner wrote: > > On Wed, 2024-04-24 at 15:12 -0500, Bjorn Helgaas wrote: > > > On Mon, Apr 08, 2024 at 10:44:12AM +0200, Philipp Stanner wrote: > > > > ... > > > > PCI's devres API suffers several weaknesses: > > > > > > > > 1. There are functions prefixed with pcim_. Those are always > > > > managed > > > > counterparts to never-managed functions prefixed with pci_ – > > > > or > > > > so one > > > > would like to think. There are some apparently unmanaged > > > > functions > > > > (all region-request / release functions, and pci_intx()) > > > > which > > > > suddenly become managed once the user has initialized the > > > > device > > > > with > > > > pcim_enable_device() instead of pci_enable_device(). This > > > > "sometimes > > > > yes, sometimes no" nature of those functions is confusing > > > > and > > > > therefore bug-provoking. In fact, it has already caused a > > > > bug in > > > > DRM. > > > > The last patch in this series fixes that bug. > > > > 2. iomappings: Instead of giving each mapping its own callback, > > > > the > > > > existing API uses a statically allocated struct tracking one > > > > mapping > > > > per bar. This is not extensible. Especially, you can't > > > > create > > > > _ranged_ managed mappings that way, which many drivers want. > > > > 3. Managed request functions only exist as "plural versions" > > > > with a > > > > bit-mask as a parameter. That's quite over-engineered > > > > considering > > > > that each user only ever mapps one, maybe two bars. > > > > > > > > This series: > > > > - add a set of new "singular" devres functions that use devres > > > > the > > > > way > > > > its intended, with one callback per resource. > > > > - deprecates the existing iomap-table mechanism. > > > > - deprecates the hybrid nature of pci_ functions. > > > > - preserves backwards compatibility so that drivers using the > > > > existing > > > > API won't notice any changes. > > > > - adds documentation, especially some warning users about the > > > > complicated nature of PCI's devres. > > > > > > There's a lot of good work here; thanks for working on it. > > > > Thanks! > > Good to get some more feedback from you > > > > > > > > > Philipp Stanner (10): > > > > PCI: Add new set of devres functions > > > > > > This first patch adds some infrastructure and several new > > > exported > > > interfaces: > > > > > > void __iomem *pcim_iomap_region(struct pci_dev *pdev, int bar, > > > const char *name) > > > void pcim_iounmap_region(struct pci_dev *pdev, int bar) > > > int pcim_request_region(struct pci_dev *pdev, int bar, const > > > char > > > *name) > > > void pcim_release_region(struct pci_dev *pdev, int bar) > > > void __iomem *pcim_iomap_range(struct pci_dev *pdev, int bar, > > > void __iomem *pcim_iomap_region_range(struct pci_dev *pdev, int > > > bar, > > > void pcim_iounmap_region_range(struct pci_dev *pdev, int bar, > > > > > > > PCI: Deprecate iomap-table functions > > > > > > This adds a little bit of infrastructure (add/remove to > > > legacy_table), > > > reimplements these existing interfaces: > > > > > > void __iomem *pcim_iomap(struct pci_dev *pdev, int bar, > > > unsigned > > > long maxlen) > > > void pcim_iounmap(struct pci_dev *pdev, void __iomem *addr) > > > int pcim_iomap_regions(struct pci_dev *pdev, int mask, const > > > char > > > *name) > > > int pcim_iomap_regions_request_all(struct pci_dev *pdev, int > > > mask, > > > void pcim_iounmap_regions(struct pci_dev *pdev, int mask) > > > > > > and adds a couple new exported interfaces: > > > > > > void pcim_release_all_regions(struct pci_dev *pdev) > > > int pcim_request_all_regions(struct pci_dev *pdev, const char > > > *name) > > > > > > There's a lot going on in these two patches, so they're hard to > > > review. I think it would be easier if you could do the fixes to > > > existing interfaces first, > > > > I agree that the patches can be further split into smaller chunks > > to > > make them more atomic and easier to review. I can do that. > > > > BUT I'd need some more details about what you mean by "do the fixes > > first" – which fixes? > > The later patches at least in part rely on the new better functions > > being available. > > > > > followed by adding new things, maybe > > > something like separate patches that: > > > > > > - Add pcim_addr_devres_alloc(), pcim_addr_devres_free(), > > > pcim_addr_devres_clear(). > > > > > > - Add pcim_add_mapping_to_legacy_table(), > > > pcim_remove_mapping_from_legacy_table(), > > > pcim_remove_bar_from_legacy_table(). > > > > > > - Reimplement pcim_iomap(), pcim_iomap_regions(), > > > pcim_iounmap(). > > This is the part I meant by "fixes", but maybe it's not so much a fix > as it is reimplementing based on different infrastructure. The diffs > in "PCI: Deprecate iomap-table functions" for pcim_iomap() and > pcim_iounmap() are fairly straightforward and only depend on the > above. > > pcim_iomap_regions() is a bit more complicated and probably needs > pcim_iomap_region() but not necessarily __pcim_request_region() and > __pcim_request_region_range(). Well, pcim_iomap_region() relies on __pcim_request_region() and __pcim_request_region_range(). The entire architecture circles around those: At the bottom you have generic functions that can reserve you any range, and at the top you have functions that use those to get a whole BAR if needed. That was done very specifically to have an extensible API that doesn't suffer the limitations of the current devres API. It's super easy this way to add all sorts of request() functions should the need ever arise, all you need to do is add a PCIM_ADDR_* counterpart, basically. Right now, I wouldn't even know how I could implement pcim_iomap_region() without my __pcim_* helpers, since the later are (see the comment above them) written as counter parts to the ones in pci.c – which are hybrid again. So I couldn't write it that way, because then we'd have circle where the pcim_ function calls the pci_ function which might call the pcim_ function again... So the one thing I really want is to keep __pcim_request_region_range() and its wrappers. The algorithms for the region ranges in __pcim_request_region_range() can IMO be trusted to work because they are just copies of the existing ones in pci_iomap_region(). Only difference is that they request instead of ioremap(). And the request part in turn is copied from __pci_request_region(), only difference being that I tried to make it more readable by storing the pci_resource_*() return values at the top :) There's really a reason why I did it this way: 1. Add new, better interfaces 2. Reimplement pcim_iomap() & partners 3. Separate that hybrid API as much as possible I really tried to put some thought into the existing approach. Changing that now in v7 would be very work intensive, because I'd have to think everything through again, refactor it, drop APIs again, split commits, readjust commit messages and all the clean up comments inline (which wouldn't fit anymore), test the build commit-by-commit carefully once again with KASAN & Co... Yesterday I also found that abandoning that "inner nature" of the series would actually decrease readability in many cases, because pcim_addr_resources_match() would be added later, whereas the logic added in pcim_addr_resource_release() would be now empty at the beginning and scattered over several patches etc. > > This would be a pretty small patch and defer making them deprecated > until replacements are added. I understand your desire to make those two patches smaller. We can work on making things more readable and reduce the exported APIs Let me suggest this: * I drop luxury functions no one needs (neither internally nor externally) yet (e.g., pcim_request_region_range()). That will already make patch #1 quite a bit smaller. * We don't export anything else which is not needed by users yet (pcim_request_all_regions(), pcim_request_region_exclusive() etc.) * I split patches #1 and #2 into smaller chunks where possible according to your suggestions above. * We keep __pcim_request_region(), and use it as the working horse cleanly separated from the hybrid counterparts in pci.c. Would that work for you? P. > > > > - Add new interfaces like pcim_iomap_region(), > > > pcim_request_region(), etc. > > > > > > AFAICS, except for pcim_iomap_range() (used by vbox), these > > > new > > > interfaces have no users outside drivers/pci, so ... we might > > > defer adding them, or at least defer exposing them via > > > include/linux/pci.h, until we have users for them. > > > > Dropping (the export of) functions like pcim_request_region_range() > > or > > pcim_request_all_regions() is not a problem. > > > > What I quite fundamentally have to disagree with, however, is not > > to > > export the functions > > > > * pcim_request_region() > > * pcim_iomap_region() > > > > the main point of this series is to deprecate that hybrid nature of > > those existing pci_* functions. You can only deprecate something > > when > > you provide users with new, better alternatives. > > Right. But the new alternatives are only better when there are > actual > examples in the tree for people to look at. If there are no users, > more interfaces are at best confusing and at worst dead code. > > Bjorn >
Changes in v6: - Restructure the cleanup in pcim_iomap_regions_request_all() so that it doesn't trigger a (false positive) test robot warning. No behavior change intended. (Dan Carpenter) Changes in v5: - Add Hans's Reviewed-by to vboxvideo patch (Hans de Goede) - Remove stable-kernel from CC in vboxvideo patch (Hans de Goede) Changes in v4: - Rebase against linux-next Changes in v3: - Use the term "PCI devres API" at some forgotten places. - Fix more grammar errors in patch #3. - Remove the comment advising to call (the outdated) pcim_intx() in pci.c - Rename __pcim_request_region_range() flags-field "exclusive" to "req_flags", since this is what the int actually represents. - Remove the call to pcim_region_request() from patch #10. (Hans) Changes in v2: - Make commit head lines congruent with PCI's style (Bjorn) - Add missing error checks for devm_add_action(). (Andy) - Repair the "Returns: " marks for docu generation (Andy) - Initialize the addr_devres struct with memset(). (Andy) - Make pcim_intx() a PCI-internal function so that new drivers won't be encouraged to use the outdated pci_intx() mechanism. (Andy / Philipp) - Fix grammar and spelling (Bjorn) - Be more precise on why pcim_iomap_table() is problematic (Bjorn) - Provide the actual structs' and functions' names in the commit messages (Bjorn) - Remove redundant variable initializers (Andy) - Regroup PM bitfield members in struct pci_dev (Andy) - Make pcim_intx() visible only for the PCI subsystem so that new drivers won't use this outdated API (Andy, Myself) - Add a NOTE to pcim_iomap() to warn about this function being the onee xception that does just return NULL. - Consistently use the term "PCI devres API"; also in Patch #10 (Bjorn) ¡Hola! PCI's devres API suffers several weaknesses: 1. There are functions prefixed with pcim_. Those are always managed counterparts to never-managed functions prefixed with pci_ – or so one would like to think. There are some apparently unmanaged functions (all region-request / release functions, and pci_intx()) which suddenly become managed once the user has initialized the device with pcim_enable_device() instead of pci_enable_device(). This "sometimes yes, sometimes no" nature of those functions is confusing and therefore bug-provoking. In fact, it has already caused a bug in DRM. The last patch in this series fixes that bug. 2. iomappings: Instead of giving each mapping its own callback, the existing API uses a statically allocated struct tracking one mapping per bar. This is not extensible. Especially, you can't create _ranged_ managed mappings that way, which many drivers want. 3. Managed request functions only exist as "plural versions" with a bit-mask as a parameter. That's quite over-engineered considering that each user only ever mapps one, maybe two bars. This series: - add a set of new "singular" devres functions that use devres the way its intended, with one callback per resource. - deprecates the existing iomap-table mechanism. - deprecates the hybrid nature of pci_ functions. - preserves backwards compatibility so that drivers using the existing API won't notice any changes. - adds documentation, especially some warning users about the complicated nature of PCI's devres. Note that this series is based on my "unify pci_iounmap"-series from a few weeks ago. [1] I tested this on a x86 VM with a simple pci test-device with two regions. Operates and reserves resources as intended on my system. Kasan and kmemleak didn't find any problems. I believe this series cleans the API up as much as possible without having to port all existing drivers to the new API. Especially, I think that this implementation is easy to extend if the need for new managed functions arises :) Greetings, P. Philipp Stanner (10): PCI: Add new set of devres functions PCI: Deprecate iomap-table functions PCI: Warn users about complicated devres nature PCI: Make devres region requests consistent PCI: Move dev-enabled status bit to struct pci_dev PCI: Move pinned status bit to struct pci_dev PCI: Give pcim_set_mwi() its own devres callback PCI: Give pci(m)_intx its own devres callback PCI: Remove legacy pcim_release() drm/vboxvideo: fix mapping leaks drivers/gpu/drm/vboxvideo/vbox_main.c | 20 +- drivers/pci/devres.c | 1011 +++++++++++++++++++++---- drivers/pci/iomap.c | 18 + drivers/pci/pci.c | 123 ++- drivers/pci/pci.h | 21 +- include/linux/pci.h | 18 +- 6 files changed, 999 insertions(+), 212 deletions(-)