Re: [PATCH] PCI: add quirk for 3ware 9650SE controller

[Bjorn Helgaas]

On Fri, Sep 6, 2013 at 3:51 AM, Jiri Kosina <jkosina@xxxxxxx> wrote:
> On Wed, 28 Aug 2013, Bjorn Helgaas wrote:
>
>> [+cc another email addr for Adam from git logs]
>
> Thanks. Adam, would you happen to have any possible explanation /
> background?
>
>> >> > Commit d5dea7d95 ("PCI: msi: Disable msi interrupts when we initialize a
>> >> > pci device") makes MSIs be forcibly disabled at boot time.
>> >> >
>> >> > It turns out that this breaks 3ware controller -- if MSIs are disabled
>> >> > during PCI discovery of this controller, the device doesn't work properly
>> >> > (it doesn't respond to any commands that are being sent to it after
>> >> > initialization).

Is there a bug report for this issue?  It's nice to have a pointer to,
e.g., a bugzilla.kernel.org bug report with info such as dmesg logs,
lspci output, etc., for future reference.  Maybe somebody will figure
out a more generic change that could make this quirk unnecessary, and
details will help in figuring that out.

I assume the actual PCI discovery done in the PCI core works fine;
it's just that the driver doesn't work if MSIs are disabled on the
device.  If that's the case, can this be fixed by some driver change?
Maybe the driver needs to enable MSI before it sends commands to the
device?

Any description of what this failure looks like to a user?  How can a
user or a distro connect a symptom (driver timeout, console message,
or whatever) to this patch?

>> >> > Reverting d5dea7d95 or not force-disabling MSIs in pci_msi_init_pci_dev()
>> >> > makes the device work properly again.
>> >> >
>> >> > Signed-off-by: Jiri Kosina <jkosina@xxxxxxx>
>> >> >
>> >> > ---
>> >> >
>> >> > I am adding Adam Radford as a recepient as well, to see whether he is able
>> >> > to provide some more explanation why this device would expose this
>> >> > behavior.
>> >
>> > OK, so Adam Radford's lsi.com address is bouncing, hence I guess we can't
>> > expect any feedback from him.
>> >
>> > Bjorn, Jesse, any word on this please?
>>
>> It's on my list to look at.  It's too late to put it in for v3.11, and
>> it's doubtful that it will even make the v3.12 merge window (though
>> possibly it could go in post-merge window).  d5dea7d95 is several
>> years old, so hopefully this issue isn't super-urgent.  Let me know if
>> otherwise.
>
> I agree that this should be applicable to 3.12-rcX as well, as it's very
> device-specific.
>
> Thanks.
>
>>
>> Bjorn
>>
>> >> >  drivers/pci/msi.c    |    3 +++
>> >> >  drivers/pci/quirks.c |   10 ++++++++++
>> >> >  include/linux/pci.h  |    1 +
>> >> >  3 files changed, 14 insertions(+), 0 deletions(-)
>> >> >
>> >> > diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
>> >> > index aca7578..4f36b8b 100644
>> >> > --- a/drivers/pci/msi.c
>> >> > +++ b/drivers/pci/msi.c
>> >> > @@ -1040,6 +1040,9 @@ void pci_msi_init_pci_dev(struct pci_dev *dev)
>> >> >  {
>> >> >     INIT_LIST_HEAD(&dev->msi_list);
>> >> >
>> >> > +   if (dev->broken_msi_disable)
>> >> > +           return;
>> >> > +
>> >> >     /* Disable the msi hardware to avoid screaming interrupts
>> >> >      * during boot.  This is the power on reset default so
>> >> >      * usually this should be a noop.
>> >> > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
>> >> > index e85d230..4ba3400 100644
>> >> > --- a/drivers/pci/quirks.c
>> >> > +++ b/drivers/pci/quirks.c
>> >> > @@ -2890,6 +2890,16 @@ static void quirk_intel_ntb(struct pci_dev *dev)
>> >> >  DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x0e08, quirk_intel_ntb);
>> >> >  DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x0e0d, quirk_intel_ntb);
>> >> >
>> >> > +/*
>> >> > + * 3ware 9650SE controller doesn't properly initialize if MSI are
>> >> > + * disabled on it during PCI device discovery
>> >> > + */
>> >> > +static void quirk_broken_msi_disable(struct pci_dev *dev)
>> >> > +{
>> >> > +   dev->broken_msi_disable = 1;
>> >> > +}
>> >> > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_3WARE, 0x1004, quirk_broken_msi_disable);
>> >> > +
>> >> >  static ktime_t fixup_debug_start(struct pci_dev *dev,
>> >> >                              void (*fn)(struct pci_dev *dev))
>> >> >  {
>> >> > diff --git a/include/linux/pci.h b/include/linux/pci.h
>> >> > index 0fd1f15..c327d74 100644
>> >> > --- a/include/linux/pci.h
>> >> > +++ b/include/linux/pci.h
>> >> > @@ -341,6 +341,7 @@ struct pci_dev {
>> >> >  #ifdef CONFIG_PCI_MSI
>> >> >     struct list_head msi_list;
>> >> >     struct kset *msi_kset;
>> >> > +   unsigned int    broken_msi_disable:1;
>> >> >  #endif
>> >> >     struct pci_vpd *vpd;
>> >> >  #ifdef CONFIG_PCI_ATS
>> >> >
>> >> > --
>> >> > Jiri Kosina
>> >> > SUSE Labs
>> >> >
>> >>
>> >> --
>> >> Jiri Kosina
>> >> SUSE Labs
>> >>
>> >
>> > --
>> > Jiri Kosina
>> > SUSE Labs
>>
>
> --
> Jiri Kosina
> SUSE Labs
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/