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

[quic_zijuhu]

On 9/10/2024 8:45 AM, Dan Williams wrote:
> Ira Weiny wrote:
> [..]
>>> 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?
> 
> I would prefer just dropping usage of device_find_ or device_for_each_
> with storing an array decoders in the port directly. The port already
> has arrays for dports , endpoints, and regions. Using the "device" APIs
> to iterate children was a bit lazy, and if the id is used as the array
> key then a direct lookup makes some cases simpler.

it seems Ira and Dan have corrected original logic to ensure
that all child decoders are sorted by ID in ascending order as shown
by below link.

https://lore.kernel.org/all/66df666ded3f7_3c80f229439@iweiny-mobl.notmuch/

based on above correction, as shown by my another exclusive fix
https://lore.kernel.org/all/20240905-fix_cxld-v2-1-51a520a709e4@xxxxxxxxxxx/
there are a very simple change to solve the remaining original concern
that device_find_child() modifies caller's match data.

here is the simple change.

--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -797,23 +797,13 @@ static size_t show_targetN(struct cxl_region
*cxlr, char *buf, int pos)
 static int match_free_decoder(struct device *dev, void *data)
 {
        struct cxl_decoder *cxld;
-       int *id = data;

        if (!is_switch_decoder(dev))
                return 0;

        cxld = to_cxl_decoder(dev);

-       /* enforce ordered allocation */
-       if (cxld->id != *id)
-               return 0;
-
-       if (!cxld->region)
-               return 1;
-
-       (*id)++;
-
-       return 0;
+       return cxld->region ? 0 : 1;
 }

 static int match_auto_decoder(struct device *dev, void *data)
@@ -840,7 +830,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 +838,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 = device_find_child(&port->dev, NULL,
match_free_decoder);
        if (!dev)
                return NULL;