> -----Original Message----- > From: Sarah Sharp [mailto:sarah.a.sharp@xxxxxxxxxxxxxxx] > Sent: Wednesday, July 14, 2010 3:46 AM > To: Xu, Andiry > Cc: linux-usb@xxxxxxxxxxxxxxx; gregkh@xxxxxxx > Subject: Re: [PATCH 8/9 v7] xHCI: Isochronous transfer implementation > > Comments below. > > > + case COMP_BW_OVER: > > + td->urb->iso_frame_desc[idx].status = -ECOMM; > > + skip_td = 1; > > + break; > > + case COMP_MISSED_INT: > > + td->urb->iso_frame_desc[idx].status = -ECOMM; > > + skip_td = 1; > > + break; > > Small suggestion: combine case statements to make the whole switch > statement shorter, like so: > > case COMP_BW_OVER: > case COMP_MISSED_INT: > td->urb->iso_frame_desc[idx].status = -ECOMM; > skip_td = 1; > break; > > The same could be done by grouping COMP_BUFF_OVER and COMP_BABBLE. OK. > > > + case COMP_BUFF_OVER: > > + td->urb->iso_frame_desc[idx].status = -EOVERFLOW; > > + skip_td = 1; > > + break; > > + case COMP_STALL: > > + td->urb->iso_frame_desc[idx].status = -EPROTO; > > + skip_td = 1; > > + break; > > + case COMP_BABBLE: > > + td->urb->iso_frame_desc[idx].status = -EOVERFLOW; > > + skip_td = 1; > > + break; > > + case COMP_STOP_INVAL: > > + td->urb->iso_frame_desc[idx].status = -EREMOTEIO; > > + break; > > + case COMP_STOP: > > + td->urb->iso_frame_desc[idx].status = -EREMOTEIO; > > + break; > > You shouldn't set the status for the isochronous transfer if the > endpoint ring is stopped. That's just a temporary hardware condition > that the device driver doesn't need to know about. The transfer should > be restarted (or skipped in the case of a missed service event) when the > endpoint ring is restarted. > > I know that finish_td() will not giveback the URB with the short packet > status in this case, but the driver could see this intermediate status, > much like the case with urb->status. > Hmm... I'll consider this. > > + > > + /* Queue the first TRB, even if it's zero-length */ > > + for (i = 0; i < num_tds; i++) { > > + first_trb = true; > > + > > + running_total = 0; > > + start_trb = &ep_ring->enqueue->generic; > > + start_cycle = ep_ring->cycle_state; > > + > > + addr = start_addr + urb->iso_frame_desc[i].offset; > > + td_len = urb->iso_frame_desc[i].length; > > + td_remain_len = td_len; > > + > > + trbs_per_td = count_isoc_trbs_needed(xhci, urb, i); > > + > > + ret = prepare_transfer(xhci, xhci->devs[slot_id], ep_index, > > + urb->stream_id, trbs_per_td, urb, i, mem_flags); > > Ok, so it looks like you're calling prepare_transfer() before enqueuing > each isochronous TD. So you shouldn't run into issues with John's link > TRB handoff change patch. You will need to change the call to > queue_trb() to pass false for the more_trbs_coming field. > I'll modify the queue_trb() calling. But I got question about prepare_transfer() calling you mentioned in another mail. > > + > > + wmb(); > > + start_trb->field[3] |= start_cycle; > > + } > > It's going to be expensive (time and hardware-wise) to have a write > memory barrier between each queued isochronous TD. The current code > will force 10 wmb() calls if you enqueue an URB with number_of_packets > set to 10. > > I think it's better if you write the first TRB of the very first TD > outside this loop. Then you only need to do one wmb() before that call. > Thanks. It's really nice suggestion. Thanks, Andiry -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
