linux-kernel - Re: [PATCH v4 1/1] xhci: Correctly handle last TRB of isoc TD on Etron xHCI host

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: <CAHN5xi1HoTHx5bye6v24eRWzuKLXcyp6zc4wVpYDyHcR4yu99A@mail.gmail.com>
Date: Fri, 7 Feb 2025 14:59:25 +0800
From: Kuangyi Chiang <ki.chiang65@...il.com>
To: Michał Pecio <michal.pecio@...il.com>
Cc: gregkh@...uxfoundation.org, linux-kernel@...r.kernel.org, 
	linux-usb@...r.kernel.org, mathias.nyman@...el.com, stable@...r.kernel.org
Subject: Re: [PATCH v4 1/1] xhci: Correctly handle last TRB of isoc TD on
 Etron xHCI host

Hi,

Thank you for the review.

Michał Pecio <michal.pecio@...il.com> 於 2025年2月6日 週四 上午5:45寫道：
>
> >       case COMP_STOPPED:
> > +             /* Think of it as the final event if TD had an error */
> > +             if (td->error_mid_td)
> > +                     td->error_mid_td = false;
> >               sum_trbs_for_length = true;
> >               break;
>
> What was the reason for this part?

To prevent the driver from printing the following debug message twice:

"Error mid isoc TD, wait for final completion event"

This can happen if the driver queues a Stop Endpoint command after
mid isoc TD error occurred, see my debug messages below:

[  110.514149] xhci_hcd 0000:01:00.0: xhci_queue_isoc_tx
[  110.514164] xhci_hcd 0000:01:00.0: @000000002d119510 59600000
00000000 00009000 800b1725
[  110.514169] xhci_hcd 0000:01:00.0: @000000002d119520 59609000
00000000 00107000 800b1515
[  110.514173] xhci_hcd 0000:01:00.0: @000000002d119530 59610000
00000000 00002000 00000625
...
[  110.530263] xhci_hcd 0000:01:00.0: xhci_handle_event_trb
[  110.530266] xhci_hcd 0000:01:00.0: @000000010afe6350 2d119510
00000000 04009000 01138001
[  110.530271] xhci_hcd 0000:01:00.0: Error mid isoc TD, wait for
final completion event
[  110.530373] xhci_hcd 0000:01:00.0: xhci_handle_event_trb
[  110.530378] xhci_hcd 0000:01:00.0: @000000010afe6360 2d119510
00000000 01009000 01138001
[  110.530387] xhci_hcd 0000:01:00.0: xhci_handle_event_trb
[  110.530391] xhci_hcd 0000:01:00.0: @000000010afe6370 2d119520
00000000 04007000 01138001
[  110.530395] xhci_hcd 0000:01:00.0: Error mid isoc TD, wait for
final completion event
[  110.530430] xhci_hcd 0000:01:00.0: queue_command
[  110.530434] xhci_hcd 0000:01:00.0: @000000010afe5230 00000000
00000000 00000000 01133c01
[  110.530470] xhci_hcd 0000:01:00.0: xhci_handle_event_trb
[  110.530473] xhci_hcd 0000:01:00.0: @000000010afe6380 2d119520
00000000 1a000000 01138001
[  110.530478] xhci_hcd 0000:01:00.0: Error mid isoc TD, wait for
final completion event
[  110.530481] xhci_hcd 0000:01:00.0: xhci_handle_event_trb
[  110.530484] xhci_hcd 0000:01:00.0: @000000010afe6390 0afe5230
00000001 01000000 01008401
...

This may become confusing.

>
> As written it is going to cause problems, the driver will forget about
> earlier errors if the endpoint is stopped and resumed on the same TD.

Yes, this can happen, I didn't account for this scenario.

>
>
> I think that the whole patch could be much simpler, like:
>
> case X_ERROR:
>         frame->status = X;
>         td->error_mid_td = true;
>         break;
> case Y_ERROR:
>         frame->status = Y;
>         td->error_mid_td = true;
>         break;
>
> and then
>
> if (error_mid_td && (ep_trb != td->end_trb || ETRON && SUPERSPEED)) {
>         // error mid TD, wait for final event
> }
>
> finish_td()

Yes, this is much simpler than my patch, but we still need to fix
the issue with debug message being printed twice.

Maybe silence the debug message like this:

if (error_mid_td && (ep_trb != td->end_trb || ETRON && SUPERSPEED)) {
        if (trb_comp_code != COMP_STOPPED)
                xhci_dbg(xhci, "Error mid isoc TD, wait for final
completion event\n");
...
}

, or do nothing. Could you help with some suggestions?

>
>
> Regards,
> Michal

Thanks,
Kuangyi Chiang