* Re: [PATCH v2] PCI: pciehp: Quirk to ignore spurious DLLSC when off
2021-08-23 18:49 [PATCH v2] PCI: pciehp: Quirk to ignore spurious DLLSC when off Jon Derrick
@ 2021-08-23 21:19 ` kernel test robot
2021-08-24 0:37 ` Bjorn Helgaas
2021-08-24 0:41 ` Raj, Ashok
2 siblings, 0 replies; 5+ messages in thread
From: kernel test robot @ 2021-08-23 21:19 UTC (permalink / raw)
To: Jon Derrick, linux-pci, Bjorn Helgaas
Cc: kbuild-all, Ashok Raj, Lukas Wunner, jonathan.derrick,
James Puthukattukaran, Jon Derrick
[-- Attachment #1: Type: text/plain, Size: 3672 bytes --]
Hi Jon,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on pci/next]
[also build test ERROR on v5.14-rc7 next-20210823]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Jon-Derrick/PCI-pciehp-Quirk-to-ignore-spurious-DLLSC-when-off/20210824-025108
base: https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git next
config: arm-randconfig-r021-20210822 (attached as .config)
compiler: arm-linux-gnueabi-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/64e85d3cc3a64aea26c4b89c9661ee4eef99ac5e
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Jon-Derrick/PCI-pciehp-Quirk-to-ignore-spurious-DLLSC-when-off/20210824-025108
git checkout 64e85d3cc3a64aea26c4b89c9661ee4eef99ac5e
# save the attached .config to linux build tree
mkdir build_dir
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=arm SHELL=/bin/bash drivers/
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
drivers/pci/quirks.c: In function 'shared_pcc_and_link_slot':
>> drivers/pci/quirks.c:5778:31: error: 'struct pci_dev' has no member named 'shared_pcc_and_link_slot'
5778 | parent->shared_pcc_and_link_slot = 1;
| ^~
vim +5778 drivers/pci/quirks.c
5745
5746 static void apex_pci_fixup_class(struct pci_dev *pdev)
5747 {
5748 pdev->class = (PCI_CLASS_SYSTEM_OTHER << 8) | pdev->class;
5749 }
5750 DECLARE_PCI_FIXUP_CLASS_HEADER(0x1ac1, 0x089a,
5751 PCI_CLASS_NOT_DEFINED, 8, apex_pci_fixup_class);
5752
5753 /*
5754 * This is a special card that sits in a x8 pciehp slot but is bifurcated as
5755 * a x4x4 and manifests as two slots with respect to PCIe hot plug register
5756 * states. However, the hotplug controller treats these slots as a single x8
5757 * slot for link and power. Either one of the two slots can be powered down
5758 * separately but real power and link will be active till the last of the two
5759 * slots is powered down. When the last of the two x4 slots is turned off,
5760 * power and link will be turned off for the x8 slot by the HP controller.
5761 * This configuration causes some interesting behavior in bringup sequence
5762 *
5763 * When the second slot is powered off to remove the card, this will cause
5764 * the link to go down for both x4 slots. So, the x4 that is already powered
5765 * down earlier will see a DLLSC event and attempt to bring itself up (card
5766 * present, link change event, link state is down). Special handling is
5767 * required in pciehp_handle_presence_or_link_change to prevent this unintended
5768 * bring up
5769 *
5770 */
5771 static void shared_pcc_and_link_slot(struct pci_dev *pdev)
5772 {
5773 struct pci_dev *parent = pci_upstream_bridge(pdev);
5774
5775 if (pdev->subsystem_vendor == 0x108e &&
5776 pdev->subsystem_device == 0x487d) {
5777 if (parent)
> 5778 parent->shared_pcc_and_link_slot = 1;
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 30148 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH v2] PCI: pciehp: Quirk to ignore spurious DLLSC when off
2021-08-23 18:49 [PATCH v2] PCI: pciehp: Quirk to ignore spurious DLLSC when off Jon Derrick
2021-08-23 21:19 ` kernel test robot
@ 2021-08-24 0:37 ` Bjorn Helgaas
2021-08-24 0:41 ` Raj, Ashok
2 siblings, 0 replies; 5+ messages in thread
From: Bjorn Helgaas @ 2021-08-24 0:37 UTC (permalink / raw)
To: Jon Derrick
Cc: linux-pci, Ashok Raj, Lukas Wunner, jonathan.derrick,
James Puthukattukaran
Subject should start with a verb (see history for examples). "when
off"? Needs a little more context to make sense by itself.
On Mon, Aug 23, 2021 at 12:49:19PM -0600, Jon Derrick wrote:
> From: James Puthukattukaran <james.puthukattukaran@oracle.com>
>
> When a specific x8 CEM card is bifurcated into x4x4 mode, and the
> upstream ports both support hotplugging on each respective x4 device, a
> slot management system for the CEM card requires both x4 devices to be
> sysfs removed from the OS before it can safely turn-off physical power.
> The implications are that Slot Control will display Powered Off status
> for the device where the device is actually powered until both ports
> have powered off.
Apparently this is related to a "specific x8 CEM card"? Please
identify it (marketing name, model, etc -- some way a user can tell
whether this quirk applies to the hardware he/she is holding),
describe it, and make a case for why we care about it. E.g., if this
is a shipping product, we probably do care; if it's just a lab
fixture, maybe not.
> When power is removed from the first half, real power and link remains
> active while waiting for the second half to have power removed. When
> power is then removed from the second half, the first half starts
> shutdown sequence and will trigger a DLLSC event. This is misinterpreted
> as an enabling event and causes the first half to be re-enabled.
>
> The spurious enable can be resolved by ignoring link status change
> events when no link is active when in the off state. This patch adds a
> quirk for the card.
>
> Acked-by: Jon Derrick <jonathan.derrick@intel.com>
> Signed-off-by: James Puthukattukaran <james.puthukattukaran@oracle.com>
> ---
> v1->v2: Device-specific quirk
>
> drivers/pci/hotplug/pciehp_ctrl.c | 7 +++++++
> drivers/pci/quirks.c | 30 ++++++++++++++++++++++++++++++
> include/linux/pci.h | 1 +
> 3 files changed, 38 insertions(+)
>
> diff --git a/drivers/pci/hotplug/pciehp_ctrl.c b/drivers/pci/hotplug/pciehp_ctrl.c
> index 529c34808440..db41f78bfac8 100644
> --- a/drivers/pci/hotplug/pciehp_ctrl.c
> +++ b/drivers/pci/hotplug/pciehp_ctrl.c
> @@ -225,6 +225,7 @@ void pciehp_handle_disable_request(struct controller *ctrl)
> void pciehp_handle_presence_or_link_change(struct controller *ctrl, u32 events)
> {
> int present, link_active;
> + struct pci_dev *pdev = ctrl->pcie->port;
>
> /*
> * If the slot is on and presence or link has changed, turn it off.
> @@ -265,6 +266,12 @@ void pciehp_handle_presence_or_link_change(struct controller *ctrl, u32 events)
> cancel_delayed_work(&ctrl->button_work);
> fallthrough;
> case OFF_STATE:
> + if (pdev->shared_pcc_and_link_slot &&
> + (events & PCI_EXP_SLTSTA_DLLSC) && !link_active) {
> + mutex_unlock(&ctrl->state_lock);
> + break;
> + }
> +
> ctrl->state = POWERON_STATE;
> mutex_unlock(&ctrl->state_lock);
> if (present)
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index 43fbf55871ef..92a5bae8926e 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -5749,3 +5749,33 @@ static void apex_pci_fixup_class(struct pci_dev *pdev)
> }
> DECLARE_PCI_FIXUP_CLASS_HEADER(0x1ac1, 0x089a,
> PCI_CLASS_NOT_DEFINED, 8, apex_pci_fixup_class);
> +
> +/*
> + * This is a special card that sits in a x8 pciehp slot but is bifurcated as
> + * a x4x4 and manifests as two slots with respect to PCIe hot plug register
> + * states. However, the hotplug controller treats these slots as a single x8
> + * slot for link and power. Either one of the two slots can be powered down
> + * separately but real power and link will be active till the last of the two
> + * slots is powered down. When the last of the two x4 slots is turned off,
> + * power and link will be turned off for the x8 slot by the HP controller.
> + * This configuration causes some interesting behavior in bringup sequence
> + *
> + * When the second slot is powered off to remove the card, this will cause
> + * the link to go down for both x4 slots. So, the x4 that is already powered
> + * down earlier will see a DLLSC event and attempt to bring itself up (card
> + * present, link change event, link state is down). Special handling is
> + * required in pciehp_handle_presence_or_link_change to prevent this unintended
> + * bring up
> + *
> + */
> +static void shared_pcc_and_link_slot(struct pci_dev *pdev)
> +{
> + struct pci_dev *parent = pci_upstream_bridge(pdev);
> +
> + if (pdev->subsystem_vendor == 0x108e &&
> + pdev->subsystem_device == 0x487d) {
> + if (parent)
> + parent->shared_pcc_and_link_slot = 1;
> + }
> +}
> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x0B60, shared_pcc_and_link_slot);
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index e752cc39a1fe..ba84f7c93c31 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -469,6 +469,7 @@ struct pci_dev {
>
> #ifdef CONFIG_HOTPLUG_PCI_PCIE
> unsigned int broken_cmd_compl:1; /* No compl for some cmds */
> + unsigned int shared_pcc_and_link_slot:1;
> #endif
> #ifdef CONFIG_PCIE_PTM
> unsigned int ptm_root:1;
> --
> 2.27.0
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH v2] PCI: pciehp: Quirk to ignore spurious DLLSC when off
2021-08-23 18:49 [PATCH v2] PCI: pciehp: Quirk to ignore spurious DLLSC when off Jon Derrick
2021-08-23 21:19 ` kernel test robot
2021-08-24 0:37 ` Bjorn Helgaas
@ 2021-08-24 0:41 ` Raj, Ashok
2021-08-26 19:05 ` Jon Derrick
2 siblings, 1 reply; 5+ messages in thread
From: Raj, Ashok @ 2021-08-24 0:41 UTC (permalink / raw)
To: Jon Derrick
Cc: linux-pci, Bjorn Helgaas, Lukas Wunner, jonathan.derrick,
James Puthukattukaran, Ashok Raj
On Mon, Aug 23, 2021 at 12:49:19PM -0600, Derrick, Jonathan wrote:
> From: James Puthukattukaran <james.puthukattukaran@oracle.com>
>
> When a specific x8 CEM card is bifurcated into x4x4 mode, and the
> upstream ports both support hotplugging on each respective x4 device, a
> slot management system for the CEM card requires both x4 devices to be
> sysfs removed from the OS before it can safely turn-off physical power.
sysfs removed from the OS seems a bit confusing.. Do you mean removed via
sysfs by echo 0 > power?
> The implications are that Slot Control will display Powered Off status
> for the device where the device is actually powered until both ports
> have powered off.
>
> When power is removed from the first half, real power and link remains
> active while waiting for the second half to have power removed. When
> power is then removed from the second half, the first half starts
> shutdown sequence and will trigger a DLLSC event. This is misinterpreted
> as an enabling event and causes the first half to be re-enabled.
>
> The spurious enable can be resolved by ignoring link status change
> events when no link is active when in the off state. This patch adds a
> quirk for the card.
>
> Acked-by: Jon Derrick <jonathan.derrick@intel.com>
> Signed-off-by: James Puthukattukaran <james.puthukattukaran@oracle.com>
> ---
> v1->v2: Device-specific quirk
>
> drivers/pci/hotplug/pciehp_ctrl.c | 7 +++++++
> drivers/pci/quirks.c | 30 ++++++++++++++++++++++++++++++
> include/linux/pci.h | 1 +
> 3 files changed, 38 insertions(+)
>
> diff --git a/drivers/pci/hotplug/pciehp_ctrl.c b/drivers/pci/hotplug/pciehp_ctrl.c
> index 529c34808440..db41f78bfac8 100644
> --- a/drivers/pci/hotplug/pciehp_ctrl.c
> +++ b/drivers/pci/hotplug/pciehp_ctrl.c
> @@ -225,6 +225,7 @@ void pciehp_handle_disable_request(struct controller *ctrl)
> void pciehp_handle_presence_or_link_change(struct controller *ctrl, u32 events)
> {
> int present, link_active;
> + struct pci_dev *pdev = ctrl->pcie->port;
>
> /*
> * If the slot is on and presence or link has changed, turn it off.
> @@ -265,6 +266,12 @@ void pciehp_handle_presence_or_link_change(struct controller *ctrl, u32 events)
> cancel_delayed_work(&ctrl->button_work);
> fallthrough;
> case OFF_STATE:
> + if (pdev->shared_pcc_and_link_slot &&
> + (events & PCI_EXP_SLTSTA_DLLSC) && !link_active) {
Is it !link_active? Based on the explanation, until we turn off both slots
you would see link is still active correct?
> + mutex_unlock(&ctrl->state_lock);
> + break;
> + }
> +
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH v2] PCI: pciehp: Quirk to ignore spurious DLLSC when off
2021-08-24 0:41 ` Raj, Ashok
@ 2021-08-26 19:05 ` Jon Derrick
0 siblings, 0 replies; 5+ messages in thread
From: Jon Derrick @ 2021-08-26 19:05 UTC (permalink / raw)
To: Raj, Ashok
Cc: linux-pci@vger.kernel.org, Bjorn Helgaas, Lukas Wunner,
jonathan.derrick@linux.dev, James Puthukattukaran
On 8/23/21 6:41 PM, Raj, Ashok wrote:
> On Mon, Aug 23, 2021 at 12:49:19PM -0600, Derrick, Jonathan wrote:
>> From: James Puthukattukaran <james.puthukattukaran@oracle.com>
>>
>> When a specific x8 CEM card is bifurcated into x4x4 mode, and the
>> upstream ports both support hotplugging on each respective x4 device, a
>> slot management system for the CEM card requires both x4 devices to be
>> sysfs removed from the OS before it can safely turn-off physical power.
>
> sysfs removed from the OS seems a bit confusing.. Do you mean removed via
> sysfs by echo 0 > power?
Yes it seems to be related to slot power and not just virtual removal
>
>> The implications are that Slot Control will display Powered Off status
>> for the device where the device is actually powered until both ports
>> have powered off.
>>
>> When power is removed from the first half, real power and link remains
>> active while waiting for the second half to have power removed. When
>> power is then removed from the second half, the first half starts
>> shutdown sequence and will trigger a DLLSC event. This is misinterpreted
>> as an enabling event and causes the first half to be re-enabled.
>>
>> The spurious enable can be resolved by ignoring link status change
>> events when no link is active when in the off state. This patch adds a
>> quirk for the card.
>>
>> Acked-by: Jon Derrick <jonathan.derrick@intel.com>
>> Signed-off-by: James Puthukattukaran <james.puthukattukaran@oracle.com>
>> ---
>> v1->v2: Device-specific quirk
>>
>> drivers/pci/hotplug/pciehp_ctrl.c | 7 +++++++
>> drivers/pci/quirks.c | 30 ++++++++++++++++++++++++++++++
>> include/linux/pci.h | 1 +
>> 3 files changed, 38 insertions(+)
>>
>> diff --git a/drivers/pci/hotplug/pciehp_ctrl.c b/drivers/pci/hotplug/pciehp_ctrl.c
>> index 529c34808440..db41f78bfac8 100644
>> --- a/drivers/pci/hotplug/pciehp_ctrl.c
>> +++ b/drivers/pci/hotplug/pciehp_ctrl.c
>> @@ -225,6 +225,7 @@ void pciehp_handle_disable_request(struct controller *ctrl)
>> void pciehp_handle_presence_or_link_change(struct controller *ctrl, u32 events)
>> {
>> int present, link_active;
>> + struct pci_dev *pdev = ctrl->pcie->port;
>>
>> /*
>> * If the slot is on and presence or link has changed, turn it off.
>> @@ -265,6 +266,12 @@ void pciehp_handle_presence_or_link_change(struct controller *ctrl, u32 events)
>> cancel_delayed_work(&ctrl->button_work);
>> fallthrough;
>> case OFF_STATE:
>> + if (pdev->shared_pcc_and_link_slot &&
>> + (events & PCI_EXP_SLTSTA_DLLSC) && !link_active) {
>
> Is it !link_active? Based on the explanation, until we turn off both slots
> you would see link is still active correct?
My understanding is the internal link is active but link_active shows negative
status while the one side is faking being 'powered off'.
Then if a DLLSC occurs while the slot is already OFF_STATE and !link_active, I
believe we can assume it was previously !link_active as well and it's an event
from the other side.
>
>> + mutex_unlock(&ctrl->state_lock);
>> + break;
>> + }
>> +
^ permalink raw reply [flat|nested] 5+ messages in thread