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

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

fan wrote:
> On Wed, Nov 08, 2023 at 03:05:08PM -0700, 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>
> > ---
> >  drivers/cxl/core/hdm.c |    4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> > index bc8ad4a8afca..a8f960c496cb 100644
> > --- a/drivers/cxl/core/hdm.c
> > +++ b/drivers/cxl/core/hdm.c
> > @@ -854,10 +854,12 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
> >  			cxld->target_type = CXL_DECODER_HOSTONLYMEM;
> >  		else
> >  			cxld->target_type = CXL_DECODER_DEVMEM;
> > +		down_write(&cxl_region_rwsem);
> >  		if (cxld->id != cxl_num_decoders_committed(port)) {
> >  			dev_warn(&port->dev,
> >  				 "decoder%d.%d: Committed out of order\n",
> >  				 port->id, cxld->id);
> > +			up_write(&cxl_region_rwsem);
> >  			return -ENXIO;
> >  		}
> >  
> > @@ -865,9 +867,11 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
> >  			dev_warn(&port->dev,
> >  				 "decoder%d.%d: Committed with zero size\n",
> >  				 port->id, cxld->id);
> > +			up_write(&cxl_region_rwsem);
> >  			return -ENXIO;
> >  		}
> >  		port->commit_end = cxld->id;
> > +		up_write(&cxl_region_rwsem);
> >  	} else {
> >  		if (cxled) {
> >  			struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
> > 
> > 
> 
> I have two questions:
> 
> 1. There are some other locations in the code where commit_end has been
> read or updated, and not guarded by a lock, for example in
> cxl_decoder_commit. Do they need to be protected by cxl_region_rwsem
> also?

cxl_decoder_commit() is already called under the region rwsem, but yes at least
the poison lookup code is currently missing the lock.

> 2. Can we have a race condition here in init_hdm_decoder so commit_end
> need to be protected?

In this case it seems to be accidentally protected by the fact that decoders
must be committed in order and the discovery happens in order, so it is
difficult to see a path for a region commit / decommit operation racing
this update of the committed decoders.

That said, 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().