Message ID | 20230724110204.46285-3-andriy.shevchenko@linux.intel.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:9010:0:b0:3e4:2afc:c1 with SMTP id l16csp1735159vqg; Mon, 24 Jul 2023 04:34:28 -0700 (PDT) X-Google-Smtp-Source: APBJJlFr1PfpCeDdWrUlguOlvI3EqKiU7WP8fvUJ0amsYfhcHCKf19R9Eej4rtYlH0Z9Q1iEwN4I X-Received: by 2002:a05:6a00:18aa:b0:686:290b:91f7 with SMTP id x42-20020a056a0018aa00b00686290b91f7mr6523132pfh.22.1690198467889; Mon, 24 Jul 2023 04:34:27 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1690198467; cv=none; d=google.com; s=arc-20160816; b=a+NgS0HyfBMOQgvFcBgLhApdwydRMnT3QIGEfO2KKFJUeoqSSOFk3ZlWvj102lmHFX FZ6quGhZ7YwRMNxfb+kXoYNEyqCBTbdVeE4YyBuVOLulD8e17gdiQ4JCtnK562LnV+t6 j9Szyed6BTWbt2omjRAKoI6S9/Ln+fMjIjNzp1zmz+bsjsAE0/iUVURFywGnpfz+y7uX ZcmCeKGl68jJKMj9DmDsM9iuuGApz8WBCTVBcL9zyxpgqe/gkLg5mPSpUIojKuZ2q/7P dLiLISGAZnakX0b6TS3wDqL+Z1iuCq9Dio4JOHX3zbpdP1bS+1z6IAY0KpVXEHLmZEmT ivFw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :references:in-reply-to:message-id:date:subject:cc:to:from :dkim-signature; bh=k4RR+ylOCB+AvcU+B9azRUjCf1/Luik4O9JhP/J6RzE=; fh=5utW/9xXK/HhFENJnNjAglXdo9AjnwLkfcYM2Wwa7jE=; b=sxHvUrlWIabP/wz72bA+nriAfHWFr/fQY9FTvMz57mEnsO4a6LOg18eLDiWzpwM7NE uUfQsZ40fXThO0DLlZ4/qZ2kJ1zBw2hEM1LfU5MnjUPkKTVZd9NNkGmHLAI5HbQ5KS90 kTmFswoIR7UAqsGt2FPx+73YAPUOtxIlKGiszBP78fosHAgJgq4kbhPW01hXSBS0kyyE FxsFzISaQQywwgXcx69y7wVDOnWKm1zOH9ykz4IzJE6dU9XYqqaJ5/x9myQkbe6E/rFI 4EEwyfL/c3SyM/ZHkbjIb+v5dYdXipW6YNz7Z6eAH67nv0pm/r6PrrLn9iYIrF4oqWBj LXwQ== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@intel.com header.s=Intel header.b="GP9Ylxq/"; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=intel.com Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id u27-20020a056a00099b00b00666eacb9002si9408030pfg.371.2023.07.24.04.34.14; Mon, 24 Jul 2023 04:34:27 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@intel.com header.s=Intel header.b="GP9Ylxq/"; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=intel.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S233265AbjGXLCX (ORCPT <rfc822;kloczko.tomasz@gmail.com> + 99 others); Mon, 24 Jul 2023 07:02:23 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:55976 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S233298AbjGXLCN (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Mon, 24 Jul 2023 07:02:13 -0400 Received: from mga12.intel.com (mga12.intel.com [192.55.52.136]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id EBBE21A2; Mon, 24 Jul 2023 04:02:08 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1690196528; x=1721732528; h=from:to:cc:subject:date:message-id:in-reply-to: references:mime-version:content-transfer-encoding; bh=knWfonp+Irc22tGc7fH1YtzHMLxiJcg4IV2GgKYEb+M=; b=GP9Ylxq/NvmyQE2GuXW+1tGgB0qaiCmdh8TLE763E2QvgvksijVXdwhy 3u6KA9TiiJuO9VkZWhMfUcz6463n6QlwaefFvFV41hfi57X49SizOJ3Or pEtK0x0sLPNsJ4hdj7d7Cf8lAM7uE3kOGGJ8cp441nmgvBrDr4fdPQFvo XKk4ZzBf/WogasZODq4Ei3rkUXNHyKJ7VBMrwAjHJt3LXjHdLYgllbyeO 5uiJnphbTxYmqV9uV98MazY5ji9sT8cl7jzGM4IjsVAKhZniSPc1CNrsG 2riy9MPhsEgK0RizjHBY9lmGagN0R4jT2A5npYZPR2XWhnCvgugE/D3hf w==; X-IronPort-AV: E=McAfee;i="6600,9927,10780"; a="347012828" X-IronPort-AV: E=Sophos;i="6.01,228,1684825200"; d="scan'208";a="347012828" Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by fmsmga106.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 24 Jul 2023 04:02:00 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6600,9927,10780"; a="815775355" X-IronPort-AV: E=Sophos;i="6.01,228,1684825200"; d="scan'208";a="815775355" Received: from black.fi.intel.com ([10.237.72.28]) by FMSMGA003.fm.intel.com with ESMTP; 24 Jul 2023 04:01:58 -0700 Received: by black.fi.intel.com (Postfix, from userid 1003) id C2CF31FC; Mon, 24 Jul 2023 14:02:06 +0300 (EEST) From: Andy Shevchenko <andriy.shevchenko@linux.intel.com> To: Andy Shevchenko <andriy.shevchenko@linux.intel.com>, linux-iio@vger.kernel.org, linux-kernel@vger.kernel.org Cc: Jonathan Cameron <jic23@kernel.org>, Lars-Peter Clausen <lars@metafoo.de>, =?utf-8?q?Uwe_Kleine-K=C3=B6nig?= <u.kleine-koenig@pengutronix.de>, Kees Cook <keescook@chromium.org>, Nuno Sa <nuno.sa@analog.com> Subject: [PATCH v3 2/4] iio: core: Add opaque_struct_size() helper and use it Date: Mon, 24 Jul 2023 14:02:02 +0300 Message-Id: <20230724110204.46285-3-andriy.shevchenko@linux.intel.com> X-Mailer: git-send-email 2.40.0.1.gaa8946217a0b In-Reply-To: <20230724110204.46285-1-andriy.shevchenko@linux.intel.com> References: <20230724110204.46285-1-andriy.shevchenko@linux.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-4.3 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_EF,RCVD_IN_DNSWL_MED,SPF_HELO_PASS, SPF_NONE,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1772301548432266628 X-GMAIL-MSGID: 1772301548432266628 |
Series |
iio: core: A few code cleanups and documentation fixes
|
|
Commit Message
Andy Shevchenko
July 24, 2023, 11:02 a.m. UTC
Introduce opaque_struct_size() helper, which may be moved to overflow.h in the future, and use it in the IIO core. Potential users could be (among possible others): __spi_alloc_controller() in drivers/spi/spi.c alloc_netdev_mqs in net/core/dev.c Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Cc: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> Cc: Kees Cook <keescook@chromium.org> Reviewed-by: Nuno Sa <nuno.sa@analog.com> --- drivers/iio/industrialio-core.c | 28 ++++++++++++++++++++-------- 1 file changed, 20 insertions(+), 8 deletions(-)
Comments
On Mon, Jul 24, 2023 at 02:02:02PM +0300, Andy Shevchenko wrote: > Introduce opaque_struct_size() helper, which may be moved > to overflow.h in the future, and use it in the IIO core. > > Potential users could be (among possible others): > > __spi_alloc_controller() in drivers/spi/spi.c > alloc_netdev_mqs in net/core/dev.c ... > +#define opaque_struct_size(p, a, s) size_add(ALIGN(sizeof(*(p)), (a)), (s)) This actually might need something like __safe_aling() which takes care about possible overflow. Whatever, I want to hear Kees on this.
On Mon, Jul 24, 2023 at 02:11:24PM +0300, Andy Shevchenko wrote: > On Mon, Jul 24, 2023 at 02:02:02PM +0300, Andy Shevchenko wrote: > > Introduce opaque_struct_size() helper, which may be moved > > to overflow.h in the future, and use it in the IIO core. > > > > Potential users could be (among possible others): > > > > __spi_alloc_controller() in drivers/spi/spi.c > > alloc_netdev_mqs in net/core/dev.c Can you include the specific replacement you're thinking for these? It's almost clear to me, but I'm trying to understand the benefit over what's already there. > > ... > > > +#define opaque_struct_size(p, a, s) size_add(ALIGN(sizeof(*(p)), (a)), (s)) > > This actually might need something like __safe_aling() which takes care about > possible overflow. > > Whatever, I want to hear Kees on this. i.e. if "a" were huge? What would sanity-checking of "a" look like in this case? I'm not really sure how to handle a pathological alignment request, but I'd agree it'd be nice to handle it. :) -Kees
On Mon, 24 Jul 2023 14:02:02 +0300 Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > Introduce opaque_struct_size() helper, which may be moved > to overflow.h in the future, and use it in the IIO core. > > Potential users could be (among possible others): > > __spi_alloc_controller() in drivers/spi/spi.c > alloc_netdev_mqs in net/core/dev.c > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Cc: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > Cc: Kees Cook <keescook@chromium.org> > Reviewed-by: Nuno Sa <nuno.sa@analog.com> > --- > drivers/iio/industrialio-core.c | 28 ++++++++++++++++++++-------- > 1 file changed, 20 insertions(+), 8 deletions(-) > > diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c > index b153adc5bc84..118ca6b59504 100644 > --- a/drivers/iio/industrialio-core.c > +++ b/drivers/iio/industrialio-core.c > @@ -1607,6 +1607,24 @@ const struct device_type iio_device_type = { > .release = iio_dev_release, > }; > > +/** > + * opaque_struct_size() - Calculate size of opaque structure with trailing aligned data. > + * @p: Pointer to the opaque structure. > + * @a: Alignment in bytes before trailing data. > + * @s: Data size in bytes (preferred not to be 0). > + * > + * Calculates size of memory needed for structure of @p followed by > + * the aligned data of size @s. > + * > + * Note, when @s is 0, the alignment @a is added to the sizeof(*(@p)) > + * and the result, depending on the @a, may be way off the initial size. How often is this true? A quick and dirty grep suggests at least 2 so perhaps worth retaining the old behaviour. Can we take that into account? Maybe something like #define opaque_struct_size(p, a, s) ((s) ? size_add(ALIGN(sizeof(*(p)), (a)), (s)): sizeof(*p)) Or do it at the call site below. > + * > + * Returns: Number of bytes needed or SIZE_MAX on overflow. > + */ > +#define opaque_struct_size(p, a, s) size_add(ALIGN(sizeof(*(p)), (a)), (s)) > + > +#define opaque_struct_data(p, a) PTR_ALIGN((void *)((p) + 1), (a)) > + > /** > * iio_device_alloc() - allocate an iio_dev from a driver > * @parent: Parent device. > @@ -1618,19 +1636,13 @@ struct iio_dev *iio_device_alloc(struct device *parent, int sizeof_priv) > struct iio_dev *indio_dev; > size_t alloc_size; > > - alloc_size = sizeof(struct iio_dev_opaque); > - if (sizeof_priv) { > - alloc_size = ALIGN(alloc_size, IIO_DMA_MINALIGN); > - alloc_size += sizeof_priv; > - } > - if (sizeof_priv) alloc_size = opaque_struct_size(iio_dev_opaque, IIO_DMA_MINALIGN, sizeof_priv); else alloc_size = sizeof(struct iio_dev_opaque); > + alloc_size = opaque_struct_size(iio_dev_opaque, IIO_DMA_MINALIGN, sizeof_priv); > iio_dev_opaque = kzalloc(alloc_size, GFP_KERNEL); > if (!iio_dev_opaque) > return NULL; > > indio_dev = &iio_dev_opaque->indio_dev; > - indio_dev->priv = (char *)iio_dev_opaque + > - ALIGN(sizeof(struct iio_dev_opaque), IIO_DMA_MINALIGN); > + indio_dev->priv = opaque_struct_data(iio_dev_opaque, IIO_DMA_MINALIGN); Would have been safer if original code set this to NULL if sizeof_priv == 0 A driver doing that should never have used iio_priv() but nicer if it was NULL rather than off the end of the allocation. Jonathan > > indio_dev->dev.parent = parent; > indio_dev->dev.type = &iio_device_type;
On Sat, Jul 29, 2023 at 12:46:18PM +0100, Jonathan Cameron wrote: > On Mon, 24 Jul 2023 14:02:02 +0300 > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: ... > > + * Note, when @s is 0, the alignment @a is added to the sizeof(*(@p)) > > + * and the result, depending on the @a, may be way off the initial size. > > How often is this true? A quick and dirty grep suggests at least 2 so perhaps > worth retaining the old behaviour. You mean that the sizeof(_some_grepped_struct_) is much less than an alignment in those uses? > Can we take that into account? Maybe something like > > #define opaque_struct_size(p, a, s) ((s) ? size_add(ALIGN(sizeof(*(p)), (a)), (s)): sizeof(*p)) (s) will be evaluated twice, not good. So, not in this form. > Or do it at the call site below. Looks much better to me. ... > if (sizeof_priv) > alloc_size = opaque_struct_size(iio_dev_opaque, IIO_DMA_MINALIGN, sizeof_priv); > else > alloc_size = sizeof(struct iio_dev_opaque); Right. ... > > - indio_dev->priv = (char *)iio_dev_opaque + > > - ALIGN(sizeof(struct iio_dev_opaque), IIO_DMA_MINALIGN); > > + indio_dev->priv = opaque_struct_data(iio_dev_opaque, IIO_DMA_MINALIGN); > > Would have been safer if original code set this to NULL if > sizeof_priv == 0 Yeah, original code and proposed change has no difference in this sense. > A driver doing that should never have used iio_priv() but nicer if it was > NULL rather than off the end of the allocation. Agree. But looking at the above, I would rather see that in a form of if (...) priv = opaque_struct_data(...); else priv = NULL;
On Mon, 31 Jul 2023 23:01:15 +0300 Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > On Sat, Jul 29, 2023 at 12:46:18PM +0100, Jonathan Cameron wrote: > > On Mon, 24 Jul 2023 14:02:02 +0300 > > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > ... > > > > + * Note, when @s is 0, the alignment @a is added to the sizeof(*(@p)) > > > + * and the result, depending on the @a, may be way off the initial size. > > > > How often is this true? A quick and dirty grep suggests at least 2 so perhaps > > worth retaining the old behaviour. > > You mean that the sizeof(_some_grepped_struct_) is much less than an alignment > in those uses? In two case the size of the extra space this is putting on the end of the opaque structure is 0. I've not checked how bit the main structure is - maybe it doesn't make any real difference. Ugly even so! > > > Can we take that into account? Maybe something like > > > > #define opaque_struct_size(p, a, s) ((s) ? size_add(ALIGN(sizeof(*(p)), (a)), (s)): sizeof(*p)) > > (s) will be evaluated twice, not good. So, not in this form. Good point... > > > Or do it at the call site below. > > Looks much better to me. > > ... > > > if (sizeof_priv) > > alloc_size = opaque_struct_size(iio_dev_opaque, IIO_DMA_MINALIGN, sizeof_priv); > > else > > alloc_size = sizeof(struct iio_dev_opaque); > > Right. > > ... > > > > - indio_dev->priv = (char *)iio_dev_opaque + > > > - ALIGN(sizeof(struct iio_dev_opaque), IIO_DMA_MINALIGN); > > > + indio_dev->priv = opaque_struct_data(iio_dev_opaque, IIO_DMA_MINALIGN); > > > > Would have been safer if original code set this to NULL if > > sizeof_priv == 0 > > Yeah, original code and proposed change has no difference in this sense. > > > A driver doing that should never have used iio_priv() but nicer if it was > > NULL rather than off the end of the allocation. > > Agree. > But looking at the above, I would rather see that in a form of > > if (...) > priv = opaque_struct_data(...); > else > priv = NULL; > Agreed - that would be nicer
diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c index b153adc5bc84..118ca6b59504 100644 --- a/drivers/iio/industrialio-core.c +++ b/drivers/iio/industrialio-core.c @@ -1607,6 +1607,24 @@ const struct device_type iio_device_type = { .release = iio_dev_release, }; +/** + * opaque_struct_size() - Calculate size of opaque structure with trailing aligned data. + * @p: Pointer to the opaque structure. + * @a: Alignment in bytes before trailing data. + * @s: Data size in bytes (preferred not to be 0). + * + * Calculates size of memory needed for structure of @p followed by + * the aligned data of size @s. + * + * Note, when @s is 0, the alignment @a is added to the sizeof(*(@p)) + * and the result, depending on the @a, may be way off the initial size. + * + * Returns: Number of bytes needed or SIZE_MAX on overflow. + */ +#define opaque_struct_size(p, a, s) size_add(ALIGN(sizeof(*(p)), (a)), (s)) + +#define opaque_struct_data(p, a) PTR_ALIGN((void *)((p) + 1), (a)) + /** * iio_device_alloc() - allocate an iio_dev from a driver * @parent: Parent device. @@ -1618,19 +1636,13 @@ struct iio_dev *iio_device_alloc(struct device *parent, int sizeof_priv) struct iio_dev *indio_dev; size_t alloc_size; - alloc_size = sizeof(struct iio_dev_opaque); - if (sizeof_priv) { - alloc_size = ALIGN(alloc_size, IIO_DMA_MINALIGN); - alloc_size += sizeof_priv; - } - + alloc_size = opaque_struct_size(iio_dev_opaque, IIO_DMA_MINALIGN, sizeof_priv); iio_dev_opaque = kzalloc(alloc_size, GFP_KERNEL); if (!iio_dev_opaque) return NULL; indio_dev = &iio_dev_opaque->indio_dev; - indio_dev->priv = (char *)iio_dev_opaque + - ALIGN(sizeof(struct iio_dev_opaque), IIO_DMA_MINALIGN); + indio_dev->priv = opaque_struct_data(iio_dev_opaque, IIO_DMA_MINALIGN); indio_dev->dev.parent = parent; indio_dev->dev.type = &iio_device_type;