[PATCH v2 0/3] MIPS: ralink: properly handle pci IO resources

[PATCH v2 0/3] MIPS: ralink: properly handle pci IO resources

[Sergio Paracuellos]

Hi all,

Defining PCI_IOBASE for MIPS ralink platform results in resource handling working
but the addresses generated for IO access being wrong, because the iomap tries to
ioremap it to a fixed virtual address (PCI_IOBASE), which can't work for KSEG1 
addresses. To get it working this way, we would need to put PCI_IOBASE somewhere
into KSEG2, which will create TLB entries for IO addresses, which most of the
time isn't needed on MIPS because of access via KSEG1. Instead of doing that and
taking into account that we need to get a valid IO address from 'pci_address_to_pio'
and ralink platforms have IO addresses higher than 0xffff, the following approach
will be preferred to get expected working behaviour from PCI core APIs and pci 
drivers working together:
 
1) Avoid to define PCI_IOBASE.
2) Set IO_SPACE_LIMIT to 0x1fffffff which is a valid range for this SoCs.
3) Return zero from 'pci_remap_iospace' if PCI_IOBASE is not defined.
3) Set ioport_resource end limit to this new IO_SPACE_LIMIT.

Doing in this way we end up with a properly working PCI IO in ralink SoCs.
These changes metioned above are in the three patches included in this series.

Thanks in advance for your time and comments.

Changes in v2 (only PATCH v2 2/3 afected):
  - Instead of avoid to call 'devm_pci_remap_iospace' fix 'pci_remap_iospace'
    to return zero for PCI_IOBASE not defined architectures.

Bjorn, I don't know if I should add any kind of 'Fixes' tag for this or
what is the way of handle this kind of changes inside the PCI tree. Thanks
in advance for clarification.

Sergio Paracuellos (3):
  MIPS: ralink: don't define PC_IOBASE but increase IO_SPACE_LIMIT
  PCI: fix 'pci_remap_iospace' for architectures with PCI_IOBASE not
    defined
  staging: mt7621-pci: set end limit for 'ioport_resource'

 arch/mips/include/asm/mach-ralink/spaces.h |  4 +---
 drivers/pci/pci.c                          | 12 ++++++------
 drivers/staging/mt7621-pci/pci-mt7621.c    |  2 ++
 3 files changed, 9 insertions(+), 9 deletions(-)

-- 
2.25.1

[PATCH v2 1/3] MIPS: ralink: don't define PC_IOBASE but increase IO_SPACE_LIMIT

[Sergio Paracuellos]

Defining PCI_IOBASE results in pci resource handling working but the
addresses generated for IO accesses are wrong since the ioremap in the pci core
function 'pci_parse_request_of_pci_ranges' tries to remap to a fixed virtual
address (PC_IOBASE) which can't work for KSEG1 addresses. To get it working this
way, we would need to put PCI_IOBASE somewhere into KSEG2 which will result in
creating TLB entries for IO addresses, which most of the time isn't needed on
MIPS because of access via KSEG1. So avoid to define PCI_IOBASE and increase
IO_SPACE_LIMIT resource for ralink MIPS platform instead, to get valid IO
addresses for resources from pci core 'pci_address_to_pio' function.

Fixes: 222b27713d7f ("MIPS: ralink: Define PCI_IOBASE)
Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
---
 arch/mips/include/asm/mach-ralink/spaces.h | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/arch/mips/include/asm/mach-ralink/spaces.h b/arch/mips/include/asm/mach-ralink/spaces.h
index 87d085c9ad61..31a3525213cf 100644
--- a/arch/mips/include/asm/mach-ralink/spaces.h
+++ b/arch/mips/include/asm/mach-ralink/spaces.h
@@ -2,9 +2,7 @@
 #ifndef __ASM_MACH_RALINK_SPACES_H_
 #define __ASM_MACH_RALINK_SPACES_H_
 
-#define PCI_IOBASE	_AC(0xa0000000, UL)
-#define PCI_IOSIZE	SZ_16M
-#define IO_SPACE_LIMIT	(PCI_IOSIZE - 1)
+#define IO_SPACE_LIMIT	0x1fffffff
 
 #include <asm/mach-generic/spaces.h>
 #endif
-- 
2.25.1

[PATCH v2 2/3] PCI: fix 'pci_remap_iospace' for architectures with PCI_IOBASE not defined

[Sergio Paracuellos]

Request for I/O resources from device tree call 'pci_remap_iospace' from
'devm_pci_remap_iospace' which is also called from device tree function
'pci_parse_request_of_pci_ranges'. if PCI_IOBASE is not defined and I/O
resources are requested the following warning appears:

------------[ cut here ]------------
WARNING: CPU: 2 PID: 1 at ../drivers/pci/pci.c:4066 pci_remap_iospace+0x3c/0x54
This architecture does not support memory mapped I/O
Modules linked in:
CPU: 2 PID: 1 Comm: swapper/0 Not tainted 5.10.1+ #1228
Stack : 00000000 00000000 807fa974 00000000 827ffa80 80066b48 80710000 0000000b
        00000000 00000000 81c59aac 7d06ddec 80850000 00000001 81c59a40 7d06ddec
        00000000 00000000 807c909c 81c598f0 00000001 81c59904 00000000 0000000a
        203a6d6d 80708880 0000000f 70617773 80850000 00000000 00000000 807d0000
        807ffecc 1e160000 00000001 00000200 00000000 8054e920 00000008 815e0008
        ...
Call Trace:
[<80008efc>] show_stack+0x8c/0x130
[<806e1674>] dump_stack+0x9c/0xc8
[<80024a3c>] __warn+0xc0/0xe8
[<80024ad0>] warn_slowpath_fmt+0x6c/0xbc
[<80410ca8>] pci_remap_iospace+0x3c/0x54
[<80410d20>] devm_pci_remap_iospace+0x58/0xa4
[<8042019c>] devm_of_pci_bridge_init+0x4dc/0x55c
[<80408de8>] devm_pci_alloc_host_bridge+0x78/0x88
[<80424e44>] mt7621_pci_probe+0x68/0x9a4
[<80464804>] platform_drv_probe+0x40/0x7c
[<804628bc>] really_probe+0x2fc/0x4e4
[<80463214>] device_driver_attach+0x4c/0x74
[<80463384>] __driver_attach+0x148/0x150
[<8046047c>] bus_for_each_dev+0x6c/0xb0
[<804614dc>] bus_add_driver+0x1b4/0x1fc
[<80463aa0>] driver_register+0xd0/0x110
[<80001714>] do_one_initcall+0x84/0x1c0
[<808e7fd0>] kernel_init_freeable+0x214/0x24c
[<806e4164>] kernel_init+0x14/0x118
[<80003358>] ret_from_kernel_thread+0x14/0x1c

---[ end trace 1c9d4412bd51b53c ]---
mt7621-pci 1e140000.pcie: error -19: failed to map resource [io  0x1e160000-0x1e16ffff]

Since there are architectures (like MIPS ralink) that can request I/O
resources from device tree but have not mapeable I/O space and also PCI_IOBASE
not defined, avoid this warning and just return zero to make the I/O ranges
assignment work.

Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
---
 drivers/pci/pci.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index aacf575c15cf..10bb2191f376 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -4102,9 +4102,9 @@ unsigned long __weak pci_address_to_pio(phys_addr_t address)
  * @phys_addr: physical address of range to be mapped
  *
  * Remap the memory mapped I/O space described by the @res and the CPU
- * physical address @phys_addr into virtual address space.  Only
- * architectures that have memory mapped IO functions defined (and the
- * PCI_IOBASE value defined) should call this function.
+ * physical address @phys_addr into virtual address space. There
+ * are architectures that don't define PCI_IOBASE but can have not
+ * mapeable IO space. Return zero for those cases.
  */
 int pci_remap_iospace(const struct resource *res, phys_addr_t phys_addr)
 {
@@ -4122,10 +4122,10 @@ int pci_remap_iospace(const struct resource *res, phys_addr_t phys_addr)
 #else
 	/*
 	 * This architecture does not have memory mapped I/O space,
-	 * so this function should never be called
+	 * but can have not mapeable I/O space, so just return ok
+	 * here.
 	 */
-	WARN_ONCE(1, "This architecture does not support memory mapped I/O\n");
-	return -ENODEV;
+	return 0;
 #endif
 }
 EXPORT_SYMBOL(pci_remap_iospace);
-- 
2.25.1

[PATCH v2 3/3] staging: mt7621-pci: set end limit for 'ioport_resource'

[Sergio Paracuellos]

We have increase IO_SPACE_LIMIT for ralink platform to get PCI IO resources
properly handled using PCI core APIs. To align those changes with driver
code we have to set 'ioport_resource' end limit to IO_SPACE_LIMIT to avoid
errors.

Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
---
 drivers/staging/mt7621-pci/pci-mt7621.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/staging/mt7621-pci/pci-mt7621.c b/drivers/staging/mt7621-pci/pci-mt7621.c
index 691030e1a5ed..6301397c3987 100644
--- a/drivers/staging/mt7621-pci/pci-mt7621.c
+++ b/drivers/staging/mt7621-pci/pci-mt7621.c
@@ -522,6 +522,8 @@ static int mt7621_pci_probe(struct platform_device *pdev)
 	if (!dev->of_node)
 		return -ENODEV;
 
+	ioport_resource.end = IO_SPACE_LIMIT;
+
 	bridge = devm_pci_alloc_host_bridge(dev, sizeof(*pcie));
 	if (!bridge)
 		return -ENOMEM;
-- 
2.25.1

Re: [PATCH v2 3/3] staging: mt7621-pci: set end limit for 'ioport_resource'

[Greg KH]

On Sun, Aug 22, 2021 at 06:10:05PM +0200, Sergio Paracuellos wrote:
> We have increase IO_SPACE_LIMIT for ralink platform to get PCI IO resources
> properly handled using PCI core APIs. To align those changes with driver
> code we have to set 'ioport_resource' end limit to IO_SPACE_LIMIT to avoid
> errors.
> 
> Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>

Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Re: [PATCH v2 3/3] staging: mt7621-pci: set end limit for 'ioport_resource'

[Sergio Paracuellos]

On Fri, Aug 27, 2021 at 11:01 AM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Sun, Aug 22, 2021 at 06:10:05PM +0200, Sergio Paracuellos wrote:
> > We have increase IO_SPACE_LIMIT for ralink platform to get PCI IO resources
> > properly handled using PCI core APIs. To align those changes with driver
> > code we have to set 'ioport_resource' end limit to IO_SPACE_LIMIT to avoid
> > errors.
> >
> > Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
>
> Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Thanks. Since I am planning to move 'mt7621-pci' from staging to
'drivers/pci/controller' and send v3 after the next merge window, I
prefer this patch to go through the staging tree. For the other two I
don't have any preference and it is ok for me to go through mips or
pci trees. So, Bjorn and Thomas is up to you if you are ok with the
changes.

Thanks in advance for your time.

Best regards,
    Sergio Paracuellos

Re: [PATCH v2 3/3] staging: mt7621-pci: set end limit for 'ioport_resource'

[Greg KH]

On Sun, Aug 29, 2021 at 05:14:27PM +0200, Sergio Paracuellos wrote:
> On Fri, Aug 27, 2021 at 11:01 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > On Sun, Aug 22, 2021 at 06:10:05PM +0200, Sergio Paracuellos wrote:
> > > We have increase IO_SPACE_LIMIT for ralink platform to get PCI IO resources
> > > properly handled using PCI core APIs. To align those changes with driver
> > > code we have to set 'ioport_resource' end limit to IO_SPACE_LIMIT to avoid
> > > errors.
> > >
> > > Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
> >
> > Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> 
> Thanks. Since I am planning to move 'mt7621-pci' from staging to
> 'drivers/pci/controller' and send v3 after the next merge window, I
> prefer this patch to go through the staging tree. For the other two I
> don't have any preference and it is ok for me to go through mips or
> pci trees. So, Bjorn and Thomas is up to you if you are ok with the
> changes.

Yes, I would need acks for the other patches in the series if this is to
come through the staging tree.

thanks,

greg k-h

Re: [PATCH v2 3/3] staging: mt7621-pci: set end limit for 'ioport_resource'

[Sergio Paracuellos]

On Thu, Sep 2, 2021 at 11:15 AM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Sun, Aug 29, 2021 at 05:14:27PM +0200, Sergio Paracuellos wrote:
> > On Fri, Aug 27, 2021 at 11:01 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> > >
> > > On Sun, Aug 22, 2021 at 06:10:05PM +0200, Sergio Paracuellos wrote:
> > > > We have increase IO_SPACE_LIMIT for ralink platform to get PCI IO resources
> > > > properly handled using PCI core APIs. To align those changes with driver
> > > > code we have to set 'ioport_resource' end limit to IO_SPACE_LIMIT to avoid
> > > > errors.
> > > >
> > > > Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
> > >
> > > Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> >
> > Thanks. Since I am planning to move 'mt7621-pci' from staging to
> > 'drivers/pci/controller' and send v3 after the next merge window, I
> > prefer this patch to go through the staging tree. For the other two I
> > don't have any preference and it is ok for me to go through mips or
> > pci trees. So, Bjorn and Thomas is up to you if you are ok with the
> > changes.
>
> Yes, I would need acks for the other patches in the series if this is to
> come through the staging tree.

Yes, I know it. Let's wait for Thomas and Bjorn preference for those
remaining two.

Best regards,
    Sergio Paracuellos
>
> thanks,
>
> greg k-h

Re: [PATCH v2 1/3] MIPS: ralink: don't define PC_IOBASE but increase IO_SPACE_LIMIT

[Thomas Bogendoerfer]

On Sun, Aug 22, 2021 at 06:10:03PM +0200, Sergio Paracuellos wrote:
> Defining PCI_IOBASE results in pci resource handling working but the
> addresses generated for IO accesses are wrong since the ioremap in the pci core
> function 'pci_parse_request_of_pci_ranges' tries to remap to a fixed virtual
> address (PC_IOBASE) which can't work for KSEG1 addresses. To get it working this
> way, we would need to put PCI_IOBASE somewhere into KSEG2 which will result in
> creating TLB entries for IO addresses, which most of the time isn't needed on
> MIPS because of access via KSEG1. So avoid to define PCI_IOBASE and increase
> IO_SPACE_LIMIT resource for ralink MIPS platform instead, to get valid IO
> addresses for resources from pci core 'pci_address_to_pio' function.
> 
> Fixes: 222b27713d7f ("MIPS: ralink: Define PCI_IOBASE)
> Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
> ---
>  arch/mips/include/asm/mach-ralink/spaces.h | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/arch/mips/include/asm/mach-ralink/spaces.h b/arch/mips/include/asm/mach-ralink/spaces.h
> index 87d085c9ad61..31a3525213cf 100644
> --- a/arch/mips/include/asm/mach-ralink/spaces.h
> +++ b/arch/mips/include/asm/mach-ralink/spaces.h
> @@ -2,9 +2,7 @@
>  #ifndef __ASM_MACH_RALINK_SPACES_H_
>  #define __ASM_MACH_RALINK_SPACES_H_
>  
> -#define PCI_IOBASE	_AC(0xa0000000, UL)
> -#define PCI_IOSIZE	SZ_16M
> -#define IO_SPACE_LIMIT	(PCI_IOSIZE - 1)
> +#define IO_SPACE_LIMIT	0x1fffffff
>  
>  #include <asm/mach-generic/spaces.h>
>  #endif
> -- 
> 2.25.1


Acked-by: Thomas Bogendoerfer <tsbogend@alpha.franken.de>

-- 
Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea.                                                [ RFC1925, 2.3 ]

Re: [PATCH v2 3/3] staging: mt7621-pci: set end limit for 'ioport_resource'

[Thomas Bogendoerfer]

On Thu, Sep 02, 2021 at 12:15:12PM +0200, Sergio Paracuellos wrote:
> On Thu, Sep 2, 2021 at 11:15 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > On Sun, Aug 29, 2021 at 05:14:27PM +0200, Sergio Paracuellos wrote:
> > > On Fri, Aug 27, 2021 at 11:01 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> > > >
> > > > On Sun, Aug 22, 2021 at 06:10:05PM +0200, Sergio Paracuellos wrote:
> > > > > We have increase IO_SPACE_LIMIT for ralink platform to get PCI IO resources
> > > > > properly handled using PCI core APIs. To align those changes with driver
> > > > > code we have to set 'ioport_resource' end limit to IO_SPACE_LIMIT to avoid
> > > > > errors.
> > > > >
> > > > > Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
> > > >
> > > > Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > >
> > > Thanks. Since I am planning to move 'mt7621-pci' from staging to
> > > 'drivers/pci/controller' and send v3 after the next merge window, I
> > > prefer this patch to go through the staging tree. For the other two I
> > > don't have any preference and it is ok for me to go through mips or
> > > pci trees. So, Bjorn and Thomas is up to you if you are ok with the
> > > changes.
> >
> > Yes, I would need acks for the other patches in the series if this is to
> > come through the staging tree.
> 
> Yes, I know it. Let's wait for Thomas and Bjorn preference for those
> remaining two.

I've sent my acked-by for the MIPS patch. 

Thomas.

-- 
Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea.                                                [ RFC1925, 2.3 ]

Re: [PATCH v2 3/3] staging: mt7621-pci: set end limit for 'ioport_resource'

[Sergio Paracuellos]

On Thu, Sep 2, 2021 at 1:08 PM Thomas Bogendoerfer
<tsbogend@alpha.franken.de> wrote:
>
> On Thu, Sep 02, 2021 at 12:15:12PM +0200, Sergio Paracuellos wrote:
> > On Thu, Sep 2, 2021 at 11:15 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> > >
> > > On Sun, Aug 29, 2021 at 05:14:27PM +0200, Sergio Paracuellos wrote:
> > > > On Fri, Aug 27, 2021 at 11:01 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> > > > >
> > > > > On Sun, Aug 22, 2021 at 06:10:05PM +0200, Sergio Paracuellos wrote:
> > > > > > We have increase IO_SPACE_LIMIT for ralink platform to get PCI IO resources
> > > > > > properly handled using PCI core APIs. To align those changes with driver
> > > > > > code we have to set 'ioport_resource' end limit to IO_SPACE_LIMIT to avoid
> > > > > > errors.
> > > > > >
> > > > > > Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
> > > > >
> > > > > Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > >
> > > > Thanks. Since I am planning to move 'mt7621-pci' from staging to
> > > > 'drivers/pci/controller' and send v3 after the next merge window, I
> > > > prefer this patch to go through the staging tree. For the other two I
> > > > don't have any preference and it is ok for me to go through mips or
> > > > pci trees. So, Bjorn and Thomas is up to you if you are ok with the
> > > > changes.
> > >
> > > Yes, I would need acks for the other patches in the series if this is to
> > > come through the staging tree.
> >
> > Yes, I know it. Let's wait for Thomas and Bjorn preference for those
> > remaining two.
>
> I've sent my acked-by for the MIPS patch.

Thanks!

>
> Thomas.
>
> --
> Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
> good idea.                                                [ RFC1925, 2.3 ]

Re: [PATCH v2 2/3] PCI: fix 'pci_remap_iospace' for architectures with PCI_IOBASE not defined

[Sergio Paracuellos]

On Sun, Aug 22, 2021 at 6:10 PM Sergio Paracuellos
<sergio.paracuellos@gmail.com> wrote:
>
> Request for I/O resources from device tree call 'pci_remap_iospace' from
> 'devm_pci_remap_iospace' which is also called from device tree function
> 'pci_parse_request_of_pci_ranges'. if PCI_IOBASE is not defined and I/O
> resources are requested the following warning appears:
>
> ------------[ cut here ]------------
> WARNING: CPU: 2 PID: 1 at ../drivers/pci/pci.c:4066 pci_remap_iospace+0x3c/0x54
> This architecture does not support memory mapped I/O
> Modules linked in:
> CPU: 2 PID: 1 Comm: swapper/0 Not tainted 5.10.1+ #1228
> Stack : 00000000 00000000 807fa974 00000000 827ffa80 80066b48 80710000 0000000b
>         00000000 00000000 81c59aac 7d06ddec 80850000 00000001 81c59a40 7d06ddec
>         00000000 00000000 807c909c 81c598f0 00000001 81c59904 00000000 0000000a
>         203a6d6d 80708880 0000000f 70617773 80850000 00000000 00000000 807d0000
>         807ffecc 1e160000 00000001 00000200 00000000 8054e920 00000008 815e0008
>         ...
> Call Trace:
> [<80008efc>] show_stack+0x8c/0x130
> [<806e1674>] dump_stack+0x9c/0xc8
> [<80024a3c>] __warn+0xc0/0xe8
> [<80024ad0>] warn_slowpath_fmt+0x6c/0xbc
> [<80410ca8>] pci_remap_iospace+0x3c/0x54
> [<80410d20>] devm_pci_remap_iospace+0x58/0xa4
> [<8042019c>] devm_of_pci_bridge_init+0x4dc/0x55c
> [<80408de8>] devm_pci_alloc_host_bridge+0x78/0x88
> [<80424e44>] mt7621_pci_probe+0x68/0x9a4
> [<80464804>] platform_drv_probe+0x40/0x7c
> [<804628bc>] really_probe+0x2fc/0x4e4
> [<80463214>] device_driver_attach+0x4c/0x74
> [<80463384>] __driver_attach+0x148/0x150
> [<8046047c>] bus_for_each_dev+0x6c/0xb0
> [<804614dc>] bus_add_driver+0x1b4/0x1fc
> [<80463aa0>] driver_register+0xd0/0x110
> [<80001714>] do_one_initcall+0x84/0x1c0
> [<808e7fd0>] kernel_init_freeable+0x214/0x24c
> [<806e4164>] kernel_init+0x14/0x118
> [<80003358>] ret_from_kernel_thread+0x14/0x1c
>
> ---[ end trace 1c9d4412bd51b53c ]---
> mt7621-pci 1e140000.pcie: error -19: failed to map resource [io  0x1e160000-0x1e16ffff]
>
> Since there are architectures (like MIPS ralink) that can request I/O
> resources from device tree but have not mapeable I/O space and also PCI_IOBASE
> not defined, avoid this warning and just return zero to make the I/O ranges
> assignment work.
>
> Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
> ---
>  drivers/pci/pci.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)

Hi Bjorn,

Any comments on this patch? Are you ok with this one also going
through the staging tree like the other two patches in the series?

Thanks in advance for your time.

Best regards,
    Sergio Paracuellos

>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index aacf575c15cf..10bb2191f376 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -4102,9 +4102,9 @@ unsigned long __weak pci_address_to_pio(phys_addr_t address)
>   * @phys_addr: physical address of range to be mapped
>   *
>   * Remap the memory mapped I/O space described by the @res and the CPU
> - * physical address @phys_addr into virtual address space.  Only
> - * architectures that have memory mapped IO functions defined (and the
> - * PCI_IOBASE value defined) should call this function.
> + * physical address @phys_addr into virtual address space. There
> + * are architectures that don't define PCI_IOBASE but can have not
> + * mapeable IO space. Return zero for those cases.
>   */
>  int pci_remap_iospace(const struct resource *res, phys_addr_t phys_addr)
>  {
> @@ -4122,10 +4122,10 @@ int pci_remap_iospace(const struct resource *res, phys_addr_t phys_addr)
>  #else
>         /*
>          * This architecture does not have memory mapped I/O space,
> -        * so this function should never be called
> +        * but can have not mapeable I/O space, so just return ok
> +        * here.
>          */
> -       WARN_ONCE(1, "This architecture does not support memory mapped I/O\n");
> -       return -ENODEV;
> +       return 0;
>  #endif
>  }
>  EXPORT_SYMBOL(pci_remap_iospace);
> --
> 2.25.1
>

Re: [PATCH v2 3/3] staging: mt7621-pci: set end limit for 'ioport_resource'

[Greg KH]

On Thu, Sep 02, 2021 at 01:19:53PM +0200, Sergio Paracuellos wrote:
> On Thu, Sep 2, 2021 at 1:08 PM Thomas Bogendoerfer
> <tsbogend@alpha.franken.de> wrote:
> >
> > On Thu, Sep 02, 2021 at 12:15:12PM +0200, Sergio Paracuellos wrote:
> > > On Thu, Sep 2, 2021 at 11:15 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> > > >
> > > > On Sun, Aug 29, 2021 at 05:14:27PM +0200, Sergio Paracuellos wrote:
> > > > > On Fri, Aug 27, 2021 at 11:01 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> > > > > >
> > > > > > On Sun, Aug 22, 2021 at 06:10:05PM +0200, Sergio Paracuellos wrote:
> > > > > > > We have increase IO_SPACE_LIMIT for ralink platform to get PCI IO resources
> > > > > > > properly handled using PCI core APIs. To align those changes with driver
> > > > > > > code we have to set 'ioport_resource' end limit to IO_SPACE_LIMIT to avoid
> > > > > > > errors.
> > > > > > >
> > > > > > > Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
> > > > > >
> > > > > > Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > > >
> > > > > Thanks. Since I am planning to move 'mt7621-pci' from staging to
> > > > > 'drivers/pci/controller' and send v3 after the next merge window, I
> > > > > prefer this patch to go through the staging tree. For the other two I
> > > > > don't have any preference and it is ok for me to go through mips or
> > > > > pci trees. So, Bjorn and Thomas is up to you if you are ok with the
> > > > > changes.
> > > >
> > > > Yes, I would need acks for the other patches in the series if this is to
> > > > come through the staging tree.
> > >
> > > Yes, I know it. Let's wait for Thomas and Bjorn preference for those
> > > remaining two.
> >
> > I've sent my acked-by for the MIPS patch.
> 
> Thanks!

Ok, I took patches 1 and 3 in my tree now.  Please submit patch 2 to the
PCI developers and maintainer, as that is up to them to take, not me.

thanks,

greg k-h

Re: [PATCH v2 3/3] staging: mt7621-pci: set end limit for 'ioport_resource'

[Sergio Paracuellos]

On Tue, Sep 21, 2021 at 5:28 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Thu, Sep 02, 2021 at 01:19:53PM +0200, Sergio Paracuellos wrote:
> > On Thu, Sep 2, 2021 at 1:08 PM Thomas Bogendoerfer
> > <tsbogend@alpha.franken.de> wrote:
> > >
> > > On Thu, Sep 02, 2021 at 12:15:12PM +0200, Sergio Paracuellos wrote:
> > > > On Thu, Sep 2, 2021 at 11:15 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> > > > >
> > > > > On Sun, Aug 29, 2021 at 05:14:27PM +0200, Sergio Paracuellos wrote:
> > > > > > On Fri, Aug 27, 2021 at 11:01 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> > > > > > >
> > > > > > > On Sun, Aug 22, 2021 at 06:10:05PM +0200, Sergio Paracuellos wrote:
> > > > > > > > We have increase IO_SPACE_LIMIT for ralink platform to get PCI IO resources
> > > > > > > > properly handled using PCI core APIs. To align those changes with driver
> > > > > > > > code we have to set 'ioport_resource' end limit to IO_SPACE_LIMIT to avoid
> > > > > > > > errors.
> > > > > > > >
> > > > > > > > Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
> > > > > > >
> > > > > > > Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > > > >
> > > > > > Thanks. Since I am planning to move 'mt7621-pci' from staging to
> > > > > > 'drivers/pci/controller' and send v3 after the next merge window, I
> > > > > > prefer this patch to go through the staging tree. For the other two I
> > > > > > don't have any preference and it is ok for me to go through mips or
> > > > > > pci trees. So, Bjorn and Thomas is up to you if you are ok with the
> > > > > > changes.
> > > > >
> > > > > Yes, I would need acks for the other patches in the series if this is to
> > > > > come through the staging tree.
> > > >
> > > > Yes, I know it. Let's wait for Thomas and Bjorn preference for those
> > > > remaining two.
> > >
> > > I've sent my acked-by for the MIPS patch.
> >
> > Thanks!
>
> Ok, I took patches 1 and 3 in my tree now.  Please submit patch 2 to the
> PCI developers and maintainer, as that is up to them to take, not me.

Ok, thanks. I will resend the remaining patch if that is needed. Only
one concern here, only with those two patches applied the driver is
totally broken since it needs the remaining PATCH to make all the pci
subsystem work. Is this ok?

Best regards,
    Sergio Paracuellos
>
> thanks,
>
> greg k-h

Re: [PATCH v2 3/3] staging: mt7621-pci: set end limit for 'ioport_resource'

[Greg KH]

On Tue, Sep 21, 2021 at 07:02:55PM +0200, Sergio Paracuellos wrote:
> On Tue, Sep 21, 2021 at 5:28 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > On Thu, Sep 02, 2021 at 01:19:53PM +0200, Sergio Paracuellos wrote:
> > > On Thu, Sep 2, 2021 at 1:08 PM Thomas Bogendoerfer
> > > <tsbogend@alpha.franken.de> wrote:
> > > >
> > > > On Thu, Sep 02, 2021 at 12:15:12PM +0200, Sergio Paracuellos wrote:
> > > > > On Thu, Sep 2, 2021 at 11:15 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> > > > > >
> > > > > > On Sun, Aug 29, 2021 at 05:14:27PM +0200, Sergio Paracuellos wrote:
> > > > > > > On Fri, Aug 27, 2021 at 11:01 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> > > > > > > >
> > > > > > > > On Sun, Aug 22, 2021 at 06:10:05PM +0200, Sergio Paracuellos wrote:
> > > > > > > > > We have increase IO_SPACE_LIMIT for ralink platform to get PCI IO resources
> > > > > > > > > properly handled using PCI core APIs. To align those changes with driver
> > > > > > > > > code we have to set 'ioport_resource' end limit to IO_SPACE_LIMIT to avoid
> > > > > > > > > errors.
> > > > > > > > >
> > > > > > > > > Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
> > > > > > > >
> > > > > > > > Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > > > > >
> > > > > > > Thanks. Since I am planning to move 'mt7621-pci' from staging to
> > > > > > > 'drivers/pci/controller' and send v3 after the next merge window, I
> > > > > > > prefer this patch to go through the staging tree. For the other two I
> > > > > > > don't have any preference and it is ok for me to go through mips or
> > > > > > > pci trees. So, Bjorn and Thomas is up to you if you are ok with the
> > > > > > > changes.
> > > > > >
> > > > > > Yes, I would need acks for the other patches in the series if this is to
> > > > > > come through the staging tree.
> > > > >
> > > > > Yes, I know it. Let's wait for Thomas and Bjorn preference for those
> > > > > remaining two.
> > > >
> > > > I've sent my acked-by for the MIPS patch.
> > >
> > > Thanks!
> >
> > Ok, I took patches 1 and 3 in my tree now.  Please submit patch 2 to the
> > PCI developers and maintainer, as that is up to them to take, not me.
> 
> Ok, thanks. I will resend the remaining patch if that is needed. Only
> one concern here, only with those two patches applied the driver is
> totally broken since it needs the remaining PATCH to make all the pci
> subsystem work. Is this ok?

Get the change acked by the PCI developers and I will be glad to also
take it through my tree :)

Re: [PATCH v2 2/3] PCI: fix 'pci_remap_iospace' for architectures with PCI_IOBASE not defined

[Bjorn Helgaas]

[+cc Liviu, Rob, Catalin, Arnd, since this warning was added by
8b921acfeffd ("PCI: Add pci_remap_iospace() to map bus I/O resources"),
https://git.kernel.org/linus/8b921acfeffd]

On Sun, Aug 22, 2021 at 06:10:04PM +0200, Sergio Paracuellos wrote:
> Request for I/O resources from device tree call 'pci_remap_iospace' from
> 'devm_pci_remap_iospace' which is also called from device tree function
> 'pci_parse_request_of_pci_ranges'. if PCI_IOBASE is not defined and I/O
> resources are requested the following warning appears:
> 
> ------------[ cut here ]------------
> WARNING: CPU: 2 PID: 1 at ../drivers/pci/pci.c:4066 pci_remap_iospace+0x3c/0x54
> This architecture does not support memory mapped I/O
> Modules linked in:
> CPU: 2 PID: 1 Comm: swapper/0 Not tainted 5.10.1+ #1228
> Stack : 00000000 00000000 807fa974 00000000 827ffa80 80066b48 80710000 0000000b
>         00000000 00000000 81c59aac 7d06ddec 80850000 00000001 81c59a40 7d06ddec
>         00000000 00000000 807c909c 81c598f0 00000001 81c59904 00000000 0000000a
>         203a6d6d 80708880 0000000f 70617773 80850000 00000000 00000000 807d0000
>         807ffecc 1e160000 00000001 00000200 00000000 8054e920 00000008 815e0008
>         ...
> Call Trace:
> [<80008efc>] show_stack+0x8c/0x130
> [<806e1674>] dump_stack+0x9c/0xc8
> [<80024a3c>] __warn+0xc0/0xe8
> [<80024ad0>] warn_slowpath_fmt+0x6c/0xbc
> [<80410ca8>] pci_remap_iospace+0x3c/0x54
> [<80410d20>] devm_pci_remap_iospace+0x58/0xa4
> [<8042019c>] devm_of_pci_bridge_init+0x4dc/0x55c
> [<80408de8>] devm_pci_alloc_host_bridge+0x78/0x88
> [<80424e44>] mt7621_pci_probe+0x68/0x9a4
> [<80464804>] platform_drv_probe+0x40/0x7c
> [<804628bc>] really_probe+0x2fc/0x4e4
> [<80463214>] device_driver_attach+0x4c/0x74
> [<80463384>] __driver_attach+0x148/0x150
> [<8046047c>] bus_for_each_dev+0x6c/0xb0
> [<804614dc>] bus_add_driver+0x1b4/0x1fc
> [<80463aa0>] driver_register+0xd0/0x110
> [<80001714>] do_one_initcall+0x84/0x1c0
> [<808e7fd0>] kernel_init_freeable+0x214/0x24c
> [<806e4164>] kernel_init+0x14/0x118
> [<80003358>] ret_from_kernel_thread+0x14/0x1c
> 
> ---[ end trace 1c9d4412bd51b53c ]---
> mt7621-pci 1e140000.pcie: error -19: failed to map resource [io  0x1e160000-0x1e16ffff]
> 
> Since there are architectures (like MIPS ralink) that can request I/O
> resources from device tree but have not mapeable I/O space and also PCI_IOBASE
> not defined, avoid this warning and just return zero to make the I/O ranges
> assignment work.
> 
> Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
> ---
>  drivers/pci/pci.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index aacf575c15cf..10bb2191f376 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -4102,9 +4102,9 @@ unsigned long __weak pci_address_to_pio(phys_addr_t address)
>   * @phys_addr: physical address of range to be mapped
>   *
>   * Remap the memory mapped I/O space described by the @res and the CPU
> - * physical address @phys_addr into virtual address space.  Only
> - * architectures that have memory mapped IO functions defined (and the
> - * PCI_IOBASE value defined) should call this function.
> + * physical address @phys_addr into virtual address space. There
> + * are architectures that don't define PCI_IOBASE but can have not
> + * mapeable IO space. Return zero for those cases.
>   */
>  int pci_remap_iospace(const struct resource *res, phys_addr_t phys_addr)
>  {
> @@ -4122,10 +4122,10 @@ int pci_remap_iospace(const struct resource *res, phys_addr_t phys_addr)
>  #else
>  	/*
>  	 * This architecture does not have memory mapped I/O space,
> -	 * so this function should never be called
> +	 * but can have not mapeable I/O space, so just return ok
> +	 * here.
>  	 */
> -	WARN_ONCE(1, "This architecture does not support memory mapped I/O\n");
> -	return -ENODEV;
> +	return 0;

This doesn't seem right to me.  pci_remap_iospace() remaps
memory-mapped I/O space into virtual address space.

If the architecture doesn't support that remapping, we shouldn't claim
that it succeeded.

The analogous path for ACPI is in acpi_pci_root_remap_iospace(), where
we only call pci_remap_iospace() if PCI_IOBASE is defined.  Maybe we
should use the same approach here, i.e., add the corresonding #ifdef
in pci_parse_request_of_pci_ranges()?

>  #endif
>  }
>  EXPORT_SYMBOL(pci_remap_iospace);
> -- 
> 2.25.1
> 

Re: [PATCH v2 2/3] PCI: fix 'pci_remap_iospace' for architectures with PCI_IOBASE not defined

[Sergio Paracuellos]

Hi Bjorn,

On Tue, Sep 21, 2021 at 7:49 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> [+cc Liviu, Rob, Catalin, Arnd, since this warning was added by
> 8b921acfeffd ("PCI: Add pci_remap_iospace() to map bus I/O resources"),
> https://git.kernel.org/linus/8b921acfeffd]
>
> On Sun, Aug 22, 2021 at 06:10:04PM +0200, Sergio Paracuellos wrote:
> > Request for I/O resources from device tree call 'pci_remap_iospace' from
> > 'devm_pci_remap_iospace' which is also called from device tree function
> > 'pci_parse_request_of_pci_ranges'. if PCI_IOBASE is not defined and I/O
> > resources are requested the following warning appears:
> >
> > ------------[ cut here ]------------
> > WARNING: CPU: 2 PID: 1 at ../drivers/pci/pci.c:4066 pci_remap_iospace+0x3c/0x54
> > This architecture does not support memory mapped I/O
> > Modules linked in:
> > CPU: 2 PID: 1 Comm: swapper/0 Not tainted 5.10.1+ #1228
> > Stack : 00000000 00000000 807fa974 00000000 827ffa80 80066b48 80710000 0000000b
> >         00000000 00000000 81c59aac 7d06ddec 80850000 00000001 81c59a40 7d06ddec
> >         00000000 00000000 807c909c 81c598f0 00000001 81c59904 00000000 0000000a
> >         203a6d6d 80708880 0000000f 70617773 80850000 00000000 00000000 807d0000
> >         807ffecc 1e160000 00000001 00000200 00000000 8054e920 00000008 815e0008
> >         ...
> > Call Trace:
> > [<80008efc>] show_stack+0x8c/0x130
> > [<806e1674>] dump_stack+0x9c/0xc8
> > [<80024a3c>] __warn+0xc0/0xe8
> > [<80024ad0>] warn_slowpath_fmt+0x6c/0xbc
> > [<80410ca8>] pci_remap_iospace+0x3c/0x54
> > [<80410d20>] devm_pci_remap_iospace+0x58/0xa4
> > [<8042019c>] devm_of_pci_bridge_init+0x4dc/0x55c
> > [<80408de8>] devm_pci_alloc_host_bridge+0x78/0x88
> > [<80424e44>] mt7621_pci_probe+0x68/0x9a4
> > [<80464804>] platform_drv_probe+0x40/0x7c
> > [<804628bc>] really_probe+0x2fc/0x4e4
> > [<80463214>] device_driver_attach+0x4c/0x74
> > [<80463384>] __driver_attach+0x148/0x150
> > [<8046047c>] bus_for_each_dev+0x6c/0xb0
> > [<804614dc>] bus_add_driver+0x1b4/0x1fc
> > [<80463aa0>] driver_register+0xd0/0x110
> > [<80001714>] do_one_initcall+0x84/0x1c0
> > [<808e7fd0>] kernel_init_freeable+0x214/0x24c
> > [<806e4164>] kernel_init+0x14/0x118
> > [<80003358>] ret_from_kernel_thread+0x14/0x1c
> >
> > ---[ end trace 1c9d4412bd51b53c ]---
> > mt7621-pci 1e140000.pcie: error -19: failed to map resource [io  0x1e160000-0x1e16ffff]
> >
> > Since there are architectures (like MIPS ralink) that can request I/O
> > resources from device tree but have not mapeable I/O space and also PCI_IOBASE
> > not defined, avoid this warning and just return zero to make the I/O ranges
> > assignment work.
> >
> > Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
> > ---
> >  drivers/pci/pci.c | 12 ++++++------
> >  1 file changed, 6 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > index aacf575c15cf..10bb2191f376 100644
> > --- a/drivers/pci/pci.c
> > +++ b/drivers/pci/pci.c
> > @@ -4102,9 +4102,9 @@ unsigned long __weak pci_address_to_pio(phys_addr_t address)
> >   * @phys_addr: physical address of range to be mapped
> >   *
> >   * Remap the memory mapped I/O space described by the @res and the CPU
> > - * physical address @phys_addr into virtual address space.  Only
> > - * architectures that have memory mapped IO functions defined (and the
> > - * PCI_IOBASE value defined) should call this function.
> > + * physical address @phys_addr into virtual address space. There
> > + * are architectures that don't define PCI_IOBASE but can have not
> > + * mapeable IO space. Return zero for those cases.
> >   */
> >  int pci_remap_iospace(const struct resource *res, phys_addr_t phys_addr)
> >  {
> > @@ -4122,10 +4122,10 @@ int pci_remap_iospace(const struct resource *res, phys_addr_t phys_addr)
> >  #else
> >       /*
> >        * This architecture does not have memory mapped I/O space,
> > -      * so this function should never be called
> > +      * but can have not mapeable I/O space, so just return ok
> > +      * here.
> >        */
> > -     WARN_ONCE(1, "This architecture does not support memory mapped I/O\n");
> > -     return -ENODEV;
> > +     return 0;
>
> This doesn't seem right to me.  pci_remap_iospace() remaps
> memory-mapped I/O space into virtual address space.
>
> If the architecture doesn't support that remapping, we shouldn't claim
> that it succeeded.
>
> The analogous path for ACPI is in acpi_pci_root_remap_iospace(), where
> we only call pci_remap_iospace() if PCI_IOBASE is defined.  Maybe we
> should use the same approach here, i.e., add the corresonding #ifdef
> in pci_parse_request_of_pci_ranges()?

This approach you are talking here is what I did in v1 of this series
[0] and you tell me to properly fix that in the 'pci_remap_iospace'.
Both of these v1 and v2 obviously end up in a working pci system for
mt7621 ralink.
Another option I guess would be to mark 'pci_remap_iospace' as weak
and redefine the symbol for ralink but since it is only for one simple
return zero maybe it makes no sense at all... Please, tell me the
correct approach to properly fix this and make things work for this
platform.

Thanks in advance for your time,
     Sergio Paracuellos

[0]: https://lore.kernel.org/linux-pci/20210807072409.9018-2-sergio.paracuellos@gmail.com/T/#m73c19c3b72fdf8c63e6d3fe8dc80aeee4e4adcaa

>
> >  #endif
> >  }
> >  EXPORT_SYMBOL(pci_remap_iospace);
> > --
> > 2.25.1
> >

Re: [PATCH v2 2/3] PCI: fix 'pci_remap_iospace' for architectures with PCI_IOBASE not defined

[Bjorn Helgaas]

If you repost this, please update the subject line like:

  PCI: of: Avoid pci_remap_iospace() when PCI_IOBASE not defined

On Tue, Sep 21, 2021 at 08:36:07PM +0200, Sergio Paracuellos wrote:
> On Tue, Sep 21, 2021 at 7:49 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> >
> > [+cc Liviu, Rob, Catalin, Arnd, since this warning was added by
> > 8b921acfeffd ("PCI: Add pci_remap_iospace() to map bus I/O resources"),
> > https://git.kernel.org/linus/8b921acfeffd]
> >
> > On Sun, Aug 22, 2021 at 06:10:04PM +0200, Sergio Paracuellos wrote:
> > > Request for I/O resources from device tree call 'pci_remap_iospace' from
> > > 'devm_pci_remap_iospace' which is also called from device tree function
> > > 'pci_parse_request_of_pci_ranges'. if PCI_IOBASE is not defined and I/O
> > > resources are requested the following warning appears:

s/'pci_remap_iospace'/pci_remap_iospace()/
s/'devm_pci_remap_iospace'/devm_pci_remap_iospace()/
s/'pci_parse_request_of_pci_ranges'/pci_parse_request_of_pci_ranges()/
s/if PCI_IOBASE/If PCI_IOBASE/

I think you can trim the stacktrace below and keep just this:

  WARNING: CPU: 2 PID: 1 at ../drivers/pci/pci.c:4066 pci_remap_iospace+0x3c/0x54
  This architecture does not support memory mapped I/O
  mt7621-pci 1e140000.pcie: error -19: failed to map resource [io  0x1e160000-0x1e16ffff]

> > >
> > > ------------[ cut here ]------------
> > > WARNING: CPU: 2 PID: 1 at ../drivers/pci/pci.c:4066 pci_remap_iospace+0x3c/0x54
> > > This architecture does not support memory mapped I/O
> > > Modules linked in:
> > > CPU: 2 PID: 1 Comm: swapper/0 Not tainted 5.10.1+ #1228
> > > Stack : 00000000 00000000 807fa974 00000000 827ffa80 80066b48 80710000 0000000b
> > >         00000000 00000000 81c59aac 7d06ddec 80850000 00000001 81c59a40 7d06ddec
> > >         00000000 00000000 807c909c 81c598f0 00000001 81c59904 00000000 0000000a
> > >         203a6d6d 80708880 0000000f 70617773 80850000 00000000 00000000 807d0000
> > >         807ffecc 1e160000 00000001 00000200 00000000 8054e920 00000008 815e0008
> > >         ...
> > > Call Trace:
> > > [<80008efc>] show_stack+0x8c/0x130
> > > [<806e1674>] dump_stack+0x9c/0xc8
> > > [<80024a3c>] __warn+0xc0/0xe8
> > > [<80024ad0>] warn_slowpath_fmt+0x6c/0xbc
> > > [<80410ca8>] pci_remap_iospace+0x3c/0x54
> > > [<80410d20>] devm_pci_remap_iospace+0x58/0xa4
> > > [<8042019c>] devm_of_pci_bridge_init+0x4dc/0x55c
> > > [<80408de8>] devm_pci_alloc_host_bridge+0x78/0x88
> > > [<80424e44>] mt7621_pci_probe+0x68/0x9a4
> > > [<80464804>] platform_drv_probe+0x40/0x7c
> > > [<804628bc>] really_probe+0x2fc/0x4e4
> > > [<80463214>] device_driver_attach+0x4c/0x74
> > > [<80463384>] __driver_attach+0x148/0x150
> > > [<8046047c>] bus_for_each_dev+0x6c/0xb0
> > > [<804614dc>] bus_add_driver+0x1b4/0x1fc
> > > [<80463aa0>] driver_register+0xd0/0x110
> > > [<80001714>] do_one_initcall+0x84/0x1c0
> > > [<808e7fd0>] kernel_init_freeable+0x214/0x24c
> > > [<806e4164>] kernel_init+0x14/0x118
> > > [<80003358>] ret_from_kernel_thread+0x14/0x1c
> > >
> > > ---[ end trace 1c9d4412bd51b53c ]---
> > > mt7621-pci 1e140000.pcie: error -19: failed to map resource [io  0x1e160000-0x1e16ffff]
> > >
> > > Since there are architectures (like MIPS ralink) that can request I/O
> > > resources from device tree but have not mapeable I/O space and also PCI_IOBASE
> > > not defined, avoid this warning and just return zero to make the I/O ranges
> > > assignment work.

s/have not mapeable I/O space and also PCI_IOBASE not defined/
  don't have mappable I/O space and therefore don't define PCI_IOBASE/
s/avoid this warning .../
  avoid calling devm_pci_remap_iospace() in that case/

> > > Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
> > > ---
> > >  drivers/pci/pci.c | 12 ++++++------
> > >  1 file changed, 6 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > > index aacf575c15cf..10bb2191f376 100644
> > > --- a/drivers/pci/pci.c
> > > +++ b/drivers/pci/pci.c
> > > @@ -4102,9 +4102,9 @@ unsigned long __weak pci_address_to_pio(phys_addr_t address)
> > >   * @phys_addr: physical address of range to be mapped
> > >   *
> > >   * Remap the memory mapped I/O space described by the @res and the CPU
> > > - * physical address @phys_addr into virtual address space.  Only
> > > - * architectures that have memory mapped IO functions defined (and the
> > > - * PCI_IOBASE value defined) should call this function.
> > > + * physical address @phys_addr into virtual address space. There
> > > + * are architectures that don't define PCI_IOBASE but can have not
> > > + * mapeable IO space. Return zero for those cases.
> > >   */
> > >  int pci_remap_iospace(const struct resource *res, phys_addr_t phys_addr)
> > >  {
> > > @@ -4122,10 +4122,10 @@ int pci_remap_iospace(const struct resource *res, phys_addr_t phys_addr)
> > >  #else
> > >       /*
> > >        * This architecture does not have memory mapped I/O space,
> > > -      * so this function should never be called
> > > +      * but can have not mapeable I/O space, so just return ok
> > > +      * here.
> > >        */
> > > -     WARN_ONCE(1, "This architecture does not support memory mapped I/O\n");
> > > -     return -ENODEV;
> > > +     return 0;
> >
> > This doesn't seem right to me.  pci_remap_iospace() remaps
> > memory-mapped I/O space into virtual address space.
> >
> > If the architecture doesn't support that remapping, we shouldn't claim
> > that it succeeded.
> >
> > The analogous path for ACPI is in acpi_pci_root_remap_iospace(), where
> > we only call pci_remap_iospace() if PCI_IOBASE is defined.  Maybe we
> > should use the same approach here, i.e., add the corresonding #ifdef
> > in pci_parse_request_of_pci_ranges()?
> 
> This approach you are talking here is what I did in v1 of this series
> [0] and you tell me to properly fix that in the 'pci_remap_iospace'.

Hmm, sorry, you're absolutely right.

I guess I was thinking that pci_remap_iospace() was broken because it
returns -ENODEV when the arch can't remap I/O port space into virtual
memory space.  But that doesn't seem right.  If the arch literally
cannot do that remapping, why should pci_remap_iospace() return
success and basically pretend that it can?

And the acpi_pci_root_remap_iospace() case is doing exactly the same
thing, and we live with the #ifdef there.  So I think I was just wrong
before, and we should make the DT case similar by adding the #ifdef
where you initially put it, in pci_parse_request_of_pci_ranges().

> Both of these v1 and v2 obviously end up in a working pci system for
> mt7621 ralink.
> Another option I guess would be to mark 'pci_remap_iospace' as weak
> and redefine the symbol for ralink but since it is only for one simple
> return zero maybe it makes no sense at all... Please, tell me the
> correct approach to properly fix this and make things work for this
> platform.

Making pci_remap_iospace() weak could work (and I think it was
originally that way), but seems overkill to me, unless you want a way
to make generic mips kernels that work both on ralink and other mips
platforms.

> Thanks in advance for your time,
>      Sergio Paracuellos
> 
> [0]: https://lore.kernel.org/linux-pci/20210807072409.9018-2-sergio.paracuellos@gmail.com/T/#m73c19c3b72fdf8c63e6d3fe8dc80aeee4e4adcaa
> 
> >
> > >  #endif
> > >  }
> > >  EXPORT_SYMBOL(pci_remap_iospace);
> > > --
> > > 2.25.1
> > >

Re: [PATCH v2 2/3] PCI: fix 'pci_remap_iospace' for architectures with PCI_IOBASE not defined

[Sergio Paracuellos]

Hi Bjorn,

On Tue, Sep 21, 2021 at 9:38 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> If you repost this, please update the subject line like:
>
>   PCI: of: Avoid pci_remap_iospace() when PCI_IOBASE not defined
>
> On Tue, Sep 21, 2021 at 08:36:07PM +0200, Sergio Paracuellos wrote:
> > On Tue, Sep 21, 2021 at 7:49 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > >
> > > [+cc Liviu, Rob, Catalin, Arnd, since this warning was added by
> > > 8b921acfeffd ("PCI: Add pci_remap_iospace() to map bus I/O resources"),
> > > https://git.kernel.org/linus/8b921acfeffd]
> > >
> > > On Sun, Aug 22, 2021 at 06:10:04PM +0200, Sergio Paracuellos wrote:
> > > > Request for I/O resources from device tree call 'pci_remap_iospace' from
> > > > 'devm_pci_remap_iospace' which is also called from device tree function
> > > > 'pci_parse_request_of_pci_ranges'. if PCI_IOBASE is not defined and I/O
> > > > resources are requested the following warning appears:
>
> s/'pci_remap_iospace'/pci_remap_iospace()/
> s/'devm_pci_remap_iospace'/devm_pci_remap_iospace()/
> s/'pci_parse_request_of_pci_ranges'/pci_parse_request_of_pci_ranges()/
> s/if PCI_IOBASE/If PCI_IOBASE/
>
> I think you can trim the stacktrace below and keep just this:
>
>   WARNING: CPU: 2 PID: 1 at ../drivers/pci/pci.c:4066 pci_remap_iospace+0x3c/0x54
>   This architecture does not support memory mapped I/O
>   mt7621-pci 1e140000.pcie: error -19: failed to map resource [io  0x1e160000-0x1e16ffff]
>
> > > >
> > > > ------------[ cut here ]------------
> > > > WARNING: CPU: 2 PID: 1 at ../drivers/pci/pci.c:4066 pci_remap_iospace+0x3c/0x54
> > > > This architecture does not support memory mapped I/O
> > > > Modules linked in:
> > > > CPU: 2 PID: 1 Comm: swapper/0 Not tainted 5.10.1+ #1228
> > > > Stack : 00000000 00000000 807fa974 00000000 827ffa80 80066b48 80710000 0000000b
> > > >         00000000 00000000 81c59aac 7d06ddec 80850000 00000001 81c59a40 7d06ddec
> > > >         00000000 00000000 807c909c 81c598f0 00000001 81c59904 00000000 0000000a
> > > >         203a6d6d 80708880 0000000f 70617773 80850000 00000000 00000000 807d0000
> > > >         807ffecc 1e160000 00000001 00000200 00000000 8054e920 00000008 815e0008
> > > >         ...
> > > > Call Trace:
> > > > [<80008efc>] show_stack+0x8c/0x130
> > > > [<806e1674>] dump_stack+0x9c/0xc8
> > > > [<80024a3c>] __warn+0xc0/0xe8
> > > > [<80024ad0>] warn_slowpath_fmt+0x6c/0xbc
> > > > [<80410ca8>] pci_remap_iospace+0x3c/0x54
> > > > [<80410d20>] devm_pci_remap_iospace+0x58/0xa4
> > > > [<8042019c>] devm_of_pci_bridge_init+0x4dc/0x55c
> > > > [<80408de8>] devm_pci_alloc_host_bridge+0x78/0x88
> > > > [<80424e44>] mt7621_pci_probe+0x68/0x9a4
> > > > [<80464804>] platform_drv_probe+0x40/0x7c
> > > > [<804628bc>] really_probe+0x2fc/0x4e4
> > > > [<80463214>] device_driver_attach+0x4c/0x74
> > > > [<80463384>] __driver_attach+0x148/0x150
> > > > [<8046047c>] bus_for_each_dev+0x6c/0xb0
> > > > [<804614dc>] bus_add_driver+0x1b4/0x1fc
> > > > [<80463aa0>] driver_register+0xd0/0x110
> > > > [<80001714>] do_one_initcall+0x84/0x1c0
> > > > [<808e7fd0>] kernel_init_freeable+0x214/0x24c
> > > > [<806e4164>] kernel_init+0x14/0x118
> > > > [<80003358>] ret_from_kernel_thread+0x14/0x1c
> > > >
> > > > ---[ end trace 1c9d4412bd51b53c ]---
> > > > mt7621-pci 1e140000.pcie: error -19: failed to map resource [io  0x1e160000-0x1e16ffff]
> > > >
> > > > Since there are architectures (like MIPS ralink) that can request I/O
> > > > resources from device tree but have not mapeable I/O space and also PCI_IOBASE
> > > > not defined, avoid this warning and just return zero to make the I/O ranges
> > > > assignment work.
>
> s/have not mapeable I/O space and also PCI_IOBASE not defined/
>   don't have mappable I/O space and therefore don't define PCI_IOBASE/
> s/avoid this warning .../
>   avoid calling devm_pci_remap_iospace() in that case/
>
> > > > Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
> > > > ---
> > > >  drivers/pci/pci.c | 12 ++++++------
> > > >  1 file changed, 6 insertions(+), 6 deletions(-)
> > > >
> > > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > > > index aacf575c15cf..10bb2191f376 100644
> > > > --- a/drivers/pci/pci.c
> > > > +++ b/drivers/pci/pci.c
> > > > @@ -4102,9 +4102,9 @@ unsigned long __weak pci_address_to_pio(phys_addr_t address)
> > > >   * @phys_addr: physical address of range to be mapped
> > > >   *
> > > >   * Remap the memory mapped I/O space described by the @res and the CPU
> > > > - * physical address @phys_addr into virtual address space.  Only
> > > > - * architectures that have memory mapped IO functions defined (and the
> > > > - * PCI_IOBASE value defined) should call this function.
> > > > + * physical address @phys_addr into virtual address space. There
> > > > + * are architectures that don't define PCI_IOBASE but can have not
> > > > + * mapeable IO space. Return zero for those cases.
> > > >   */
> > > >  int pci_remap_iospace(const struct resource *res, phys_addr_t phys_addr)
> > > >  {
> > > > @@ -4122,10 +4122,10 @@ int pci_remap_iospace(const struct resource *res, phys_addr_t phys_addr)
> > > >  #else
> > > >       /*
> > > >        * This architecture does not have memory mapped I/O space,
> > > > -      * so this function should never be called
> > > > +      * but can have not mapeable I/O space, so just return ok
> > > > +      * here.
> > > >        */
> > > > -     WARN_ONCE(1, "This architecture does not support memory mapped I/O\n");
> > > > -     return -ENODEV;
> > > > +     return 0;
> > >
> > > This doesn't seem right to me.  pci_remap_iospace() remaps
> > > memory-mapped I/O space into virtual address space.
> > >
> > > If the architecture doesn't support that remapping, we shouldn't claim
> > > that it succeeded.
> > >
> > > The analogous path for ACPI is in acpi_pci_root_remap_iospace(), where
> > > we only call pci_remap_iospace() if PCI_IOBASE is defined.  Maybe we
> > > should use the same approach here, i.e., add the corresonding #ifdef
> > > in pci_parse_request_of_pci_ranges()?
> >
> > This approach you are talking here is what I did in v1 of this series
> > [0] and you tell me to properly fix that in the 'pci_remap_iospace'.
>

I have already send v3 of ths patch with all the changes in commit
message and description that you are pointing out here, thanks:

https://lore.kernel.org/linux-pci/20210922042041.16326-1-sergio.paracuellos@gmail.com/T/#u

> Hmm, sorry, you're absolutely right.
>
> I guess I was thinking that pci_remap_iospace() was broken because it
> returns -ENODEV when the arch can't remap I/O port space into virtual
> memory space.  But that doesn't seem right.  If the arch literally
> cannot do that remapping, why should pci_remap_iospace() return
> success and basically pretend that it can?

No problem at all :). I also should have asked a bit more about why to
fix things there since I thought initially it seemed to not be the
right thing to do.

>
> And the acpi_pci_root_remap_iospace() case is doing exactly the same
> thing, and we live with the #ifdef there.  So I think I was just wrong
> before, and we should make the DT case similar by adding the #ifdef
> where you initially put it, in pci_parse_request_of_pci_ranges().
>
> > Both of these v1 and v2 obviously end up in a working pci system for
> > mt7621 ralink.
> > Another option I guess would be to mark 'pci_remap_iospace' as weak
> > and redefine the symbol for ralink but since it is only for one simple
> > return zero maybe it makes no sense at all... Please, tell me the
> > correct approach to properly fix this and make things work for this
> > platform.
>
> Making pci_remap_iospace() weak could work (and I think it was
> originally that way), but seems overkill to me, unless you want a way
> to make generic mips kernels that work both on ralink and other mips
> platforms.

That is not the case and agree with you that seems overkill.

Thanks in advance for your time.

Best regards,
     Sergio Paracuellos

>
> > Thanks in advance for your time,
> >      Sergio Paracuellos
> >
> > [0]: https://lore.kernel.org/linux-pci/20210807072409.9018-2-sergio.paracuellos@gmail.com/T/#m73c19c3b72fdf8c63e6d3fe8dc80aeee4e4adcaa
> >
> > >
> > > >  #endif
> > > >  }
> > > >  EXPORT_SYMBOL(pci_remap_iospace);
> > > > --
> > > > 2.25.1
> > > >