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:     Bjorn Helgaas <helgaas@kernel.org>
Cc:     =?UTF-8?Q?Krzysztof_Wilczy=c5=84ski?= <kw@linux.com>,
        Bjorn Helgaas <bhelgaas@google.com>,
        Heiner Kallweit <hkallweit1@gmail.com>,
        linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20201115202719.GA1239987@bjorn-Precision-5520>
From:   Maximilian Luz <luzmaximilian@gmail.com>
Message-ID: <96da5aa3-a6ff-aeee-430b-bc9958f5aefa@gmail.com>
Date:   Sun, 15 Nov 2020 22:19:22 +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: <20201115202719.GA1239987@bjorn-Precision-5520>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

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