Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH] PCI: Add sysfs attribute for PCI device power state
To:     =?UTF-8?Q?Krzysztof_Wilczy=c5=84ski?= <kw@linux.com>
Cc:     Bjorn Helgaas <bhelgaas@google.com>,
        Heiner Kallweit <hkallweit1@gmail.com>,
        linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20201102141520.831630-1-luzmaximilian@gmail.com>
 <X7DF2ZyVnyIFjdC1@rocinante>
From:   Maximilian Luz <luzmaximilian@gmail.com>
Message-ID: <688dcd74-3acc-bc28-7ba3-55fdedbe6e70@gmail.com>
Date:   Sun, 15 Nov 2020 13:45:18 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.12.0
MIME-Version: 1.0
In-Reply-To: <X7DF2ZyVnyIFjdC1@rocinante>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Precedence: bulk

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