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.
>
>
>
>
> cxlsd = kzalloc()
> if (!cxlsd)
> return ERR_PTR(-ENOMEM);
>
> cxlsd->nr_targets = nr_targets;
> seqlock_init(...)
>
> } else {
> cxld = kzalloc(sizerof(*cxld), GFP_KERNEL);
> if (!cxld)
> return ERR_PTR(-ENOMEM);
>
> > + cxlsd = alloc;
> > + if (cxlsd) {
> > + cxlsd->nr_targets = nr_targets;
> > + seqlock_init(&cxlsd->target_lock);
> > + cxld = &cxlsd->cxld;
> > + }
> > + } else {
> > + alloc = kzalloc(sizeof(*cxld), GFP_KERNEL);
> > + cxld = alloc;
> > + }
> > + if (!alloc)
> > return ERR_PTR(-ENOMEM);
> >
> > rc = ida_alloc(&port->decoder_ida, GFP_KERNEL);
> > @@ -1196,8 +1241,6 @@ static struct cxl_decoder *cxl_decoder_alloc(struct cxl_port *port,
> > get_device(&port->dev);
> > cxld->id = rc;
> >
> > - cxld->nr_targets = nr_targets;
> > - seqlock_init(&cxld->target_lock);
> > dev = &cxld->dev;
> > device_initialize(dev);
> > lockdep_set_class(&dev->mutex, &cxl_decoder_key);
> > @@ -1222,7 +1265,7 @@ static struct cxl_decoder *cxl_decoder_alloc(struct cxl_port *port,
> >
> > return cxld;
> > err:
> > - kfree(cxld);
> > + kfree(alloc);
> > return ERR_PTR(rc);
> > }
