Re: [PATCH] VIDEO: ivtvfb, remove unneeded NULL test

[Jiri Slaby]

On 07/04/2010 03:22 PM, Andy Walls wrote:
> On Sun, 2010-07-04 at 09:24 +0200, Jiri Slaby wrote:
>> On 07/04/2010 06:11 AM, Andy Walls wrote:
>>> --- a/drivers/media/video/ivtv/ivtvfb.c
>>> +++ b/drivers/media/video/ivtv/ivtvfb.c
>>> @@ -1201,9 +1201,14 @@ static int ivtvfb_init_card(struct ivtv *itv)
>>>  static int __init ivtvfb_callback_init(struct device *dev, void *p)
>>>  {
>>>         struct v4l2_device *v4l2_dev = dev_get_drvdata(dev);
>>> -       struct ivtv *itv = container_of(v4l2_dev, struct ivtv, v4l2_dev);
>>> +       struct ivtv *itv;
>>>  
>>> -       if (itv && (itv->v4l2_cap & V4L2_CAP_VIDEO_OUTPUT)) {
>>> +       if (v4l2_dev == NULL)
>>> +               return 0;
>>> +
>>> +       itv = container_of(v4l2_dev, struct ivtv, v4l2_dev);
>>> +
>>> +       if (itv->v4l2_cap & V4L2_CAP_VIDEO_OUTPUT) {
>>>                 if (ivtvfb_init_card(itv) == 0) {
>>>                         IVTVFB_INFO("Framebuffer registered on %s\n",
>>>                                         itv->v4l2_dev.name);
>>> @@ -1216,10 +1221,16 @@ static int __init ivtvfb_callback_init(struct device *de
>>>  static int ivtvfb_callback_cleanup(struct device *dev, void *p)
>>>  {
>>>         struct v4l2_device *v4l2_dev = dev_get_drvdata(dev);
>>> -       struct ivtv *itv = container_of(v4l2_dev, struct ivtv, v4l2_dev);
>>> -       struct osd_info *oi = itv->osd_info;
>>> +       struct ivtv *itv;
>>> +       struct osd_info *oi;
>>> +
>>> +       if (v4l2_dev == NULL)
>>> +               return 0;
>>
>> >From my POV I NACK this. Given that it never triggered and drvdata are
>> set in v4l2_device_register called from ivtv_probe I can't see how
>> v4l2_dev be NULL. Could you elaborate?
> 
> I hemmed and hawed over that too.  I didn't do a very thorough analysis
> of the restrictions on unloading modules, but here was my line of
> reasoning:
...
> 2. Note that the ivtvfb driver is not automatically loaded nor unloaded
> by the kernel ivtv driver.  Something from userspace will reqeust the
> load and unload of the module.
> 
> There are windows of time where a struct device * will exist for a card
> in the ivtv driver, but a struct v4l2_device * may not: the end of
> ivtv_remove() and the beginning of ivtv_probe().

If there is no locking or refcounting, this won't change with the added
check. The window is still there, but it begins after the check with
your patch. Hence will still cause oopses.

> I was contemplating a case where user space requested unloading both the
> ivtvfb and the ivtv driver.  Given all the I2C devices these cards can
> have, I thought v4l2_device_unregister() at the end of ivtv_remove()
> could present a window of time large enough to worry about a race.
> v4l2_device_unregister() sets the struct device  drvdata pointer to
> NULL, and then begins unregistering the i2c clients.  I haven't profiled
> the process to know how long it typically takes, though.
> 
> I also don't know if kernel mechanisms will absolutely prevent
> initiating the unload of ivtv.ko before the unload of ivtvfb.ko is
> completely finished.  Will they?

Given ivtvfb uses functions from ivtv, no, it can't unload ivtv earlier
than ivtvfb.

regards,
-- 
js
suse labs
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/