Re: [PATCH v3 25/28] cxl/bus: Populate the target list at decoder create

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Fri, 3 Sep 2021 15:43:25 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> On Fri, Sep 3, 2021 at 5:59 AM Jonathan Cameron
> <Jonathan.Cameron@huawei.com> wrote:
> >
> > On Tue, 24 Aug 2021 09:07:39 -0700
> > Dan Williams <dan.j.williams@intel.com> wrote:
> >  
> > > As found by cxl_test, the implementation populated the target_list for
> > > the single dport exceptional case, it missed populating the target_list
> > > for the typical multi-dport case.  
> >
> > Description makes this sound like a fix, rather than what I think it is
> > which is implementing a new feature...  
> 
> It is finishing a feature where the unfinished state is broken. It
> should never be the case that target_list_show() returns nothing.
> 
> [..]
> > > diff --git a/drivers/cxl/core/bus.c b/drivers/cxl/core/bus.c
> > > index 8073354ba232..9a755a37eadf 100644
> > > --- a/drivers/cxl/core/bus.c
> > > +++ b/drivers/cxl/core/bus.c  
> [..]
> > > @@ -493,10 +494,19 @@ cxl_decoder_alloc(struct cxl_port *port, int nr_targets, resource_size_t base,
> > >               .target_type = type,
> > >       };
> > >
> > > -     /* handle implied target_list */
> > > -     if (interleave_ways == 1)
> > > -             cxld->target[0] =
> > > -                     list_first_entry(&port->dports, struct cxl_dport, list);
> > > +     device_lock(&port->dev);
> > > +     for (i = 0; target_map && i < nr_targets; i++) {  
> >
> > Perhaps move target map check much earlier rather than putting it
> > int he loop condition?  I don't think the loop is modifying it...  
> 
> The loop is not modifying target_map, but target_map is allowed to be
> NULL. I was trying to avoid a non-error goto, but a better way to
> solve that would be to make the loop a helper function taken under the
> lock.
Ah. Understood.  I was not appreciating that check need to be under
the device_lock(). Helper function would indeed make this clean - good plan.

Thanks,

Jonathan