linux-kernel - Re: [PATCH v12 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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAOuDEK3k6Xnev_QUihv+XDMd4YDY5fz+6U7qewo=DSyE7duf_w@mail.gmail.com>
Date: Thu, 15 May 2025 19:28:00 +0800
From: Guan-Yu Lin <guanyulin@...gle.com>
To: Greg KH <gregkh@...uxfoundation.org>
Cc: mathias.nyman@...el.com, stern@...land.harvard.edu, sumit.garg@...nel.org, 
	gargaditya08@...e.com, kekrby@...il.com, jeff.johnson@....qualcomm.com, 
	quic_zijuhu@...cinc.com, andriy.shevchenko@...ux.intel.com, 
	ben@...adent.org.uk, broonie@...nel.org, quic_wcheng@...cinc.com, 
	linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v12 3/4] xhci: sideband: add api to trace sideband usage

On Fri, Apr 25, 2025 at 7:14 PM Greg KH <gregkh@...uxfoundation.org> wrote:
>
> On Wed, Apr 16, 2025 at 02:43:03PM +0000, Guan-Yu Lin wrote:
> > +bool xhci_sideband_check(struct usb_hcd *hcd)
> > +{
> > +     struct usb_device *udev = hcd->self.root_hub;
> > +     bool active;
> > +
> > +     device_lock(&udev->dev);
> > +     active = usb_offload_check(udev);
> > +     device_unlock(&udev->dev);
> > +
> > +     return active;
>
> What happens if the value changes right after reading it?  What are you
> going to do with the value?
>

Currently xhci_sideband_check() is only called when the xhci platform
device is going to suspend. Given that the usb devices should be
either already suspended or being marked as "offload_at_suspend" right
now, it should be safe if we ensure that "offload_usage" doesn't
change at this moment. Let me update
usb_offload_get()/usb_offload_put() to achieve this.

> >
> > +     udev = sb->vdev->udev;
> > +     device_lock(&udev->dev);
> > +     ret = usb_offload_get(udev);
> > +     device_unlock(&udev->dev);
>
> A "raw" call to device_lock/unlock feels rough, and harsh, why doesn't
> the function do that itself?
>
> thanks,
>
> greg k-h

The design is to align with usb_offload_put(). For usb_offload_put(), we
don't need to lock the device when the device state is
USB_STATE_NOTATTACHED. Hence, we put the device_lock()/device_unlock()
outside of usb_offload_get()/usb_offload_put(). Let me also change the
functions to usb_lock_device()/usb_unlock_device() for better coding
style.

Regards,
Guan-Yu