| Message ID | 20240722141440.7210-1-blazej.kucman@intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | PCI: pciehp_hpc: Fix set raw indicator status | expand |
[+cc Myron, Kai-Heng, Joerg: fix for regression already in distros] On Mon, Jul 22, 2024 at 04:14:40PM +0200, Blazej Kucman wrote: > Set raw indicator status interface has been designed to respect bits > responsible for Attention and Power Indicator Control [1]. But since > abaaac4845a0 ("PCI: hotplug: Use FIELD_GET/PREP()") only processes > Attention Indicator Control bits. > > Mentioned change replace direct bit shift, via macro FIELD_PREP, which > additionally performs an AND operation with the passed bitmask. The > regression results from an incorrect bitmask, the mask should include bits > responsible for Attention and Power Indicator Control, but only Attention > Indicator Control was passed. This lead to a loss of passed bits > responsible for Power Indicator control. > > This fix restores Power Indicator Control bits support. > > [1] 576243b3f9ea ("PCI: pciehp: Allow exclusive userspace control of indicators") > > Fixes: abaaac4845a0 ("PCI: hotplug: Use FIELD_GET/PREP()") > Signed-off-by: Blazej Kucman <blazej.kucman@intel.com> Thanks for the regression report and for debugging and fixing it! I propose the following commit log update to add more details of what broke and what's affects (only Intel VMD, IIUC). And also a stable tag since abaaac4845a0 appeared in v6.7 and is already in distros. I plan to put this on for-linus for v6.11. PCI: pciehp: Retain Power Indicator bits for userspace indicators The sysfs "attention" file normally controls the Slot Control Attention Indicator with 0 (off), 1 (on), 2 (blink) settings. 576243b3f9ea ("PCI: pciehp: Allow exclusive userspace control of indicators") added pciehp_set_raw_indicator_status() to allow userspace to directly control all four bits in both the Attention Indicator and the Power Indicator fields via the "attention" file. This is used on Intel VMD bridges so utilities like "ledmon" can use sysfs "attention" to control up to 16 indicators for NVMe device RAID status. abaaac4845a0 ("PCI: hotplug: Use FIELD_GET/PREP()") broke this by masking the sysfs data with PCI_EXP_SLTCTL_AIC, which discards the upper two bits intended for the Power Indicator Control field (PCI_EXP_SLTCTL_PIC). For NVMe devices behind an Intel VMD, ledmon settings that use the PCI_EXP_SLTCTL_PIC bits, i.e., ATTENTION_REBUILD (0x5), ATTENTION_LOCATE (0x7), ATTENTION_FAILURE (0xD), ATTENTION_OFF (0xF), no longer worked correctly. Mask with PCI_EXP_SLTCTL_AIC | PCI_EXP_SLTCTL_PIC to retain both the Attention Indicator and the Power Indicator bits. Fixes: abaaac4845a0 ("PCI: hotplug: Use FIELD_GET/PREP()") Link: https://lore.kernel.org/r/20240722141440.7210-1-blazej.kucman@intel.com Signed-off-by: Blazej Kucman <blazej.kucman@intel.com> [bhelgaas: commit log] Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> Cc: stable@vger.kernel.org # v6.7+ > --- > drivers/pci/hotplug/pciehp_hpc.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c > index 061f01f60db4..736ad8baa2a5 100644 > --- a/drivers/pci/hotplug/pciehp_hpc.c > +++ b/drivers/pci/hotplug/pciehp_hpc.c > @@ -485,7 +485,9 @@ int pciehp_set_raw_indicator_status(struct hotplug_slot *hotplug_slot, > struct pci_dev *pdev = ctrl_dev(ctrl); > > pci_config_pm_runtime_get(pdev); > - pcie_write_cmd_nowait(ctrl, FIELD_PREP(PCI_EXP_SLTCTL_AIC, status), > + > + /* Attention and Power Indicator Control bits are supported */ > + pcie_write_cmd_nowait(ctrl, FIELD_PREP(PCI_EXP_SLTCTL_AIC | PCI_EXP_SLTCTL_PIC, status), > PCI_EXP_SLTCTL_AIC | PCI_EXP_SLTCTL_PIC); > pci_config_pm_runtime_put(pdev); > return 0; > -- > 2.35.3 >
diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c index 061f01f60db4..736ad8baa2a5 100644 --- a/drivers/pci/hotplug/pciehp_hpc.c +++ b/drivers/pci/hotplug/pciehp_hpc.c @@ -485,7 +485,9 @@ int pciehp_set_raw_indicator_status(struct hotplug_slot *hotplug_slot, struct pci_dev *pdev = ctrl_dev(ctrl); pci_config_pm_runtime_get(pdev); - pcie_write_cmd_nowait(ctrl, FIELD_PREP(PCI_EXP_SLTCTL_AIC, status), + + /* Attention and Power Indicator Control bits are supported */ + pcie_write_cmd_nowait(ctrl, FIELD_PREP(PCI_EXP_SLTCTL_AIC | PCI_EXP_SLTCTL_PIC, status), PCI_EXP_SLTCTL_AIC | PCI_EXP_SLTCTL_PIC); pci_config_pm_runtime_put(pdev); return 0;
Set raw indicator status interface has been designed to respect bits responsible for Attention and Power Indicator Control [1]. But since abaaac4845a0 ("PCI: hotplug: Use FIELD_GET/PREP()") only processes Attention Indicator Control bits. Mentioned change replace direct bit shift, via macro FIELD_PREP, which additionally performs an AND operation with the passed bitmask. The regression results from an incorrect bitmask, the mask should include bits responsible for Attention and Power Indicator Control, but only Attention Indicator Control was passed. This lead to a loss of passed bits responsible for Power Indicator control. This fix restores Power Indicator Control bits support. [1] 576243b3f9ea ("PCI: pciehp: Allow exclusive userspace control of indicators") Fixes: abaaac4845a0 ("PCI: hotplug: Use FIELD_GET/PREP()") Signed-off-by: Blazej Kucman <blazej.kucman@intel.com> --- drivers/pci/hotplug/pciehp_hpc.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)