From patchwork Tue Nov 5 12:03:18 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: 3140521 Return-Path: X-Original-To: patchwork-linux-media@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork2.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.19.201]) by patchwork2.web.kernel.org (Postfix) with ESMTP id 65C36BEEB2 for ; Tue, 5 Nov 2013 12:03:46 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id E300D20498 for ; Tue, 5 Nov 2013 12:03:44 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 1FBFD20411 for ; Tue, 5 Nov 2013 12:03:43 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754026Ab3KEMDj (ORCPT ); Tue, 5 Nov 2013 07:03:39 -0500 Received: from mailout4.w2.samsung.com ([211.189.100.14]:44411 "EHLO usmailout4.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754483Ab3KEMDZ (ORCPT ); Tue, 5 Nov 2013 07:03:25 -0500 Received: from uscpsbgm2.samsung.com (u115.gpu85.samsung.co.kr [203.254.195.115]) by usmailout4.samsung.com (Oracle Communications Messaging Server 7u4-24.01(7.0.4.24.0) 64bit (built Nov 17 2011)) with ESMTP id <0MVS003ATHHE2Q30@usmailout4.samsung.com> for linux-media@vger.kernel.org; Tue, 05 Nov 2013 07:03:24 -0500 (EST) X-AuditID: cbfec373-b7f6d6d00000330d-28-5278de8c7e77 Received: from ussync3.samsung.com ( [203.254.195.83]) by uscpsbgm2.samsung.com (USCPMTA) with SMTP id FE.02.13069.C8ED8725; Tue, 05 Nov 2013 07:03:24 -0500 (EST) Received: from localhost.localdomain ([105.144.34.10]) by ussync3.samsung.com (Oracle Communications Messaging Server 7u4-24.01 (7.0.4.24.0) 64bit (built Nov 17 2011)) with ESMTPA id <0MVS00IL9HHJTV70@ussync3.samsung.com>; Tue, 05 Nov 2013 07:03:24 -0500 (EST) Date: Tue, 05 Nov 2013 10:03:18 -0200 From: Mauro Carvalho Chehab To: Sylwester Nawrocki Cc: Hans Verkuil , Linux Media Mailing List , Mauro Carvalho Chehab , Guennadi Liakhovetski Subject: Re: [PATCHv2 22/29] v4l2-async: Don't use dynamic static allocation Message-id: <20131105100318.31da034b@samsung.com> In-reply-to: <5278D99F.5050508@samsung.com> 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> 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/hYN2eexVBBn2LeS3eb5zHZHFq8jMm i54NW1ktdpxaxGxx+E07qwOrx4ePcR6bV2h59G1ZxejxeZOcx6mvn9kDWKO4bFJSczLLUov0 7RK4Mm4fWslWsM2sYtquq6wNjJPVuxg5OSQETCSOb/zFCmGLSVy4t56ti5GLQ0hgCaPE080T GCGcHiaJDycOM4NUsQioSuxc8wrMZhMwknjV2ALWLSKgL7Fk1UWwbmaBg4wS985uYQdJCAv4 SFz6v5MFxOYVMJRY8+AKWAOngLbE+bOTodY9Y5RYdqaLqYuRA+gOJ4ndB6Qg6gUlfky+B9bL LKAlsXlbEyuELS+xec1b5gmMArOQlM1CUjYLSdkCRuZVjKKlxckFxUnpuUZ6xYm5xaV56XrJ +bmbGCEBXbyD8cUGq0OMAhyMSjy8D86WBwmxJpYVV+YeYpTgYFYS4W07UhEkxJuSWFmVWpQf X1Sak1p8iJGJg1OqgZFRnc9YXWjvG0/mXt5f9U2vj/Hb96X+nrMyIXjjc8XXtT7/i+wallhd St23O/zVZA0BuayLZyLrH/b4+sgqTl+37Yuixee6ZTNufp/l9nCBzIJ2kbv8whkzWi9rpnju FTWwuB3laa/2+rRzWNerE78qN4d6MjlpR2bZnDx0xTjP4rCZ31HvZ0osxRmJhlrMRcWJALdt WGVGAgAA 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 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 Regards, Mauro --- 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 --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); 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); 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 + * 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);