This patch fixes regression caused by commit a16dad77634 "MIPS: Fix
potencial corruption". That commit fixes one corruption scenario in
cost of adding another one, which actually start to cause crashes
on Yeeloong laptop when rtl8187 driver is used.
For correct DMA read operation on machines without DMA coherence, kernel
have to invalidate cache, such it will refill later with new data that
device wrote to memory, when that data is needed to process. We can only
invalidate full cache line. Hence when cache line includes both dma
buffer and some other data (written in cache, but not yet in main
memory), the other data can not hit memory due to invalidation. That
happen on rtl8187 where struct rtl8187_priv fields are located just
before and after small buffers that are passed to USB layer and DMA
is performed on them.
To fix the problem we align buffers and reserve space after them to make
them match cache line.
This patch does not resolve all possible MIPS problems entirely, for
that we have to assure that we always map cache aligned buffers for DMA,
what can be complex or even not possible. But patch fixes visible and
reproducible regression and seems other possible corruptions do not
happen in practice, since Yeeloong laptop works stable without rtl8187
driver.
Bug report:
https://bugzilla.kernel.org/show_bug.cgi?id=54391
Reported-by: Petr Pisar <[email protected]>
Bisected-by: Tom Li <[email protected]>
Reported-and-tested-by: Tom Li <[email protected]>
Cc: [email protected]
Signed-off-by: Stanislaw Gruszka <[email protected]>
---
drivers/net/wireless/rtl818x/rtl8187/rtl8187.h | 10 ++++++++--
1 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/drivers/net/wireless/rtl818x/rtl8187/rtl8187.h b/drivers/net/wireless/rtl818x/rtl8187/rtl8187.h
index 56aee06..a6ad79f 100644
--- a/drivers/net/wireless/rtl818x/rtl8187/rtl8187.h
+++ b/drivers/net/wireless/rtl818x/rtl8187/rtl8187.h
@@ -15,6 +15,8 @@
#ifndef RTL8187_H
#define RTL8187_H
+#include <linux/cache.h>
+
#include "rtl818x.h"
#include "leds.h"
@@ -139,7 +141,10 @@ struct rtl8187_priv {
u8 aifsn[4];
u8 rfkill_mask;
struct {
- __le64 buf;
+ union {
+ __le64 buf;
+ u8 dummy1[L1_CACHE_BYTES];
+ } ____cacheline_aligned;
struct sk_buff_head queue;
} b_tx_status; /* This queue is used by both -b and non-b devices */
struct mutex io_mutex;
@@ -147,7 +152,8 @@ struct rtl8187_priv {
u8 bits8;
__le16 bits16;
__le32 bits32;
- } *io_dmabuf;
+ u8 dummy2[L1_CACHE_BYTES];
+ } *io_dmabuf ____cacheline_aligned;
bool rfkill_off;
u16 seqno;
};
--
1.7.4.4
On 02/10/2014 03:38 PM, Stanislaw Gruszka wrote:
> This patch fixes regression caused by commit a16dad77634 "MIPS: Fix
> potencial corruption". That commit fixes one corruption scenario in
> cost of adding another one, which actually start to cause crashes
> on Yeeloong laptop when rtl8187 driver is used.
>
> For correct DMA read operation on machines without DMA coherence, kernel
> have to invalidate cache, such it will refill later with new data that
> device wrote to memory, when that data is needed to process. We can only
> invalidate full cache line. Hence when cache line includes both dma
> buffer and some other data (written in cache, but not yet in main
> memory), the other data can not hit memory due to invalidation. That
> happen on rtl8187 where struct rtl8187_priv fields are located just
> before and after small buffers that are passed to USB layer and DMA
> is performed on them.
>
> To fix the problem we align buffers and reserve space after them to make
> them match cache line.
>
> This patch does not resolve all possible MIPS problems entirely, for
> that we have to assure that we always map cache aligned buffers for DMA,
> what can be complex or even not possible. But patch fixes visible and
> reproducible regression and seems other possible corruptions do not
> happen in practice, since Yeeloong laptop works stable without rtl8187
> driver.
>
> Bug report:
> https://bugzilla.kernel.org/show_bug.cgi?id=54391
>
> Reported-by: Petr Pisar <[email protected]>
> Bisected-by: Tom Li <[email protected]>
> Reported-and-tested-by: Tom Li <[email protected]>
> Cc: [email protected]
> Signed-off-by: Stanislaw Gruszka <[email protected]>
> ---
Congratulations to all for sorting this out. It could not have been too easy.
The only effect I see on architectures with DMA coherence is that the private
data area has grown a little. Certainly, my RTL8187B device still works on x86_64.
Acked-by: Larry Finger <[email protected]>
Larry
> drivers/net/wireless/rtl818x/rtl8187/rtl8187.h | 10 ++++++++--
> 1 files changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/wireless/rtl818x/rtl8187/rtl8187.h b/drivers/net/wireless/rtl818x/rtl8187/rtl8187.h
> index 56aee06..a6ad79f 100644
> --- a/drivers/net/wireless/rtl818x/rtl8187/rtl8187.h
> +++ b/drivers/net/wireless/rtl818x/rtl8187/rtl8187.h
> @@ -15,6 +15,8 @@
> #ifndef RTL8187_H
> #define RTL8187_H
>
> +#include <linux/cache.h>
> +
> #include "rtl818x.h"
> #include "leds.h"
>
> @@ -139,7 +141,10 @@ struct rtl8187_priv {
> u8 aifsn[4];
> u8 rfkill_mask;
> struct {
> - __le64 buf;
> + union {
> + __le64 buf;
> + u8 dummy1[L1_CACHE_BYTES];
> + } ____cacheline_aligned;
> struct sk_buff_head queue;
> } b_tx_status; /* This queue is used by both -b and non-b devices */
> struct mutex io_mutex;
> @@ -147,7 +152,8 @@ struct rtl8187_priv {
> u8 bits8;
> __le16 bits16;
> __le32 bits32;
> - } *io_dmabuf;
> + u8 dummy2[L1_CACHE_BYTES];
> + } *io_dmabuf ____cacheline_aligned;
> bool rfkill_off;
> u16 seqno;
> };
>
------------------------------
On Mon, Feb 10, 2014 10:50 PM GMT Larry Finger wrote:
>On 02/10/2014 03:38 PM, Stanislaw Gruszka wrote:
>> This patch fixes regression caused by commit a16dad77634 "MIPS: Fix
>> potencial corruption". That commit fixes one corruption scenario in
>> cost of adding another one, which actually start to cause crashes
>> on Yeeloong laptop when rtl8187 driver is used.
>>
>> For correct DMA read operation on machines without DMA coherence, kernel
>> have to invalidate cache, such it will refill later with new data that
>> device wrote to memory, when that data is needed to process. We can only
>> invalidate full cache line. Hence when cache line includes both dma
>> buffer and some other data (written in cache, but not yet in main
>> memory), the other data can not hit memory due to invalidation. That
>> happen on rtl8187 where struct rtl8187_priv fields are located just
>> before and after small buffers that are passed to USB layer and DMA
>> is performed on them.
>>
>> To fix the problem we align buffers and reserve space after them to make
>> them match cache line.
>>
>> This patch does not resolve all possible MIPS problems entirely, for
>> that we have to assure that we always map cache aligned buffers for DMA,
>> what can be complex or even not possible. But patch fixes visible and
>> reproducible regression and seems other possible corruptions do not
>> happen in practice, since Yeeloong laptop works stable without rtl8187
>> driver.
>>
>> Bug report:
>> https://bugzilla.kernel.org/show_bug.cgi?id=54391
>>
>> Reported-by: Petr Pisar <[email protected]>
>> Bisected-by: Tom Li <[email protected]>
>> Reported-and-tested-by: Tom Li <[email protected]>
>> Cc: [email protected]
>> Signed-off-by: Stanislaw Gruszka <[email protected]>
>> ---
>
>Congratulations to all for sorting this out. It could not have been too easy.
>
>The only effect I see on architectures with DMA coherence is that the private
>data area has grown a little. Certainly, my RTL8187B device still works on x86_64.
>
>Acked-by: Larry Finger <[email protected]>
>
>Larry
>
>> drivers/net/wireless/rtl818x/rtl8187/rtl8187.h | 10 ++++++++--
>> 1 files changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/wireless/rtl818x/rtl8187/rtl8187.h b/drivers/net/wireless/rtl818x/rtl8187/rtl8187.h
>> index 56aee06..a6ad79f 100644
>> --- a/drivers/net/wireless/rtl818x/rtl8187/rtl8187.h
>> +++ b/drivers/net/wireless/rtl818x/rtl8187/rtl8187.h
>> @@ -15,6 +15,8 @@
>> #ifndef RTL8187_H
>> #define RTL8187_H
>>
>> +#include <linux/cache.h>
>> +
>> #include "rtl818x.h"
>> #include "leds.h"
>>
>> @@ -139,7 +141,10 @@ struct rtl8187_priv {
>> u8 aifsn[4];
>> u8 rfkill_mask;
>> struct {
>> - __le64 buf;
>> + union {
>> + __le64 buf;
>> + u8 dummy1[L1_CACHE_BYTES];
>> + } ____cacheline_aligned;
>> struct sk_buff_head queue;
>> } b_tx_status; /* This queue is used by both -b and non-b devices */
>> struct mutex io_mutex;
>> @@ -147,7 +152,8 @@ struct rtl8187_priv {
>> u8 bits8;
>> __le16 bits16;
>> __le32 bits32;
>> - } *io_dmabuf;
>> + u8 dummy2[L1_CACHE_BYTES];
>> + } *io_dmabuf ____cacheline_aligned;
>> bool rfkill_off;
>> u16 seqno;
>> };
>>
>
------------------------------
On Mon, Feb 10, 2014 10:50 PM GMT Larry Finger wrote:
>On 02/10/2014 03:38 PM, Stanislaw Gruszka wrote:
>> This patch fixes regression caused by commit a16dad77634 "MIPS: Fix
>> potencial corruption". That commit fixes one corruption scenario in
>> cost of adding another one, which actually start to cause crashes
>> on Yeeloong laptop when rtl8187 driver is used.
>>
>> For correct DMA read operation on machines without DMA coherence, kernel
>> have to invalidate cache, such it will refill later with new data that
>> device wrote to memory, when that data is needed to process. We can only
>> invalidate full cache line. Hence when cache line includes both dma
>> buffer and some other data (written in cache, but not yet in main
>> memory), the other data can not hit memory due to invalidation. That
>> happen on rtl8187 where struct rtl8187_priv fields are located just
>> before and after small buffers that are passed to USB layer and DMA
>> is performed on them.
>>
>> To fix the problem we align buffers and reserve space after them to make
>> them match cache line.
>>
>> This patch does not resolve all possible MIPS problems entirely, for
>> that we have to assure that we always map cache aligned buffers for DMA,
>> what can be complex or even not possible. But patch fixes visible and
>> reproducible regression and seems other possible corruptions do not
>> happen in practice, since Yeeloong laptop works stable without rtl8187
>> driver.
>>
>> Bug report:
>> https://bugzilla.kernel.org/show_bug.cgi?id=54391
>>
>> Reported-by: Petr Pisar <[email protected]>
>> Bisected-by: Tom Li <[email protected]>
>> Reported-and-tested-by: Tom Li <[email protected]>
>> Cc: [email protected]
>> Signed-off-by: Stanislaw Gruszka <[email protected]>
>> ---
>
>Congratulations to all for sorting this out. It could not have been too easy.
>
>The only effect I see on architectures with DMA coherence is that the private
>data area has grown a little. Certainly, my RTL8187B device still works on x86_64.
>
>Acked-by: Larry Finger <[email protected]>
>
>Larry
>
Acked-by: Hin-Tak Leung <[email protected]>
Looks fine. L1_CACHE_BYTES is defined in <asm/cache.h> (arch dependent)
which is included by <linux/cache.h> (where ____cacheline_aligned is).
Hin-Tak
P.S. Apologies about the blank message - problem with typing on tablet...
>>???drivers/net/wireless/rtl818x/rtl8187/rtl8187.h |???10 ++++++++--
>>???1 files changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/wireless/rtl818x/rtl8187/rtl8187.h b/drivers/net/wireless/rtl818x/rtl8187/rtl8187.h
>> index 56aee06..a6ad79f 100644
>> --- a/drivers/net/wireless/rtl818x/rtl8187/rtl8187.h
>> +++ b/drivers/net/wireless/rtl818x/rtl8187/rtl8187.h
>> @@ -15,6 +15,8 @@
>>???#ifndef RTL8187_H
>>???#define RTL8187_H
>>
>> +#include <linux/cache.h>
>> +
>>???#include "rtl818x.h"
>>???#include "leds.h"
>>
>> @@ -139,7 +141,10 @@ struct rtl8187_priv {
>>?????? u8 aifsn[4];
>>?????? u8 rfkill_mask;
>>?????? struct {
>> -??? ??? __le64 buf;
>> +??? ??? union {
>> +??? ??? ??? __le64 buf;
>> +??? ??? ??? u8 dummy1[L1_CACHE_BYTES];
>> +??? ??? } ____cacheline_aligned;
>>?????? ??? struct sk_buff_head queue;
>>?????? } b_tx_status; /* This queue is used by both -b and non-b devices */
>>?????? struct mutex io_mutex;
>> @@ -147,7 +152,8 @@ struct rtl8187_priv {
>>?????? ??? u8 bits8;
>>?????? ??? __le16 bits16;
>>?????? ??? __le32 bits32;
>> -??? } *io_dmabuf;
>> +??? ??? u8 dummy2[L1_CACHE_BYTES];
>> +??? } *io_dmabuf ____cacheline_aligned;
>>?????? bool rfkill_off;
>>?????? u16 seqno;
>>???};
>>
>