On Wed, Nov 03, 2021 at 02:33:37PM +0530, Shivaprasad G Bhat wrote:
> Thanks for the comments Ira. Replies inline..
>
> On 10/28/21 10:15, Ira Weiny wrote:
>
> On Fri, Oct 22, 2021 at 09:57:07AM -0500, Shivaprasad G Bhat wrote:
>
> From: Vaibhav Jain [1]<vaib...@linux.ibm.com>
>
>
> [snip]
>
> < snip>
>
> ndctl_cmd_unref(si_cmd);
> return rc;
> @@ -415,8 +423,10 @@ static int dimm_inject_smart(struct ndctl_dimm *dimm)
> struct json_object *jhealth;
> struct json_object *jdimms;
> struct json_object *jdimm;
> + unsigned int supported_types;
> int rc;
>
> + /* Get supported smart injection types */
>
> NIT: this comment is probably unnecessary.
>
> The code changes definition of the return value hence wanted to
>
> clarify that, it indicates a set of flags instead of a boolean.
>
> From that pov, the comment helps avoiding the confusion.
>
> Please let me know If you think the otherway.
But that change is documented in the change log. When reading the code after
the change the function is pretty self documenting.
And if you want to document the call, that should be done with the definition
in the *.c file. Not at the call sites.
Ira
>
> rc = ndctl_dimm_smart_inject_supported(dimm);
> switch (rc) {
> case -ENOTTY:
> @@ -431,6 +441,15 @@ static int dimm_inject_smart(struct ndctl_dimm *dimm)
> error("%s: smart injection not supported by either platform
> firmware or the kernel.",
> ndctl_dimm_get_devname(dimm));
> return rc;
> + default:
> + if (rc < 0) {
> + error("%s: Unknown error %d while checking for smart
> injection support",
> + ndctl_dimm_get_devname(dimm), rc);
> + return rc;
> + }
> + /* Assigning to an unsigned type since rc < 0 */
>
> Comment wrong?
>
> Will get rid of it.
>
> + supported_types = rc;
> + break;
> }
>
> if (sctx.op_mask & (1 << OP_SET)) {
>
> [snip]
>
> Other than the comment it looks fine.
>
> Reviewed-by: Ira Weiny [2]<ira.we...@intel.com>
>
>
> Thanks!
>
> -Shivaprasad
>
> References
>
> Visible links
> 1. mailto:vaib...@linux.ibm.com
> 2. mailto:ira.we...@intel.com
