From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-15.5 required=3.0 tests=BAYES_00,INCLUDES_CR_TRAILER, INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_2 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 78F20C433ED for ; Sun, 2 May 2021 16:08:01 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 475CF6134F for ; Sun, 2 May 2021 16:08:01 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231788AbhEBQIv convert rfc822-to-8bit (ORCPT ); Sun, 2 May 2021 12:08:51 -0400 Received: from mail.kernel.org ([198.145.29.99]:37380 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230110AbhEBQIv (ORCPT ); Sun, 2 May 2021 12:08:51 -0400 Received: from jic23-huawei (cpc108967-cmbg20-2-0-cust86.5-4.cable.virginm.net [81.101.6.87]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id 3B92661352; Sun, 2 May 2021 16:07:58 +0000 (UTC) Date: Sun, 2 May 2021 17:08:49 +0100 From: Jonathan Cameron To: "Sa, Nuno" Cc: "linux-iio@vger.kernel.org" , Jonathan Cameron Subject: Re: [RFC PATCH 1/4] iio: core: Introduce iio_push_to_buffers_with_ts_na() for non aligned case. Message-ID: <20210502170849.2f8f7dcb@jic23-huawei> In-Reply-To: References: <20210501172515.513486-1-jic23@kernel.org> <20210501172515.513486-2-jic23@kernel.org> X-Mailer: Claws Mail 3.17.8 (GTK+ 2.24.33; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8BIT Precedence: bulk List-ID: X-Mailing-List: linux-iio@vger.kernel.org On Sun, 2 May 2021 09:10:56 +0000 "Sa, Nuno" wrote: > > From: Jonathan Cameron > > Sent: Saturday, May 1, 2021 7:25 PM > > To: linux-iio@vger.kernel.org > > Cc: Jonathan Cameron > > Subject: [RFC PATCH 1/4] iio: core: Introduce > > iio_push_to_buffers_with_ts_na() for non aligned case. > > > > From: Jonathan Cameron > > > > Whilst it is almost always possible to arrange for scan data to be > > read directly into a buffer that is suitable for passing to > > iio_push_to_buffers_with_timestamp(), there are a few places where > > leading data needs to be skipped over. > > > > For these cases introduce a function that will allocate an appropriate > > sized and aligned bounce buffer (if not already allocated) and copy > > the unaligned data into that before calling > > iio_push_to_buffers_with_timestamp() on the bounce buffer. > > We tie the lifespace of this buffer to that of the iio_dev.dev > > which should ensure no memory leaks occur. > > > > Signed-off-by: Jonathan Cameron > > --- > > drivers/iio/industrialio-buffer.c | 36 > > +++++++++++++++++++++++++++++++ > > include/linux/iio/buffer.h | 4 ++++ > > include/linux/iio/iio-opaque.h | 4 ++++ > > 3 files changed, 44 insertions(+) > > > > diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio- > > buffer.c > > index 9a8e16c7e9af..818dfaa73665 100644 > > --- a/drivers/iio/industrialio-buffer.c > > +++ b/drivers/iio/industrialio-buffer.c > > @@ -1727,6 +1727,42 @@ int iio_push_to_buffers(struct iio_dev > > *indio_dev, const void *data) > > } > > EXPORT_SYMBOL_GPL(iio_push_to_buffers); > > > > +/** > > + * iio_push_to_buffers_with_ts_na() - push to registered buffer, > > + * no alignment or space requirements. > > + * @indio_dev: iio_dev structure for device. > > + * @data: channel data excluding the timestamp. > > + * @data_sz: size of data. > > + * @timestamp: timestamp for the sample data. > > + * > > + * This special variant of iio_push_to_buffers_with_timetamp() does > > + * not require space for the timestamp, or 8 byte alignment of data. > > + * It does however require an allocation on first call and additional > > + * coppies on all calls, so should be avoided if possible > > + */ > > +int iio_push_to_buffers_with_ts_na(struct iio_dev *indio_dev, > > + const void *data, > > + size_t data_sz, > > + int64_t timestamp) > > +{ > > + struct iio_dev_opaque *iio_dev_opaque = > > to_iio_dev_opaque(indio_dev); > > + > > + data_sz = min_t(size_t, indio_dev->scan_bytes, data_sz); > I'm not really sure about this. Is it really a good idea to silently truncate > the data if some erroneous driver calls this with data_sz > scan_bytes? > (I don't think it's ever valid for data_sz to be bigger than scan_bytes) It's not probably not. This is just a fairly nasty way of papering over anyone doing that. > We might be discarding data that userland actually requested. So, I'm > not sure if it would not be better to be strict here and return error, so that > driver developers could catch this early on... I'm fairly sure we'll never be discarding data userspace requested because scan_bytes will be the maximum possible and is the size of the kfifo element. Any more data that comes in is going to be dropped anyway assuming it ends up in a kfifo_put() I wondered if we wanted to enforce this? It would not seem totally silly for a driver that wasn't bothering to track the size of it's enabled channels to provide data_sz == maximum size it ever needs. Docs should definitely say if that's valid however, and it's definitely less than optimal because we may copy garbage that will then be overwritten by the timestamp. > > data_sz < scan_bytes is also probably not valid as that might happen > if some drivers call this without the scan elements properly aligned > which will definitely be problematic... Naturally in this case we would > have to take into account timestamp possible alignment fill bytes as it > could be possible (and likely) that data_sz won't include those bits... data_sz < scan_bytes is intentionally valid (and indeed I made one of the drivers do this). Given we are bouncing anyway, why insist the driver provides space for padding + timestamp? That can be done for the bounce buffer. > > Alternatively, we could be consistent with 'iio_push_to_buffers_with_timestamp()' > and assume drivers play along in terms of alignment and size > (so, removing the data_sz parameter)... > > My point is, if we are taking the data_sz parameter, maybe we should be pedantic > about it... > > + if (iio_dev_opaque->bounce_buffer_size != indio_dev- > > >scan_bytes) { > > Where do we set ' bounce_buffer_size '? I guess we do not want to > get here all the time... :( oops > > + iio_dev_opaque->bounce_buffer = > > + devm_krealloc(&indio_dev->dev, > > + iio_dev_opaque->bounce_buffer, > > + indio_dev->scan_bytes, > > GFP_KERNEL); > > I'm also not a big fan of realloc so I prefer to avoid it unless really needed... > Here, maybe we could just allocate this once during device/buffer initialization > and allocate it for the maximum possible size, i.e all scan elements enabled... We can but to do that requires more invasive changes. Perhaps not horrible ones, but we'd need to add stuff to caller drivers to configure the bounce buffer, or some flag to see they needed to enable support for this. Only 3 drivers out of all those in IIO are currently using this code (might be a few others that could take advantage but not many - so need to keep it localized). I suppose we could have an iio_unaligned_push_to_buffers_prepare() to be called from the update_scan_mode() callback (typed to be useable as that callback if nothing else to be done in the relevant drivers. > That could also make this API simpler to the point of actually be inlined... Can't make it inline without making the bounce buffer visible to the driver and I don't want the driver to be able to see it as it adds another place for things to be done wrongly in drivers. Any driver doing this is having to deal with a bounce buffer anyway, so the indirection doesn't worry me too much. > > - Nuno Sá Thanks, Jonathan