Re: [PATCH] PCI: pciehp_hpc: Fix set raw indicator status

[Bjorn Helgaas <helgaas@kernel.org>]

[+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
>