Re: [PATCH v4 1/2] cxl/region: Find free cxl decoder by device_for_each_child()

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

Greg Kroah-Hartman wrote:
> On Thu, Sep 05, 2024 at 08:36:09AM +0800, Zijun Hu wrote:
> > From: Zijun Hu <quic_zijuhu@quicinc.com>
> > 
> > To prepare for constifying the following old driver core API:
> > 
> > struct device *device_find_child(struct device *dev, void *data,
> > 		int (*match)(struct device *dev, void *data));
> > to new:
> > struct device *device_find_child(struct device *dev, const void *data,
> > 		int (*match)(struct device *dev, const void *data));
> > 
> > The new API does not allow its match function (*match)() to modify
> > caller's match data @*data, but match_free_decoder() as the old API's
> > match function indeed modifies relevant match data, so it is not suitable
> > for the new API any more, solved by using device_for_each_child() to
> > implement relevant finding free cxl decoder function.
> > 
> > By the way, this commit does not change any existing logic.
> > 
> > Suggested-by: Ira Weiny <ira.weiny@intel.com>
> > Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
> > ---
> >  drivers/cxl/core/region.c | 30 ++++++++++++++++++++++++------
> >  1 file changed, 24 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> > index 21ad5f242875..c2068e90bf2f 100644
> > --- a/drivers/cxl/core/region.c
> > +++ b/drivers/cxl/core/region.c
> > @@ -794,10 +794,15 @@ static size_t show_targetN(struct cxl_region *cxlr, char *buf, int pos)
> >  	return rc;
> >  }
> >  
> > +struct cxld_match_data {
> > +	int id;
> > +	struct device *target_device;
> > +};
> > +
> >  static int match_free_decoder(struct device *dev, void *data)
> >  {
> > +	struct cxld_match_data *match_data = data;
> >  	struct cxl_decoder *cxld;
> > -	int *id = data;
> >  
> >  	if (!is_switch_decoder(dev))
> >  		return 0;
> > @@ -805,17 +810,31 @@ static int match_free_decoder(struct device *dev, void *data)
> >  	cxld = to_cxl_decoder(dev);
> >  
> >  	/* enforce ordered allocation */
> > -	if (cxld->id != *id)
> > +	if (cxld->id != match_data->id)
> >  		return 0;
> >  
> > -	if (!cxld->region)
> > +	if (!cxld->region) {
> > +		match_data->target_device = get_device(dev);
> 
> Where is put_device() called?
> 
> Ah, it's on the drop later on after find_free_decoder(), right?
> 
> >  		return 1;
> > +	}
> >  
> > -	(*id)++;
> > +	match_data->id++;
> >  
> >  	return 0;
> >  }
> >  
> > +/* NOTE: need to drop the reference with put_device() after use. */
> > +static struct device *find_free_decoder(struct device *parent)
> > +{
> > +	struct cxld_match_data match_data = {
> > +		.id = 0,
> > +		.target_device = NULL,
> > +	};
> > +
> > +	device_for_each_child(parent, &match_data, match_free_decoder);
> > +	return match_data.target_device;
> > +}
> > +
> >  static int match_auto_decoder(struct device *dev, void *data)
> >  {
> >  	struct cxl_region_params *p = data;
> > @@ -840,7 +859,6 @@ cxl_region_find_decoder(struct cxl_port *port,
> >  			struct cxl_region *cxlr)
> >  {
> >  	struct device *dev;
> > -	int id = 0;
> >  
> >  	if (port == cxled_to_port(cxled))
> >  		return &cxled->cxld;
> > @@ -849,7 +867,7 @@ cxl_region_find_decoder(struct cxl_port *port,
> >  		dev = device_find_child(&port->dev, &cxlr->params,
> >  					match_auto_decoder);
> >  	else
> > -		dev = device_find_child(&port->dev, &id, match_free_decoder);
> > +		dev = find_free_decoder(&port->dev);
> 
> This still feels more complex that I think it should be.  Why not just
> modify the needed device information after the device is found?  What
> exactly is being changed in the match_free_decoder that needs to keep
> "state"?  This feels odd.

Agreed it is odd.

How about adding?


diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index c2068e90bf2f..5d9017e6f16e 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -814,7 +814,7 @@ static int match_free_decoder(struct device *dev, void *data)
                return 0;
 
        if (!cxld->region) {
-               match_data->target_device = get_device(dev);
+               match_data->target_device = dev;
                return 1;
        }
 
@@ -853,6 +853,22 @@ static int match_auto_decoder(struct device *dev, void *data)
        return 0;
 }
 
+static struct device *find_auto_decoder(struct device *parent,
+                                       struct cxl_region *cxlr)
+{
+       struct device *dev;
+
+       dev = device_find_child(parent, &cxlr->params, match_auto_decoder);
+       /*
+        * This decoder is pinned registered as long as the endpoint decoder is
+        * registered, and endpoint decoder unregistration holds the
+        * cxl_region_rwsem over unregister events, so no need to hold on to
+        * this extra reference.
+        */
+       put_device(dev);
+       return dev;
+}
+
 static struct cxl_decoder *
 cxl_region_find_decoder(struct cxl_port *port,
                        struct cxl_endpoint_decoder *cxled,
@@ -864,19 +880,11 @@ cxl_region_find_decoder(struct cxl_port *port,
                return &cxled->cxld;
 
        if (test_bit(CXL_REGION_F_AUTO, &cxlr->flags))
-               dev = device_find_child(&port->dev, &cxlr->params,
-                                       match_auto_decoder);
+               dev = find_auto_decoder(&port->dev, cxlr);
        else
                dev = find_free_decoder(&port->dev);
        if (!dev)
                return NULL;
-       /*
-        * This decoder is pinned registered as long as the endpoint decoder is
-        * registered, and endpoint decoder unregistration holds the
-        * cxl_region_rwsem over unregister events, so no need to hold on to
-        * this extra reference.
-        */
-       put_device(dev);
        return to_cxl_decoder(dev);
 }