Received-SPF: pass (google.com: domain of linux-kernel+bounces-86185-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <04776f37-92bb-40bf-a3ca-3da4ce3bef88@wolfvision.net>
Date: Thu, 29 Feb 2024 07:38:11 +0100
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v5 6/8] usb: misc: onboard_dev: add support for non-hub
 devices
To: Matthias Kaehlcke <mka@chromium.org>
Cc: Liam Girdwood <lgirdwood@gmail.com>, Mark Brown <broonie@kernel.org>,
 Rob Herring <robh+dt@kernel.org>,
 Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
 Conor Dooley <conor+dt@kernel.org>,
 Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
 Helen Koike <helen.koike@collabora.com>,
 Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
 Maxime Ripard <mripard@kernel.org>, Thomas Zimmermann <tzimmermann@suse.de>,
 David Airlie <airlied@gmail.com>, Daniel Vetter <daniel@ffwll.ch>,
 Catalin Marinas <catalin.marinas@arm.com>, Will Deacon <will@kernel.org>,
 Russell King <linux@armlinux.org.uk>, linux-sound@vger.kernel.org,
 devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
 linux-usb@vger.kernel.org, dri-devel@lists.freedesktop.org,
 linux-arm-kernel@lists.infradead.org
References: <20240228-onboard_xvf3500-v5-0-76b805fd3fe6@wolfvision.net>
 <20240228-onboard_xvf3500-v5-6-76b805fd3fe6@wolfvision.net>
 <Zd93JZTlN4BCxWm7@google.com>
 <ecf303c3-d7a1-49d5-a997-32596215e43d@wolfvision.net>
 <Zd-ahtPpI8zbAYQ9@google.com>
 <63d9be60-40dd-49f6-9a75-72d4be746024@wolfvision.net>
 <Zd-m6WNd5ukXyJGx@google.com>
Content-Language: en-US
From: Javier Carrasco <javier.carrasco@wolfvision.net>
In-Reply-To: <Zd-m6WNd5ukXyJGx@google.com>
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?cklWSEcxcWtSM3NBeG8rbTlOL1ZmT2tvWmsva3dJSVBwMTQ5b0YrTDNEVTMw?=
 =?utf-8?B?Wm8ySHRrenoxakFYNWw0YkpTNnZmT2FSY3RVZEdCQ1RodmlXSEg5eDFNQVow?=
 =?utf-8?B?Tzc4ZG1pMTdjVllPTVpBMXhUZmJnTVBBcjIyK3FQbE9uZERZdEF5QzlCQVFt?=
 =?utf-8?B?S3J0WkMvVmRzY2pZaXFvcnFiY2RjbXZHL3BPRHRmT2RpdzJPbmMrRldqSEow?=
 =?utf-8?B?SE5sZkQ1OWFhMXRyVHNtQ1hvWnVPZy80MFJ5S2tEbVcveG0xYldrTTBJVU9U?=
 =?utf-8?B?VnFYSjJoMnBkcDd6ZFFWcXI2UFc5bVdYM2ZQYTE4bmduSi9qb3lYZWJDTnR3?=
 =?utf-8?B?blJRWVoyM29vamhjcWlBQTUyZ1lOQy9BVlFkRy9kTTBoV044bWdYeTJPdEh5?=
 =?utf-8?B?RnlmdTBhazVKR3d5NGhiK2JZZVlEcTV4TXd1Z1hiczY2SjM0ZDAvMllDUHRi?=
 =?utf-8?B?Z1NoWDhlNTFCNERmSGhKa3dqdTh2cFQ4NkVWY2lBdGhkNHZFdFZNMEFOQXJC?=
 =?utf-8?B?TWlYVU1aWERXM1FQZXk2YlltR0Z3OHVmQ0ZlUnpQZzNLaWNmZVlqMkY2QmZC?=
 =?utf-8?B?L3J6dFh2UXJKemszWUdpMmd5SVZxeWNraG96eGU3ZUMvOTFzYjN1dmhhTTA4?=
 =?utf-8?B?bloySkFsMmxWNHJCaFZUMmVWYnhkRnI5RnBPYVAwMFE4U0g5L2prbDdnWi81?=
 =?utf-8?B?M2UxQkdvWkY5b3ZsQThhUVFLQkMyeCs2N0V1RHBmMDRmdGxjMUkxTi91cm9H?=
 =?utf-8?B?TzFKUHMveGpuRFF5d21GcE1NWEkxQ1JhcXNWQU9LMmRmZWMvRWZKcEJMc2sz?=
 =?utf-8?B?UkxycFpoZ29XUUNXdDhEcDdyNERIYzBCekhUbVJOVmxDazBKd3BTMG1JWlBD?=
 =?utf-8?B?cmtSTmh5c01YUkpUR2JObmNmNXhNR21aQWZEbGs0ZkwzQTFveEN0WHBDclEr?=
 =?utf-8?B?Y0dJTlUyYVdLdU5zcWNzUDJQK2g0Y2ZreURob3pKd0s3SWw0T3JCOHdwYUNU?=
 =?utf-8?B?TkFZTnNqMEdTZ0xHbVZDbXJmbmRLc3M1UGI4MUs1N1ZoeHJ1bXdiNHB5c1RQ?=
 =?utf-8?B?Z05GaE16TWxMOFVQVitFYTN4dzZnaXMrUVRSK2c4TFpnMksrci9iREVTWlZu?=
 =?utf-8?B?MlRHdzJwRzMxWVlISitKS0ozYWkxejRMSzJWRzRYTEgwcE1STlR3YjQ5NDZy?=
 =?utf-8?B?N1c0ci8vb3dlQ3lWcVVRYWN0UVZFMWZIMzhXdFhlOEs0LzdMT2x1RFlMdmE0?=
 =?utf-8?B?cmtxMXUyM00wNmFGcklGOXV2V2xjZ0VmNVVvaGFiRTlSSjNTek1HQzhqSVNW?=
 =?utf-8?B?SlFxUTRUMXlOTVEvNzBxMFRXME9vYzVhdnJITGpPUFJORitQd1ZJcm9IWW5X?=
 =?utf-8?B?a0x3NlJ1WXdrL1NzbVZaYzJMZ1Bpd1dhVTZKV0ZYbjg5anVGVlR1Q3hsR283?=
 =?utf-8?B?U2RUVGszNTdJdk5vd3lrVDJtTWtWQmhJTEdMVTJtWVZtckFiRytqMm1hY0F3?=
 =?utf-8?B?R3FYQjd6dHRlVjl0YlJMb2Q3TXJhUlNjZ2NjVFNEN1dCbHlLNE9Za2hBYkx4?=
 =?utf-8?B?T3kzbzdXWnRVRDZQMGZzNHZ6WXpjYnNyZlRwT1lnOVpSNDJlQUtuRm94cTNU?=
 =?utf-8?B?Wnh0c2ZrLzJ1MjlWS3VRTnVzcUtOeGhHQ0xSemR0OGV5aFdqWlpjN0JwWmNK?=
 =?utf-8?B?anZZdENkWEhIK0c2cTh1TVdBMGdTQnpVYmtENGM4Z0dzTC9GK3hxUWFxU1dL?=
 =?utf-8?B?d29RQVRyeGFsNXdtdXR1TVBMY0gxSjUvbVdlR1YvNnZLZnhsOVAxM2ZKNjFF?=
 =?utf-8?B?Ri9TblNRbjZOUEtHYzhLU3pyWkZXbUV2U2p0NU96bWJsQTVmOHlVVmhVbFFi?=
 =?utf-8?B?OXpuYkRuYW11QXUrV2x0WUh2RHByRXIrZUVuSFp3SEs5aEtjN0Iwak8zWGgw?=
 =?utf-8?B?Q0tGai9WYjFLRlYxOW9oUWRvczFzNWhyOWJudjQ2REpFbk9SV3ZFVmVtbW5F?=
 =?utf-8?B?Nk9oRnc0d2JvK1dnZlM5K3RCRkJYMUhvSjdja2xKVnFkSmlTSDY3N3JsRGJl?=
 =?utf-8?B?cUZVVnQ5bUFSVUdvS3oxZ2RiVDBTcHl0ekhIUDMwa2crNHIwSFZ0eFUrWlh3?=
 =?utf-8?B?Z3BvZDg5cU9WZExoT0V1NTJ3Vmd2d2FMU2xFOU50ZHJQTHJxa0h6OFdzMHo0?=
 =?utf-8?Q?za+tqGJP61fs7/cmEyirZTw=3D?=
X-OriginatorOrg: wolfvision.net
X-MS-Exchange-CrossTenant-Network-Message-Id: c18d1239-a9e7-4755-b2f4-08dc38f100fd
X-MS-Exchange-CrossTenant-AuthSource: VE1PR08MB4974.eurprd08.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 29 Feb 2024 06:38:13.5994
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: e94ec9da-9183-471e-83b3-51baa8eb804f
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: gox6lU75vxQxh16DXKf4Clq43fGErZ8hIOWUMioF6qqqwHuF2y6la1LxpOGGMXZ4Kbebr4N6SSz99JvE6g57C35xFPdZCI2IyZ8ReOzx1NE=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: AS8PR08MB10316

On 28.02.24 22:34, Matthias Kaehlcke wrote:
> On Wed, Feb 28, 2024 at 09:50:22PM +0100, Javier Carrasco wrote:
>> On 28.02.24 21:41, Matthias Kaehlcke wrote:
>>> On Wed, Feb 28, 2024 at 09:21:00PM +0100, Javier Carrasco wrote:
>>>> On 28.02.24 19:10, Matthias Kaehlcke wrote:
>>>>> On Wed, Feb 28, 2024 at 02:51:33PM +0100, Javier Carrasco wrote:
>>>>>> Most of the functionality this driver provides can be used by non-hub
>>>>>> devices as well.
>>>>>>
>>>>>> To account for the hub-specific code, add a flag to the device data
>>>>>> structure and check its value for hub-specific code.
>>>>>>
>>>>>> The 'always_powered_in_supend' attribute is only available for hub
>>>>>> devices, keeping the driver's default behavior for non-hub devices (keep
>>>>>> on in suspend).
>>>>>>
>>>>>> Signed-off-by: Javier Carrasco <javier.carrasco@wolfvision.net>
>>>>>> ---
>>>>>>  drivers/usb/misc/onboard_usb_dev.c | 25 +++++++++++++++++++++++--
>>>>>>  drivers/usb/misc/onboard_usb_dev.h | 10 ++++++++++
>>>>>>  2 files changed, 33 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/usb/misc/onboard_usb_dev.c b/drivers/usb/misc/onboard_usb_dev.c
>>>>>> index e1779bd2d126..df0ed172c7ec 100644
>>>>>> --- a/drivers/usb/misc/onboard_usb_dev.c
>>>>>> +++ b/drivers/usb/misc/onboard_usb_dev.c
>>>>>> @@ -132,7 +132,8 @@ static int __maybe_unused onboard_dev_suspend(struct device *dev)
>>>>>>  	struct usbdev_node *node;
>>>>>>  	bool power_off = true;
>>>>>>  
>>>>>> -	if (onboard_dev->always_powered_in_suspend)
>>>>>> +	if (onboard_dev->always_powered_in_suspend &&
>>>>>> +	    !onboard_dev->pdata->is_hub)
>>>>>>  		return 0;
>>>>>
>>>>> With this non-hub devices would always be powered down, since
>>>>> 'always_powerd_in_suspend' is not set for them. This should be:
>>>>>
>>>>
>>>> May I ask you what you meant in v4 with this comment?
>>>>
>>>>> Even without the sysfs attribute the field 'always_powered_in_suspend'
>>>>> could
>>>>> be set to true by probe() for non-hub devices.
>>>
>>> struct onboard_dev always has the field 'always_powered_in_suspend',
>>> even for non-hubs, that don't have the corresponding sysfs attribute.
>>> Currently it is left uninitialized (i.e. false) for non-hubs. Instead
>>> it could be initialized to true by probe() for non-hubs, which would
>>> be semantically correct. With that it wouldn't be necessary to check
>>> here whether a device is hub, because the field would provide the
>>> necessary information.
>>>
>>
>> That is maybe what is confusing me a bit. Should it not be false for
>> non-hub devices? That property is only meant for hubs, so why should
>> non-hub devices be always powered in suspend? I thought it should always
>> be false for non-hub devices, and configurable for hubs.
> 
> I suspect the confusion stems from the sysfs attribute 'always_powered_...'
> vs. the struct field with the same name.
> 
> The sysfs attribute defaults to 'false', which results in USB devices
> being powered down in suspend. That was the desired behavior for a device
> I was working on when I implemented this driver, but in hindsight I think
> the default should have been 'true'.
> 
> We agreed that non-hub devices shouldn't be powered down in suspend. It
> would be unexpected for users and could have side effects like delays
> or losing status. Since (I think) we can't change the default of the
> attribute we said we'd limit it to hubs, and not create it for other
> types of USB devices. Other USB devices would remain powered during
> system suspend.
> 
> Are we in agreement up to this point, in particular that non-hub
> devices should remain powered?
> 
> struct onboard_dev has the field 'always_powered_...', which in the
> existing code is *always* associated with the sysfs attribute of
> the same name. But there is no reason to not to use the field when
> the sysfs attribute does not exist. For any device at any given time
> the field could indicate whether the device should be remain powered
> during suspend. For hubs the value can be changed at runtime
> through the sysfs attribute, for non-hubs it would be static and
> always indicate that the device should remain powered.
> 
> Does that clarify your doubts?

It is crystal clear now, thank you.

Best regards,
Javier Carrasco