lkml.org 
[lkml]   [2024]   [Apr]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [PATCH] ata: Add sdev attribute to lower link speed in runtime
    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@axis.com>
    > ---
    > 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@vger.kernel.org
    > +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
    >

    \
     
     \ /
      Last update: 2024-04-12 19:00    [W:6.910 / U:0.916 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site