Messages in this thread |  | | Subject | Re: [PATCH] PCI: Add sysfs attribute for PCI device power state | From | Maximilian Luz <> | Date | Sun, 15 Nov 2020 13:45:18 +0100 |
| |
On 11/15/20 7:08 AM, Krzysztof Wilczyński wrote: > Hi Maximilian, > > On 20-11-02 15:15:20, Maximilian Luz wrote: >> While most PCI power-states can be queried from user-space via lspci, >> this has some limits. Specifically, lspci fails to provide an accurate >> value when the device is in D3cold as it has to resume the device before >> it can access its power state via the configuration space, leading to it >> reporting D0 or another on-state. Thus lspci can, for example, not be >> used to diagnose power-consumption issues for devices that can enter >> D3cold or to ensure that devices properly enter D3cold at all. >> >> To alleviate this issue, introduce a new sysfs device attribute for the >> PCI power state, showing the current power state as seen by the kernel. > > Very nice! Thank you for adding this. > > [...] >> +/* PCI power state */ >> +static ssize_t power_state_show(struct device *dev, >> + struct device_attribute *attr, char *buf) >> +{ >> + struct pci_dev *pci_dev = to_pci_dev(dev); >> + pci_power_t state = READ_ONCE(pci_dev->current_state); >> + >> + return sprintf(buf, "%s\n", pci_power_name(state)); >> +} >> +static DEVICE_ATTR_RO(power_state); > [...] > > Curious, why did you decide to use the READ_ONCE() macro here? Some > other drivers exposing data through sysfs use, but certainly not all.
As far as I can tell current_state is normally guarded by the device lock, but here we don't hold that lock. I generally prefer to be explicit about those kinds of access, if only to document that the value can change here.
In this case it should work fine without it, but this also has the benefit that if someone were to add a change like
if (state > x) state = y;
later on (here or even in pci_power_name() due to inlining), things wouldn't break as we explicitly forbid the compiler to load current_state more than once. Without the READ_ONCE, the compiler would be theoretically allowed to do two separate reads then (although arguably unlikely it would end up doing that).
Also there's no downside of marking it as READ_ONCE here as far as I can tell, as in that context the value will always have to be loaded from memory.
So in short, mostly personal preference rooted in documentation and avoiding potential (unlikely) future mishaps.
Regards, Max
|  |