On Mon, Jul 14, 2014 at 10:11:57AM +0800, Yijing Wang wrote:
> > /**
> > - * pci_msi_check_device - check whether MSI may be enabled on a device
> > + * msi_check_device - check whether MSI may be enabled on a device
> > * @dev: pointer to the pci_dev data structure of MSI device function
> > * @nvec: how many MSIs have been requested ?
> > - * @type: are we checking for MSI or MSI-X ?
> > *
> > * Look at global flags, the device itself, and its parent buses
> > * to determine if MSI/-X are supported for the device. If MSI/-X is
> > * supported return 0, else return an error code.
> > **/
> > -static int pci_msi_check_device(struct pci_dev *dev, int nvec, int type)
> > +static int msi_check_device(struct pci_dev *dev, int nvec)
> > {
> > struct pci_bus *bus;
> > - int ret;
> >
> > /* MSI must be globally enabled and supported by the device */
> > - if (!pci_msi_enable || !dev || dev->no_msi)
> > + if (!pci_msi_enable)
> > + return -EINVAL;
> > +
> > + if (!dev || dev->no_msi || dev->current_state != PCI_D0)
> > return -EINVAL;
> >
> > /*
> > @@ -846,10 +837,6 @@ static int pci_msi_check_device(struct pci_dev *dev,
> > int nvec, int type)
> > if (bus->bus_flags & PCI_BUS_FLAGS_NO_MSI)
> > return -EINVAL;
> >
> > - ret = arch_msi_check_device(dev, nvec, type);
> > - if (ret)
> > - return ret;
> > -
>
> Move the arch_msi_check_device() into arch_msi_setup_irq(), make we can not
> detect whether the device in this platform
> supports MSI or MSI-X aeap. If we delay this, maybe we will do a lot
> unnecessary working for MSI/MSI-X setup.
A traditional approach for a function is first to make sanity check and
then allocate resources. I do not see a reason to keep these two steps
in separate functions: arch_msi_check_device() and arch_setup_msi_irq().
Just make checks within arch_setup_msi_irq() and bail out early would be as
cheap as it is now, but more natural and would deflate the interface.
Moreover, some platforms duplicate checks in arch_msi_check_device() and
arch_setup_msi_irq(), which does not add to readability.
> Thanks!
> Yijing.
>
> > return 0;
> > }
> >
> > @@ -954,13 +941,13 @@ int pci_enable_msix(struct pci_dev *dev, struct
> > msix_entry *entries, int nvec)
> > int status, nr_entries;
> > int i, j;
> >
> > - if (!entries || !dev->msix_cap || dev->current_state != PCI_D0)
> > - return -EINVAL;
> > -
> > - status = pci_msi_check_device(dev, nvec, PCI_CAP_ID_MSIX);
> > + status = msi_check_device(dev, nvec);
> > if (status)
> > return status;
> >
> > + if (!entries)
> > + return -EINVAL;
> > +
> > nr_entries = pci_msix_vec_count(dev);
> > if (nr_entries < 0)
> > return nr_entries;
> > @@ -1085,8 +1072,9 @@ int pci_enable_msi_range(struct pci_dev *dev, int
> > minvec, int maxvec)
> > int nvec;
> > int rc;
> >
> > - if (dev->current_state != PCI_D0)
> > - return -EINVAL;
> > + rc = msi_check_device(dev, minvec);
> > + if (rc)
> > + return rc;
> >
> > WARN_ON(!!dev->msi_enabled);
> >
> > @@ -1109,17 +1097,6 @@ int pci_enable_msi_range(struct pci_dev *dev, int
> > minvec, int maxvec)
> > nvec = maxvec;
> >
> > do {
> > - rc = pci_msi_check_device(dev, nvec, PCI_CAP_ID_MSI);
> > - if (rc < 0) {
> > - return rc;
> > - } else if (rc > 0) {
> > - if (rc < minvec)
> > - return -ENOSPC;
> > - nvec = rc;
> > - }
> > - } while (rc);
> > -
> > - do {
> > rc = msi_capability_init(dev, nvec);
> > if (rc < 0) {
> > return rc;
> > diff --git a/include/linux/msi.h b/include/linux/msi.h
> > index 92a2f99..3b873bc 100644
> > --- a/include/linux/msi.h
> > +++ b/include/linux/msi.h
> > @@ -59,7 +59,6 @@ int arch_setup_msi_irq(struct pci_dev *dev, struct
> > msi_desc *desc);
> > void arch_teardown_msi_irq(unsigned int irq);
> > int arch_setup_msi_irqs(struct pci_dev *dev, int nvec, int type);
> > void arch_teardown_msi_irqs(struct pci_dev *dev);
> > -int arch_msi_check_device(struct pci_dev* dev, int nvec, int type);
> > void arch_restore_msi_irqs(struct pci_dev *dev);
> >
> > void default_teardown_msi_irqs(struct pci_dev *dev);
> > @@ -76,8 +75,6 @@ struct msi_chip {
> > 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 */
> >
>
>
> --
> Thanks!
> Yijing
>
--
Regards,
Alexander Gordeev
agord...@redhat.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
