Jonathan Cameron wrote:
> On Tue, 28 Jun 2022 17:12:04 +0100
> Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx> wrote:
>
> > On Thu, 23 Jun 2022 19:45:57 -0700
> > Dan Williams <dan.j.williams@xxxxxxxxx> wrote:
> >
> > > Currently 'struct cxl_decoder' contains the superset of attributes
> > > needed for all decoder types. Before more type-specific attributes are
> > > added to the common definition, reorganize 'struct cxl_decoder' into type
> > > specific objects.
> > >
> > > This patch, the first of three, factors out a cxl_switch_decoder type.
> > > The 'switch' decoder type represents the decoder instances of cxl_port's
> > > that route from the root of a CXL memory decode topology to the
> > > endpoints. They come in two flavors, root-level decoders, statically
> > > defined by platform firmware, and mid-level decoders, where
> > > interleave-granularity, interleave-width, and the target list are
> > > mutable.
> >
> > I'd like to see this info on cxl_switch_decoder being used for
> > switches AND other stuff as docs next to the definition. It confused
> > me when looked directly at the resulting of applying this series
> > and made more sense once I read to this patch.
> >
> > >
> > > Co-developed-by: Ben Widawsky <bwidawsk@xxxxxxxxxx>
> > > Signed-off-by: Ben Widawsky <bwidawsk@xxxxxxxxxx>
> > > Signed-off-by: Dan Williams <dan.j.williams@xxxxxxxxx>
> >
> > Basic idea is fine, but there are a few places where I think this is
> > 'too clever' with error handling and it's worth duplicating a few
> > error messages to keep the flow simpler.
> >
>
> follow up on that. I'd missed the kfree(alloc) hiding in plain
> sight at the end of the function.
>
>
>
> > > @@ -1179,13 +1210,27 @@ static struct cxl_decoder *cxl_decoder_alloc(struct cxl_port *port,
> > > {
> > > struct cxl_decoder *cxld;
> > > struct device *dev;
> > > + void *alloc;
> > > int rc = 0;
> > >
> > > if (nr_targets > CXL_DECODER_MAX_INTERLEAVE)
> > > return ERR_PTR(-EINVAL);
> > >
> > > - cxld = kzalloc(struct_size(cxld, target, nr_targets), GFP_KERNEL);
> > > - if (!cxld)
> > > + if (nr_targets) {
> > > + struct cxl_switch_decoder *cxlsd;
> > > +
> > > + alloc = kzalloc(struct_size(cxlsd, target, nr_targets), GFP_KERNEL);
> >
> > I'd rather see a local check on the allocation failure even if it adds a few lines
> > of duplicated code - which after you've dropped the local alloc variable won't be
> > much even after a later patch adds another path in here. The eventual code
> > of this function is more than a little nasty when an early return in each
> > path would, as far as I can tell, give the same result without the at least
> > 3 null checks prior to returning (to ensure nothing happens before reaching
> > the if (!alloc)
>
> clearly not enough caffeine that day as I'd missed the use for unifying
> the frees at the end of the function... Just noticed that in a later patch
> that touches the error path.
>
> I still don't much like the complexity of the flow, but can see why you did it
> this way now.
Appreciate it, it's much cleaner now with the nudge to take a second
look at reducing the complexity.
