On Fri, 6 Mar 2020, [iso-8859-1] M� Rullg� wrote:
> I found the problem. Initially, usb_sg_init() sets transfer_buffer to
> NULL like this:
>
> if (!PageHighMem(sg_page(sg)))
> urb->transfer_buffer = sg_virt(sg);
> else
> urb->transfer_buffer = NULL;
>
> Later, musb_host_rx() uses sg_miter_next() to assign a temporary
> address:
>
> /*
> * We need to map sg if the transfer_buffer is
> * NULL.
> */
> if (!urb->transfer_buffer) {
> qh->use_sg = true;
> sg_miter_start(&qh->sg_miter, urb->sg, 1,
> sg_flags);
> }
>
> if (qh->use_sg) {
> if (!sg_miter_next(&qh->sg_miter)) {
> dev_err(musb->controller, "error: sg list empty\n");
> sg_miter_stop(&qh->sg_miter);
> status = -EINVAL;
> done = true;
> goto finish;
> }
> urb->transfer_buffer = qh->sg_miter.addr;
> received_len = urb->actual_length;
> qh->offset = 0x0;
> done = musb_host_packet_rx(musb, urb, epnum,
> iso_err);
> /* Calculate the number of bytes received */
> received_len = urb->actual_length -
> received_len;
> qh->sg_miter.consumed = received_len;
> sg_miter_stop(&qh->sg_miter);
> } else {
> done = musb_host_packet_rx(musb, urb,
> epnum, iso_err);
> }
>
> When the transfer has completed, a bogus value is left behind in
> urb->transfer_buffer, and this trips up usbmon. Apparently nothing else
> uses that value before the urb is released.
>
> This patch makes it not crash:
>
> diff --git a/drivers/usb/musb/musb_host.c b/drivers/usb/musb/musb_host.c
> index 1c813c37462a..b67b40de1947 100644
> --- a/drivers/usb/musb/musb_host.c
> +++ b/drivers/usb/musb/musb_host.c
> @@ -1459,8 +1459,10 @@ void musb_host_tx(struct musb *musb, u8 epnum)
> qh->segsize = length;
>
> if (qh->use_sg) {
> - if (offset + length >= urb->transfer_buffer_length)
> + if (offset + length >= urb->transfer_buffer_length) {
> qh->use_sg = false;
> + urb->transfer_buffer = NULL;
> + }
> }
>
> musb_ep_select(mbase, epnum);
> @@ -1977,8 +1979,10 @@ void musb_host_rx(struct musb *musb, u8 epnum)
> urb->actual_length += xfer_len;
> qh->offset += xfer_len;
> if (done) {
> - if (qh->use_sg)
> + if (qh->use_sg) {
> qh->use_sg = false;
> + urb->transfer_buffer = NULL;
> + }
>
> if (urb->status == -EINPROGRESS)
> urb->status = status;
Good detective work, and I'm glad I was able to help.
Alan Stern
