Re: [PATCH 10/23] scsi: add a device_configure method to the host template

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 3/25/24 08:54, Christoph Hellwig wrote:
> This is a version of ->slave_configure that also takes a queue_limits
> structure that the caller applies, and thus allows drivers to reconfigure
> the queue using the atomic queue limits API.
> 
> In the long run it should also replace ->slave_configure entirely as
> there is no need to have two different methods here, and the slave
> name in addition to being politically charged also has no basis in
> the SCSI standards or the kernel code.
> 
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>
> ---
>  drivers/scsi/scsi_scan.c | 33 +++++++++++++++++++--------------
>  include/scsi/scsi_host.h |  4 ++++
>  2 files changed, 23 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
> index 699356d7d17545..8e05780f802523 100644
> --- a/drivers/scsi/scsi_scan.c
> +++ b/drivers/scsi/scsi_scan.c
> @@ -227,7 +227,7 @@ static int scsi_realloc_sdev_budget_map(struct scsi_device *sdev,
>  
>  	/*
>  	 * realloc if new shift is calculated, which is caused by setting
> -	 * up one new default queue depth after calling ->slave_configure
> +	 * up one new default queue depth after calling ->device_configure
>  	 */
>  	if (!need_alloc && new_shift != sdev->budget_map.shift)
>  		need_alloc = need_free = true;
> @@ -874,8 +874,9 @@ static int scsi_probe_lun(struct scsi_device *sdev, unsigned char *inq_result,
>  static int scsi_add_lun(struct scsi_device *sdev, unsigned char *inq_result,
>  		blist_flags_t *bflags, int async)
>  {
> +	const struct scsi_host_template *hostt = sdev->host->hostt;
>  	struct queue_limits lim;
> -	int ret;
> +	int ret, ret2;
>  
>  	/*
>  	 * XXX do not save the inquiry, since it can change underneath us,
> @@ -1073,22 +1074,26 @@ static int scsi_add_lun(struct scsi_device *sdev, unsigned char *inq_result,
>  		lim.max_hw_sectors = 512;
>  	else if (*bflags & BLIST_MAX_1024)
>  		lim.max_hw_sectors = 1024;
> -	ret = queue_limits_commit_update(sdev->request_queue, &lim);
> +
> +	if (hostt->device_configure)
> +		ret = hostt->device_configure(sdev, &lim);
> +	else if (hostt->slave_configure)
> +		ret = hostt->slave_configure(sdev);
> +
> +	ret2 = queue_limits_commit_update(sdev->request_queue, &lim);

Why do this if ->device_configure() or ->slave_configure() failed ?
Shouldn't the "if (ret) goto fail" hunk be moved above this call ?

> diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
> index b0948ab69e0fa6..1959193d47e7f5 100644
> --- a/include/scsi/scsi_host.h
> +++ b/include/scsi/scsi_host.h
> @@ -211,7 +211,11 @@ struct scsi_host_template {
>  	 *     up after yourself before returning non-0
>  	 *
>  	 * Status: OPTIONAL
> +	 *
> +	 * Note: slave_configure is the legacy version, use device_configure for
> +	 * all new code.

Maybe explictly mention that both *cannot* be defined here ?

>  	 */
> +	int (* device_configure)(struct scsi_device *, struct queue_limits *lim);
>  	int (* slave_configure)(struct scsi_device *);
>  
>  	/*

With these 2 nits addressed, looks all goo to me. Feel free to add:

Reviewed-by: Damien Le Moal <dlemoal@xxxxxxxxxx>

-- 
Damien Le Moal
Western Digital Research





[Index of Archives]     [Linux Filesystems]     [Linux SCSI]     [Linux RAID]     [Git]     [Kernel Newbies]     [Linux Newbie]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Samba]     [Device Mapper]

  Powered by Linux