Re: [PATCH v5 6/8] usb: misc: onboard_dev: add support for non-hub devices

From: Javier Carrasco <javier.carrasco@wolfvision.net>
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
Subject: Re: [PATCH v5 6/8] usb: misc: onboard_dev: add support for non-hub devices
Date: Thu, 29 Feb 2024 07:38:11 +0100	[thread overview]
Message-ID: <04776f37-92bb-40bf-a3ca-3da4ce3bef88@wolfvision.net> (raw)
In-Reply-To: <Zd-m6WNd5ukXyJGx@google.com>

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