On Mon, Jul 8, 2013 at 8:51 AM, Thomas Petazzoni
<thomas.petazzoni@xxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
