[PATCH] spi: reinitialize transfer parameters for every message

[PATCH] spi: reinitialize transfer parameters for every message

[Brian Niebuhr]

This patch fixes the setup_transfer logic to account for the case where
multiple messages to different SPI devices are queued simultaneously.
With the current logic, the second message in the queue will end up
using the transfer parameters for the previous message in the queue.

The fix is to reinitialize the transfer parameters for each message
rather than only once on the first message.

Signed-off-by: Brian Niebuhr <bniebuhr-JaPwekKOx1yaMJb+Lgu22Q@public.gmane.org>
---
 drivers/spi/spi_bitbang.c |   12 +++++++-----
 1 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/spi/spi_bitbang.c b/drivers/spi/spi_bitbang.c
index 5265330..8c0c0b8 100644
--- a/drivers/spi/spi_bitbang.c
+++ b/drivers/spi/spi_bitbang.c
@@ -259,7 +259,6 @@ static void bitbang_work(struct work_struct *work)
 	struct spi_bitbang	*bitbang =
 		container_of(work, struct spi_bitbang, work);
 	unsigned long		flags;
-	int			do_setup = -1;
 	int			(*setup_transfer)(struct spi_device *,
 					struct spi_transfer *);
 
@@ -275,6 +274,8 @@ static void bitbang_work(struct work_struct *work)
 		unsigned		tmp;
 		unsigned		cs_change;
 		int			status;
+		int			do_setup = 1;
+		int			restore_param = 0;
 
 		m = container_of(bitbang->queue.next, struct spi_message,
 				queue);
@@ -295,10 +296,11 @@ static void bitbang_work(struct work_struct *work)
 		list_for_each_entry (t, &m->transfers, transfer_list) {
 
 			/* override speed or wordsize? */
-			if (t->speed_hz || t->bits_per_word)
+			if (t->speed_hz || t->bits_per_word) {
 				do_setup = 1;
+				restore_param = 1;
+			}
 
-			/* init (-1) or override (1) transfer params */
 			if (do_setup != 0) {
 				if (!setup_transfer) {
 					status = -ENOPROTOOPT;
@@ -307,6 +309,7 @@ static void bitbang_work(struct work_struct *work)
 				status = setup_transfer(spi, t);
 				if (status < 0)
 					break;
+				do_setup = 0;
 			}
 
 			/* set up default clock polarity, and activate chip;
@@ -368,9 +371,8 @@ static void bitbang_work(struct work_struct *work)
 		m->complete(m->context);
 
 		/* restore speed and wordsize if it was overridden */
-		if (do_setup == 1)
+		if (restore_param == 1)
 			setup_transfer(spi, NULL);
-		do_setup = 0;
 
 		/* normally deactivate chipselect ... unless no error and
 		 * cs_change has hinted that the next message will probably
-- 
1.6.3.3


------------------------------------------------------------------------------
ThinkGeek and WIRED's GeekDad team up for the Ultimate 
GeekDad Father's Day Giveaway. ONE MASSIVE PRIZE to the 
lucky parental unit.  See the prize list and enter to win: 
http://p.sf.net/sfu/thinkgeek-promo

Re: [PATCH] spi: reinitialize transfer parameters for every message

[Brian Niebuhr]

On Fri, Jun 4, 2010 at 3:13 PM, Brian Niebuhr <bniebuhr3-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> This patch fixes the setup_transfer logic to account for the case where
> multiple messages to different SPI devices are queued simultaneously.
> With the current logic, the second message in the queue will end up
> using the transfer parameters for the previous message in the queue.
>
> The fix is to reinitialize the transfer parameters for each message
> rather than only once on the first message.
>
> Signed-off-by: Brian Niebuhr <bniebuhr-JaPwekKOx1yaMJb+Lgu22Q@public.gmane.org>
> ---
>  drivers/spi/spi_bitbang.c |   12 +++++++-----
>  1 files changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/spi/spi_bitbang.c b/drivers/spi/spi_bitbang.c
> index 5265330..8c0c0b8 100644
> --- a/drivers/spi/spi_bitbang.c
> +++ b/drivers/spi/spi_bitbang.c
> @@ -259,7 +259,6 @@ static void bitbang_work(struct work_struct *work)
>        struct spi_bitbang      *bitbang =
>                container_of(work, struct spi_bitbang, work);
>        unsigned long           flags;
> -       int                     do_setup = -1;
>        int                     (*setup_transfer)(struct spi_device *,
>                                        struct spi_transfer *);
>
> @@ -275,6 +274,8 @@ static void bitbang_work(struct work_struct *work)
>                unsigned                tmp;
>                unsigned                cs_change;
>                int                     status;
> +               int                     do_setup = 1;
> +               int                     restore_param = 0;
>
>                m = container_of(bitbang->queue.next, struct spi_message,
>                                queue);
> @@ -295,10 +296,11 @@ static void bitbang_work(struct work_struct *work)
>                list_for_each_entry (t, &m->transfers, transfer_list) {
>
>                        /* override speed or wordsize? */
> -                       if (t->speed_hz || t->bits_per_word)
> +                       if (t->speed_hz || t->bits_per_word) {
>                                do_setup = 1;
> +                               restore_param = 1;
> +                       }
>
> -                       /* init (-1) or override (1) transfer params */
>                        if (do_setup != 0) {
>                                if (!setup_transfer) {
>                                        status = -ENOPROTOOPT;
> @@ -307,6 +309,7 @@ static void bitbang_work(struct work_struct *work)
>                                status = setup_transfer(spi, t);
>                                if (status < 0)
>                                        break;
> +                               do_setup = 0;
>                        }
>
>                        /* set up default clock polarity, and activate chip;
> @@ -368,9 +371,8 @@ static void bitbang_work(struct work_struct *work)
>                m->complete(m->context);
>
>                /* restore speed and wordsize if it was overridden */
> -               if (do_setup == 1)
> +               if (restore_param == 1)
>                        setup_transfer(spi, NULL);
> -               do_setup = 0;
>
>                /* normally deactivate chipselect ... unless no error and
>                 * cs_change has hinted that the next message will probably
> --
> 1.6.3.3

Grant -

I am resubmitting this patch with some changes given your previous
comments.  Basically the problem that David fixed with his original
patch still exists for the case where multiple messages to different
SPI devices on the same controller are in the work queue
simultaneously.  I have tested this on a board that has multiple SPI
devices on the same bus that use dramatically different transfer
parameters.  Without the patch a stress test fails quickly because the
driver ends up using the wrong transfer parameters.  With the patch
the problems are cleared up.

David -

Can you take a look at this and see if it is still in line with the
intent of your original fix?

Thanks,

Brian

------------------------------------------------------------------------------
ThinkGeek and WIRED's GeekDad team up for the Ultimate 
GeekDad Father's Day Giveaway. ONE MASSIVE PRIZE to the 
lucky parental unit.  See the prize list and enter to win: 
http://p.sf.net/sfu/thinkgeek-promo

Re: [PATCH] spi: reinitialize transfer parameters for every message

[Grant Likely]

Vitaly and David, can you please take a look at this.  You both know
this code better than I, and I'd like as second opinion. (see my
comments below)

On Fri, Jun 4, 2010 at 2:13 PM, Brian Niebuhr <bniebuhr3-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> This patch fixes the setup_transfer logic to account for the case where
> multiple messages to different SPI devices are queued simultaneously.
> With the current logic, the second message in the queue will end up
> using the transfer parameters for the previous message in the queue.
>
> The fix is to reinitialize the transfer parameters for each message
> rather than only once on the first message.
>
> Signed-off-by: Brian Niebuhr <bniebuhr-JaPwekKOx1yaMJb+Lgu22Q@public.gmane.org>
> ---
>  drivers/spi/spi_bitbang.c |   12 +++++++-----
>  1 files changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/spi/spi_bitbang.c b/drivers/spi/spi_bitbang.c
> index 5265330..8c0c0b8 100644
> --- a/drivers/spi/spi_bitbang.c
> +++ b/drivers/spi/spi_bitbang.c
> @@ -259,7 +259,6 @@ static void bitbang_work(struct work_struct *work)
>        struct spi_bitbang      *bitbang =
>                container_of(work, struct spi_bitbang, work);
>        unsigned long           flags;
> -       int                     do_setup = -1;
>        int                     (*setup_transfer)(struct spi_device *,
>                                        struct spi_transfer *);
>
> @@ -275,6 +274,8 @@ static void bitbang_work(struct work_struct *work)
>                unsigned                tmp;
>                unsigned                cs_change;
>                int                     status;
> +               int                     do_setup = 1;
> +               int                     restore_param = 0;
>
>                m = container_of(bitbang->queue.next, struct spi_message,
>                                queue);
> @@ -295,10 +296,11 @@ static void bitbang_work(struct work_struct *work)
>                list_for_each_entry (t, &m->transfers, transfer_list) {
>
>                        /* override speed or wordsize? */
> -                       if (t->speed_hz || t->bits_per_word)
> +                       if (t->speed_hz || t->bits_per_word) {
>                                do_setup = 1;
> +                               restore_param = 1;
> +                       }
>
> -                       /* init (-1) or override (1) transfer params */
>                        if (do_setup != 0) {
>                                if (!setup_transfer) {
>                                        status = -ENOPROTOOPT;
> @@ -307,6 +309,7 @@ static void bitbang_work(struct work_struct *work)
>                                status = setup_transfer(spi, t);
>                                if (status < 0)
>                                        break;
> +                               do_setup = 0;
>                        }
>
>                        /* set up default clock polarity, and activate chip;
> @@ -368,9 +371,8 @@ static void bitbang_work(struct work_struct *work)
>                m->complete(m->context);
>
>                /* restore speed and wordsize if it was overridden */
> -               if (do_setup == 1)
> +               if (restore_param == 1)
>                        setup_transfer(spi, NULL);

Hi Brian,

It seems to me that with this change that always forces
setup_transfer() to be run at the beginning of a message, that this
setup_transfer() call becomes completely superfluous and can be
removed.  Why bother setting up the default parameters for the last
device when all transfers to that device have completed?

The rest of the patch does seem correct.  One note: the patch looks
like it removes an optimization to avoid setup_transfer() whenever
possible.  I don't have any problem with this myself.  In general I
prefer straightforward coding, and I think that each setup_transfer
hook should be smart enough anyway not to reconfigure things that
don't need to be change.  However, I'd like to know more about the
original intent of this code.  Is this just and optimization, or am I
missing something?

> -               do_setup = 0;
>
>                /* normally deactivate chipselect ... unless no error and
>                 * cs_change has hinted that the next message will probably

However, looking at this code deeper, I wonder if there is another
bug.  Let me add the lines that finish the above comment and
manipulate the cs:

>                 * be for this chip too.
>                 */
>                if (!(status == 0 && cs_change)) {
>                        ndelay(nsecs);
>                        bitbang->chipselect(spi, BITBANG_CS_INACTIVE);
>                        ndelay(nsecs);
>                }

It seems that it is trying to provide the option for leaving the CS
line active at the end of a transfer.  However, I don't see any code
that verifies that when cs_change == 0 at the end of a message, that
the next message will definitely be for the same device.  It could be
that the CS won't get explicitly deactivated before a message for
another device begins.

Granted, ->chipselect() will still get called regardless to set the
new device chip select, which /should/ implicitly clear the previous
chip select, but it won't get exactly the same behaviour and it could
result in weird transfer failures.  Thoughts?

g.

------------------------------------------------------------------------------
This SF.net email is sponsored by Sprint
What will you do first with EVO, the first 4G phone?
Visit sprint.com/first -- http://p.sf.net/sfu/sprint-com-first

Re: [PATCH] spi: reinitialize transfer parameters for every message

[David Brownell]

--- On Sat, 6/26/10, Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org> wrote:

> Vitaly and David, can you please take
> a look at this.  You both know
> this code better than I, and I'd like as second opinion.
> (see my
> comments below)

First fix $SUBJECT:  it's not a generic SPI
issue, it's just for one driver!
> 


------------------------------------------------------------------------------
This SF.net email is sponsored by Sprint
What will you do first with EVO, the first 4G phone?
Visit sprint.com/first -- http://p.sf.net/sfu/sprint-com-first

Re: [PATCH] spi: reinitialize transfer parameters for every message

[David Brownell]

> > This patch fixes the setup_transfer logic to account for the case where
> > multiple messages to different SPI devices are queued simultaneously.

Make that "sequentially"; queues are FIFO,
not parallel!!

... like it  was already supposed to be doing; but
you say it wasn't (I've nt yet evaluated that):

> > With the current logic, the second message in the queue will end up
> > using the transfer parameters for the previous message in the queue.

ISTR it was supposed to change only the
values which were incorrect.  (Which were
of course all incorrect initially, but for
later transfers might often be OK.


 
> It seems to me that with this change that always
> forces setup_transfer() to be run at the beginning of a message,
> that this
> setup_transfer() call becomes completely superfluous and
> can be
> removed.

Surely it's a useful abstraction, regardless?
And in any case it can be overridden by the
lower-level code to do things like write to
hardware registers ... I can't see removing it.


  Why bother setting up the default parameters
> for the last
> device when all transfers to that device have completed?


(have they?)
If that chip is still selected (because the
cs_change for the last transfer of the last
message said to do so) that's correct ...


>   However, I'd like to know
> more about the
> original intent of this code.  Is this just and
> optimization, or am I
> missing something?
> 
> > -               do_setup = 0;

Looks like "do_setup" semantics got
swapped around at some point too..
that zero case isn't commented as
either init/-1 or override/1 ...

> >
> >                /* normally deactivate
> chipselect ... unless no error and
> >                 * cs_change has hinted that
> the next message will probably

I recall trying to be sure this code got those
cs_change semantics correct, and having a few
messy issues associated with that ... this code
should be the semantic reference for the whole
SPI messaging behavior.  Maybe some bugs crept
in; this specific behavior can be confusing.


> However, looking at this code deeper, I wonder if there is
> another
> bug.  Let me add the lines that finish the above
> comment and
> manipulate the cs:
> 
> >             
>    * be for this chip too.


Yeah, truncated comment should get fixed 

> >             
>    */
> >               
> if (!(status == 0 && cs_change)) {
> >               
>         ndelay(nsecs);
> >               
>         bitbang->chipselect(spi,
> BITBANG_CS_INACTIVE);
> >               
>         ndelay(nsecs);
> >               
> }
> 
> It seems that it is trying to provide the option for
> leaving the CS
> line active at the end of a transfer.

Yes, though it's not done if the message
itself triggered an error



  However, I
> don't see any code
> that verifies that when cs_change == 0 at the end of a
> message, that
> the next message will definitely be for the same
> device.

Not required.  That's a hint.  If the hint is
incorrect, the driver should recover and then
activate the correct chipselect.



  It could be
> that the CS won't get explicitly deactivated before a
> message for
> another device begins.

ISTR coding it so the chip getting activated
was always the right one
 
> Granted, ->chipselect() will still get called regardless
> to set the
> new device chip select, which /should/ implicitly clear the
> previous
> chip select, but it won't get exactly the same behaviour

No, the request was to avoid toggling the
chipselect line, which can have side effects.
Calling chipselect() generally toggles it, so
the device would do something that's potentially
undesirable.  Ergo, avoid chipselect() there.


> and it could
> result in weird transfer failures.  Thoughts?




------------------------------------------------------------------------------
This SF.net email is sponsored by Sprint
What will you do first with EVO, the first 4G phone?
Visit sprint.com/first -- http://p.sf.net/sfu/sprint-com-first

Re: [PATCH] spi: reinitialize transfer parameters for every message

[Brian Niebuhr]

On Sun, Jun 27, 2010 at 3:32 AM, David Brownell <david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org> wrote:
>> > This patch fixes the setup_transfer logic to account for the case where
>> > multiple messages to different SPI devices are queued simultaneously.
>
> Make that "sequentially"; queues are FIFO,
> not parallel!!

What I was saying was that there are multiple messages in the queue at
the same time.  In other words, the loop:

while (!list_empty(&bitbang->queue)) {
...
}

will be executed multiple times before bitbang_work exits.


> ... like it  was already supposed to be doing; but
> you say it wasn't (I've nt yet evaluated that):

I think what your original patch fixed was if there were two SPI
devices in the system, and they were used alternately, the parameters
for one device would be used for the other.  So you added the do_setup
initialization to make sure that the parameters were reset every time
bitbang_work was called.  For the most part that solution solves the
problem.  However, if messages to different devices are in the queue
and bitbang_work stays in the message loop and doesn't exit, you end
up with the same problem where the parameters for one device are used
for the next device.  That's what I'm trying to solve.

>> > With the current logic, the second message in the queue will end up
>> > using the transfer parameters for the previous message in the queue.
>
> ISTR it was supposed to change only the
> values which were incorrect.  (Which were
> of course all incorrect initially, but for
> later transfers might often be OK.

Yes, I think the transfer loop (processing transfers for a single
message) is fine.  setup_transfer() gets run again only if the
parameters changed for a particular transfer within a message.  My
patch was dealing with reinitialization in the message loop, since
messages can be to different devices.

>> It seems to me that with this change that always
>> forces setup_transfer() to be run at the beginning of a message,
>> that this
>> setup_transfer() call becomes completely superfluous and
>> can be
>> removed.
>
> Surely it's a useful abstraction, regardless?
> And in any case it can be overridden by the
> lower-level code to do things like write to
> hardware registers ... I can't see removing it.
>
>   Why bother setting up the default parameters
>> for the last
>> device when all transfers to that device have completed?

I think I understand what you're saying now.  I believe you are
correct - the final setup_transfer() doesn't really do anything
because we are going to call setup_transfer() again when the next
message is processed.  I can remove that.

> (have they?)
> If that chip is still selected (because the
> cs_change for the last transfer of the last
> message said to do so) that's correct ...
>
>
>>   However, I'd like to know
>> more about the
>> original intent of this code.  Is this just and
>> optimization, or am I
>> missing something?
>>
>> > -               do_setup = 0;

Originally  do_setup was set to zero after processing the first
message in the queue.  That worked fine as long as all messages in the
queue were for the same device.  But if the next message is for a
different device then setup_transfer doesn't get called.  I moved the
do_setup=0 to fix this problem, but now that you point it out, I think
it doesn't provide the exact same behavior as before.  I'll resubmit
the patch with a fix that keeps the same behavior.

> Looks like "do_setup" semantics got
> swapped around at some point too..
> that zero case isn't commented as
> either init/-1 or override/1 ...

Yes, that is true.  do_setup was changed to be either true or false.
However, I'm going to resubmit the patch with the init/override
semantics restored.

>> >
>> >                /* normally deactivate
>> chipselect ... unless no error and
>> >                 * cs_change has hinted that
>> the next message will probably
>
> I recall trying to be sure this code got those
> cs_change semantics correct, and having a few
> messy issues associated with that ... this code
> should be the semantic reference for the whole
> SPI messaging behavior.  Maybe some bugs crept
> in; this specific behavior can be confusing.

I believe the cs_change code is correct and I didn't make any changes to that.

>> However, looking at this code deeper, I wonder if there is
>> another
>> bug.  Let me add the lines that finish the above
>> comment and
>> manipulate the cs:
>>
>> >
>>    * be for this chip too.
>
>
> Yeah, truncated comment should get fixed
>
>> >
>>    */
>> >
>> if (!(status == 0 && cs_change)) {
>> >
>>         ndelay(nsecs);
>> >
>>         bitbang->chipselect(spi,
>> BITBANG_CS_INACTIVE);
>> >
>>         ndelay(nsecs);
>> >
>> }
>>
>> It seems that it is trying to provide the option for
>> leaving the CS
>> line active at the end of a transfer.
>
> Yes, though it's not done if the message
> itself triggered an error
>
>
>
>   However, I
>> don't see any code
>> that verifies that when cs_change == 0 at the end of a
>> message, that
>> the next message will definitely be for the same
>> device.
>
> Not required.  That's a hint.  If the hint is
> incorrect, the driver should recover and then
> activate the correct chipselect.
>
>
>
>   It could be
>> that the CS won't get explicitly deactivated before a
>> message for
>> another device begins.
>
> ISTR coding it so the chip getting activated
> was always the right one
>
>> Granted, ->chipselect() will still get called regardless
>> to set the
>> new device chip select, which /should/ implicitly clear the
>> previous
>> chip select, but it won't get exactly the same behaviour
>
> No, the request was to avoid toggling the
> chipselect line, which can have side effects.
> Calling chipselect() generally toggles it, so
> the device would do something that's potentially
> undesirable.  Ergo, avoid chipselect() there.

As I mentioned above, I believe the chip select logic is correct.  It
is possible that a chip select for one device will remain active at
the end of a message, and that the next message will be for a new
device.  In that case, the chip select logic is required to deselect
the previous device and select the new device.

I will resubmit the patch again given your comments.

------------------------------------------------------------------------------
This SF.net email is sponsored by Sprint
What will you do first with EVO, the first 4G phone?
Visit sprint.com/first -- http://p.sf.net/sfu/sprint-com-first