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

[fan <nifan.cxl@gmail.com>]

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?
2. Can we have a race condition here in init_hdm_decoder so commit_end
need to be protected?

Fan