On 28.03.2024 18:35, Heiner Kallweit wrote: > 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. > When looking at the intx members of struct pci_devres: Why not simply store the initial state of bit PCI_COMMAND_INTX_DISABLE in struct pci_dev and restore this bit in do_pci_disable_device()? This should allow us to get rid of these members. > 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
