Re: Problem with VMAP_STACK=y
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
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
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, &cmd, 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
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, &cmd, 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
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, &cmd, 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
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, &cmd, 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
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
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
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
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
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) { + if (st->data[3] != 0x0d) { err("generic read/w
Re: Problem with VMAP_STACK=y
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, &cmd, 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, st->data); } return 0; }
Re: Problem with VMAP_STACK=y
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)
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
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
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
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
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