Re: [PATCH] [Patch v2] usbtv: Fix refcounting mixup
On 05/16/18 11:23, Oliver Neukum wrote: > Am Dienstag, den 15.05.2018, 18:01 +0200 schrieb Hans Verkuil: >> On 05/15/2018 05:46 PM, Oliver Neukum wrote: >>> Am Dienstag, den 15.05.2018, 16:28 +0200 schrieb Hans Verkuil: On 05/15/18 15:07, Oliver Neukum wrote: > > usbtv_audio_fail: > /* we must not free at this point */ > - usb_get_dev(usbtv->udev); > + v4l2_device_get(&usbtv->v4l2_dev); This is very confusing. I think it is much better to move the >>> >>> Yes. It confused me. >>> v4l2_device_register() call from usbtv_video_init to this probe function. >>> >>> Yes, but it is called here. So you want to do it after registering the >>> audio? >> >> No, before. It's a global data structure, so this can be done before the >> call to usbtv_video_init() as part of the top-level initialization of the >> driver. > > Eh, but we cannot create a V4L device before the first device > is connected and we must certainly create multiple V4L devices if > multiple physical devices are connected. v4l2_device_register is a terrible name. It does not create devices or register with anything, it just initializes a root data structure. I have proposed renaming this to v4l2_root_init() in the past, but people didn't want a big rename action. BTW, with 'global data structure' I meant a data structure in struct usbtv. All I meant to say is that v4l2_device_register should be called in probe(), not in usbtv_video_init(). Regards, Hans > > Maybe I am dense. Please elaborate. > It seem to me that the driver is confusing because it uses > multiple refcounts. > > Regards > Oliver >
Re: [PATCH] [Patch v2] usbtv: Fix refcounting mixup
Am Dienstag, den 15.05.2018, 18:01 +0200 schrieb Hans Verkuil: > On 05/15/2018 05:46 PM, Oliver Neukum wrote: > > Am Dienstag, den 15.05.2018, 16:28 +0200 schrieb Hans Verkuil: > > > On 05/15/18 15:07, Oliver Neukum wrote: > > > > usbtv_audio_fail: > > > > /* we must not free at this point */ > > > > - usb_get_dev(usbtv->udev); > > > > + v4l2_device_get(&usbtv->v4l2_dev); > > > > > > This is very confusing. I think it is much better to move the > > > > Yes. It confused me. > > > > > v4l2_device_register() call from usbtv_video_init to this probe function. > > > > Yes, but it is called here. So you want to do it after registering the > > audio? > > No, before. It's a global data structure, so this can be done before the > call to usbtv_video_init() as part of the top-level initialization of the > driver. Eh, but we cannot create a V4L device before the first device is connected and we must certainly create multiple V4L devices if multiple physical devices are connected. Maybe I am dense. Please elaborate. It seem to me that the driver is confusing because it uses multiple refcounts. Regards Oliver
Re: [PATCH] [Patch v2] usbtv: Fix refcounting mixup
On 05/15/2018 05:46 PM, Oliver Neukum wrote:
> Am Dienstag, den 15.05.2018, 16:28 +0200 schrieb Hans Verkuil:
>> On 05/15/18 15:07, Oliver Neukum wrote:
>>> The premature free in the error path is blocked by V4L
>>> refcounting, not USB refcounting. Thanks to
>>> Ben Hutchings for review.
>>>
>>> [v2] corrected attributions
>>>
>>> Signed-off-by: Oliver Neukum
>>> Fixes: 50e704453553 ("media: usbtv: prevent double free in error case")
>>> CC: sta...@vger.kernel.org
>>> Reported-by: Ben Hutchings
>>> ---
>>> drivers/media/usb/usbtv/usbtv-core.c | 3 ++-
>>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/media/usb/usbtv/usbtv-core.c
>>> b/drivers/media/usb/usbtv/usbtv-core.c
>>> index 5095c380b2c1..4a03c4d66314 100644
>>> --- a/drivers/media/usb/usbtv/usbtv-core.c
>>> +++ b/drivers/media/usb/usbtv/usbtv-core.c
>>> @@ -113,7 +113,8 @@ static int usbtv_probe(struct usb_interface *intf,
>>>
>>> usbtv_audio_fail:
>>> /* we must not free at this point */
>>> - usb_get_dev(usbtv->udev);
>>> + v4l2_device_get(&usbtv->v4l2_dev);
>>
>> This is very confusing. I think it is much better to move the
>
> Yes. It confused me.
>
>> v4l2_device_register() call from usbtv_video_init to this probe function.
>
> Yes, but it is called here. So you want to do it after registering the
> audio?
No, before. It's a global data structure, so this can be done before the
call to usbtv_video_init() as part of the top-level initialization of the
driver.
Regards,
Hans
Re: [PATCH] [Patch v2] usbtv: Fix refcounting mixup
Am Dienstag, den 15.05.2018, 16:28 +0200 schrieb Hans Verkuil:
> On 05/15/18 15:07, Oliver Neukum wrote:
> > The premature free in the error path is blocked by V4L
> > refcounting, not USB refcounting. Thanks to
> > Ben Hutchings for review.
> >
> > [v2] corrected attributions
> >
> > Signed-off-by: Oliver Neukum
> > Fixes: 50e704453553 ("media: usbtv: prevent double free in error case")
> > CC: sta...@vger.kernel.org
> > Reported-by: Ben Hutchings
> > ---
> > drivers/media/usb/usbtv/usbtv-core.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/media/usb/usbtv/usbtv-core.c
> > b/drivers/media/usb/usbtv/usbtv-core.c
> > index 5095c380b2c1..4a03c4d66314 100644
> > --- a/drivers/media/usb/usbtv/usbtv-core.c
> > +++ b/drivers/media/usb/usbtv/usbtv-core.c
> > @@ -113,7 +113,8 @@ static int usbtv_probe(struct usb_interface *intf,
> >
> > usbtv_audio_fail:
> > /* we must not free at this point */
> > - usb_get_dev(usbtv->udev);
> > + v4l2_device_get(&usbtv->v4l2_dev);
>
> This is very confusing. I think it is much better to move the
Yes. It confused me.
> v4l2_device_register() call from usbtv_video_init to this probe function.
Yes, but it is called here. So you want to do it after registering the
audio?
Regards
Oliver
Re: [PATCH] [Patch v2] usbtv: Fix refcounting mixup
On 05/15/18 15:07, Oliver Neukum wrote:
> The premature free in the error path is blocked by V4L
> refcounting, not USB refcounting. Thanks to
> Ben Hutchings for review.
>
> [v2] corrected attributions
>
> Signed-off-by: Oliver Neukum
> Fixes: 50e704453553 ("media: usbtv: prevent double free in error case")
> CC: sta...@vger.kernel.org
> Reported-by: Ben Hutchings
> ---
> drivers/media/usb/usbtv/usbtv-core.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/usb/usbtv/usbtv-core.c
> b/drivers/media/usb/usbtv/usbtv-core.c
> index 5095c380b2c1..4a03c4d66314 100644
> --- a/drivers/media/usb/usbtv/usbtv-core.c
> +++ b/drivers/media/usb/usbtv/usbtv-core.c
> @@ -113,7 +113,8 @@ static int usbtv_probe(struct usb_interface *intf,
>
> usbtv_audio_fail:
> /* we must not free at this point */
> - usb_get_dev(usbtv->udev);
> + v4l2_device_get(&usbtv->v4l2_dev);
This is very confusing. I think it is much better to move the
v4l2_device_register() call from usbtv_video_init to this probe function.
The extra v4l2_device_get in the probe() can just be dropped and
usbtv_video_free() no longer needs to call v4l2_device_put().
The only place you need a v4l2_device_put() is in the disconnect()
function at the end.
Regards,
Hans
> + /* this will undo the v4l2_device_get() */
> usbtv_video_free(usbtv);
>
> usbtv_video_fail:
>
[PATCH] [Patch v2] usbtv: Fix refcounting mixup
The premature free in the error path is blocked by V4L
refcounting, not USB refcounting. Thanks to
Ben Hutchings for review.
[v2] corrected attributions
Signed-off-by: Oliver Neukum
Fixes: 50e704453553 ("media: usbtv: prevent double free in error case")
CC: sta...@vger.kernel.org
Reported-by: Ben Hutchings
---
drivers/media/usb/usbtv/usbtv-core.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/media/usb/usbtv/usbtv-core.c
b/drivers/media/usb/usbtv/usbtv-core.c
index 5095c380b2c1..4a03c4d66314 100644
--- a/drivers/media/usb/usbtv/usbtv-core.c
+++ b/drivers/media/usb/usbtv/usbtv-core.c
@@ -113,7 +113,8 @@ static int usbtv_probe(struct usb_interface *intf,
usbtv_audio_fail:
/* we must not free at this point */
- usb_get_dev(usbtv->udev);
+ v4l2_device_get(&usbtv->v4l2_dev);
+ /* this will undo the v4l2_device_get() */
usbtv_video_free(usbtv);
usbtv_video_fail:
--
2.13.6
