| Message ID | 982b02cb-a095-4131-84a7-24817ac68857@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | PCI: Add and use pcim_iomap_region() | expand |
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. But maybe you could do a review, since you're now also familiar with the code? Greetings, P. [1] https://lore.kernel.org/all/20240301112959.21947-1-pstanner@redhat.com/ > > 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(-) >
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@redhat.com/ > > >> >> 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
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@redhat.com/ >> >> >>> >>> 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
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@redhat.com/ > > > > > > > > > > > > > > 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 >
On Thu, 2024-03-28 at 18:35 +0100, 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. Thx for the feedback so far. See my answers below: > > > 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. I guess patches 5, 6, 7 and 8 could come first. However, 9 has to be last because it kills the legacy struct pci_devres in pci.h and can only do so once the old functions use the new API below (patches 1, 2 and 4). So one could say patches 5-9 form a row, aiming at blowing up that struct, and relying on 1, 2 and 4 to do so. > > 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. All my functions' docstrings do explicitely name the return code. It's indeed important to always do that except for super trivial things, I agree > 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. Well, the thought behind it was that to be more verbose and precise is better than absolute consistency. I'd agree that we could go for pure NULL implementations for functions that just ioremap(), but the more sophisticated PCI wrappers do several things at once: * perform sanity (region ranges etc.) checks on user input * do region requests * iomap * do devres specific stuff, including allocations Especially informing a user specifically about -EBUSY is very valuable IMO. It certainly is for our use case (Nvidia graphics cards with several 'competing' drivers) where several drivers could try to grab the same BAR. A user should get the info "someone is using that thing already". With NULL he'd just see "failed loading driver for some reason". So I think the bes thing would be to remain consistent *within* PCI, since all PCI users should only ever use PCI functions anyways and not ioremap() things manually. So PCI should be as consistent as possible, I think. pcim_iomap() unfortunately has 50 callers. The cleanest way would be to port them to a pcim_iomap() that returns an ERR_PTR(), but I don't have capacity for that right now. > > Then you add support for mapping BAR's partially. I never had such a > use > case. Are there use cases for this? Yes (I was surprised by that as well). Patch #10's vboxvideo does that and mapps a BAR partially. The function pcim_iomap_region_range() just also takes care of a partial region request. It could have been used as an alternative for solving patch #10's issues. > 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. Well, the implementation is 100% based on common infrastructure that's also used by pcim_iomap_region() (that would be enum pcim_addr_devres_typ). One could argue that as long as no one uses the function, it can cause trouble – and once someone uses it there would be a use case. I tested the function previously and it behaved as intended. But, indeed, I'm not sure whether it really hurts to remove or keep it. I'm quite indifferent about it. Regards, P. > > > Greetings, > > P. > > > > [1] > > https://lore.kernel.org/all/20240301112959.21947-1-pstanner@redhat.com/ > > > > > > > > > > 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 >