Re: [PATCHv2 22/29] v4l2-async: Don't use dynamic static allocation

[Sylwester Nawrocki <s.nawrocki@samsung.com>]

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(&notifier->list);
>  
>         list_for_each_entry_safe(sd, tmp, &notifier->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