On 05/23/2014 03:18 PM, George Cherian wrote:
> On 5/23/2014 3:01 PM, Daniel Mack wrote:
>> Before accessing any of an endpoint's CSR registers, make sure the
>> correct endpoint is selected. Otherwise, data read from or written to
>> the registers is likely to affect the wrong endpoint as long as the
>> connected device has more than one endpoint.
>>
>> This, of course, leads to all sorts of strange effects such as stream
>> starvation and driver internal state machine confusion due to spurious
>> interrupts.
>>
>> Signed-off-by: Daniel Mack <[email protected]>
>> ---
>> drivers/usb/musb/musb_cppi41.c | 17 ++++++++++++++---
>> 1 file changed, 14 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/usb/musb/musb_cppi41.c b/drivers/usb/musb/musb_cppi41.c
>> index a11bbb6..db497dd 100644
>> --- a/drivers/usb/musb/musb_cppi41.c
>> +++ b/drivers/usb/musb/musb_cppi41.c
>> @@ -58,14 +58,17 @@ struct cppi41_dma_controller {
>>
>> static void save_rx_toggle(struct cppi41_dma_channel *cppi41_channel)
>> {
>> + struct musb_hw_ep *hw_ep = cppi41_channel->hw_ep;
>> + struct musb *musb = hw_ep->musb;
>> u16 csr;
>> u8 toggle;
>>
>> if (cppi41_channel->is_tx)
>> return;
>> - if (!is_host_active(cppi41_channel->controller->musb))
>> + if (!is_host_active(musb))
>> return;
>>
>> + musb_ep_select(musb->mregs, hw_ep->epnum);
>
> save_rx_toggle is called from cppi41_configure_channel as part of
> channel_program() from musb_host_rx()
> and musb_ep_program(). Both these functions call musb_ep_select() before
> calling channel_program().
>
> Both musb_ep_program() and musb_host_rx() are called with IRQ's disabled.
> Still do we need this musb_ep_select() in save_rx_toggle? Or am I confused?
No, you're right. It seems not necessary here.
>> @@ -173,6 +180,7 @@ static void cppi41_trans_done(struct cppi41_dma_channel
>> *cppi41_channel)
>> dma_async_issue_pending(dc);
>>
>> if (!cppi41_channel->is_tx) {
>> + musb_ep_select(musb->mregs, hw_ep->epnum);
> Since in update_rx_toggle () we did musb_epe_select(), do we really it
> again here?
I'd say so, because cppi41_trans_done() may be called from a work
tasklet or hrtimer callback. In such cases, another endpoint might have
been selected.
>> @@ -565,6 +574,8 @@ static int cppi41_dma_channel_abort(struct dma_channel
>> *channel)
>> if (cppi41_channel->channel.status == MUSB_DMA_STATUS_FREE)
>> return 0;
>>
> musb_ep_select is done before calling channel_abort.
Right, thanks for spotting this. I'll drop it in v3.
Thanks,
Daniel
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
