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

[Dan Williams <dan.j.williams@intel.com>]

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:

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")