| Message ID | 20241017-const_dfc_prepare-v6-1-fc842264a1cc@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v6] cxl/region: Fix wrong logic for finding a free switch cxl decoder | expand |
Zijun Hu wrote: > From: Zijun Hu <quic_zijuhu@quicinc.com> > > Provided that all child switch cxl decoders are sorted by ID in ascending > order, then it is wrong for current match_free_decoder()'s logic to find > a free cxl decoder as explained below: > > Port > ├── cxld A <----> region A > ├── cxld B // no region > ├── cxld C <----> region C > > Current logic will find cxld B as a free one, but cxld B is not true > free since region C has not been torn down, so current logic is wrong. > > Fixed by verifying if cxl decoder with ID (@port->commit_end + 1) can > be returned as finding result. > > Link: https://lore.kernel.org/all/670af54931b8_964fe29427@dwillia2-xfh.jf.intel.com.notmuch/ > Suggested-by: Dan Williams <dan.j.williams@intel.com> > Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com> > --- > This patch is incremental to below patch series with title "cxl: Initialization and shutdown fixes" > http://lore.kernel.org/172862483180.2150669.5564474284074502692.stgit@dwillia2-xfh.jf.intel.com > > This patch is based on Dan's recommendation shown by below link: > https://lore.kernel.org/all/670af54931b8_964fe29427@dwillia2-xfh.jf.intel.com.notmuch/ ...and Dan and already sent a patch he is happier with here: http://lore.kernel.org/172895072669.39002.9296583943188706348.stgit@dwillia2-xfh.jf.intel.com
On 2024/10/17 07:02, Dan Williams wrote: > Zijun Hu wrote: >> From: Zijun Hu <quic_zijuhu@quicinc.com> >> >> Provided that all child switch cxl decoders are sorted by ID in ascending >> order, then it is wrong for current match_free_decoder()'s logic to find >> a free cxl decoder as explained below: >> >> Port >> ├── cxld A <----> region A >> ├── cxld B // no region >> ├── cxld C <----> region C >> >> Current logic will find cxld B as a free one, but cxld B is not true >> free since region C has not been torn down, so current logic is wrong. >> >> Fixed by verifying if cxl decoder with ID (@port->commit_end + 1) can >> be returned as finding result. >> >> Link: https://lore.kernel.org/all/670af54931b8_964fe29427@dwillia2-xfh.jf.intel.com.notmuch/ >> Suggested-by: Dan Williams <dan.j.williams@intel.com> >> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com> >> --- >> This patch is incremental to below patch series with title "cxl: Initialization and shutdown fixes" >> http://lore.kernel.org/172862483180.2150669.5564474284074502692.stgit@dwillia2-xfh.jf.intel.com >> >> This patch is based on Dan's recommendation shown by below link: >> https://lore.kernel.org/all/670af54931b8_964fe29427@dwillia2-xfh.jf.intel.com.notmuch/ > > ...and Dan and already sent a patch he is happier with here: > > http://lore.kernel.org/172895072669.39002.9296583943188706348.stgit@dwillia2-xfh.jf.intel.com this is a optimized one based your recommendation, namely, above link. please don't hesitate to use it as your V2 if you like this one. you maybe also have below change when you send V2 Reported-by: Zijun Hu <zijun_hu@icloud.com> to Reported-by: Zijun Hu <quic_zijuhu@quicinc.com> So let us close discussion for this mail thread and go to your above thread.
diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c index e701e4b04032..a7a82da39393 100644 --- a/drivers/cxl/core/region.c +++ b/drivers/cxl/core/region.c @@ -796,8 +796,9 @@ static size_t show_targetN(struct cxl_region *cxlr, char *buf, int pos) static int match_free_decoder(struct device *dev, void *data) { + struct cxl_port *port = to_cxl_port(dev->parent); struct cxl_decoder *cxld; - int *id = data; + struct device **target_dev = data; if (!is_switch_decoder(dev)) return 0; @@ -805,15 +806,24 @@ static int match_free_decoder(struct device *dev, void *data) cxld = to_cxl_decoder(dev); /* enforce ordered allocation */ - if (cxld->id != *id) - return 0; + if (cxld->id == port->commit_end + 1) { + if (!cxld->region) { + *target_dev = dev; + return 1; + } - if (!cxld->region) - return 1; + dev_dbg(dev->parent, + "%s: cxld '%s' was reserved by '%s'\n", __func__, + dev_name(dev), dev_name(&cxld->region->dev)); + return -EBUSY; + } - (*id)++; + if (cxld->flags & CXL_DECODER_F_ENABLE) + return 0; - return 0; + dev_dbg(dev->parent, "%s: cxld '%s' was out of order shut down\n", + __func__, dev_name(dev)); + return -EINVAL; } static int match_auto_decoder(struct device *dev, void *data) @@ -839,17 +849,18 @@ cxl_region_find_decoder(struct cxl_port *port, struct cxl_endpoint_decoder *cxled, struct cxl_region *cxlr) { - struct device *dev; - int id = 0; + struct device *dev = NULL; if (port == cxled_to_port(cxled)) return &cxled->cxld; - if (test_bit(CXL_REGION_F_AUTO, &cxlr->flags)) + if (test_bit(CXL_REGION_F_AUTO, &cxlr->flags)) { dev = device_find_child(&port->dev, &cxlr->params, match_auto_decoder); - else - dev = device_find_child(&port->dev, &id, match_free_decoder); + } else { + device_for_each_child(&port->dev, &dev, match_free_decoder); + get_device(dev); + } if (!dev) return NULL; /*