Re: [PATCH] PCI: Fix return value from pci_user_{read,write}_config_*()

[Gavin Shan <gwshan@linux.vnet.ibm.com>]

On Mon, May 19, 2014 at 08:59:29AM -0700, Greg Thelen wrote:
>
>On Mon, May 12 2014, Bjorn Helgaas <bhelgaas@google.com> wrote:
>
>> [+cc Greg]
>>
>> On Mon, May 12, 2014 at 6:58 PM, Gavin Shan <gwshan@linux.vnet.ibm.com> wrote:
>>> PCI accessors from user space pci_user_{read,write}_config_*()
>>> return negative error number, which was introduced by commit
>>> 34e32072 ("PCI: handle positive error codes"). That patch coverts
>>> all positive error numbers from platform specific PCI config
>>> accessors to -EINVAL. The upper layer calling to those PCI config
>>> accessors hardly know the specific cause from the return value
>>> when hitting failures.
>>>
>>> The patch fixes the issue by doing the conversion (from positive
>>> to negative) using existing function pcibios_err_to_errno().
>>>
>>> Signed-off-by: Gavin Shan <shangw@linux.vnet.ibm.com>
>>> ---
>>>  drivers/pci/access.c | 12 ++++--------
>>>  include/linux/pci.h  |  6 ++++--
>>>  2 files changed, 8 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/drivers/pci/access.c b/drivers/pci/access.c
>>> index 7f8b78c..8c148f3 100644
>>> --- a/drivers/pci/access.c
>>> +++ b/drivers/pci/access.c
>>> @@ -148,7 +148,7 @@ static noinline void pci_wait_cfg(struct pci_dev *dev)
>>>  int pci_user_read_config_##size                                                \
>>>         (struct pci_dev *dev, int pos, type *val)                       \
>>>  {                                                                      \
>>> -       int ret = 0;                                                    \
>>> +       int ret = PCIBIOS_SUCCESSFUL;                                   \
>>>         u32 data = -1;                                                  \
>>>         if (PCI_##size##_BAD)                                           \
>>>                 return -EINVAL;                                         \
>>> @@ -159,9 +159,7 @@ int pci_user_read_config_##size                                             \
>>>                                         pos, sizeof(type), &data);      \
>>>         raw_spin_unlock_irq(&pci_lock);                         \
>>>         *val = (type)data;                                              \
>>> -       if (ret > 0)                                                    \
>>> -               ret = -EINVAL;                                          \
>>> -       return ret;                                                     \
>>> +       return pcibios_err_to_errno(ret);                               \
>>>  }                                                                      \
>>>  EXPORT_SYMBOL_GPL(pci_user_read_config_##size);
>>>
>>> @@ -170,7 +168,7 @@ EXPORT_SYMBOL_GPL(pci_user_read_config_##size);
>>>  int pci_user_write_config_##size                                       \
>>>         (struct pci_dev *dev, int pos, type val)                        \
>>>  {                                                                      \
>>> -       int ret = -EIO;                                                 \
>>> +       int ret = PCIBIOS_SUCCESSFUL;                                   \
>>>         if (PCI_##size##_BAD)                                           \
>>>                 return -EINVAL;                                         \
>>>         raw_spin_lock_irq(&pci_lock);                           \
>>> @@ -179,9 +177,7 @@ int pci_user_write_config_##size                                    \
>>>         ret = dev->bus->ops->write(dev->bus, dev->devfn,                \
>>>                                         pos, sizeof(type), val);        \
>>>         raw_spin_unlock_irq(&pci_lock);                         \
>>> -       if (ret > 0)                                                    \
>>> -               ret = -EINVAL;                                          \
>>> -       return ret;                                                     \
>>> +       return pcibios_err_to_errno(ret);                               \
>>>  }                                                                      \
>>>  EXPORT_SYMBOL_GPL(pci_user_write_config_##size);
>>>
>>> diff --git a/include/linux/pci.h b/include/linux/pci.h
>>> index aab57b4..1682cb1 100644
>>> --- a/include/linux/pci.h
>>> +++ b/include/linux/pci.h
>>> @@ -518,7 +518,7 @@ static inline int pcibios_err_to_errno(int err)
>>>         case PCIBIOS_FUNC_NOT_SUPPORTED:
>>>                 return -ENOENT;
>>>         case PCIBIOS_BAD_VENDOR_ID:
>>> -               return -EINVAL;
>>> +               return -ENOTTY;
>>>         case PCIBIOS_DEVICE_NOT_FOUND:
>>>                 return -ENODEV;
>>>         case PCIBIOS_BAD_REGISTER_NUMBER:
>>> @@ -527,9 +527,11 @@ static inline int pcibios_err_to_errno(int err)
>>>                 return -EIO;
>>>         case PCIBIOS_BUFFER_TOO_SMALL:
>>>                 return -ENOSPC;
>>> +       default:
>>> +               return -err;
>
>Probably worth a comment describing what kinds of positive errors other
>than PCIBIOS_* are expected here.
>
>>>         }
>>>
>>> -       return -ENOTTY;
>>> +       return -ERANGE;
>
>Given the newly added default case above, this is dead code and should be
>deleted.  no?
>

Perhaps, I just need remove the "default" case. I think the PCI config accessors
of individual platforms should return one of the following values, which are
defined in include/linux/pci.h. The unrecognized return value should be translated
to "-ERANGE" directly.

#define PCIBIOS_SUCCESSFUL              0x00
#define PCIBIOS_FUNC_NOT_SUPPORTED      0x81
#define PCIBIOS_BAD_VENDOR_ID           0x83
#define PCIBIOS_DEVICE_NOT_FOUND        0x86
#define PCIBIOS_BAD_REGISTER_NUMBER     0x87
#define PCIBIOS_SET_FAILED              0x88
#define PCIBIOS_BUFFER_TOO_SMALL        0x89

>>>  }
>>>
>>>  /* Low-level architecture-dependent routines */

Thanks,
Gavin