[PATCH 0/2] PCI: Add and use pcim_iomap_region()

[PATCH 0/2] PCI: Add and use pcim_iomap_region()

[Heiner Kallweit]

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.

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(-)

-- 
2.44.0

[PATCH 1/2] PCI: Add pcim_iomap_region

[Heiner Kallweit]

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.

Note: The check for !pci_resource_len() is included in
pcim_iomap(), so we don't have to duplicate it.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/pci/devres.c | 28 ++++++++++++++++++++++++++++
 include/linux/pci.h  |  2 ++
 2 files changed, 30 insertions(+)

diff --git a/drivers/pci/devres.c b/drivers/pci/devres.c
index 2c562b9ea..afbb8860b 100644
--- a/drivers/pci/devres.c
+++ b/drivers/pci/devres.c
@@ -343,6 +343,34 @@ void pcim_iounmap(struct pci_dev *pdev, void __iomem *addr)
 }
 EXPORT_SYMBOL(pcim_iounmap);
 
+/**
+ * pcim_iomap_region - Request and iomap a PCI BAR
+ * @pdev: PCI device to map IO resources for
+ * @bar: BAR to request and iomap
+ * @name: Name used when requesting regions
+ *
+ * Request and iomap a region specified by @bar.
+ */
+void __iomem *pcim_iomap_region(struct pci_dev *pdev, int bar, const char *name)
+{
+	void __iomem *addr;
+	int rc;
+
+	if (bar >= DEVICE_COUNT_RESOURCE)
+		return NULL;
+
+	rc = pci_request_region(pdev, bar, name);
+	if (rc)
+		return NULL;
+
+	addr = pcim_iomap(pdev, bar, 0);
+	if (!addr)
+		pci_release_region(pdev, bar);
+
+	return addr;
+}
+EXPORT_SYMBOL_GPL(pcim_iomap_region);
+
 /**
  * pcim_iomap_regions - Request and iomap PCI BARs
  * @pdev: PCI device to map IO resources for
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 16493426a..751ffe8c4 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -2323,6 +2323,8 @@ static inline void pci_fixup_device(enum pci_fixup_pass pass,
 void __iomem *pcim_iomap(struct pci_dev *pdev, int bar, unsigned long maxlen);
 void pcim_iounmap(struct pci_dev *pdev, void __iomem *addr);
 void __iomem * const *pcim_iomap_table(struct pci_dev *pdev);
+void __iomem *pcim_iomap_region(struct pci_dev *pdev, int bar,
+				const char *name);
 int pcim_iomap_regions(struct pci_dev *pdev, int mask, const char *name);
 int pcim_iomap_regions_request_all(struct pci_dev *pdev, int mask,
 				   const char *name);
-- 
2.44.0

[PATCH 2/2] r8169: use new function pcim_iomap_region()

[Heiner Kallweit]

Use new function pcim_iomap_region() to simplify the code.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/ethernet/realtek/r8169_main.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
index 5c879a5c8..7411cf1a1 100644
--- a/drivers/net/ethernet/realtek/r8169_main.c
+++ b/drivers/net/ethernet/realtek/r8169_main.c
@@ -5333,11 +5333,9 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 	if (region < 0)
 		return dev_err_probe(&pdev->dev, -ENODEV, "no MMIO resource found\n");
 
-	rc = pcim_iomap_regions(pdev, BIT(region), KBUILD_MODNAME);
-	if (rc < 0)
-		return dev_err_probe(&pdev->dev, rc, "cannot remap MMIO, aborting\n");
-
-	tp->mmio_addr = pcim_iomap_table(pdev)[region];
+	tp->mmio_addr = pcim_iomap_region(pdev, region, KBUILD_MODNAME);
+	if (!tp->mmio_addr)
+		return dev_err_probe(&pdev->dev, -ENOMEM, "cannot remap MMIO, aborting\n");
 
 	txconfig = RTL_R32(tp, TxConfig);
 	if (txconfig == ~0U)
-- 
2.44.0

Re: [PATCH 2/2] r8169: use new function pcim_iomap_region()

[Philipp Stanner]

On Wed, 2024-03-27 at 12:54 +0100, Heiner Kallweit wrote:
> Use new function pcim_iomap_region() to simplify the code.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  drivers/net/ethernet/realtek/r8169_main.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/ethernet/realtek/r8169_main.c
> b/drivers/net/ethernet/realtek/r8169_main.c
> index 5c879a5c8..7411cf1a1 100644
> --- a/drivers/net/ethernet/realtek/r8169_main.c
> +++ b/drivers/net/ethernet/realtek/r8169_main.c
> @@ -5333,11 +5333,9 @@ static int rtl_init_one(struct pci_dev *pdev,
> const struct pci_device_id *ent)
>         if (region < 0)
>                 return dev_err_probe(&pdev->dev, -ENODEV, "no MMIO
> resource found\n");
>  
> -       rc = pcim_iomap_regions(pdev, BIT(region), KBUILD_MODNAME);
> -       if (rc < 0)
> -               return dev_err_probe(&pdev->dev, rc, "cannot remap
> MMIO, aborting\n");
> -
> -       tp->mmio_addr = pcim_iomap_table(pdev)[region];
> +       tp->mmio_addr = pcim_iomap_region(pdev, region,
> KBUILD_MODNAME);
> +       if (!tp->mmio_addr)
> +               return dev_err_probe(&pdev->dev, -ENOMEM, "cannot
> remap MMIO, aborting\n");
>  
>         txconfig = RTL_R32(tp, TxConfig);
>         if (txconfig == ~0U)

You could use this patch then on top of my series; the only little
change necessary would be that you have to check for an ERR_PTR:

if (IS_ERR(tp->mmio_addr))
   ...


Looks very good otherwise.

P.

Re: [PATCH 0/2] PCI: Add and use pcim_iomap_region()

[Philipp Stanner]

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(-)
> 

Re: [PATCH 0/2] PCI: Add and use pcim_iomap_region()

[Heiner Kallweit]

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

Re: [PATCH 0/2] PCI: Add and use pcim_iomap_region()

[Heiner Kallweit]

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

Re: [PATCH 0/2] PCI: Add and use pcim_iomap_region()

[Philipp Stanner]

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
> 

Re: [PATCH 0/2] PCI: Add and use pcim_iomap_region()

[Heiner Kallweit]

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

Re: [PATCH 0/2] PCI: Add and use pcim_iomap_region()

[Philipp Stanner]

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


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

Re: [PATCH 0/2] PCI: Add and use pcim_iomap_region()

[Heiner Kallweit]

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

Re: [PATCH 0/2] PCI: Add and use pcim_iomap_region()

[Philipp Stanner]

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
>