Re: [PATCH RESEND] usb: octeon-hcd: keep SOF interrupts while a periodic pipe is due

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] usb: octeon-hcd: periodic pipe starvation due to bypassed due_now evaluation
- [High] usb: octeon-hcd: control/bulk split transfers stall with SOF disabled
--

commit ba5443b476ec252047a7c43f65661c3131a5d94c
Author: Orgad Shaneh <orgads@gmail.com>

usb: octeon-hcd: keep SOF interrupts while a periodic pipe is due

This commit modifies cvmx_usb_schedule() to track when a periodic pipe is
due but not yet started, setting a due_now flag. This flag is used to keep
SOF interrupts enabled instead of sleeping on an hrtimer, ensuring due pipes
are retried promptly.

Link: https://lore.kernel.org/all/20260915081724.25637-1-orgads@gmail.com/

> diff --git a/drivers/usb/host/octeon-hcd.c b/drivers/usb/host/octeon-hcd.c
> index 256e459ff6d8d..197e04317a264 100644
> --- a/drivers/usb/host/octeon-hcd.c
> +++ b/drivers/usb/host/octeon-hcd.c
> @@ -1964,12 +1965,23 @@ static void cvmx_usb_schedule(struct octeon_hcd *usb, int is_sof)
>  	 */
>  	need_sof = 0;
>  	min_due = ~0ull;
> +	due_now = false;
>  	for (ttype = CVMX_USB_TRANSFER_CONTROL;
>  	     ttype <= CVMX_USB_TRANSFER_INTERRUPT; ttype++) {
>  		list_for_each_entry(pipe, &usb->active_pipes[ttype], node) {
> -			if (pipe->next_tx_frame > usb->frame_number &&
> -			    pipe->next_tx_frame < min_due)
> +			if (pipe->next_tx_frame <= usb->frame_number) {
> +				/*
> +				 * A periodic pipe that is due but was not
> +				 * started - no idle channel, or its split
> +				 * window is closed - is only retried from a
> +				 * SOF, so do not sleep past it.
> +				 */
> +				if (ttype == CVMX_USB_TRANSFER_ISOCHRONOUS ||
> +				    ttype == CVMX_USB_TRANSFER_INTERRUPT)
> +					due_now = true;

[Severity: High]
Can this explicit exclusion of control and bulk pipes cause SPLIT COMPLETE
transactions to stall indefinitely if SOF interrupts are disabled?

If a low/full-speed device is connected via a high-speed hub using split
transactions, a control or bulk transfer initiates a START SPLIT and
completes with an ACK. The hardware channel is disabled and the transaction
advances to CVMX_USB_STAGE_NON_CONTROL_SPLIT_COMPLETE, but its
next_tx_frame is not advanced.

The pipe must wait until usb->frame_number reaches split_sc_frame:

cvmx_usb_find_ready_pipe() {
    ...
    ((pipe->split_sc_frame == -1) ||
     ((((int)current_frame - pipe->split_sc_frame) & 0x7f) < 0x40)) &&
    ...
}

Since control and bulk pipes skip setting due_now and their next_tx_frame
remains in the past (so they don't update min_due), need_sof will be 0 if
no other periodic pipes are active. Does this mean usb->frame_number stops
updating, permanently stalling the pipe until an unrelated interrupt fires?

> +			} else if (pipe->next_tx_frame < min_due) {
>  				min_due = pipe->next_tx_frame;
> +			}
>  		}
>  	}
>  	if (min_due != ~0ull) {

[Severity: High]
Will the due_now flag be completely ignored if there are only due pipes
and no future periodic deadlines exist?

In the loop above, if a pipe is due (pipe->next_tx_frame <= usb->frame_number),
due_now is set to true but min_due is not updated.

If there are no other active periodic pipes with a future deadline, min_due
remains ~0ull. The code then completely bypasses this entire if
(min_due != ~0ull) block.

As a result, need_sof remains 0 and the timer is not armed. Since periodic
pipes are only retried when is_sof is true, does this result in a permanent
starvation of the unstarted due periodic pipes?

> @@ -1983,7 +1995,7 @@ static void cvmx_usb_schedule(struct octeon_hcd *usb, int is_sof)
>  		 * a few frames. Stay well below the 16383-frame wrap of
>  		 * HFNUM. One (micro)frame is 125us in high-speed mode.
>  		 */
> -		if (delta <= 4 || delta > 8000)
> +		if (due_now || delta <= 4 || delta > 8000)
>  			need_sof = 1;
>  		else
>  			hrtimer_start(&usb->sof_timer,

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261011072236.2381546-1-orgads@gmail.com?part=1