[PATCH] spi: core: make zero length transfer valid again

[PATCH] spi: core: make zero length transfer valid again

[Atsushi Nemoto]

Zero length transfer becomes invalid since
"spi: core: Validate length of the transfers in message" commit,
but it should be valid to support an odd device, for example, which
requires long delay between chipselect and the first transfer, etc.

Signed-off-by: Atsushi Nemoto <anemo-7JcRY8pycbNHfZP73Gtkiw@public.gmane.org>
---
 drivers/spi/spi.c |    5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index c2605aa..454a523 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -1808,7 +1808,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;
+	int w_size;
 
 	if (list_empty(&message->transfers))
 		return -EINVAL;
@@ -1871,9 +1871,8 @@ static int __spi_validate(struct spi_device *spi, struct spi_message *message)
 		else
 			w_size = 4;
 
-		n_words = xfer->len / w_size;
 		/* No partial transfers accepted */
-		if (!n_words || xfer->len % w_size)
+		if (xfer->len % w_size)
 			return -EINVAL;
 
 		if (xfer->speed_hz && master->min_speed_hz &&
-- 
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

[PATCH] spi: core: make zero length transfer valid again

[Atsushi Nemoto]

Zero length transfer becomes invalid since
"spi: core: Validate length of the transfers in message" commit,
but it should be valid to support an odd device, for example, which
requires long delay between chipselect and the first transfer, etc.

Signed-off-by: Atsushi Nemoto <anemo@mba.ocn.ne.jp>
---
 drivers/spi/spi.c |    5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index c2605aa..454a523 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -1808,7 +1808,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;
+	int w_size;
 
 	if (list_empty(&message->transfers))
 		return -EINVAL;
@@ -1871,9 +1871,8 @@ static int __spi_validate(struct spi_device *spi, struct spi_message *message)
 		else
 			w_size = 4;
 
-		n_words = xfer->len / w_size;
 		/* No partial transfers accepted */
-		if (!n_words || xfer->len % w_size)
+		if (xfer->len % w_size)
 			return -EINVAL;
 
 		if (xfer->speed_hz && master->min_speed_hz &&
-- 
1.7.9.5

Re: [PATCH] spi: core: make zero length transfer valid again

[Mark Brown]

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

On Fri, Feb 28, 2014 at 11:03:16PM +0900, Atsushi Nemoto wrote:
> Zero length transfer becomes invalid since
> "spi: core: Validate length of the transfers in message" commit,
> but it should be valid to support an odd device, for example, which
> requires long delay between chipselect and the first transfer, etc.

Applied, thanks.

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

Re: [PATCH] spi: core: make zero length transfer valid again

[Mark Brown]

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

On Fri, Feb 28, 2014 at 11:03:16PM +0900, Atsushi Nemoto wrote:
> Zero length transfer becomes invalid since
> "spi: core: Validate length of the transfers in message" commit,
> but it should be valid to support an odd device, for example, which
> requires long delay between chipselect and the first transfer, etc.

Applied, thanks.

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

Re: [PATCH] spi: core: make zero length transfer valid again

[Martin Sperl]

On 01.03.2014, at 05:13, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> On Fri, Feb 28, 2014 at 11:03:16PM +0900, Atsushi Nemoto wrote:
>> Zero length transfer becomes invalid since
>> "spi: core: Validate length of the transfers in message" commit,
>> but it should be valid to support an odd device, for example, which
>> requires long delay between chipselect and the first transfer, etc.

This "odd-device support" described sounds a like a work-arround
for missing functionality in spi_core.

Would it not be better to implement this as a separate member -
say: spi_transfer.pre_transfer_delay_usecs - and keep the 
spi_transfer.len > 0 requirement? 
Initially maybe make it a warning to find those odd-devices...

I am not sure if it might make some bus-drivers more complicated
/inefficient just to support this zero length.

For example: the spi-bcm2835.c driver would do the following with a 
spi_transfer.len == 0 in the transfer_on method:
* enables SPI and wait for interrupt completion
* the above which will trigger an interrupt
** in the interrupt we find out that there is nothing to transfer,
    so we signal completion to transfer_one, so it may continue.
* the main transfer_one will get woken up
** it will do a delay_usecs
** it will handle CS_CHANGE
** it will disable SPI/reset HW again

So this implementation shows that there is a lot of inefficient 
overhead/delay just to trigger a delay...
This example requires 2 context switches (dwait for completion)
and its corresponding delays to get back to processing - so the
effective delay may be longer than 2ms just because of the delays 
introduced via the scheduler and thus way above the delay requested
by the transfer...

OK - for the spi-bcm2835.c driver the following in 
bcm2835_spi_start_transfer:
    if (xfer->len == 0) 
	return 0;
would solve it, but then we might implement this:
    if (xfer->pre_transfer_delay_usecs)
        udelay(xfer->pre_transfer_delay_usecs);
instead and be more explicit about this delay.

I guess other drivers will show similar code-artefacts and
some may even make the implicit assumption it has to be non-zero,
which would break functionality those odd devices.


Martin


--
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] spi: core: make zero length transfer valid again

[Martin Sperl]

On 01.03.2014, at 05:13, Mark Brown <broonie@kernel.org> wrote:
> On Fri, Feb 28, 2014 at 11:03:16PM +0900, Atsushi Nemoto wrote:
>> Zero length transfer becomes invalid since
>> "spi: core: Validate length of the transfers in message" commit,
>> but it should be valid to support an odd device, for example, which
>> requires long delay between chipselect and the first transfer, etc.

This "odd-device support" described sounds a like a work-arround
for missing functionality in spi_core.

Would it not be better to implement this as a separate member -
say: spi_transfer.pre_transfer_delay_usecs - and keep the 
spi_transfer.len > 0 requirement? 
Initially maybe make it a warning to find those odd-devices...

I am not sure if it might make some bus-drivers more complicated
/inefficient just to support this zero length.

For example: the spi-bcm2835.c driver would do the following with a 
spi_transfer.len == 0 in the transfer_on method:
* enables SPI and wait for interrupt completion
* the above which will trigger an interrupt
** in the interrupt we find out that there is nothing to transfer,
    so we signal completion to transfer_one, so it may continue.
* the main transfer_one will get woken up
** it will do a delay_usecs
** it will handle CS_CHANGE
** it will disable SPI/reset HW again

So this implementation shows that there is a lot of inefficient 
overhead/delay just to trigger a delay...
This example requires 2 context switches (dwait for completion)
and its corresponding delays to get back to processing - so the
effective delay may be longer than 2ms just because of the delays 
introduced via the scheduler and thus way above the delay requested
by the transfer...

OK - for the spi-bcm2835.c driver the following in 
bcm2835_spi_start_transfer:
    if (xfer->len == 0) 
	return 0;
would solve it, but then we might implement this:
    if (xfer->pre_transfer_delay_usecs)
        udelay(xfer->pre_transfer_delay_usecs);
instead and be more explicit about this delay.

I guess other drivers will show similar code-artefacts and
some may even make the implicit assumption it has to be non-zero,
which would break functionality those odd devices.


Martin

Re: [PATCH] spi: core: make zero length transfer valid again

[Mark Brown]

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

On Sat, Mar 01, 2014 at 12:40:39PM +0100, Martin Sperl wrote:

> I am not sure if it might make some bus-drivers more complicated
> /inefficient just to support this zero length.

For most of them it should be relatively straightforward, especially as
we factor things out into the core so that the drivers don't need to
implement the delays at all and it's all handled in the core.

> For example: the spi-bcm2835.c driver would do the following with a
> spi_transfer.len == 0 in the transfer_on method:
> * enables SPI and wait for interrupt completion
> * the above which will trigger an interrupt
> ** in the interrupt we find out that there is nothing to transfer,
>     so we signal completion to transfer_one, so it may continue.
> * the main transfer_one will get woken up
> ** it will do a delay_usecs
> ** it will handle CS_CHANGE
> ** it will disable SPI/reset HW again

> So this implementation shows that there is a lot of inefficient
> overhead/delay just to trigger a delay...

You really ought to be deferring to task context to implement the delays
anyway - delaying in interrupt context is rude (though doable for very
short delays).  I'd have expected that the enable/disable to be
bypassable, unless the hardware needs to be reset between transfers this
should only be happening when the device goes idle.

> I guess other drivers will show similar code-artefacts and
> some may even make the implicit assumption it has to be non-zero,
> which would break functionality those odd devices.

There's lots of stuff that's broken with individual drivers - anything
that relies on cs_change is going to break with half the drivers out
there.  Some of this is legitimate hardware limitations (where the /CS
control is out of the control of software) but a lot of it is just bugs
due to people open coding things.

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

Re: [PATCH] spi: core: make zero length transfer valid again

[Mark Brown]

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

On Sat, Mar 01, 2014 at 12:40:39PM +0100, Martin Sperl wrote:

> I am not sure if it might make some bus-drivers more complicated
> /inefficient just to support this zero length.

For most of them it should be relatively straightforward, especially as
we factor things out into the core so that the drivers don't need to
implement the delays at all and it's all handled in the core.

> For example: the spi-bcm2835.c driver would do the following with a
> spi_transfer.len == 0 in the transfer_on method:
> * enables SPI and wait for interrupt completion
> * the above which will trigger an interrupt
> ** in the interrupt we find out that there is nothing to transfer,
>     so we signal completion to transfer_one, so it may continue.
> * the main transfer_one will get woken up
> ** it will do a delay_usecs
> ** it will handle CS_CHANGE
> ** it will disable SPI/reset HW again

> So this implementation shows that there is a lot of inefficient
> overhead/delay just to trigger a delay...

You really ought to be deferring to task context to implement the delays
anyway - delaying in interrupt context is rude (though doable for very
short delays).  I'd have expected that the enable/disable to be
bypassable, unless the hardware needs to be reset between transfers this
should only be happening when the device goes idle.

> I guess other drivers will show similar code-artefacts and
> some may even make the implicit assumption it has to be non-zero,
> which would break functionality those odd devices.

There's lots of stuff that's broken with individual drivers - anything
that relies on cs_change is going to break with half the drivers out
there.  Some of this is legitimate hardware limitations (where the /CS
control is out of the control of software) but a lot of it is just bugs
due to people open coding things.

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

Re: [PATCH] spi: core: make zero length transfer valid again

[Thierry Reding]

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

On Fri, Feb 28, 2014 at 11:03:16PM +0900, Atsushi Nemoto wrote:
> Zero length transfer becomes invalid since
> "spi: core: Validate length of the transfers in message" commit,
> but it should be valid to support an odd device, for example, which
> requires long delay between chipselect and the first transfer, etc.
> 
> Signed-off-by: Atsushi Nemoto <anemo@mba.ocn.ne.jp>
> ---
>  drivers/spi/spi.c |    5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)

This fixes a regression on Venice2, where the keyboard controller relied
on similar behaviour, so even if this is already applied,

Tested-by: Thierry Reding <treding@nvidia.com>

Thanks,
Thierry

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