| Message ID | 20240324235448.2039074-11-hch@lst.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [01/23] block: don't reject too large max_user_setors in blk_validate_limits | expand |
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@lst.de> > --- > 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@kernel.org>
On 3/24/24 16: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. There are two methods with names that are politically charged: slave_configure() and slave_alloc(). Shouldn't both be renamed? > * Status: OPTIONAL > + * > + * Note: slave_configure is the legacy version, use device_configure for > + * all new code. > */ > + int (* device_configure)(struct scsi_device *, struct queue_limits *lim); > int (* slave_configure)(struct scsi_device *); The name "device_configure" may make people wonder whether that method perhaps configures a struct device instance. How about using the name "sdev_configure" instead of "device_configure" to make it more clear that this method is used to configure a SCSI device? Thanks, Bart.
On Mon, Mar 25, 2024 at 01:35:08PM -0700, Bart Van Assche wrote: > There are two methods with names that are politically charged: > slave_configure() and slave_alloc(). Shouldn't both be renamed? Probably. This series howerver doesn't actually renames anything, it just adds a new method that takes the queue_limits and avoids the name while we're at it. > The name "device_configure" may make people wonder whether that method > perhaps configures a struct device instance. How about using the name > "sdev_configure" instead of "device_configure" to make it more clear > that this method is used to configure a SCSI device? I think device_ is probably better as it matches the target_ naming. I could live with sdev_ if everyone else would prefer it.
On Mon, Mar 25, 2024 at 04:38:43PM +0900, Damien Le Moal wrote: > > + 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 ? queue_limits_commit_update unlocks the limits lock, which we'd otherwise leak. We could have a queue_limits_commit_abort, but it seems a bit pointless. > > + * > > + * Note: slave_configure is the legacy version, use device_configure for > > + * all new code. > > Maybe explictly mention that both *cannot* be defined here ? Will do.
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); + if (ret2 && !ret) + ret = ret2; if (ret) goto fail; - if (sdev->host->hostt->slave_configure) { - ret = sdev->host->hostt->slave_configure(sdev); - if (ret) - goto fail; - - /* - * The queue_depth is often changed in ->slave_configure. - * Set up budget map again since memory consumption of - * the map depends on actual queue depth. - */ + /* + * The queue_depth is often changed in ->device_configure. + * + * Set up budget map again since memory consumption of the map depends + * on actual queue depth. + */ + if (hostt->device_configure || hostt->slave_configure) scsi_realloc_sdev_budget_map(sdev, sdev->queue_depth); - } if (sdev->scsi_level >= SCSI_3) scsi_attach_vpd(sdev); 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. */ + int (* device_configure)(struct scsi_device *, struct queue_limits *lim); int (* slave_configure)(struct scsi_device *); /*
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@lst.de> --- drivers/scsi/scsi_scan.c | 33 +++++++++++++++++++-------------- include/scsi/scsi_host.h | 4 ++++ 2 files changed, 23 insertions(+), 14 deletions(-)