linux-kernel - Re: [PATCH v1 3/3] usb: dwc3: gadget: Make gadget

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: <20250409215151.thkmgzyeohyxhslh@synopsys.com>
Date: Wed, 9 Apr 2025 21:51:54 +0000
From: Thinh Nguyen <Thinh.Nguyen@...opsys.com>
To: Prashanth K <prashanth.k@....qualcomm.com>
CC: Thinh Nguyen <Thinh.Nguyen@...opsys.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Kees Bakker <kees@...erbout.nl>,
        William McVicker <willmcvicker@...gle.com>,
        Marek Szyprowski <m.szyprowski@...sung.com>,
        "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "stable@...nel.org" <stable@...nel.org>
Subject: Re: [PATCH v1 3/3] usb: dwc3: gadget: Make gadget_wakeup asynchronous

On Wed, Apr 09, 2025, Prashanth K wrote:
> 
> 
> On 09-04-25 06:13 am, Thinh Nguyen wrote:
> > On Tue, Apr 08, 2025, Prashanth K wrote:
> >>
> >>
> >> On 08-04-25 05:08 am, Thinh Nguyen wrote:
> >>> On Thu, Apr 03, 2025, Prashanth K wrote:
> > 
> >>>> @@ -4398,6 +4371,21 @@ static void dwc3_gadget_linksts_change_interrupt(struct dwc3 *dwc,
> >>>>  	}
> >>>>  
> >>>>  	dwc->link_state = next;
> >>>> +
> >>>> +	/* Proceed with func wakeup if any interfaces that has requested */
> >>>> +	while (dwc->func_wakeup_pending && (next == DWC3_LINK_STATE_U0)) {
> >>>> +		if (dwc->func_wakeup_pending & BIT(0)) {
> >>>> +			ret = dwc3_send_gadget_generic_command(dwc, DWC3_DGCMD_DEV_NOTIFICATION,
> >>>> +							       DWC3_DGCMDPAR_DN_FUNC_WAKE |
> >>>> +							       DWC3_DGCMDPAR_INTF_SEL(intf_id));
> >>>> +			if (ret)
> >>>> +				dev_err(dwc->dev, "function remote wakeup failed for %d, ret:%d\n",
> >>>> +					intf_id, ret);
> >>>> +		}
> >>>> +		dwc->func_wakeup_pending >>= 1;
> >>>
> >>> This would break the bitmap of dwc->func_wakeup_pending. Perhaps we can
> >>> use ffs(x) to properly find and clear the interface ID from the bitmap
> >>> one at a time.
> >>>
> >> Yes, we can use ffs(x). But I didn't understand how this would break
> >> bitmap of dwc->func_wakeup_pending.
> >>
> > 
> > Since you're sending device notification to all the wakeup armed
> > interfaces and not reusing the func_wakeup_pending afterward, the result
> > of what you're doing here will be the same.
> > 
> > IMHO, for clarity, what I was suggesting is to revise the logic to
> > preserve the dwc->func_wakeup_pending bitmap instead of shifting and
> > overwriting it, causing the bitmap to no longer match the intf_id. We
> > get the intf_id from the bitmap and clear the intf_id bit from the
> > bitmap as we go.
> > 
> The above logic works, but as you suggested I'll refactor it using
> ffs(x) and clear the intf_id directly (instead of shifting).
> 
> Does this look good?

It looks great!

> 
> /* Proceed with func wakeup if any interfaces that has requested */
> while (dwc->func_wakeup_pending && (next == DWC3_LINK_STATE_U0)) {
> 	intf_id = ffs(dwc->func_wakeup_pending);
> 	if (intf_id)
> 		ret = dwc3_send_gadget_generic_command(dwc, DWC3_DGCMD_DEV_NOTIFICATION,
> 							   DWC3_DGCMDPAR_DN_FUNC_WAKE |
> 							   DWC3_DGCMDPAR_INTF_SEL(intf_id - 1));
> 		if (ret)
> 			dev_err(dwc->dev, "function remote wakeup failed for %d, ret:%d\n",
> 				intf_id, ret);
> 	}
> 	dwc->func_wakeup_pending &= ~(1 << (intf_id - 1));


Some minor revision. How does this look? (not tested)

while (dwc->func_wakeup_pending && (next == DWC3_LINK_STATE_U0)) {
	intf_id = ffs(dwc->func_wakeup_pending) - 1;
	ret = dwc3_send_gadget_generic_command(dwc, DWC3_DGCMD_DEV_NOTIFICATION,
					       DWC3_DGCMDPAR_DN_FUNC_WAKE |
			                       DWC3_DGCMDPAR_INTF_SEL(intf_id));
	if (ret)
		dev_err(dwc->dev, "Failed to send DN wake for intf %d\n", intf_id);

	dwc->func_wakeup_pending &= ~BIT(intf_id);
}

Thanks,
Thinh