[PATCH] usb: dwc2: fix regression on big-endian PowerPC/ARM systems

[PATCH] usb: dwc2: fix regression on big-endian PowerPC/ARM systems

[Arnd Bergmann]

A patch that went into Linux-4.4 to fix big-endian mode on a Lantiq
MIPS system unfortunately broke big-endian operation on PowerPC
APM82181 as reported by Christian Lamparter, and likely other
systems.

It actually introduced multiple issues:

- it broke big-endian ARM kernels: any machine that was working
  correctly with a little-endian kernel is no longer using byteswaps
  on big-endian kernels, which clearly breaks them.
- On PowerPC the same thing must be true: if it was working before,
  using big-endian kernels is now broken. Unlike ARM, 32-bit PowerPC
  usually uses big-endian kernels, so they are likely all broken.
- The barrier for dwc2_writel is on the wrong side of the __raw_writel(),
  so the MMIO no longer synchronizes with DMA operations.
- On architectures that require specific CPU instructions for MMIO
  access, using the __raw_ variant may turn this into a pointer
  dereference that does not have the same effect as the readl/writel.

This patch is a simple revert for all architectures other than MIPS,
in the hope that we can more easily backport it to fix the regression
on PowerPC and ARM systems without breaking the Lantiq system again.

We should follow this up with a more elaborate change to add runtime
detection of endianess, to make sure it also works on all other
combinations of architectures and implementations of the usb-dwc2
device. That patch however will be fairly large and not appropriate
for backports to stable kernels.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Fixes: 95c8bc360944 ("usb: dwc2: Use platform endianness when accessing registers")
---
 drivers/usb/dwc2/core.h | 41 +++++++++++++++++++++++++++++++++++------
 1 file changed, 35 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
index 3c58d633ce80..1f8ed149a40f 100644
--- a/drivers/usb/dwc2/core.h
+++ b/drivers/usb/dwc2/core.h
@@ -64,12 +64,24 @@
 	DWC2_TRACE_SCHEDULER_VB(pr_fmt("%s: SCH: " fmt),		\
 				dev_name(hsotg->dev), ##__VA_ARGS__)
 
+
+#ifdef CONFIG_MIPS
+/*
+ * There are some MIPS machines that can run in either big-endian
+ * or little-endian mode and that use the dwc2 register without
+ * a byteswap in both ways.
+ * Unlike other architectures, MIPS does not require a barrier
+ * before the __raw_writel() to synchronize with DMA but does
+ * require the barrier after the writel() to serialize a series
+ * of writes. This set of operations was added specifically for
+ * MIPS and should only be used there.
+ */
 static inline u32 dwc2_readl(const void __iomem *addr)
 {
 	u32 value = __raw_readl(addr);
 
-	/* In order to preserve endianness __raw_* operation is used. Therefore
-	 * a barrier is needed to ensure IO access is not re-ordered across
+	/* in order to preserve endianness __raw_* operation is used. therefore
+	 * a barrier is needed to ensure io access is not re-ordered across
 	 * reads or writes
 	 */
 	mb();
@@ -81,15 +93,32 @@ static inline void dwc2_writel(u32 value, void __iomem *addr)
 	__raw_writel(value, addr);
 
 	/*
-	 * In order to preserve endianness __raw_* operation is used. Therefore
-	 * a barrier is needed to ensure IO access is not re-ordered across
+	 * in order to preserve endianness __raw_* operation is used. therefore
+	 * a barrier is needed to ensure io access is not re-ordered across
 	 * reads or writes
 	 */
 	mb();
-#ifdef DWC2_LOG_WRITES
-	pr_info("INFO:: wrote %08x to %p\n", value, addr);
+#ifdef dwc2_log_writes
+	pr_info("info:: wrote %08x to %p\n", value, addr);
 #endif
 }
+#else
+/* Normal architectures just use readl/write */
+static inline u32 dwc2_readl(const void __iomem *addr)
+{
+	u32 value = readl(addr);
+	return value;
+}
+
+static inline void dwc2_writel(u32 value, void __iomem *addr)
+{
+	writel(value, addr);
+
+#ifdef dwc2_log_writes
+	pr_info("info:: wrote %08x to %p\n", value, addr);
+#endif
+}
+#endif
 
 /* Maximum number of Endpoints/HostChannels */
 #define MAX_EPS_CHANNELS	16
-- 
2.7.0

Re: [PATCH] usb: dwc2: fix regression on big-endian PowerPC/ARM systems

[Felipe Balbi]

Hi,

Arnd Bergmann <arnd@arndb.de> writes:
> A patch that went into Linux-4.4 to fix big-endian mode on a Lantiq
> MIPS system unfortunately broke big-endian operation on PowerPC
> APM82181 as reported by Christian Lamparter, and likely other
> systems.
>
> It actually introduced multiple issues:
>
> - it broke big-endian ARM kernels: any machine that was working
>   correctly with a little-endian kernel is no longer using byteswaps
>   on big-endian kernels, which clearly breaks them.
> - On PowerPC the same thing must be true: if it was working before,
>   using big-endian kernels is now broken. Unlike ARM, 32-bit PowerPC
>   usually uses big-endian kernels, so they are likely all broken.
> - The barrier for dwc2_writel is on the wrong side of the __raw_writel(),
>   so the MMIO no longer synchronizes with DMA operations.
> - On architectures that require specific CPU instructions for MMIO
>   access, using the __raw_ variant may turn this into a pointer
>   dereference that does not have the same effect as the readl/writel.
>
> This patch is a simple revert for all architectures other than MIPS,
> in the hope that we can more easily backport it to fix the regression
> on PowerPC and ARM systems without breaking the Lantiq system again.
>
> We should follow this up with a more elaborate change to add runtime
> detection of endianess, to make sure it also works on all other
> combinations of architectures and implementations of the usb-dwc2
> device. That patch however will be fairly large and not appropriate
> for backports to stable kernels.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> Fixes: 95c8bc360944 ("usb: dwc2: Use platform endianness when accessing registers")
> ---
>  drivers/usb/dwc2/core.h | 41 +++++++++++++++++++++++++++++++++++------
>  1 file changed, 35 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
> index 3c58d633ce80..1f8ed149a40f 100644
> --- a/drivers/usb/dwc2/core.h
> +++ b/drivers/usb/dwc2/core.h
> @@ -64,12 +64,24 @@
>  	DWC2_TRACE_SCHEDULER_VB(pr_fmt("%s: SCH: " fmt),		\
>  				dev_name(hsotg->dev), ##__VA_ARGS__)
>  
> +
> +#ifdef CONFIG_MIPS
> +/*
> + * There are some MIPS machines that can run in either big-endian
> + * or little-endian mode and that use the dwc2 register without
> + * a byteswap in both ways.
> + * Unlike other architectures, MIPS does not require a barrier
> + * before the __raw_writel() to synchronize with DMA but does
> + * require the barrier after the writel() to serialize a series
> + * of writes. This set of operations was added specifically for
> + * MIPS and should only be used there.
> + */
>  static inline u32 dwc2_readl(const void __iomem *addr)
>  {
>  	u32 value = __raw_readl(addr);
>  
> -	/* In order to preserve endianness __raw_* operation is used. Therefore
> -	 * a barrier is needed to ensure IO access is not re-ordered across
> +	/* in order to preserve endianness __raw_* operation is used. therefore
> +	 * a barrier is needed to ensure io access is not re-ordered across
>  	 * reads or writes
>  	 */
>  	mb();
> @@ -81,15 +93,32 @@ static inline void dwc2_writel(u32 value, void __iomem *addr)
>  	__raw_writel(value, addr);
>  
>  	/*
> -	 * In order to preserve endianness __raw_* operation is used. Therefore
> -	 * a barrier is needed to ensure IO access is not re-ordered across
> +	 * in order to preserve endianness __raw_* operation is used. therefore
> +	 * a barrier is needed to ensure io access is not re-ordered across
>  	 * reads or writes
>  	 */
>  	mb();
> -#ifdef DWC2_LOG_WRITES
> -	pr_info("INFO:: wrote %08x to %p\n", value, addr);
> +#ifdef dwc2_log_writes
> +	pr_info("info:: wrote %08x to %p\n", value, addr);
>  #endif
>  }
> +#else

I still think this is something that should be handled at MIPS side, no ?

How many more drivers will we have to 'fix' like this ?

-- 
balbi

Re: [PATCH] usb: dwc2: fix regression on big-endian PowerPC/ARM systems

[Arnd Bergmann]

On Thursday 12 May 2016 14:25:49 Felipe Balbi wrote:
> >  {
> >       u32 value = __raw_readl(addr);
> >  
> > -     /* In order to preserve endianness __raw_* operation is used. Therefore
> > -      * a barrier is needed to ensure IO access is not re-ordered across
> > +     /* in order to preserve endianness __raw_* operation is used. therefore
> > +      * a barrier is needed to ensure io access is not re-ordered across
> >        * reads or writes
> >        */
> >       mb();
> > @@ -81,15 +93,32 @@ static inline void dwc2_writel(u32 value, void __iomem *addr)
> >       __raw_writel(value, addr);
> >  
> >       /*
> > -      * In order to preserve endianness __raw_* operation is used. Therefore
> > -      * a barrier is needed to ensure IO access is not re-ordered across
> > +      * in order to preserve endianness __raw_* operation is used. therefore
> > +      * a barrier is needed to ensure io access is not re-ordered across
> >        * reads or writes
> >        */
> >       mb();
> > -#ifdef DWC2_LOG_WRITES
> > -     pr_info("INFO:: wrote %08x to %p\n", value, addr);
> > +#ifdef dwc2_log_writes
> > +     pr_info("info:: wrote %08x to %p\n", value, addr);
> >  #endif
> >  }
> > +#else

Oops, the accidental lowercase conversion is still in here, I'll fix it
up once we agree on the approach.

> I still think this is something that should be handled at MIPS side, no ?

As I explained, there isn't really anything we can do in MIPS code
because of the way they have to handle PCI.

> How many more drivers will we have to 'fix' like this ?

Endianess problems will keep coming up, and we have hundreds or thousands
of drivers that are written with a particular design in mind that could
be wrong as soon as someone chooses to build an SoC that does things
differently. Once that happens, we'll fix them.

Also, Christian has already posted a better version of the patch
that fixes this driver in an architecture independent way, but we still
need a workaround for the stable backports.

	Arnd

Re: [PATCH] usb: dwc2: fix regression on big-endian PowerPC/ARM systems

[Felipe Balbi]

Hi,

(Arnd, you didn't Cc dwc2's maintainer. I'm also not part of TI anymore)

Arnd Bergmann <arnd@arndb.de> writes:
> On Thursday 12 May 2016 14:25:49 Felipe Balbi wrote:
>> >  {
>> >       u32 value = __raw_readl(addr);
>> >  
>> > -     /* In order to preserve endianness __raw_* operation is used. Therefore
>> > -      * a barrier is needed to ensure IO access is not re-ordered across
>> > +     /* in order to preserve endianness __raw_* operation is used. therefore
>> > +      * a barrier is needed to ensure io access is not re-ordered across
>> >        * reads or writes
>> >        */
>> >       mb();
>> > @@ -81,15 +93,32 @@ static inline void dwc2_writel(u32 value, void __iomem *addr)
>> >       __raw_writel(value, addr);
>> >  
>> >       /*
>> > -      * In order to preserve endianness __raw_* operation is used. Therefore
>> > -      * a barrier is needed to ensure IO access is not re-ordered across
>> > +      * in order to preserve endianness __raw_* operation is used. therefore
>> > +      * a barrier is needed to ensure io access is not re-ordered across
>> >        * reads or writes
>> >        */
>> >       mb();
>> > -#ifdef DWC2_LOG_WRITES
>> > -     pr_info("INFO:: wrote %08x to %p\n", value, addr);
>> > +#ifdef dwc2_log_writes
>> > +     pr_info("info:: wrote %08x to %p\n", value, addr);
>> >  #endif
>> >  }
>> > +#else
>
> Oops, the accidental lowercase conversion is still in here, I'll fix it
> up once we agree on the approach.
>
>> I still think this is something that should be handled at MIPS side, no ?
>
> As I explained, there isn't really anything we can do in MIPS code
> because of the way they have to handle PCI.
>
>> How many more drivers will we have to 'fix' like this ?
>
> Endianess problems will keep coming up, and we have hundreds or thousands
> of drivers that are written with a particular design in mind that could
> be wrong as soon as someone chooses to build an SoC that does things
> differently. Once that happens, we'll fix them.
>
> Also, Christian has already posted a better version of the patch
> that fixes this driver in an architecture independent way, but we still
> need a workaround for the stable backports.

hmmm, at least dwc3 (also from SNPS) has a couple bits where we can
choose endianess for registers and DMA descriptors. John, do we have the
same for dwc2 ? Wouldn't that be a better way to solve the problem ?

-- 
balbi

Re: [PATCH] usb: dwc2: fix regression on big-endian PowerPC/ARM systems

[Arnd Bergmann]

On Thursday 12 May 2016 14:43:43 Felipe Balbi wrote:
> >> How many more drivers will we have to 'fix' like this ?
> >
> > Endianess problems will keep coming up, and we have hundreds or thousands
> > of drivers that are written with a particular design in mind that could
> > be wrong as soon as someone chooses to build an SoC that does things
> > differently. Once that happens, we'll fix them.
> >
> > Also, Christian has already posted a better version of the patch
> > that fixes this driver in an architecture independent way, but we still
> > need a workaround for the stable backports.
> 
> hmmm, at least dwc3 (also from SNPS) has a couple bits where we can
> choose endianess for registers and DMA descriptors. John, do we have the
> same for dwc2 ? Wouldn't that be a better way to solve the problem ?

Yes, I think that would be the best solution (provided it works correctly).

My understanding is that the descriptors don't need to change
for the particular MIPS machine, only the registers do. If we have
another machine that requires the descriptor endianess to be flipped
from the default, we probably need a DT property or platform_data
flag to encode that.

We can do the register endianess detection from Christian's patch to
flip it around if necessary, and then revert back to the previous
state of always using readl/writel.

	Arnd