[PATCH 0/4] MIPS: Cleanups to PCI related code

[PATCH 0/4] MIPS: Cleanups to PCI related code

[Ilpo Järvinen]

While auditing PCI config space read/write callbacks, I came across a
few places which were asking for cleanup. Thus, this cleanup series
came to be.

Ilpo Järvinen (4):
  MIPS: lantiq: Remove unused function pointer variables
  MIPS: ath79: Don't return PCIBIOS_* code from pcibios_enable_device()
  MIPS: PCI: Return PCIBIOS_* from tx4927_pci_config_read/write()
  MIPS: TXx9: Use PCI_SET_ERROR_RESPONSE()

 arch/mips/pci/fixup-ath79.c  |  2 +-
 arch/mips/pci/fixup-lantiq.c |  9 ---------
 arch/mips/pci/ops-tx4927.c   | 16 ++++++++++------
 3 files changed, 11 insertions(+), 16 deletions(-)

-- 
2.39.2

[PATCH 1/4] MIPS: lantiq: Remove unused function pointer variables

[Ilpo Järvinen]

Ever since introduction in the commit e47d488935ed ("MIPS: Lantiq: Add
PCI controller support.") ltqpci_plat_dev_init has been unused. In
57c8cb8f2429 ("MIPS: pci: convert lantiq driver to OF") also
ltq_pci_plat_arch_init was introduced.

With those commit being more than 10 years ago, it seem neither is
going to get used anytime soon. Thus, remove both unused function
pointer variables.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 arch/mips/pci/fixup-lantiq.c | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/arch/mips/pci/fixup-lantiq.c b/arch/mips/pci/fixup-lantiq.c
index 13009666204f..8bcc136976dc 100644
--- a/arch/mips/pci/fixup-lantiq.c
+++ b/arch/mips/pci/fixup-lantiq.c
@@ -7,17 +7,8 @@
 #include <linux/of_pci.h>
 #include <linux/pci.h>
 
-int (*ltq_pci_plat_arch_init)(struct pci_dev *dev) = NULL;
-int (*ltq_pci_plat_dev_init)(struct pci_dev *dev) = NULL;
-
 int pcibios_plat_dev_init(struct pci_dev *dev)
 {
-	if (ltq_pci_plat_arch_init)
-		return ltq_pci_plat_arch_init(dev);
-
-	if (ltq_pci_plat_dev_init)
-		return ltq_pci_plat_dev_init(dev);
-
 	return 0;
 }
 
-- 
2.39.2

[PATCH 2/4] MIPS: ath79: Don't return PCIBIOS_* code from pcibios_enable_device()

[Ilpo Järvinen]

pcibios_plat_dev_init() is called from pcibios_enable_device() that
should return normal errnos, not PCIBIOS return codes. In this case the
impact is only cosmetic because PCIBIOS_SUCCESSFUL equals 0 that is
success code with errnos as well. Nonetheless, remove the inconsistency
by replacing the PCIBIOS_SUCCESSFUL with 0.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 arch/mips/pci/fixup-ath79.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/mips/pci/fixup-ath79.c b/arch/mips/pci/fixup-ath79.c
index 09a4ce53424f..6a6c4f58f7f4 100644
--- a/arch/mips/pci/fixup-ath79.c
+++ b/arch/mips/pci/fixup-ath79.c
@@ -9,7 +9,7 @@
 
 int pcibios_plat_dev_init(struct pci_dev *dev)
 {
-	return PCIBIOS_SUCCESSFUL;
+	return 0;
 }
 
 int pcibios_map_irq(const struct pci_dev *dev, u8 slot, u8 pin)
-- 
2.39.2

[PATCH 3/4] MIPS: PCI: Return PCIBIOS_* from tx4927_pci_config_read/write()

[Ilpo Järvinen]

pci_ops .read/.write must return PCIBIOS_* codes but
tx4927_pci_config_read/write() return -1 when mkaddr() cannot find
devfn from the root bus. Return PCIBIOS_DEVICE_NOT_FOUND instead and
pass that onward in the call chain instead of overwriting the return
value.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 arch/mips/pci/ops-tx4927.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/arch/mips/pci/ops-tx4927.c b/arch/mips/pci/ops-tx4927.c
index f7802f100401..4dd8b93985fb 100644
--- a/arch/mips/pci/ops-tx4927.c
+++ b/arch/mips/pci/ops-tx4927.c
@@ -60,7 +60,7 @@ static int mkaddr(struct pci_bus *bus, unsigned int devfn, int where,
 {
 	if (bus->parent == NULL &&
 	    devfn >= PCI_DEVFN(TX4927_PCIC_MAX_DEVNU, 0))
-		return -1;
+		return PCIBIOS_DEVICE_NOT_FOUND;
 	__raw_writel(((bus->number & 0xff) << 0x10)
 		     | ((devfn & 0xff) << 0x08) | (where & 0xfc)
 		     | (bus->parent ? 1 : 0),
@@ -140,10 +140,12 @@ static int tx4927_pci_config_read(struct pci_bus *bus, unsigned int devfn,
 				  int where, int size, u32 *val)
 {
 	struct tx4927_pcic_reg __iomem *pcicptr = pci_bus_to_pcicptr(bus);
+	int ret;
 
-	if (mkaddr(bus, devfn, where, pcicptr)) {
+	ret = mkaddr(bus, devfn, where, pcicptr);
+	if (ret != PCIBIOS_SUCCESSFUL) {
 		*val = 0xffffffff;
-		return -1;
+		return ret;
 	}
 	switch (size) {
 	case 1:
@@ -162,9 +164,11 @@ static int tx4927_pci_config_write(struct pci_bus *bus, unsigned int devfn,
 				   int where, int size, u32 val)
 {
 	struct tx4927_pcic_reg __iomem *pcicptr = pci_bus_to_pcicptr(bus);
+	int ret;
 
-	if (mkaddr(bus, devfn, where, pcicptr))
-		return -1;
+	ret = mkaddr(bus, devfn, where, pcicptr);
+	if (ret != PCIBIOS_SUCCESSFUL)
+		return ret;
 	switch (size) {
 	case 1:
 		icd_writeb(val, where & 3, pcicptr);
-- 
2.39.2

[PATCH 4/4] MIPS: TXx9: Use PCI_SET_ERROR_RESPONSE()

[Ilpo Järvinen]

Instead of literal, PCI error value should be set with
PCI_SET_ERROR_RESPONSE(). Use it in tx4927_pci_config_read().

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 arch/mips/pci/ops-tx4927.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/mips/pci/ops-tx4927.c b/arch/mips/pci/ops-tx4927.c
index 4dd8b93985fb..2db4938d3c75 100644
--- a/arch/mips/pci/ops-tx4927.c
+++ b/arch/mips/pci/ops-tx4927.c
@@ -144,7 +144,7 @@ static int tx4927_pci_config_read(struct pci_bus *bus, unsigned int devfn,
 
 	ret = mkaddr(bus, devfn, where, pcicptr);
 	if (ret != PCIBIOS_SUCCESSFUL) {
-		*val = 0xffffffff;
+		PCI_SET_ERROR_RESPONSE(val);
 		return ret;
 	}
 	switch (size) {
-- 
2.39.2

Re: [PATCH 3/4] MIPS: PCI: Return PCIBIOS_* from tx4927_pci_config_read/write()

[Sergio Paracuellos]

Hi,

On Mon, Feb 5, 2024 at 3:35 PM Ilpo Järvinen
<ilpo.jarvinen@linux.intel.com> wrote:
>
> pci_ops .read/.write must return PCIBIOS_* codes but
> tx4927_pci_config_read/write() return -1 when mkaddr() cannot find
> devfn from the root bus. Return PCIBIOS_DEVICE_NOT_FOUND instead and
> pass that onward in the call chain instead of overwriting the return
> value.
>
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> ---
>  arch/mips/pci/ops-tx4927.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/arch/mips/pci/ops-tx4927.c b/arch/mips/pci/ops-tx4927.c
> index f7802f100401..4dd8b93985fb 100644
> --- a/arch/mips/pci/ops-tx4927.c
> +++ b/arch/mips/pci/ops-tx4927.c
> @@ -60,7 +60,7 @@ static int mkaddr(struct pci_bus *bus, unsigned int devfn, int where,
>  {
>         if (bus->parent == NULL &&
>             devfn >= PCI_DEVFN(TX4927_PCIC_MAX_DEVNU, 0))
> -               return -1;
> +               return PCIBIOS_DEVICE_NOT_FOUND;
>         __raw_writel(((bus->number & 0xff) << 0x10)
>                      | ((devfn & 0xff) << 0x08) | (where & 0xfc)
>                      | (bus->parent ? 1 : 0),

Should we also return PCIBIOS_SUCCESSFUL instead of 'return 0' in
'mkaddr' for coherency?

Other than that, changes look good to me.

Reviewed-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>

Thanks,
    Sergio Paracuellos

> @@ -140,10 +140,12 @@ static int tx4927_pci_config_read(struct pci_bus *bus, unsigned int devfn,
>                                   int where, int size, u32 *val)
>  {
>         struct tx4927_pcic_reg __iomem *pcicptr = pci_bus_to_pcicptr(bus);
> +       int ret;
>
> -       if (mkaddr(bus, devfn, where, pcicptr)) {
> +       ret = mkaddr(bus, devfn, where, pcicptr);
> +       if (ret != PCIBIOS_SUCCESSFUL) {
>                 *val = 0xffffffff;
> -               return -1;
> +               return ret;
>         }
>         switch (size) {
>         case 1:
> @@ -162,9 +164,11 @@ static int tx4927_pci_config_write(struct pci_bus *bus, unsigned int devfn,
>                                    int where, int size, u32 val)
>  {
>         struct tx4927_pcic_reg __iomem *pcicptr = pci_bus_to_pcicptr(bus);
> +       int ret;
>
> -       if (mkaddr(bus, devfn, where, pcicptr))
> -               return -1;
> +       ret = mkaddr(bus, devfn, where, pcicptr);
> +       if (ret != PCIBIOS_SUCCESSFUL)
> +               return ret;
>         switch (size) {
>         case 1:
>                 icd_writeb(val, where & 3, pcicptr);
> --
> 2.39.2
>
>

Re: [PATCH 3/4] MIPS: PCI: Return PCIBIOS_* from tx4927_pci_config_read/write()

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 2202 bytes --]

On Mon, 5 Feb 2024, Sergio Paracuellos wrote:
> On Mon, Feb 5, 2024 at 3:35 PM Ilpo Järvinen
> <ilpo.jarvinen@linux.intel.com> wrote:
> >
> > pci_ops .read/.write must return PCIBIOS_* codes but
> > tx4927_pci_config_read/write() return -1 when mkaddr() cannot find
> > devfn from the root bus. Return PCIBIOS_DEVICE_NOT_FOUND instead and
> > pass that onward in the call chain instead of overwriting the return
> > value.
> >
> > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > ---
> >  arch/mips/pci/ops-tx4927.c | 14 +++++++++-----
> >  1 file changed, 9 insertions(+), 5 deletions(-)
> >
> > diff --git a/arch/mips/pci/ops-tx4927.c b/arch/mips/pci/ops-tx4927.c
> > index f7802f100401..4dd8b93985fb 100644
> > --- a/arch/mips/pci/ops-tx4927.c
> > +++ b/arch/mips/pci/ops-tx4927.c
> > @@ -60,7 +60,7 @@ static int mkaddr(struct pci_bus *bus, unsigned int devfn, int where,
> >  {
> >         if (bus->parent == NULL &&
> >             devfn >= PCI_DEVFN(TX4927_PCIC_MAX_DEVNU, 0))
> > -               return -1;
> > +               return PCIBIOS_DEVICE_NOT_FOUND;
> >         __raw_writel(((bus->number & 0xff) << 0x10)
> >                      | ((devfn & 0xff) << 0x08) | (where & 0xfc)
> >                      | (bus->parent ? 1 : 0),
> 
> Should we also return PCIBIOS_SUCCESSFUL instead of 'return 0' in
> 'mkaddr' for coherency?

Yeah right, I'll change it too.

I didn't take notice of that because the reason for all this is that I 
intend to convert these functions to return generic errno and push the 
PCIBIOS error code -> errno conversion into where it's really needed (real 
PCIBIOS access functions in arch/x86/pci/pcbios.c). Returning 0 as literal 
is very common cosmetic "error" in these functions. While calculating the 
error rate in return values of these functions (I'm able to do that 
because of the audit), those were not even included to 15% returning 
-Esomething instead of PCIBIOS_*. It would be way above that if I'd count 
return 0 also as an error.

> Other than that, changes look good to me.
> 
> Reviewed-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>

Thanks for the review.

-- 
 i.