diff mbox

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

Message ID 20131105111601.2c21f918@samsung.com (mailing list archive)
State New, archived
Headers show

Commit Message

Mauro Carvalho Chehab Nov. 5, 2013, 1:16 p.m. UTC 
Em Tue, 05 Nov 2013 13:35:49 +0100
Hans Verkuil <hverkuil@xs4all.nl> escreveu:

> On 11/05/13 13:03, Mauro Carvalho Chehab wrote:
> > Em Tue, 05 Nov 2013 12:42:23 +0100
> > Sylwester Nawrocki <s.nawrocki@samsung.com> 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(&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
> > 
> > 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@samsung.com>
> > Cc: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> > 
> > 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(&notifier->list);
> >  
> >  	list_for_each_entry_safe(sd, tmp, &notifier->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 :-)

Especially inside a function, I think it looks cleaner to have it on
separate lines ;)

Anyway, this is a matter of personal taste.

> >  
> >  		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...

Agreed.

> 
> >  
> >  		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

Thanks for pointing it. My keyboard seems to have some bad contact:
sometimes, a keypress is missed here. I would be replacing it, but the thing
is that buying an US keyboard in Brazil is not easy, and my KVM switch doesn't
like Brazilian ABNT2 keyboards.

> 
> > +	 * 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

New patch enclosed. Please reply with your reviewed-by if you're ok with
it.

Thanks!
Mauro

commit 268e5878716eadfb981977041cb2f6d773b09174
Author: Mauro Carvalho Chehab <m.chehab@samsung.com>
Date:   Sat Nov 2 06:20:16 2013 -0300

    [media] 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@samsung.com>

Comments

Hans Verkuil Nov. 5, 2013, 2:18 p.m. UTC | #1 
On 11/05/13 14:16, Mauro Carvalho Chehab wrote:
> New patch enclosed. Please reply with your reviewed-by if you're ok with
> it.

Reviewed-by: Hans Verkuil <hans.verkuil@cisco.com>

Regards,

	Hans

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
index c85d69da35bd..85a6a34128a8 100644
--- a/drivers/media/v4l2-core/v4l2-async.c
+++ b/drivers/media/v4l2-core/v4l2-async.c
@@ -189,30 +189,53 @@  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(&notifier->list);
 
 	list_for_each_entry_safe(sd, tmp, &notifier->done, async_list) {
-		dev[i] = get_device(sd->dev);
+		struct device *d;
+
+		d = 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++]);
+		device_release_driver(d);
 
 		if (notifier->unbind)
 			notifier->unbind(notifier, sd, sd->asd);
+
+		/*
+		 * 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);
 	}
 
 	mutex_unlock(&list_lock);
 
+	/*
+	 * Call device_attach() to reprobe devices
+	 *
+	 * NOTE: If dev allocation fails, i is 0, and the whole loop won't be
+	 * executed.
+	 */
 	while (i--) {
 		struct device *d = dev[i];
 
@@ -228,6 +251,7 @@  void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier)
 		}
 		put_device(d);
 	}
+	kfree(dev);
 
 	notifier->v4l2_dev = NULL;