diff mbox series

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

Message ID 20201109193117.2017-1-TheSven73@gmail.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series [net-next,v1] net: phy: spi_ks8995: Do not overwrite SPI mode flags | expand

Checks

Context Check Description
netdev/cover_letter success Link
netdev/fixes_present success Link
netdev/patch_count success Link
netdev/tree_selection success Clearly marked for net-next
netdev/subject_prefix success Link
netdev/source_inline success Was 0 now: 0
netdev/verify_signedoff success Link
netdev/module_param success Was 0 now: 0
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/verify_fixes success Link
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 10 lines checked
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/header_inline success Link
netdev/stable success Stable not CCed

Commit Message

Sven Van Asbroeck Nov. 9, 2020, 7:31 p.m. UTC 
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(-)

Comments

Andrew Lunn Nov. 9, 2020, 7:49 p.m. UTC | #1 
> +++ 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
Sven Van Asbroeck Nov. 9, 2020, 7:56 p.m. UTC | #2 
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
Andrew Lunn Nov. 9, 2020, 8:25 p.m. UTC | #3 
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
Sven Van Asbroeck Nov. 9, 2020, 8:34 p.m. UTC | #4 
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.
Jakub Kicinski Nov. 9, 2020, 9:09 p.m. UTC | #5 
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.
Sven Van Asbroeck Nov. 9, 2020, 9:20 p.m. UTC | #6 
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?
Jakub Kicinski Nov. 9, 2020, 9:24 p.m. UTC | #7 
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.
Sven Van Asbroeck Nov. 9, 2020, 9:29 p.m. UTC | #8 
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?
Jakub Kicinski Nov. 9, 2020, 10:04 p.m. UTC | #9 
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
Sven Van Asbroeck Nov. 9, 2020, 10:19 p.m. UTC | #10 
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
Jakub Kicinski Nov. 9, 2020, 10:23 p.m. UTC | #11 
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?
Sven Van Asbroeck Nov. 9, 2020, 10:27 p.m. UTC | #12 
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?
Jakub Kicinski Nov. 9, 2020, 10:36 p.m. UTC | #13 
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.
Sven Van Asbroeck Nov. 9, 2020, 10:39 p.m. UTC | #14 
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.
Jakub Kicinski Nov. 9, 2020, 10:50 p.m. UTC | #15 
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.
Sven Van Asbroeck Nov. 9, 2020, 10:57 p.m. UTC | #16 
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.
diff mbox series

Patch

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) {