On 02.04.2024 15:17, Philipp Stanner wrote: > On Thu, 2024-03-28 at 23:03 +0100, Heiner Kallweit wrote: >> 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. > > Those members have been there before I touched anything. > Patch #8 removes them entirely, so I'd say that result has been > achieved. > - all the networking people because discussion is solely about PCI core now I think the proposed pcim_intx() is more complex than needed, and I see issues if it's called multiple times. In addition you state that pci_intx() is outdated, but add an API call for the same functionality. Did you see the RFC patches I sent? they could help to reduce the complexity of the pcim refactoring. > Besides, considering the current fragmentation of devres within the PCI > subsystem, I think it's wise to do 100% of devres operations in > devres.c > > P. > >> >>> 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 >> >
