On 07/20/2014 05:08 PM, Jonathan Cameron wrote:
On 17/07/14 16:59, Lars-Peter Clausen wrote:
ALIGN() only works correctly if the alignment is a power of two. Some drivers
use 3 bytes for the storage size of the word in which case ALIGN() will cause
incorrect results. Use the more generic roundup() instead.
Signed-off-by: Lars-Peter Clausen <lars@xxxxxxxxxx>
Gah. Which driver is using a non power of two storage size? I thought I'd
caught
all of those at review. As you've noted it's a very bad idea.
From a quick dubious grep I have:
dac/ad5791.c (not effected as such given we aren't using this stuff for output)
pressure/st_pressure_core.c
Yes, this were the two I was seeing as well. But its noteworthy that the
ad5791 driver doesn't have any buffer support yet, so that's not really an
issue.
I'd be tempted to fix those up rather than change this and introduce the
fun you've pointed out below.
What do you think?
I'm all for fixing this up and also adding a big return -EINVAL in case
somebody tries to register a channel with a non power of two storage size.
But I can also see potential issues here with hardware that has 24-bit
samples and tightly packs them when transferring them over the bus. E.g. a 2
channel sensor with two 24-bit that are transferred in a 48-bit word.
Especially for high-speed converters having a extra step that adds a 8-bit
gap after/before each 24-bit word will be a undesirable performance
overhead. But maybe to properly support this we'll need an extension that
allows a driver to explicitly define its data layout rather than implicitly
inferring it from the sample sizes.
Btw. looking at drivers/iio/common/st_sensors/st_sensors_buffer.c it seems
as if the driver doesn't support having multiple channels with different
storage sizes for the same chip.
---
This at least makes the rules for alignment predictable and consistent. But
mixing 3 bytes words with other word sizes will still result in strange
layouts.
E.g. 4-byte word, 3-byte word will result in 4-byte word, 2-byte alignment
gap,
3-byte word.
---
drivers/iio/industrialio-buffer.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/iio/industrialio-buffer.c
b/drivers/iio/industrialio-buffer.c
index 0472ee2..6da5272 100644
--- a/drivers/iio/industrialio-buffer.c
+++ b/drivers/iio/industrialio-buffer.c
@@ -486,7 +486,7 @@ static int iio_compute_scan_bytes(struct iio_dev
*indio_dev,
ch->scan_type.repeat;
else
length = ch->scan_type.storagebits / 8;
- bytes = ALIGN(bytes, length);
+ bytes = roundup(bytes, length);
bytes += length;
}
if (timestamp) {
@@ -497,7 +497,7 @@ static int iio_compute_scan_bytes(struct iio_dev
*indio_dev,
ch->scan_type.repeat;
else
length = ch->scan_type.storagebits / 8;
- bytes = ALIGN(bytes, length);
+ bytes = roundup(bytes, length);
bytes += length;
}
return bytes;
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
