From: Gustav Ekelund <gustaek@axis.com>
To: Niklas Cassel <cassel@kernel.org>
Cc: Damien Le Moal <dlemoal@kernel.org>,
Gustav Ekelund <gustav.ekelund@axis.com>,
hare@suse.de, martin.petersen@oracle.com,
linux-ide@vger.kernel.org, linux-kernel@vger.kernel.org,
kernel@axis.com
Subject: Re: [PATCH] ata: Add sdev attribute to lower link speed in runtime
Date: Thu, 18 Apr 2024 14:47:20 +0200 [thread overview]
Message-ID: <d9fe9fc1-1b28-4547-8845-399aaddc0863@axis.com> (raw)
In-Reply-To: <Zh-hASYS4XkyNJc9@ryzen>
On 4/17/24 12:14, Niklas Cassel wrote:
> On Mon, Apr 15, 2024 at 04:49:46PM +0200, Gustav Ekelund wrote:
>> On 4/13/24 02:29, Damien Le Moal wrote:
>>> On 4/12/24 22:48, 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.
>>>
>>> may, perhaps, could... This does not sound very deterministic. Do you have an
>>> actual practical use case where this patch is useful and solve a real problem ?
>>>
>>> Strictly speaking, if you are seeing link stability issues due to temperature or
>>> other environmental factors (humidity, altitude), then either you are operating
>>> your hardware (board and/or HDD) outside of their environmental specifications,
>>> or you have some serious hardware issues (which can be a simple as a bad SATA
>>> cable or an inappropriate power supply). In both cases, I do not think that this
>>> patch will be of any help.
>>>
>>> Furthermore, libata already lowers a link speed automatically at runtime if it
>>> sees too many NCQ errors. Isn't that enough ? And we also have the horkage flags
>>> to force a maximum link speed for a device/adapter, which can also be specified
>>> as a libata module argument (libata.force).
>>>
>>>> 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.
>>>
>>> We already have "/sys/class/ata_link/*/hw_sata_spd_limit", which is read-only
>>> for now. So if you can really justify this manual link speed tuning for an
>>> actual use case (not a hypothetical one), then the way to go would be to make
>>> that attribute RW and implement its store() method to lower the link speed at
>>> runtime.
>>>
>>> And by the way, looking at what that attribute says, I always get:
>>> <unknown>
>>>
>>> So it looks like there is an issue with it that went unnoticed (because no one
>>> is using it...). This needs some fixing.
>>>
>> Hello Damien and Niklas,
>>
>> Thank you for the feedback.
>>
>> I have a hotplug system, where the links behave differently depending
>> on the disk model connected. For some models the kernel emits a lot of
>> bus errors, but mostly not enough errors for it to automatically lower
>> the link speed, except during high workloads. I have not observed any
>> data-loss regarding the errors, but the excessive logging becomes a problem.
>
> It might be interesting to compare the output of:
> $ hdparm -I
>
> for a drive that you can hot plug insert without errors, against a drive
> that gives you errors on hot plug insertion, to see if this can give you
> a hint of why they behave differently.
>
> (e.g. certain features, e.g. DevSleep, is only enabled if there is support
> in the HBA, the port, and the drive.)
>
>
> Kind regards,
> Niklas
Hi Niklas,
I mostly tested on the 6.1 kernel, and it is a quite peculiar hardware
problem, so it isn't caused by anythin in the latest 6.9 rc.
Thank you for the advice of using hdparm, maybe I can diff preset of
features between the models. I will share any interesting findings I
come across.
Best regards
Gustav
prev parent reply other threads:[~2024-04-18 12:47 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-12 13:48 [PATCH] ata: Add sdev attribute to lower link speed in runtime Gustav Ekelund
2024-04-12 16:59 ` Niklas Cassel
2024-04-13 0:29 ` Damien Le Moal
2024-04-15 14:49 ` Gustav Ekelund
2024-04-16 22:59 ` Damien Le Moal
2024-04-17 9:20 ` Gustav Ekelund
2024-04-17 9:56 ` Niklas Cassel
2024-04-17 10:14 ` Niklas Cassel
2024-04-18 12:47 ` Gustav Ekelund [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=d9fe9fc1-1b28-4547-8845-399aaddc0863@axis.com \
--to=gustaek@axis.com \
--cc=cassel@kernel.org \
--cc=dlemoal@kernel.org \
--cc=gustav.ekelund@axis.com \
--cc=hare@suse.de \
--cc=kernel@axis.com \
--cc=linux-ide@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=martin.petersen@oracle.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).