| Message ID | 167124081824.1626103.1555704405392757219.stgit@dwillia2-xfh.jf.intel.com |
|---|---|
| State | Accepted |
| Commit | af3ea9ab61d728d5a8be01bbec6d5cf7551b9600 |
| Headers | show |
| Series | cxl: Misc fixups that missed v6.2 | expand |
On Fri, 16 Dec 2022 17:33:38 -0800 Dan Williams <dan.j.williams@intel.com> wrote: > Both cxl_switch_decoders() and cxl_endpoint_decoders() are considered by > cxl_region_decode_commit(). Flag cases where cxl_switch_decoders with > multiple targets, or cxl_endpoint_decoders do not have a commit callback > set. The switch case is unlikely to happen since switches are only > enumerated by the CXL core, but the endpoint case may support decoders > defined by drivers outside of drivers/cxl, like accerator drivers. > > Signed-off-by: Dan Williams <dan.j.williams@intel.com> Seems reasonable though (just for consistency with earlier discussions) I don't like assumption that nr->targets is the right thing to decide on. It's fine to have decoders in single target case and if we do they should be committed / not registered as pass through decoders etc. Hmm. I've never tested the single downstream CXL switch port case. Similar to HB in that case HDM decoders are optional. I've no idea what we do about that corner currently. One to add to my tests ;) Anyhow, this is fine Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > --- > drivers/cxl/core/region.c | 19 +++++++++++++++++-- > 1 file changed, 17 insertions(+), 2 deletions(-) > > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c > index c11a6ab5e48d..60828d01972a 100644 > --- a/drivers/cxl/core/region.c > +++ b/drivers/cxl/core/region.c > @@ -156,6 +156,22 @@ static int cxl_region_decode_reset(struct cxl_region *cxlr, int count) > return 0; > } > > +static int commit_decoder(struct cxl_decoder *cxld) > +{ > + struct cxl_switch_decoder *cxlsd = NULL; > + > + if (cxld->commit) > + return cxld->commit(cxld); > + > + if (is_switch_decoder(&cxld->dev)) > + cxlsd = to_cxl_switch_decoder(&cxld->dev); > + > + if (dev_WARN_ONCE(&cxld->dev, !cxlsd || cxlsd->nr_targets > 1, > + "->commit() is required\n")) > + return -ENXIO; > + return 0; > +} > + > static int cxl_region_decode_commit(struct cxl_region *cxlr) > { > struct cxl_region_params *p = &cxlr->params; > @@ -174,8 +190,7 @@ static int cxl_region_decode_commit(struct cxl_region *cxlr) > iter = to_cxl_port(iter->dev.parent)) { > cxl_rr = cxl_rr_load(iter, cxlr); > cxld = cxl_rr->decoder; > - if (cxld->commit) > - rc = cxld->commit(cxld); > + rc = commit_decoder(cxld); > if (rc) > break; > } >
Jonathan Cameron wrote: > On Fri, 16 Dec 2022 17:33:38 -0800 > Dan Williams <dan.j.williams@intel.com> wrote: > > > Both cxl_switch_decoders() and cxl_endpoint_decoders() are considered by > > cxl_region_decode_commit(). Flag cases where cxl_switch_decoders with > > multiple targets, or cxl_endpoint_decoders do not have a commit callback > > set. The switch case is unlikely to happen since switches are only > > enumerated by the CXL core, but the endpoint case may support decoders > > defined by drivers outside of drivers/cxl, like accerator drivers. > > > > Signed-off-by: Dan Williams <dan.j.williams@intel.com> > > Seems reasonable though (just for consistency with earlier discussions) > I don't like assumption that nr->targets is the right thing to decide on. > It's fine to have decoders in single target case and if we do > they should be committed / not registered as pass through decoders etc. Thanks, yes, I can see there being cases where a decoder only has 1 target, but needs to be committed anyway just to pass the assigned HPA through. For this patch that missed consideration it just leads to a false negative report on a kernel bug failing to install a ->commit() callback for such a decoder. > Hmm. I've never tested the single downstream CXL switch port case. > Similar to HB in that case HDM decoders are optional. I've no idea > what we do about that corner currently. One to add to my tests ;) Yes, and certainly a heads up for anyone in the industry who might be thinking of buliding such a device. > Anyhow, this is fine > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> Thanks!
diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c index c11a6ab5e48d..60828d01972a 100644 --- a/drivers/cxl/core/region.c +++ b/drivers/cxl/core/region.c @@ -156,6 +156,22 @@ static int cxl_region_decode_reset(struct cxl_region *cxlr, int count) return 0; } +static int commit_decoder(struct cxl_decoder *cxld) +{ + struct cxl_switch_decoder *cxlsd = NULL; + + if (cxld->commit) + return cxld->commit(cxld); + + if (is_switch_decoder(&cxld->dev)) + cxlsd = to_cxl_switch_decoder(&cxld->dev); + + if (dev_WARN_ONCE(&cxld->dev, !cxlsd || cxlsd->nr_targets > 1, + "->commit() is required\n")) + return -ENXIO; + return 0; +} + static int cxl_region_decode_commit(struct cxl_region *cxlr) { struct cxl_region_params *p = &cxlr->params; @@ -174,8 +190,7 @@ static int cxl_region_decode_commit(struct cxl_region *cxlr) iter = to_cxl_port(iter->dev.parent)) { cxl_rr = cxl_rr_load(iter, cxlr); cxld = cxl_rr->decoder; - if (cxld->commit) - rc = cxld->commit(cxld); + rc = commit_decoder(cxld); if (rc) break; }
Both cxl_switch_decoders() and cxl_endpoint_decoders() are considered by cxl_region_decode_commit(). Flag cases where cxl_switch_decoders with multiple targets, or cxl_endpoint_decoders do not have a commit callback set. The switch case is unlikely to happen since switches are only enumerated by the CXL core, but the endpoint case may support decoders defined by drivers outside of drivers/cxl, like accerator drivers. Signed-off-by: Dan Williams <dan.j.williams@intel.com> --- drivers/cxl/core/region.c | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-)