On Friday, October 15, 2010 18:49:47 David Ellingsworth wrote:
> On Mon, Oct 11, 2010 at 2:05 PM, David Ellingsworth
> <david@xxxxxxxxxxxxxxxxx> wrote:
> > On Mon, Oct 11, 2010 at 11:40 AM, Hans Verkuil <hverkuil@xxxxxxxxx> wrote:
> >> On Sunday, October 10, 2010 19:33:48 David Ellingsworth wrote:
> >>> Hans,
> >>>
> >>> On Sun, Sep 26, 2010 at 8:25 AM, Hans Verkuil <hverkuil@xxxxxxxxx> wrote:
> >>> > Hi Mauro,
> >>> >
> >>> > These are the locking patches. It's based on my previous test tree, but with
> >>> > more testing with em28xx and radio-mr800 and some small tweaks relating to
> >>> > disconnect handling and video_is_registered().
> >>> >
> >>> > I also removed the unused get_unmapped_area file op and I am now blocking
> >>> > any further (unlocked_)ioctl calls after the device node is unregistered.
> >>> > The only things an application can do legally after a disconnect is unmap()
> >>> > and close().
> >>> >
> >>> > This patch series also contains a small em28xx fix that I found while testing
> >>> > the em28xx BKL removal patch.
> >>> >
> >>> > Regards,
> >>> >
> >>> > Hans
> >>> >
> >>> > The following changes since commit dace3857de7a16b83ae7d4e13c94de8e4b267d2a:
> >>> > Hans Verkuil (1):
> >>> > V4L/DVB: tvaudio: remove obsolete tda8425 initialization
> >>> >
> >>> > are available in the git repository at:
> >>> >
> >>> > ssh://linuxtv.org/git/hverkuil/v4l-dvb.git bkl
> >>> >
> >>> > Hans Verkuil (10):
> >>> > v4l2-dev: after a disconnect any ioctl call will be blocked.
> >>> > v4l2-dev: remove get_unmapped_area
> >>> > v4l2: add core serialization lock.
> >>> > videobuf: prepare to make locking optional in videobuf
> >>> > videobuf: add ext_lock argument to the queue init functions
> >>> > videobuf: add queue argument to videobuf_waiton()
> >>> > vivi: remove BKL.
> >>> > em28xx: remove BKL.
> >>> > em28xx: the default std was not passed on to the subdevs
> >>> > radio-mr800: remove BKL
> >>>
> >>> Did you even test these patches?
> >>
> >> Yes, I did test. And it works for me. But you are correct in that it shouldn't
> >> work since the struct will indeed be freed. I'll fix this and post a patch.
> >>
> >> I'm not sure why it works fine when I test it.
> >>
> >> There is a problem as well with unlocking before unregistering the device in
> >> that it leaves a race condition where another app can open the device again
> >> before it is registered. I have to think about that some more.
> >
> > Actually, no this problem did not exist. The previous version of the
> > driver cleared the USB device member to indicate that the device had
> > been disconnected. During open, if the USB device member was null, it
> > aborted with -EIO. If there's a race there now, it's only because you
> > introduced it.
> >
> > One thing I noticed while looking at this driver is that there's a
> > call to usb_autopm_put_interface in usb_amradio_close. I'm not sure if
> > it's a problem or not, but is it valid to call that after the device
> > has been disconnected? I only ask, because it wasn't called in
> > previous versions if the driver was disconnected before all handles to
> > the device were closed. If it's incorrect to call it within this
> > context, then this introduces another bug as well. It seems logical
> > that for every get there should be a put, but I don't know in this
> > case.
>
> Just came across this in power-management.txt kernel documentation:
>
> 343 Drivers need not be concerned about balancing changes to the usage
> 344 counter; the USB core will undo any remaining "get"s when a driver
> 345 is unbound from its interface. As a corollary, drivers must not call
> 346 any of the usb_autopm_* functions after their diconnect() routine has
> 347 returned.
>
> According to this documentation, the usb_autopm_put_interface call in
> usb_amradio_close should not occur if the device has been
> disconnected. The code you removed, used to prevent this special case.
>
> Regards,
>
> David Ellingsworth
I'll post a patch fixing these issues this weekend.
Regards,
Hans
--
Hans Verkuil - video4linux developer - sponsored by TANDBERG, part of Cisco
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
