On 05/11/13 12:36, Mauro Carvalho Chehab wrote:
>>> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
>>> > > index c85d69da35bd..071596869036 100644
>>> > > --- a/drivers/media/v4l2-core/v4l2-async.c
>>> > > +++ b/drivers/media/v4l2-core/v4l2-async.c
>>> > > @@ -189,12 +189,14 @@ void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier)
>>> > > struct v4l2_subdev *sd, *tmp;
>>> > > unsigned int notif_n_subdev = notifier->num_subdevs;
>>> > > unsigned int n_subdev = min(notif_n_subdev, V4L2_MAX_SUBDEVS);
>>> > > - struct device *dev[n_subdev];
>>> > > + struct device **dev;
>>> > > int i = 0;
>>> > >
>>> > > if (!notifier->v4l2_dev)
>>> > > return;
>>> > >
>>> > > + dev = kmalloc(sizeof(*dev) * n_subdev, GFP_KERNEL);
>>> > > +
>> >
>> > No check for dev == NULL?
> Well, what should be done in this case?
>
> We could do the changes below:
>
> void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier)
> {
> struct v4l2_subdev *sd, *tmp;
> unsigned int notif_n_subdev = notifier->num_subdevs;
> unsigned int n_subdev = min(notif_n_subdev, V4L2_MAX_SUBDEVS);
> - struct device *dev[n_subdev];
> + struct device **dev;
> int i = 0;
>
> if (!notifier->v4l2_dev)
> return;
>
> + dev = kmalloc(sizeof(*dev) * n_subdev, GFP_KERNEL);
> + if (!dev) {
> + WARN_ON(true);
> + return;
> + }
> +
> mutex_lock(&list_lock);
>
> list_del(¬ifier->list);
>
> list_for_each_entry_safe(sd, tmp, ¬ifier->done, async_list) {
> dev[i] = get_device(sd->dev);
>
> v4l2_async_cleanup(sd);
>
> /* If we handled USB devices, we'd have to lock the parent too */
> device_release_driver(dev[i++]);
>
> if (notifier->unbind)
> notifier->unbind(notifier, sd, sd->asd);
> }
>
> mutex_unlock(&list_lock);
>
> while (i--) {
> struct device *d = dev[i];
>
> if (d && device_attach(d) < 0) {
> const char *name = "(none)";
> int lock = device_trylock(d);
>
> if (lock && d->driver)
> name = d->driver->name;
> dev_err(d, "Failed to re-probe to %s\n", name);
> if (lock)
> device_unlock(d);
> }
> put_device(d);
> }
> + kfree(dev);
>
> notifier->v4l2_dev = NULL;
>
> /*
> * Don't care about the waiting list, it is initialised and populated
> * upon notifier registration.
> */
> }
> EXPORT_SYMBOL(v4l2_async_notifier_unregister);
>
> But I suspect that this will cause an OOPS anyway, as the device will be
> only half-removed. So, it would likely OOPS at device removal or if the
> device got probed again, at probing time.
>
> So, IMHO, we should have at least a WARN_ON() for this case.
>
> Do you have a better idea?
This is how Guennadi's patch looked like when it used dynamic allocation:
http://www.spinics.net/lists/linux-sh/msg18194.html
--
Regards,
Sylwester
--
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
