Re: [PATCHv3 04/11] nvmet: implement supported features log

[Christoph Hellwig <hch@lst.de>]

On Tue, Nov 05, 2024 at 09:48:57AM -0800, Keith Busch wrote:
> From: Keith Busch <kbusch@kernel.org>
> 
> This log is required for nvme 2.1.
> 
> Signed-off-by: Keith Busch <kbusch@kernel.org>
> ---
>  drivers/nvme/target/admin-cmd.c | 25 +++++++++++++++++++++++++
>  include/linux/nvme.h            |  1 +
>  2 files changed, 26 insertions(+)
> 
> diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
> index b8229d6c9998d..c9b38d593fda5 100644
> --- a/drivers/nvme/target/admin-cmd.c
> +++ b/drivers/nvme/target/admin-cmd.c
> @@ -344,6 +344,29 @@ static void nvmet_execute_get_log_page_ana(struct nvmet_req *req)
>  	nvmet_req_complete(req, status);
>  }
>  
> +static void nvmet_execute_get_log_page_features(struct nvmet_req *req)
> +{
> +	__le32 *features;
> +	u16 status;
> +
> +	features = kzalloc(1024, GFP_KERNEL);

The magic number here is a bit weird.  Maybe add a

#define NVME_FEAT_MAX		256

and then make this ΝVME_FEAT_MAX * sizeof(*features)?

Same below.

> +	if (!features) {
> +		status = NVME_SC_INTERNAL;
> +		goto out;
> +	}
> +
> +	features[NVME_FEAT_NUM_QUEUES] = cpu_to_le32(1 << 21 | 1);
> +	features[NVME_FEAT_KATO] = cpu_to_le32(1 << 21 | 1);
> +	features[NVME_FEAT_ASYNC_EVENT] = cpu_to_le32(1 << 21 | 1);
> +	features[NVME_FEAT_HOST_ID] = cpu_to_le32(1 << 21 | 1);
> +	features[NVME_FEAT_WRITE_PROTECT] = cpu_to_le32(1 << 20 | 1);

And having names for the bits / scopes would be kinda nice as well.
(maybe same for the logs in the last patch)

Otherwise this looks good to me.