On 11/05/13 13:03, Mauro Carvalho Chehab wrote:
> Em Tue, 05 Nov 2013 12:42:23 +0100
> Sylwester Nawrocki <s.nawrocki@xxxxxxxxxxx> escreveu:
>
>> 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
>
> Thanks for the tip!
>
> The following patch should do the trick (generated with -U10, in order
> to show the entire function):
>
> [PATCHv3] v4l2-async: Don't use dynamic static allocation
>
> Dynamic static allocation is evil, as Kernel stack is too low, and
> compilation complains about it on some archs:
>
> drivers/media/v4l2-core/v4l2-async.c:238:1: warning: 'v4l2_async_notifier_unregister' uses dynamic stack allocation [enabled by default]
>
> Instead, let's enforce a limit for the buffer.
>
> In this specific case, there's a hard limit imposed by V4L2_MAX_SUBDEVS,
> with is currently 128. That means that the buffer size can be up to
> 128x8 = 1024 bytes (on a 64bits kernel), with is too big for stack.
>
> Worse than that, someone could increase it and cause real troubles.
>
> So, let's use dynamically allocated data, instead.
>
> Signed-off-by: Mauro Carvalho Chehab <m.chehab@xxxxxxxxxxx>
> Cc: Guennadi Liakhovetski <g.liakhovetski@xxxxxx>
>
> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> index c85d69da35bd..b56c9f300ecb 100644
> --- a/drivers/media/v4l2-core/v4l2-async.c
> +++ b/drivers/media/v4l2-core/v4l2-async.c
> @@ -182,59 +182,84 @@ int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev,
>
> return 0;
> }
> EXPORT_SYMBOL(v4l2_async_notifier_register);
>
> 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(n_subdev * sizeof(*dev), GFP_KERNEL);
> + if (!dev) {
> + dev_err(notifier->v4l2_dev->dev,
> + "Failed to allocate device cache!\n");
> + }
> +
> 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);
> + struct device *d;
> +
> + d = get_device(sd->dev);
I would combine these two lines in one, but that's just me :-)
>
> v4l2_async_cleanup(sd);
>
> /* If we handled USB devices, we'd have to lock the parent too */
> - device_release_driver(dev[i++]);
> + device_release_driver(d);
> +
> +
> + /*
> + * Store device at the device cache, in order to call
> + * put_device() on the final step
> + */
> + if (dev)
> + dev[i++] = d;
> + else
> + put_device(d);
Shouldn't the put_device be moved to after the unbind? It certainly would
'feel' safer that way...
>
> if (notifier->unbind)
> notifier->unbind(notifier, sd, sd->asd);
> }
>
> mutex_unlock(&list_lock);
>
> + /*
> + * Call device_attach() to reprobe devices
> + *
> + * NOTE: If dev allocation fails, i is 0, and the hole loop won't be
Typo: hole -> whole
> + * executed.
> + */
> 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);
>
> Regards,
> Mauro
>
Regards,
Hans
--
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
