[PATCH v2] spi: core: Validate lenght of the transfers in message

[PATCH v2] spi: core: Validate lenght of the transfers in message

[Ivan T. Ivanov]

From: "Ivan T. Ivanov" <iivanov-NEYub+7Iv8PQT0dZR+AlfA@public.gmane.org>

SPI transfer lenght should be a power-of-two multiple
of eight bits.

Signed-off-by: Ivan T. Ivanov <iivanov-NEYub+7Iv8PQT0dZR+AlfA@public.gmane.org>
---
 drivers/spi/spi.c |   17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index d0b28bb..9bed365 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -1617,6 +1617,7 @@ static int __spi_validate(struct spi_device *spi, struct spi_message *message)
 {
 	struct spi_master *master = spi->master;
 	struct spi_transfer *xfer;
+	int w_size, n_words;
 
 	if (list_empty(&message->transfers))
 		return -EINVAL;
@@ -1668,6 +1669,22 @@ static int __spi_validate(struct spi_device *spi, struct spi_message *message)
 				return -EINVAL;
 		}
 
+		/*
+		 * SPI transfer length should be multiple of SPI word size
+		 * where SPI word size should be power-of-two multiple
+		 */
+		if (xfer->bits_per_word <= 8)
+			w_size = 1;
+		else if (xfer->bits_per_word <= 16)
+			w_size = 2;
+		else
+			w_size = 4;
+
+		n_words = xfer->len / w_size;
+		/* No partial transfers accepted */
+		if (!n_words || xfer->len % xfer->bits_per_word)
+			return -EINVAL;
+
 		if (xfer->speed_hz && master->min_speed_hz &&
 		    xfer->speed_hz < master->min_speed_hz)
 			return -EINVAL;
-- 
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] spi: core: Validate lenght of the transfers in message

[Gerhard Sittig]

On Sat, Feb 15, 2014 at 08:09 +0200, Ivan T. Ivanov wrote:
> 
> From: "Ivan T. Ivanov" <iivanov-NEYub+7Iv8PQT0dZR+AlfA@public.gmane.org>
> 
> SPI transfer lenght should be a power-of-two multiple
> of eight bits.

Please re-check for "lenght" typos in subjects and commit logs
(code comments appear to be correct already).


The commit message still confuses me.  You don't check that the
length of an SPI transfer has a power-of-two length in bytes.

Instead what the check implements is IIUC
- pad "odd" bits-per-word settings before the check to full 1/2/4
  byte length specs (this is the power-of-two thing)
- the total length of the SPI transfer cannot be empty (which I'd
  consider an optimization, not a violation, and may need a
  separate discussion)
- the total length of the SPI transfer must be such that each
  "word" must be provided within a full 1/2/4 byte entity, with
  padding bits if the bits-per-word is "odd"

Is this a misunderstanding on my side?  A terminology thing?  To
me, the "SPI transfer" is the total payload and may have any
arbitrary length.  What you check for is a constraint on the
transfer's length derived from or based on the "word length"
('word' in SPI context).

So the code may be appropriate, yet the description may need an
update, to not have the next person ask the same questions again.

> @@ -1668,6 +1669,22 @@ static int __spi_validate(struct spi_device *spi, struct spi_message *message)
>  				return -EINVAL;
>  		}
>  
> +		/*
> +		 * SPI transfer length should be multiple of SPI word size
> +		 * where SPI word size should be power-of-two multiple
> +		 */
> +		if (xfer->bits_per_word <= 8)
> +			w_size = 1;
> +		else if (xfer->bits_per_word <= 16)
> +			w_size = 2;
> +		else
> +			w_size = 4;
> +
> +		n_words = xfer->len / w_size;
> +		/* No partial transfers accepted */
> +		if (!n_words || xfer->len % xfer->bits_per_word)
> +			return -EINVAL;
> +
>  		if (xfer->speed_hz && master->min_speed_hz &&
>  		    xfer->speed_hz < master->min_speed_hz)
>  			return -EINVAL;

[ just left the code here for comparison with the above description ]


virtually yours
Gerhard Sittig
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office-ynQEQJNshbs@public.gmane.org
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] spi: core: Validate lenght of the transfers in message

[Ivan T. Ivanov]

Hi, 

On Sun, 2014-02-16 at 15:25 +0100, Gerhard Sittig wrote: 
> On Sat, Feb 15, 2014 at 08:09 +0200, Ivan T. Ivanov wrote:
> > 
> > From: "Ivan T. Ivanov" <iivanov-NEYub+7Iv8PQT0dZR+AlfA@public.gmane.org>
> > 
> > SPI transfer lenght should be a power-of-two multiple
> > of eight bits.
> 
> Please re-check for "lenght" typos in subjects and commit logs
> (code comments appear to be correct already).

Thanks, will correct them.

> 
> 
> The commit message still confuses me.  You don't check that the
> length of an SPI transfer has a power-of-two length in bytes.
> 
> Instead what the check implements is IIUC
> - pad "odd" bits-per-word settings before the check to full 1/2/4
>   byte length specs (this is the power-of-two thing)
> - the total length of the SPI transfer cannot be empty (which I'd
>   consider an optimization, not a violation, and may need a
>   separate discussion)

I am not sure that I understand this one. I will say "the total
length of the SPI transfer _should not_ be empty". There is no
check for this currently.

> - the total length of the SPI transfer must be such that each
>   "word" must be provided within a full 1/2/4 byte entity, with
>   padding bits if the bits-per-word is "odd"
> 
> Is this a misunderstanding on my side?  A terminology thing?  To
> me, the "SPI transfer" is the total payload and may have any
> arbitrary length.  What you check for is a constraint on the
> transfer's length derived from or based on the "word length"
> ('word' in SPI context).
> 
> So the code may be appropriate, yet the description may need an
> update, to not have the next person ask the same questions again.
> 

I just forgot to update commit message. Will try to be more
careful next time.

Regards,
Ivan

> 
> virtually yours
> Gerhard Sittig


--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] spi: core: Validate lenght of the transfers in message

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1203 bytes --]

On Sun, Feb 16, 2014 at 03:25:12PM +0100, Gerhard Sittig wrote:
> On Sat, Feb 15, 2014 at 08:09 +0200, Ivan T. Ivanov wrote:

> - the total length of the SPI transfer cannot be empty (which I'd
>   consider an optimization, not a violation, and may need a
>   separate discussion)

We probably want to allow that for people doing fun stuff with cs_change
though I'm not convinced anything doing that is actually a good idea.

> - the total length of the SPI transfer must be such that each
>   "word" must be provided within a full 1/2/4 byte entity, with
>   padding bits if the bits-per-word is "odd"

> Is this a misunderstanding on my side?  A terminology thing?  To
> me, the "SPI transfer" is the total payload and may have any
> arbitrary length.  What you check for is a constraint on the
> transfer's length derived from or based on the "word length"
> ('word' in SPI context).

> So the code may be appropriate, yet the description may need an
> update, to not have the next person ask the same questions again.

It seems fairly clear to me - if we're transferring 16 bit words we need
the transfer to me a multiple of 16 bits and so on?  The requirement for
padding is unclear I have to say.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v2] spi: core: Validate lenght of the transfers in message

[Gerhard Sittig]

On Tue, Feb 18, 2014 at 09:09 +0900, Mark Brown wrote:
> 
> On Sun, Feb 16, 2014 at 03:25:12PM +0100, Gerhard Sittig wrote:
> > On Sat, Feb 15, 2014 at 08:09 +0200, Ivan T. Ivanov wrote:
> 
> > - the total length of the SPI transfer cannot be empty (which I'd
> >   consider an optimization, not a violation, and may need a
> >   separate discussion)
> 
> We probably want to allow that for people doing fun stuff with cs_change
> though I'm not convinced anything doing that is actually a good idea.
> 
> > - the total length of the SPI transfer must be such that each
> >   "word" must be provided within a full 1/2/4 byte entity, with
> >   padding bits if the bits-per-word is "odd"
> 
> > Is this a misunderstanding on my side?  A terminology thing?  To
> > me, the "SPI transfer" is the total payload and may have any
> > arbitrary length.  What you check for is a constraint on the
> > transfer's length derived from or based on the "word length"
> > ('word' in SPI context).
> 
> > So the code may be appropriate, yet the description may need an
> > update, to not have the next person ask the same questions again.
> 
> It seems fairly clear to me - if we're transferring 16 bit words we need
> the transfer to me a multiple of 16 bits and so on?  The requirement for
> padding is unclear I have to say.

I meant "padding" in the sense that e.g. 12bit bits-per-word
require data to be provided or consumed in 16bit quantities (2
full bytes), 20bit bits-per-word require 4 bytes per SPI word.
Why not 3 bytes?  I'd guess this is due to FIFO port width.

At least this is how I read the check which this patch
implements.


virtually yours
Gerhard Sittig
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office-ynQEQJNshbs@public.gmane.org
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] spi: core: Validate lenght of the transfers in message

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1033 bytes --]

On Tue, Feb 18, 2014 at 08:37:47PM +0100, Gerhard Sittig wrote:
> On Tue, Feb 18, 2014 at 09:09 +0900, Mark Brown wrote:

> > It seems fairly clear to me - if we're transferring 16 bit words we need
> > the transfer to me a multiple of 16 bits and so on?  The requirement for
> > padding is unclear I have to say.

> I meant "padding" in the sense that e.g. 12bit bits-per-word
> require data to be provided or consumed in 16bit quantities (2
> full bytes), 20bit bits-per-word require 4 bytes per SPI word.
> Why not 3 bytes?  I'd guess this is due to FIFO port width.

I do tend to agree that things need to be rounded up just to the nearest
byte, I'm not sure what the standard thing for hardware to require in
terms of port sizing is here - it's going to need an audit of the users
to see if we need to worry and what they do if it is an issue I think.

> At least this is how I read the check which this patch
> implements.

I don't recall the actual code now, sorry - I was mostly just following
up on the review comment here.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v2] spi: core: Validate lenght of the transfers in message

[Ivan T. Ivanov]

Hi, Please ignore this patch.

On Sat, 2014-02-15 at 08:09 +0200, Ivan T. Ivanov wrote:

> 
> +		/*
> +		 * SPI transfer length should be multiple of SPI word size
> +		 * where SPI word size should be power-of-two multiple
> +		 */
> +		if (xfer->bits_per_word <= 8)
> +			w_size = 1;
> +		else if (xfer->bits_per_word <= 16)
> +			w_size = 2;
> +		else
> +			w_size = 4;
> +
> +		n_words = xfer->len / w_size;
> +		/* No partial transfers accepted */
> +		if (!n_words || xfer->len % xfer->bits_per_word)

This should be
if (!n_words || xfer->len % w_size)


Regards,
Ivan

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html