[PATCH] nvme: remove nssa from struct nvme_ctrl

[PATCH] nvme: remove nssa from struct nvme_ctrl

[Keith Busch]

The reported number of streams is not used outside the function that
gets it, so no need to stash it in the controller structure. Use a local
variable instead.

Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 drivers/nvme/host/core.c | 9 +++++----
 drivers/nvme/host/nvme.h | 1 -
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index fda837ce67ce..7d5e8b6f7d46 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -758,6 +758,7 @@ static int nvme_get_stream_params(struct nvme_ctrl *ctrl,
 static int nvme_configure_directives(struct nvme_ctrl *ctrl)
 {
 	struct streams_directive_params s;
+	u16 nssa;
 	int ret;
 
 	if (!(ctrl->oacs & NVME_CTRL_OACS_DIRECTIVES))
@@ -773,16 +774,16 @@ static int nvme_configure_directives(struct nvme_ctrl *ctrl)
 	if (ret)
 		goto out_disable_stream;
 
-	ctrl->nssa = le16_to_cpu(s.nssa);
-	if (ctrl->nssa < BLK_MAX_WRITE_HINTS - 1) {
+	nssa = le16_to_cpu(s.nssa);
+	if (nssa < BLK_MAX_WRITE_HINTS - 1) {
 		dev_info(ctrl->device, "too few streams (%u) available\n",
-					ctrl->nssa);
+					nssa);
 		/* this condition is not an error: streams are optional */
 		ret = 0;
 		goto out_disable_stream;
 	}
 
-	ctrl->nr_streams = min_t(u16, ctrl->nssa, BLK_MAX_WRITE_HINTS - 1);
+	ctrl->nr_streams = min_t(u16, nssa, BLK_MAX_WRITE_HINTS - 1);
 	dev_info(ctrl->device, "Using %u streams\n", ctrl->nr_streams);
 	return 0;
 
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index a162f6c6da6e..69796f96dc8c 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -280,7 +280,6 @@ struct nvme_ctrl {
 	u16 crdt[3];
 	u16 oncs;
 	u16 oacs;
-	u16 nssa;
 	u16 nr_streams;
 	u16 sqsize;
 	u32 max_namespaces;
-- 
2.25.4

Re: [PATCH] nvme: remove nssa from struct nvme_ctrl

[Chaitanya Kulkarni]

On 2/15/22 07:37, Keith Busch wrote:
> The reported number of streams is not used outside the function that
> gets it, so no need to stash it in the controller structure. Use a local
> variable instead.
> 
> Signed-off-by: Keith Busch <kbusch@kernel.org>
> ---
>

Looks good, can we make this feature configurable from KConfig ?

Are you also okay if we move features code from core.c to its
respective files, its getting really big e.g. mainly pr code,
sysfa code etc ?

Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>

Re: [PATCH] nvme: remove nssa from struct nvme_ctrl

[Chaitanya Kulkarni]

On 2/15/22 09:03, Chaitanya Kulkarni wrote:
> On 2/15/22 07:37, Keith Busch wrote:
>> The reported number of streams is not used outside the function that
>> gets it, so no need to stash it in the controller structure. Use a local
>> variable instead.
>>
>> Signed-off-by: Keith Busch <kbusch@kernel.org>
>> ---
>>
> 
> Looks good, can we make this feature configurable from KConfig ?
> 

right question is should we make optional features configurable ?

-ck

Re: [PATCH] nvme: remove nssa from struct nvme_ctrl

[Keith Busch]

On Tue, Feb 15, 2022 at 05:09:44PM +0000, Chaitanya Kulkarni wrote:
> On 2/15/22 09:03, Chaitanya Kulkarni wrote:
> > On 2/15/22 07:37, Keith Busch wrote:
> >> The reported number of streams is not used outside the function that
> >> gets it, so no need to stash it in the controller structure. Use a local
> >> variable instead.
> >>
> >> Signed-off-by: Keith Busch <kbusch@kernel.org>
> >> ---
> >>
> > 
> > Looks good, can we make this feature configurable from KConfig ?
> > 
> 
> right question is should we make optional features configurable ?

I don't see why. What benefit does the driver stand to gain from
conditionally compiling such capabilities?

Re: [PATCH] nvme: remove nssa from struct nvme_ctrl

[hch]

On Tue, Feb 15, 2022 at 10:37:36AM -0800, Keith Busch wrote:
> > > Looks good, can we make this feature configurable from KConfig ?
> > > 
> > 
> > right question is should we make optional features configurable ?
> 
> I don't see why. What benefit does the driver stand to gain from
> conditionally compiling such capabilities?

I could see the benefit for a really big feature that is not required
on typical embedded setups.  But multipathing is the only feature
anywhere that threshold.

Re: [PATCH] nvme: remove nssa from struct nvme_ctrl

[Chaitanya Kulkarni]

On 2/15/22 10:38, hch@lst.de wrote:
> On Tue, Feb 15, 2022 at 10:37:36AM -0800, Keith Busch wrote:
>>>> Looks good, can we make this feature configurable from KConfig ?
>>>>
>>>
>>> right question is should we make optional features configurable ?
>>
>> I don't see why. What benefit does the driver stand to gain from
>> conditionally compiling such capabilities?
> 

non data center setup don't use all the optional features, even
SSD cloud specification released by one hyperscaler which accessible
to public only uses small subset of base NVMe features, but as Christoph
said multipathing is only feature that can qualify so it doesn't make
any sense for small features..

> I could see the benefit for a really big feature that is not required
> on typical embedded setups.  But multipathing is the only feature
> anywhere that threshold.
> 


-ck

Re: [PATCH] nvme: remove nssa from struct nvme_ctrl

[Christoph Hellwig]

Thanks,

applied to nvme-5.18.