[PATCH 1/1] nvme-fabrics: don't check ioccsz/iorcsz for discovery

[PATCH 1/1] nvme-fabrics: don't check ioccsz/iorcsz for discovery

[Max Gurtovoy]

IOCCSZ and IORCSZ are reserved for discovery controllers. Avoid checking
their values during identify controller phase.

Fixes: 2fcd3ab39826 ("nvme-fabrics: check ioccsz and iorcsz")
Reported-by: Daniel Wagner <dwagner@suse.de>
Tested-by: Daniel Wagner <dwagner@suse.de>
Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
---
 drivers/nvme/host/core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index ee2e4c49892d..b218ac88fcf8 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -3068,14 +3068,14 @@ static int nvme_check_ctrl_fabric_info(struct nvme_ctrl *ctrl, struct nvme_id_ct
 		return -EINVAL;
 	}
 
-	if (ctrl->ioccsz < 4) {
+	if (!nvme_discovery_ctrl(ctrl) && ctrl->ioccsz < 4) {
 		dev_err(ctrl->device,
 			"I/O queue command capsule supported size %d < 4\n",
 			ctrl->ioccsz);
 		return -EINVAL;
 	}
 
-	if (ctrl->iorcsz < 1) {
+	if (!nvme_discovery_ctrl(ctrl) && ctrl->iorcsz < 1) {
 		dev_err(ctrl->device,
 			"I/O queue response capsule supported size %d < 1\n",
 			ctrl->iorcsz);
-- 
2.18.1

Re: [PATCH 1/1] nvme-fabrics: don't check ioccsz/iorcsz for discovery

[Sagi Grimberg]

> IOCCSZ and IORCSZ are reserved for discovery controllers. Avoid checking
> their values during identify controller phase.
> 
> Fixes: 2fcd3ab39826 ("nvme-fabrics: check ioccsz and iorcsz")
> Reported-by: Daniel Wagner <dwagner@suse.de>
> Tested-by: Daniel Wagner <dwagner@suse.de>
> Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
> ---
>   drivers/nvme/host/core.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index ee2e4c49892d..b218ac88fcf8 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -3068,14 +3068,14 @@ static int nvme_check_ctrl_fabric_info(struct nvme_ctrl *ctrl, struct nvme_id_ct
>   		return -EINVAL;
>   	}
>   
> -	if (ctrl->ioccsz < 4) {
> +	if (!nvme_discovery_ctrl(ctrl) && ctrl->ioccsz < 4) {
>   		dev_err(ctrl->device,
>   			"I/O queue command capsule supported size %d < 4\n",
>   			ctrl->ioccsz);
>   		return -EINVAL;
>   	}
>   
> -	if (ctrl->iorcsz < 1) {
> +	if (!nvme_discovery_ctrl(ctrl) && ctrl->iorcsz < 1) {
>   		dev_err(ctrl->device,
>   			"I/O queue response capsule supported size %d < 1\n",
>   			ctrl->iorcsz);

I still think we should at least warn (and ignore) for discovery
controllers.

Re: [PATCH 1/1] nvme-fabrics: don't check ioccsz/iorcsz for discovery

[Caleb Sander]

Why? As Max points out, the base NVMe spec says those fields are
reserved for discovery controllers.

On Wed, Dec 20, 2023 at 2:31 PM Sagi Grimberg <sagi@grimberg.me> wrote:
>
>
> > IOCCSZ and IORCSZ are reserved for discovery controllers. Avoid checking
> > their values during identify controller phase.
> >
> > Fixes: 2fcd3ab39826 ("nvme-fabrics: check ioccsz and iorcsz")
> > Reported-by: Daniel Wagner <dwagner@suse.de>
> > Tested-by: Daniel Wagner <dwagner@suse.de>
> > Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
> > ---
> >   drivers/nvme/host/core.c | 4 ++--
> >   1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> > index ee2e4c49892d..b218ac88fcf8 100644
> > --- a/drivers/nvme/host/core.c
> > +++ b/drivers/nvme/host/core.c
> > @@ -3068,14 +3068,14 @@ static int nvme_check_ctrl_fabric_info(struct nvme_ctrl *ctrl, struct nvme_id_ct
> >               return -EINVAL;
> >       }
> >
> > -     if (ctrl->ioccsz < 4) {
> > +     if (!nvme_discovery_ctrl(ctrl) && ctrl->ioccsz < 4) {
> >               dev_err(ctrl->device,
> >                       "I/O queue command capsule supported size %d < 4\n",
> >                       ctrl->ioccsz);
> >               return -EINVAL;
> >       }
> >
> > -     if (ctrl->iorcsz < 1) {
> > +     if (!nvme_discovery_ctrl(ctrl) && ctrl->iorcsz < 1) {
> >               dev_err(ctrl->device,
> >                       "I/O queue response capsule supported size %d < 1\n",
> >                       ctrl->iorcsz);
>
> I still think we should at least warn (and ignore) for discovery
> controllers.
>

Re: [PATCH 1/1] nvme-fabrics: don't check ioccsz/iorcsz for discovery

[Sagi Grimberg]

> Why? As Max points out, the base NVMe spec says those fields are
> reserved for discovery controllers.

Oh, didn't understand that it was already the case, I thought
that the intention was to made it as such. Can you point us
to this part in the spec?

Re: [PATCH 1/1] nvme-fabrics: don't check ioccsz/iorcsz for discovery

[Caleb Sander]

On Thu, Dec 21, 2023 at 4:02 AM Sagi Grimberg <sagi@grimberg.me> wrote:
>
>
> > Why? As Max points out, the base NVMe spec says those fields are
> > reserved for discovery controllers.
>
> Oh, didn't understand that it was already the case, I thought
> that the intention was to made it as such. Can you point us
> to this part in the spec?

https://nvmexpress.org/wp-content/uploads/NVM-Express-Base-Specification-2.0c-2022.10.04-Ratified.pdf
Figure 275: Identify – Identify Controller Data Structure, I/O Command
Set Independent
Both fields IOCCSZ and IORCSZ have "R" in the "Disc" column,
indicating that they are reserved for discovery controllers.

Re: [PATCH 1/1] nvme-fabrics: don't check ioccsz/iorcsz for discovery

[Keith Busch]

On Thu, Dec 21, 2023 at 08:45:05AM -0500, Caleb Sander wrote:
> On Thu, Dec 21, 2023 at 4:02 AM Sagi Grimberg <sagi@grimberg.me> wrote:
> >
> >
> > > Why? As Max points out, the base NVMe spec says those fields are
> > > reserved for discovery controllers.
> >
> > Oh, didn't understand that it was already the case, I thought
> > that the intention was to made it as such. Can you point us
> > to this part in the spec?
> 
> https://nvmexpress.org/wp-content/uploads/NVM-Express-Base-Specification-2.0c-2022.10.04-Ratified.pdf
> Figure 275: Identify – Identify Controller Data Structure, I/O Command
> Set Independent
> Both fields IOCCSZ and IORCSZ have "R" in the "Disc" column,
> indicating that they are reserved for discovery controllers.

Sounds good to me. I've applied for nvme-6.8.

Re: [PATCH 1/1] nvme-fabrics: don't check ioccsz/iorcsz for discovery

[Sagi Grimberg]

>>> Why? As Max points out, the base NVMe spec says those fields are
>>> reserved for discovery controllers.
>>
>> Oh, didn't understand that it was already the case, I thought
>> that the intention was to made it as such. Can you point us
>> to this part in the spec?
> 
> https://nvmexpress.org/wp-content/uploads/NVM-Express-Base-Specification-2.0c-2022.10.04-Ratified.pdf
> Figure 275: Identify – Identify Controller Data Structure, I/O Command
> Set Independent
> Both fields IOCCSZ and IORCSZ have "R" in the "Disc" column,
> indicating that they are reserved for discovery controllers.

Thanks Caleb.