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

[Ira Weiny]

Jonathan Cameron wrote:
> On Sat, 24 Aug 2024 17:07:44 +0800
> Zijun Hu <zijun_hu@xxxxxxxxxx> wrote:
> 

[snip]

> >  
> > +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)
> 
> Why do we carry on in this case?
> Conditions are:
> 1. Start match_data->id == 0
> 2. First pass cxld->id == 0 (all good) or
>    cxld->id == 1 say (and we skip until we match
>    on cxld->id == 0 (perhaps on the second child if they are
>    ordered (1, 0, 2) etc. 
> 
> If we skipped and then matched on second child but it was
> already in use (so region set), we will increment match_data->id to 1
> but never find that as it was the one we skipped.
> 
> So this can only work if the children are ordered.
> So if that's the case and the line above is just a sanity check
> on that, it should be noisier (so an error print) and might
> as well fail as if it doesn't match all bets are off.
> 

I've worked through this with Dan and the devices are added in order in
devm_cxl_enumerate_decoders().

So I don't think there is an issue with converting the code directly.

Sorry for the noise Jijun,
Ira