Fan Ni wrote:
> On Fri, Aug 16, 2024 at 02:45:47PM -0700, Dave Jiang wrote:
[snip]
> > > +
> > > +/**
> > > + * cxl_dev_dynamic_capacity_identify() - Reads the dynamic capacity
> > > + * information from the device.
> > > + * @mds: The memory device state
> > > + *
> > > + * Read Dynamic Capacity information from the device and populate the state
> > > + * structures for later use.
> > > + *
> > > + * Return: 0 if identify was executed successfully, -ERRNO on error.
> > > + */
> > > +int cxl_dev_dynamic_capacity_identify(struct cxl_memdev_state *mds)
> > > +{
> > > + size_t dc_resp_size = mds->payload_size;
> > > + struct device *dev = mds->cxlds.dev;
> > > + u8 start_region, i;
> > > +
> > > + for (i = 0; i < CXL_MAX_DC_REGION; i++)
> > > + snprintf(mds->dc_region[i].name, CXL_DC_REGION_STRLEN, "<nil>");
> > > +
> > > + if (!cxl_dcd_supported(mds)) {
> > > + dev_dbg(dev, "DCD not supported\n");
> > > + return 0;
> > > + }
> >
> > This should happen before you pre-format the name string? I would assume that if DCD is not supported then the dcd name sysfs attribs would be not be visible?
> >
No this string is not used for sysfs. It is used to label the dpa
resources... That said in review I don't recall why it was necessary to
add the '<nil>' to them by default. I'm actually going to remove that and
continue testing and if I recall where this was showing up I might add it
back in.
> > > +
> > > + struct cxl_mbox_get_dc_config_out *dc_resp __free(kfree) =
> > > + kvmalloc(dc_resp_size, GFP_KERNEL);
> > > + if (!dc_resp)
> > > + return -ENOMEM;
> > > +
> > > + start_region = 0;
> > > + do {
> > > + int rc, j;
> > > +
> > > + rc = cxl_get_dc_config(mds, start_region, dc_resp, dc_resp_size);
> > > + if (rc < 0) {
> > > + dev_dbg(dev, "Failed to get DC config: %d\n", rc);
> > > + return rc;
> > > + }
> > > +
> > > + mds->nr_dc_region += rc;
> > > +
> > > + if (mds->nr_dc_region < 1 || mds->nr_dc_region > CXL_MAX_DC_REGION) {
> > > + dev_err(dev, "Invalid num of dynamic capacity regions %d\n",
> > > + mds->nr_dc_region);
> > > + return -EINVAL;
> > > + }
> > > +
> > > + for (i = start_region, j = 0; i < mds->nr_dc_region; i++, j++) {
> >
> > This should be 'j < mds->nr_dc_region'? Otherwise if your start region say is '3' and you have '2' DC regions, you never enter the loop. Or does that not happen? I also wonder if you need to check if 'start_region + mds->nr_dc_region > CXL_MAX_DC_REGION'.
> >
> That can not happen, start_region was updated to the number of regions
> has returned till now (not counting the current call), while
> nr_dc_region is the total number of regions returned till now (including
> the current call) as we update it above, so start_region should never be larger
> than nr_dc_region.
Yep.
>
> > > + rc = cxl_dc_save_region_info(mds, i, &dc_resp->region[j]);
> > > + if (rc) {
> > > + dev_dbg(dev, "Failed to save region info: %d\n", rc);
>
> I am not sure why we sometimes use dev_err and sometimes we use dev_dbg
> here, if dcd is supported, error from getting dc configuration is an
> error to me.
We are trying to reduce the dev_err() use. cxl_dc_save_region_info() has
dev_err() which is much more specific as to the error. At worse this is
just redundant as a debug.
I'll remove it because the debug output is pretty verbose too.
Ira
>
> Fan
[snip]
