Jonathan Cameron wrote: > On Tue, 28 Jun 2022 17:12:04 +0100 > Jonathan Cameron <[email protected]> wrote: > > > On Thu, 23 Jun 2022 19:45:57 -0700 > > Dan Williams <[email protected]> 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 <[email protected]> > > > Signed-off-by: Ben Widawsky <[email protected]> > > > Signed-off-by: Dan Williams <[email protected]> > > > > 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.
