Hello Gustav,
On Fri, Apr 12, 2024 at 03:48:38PM +0200, Gustav Ekelund wrote:
> Expose a new sysfs attribute to userspace that gives root the ability to
> lower the link speed in a scsi_device at runtime. The handle enables
> programs to, based on external circumstances that may be unbeknownst to
> the kernel, determine if a link should slow down to perhaps achieve a
> stabler signal. External circumstances could include the mission time
> of the connected hardware or observations to temperature trends.
>
> Writing 1 to /sys/block/*/device/down_link_spd signals the kernel to
> first lower the link speed one step with sata_down_spd_limit and then
> finish off with sata_link_hardreset.
>
> Signed-off-by: Gustav Ekelund <gustav.ekelund@xxxxxxxx>
> ---
> Documentation/ABI/testing/sysfs-block-device | 13 +++++++++
> drivers/ata/libahci.c | 1 +
> drivers/ata/libata-sata.c | 29 ++++++++++++++++++++
> include/linux/libata.h | 1 +
> 4 files changed, 44 insertions(+)
>
> diff --git a/Documentation/ABI/testing/sysfs-block-device b/Documentation/ABI/testing/sysfs-block-device
> index 2d543cfa4079..dba537fdf787 100644
> --- a/Documentation/ABI/testing/sysfs-block-device
> +++ b/Documentation/ABI/testing/sysfs-block-device
> @@ -117,3 +117,16 @@ Description:
> Writing "1" to this file enables the use of command duration
> limits for read and write commands in the kernel and turns on
> the feature on the device. Writing "0" disables the feature.
> +
> +
> +What: /sys/block/*/device/down_link_spd
> +Date: Mar, 2024
> +KernelVersion: v6.10
> +Contact: linux-ide@xxxxxxxxxxxxxxx
> +Description:
> + (WO) Write 1 to the file to lower the SATA link speed one step
> + and then hard reset. Other values are rejected with -EINVAL.
> +
> + Consider using libata.force for setting the link speed at boot.
> + Resort to down_link_spd in systems that deem it useful to lower
> + the link speed at runtime for particular links.
> diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
> index 1a63200ea437..2622e965d98c 100644
> --- a/drivers/ata/libahci.c
> +++ b/drivers/ata/libahci.c
> @@ -138,6 +138,7 @@ static struct attribute *ahci_sdev_attrs[] = {
> &dev_attr_unload_heads.attr,
> &dev_attr_ncq_prio_supported.attr,
> &dev_attr_ncq_prio_enable.attr,
> + &dev_attr_down_link_spd.attr,
> NULL
> };
>
> diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
> index 0fb1934875f2..5e1257b15f95 100644
> --- a/drivers/ata/libata-sata.c
> +++ b/drivers/ata/libata-sata.c
> @@ -1538,3 +1538,32 @@ void ata_eh_analyze_ncq_error(struct ata_link *link)
> ehc->i.err_mask &= ~AC_ERR_DEV;
> }
> EXPORT_SYMBOL_GPL(ata_eh_analyze_ncq_error);
> +
> +static ssize_t down_link_spd_store(struct device *device,
> + struct device_attribute *attr,
> + const char *buf, size_t len)
> +{
> + struct scsi_device *sdev;
In general, please compare your code with ata_ncq_prio_enable_store().
Please assign sdev directly:
struct scsi_device *sdev = to_scsi_device(device);
> + struct ata_port *ap;
> + long input;
> + int rc;
> +
> + rc = kstrtol(buf, 10, &input);
> + if (rc)
> + return rc;
> + if ((input < 0) || (input > 1))
> + return -EINVAL;
> +
> + sdev = to_scsi_device(device);
> + ap = ata_shost_to_port(sdev->host);
Please also check ata_scsi_find_dev() like ata_ncq_prio_enable_store() does.
> +
> + rc = sata_down_spd_limit(&ap->link, 0);
Damien,
the kdoc for sata_down_spd_limit() is just
"LOCKING: Inherited from caller"
Do we perhaps need to hold the ap->lock here?
> + if (rc)
> + return rc;
> + rc = sata_link_hardreset(&ap->link, sata_deb_timing_normal,
> + ata_deadline(jiffies, ATA_TMOUT_INTERNAL_QUICK), NULL, NULL);
You shouldn't call sata_link_hardreset() directly.
Instead you should do:
spin_lock_irqsave(ap->lock, flags);
ehi->action |= ATA_EH_RESET;
ehi->flags |= ATA_EHI_NO_AUTOPSY | ATA_EHI_QUIET;
ata_port_schedule_eh(ap);
spin_unlock_irqrestore(ap->lock, flags);
See:
https://github.com/torvalds/linux/blob/v6.9-rc3/drivers/ata/libata-core.c#L5873-L5883
and
https://github.com/torvalds/linux/blob/v6.9-rc3/drivers/ata/libata-core.c#L5265-L5267
> +
> + return rc ? rc : len;
Wouldn't it be more helpful if you could set a target speed instead?
How else are you supposed to be able to go back to the faster speed.
Perhaps you could look how libata.force forces a speed, and reuse
that struct member, or possibly add a new struct member if needed,
to store the forced target speed.
And possible a _show() to read the current target speed.
Writing "0" to target speed could possibly clear the forced speed.
> +}
> +DEVICE_ATTR_WO(down_link_spd);
> +EXPORT_SYMBOL_GPL(dev_attr_down_link_spd);
> diff --git a/include/linux/libata.h b/include/linux/libata.h
> index 26d68115afb8..51d23a60355b 100644
> --- a/include/linux/libata.h
> +++ b/include/linux/libata.h
> @@ -510,6 +510,7 @@ extern struct device_attribute dev_attr_ncq_prio_enable;
> extern struct device_attribute dev_attr_em_message_type;
> extern struct device_attribute dev_attr_em_message;
> extern struct device_attribute dev_attr_sw_activity;
> +extern struct device_attribute dev_attr_down_link_spd;
> #endif
>
> enum sw_activity {
> --
> 2.39.2
>
