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
