[PATCH 0/2] nvme: pci: Fix queue_count parameter

[PATCH 0/2] nvme: pci: Fix queue_count parameter

[Minwoo Im]

The first patch will make it return an error if given value is larger
than number of possible cpus.  It has not been doing anything about it.

The second patch is just a nit for clean-up of local variable.  It does
not need to be initialized in this scope because kstrtoint() will return
an error if it fails without referring local variable after it.

It has been reported:
	http://lists.infradead.org/pipermail/linux-nvme/2019-April/023333.html

Minwoo Im (2):
  nvme: pci: Return -EINVAL if given nr_queue larger than nr_cpu
  nvme: pci: Do not initialize local_var

 drivers/nvme/host/pci.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

-- 
2.17.1

[PATCH 1/2] nvme: pci: Return -EINVAL if given nr_queue larger than nr_cpu

[Minwoo Im]

"write_queues" and "poll_queues" can be configured by module parameters
with callback queue_count_set().

This function shall return a error if given number of queue is larger
than nr_possible_cpus due to blk-mq.

Signed-off-by: Minwoo Im <minwoo.im.dev at gmail.com>
---
 drivers/nvme/host/pci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index a90cf5d63aac..1a7f44924e0f 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -152,7 +152,7 @@ static int queue_count_set(const char *val, const struct kernel_param *kp)
 	if (ret)
 		return ret;
 	if (n > num_possible_cpus())
-		n = num_possible_cpus();
+		return -EINVAL;
 
 	return param_set_int(val, kp);
 }
-- 
2.17.1

[PATCH 2/2] nvme: pci: Do not initialize local_var

[Minwoo Im]

Variable "n" will be assigned once kstrtoint() succeeds, otherwise it
will not be referred because kstrtoint() will return an error which
means go out from this function.

Signed-off-by: Minwoo Im <minwoo.im.dev at gmail.com>
---
 drivers/nvme/host/pci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 1a7f44924e0f..db894d4a37b6 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -146,7 +146,7 @@ static int io_queue_depth_set(const char *val, const struct kernel_param *kp)
 
 static int queue_count_set(const char *val, const struct kernel_param *kp)
 {
-	int n = 0, ret;
+	int n, ret;
 
 	ret = kstrtoint(val, 10, &n);
 	if (ret)
-- 
2.17.1

[PATCH 0/2] nvme: pci: Fix queue_count parameter

[Minwoo Im]

> -----Original Message-----
> From: Linux-nvme [mailto:linux-nvme-bounces at lists.infradead.org] On Behalf
> Of Minwoo Im
> Sent: Friday, April 12, 2019 12:53 AM
> To: linux-nvme at lists.infradead.org
> Cc: Keith Busch; Jens Axboe; Minwoo Im; Christoph Hellwig; Sagi Grimberg
> Subject: [PATCH 0/2] nvme: pci: Fix queue_count parameter
> 
> The first patch will make it return an error if given value is larger
> than number of possible cpus.  It has not been doing anything about it.
> 
> The second patch is just a nit for clean-up of local variable.  It does
> not need to be initialized in this scope because kstrtoint() will return
> an error if it fails without referring local variable after it.
> 
> It has been reported:
> 	http://lists.infradead.org/pipermail/linux-nvme/2019-April/023333.html
> 

Jens, 

Could you please have a look at this? I think it might fix
commit 3b6592f70("nvme: utilize two queue maps, one for reads and one for writes").

Thanks,

> Minwoo Im (2):
>   nvme: pci: Return -EINVAL if given nr_queue larger than nr_cpu
>   nvme: pci: Do not initialize local_var
> 
>  drivers/nvme/host/pci.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> --
> 2.17.1
> 
> 
> _______________________________________________
> Linux-nvme mailing list
> Linux-nvme at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-nvme

[PATCH 1/2] nvme: pci: Return -EINVAL if given nr_queue larger than nr_cpu

[Sagi Grimberg]

> "write_queues" and "poll_queues" can be configured by module parameters
> with callback queue_count_set().
> 
> This function shall return a error if given number of queue is larger
> than nr_possible_cpus due to blk-mq.

Why?

[PATCH 2/2] nvme: pci: Do not initialize local_var

[Sagi Grimberg]

> Variable "n" will be assigned once kstrtoint() succeeds, otherwise it
> will not be referred because kstrtoint() will return an error which
> means go out from this function.

Makes sense, but did you verify that static checkers don't complain on
this?

> 
> Signed-off-by: Minwoo Im <minwoo.im.dev at gmail.com>
> ---
>   drivers/nvme/host/pci.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
> index 1a7f44924e0f..db894d4a37b6 100644
> --- a/drivers/nvme/host/pci.c
> +++ b/drivers/nvme/host/pci.c
> @@ -146,7 +146,7 @@ static int io_queue_depth_set(const char *val, const struct kernel_param *kp)
>   
>   static int queue_count_set(const char *val, const struct kernel_param *kp)
>   {
> -	int n = 0, ret;
> +	int n, ret;
>   
>   	ret = kstrtoint(val, 10, &n);
>   	if (ret)
> 

[PATCH 2/2] nvme: pci: Do not initialize local_var

[Minwoo Im]

On Thu, Apr 25, 2019@1:32 AM Sagi Grimberg <sagi@grimberg.me> wrote:
>
> > Variable "n" will be assigned once kstrtoint() succeeds, otherwise it
> > will not be referred because kstrtoint() will return an error which
> > means go out from this function.
>
> Makes sense, but did you verify that static checkers don't complain on
> this?

Thanks for your comment on this. I didn't check static-check for this code,
by the way. If it needs to be done, I'll do it right away.

Thanks,

[PATCH 1/2] nvme: pci: Return -EINVAL if given nr_queue larger than nr_cpu

[Minwoo Im]

 >>>> "write_queues" and "poll_queues" can be configured by module 
parameters
 >>>> with callback queue_count_set().
 >>>>
 >>>> This function shall return a error if given number of queue is larger
 >>>> than nr_possible_cpus due to blk-mq.
 >>>
 >>> Why?
 >>
 >> I think I missed to write the details about what you asked for.  The
 >> reason is that
 >> now the variable n seems to be assigned without being used after all.
 >>
 >> I'm not sure whether it needs to return error or go through with
 >> reduced value for
 >> queue_cnt which is not greater than num_possible_cpus().
 >>
 >> If you would let me know what's going to be better for this, that
 >> would be great.
 >
 > I think its consistent with the rest of the fabric implementations..

Hi Sagi,

Do you mean that it needs to be reduced if value which is greater than 
num_possible_cpus() is given instead returning an error ?

Thanks,

[PATCH 2/2] nvme: pci: Do not initialize local_var

[Minwoo Im]

On 4/25/19 3:05 AM, Minwoo Im wrote:
> On Thu, Apr 25, 2019@1:32 AM Sagi Grimberg <sagi@grimberg.me> wrote:
>>
>>> Variable "n" will be assigned once kstrtoint() succeeds, otherwise it
>>> will not be referred because kstrtoint() will return an error which
>>> means go out from this function.
>>
>> Makes sense, but did you verify that static checkers don't complain on
>> this?
> 
> Thanks for your comment on this. I didn't check static-check for this code,
> by the way. If it needs to be done, I'll do it right away.
> 
> Thanks,
> 

There was nothing on sparse for the static check.
Thanks,

[PATCH 2/2] nvme: pci: Do not initialize local_var

[Christoph Hellwig]

Thanks, applied to nvme-5.2.