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@xxxxxxxxxxxxxx> >>>>>> --- >>>>>> 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
