linux-kernel - Re: [PATCH v15 3/4] xhci: sideband: add api to trace sideband usage

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAOuDEK3Zv0qErDfCaRX_AH3buht4hP3XnuF3+T6-3aLw1_a2Ag@mail.gmail.com>
Date: Thu, 11 Sep 2025 12:13:00 +0800
From: Guan-Yu Lin <guanyulin@...gle.com>
To: Greg KH <gregkh@...uxfoundation.org>
Cc: mathias.nyman@...el.com, hannelotta@...il.com, zijun.hu@....qualcomm.com, 
	xu.yang_2@....com, stern@...land.harvard.edu, 
	andriy.shevchenko@...ux.intel.com, ben@...adent.org.uk, 
	quic_wcheng@...cinc.com, krzysztof.kozlowski@...aro.org, 
	dh10.jung@...sung.com, linux-usb@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v15 3/4] xhci: sideband: add api to trace sideband usage

On Sat, Sep 6, 2025 at 9:11 PM Greg KH <gregkh@...uxfoundation.org> wrote:
>
> On Tue, Aug 26, 2025 at 12:37:00PM +0800, Guan-Yu Lin wrote:
> > On Wed, Aug 13, 2025 at 10:51 PM Greg KH <gregkh@...uxfoundation.org> wrote:
> > >
> > > On Fri, Aug 01, 2025 at 03:39:32AM +0000, Guan-Yu Lin wrote:
> > > > +config USB_XHCI_SIDEBAND_SUSPEND
> > >
> > > Again, why is a new config option needed here?  Why can't we just use
> > > the existing one?  Why would you ever want one and not the other?
> > >
> > USB_XHCI_SIDEBAND focuses on the normal use case where the system is
> > active, while USB_XHCI_SIDEBAND_SUSPEND enables the sideband during
> > system sleep (Suspend-to-RAM). The design allows the user to determine
> > the degree of support for the sideband in the system. We could add the
> > "select" keyword to automatically enable USB_XHCI_SIDEBAND once
> > USB_XHCI_SIDEBAND_SUSPEND is enabled.
>
> But why would you want only one of these options and not both?  The
> whole goal of this feature is for power savings, which means that
> suspend is needed by everyone.  Don't increase the config variable
> combinations for no good reason.
>

Thanks for the suggestions. I'll remove USB_XHCI_SIDEBAND_SUSPEND in
the next version.

> > > > +EXPORT_SYMBOL_GPL(xhci_sideband_check);
> > > > +#endif /* IS_ENABLED(CONFIG_USB_XHCI_SIDEBAND_SUSPEND) */
> > >
> > > #ifdef in .c files is generally not a good idea, is it really needed
> > > here?  Maybe it is, it's hard to unwind...
> > >
> > > thanks,
> > >
> > > greg k-h
> >
> > Given that CONFIG_USB_XHCI_SIDEBAND_SUSPEND depends on
> > CONFIG_USB_XHCI_SIDEBAND and adds only a single function, I think it
> > is preferable to place the new code in the same file. This approach
> > prevents unnecessary code fragmentation and improves maintainability
> > by keeping related functions together.
>
> We put #ifdefs in .h files.  That's a long-time-rule for decades to
> ensure that we can maintain this codebase for even more decades to come.
> Please do not break that rule just to keep things close if it's not
> required.
>
> thanks,
>
> greg k-h

Thanks for the suggestions. This concern would be addressed once we
use only CONFIG_USB_XHCI_SIDEBAND to control all features.

Regards,
Guan-Yu