[ndctl] libndctl: Fix buffer 'offset' calculation in do_cmd()
diff mbox series

Message ID 20200416185223.48486-1-vaibhav@linux.ibm.com
State New, archived
Headers show
Series
  • [ndctl] libndctl: Fix buffer 'offset' calculation in do_cmd()
Related show

Commit Message

Vaibhav Jain April 16, 2020, 6:52 p.m. UTC
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(-)

Comments

Ira Weiny April 16, 2020, 8:02 p.m. UTC | #1
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
Vaibhav Jain April 17, 2020, 6:51 a.m. UTC | #2
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

Patch
diff mbox series

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",