[PATCH] PCI: Add sysfs attribute for PCI device power state

[PATCH] PCI: Add sysfs attribute for PCI device power state

[Maximilian Luz]

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.

Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com>
---
 Documentation/ABI/testing/sysfs-bus-pci |  9 +++++++++
 drivers/pci/pci-sysfs.c                 | 12 ++++++++++++
 2 files changed, 21 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-bus-pci b/Documentation/ABI/testing/sysfs-bus-pci
index 450296cc7948..881040af2611 100644
--- a/Documentation/ABI/testing/sysfs-bus-pci
+++ b/Documentation/ABI/testing/sysfs-bus-pci
@@ -360,3 +360,12 @@ Contact:	Heiner Kallweit <hkallweit1@gmail.com>
 Description:	If ASPM is supported for an endpoint, these files can be
 		used to disable or enable the individual power management
 		states. Write y/1/on to enable, n/0/off to disable.
+
+What:		/sys/bus/pci/devices/.../power_state
+Date:		November 2020
+Contact:	Linux PCI developers <linux-pci@vger.kernel.org>
+Description:
+		This file contains the current PCI power state of the device.
+		The value comes from the PCI kernel device state and can be one
+		of: "unknown", "error", "D0", D1", "D2", "D3hot", "D3cold".
+		The file is read only.
diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index d15c881e2e7e..b15f754e6346 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -124,6 +124,17 @@ static ssize_t cpulistaffinity_show(struct device *dev,
 }
 static DEVICE_ATTR_RO(cpulistaffinity);
 
+/* 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);
+
 /* show resources */
 static ssize_t resource_show(struct device *dev, struct device_attribute *attr,
 			     char *buf)
@@ -581,6 +592,7 @@ static ssize_t driver_override_show(struct device *dev,
 static DEVICE_ATTR_RW(driver_override);
 
 static struct attribute *pci_dev_attrs[] = {
+	&dev_attr_power_state.attr,
 	&dev_attr_resource.attr,
 	&dev_attr_vendor.attr,
 	&dev_attr_device.attr,
-- 
2.29.2

Re: [PATCH] PCI: Add sysfs attribute for PCI device power state

[Krzysztof Wilczyński]

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.

Krzysztof

Re: [PATCH] PCI: Add sysfs attribute for PCI device power state

[Maximilian Luz]

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

Re: [PATCH] PCI: Add sysfs attribute for PCI device power state

[Bjorn Helgaas]

On Sun, Nov 15, 2020 at 01:45:18PM +0100, Maximilian Luz wrote:
> On 11/15/20 7:08 AM, Krzysztof Wilczyński wrote:
> > 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.

I think something read from sysfs is a snapshot with no guarantee
about how long it will remain valid, so I don't see a problem with the
value being stale by the time userspace consumes it.

If there's a downside to doing two separate reads, we could mention
that in the commit log or a comment.

If there's not a specific reason for using READ_ONCE(), I think we
should omit it because using it in one place but not others suggests
that there's something special about this place.

Bjorn

Re: [PATCH] PCI: Add sysfs attribute for PCI device power state

[Maximilian Luz]

On 11/15/20 9:27 PM, Bjorn Helgaas wrote:

[...]

> I think something read from sysfs is a snapshot with no guarantee
> about how long it will remain valid, so I don't see a problem with the
> value being stale by the time userspace consumes it.

I agree on this, and the READ_ONCE won't protect against it. The
READ_ONCE would only protect against future changes, e.g. something like

     const char *state_names[] = { ... };

     // check if state is invalid
     if (READ(pci_dev->current_state) >= ARRAY_SIZE(state_names))
             return sprintf(..., "invalid");
     else    // look state up in table
             return sprintf(..., state_names[READ(pci_dev->current_state)])

Note that I've explicitly marked the problematic reads here: If those
are done separately, the invalidity check may pass, but by the time the
state name is looked up, the value may have changed and may be invalid.

Note further that if we have something like

     pci_power_t state = pci_dev->current_state;

the compiler is, in theory, free to replace each access to "state" with
a read to pci_dev->current_state. As far as I can tell, the whole point
of READ_ONCE is to prevent that and ensure that there is only one read.

Note also that something like this could be easily introduced by
changing the code in pci_power_name(), as that is likely inlined by the
compiler. I'm not entirely sure, but I think that the compiler is allowed
to, at least theoretically, split that into two reads here and inlining
might be done before further optimization.

On the other hand, the changes that could lead to issues above are
fairly unlikely to cause them as the compiler will _probably_ read the
value only once anyways.

> If there's a downside to doing two separate reads, we could mention
> that in the commit log or a comment.
> 
> If there's not a specific reason for using READ_ONCE(), I think we
> should omit it because using it in one place but not others suggests
> that there's something special about this place.

I'd argue that there is indeed something special about this place:
current_state is accessed without holding the device lock (unless I'm
mistaken and sysfs attributes do acquire the device lock automatically)
and the state is normally only accessed/changed under it.

Apart from (hopefully) preventing somewhat unlikely future issues and
highlighting that it is (somewhat of a) special case, the READ_ONCE does
not serve any purpose here. As the code is now, omitting it will not
cause any issues (or really should not make any difference in produced
code).

All in all, I'm not entirely sure that it's a good idea to drop the
READ_ONCE, but I'll defer to you for that judgement.

Regards,
Max

Re: [PATCH] PCI: Add sysfs attribute for PCI device power state

[Bjorn Helgaas]

On Sun, Nov 15, 2020 at 10:19:22PM +0100, Maximilian Luz wrote:
> On 11/15/20 9:27 PM, Bjorn Helgaas wrote:
> 
> [...]
> 
> > I think something read from sysfs is a snapshot with no guarantee
> > about how long it will remain valid, so I don't see a problem with the
> > value being stale by the time userspace consumes it.
> 
> I agree on this, and the READ_ONCE won't protect against it. The
> READ_ONCE would only protect against future changes, e.g. something like
> 
>     const char *state_names[] = { ... };
> 
>     // check if state is invalid
>     if (READ(pci_dev->current_state) >= ARRAY_SIZE(state_names))
>             return sprintf(..., "invalid");
>     else    // look state up in table
>             return sprintf(..., state_names[READ(pci_dev->current_state)])
> 
> Note that I've explicitly marked the problematic reads here: If those
> are done separately, the invalidity check may pass, but by the time the
> state name is looked up, the value may have changed and may be invalid.
> 
> Note further that if we have something like
> 
>     pci_power_t state = pci_dev->current_state;
> 
> the compiler is, in theory, free to replace each access to "state" with
> a read to pci_dev->current_state. As far as I can tell, the whole point
> of READ_ONCE is to prevent that and ensure that there is only one read.
> 
> Note also that something like this could be easily introduced by
> changing the code in pci_power_name(), as that is likely inlined by the
> compiler. I'm not entirely sure, but I think that the compiler is allowed
> to, at least theoretically, split that into two reads here and inlining
> might be done before further optimization.
> 
> On the other hand, the changes that could lead to issues above are
> fairly unlikely to cause them as the compiler will _probably_ read the
> value only once anyways.

Well, OK, I see your point.  But I'm not convinced it's worth
cluttering the code for this.  There must be dozens of similar cases,
and if we do need to worry about this, I'd like to do it
systematically for all of drivers/pci/ instead of doing it piecemeal.

I do think it's probably worth making sure we can't set
dev->current_state to something that's invalid, and also taking a look
at the PCI core interfaces that take a pci_power_t, i.e., those in
include/linux/pci.h, to make sure they do the right thing if a driver
supplies garbage.

> > If there's a downside to doing two separate reads, we could mention
> > that in the commit log or a comment.
> > 
> > If there's not a specific reason for using READ_ONCE(), I think we
> > should omit it because using it in one place but not others suggests
> > that there's something special about this place.
> 
> I'd argue that there is indeed something special about this place:
> current_state is accessed without holding the device lock (unless I'm
> mistaken and sysfs attributes do acquire the device lock automatically)
> and the state is normally only accessed/changed under it.
> 
> Apart from (hopefully) preventing somewhat unlikely future issues and
> highlighting that it is (somewhat of a) special case, the READ_ONCE does
> not serve any purpose here. As the code is now, omitting it will not
> cause any issues (or really should not make any difference in produced
> code).
> 
> All in all, I'm not entirely sure that it's a good idea to drop the
> READ_ONCE, but I'll defer to you for that judgement.
> 
> Regards,
> Max

Re: [PATCH] PCI: Add sysfs attribute for PCI device power state

[Bjorn Helgaas]

[+cc Krzysztof, Rafael in case you have a suggestion about the
filename (or anythnig else :))]

On Mon, Nov 02, 2020 at 03:15:20PM +0100, 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.
> 
> Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com>

Applied as below to pci/pm for v5.11.

> ---
>  Documentation/ABI/testing/sysfs-bus-pci |  9 +++++++++
>  drivers/pci/pci-sysfs.c                 | 12 ++++++++++++
>  2 files changed, 21 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-pci b/Documentation/ABI/testing/sysfs-bus-pci
> index 450296cc7948..881040af2611 100644
> --- a/Documentation/ABI/testing/sysfs-bus-pci
> +++ b/Documentation/ABI/testing/sysfs-bus-pci
> @@ -360,3 +360,12 @@ Contact:	Heiner Kallweit <hkallweit1@gmail.com>
>  Description:	If ASPM is supported for an endpoint, these files can be
>  		used to disable or enable the individual power management
>  		states. Write y/1/on to enable, n/0/off to disable.
> +
> +What:		/sys/bus/pci/devices/.../power_state

I guess this will be alongside the existing "power/" directory.
Rafael, is there any precedent we should be following here?

> +Date:		November 2020
> +Contact:	Linux PCI developers <linux-pci@vger.kernel.org>
> +Description:
> +		This file contains the current PCI power state of the device.
> +		The value comes from the PCI kernel device state and can be one
> +		of: "unknown", "error", "D0", D1", "D2", "D3hot", "D3cold".
> +		The file is read only.
> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> index d15c881e2e7e..b15f754e6346 100644
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
> @@ -124,6 +124,17 @@ static ssize_t cpulistaffinity_show(struct device *dev,
>  }
>  static DEVICE_ATTR_RO(cpulistaffinity);
>  
> +/* 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);
> +
>  /* show resources */
>  static ssize_t resource_show(struct device *dev, struct device_attribute *attr,
>  			     char *buf)
> @@ -581,6 +592,7 @@ static ssize_t driver_override_show(struct device *dev,
>  static DEVICE_ATTR_RW(driver_override);
>  
>  static struct attribute *pci_dev_attrs[] = {
> +	&dev_attr_power_state.attr,
>  	&dev_attr_resource.attr,
>  	&dev_attr_vendor.attr,
>  	&dev_attr_device.attr,

commit 9f1c0ebea21a ("PCI: Add sysfs attribute for device power state")
Author: Maximilian Luz <luzmaximilian@gmail.com>
Date:   Mon Nov 2 15:15:20 2020 +0100

    PCI: Add sysfs attribute for device power state
    
    While PCI power states D0-D3hot can be queried from user-space via lspci,
    D3cold cannot.  lspci cannot provide an accurate value when the device is
    in D3cold as it has to restore the device to D0 before it can access its
    power state via the configuration space, leading to it reporting D0 or
    another on-state. Thus lspci cannot be used to diagnose power consumption
    issues for devices that can enter D3cold or to ensure that devices properly
    enter D3cold at all.
    
    Add a new sysfs device attribute for the PCI power state, showing the
    current power state as seen by the kernel.
    
    [bhelgaas: drop READ_ONCE(), see discussion at the link]
    Link: https://lore.kernel.org/r/20201102141520.831630-1-luzmaximilian@gmail.com
    Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com>
    Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>

diff --git a/Documentation/ABI/testing/sysfs-bus-pci b/Documentation/ABI/testing/sysfs-bus-pci
index 450296cc7948..881040af2611 100644
--- a/Documentation/ABI/testing/sysfs-bus-pci
+++ b/Documentation/ABI/testing/sysfs-bus-pci
@@ -360,3 +360,12 @@ Contact:	Heiner Kallweit <hkallweit1@gmail.com>
 Description:	If ASPM is supported for an endpoint, these files can be
 		used to disable or enable the individual power management
 		states. Write y/1/on to enable, n/0/off to disable.
+
+What:		/sys/bus/pci/devices/.../power_state
+Date:		November 2020
+Contact:	Linux PCI developers <linux-pci@vger.kernel.org>
+Description:
+		This file contains the current PCI power state of the device.
+		The value comes from the PCI kernel device state and can be one
+		of: "unknown", "error", "D0", D1", "D2", "D3hot", "D3cold".
+		The file is read only.
diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index d15c881e2e7e..fb072f4b3176 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -124,6 +124,15 @@ static ssize_t cpulistaffinity_show(struct device *dev,
 }
 static DEVICE_ATTR_RO(cpulistaffinity);
 
+static ssize_t power_state_show(struct device *dev,
+				struct device_attribute *attr, char *buf)
+{
+	struct pci_dev *pdev = to_pci_dev(dev);
+
+	return sprintf(buf, "%s\n", pci_power_name(pdev->current_state));
+}
+static DEVICE_ATTR_RO(power_state);
+
 /* show resources */
 static ssize_t resource_show(struct device *dev, struct device_attribute *attr,
 			     char *buf)
@@ -581,6 +590,7 @@ static ssize_t driver_override_show(struct device *dev,
 static DEVICE_ATTR_RW(driver_override);
 
 static struct attribute *pci_dev_attrs[] = {
+	&dev_attr_power_state.attr,
 	&dev_attr_resource.attr,
 	&dev_attr_vendor.attr,
 	&dev_attr_device.attr,

Re: [PATCH] PCI: Add sysfs attribute for PCI device power state

[Maximilian Luz]

On 11/18/20 6:49 PM, Bjorn Helgaas wrote:
> On Sun, Nov 15, 2020 at 10:19:22PM +0100, Maximilian Luz wrote:
>> On 11/15/20 9:27 PM, Bjorn Helgaas wrote:
>>
>> [...]
>>
>>> I think something read from sysfs is a snapshot with no guarantee
>>> about how long it will remain valid, so I don't see a problem with the
>>> value being stale by the time userspace consumes it.
>>
>> I agree on this, and the READ_ONCE won't protect against it. The
>> READ_ONCE would only protect against future changes, e.g. something like
>>
>>      const char *state_names[] = { ... };
>>
>>      // check if state is invalid
>>      if (READ(pci_dev->current_state) >= ARRAY_SIZE(state_names))
>>              return sprintf(..., "invalid");
>>      else    // look state up in table
>>              return sprintf(..., state_names[READ(pci_dev->current_state)])
>>
>> Note that I've explicitly marked the problematic reads here: If those
>> are done separately, the invalidity check may pass, but by the time the
>> state name is looked up, the value may have changed and may be invalid.
>>
>> Note further that if we have something like
>>
>>      pci_power_t state = pci_dev->current_state;
>>
>> the compiler is, in theory, free to replace each access to "state" with
>> a read to pci_dev->current_state. As far as I can tell, the whole point
>> of READ_ONCE is to prevent that and ensure that there is only one read.
>>
>> Note also that something like this could be easily introduced by
>> changing the code in pci_power_name(), as that is likely inlined by the
>> compiler. I'm not entirely sure, but I think that the compiler is allowed
>> to, at least theoretically, split that into two reads here and inlining
>> might be done before further optimization.
>>
>> On the other hand, the changes that could lead to issues above are
>> fairly unlikely to cause them as the compiler will _probably_ read the
>> value only once anyways.
> 
> Well, OK, I see your point.  But I'm not convinced it's worth
> cluttering the code for this.  There must be dozens of similar cases,
> and if we do need to worry about this, I'd like to do it
> systematically for all of drivers/pci/ instead of doing it piecemeal.

Fair enough, that's a valid point.

For full formal correctness, writes to current_state should probably
have also been guarded with WRITE_ONCE to prevent the compiler from
splitting the write instruction in addition to the read with READ_ONCE
in this patch.

Again, that's mostly a point about formal correctness and issues like
that shouldn't happen in practice (or if they would, would probably have
broken other parts of the kernel already).

Looking at other sysfs functions, it seems like most of them would have
the same issues, so it makes sense to drop the READ_ONCE.

> I do think it's probably worth making sure we can't set
> dev->current_state to something that's invalid, and also taking a look
> at the PCI core interfaces that take a pci_power_t, i.e., those in
> include/linux/pci.h, to make sure they do the right thing if a driver
> supplies garbage.

As far as I can tell, those checks should already be there.

Thanks,
Max