Re: [PATCH v2 02/31] cinergyT2-core: don't do DMA on stack

[Mauro Carvalho Chehab]

Em Sat, 15 Oct 2016 22:54:49 +0200
Johannes Stezenbach  escreveu:

> On Tue, Oct 11, 2016 at 07:09:17AM -0300, Mauro Carvalho Chehab wrote:
> > --- a/drivers/media/usb/dvb-usb/cinergyT2-core.c
> > +++ b/drivers/media/usb/dvb-usb/cinergyT2-core.c
> > @@ -41,6 +41,8 @@ DVB_DEFINE_MOD_OPT_ADAPTER_NR(adapter_nr);
> >  
> >  struct cinergyt2_state {
> > u8 rc_counter;
> > +   unsigned char data[64];
> > +   struct mutex data_mutex;
> >  };  
> 
> Sometimes my thinking is slow but it just occured to me
> that this creates a potential issue with cache line sharing.
> On an architecture which manages cache coherence in software
> (ARM, MIPS etc.) a write to e.g. rc_counter in this example
> would dirty the cache line, and a later writeback from the
> cache could overwrite parts of data[] which was received via DMA.
> In contrast, if the DMA buffer is allocated seperately via
> kmalloc it is guaranteed to be safe wrt cache line sharing.
> (see bottom of Documentation/DMA-API-HOWTO.txt).
> 

Interesting point. I'm not sure well this would work with non-fully
coherent cache lines. I guess that will depend on how the USB
driver will be handling it.

> But of course DMA on stack also had the same issue
> and no one ever noticed so it's apparently not critical...

Yes, this shouldn't do it any worse than what we currently have.
In the past, I tested some drivers that uses a shared buffed for control
URB transfers in the past, on arm32 and arm64. I don't remember seeing
anything weird there that could be related to cache coherency, although
I remember several problems with USB on OMAP and RPi version 1, leading
troubles after several minutes of ISOC transfers on analog TV,
but they seemed to be unrelated to URB control traffic.

I'd say that we should keep our eyes on those drivers, after applying
this patch series and see if people will notice bad behavior on non-x86
archs.

Thanks,
Mauro

Re: [PATCH v2 02/31] cinergyT2-core: don't do DMA on stack

[Mauro Carvalho Chehab]

Em Sat, 15 Oct 2016 22:54:49 +0200
Johannes Stezenbach  escreveu:

> On Tue, Oct 11, 2016 at 07:09:17AM -0300, Mauro Carvalho Chehab wrote:
> > --- a/drivers/media/usb/dvb-usb/cinergyT2-core.c
> > +++ b/drivers/media/usb/dvb-usb/cinergyT2-core.c
> > @@ -41,6 +41,8 @@ DVB_DEFINE_MOD_OPT_ADAPTER_NR(adapter_nr);
> >  
> >  struct cinergyt2_state {
> > u8 rc_counter;
> > +   unsigned char data[64];
> > +   struct mutex data_mutex;
> >  };  
> 
> Sometimes my thinking is slow but it just occured to me
> that this creates a potential issue with cache line sharing.
> On an architecture which manages cache coherence in software
> (ARM, MIPS etc.) a write to e.g. rc_counter in this example
> would dirty the cache line, and a later writeback from the
> cache could overwrite parts of data[] which was received via DMA.
> In contrast, if the DMA buffer is allocated seperately via
> kmalloc it is guaranteed to be safe wrt cache line sharing.
> (see bottom of Documentation/DMA-API-HOWTO.txt).
> 

Interesting point. I'm not sure well this would work with non-fully
coherent cache lines. I guess that will depend on how the USB
driver will be handling it.

> But of course DMA on stack also had the same issue
> and no one ever noticed so it's apparently not critical...

Yes, this shouldn't do it any worse than what we currently have.
In the past, I tested some drivers that uses a shared buffed for control
URB transfers in the past, on arm32 and arm64. I don't remember seeing
anything weird there that could be related to cache coherency, although
I remember several problems with USB on OMAP and RPi version 1, leading
troubles after several minutes of ISOC transfers on analog TV,
but they seemed to be unrelated to URB control traffic.

I'd say that we should keep our eyes on those drivers, after applying
this patch series and see if people will notice bad behavior on non-x86
archs.

Thanks,
Mauro

Re: [PATCH v2 02/31] cinergyT2-core: don't do DMA on stack

[Johannes Stezenbach]

On Tue, Oct 11, 2016 at 07:09:17AM -0300, Mauro Carvalho Chehab wrote:
> --- a/drivers/media/usb/dvb-usb/cinergyT2-core.c
> +++ b/drivers/media/usb/dvb-usb/cinergyT2-core.c
> @@ -41,6 +41,8 @@ DVB_DEFINE_MOD_OPT_ADAPTER_NR(adapter_nr);
>  
>  struct cinergyt2_state {
>   u8 rc_counter;
> + unsigned char data[64];
> + struct mutex data_mutex;
>  };

Sometimes my thinking is slow but it just occured to me
that this creates a potential issue with cache line sharing.
On an architecture which manages cache coherence in software
(ARM, MIPS etc.) a write to e.g. rc_counter in this example
would dirty the cache line, and a later writeback from the
cache could overwrite parts of data[] which was received via DMA.
In contrast, if the DMA buffer is allocated seperately via
kmalloc it is guaranteed to be safe wrt cache line sharing.
(see bottom of Documentation/DMA-API-HOWTO.txt).

But of course DMA on stack also had the same issue
and no one ever noticed so it's apparently not critical...


Johannes

Re: [PATCH v2 02/31] cinergyT2-core: don't do DMA on stack

[Johannes Stezenbach]

On Tue, Oct 11, 2016 at 07:09:17AM -0300, Mauro Carvalho Chehab wrote:
> --- a/drivers/media/usb/dvb-usb/cinergyT2-core.c
> +++ b/drivers/media/usb/dvb-usb/cinergyT2-core.c
> @@ -41,6 +41,8 @@ DVB_DEFINE_MOD_OPT_ADAPTER_NR(adapter_nr);
>  
>  struct cinergyt2_state {
>   u8 rc_counter;
> + unsigned char data[64];
> + struct mutex data_mutex;
>  };

Sometimes my thinking is slow but it just occured to me
that this creates a potential issue with cache line sharing.
On an architecture which manages cache coherence in software
(ARM, MIPS etc.) a write to e.g. rc_counter in this example
would dirty the cache line, and a later writeback from the
cache could overwrite parts of data[] which was received via DMA.
In contrast, if the DMA buffer is allocated seperately via
kmalloc it is guaranteed to be safe wrt cache line sharing.
(see bottom of Documentation/DMA-API-HOWTO.txt).

But of course DMA on stack also had the same issue
and no one ever noticed so it's apparently not critical...


Johannes

[PATCH v2 02/31] cinergyT2-core: don't do DMA on stack

[Mauro Carvalho Chehab]

The USB control messages require DMA to work. We cannot pass
a stack-allocated buffer, as it is not warranted that the
stack would be into a DMA enabled area.

Reviewed-By: Patrick Boettcher 
Signed-off-by: Mauro Carvalho Chehab 
---
 drivers/media/usb/dvb-usb/cinergyT2-core.c | 84 ++
 1 file changed, 61 insertions(+), 23 deletions(-)

diff --git a/drivers/media/usb/dvb-usb/cinergyT2-core.c 
b/drivers/media/usb/dvb-usb/cinergyT2-core.c
index 9fd1527494eb..d85c0c4d4042 100644
--- a/drivers/media/usb/dvb-usb/cinergyT2-core.c
+++ b/drivers/media/usb/dvb-usb/cinergyT2-core.c
@@ -41,6 +41,8 @@ DVB_DEFINE_MOD_OPT_ADAPTER_NR(adapter_nr);
 
 struct cinergyt2_state {
u8 rc_counter;
+   unsigned char data[64];
+   struct mutex data_mutex;
 };
 
 /* We are missing a release hook with usb_device data */
@@ -50,33 +52,52 @@ static struct dvb_usb_device_properties 
cinergyt2_properties;
 
 static int cinergyt2_streaming_ctrl(struct dvb_usb_adapter *adap, int enable)
 {
-   char buf[] = { CINERGYT2_EP1_CONTROL_STREAM_TRANSFER, enable ? 1 : 0 };
-   char result[64];
-   return dvb_usb_generic_rw(adap->dev, buf, sizeof(buf), result,
-   sizeof(result), 0);
+   struct dvb_usb_device *d = adap->dev;
+   struct cinergyt2_state *st = d->priv;
+   int ret;
+
+   mutex_lock(>data_mutex);
+   st->data[0] = CINERGYT2_EP1_CONTROL_STREAM_TRANSFER;
+   st->data[1] = enable ? 1 : 0;
+
+   ret = dvb_usb_generic_rw(d, st->data, 2, st->data, 64, 0);
+   mutex_unlock(>data_mutex);
+
+   return ret;
 }
 
 static int cinergyt2_power_ctrl(struct dvb_usb_device *d, int enable)
 {
-   char buf[] = { CINERGYT2_EP1_SLEEP_MODE, enable ? 0 : 1 };
-   char state[3];
-   return dvb_usb_generic_rw(d, buf, sizeof(buf), state, sizeof(state), 0);
+   struct cinergyt2_state *st = d->priv;
+   int ret;
+
+   mutex_lock(>data_mutex);
+   st->data[0] = CINERGYT2_EP1_SLEEP_MODE;
+   st->data[1] = enable ? 0 : 1;
+
+   ret = dvb_usb_generic_rw(d, st->data, 2, st->data, 3, 0);
+   mutex_unlock(>data_mutex);
+
+   return ret;
 }
 
 static int cinergyt2_frontend_attach(struct dvb_usb_adapter *adap)
 {
-   char query[] = { CINERGYT2_EP1_GET_FIRMWARE_VERSION };
-   char state[3];
+   struct dvb_usb_device *d = adap->dev;
+   struct cinergyt2_state *st = d->priv;
int ret;
 
adap->fe_adap[0].fe = cinergyt2_fe_attach(adap->dev);
 
-   ret = dvb_usb_generic_rw(adap->dev, query, sizeof(query), state,
-   sizeof(state), 0);
+   mutex_lock(>data_mutex);
+   st->data[0] = CINERGYT2_EP1_GET_FIRMWARE_VERSION;
+
+   ret = dvb_usb_generic_rw(d, st->data, 1, st->data, 3, 0);
if (ret < 0) {
deb_rc("cinergyt2_power_ctrl() Failed to retrieve sleep "
"state info\n");
}
+   mutex_unlock(>data_mutex);
 
/* Copy this pointer as we are gonna need it in the release phase */
cinergyt2_usb_device = adap->dev;
@@ -141,13 +162,16 @@ static int repeatable_keys[] = {
 static int cinergyt2_rc_query(struct dvb_usb_device *d, u32 *event, int *state)
 {
struct cinergyt2_state *st = d->priv;
-   u8 key[5] = {0, 0, 0, 0, 0}, cmd = CINERGYT2_EP1_GET_RC_EVENTS;
int i;
 
*state = REMOTE_NO_KEY_PRESSED;
 
-   dvb_usb_generic_rw(d, , 1, key, sizeof(key), 0);
-   if (key[4] == 0xff) {
+   mutex_lock(>data_mutex);
+   st->data[0] = CINERGYT2_EP1_GET_RC_EVENTS;
+
+   dvb_usb_generic_rw(d, st->data, 1, st->data, 5, 0);
+
+   if (st->data[4] == 0xff) {
/* key repeat */
st->rc_counter++;
if (st->rc_counter > RC_REPEAT_DELAY) {
@@ -157,31 +181,45 @@ static int cinergyt2_rc_query(struct dvb_usb_device *d, 
u32 *event, int *state)
*event = d->last_event;
deb_rc("repeat key, event %x\n",
   *event);
-   return 0;
+   goto ret;
}
}
deb_rc("repeated key (non repeatable)\n");
}
-   return 0;
+   goto ret;
}
 
/* hack to pass checksum on the custom field */
-   key[2] = ~key[1];
-   dvb_usb_nec_rc_key_to_event(d, key, event, state);
-   if (key[0] != 0) {
+   st->data[2] = ~st->data[1];
+   dvb_usb_nec_rc_key_to_event(d, st->data, event, state);
+   if (st->data[0] != 0) {
if (*event != d->last_event)
st->rc_counter = 0;
 
-   deb_rc("key: %*ph\n", 5, key);
+   deb_rc("key: %*ph\n", 5, st->data);
}
-   return 0;
+
+ret:
+ 

[PATCH v2 02/31] cinergyT2-core: don't do DMA on stack

[Mauro Carvalho Chehab]

The USB control messages require DMA to work. We cannot pass
a stack-allocated buffer, as it is not warranted that the
stack would be into a DMA enabled area.

Reviewed-By: Patrick Boettcher 
Signed-off-by: Mauro Carvalho Chehab 
---
 drivers/media/usb/dvb-usb/cinergyT2-core.c | 84 ++
 1 file changed, 61 insertions(+), 23 deletions(-)

diff --git a/drivers/media/usb/dvb-usb/cinergyT2-core.c 
b/drivers/media/usb/dvb-usb/cinergyT2-core.c
index 9fd1527494eb..d85c0c4d4042 100644
--- a/drivers/media/usb/dvb-usb/cinergyT2-core.c
+++ b/drivers/media/usb/dvb-usb/cinergyT2-core.c
@@ -41,6 +41,8 @@ DVB_DEFINE_MOD_OPT_ADAPTER_NR(adapter_nr);
 
 struct cinergyt2_state {
u8 rc_counter;
+   unsigned char data[64];
+   struct mutex data_mutex;
 };
 
 /* We are missing a release hook with usb_device data */
@@ -50,33 +52,52 @@ static struct dvb_usb_device_properties 
cinergyt2_properties;
 
 static int cinergyt2_streaming_ctrl(struct dvb_usb_adapter *adap, int enable)
 {
-   char buf[] = { CINERGYT2_EP1_CONTROL_STREAM_TRANSFER, enable ? 1 : 0 };
-   char result[64];
-   return dvb_usb_generic_rw(adap->dev, buf, sizeof(buf), result,
-   sizeof(result), 0);
+   struct dvb_usb_device *d = adap->dev;
+   struct cinergyt2_state *st = d->priv;
+   int ret;
+
+   mutex_lock(>data_mutex);
+   st->data[0] = CINERGYT2_EP1_CONTROL_STREAM_TRANSFER;
+   st->data[1] = enable ? 1 : 0;
+
+   ret = dvb_usb_generic_rw(d, st->data, 2, st->data, 64, 0);
+   mutex_unlock(>data_mutex);
+
+   return ret;
 }
 
 static int cinergyt2_power_ctrl(struct dvb_usb_device *d, int enable)
 {
-   char buf[] = { CINERGYT2_EP1_SLEEP_MODE, enable ? 0 : 1 };
-   char state[3];
-   return dvb_usb_generic_rw(d, buf, sizeof(buf), state, sizeof(state), 0);
+   struct cinergyt2_state *st = d->priv;
+   int ret;
+
+   mutex_lock(>data_mutex);
+   st->data[0] = CINERGYT2_EP1_SLEEP_MODE;
+   st->data[1] = enable ? 0 : 1;
+
+   ret = dvb_usb_generic_rw(d, st->data, 2, st->data, 3, 0);
+   mutex_unlock(>data_mutex);
+
+   return ret;
 }
 
 static int cinergyt2_frontend_attach(struct dvb_usb_adapter *adap)
 {
-   char query[] = { CINERGYT2_EP1_GET_FIRMWARE_VERSION };
-   char state[3];
+   struct dvb_usb_device *d = adap->dev;
+   struct cinergyt2_state *st = d->priv;
int ret;
 
adap->fe_adap[0].fe = cinergyt2_fe_attach(adap->dev);
 
-   ret = dvb_usb_generic_rw(adap->dev, query, sizeof(query), state,
-   sizeof(state), 0);
+   mutex_lock(>data_mutex);
+   st->data[0] = CINERGYT2_EP1_GET_FIRMWARE_VERSION;
+
+   ret = dvb_usb_generic_rw(d, st->data, 1, st->data, 3, 0);
if (ret < 0) {
deb_rc("cinergyt2_power_ctrl() Failed to retrieve sleep "
"state info\n");
}
+   mutex_unlock(>data_mutex);
 
/* Copy this pointer as we are gonna need it in the release phase */
cinergyt2_usb_device = adap->dev;
@@ -141,13 +162,16 @@ static int repeatable_keys[] = {
 static int cinergyt2_rc_query(struct dvb_usb_device *d, u32 *event, int *state)
 {
struct cinergyt2_state *st = d->priv;
-   u8 key[5] = {0, 0, 0, 0, 0}, cmd = CINERGYT2_EP1_GET_RC_EVENTS;
int i;
 
*state = REMOTE_NO_KEY_PRESSED;
 
-   dvb_usb_generic_rw(d, , 1, key, sizeof(key), 0);
-   if (key[4] == 0xff) {
+   mutex_lock(>data_mutex);
+   st->data[0] = CINERGYT2_EP1_GET_RC_EVENTS;
+
+   dvb_usb_generic_rw(d, st->data, 1, st->data, 5, 0);
+
+   if (st->data[4] == 0xff) {
/* key repeat */
st->rc_counter++;
if (st->rc_counter > RC_REPEAT_DELAY) {
@@ -157,31 +181,45 @@ static int cinergyt2_rc_query(struct dvb_usb_device *d, 
u32 *event, int *state)
*event = d->last_event;
deb_rc("repeat key, event %x\n",
   *event);
-   return 0;
+   goto ret;
}
}
deb_rc("repeated key (non repeatable)\n");
}
-   return 0;
+   goto ret;
}
 
/* hack to pass checksum on the custom field */
-   key[2] = ~key[1];
-   dvb_usb_nec_rc_key_to_event(d, key, event, state);
-   if (key[0] != 0) {
+   st->data[2] = ~st->data[1];
+   dvb_usb_nec_rc_key_to_event(d, st->data, event, state);
+   if (st->data[0] != 0) {
if (*event != d->last_event)
st->rc_counter = 0;
 
-   deb_rc("key: %*ph\n", 5, key);
+   deb_rc("key: %*ph\n", 5, st->data);
}
-   return 0;
+
+ret:
+   mutex_unlock(>data_mutex);
+   return ret;
 }