linux-kernel - Re: [PATCH v1 28/29] cxl/region: Show message on broken target list

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <Z6xlB5Gw-8vJjHe1@rric.localdomain>
Date: Wed, 12 Feb 2025 10:08:23 +0100
From: Robert Richter <rrichter@....com>
To: Jonathan Cameron <Jonathan.Cameron@...wei.com>
Cc: Alison Schofield <alison.schofield@...el.com>,
	Vishal Verma <vishal.l.verma@...el.com>,
	Ira Weiny <ira.weiny@...el.com>,
	Dan Williams <dan.j.williams@...el.com>,
	Dave Jiang <dave.jiang@...el.com>,
	Davidlohr Bueso <dave@...olabs.net>, linux-cxl@...r.kernel.org,
	linux-kernel@...r.kernel.org, Gregory Price <gourry@...rry.net>,
	"Fabio M. De Francesco" <fabio.m.de.francesco@...ux.intel.com>,
	Terry Bowman <terry.bowman@....com>
Subject: Re: [PATCH v1 28/29] cxl/region: Show message on broken target list

On 07.02.25 17:51:39, Jonathan Cameron wrote:
> On Thu, 6 Feb 2025 22:23:40 +0100
> Robert Richter <rrichter@....com> wrote:
> 
> > On 14.01.25 11:16:41, Jonathan Cameron wrote:
> > > On Tue, 7 Jan 2025 15:10:14 +0100
> > > Robert Richter <rrichter@....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@....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