Thanks Vishal, I have addressed your review comments in v5 of the patch here at https://lore.kernel.org/nvdimm/[email protected]
~ Vaibhav "Verma, Vishal L" <[email protected]> writes: > On Sat, 2022-02-19 at 18:09 +0530, Vaibhav Jain wrote: >> >> > > @@ -1924,9 +1940,13 @@ static void *add_dimm(void *parent, int id, const >> > > char *dimm_base) >> > > dimm->formats = formats; >> > > /* Check if the given dimm supports nfit */ >> > > if (ndctl_bus_has_nfit(bus)) { >> > > - rc = populate_dimm_attributes(dimm, dimm_base, "nfit"); >> > > + dimm->bus_prefix = strdup("nfit"); >> > > + rc = dimm->bus_prefix ? >> > > + populate_dimm_attributes(dimm, dimm_base) : >> > > -ENOMEM >> > > } else if (ndctl_bus_has_of_node(bus)) { >> > > - rc = add_papr_dimm(dimm, dimm_base); >> > > + dimm->bus_prefix = strdup("papr"); >> > > + rc = dimm->bus_prefix ? >> > > + add_papr_dimm(dimm, dimm_base) : -ENOMEM; >> > >> > For both of the above, it would be a bit more readable to just return >> > ENOMEM directly after strdup() if it fails, and then carry on with >> > add_<foo>_dimm(). >> > >> > dimm->bus_prefix = strdup("papr"); >> > if (!dimm->bus_prefix) >> > return -ENOMEM; >> > rc = add_papr_dimm(dimm, dimm_base); >> > ... >> > >> Agree on the readability part but returning from there right away would >> prevent the allocated 'struct ndctl_dimm *dimm' from being freed in the >> error path. Also the function add_dimm() returns a 'void *' to 'struct >> ndctl_dimm*' right now rather than an 'int'. >> >> I propose updating the code as: >> >> if (ndctl_bus_has_nfit(bus)) { >> dimm->bus_prefix = strdup("nfit"); >> if (!dimm->bus_prefix) { >> rc = -ENOMEM; >> goto out; >> } >> rc = populate_dimm_attributes(dimm, dimm_base); >> } > > Yes, that looks good, thanks! > >> >> > > } >> > > >> > > if (rc == -ENODEV) { >> > > @@ -3506,6 +3526,10 @@ NDCTL_EXPORT int ndctl_cmd_submit(struct >> > > ndctl_cmd *cmd) >> > > rc = -ENXIO; >> > > } >> > > close(fd); >> > > + >> > > + /* update dimm-flags if command submitted successfully */ >> > > + if (!rc && cmd->dimm) >> > > + ndctl_refresh_dimm_flags(cmd->dimm); >> > > out: >> > > cmd->status = rc; >> > > return rc; >> > > diff --git a/ndctl/lib/private.h b/ndctl/lib/private.h >> > > index 4d8622978790..e5c56295556d 100644 >> > > --- a/ndctl/lib/private.h >> > > +++ b/ndctl/lib/private.h >> > > @@ -75,6 +75,7 @@ struct ndctl_dimm { >> > > char *unique_id; >> > > char *dimm_path; >> > > char *dimm_buf; >> > > + char *bus_prefix; >> > > int health_eventfd; >> > > int buf_len; >> > > int id; >> > > diff --git a/ndctl/libndctl.h b/ndctl/libndctl.h >> > > index 4d5cdbf6f619..b1bafd6d9788 100644 >> > > --- a/ndctl/libndctl.h >> > > +++ b/ndctl/libndctl.h >> > > @@ -223,6 +223,7 @@ int ndctl_dimm_is_active(struct ndctl_dimm *dimm); >> > > int ndctl_dimm_is_enabled(struct ndctl_dimm *dimm); >> > > int ndctl_dimm_disable(struct ndctl_dimm *dimm); >> > > int ndctl_dimm_enable(struct ndctl_dimm *dimm); >> > > +void ndctl_refresh_dimm_flags(struct ndctl_dimm *dimm); >> > > >> > > struct ndctl_cmd; >> > > struct ndctl_cmd *ndctl_bus_cmd_new_ars_cap(struct ndctl_bus *bus, >> > >> > -- Cheers ~ Vaibhav
