On 02.04.2024 16:11, Philipp Stanner wrote: > On Tue, 2024-04-02 at 15:54 +0200, Heiner Kallweit wrote: >> 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, > > It is complex – but that complexity has been there before. It's just > moved from pci.c to devres.c and gets coupled with devres. > > Note that it's very hard to clean up the PCI devres API because it's > ossificated and used everywhere. That's why several hybrid functions in > pci.c get redirected to devres.c, including pci_intx(). > >> and I see >> issues if it's called multiple times. > > Which issues would that be? > If pcim_intx() is called more than once, then you don't know what the initial status at driver load time was. And it's my understanding that we do our best to restore the initial state. > If I implemented everything correctly (please say if I didn't), then a > driver using pcim_enable_device() + pci_intx() (without 'm') will > experience exactly the same behavior as before. > > If pcim_intx() is buggy or problematic, it only is because pci_intx() > has always been that way. > > So this would be bug-for-bug-compatible. > pci_intx() and pcim_intx() should be removed in the long term. > >> In addition you state that pci_intx() >> is outdated, but add an API call for the same functionality. > > I do that because that's the only way to kill struct pci_devres in > pci.h. > Take a look at the current implementation of pci_intx(). This is a > hybrid function. We can not just remove the devres part because we'd > break backwards compatiblity. > > And, on Andy Shevchenko's request, the pcim_intx() call is not exposed > to users. It's just visible within the subsystem, to maintain > pci_intx()'s hybrid nature while being. > > We could add a more detailed comment explaining the reasoning, if you > think that's worthwhile. > >> >> Did you see the RFC patches I sent? they could help to reduce the >> complexity >> of the pcim refactoring. > > Nope. Do you have a pointer? > https://www.spinics.net/lists/linux-pci/msg151308.html You were on the addressee list, so you should have the series in your mailbox. > > P. > >> >>> 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 >>>> >>> >> >
