diff mbox series

[v4,07/18] nvme: Fix reservation status related structs

Message ID 20230224174502.321490-8-michael.christie@oracle.com (mailing list archive)
State Not Applicable, archived
Delegated to: Mike Snitzer
Headers show
Series [v4,01/18] block: Add PR callouts for read keys and reservation | expand

Commit Message

Mike Christie Feb. 24, 2023, 5:44 p.m. UTC 
This fixes the following issues with the reservation status structs:

1. resv10 is bytes 23:10 so it should be 14 bytes.
2. regctl_ds only supports 64 bit host IDs.

These are not currently used, but will be in this patchset which adds
support for the reservation report command.

Signed-off-by: Mike Christie <michael.christie@oracle.com>
Reviewed-by: Keith Busch <kbusch@kernel.org>
---
 include/linux/nvme.h | 33 +++++++++++++++++++++++++--------
 1 file changed, 25 insertions(+), 8 deletions(-)

Comments

Chaitanya Kulkarni March 5, 2023, 9:26 p.m. UTC | #1 
On 2/24/2023 9:44 AM, Mike Christie wrote:
> This fixes the following issues with the reservation status structs:
> 
> 1. resv10 is bytes 23:10 so it should be 14 bytes.
> 2. regctl_ds only supports 64 bit host IDs.
> 
> These are not currently used, but will be in this patchset which adds
> support for the reservation report command.
> 
> Signed-off-by: Mike Christie <michael.christie@oracle.com>
> Reviewed-by: Keith Busch <kbusch@kernel.org>
> ---

Looks good.

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

-ck

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel
Christoph Hellwig March 14, 2023, 5:12 p.m. UTC | #2 
Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel
Christoph Hellwig March 14, 2023, 5:15 p.m. UTC | #3 
On Fri, Feb 24, 2023 at 11:44:51AM -0600, Mike Christie wrote:
> +	__u8	resv10[14];
> +	union {
> +		struct {
> +			__u8	rsvd24[40];
> +			struct nvme_registered_ctrl_ext regctl_eds[0];
> +		};
> +		struct nvme_registered_ctrl regctl_ds[0];
> +	};

... actually - I think both these zero sized arrays should
be the modern [] notation.

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel
Mike Christie March 14, 2023, 10:23 p.m. UTC | #4 
On 3/14/23 12:15 PM, Christoph Hellwig wrote:
> On Fri, Feb 24, 2023 at 11:44:51AM -0600, Mike Christie wrote:
>> +	__u8	resv10[14];
>> +	union {
>> +		struct {
>> +			__u8	rsvd24[40];
>> +			struct nvme_registered_ctrl_ext regctl_eds[0];
>> +		};
>> +		struct nvme_registered_ctrl regctl_ds[0];
>> +	};
> 
> ... actually - I think both these zero sized arrays should
> be the modern [] notation.

gcc at least doesn't let you use [] on a member in a union. You get:

./include/linux/nvme.h:804:31: error: flexible array member in union
  804 |   struct nvme_registered_ctrl regctl_ds[];


We could do separate structs though:


struct nvme_registered_ctrl {
	__le16	cntlid;
	__u8	rcsts;
	__u8	rsvd3[5];
	__le64	hostid;
	__le64	rkey;
};

struct nvme_reservation_status {
	__le32	gen;
	__u8	rtype;
	__u8	regctl[2];
	__u8	resv5[2];
	__u8	ptpls;
	__u8	resv10[14];
	struct nvme_registered_ctrl regctl_ds[];
};

struct nvme_registered_ctrl_ext {
	__le16	cntlid;
	__u8	rcsts;
	__u8	rsvd3[5];
	__le64	rkey;
	__u8	hostid[16];
	__u8	rsvd32[32];
};

struct nvme_reservation_status_ext {
	__le32	gen;
	__u8	rtype;
	__u8	regctl[2];
	__u8	resv5[2];
	__u8	ptpls;
	__u8	resv10[14];
	__u8	rsvd24[40];
	struct nvme_registered_ctrl_ext regctl_eds[];
};

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel
Christoph Hellwig March 15, 2023, 5:40 a.m. UTC | #5 
On Tue, Mar 14, 2023 at 05:23:16PM -0500, Mike Christie wrote:
> We could do separate structs though:

I suspect that's probably better in the long run, as the [0] notation
is on its way out.

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel
Mike Christie March 20, 2023, 5:08 p.m. UTC | #6 
On 3/15/23 12:40 AM, Christoph Hellwig wrote:
> On Tue, Mar 14, 2023 at 05:23:16PM -0500, Mike Christie wrote:
>> We could do separate structs though:
> 
> I suspect that's probably better in the long run, as the [0] notation
> is on its way out.

Ok. I was able to use the separate structs and then use [] and struct_size.

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel
diff mbox series

Patch

diff --git a/include/linux/nvme.h b/include/linux/nvme.h
index 4fad4aa245fb..c8c504926462 100644
--- a/include/linux/nvme.h
+++ b/include/linux/nvme.h
@@ -759,20 +759,37 @@  enum {
 	NVME_LBART_ATTRIB_HIDE	= 1 << 1,
 };
 
+struct nvme_registered_ctrl {
+	__le16	cntlid;
+	__u8	rcsts;
+	__u8	rsvd3[5];
+	__le64	hostid;
+	__le64	rkey;
+};
+
+struct nvme_registered_ctrl_ext {
+	__le16	cntlid;
+	__u8	rcsts;
+	__u8	rsvd3[5];
+	__le64	rkey;
+	__u8	hostid[16];
+	__u8	rsvd32[32];
+};
+
 struct nvme_reservation_status {
 	__le32	gen;
 	__u8	rtype;
 	__u8	regctl[2];
 	__u8	resv5[2];
 	__u8	ptpls;
-	__u8	resv10[13];
-	struct {
-		__le16	cntlid;
-		__u8	rcsts;
-		__u8	resv3[5];
-		__le64	hostid;
-		__le64	rkey;
-	} regctl_ds[];
+	__u8	resv10[14];
+	union {
+		struct {
+			__u8	rsvd24[40];
+			struct nvme_registered_ctrl_ext regctl_eds[0];
+		};
+		struct nvme_registered_ctrl regctl_ds[0];
+	};
 };
 
 enum nvme_async_event_type {