Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1290015 > unrolled thread

Re: [PATCH v2] PCI/AER: enable SERR# forwarding for bridges and switches

Started bySinan Kaya <okaya@codeaurora.org>
First post2015-12-12 00:40 +0100
Last post2015-12-30 14:30 +0100
Articles 4 — 2 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v2] PCI/AER: enable SERR# forwarding for bridges and  switches Sinan Kaya <okaya@codeaurora.org> - 2015-12-12 00:40 +0100
    Re: [PATCH v2] PCI/AER: enable SERR# forwarding for bridges and  switches Sinan Kaya <okaya@codeaurora.org> - 2015-12-14 19:30 +0100
      Re: [PATCH v2] PCI/AER: enable SERR# forwarding for bridges and  switches Bjorn Helgaas <helgaas@kernel.org> - 2015-12-28 23:30 +0100
        Re: [PATCH v2] PCI/AER: enable SERR# forwarding for bridges and  switches Sinan Kaya <okaya@codeaurora.org> - 2015-12-30 14:30 +0100

#1290015 — Re: [PATCH v2] PCI/AER: enable SERR# forwarding for bridges and switches

FromSinan Kaya <okaya@codeaurora.org>
Date2015-12-12 00:40 +0100
SubjectRe: [PATCH v2] PCI/AER: enable SERR# forwarding for bridges and switches
Message-ID<qEFs6-2HI-11@gated-at.bofh.it>
On 12/10/2015 5:37 PM, Bjorn Helgaas wrote:
> On Thu, Dec 10, 2015 at 03:28:35PM -0500, Sinan Kaya wrote:
>> Hi Bjorn,
>>
>> On 12/4/2015 4:06 PM, Bjorn Helgaas wrote:
>>> I'm sitting on this for the moment because if you have _HPP, it seems
>>> like that should be enough to get SERR# forwarding turned on, and if
>>> it's not, I'd like to understand why.  So no hurry, but I'm waiting on
>>> your investigation.
>>>
>>> Bjorn
>>
>> Here is the overall summary after my investigation.
>>
>> It looks like the kernel covers the hotplug use case. This patch is
>> needed for systems without hotplug support and when the firmware is not
>> setting up the SERR.
> 
> Here's how I understand your results:
> 
>   Firmware   _HPP or     Devices      Hot-added    Hot-added
>   enables    _HPX sets   present at   root ports   endpoints
>   SERR#      SERR#       boot work    work         work
>   --------   ---------   ----------   ----------   ---------
>   no         no          no (1)       no (2)       no (4)
>   no         yes         yes          yes          yes
>   yes        no          yes          no (3)       no (5)
>   yes        yes         yes          yes          yes
> 
> Your patch fixes cases 1-3 above, but I don't think it fixes cases 4
> or 5.
> 

I think so. I don't supported 4 and 5 cases on our platform. But, I
could write up a patch if somebody can test it on such a platform.

> The difference is that in cases 2 and 3, when we hot-add a root port,
> the AER driver binds to the root port and (with your patch) enables
> SERR for anything below it.
> 
> But in cases 4 and 5, the root port is already there, the AER driver
> has already bound to it.  The AER driver tried to enable SERR for the
> hierarchy below the root port, but there was nothing there.  Now we
> add the endpoint, and the AER driver isn't involved, so I don't think
> anything will enable SERR for the new endpoint.
> 
> I think the best way to fix all the cases would be to do something in
> in pci_configure_device().  Then we could drop the AER bus walk in
> set_downstream_devices_error_reporting().  A bus walk like that is
> always an issue for hotplug.
> 

Let me read some code.

> In principle, we should be able to just enable PCI_COMMAND_SERR and
> PCI_BRIDGE_CTL_SERR for everything, and then errors would get
> forwarded to the root port, and if/when the AER driver claimed the
> root port, it would start collecting them.
> 
> But I'm a little leery of doing it unconditionally because there are a
> lot of platform- and driver-specific uses of those bits, and I'm
> afraid of breaking something.  It might be possible, but it'll take
> some care to do it safely.

Sure, when we were talking about ECRC the other day; you said we could
enable it on platforms post 2000 using some SMBIOS API. We could go the
same route here.

> 
>> after boot
>>
>> /# dmesg | grep hpp
>>
>> [    3.115227] pci 0004:01:00.0: program_hpp_type0:1376
>> [    3.128870] pci 0004:01:00.0: program_hpp_type0:1378 pci_cmd |=
>> PCI_COMMAND_SERR
>> [    3.149597] pci 0004:01:00.0: program_hpp_type0:1391 pci_bctl |=
>> PCI_BRIDGE_CTL_SERR
>> [    3.191601] pci 0004:02:08.0: program_hpp_type0:1376
>> [    3.191611] pci 0004:02:08.0: program_hpp_type0:1378 pci_cmd |=
>> PCI_COMMAND_SERR
>> [    3.206630] pci 0004:02:08.0: program_hpp_type0:1391 pci_bctl |=
>> PCI_BRIDGE_CTL_SERR
>> [    3.253760] pci 0004:03:00.0: program_hpp_type0:1376
>> [    3.267335] pci 0004:03:00.0: program_hpp_type0:1378 pci_cmd |=
>> PCI_COMMAND_SERR
>> [    3.288046] pci 0004:03:00.0: program_hpp_type0:1391 pci_bctl |=
>> PCI_BRIDGE_CTL_SERR
>> [    3.355296] pci 0004:04:00.0: program_hpp_type0:1376
>> [    3.355306] pci 0004:04:00.0: program_hpp_type0:1378 pci_cmd |=
>> PCI_COMMAND_SERR
>> [    3.370334] pci 0004:04:00.0: program_hpp_type0:1391 pci_bctl |=
>> PCI_BRIDGE_CTL_SERR
>>
>> / # lspci
>> 00:00.0 Class 0604: 17cb:0400
>> 01:00.0 Class 0604: 10b5:8732
>> 02:08.0 Class 0604: 10b5:8732
>> 03:00.0 Class 0604: 10b5:8732
>> 04:00.0 Class 0604: 10b5:8732
>> / #
>>
>>
>> Without hpp in ACPI table, SERR is not enabled.
>>
>> /# dmesg | grep type0
>> /#
>>
>> Power up with HPP after boot.
>>
>> [    3.129325]_pci_0004:01:00.0:_program_hpp_type0:1376
>> [    3.143286] pci 0004:01:00.0: program_hpp_type0:1378 pci_cmd |=
>> PCI_COMMAND_SERR
>> [    3.164016] pci 0004:01:00.0: program_hpp_type0:1391 pci_bctl |=
>> PCI_BRIDGE_CTL_SERR
>> [    3.206019] pci 0004:02:08.0: program_hpp_type0:1376
>> [    3.206028] pci 0004:02:08.0: program_hpp_type0:1378 pci_cmd |=
>> PCI_COMMAND_SERR
>> [    3.220609] pci 0004:02:08.0: program_hpp_type0:1391 pci_bctl |=
>> PCI_BRIDGE_CTL_SERR
>> [    3.267783] pci 0004:03:00.0: program_hpp_type0:1376
>> [    3.281420] pci 0004:03:00.0: program_hpp_type0:1378 pci_cmd |=
>> PCI_COMMAND_SERR
>> [    3.302197] pci 0004:03:00.0: program_hpp_type0:1391 pci_bctl |=
>> PCI_BRIDGE_CTL_SERR
>> [    3.369684] pci 0004:04:00.0: program_hpp_type0:1376
>> [    3.369694] pci 0004:04:00.0: program_hpp_type0:1378 pci_cmd |=
>> PCI_COMMAND_SERR
>> [    3.384080] pci 0004:04:00.0: program_hpp_type0:1391 pci_bctl |=
>> PCI_BRIDGE_CTL_SERR
>>
>> hotplug eject
>>
>> hotplug insert
>>
>> [   98.338131] pci 0004:01:00.0: program_hpp_type0:1376
>> [   98.351813] pci 0004:01:00.0: program_hpp_type0:1378 pci_cmd |=
>> PCI_COMMAND_SERR
>> [   98.373147] pci 0004:01:00.0: program_hpp_type0:1391 pci_bctl |=
>> PCI_BRIDGE_CTL_SERR
>> [   98.452051] pci 0004:02:08.0: program_hpp_type0:1376
>> [   98.465772] pci 0004:02:08.0: program_hpp_type0:1378 pci_cmd |=
>> PCI_COMMAND_SERR
>> [   98.487142] pci 0004:02:08.0: program_hpp_type0:1391 pci_bctl |=
>> PCI_BRIDGE_CTL_SERR
>> [   98.597579] pci 0004:03:00.0: program_hpp_type0:1376
>> [   98.611290] pci 0004:03:00.0: program_hpp_type0:1378 pci_cmd |=
>> PCI_COMMAND_SERR
>> [   98.632181] pci 0004:03:00.0: program_hpp_type0:1391 pci_bctl |=
>> PCI_BRIDGE_CTL_SERR
>> [   98.736153] pci 0004:04:00.0: program_hpp_type0:1376
>> [   98.750437] pci 0004:04:00.0: program_hpp_type0:1378 pci_cmd |=
>> PCI_COMMAND_SERR
>> [   98.771202] pci 0004:04:00.0: program_hpp_type0:1391 pci_bctl |=
>> PCI_BRIDGE_CTL_SERR
>> / #
>>
>>
>>
>> -- 
>> Sinan Kaya
>> Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
>> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a
>> Linux Foundation Collaborative Project
>> --
>> 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/
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 


-- 
Sinan Kaya
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a
Linux Foundation Collaborative Project
--
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/

[toc] | [next] | [standalone]

#1291421

FromSinan Kaya <okaya@codeaurora.org>
Date2015-12-14 19:30 +0100
Message-ID<qFG2K-1kV-17@gated-at.bofh.it>
In reply to#1290015
On 12/11/2015 6:30 PM, Sinan Kaya wrote:
>> I think the best way to fix all the cases would be to do something in
>> > in pci_configure_device().  Then we could drop the AER bus walk in
>> > set_downstream_devices_error_reporting().  A bus walk like that is
>> > always an issue for hotplug.
>> > 
> Let me read some code.
> 

OK, If I understand it right; pci_configure_device is where
program_hpp_type0 called. You also want to enable AER in this function.

Move the contents of set_device_error_reporting into
pci_configure_device like this below ?

...
+ int type = pci_pcie_type(dev);

pci_configure_mps(dev);

+	if ((type == PCI_EXP_TYPE_ROOT_PORT) ||
+	    (type == PCI_EXP_TYPE_UPSTREAM) ||
+	    (type == PCI_EXP_TYPE_DOWNSTREAM)) {
+		pci_enable_pcie_error_reporting(dev);
+	}

+	pcie_set_ecrc_checking(dev);




-- 
Sinan Kaya
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a
Linux Foundation Collaborative Project
--
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/

[toc] | [prev] | [next] | [standalone]

#1298780

FromBjorn Helgaas <helgaas@kernel.org>
Date2015-12-28 23:30 +0100
Message-ID<qKOsG-8lW-23@gated-at.bofh.it>
In reply to#1291421
Hi Sinan,

Sorry for the delay in responding; I was on vacation when you sent
this, and I missed it when I returned.

On Mon, Dec 14, 2015 at 01:22:39PM -0500, Sinan Kaya wrote:
> On 12/11/2015 6:30 PM, Sinan Kaya wrote:
> >> I think the best way to fix all the cases would be to do something in
> >> > in pci_configure_device().  Then we could drop the AER bus walk in
> >> > set_downstream_devices_error_reporting().  A bus walk like that is
> >> > always an issue for hotplug.
> >> > 
> > Let me read some code.
> 
> OK, If I understand it right; pci_configure_device is where
> program_hpp_type0 called. You also want to enable AER in this function.
> 
> Move the contents of set_device_error_reporting into
> pci_configure_device like this below ?
> 
> ...
> + int type = pci_pcie_type(dev);
> 
> pci_configure_mps(dev);
> 
> +	if ((type == PCI_EXP_TYPE_ROOT_PORT) ||
> +	    (type == PCI_EXP_TYPE_UPSTREAM) ||
> +	    (type == PCI_EXP_TYPE_DOWNSTREAM)) {
> +		pci_enable_pcie_error_reporting(dev);
> +	}
> 
> +	pcie_set_ecrc_checking(dev);

Yep, that's the sort of thing I'm thinking.

I think there are some subtleties to consider.

_HPP/_HPX can twiddle some of the same bits.  Should we allow _HPP to
clear a bit that pci_enable_pcie_error_reporting() would set?  What
about the reverse?

There are a ridiculous number of places that call
pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ERR).  We should
do it once and cache the result in struct pci_dev.

Bjorn
--
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/

[toc] | [prev] | [next] | [standalone]

#1299349

FromSinan Kaya <okaya@codeaurora.org>
Date2015-12-30 14:30 +0100
Message-ID<qLoZc-79S-11@gated-at.bofh.it>
In reply to#1298780
On 12/28/2015 5:29 PM, Bjorn Helgaas wrote:
> Hi Sinan,
> 
> Sorry for the delay in responding; I was on vacation when you sent
> this, and I missed it when I returned.
> 
> On Mon, Dec 14, 2015 at 01:22:39PM -0500, Sinan Kaya wrote:
>> On 12/11/2015 6:30 PM, Sinan Kaya wrote:
>>>> I think the best way to fix all the cases would be to do something in
>>>>> in pci_configure_device().  Then we could drop the AER bus walk in
>>>>> set_downstream_devices_error_reporting().  A bus walk like that is
>>>>> always an issue for hotplug.
>>>>>
>>> Let me read some code.
>>
>> OK, If I understand it right; pci_configure_device is where
>> program_hpp_type0 called. You also want to enable AER in this function.
>>
>> Move the contents of set_device_error_reporting into
>> pci_configure_device like this below ?
>>
>> ...
>> + int type = pci_pcie_type(dev);
>>
>> pci_configure_mps(dev);
>>
>> +	if ((type == PCI_EXP_TYPE_ROOT_PORT) ||
>> +	    (type == PCI_EXP_TYPE_UPSTREAM) ||
>> +	    (type == PCI_EXP_TYPE_DOWNSTREAM)) {
>> +		pci_enable_pcie_error_reporting(dev);
>> +	}
>>
>> +	pcie_set_ecrc_checking(dev);
> 
> Yep, that's the sort of thing I'm thinking.
> 
> I think there are some subtleties to consider.
> 
> _HPP/_HPX can twiddle some of the same bits.  Should we allow _HPP to
> clear a bit that pci_enable_pcie_error_reporting() would set?  What
> about the reverse?
> 
> There are a ridiculous number of places that call
> pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ERR).  We should
> do it once and cache the result in struct pci_dev.
> 
> Bjorn
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Thanks for the heads up. I had another pending question to you on another patch.
I just sent a ping. 

I'll start working on this patch after the holidays.




-- 
Sinan Kaya
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
--
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/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web