diff mbox series

[v3,01/16] nvmet: report ioccsz and iorcsz for disc ctrl

Message ID 20231218153105.12717-2-dwagner@suse.de
State New
Headers
Series enable nvmet-fc for blktests |

Commit Message

Daniel Wagner Dec. 18, 2023, 3:30 p.m. UTC 
  The host started to verify the ioccsz and iorcsz values. I/O controllers
return valid values but not the discovery controllers. Use the same
values as for I/O controllers.

Fixes: 2fcd3ab39826 ("nvme-fabrics: check ioccsz and iorcsz")
Signed-off-by: Daniel Wagner <dwagner@suse.de>
---
 drivers/nvme/target/discovery.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

Comments

Christoph Hellwig Dec. 19, 2023, 4:27 a.m. UTC | #1 
On Mon, Dec 18, 2023 at 04:30:49PM +0100, Daniel Wagner wrote:
> +	/*
> +	 * Max command capsule size is sqe + in-capsule data size.
> +	 * Disable in-capsule data for Metadata capable controllers.

A why on the disable would be useful here - the fact that it is disabled
is pretty obvious from the code.
Hannes Reinecke Dec. 19, 2023, 7:24 a.m. UTC | #2 
On 12/18/23 16:30, Daniel Wagner wrote:
> The host started to verify the ioccsz and iorcsz values. I/O controllers
> return valid values but not the discovery controllers. Use the same
> values as for I/O controllers.
> 
> Fixes: 2fcd3ab39826 ("nvme-fabrics: check ioccsz and iorcsz")
> Signed-off-by: Daniel Wagner <dwagner@suse.de>
> ---
>   drivers/nvme/target/discovery.c | 13 +++++++++++++
>   1 file changed, 13 insertions(+)
> 
> diff --git a/drivers/nvme/target/discovery.c b/drivers/nvme/target/discovery.c
> index 668d257fa986..e3c4d247dd23 100644
> --- a/drivers/nvme/target/discovery.c
> +++ b/drivers/nvme/target/discovery.c
> @@ -249,6 +249,7 @@ static void nvmet_execute_disc_identify(struct nvmet_req *req)
>   {
>   	struct nvmet_ctrl *ctrl = req->sq->ctrl;
>   	struct nvme_id_ctrl *id;
> +	u32 cmd_capsule_size;
>   	u16 status = 0;
>   
>   	if (!nvmet_check_transfer_len(req, NVME_IDENTIFY_DATA_SIZE))
> @@ -294,6 +295,18 @@ static void nvmet_execute_disc_identify(struct nvmet_req *req)
>   
>   	strscpy(id->subnqn, ctrl->subsys->subsysnqn, sizeof(id->subnqn));
>   
> +	/*
> +	 * Max command capsule size is sqe + in-capsule data size.

'is sqe size + in-capsule data size'

> +	 * Disable in-capsule data for Metadata capable controllers.
> +	 */
> +	cmd_capsule_size = sizeof(struct nvme_command);
> +	if (!ctrl->pi_support)
> +		cmd_capsule_size += req->port->inline_data_size;
> +	id->ioccsz = cpu_to_le32(cmd_capsule_size / 16);

Why the division by 16?

> +
> +	/* Max response capsule size is cqe */
'is cqe size'

> +	id->iorcsz = cpu_to_le32(sizeof(struct nvme_completion) / 16);
> +
>   	status = nvmet_copy_to_sgl(req, 0, id, sizeof(*id));
>   
>   	kfree(id);

Otherwise looks good.

Cheers,

Hannes
Daniel Wagner Dec. 19, 2023, 3:15 p.m. UTC | #3 
On Tue, Dec 19, 2023 at 08:24:58AM +0100, Hannes Reinecke wrote:
> > +	 * Disable in-capsule data for Metadata capable controllers.
> > +	 */
> > +	cmd_capsule_size = sizeof(struct nvme_command);
> > +	if (!ctrl->pi_support)
> > +		cmd_capsule_size += req->port->inline_data_size;
> > +	id->ioccsz = cpu_to_le32(cmd_capsule_size / 16);
> 
> Why the division by 16?

The unit size is 16 bytes:

  I/O Queue Command Capsule Supported Size (IOCCSZ): This field defines the
  maximum I/O command capsule size in 16 byte units.
Max Gurtovoy Dec. 20, 2023, 12:50 a.m. UTC | #4 
On 19/12/2023 6:27, Christoph Hellwig wrote:
> On Mon, Dec 18, 2023 at 04:30:49PM +0100, Daniel Wagner wrote:
>> +	/*
>> +	 * Max command capsule size is sqe + in-capsule data size.
>> +	 * Disable in-capsule data for Metadata capable controllers.
> 
> A why on the disable would be useful here - the fact that it is disabled
> is pretty obvious from the code.
> 

We have another thread on this patch that I think is wrong since the 
discovery controller shouldn't set these values according to the spec. 
It is explicitly say that ioccsz and iorcsz are reserved for discovery 
controllers.

I've sent a proposal to fix the initiator/host code.
diff mbox series

Patch

diff --git a/drivers/nvme/target/discovery.c b/drivers/nvme/target/discovery.c
index 668d257fa986..e3c4d247dd23 100644
--- a/drivers/nvme/target/discovery.c
+++ b/drivers/nvme/target/discovery.c
@@ -249,6 +249,7 @@  static void nvmet_execute_disc_identify(struct nvmet_req *req)
 {
 	struct nvmet_ctrl *ctrl = req->sq->ctrl;
 	struct nvme_id_ctrl *id;
+	u32 cmd_capsule_size;
 	u16 status = 0;
 
 	if (!nvmet_check_transfer_len(req, NVME_IDENTIFY_DATA_SIZE))
@@ -294,6 +295,18 @@  static void nvmet_execute_disc_identify(struct nvmet_req *req)
 
 	strscpy(id->subnqn, ctrl->subsys->subsysnqn, sizeof(id->subnqn));
 
+	/*
+	 * Max command capsule size is sqe + in-capsule data size.
+	 * Disable in-capsule data for Metadata capable controllers.
+	 */
+	cmd_capsule_size = sizeof(struct nvme_command);
+	if (!ctrl->pi_support)
+		cmd_capsule_size += req->port->inline_data_size;
+	id->ioccsz = cpu_to_le32(cmd_capsule_size / 16);
+
+	/* Max response capsule size is cqe */
+	id->iorcsz = cpu_to_le32(sizeof(struct nvme_completion) / 16);
+
 	status = nvmet_copy_to_sgl(req, 0, id, sizeof(*id));
 
 	kfree(id);