[PATCHv4,04/11] PCI: Introduce new MSI chip infrastructure

From: Thierry Reding <thierry.reding@avionic-design.de>

The new struct msi_chip is used to associated an MSI controller with a
PCI bus. It is automatically handed down from the root to its children
during bus enumeration.

This patch provides default (weak) implementations for the architecture-
specific MSI functions (arch_setup_msi_irq(), arch_teardown_msi_irq()
and arch_msi_check_device()) which check if a PCI device's bus has an
attached MSI chip and forward the call appropriately.

Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de>
Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 drivers/pci/msi.c   | 22 ++++++++++++++++++++++
 drivers/pci/probe.c |  1 +
 include/linux/msi.h | 11 +++++++++++
 include/linux/pci.h |  1 +
 4 files changed, 35 insertions(+)

On Mon, Jul 1, 2013 at 7:42 AM, Thomas Petazzoni
<thomas.petazzoni@free-electrons.com> wrote:
> From: Thierry Reding <thierry.reding@avionic-design.de>
>
> The new struct msi_chip is used to associated an MSI controller with a
> PCI bus. It is automatically handed down from the root to its children
> during bus enumeration.
>
> This patch provides default (weak) implementations for the architecture-
> specific MSI functions (arch_setup_msi_irq(), arch_teardown_msi_irq()
> and arch_msi_check_device()) which check if a PCI device's bus has an
> attached MSI chip and forward the call appropriately.
>
> Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de>
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>

I'm OK with this patch in principle, but I do have a couple comments below.

> ---
>  drivers/pci/msi.c   | 22 ++++++++++++++++++++++
>  drivers/pci/probe.c |  1 +
>  include/linux/msi.h | 11 +++++++++++
>  include/linux/pci.h |  1 +
>  4 files changed, 35 insertions(+)
>
> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> index 289fbfd..62eb3d5 100644
> --- a/drivers/pci/msi.c
> +++ b/drivers/pci/msi.c
> @@ -32,15 +32,37 @@ static int pci_msi_enable = 1;
>
>  int __weak arch_setup_msi_irq(struct pci_dev *dev, struct msi_desc *desc)
>  {
> +       struct msi_chip *chip = dev->bus->msi;
> +
> +       if (chip && chip->setup_irq) {
> +               int err;
> +
> +               err = chip->setup_irq(chip, dev, desc);
> +               if (err < 0)
> +                       return err;
> +
> +               irq_set_chip_data(desc->irq, chip);
> +               return err;
> +       }
> +
>         return -EINVAL;

It's sub-optimal to indent the whole body of a function like this.  I
think this is a bit more readable:

    if (!chip || !chip->setup_irq)
        return -EINVAL

    err = chip->setup_irq(...);
    ...
    return err;

The return value of ->setup_irq() (and hence of arch_setup_msi_irq())
is a bit unclear.  Apparently it can return negative values (errors)
or positive values (not sure what they mean), or zero (again, not
sure).  A comment would clear this up.

It might even be worth introducing a no-op chip with pointers to no-op
functions so we don't have to do these checks ("if (chip &&
chip->xxx)" everywhere.  I'm not sure if there's a Linux consensus on
that -- certainly there are many examples of code that *does* make
these checks everywhere -- so I'll ack it either way.

>  }
>
>  void __weak arch_teardown_msi_irq(unsigned int irq)
>  {
> +       struct msi_chip *chip = irq_get_chip_data(irq);
> +
> +       if (chip && chip->teardown_irq)
> +               chip->teardown_irq(chip, irq);
>  }
>
>  int __weak arch_msi_check_device(struct pci_dev *dev, int nvec, int type)
>  {
> +       struct msi_chip *chip = dev->bus->msi;
> +
> +       if (chip && chip->check_device)
> +               return chip->check_device(chip, dev, nvec, type);
> +

These functions are poorly named.  They give no clue what
"check_device" means.  Are we checking that it exists, that it
supports some property, that it's enabled, ... ?

>         return 0;
>  }
>
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 70f10fa..c8591e4 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -634,6 +634,7 @@ static struct pci_bus *pci_alloc_child_bus(struct pci_bus *parent,
>
>         child->parent = parent;
>         child->ops = parent->ops;
> +       child->msi = parent->msi;
>         child->sysdata = parent->sysdata;
>         child->bus_flags = parent->bus_flags;
>
> diff --git a/include/linux/msi.h b/include/linux/msi.h
> index c82ff8d..5b357d92 100644
> --- a/include/linux/msi.h
> +++ b/include/linux/msi.h
> @@ -63,4 +63,15 @@ void arch_restore_msi_irqs(struct pci_dev *dev, int irq);
>
>  void default_teardown_msi_irqs(struct pci_dev *dev);
>
> +struct msi_chip {
> +       struct module *owner;
> +       struct device *dev;
> +
> +       int (*setup_irq)(struct msi_chip *chip, struct pci_dev *dev,
> +                        struct msi_desc *desc);
> +       void (*teardown_irq)(struct msi_chip *chip, unsigned int irq);
> +       int (*check_device)(struct msi_chip *chip, struct pci_dev *dev,
> +                           int nvec, int type);
> +};
> +
>  #endif /* LINUX_MSI_H */
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 3a24e4f..7ffc012 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -432,6 +432,7 @@ struct pci_bus {
>         struct resource busn_res;       /* bus numbers routed to this bus */
>
>         struct pci_ops  *ops;           /* configuration access functions */
> +       struct msi_chip *msi;           /* MSI controller */
>         void            *sysdata;       /* hook for sys-specific extension */
>         struct proc_dir_entry *procdir; /* directory entry in /proc/bus/pci */
>
> --
> 1.8.1.2
>
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Dear Bjorn Helgaas,

On Fri, 5 Jul 2013 15:51:10 -0600, Bjorn Helgaas wrote:

> > ---
> >  drivers/pci/msi.c   | 22 ++++++++++++++++++++++
> >  drivers/pci/probe.c |  1 +
> >  include/linux/msi.h | 11 +++++++++++
> >  include/linux/pci.h |  1 +
> >  4 files changed, 35 insertions(+)
> >
> > diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> > index 289fbfd..62eb3d5 100644
> > --- a/drivers/pci/msi.c
> > +++ b/drivers/pci/msi.c
> > @@ -32,15 +32,37 @@ static int pci_msi_enable = 1;
> >
> >  int __weak arch_setup_msi_irq(struct pci_dev *dev, struct msi_desc *desc)
> >  {
> > +       struct msi_chip *chip = dev->bus->msi;
> > +
> > +       if (chip && chip->setup_irq) {
> > +               int err;
> > +
> > +               err = chip->setup_irq(chip, dev, desc);
> > +               if (err < 0)
> > +                       return err;
> > +
> > +               irq_set_chip_data(desc->irq, chip);
> > +               return err;
> > +       }
> > +
> >         return -EINVAL;
> 
> It's sub-optimal to indent the whole body of a function like this.  I
> think this is a bit more readable:
> 
>     if (!chip || !chip->setup_irq)
>         return -EINVAL
> 
>     err = chip->setup_irq(...);
>     ...
>     return err;

Right.

> The return value of ->setup_irq() (and hence of arch_setup_msi_irq())
> is a bit unclear.  Apparently it can return negative values (errors)
> or positive values (not sure what they mean), or zero (again, not
> sure).  A comment would clear this up.

Ok, I'll have to look into this. Maybe Thierry Redding can comment on
this.

> It might even be worth introducing a no-op chip with pointers to no-op
> functions so we don't have to do these checks ("if (chip &&
> chip->xxx)" everywhere.  I'm not sure if there's a Linux consensus on
> that -- certainly there are many examples of code that *does* make
> these checks everywhere -- so I'll ack it either way.

Ok, I'll see if it makes the overall thing cleaner.


> >  int __weak arch_msi_check_device(struct pci_dev *dev, int nvec, int type)
> >  {
> > +       struct msi_chip *chip = dev->bus->msi;
> > +
> > +       if (chip && chip->check_device)
> > +               return chip->check_device(chip, dev, nvec, type);
> > +
> 
> These functions are poorly named.  They give no clue what
> "check_device" means.  Are we checking that it exists, that it
> supports some property, that it's enabled, ... ?

Maybe Thierry Redding can comment on this one?

Thanks,

Thomas

Dear Bjorn Helgaas,

On Fri, 5 Jul 2013 15:51:10 -0600, Bjorn Helgaas wrote:

> >  int __weak arch_setup_msi_irq(struct pci_dev *dev, struct msi_desc *desc)
> >  {
> > +       struct msi_chip *chip = dev->bus->msi;
> > +
> > +       if (chip && chip->setup_irq) {
> > +               int err;
> > +
> > +               err = chip->setup_irq(chip, dev, desc);
> > +               if (err < 0)
> > +                       return err;
> > +
> > +               irq_set_chip_data(desc->irq, chip);
> > +               return err;
> > +       }
> > +
> >         return -EINVAL;
> 
> It's sub-optimal to indent the whole body of a function like this.  I
> think this is a bit more readable:
> 
>     if (!chip || !chip->setup_irq)
>         return -EINVAL
> 
>     err = chip->setup_irq(...);
>     ...
>     return err;
> 
> The return value of ->setup_irq() (and hence of arch_setup_msi_irq())
> is a bit unclear.  Apparently it can return negative values (errors)
> or positive values (not sure what they mean), or zero (again, not
> sure).  A comment would clear this up.

I've changed ->setup_irq() to simply return 0 on success, and a
negative error code on failure.

Apparently, according to the default implementation
of arch_msi_setup_irqs(), the arch_msi_setup_irq() hook is supposed to
return:
 * A negative value on error, the value being an error code that is
   propagated to the caller.
 * A positive value on some other errors, in which case the -ENOSPC
   error value is returned. To me, it doesn't make a lot of sense, as
   arch_msi_setup_irq() could just as well return -ENOSPC directly.
 * A 0 value on success.

> It might even be worth introducing a no-op chip with pointers to no-op
> functions so we don't have to do these checks ("if (chip &&
> chip->xxx)" everywhere.  I'm not sure if there's a Linux consensus on
> that -- certainly there are many examples of code that *does* make
> these checks everywhere -- so I'll ack it either way.

The problem with this is that I'm not sure where in the PCI code this
association to the default implementation should be done. And there are
also two levels to take into account here:

 * The PCI driver may not specify any msi_chip structure for a
   particular pci_bus. This would have to be detected by the PCI core
   when the bus is registered, and bus->msi would be set to the special
   no-op msi_chip.

 * The PCI driver may specify an msi_chip structure, but without
   necessarily implementation all the methods. This could be solved by
   offering a pci_msi_chip_assign(struct pci_bus *, struct msi_chip *)
   function to be used by PCI drivers to assign their msi_chip to a
   given pci_bus, and this function would fill in the missing msi_chip
   operations with the default implementation.

I am not sure it is really worth doing at this point, but I'm open to
suggestions on this.

> >  int __weak arch_msi_check_device(struct pci_dev *dev, int nvec, int type)
> >  {
> > +       struct msi_chip *chip = dev->bus->msi;
> > +
> > +       if (chip && chip->check_device)
> > +               return chip->check_device(chip, dev, nvec, type);
> > +
> 
> These functions are poorly named.  They give no clue what
> "check_device" means.  Are we checking that it exists, that it
> supports some property, that it's enabled, ... ?

Well the naming clearly doesn't come from this commit. The
arch_msi_check_device() hook was around before this commit, and the
reason the operation is named ->check_device() is just because it used
the same terminology as the existing arch_msi_check_device() call.

This hook is only used in one place, the PowerPC architecture, in
arch/powerpc/kernel/msi.c, to actually check whether the given device
supports MSI.

I can rename if to arch_msi_device_supports_msi() and
->device_supports_msi(), or arch_msi_device_has_msi() and
->device_has_msi(), but that a change that relatively unrelated to this
commit, I'd say.

Here as well, I'm open to suggestions,

Thomas

On Mon, Jul 08, 2013 at 04:51:49PM +0200, Thomas Petazzoni wrote:
> Dear Bjorn Helgaas,

Sorry for chiming in so late, I've had bad internet connectivity and
email access for a few days.

> On Fri, 5 Jul 2013 15:51:10 -0600, Bjorn Helgaas wrote:
>
> > >  int __weak arch_setup_msi_irq(struct pci_dev *dev, struct msi_desc *desc)
> > >  {
> > > +       struct msi_chip *chip = dev->bus->msi;
> > > +
> > > +       if (chip && chip->setup_irq) {
> > > +               int err;
> > > +
> > > +               err = chip->setup_irq(chip, dev, desc);
> > > +               if (err < 0)
> > > +                       return err;
> > > +
> > > +               irq_set_chip_data(desc->irq, chip);
> > > +               return err;
> > > +       }
> > > +
> > >         return -EINVAL;
> > 
> > It's sub-optimal to indent the whole body of a function like this.  I
> > think this is a bit more readable:
> > 
> >     if (!chip || !chip->setup_irq)
> >         return -EINVAL
> > 
> >     err = chip->setup_irq(...);
> >     ...
> >     return err;
> > 
> > The return value of ->setup_irq() (and hence of arch_setup_msi_irq())
> > is a bit unclear.  Apparently it can return negative values (errors)
> > or positive values (not sure what they mean), or zero (again, not
> > sure).  A comment would clear this up.
>
> I've changed ->setup_irq() to simply return 0 on success, and a
> negative error code on failure.
>
> Apparently, according to the default implementation
> of arch_msi_setup_irqs(), the arch_msi_setup_irq() hook is supposed to
> return:
>  * A negative value on error, the value being an error code that is
>    propagated to the caller.
>  * A positive value on some other errors, in which case the -ENOSPC
>    error value is returned. To me, it doesn't make a lot of sense, as
>    arch_msi_setup_irq() could just as well return -ENOSPC directly.
>  * A 0 value on success.

Yes, I think that'd be much more straightforward. Having > 0 as a
special case isn't useful here.

> > It might even be worth introducing a no-op chip with pointers to no-op
> > functions so we don't have to do these checks ("if (chip &&
> > chip->xxx)" everywhere.  I'm not sure if there's a Linux consensus on
> > that -- certainly there are many examples of code that *does* make
> > these checks everywhere -- so I'll ack it either way.
>
> The problem with this is that I'm not sure where in the PCI code this
> association to the default implementation should be done. And there are
> also two levels to take into account here:
>
>  * The PCI driver may not specify any msi_chip structure for a
>    particular pci_bus. This would have to be detected by the PCI core
>    when the bus is registered, and bus->msi would be set to the special
>    no-op msi_chip.

You could check if bus->msi was set after the call to pcibios_bus_add()
but that's a bit wonky because it may break once the first architecture
appears that assigns it in a different place.

>  * The PCI driver may specify an msi_chip structure, but without
>    necessarily implementation all the methods. This could be solved by
>    offering a pci_msi_chip_assign(struct pci_bus *, struct msi_chip *)
>    function to be used by PCI drivers to assign their msi_chip to a
>    given pci_bus, and this function would fill in the missing msi_chip
>    operations with the default implementation.
>
> I am not sure it is really worth doing at this point, but I'm open to
> suggestions on this.

I think these extra calls aren't that bad given how other alternatives
are much easier to break things. Perhaps checking for the methods could
be dropped, therefore making them mandatory (by crashing if they're not
implemented).

> > >  int __weak arch_msi_check_device(struct pci_dev *dev, int nvec, int type)
> > >  {
> > > +       struct msi_chip *chip = dev->bus->msi;
> > > +
> > > +       if (chip && chip->check_device)
> > > +               return chip->check_device(chip, dev, nvec, type);
> > > +
> > 
> > These functions are poorly named.  They give no clue what
> > "check_device" means.  Are we checking that it exists, that it
> > supports some property, that it's enabled, ... ?
>
> Well the naming clearly doesn't come from this commit. The
> arch_msi_check_device() hook was around before this commit, and the
> reason the operation is named ->check_device() is just because it used
> the same terminology as the existing arch_msi_check_device() call.
>
> This hook is only used in one place, the PowerPC architecture, in
> arch/powerpc/kernel/msi.c, to actually check whether the given device
> supports MSI.
>
> I can rename if to arch_msi_device_supports_msi() and
> ->device_supports_msi(), or arch_msi_device_has_msi() and
> ->device_has_msi(), but that a change that relatively unrelated to this
> commit, I'd say.
>
> Here as well, I'm open to suggestions,

I don't think *_check_device() is all that bad. Some of the
implementations in PowerPC only use it to output a debug message, others
do some more checking. Some of the checks look generic enough to move
them into the core.

If you rename it to *_has_msi() or *_supports_msi() then I'd expect it
to return a boolean and you loose the advantage of being able to return
a more meaningful value. Although I'm not sure how useful that error
code really is. It is passed all the way up to the caller of
pci_enable_msi(), so a driver could use it to construct a better error
message based on the error code. From a quick glance a few of them seem
to do that.

Thierry

On Mon, Jul 8, 2013 at 8:51 AM, Thomas Petazzoni
<thomas.petazzoni@free-electrons.com> wrote:
> Dear Bjorn Helgaas,
>
> On Fri, 5 Jul 2013 15:51:10 -0600, Bjorn Helgaas wrote:
>
>> >  int __weak arch_setup_msi_irq(struct pci_dev *dev, struct msi_desc *desc)
>> >  {
>> > +       struct msi_chip *chip = dev->bus->msi;
>> > +
>> > +       if (chip && chip->setup_irq) {
>> > +               int err;
>> > +
>> > +               err = chip->setup_irq(chip, dev, desc);
>> > +               if (err < 0)
>> > +                       return err;
>> > +
>> > +               irq_set_chip_data(desc->irq, chip);
>> > +               return err;
>> > +       }
>> > +
>> >         return -EINVAL;
>>
>> It's sub-optimal to indent the whole body of a function like this.  I
>> think this is a bit more readable:
>>
>>     if (!chip || !chip->setup_irq)
>>         return -EINVAL
>>
>>     err = chip->setup_irq(...);
>>     ...
>>     return err;
>>
>> The return value of ->setup_irq() (and hence of arch_setup_msi_irq())
>> is a bit unclear.  Apparently it can return negative values (errors)
>> or positive values (not sure what they mean), or zero (again, not
>> sure).  A comment would clear this up.
>
> I've changed ->setup_irq() to simply return 0 on success, and a
> negative error code on failure.

Perfect.

> Apparently, according to the default implementation
> of arch_msi_setup_irqs(), the arch_msi_setup_irq() hook is supposed to
> return:
>  * A negative value on error, the value being an error code that is
>    propagated to the caller.
>  * A positive value on some other errors, in which case the -ENOSPC
>    error value is returned. To me, it doesn't make a lot of sense, as
>    arch_msi_setup_irq() could just as well return -ENOSPC directly.
>  * A 0 value on success.
>
>> It might even be worth introducing a no-op chip with pointers to no-op
>> functions so we don't have to do these checks ("if (chip &&
>> chip->xxx)" everywhere.  I'm not sure if there's a Linux consensus on
>> that -- certainly there are many examples of code that *does* make
>> these checks everywhere -- so I'll ack it either way.
>
> The problem with this is that I'm not sure where in the PCI code this
> association to the default implementation should be done. And there are
> also two levels to take into account here:
>
>  * The PCI driver may not specify any msi_chip structure for a
>    particular pci_bus. This would have to be detected by the PCI core
>    when the bus is registered, and bus->msi would be set to the special
>    no-op msi_chip.
>
>  * The PCI driver may specify an msi_chip structure, but without
>    necessarily implementation all the methods. This could be solved by
>    offering a pci_msi_chip_assign(struct pci_bus *, struct msi_chip *)
>    function to be used by PCI drivers to assign their msi_chip to a
>    given pci_bus, and this function would fill in the missing msi_chip
>    operations with the default implementation.
>
> I am not sure it is really worth doing at this point, but I'm open to
> suggestions on this.

Sounds like it's not worth doing.  Thanks for checking into it.

>> >  int __weak arch_msi_check_device(struct pci_dev *dev, int nvec, int type)
>> >  {
>> > +       struct msi_chip *chip = dev->bus->msi;
>> > +
>> > +       if (chip && chip->check_device)
>> > +               return chip->check_device(chip, dev, nvec, type);
>> > +
>>
>> These functions are poorly named.  They give no clue what
>> "check_device" means.  Are we checking that it exists, that it
>> supports some property, that it's enabled, ... ?
>
> Well the naming clearly doesn't come from this commit. The
> arch_msi_check_device() hook was around before this commit, and the
> reason the operation is named ->check_device() is just because it used
> the same terminology as the existing arch_msi_check_device() call.
>
> This hook is only used in one place, the PowerPC architecture, in
> arch/powerpc/kernel/msi.c, to actually check whether the given device
> supports MSI.
>
> I can rename if to arch_msi_device_supports_msi() and
> ->device_supports_msi(), or arch_msi_device_has_msi() and
> ->device_has_msi(), but that a change that relatively unrelated to this
> commit, I'd say.
>
> Here as well, I'm open to suggestions,

You're right, you didn't make that mess, so I guess it's OK if you
don't clean it up here.  I spent a fair amount of time yesterday
analyzing the return values, and lost interest before all became
clear.  So I stand by my assertion that it is hard to read, but I'm OK
with leaving it as-is for now.

Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html