cvs commit: src/sys/dev/usb ehci.c ohci.c
Sam Leffler
sam at freebsd.org
Sun May 11 22:27:42 UTC 2008
Marius Strobl wrote:
> On Fri, May 09, 2008 at 09:21:02AM -0700, Sam Leffler wrote:
>
>> Marius Strobl wrote:
>>
>>> On Wed, Apr 23, 2008 at 02:03:27PM -0700, Sam Leffler wrote:
>>>
>>>
>>>> Marius Strobl wrote:
>>>>
>>>>
>>>>> On Wed, Apr 23, 2008 at 01:43:55PM -0700, Sam Leffler wrote:
>>>>>
>>>>>
>>>>>
>>>>>> Marius Strobl wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>>> On Sat, Apr 12, 2008 at 09:33:58PM +0200, Marius Strobl wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>> On Thu, Mar 20, 2008 at 04:19:26PM +0000, Sam Leffler wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>> sam 2008-03-20 16:19:25 UTC
>>>>>>>>>
>>>>>>>>> FreeBSD src repository
>>>>>>>>>
>>>>>>>>> Modified files:
>>>>>>>>> sys/dev/usb ehci.c ohci.c
>>>>>>>>> Log:
>>>>>>>>> Workaround design botch in usb: blindly mixing bus_dma with PIO does
>>>>>>>>> not
>>>>>>>>> work on architectures with a write-back cache as the PIO writes end
>>>>>>>>> up
>>>>>>>>> in the cache which the sync(BUS_DMASYNC_POSTREAD) in
>>>>>>>>> usb_transfer_complete
>>>>>>>>> then discards; compensate in the xfer methods that do PIO by pushing
>>>>>>>>> the
>>>>>>>>> writes out of the cache before usb_transfer_complete is called.
>>>>>>>>>
>>>>>>>>> This fixes USB on xscale and likely other places.
>>>>>>>>>
>>>>>>>>> Sponsored by: hobnob
>>>>>>>>> Reviewed by: cognet, imp
>>>>>>>>> MFC after: 1 month
>>>>>>>>>
>>>>>>>>> Revision Changes Path
>>>>>>>>> 1.62 +16 -0 src/sys/dev/usb/ehci.c
>>>>>>>>> 1.171 +16 -0 src/sys/dev/usb/ohci.c
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>> This causes a crash during boot on sparc64. Looks like map is still
>>>>>>>> NULL at that point.
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>> Are you ok with the change below or would that also prevent
>>>>>>> your kludge from taking effect?
>>>>>>>
>>>>>>> Marius
>>>>>>>
>>>>>>> Index: ehci.c
>>>>>>> ===================================================================
>>>>>>> RCS file: /usr/data/bsd/cvs/fbsd/src/sys/dev/usb/ehci.c,v
>>>>>>> retrieving revision 1.62
>>>>>>> diff -u -r1.62 ehci.c
>>>>>>> --- ehci.c 20 Mar 2008 16:19:25 -0000 1.62
>>>>>>> +++ ehci.c 23 Apr 2008 20:23:58 -0000
>>>>>>> @@ -664,6 +664,8 @@
>>>>>>> usbd_pipe_handle pipe = xfer->pipe;
>>>>>>> bus_dma_tag_t tag = pipe->device->bus->buffer_dmatag;
>>>>>>> struct usb_dma_mapping *dmap = &xfer->dmamap;
>>>>>>> + if (dmap->map == NULL)
>>>>>>> + return;
>>>>>>> bus_dmamap_sync(tag, dmap->map, BUS_DMASYNC_PREWRITE);
>>>>>>> }
>>>>>>>
>>>>>>> Index: ohci.c
>>>>>>> ===================================================================
>>>>>>> RCS file: /usr/data/bsd/cvs/fbsd/src/sys/dev/usb/ohci.c,v
>>>>>>> retrieving revision 1.171
>>>>>>> diff -u -r1.171 ohci.c
>>>>>>> --- ohci.c 20 Mar 2008 16:19:25 -0000 1.171
>>>>>>> +++ ohci.c 21 Apr 2008 19:13:54 -0000
>>>>>>> @@ -1571,6 +1571,8 @@
>>>>>>> usbd_pipe_handle pipe = xfer->pipe;
>>>>>>> bus_dma_tag_t tag = pipe->device->bus->buffer_dmatag;
>>>>>>> struct usb_dma_mapping *dmap = &xfer->dmamap;
>>>>>>> + if (dmap->map == NULL)
>>>>>>> + return;
>>>>>>> bus_dmamap_sync(tag, dmap->map, BUS_DMASYNC_PREWRITE);
>>>>>>> }
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>> You have not identified why you don't have a dma map. I don't have a
>>>>>> way to diagnose your problem and so far as I know no other platform had
>>>>>> an issue w/ the change. I suggest you figure out why your map is not
>>>>>> setup instead of adding a hack.
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>> It's because the usb(4) code doesn't create DMA maps for
>>>>> zero-length transfers, see usbd_transfer(). In the case of
>>>>> the backtrace I posted not for usbd_set_address(), which
>>>>> does USETW(req.wLength, 0) so later on size is 0 in
>>>>> usbd_transfer() hence no DMA map. I don't know why your
>>>>> hack doesn't also crash other platforms.
>>>>>
>>>>>
>>>>>
>>>> Thanks for explaining, I will look. Please hold off for a bit.
>>>>
>>>>
>>>>
>>> Style-wise the version below probably is more appropriate than
>>> the above one. The question still is whether that fix prevents
>>> hacksync() taking effect as desired, which would make it a very
>>> evil hack though as hacksync() then relies on bus_dmamap_sync()
>>> working on uninitialized DMA maps.
>>>
>>> Marius
>>>
>>> Index: ehci.c
>>> ===================================================================
>>> RCS file: /usr/data/bsd/cvs/fbsd/src/sys/dev/usb/ehci.c,v
>>> retrieving revision 1.62
>>> diff -u -p -r1.62 ehci.c
>>> --- ehci.c 20 Mar 2008 16:19:25 -0000 1.62
>>> +++ ehci.c 27 Apr 2008 14:09:53 -0000
>>> @@ -661,9 +661,13 @@ ehci_pcd_enable(void *v_sc)
>>> static __inline void
>>> hacksync(usbd_xfer_handle xfer)
>>> {
>>> - usbd_pipe_handle pipe = xfer->pipe;
>>> - bus_dma_tag_t tag = pipe->device->bus->buffer_dmatag;
>>> - struct usb_dma_mapping *dmap = &xfer->dmamap;
>>> + bus_dma_tag_t tag;
>>> + struct usb_dma_mapping *dmap;
>>> +
>>> + if (xfer->length == 0)
>>> + return;
>>> + tag = xfer->pipe->device->bus->buffer_dmatag;
>>> + dmap = &xfer->dmamap;
>>> bus_dmamap_sync(tag, dmap->map, BUS_DMASYNC_PREWRITE);
>>> }
>>>
>>> Index: ohci.c
>>> ===================================================================
>>> RCS file: /usr/data/bsd/cvs/fbsd/src/sys/dev/usb/ohci.c,v
>>> retrieving revision 1.171
>>> diff -u -p -r1.171 ohci.c
>>> --- ohci.c 20 Mar 2008 16:19:25 -0000 1.171
>>> +++ ohci.c 27 Apr 2008 14:09:37 -0000
>>> @@ -1568,9 +1568,13 @@ ohci_device_bulk_done(usbd_xfer_handle x
>>> static __inline void
>>> hacksync(usbd_xfer_handle xfer)
>>> {
>>> - usbd_pipe_handle pipe = xfer->pipe;
>>> - bus_dma_tag_t tag = pipe->device->bus->buffer_dmatag;
>>> - struct usb_dma_mapping *dmap = &xfer->dmamap;
>>> + bus_dma_tag_t tag;
>>> + struct usb_dma_mapping *dmap;
>>> +
>>> + if (xfer->length == 0)
>>> + return;
>>> + tag = xfer->pipe->device->bus->buffer_dmatag;
>>> + dmap = &xfer->dmamap;
>>> bus_dmamap_sync(tag, dmap->map, BUS_DMASYNC_PREWRITE);
>>> }
>>>
>>>
>>>
>>>
>>>
>> Sorry for taking so long to look at this. It appears the reason things
>> work everywhere but sparc is because all the other archs use the
>> definition of bus_dmamap_sync which looks like this:
>>
>> #define bus_dmamap_sync(dmat, dmamap, op) \
>> do { \
>> if ((dmamap) != NULL) \
>> _bus_dmamap_sync(dmat, dmamap, op); \
>> } while (0)
>>
>> So it explicitly checks for the map being NULL and since sparc does not
>> it blows up. Now checking for a NULL map seems very wrong (as the map
>> should be opaque as scott noted) but having sparc have different
>> semantics seems wrong. So rather than add yet another hack in the usb
>> code perhaps we should make sparc's bus_dma code consistent with
>> everyone else on this?
>>
>> There's no mention of this in the man page.
>>
>
> My guess would be that this is due to the other archs letting
> bus_dmamem_alloc() return a NULL DMA map to denote no mapping
> or bouncing and thus no syncing is required as jhb@ explained.
> This check is irrelevant to sparc64 and sun4v as these require
> syncing in any case. I think letting hacksync() rely on this
> internal optimization of bus_dmamap_sync() is very wrong also.
> Neither do I see why letting hacksync() just do nothing if
> xfer->length == 0 is a hack as it's basically the same check
> usbd_transfer() uses to decide whether to create a DMA map in
> the first place. Besides, hacksync() already is a very crude
> hack so one hardly can make it worse. I can just #ifdef arm or
> #ifndef sparc64 it if you prefer.
>
>
The check for NULL in bus_dmamap_sync is a dubious optimization. I
simply suggested you might want to apply it to sparc's bus_dma code
since there's no way of knowing whether there are other places that this
is likewise assumed. At this point I don't care; if you want to hack
the hacksync routine to check for a null dmapmap or xfer length zero or
whatever feel free. It's all just covering up for the brokeness of the
usb code.
Sam
More information about the cvs-src
mailing list