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 <[email protected]>
---
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
Hi,
On Mon, Feb 5, 2024 at 3:35 PM Ilpo Järvinen
<[email protected]> 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 <[email protected]>
> ---
> 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 <[email protected]>
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
>
>
On Mon, 5 Feb 2024, Sergio Paracuellos wrote:
> On Mon, Feb 5, 2024 at 3:35 PM Ilpo Järvinen
> <[email protected]> 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 <[email protected]>
> > ---
> > 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 <[email protected]>
Thanks for the review.
--
i.