Re: [PATCH v3] cxl: Add cxl_region_rwsem around commit_end write during decoder enumeration

[Davidlohr Bueso <dave@stgolabs.net>]

On Wed, 22 Nov 2023, Dan Williams wrote:

>Dan Williams wrote:
>> Davidlohr Bueso wrote:
>> > On Fri, 17 Nov 2023, Dave Jiang wrote:
>> >
>> > >init_hdm_decoder() modifies port->commit_end without taking the
>> > >cxl_region_rwsem. An assert splat emitted by cxl_num_decoders_committed().
>> > >However looking at the code, it looks like the write version of the rwsem
>> > >needs to be taken due to the modification of commit_end. Wrap the write
>> > >version of the rwsem around reading and writing of commit_end.
>> > >
>> > >Fixes: 176baefb2eb5 ("cxl/hdm: Commit decoder state to hardware")
>> > >Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>> >
>> > Uhmm but do we expect concurrency during the switch/port probing phase?
>>
>> I answered that that detail here in response to Fan:
>>
>> http://lore.kernel.org/r/654eb8ed72ced_46f0294c@dwillia2-mobl3.amr.corp.intel.com.notmuch
>>
>> The takeaway is:
>>
>> "I am in favor of adding the locking rather than depend on a subtle
>> side-effect of how CXL operates, and to avoid adding an unlocked version
>> of cxl_num_decoders_committed()"
>>
>> I will add a note to the changelog to that effect, and drop Fixes: since
>> the lock is not strictly required for correctness in this path.
>
>I rewrote the changelog to this:

Thanks, this clarified my doubt.

>
>Author: Dave Jiang <dave.jiang@intel.com>
>Date:   Fri Nov 17 13:18:48 2023 -0700
>
>    cxl/hdm: Fix a benign lockdep splat
>
>    The new helper "cxl_num_decoders_committed()" added a lockdep assertion
>    to validate that port->commit_end is protected against modification.
>    That assertion fires in init_hdm_decoder() where it is initializing
>    port->commit_end. Given that it is both accessing and writing that
>    property it obstensibly needs the lock.
>
>    In practice, CXL decoder commit rules (must commit in order) and the
>    in-order discovery of device decoders makes the manipulation of
>    ->commit_end in init_hdm_decoder() safe. However, rather than rely on
>    the subtle rules of CXL hardware, just make the implementation obviously
>    correct from a software perspective.
>
>    The Fixes: tag is only for cleaning up a lockdep splat, there is no
>    functional issue addressed by this fix.
>
>    Fixes: 458ba8189cb4 ("cxl: Add cxl_decoders_committed() helper")
>