[PATCH 0/5] nvme-fabrics: small cleanup

[PATCH 0/5] nvme-fabrics: small cleanup

[Chaitanya Kulkarni]

From: Chaitanya Kulkarni <kch@nvidia.com>

Hi,

This has small cleaups and a fix suggested by Sagi for
nvmf_dev_show().

-ck

Chaitanya Kulkarni (5):
  nvme-fabrics: use consistent zeroout pattern
  nvme-fabrics: remove unnecessary braces for case
  nvme-fabrics: use unsigned int type
  nvme-fabrics: use unsigned int type
  nvme-fabrics: set ret to -ENODEV for error case

 drivers/nvme/host/fabrics.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

-- 
2.29.0

[PATCH 1/5] nvme-fabrics: use consistent zeroout pattern

[Chaitanya Kulkarni]

From: Chaitanya Kulkarni <kch@nvidia.com>

Remove zeroout memeset call & zeroout local variable cmd at the time
of declaration in nvmf_ref_read32() similar to what we have done in
nvmf_reg_read64(), nvmf_reg_write32(), nvmf_connect_admin_queue(), and
nvmf_connect_io_queue().

Signed-off-by: Chaitanya Kulkarni <kch@nvidia.com>
---
 drivers/nvme/host/fabrics.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c
index 7ae041e2b3fb..a0640986add9 100644
--- a/drivers/nvme/host/fabrics.c
+++ b/drivers/nvme/host/fabrics.c
@@ -144,11 +144,10 @@ EXPORT_SYMBOL_GPL(nvmf_get_address);
  */
 int nvmf_reg_read32(struct nvme_ctrl *ctrl, u32 off, u32 *val)
 {
-	struct nvme_command cmd;
+	struct nvme_command cmd = { };
 	union nvme_result res;
 	int ret;
 
-	memset(&cmd, 0, sizeof(cmd));
 	cmd.prop_get.opcode = nvme_fabrics_command;
 	cmd.prop_get.fctype = nvme_fabrics_type_property_get;
 	cmd.prop_get.offset = cpu_to_le32(off);
-- 
2.29.0

[PATCH 2/5] nvme-fabrics: remove unnecessary braces for case

[Chaitanya Kulkarni]

From: Chaitanya Kulkarni <kch@nvidia.com>

Braces are not required for enum value NVME_SC_CONNECT_INVALID_PARAM
when used on the switch-case statement, remove the braces.

Signed-off-by: Chaitanya Kulkarni <kch@nvidia.com>
---
 drivers/nvme/host/fabrics.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c
index a0640986add9..d38a630ce3dc 100644
--- a/drivers/nvme/host/fabrics.c
+++ b/drivers/nvme/host/fabrics.c
@@ -271,7 +271,7 @@ static void nvmf_log_connect_error(struct nvme_ctrl *ctrl,
 	int err_sctype = errval & ~NVME_SC_DNR;
 
 	switch (err_sctype) {
-	case (NVME_SC_CONNECT_INVALID_PARAM):
+	case NVME_SC_CONNECT_INVALID_PARAM:
 		if (offset >> 16) {
 			char *inv_data = "Connect Invalid Data Parameter";
 
-- 
2.29.0

[PATCH 3/5] nvme-fabrics: use unsigned int type

[Chaitanya Kulkarni]

From: Chaitanya Kulkarni <kch@nvidia.com>

Loop variable i will never have a negative value, so use
unsigned int type instaed of int.

Signed-off-by: Chaitanya Kulkarni <kch@nvidia.com>
---
 drivers/nvme/host/fabrics.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c
index d38a630ce3dc..90b3474ea276 100644
--- a/drivers/nvme/host/fabrics.c
+++ b/drivers/nvme/host/fabrics.c
@@ -872,7 +872,7 @@ static int nvmf_check_required_opts(struct nvmf_ctrl_options *opts,
 		unsigned int required_opts)
 {
 	if ((opts->mask & required_opts) != required_opts) {
-		int i;
+		unsigned int i;
 
 		for (i = 0; i < ARRAY_SIZE(opt_tokens); i++) {
 			if ((opt_tokens[i].token & required_opts) &&
-- 
2.29.0

[PATCH 4/5] nvme-fabrics: use unsigned int type

[Chaitanya Kulkarni]

From: Chaitanya Kulkarni <kch@nvidia.com>

Loop variable i will never have a negative value, so use
unsigned int type instaed of int.

Signed-off-by: Chaitanya Kulkarni <kch@nvidia.com>
---
 drivers/nvme/host/fabrics.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c
index 90b3474ea276..df0326db306f 100644
--- a/drivers/nvme/host/fabrics.c
+++ b/drivers/nvme/host/fabrics.c
@@ -922,7 +922,7 @@ static int nvmf_check_allowed_opts(struct nvmf_ctrl_options *opts,
 		unsigned int allowed_opts)
 {
 	if (opts->mask & ~allowed_opts) {
-		int i;
+		unsigned int i;
 
 		for (i = 0; i < ARRAY_SIZE(opt_tokens); i++) {
 			if ((opt_tokens[i].token & opts->mask) &&
-- 
2.29.0

[PATCH 5/5] nvme-fabrics: set ret to -ENODEV for error case

[Chaitanya Kulkarni]

From: Chaitanya Kulkarni <kch@nvidia.com>

In function nvmf_dev_show() if we don't find the controller set the
ret variable to -ENODEV.

Signed-off-by: Chaitanya Kulkarni <kch@nvidia.com>
Suggested-by: Sagi Grimberg <sagi@grimberg.me>
---
 drivers/nvme/host/fabrics.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c
index df0326db306f..ca23ac926710 100644
--- a/drivers/nvme/host/fabrics.c
+++ b/drivers/nvme/host/fabrics.c
@@ -1097,6 +1097,7 @@ static int nvmf_dev_show(struct seq_file *seq_file, void *private)
 	ctrl = seq_file->private;
 	if (!ctrl) {
 		__nvmf_concat_opt_tokens(seq_file);
+		ret = -ENODEV;
 		goto out_unlock;
 	}
 
-- 
2.29.0

Re: [PATCH 3/5] nvme-fabrics: use unsigned int type

[Max Gurtovoy]

On 1/12/2022 8:20 AM, Chaitanya Kulkarni wrote:
> From: Chaitanya Kulkarni <kch@nvidia.com>
>
> Loop variable i will never have a negative value, so use
> unsigned int type instaed of int.
>
> Signed-off-by: Chaitanya Kulkarni <kch@nvidia.com>
> ---
>   drivers/nvme/host/fabrics.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c
> index d38a630ce3dc..90b3474ea276 100644
> --- a/drivers/nvme/host/fabrics.c
> +++ b/drivers/nvme/host/fabrics.c
> @@ -872,7 +872,7 @@ static int nvmf_check_required_opts(struct nvmf_ctrl_options *opts,
>   		unsigned int required_opts)
>   {
>   	if ((opts->mask & required_opts) != required_opts) {
> -		int i;
> +		unsigned int i;
>   
>   		for (i = 0; i < ARRAY_SIZE(opt_tokens); i++) {
>   			if ((opt_tokens[i].token & required_opts) &&

I'm not against this but this means that we should replace the entire 
kernel that uses int inside a for loop.

If we decide to go with this approach - patch 3/5 and 4/5 should be 
squashed.

Re: [PATCH 1/5] nvme-fabrics: use consistent zeroout pattern

[Keith Busch]

On Tue, Jan 11, 2022 at 10:20:57PM -0800, Chaitanya Kulkarni wrote:
> From: Chaitanya Kulkarni <kch@nvidia.com>
> 
> Remove zeroout memeset call & zeroout local variable cmd at the time
> of declaration in nvmf_ref_read32() similar to what we have done in
> nvmf_reg_read64(), nvmf_reg_write32(), nvmf_connect_admin_queue(), and
> nvmf_connect_io_queue().
> 
> Signed-off-by: Chaitanya Kulkarni <kch@nvidia.com>

Looks good.

Reviewed-by: Keith Busch <kbusch@kernel.org>

Re: [PATCH 2/5] nvme-fabrics: remove unnecessary braces for case

[Keith Busch]

On Tue, Jan 11, 2022 at 10:20:58PM -0800, Chaitanya Kulkarni wrote:
> From: Chaitanya Kulkarni <kch@nvidia.com>
> 
> Braces are not required for enum value NVME_SC_CONNECT_INVALID_PARAM
> when used on the switch-case statement, remove the braces.
> 
> Signed-off-by: Chaitanya Kulkarni <kch@nvidia.com>

Looks good.

Reviewed-by: Keith Busch <kbusch@kernel.org>

Re: [PATCH 3/5] nvme-fabrics: use unsigned int type

[Keith Busch]

On Tue, Jan 11, 2022 at 10:20:59PM -0800, Chaitanya Kulkarni wrote:
> From: Chaitanya Kulkarni <kch@nvidia.com>
> 
> Loop variable i will never have a negative value, so use
> unsigned int type instaed of int.
> 
> Signed-off-by: Chaitanya Kulkarni <kch@nvidia.com>

Just IMO, I do not particularly care for this kind of change. The driver
and kernel use 'int i' for loop control in so many places. Granted,
'unsigned i' is also used too...

Re: [PATCH 5/5] nvme-fabrics: set ret to -ENODEV for error case

[Keith Busch]

On Tue, Jan 11, 2022 at 10:21:01PM -0800, Chaitanya Kulkarni wrote:
> From: Chaitanya Kulkarni <kch@nvidia.com>
> 
> In function nvmf_dev_show() if we don't find the controller set the
> ret variable to -ENODEV.
> 
> Signed-off-by: Chaitanya Kulkarni <kch@nvidia.com>
> Suggested-by: Sagi Grimberg <sagi@grimberg.me>
> ---
>  drivers/nvme/host/fabrics.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c
> index df0326db306f..ca23ac926710 100644
> --- a/drivers/nvme/host/fabrics.c
> +++ b/drivers/nvme/host/fabrics.c
> @@ -1097,6 +1097,7 @@ static int nvmf_dev_show(struct seq_file *seq_file, void *private)
>  	ctrl = seq_file->private;
>  	if (!ctrl) {
>  		__nvmf_concat_opt_tokens(seq_file);
> +		ret = -ENODEV;
>  		goto out_unlock;
>  	}

I missed the discussion on this. Doesn't the __nvmf_concat_opt_tokens()
already return parameters that indicate to the caller that no controller
was found?

Re: [PATCH 5/5] nvme-fabrics: set ret to -ENODEV for error case

[Chaitanya Kulkarni]

Keith,

On 1/13/22 08:28, Keith Busch wrote:
> External email: Use caution opening links or attachments
> 
> 
> On Tue, Jan 11, 2022 at 10:21:01PM -0800, Chaitanya Kulkarni wrote:
>> From: Chaitanya Kulkarni <kch@nvidia.com>
>>
>> In function nvmf_dev_show() if we don't find the controller set the
>> ret variable to -ENODEV.
>>
>> Signed-off-by: Chaitanya Kulkarni <kch@nvidia.com>
>> Suggested-by: Sagi Grimberg <sagi@grimberg.me>
>> ---
>>   drivers/nvme/host/fabrics.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c
>> index df0326db306f..ca23ac926710 100644
>> --- a/drivers/nvme/host/fabrics.c
>> +++ b/drivers/nvme/host/fabrics.c
>> @@ -1097,6 +1097,7 @@ static int nvmf_dev_show(struct seq_file *seq_file, void *private)
>>        ctrl = seq_file->private;
>>        if (!ctrl) {
>>                __nvmf_concat_opt_tokens(seq_file);
>> +             ret = -ENODEV;
>>                goto out_unlock;
>>        }
> 
> I missed the discussion on this. Doesn't the __nvmf_concat_opt_tokens()
> already return parameters that indicate to the caller that no controller
> was found?
> 

The function I found in the repo returns nothing, is that the one you
are referring to ?

static void __nvmf_concat_opt_tokens(struct seq_file *seq_file)

-ck

Re: [PATCH 5/5] nvme-fabrics: set ret to -ENODEV for error case

[Sagi Grimberg]

>>> From: Chaitanya Kulkarni <kch@nvidia.com>
>>>
>>> In function nvmf_dev_show() if we don't find the controller set the
>>> ret variable to -ENODEV.
>>>
>>> Signed-off-by: Chaitanya Kulkarni <kch@nvidia.com>
>>> Suggested-by: Sagi Grimberg <sagi@grimberg.me>
>>> ---
>>>    drivers/nvme/host/fabrics.c | 1 +
>>>    1 file changed, 1 insertion(+)
>>>
>>> diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c
>>> index df0326db306f..ca23ac926710 100644
>>> --- a/drivers/nvme/host/fabrics.c
>>> +++ b/drivers/nvme/host/fabrics.c
>>> @@ -1097,6 +1097,7 @@ static int nvmf_dev_show(struct seq_file *seq_file, void *private)
>>>         ctrl = seq_file->private;
>>>         if (!ctrl) {
>>>                 __nvmf_concat_opt_tokens(seq_file);
>>> +             ret = -ENODEV;
>>>                 goto out_unlock;
>>>         }
>>
>> I missed the discussion on this. Doesn't the __nvmf_concat_opt_tokens()
>> already return parameters that indicate to the caller that no controller
>> was found?
>>
> 
> The function I found in the repo returns nothing, is that the one you
> are referring to ?
> 
> static void __nvmf_concat_opt_tokens(struct seq_file *seq_file)

This is the path that Hannes added for userspace to detect kernel
capabilities. So probably its not appropriate to return an error
here, but rather take care of the original warning instead.

Re: [PATCH 5/5] nvme-fabrics: set ret to -ENODEV for error case

[Chaitanya Kulkarni]

>>> I missed the discussion on this. Doesn't the __nvmf_concat_opt_tokens()
>>> already return parameters that indicate to the caller that no controller
>>> was found?
>>>
>>
>> The function I found in the repo returns nothing, is that the one you
>> are referring to ?
>>
>> static void __nvmf_concat_opt_tokens(struct seq_file *seq_file)
> 
> This is the path that Hannes added for userspace to detect kernel
> capabilities. So probably its not appropriate to return an error
> here, but rather take care of the original warning instead.

When I look at the code it looks unconventional to not return
error when controller not found case, why is it not appropriate
to return error can you please explain ?

Can we add a meaningful comment to avoid future such patches.

Re: [PATCH 5/5] nvme-fabrics: set ret to -ENODEV for error case

[Sagi Grimberg]

>>>> I missed the discussion on this. Doesn't the __nvmf_concat_opt_tokens()
>>>> already return parameters that indicate to the caller that no controller
>>>> was found?
>>>>
>>>
>>> The function I found in the repo returns nothing, is that the one you
>>> are referring to ?
>>>
>>> static void __nvmf_concat_opt_tokens(struct seq_file *seq_file)
>>
>> This is the path that Hannes added for userspace to detect kernel
>> capabilities. So probably its not appropriate to return an error
>> here, but rather take care of the original warning instead.
> 
> When I look at the code it looks unconventional to not return
> error when controller not found case, why is it not appropriate
> to return error can you please explain ?
> 
> Can we add a meaningful comment to avoid future such patches.

That is how Hannes proposed to read capabilities from the kernel
in terms of valid connection string arguments. Read /dev/nvme-fabrics
without a controller.

Re: [PATCH 0/5] nvme-fabrics: small cleanup

[Christoph Hellwig]

Thanks,

applied patches 1-4 to nvme-5.18.