| Message ID | 166326546707.3348078.8667496731861557941.stgit@djiang5-desk3.ch.intel.com |
|---|---|
| State | Superseded |
| Delegated to: | Dan Williams |
| Headers | show |
| Series | cxl: check decoder count for end device | expand |
On Thu, 15 Sep 2022 11:11:07 -0700 Dave Jiang <dave.jiang@intel.com> wrote: > CXL spec rev3.0 8.2.4.19.1 added definition for up to 32 decoders. It also > indicates that for devices, only 10 decoders should be advertised. Add > check on number of decoders greater than 10 for devices and reset to 10 to > force spec compliance. > > Signed-off-by: Dave Jiang <dave.jiang@intel.com> Hi Dave Seems reasonable. I'm a bit curious to whether there is an actual problem if a device is spec non compliant and advertises more decoders? Either way does no harm and might let us know something fishy is happening. Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > --- > drivers/cxl/core/hdm.c | 16 +++++++++++++++- > 1 file changed, 15 insertions(+), 1 deletion(-) > > diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c > index d1d2caea5c62..1919d99d157e 100644 > --- a/drivers/cxl/core/hdm.c > +++ b/drivers/cxl/core/hdm.c > @@ -71,9 +71,23 @@ EXPORT_SYMBOL_NS_GPL(devm_cxl_add_passthrough_decoder, CXL); > static void parse_hdm_decoder_caps(struct cxl_hdm *cxlhdm) > { > u32 hdm_cap; > + int decoder_count; > + struct device *dev = &cxlhdm->port->dev; > > hdm_cap = readl(cxlhdm->regs.hdm_decoder + CXL_HDM_DECODER_CAP_OFFSET); > - cxlhdm->decoder_count = cxl_hdm_decoder_count(hdm_cap); > + decoder_count = cxl_hdm_decoder_count(hdm_cap); > + /* > + * CXL spec rev3.0 8.2.4.19.1 indicates CXL devices shall not advertise > + * more than 10 decoders. Switches and Host Bridges may advertise up to > + * 32 decoders. Set the decoders to 10 for devices if more than 10 are > + * found. > + */ > + if (is_cxl_endpoint(cxlhdm->port) && decoder_count > 10) { > + dev_warn(dev, "Reset decoders count (%d) to 10, spec violation!\n", > + decoder_count); > + decoder_count = 10; > + } > + cxlhdm->decoder_count = decoder_count; > cxlhdm->target_count = > FIELD_GET(CXL_HDM_DECODER_TARGET_COUNT_MASK, hdm_cap); > if (FIELD_GET(CXL_HDM_DECODER_INTERLEAVE_11_8, hdm_cap)) > >
On 10/11/2022 9:45 AM, Jonathan Cameron wrote: > On Thu, 15 Sep 2022 11:11:07 -0700 > Dave Jiang <dave.jiang@intel.com> wrote: > >> CXL spec rev3.0 8.2.4.19.1 added definition for up to 32 decoders. It also >> indicates that for devices, only 10 decoders should be advertised. Add >> check on number of decoders greater than 10 for devices and reset to 10 to >> force spec compliance. >> >> Signed-off-by: Dave Jiang <dave.jiang@intel.com> > Hi Dave > > Seems reasonable. I'm a bit curious to whether there is an actual problem if > a device is spec non compliant and advertises more decoders? Not that I'm aware of. Just being cautious. > > Either way does no harm and might let us know something fishy is happening. > > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> Thanks! > >> --- >> drivers/cxl/core/hdm.c | 16 +++++++++++++++- >> 1 file changed, 15 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c >> index d1d2caea5c62..1919d99d157e 100644 >> --- a/drivers/cxl/core/hdm.c >> +++ b/drivers/cxl/core/hdm.c >> @@ -71,9 +71,23 @@ EXPORT_SYMBOL_NS_GPL(devm_cxl_add_passthrough_decoder, CXL); >> static void parse_hdm_decoder_caps(struct cxl_hdm *cxlhdm) >> { >> u32 hdm_cap; >> + int decoder_count; >> + struct device *dev = &cxlhdm->port->dev; >> >> hdm_cap = readl(cxlhdm->regs.hdm_decoder + CXL_HDM_DECODER_CAP_OFFSET); >> - cxlhdm->decoder_count = cxl_hdm_decoder_count(hdm_cap); >> + decoder_count = cxl_hdm_decoder_count(hdm_cap); >> + /* >> + * CXL spec rev3.0 8.2.4.19.1 indicates CXL devices shall not advertise >> + * more than 10 decoders. Switches and Host Bridges may advertise up to >> + * 32 decoders. Set the decoders to 10 for devices if more than 10 are >> + * found. >> + */ >> + if (is_cxl_endpoint(cxlhdm->port) && decoder_count > 10) { >> + dev_warn(dev, "Reset decoders count (%d) to 10, spec violation!\n", >> + decoder_count); >> + decoder_count = 10; >> + } >> + cxlhdm->decoder_count = decoder_count; >> cxlhdm->target_count = >> FIELD_GET(CXL_HDM_DECODER_TARGET_COUNT_MASK, hdm_cap); >> if (FIELD_GET(CXL_HDM_DECODER_INTERLEAVE_11_8, hdm_cap)) >> >>
Dave Jiang wrote: > CXL spec rev3.0 8.2.4.19.1 added definition for up to 32 decoders. It also > indicates that for devices, only 10 decoders should be advertised. Add > check on number of decoders greater than 10 for devices and reset to 10 to > force spec compliance. > > Signed-off-by: Dave Jiang <dave.jiang@intel.com> > --- > drivers/cxl/core/hdm.c | 16 +++++++++++++++- > 1 file changed, 15 insertions(+), 1 deletion(-) > > diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c > index d1d2caea5c62..1919d99d157e 100644 > --- a/drivers/cxl/core/hdm.c > +++ b/drivers/cxl/core/hdm.c > @@ -71,9 +71,23 @@ EXPORT_SYMBOL_NS_GPL(devm_cxl_add_passthrough_decoder, CXL); > static void parse_hdm_decoder_caps(struct cxl_hdm *cxlhdm) > { > u32 hdm_cap; > + int decoder_count; > + struct device *dev = &cxlhdm->port->dev; > > hdm_cap = readl(cxlhdm->regs.hdm_decoder + CXL_HDM_DECODER_CAP_OFFSET); > - cxlhdm->decoder_count = cxl_hdm_decoder_count(hdm_cap); > + decoder_count = cxl_hdm_decoder_count(hdm_cap); > + /* > + * CXL spec rev3.0 8.2.4.19.1 indicates CXL devices shall not advertise > + * more than 10 decoders. Switches and Host Bridges may advertise up to > + * 32 decoders. Set the decoders to 10 for devices if more than 10 are > + * found. > + */ > + if (is_cxl_endpoint(cxlhdm->port) && decoder_count > 10) { > + dev_warn(dev, "Reset decoders count (%d) to 10, spec violation!\n", > + decoder_count); > + decoder_count = 10; I agree with the reporting, but not the limitation. The Robustness Principle says if allowing it does no harm, then let it through.
On 10/21/2022 2:17 PM, Dan Williams wrote: > Dave Jiang wrote: >> CXL spec rev3.0 8.2.4.19.1 added definition for up to 32 decoders. It also >> indicates that for devices, only 10 decoders should be advertised. Add >> check on number of decoders greater than 10 for devices and reset to 10 to >> force spec compliance. >> >> Signed-off-by: Dave Jiang <dave.jiang@intel.com> >> --- >> drivers/cxl/core/hdm.c | 16 +++++++++++++++- >> 1 file changed, 15 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c >> index d1d2caea5c62..1919d99d157e 100644 >> --- a/drivers/cxl/core/hdm.c >> +++ b/drivers/cxl/core/hdm.c >> @@ -71,9 +71,23 @@ EXPORT_SYMBOL_NS_GPL(devm_cxl_add_passthrough_decoder, CXL); >> static void parse_hdm_decoder_caps(struct cxl_hdm *cxlhdm) >> { >> u32 hdm_cap; >> + int decoder_count; >> + struct device *dev = &cxlhdm->port->dev; >> >> hdm_cap = readl(cxlhdm->regs.hdm_decoder + CXL_HDM_DECODER_CAP_OFFSET); >> - cxlhdm->decoder_count = cxl_hdm_decoder_count(hdm_cap); >> + decoder_count = cxl_hdm_decoder_count(hdm_cap); >> + /* >> + * CXL spec rev3.0 8.2.4.19.1 indicates CXL devices shall not advertise >> + * more than 10 decoders. Switches and Host Bridges may advertise up to >> + * 32 decoders. Set the decoders to 10 for devices if more than 10 are >> + * found. >> + */ >> + if (is_cxl_endpoint(cxlhdm->port) && decoder_count > 10) { >> + dev_warn(dev, "Reset decoders count (%d) to 10, spec violation!\n", >> + decoder_count); >> + decoder_count = 10; > I agree with the reporting, but not the limitation. The Robustness > Principle says if allowing it does no harm, then let it through. Ok I'll remove the reassigment.
diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c index d1d2caea5c62..1919d99d157e 100644 --- a/drivers/cxl/core/hdm.c +++ b/drivers/cxl/core/hdm.c @@ -71,9 +71,23 @@ EXPORT_SYMBOL_NS_GPL(devm_cxl_add_passthrough_decoder, CXL); static void parse_hdm_decoder_caps(struct cxl_hdm *cxlhdm) { u32 hdm_cap; + int decoder_count; + struct device *dev = &cxlhdm->port->dev; hdm_cap = readl(cxlhdm->regs.hdm_decoder + CXL_HDM_DECODER_CAP_OFFSET); - cxlhdm->decoder_count = cxl_hdm_decoder_count(hdm_cap); + decoder_count = cxl_hdm_decoder_count(hdm_cap); + /* + * CXL spec rev3.0 8.2.4.19.1 indicates CXL devices shall not advertise + * more than 10 decoders. Switches and Host Bridges may advertise up to + * 32 decoders. Set the decoders to 10 for devices if more than 10 are + * found. + */ + if (is_cxl_endpoint(cxlhdm->port) && decoder_count > 10) { + dev_warn(dev, "Reset decoders count (%d) to 10, spec violation!\n", + decoder_count); + decoder_count = 10; + } + cxlhdm->decoder_count = decoder_count; cxlhdm->target_count = FIELD_GET(CXL_HDM_DECODER_TARGET_COUNT_MASK, hdm_cap); if (FIELD_GET(CXL_HDM_DECODER_INTERLEAVE_11_8, hdm_cap))
CXL spec rev3.0 8.2.4.19.1 added definition for up to 32 decoders. It also indicates that for devices, only 10 decoders should be advertised. Add check on number of decoders greater than 10 for devices and reset to 10 to force spec compliance. Signed-off-by: Dave Jiang <dave.jiang@intel.com> --- drivers/cxl/core/hdm.c | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-)