* Re: [PATCH] PCI: Mark INTx masking support of Chelsio T310 10GbE NIC as broken
2012-05-25 14:02 [PATCH] PCI: Mark INTx masking support of Chelsio T310 10GbE NIC as broken Jan Kiszka
@ 2012-05-25 14:11 ` Bjorn Helgaas
2012-05-25 14:20 ` Jan Kiszka
2012-05-28 12:39 ` Michael S. Tsirkin
2012-06-05 14:38 ` Jan Kiszka
2 siblings, 1 reply; 14+ messages in thread
From: Bjorn Helgaas @ 2012-05-25 14:11 UTC (permalink / raw)
To: Jan Kiszka
Cc: linux-pci, Alexey Kardashevskiy, Linux Kernel Mailing List,
Alex Williamson, Benjamin Herrenschmidt, David Gibson,
Alexander Graf, kvm
On Fri, May 25, 2012 at 8:02 AM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> According to Alexey, the T310 does not properly support INTx masking as
> it fails to keep the PCI_STATUS_INTERRUPT bit updated once the interrupt
> is masked. Mark this adapter as broken so that pci_intx_mask_supported
> won't report it as compatible.
Please include a reference to a bugzilla, mailing list discussion, or
other details about how this was found and debugged. Thanks!
> Reported-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> Signed-off-by: Jan Kiszka <jan.kiszka@web.de>
> ---
>
> Alexey, please test if this catches your case correctly.
>
> drivers/pci/pci.c | 3 +++
> drivers/pci/quirks.c | 12 ++++++++++++
> include/linux/pci.h | 2 ++
> 3 files changed, 17 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 8f16900..3a1aeb5 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -2876,6 +2876,9 @@ bool pci_intx_mask_supported(struct pci_dev *dev)
> bool mask_supported = false;
> u16 orig, new;
>
> + if (dev->broken_intx_masking)
> + return false;
> +
> pci_cfg_access_lock(dev);
>
> pci_read_config_word(dev, PCI_COMMAND, &orig);
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index 2a75216..151e174 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -2929,6 +2929,18 @@ static void __devinit disable_igfx_irq(struct pci_dev *dev)
> DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x0102, disable_igfx_irq);
> DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x010a, disable_igfx_irq);
>
> +/*
> + * Some devices may pass our check in pci_intx_mask_supported if
> + * PCI_COMMAND_INTX_DISABLE works though they actually do not properly
> + * support this feature.
> + */
> +static void __devinit quirk_broken_intx_masking(struct pci_dev *dev)
> +{
> + dev->broken_intx_masking = 1;
> +}
> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_CHELSIO, 0x0010,
> + quirk_broken_intx_masking);
> +
> static void pci_do_fixups(struct pci_dev *dev, struct pci_fixup *f,
> struct pci_fixup *end)
> {
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 17b7b5b..c7cfd73 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -324,6 +324,8 @@ struct pci_dev {
> unsigned int is_hotplug_bridge:1;
> unsigned int __aer_firmware_first_valid:1;
> unsigned int __aer_firmware_first:1;
> + unsigned int broken_intx_masking:1; /* device's INTx masking
> + support is not working */
> pci_dev_flags_t dev_flags;
> atomic_t enable_cnt; /* pci_enable_device has been called */
>
> --
> 1.7.3.4
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH] PCI: Mark INTx masking support of Chelsio T310 10GbE NIC as broken
2012-05-25 14:11 ` Bjorn Helgaas
@ 2012-05-25 14:20 ` Jan Kiszka
0 siblings, 0 replies; 14+ messages in thread
From: Jan Kiszka @ 2012-05-25 14:20 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: linux-pci, Alexey Kardashevskiy, Linux Kernel Mailing List,
Alex Williamson, Benjamin Herrenschmidt, David Gibson,
Alexander Graf, kvm
On 2012-05-25 11:11, Bjorn Helgaas wrote:
> On Fri, May 25, 2012 at 8:02 AM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>> According to Alexey, the T310 does not properly support INTx masking as
>> it fails to keep the PCI_STATUS_INTERRUPT bit updated once the interrupt
>> is masked. Mark this adapter as broken so that pci_intx_mask_supported
>> won't report it as compatible.
>
> Please include a reference to a bugzilla, mailing list discussion, or
> other details about how this was found and debugged. Thanks!
Sorry, it's documented here:
http://thread.gmane.org/gmane.comp.emulators.kvm.devel/91388. Will
repost with this information included once Alexey provided his tested-by.
Jan
>
>> Reported-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>> Signed-off-by: Jan Kiszka <jan.kiszka@web.de>
>> ---
>>
>> Alexey, please test if this catches your case correctly.
>>
>> drivers/pci/pci.c | 3 +++
>> drivers/pci/quirks.c | 12 ++++++++++++
>> include/linux/pci.h | 2 ++
>> 3 files changed, 17 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>> index 8f16900..3a1aeb5 100644
>> --- a/drivers/pci/pci.c
>> +++ b/drivers/pci/pci.c
>> @@ -2876,6 +2876,9 @@ bool pci_intx_mask_supported(struct pci_dev *dev)
>> bool mask_supported = false;
>> u16 orig, new;
>>
>> + if (dev->broken_intx_masking)
>> + return false;
>> +
>> pci_cfg_access_lock(dev);
>>
>> pci_read_config_word(dev, PCI_COMMAND, &orig);
>> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
>> index 2a75216..151e174 100644
>> --- a/drivers/pci/quirks.c
>> +++ b/drivers/pci/quirks.c
>> @@ -2929,6 +2929,18 @@ static void __devinit disable_igfx_irq(struct pci_dev *dev)
>> DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x0102, disable_igfx_irq);
>> DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x010a, disable_igfx_irq);
>>
>> +/*
>> + * Some devices may pass our check in pci_intx_mask_supported if
>> + * PCI_COMMAND_INTX_DISABLE works though they actually do not properly
>> + * support this feature.
>> + */
>> +static void __devinit quirk_broken_intx_masking(struct pci_dev *dev)
>> +{
>> + dev->broken_intx_masking = 1;
>> +}
>> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_CHELSIO, 0x0010,
>> + quirk_broken_intx_masking);
>> +
>> static void pci_do_fixups(struct pci_dev *dev, struct pci_fixup *f,
>> struct pci_fixup *end)
>> {
>> diff --git a/include/linux/pci.h b/include/linux/pci.h
>> index 17b7b5b..c7cfd73 100644
>> --- a/include/linux/pci.h
>> +++ b/include/linux/pci.h
>> @@ -324,6 +324,8 @@ struct pci_dev {
>> unsigned int is_hotplug_bridge:1;
>> unsigned int __aer_firmware_first_valid:1;
>> unsigned int __aer_firmware_first:1;
>> + unsigned int broken_intx_masking:1; /* device's INTx masking
>> + support is not working */
>> pci_dev_flags_t dev_flags;
>> atomic_t enable_cnt; /* pci_enable_device has been called */
>>
>> --
>> 1.7.3.4
--
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] PCI: Mark INTx masking support of Chelsio T310 10GbE NIC as broken
2012-05-25 14:02 [PATCH] PCI: Mark INTx masking support of Chelsio T310 10GbE NIC as broken Jan Kiszka
2012-05-25 14:11 ` Bjorn Helgaas
@ 2012-05-28 12:39 ` Michael S. Tsirkin
2012-05-28 12:51 ` Jan Kiszka
2012-06-05 14:38 ` Jan Kiszka
2 siblings, 1 reply; 14+ messages in thread
From: Michael S. Tsirkin @ 2012-05-28 12:39 UTC (permalink / raw)
To: Jan Kiszka
Cc: linux-pci, Bjorn Helgaas, Alexey Kardashevskiy,
Linux Kernel Mailing List, Alex Williamson,
Benjamin Herrenschmidt, David Gibson, Alexander Graf, kvm
On Fri, May 25, 2012 at 11:02:13AM -0300, Jan Kiszka wrote:
> According to Alexey, the T310 does not properly support INTx masking as
> it fails to keep the PCI_STATUS_INTERRUPT bit updated once the interrupt
> is masked. Mark this adapter as broken so that pci_intx_mask_supported
> won't report it as compatible.
>
> Reported-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> Signed-off-by: Jan Kiszka <jan.kiszka@web.de>
Just a thought: would be nice to have a way to discover
the quirk was activated. Add an attribute so that
userspace can detect and report this properly to users?
Or just log a warning message ...
> ---
>
> Alexey, please test if this catches your case correctly.
>
> drivers/pci/pci.c | 3 +++
> drivers/pci/quirks.c | 12 ++++++++++++
> include/linux/pci.h | 2 ++
> 3 files changed, 17 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 8f16900..3a1aeb5 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -2876,6 +2876,9 @@ bool pci_intx_mask_supported(struct pci_dev *dev)
> bool mask_supported = false;
> u16 orig, new;
>
> + if (dev->broken_intx_masking)
> + return false;
> +
> pci_cfg_access_lock(dev);
>
> pci_read_config_word(dev, PCI_COMMAND, &orig);
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index 2a75216..151e174 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -2929,6 +2929,18 @@ static void __devinit disable_igfx_irq(struct pci_dev *dev)
> DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x0102, disable_igfx_irq);
> DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x010a, disable_igfx_irq);
>
> +/*
> + * Some devices may pass our check in pci_intx_mask_supported if
> + * PCI_COMMAND_INTX_DISABLE works though they actually do not properly
> + * support this feature.
> + */
> +static void __devinit quirk_broken_intx_masking(struct pci_dev *dev)
> +{
> + dev->broken_intx_masking = 1;
> +}
> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_CHELSIO, 0x0010,
> + quirk_broken_intx_masking);
> +
> static void pci_do_fixups(struct pci_dev *dev, struct pci_fixup *f,
> struct pci_fixup *end)
> {
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 17b7b5b..c7cfd73 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -324,6 +324,8 @@ struct pci_dev {
> unsigned int is_hotplug_bridge:1;
> unsigned int __aer_firmware_first_valid:1;
> unsigned int __aer_firmware_first:1;
> + unsigned int broken_intx_masking:1; /* device's INTx masking
> + support is not working */
> pci_dev_flags_t dev_flags;
> atomic_t enable_cnt; /* pci_enable_device has been called */
>
> --
> 1.7.3.4
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH] PCI: Mark INTx masking support of Chelsio T310 10GbE NIC as broken
2012-05-28 12:39 ` Michael S. Tsirkin
@ 2012-05-28 12:51 ` Jan Kiszka
2012-05-28 13:21 ` Michael S. Tsirkin
0 siblings, 1 reply; 14+ messages in thread
From: Jan Kiszka @ 2012-05-28 12:51 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: linux-pci, Bjorn Helgaas, Alexey Kardashevskiy,
Linux Kernel Mailing List, Alex Williamson,
Benjamin Herrenschmidt, David Gibson, Alexander Graf, kvm
[-- Attachment #1: Type: text/plain, Size: 815 bytes --]
On 2012-05-28 14:39, Michael S. Tsirkin wrote:
> On Fri, May 25, 2012 at 11:02:13AM -0300, Jan Kiszka wrote:
>> According to Alexey, the T310 does not properly support INTx masking as
>> it fails to keep the PCI_STATUS_INTERRUPT bit updated once the interrupt
>> is masked. Mark this adapter as broken so that pci_intx_mask_supported
>> won't report it as compatible.
>>
>> Reported-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>> Signed-off-by: Jan Kiszka <jan.kiszka@web.de>
>
>
> Just a thought: would be nice to have a way to discover
> the quirk was activated. Add an attribute so that
> userspace can detect and report this properly to users?
> Or just log a warning message ...
pr_notice_once? A flag for userspace would be significantly more
complicated (and not PCI layer hands).
Jan
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] PCI: Mark INTx masking support of Chelsio T310 10GbE NIC as broken
2012-05-28 12:51 ` Jan Kiszka
@ 2012-05-28 13:21 ` Michael S. Tsirkin
2012-05-28 13:29 ` Jan Kiszka
0 siblings, 1 reply; 14+ messages in thread
From: Michael S. Tsirkin @ 2012-05-28 13:21 UTC (permalink / raw)
To: Jan Kiszka
Cc: linux-pci, Bjorn Helgaas, Alexey Kardashevskiy,
Linux Kernel Mailing List, Alex Williamson,
Benjamin Herrenschmidt, David Gibson, Alexander Graf, kvm
On Mon, May 28, 2012 at 02:51:25PM +0200, Jan Kiszka wrote:
> On 2012-05-28 14:39, Michael S. Tsirkin wrote:
> > On Fri, May 25, 2012 at 11:02:13AM -0300, Jan Kiszka wrote:
> >> According to Alexey, the T310 does not properly support INTx masking as
> >> it fails to keep the PCI_STATUS_INTERRUPT bit updated once the interrupt
> >> is masked. Mark this adapter as broken so that pci_intx_mask_supported
> >> won't report it as compatible.
> >>
> >> Reported-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> >> Signed-off-by: Jan Kiszka <jan.kiszka@web.de>
> >
> >
> > Just a thought: would be nice to have a way to discover
> > the quirk was activated. Add an attribute so that
> > userspace can detect and report this properly to users?
> > Or just log a warning message ...
>
> pr_notice_once?
OK IMO.
> A flag for userspace would be significantly more
> complicated (and not PCI layer hands).
Why not? I meant e.g. an attribute in pci-sysfs.
> Jan
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] PCI: Mark INTx masking support of Chelsio T310 10GbE NIC as broken
2012-05-28 13:21 ` Michael S. Tsirkin
@ 2012-05-28 13:29 ` Jan Kiszka
2012-05-28 13:39 ` Michael S. Tsirkin
0 siblings, 1 reply; 14+ messages in thread
From: Jan Kiszka @ 2012-05-28 13:29 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: linux-pci, Bjorn Helgaas, Alexey Kardashevskiy,
Linux Kernel Mailing List, Alex Williamson,
Benjamin Herrenschmidt, David Gibson, Alexander Graf, kvm
[-- Attachment #1: Type: text/plain, Size: 1128 bytes --]
On 2012-05-28 15:21, Michael S. Tsirkin wrote:
> On Mon, May 28, 2012 at 02:51:25PM +0200, Jan Kiszka wrote:
>> On 2012-05-28 14:39, Michael S. Tsirkin wrote:
>>> On Fri, May 25, 2012 at 11:02:13AM -0300, Jan Kiszka wrote:
>>>> According to Alexey, the T310 does not properly support INTx masking as
>>>> it fails to keep the PCI_STATUS_INTERRUPT bit updated once the interrupt
>>>> is masked. Mark this adapter as broken so that pci_intx_mask_supported
>>>> won't report it as compatible.
>>>>
>>>> Reported-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>>>> Signed-off-by: Jan Kiszka <jan.kiszka@web.de>
>>>
>>>
>>> Just a thought: would be nice to have a way to discover
>>> the quirk was activated. Add an attribute so that
>>> userspace can detect and report this properly to users?
>>> Or just log a warning message ...
>>
>> pr_notice_once?
>
> OK IMO.
>
>> A flag for userspace would be significantly more
>> complicated (and not PCI layer hands).
>
> Why not? I meant e.g. an attribute in pci-sysfs.
Possible. But what is the preferred way of doing this? Are there any
precedences?
Jan
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] PCI: Mark INTx masking support of Chelsio T310 10GbE NIC as broken
2012-05-28 13:29 ` Jan Kiszka
@ 2012-05-28 13:39 ` Michael S. Tsirkin
2012-05-29 7:51 ` Jan Kiszka
0 siblings, 1 reply; 14+ messages in thread
From: Michael S. Tsirkin @ 2012-05-28 13:39 UTC (permalink / raw)
To: Jan Kiszka
Cc: linux-pci, Bjorn Helgaas, Alexey Kardashevskiy,
Linux Kernel Mailing List, Alex Williamson,
Benjamin Herrenschmidt, David Gibson, Alexander Graf, kvm
On Mon, May 28, 2012 at 03:29:58PM +0200, Jan Kiszka wrote:
> On 2012-05-28 15:21, Michael S. Tsirkin wrote:
> > On Mon, May 28, 2012 at 02:51:25PM +0200, Jan Kiszka wrote:
> >> On 2012-05-28 14:39, Michael S. Tsirkin wrote:
> >>> On Fri, May 25, 2012 at 11:02:13AM -0300, Jan Kiszka wrote:
> >>>> According to Alexey, the T310 does not properly support INTx masking as
> >>>> it fails to keep the PCI_STATUS_INTERRUPT bit updated once the interrupt
> >>>> is masked. Mark this adapter as broken so that pci_intx_mask_supported
> >>>> won't report it as compatible.
> >>>>
> >>>> Reported-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> >>>> Signed-off-by: Jan Kiszka <jan.kiszka@web.de>
> >>>
> >>>
> >>> Just a thought: would be nice to have a way to discover
> >>> the quirk was activated. Add an attribute so that
> >>> userspace can detect and report this properly to users?
> >>> Or just log a warning message ...
> >>
> >> pr_notice_once?
> >
> > OK IMO.
> >
> >> A flag for userspace would be significantly more
> >> complicated (and not PCI layer hands).
> >
> > Why not? I meant e.g. an attribute in pci-sysfs.
>
> Possible. But what is the preferred way of doing this? Are there any
> precedences?
>
> Jan
>
E.g. a reset attribute is there only if device reset is supported.
I don't insist on this - merely asking how does userspace report
an attempt to share IRQs and whether the reason is
discoverable in some way.
--
MST
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] PCI: Mark INTx masking support of Chelsio T310 10GbE NIC as broken
2012-05-28 13:39 ` Michael S. Tsirkin
@ 2012-05-29 7:51 ` Jan Kiszka
2012-05-29 9:51 ` Michael S. Tsirkin
0 siblings, 1 reply; 14+ messages in thread
From: Jan Kiszka @ 2012-05-29 7:51 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: linux-pci, Bjorn Helgaas, Alexey Kardashevskiy,
Linux Kernel Mailing List, Alex Williamson,
Benjamin Herrenschmidt, David Gibson, Alexander Graf, kvm
On 2012-05-28 15:39, Michael S. Tsirkin wrote:
> On Mon, May 28, 2012 at 03:29:58PM +0200, Jan Kiszka wrote:
>> On 2012-05-28 15:21, Michael S. Tsirkin wrote:
>>> On Mon, May 28, 2012 at 02:51:25PM +0200, Jan Kiszka wrote:
>>>> On 2012-05-28 14:39, Michael S. Tsirkin wrote:
>>>>> On Fri, May 25, 2012 at 11:02:13AM -0300, Jan Kiszka wrote:
>>>>>> According to Alexey, the T310 does not properly support INTx masking as
>>>>>> it fails to keep the PCI_STATUS_INTERRUPT bit updated once the interrupt
>>>>>> is masked. Mark this adapter as broken so that pci_intx_mask_supported
>>>>>> won't report it as compatible.
>>>>>>
>>>>>> Reported-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>>>>>> Signed-off-by: Jan Kiszka <jan.kiszka@web.de>
>>>>>
>>>>>
>>>>> Just a thought: would be nice to have a way to discover
>>>>> the quirk was activated. Add an attribute so that
>>>>> userspace can detect and report this properly to users?
>>>>> Or just log a warning message ...
>>>>
>>>> pr_notice_once?
>>>
>>> OK IMO.
>>>
>>>> A flag for userspace would be significantly more
>>>> complicated (and not PCI layer hands).
>>>
>>> Why not? I meant e.g. an attribute in pci-sysfs.
>>
>> Possible. But what is the preferred way of doing this? Are there any
>> precedences?
>>
>> Jan
>>
>
> E.g. a reset attribute is there only if device reset is supported.
> I don't insist on this - merely asking how does userspace report
> an attempt to share IRQs and whether the reason is
> discoverable in some way.
Well, so far there is no attribute associated with INTx masking that we
could hide to express this.
Jan
--
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] PCI: Mark INTx masking support of Chelsio T310 10GbE NIC as broken
2012-05-29 7:51 ` Jan Kiszka
@ 2012-05-29 9:51 ` Michael S. Tsirkin
0 siblings, 0 replies; 14+ messages in thread
From: Michael S. Tsirkin @ 2012-05-29 9:51 UTC (permalink / raw)
To: Jan Kiszka
Cc: linux-pci, Bjorn Helgaas, Alexey Kardashevskiy,
Linux Kernel Mailing List, Alex Williamson,
Benjamin Herrenschmidt, David Gibson, Alexander Graf, kvm
On Tue, May 29, 2012 at 09:51:09AM +0200, Jan Kiszka wrote:
> On 2012-05-28 15:39, Michael S. Tsirkin wrote:
> > On Mon, May 28, 2012 at 03:29:58PM +0200, Jan Kiszka wrote:
> >> On 2012-05-28 15:21, Michael S. Tsirkin wrote:
> >>> On Mon, May 28, 2012 at 02:51:25PM +0200, Jan Kiszka wrote:
> >>>> On 2012-05-28 14:39, Michael S. Tsirkin wrote:
> >>>>> On Fri, May 25, 2012 at 11:02:13AM -0300, Jan Kiszka wrote:
> >>>>>> According to Alexey, the T310 does not properly support INTx masking as
> >>>>>> it fails to keep the PCI_STATUS_INTERRUPT bit updated once the interrupt
> >>>>>> is masked. Mark this adapter as broken so that pci_intx_mask_supported
> >>>>>> won't report it as compatible.
> >>>>>>
> >>>>>> Reported-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> >>>>>> Signed-off-by: Jan Kiszka <jan.kiszka@web.de>
> >>>>>
> >>>>>
> >>>>> Just a thought: would be nice to have a way to discover
> >>>>> the quirk was activated. Add an attribute so that
> >>>>> userspace can detect and report this properly to users?
> >>>>> Or just log a warning message ...
> >>>>
> >>>> pr_notice_once?
> >>>
> >>> OK IMO.
> >>>
> >>>> A flag for userspace would be significantly more
> >>>> complicated (and not PCI layer hands).
> >>>
> >>> Why not? I meant e.g. an attribute in pci-sysfs.
> >>
> >> Possible. But what is the preferred way of doing this? Are there any
> >> precedences?
> >>
> >> Jan
> >>
> >
> > E.g. a reset attribute is there only if device reset is supported.
> > I don't insist on this - merely asking how does userspace report
> > an attempt to share IRQs and whether the reason is
> > discoverable in some way.
>
> Well, so far there is no attribute associated with INTx masking that we
> could hide to express this.
>
> Jan
Thinking about it some more, userspace using this functionality
is pretty recent. So if we just teach it to report
'intx mask or status bit unsupported' on failure,
plus add pr_notice as you suggested, then that's
probably enough.
> --
> Siemens AG, Corporate Technology, CT T DE IT 1
> Corporate Competence Center Embedded Linux
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] PCI: Mark INTx masking support of Chelsio T310 10GbE NIC as broken
2012-05-25 14:02 [PATCH] PCI: Mark INTx masking support of Chelsio T310 10GbE NIC as broken Jan Kiszka
2012-05-25 14:11 ` Bjorn Helgaas
2012-05-28 12:39 ` Michael S. Tsirkin
@ 2012-06-05 14:38 ` Jan Kiszka
2012-06-07 5:14 ` Alexey Kardashevskiy
2 siblings, 1 reply; 14+ messages in thread
From: Jan Kiszka @ 2012-06-05 14:38 UTC (permalink / raw)
To: Alexey Kardashevskiy
Cc: linux-pci, Bjorn Helgaas, Linux Kernel Mailing List,
Alex Williamson, Benjamin Herrenschmidt, David Gibson,
Alexander Graf, kvm
On 2012-05-25 16:02, Jan Kiszka wrote:
> According to Alexey, the T310 does not properly support INTx masking as
> it fails to keep the PCI_STATUS_INTERRUPT bit updated once the interrupt
> is masked. Mark this adapter as broken so that pci_intx_mask_supported
> won't report it as compatible.
>
> Reported-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> Signed-off-by: Jan Kiszka <jan.kiszka@web.de>
> ---
>
> Alexey, please test if this catches your case correctly.
Alexey? Ping for testing.
Jan
>
> drivers/pci/pci.c | 3 +++
> drivers/pci/quirks.c | 12 ++++++++++++
> include/linux/pci.h | 2 ++
> 3 files changed, 17 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 8f16900..3a1aeb5 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -2876,6 +2876,9 @@ bool pci_intx_mask_supported(struct pci_dev *dev)
> bool mask_supported = false;
> u16 orig, new;
>
> + if (dev->broken_intx_masking)
> + return false;
> +
> pci_cfg_access_lock(dev);
>
> pci_read_config_word(dev, PCI_COMMAND, &orig);
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index 2a75216..151e174 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -2929,6 +2929,18 @@ static void __devinit disable_igfx_irq(struct pci_dev *dev)
> DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x0102, disable_igfx_irq);
> DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x010a, disable_igfx_irq);
>
> +/*
> + * Some devices may pass our check in pci_intx_mask_supported if
> + * PCI_COMMAND_INTX_DISABLE works though they actually do not properly
> + * support this feature.
> + */
> +static void __devinit quirk_broken_intx_masking(struct pci_dev *dev)
> +{
> + dev->broken_intx_masking = 1;
> +}
> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_CHELSIO, 0x0010,
> + quirk_broken_intx_masking);
> +
> static void pci_do_fixups(struct pci_dev *dev, struct pci_fixup *f,
> struct pci_fixup *end)
> {
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 17b7b5b..c7cfd73 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -324,6 +324,8 @@ struct pci_dev {
> unsigned int is_hotplug_bridge:1;
> unsigned int __aer_firmware_first_valid:1;
> unsigned int __aer_firmware_first:1;
> + unsigned int broken_intx_masking:1; /* device's INTx masking
> + support is not working */
> pci_dev_flags_t dev_flags;
> atomic_t enable_cnt; /* pci_enable_device has been called */
>
--
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH] PCI: Mark INTx masking support of Chelsio T310 10GbE NIC as broken
2012-06-05 14:38 ` Jan Kiszka
@ 2012-06-07 5:14 ` Alexey Kardashevskiy
2012-06-07 8:30 ` [PATCH v2] " Jan Kiszka
0 siblings, 1 reply; 14+ messages in thread
From: Alexey Kardashevskiy @ 2012-06-07 5:14 UTC (permalink / raw)
To: Jan Kiszka
Cc: linux-pci, Bjorn Helgaas, Linux Kernel Mailing List,
Alex Williamson, Benjamin Herrenschmidt, David Gibson,
Alexander Graf, kvm
On 06/06/12 00:38, Jan Kiszka wrote:
> On 2012-05-25 16:02, Jan Kiszka wrote:
>> According to Alexey, the T310 does not properly support INTx masking as
>> it fails to keep the PCI_STATUS_INTERRUPT bit updated once the interrupt
>> is masked. Mark this adapter as broken so that pci_intx_mask_supported
>> won't report it as compatible.
>>
>> Reported-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>> Signed-off-by: Jan Kiszka <jan.kiszka@web.de>
>> ---
>>
>> Alexey, please test if this catches your case correctly.
>
> Alexey? Ping for testing.
Sorry, was in vacation and then a bit busy.
Yes, that works, thanks. It just a device ID is wrong, should be 0x0030 rather than 0x0010 in your patch - may be 0x10 is broken too, I do not know, mine is 0x30 :)
Here:
>> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_CHELSIO, 0x0010,
>> + quirk_broken_intx_masking);
>
> Jan
>
>>
>> drivers/pci/pci.c | 3 +++
>> drivers/pci/quirks.c | 12 ++++++++++++
>> include/linux/pci.h | 2 ++
>> 3 files changed, 17 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>> index 8f16900..3a1aeb5 100644
>> --- a/drivers/pci/pci.c
>> +++ b/drivers/pci/pci.c
>> @@ -2876,6 +2876,9 @@ bool pci_intx_mask_supported(struct pci_dev *dev)
>> bool mask_supported = false;
>> u16 orig, new;
>>
>> + if (dev->broken_intx_masking)
>> + return false;
>> +
>> pci_cfg_access_lock(dev);
>>
>> pci_read_config_word(dev, PCI_COMMAND, &orig);
>> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
>> index 2a75216..151e174 100644
>> --- a/drivers/pci/quirks.c
>> +++ b/drivers/pci/quirks.c
>> @@ -2929,6 +2929,18 @@ static void __devinit disable_igfx_irq(struct pci_dev *dev)
>> DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x0102, disable_igfx_irq);
>> DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x010a, disable_igfx_irq);
>>
>> +/*
>> + * Some devices may pass our check in pci_intx_mask_supported if
>> + * PCI_COMMAND_INTX_DISABLE works though they actually do not properly
>> + * support this feature.
>> + */
>> +static void __devinit quirk_broken_intx_masking(struct pci_dev *dev)
>> +{
>> + dev->broken_intx_masking = 1;
>> +}
>> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_CHELSIO, 0x0010,
>> + quirk_broken_intx_masking);
>> +
>> static void pci_do_fixups(struct pci_dev *dev, struct pci_fixup *f,
>> struct pci_fixup *end)
>> {
>> diff --git a/include/linux/pci.h b/include/linux/pci.h
>> index 17b7b5b..c7cfd73 100644
>> --- a/include/linux/pci.h
>> +++ b/include/linux/pci.h
>> @@ -324,6 +324,8 @@ struct pci_dev {
>> unsigned int is_hotplug_bridge:1;
>> unsigned int __aer_firmware_first_valid:1;
>> unsigned int __aer_firmware_first:1;
>> + unsigned int broken_intx_masking:1; /* device's INTx masking
>> + support is not working */
>> pci_dev_flags_t dev_flags;
>> atomic_t enable_cnt; /* pci_enable_device has been called */
>>
>
--
Alexey
^ permalink raw reply [flat|nested] 14+ messages in thread* [PATCH v2] PCI: Mark INTx masking support of Chelsio T310 10GbE NIC as broken
2012-06-07 5:14 ` Alexey Kardashevskiy
@ 2012-06-07 8:30 ` Jan Kiszka
2012-06-18 18:29 ` Bjorn Helgaas
0 siblings, 1 reply; 14+ messages in thread
From: Jan Kiszka @ 2012-06-07 8:30 UTC (permalink / raw)
To: linux-pci, Bjorn Helgaas
Cc: Alexey Kardashevskiy, Linux Kernel Mailing List, Alex Williamson,
Benjamin Herrenschmidt, David Gibson, Alexander Graf, kvm
From: Jan Kiszka <jan.kiszka@siemens.com>
According to
http://thread.gmane.org/gmane.comp.emulators.kvm.devel/91388
the T310 does not properly support INTx masking as it fails to keep the
PCI_STATUS_INTERRUPT bit updated once the interrupt is masked. Mark this
adapter as broken so that pci_intx_mask_supported won't report it as
compatible.
Tested-by: Alexey Kardashevskiy <aik@ozlabs.ru>
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
Changes in v2:
- Fixed device ID to Alexey's report
- Added reference to the original report
drivers/pci/pci.c | 3 +++
drivers/pci/quirks.c | 12 ++++++++++++
include/linux/pci.h | 2 ++
3 files changed, 17 insertions(+), 0 deletions(-)
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 447e834..9ae517a 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -2876,6 +2876,9 @@ bool pci_intx_mask_supported(struct pci_dev *dev)
bool mask_supported = false;
u16 orig, new;
+ if (dev->broken_intx_masking)
+ return false;
+
pci_cfg_access_lock(dev);
pci_read_config_word(dev, PCI_COMMAND, &orig);
diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 2a75216..28804f4 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -2929,6 +2929,18 @@ static void __devinit disable_igfx_irq(struct pci_dev *dev)
DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x0102, disable_igfx_irq);
DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x010a, disable_igfx_irq);
+/*
+ * Some devices may pass our check in pci_intx_mask_supported if
+ * PCI_COMMAND_INTX_DISABLE works though they actually do not properly
+ * support this feature.
+ */
+static void __devinit quirk_broken_intx_masking(struct pci_dev *dev)
+{
+ dev->broken_intx_masking = 1;
+}
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_CHELSIO, 0x0030,
+ quirk_broken_intx_masking);
+
static void pci_do_fixups(struct pci_dev *dev, struct pci_fixup *f,
struct pci_fixup *end)
{
diff --git a/include/linux/pci.h b/include/linux/pci.h
index d8c379d..7ea6cf8 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -324,6 +324,8 @@ struct pci_dev {
unsigned int is_hotplug_bridge:1;
unsigned int __aer_firmware_first_valid:1;
unsigned int __aer_firmware_first:1;
+ unsigned int broken_intx_masking:1; /* device's INTx masking
+ support is not working */
pci_dev_flags_t dev_flags;
atomic_t enable_cnt; /* pci_enable_device has been called */
--
1.7.3.4
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH v2] PCI: Mark INTx masking support of Chelsio T310 10GbE NIC as broken
2012-06-07 8:30 ` [PATCH v2] " Jan Kiszka
@ 2012-06-18 18:29 ` Bjorn Helgaas
0 siblings, 0 replies; 14+ messages in thread
From: Bjorn Helgaas @ 2012-06-18 18:29 UTC (permalink / raw)
To: Jan Kiszka
Cc: linux-pci, Alexey Kardashevskiy, Linux Kernel Mailing List,
Alex Williamson, Benjamin Herrenschmidt, David Gibson,
Alexander Graf, kvm
On Thu, Jun 7, 2012 at 2:30 AM, Jan Kiszka <jan.kiszka@web.de> wrote:
> From: Jan Kiszka <jan.kiszka@siemens.com>
>
> According to
>
> http://thread.gmane.org/gmane.comp.emulators.kvm.devel/91388
>
> the T310 does not properly support INTx masking as it fails to keep the
> PCI_STATUS_INTERRUPT bit updated once the interrupt is masked. Mark this
> adapter as broken so that pci_intx_mask_supported won't report it as
> compatible.
>
> Tested-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>
> Changes in v2:
> - Fixed device ID to Alexey's report
> - Added reference to the original report
>
> drivers/pci/pci.c | 3 +++
> drivers/pci/quirks.c | 12 ++++++++++++
> include/linux/pci.h | 2 ++
> 3 files changed, 17 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 447e834..9ae517a 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -2876,6 +2876,9 @@ bool pci_intx_mask_supported(struct pci_dev *dev)
> bool mask_supported = false;
> u16 orig, new;
>
> + if (dev->broken_intx_masking)
> + return false;
> +
> pci_cfg_access_lock(dev);
>
> pci_read_config_word(dev, PCI_COMMAND, &orig);
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index 2a75216..28804f4 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -2929,6 +2929,18 @@ static void __devinit disable_igfx_irq(struct pci_dev *dev)
> DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x0102, disable_igfx_irq);
> DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x010a, disable_igfx_irq);
>
> +/*
> + * Some devices may pass our check in pci_intx_mask_supported if
> + * PCI_COMMAND_INTX_DISABLE works though they actually do not properly
> + * support this feature.
> + */
> +static void __devinit quirk_broken_intx_masking(struct pci_dev *dev)
> +{
> + dev->broken_intx_masking = 1;
> +}
> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_CHELSIO, 0x0030,
> + quirk_broken_intx_masking);
> +
> static void pci_do_fixups(struct pci_dev *dev, struct pci_fixup *f,
> struct pci_fixup *end)
> {
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index d8c379d..7ea6cf8 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -324,6 +324,8 @@ struct pci_dev {
> unsigned int is_hotplug_bridge:1;
> unsigned int __aer_firmware_first_valid:1;
> unsigned int __aer_firmware_first:1;
> + unsigned int broken_intx_masking:1; /* device's INTx masking
> + support is not working */
> pci_dev_flags_t dev_flags;
> atomic_t enable_cnt; /* pci_enable_device has been called */
>
> --
> 1.7.3.4
I applied this to my "next" branch with minor tweaks (split the
infrastructure part from the device-specific quirk, and changed from a
FINAL quirk to a HEADER quirk because FINAL quirks currently aren't
called for hot-added devices). Let me know if you see anything wrong.
Thanks!
Bjorn
^ permalink raw reply [flat|nested] 14+ messages in thread