On Fri, Apr 17, 2020 at 12:22:23AM +0530, Vaibhav Jain wrote:
> The 'for' loop in do_cmd() that generates multiple ioctls to
> libnvdimm assumes that each ioctl will result in transfer of
> 'iter->max_xfer' bytes. Hence after each successful iteration the
> buffer 'offset' is incremented by 'iter->max_xfer'.
>
> This is in contrast to similar implementation in libnvdimm kernel
> function nvdimm_get_config_data() which increments the buffer offset
> by 'cmd->in_length' thats the actual number of bytes transferred from
> the dimm/bus control function.
>
> The implementation in kernel function nvdimm_get_config_data() is more
> accurate compared to one in do_cmd() as former doesn't assume that
> config/label-area data size is in multiples of 'iter->max_xfer'.
>
> Hence the patch updates do_cmd() to increment the buffer 'offset'
> variable by 'cmd->get_xfer()' rather than 'iter->max_xfer' after each
> iteration.
This commit message is not correct. The problem is that commit
a2fd7017b72d113ff7dfcf1b92b6a0a4de34c8b3 introduced the concept of
{get,set}_xfer() and did not change the loop iterator to match.
Having the loop iterator match is not 100% required here as ->set_xfer() will
only change alter the cmd length written on the final command when the loop is
exiting anyway.
That said should set_xfer() ever do something more clever using get_xfer() is
cleaner.
>
> Signed-off-by: Vaibhav Jain <vaib...@linux.ibm.com>
> ---
> ndctl/lib/libndctl.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/ndctl/lib/libndctl.c b/ndctl/lib/libndctl.c
> index cda17ff32410..24fa67f0ccd0 100644
> --- a/ndctl/lib/libndctl.c
> +++ b/ndctl/lib/libndctl.c
> @@ -3089,7 +3089,7 @@ NDCTL_EXPORT int ndctl_cmd_xlat_firmware_status(struct
> ndctl_cmd *cmd)
> static int do_cmd(int fd, int ioctl_cmd, struct ndctl_cmd *cmd)
> {
> int rc;
> - u32 offset;
> + u32 offset = 0;
> const char *name, *sub_name = NULL;
> struct ndctl_dimm *dimm = cmd->dimm;
> struct ndctl_bus *bus = cmd_to_bus(cmd);
> @@ -3116,7 +3116,7 @@ static int do_cmd(int fd, int ioctl_cmd, struct
> ndctl_cmd *cmd)
> return rc;
> }
>
> - for (offset = 0; offset < iter->total_xfer; offset += iter->max_xfer) {
> + while (offset < iter->total_xfer) {
FWIW I prefer for loops if possible.
> cmd->set_xfer(cmd, min(iter->total_xfer - offset,
> iter->max_xfer));
> cmd->set_offset(cmd, offset);
> @@ -3136,6 +3136,8 @@ static int do_cmd(int fd, int ioctl_cmd, struct
> ndctl_cmd *cmd)
> rc = offset + cmd->get_xfer(cmd) - rc;
> break;
> }
> +
> + offset += cmd->get_xfer(cmd) - rc;
Why "- rc"? Doesn't this break WRITE?
Ira
> }
>
> dbg(ctx, "bus: %d dimm: %#x cmd: %s%s%s total: %d max_xfer: %d status:
> %d fw: %d (%s)\n",
> --
> 2.25.2
> _______________________________________________
> Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
> To unsubscribe send an email to linux-nvdimm-le...@lists.01.org
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-le...@lists.01.org
