Re: [PATCH v2] usb: fix mass storage gadgets to work with Synopsys UDC

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, May 31, 2012 at 3:24 PM, Felipe Balbi <balbi@xxxxxx> wrote:
> On Thu, May 31, 2012 at 03:09:26PM +0530, Rajaram R wrote:
>> On Thu, May 31, 2012 at 1:41 PM, Felipe Balbi <balbi@xxxxxx> wrote:
>> > Hi,
>> >
>> > (please don't top-post, ever)
>>
>> hmm i missed that... thanks for pointing ;-)
>>
>> >
>> > On Thu, May 31, 2012 at 12:06:21PM +0530, Rajaram R wrote:
>> >>  Hi Felipe/Paul
>> >>
>> >>  Any reason for removing short_not_ok ? I believe it still holds true
>> >>  even after this patch. I will push a patch for the same if there is no
>> >>  objection.
>> >
>> > It doesn't hold true at all. We _want_ a short packet in order to end
>> > the transfer. Let me illustrate:
>> >
>> > We want to receive 31-byte CBw, but our UDC can't handle short packets
>> > on OUT direction, so we start a transfer for wMaxPacketSize. If we said
>> > short_not_ok, the driver wouldn't like when we received 31-byte and
>> > ended the transfer.
>>
>> IMO any tweak specific to a controller should be in the platform
>> files.. In case of musb also the controller has some limitation and
>> short packets are handled appropriately within the musb driver.
>
> I have explained why it wouldn't be a good choice to do that for dwc3.
> USB3 transfers are quite high bandwidth (5Gbps), if we needed to use a
> bounce buffer for every non-aligned transfer, we would have to do quite
> a lot of memcpy() and that would degrade performance quite a lot and
> cause quite some load on the CPU.

In such a case shouldnt we do this change only for SuperSpeed
scenarios ? As Alan tracked earlier when this patch was pushed

       http://marc.info/?l=linux-usb&m=130096570404235&w=2
       http://marc.info/?l=linux-usb&m=130164710522223&w=2
       http://marc.info/?l=linux-usb&m=130279510629643&w=2

Our setup is getting affected by this change !
>
> That was the reason why we chose to change g_mass_storage instead :-s
>
>> In case of DWC, if there is a limitation you can always ignore this
>> field. This field is being used by musb driver for managing DMA
>> transfers. Removing it will affect musb drivers. Please clarify
>
> yeah, musb is quite buggy :-p patches fixing that are very welcome
> actually.

Since you also maintain musb driver could you please clarify none of
the musb driver is affected by this change??

Is no other musb user finding this change affecting their setup ??

>
> To be fair, the whole EP handling in MUSB is quite messed up and should
> change. Together with DMA setup, endpoint initialization, FIFO
> allocation (and re-allocation if necessary after SetInterface) and so
> on...
>
> Just look at rxstate() and txstate(), those two functions are too big
> and they know too much about the underlying DMA engine. I'm sure many
> many bugs are hidden on those two functions. If you would like to help
> out, just splitting txstate() and rxstate() into dma and non-dma
> functions would already help you see issues. After that, go back to the
> MUSB programming guide and unify DMA programming (it can be done) so

If that was that straight forward it would been there by now :-)

> that all those ifdefs are dropped and checks for which DMA engine are
> dropped.
>
> --
> balbi
--
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


[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux