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
