On 07.02.25 17:51:39, Jonathan Cameron wrote:
> On Thu, 6 Feb 2025 22:23:40 +0100
> Robert Richter <rrichter@amd.com> wrote:
>
> > On 14.01.25 11:16:41, Jonathan Cameron wrote:
> > > On Tue, 7 Jan 2025 15:10:14 +0100
> > > Robert Richter <rrichter@amd.com> wrote:
> > >
> > > > Broken target lists are hard to discover as the driver fails at a
> > > > later initialization stage. Add an error message for this.
> > > >
> > > > Signed-off-by: Robert Richter <rrichter@amd.com>
> > > > ---
> > > > drivers/cxl/core/region.c | 7 +++++++
> > > > 1 file changed, 7 insertions(+)
> > > >
> > > > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> > > > index 775450a1a887..2af3b6c14f46 100644
> > > > --- a/drivers/cxl/core/region.c
> > > > +++ b/drivers/cxl/core/region.c
> > > > @@ -1870,6 +1870,13 @@ static int find_pos_and_ways(struct cxl_port *port, struct range *range,
> > > > }
> > > > put_device(dev);
> > > >
> > > > + if (rc)
> > > > + dev_err(port->uport_dev,
> > > > + "failed to find %s:%s in target list of %s\n",
> > > > + dev_name(&port->dev),
> > > > + dev_name(port->parent_dport->dport_dev),
> > > > + dev_name(&cxlsd->cxld.dev));
> > > > +
> > > > return rc;
> > > > }
> > > This function would benefit from some __free() magic dust.
> > > Then we could return in the good path in the loop and not need the if (rc)
> > > check here.
> >
> > That does not really simplify the code. It would just this one
> > indentation. On the other side there is a central exit for the code
> > and we just need only that one put_device(). Plus, I like to have the
> > 'success' code path returning at the end of block.
>
> Seems simpler to me to return early on finding a match.
>
> static int find_pos_and_ways(struct cxl_port *port, struct range *range,
> int *pos, int *ways)
> {
> struct cxl_switch_decoder *cxlsd;
> struct cxl_port *parent;
>
> parent = next_port(port);
> if (!parent)
> return -ENXIO;
>
> struct device *dev __free(device) =
> device_find_child(&parent->dev, range,
> match_switch_decoder_by_range);
> if (!dev) {
> dev_err(port->uport_dev,
> "failed to find decoder mapping %#llx-%#llx\n",
> range->start, range->end);
> return -ENODEV;
> }
> cxlsd = to_cxl_switch_decoder(dev);
> *ways = cxlsd->cxld.interleave_ways;
>
> for (int i = 0; i < *ways; i++) {
> if (cxlsd->target[i] == port->parent_dport) {
> *pos = i;
> return 0;
> }
> }
> dev_err(port->uport_dev,
> "failed to find %s:%s in target list of %s\n",
> dev_name(&port->dev),
> dev_name(port->parent_dport->dport_dev),
> dev_name(&cxlsd->cxld.dev));
>
> return -ENXIO;
> }
>
> I don't mind that much though. I'd also suggest returning -ENXIO
> doesn't seem the right choice for failing to find something.
Right now I just want to add the dev_err() here. I could consider
changing to __free() and the error code in a next cleanup series. I
rather want to avoid to add (those 2) more patches to the series as
that increases time for upstream acceptance.
Thanks,
-Robert