On 2020-01-21 15:53, James Smart wrote:
> +static int
> +sdev_set_queue_depth(struct scsi_device *sdev, int depth)
> +{
> + struct scsi_host_template *sht = sdev->host->hostt;
> + int retval;
> +
> + retval = sht->change_queue_depth(sdev, depth);
> + if (retval < 0)
> + return retval;
> +
> + sdev->max_queue_depth = sdev->queue_depth;
> +
> + return 0;
> +}
'sht' is only used once, so I'm not sure whether it is worth it to keep
that local variable.
> +static ssize_t
> +store_host_lun_queue_depth(struct device *dev, struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct Scsi_Host *shost = class_to_shost(dev);
> + struct scsi_host_template *sht = shost->hostt;
> + struct scsi_device *sdev;
> + int depth, retval;
> +
> + if (!sht->change_queue_depth)
> + return -EINVAL;
> +
> + depth = simple_strtoul(buf, NULL, 0);
> + if (depth < 1 || depth > shost->can_queue)
> + return -EINVAL;
> +
> + shost_for_each_device(sdev, shost) {
> + retval = sdev_set_queue_depth(sdev, depth);
> + if (retval != 0)
> + sdev_printk(KERN_INFO, sdev,
> + "failed to set queue depth to %d (err %d)",
> + depth, retval);
> + }
> +
> + return count;
> +}
Returning -EINVAL from all error paths makes it impossible to tell the
difference between a value that is out of range and queue depth changes
not being supported. Should another error code be returned if
sht->change_queue_depth == NULL?
> @@ -992,12 +1037,10 @@ sdev_store_queue_depth(struct device *dev, struct device_attribute *attr,
> if (depth < 1 || depth > sdev->host->can_queue)
> return -EINVAL;
>
> - retval = sht->change_queue_depth(sdev, depth);
> + retval = sdev_set_queue_depth(sdev, depth);
> if (retval < 0)
> return retval;
>
> - sdev->max_queue_depth = sdev->queue_depth;
> -
> return count;
> }
How about splitting this patch into two patches: one that introduces the
function sdev_set_queue_depth() and a second patch that introduces the
lun_queue_depth sysfs attribute?
Thanks,
Bart.
