[PATCH net-next v1] net: phy: spi_ks8995: Do not overwrite SPI mode flags

[PATCH net-next v1] net: phy: spi_ks8995: Do not overwrite SPI mode flags

[Sven Van Asbroeck]

From: Sven Van Asbroeck <thesven73@gmail.com>

This driver makes sure the underlying SPI bus is set to "mode 0"
by assigning SPI_MODE_0 to spi->mode. This overwrites all other
SPI mode flags.

In some circumstances, this can break the underlying SPI bus driver.
For example, if SPI_CS_HIGH is set on the SPI bus, the driver
will clear that flag, which results in a chip-select polarity issue.

Fix by changing only the SPI_MODE_N bits, i.e. SPI_CPHA and SPI_CPOL.

Signed-off-by: Sven Van Asbroeck <thesven73@gmail.com>
---

Tree: git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git # bff6f1db91e3

To: Andrew Lunn <andrew@lunn.ch>
To: Heiner Kallweit <hkallweit1@gmail.com>
To: Jakub Kicinski <kuba@kernel.org>
Cc: Russell King <linux@armlinux.org.uk>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: netdev@vger.kernel.org
Cc: linux-kernel@vger.kernel.org

 drivers/net/phy/spi_ks8995.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/spi_ks8995.c b/drivers/net/phy/spi_ks8995.c
index 4b198399bfa2..3c6c87a09b03 100644
--- a/drivers/net/phy/spi_ks8995.c
+++ b/drivers/net/phy/spi_ks8995.c
@@ -491,7 +491,9 @@ static int ks8995_probe(struct spi_device *spi)
 
 	spi_set_drvdata(spi, ks);
 
-	spi->mode = SPI_MODE_0;
+	/* use SPI_MODE_0 without changing any other mode flags */
+	spi->mode &= ~(SPI_CPHA | SPI_CPOL);
+	spi->mode |= SPI_MODE_0;
 	spi->bits_per_word = 8;
 	err = spi_setup(spi);
 	if (err) {
-- 
2.17.1

Re: [PATCH net-next v1] net: phy: spi_ks8995: Do not overwrite SPI mode flags

[Andrew Lunn]

> +++ b/drivers/net/phy/spi_ks8995.c
> @@ -491,7 +491,9 @@ static int ks8995_probe(struct spi_device *spi)
>  
>  	spi_set_drvdata(spi, ks);
>  
> -	spi->mode = SPI_MODE_0;
> +	/* use SPI_MODE_0 without changing any other mode flags */
> +	spi->mode &= ~(SPI_CPHA | SPI_CPOL);
> +	spi->mode |= SPI_MODE_0;
>  	spi->bits_per_word = 8;

Did you check to see if there is a help to set just the mode without
changing any of the other bits?

	 Andrew

Re: [PATCH net-next v1] net: phy: spi_ks8995: Do not overwrite SPI mode flags

[Sven Van Asbroeck]

On Mon, Nov 9, 2020 at 2:49 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> Did you check to see if there is a help to set just the mode without
> changing any of the other bits?

Absolutely, but it doesn't exist, AFAIK.

It would be great if client spi drivers would use a helper function like
that. The spi bus driver on my platform (imx ecspi) was recently changed
upstream, which means SPI_CS_HIGH is now always set, irrespective
of the actual chip select polarity. This change breaks every single spi
driver which "plows over" the spi->mode flags. And there are quite
a few...

Sven

Re: [PATCH net-next v1] net: phy: spi_ks8995: Do not overwrite SPI mode flags

[Andrew Lunn]

On Mon, Nov 09, 2020 at 02:56:38PM -0500, Sven Van Asbroeck wrote:
> On Mon, Nov 9, 2020 at 2:49 PM Andrew Lunn <andrew@lunn.ch> wrote:
> >
> > Did you check to see if there is a help to set just the mode without
> > changing any of the other bits?
> 
> Absolutely, but it doesn't exist, AFAIK.

> It would be great if client spi drivers would use a helper function like
> that.

Then you should consider adding it, and cross post the SPI list.

     Andrew

Re: [PATCH net-next v1] net: phy: spi_ks8995: Do not overwrite SPI mode flags

[Sven Van Asbroeck]

On Mon, Nov 9, 2020 at 3:26 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> Then you should consider adding it, and cross post the SPI list.
>

Good idea. I will give that a try.

Re: [PATCH net-next v1] net: phy: spi_ks8995: Do not overwrite SPI mode flags

[Jakub Kicinski]

On Mon,  9 Nov 2020 14:31:17 -0500 Sven Van Asbroeck wrote:
> From: Sven Van Asbroeck <thesven73@gmail.com>
> 
> This driver makes sure the underlying SPI bus is set to "mode 0"
> by assigning SPI_MODE_0 to spi->mode. This overwrites all other
> SPI mode flags.
> 
> In some circumstances, this can break the underlying SPI bus driver.
> For example, if SPI_CS_HIGH is set on the SPI bus, the driver
> will clear that flag, which results in a chip-select polarity issue.
> 
> Fix by changing only the SPI_MODE_N bits, i.e. SPI_CPHA and SPI_CPOL.
> 
> Signed-off-by: Sven Van Asbroeck <thesven73@gmail.com>

This is a fix right? You seem to be targeting net-next and there is no
Fixes tag but it sounds like a bug.

Re: [PATCH net-next v1] net: phy: spi_ks8995: Do not overwrite SPI mode flags

[Sven Van Asbroeck]

On Mon, Nov 9, 2020 at 4:09 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> This is a fix right? You seem to be targeting net-next and there is no
> Fixes tag but it sounds like a bug.

I'm not sure. The original code used to work for me, until the spi bus
driver I'm using to communicate to this chip was changed to always
require SPI_CS_HIGH. The current ks8995 driver will now plow over
this flag, and spi communication breaks.

Is this a bug? If so, what should its Fixes commit be? The spi commit
upstream that enables SPI_CS_HIGH on my platform?

Re: [PATCH net-next v1] net: phy: spi_ks8995: Do not overwrite SPI mode flags

[Jakub Kicinski]

On Mon, 9 Nov 2020 16:20:46 -0500 Sven Van Asbroeck wrote:
> Is this a bug? If so, what should its Fixes commit be? The spi commit
> upstream that enables SPI_CS_HIGH on my platform?

I'd put two fixes tags one for the spi commit and one for the commit
which introduced the assignment in the client driver.

Re: [PATCH net-next v1] net: phy: spi_ks8995: Do not overwrite SPI mode flags

[Sven Van Asbroeck]

On Mon, Nov 9, 2020 at 4:24 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> the commit
> which introduced the assignment in the client driver.

That's the commit which adds the initial driver to the tree, back in 2011.
Should I use that for Fixes?

Re: [PATCH net-next v1] net: phy: spi_ks8995: Do not overwrite SPI mode flags

[Jakub Kicinski]

On Mon, 9 Nov 2020 16:29:19 -0500 Sven Van Asbroeck wrote:
> On Mon, Nov 9, 2020 at 4:24 PM Jakub Kicinski <kuba@kernel.org> wrote:
> >
> > the commit
> > which introduced the assignment in the client driver.  
> 
> That's the commit which adds the initial driver to the tree, back in 2011.
> Should I use that for Fixes?

Yup

Re: [PATCH net-next v1] net: phy: spi_ks8995: Do not overwrite SPI mode flags

[Sven Van Asbroeck]

On Mon, Nov 9, 2020 at 5:04 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> Yup

Just a minute. Earlier in the thread, Andrew Lunn is suggesting I
create a new spi helper function, and cross-post to the spi group(s).

That doesn't sound like a minimal fix, should it go to net or net-next?

Thanks,
Sven

Re: [PATCH net-next v1] net: phy: spi_ks8995: Do not overwrite SPI mode flags

[Jakub Kicinski]

On Mon, 9 Nov 2020 17:19:48 -0500 Sven Van Asbroeck wrote:
> On Mon, Nov 9, 2020 at 5:04 PM Jakub Kicinski <kuba@kernel.org> wrote:
> >
> > Yup  
> 
> Just a minute. Earlier in the thread, Andrew Lunn is suggesting I
> create a new spi helper function, and cross-post to the spi group(s).
> 
> That doesn't sound like a minimal fix, should it go to net or net-next?

Is it only broken for you in linux-next or just in the current 5.10
release?

Re: [PATCH net-next v1] net: phy: spi_ks8995: Do not overwrite SPI mode flags

[Sven Van Asbroeck]

On Mon, Nov 9, 2020 at 5:23 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> Is it only broken for you in linux-next or just in the current 5.10
> release?

It's broken for me in the current 5.10 release. That means it should
go to net, not net-next, correct?

Re: [PATCH net-next v1] net: phy: spi_ks8995: Do not overwrite SPI mode flags

[Jakub Kicinski]

On Mon, 9 Nov 2020 17:27:28 -0500 Sven Van Asbroeck wrote:
> On Mon, Nov 9, 2020 at 5:23 PM Jakub Kicinski <kuba@kernel.org> wrote:
> >
> > Is it only broken for you in linux-next or just in the current 5.10
> > release?  
> 
> It's broken for me in the current 5.10 release. That means it should
> go to net, not net-next, correct?

Yes, most certainly. Especially with 5.10 being LTS.

You can send a minimal fix (perhaps what you got already?), and follow
up with the helper as suggested by Andrew after ~a week when net is
merged into net-next.

But please at least repost for net and CC Mark and the SPI list for
input.

Re: [PATCH net-next v1] net: phy: spi_ks8995: Do not overwrite SPI mode flags

[Sven Van Asbroeck]

On Mon, Nov 9, 2020 at 5:36 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> Yes, most certainly. Especially with 5.10 being LTS.
>
> You can send a minimal fix (perhaps what you got already?), and follow
> up with the helper as suggested by Andrew after ~a week when net is
> merged into net-next.
>

What I already posted (as v1) should be the minimal fix.
Can we proceed on that basis? I'll follow up with the helper
after the net -> net-next merge, as you suggested.

Re: [PATCH net-next v1] net: phy: spi_ks8995: Do not overwrite SPI mode flags

[Jakub Kicinski]

On Mon, 9 Nov 2020 17:39:22 -0500 Sven Van Asbroeck wrote:
> What I already posted (as v1) should be the minimal fix.
> Can we proceed on that basis? I'll follow up with the helper
> after the net -> net-next merge, as you suggested.

Well, you cut off the relevant part of my email where I said:

  But please at least repost for net and CC Mark and the SPI list 
  for input.

Maybe Mark has a different idea on how client drivers should behave.

Also please obviously CC the author of the patch who introduced 
the breakage.

Re: [PATCH net-next v1] net: phy: spi_ks8995: Do not overwrite SPI mode flags

[Sven Van Asbroeck]

On Mon, Nov 9, 2020 at 5:50 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
>   But please at least repost for net and CC Mark and the SPI list
>   for input.
>
> Maybe Mark has a different idea on how client drivers should behave.
>
> Also please obviously CC the author of the patch who introduced
> the breakage.

I believe they're aware: I tried to propose a patch to fix this in the
spi core, but it looks like it was rejected - with feedback: "fix the
client drivers instead"

https://patchwork.kernel.org/project/spi-devel-general/patch/20201106150706.29089-1-TheSven73@gmail.com/#23747737

I will respin this minimal fix as v2, add Fixes tags and Cc the
people involved, as you suggested.