[+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@xxxxxxxxx>
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@xxxxxxxxx
Signed-off-by: Blazej Kucman <blazej.kucman@xxxxxxxxx>
[bhelgaas: commit log]
Signed-off-by: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>
Cc: stable@xxxxxxxxxxxxxxx # 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
>
