Re: [PATCH 2/2] nvme: use blk_validate_block_size() for max LBA check

[John Garry <john.g.garry@oracle.com>]

On 18/12/2024 02:02, Luis Chamberlain wrote:

nit: is this really an LBA check?

> The block layer already has support to validates proper block sizes
> with blk_validate_block_size(), we can leverage that as well.
> 
> No functional changes.
> 
> Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>

Most of my comments are minor/off topic, so:

Reviewed-by: John Garry <john.g.garry@oracle.com>

> ---
>   drivers/nvme/host/core.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index d169a30eb935..a970168a3014 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -2034,7 +2034,7 @@ static bool nvme_update_disk_info(struct nvme_ns *ns, struct nvme_id_ns *id,
>   	 * or smaller than a sector size yet, so catch this early and don't
>   	 * allow block I/O.

I'm not sure if this comment is really still of value or can be reduced

>   	 */
> -	if (head->lba_shift > PAGE_SHIFT || head->lba_shift < SECTOR_SHIFT) {
> +	if (blk_validate_block_size(bs)) {

There was only a single user of this outside the block layer, and I was 
hoping that this function could eventually be internalized - not any longer.

>   		bs = (1 << 9);

comment on original code:

		bs = SECTOR_SIZE

seems more obvious

>   		valid = false;
>   	}