Received-SPF: pass (google.com: domain of linux-kernel+bounces-150095-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <d9fe9fc1-1b28-4547-8845-399aaddc0863@axis.com>
Date: Thu, 18 Apr 2024 14:47:20 +0200
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] ata: Add sdev attribute to lower link speed in runtime
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
References: <20240412134838.788502-1-gustav.ekelund@axis.com>
 <4e5c88f1-1b24-4f6d-8c11-d7029329ba7a@kernel.org>
 <7e6eb387-5a0e-460c-af08-eff070fa35ca@axis.com> <Zh-hASYS4XkyNJc9@ryzen>
Content-Language: en-US
From: Gustav Ekelund <gustaek@axis.com>
In-Reply-To: <Zh-hASYS4XkyNJc9@ryzen>
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?NTE3RXYxbEt1VzBmQjdlOFloUTFwdDlUNDZzQ2VkTEhidlVQa0VvQmFJK1M5?=
 =?utf-8?B?RVZKY20xMVh0Q1lpWndTUVROV2E5WDV2TUFxbU15elU1L0FEd2dieDBHWHlV?=
 =?utf-8?B?MDlET2tTRE1Kcmd5dEwwQVNhQnlMaFl3TDJBQ0YxOG11MUtxSFZRcGNLWVZQ?=
 =?utf-8?B?SnFvMzQ1Wm9aN1czbHlzQjJQUEpBS2FmdGRBMEN6WHRYVDNMZFRFbG9velln?=
 =?utf-8?B?Yk9WUSszalBJQnJqa2VDUkxGU0JUZWVodmxvdkdFV1QyMWxOOW5DWUlZVjcy?=
 =?utf-8?B?K1BKdFVSQ1RMVktvNnVKbGNzT0VRTlB1bk1kcjJtbFh1SVJoRDhtcVFvYkpN?=
 =?utf-8?B?YklDOTk4ZGZiQmZxejhFYm1zTWY5anFVMmF1ODhzR3dScWdPYWREOUY5a2c3?=
 =?utf-8?B?Y1RVaStBNzRlNmZnU0I2Q2tJdFBJZmxpdjBMVVhlcG40TEtKOGRaVXk1djlX?=
 =?utf-8?B?ZlJFWjJQNXdUT014Unh2aVpkem9xREpRSlhvanBnaUp6bG5HeHlWRXlFYTZa?=
 =?utf-8?B?S2tWM3o2MW4rUi9HSG43R1lPVG5TSElRVTRnNjA0bWVWeGhkYXFJNytLQkpJ?=
 =?utf-8?B?Tk14Y3p2c1p5SUdaU2JOVzR1MjUvY3V1aGo3VFR3SEQ4WjhXYWdHc3NBa0dW?=
 =?utf-8?B?UTBBNGM0UCs0M2RiUXhwYXBHSkFUWkk4SW9XUDgxeXdFaURPTFkxbm9aL1Vw?=
 =?utf-8?B?T2VUb1ljTlRjczVTQkpQZ0ZSL2s3UVpNOVpoVjN5VENzTUNLK0g0Smg2Vjgv?=
 =?utf-8?B?eStIODQzYjA2b0dhMlIwRml3Mk5qZG5RdHpFbUJ3emxpK3N2Ni9WcDBmZE1o?=
 =?utf-8?B?NUYrcE5vV2VuYTFpRGJNeGZ2UTBDQWk0cEFrYk5meVZBeXJKdlNMaUVYTCtD?=
 =?utf-8?B?b2RCNmtuTGZBNk5saDBTelZSNDlKSjFPaDVkclEzdHYvN3RmTjRYV1JpcE84?=
 =?utf-8?B?dG1CaHhGeUc0RExpa3RVT0YwcGp1Z3d3V29zYU9NSVhkd1NhMnNyRXM5cUR0?=
 =?utf-8?B?K3drekxmTlc0cHRlTTQ5TDY2cldiQjNWVy9vMUNVcGFjdWpEZlhoNFRQdnVR?=
 =?utf-8?B?MXlpeXdzZWxuTTd2MXVUVmhQc29YK3QzRVgvSHc5a3VzcEdjSjdrTW1lSC9T?=
 =?utf-8?B?SkVLajMyTVNyODJ4aGFOYWVSOXRBNUF4b3V4WGdxRVpVWWIyM2pmd1UwcENX?=
 =?utf-8?B?QlFBa3FWdUNMNDYrNGw3N1pvUXBudFBKWTdHVjhOQ0ljSGtiWVhGNUFQaWlC?=
 =?utf-8?B?SHE1TmxqTllWQ3FybFh5eHZPemg4b2dxSFFSVmFZNWt4cUJOQ0o4dkN6OGEx?=
 =?utf-8?B?Qk5pWWlRSi9xQ24yeFNBMnBFSE1zMVVLS1EvQmZWaUREdDNRbVd0cncyQUs3?=
 =?utf-8?B?M2lIV0tiUFhVb2RkTXZsMjIxUXJ4RnZ1UzN0RVhjN1V4cThjOTZoLyt3V0x4?=
 =?utf-8?B?OFZrOGpmSFF2MHRGbExiYjVYQ3l1Q1VGSnE3UDI3Rkx4QXhTVjZaSTIzMXBt?=
 =?utf-8?B?YzVFZGJvNkR3c3ZybzIzRklQSVM3VDMyV3QydWhiRjFBNEgxQWczcEtTRHd4?=
 =?utf-8?B?QmdWcUxGY0tiajI1OVFwUExhVm1vL1E5emF1L0FIR3JYd3FNZXJxVFZaejgz?=
 =?utf-8?B?b2h2UWFmN0tYTW5sdS9ERFhZMGJMb0VTaHBJc1dESTRzS1pTZkFQSHhYVTVF?=
 =?utf-8?B?VEZTYzE3dXlZNTVWQ20wdW5sR3Y5SGE3SmN1V0FhLzh5eVd3eHk2c1llbzVM?=
 =?utf-8?B?cjRxVG9ZaVRSMjNoTFh6TUx6ZlZYTnVRcThsMzAzZ3AyVEp0emtWbGZBUzBu?=
 =?utf-8?B?TGhocmI3OER3SFdjOWl0Qlc0WnR2V0gwek00TzhuZXllOEkvYnVSRlc2WkRs?=
 =?utf-8?B?NlREMWdveWRYVWJuRlZWNWJCRXVmTzlPbTlDbGRZRmwxSXZ3dHU4dVZ4QjV5?=
 =?utf-8?B?aFZSVjFRa3RsUy84c21vc1dCUWxpQWJUaDdzdnozbndGS1dUYmJBVGlCR3RN?=
 =?utf-8?B?TEo0a0NMTWVkc200aDA0Qy9wNXpJNnE0RGhEWEFiQnY5N3dtWm8ydEVUN1Fx?=
 =?utf-8?B?NWtPWFZRLzllMWorMUxFZTRrWDI5ZzlhcVIxMWE3WXIrRW5ldXByZ1BXSGpE?=
 =?utf-8?Q?/UBs=3D?=
X-OriginatorOrg: axis.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 656e0f66-9920-43c2-5b3b-08dc5fa5b157
X-MS-Exchange-CrossTenant-AuthSource: GV1PR02MB10909.eurprd02.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 18 Apr 2024 12:47:23.0939
 (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: j0LPm3gkDHPa4v3I5tNzonNx032IwibYHSthEcLLG2WV5l5WdcFSs/7gahHb84TwspUBfjCfuuMDG7apC7Hz0Q==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PRAPR02MB7836

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