Re: [PATCH 27/53] IB/qib: Add qib_pcie.c
On Thu, 19 Nov 2009, Roland Dreier wrote:
| > +static void qib_msix_setup(struct qib_devdata *dd, int pos, u32 *msixcnt,
| > + struct msix_entry *msix_entry)
| > +{
| > + int ret;
| > + u32 tabsize = 0;
| > + u16 msix_flags;
| > +
| > + pci_read_config_word(dd->pcidev, pos + PCI_MSIX_FLAGS, &msix_flags);
| > + tabsize = 1 + (msix_flags & PCI_MSIX_FLAGS_QSIZE);
| > + if (tabsize > *msixcnt)
| > + tabsize = *msixcnt;
| > + ret = pci_enable_msix(dd->pcidev, msix_entry, tabsize);
|
| all the monkeying with config space seems dubious... fallback should
| handle the table size, and I don't think drivers should be peeking and
| poking in config space anyway. Fix the PCI core if some generic
| function is missing...
It's needed on device resets. The core kernel code (at least up through
2.6.27, we'll have to reverify on 2.6.32), doesn't handle that
sufficiently well, with the result that no further interrupts are
delivered.
These are driver-synchronous resets, so I guess we could add core code
to save and restore these, but I'd be surprised if very many (any?) other
drivers would ever use them.
We can also try to develop patches to the core pci code for the actual device
reset (as opposed to helper functions), but that would be potentially
risky, and I don't know that many pcie drivers could or would use that
functionality.
| > +/**
| > + * We save the msi lo and hi values, so we can restore them after
| > + * chip reset (the kernel PCI infrastructure doesn't yet handle that
| > + * correctly.
| > + */
|
| again... if the core doesn't do something right, fix it rather than
| hacking around it in a driver.
Same answer, I guess.
If it's seen as a big enough obstacle, we can drop this from the
upstream driver, and maintain it only in the qlogic and ofed-shipped
drivers, but I'd prefer not to do that, of course.
Dave Olson
dave.ol...@qlogic.com
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 27/53] IB/qib: Add qib_pcie.c
> +#ifdef CONFIG_PCIEAER
> +#include
> +#endif
I don't think this ifdef is needed... looks fine to
include even if PCIEAER isn't set.
> +#ifndef PCI_MSIX_FLAGS
> +#define PCI_MSIX_FLAGS 2
> +#endif
> +#ifndef PCI_MSIX_FLAGS_ENABLE
> +#define PCI_MSIX_FLAGS_ENABLE (1 << 15)
> +#endif
> +#ifndef PCI_MSIX_FLAGS_QSIZE
> +#define PCI_MSIX_FLAGS_QSIZE 0x7FF
> +#endif
None of this is needed ... all these constants are defined, right?
> +#ifdef CONFIG_PCIEAER
> +/* enable basic AER reporting. Perhaps more later */
> +if (pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_ERR)) {
> +ret = pci_enable_pcie_error_reporting(pdev);
> +if (ret)
> +qib_early_err(&pdev->dev,
> + "Unable to enable pcie error reporting"
> + ": %d\n", ret);
> +} else
> +qib_dbg("AER capability not found! AER reports not enabled\n");
> +#endif
ifdef here isn't needed either, I don't think. And neither is the test
for the capability... just call pci_enable_pcie_error_reporting always
and it will return an error if the config option isn't set or the cap
isn't found.
> +#ifdef CONFIG_PCI_MSI
again, ifdef not needed, right?
> +static void qib_msix_setup(struct qib_devdata *dd, int pos, u32 *msixcnt,
> + struct msix_entry *msix_entry)
> +{
> +int ret;
> +u32 tabsize = 0;
> +u16 msix_flags;
> +
> +pci_read_config_word(dd->pcidev, pos + PCI_MSIX_FLAGS, &msix_flags);
> +tabsize = 1 + (msix_flags & PCI_MSIX_FLAGS_QSIZE);
> +if (tabsize > *msixcnt)
> +tabsize = *msixcnt;
> +ret = pci_enable_msix(dd->pcidev, msix_entry, tabsize);
all the monkeying with config space seems dubious... fallback should
handle the table size, and I don't think drivers should be peeking and
poking in config space anyway. Fix the PCI core if some generic
function is missing...
> +/**
> + * We save the msi lo and hi values, so we can restore them after
> + * chip reset (the kernel PCI infrastructure doesn't yet handle that
> + * correctly.
> + */
again... if the core doesn't do something right, fix it rather than
hacking around it in a driver.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
