On 27.03.2024 14:20, Philipp Stanner wrote: > On Wed, 2024-03-27 at 12:52 +0100, Heiner Kallweit wrote: >> Several drivers use the following sequence for a single BAR: >> rc = pcim_iomap_regions(pdev, BIT(bar), name); >> if (rc) >> error; >> addr = pcim_iomap_table(pdev)[bar]; >> >> Let's create a simpler (from implementation and usage perspective) >> pcim_iomap_region() for this use case. > > I like that idea – in fact, I liked it so much that I wrote that > myself, although it didn't make it vor v6.9 ^^ > > You can look at the code here [1] > > Since my series cleans up the PCI devres API as much as possible, I'd > argue that prefering it would be better. > Thanks for the hint. I'm not in a hurry, so yes: We should refactor the pcim API, and then add functionality. > But maybe you could do a review, since you're now also familiar with > the code? > I'm not subscribed to linux-pci, so I missed the cover letter, but had a look at the patches in patchwork. Few remarks: Instead of first adding a lot of new stuff and then cleaning up, I'd propose to start with some cleanups. E.g. patch 7 could come first, this would already allow to remove member mwi from struct pci_devres. By the way, in patch 7 it may be a little simpler to have the following sequence: rc = pci_set_mwi() if (rc) error rc = devm_add_action_or_reset(.., __pcim_clear_mwi, ..); if (rc) error This would avoid the call to devm_remove_action(). We briefly touched the point whether the proposed new function returns NULL or an ERR_PTR. I find it annoying that often the kernel doc function description doesn't mention whether a function returns NULL or an ERR_PTR in the error case. So I have to look at the function code. It's also a typical bug source. We won't solve this in general here. But I think we should be in line with what related functions do. The iomap() functions typically return NULL in the error case. Therefore I'd say any new pcim_...iomap...() function should return NULL too. Then you add support for mapping BAR's partially. I never had such a use case. Are there use cases for this? Maybe we could omit this for now, if it helps to reduce the complexity of the refactoring. I also have bisectability in mind, therefore my personal preference would be to make the single patches as small as possible. Not sure whether there's a way to reduce the size of what currently is the first patch of the series. > Greetings, > P. > > [1] https://lore.kernel.org/all/20240301112959.21947-1-pstanner@xxxxxxxxxx/ > > >> >> Note: The check for !pci_resource_len() is included in >> pcim_iomap(), so we don't have to duplicate it. >> >> Make r8169 the first user of the new function. >> >> I'd prefer to handle this via the PCI tree. >> >> Heiner Kallweit (2): >> PCI: Add pcim_iomap_region >> r8169: use new function pcim_iomap_region() >> >> drivers/net/ethernet/realtek/r8169_main.c | 8 +++---- >> drivers/pci/devres.c | 28 >> +++++++++++++++++++++++ >> include/linux/pci.h | 2 ++ >> 3 files changed, 33 insertions(+), 5 deletions(-) >> > Heiner
