Received-SPF: pass (google.com: domain of linux-kernel+bounces-148220-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <c12cf6a4-fd03-4049-8302-d798887d7cee@axis.com>
Date: Wed, 17 Apr 2024 11:20:48 +0200
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] ata: Add sdev attribute to lower link speed in runtime
To: Damien Le Moal <dlemoal@kernel.org>,
 Gustav Ekelund <gustav.ekelund@axis.com>, cassel@kernel.org, hare@suse.de,
 martin.petersen@oracle.com
Cc: linux-ide@vger.kernel.org, linux-kernel@vger.kernel.org, kernel@axis.com
References: <20240412134838.788502-1-gustav.ekelund@axis.com>
 <4e5c88f1-1b24-4f6d-8c11-d7029329ba7a@kernel.org>
 <7e6eb387-5a0e-460c-af08-eff070fa35ca@axis.com>
 <898497f0-d279-4d01-be8d-aad4048df95d@kernel.org>
Content-Language: en-US
From: Gustav Ekelund <gustaek@axis.com>
In-Reply-To: <898497f0-d279-4d01-be8d-aad4048df95d@kernel.org>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?utf-8?B?aGpjaVJlOXBPUXBCYkR6b0Y2REpYbERkMGJMM2FhS2ZGS0hHbG1mRHJCbEV5?=
 =?utf-8?B?azBSL2ZpQXY1b0EydG1ZTmxKaW9aYmorN01QMERxTEwwMlFMY3FpSGtBc1Ur?=
 =?utf-8?B?bER2RHZ1S3JFZGVpZUVxSDFqT29iUnRORGNBYlFEaXJBYUVlcDkwaUw3T2cx?=
 =?utf-8?B?Ly82ZmR1VDg0WUM0T0dhdUVRa1hvQyt4RU5PQVUvU1NybG81K0NyK0I2Nms3?=
 =?utf-8?B?Z2s2YXdkdDExNCsrWEtKRFF6ZnRKQW43Sm45T1pwT1JINFBkYmxKaXRwZXRr?=
 =?utf-8?B?SGZnNFRQSGNYVjJDM09TS3Y2cjdwS3F4dTFZLzJUOTlpRkgxZHgrOHBiRkF4?=
 =?utf-8?B?dEdiTUtJakViTUliMnY5aVF4RmNiTUhqbzJqLzFvdzZSUEsxVU5hTXlxaExv?=
 =?utf-8?B?Y003UjBJQnp4M3dYNkdKOStnTjUxbWt2SnEyM29sRC9PT0pUNDFGelNjWWtK?=
 =?utf-8?B?cndpV0RJemhobktvTThsUnVFUjRDeDdtbDZNMENyckMzcTNvWUJObm1kZ3Rs?=
 =?utf-8?B?NkZqMDhtQmZIK3pLVnVNUk12dUJ5WUVzYTB0cTlJakJpSG5aQXRKSmdEckw2?=
 =?utf-8?B?U1oyS3pFZ0Zoc1k1U0ptT1QzUk9CWmlTbU5BcGRqUVMySlh3SzlrOU9zdllL?=
 =?utf-8?B?ZURyUEYvaGFLQkFhaGErQ3hIdHJzOVBVbjM5RUJXWjI1YW5sNUNLTnpJMUR4?=
 =?utf-8?B?eHVKd2YzRkk3UWZjSGFDKzVEei8rc0V0Z1M5amYzelByZkp2andwYmNBTGdQ?=
 =?utf-8?B?N1FTN1dyR2lLRFJoWW9TSFhPQTFTeFpYSFA0R282bnl6VFpQaXUxL2wwSEJO?=
 =?utf-8?B?c1BqNGVuZTFabW1rb2tIUFRvcjQrTTNwWiszWW1hbnpVVXE4ckdOZjhkV1Nq?=
 =?utf-8?B?c05WTmdickFVaFhmRXBmR0x1d1dRTVZ5OHRkeUFMeitCa2pseS96MmtRMU05?=
 =?utf-8?B?Q25jZHR5Nmk4bXVJRFV4bXNSc1pLUEFRRFkxcEszZVdwZlMrYmVmamNVK0g2?=
 =?utf-8?B?NWcrMHRMWFB2L2lrbkRtZmFYdEw3bjlmVzZZV0VKNGU0Q2RrTE9zbFRaK0k0?=
 =?utf-8?B?ZW51WkdCaGJzVkNPSEtVN2lKeFdZWWNUYzdrQTBTbTlTUFpWbXFjZnFNRWxP?=
 =?utf-8?B?UDIwdVFvWUxpQ1N6SUNtMmdkeVp6OUFyNTd5L1BTYktQV0RQamdteXRMU1lN?=
 =?utf-8?B?NmhKclUzRGtrcFQwRVVUUjRybUsrQzZidmNqcG1VNWt6cHNuZ25XODQ0YVo5?=
 =?utf-8?B?QTJmenk3V3dCTGZ3UzNNR21hUHdqZjlZUFhHOXZBTndGUi9zajA5WVd0Zlkw?=
 =?utf-8?B?TnpRZzZmNU1QNDh0WGROKzB4RjJqZ0FzRk1OQWpmUmJCQ1ViKytjajFVZVZR?=
 =?utf-8?B?bVA3K2c1K2Rvcis2c1MzWk51K0EvL0FsK3k5SGE2RVdoQ243U0dBTzF5bW9i?=
 =?utf-8?B?cXRGMFpDeStwRlVXdDVuSlFJMHNvQ1pqcG1WTHhOdEwxQUc5RS9QeUZMWGla?=
 =?utf-8?B?VnFWd29hRUhNOHVTaXlRbWtXdkgvQTBnL2hiY2lCL2pYMHdQZlhZT2ZGM05v?=
 =?utf-8?B?WjBBZUwxRWQ4TVZXUmxSVisvbjN5Vms1b3V6c2tYcUhZMnBXQm5nWGZ1MkpV?=
 =?utf-8?B?dFc3WWRlbVlkM0tORWpYZk95bTRDRWMvTjBUUExhQXdWaUZDbm5INThheWxz?=
 =?utf-8?B?empQdlAyNHFNZXNURFN6bVJFcnNrb0g5QjgyR3BWSUNaNDlKRUpqNzYxVExB?=
 =?utf-8?B?bTR1aGd2dm1FVitna3dPWUdvTE0vZGlIdkN2WndSQ3pqcWRWVWl4ZSsvRXhI?=
 =?utf-8?B?Qk5hWSt1bGZLNXA0MVZjbDMzYXBwaFZJcTYzUFB2WnlJREVvMEE1YWMzdEZk?=
 =?utf-8?B?amp1cW55MER4NFY2Qndlam1ab09OZHFhVWJYSDFXVWlOSTJVRFY0UFp1enhq?=
 =?utf-8?B?NTQ4dnYxOHkwOGppbnFmdXRmODA3VWJtSkRkUVRXNjlEMUJicjlYelIvM3Bv?=
 =?utf-8?B?UWxSY2dFS3lRa0g0ZGJ4QXdZTjhlZHlNRFdsTEN4NHBnT2l2RGFWT2s1MXEv?=
 =?utf-8?B?VmFRSEp2VFhIVkkzRE9ZNzRQdllGU2ZsR2FOS2N2T0k1bGR2b2ZqaWJIS2lI?=
 =?utf-8?Q?2wQM=3D?=
X-OriginatorOrg: axis.com
X-MS-Exchange-CrossTenant-Network-Message-Id: ad2c2b70-e437-469c-503c-08dc5ebfabe1
X-MS-Exchange-CrossTenant-AuthSource: GV1PR02MB10909.eurprd02.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 17 Apr 2024 09:20:49.6815
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 78703d3c-b907-432f-b066-88f7af9ca3af
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: s35NC+j0HTPJdgrq7AvOyeouxLZXG7tfVDQ6J/0QbvXgxQWzw5qJ5ZvehD+chgVV9kywo9K/tDwjLe7ZIK1uug==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: AS2PR02MB10082

On 4/17/24 00:59, Damien Le Moal wrote:
> On 2024/04/16 0:49, 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.
> 
> Hot-plugging should not be an issue in itself. When hot-plugged, the port
> scanning process should detect the maximum link speed supported by your device
> and use that speed for probing the device itself (IDENTIFY etc). If you see bus
> errors, then you are either having hardware issues (e.g. a bad cable or power
> supply) or some issues with your AHCI controller that may need patching.
> 
> Can you send examples of the errors you are seeing ? That needs to be
> investigated first before going the (drastic) route of allowing to manually
> lower link speed at run-time.
> 
>>
>> So I want to adapt the link, depending on the connected model, in a
>> running system because I know that some particular models in this case
>> will operate better in SATA2 in this system.
>>
>> Can I use the libata.force module to make changes to a particular link
>> in runtime?
> 
> Nope, libata.force is a module parameter so you can specify it as a kernel boot
> parameter, or if you compile libata as a module when loading (modprobe) libata.
> At run time, you need to rmmod+modprobe again libata, and so the ahci driver as
> well (because of dependencies).
> 
> As I mentioned, if a run-time knob really is necessary (it should not be), using
> the ata_link hw_sata_spd_limit would be a better approach. But again, that
> really should not be necessary at all.
> 
>>
>> Best regards
>> Gustav
>>
> 
Hi Damien,

Unfortunately doing this selectively per link from user space seems to
be my only way forward for this particular hardware issue. I understand
if you deem this to be too specific to fit into the upstream kernel.

I will investigate if running libata as a module might be a way around
this peculiar problem. If I need to refine my first approach, I will
attempt to modify the hw_sata_spd_limit to be rw.

Thank you Damien and Niklas.

Best regards
Gustav