Re: [PATCH 3/3] cxl/region: decrement ->nr_targets on error in cxl_region_attach()

[Dan Williams <dan.j.williams@intel.com>]

Dan Carpenter wrote:
> On Mon, Aug 01, 2022 at 01:20:58PM +0300, Dan Carpenter wrote:
> > The ++ needs a match -- on the clean up path.  If the p->nr_targets
> > value gets to be more than 16 it leads to uninitialized data in
> > cxl_port_setup_targets().
> > 
> > drivers/cxl/core/region.c:995 cxl_port_setup_targets() error: uninitialized symbol 'eiw'.
> > 
> > Fixes: 27b3f8d13830 ("cxl/region: Program target lists")
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> 
> The error handling in cxl_port_attach_region() looks like it might have
> a similar bug.  The cxl_rr->nr_targets++; might want a --.
> 
> That function is more complicated.
> 
> drivers/cxl/core/region.c
>    740  static int cxl_port_attach_region(struct cxl_port *port,
>    741                                    struct cxl_region *cxlr,
>    742                                    struct cxl_endpoint_decoder *cxled, int pos)
>    743  {
>    744          struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
>    745          struct cxl_ep *ep = cxl_ep_load(port, cxlmd);
>    746          struct cxl_region_ref *cxl_rr = NULL, *iter;
>    747          struct cxl_region_params *p = &cxlr->params;
>    748          struct cxl_decoder *cxld = NULL;
>    749          unsigned long index;
>    750          int rc = -EBUSY;
>    751  
>    752          lockdep_assert_held_write(&cxl_region_rwsem);
>    753  
>    754          xa_for_each(&port->regions, index, iter) {
>    755                  struct cxl_region_params *ip = &iter->region->params;
>    756  
>    757                  if (iter->region == cxlr)
>    758                          cxl_rr = iter;
> 
> Should there be a break statement after this assignment

Indeed. If the port already has this region attached it means that it
already passed this check previously.

> 
>    759                  if (ip->res->start > p->res->start) {
> 
> or do we really want to test every ip->res->start?  This loop is
> confusing...

Let me take a shot at reflowing this whole routine to make it less
confusing.