Backport of spi-fsl-spi bugfix to 3.14.y

Backport of spi-fsl-spi bugfix to 3.14.y

[Esben Haabendal]

Hi

Please consider commit 4302a59629f7a0bd70fd1605d2b558597517372a
(spi: fsl: Fix problem with multi message transfers) for inclusion in
stable kernel 3.14.

It fixes a regression when using mmc_spi driver on top of spi-fsl-spi
driver (tested with powerpc mpc8313 based board).

Best regards,
Esben Haabendal

Re: Backport of spi-fsl-spi bugfix to 3.14.y

[Mark Brown]

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

On Mon, Jan 05, 2015 at 04:42:44PM +0100, Esben Haabendal wrote:

> Please consider commit 4302a59629f7a0bd70fd1605d2b558597517372a
> (spi: fsl: Fix problem with multi message transfers) for inclusion in
> stable kernel 3.14.

> It fixes a regression when using mmc_spi driver on top of spi-fsl-spi
> driver (tested with powerpc mpc8313 based board).

Has this ever worked?

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

Re: Backport of spi-fsl-spi bugfix to 3.14.y

[Esben Haabendal]

On 5 January 2015 at 16:48, Mark Brown <broonie@kernel.org> wrote:
> On Mon, Jan 05, 2015 at 04:42:44PM +0100, Esben Haabendal wrote:
>
>> Please consider commit 4302a59629f7a0bd70fd1605d2b558597517372a
>> (spi: fsl: Fix problem with multi message transfers) for inclusion in
>> stable kernel 3.14.
>
>> It fixes a regression when using mmc_spi driver on top of spi-fsl-spi
>> driver (tested with powerpc mpc8313 based board).
>
> Has this ever worked?

Yes, I have used mmc_spi with spi-fsl-spi on the same board with success
with in 2.6.33, 3.0 and 3.4.

I have no idea what has caused this to break.  Maybe mmc_spi driver was
not doing multi message transfers earlier?

/Esben

Re: Backport of spi-fsl-spi bugfix to 3.14.y

[Mark Brown]

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

On Tue, Jan 06, 2015 at 10:15:47PM +0100, Esben Haabendal wrote:
> On 5 January 2015 at 16:48, Mark Brown <broonie@kernel.org> wrote:

> >> Please consider commit 4302a59629f7a0bd70fd1605d2b558597517372a
> >> (spi: fsl: Fix problem with multi message transfers) for inclusion in
> >> stable kernel 3.14.

> >> It fixes a regression when using mmc_spi driver on top of spi-fsl-spi
> >> driver (tested with powerpc mpc8313 based board).

> > Has this ever worked?

> Yes, I have used mmc_spi with spi-fsl-spi on the same board with success
> with in 2.6.33, 3.0 and 3.4.

> I have no idea what has caused this to break.  Maybe mmc_spi driver was
> not doing multi message transfers earlier?

So, I now see this has actually been merged into stable without getting
to the bottom of why it's helping anything...  this is a bit worrying
since it's adding new functionality and not really a bug fix.  It'd have
been good to at least get a followup here.

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

Re: Backport of spi-fsl-spi bugfix to 3.14.y

[Greg Kroah-Hartman]

On Fri, Jan 23, 2015 at 12:45:29PM +0000, Mark Brown wrote:
> On Tue, Jan 06, 2015 at 10:15:47PM +0100, Esben Haabendal wrote:
> > On 5 January 2015 at 16:48, Mark Brown <broonie@kernel.org> wrote:
> 
> > >> Please consider commit 4302a59629f7a0bd70fd1605d2b558597517372a
> > >> (spi: fsl: Fix problem with multi message transfers) for inclusion in
> > >> stable kernel 3.14.
> 
> > >> It fixes a regression when using mmc_spi driver on top of spi-fsl-spi
> > >> driver (tested with powerpc mpc8313 based board).
> 
> > > Has this ever worked?
> 
> > Yes, I have used mmc_spi with spi-fsl-spi on the same board with success
> > with in 2.6.33, 3.0 and 3.4.
> 
> > I have no idea what has caused this to break.  Maybe mmc_spi driver was
> > not doing multi message transfers earlier?
> 
> So, I now see this has actually been merged into stable without getting
> to the bottom of why it's helping anything...  this is a bit worrying
> since it's adding new functionality and not really a bug fix.  It'd have
> been good to at least get a followup here.

Do you want me to revert something from the stable tree?  If so, has it
also been reverted upstream?

confused,

greg k-h

Re: Backport of spi-fsl-spi bugfix to 3.14.y

[Mark Brown]

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

On Sun, Jan 25, 2015 at 09:38:53AM -0800, Greg Kroah-Hartman wrote:
> On Fri, Jan 23, 2015 at 12:45:29PM +0000, Mark Brown wrote:

> > So, I now see this has actually been merged into stable without getting
> > to the bottom of why it's helping anything...  this is a bit worrying
> > since it's adding new functionality and not really a bug fix.  It'd have
> > been good to at least get a followup here.

> Do you want me to revert something from the stable tree?  If so, has it
> also been reverted upstream?

The patch just isn't an obvious -stable patch - it's adding new
functionality, not fixing a bug, and I was surprised to discover it had
been added as a result.  It's fine as new functionality and therefore
hasn't been reverted upstream.

We can probably leave it there, though if this is OK that's a bit of a
change, but if this new functionality is a good fix then we're probably
leaving other systems unfixed which isn't good, I would be very much
happier if we understood what had changed here.  If new functionality is
OK there's a probably a bunch of other changes we should be pulling
back.

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

Re: Backport of spi-fsl-spi bugfix to 3.14.y

[Greg Kroah-Hartman]

On Sun, Jan 25, 2015 at 08:12:54PM +0000, Mark Brown wrote:
> On Sun, Jan 25, 2015 at 09:38:53AM -0800, Greg Kroah-Hartman wrote:
> > On Fri, Jan 23, 2015 at 12:45:29PM +0000, Mark Brown wrote:
> 
> > > So, I now see this has actually been merged into stable without getting
> > > to the bottom of why it's helping anything...  this is a bit worrying
> > > since it's adding new functionality and not really a bug fix.  It'd have
> > > been good to at least get a followup here.
> 
> > Do you want me to revert something from the stable tree?  If so, has it
> > also been reverted upstream?
> 
> The patch just isn't an obvious -stable patch - it's adding new
> functionality, not fixing a bug, and I was surprised to discover it had
> been added as a result.  It's fine as new functionality and therefore
> hasn't been reverted upstream.
> 
> We can probably leave it there, though if this is OK that's a bit of a
> change, but if this new functionality is a good fix then we're probably
> leaving other systems unfixed which isn't good, I would be very much
> happier if we understood what had changed here.  If new functionality is
> OK there's a probably a bunch of other changes we should be pulling
> back.

New functionality shouldn't be added, but sometimes things slip in due
to the volume of patches and how people word-smith changelog entries :(

greg k-h

Re: Backport of spi-fsl-spi bugfix to 3.14.y

[Esben Haabendal]

On Sun, 2015-01-25 at 14:07 -0800, Greg Kroah-Hartman wrote:
> On Sun, Jan 25, 2015 at 08:12:54PM +0000, Mark Brown wrote:
> > On Sun, Jan 25, 2015 at 09:38:53AM -0800, Greg Kroah-Hartman wrote:
> > > On Fri, Jan 23, 2015 at 12:45:29PM +0000, Mark Brown wrote:
> > 
> > > > So, I now see this has actually been merged into stable without getting
> > > > to the bottom of why it's helping anything...  this is a bit worrying
> > > > since it's adding new functionality and not really a bug fix.  It'd have
> > > > been good to at least get a followup here.
> > 
> > > Do you want me to revert something from the stable tree?  If so, has it
> > > also been reverted upstream?
> > 
> > The patch just isn't an obvious -stable patch - it's adding new
> > functionality, not fixing a bug, and I was surprised to discover it had
> > been added as a result.  It's fine as new functionality and therefore
> > hasn't been reverted upstream.
> > 
> > We can probably leave it there, though if this is OK that's a bit of a
> > change, but if this new functionality is a good fix then we're probably
> > leaving other systems unfixed which isn't good, I would be very much
> > happier if we understood what had changed here.  If new functionality is
> > OK there's a probably a bunch of other changes we should be pulling
> > back.
> 
> New functionality shouldn't be added, but sometimes things slip in due
> to the volume of patches and how people word-smith changelog entries :(

In this case, I don't believe anybody has tried to fiddle with changelog
entried.

I merely stated that the commit in question fixed an (observed)
regression.  The commit itself states that it changes behaviour of the
driver, although my interpretation of this change in behaviour still is
that it replaces a clearly broken behaviour (returning -EINVAL for all
multi message transfers) with a proper handling of these.

I suspect that the regression I have observed is caused by the
combination of the following two commits:

commit 059b8ffeee5b427949872bb6ed5db5ae0788054e
  spi: make sure all transfer has proper speed set

commit e6811d1d7a6a38ee637fe219c3b67dbfe17e8b3f
  spi: make sure all transfer has bits_per_word set

And the broken behaviour of the spi-fsl-spi driver prior to the (now
backported) commit 4302a59629f7a0bd70fd1605d2b558597517372a.

A side effect of the two above mentioned commits is that the spi all
transfers (in multi message transfers) have their transfer speed_hz and
bits_per_word set, which is not handled properly by spi-fsl-spi.

As such, I believe the backport is a valid bugfix backport for all
stable kernels including the two spi commits mentioned.

/Esben

Re: Backport of spi-fsl-spi bugfix to 3.14.y

[Mark Brown]

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

On Wed, Feb 04, 2015 at 10:03:33AM +0100, Esben Haabendal wrote:

> I suspect that the regression I have observed is caused by the
> combination of the following two commits:

> commit 059b8ffeee5b427949872bb6ed5db5ae0788054e
>   spi: make sure all transfer has proper speed set

> commit e6811d1d7a6a38ee637fe219c3b67dbfe17e8b3f
>   spi: make sure all transfer has bits_per_word set

> And the broken behaviour of the spi-fsl-spi driver prior to the (now
> backported) commit 4302a59629f7a0bd70fd1605d2b558597517372a.

> A side effect of the two above mentioned commits is that the spi all
> transfers (in multi message transfers) have their transfer speed_hz and
> bits_per_word set, which is not handled properly by spi-fsl-spi.

OK, so this is the analysis I was looking for before we merged the patch
into stable - we shouldn't normally be merging new features in so we
really need to understand why the new feature is appropriate.  "This
seems to make it work" isn't doing that.

The other problem was that I didn't know this was in stable until after
the fact - we should try to avoid that happening in future.

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