Re: [REGRESSION] Setting the LED status via attention stopped working.

[Blazej Kucman <blazej.kucman@linux.intel.com>]

On Fri, 19 Jul 2024 12:22:53 +0200
Blazej Kucman <blazej.kucman@linux.intel.com> wrote:

> Hi all,
> 
> We discovered regression with setting LED state through "attention"
> for VMD slot. e.g /sys/bus/pci/slots/2-1/attention.
> 
> Expected behavior:
> write status into "attention" will set the LED status correctly.
> 
> Actual behavior:
> write the status causes undefined behavior, "attention" contains a
> different value than the one entered, the requested LED status is not
> set
> 
> After bisection kernel, it turned out that the issue appeared between
> kernel v6.6..v6.7-rc1. Tested kernel:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
> 
> I found a patch that causes issue:
> PCI: hotplug: Use FIELD_GET/PREP()
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?h=v6.7-rc1&id=abaaac4845a0d6f39f83cbaba4c3b46ba5f93170 
> I confirmed this by reverting this patch from kernel 6.10.
> 
> Issue was discovered by using ledmon utility (ledctl is a part of
> ledmon software) https://github.com/intel/ledmon, 
> example command: ledctl locate={/dev/nvme6n1 }, should set locate
> status on nvme6n1
> 
> The values that ledmon writes into "attention"
> 
> #define ATTENTION_OFF        0xF  /* (1111) Attention Off, Power Off
> */ #define ATTENTION_LOCATE     0x7  /* (0111) Attention Off, Power
> On */ #define ATTENTION_REBUILD    0x5  /* (0101) Attention On, Power
> On */ #define ATTENTION_FAILURE    0xD  /* (1101) Attention On, Power
> Off */
> 
> Without mentioned patch, writing these values will set the LEDs
> correctly.
> 
> I was able to determine that the change in behavior was caused by this
> change @@ -484,7 +485,7 @@ 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, status << 6,
> +	pcie_write_cmd_nowait(ctrl, FIELD_PREP(PCI_EXP_SLTCTL_AIC,
> status), PCI_EXP_SLTCTL_AIC | PCI_EXP_SLTCTL_PIC);
>  	pci_config_pm_runtime_put(pdev)
> 	
> I added logging of bit shift values and there is a significant
> difference in the operation of this method
> 
> int pciehp_set_raw_indicator_status(struct hotplug_slot *hotplug_slot,
>                                     u8 status)
> {
>         struct controller *ctrl = to_ctrl(hotplug_slot);
>         struct pci_dev *pdev = ctrl_dev(ctrl);
> 
>         pr_err("SET_INDICATOR_ST START");
> 
>         pr_err("SET_INDICATOR_ST_INPUT: STATUS: %d %u", status,
> status); pr_err("SET_INDICATOR_ST_OLD d %d, u %u", status << 6, status
>         << 6);
> 
>         u8 test = FIELD_PREP(PCI_EXP_SLTCTL_AIC, status);
>         pr_err("SET_INDICATOR_ST_NEW d %d, u %u", test, test);
> 
>         pr_err("SET_INDICATOR_ST END");
> 
>         pci_config_pm_runtime_get(pdev);
>         pcie_write_cmd_nowait(ctrl, status << 6,
>                               PCI_EXP_SLTCTL_AIC |
> PCI_EXP_SLTCTL_PIC); pci_config_pm_runtime_put(pdev);
>         return 0;
> }
> LOGS:
> 
> [   75.814893] SET_INDICATOR_ST START
> [   75.814895] SET_INDICATOR_ST_INPUT: STATUS: 15 15
> [   75.818362] SET_INDICATOR_ST_OLD d 960, u 960
> [   75.823143] SET_INDICATOR_ST_NEW d 192, u 192
> [   75.827634] SET_INDICATOR_ST END
> [   75.832880] SET_INDICATOR_ST START
> [   75.836167] SET_INDICATOR_ST_INPUT: STATUS: 15 15
> [   75.839626] SET_INDICATOR_ST_OLD d 960, u 960
> [   75.844408] SET_INDICATOR_ST_NEW d 192, u 192
> [   75.848834] SET_INDICATOR_ST END
> [   76.225732] SET_INDICATOR_ST START
> [   76.229022] SET_INDICATOR_ST_INPUT: STATUS: 15 15
> [   76.232481] SET_INDICATOR_ST_OLD d 960, u 960
> [   76.237259] SET_INDICATOR_ST_NEW d 192, u 192
> [   76.241691] SET_INDICATOR_ST END
> [   76.250247] SET_INDICATOR_ST START
> [   76.253530] SET_INDICATOR_ST_INPUT: STATUS: 15 15
> [   76.256990] SET_INDICATOR_ST_OLD d 960, u 960
> [   76.261766] SET_INDICATOR_ST_NEW d 192, u 192
> [   76.266189] SET_INDICATOR_ST END
> [   77.223562] SET_INDICATOR_ST START
> [   77.226852] SET_INDICATOR_ST_INPUT: STATUS: 15 15
> [   77.230312] SET_INDICATOR_ST_OLD d 960, u 960
> [   77.235091] SET_INDICATOR_ST_NEW d 192, u 192
> [   77.239521] SET_INDICATOR_ST END
> [   77.244765] SET_INDICATOR_ST START
> [   77.248051] SET_INDICATOR_ST_INPUT: STATUS: 15 15
> [   77.251510] SET_INDICATOR_ST_OLD d 960, u 960
> [   77.256288] SET_INDICATOR_ST_NEW d 192, u 192
> [   77.260725] SET_INDICATOR_ST END
> [   77.267606] SET_INDICATOR_ST START
> 
> # ATTENTION_LOCATE
> [   77.270895] SET_INDICATOR_ST_INPUT: STATUS: 7 7
> [   77.274356] SET_INDICATOR_ST_OLD d 448, u 448
> [   77.278959] SET_INDICATOR_ST_NEW d 192, u 192
> [   77.283379] SET_INDICATOR_ST END
> 
> With patch PCI: hotplug: Use FIELD_GET/PREP():
> after trying to set ATTENTION_LOCATE, file "attention" contains value
> 0x3 instead expected 0x7.
> 
> [  862.150910] SET_INDICATOR_ST START
> [  862.150912] SET_INDICATOR_ST_INPUT: STATUS: 15 15
> [  862.154375] SET_INDICATOR_ST_OLD d 960, u 960
> [  862.159159] SET_INDICATOR_ST_NEW d 192, u 192
> [  862.163591] SET_INDICATOR_ST END
> [  862.169235] SET_INDICATOR_ST START
> [  862.172524] SET_INDICATOR_ST_INPUT: STATUS: 15 15
> [  862.175984] SET_INDICATOR_ST_OLD d 960, u 960
> [  862.182933] SET_INDICATOR_ST_NEW d 192, u 192
> [  862.189190] SET_INDICATOR_ST END
> [  862.198302] SET_INDICATOR_ST START
> [  862.203290] SET_INDICATOR_ST_INPUT: STATUS: 15 15
> [  862.208398] SET_INDICATOR_ST_OLD d 960, u 960
> [  862.214780] SET_INDICATOR_ST_NEW d 192, u 192
> [  862.220817] SET_INDICATOR_ST END
> [  862.231044] SET_INDICATOR_ST START
> [  862.235931] SET_INDICATOR_ST_INPUT: STATUS: 15 15
> [  862.240994] SET_INDICATOR_ST_OLD d 960, u 960
> [  862.247376] SET_INDICATOR_ST_NEW d 192, u 192
> [  862.253411] SET_INDICATOR_ST END
> [  862.262574] SET_INDICATOR_ST START
> [  862.267471] SET_INDICATOR_ST_INPUT: STATUS: 15 15
> [  862.272537] SET_INDICATOR_ST_OLD d 960, u 960
> [  862.278918] SET_INDICATOR_ST_NEW d 192, u 192
> [  862.284945] SET_INDICATOR_ST END
> [  862.291705] SET_INDICATOR_ST START
> [  862.296596] SET_INDICATOR_ST_INPUT: STATUS: 15 15
> [  862.301656] SET_INDICATOR_ST_OLD d 960, u 960
> [  862.308039] SET_INDICATOR_ST_NEW d 192, u 192
> [  862.314064] SET_INDICATOR_ST END
> 
> # ATTENTION_REBUILD
> [  862.322480] SET_INDICATOR_ST START
> [  862.327388] SET_INDICATOR_ST_INPUT: STATUS: 5 5
> [  862.332454] SET_INDICATOR_ST_OLD d 320, u 320
> [  862.338662] SET_INDICATOR_ST_NEW d 64, u 64
> [  862.344704] SET_INDICATOR_ST END
> 
> With patch PCI: hotplug: Use FIELD_GET/PREP():
> After trying to set ATTENTION_REBUILD, file "attention" contains value
> 0x1 instead expected 0x5.
> 
> This topic is quite important for us, because this change has been
> included in RHEL9.5 kernel and causes LED setting for VMD drives to
> not work.
> 
> Looking at the above logs, I don't know if it always worked wrong,
> because the input status is u8 and the bit shift did not cut off some
> of higher bits e.g. for ATTENTION_LOCATE 0x7 (0111),
> currently macro FIELD_PREP it does this through "AND" with mask.
> 
> Regards,
> Blazej
> 

Hi,

I sent a fix to mailing list.

[PATCH] PCI: pciehp_hpc: Fix set raw indicator status
https://lore.kernel.org/linux-pci/20240722141440.7210-1-blazej.kucman@intel.com/T/#u

Thanks,
Blazej