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

[Ira Weiny <ira.weiny@intel.com>]

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

This part of the patch seemed unbalanced to me.  Looking at the check for size
== 0 I feel like it would be better to do that check first outside of the lock.

Something like below.

Ira

14:36:11 > git di --patience
diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
index 1cc9be85ba4c..370ba5e39363 100644
--- a/drivers/cxl/core/hdm.c
+++ b/drivers/cxl/core/hdm.c
@@ -839,12 +839,6 @@ 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;
-               if (cxld->id != cxl_num_decoders_committed(port)) {
-                       dev_warn(&port->dev,
-                                "decoder%d.%d: Committed out of order\n",
-                                port->id, cxld->id);
-                       return -ENXIO;
-               }
 
                if (size == 0) {
                        dev_warn(&port->dev,
@@ -852,7 +846,17 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
                                 port->id, cxld->id);
                        return -ENXIO;
                }
+
+               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;
+               }
                port->commit_end = cxld->id;
+               up_write(&cxl_region_rwsem);
        } else {
                if (cxled) {
                        struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);