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@xxxxxxxxxx>
>
> 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.
