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
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
