Em 11-10-2010 12:40, Hans Verkuil escreveu: > 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. > >> The last one in the series clearly >> breaks radio-mr800 and the commit message does not describe the >> changes made. radio-mr800 has been BKL independent for quite some >> time. Hans, you of all people should know that calling >> video_unregister_device could result in the driver specific structure >> being freed. > > Jeez, relax. I'm human (really!). > >> The mutex must therefore be unlocked _before_ calling >> video_unregister_device. Otherwise you're passing freed memory to >> mutex_unlock in usb_amradio_disconnect. >> >> If each patch had been properly posted to the list, others might have >> caught issues like this earlier. Posting a link to a repository is no >> substitute for this process. >> >> Mauro, you should be ashamed for accepting a series that obviously has issues. > > Hardly obvious, and definitely not his fault. Hans, This is a somewhat recurring discussion at #v4l irc nowadays. Next time, please send your patch series first to the ML, tagged with [PATCH RFC] especially if they touch ondrivers that you're not the maintainer or when you weren't able to test. Thanks, Mauro -- 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
