| Message ID | 20200416185223.48486-1-vaibhav@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [ndctl] libndctl: Fix buffer 'offset' calculation in do_cmd() | expand |
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 <vaibhav@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-leave@lists.01.org
Hi, Thanks for reviewing this patch. My responses below: Ira Weiny <ira.weiny@intel.com> writes: > 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. The assumption to increment the buffer 'offset' by max_xfer instead of *(cmd->iter.xfer) or get_xfer() predates the commit a2fd7017b72d113ff7dfcf1b92b6a0a4de34c8b3 > > 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. Agree that this condition is presently hit only at the final iteration of the loop. But as the patch description points out that this differs from the assumption made in kernel function nvdimm_get_config_data() which doesnt assume that max_xfer bytes will be transffered at each iteration. Since the label area is stored separately from the nvdimm (in many cases accessible only via slow i2c), a nvdimm/bus providers (like papr_scm) may want to return xfer < max_xfer bytes for ND_CMD_GET_CONFIG_DATA command without blocking. nvdimm_get_config_data() provides this flexibility but do_cmd() doesnt. Infact if a bus provider does such a thing then do_cmd() will skip over the buffer range that was never trasffered to/from kernel. > > That said should set_xfer() ever do something more clever using get_xfer() is > cleaner. > >> >> Signed-off-by: Vaibhav Jain <vaibhav@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. Sure will rework the for loop. Was just trying to avoid an ugly looking for loop that didnt have an iteration-expression. > >> 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? Thanks for catching this. Will get it fixed. > > 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-leave@lists.01.org > _______________________________________________ > Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org > To unsubscribe send an email to linux-nvdimm-leave@lists.01.org
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) { 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; } dbg(ctx, "bus: %d dimm: %#x cmd: %s%s%s total: %d max_xfer: %d status: %d fw: %d (%s)\n",
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. Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com> --- ndctl/lib/libndctl.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)