[GIT PATCHES FOR 2.6.37] Move V4L2 locking into the core framework

[GIT PATCHES FOR 2.6.37] Move V4L2 locking into the core framework

[Hans Verkuil]

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

 Documentation/video4linux/v4l2-framework.txt  |   25 +++++-
 drivers/media/common/saa7146_fops.c           |    2 +-
 drivers/media/common/saa7146_vbi.c            |    2 +-
 drivers/media/common/saa7146_video.c          |    2 +-
 drivers/media/radio/radio-mr800.c             |   74 ++--------------
 drivers/media/video/au0828/au0828-video.c     |    4 +-
 drivers/media/video/bt8xx/bttv-driver.c       |    4 +-
 drivers/media/video/bt8xx/bttv-risc.c         |    2 +-
 drivers/media/video/cx231xx/cx231xx-video.c   |    6 +-
 drivers/media/video/cx23885/cx23885-417.c     |    2 +-
 drivers/media/video/cx23885/cx23885-core.c    |    2 +-
 drivers/media/video/cx23885/cx23885-dvb.c     |    2 +-
 drivers/media/video/cx23885/cx23885-video.c   |    2 +-
 drivers/media/video/cx88/cx88-blackbird.c     |    2 +-
 drivers/media/video/cx88/cx88-core.c          |    2 +-
 drivers/media/video/cx88/cx88-dvb.c           |    2 +-
 drivers/media/video/cx88/cx88-video.c         |    4 +-
 drivers/media/video/em28xx/em28xx-video.c     |   93 +++------------------
 drivers/media/video/fsl-viu.c                 |    2 +-
 drivers/media/video/mx1_camera.c              |    2 +-
 drivers/media/video/mx2_camera.c              |    2 +-
 drivers/media/video/mx3_camera.c              |    2 +-
 drivers/media/video/omap24xxcam.c             |    2 +-
 drivers/media/video/pxa_camera.c              |    2 +-
 drivers/media/video/s2255drv.c                |    2 +-
 drivers/media/video/saa7134/saa7134-core.c    |    2 +-
 drivers/media/video/saa7134/saa7134-dvb.c     |    2 +-
 drivers/media/video/saa7134/saa7134-empress.c |    2 +-
 drivers/media/video/saa7134/saa7134-video.c   |    4 +-
 drivers/media/video/sh_mobile_ceu_camera.c    |    2 +-
 drivers/media/video/sh_vou.c                  |    2 +-
 drivers/media/video/v4l2-dev.c                |  110 ++++++++++++++----------
 drivers/media/video/v4l2-event.c              |    9 ++-
 drivers/media/video/v4l2-mem2mem.c            |    8 +-
 drivers/media/video/videobuf-core.c           |  115 +++++++++++++++----------
 drivers/media/video/videobuf-dma-contig.c     |    9 +-
 drivers/media/video/videobuf-dma-sg.c         |    9 +-
 drivers/media/video/videobuf-dvb.c            |    2 +-
 drivers/media/video/videobuf-vmalloc.c        |    9 +-
 drivers/media/video/vivi.c                    |   17 ++--
 drivers/media/video/zr364xx.c                 |    2 +-
 drivers/staging/cx25821/cx25821-core.c        |    2 +-
 drivers/staging/cx25821/cx25821-video.c       |    2 +-
 drivers/staging/dt3155v4l/dt3155v4l.c         |    8 +-
 drivers/staging/tm6000/tm6000-video.c         |    2 +-
 include/media/v4l2-dev.h                      |    5 +-
 include/media/videobuf-core.h                 |   19 ++++-
 include/media/videobuf-dma-contig.h           |    3 +-
 include/media/videobuf-dma-sg.h               |    3 +-
 include/media/videobuf-vmalloc.h              |    3 +-
 50 files changed, 282 insertions(+), 315 deletions(-)
-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG, part of Cisco

Re: [GIT PATCHES FOR 2.6.37] Move V4L2 locking into the core framework

[Mauro Carvalho Chehab]

Em 26-09-2010 09:25, Hans Verkuil escreveu:
> 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

Applied, thanks.

> 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()

You forgot two to add the extra parameter on two drivers that uses vmalloc,
noticed when I tried to compile it for i386 arch. I've already added them.

Could you please double check if everything is compiling fine on the other archs?

Thanks,
Mauro

Re: [GIT PATCHES FOR 2.6.37] Move V4L2 locking into the core framework

[David Ellingsworth]

Hans,

On Sun, Sep 26, 2010 at 8:25 AM, Hans Verkuil <hverkuil@xs4all.nl> 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? 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. 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.

Regards,

David Ellingsworth

Re: [GIT PATCHES FOR 2.6.37] Move V4L2 locking into the core framework

[David Ellingsworth]

On Sun, Oct 10, 2010 at 1:33 PM, David Ellingsworth
<david@identd.dyndns.org> wrote:
> Hans,
>
> On Sun, Sep 26, 2010 at 8:25 AM, Hans Verkuil <hverkuil@xs4all.nl> 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? 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. The mutex must therefore be unlocked _before_ calling
> video_unregister_device. Otherwise you're passing freed memory to
> mutex_unlock in usb_amradio_disconnect.
>

To reiterate, the video_device struct is part of the amradio_device
struct. When video_device_unregister is called, it can cause the
release callback of the video_device struct to be called. In this
case, that results in usb_amradio_video_device_release being called
which frees the amradio_device struct. Therefore any use of the
amradio_device struct after calling video_device_unregister is a
potential use after free error. In this particular case you are trying
to access the amradio_device.lock member which has potentially been
freed already.

Regards,

David Ellingsworth

Re: [GIT PATCHES FOR 2.6.37] Move V4L2 locking into the core framework

[Hans Verkuil]

On Sunday, October 10, 2010 19:33:48 David Ellingsworth wrote:
> Hans,
> 
> On Sun, Sep 26, 2010 at 8:25 AM, Hans Verkuil <hverkuil@xs4all.nl> 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.

Regards,

	Hans

> 
> Regards,
> 
> David Ellingsworth
> 
> 

-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG, part of Cisco

Re: [GIT PATCHES FOR 2.6.37] Move V4L2 locking into the core framework

[Mauro Carvalho Chehab]

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@xs4all.nl> 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

Re: [GIT PATCHES FOR 2.6.37] Move V4L2 locking into the core framework

[Hans Verkuil]

On Monday, October 11, 2010 17:48:45 Mauro Carvalho Chehab wrote:
> 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@xs4all.nl> 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.

So this is the new policy? Post with [PATCH RFC], wait a few days, then post a
git pull request?

Should I also do this for long but mechanical conversions like the 'video_device'
to 'v4l2_devnode'rename patch series? I think posting that would be spamming the
list.

Regards,

	Hans

-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG, part of Cisco

Re: [GIT PATCHES FOR 2.6.37] Move V4L2 locking into the core framework

[Laurent Pinchart]

Hi Hans,

On Monday 11 October 2010 17:54:07 Hans Verkuil wrote:
> On Monday, October 11, 2010 17:48:45 Mauro Carvalho Chehab wrote:
> > 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@xs4all.nl> 
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.
> 
> So this is the new policy? Post with [PATCH RFC], wait a few days, then
> post a git pull request?

That's a new policy under discussion. I think it's worth being discussed a bit 
more. http://www.linuxtv.org/irc/v4l/index.php?date=2010-10-01 and 
http://www.linuxtv.org/irc/v4l/index.php?date=2010-10-11 if you want to catch 
up with the informal discussion.

> Should I also do this for long but mechanical conversions like the
> 'video_device' to 'v4l2_devnode'rename patch series? I think posting that
> would be spamming the list.

-- 
Regards,

Laurent Pinchart

Re: [GIT PATCHES FOR 2.6.37] Move V4L2 locking into the core framework

[Mauro Carvalho Chehab]

Em 11-10-2010 13:23, Laurent Pinchart escreveu:
> Hi Hans,
> 
> On Monday 11 October 2010 17:54:07 Hans Verkuil wrote:
>> On Monday, October 11, 2010 17:48:45 Mauro Carvalho Chehab wrote:
>>> 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@xs4all.nl> 
> 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.
>>
>> So this is the new policy? Post with [PATCH RFC], wait a few days, then
>> post a git pull request?

Acks from other maintainers when a patch touches on their files were always part
of the policy.
> 
> That's a new policy under discussion. I think it's worth being discussed a bit 
> more. http://www.linuxtv.org/irc/v4l/index.php?date=2010-10-01 and 
> http://www.linuxtv.org/irc/v4l/index.php?date=2010-10-11 if you want to catch 
> up with the informal discussion.
> 
>> Should I also do this for long but mechanical conversions like the
>> 'video_device' to 'v4l2_devnode'rename patch series? I think posting that
>> would be spamming the list.

In this case, posting the actual patches seem overkill to me. Yet, is a good idea to
c/c the maintainers for the driver you're touching, to avoid conflicts with their
merges.

Cheers,
Mauro

Re: [GIT PATCHES FOR 2.6.37] Move V4L2 locking into the core framework

[David Ellingsworth]

On Mon, Oct 11, 2010 at 11:40 AM, Hans Verkuil <hverkuil@xs4all.nl> 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@xs4all.nl> 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.

>
>>
>> Mauro, you should be ashamed for accepting a series that obviously has issues.
>
> Hardly obvious, and definitely not his fault.
>

This comment was more general, since Mauro admitted having to make
changes to your series to get it to compile under i386 architectures.

Regards,

David Ellingsworth

Re: [GIT PATCHES FOR 2.6.37] Move V4L2 locking into the core framework

[Mauro Carvalho Chehab]

Em 11-10-2010 15:05, David Ellingsworth escreveu:
> On Mon, Oct 11, 2010 at 11:40 AM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>> On Sunday, October 10, 2010 19:33:48 David Ellingsworth wrote:

>>> Mauro, you should be ashamed for accepting a series that obviously has issues.
>>
>> Hardly obvious, and definitely not his fault.
>>
> 
> This comment was more general, since Mauro admitted having to make
> changes to your series to get it to compile under i386 architectures.
> 

So what? I always test if the tree compiles before sending the thing upstream. My
compilation is against i686 architecture, as it enables more drivers than other
architectures.

Rejecting a patch series just because of the lack of a typecast to remove a warning
on an architecture is not a good reason. I really prefer to apply the series and then
ask (or make a fix) to one or two lines, than to have to dig the entire patch series 
again on a rev 2 of the entire patch series. Examining a patch that fixes this issue
is a way easier than having to review a series of 11 patches.

Cheers,
Mauro.

Re: [GIT PATCHES FOR 2.6.37] Move V4L2 locking into the core framework

[David Ellingsworth]

Hans,

I noticed a couple more issues with this series. In your changes to
radio-mr800, you removed the lock from usb_amradio_suspend and
usb_amradio_resume without implementing resume/suspend support in the
v4l2 core. Without resume/suspend support in the v4l2 core, the
locking within usb_amradio_suspend and usb_amradio_resume must remain
to prevent races between other open/close/ioctl/read/mmap/etc and the
resume/suspend cycle. Please revert the changes you made to these two
functions.

Regards,

David Ellingsworth

Re: [GIT PATCHES FOR 2.6.37] Move V4L2 locking into the core framework

[David Ellingsworth]

On Mon, Oct 11, 2010 at 2:05 PM, David Ellingsworth
<david@identd.dyndns.org> wrote:
> On Mon, Oct 11, 2010 at 11:40 AM, Hans Verkuil <hverkuil@xs4all.nl> 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@xs4all.nl> 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

Re: [GIT PATCHES FOR 2.6.37] Move V4L2 locking into the core framework

[Hans Verkuil]

On Friday, October 15, 2010 18:49:47 David Ellingsworth wrote:
> On Mon, Oct 11, 2010 at 2:05 PM, David Ellingsworth
> <david@identd.dyndns.org> wrote:
> > On Mon, Oct 11, 2010 at 11:40 AM, Hans Verkuil <hverkuil@xs4all.nl> 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@xs4all.nl> 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