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

From: quic_zijuhu
Date: Thu Sep 05 2024 - 04:51:41 EST


On 9/5/2024 1:32 PM, Greg Kroah-Hartman wrote:
> On Thu, Sep 05, 2024 at 08:36:09AM +0800, Zijun Hu wrote:
>> From: Zijun Hu <quic_zijuhu@xxxxxxxxxxx>
>>
>> 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@xxxxxxxxx>
>> Signed-off-by: Zijun Hu <quic_zijuhu@xxxxxxxxxxx>
>> ---
>> 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?
>

it is called within cxl_region_find_decoder()

> Ah, it's on the drop later on after find_free_decoder(), right?

yes, it shares the same put_device() which is used for original
device_find_child().

>
>> 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.
>

for match_auto_decoder() original logic, nothing of aim device is
modified, it just need to modifies state or @id to find the aim device.


> thanks,
>
> greg k-h