| Message ID | 20250227103251.390147-1-ming.li@zohomail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v1,1/1] cxl/hdm: Verify HDM decoder capabilities after parsing | expand |
On 2/27/25 3:32 AM, Li Ming wrote: > devm_cxl_setup_hdm() only checks if decoder_count is 0 after parsing HDM > decoder capability, But according to the implementation of > cxl_hdm_decoder_count(), cxlhdm->decoder_count will never be 0. > > Per CXL specification, the values ranges of decoder_count and > target_count are limited. Adding a checking for the values of them > in case hardware initialized them with wrong values. > > Signed-off-by: Li Ming <ming.li@zohomail.com> > --- > base-commit: 22eea823f69ae39dc060c4027e8d1470803d2e49 cxl/next > --- > drivers/cxl/core/hdm.c | 31 ++++++++++++++++++++++++++++++- > 1 file changed, 30 insertions(+), 1 deletion(-) > > diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c > index 70cae4ebf8a4..a98191867c22 100644 > --- a/drivers/cxl/core/hdm.c > +++ b/drivers/cxl/core/hdm.c > @@ -138,6 +138,34 @@ static bool should_emulate_decoders(struct cxl_endpoint_dvsec_info *info) > return true; > } > > +static int cxlhdm_decoder_caps_verify(struct cxl_hdm *cxlhdm) > +{ > + /* > + * CXL r3.2 section 8.2.4.20.1 > + * CXL devices shall not advertise more than 10 decoders, > + * CXL switches and HBs may advertise up to 32 decoders. > + */ > + if (is_cxl_endpoint(cxlhdm->port) && cxlhdm->decoder_count > 10) #define the limit please > + return -EINVAL; > + else if (cxlhdm->decoder_count > 32) same here DJ > + return -EINVAL; > + > + /* > + * CXL r3.2 section 8.2.4.20.1 > + * target count is applicable only to CXL upstream port and HB. > + * The number of target ports each decoder supports should be > + * one of the numbers 1, 2, 4 or 8. > + */ > + if (!is_cxl_endpoint(cxlhdm->port) && > + cxlhdm->target_count != 1 && > + cxlhdm->target_count != 2 && > + cxlhdm->target_count != 4 && > + cxlhdm->target_count != 8) > + return -EINVAL; > + > + return 0; > +} > + > /** > * devm_cxl_setup_hdm - map HDM decoder component registers > * @port: cxl_port to map > @@ -182,7 +210,8 @@ struct cxl_hdm *devm_cxl_setup_hdm(struct cxl_port *port, > } > > parse_hdm_decoder_caps(cxlhdm); > - if (cxlhdm->decoder_count == 0) { > + rc = cxlhdm_decoder_caps_verify(cxlhdm); > + if (rc) { > dev_err(dev, "Spec violation. Caps invalid\n"); > return ERR_PTR(-ENXIO); > }
On Thu, Feb 27, 2025 at 06:32:51PM +0800, Li Ming wrote: > devm_cxl_setup_hdm() only checks if decoder_count is 0 after parsing HDM > decoder capability, But according to the implementation of > cxl_hdm_decoder_count(), cxlhdm->decoder_count will never be 0. How does a check against the spec maximums benefit this driver? Is there a bad path we avoid by checking and quitting at this point. Might this catch silly decoder counts that the driver previously ignored? > > Per CXL specification, the values ranges of decoder_count and > target_count are limited. Adding a checking for the values of them > in case hardware initialized them with wrong values. Similar question - is this catching something sooner, rather than later? > > Signed-off-by: Li Ming <ming.li@zohomail.com> > --- > base-commit: 22eea823f69ae39dc060c4027e8d1470803d2e49 cxl/next > --- > drivers/cxl/core/hdm.c | 31 ++++++++++++++++++++++++++++++- > 1 file changed, 30 insertions(+), 1 deletion(-) > > diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c > index 70cae4ebf8a4..a98191867c22 100644 > --- a/drivers/cxl/core/hdm.c > +++ b/drivers/cxl/core/hdm.c > @@ -138,6 +138,34 @@ static bool should_emulate_decoders(struct cxl_endpoint_dvsec_info *info) > return true; > } > > +static int cxlhdm_decoder_caps_verify(struct cxl_hdm *cxlhdm) > +{ > + /* > + * CXL r3.2 section 8.2.4.20.1 > + * CXL devices shall not advertise more than 10 decoders, > + * CXL switches and HBs may advertise up to 32 decoders. > + */ > + if (is_cxl_endpoint(cxlhdm->port) && cxlhdm->decoder_count > 10) > + return -EINVAL; > + else if (cxlhdm->decoder_count > 32) > + return -EINVAL; > + > + /* > + * CXL r3.2 section 8.2.4.20.1 > + * target count is applicable only to CXL upstream port and HB. > + * The number of target ports each decoder supports should be > + * one of the numbers 1, 2, 4 or 8. > + */ > + if (!is_cxl_endpoint(cxlhdm->port) && > + cxlhdm->target_count != 1 && > + cxlhdm->target_count != 2 && > + cxlhdm->target_count != 4 && > + cxlhdm->target_count != 8) > + return -EINVAL; Maybe instead of manual bitwise checks try (!is_power_of_2(cxlhdm->target_count) || cxlhdm->target_count > 8)) > + > + return 0; > +} > + > /** > * devm_cxl_setup_hdm - map HDM decoder component registers > * @port: cxl_port to map > @@ -182,7 +210,8 @@ struct cxl_hdm *devm_cxl_setup_hdm(struct cxl_port *port, > } > > parse_hdm_decoder_caps(cxlhdm); > - if (cxlhdm->decoder_count == 0) { > + rc = cxlhdm_decoder_caps_verify(cxlhdm); > + if (rc) { > dev_err(dev, "Spec violation. Caps invalid\n"); Can you move the dev_err to the verify function and include the specific invalid capability. --Alison > return ERR_PTR(-ENXIO); > } > -- > 2.34.1 >
On 2/28/2025 5:47 AM, Alison Schofield wrote: > On Thu, Feb 27, 2025 at 06:32:51PM +0800, Li Ming wrote: >> devm_cxl_setup_hdm() only checks if decoder_count is 0 after parsing HDM >> decoder capability, But according to the implementation of >> cxl_hdm_decoder_count(), cxlhdm->decoder_count will never be 0. > How does a check against the spec maximums benefit this driver? Is there > a bad path we avoid by checking and quitting at this point. My understanding is that no a bad path on driver side if the decoder_count is greater than the maximum number spec defines. Driver just allocates cxl decoders on the port based on the value of decoder_count. But I am not sure if hardware will have other potential problems when it didn't follow the spec. > > Might this catch silly decoder counts that the driver previously > ignored? > >> Per CXL specification, the values ranges of decoder_count and >> target_count are limited. Adding a checking for the values of them >> in case hardware initialized them with wrong values. > Similar question - is this catching something sooner, rather than > later? Yes, the check is at the beginning of HDM setup during port probing, if value is wrong, will break HDM setup. I'm not sure if I fully understand your question, please correct me if I misunderstand it. thanks. > >> Signed-off-by: Li Ming <ming.li@zohomail.com> >> --- >> base-commit: 22eea823f69ae39dc060c4027e8d1470803d2e49 cxl/next >> --- >> drivers/cxl/core/hdm.c | 31 ++++++++++++++++++++++++++++++- >> 1 file changed, 30 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c >> index 70cae4ebf8a4..a98191867c22 100644 >> --- a/drivers/cxl/core/hdm.c >> +++ b/drivers/cxl/core/hdm.c >> @@ -138,6 +138,34 @@ static bool should_emulate_decoders(struct cxl_endpoint_dvsec_info *info) >> return true; >> } >> >> +static int cxlhdm_decoder_caps_verify(struct cxl_hdm *cxlhdm) >> +{ >> + /* >> + * CXL r3.2 section 8.2.4.20.1 >> + * CXL devices shall not advertise more than 10 decoders, >> + * CXL switches and HBs may advertise up to 32 decoders. >> + */ >> + if (is_cxl_endpoint(cxlhdm->port) && cxlhdm->decoder_count > 10) >> + return -EINVAL; >> + else if (cxlhdm->decoder_count > 32) >> + return -EINVAL; >> + >> + /* >> + * CXL r3.2 section 8.2.4.20.1 >> + * target count is applicable only to CXL upstream port and HB. >> + * The number of target ports each decoder supports should be >> + * one of the numbers 1, 2, 4 or 8. >> + */ >> + if (!is_cxl_endpoint(cxlhdm->port) && >> + cxlhdm->target_count != 1 && >> + cxlhdm->target_count != 2 && >> + cxlhdm->target_count != 4 && >> + cxlhdm->target_count != 8) >> + return -EINVAL; > Maybe instead of manual bitwise checks try > (!is_power_of_2(cxlhdm->target_count) || cxlhdm->target_count > 8)) Yes, It is clearer, thanks for that. > >> + >> + return 0; >> +} >> + >> /** >> * devm_cxl_setup_hdm - map HDM decoder component registers >> * @port: cxl_port to map >> @@ -182,7 +210,8 @@ struct cxl_hdm *devm_cxl_setup_hdm(struct cxl_port *port, >> } >> >> parse_hdm_decoder_caps(cxlhdm); >> - if (cxlhdm->decoder_count == 0) { >> + rc = cxlhdm_decoder_caps_verify(cxlhdm); >> + if (rc) { >> dev_err(dev, "Spec violation. Caps invalid\n"); > Can you move the dev_err to the verify function and include the > specific invalid capability. > > > --Alison Sure, will do that, thanks. Ming
On 2/27/2025 11:21 PM, Dave Jiang wrote: > > On 2/27/25 3:32 AM, Li Ming wrote: >> devm_cxl_setup_hdm() only checks if decoder_count is 0 after parsing HDM >> decoder capability, But according to the implementation of >> cxl_hdm_decoder_count(), cxlhdm->decoder_count will never be 0. >> >> Per CXL specification, the values ranges of decoder_count and >> target_count are limited. Adding a checking for the values of them >> in case hardware initialized them with wrong values. >> >> Signed-off-by: Li Ming <ming.li@zohomail.com> >> --- >> base-commit: 22eea823f69ae39dc060c4027e8d1470803d2e49 cxl/next >> --- >> drivers/cxl/core/hdm.c | 31 ++++++++++++++++++++++++++++++- >> 1 file changed, 30 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c >> index 70cae4ebf8a4..a98191867c22 100644 >> --- a/drivers/cxl/core/hdm.c >> +++ b/drivers/cxl/core/hdm.c >> @@ -138,6 +138,34 @@ static bool should_emulate_decoders(struct cxl_endpoint_dvsec_info *info) >> return true; >> } >> >> +static int cxlhdm_decoder_caps_verify(struct cxl_hdm *cxlhdm) >> +{ >> + /* >> + * CXL r3.2 section 8.2.4.20.1 >> + * CXL devices shall not advertise more than 10 decoders, >> + * CXL switches and HBs may advertise up to 32 decoders. >> + */ >> + if (is_cxl_endpoint(cxlhdm->port) && cxlhdm->decoder_count > 10) > #define the limit please Will do, thanks. > >> + return -EINVAL; >> + else if (cxlhdm->decoder_count > 32) > same here > > DJ Will do, thanks for review. Ming [snip]
On Fri, Feb 28, 2025 at 10:47:12AM +0800, Li Ming wrote: > On 2/28/2025 5:47 AM, Alison Schofield wrote: > > On Thu, Feb 27, 2025 at 06:32:51PM +0800, Li Ming wrote: > >> devm_cxl_setup_hdm() only checks if decoder_count is 0 after parsing HDM > >> decoder capability, But according to the implementation of > >> cxl_hdm_decoder_count(), cxlhdm->decoder_count will never be 0. > > How does a check against the spec maximums benefit this driver? Is there > > a bad path we avoid by checking and quitting at this point. > > > My understanding is that no a bad path on driver side if the decoder_count is greater than the maximum number spec defines. > > Driver just allocates cxl decoders on the port based on the value of decoder_count. But I am not sure if hardware will have other potential problems when it didn't follow the spec. I had the general thought that the driver is not responsible for compliance checking the device, unless it affects function. Excessive decoder_count's sound like they cause needless allocations, so let's stop doing that - as best we can. Is it sufficient to clamp at the spec defined max values and continue with only a dev_warn_once or even a dev_info? ie. for a device: decoder_count = min(decoder_count, EP_DECODER_MAX); That'll avoid failing a device that previously snuck by with an excessive decoder_count and protect against excessive allocations in the driver. > > > > > > Might this catch silly decoder counts that the driver previously > > ignored? > > > >> Per CXL specification, the values ranges of decoder_count and > >> target_count are limited. Adding a checking for the values of them > >> in case hardware initialized them with wrong values. > > Similar question - is this catching something sooner, rather than > > later? > > > Yes, the check is at the beginning of HDM setup during port probing, if value is wrong, will break HDM setup. > > I'm not sure if I fully understand your question, please correct me if I misunderstand it. thanks. I understand now. This one is different that decoder_count because it was heading to failure anyway and seems good to fail sooner. > > > > > >> Signed-off-by: Li Ming <ming.li@zohomail.com> > >> --- > >> base-commit: 22eea823f69ae39dc060c4027e8d1470803d2e49 cxl/next > >> --- > >> drivers/cxl/core/hdm.c | 31 ++++++++++++++++++++++++++++++- > >> 1 file changed, 30 insertions(+), 1 deletion(-) > >> > >> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c > >> index 70cae4ebf8a4..a98191867c22 100644 > >> --- a/drivers/cxl/core/hdm.c > >> +++ b/drivers/cxl/core/hdm.c > >> @@ -138,6 +138,34 @@ static bool should_emulate_decoders(struct cxl_endpoint_dvsec_info *info) > >> return true; > >> } > >> > >> +static int cxlhdm_decoder_caps_verify(struct cxl_hdm *cxlhdm) > >> +{ > >> + /* > >> + * CXL r3.2 section 8.2.4.20.1 > >> + * CXL devices shall not advertise more than 10 decoders, > >> + * CXL switches and HBs may advertise up to 32 decoders. > >> + */ > >> + if (is_cxl_endpoint(cxlhdm->port) && cxlhdm->decoder_count > 10) > >> + return -EINVAL; > >> + else if (cxlhdm->decoder_count > 32) > >> + return -EINVAL; > >> + > >> + /* > >> + * CXL r3.2 section 8.2.4.20.1 > >> + * target count is applicable only to CXL upstream port and HB. > >> + * The number of target ports each decoder supports should be > >> + * one of the numbers 1, 2, 4 or 8. > >> + */ > >> + if (!is_cxl_endpoint(cxlhdm->port) && > >> + cxlhdm->target_count != 1 && > >> + cxlhdm->target_count != 2 && > >> + cxlhdm->target_count != 4 && > >> + cxlhdm->target_count != 8) > >> + return -EINVAL; > > Maybe instead of manual bitwise checks try > > (!is_power_of_2(cxlhdm->target_count) || cxlhdm->target_count > 8)) > > > Yes, It is clearer, thanks for that. > > > > > >> + > >> + return 0; > >> +} > >> + > >> /** > >> * devm_cxl_setup_hdm - map HDM decoder component registers > >> * @port: cxl_port to map > >> @@ -182,7 +210,8 @@ struct cxl_hdm *devm_cxl_setup_hdm(struct cxl_port *port, > >> } > >> > >> parse_hdm_decoder_caps(cxlhdm); > >> - if (cxlhdm->decoder_count == 0) { > >> + rc = cxlhdm_decoder_caps_verify(cxlhdm); > >> + if (rc) { > >> dev_err(dev, "Spec violation. Caps invalid\n"); > > Can you move the dev_err to the verify function and include the > > specific invalid capability. > > > > > > --Alison > > Sure, will do that, thanks. > > > Ming > >
Alison Schofield wrote: > On Fri, Feb 28, 2025 at 10:47:12AM +0800, Li Ming wrote: > > On 2/28/2025 5:47 AM, Alison Schofield wrote: > > > On Thu, Feb 27, 2025 at 06:32:51PM +0800, Li Ming wrote: > > >> devm_cxl_setup_hdm() only checks if decoder_count is 0 after parsing HDM > > >> decoder capability, But according to the implementation of > > >> cxl_hdm_decoder_count(), cxlhdm->decoder_count will never be 0. > > > How does a check against the spec maximums benefit this driver? Is there > > > a bad path we avoid by checking and quitting at this point. > > > > > > My understanding is that no a bad path on driver side if the decoder_count is greater than the maximum number spec defines. > > > > Driver just allocates cxl decoders on the port based on the value of decoder_count. But I am not sure if hardware will have other potential problems when it didn't follow the spec. > > I had the general thought that the driver is not responsible for > compliance checking the device, unless it affects function. Excessive > decoder_count's sound like they cause needless allocations, so let's > stop doing that - as best we can. Only if we see a device in the wild that causes an actual problem. Otherwise this is a losing theoretical game of adding checks for things that will likely never be violated. The way to address devices that violate spec expectations *and* cause end user visible pain is to add quirks. The allocation of a few extra decoders is does not amount to that standard. Lets not add checks for benign issues "just because", or "just in case". If the check is cheap and we need to do it for the driver's own internal sanity, fine, but if it's just being strict for strictness sake, please no.
On 3/1/2025 7:45 AM, Dan Williams wrote: > Alison Schofield wrote: >> On Fri, Feb 28, 2025 at 10:47:12AM +0800, Li Ming wrote: >>> On 2/28/2025 5:47 AM, Alison Schofield wrote: >>>> On Thu, Feb 27, 2025 at 06:32:51PM +0800, Li Ming wrote: >>>>> devm_cxl_setup_hdm() only checks if decoder_count is 0 after parsing HDM >>>>> decoder capability, But according to the implementation of >>>>> cxl_hdm_decoder_count(), cxlhdm->decoder_count will never be 0. >>>> How does a check against the spec maximums benefit this driver? Is there >>>> a bad path we avoid by checking and quitting at this point. >>> >>> My understanding is that no a bad path on driver side if the decoder_count is greater than the maximum number spec defines. >>> >>> Driver just allocates cxl decoders on the port based on the value of decoder_count. But I am not sure if hardware will have other potential problems when it didn't follow the spec. >> I had the general thought that the driver is not responsible for >> compliance checking the device, unless it affects function. Excessive >> decoder_count's sound like they cause needless allocations, so let's >> stop doing that - as best we can. > Only if we see a device in the wild that causes an actual problem. > Otherwise this is a losing theoretical game of adding checks for things > that will likely never be violated. The way to address devices that > violate spec expectations *and* cause end user visible pain is to add > quirks. The allocation of a few extra decoders is does not amount to > that standard. > > Lets not add checks for benign issues "just because", or "just in case". > If the check is cheap and we need to do it for the driver's own internal > sanity, fine, but if it's just being strict for strictness sake, please > no. Got it, thanks for explanation. Ming
diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c index 70cae4ebf8a4..a98191867c22 100644 --- a/drivers/cxl/core/hdm.c +++ b/drivers/cxl/core/hdm.c @@ -138,6 +138,34 @@ static bool should_emulate_decoders(struct cxl_endpoint_dvsec_info *info) return true; } +static int cxlhdm_decoder_caps_verify(struct cxl_hdm *cxlhdm) +{ + /* + * CXL r3.2 section 8.2.4.20.1 + * CXL devices shall not advertise more than 10 decoders, + * CXL switches and HBs may advertise up to 32 decoders. + */ + if (is_cxl_endpoint(cxlhdm->port) && cxlhdm->decoder_count > 10) + return -EINVAL; + else if (cxlhdm->decoder_count > 32) + return -EINVAL; + + /* + * CXL r3.2 section 8.2.4.20.1 + * target count is applicable only to CXL upstream port and HB. + * The number of target ports each decoder supports should be + * one of the numbers 1, 2, 4 or 8. + */ + if (!is_cxl_endpoint(cxlhdm->port) && + cxlhdm->target_count != 1 && + cxlhdm->target_count != 2 && + cxlhdm->target_count != 4 && + cxlhdm->target_count != 8) + return -EINVAL; + + return 0; +} + /** * devm_cxl_setup_hdm - map HDM decoder component registers * @port: cxl_port to map @@ -182,7 +210,8 @@ struct cxl_hdm *devm_cxl_setup_hdm(struct cxl_port *port, } parse_hdm_decoder_caps(cxlhdm); - if (cxlhdm->decoder_count == 0) { + rc = cxlhdm_decoder_caps_verify(cxlhdm); + if (rc) { dev_err(dev, "Spec violation. Caps invalid\n"); return ERR_PTR(-ENXIO); }
devm_cxl_setup_hdm() only checks if decoder_count is 0 after parsing HDM decoder capability, But according to the implementation of cxl_hdm_decoder_count(), cxlhdm->decoder_count will never be 0. Per CXL specification, the values ranges of decoder_count and target_count are limited. Adding a checking for the values of them in case hardware initialized them with wrong values. Signed-off-by: Li Ming <ming.li@zohomail.com> --- base-commit: 22eea823f69ae39dc060c4027e8d1470803d2e49 cxl/next --- drivers/cxl/core/hdm.c | 31 ++++++++++++++++++++++++++++++- 1 file changed, 30 insertions(+), 1 deletion(-)