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

[Dan Carpenter <dan.carpenter@oracle.com>]

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

   759                  if (ip->res->start > p->res->start) {

or do we really want to test every ip->res->start?  This loop is
confusing...

   760                          dev_dbg(&cxlr->dev,
   761                                  "%s: HPA order violation %s:%pr vs %pr\n",
   762                                  dev_name(&port->dev),
   763                                  dev_name(&iter->region->dev), ip->res, p->res);
   764                          return -EBUSY;
   765                  }
   766          }
   767  
   768          if (cxl_rr) {
   769                  struct cxl_ep *ep_iter;
   770                  int found = 0;
   771  
   772                  cxld = cxl_rr->decoder;
   773                  xa_for_each(&cxl_rr->endpoints, index, ep_iter) {

regards,
dan carpenter