Re: [PATCH 1/4] nvme-core: use macro for ctrl page size default value

[Keith Busch <kbusch@kernel.org>]

On Tue, Jul 07, 2020 at 05:58:28PM -0700, Chaitanya Kulkarni wrote:
> This is a preparation patch which is needed to centralize the page shift
> value for the have ctrl->page_size.

At this point I would recommend removing 'page_size' from 'struct ctrl'
and use a #define NVME_PAGE_SIZE in its place. There hasn't been any
attempt to work with different page sizes so I don't see much reason
for keeping the older code to support it. As an added bonus, a constant
simplifies several calculations in the io path.

> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 898885630ad8..e0b47e77cbca 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -2344,12 +2344,7 @@ EXPORT_SYMBOL_GPL(nvme_disable_ctrl);
>  
>  int nvme_enable_ctrl(struct nvme_ctrl *ctrl)
>  {
> -	/*
> -	 * Default to a 4K page size, with the intention to update this
> -	 * path in the future to accomodate architectures with differing
> -	 * kernel and IO page sizes.
> -	 */
> -	unsigned dev_page_min, page_shift = 12;
> +	unsigned dev_page_min;
>  	int ret;
>  
>  	ret = ctrl->ops->reg_read64(ctrl, NVME_REG_CAP, &ctrl->cap);
> @@ -2359,20 +2354,20 @@ int nvme_enable_ctrl(struct nvme_ctrl *ctrl)
>  	}
>  	dev_page_min = NVME_CAP_MPSMIN(ctrl->cap) + 12;
>  
> -	if (page_shift < dev_page_min) {
> +	if (NVME_CTRL_PAGE_SHIFT < dev_page_min) {
>  		dev_err(ctrl->device,
>  			"Minimum device page size %u too large for host (%u)\n",
> -			1 << dev_page_min, 1 << page_shift);
> +			1 << dev_page_min, 1 << NVME_CTRL_PAGE_SHIFT);
>  		return -ENODEV;
>  	}
>  
> -	ctrl->page_size = 1 << page_shift;
> +	ctrl->page_size = 1 << NVME_CTRL_PAGE_SHIFT;
>  
>  	if (NVME_CAP_CSS(ctrl->cap) & NVME_CAP_CSS_CSI)
>  		ctrl->ctrl_config = NVME_CC_CSS_CSI;
>  	else
>  		ctrl->ctrl_config = NVME_CC_CSS_NVM;
> -	ctrl->ctrl_config |= (page_shift - 12) << NVME_CC_MPS_SHIFT;
> +	ctrl->ctrl_config |= (NVME_CTRL_PAGE_SHIFT - 12) << NVME_CC_MPS_SHIFT;
>  	ctrl->ctrl_config |= NVME_CC_AMS_RR | NVME_CC_SHN_NONE;
>  	ctrl->ctrl_config |= NVME_CC_IOSQES | NVME_CC_IOCQES;
>  	ctrl->ctrl_config |= NVME_CC_ENABLE;
> diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
> index 85d76981b66e..4a56897e9081 100644
> --- a/drivers/nvme/host/nvme.h
> +++ b/drivers/nvme/host/nvme.h
> @@ -37,6 +37,13 @@ extern unsigned int admin_timeout;
>  #define  NVME_INLINE_METADATA_SG_CNT  1
>  #endif
>  
> +/*
> + * Default to a 4K page size, with the intention to update this
> + * path in the future to accomodate architectures with differing
> + * kernel and IO page sizes.
> + */
> +#define NVME_CTRL_PAGE_SHIFT	12
> +
>  extern struct workqueue_struct *nvme_wq;
>  extern struct workqueue_struct *nvme_reset_wq;
>  extern struct workqueue_struct *nvme_delete_wq;

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme