Re: Problem with VMAP_STACK=y

[Mauro Carvalho Chehab]

Em Fri, 7 Oct 2016 09:52:56 +0200 (CEST)
Jiri Kosina  escreveu:

> On Thu, 6 Oct 2016, Mauro Carvalho Chehab wrote:
> 
> > I can't see any other obvious error on the conversion. You could try to 
> > enable debug options at DVB core/dvb-usb and/or add some printk's to the 
> > driver and see what's happening.
> 
> Mauro, also please don't forget that there are many more places in 
> drivers/media that still perform DMA on stack, and so have to be fixed for 
> 4.9 (as VMAP_STACK makes that to be immediately visible problem even on 
> x86_64, which it wasn't the case before).

Yes, I'm aware of that. I'm doing the conversion of drivers under dvb-usb,
at:

https://git.linuxtv.org/mchehab/experimental.git/log/?h=media_dmastack_fixes

I'll be sending the patches to the ML after ready.

I'll then take a look on other USB drivers that use the stack. I guess
the non-USB media drivers are safe from this issue.

Thanks,
Mauro
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Problem with VMAP_STACK=y

[Jiri Kosina]

On Thu, 6 Oct 2016, Mauro Carvalho Chehab wrote:

> I can't see any other obvious error on the conversion. You could try to 
> enable debug options at DVB core/dvb-usb and/or add some printk's to the 
> driver and see what's happening.

Mauro, also please don't forget that there are many more places in 
drivers/media that still perform DMA on stack, and so have to be fixed for 
4.9 (as VMAP_STACK makes that to be immediately visible problem even on 
x86_64, which it wasn't the case before).

Thanks,

-- 
Jiri Kosina
SUSE Labs

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Problem with VMAP_STACK=y

[Mauro Carvalho Chehab]

Em Thu, 6 Oct 2016 10:30:15 +0200
Jörg Otte  escreveu:

> 2016-10-05 20:55 GMT+02:00 Mauro Carvalho Chehab :
> > Hi Johannes,
> >
> > Em Wed, 5 Oct 2016 20:29:45 +0200
> > Johannes Stezenbach  escreveu:
> >  
> >> On Wed, Oct 05, 2016 at 06:04:50AM -0300, Mauro Carvalho Chehab wrote:  
> >> >  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);  
> >>
> >> it seems to miss this:
> >>
> >>   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");
> >> > @@ -141,13 +147,14 @@ 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) {
> >> > +   st->data[0] = CINERGYT2_EP1_SLEEP_MODE;  
> >>
> >> should probably be
> >>
> >>   st->data[0] = CINERGYT2_EP1_GET_RC_EVENTS;
> >>  
> >> > +
> >> > +   dvb_usb_generic_rw(d, st->data, 1, st->data, 5, 0);  
> >>
> >>
> >> HTH,
> >> Johannes  
> >
> >
> > Thanks for the review! Yeah, you're right: both firmware and remote
> > controller logic would be broken without the above fixes.
> >
> > Just sent a version 2 of this patch to the ML with the above fixes.
> >
> > Regards,
> > Mauro  
> 
> Applied V2 of the patch. Unfortunately no progress.
> No video, no error messages.

I can't see any other obvious error on the conversion. You could try
to enable debug options at DVB core/dvb-usb and/or add some printk's to
the driver and see what's happening.


Thanks,
Mauro
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Problem with VMAP_STACK=y

[Jörg Otte]

2016-10-05 20:55 GMT+02:00 Mauro Carvalho Chehab :
> Hi Johannes,
>
> Em Wed, 5 Oct 2016 20:29:45 +0200
> Johannes Stezenbach  escreveu:
>
>> On Wed, Oct 05, 2016 at 06:04:50AM -0300, Mauro Carvalho Chehab wrote:
>> >  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);
>>
>> it seems to miss this:
>>
>>   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");
>> > @@ -141,13 +147,14 @@ 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) {
>> > +   st->data[0] = CINERGYT2_EP1_SLEEP_MODE;
>>
>> should probably be
>>
>>   st->data[0] = CINERGYT2_EP1_GET_RC_EVENTS;
>>
>> > +
>> > +   dvb_usb_generic_rw(d, st->data, 1, st->data, 5, 0);
>>
>>
>> HTH,
>> Johannes
>
>
> Thanks for the review! Yeah, you're right: both firmware and remote
> controller logic would be broken without the above fixes.
>
> Just sent a version 2 of this patch to the ML with the above fixes.
>
> Regards,
> Mauro

Applied V2 of the patch. Unfortunately no progress.
No video, no error messages.

Jörg
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Problem with VMAP_STACK=y

[Mauro Carvalho Chehab]

Hi Johannes,

Em Wed, 5 Oct 2016 20:29:45 +0200
Johannes Stezenbach  escreveu:

> On Wed, Oct 05, 2016 at 06:04:50AM -0300, Mauro Carvalho Chehab wrote:
> >  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);  
> 
> it seems to miss this:
> 
>   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");
> > @@ -141,13 +147,14 @@ 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) {
> > +   st->data[0] = CINERGYT2_EP1_SLEEP_MODE;  
> 
> should probably be
> 
>   st->data[0] = CINERGYT2_EP1_GET_RC_EVENTS;
> 
> > +
> > +   dvb_usb_generic_rw(d, st->data, 1, st->data, 5, 0);  
> 
> 
> HTH,
> Johannes


Thanks for the review! Yeah, you're right: both firmware and remote
controller logic would be broken without the above fixes.

Just sent a version 2 of this patch to the ML with the above fixes.

Regards,
Mauro
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Problem with VMAP_STACK=y

[Johannes Stezenbach]

On Wed, Oct 05, 2016 at 06:04:50AM -0300, Mauro Carvalho Chehab wrote:
>  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);

it seems to miss this:

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");
> @@ -141,13 +147,14 @@ 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) {
> + st->data[0] = CINERGYT2_EP1_SLEEP_MODE;

should probably be

st->data[0] = CINERGYT2_EP1_GET_RC_EVENTS;

> +
> + dvb_usb_generic_rw(d, st->data, 1, st->data, 5, 0);


HTH,
Johannes
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Problem with VMAP_STACK=y

[Andy Lutomirski]

On Wed, Oct 5, 2016 at 9:45 AM, Jörg Otte  wrote:
> 2016-10-05 17:53 GMT+02:00 Andy Lutomirski :
>> On Wed, Oct 5, 2016 at 8:21 AM, Jörg Otte  wrote:
>>> 2016-10-05 11:04 GMT+02:00 Mauro Carvalho Chehab :
 Em Wed, 5 Oct 2016 09:50:42 +0200 (CEST)
 Jiri Kosina  escreveu:

> On Wed, 5 Oct 2016, Patrick Boettcher wrote:
>
> > > > Thanks for the quick response.
> > > > Drivers are:
> > > > dvb_core, dvb_usb, dbv_usb_cynergyT2
> > >
> > > This dbv_usb_cynergyT2 is not from Linus' tree, is it? I don't seem
> > > to be able to find it, and the only google hit I am getting is your
> > > very mail to LKML :)
> >
> > It's a typo, it should say dvb_usb_cinergyT2.
>
> Ah, thanks. Same issues there in
>
>   cinergyt2_frontend_attach()
>   cinergyt2_rc_query()
>
> I think this would require more in-depth review of all the media drivers
> and having all this fixed for 4.9. It should be pretty straightforward;
> all the instances I've seen so far should be just straightforward
> conversion to kmalloc() + kfree(), as the buffer is not being embedded in
> any structure etc.

 What we're doing on most cases is to put a buffer (usually with 80
 chars for USB drivers) inside the "state" struct (on DVB drivers),
 in order to avoid doing kmalloc/kfree all the times. One such patch is
 changeset c4a98793a63c4.

 I'm enclosing a non-tested patch fixing it for the cinergyT2-core.c
 driver.

 Thanks,
 Mauro

 [PATCH] cinergyT2-core: don't do DMA on stack

>>>
>>> Tried the cinergyT2 patch. No success. When I select a TV channel
>>> just nothing happens. There are no error messages.
>>
>> Could you try compiling with CONFIG_DEBUG_VIRTUAL=y and seeing if you
>> get a nice error message?
>>
>> --Andy
>
> Done. Still no error messages in dmesg and syslog.
>
> DVB adapter signals it is tuned to the channel.
> For me it looks like there`s no data reaching the application
> (similar to if I forget to connect an antenna).

I'm surprised.  CONFIG_DEBUG_VIRTUAL=y really ought to have caught the
problem, whatever it is.  You could try CONFIG_DEBUG_SG as well, but I
admit I'm grasping at straws there.  Maybe the DVB people have a
better idea as to what's going on here.

It's plausible that the patch you're testing got rid of the DMA on the
stack but is otherwise buggy.

--Andy
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Problem with VMAP_STACK=y

[Jörg Otte]

2016-10-05 17:53 GMT+02:00 Andy Lutomirski :
> On Wed, Oct 5, 2016 at 8:21 AM, Jörg Otte  wrote:
>> 2016-10-05 11:04 GMT+02:00 Mauro Carvalho Chehab :
>>> Em Wed, 5 Oct 2016 09:50:42 +0200 (CEST)
>>> Jiri Kosina  escreveu:
>>>
 On Wed, 5 Oct 2016, Patrick Boettcher wrote:

 > > > Thanks for the quick response.
 > > > Drivers are:
 > > > dvb_core, dvb_usb, dbv_usb_cynergyT2
 > >
 > > This dbv_usb_cynergyT2 is not from Linus' tree, is it? I don't seem
 > > to be able to find it, and the only google hit I am getting is your
 > > very mail to LKML :)
 >
 > It's a typo, it should say dvb_usb_cinergyT2.

 Ah, thanks. Same issues there in

   cinergyt2_frontend_attach()
   cinergyt2_rc_query()

 I think this would require more in-depth review of all the media drivers
 and having all this fixed for 4.9. It should be pretty straightforward;
 all the instances I've seen so far should be just straightforward
 conversion to kmalloc() + kfree(), as the buffer is not being embedded in
 any structure etc.
>>>
>>> What we're doing on most cases is to put a buffer (usually with 80
>>> chars for USB drivers) inside the "state" struct (on DVB drivers),
>>> in order to avoid doing kmalloc/kfree all the times. One such patch is
>>> changeset c4a98793a63c4.
>>>
>>> I'm enclosing a non-tested patch fixing it for the cinergyT2-core.c
>>> driver.
>>>
>>> Thanks,
>>> Mauro
>>>
>>> [PATCH] cinergyT2-core: don't do DMA on stack
>>>
>>
>> Tried the cinergyT2 patch. No success. When I select a TV channel
>> just nothing happens. There are no error messages.
>
> Could you try compiling with CONFIG_DEBUG_VIRTUAL=y and seeing if you
> get a nice error message?
>
> --Andy

Done. Still no error messages in dmesg and syslog.

DVB adapter signals it is tuned to the channel.
For me it looks like there`s no data reaching the application
(similar to if I forget to connect an antenna).

Jörg
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Problem with VMAP_STACK=y

[Andy Lutomirski]

On Wed, Oct 5, 2016 at 8:21 AM, Jörg Otte  wrote:
> 2016-10-05 11:04 GMT+02:00 Mauro Carvalho Chehab :
>> Em Wed, 5 Oct 2016 09:50:42 +0200 (CEST)
>> Jiri Kosina  escreveu:
>>
>>> On Wed, 5 Oct 2016, Patrick Boettcher wrote:
>>>
>>> > > > Thanks for the quick response.
>>> > > > Drivers are:
>>> > > > dvb_core, dvb_usb, dbv_usb_cynergyT2
>>> > >
>>> > > This dbv_usb_cynergyT2 is not from Linus' tree, is it? I don't seem
>>> > > to be able to find it, and the only google hit I am getting is your
>>> > > very mail to LKML :)
>>> >
>>> > It's a typo, it should say dvb_usb_cinergyT2.
>>>
>>> Ah, thanks. Same issues there in
>>>
>>>   cinergyt2_frontend_attach()
>>>   cinergyt2_rc_query()
>>>
>>> I think this would require more in-depth review of all the media drivers
>>> and having all this fixed for 4.9. It should be pretty straightforward;
>>> all the instances I've seen so far should be just straightforward
>>> conversion to kmalloc() + kfree(), as the buffer is not being embedded in
>>> any structure etc.
>>
>> What we're doing on most cases is to put a buffer (usually with 80
>> chars for USB drivers) inside the "state" struct (on DVB drivers),
>> in order to avoid doing kmalloc/kfree all the times. One such patch is
>> changeset c4a98793a63c4.
>>
>> I'm enclosing a non-tested patch fixing it for the cinergyT2-core.c
>> driver.
>>
>> Thanks,
>> Mauro
>>
>> [PATCH] cinergyT2-core: don't do DMA on stack
>>
>
> Tried the cinergyT2 patch. No success. When I select a TV channel
> just nothing happens. There are no error messages.

Could you try compiling with CONFIG_DEBUG_VIRTUAL=y and seeing if you
get a nice error message?

--Andy
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Problem with VMAP_STACK=y

[Jörg Otte]

2016-10-05 11:04 GMT+02:00 Mauro Carvalho Chehab :
> Em Wed, 5 Oct 2016 09:50:42 +0200 (CEST)
> Jiri Kosina  escreveu:
>
>> On Wed, 5 Oct 2016, Patrick Boettcher wrote:
>>
>> > > > Thanks for the quick response.
>> > > > Drivers are:
>> > > > dvb_core, dvb_usb, dbv_usb_cynergyT2
>> > >
>> > > This dbv_usb_cynergyT2 is not from Linus' tree, is it? I don't seem
>> > > to be able to find it, and the only google hit I am getting is your
>> > > very mail to LKML :)
>> >
>> > It's a typo, it should say dvb_usb_cinergyT2.
>>
>> Ah, thanks. Same issues there in
>>
>>   cinergyt2_frontend_attach()
>>   cinergyt2_rc_query()
>>
>> I think this would require more in-depth review of all the media drivers
>> and having all this fixed for 4.9. It should be pretty straightforward;
>> all the instances I've seen so far should be just straightforward
>> conversion to kmalloc() + kfree(), as the buffer is not being embedded in
>> any structure etc.
>
> What we're doing on most cases is to put a buffer (usually with 80
> chars for USB drivers) inside the "state" struct (on DVB drivers),
> in order to avoid doing kmalloc/kfree all the times. One such patch is
> changeset c4a98793a63c4.
>
> I'm enclosing a non-tested patch fixing it for the cinergyT2-core.c
> driver.
>
> Thanks,
> Mauro
>
> [PATCH] cinergyT2-core: don't do DMA on stack
>

Tried the cinergyT2 patch. No success. When I select a TV channel
just nothing happens. There are no error messages.

Jörg
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Problem with VMAP_STACK=y

[Mauro Carvalho Chehab]

Em Wed, 5 Oct 2016 06:04:50 -0300
Mauro Carvalho Chehab  escreveu:

> Em Wed, 5 Oct 2016 09:50:42 +0200 (CEST)
> Jiri Kosina  escreveu:
> 
> > On Wed, 5 Oct 2016, Patrick Boettcher wrote:
> > 
> > > > > Thanks for the quick response.
> > > > > Drivers are:
> > > > > dvb_core, dvb_usb, dbv_usb_cynergyT2
> > > > 
> > > > This dbv_usb_cynergyT2 is not from Linus' tree, is it? I don't seem
> > > > to be able to find it, and the only google hit I am getting is your
> > > > very mail to LKML :)  
> > > 
> > > It's a typo, it should say dvb_usb_cinergyT2.  
> > 
> > Ah, thanks. Same issues there in
> > 
> > cinergyt2_frontend_attach()
> > cinergyt2_rc_query()
> > 
> > I think this would require more in-depth review of all the media drivers 
> > and having all this fixed for 4.9. It should be pretty straightforward; 
> > all the instances I've seen so far should be just straightforward 
> > conversion to kmalloc() + kfree(), as the buffer is not being embedded in 
> > any structure etc.
> 
> What we're doing on most cases is to put a buffer (usually with 80
> chars for USB drivers) inside the "state" struct (on DVB drivers),
> in order to avoid doing kmalloc/kfree all the times. One such patch is 
> changeset c4a98793a63c4.
> 
> I'm enclosing a non-tested patch fixing it for the cinergyT2-core.c
> driver.
> 
> Thanks,
> Mauro

And this is another such patch for af9005, also untested. If I
remember well, the firmware load and warm/cold state logic calls
happen before allocating space for struct state. So, it needs to
call of kmalloc on two places.

I may write similar patches for other drivers under drivers/media/usb,
if I have enough time for that.

Regards,
Mauro


[PATCH] af9005: don't do DMA on stack

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.

Signed-off-by: Mauro Carvalho Chehab 

diff --git a/drivers/media/usb/dvb-usb/af9005.c 
b/drivers/media/usb/dvb-usb/af9005.c
index efa782ed6e2d..cc5815de1cfb 100644
--- a/drivers/media/usb/dvb-usb/af9005.c
+++ b/drivers/media/usb/dvb-usb/af9005.c
@@ -52,17 +52,15 @@ u8 regmask[8] = { 0x01, 0x03, 0x07, 0x0f, 0x1f, 0x3f, 0x7f, 
0xff };
 struct af9005_device_state {
u8 sequence;
int led_state;
+   unsigned char data[256];
 };
 
 static int af9005_generic_read_write(struct dvb_usb_device *d, u16 reg,
  int readwrite, int type, u8 * values, int len)
 {
struct af9005_device_state *st = d->priv;
-   u8 obuf[16] = { 0 };
-   u8 ibuf[17] = { 0 };
-   u8 command;
-   int i;
-   int ret;
+   u8 command, seq;
+   int i, ret;
 
if (len < 1) {
err("generic read/write, less than 1 byte. Makes no sense.");
@@ -73,16 +71,16 @@ static int af9005_generic_read_write(struct dvb_usb_device 
*d, u16 reg,
return -EINVAL;
}
 
-   obuf[0] = 14;   /* rest of buffer length low */
-   obuf[1] = 0;/* rest of buffer length high */
+   st->data[0] = 14;   /* rest of buffer length low */
+   st->data[1] = 0;/* rest of buffer length high */
 
-   obuf[2] = AF9005_REGISTER_RW;   /* register operation */
-   obuf[3] = 12;   /* rest of buffer length */
+   st->data[2] = AF9005_REGISTER_RW;   /* register operation */
+   st->data[3] = 12;   /* rest of buffer length */
 
-   obuf[4] = st->sequence++;   /* sequence number */
+   st->data[4] = seq = st->sequence++; /* sequence number */
 
-   obuf[5] = (u8) (reg >> 8);  /* register address */
-   obuf[6] = (u8) (reg & 0xff);
+   st->data[5] = (u8) (reg >> 8);  /* register address */
+   st->data[6] = (u8) (reg & 0xff);
 
if (type == AF9005_OFDM_REG) {
command = AF9005_CMD_OFDM_REG;
@@ -96,49 +94,43 @@ static int af9005_generic_read_write(struct dvb_usb_device 
*d, u16 reg,
command |= readwrite;
if (readwrite == AF9005_CMD_WRITE)
for (i = 0; i < len; i++)
-   obuf[8 + i] = values[i];
+   st->data[8 + i] = values[i];
else if (type == AF9005_TUNER_REG)
/* read command for tuner, the first byte contains the i2c 
address */
-   obuf[8] = values[0];
-   obuf[7] = command;
+   st->data[8] = values[0];
+   st->data[7] = command;
 
-   ret = dvb_usb_generic_rw(d, obuf, 16, ibuf, 17, 0);
+   ret = dvb_usb_generic_rw(d, st->data, 16, st->data, 17, 0);
if (ret)
return ret;
 
/* sanity check */
-   if (ibuf[2] != AF9005_REGISTER_RW_ACK) {
+   if (st->data[2] != AF9005_REGISTER_RW_ACK) {
err("generic read/write, wrong reply code.");
return -EIO;
}
-   if (ibuf[3] != 0x0d) {

Re: Problem with VMAP_STACK=y

[Mauro Carvalho Chehab]

Em Wed, 5 Oct 2016 09:50:42 +0200 (CEST)
Jiri Kosina  escreveu:

> On Wed, 5 Oct 2016, Patrick Boettcher wrote:
> 
> > > > Thanks for the quick response.
> > > > Drivers are:
> > > > dvb_core, dvb_usb, dbv_usb_cynergyT2
> > > 
> > > This dbv_usb_cynergyT2 is not from Linus' tree, is it? I don't seem
> > > to be able to find it, and the only google hit I am getting is your
> > > very mail to LKML :)  
> > 
> > It's a typo, it should say dvb_usb_cinergyT2.  
> 
> Ah, thanks. Same issues there in
> 
>   cinergyt2_frontend_attach()
>   cinergyt2_rc_query()
> 
> I think this would require more in-depth review of all the media drivers 
> and having all this fixed for 4.9. It should be pretty straightforward; 
> all the instances I've seen so far should be just straightforward 
> conversion to kmalloc() + kfree(), as the buffer is not being embedded in 
> any structure etc.

What we're doing on most cases is to put a buffer (usually with 80
chars for USB drivers) inside the "state" struct (on DVB drivers),
in order to avoid doing kmalloc/kfree all the times. One such patch is 
changeset c4a98793a63c4.

I'm enclosing a non-tested patch fixing it for the cinergyT2-core.c
driver.

Thanks,
Mauro

[PATCH] cinergyT2-core: don't do DMA on stack

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.

Signed-off-by: Mauro Carvalho Chehab 

diff --git a/drivers/media/usb/dvb-usb/cinergyT2-core.c 
b/drivers/media/usb/dvb-usb/cinergyT2-core.c
index 9fd1527494eb..2787acc74fcc 100644
--- a/drivers/media/usb/dvb-usb/cinergyT2-core.c
+++ b/drivers/media/usb/dvb-usb/cinergyT2-core.c
@@ -41,6 +41,7 @@ DVB_DEFINE_MOD_OPT_ADAPTER_NR(adapter_nr);
 
 struct cinergyt2_state {
u8 rc_counter;
+   unsigned char data[64];
 };
 
 /* We are missing a release hook with usb_device data */
@@ -50,29 +51,34 @@ 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;
+
+   st->data[0] = CINERGYT2_EP1_CONTROL_STREAM_TRANSFER;
+   st->data[1] = enable ? 1 : 0;
+
+   return dvb_usb_generic_rw(d, st->data, 2, st->data, 64, 0);
 }
 
 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;
+
+   st->data[0] = CINERGYT2_EP1_SLEEP_MODE;
+   st->data[1] = enable ? 1 : 0;
+
+   return dvb_usb_generic_rw(d, st->data, 2, st->data, 3, 0);
 }
 
 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);
+   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");
@@ -141,13 +147,14 @@ 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) {
+   st->data[0] = CINERGYT2_EP1_SLEEP_MODE;
+
+   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) {
@@ -166,13 +173,13 @@ static int cinergyt2_rc_query(struct dvb_usb_device *d, 
u32 *event, int *state)
}
 
/* 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, 

Re: Problem with VMAP_STACK=y

[Jiri Kosina]

On Wed, 5 Oct 2016, Patrick Boettcher wrote:

> > > Thanks for the quick response.
> > > Drivers are:
> > > dvb_core, dvb_usb, dbv_usb_cynergyT2  
> > 
> > This dbv_usb_cynergyT2 is not from Linus' tree, is it? I don't seem
> > to be able to find it, and the only google hit I am getting is your
> > very mail to LKML :)
> 
> It's a typo, it should say dvb_usb_cinergyT2.

Ah, thanks. Same issues there in

cinergyt2_frontend_attach()
cinergyt2_rc_query()

I think this would require more in-depth review of all the media drivers 
and having all this fixed for 4.9. It should be pretty straightforward; 
all the instances I've seen so far should be just straightforward 
conversion to kmalloc() + kfree(), as the buffer is not being embedded in 
any structure etc.

-- 
Jiri Kosina
SUSE Labs

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

dvb-usb stack-memory used for URB-buffers (was: Re: Problem with VMAP_STACK=y)

[Patrick Boettcher]

Hi,

On Tue, 4 Oct 2016 15:26:28 +0200 (CEST)
Jiri Kosina  wrote:

> On Tue, 4 Oct 2016, Jörg Otte wrote:
> 
> > With kernel 4.8.0-01558-g21f54dd I get thousands of
> > "dvb-usb: bulk message failed: -11 (1/0)"
> > messages in the logs and the DVB adapter is not working.
> > 
> > It tourned out the new config option VMAP_STACK=y (which is the
> > default) is the culprit.
> > No problems for me with VMAP_STACK=n.  
> 
> I'd guess that this is EAGAIN coming from usb_hcd_map_urb_for_dma()
> as the DVB driver is trying to perform on-stack DMA.

I really thought that this youngster-mistake of mien (these
drivers+framework date from 2004-2006 and there it worked just fine)
had been fixed some years ago. 

Seems not the be the case :-(.

However, I'm happy to see people using these devices now. Even
though I don't have them anymore (or never had them in the first place).

Mauro, could you please bring me up to speed or remind when and
where you did changes in this regard? I got a little bit rusty
regarding linux-media, but I'd be happy to help.

regards,
-- 
Patrick.
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Problem with VMAP_STACK=y

[Patrick Boettcher]

On Wed, 5 Oct 2016 09:26:29 +0200 (CEST)
Jiri Kosina  wrote:

> On Tue, 4 Oct 2016, Jörg Otte wrote:
> 
> > Thanks for the quick response.
> > Drivers are:
> > dvb_core, dvb_usb, dbv_usb_cynergyT2  
> 
> This dbv_usb_cynergyT2 is not from Linus' tree, is it? I don't seem
> to be able to find it, and the only google hit I am getting is your
> very mail to LKML :)

It's a typo, it should say dvb_usb_cinergyT2.

-- 
Patrick.
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Problem with VMAP_STACK=y

[Jiri Kosina]

On Tue, 4 Oct 2016, Jörg Otte wrote:

> Thanks for the quick response.
> Drivers are:
> dvb_core, dvb_usb, dbv_usb_cynergyT2

This dbv_usb_cynergyT2 is not from Linus' tree, is it? I don't seem to be 
able to find it, and the only google hit I am getting is your very mail to 
LKML :)

-- 
Jiri Kosina
SUSE Labs

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Problem with VMAP_STACK=y

[Jörg Otte]

2016-10-04 15:26 GMT+02:00 Jiri Kosina :
> On Tue, 4 Oct 2016, Jörg Otte wrote:
>
>> With kernel 4.8.0-01558-g21f54dd I get thousands of
>> "dvb-usb: bulk message failed: -11 (1/0)"
>> messages in the logs and the DVB adapter is not working.
>>
>> It tourned out the new config option VMAP_STACK=y (which is the default)
>> is the culprit.
>> No problems for me with VMAP_STACK=n.
>
> I'd guess that this is EAGAIN coming from usb_hcd_map_urb_for_dma() as the
> DVB driver is trying to perform on-stack DMA.
>
> Not really knowing which driver exactly you're using, I quickly skimmed
> through DVB sources, and it turns out this indeed seems to be rather
> common antipattern, and it should be fixed nevertheless. See
>
> cxusb_ctrl_msg()
> dibusb_power_ctrl()
> dibusb2_0_streaming_ctrl()
> dibusb2_0_power_ctrl()
> digitv_ctrl_msg()
> dtt200u_fe_init()
> dtt200u_fe_set_frontend()
> dtt200u_power_ctrl()
> dtt200u_streaming_ctrl()
> dtt200u_pid_filter()
>
> Adding relevant CCs.
>
> --
> Jiri Kosina
> SUSE Labs

Thanks for the quick response.
Drivers are:
dvb_core, dvb_usb, dbv_usb_cynergyT2


Jörg
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Problem with VMAP_STACK=y

[Jiri Kosina]

On Tue, 4 Oct 2016, Jörg Otte wrote:

> With kernel 4.8.0-01558-g21f54dd I get thousands of
> "dvb-usb: bulk message failed: -11 (1/0)"
> messages in the logs and the DVB adapter is not working.
> 
> It tourned out the new config option VMAP_STACK=y (which is the default)
> is the culprit.
> No problems for me with VMAP_STACK=n.

I'd guess that this is EAGAIN coming from usb_hcd_map_urb_for_dma() as the 
DVB driver is trying to perform on-stack DMA.

Not really knowing which driver exactly you're using, I quickly skimmed 
through DVB sources, and it turns out this indeed seems to be rather 
common antipattern, and it should be fixed nevertheless. See

cxusb_ctrl_msg()
dibusb_power_ctrl()
dibusb2_0_streaming_ctrl()
dibusb2_0_power_ctrl()
digitv_ctrl_msg()
dtt200u_fe_init()
dtt200u_fe_set_frontend()
dtt200u_power_ctrl()
dtt200u_streaming_ctrl()
dtt200u_pid_filter()

Adding relevant CCs.

-- 
Jiri Kosina
SUSE Labs
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html