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
--
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
