[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openbsd-tech
Subject:    Re: USBD_NO_COPY problems
From:       Martin Pieuchot <mpieuchot () nolizard ! org>
Date:       2015-02-28 8:43:38
Message-ID: 20150228084338.GF7426 () noz ! nolizard ! org
[Download RAW message or body]

On 19/02/15(Thu) 21:49, David Higgs wrote:
> On Feb 13, 2015, at 7:29 AM, David Higgs <higgsd@gmail.com> wrote:
> > On Friday, February 13, 2015, Martin Pieuchot <mpieuchot@nolizard.org> wrote:
> > On 13/02/15(Fri) 00:28, David Higgs wrote:
> > > I guess nobody else has tried calling uhidev_get_report_async() yet.  :)
> > > 
> > > First I was getting a NULL pointer deref in the uhidev async callback.
> > > Then I realized that due to USBD_NO_COPY, xfer->buffer was always
> > > NULL.  Next, I tried to use the DMA buffer, but I ended up in DDB in a
> > > very cryptic way.  I believe this is because the DMA buffer isn't
> > > available when the callback is invoked.
> > > 
> > > For the async callback to get a valid dmabuf, it needs to be invoked
> > > prior to usb_freemem() in usbd_transfer_complete().  The xfer->status
> > > determination would need to move up too.  I'd do this myself but I
> > > don't understand the logic and ordering of pipe->repeat stuff, and am
> > > concerned about unintentionally breaking other devices.
> > > 
> > > This is partially my fault, because I "tested" the original diff that
> > > added the USBD_NO_COPY semantics to verify that it didn't break my
> > > synchronous code paths, but hadn't yet written anything for upd(4) to
> > > check the async ones.
> > 
> > Does the diff below help? 
> > 
> > Partially but not enough.  I had already figured out that I needed that to solve \
> > the NULL pointer dereference.  See my 2nd paragraph above. 
> OK, I figured out my issue - the crazy DDB backtrace is produced when you execute a \
> NULL callback. 
> It still doesn't seem legal for the callback to access DMA buffer contents after \
> they are "freed".  I assume this won't work in all cases (host controllers / \
> architectures / cache behaviors), but I don't experience any problems in my i386 \
> VM.  I tried reordering parts of usbd_transfer_complete(), but DIAGNOSTIC code \
> became very unhappy with the results. 
> Fortunately, the diff below doesn't touch that code path and just fixes the uhidev \
> layer.  My async upd(4) changes will be forthcoming in a different thread.

Committed since nothing uses it at the moment.  Thanks!

Martin


[prev in list] [next in list] [prev in thread] [next in thread]