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. 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 >
