From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 27832C5478C for ; Wed, 28 Feb 2024 21:34:46 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=xmqERhz59c9+IOWiEoKajAbRPfON8X39p3SPR6kry34=; b=IqgCNL/rPkKiLs MxTKOnVv3DTvgnV8UnNNHQZv5x/YjiNyDOrDiSLI2gA361B5KCn0lP988DvxEyCHfMRknofpq7Spm 14OroW3x0E05owPErBa2mdtdaZP5wZF4Cp68exegtNdVj5tVLaOxAaH3DH7s7ikVBFWNokDSj7X6d MKTy7/JzNShDbRW479iyWqXH9Qr2BRusoyhx1H1N7TzzIQfO6V2X2XR2IBKntBLEJ87f4YqGjamNm dyeciKXmV9aOJIgWdkzobz5vc6DpWLBcXJKKtCMC4vE8wiH/ghpuUIgwGrd7i5Izn0Gw3DIst3+YJ Bb9oftTBxdZlDaW7C/Iw==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1rfRZV-0000000B05I-2j3v; Wed, 28 Feb 2024 21:34:37 +0000 Received: from mail-il1-x134.google.com ([2607:f8b0:4864:20::134]) by bombadil.infradead.org with esmtps (Exim 4.97.1 #2 (Red Hat Linux)) id 1rfRZT-0000000B04G-1kIy for linux-arm-kernel@lists.infradead.org; Wed, 28 Feb 2024 21:34:36 +0000 Received: by mail-il1-x134.google.com with SMTP id e9e14a558f8ab-36503a4980aso958785ab.3 for ; Wed, 28 Feb 2024 13:34:34 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; t=1709156074; x=1709760874; darn=lists.infradead.org; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=Ud3jsbNEeuU2tmgvB1e9X78Ey55SQRcSwnTZ1T/aebs=; b=GXiYExFubsktaGi+rr2lgCft5982rwyn81KYpwdyUSgVgb6lETKVULFhVUjgIgo0R0 TqW9fnRzBDWCikPqB6Tmizpwwl2rKkvGhBq8jTy5HxqpAuDMTNe4A9MAr0vU7oHcdMHR MTjYTDC6JkHeEctVBB9klzn7Dz4h1bxa9WoeQ= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1709156074; x=1709760874; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=Ud3jsbNEeuU2tmgvB1e9X78Ey55SQRcSwnTZ1T/aebs=; b=NybopZ7NKBw7kluSYDJQBRXXT+WPrC8CI0x4ZIvJ2Y53579plOwYU5F4S1VFzGgnym eEKSVi6nVZO2047R5SZa1NODPUC20nd80Vcac3fczlyAAN0l1v+DN5/7h+AHafZNEIWN keRg3u7P/puZi4OpW8onsWEh8SgzR4SuC2Rjz7e3Wssn5LZy8RfKoAhgqaM0VgaeSnKc q1pBowDuAJa1hov9LmN10e6wBm2PejfADv/MGoZXHT1Jmxi7HZs30QajRvzdf1nO2afM kk9gyNIsYfeiIPEIUKlLKnSAAYSvf6Sdf4jEjvvirNvqy4k8Pi38bye3fstK4sp10L/I lZTg== X-Forwarded-Encrypted: i=1; AJvYcCU1RbfHj2k9twuWiSHLK9/o9waxtz+MuEUEFpmgsmBknM7VdPjyHoLoOlz99F1gkhpDztgEkm4+hjT0CrLuy53K03wQjk2H2ZKRj9HIpb41BqltexM= X-Gm-Message-State: AOJu0Yywqi6scaaFWt0vwCiOu4x2ueQMplYh/1T7t8vmci7UN6AgNPHt ndhR4Utu791WSfGvVobIP1Owgni/lqNSR94Gtp3dBmAzyfy7flq5pNvC3pGLGw== X-Google-Smtp-Source: AGHT+IFRn4MDh7nvzu0eSOUCvjYdM8/HpCDcsTdPwQHR4UeGWg9wLpczoWIYWROWR4Qj4A2q46iIUg== X-Received: by 2002:a05:6e02:524:b0:365:1749:cadf with SMTP id h4-20020a056e02052400b003651749cadfmr613056ils.16.1709156074288; Wed, 28 Feb 2024 13:34:34 -0800 (PST) Received: from localhost (144.57.222.35.bc.googleusercontent.com. [35.222.57.144]) by smtp.gmail.com with UTF8SMTPSA id e4-20020a92de44000000b00365760eb777sm68026ilr.81.2024.02.28.13.34.33 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 28 Feb 2024 13:34:33 -0800 (PST) Date: Wed, 28 Feb 2024 21:34:33 +0000 From: Matthias Kaehlcke To: Javier Carrasco Cc: Liam Girdwood , Mark Brown , Rob Herring , Krzysztof Kozlowski , Conor Dooley , Greg Kroah-Hartman , Helen Koike , Maarten Lankhorst , Maxime Ripard , Thomas Zimmermann , David Airlie , Daniel Vetter , Catalin Marinas , Will Deacon , Russell King , 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 Message-ID: References: <20240228-onboard_xvf3500-v5-0-76b805fd3fe6@wolfvision.net> <20240228-onboard_xvf3500-v5-6-76b805fd3fe6@wolfvision.net> <63d9be60-40dd-49f6-9a75-72d4be746024@wolfvision.net> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <63d9be60-40dd-49f6-9a75-72d4be746024@wolfvision.net> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240228_133435_480908_56EFA4B9 X-CRM114-Status: GOOD ( 37.94 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org 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 > >>>> --- > >>>> 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? _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel