Since commit 60f481b970386 ("i40e: change flags to use 64 bits"),
the i40e driver uses cmpxchg64(). This causes mips:allmodconfig builds
to fail with
drivers/net/ethernet/intel/i40e/i40e_ethtool.c:
In function 'i40e_set_priv_flags':
drivers/net/ethernet/intel/i40e/i40e_ethtool.c:4443:2: error:
implicit declaration of function 'cmpxchg64'
Implement a poor-mans-version of cmpxchg64() to fix the problem for 32-bit
mips builds. The code is derived from sparc32, but only uses a single
spinlock.
Fixes: 60f481b970386 ("i40e: change flags to use 64 bits")
Signed-off-by: Guenter Roeck <[email protected]>
---
Compile-tested only.
arch/mips/include/asm/cmpxchg.h | 3 +++
arch/mips/kernel/cmpxchg.c | 17 +++++++++++++++++
2 files changed, 20 insertions(+)
diff --git a/arch/mips/include/asm/cmpxchg.h b/arch/mips/include/asm/cmpxchg.h
index 89e9fb7976fe..9f7b1df95b99 100644
--- a/arch/mips/include/asm/cmpxchg.h
+++ b/arch/mips/include/asm/cmpxchg.h
@@ -206,6 +206,9 @@ static inline unsigned long __cmpxchg(volatile void *ptr, unsigned long old,
#define cmpxchg64_local(ptr, o, n) __cmpxchg64_local_generic((ptr), (o), (n))
#ifndef CONFIG_SMP
#define cmpxchg64(ptr, o, n) cmpxchg64_local((ptr), (o), (n))
+#else
+u64 __cmpxchg_u64(u64 *ptr, u64 old, u64 new);
+#define cmpxchg64(ptr, old, new) __cmpxchg_u64(ptr, old, new)
#endif
#endif
diff --git a/arch/mips/kernel/cmpxchg.c b/arch/mips/kernel/cmpxchg.c
index 0b9535bc2c53..30216beb2334 100644
--- a/arch/mips/kernel/cmpxchg.c
+++ b/arch/mips/kernel/cmpxchg.c
@@ -9,6 +9,7 @@
*/
#include <linux/bitops.h>
+#include <linux/spinlock.h>
#include <asm/cmpxchg.h>
unsigned long __xchg_small(volatile void *ptr, unsigned long val, unsigned int size)
@@ -107,3 +108,19 @@ unsigned long __cmpxchg_small(volatile void *ptr, unsigned long old,
return old;
}
}
+
+static DEFINE_SPINLOCK(cmpxchg_lock);
+
+u64 __cmpxchg_u64(u64 *ptr, u64 old, u64 new)
+{
+ unsigned long flags;
+ u64 prev;
+
+ spin_lock_irqsave(&cmpxchg_lock, flags);
+ if ((prev = *ptr) == old)
+ *ptr = new;
+ spin_unlock_irqrestore(&cmpxchg_lock, flags);
+
+ return prev;
+}
+EXPORT_SYMBOL(__cmpxchg_u64);
--
2.7.4
Hi Guenter,
On Mon, Feb 12, 2018 at 02:37:01PM -0800, Guenter Roeck wrote:
> Since commit 60f481b970386 ("i40e: change flags to use 64 bits"),
> the i40e driver uses cmpxchg64(). This causes mips:allmodconfig builds
> to fail with
>
> drivers/net/ethernet/intel/i40e/i40e_ethtool.c:
> In function 'i40e_set_priv_flags':
> drivers/net/ethernet/intel/i40e/i40e_ethtool.c:4443:2: error:
> implicit declaration of function 'cmpxchg64'
>
> Implement a poor-mans-version of cmpxchg64() to fix the problem for 32-bit
> mips builds. The code is derived from sparc32, but only uses a single
> spinlock.
Will this be implemened for all 32-bit architectures which are currently
missing cmpxchg64()?
If so, any particular reason not to do it in generic code?
If not then I think that driver should be fixed to either depend on some
appropriate Kconfig symbol or to not use this API since it clearly isn't
portable at the moment.
See also Shannon's comment about that specific driver:
https://lkml.kernel.org/r/[email protected]
Cheers
James
>
> Fixes: 60f481b970386 ("i40e: change flags to use 64 bits")
> Signed-off-by: Guenter Roeck <[email protected]>
> ---
> Compile-tested only.
>
> arch/mips/include/asm/cmpxchg.h | 3 +++
> arch/mips/kernel/cmpxchg.c | 17 +++++++++++++++++
> 2 files changed, 20 insertions(+)
>
> diff --git a/arch/mips/include/asm/cmpxchg.h b/arch/mips/include/asm/cmpxchg.h
> index 89e9fb7976fe..9f7b1df95b99 100644
> --- a/arch/mips/include/asm/cmpxchg.h
> +++ b/arch/mips/include/asm/cmpxchg.h
> @@ -206,6 +206,9 @@ static inline unsigned long __cmpxchg(volatile void *ptr, unsigned long old,
> #define cmpxchg64_local(ptr, o, n) __cmpxchg64_local_generic((ptr), (o), (n))
> #ifndef CONFIG_SMP
> #define cmpxchg64(ptr, o, n) cmpxchg64_local((ptr), (o), (n))
> +#else
> +u64 __cmpxchg_u64(u64 *ptr, u64 old, u64 new);
> +#define cmpxchg64(ptr, old, new) __cmpxchg_u64(ptr, old, new)
> #endif
> #endif
>
> diff --git a/arch/mips/kernel/cmpxchg.c b/arch/mips/kernel/cmpxchg.c
> index 0b9535bc2c53..30216beb2334 100644
> --- a/arch/mips/kernel/cmpxchg.c
> +++ b/arch/mips/kernel/cmpxchg.c
> @@ -9,6 +9,7 @@
> */
>
> #include <linux/bitops.h>
> +#include <linux/spinlock.h>
> #include <asm/cmpxchg.h>
>
> unsigned long __xchg_small(volatile void *ptr, unsigned long val, unsigned int size)
> @@ -107,3 +108,19 @@ unsigned long __cmpxchg_small(volatile void *ptr, unsigned long old,
> return old;
> }
> }
> +
> +static DEFINE_SPINLOCK(cmpxchg_lock);
> +
> +u64 __cmpxchg_u64(u64 *ptr, u64 old, u64 new)
> +{
> + unsigned long flags;
> + u64 prev;
> +
> + spin_lock_irqsave(&cmpxchg_lock, flags);
> + if ((prev = *ptr) == old)
> + *ptr = new;
> + spin_unlock_irqrestore(&cmpxchg_lock, flags);
> +
> + return prev;
> +}
> +EXPORT_SYMBOL(__cmpxchg_u64);
> --
> 2.7.4
>
On Mon, Feb 12, 2018 at 11:42:02PM +0000, James Hogan wrote:
> Hi Guenter,
>
> On Mon, Feb 12, 2018 at 02:37:01PM -0800, Guenter Roeck wrote:
> > Since commit 60f481b970386 ("i40e: change flags to use 64 bits"),
> > the i40e driver uses cmpxchg64(). This causes mips:allmodconfig builds
> > to fail with
> >
> > drivers/net/ethernet/intel/i40e/i40e_ethtool.c:
> > In function 'i40e_set_priv_flags':
> > drivers/net/ethernet/intel/i40e/i40e_ethtool.c:4443:2: error:
> > implicit declaration of function 'cmpxchg64'
> >
> > Implement a poor-mans-version of cmpxchg64() to fix the problem for 32-bit
> > mips builds. The code is derived from sparc32, but only uses a single
> > spinlock.
>
> Will this be implemened for all 32-bit architectures which are currently
> missing cmpxchg64()?
>
No idea.
> If so, any particular reason not to do it in generic code?
>
Again, no idea. When the problem was previously seen on sparc32,
it was implemented there.
> If not then I think that driver should be fixed to either depend on some
> appropriate Kconfig symbol or to not use this API since it clearly isn't
> portable at the moment.
>
Good point.
> See also Shannon's comment about that specific driver:
> https://lkml.kernel.org/r/[email protected]
>
Well, this was an RFC only. Feel free to ignore it.
FWIW, this is the second time that the call was introduced in the i40 driver.
After the first time the code was rewritten to avoid the problem, but now it
came back. Someone must really like it ;-). For my part, I may just blacklist
the offending driver in my builds; that is less than perfect, but much easier
than having to deal with the same problem over and over again. Guess I'll wait
for a while and do just that if the problem isn't fixed in a later RC.
Guenter
As has previously been said, we're going to be removing the need for cmpxchg64. But it takes a little bit of time and work to do so. I'm adding the dev that is taking care of the work back onto this email thread as well so he can see any concerns with it.
Alice
-----Original Message-----
From: Guenter Roeck [mailto:[email protected]] On Behalf Of Guenter Roeck
Sent: Monday, February 12, 2018 3:57 PM
To: James Hogan <[email protected]>
Cc: Ralf Baechle <[email protected]>; [email protected]; [email protected]; Michael, Alice <[email protected]>; Kirsher, Jeffrey T <[email protected]>; Shannon Nelson <[email protected]>
Subject: Re: [RFC PATCH] MIPS: Provide cmpxchg64 for 32-bit builds
On Mon, Feb 12, 2018 at 11:42:02PM +0000, James Hogan wrote:
> Hi Guenter,
>
> On Mon, Feb 12, 2018 at 02:37:01PM -0800, Guenter Roeck wrote:
> > Since commit 60f481b970386 ("i40e: change flags to use 64 bits"),
> > the i40e driver uses cmpxchg64(). This causes mips:allmodconfig
> > builds to fail with
> >
> > drivers/net/ethernet/intel/i40e/i40e_ethtool.c:
> > In function 'i40e_set_priv_flags':
> > drivers/net/ethernet/intel/i40e/i40e_ethtool.c:4443:2: error:
> > implicit declaration of function 'cmpxchg64'
> >
> > Implement a poor-mans-version of cmpxchg64() to fix the problem for
> > 32-bit mips builds. The code is derived from sparc32, but only uses
> > a single spinlock.
>
> Will this be implemened for all 32-bit architectures which are
> currently missing cmpxchg64()?
>
No idea.
> If so, any particular reason not to do it in generic code?
>
Again, no idea. When the problem was previously seen on sparc32, it was implemented there.
> If not then I think that driver should be fixed to either depend on
> some appropriate Kconfig symbol or to not use this API since it
> clearly isn't portable at the moment.
>
Good point.
> See also Shannon's comment about that specific driver:
> https://lkml.kernel.org/r/e7c934d7-e5f4-ee1b-0647-c31a98d9e944@oracle.
> com
>
Well, this was an RFC only. Feel free to ignore it.
FWIW, this is the second time that the call was introduced in the i40 driver.
After the first time the code was rewritten to avoid the problem, but now it came back. Someone must really like it ;-). For my part, I may just blacklist the offending driver in my builds; that is less than perfect, but much easier than having to deal with the same problem over and over again. Guess I'll wait for a while and do just that if the problem isn't fixed in a later RC.
Guenter
> -----Original Message-----
> From: Michael, Alice
> Sent: Wednesday, February 14, 2018 1:03 PM
> To: Guenter Roeck <[email protected]>; James Hogan <[email protected]>;
> Keller, Jacob E <[email protected]>
> Cc: Ralf Baechle <[email protected]>; [email protected]; linux-
> [email protected]; Kirsher, Jeffrey T <[email protected]>; Shannon
> Nelson <[email protected]>
> Subject: RE: [RFC PATCH] MIPS: Provide cmpxchg64 for 32-bit builds
>
> As has previously been said, we're going to be removing the need for cmpxchg64.
> But it takes a little bit of time and work to do so. I'm adding the dev that is taking
> care of the work back onto this email thread as well so he can see any concerns with
> it.
>
> Alice
>
> -----Original Message-----
> From: Guenter Roeck [mailto:[email protected]] On Behalf Of Guenter Roeck
> Sent: Monday, February 12, 2018 3:57 PM
> To: James Hogan <[email protected]>
> Cc: Ralf Baechle <[email protected]>; [email protected]; linux-
> [email protected]; Michael, Alice <[email protected]>; Kirsher, Jeffrey T
> <[email protected]>; Shannon Nelson <[email protected]>
> Subject: Re: [RFC PATCH] MIPS: Provide cmpxchg64 for 32-bit builds
>
> On Mon, Feb 12, 2018 at 11:42:02PM +0000, James Hogan wrote:
> > Hi Guenter,
> >
> > On Mon, Feb 12, 2018 at 02:37:01PM -0800, Guenter Roeck wrote:
> > > Since commit 60f481b970386 ("i40e: change flags to use 64 bits"),
> > > the i40e driver uses cmpxchg64(). This causes mips:allmodconfig
> > > builds to fail with
> > >
> > > drivers/net/ethernet/intel/i40e/i40e_ethtool.c:
> > > In function 'i40e_set_priv_flags':
> > > drivers/net/ethernet/intel/i40e/i40e_ethtool.c:4443:2: error:
> > > implicit declaration of function 'cmpxchg64'
> > >
> > > Implement a poor-mans-version of cmpxchg64() to fix the problem for
> > > 32-bit mips builds. The code is derived from sparc32, but only uses
> > > a single spinlock.
> >
> > Will this be implemened for all 32-bit architectures which are
> > currently missing cmpxchg64()?
> >
> No idea.
>
> > If so, any particular reason not to do it in generic code?
> >
> Again, no idea. When the problem was previously seen on sparc32, it was
> implemented there.
>
> > If not then I think that driver should be fixed to either depend on
> > some appropriate Kconfig symbol or to not use this API since it
> > clearly isn't portable at the moment.
> >
> Good point.
>
> > See also Shannon's comment about that specific driver:
> > https://lkml.kernel.org/r/e7c934d7-e5f4-ee1b-0647-c31a98d9e944@oracle.
> > com
> >
>
> Well, this was an RFC only. Feel free to ignore it.
>
> FWIW, this is the second time that the call was introduced in the i40 driver.
> After the first time the code was rewritten to avoid the problem, but now it came
> back. Someone must really like it ;-). For my part, I may just blacklist the offending
> driver in my builds; that is less than perfect, but much easier than having to deal with
> the same problem over and over again. Guess I'll wait for a while and do just that if
> the problem isn't fixed in a later RC.
>
> Guenter
Hi,
I've been working on re-writing some of the code so that the need for a compare-and-exchange in the i40e_set_priv_flags() is not necessary. This mostly involved moving many flags out into an atomic bitops field instead, it should be posted to IWL soon.
Thanks,
Jake
On Wed, Feb 14, 2018 at 09:36:33PM +0000, Keller, Jacob E wrote:
> > -----Original Message-----
> > From: Michael, Alice
> > Sent: Wednesday, February 14, 2018 1:03 PM
> > To: Guenter Roeck <[email protected]>; James Hogan <[email protected]>;
> > Keller, Jacob E <[email protected]>
> > Cc: Ralf Baechle <[email protected]>; [email protected]; linux-
> > [email protected]; Kirsher, Jeffrey T <[email protected]>; Shannon
> > Nelson <[email protected]>
> > Subject: RE: [RFC PATCH] MIPS: Provide cmpxchg64 for 32-bit builds
> >
> > As has previously been said, we're going to be removing the need for cmpxchg64.
> > But it takes a little bit of time and work to do so. I'm adding the dev that is taking
> > care of the work back onto this email thread as well so he can see any concerns with
> > it.
> >
> > Alice
> >
> > -----Original Message-----
> > From: Guenter Roeck [mailto:[email protected]] On Behalf Of Guenter Roeck
> > Sent: Monday, February 12, 2018 3:57 PM
> > To: James Hogan <[email protected]>
> > Cc: Ralf Baechle <[email protected]>; [email protected]; linux-
> > [email protected]; Michael, Alice <[email protected]>; Kirsher, Jeffrey T
> > <[email protected]>; Shannon Nelson <[email protected]>
> > Subject: Re: [RFC PATCH] MIPS: Provide cmpxchg64 for 32-bit builds
> >
> > On Mon, Feb 12, 2018 at 11:42:02PM +0000, James Hogan wrote:
> > > Hi Guenter,
> > >
> > > On Mon, Feb 12, 2018 at 02:37:01PM -0800, Guenter Roeck wrote:
> > > > Since commit 60f481b970386 ("i40e: change flags to use 64 bits"),
> > > > the i40e driver uses cmpxchg64(). This causes mips:allmodconfig
> > > > builds to fail with
> > > >
> > > > drivers/net/ethernet/intel/i40e/i40e_ethtool.c:
> > > > In function 'i40e_set_priv_flags':
> > > > drivers/net/ethernet/intel/i40e/i40e_ethtool.c:4443:2: error:
> > > > implicit declaration of function 'cmpxchg64'
> > > >
> > > > Implement a poor-mans-version of cmpxchg64() to fix the problem for
> > > > 32-bit mips builds. The code is derived from sparc32, but only uses
> > > > a single spinlock.
> > >
> > > Will this be implemened for all 32-bit architectures which are
> > > currently missing cmpxchg64()?
> > >
> > No idea.
> >
> > > If so, any particular reason not to do it in generic code?
> > >
> > Again, no idea. When the problem was previously seen on sparc32, it was
> > implemented there.
> >
> > > If not then I think that driver should be fixed to either depend on
> > > some appropriate Kconfig symbol or to not use this API since it
> > > clearly isn't portable at the moment.
> > >
> > Good point.
> >
> > > See also Shannon's comment about that specific driver:
> > > https://lkml.kernel.org/r/e7c934d7-e5f4-ee1b-0647-c31a98d9e944@oracle.
> > > com
> > >
> >
> > Well, this was an RFC only. Feel free to ignore it.
> >
> > FWIW, this is the second time that the call was introduced in the i40 driver.
> > After the first time the code was rewritten to avoid the problem, but now it came
> > back. Someone must really like it ;-). For my part, I may just blacklist the offending
> > driver in my builds; that is less than perfect, but much easier than having to deal with
> > the same problem over and over again. Guess I'll wait for a while and do just that if
> > the problem isn't fixed in a later RC.
> >
> > Guenter
>
> Hi,
>
> I've been working on re-writing some of the code so that the need for a compare-and-exchange in the i40e_set_priv_flags() is not necessary. This mostly involved moving many flags out into an atomic bitops field instead, it should be posted to IWL soon.
Any update on this? Will a fix to the driver make it into 4.16 or is it
going to be too big a change?
As far as I can tell from grepping around, of the architectures which
support 32-bit SMP with PCI, these ones implement cmpxchg64 on 32-bit:
arch/arm
arch/ia64
arch/x86
arch/riscv (blindly implements using 64-bit instructions, broken?)
arch/parisc (with spinlock)
arch/sparc (with spinlock)
And these don't:
arch/arc
arch/mips
arch/powerpc
arch/sh
arch/xtensa
(I've excluded arches which are already being removed)
Cheers
James
> -----Original Message-----
> From: James Hogan [mailto:[email protected]]
> Sent: Monday, March 12, 2018 4:12 PM
> To: Keller, Jacob E <[email protected]>
> Cc: Michael, Alice <[email protected]>; Guenter Roeck <linux@roeck-
> us.net>; Ralf Baechle <[email protected]>; [email protected]; linux-
> [email protected]; Kirsher, Jeffrey T <[email protected]>;
> Shannon Nelson <[email protected]>
> Subject: Re: [RFC PATCH] MIPS: Provide cmpxchg64 for 32-bit builds
>
> On Wed, Feb 14, 2018 at 09:36:33PM +0000, Keller, Jacob E wrote:
> > > -----Original Message-----
> > > From: Michael, Alice
> > > Sent: Wednesday, February 14, 2018 1:03 PM
> > > To: Guenter Roeck <[email protected]>; James Hogan
> <[email protected]>;
> > > Keller, Jacob E <[email protected]>
> > > Cc: Ralf Baechle <[email protected]>; [email protected]; linux-
> > > [email protected]; Kirsher, Jeffrey T <[email protected]>;
> Shannon
> > > Nelson <[email protected]>
> > > Subject: RE: [RFC PATCH] MIPS: Provide cmpxchg64 for 32-bit builds
> > >
> > > As has previously been said, we're going to be removing the need for
> cmpxchg64.
> > > But it takes a little bit of time and work to do so. I'm adding the dev that is
> taking
> > > care of the work back onto this email thread as well so he can see any
> concerns with
> > > it.
> > >
> > > Alice
> > >
> > > -----Original Message-----
> > > From: Guenter Roeck [mailto:[email protected]] On Behalf Of Guenter
> Roeck
> > > Sent: Monday, February 12, 2018 3:57 PM
> > > To: James Hogan <[email protected]>
> > > Cc: Ralf Baechle <[email protected]>; [email protected]; linux-
> > > [email protected]; Michael, Alice <[email protected]>; Kirsher,
> Jeffrey T
> > > <[email protected]>; Shannon Nelson
> <[email protected]>
> > > Subject: Re: [RFC PATCH] MIPS: Provide cmpxchg64 for 32-bit builds
> > >
> > > On Mon, Feb 12, 2018 at 11:42:02PM +0000, James Hogan wrote:
> > > > Hi Guenter,
> > > >
> > > > On Mon, Feb 12, 2018 at 02:37:01PM -0800, Guenter Roeck wrote:
> > > > > Since commit 60f481b970386 ("i40e: change flags to use 64 bits"),
> > > > > the i40e driver uses cmpxchg64(). This causes mips:allmodconfig
> > > > > builds to fail with
> > > > >
> > > > > drivers/net/ethernet/intel/i40e/i40e_ethtool.c:
> > > > > In function 'i40e_set_priv_flags':
> > > > > drivers/net/ethernet/intel/i40e/i40e_ethtool.c:4443:2: error:
> > > > > implicit declaration of function 'cmpxchg64'
> > > > >
> > > > > Implement a poor-mans-version of cmpxchg64() to fix the problem for
> > > > > 32-bit mips builds. The code is derived from sparc32, but only uses
> > > > > a single spinlock.
> > > >
> > > > Will this be implemened for all 32-bit architectures which are
> > > > currently missing cmpxchg64()?
> > > >
> > > No idea.
> > >
> > > > If so, any particular reason not to do it in generic code?
> > > >
> > > Again, no idea. When the problem was previously seen on sparc32, it was
> > > implemented there.
> > >
> > > > If not then I think that driver should be fixed to either depend on
> > > > some appropriate Kconfig symbol or to not use this API since it
> > > > clearly isn't portable at the moment.
> > > >
> > > Good point.
> > >
> > > > See also Shannon's comment about that specific driver:
> > > > https://lkml.kernel.org/r/e7c934d7-e5f4-ee1b-0647-c31a98d9e944@oracle.
> > > > com
> > > >
> > >
> > > Well, this was an RFC only. Feel free to ignore it.
> > >
> > > FWIW, this is the second time that the call was introduced in the i40 driver.
> > > After the first time the code was rewritten to avoid the problem, but now it
> came
> > > back. Someone must really like it ;-). For my part, I may just blacklist the
> offending
> > > driver in my builds; that is less than perfect, but much easier than having to
> deal with
> > > the same problem over and over again. Guess I'll wait for a while and do just
> that if
> > > the problem isn't fixed in a later RC.
> > >
> > > Guenter
> >
> > Hi,
> >
> > I've been working on re-writing some of the code so that the need for a
> compare-and-exchange in the i40e_set_priv_flags() is not necessary. This mostly
> involved moving many flags out into an atomic bitops field instead, it should be
> posted to IWL soon.
>
> Any update on this? Will a fix to the driver make it into 4.16 or is it
> going to be too big a change?
I am not sure if it can make it into 4.16, it's basically a bunch of patches to migrate things in the pf->flags variable so that the flags is only modified while under rtnl_lock. Thus, any runtime flags which change at any time without the lock will be moved to state bits.
The tricky part is that backporting can be troublesome since flags are accessed in a lot of places.
The patch series should be published to iwl within this week (Thanks Alice!)
If we just remove the cmpxchg without doing the flag changes we open ourselves back up to the risk of the flags being modified simultaneously and causing difficult to debug "flag silently disabled/enabled" issues. In practice this race window is pretty tiny though.
Thanks,
Jake
>
> As far as I can tell from grepping around, of the architectures which
> support 32-bit SMP with PCI, these ones implement cmpxchg64 on 32-bit:
>
> arch/arm
> arch/ia64
> arch/x86
> arch/riscv (blindly implements using 64-bit instructions, broken?)
> arch/parisc (with spinlock)
> arch/sparc (with spinlock)
>
> And these don't:
>
> arch/arc
> arch/mips
> arch/powerpc
> arch/sh
> arch/xtensa
>
> (I've excluded arches which are already being removed)
>
> Cheers
> James