From patchwork Tue Nov 5 13:16:01 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Mauro Carvalho Chehab X-Patchwork-Id: 3141441 Return-Path: X-Original-To: patchwork-linux-media@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork1.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.19.201]) by patchwork1.web.kernel.org (Postfix) with ESMTP id 09F489F407 for ; Tue, 5 Nov 2013 13:16:23 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 5CF67205C4 for ; Tue, 5 Nov 2013 13:16:21 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 5665D205FC for ; Tue, 5 Nov 2013 13:16:19 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755173Ab3KENQO (ORCPT ); Tue, 5 Nov 2013 08:16:14 -0500 Received: from mailout3.w2.samsung.com ([211.189.100.13]:33538 "EHLO usmailout3.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754905Ab3KENQH (ORCPT ); Tue, 5 Nov 2013 08:16:07 -0500 Received: from uscpsbgm1.samsung.com (u114.gpu85.samsung.co.kr [203.254.195.114]) by usmailout3.samsung.com (Oracle Communications Messaging Server 7u4-24.01(7.0.4.24.0) 64bit (built Nov 17 2011)) with ESMTP id <0MVS00MO6KUUTE20@usmailout3.samsung.com> for linux-media@vger.kernel.org; Tue, 05 Nov 2013 08:16:06 -0500 (EST) X-AuditID: cbfec372-b7fe76d000003347-eb-5278ef96a418 Received: from ussync2.samsung.com ( [203.254.195.82]) by uscpsbgm1.samsung.com (USCPMTA) with SMTP id D2.45.13127.69FE8725; Tue, 05 Nov 2013 08:16:06 -0500 (EST) Received: from localhost.localdomain ([105.144.34.10]) by ussync2.samsung.com (Oracle Communications Messaging Server 7u4-23.01 (7.0.4.23.0) 64bit (built Aug 10 2011)) with ESMTPA id <0MVS00JXMKUQNP40@ussync2.samsung.com>; Tue, 05 Nov 2013 08:16:06 -0500 (EST) Date: Tue, 05 Nov 2013 11:16:01 -0200 From: Mauro Carvalho Chehab To: Hans Verkuil Cc: Sylwester Nawrocki , Linux Media Mailing List , Mauro Carvalho Chehab , Guennadi Liakhovetski Subject: Re: [PATCHv2 22/29] v4l2-async: Don't use dynamic static allocation Message-id: <20131105111601.2c21f918@samsung.com> In-reply-to: <5278E625.4020703@xs4all.nl> References: <1383399097-11615-1-git-send-email-m.chehab@samsung.com> <1383399097-11615-23-git-send-email-m.chehab@samsung.com> <52779DD8.3080401@xs4all.nl> <20131105093628.6da1a600@samsung.com> <5278D99F.5050508@samsung.com> <20131105100318.31da034b@samsung.com> <5278E625.4020703@xs4all.nl> X-Mailer: Claws Mail 3.9.2 (GTK+ 2.24.22; x86_64-redhat-linux-gnu) MIME-version: 1.0 Content-type: text/plain; charset=US-ASCII Content-transfer-encoding: 7bit X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFtrMLMWRmVeSWpSXmKPExsVy+t/hIN1p7yuCDD6sEbV4v3Eek8Wpyc+Y LHo2bGW12HFqEbPF4TftrA6sHh8+xnlsXqHl0bdlFaPH501yHqe+fmYPYI3isklJzcksSy3S t0vgylgw5xhTwZ6IiqWXD7A2ME527GLk5JAQMJHY9nEKG4QtJnHh3nogm4tDSGAJo8TDg+2M EE4Pk8S3P3OYQKpYBFQlvjVNYQax2QSMJF41trCC2CJA8de7fjGDNDALnGaU2PPlLdhYYQEf iUv/d7KA2LwChhKnX/1kBLE5BTQlljb2Qq1bzSSxv7cXqIgD6A4nid0HpCDqBSV+TL4H1sss oCWxeVsTK4QtL7F5zVvmCYwCs5CUzUJSNgtJ2QJG5lWMoqXFyQXFSem5hnrFibnFpXnpesn5 uZsYIQFdtIPx2QarQ4wCHIxKPLwPzpYHCbEmlhVX5h5ilOBgVhLhbTtSESTEm5JYWZValB9f VJqTWnyIkYmDU6qBsXPx+p55m+RUJzg1XC49eG/+xNTEjZnTetuOCTBJHyp89nPb/q/lsf+l 39flbl4y4+2CJ3pzZbRNzPyVjI8vTEm0mZfS3hflXt3xvNr7s/zc46pFn6dsZFt1837YtPvv 7ol5WfDujZ75OUy7S/t8Q/6+M1cfVz5dteDv3DdmQrqPz+0QzsthjVdiKc5INNRiLipOBAC9 VwsIRgIAAA== Sender: linux-media-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-media@vger.kernel.org X-Spam-Status: No, score=-6.9 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_HI, RP_MATCHES_RCVD, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP Em Tue, 05 Nov 2013 13:35:49 +0100 Hans Verkuil escreveu: > On 11/05/13 13:03, Mauro Carvalho Chehab wrote: > > Em Tue, 05 Nov 2013 12:42:23 +0100 > > Sylwester Nawrocki 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 > > Cc: Guennadi Liakhovetski > > > > 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 :-) 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 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 Reviewed-by: Hans Verkuil 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(¬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); 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;