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]

Reply via email to