On Fri, 19 Jul 2024 12:22:53 +0200 Blazej Kucman <blazej.kucman@xxxxxxxxxxxxxxx> 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@xxxxxxxxx/T/#u Thanks, Blazej
