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

[Dan Williams]

quic_zijuhu wrote:
> 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;

So I wanted to write a comment here to stop the next person from
tripping over this dependency on decoder 'add' order, but there is a
problem. For this simple version to work it needs 3 things:

1/ decoders are added in hardware id order: done,
devm_cxl_enumerate_decoders() handles that

2/ search for decoders in their added order: done, device_find_child()
guarantees this, although it is not obvious without reading the internals
of device_add().

3/ regions are de-allocated from decoders in reverse decoder id order.
This is not enforced, in fact it is impossible to enforce. Consider that
any memory device can be removed at any time and may not be removed in
the order in which the device allocated switch decoders in the topology.

So, that existing comment of needing to enforce ordered allocation is
still relevant even though the implementation fails to handle the
out-of-order region deallocation problem.

I alluded to the need for a "tear down the world" implementation back in
2022 [1], but never got around to finishing that.

Now, the cxl_port.hdm_end attribute tracks the "last" decoder to be
allocated for endpoint ports. That same tracking needs to be added for
switch ports, then this routine could check for ordering constraints by:

    /* enforce hardware ordered allocation */
    if (!cxld->region && port->hdm_end + 1 == cxld->id)
        return 1;
    return 0;

As it stands now @hdm_end is never updated for switch ports.

[1]: 176baefb2eb5 cxl/hdm: Commit decoder state to hardware







Yes, that looks simple enough for now, although lets not use a ternary
condition and lets leave a comment for the next person:

/* decoders are added in hardware id order
 * (devm_cxl_enumerate_decoders), allocated to regions in id order
 * (device_find_child() walks children in 'add' order)
 */
>  }
> 
>  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;
> 
>