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.
Thanks, Jörg
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
2016-10-04 15:26 GMT+02:00 Jiri Kosina <[email protected]>:
> 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
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
On Wed, 5 Oct 2016 09:26:29 +0200 (CEST)
Jiri Kosina <[email protected]> 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.
Hi,
On Tue, 4 Oct 2016 15:26:28 +0200 (CEST)
Jiri Kosina <[email protected]> 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.
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
Em Wed, 5 Oct 2016 09:50:42 +0200 (CEST)
Jiri Kosina <[email protected]> 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 <[email protected]>
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;
}
Em Wed, 5 Oct 2016 06:04:50 -0300
Mauro Carvalho Chehab <[email protected]> escreveu:
> Em Wed, 5 Oct 2016 09:50:42 +0200 (CEST)
> Jiri Kosina <[email protected]> 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 <[email protected]>
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/write, wrong length in reply.");
return -EIO;
}
- if (ibuf[4] != obuf[4]) {
+ if (st->data[4] != seq) {
err("generic read/write, wrong sequence in reply.");
return -EIO;
}
/*
- Windows driver doesn't check these fields, in fact sometimes
- the register in the reply is different that what has been sent
-
- if (ibuf[5] != obuf[5] || ibuf[6] != obuf[6]) {
- err("generic read/write, wrong register in reply.");
- return -EIO;
- }
- if (ibuf[7] != command) {
- err("generic read/write wrong command in reply.");
- return -EIO;
- }
+ * In thesis, both input and output buffers should have
+ * identical values for st->data[5] to st->data[8].
+ * However, windows driver doesn't check these fields, in fact
+ * sometimes the register in the reply is different that what
+ * has been sent
*/
- if (ibuf[16] != 0x01) {
+ if (st->data[16] != 0x01) {
err("generic read/write wrong status code in reply.");
return -EIO;
}
if (readwrite == AF9005_CMD_READ)
for (i = 0; i < len; i++)
- values[i] = ibuf[8 + i];
+ values[i] = st->data[8 + i];
return 0;
@@ -464,8 +456,7 @@ int af9005_send_command(struct dvb_usb_device *d, u8 command, u8 * wbuf,
struct af9005_device_state *st = d->priv;
int ret, i, packet_len;
- u8 buf[64];
- u8 ibuf[64];
+ u8 seq;
if (wlen < 0) {
err("send command, wlen less than 0 bytes. Makes no sense.");
@@ -480,37 +471,37 @@ int af9005_send_command(struct dvb_usb_device *d, u8 command, u8 * wbuf,
return -EINVAL;
}
packet_len = wlen + 5;
- buf[0] = (u8) (packet_len & 0xff);
- buf[1] = (u8) ((packet_len & 0xff00) >> 8);
-
- buf[2] = 0x26; /* packet type */
- buf[3] = wlen + 3;
- buf[4] = st->sequence++;
- buf[5] = command;
- buf[6] = wlen;
+ st->data[0] = (u8) (packet_len & 0xff);
+ st->data[1] = (u8) ((packet_len & 0xff00) >> 8);
+
+ st->data[2] = 0x26; /* packet type */
+ st->data[3] = wlen + 3;
+ st->data[4] = seq = st->sequence++;
+ st->data[5] = command;
+ st->data[6] = wlen;
for (i = 0; i < wlen; i++)
- buf[7 + i] = wbuf[i];
- ret = dvb_usb_generic_rw(d, buf, wlen + 7, ibuf, rlen + 7, 0);
+ st->data[7 + i] = wbuf[i];
+ ret = dvb_usb_generic_rw(d, st->data, wlen + 7, st->data, rlen + 7, 0);
if (ret)
return ret;
- if (ibuf[2] != 0x27) {
+ if (st->data[2] != 0x27) {
err("send command, wrong reply code.");
return -EIO;
}
- if (ibuf[4] != buf[4]) {
+ if (st->data[4] != seq) {
err("send command, wrong sequence in reply.");
return -EIO;
}
- if (ibuf[5] != 0x01) {
+ if (st->data[5] != 0x01) {
err("send command, wrong status code in reply.");
return -EIO;
}
- if (ibuf[6] != rlen) {
+ if (st->data[6] != rlen) {
err("send command, invalid data length in reply.");
return -EIO;
}
for (i = 0; i < rlen; i++)
- rbuf[i] = ibuf[i + 7];
+ rbuf[i] = st->data[i + 7];
return 0;
}
@@ -518,56 +509,56 @@ int af9005_read_eeprom(struct dvb_usb_device *d, u8 address, u8 * values,
int len)
{
struct af9005_device_state *st = d->priv;
- u8 obuf[16], ibuf[14];
+ u8 seq;
int ret, i;
- memset(obuf, 0, sizeof(obuf));
- memset(ibuf, 0, sizeof(ibuf));
+ memset(st->data, 0, sizeof(st->data));
- obuf[0] = 14; /* length of rest of packet low */
- obuf[1] = 0; /* length of rest of packer high */
+ st->data[0] = 14; /* length of rest of packet low */
+ st->data[1] = 0; /* length of rest of packer high */
- obuf[2] = 0x2a; /* read/write eeprom */
+ st->data[2] = 0x2a; /* read/write eeprom */
- obuf[3] = 12; /* size */
+ st->data[3] = 12; /* size */
- obuf[4] = st->sequence++;
+ st->data[4] = seq = st->sequence++;
- obuf[5] = 0; /* read */
+ st->data[5] = 0; /* read */
- obuf[6] = len;
- obuf[7] = address;
- ret = dvb_usb_generic_rw(d, obuf, 16, ibuf, 14, 0);
+ st->data[6] = len;
+ st->data[7] = address;
+ ret = dvb_usb_generic_rw(d, st->data, 16, st->data, 14, 0);
if (ret)
return ret;
- if (ibuf[2] != 0x2b) {
+ if (st->data[2] != 0x2b) {
err("Read eeprom, invalid reply code");
return -EIO;
}
- if (ibuf[3] != 10) {
+ if (st->data[3] != 10) {
err("Read eeprom, invalid reply length");
return -EIO;
}
- if (ibuf[4] != obuf[4]) {
+ if (st->data[4] != seq) {
err("Read eeprom, wrong sequence in reply ");
return -EIO;
}
- if (ibuf[5] != 1) {
+ if (st->data[5] != 1) {
err("Read eeprom, wrong status in reply ");
return -EIO;
}
for (i = 0; i < len; i++) {
- values[i] = ibuf[6 + i];
+ values[i] = st->data[6 + i];
}
return 0;
}
-static int af9005_boot_packet(struct usb_device *udev, int type, u8 * reply)
+static int af9005_boot_packet(struct usb_device *udev, int type, u8 *reply,
+ u8 *buf, int size)
{
- u8 buf[FW_BULKOUT_SIZE + 2];
u16 checksum;
int act_len, i, ret;
- memset(buf, 0, sizeof(buf));
+
+ memset(buf, 0, size);
buf[0] = (u8) (FW_BULKOUT_SIZE & 0xff);
buf[1] = (u8) ((FW_BULKOUT_SIZE >> 8) & 0xff);
switch (type) {
@@ -720,15 +711,21 @@ static int af9005_download_firmware(struct usb_device *udev, const struct firmwa
{
int i, packets, ret, act_len;
- u8 buf[FW_BULKOUT_SIZE + 2];
+ u8 *buf;
u8 reply;
- ret = af9005_boot_packet(udev, FW_CONFIG, &reply);
+ buf = kmalloc(FW_BULKOUT_SIZE + 2, GFP_KERNEL);
+ if (!buf)
+ return -ENOMEM;
+
+ ret = af9005_boot_packet(udev, FW_CONFIG, &reply, buf,
+ FW_BULKOUT_SIZE + 2);
if (ret)
- return ret;
+ goto err;
if (reply != 0x01) {
err("before downloading firmware, FW_CONFIG expected 0x01, received 0x%x", reply);
- return -EIO;
+ ret = -EIO;
+ goto err;
}
packets = fw->size / FW_BULKOUT_SIZE;
buf[0] = (u8) (FW_BULKOUT_SIZE & 0xff);
@@ -743,28 +740,35 @@ static int af9005_download_firmware(struct usb_device *udev, const struct firmwa
buf, FW_BULKOUT_SIZE + 2, &act_len, 1000);
if (ret) {
err("firmware download failed at packet %d with code %d", i, ret);
- return ret;
+ goto err;
}
}
- ret = af9005_boot_packet(udev, FW_CONFIRM, &reply);
+ ret = af9005_boot_packet(udev, FW_CONFIRM, &reply,
+ buf, FW_BULKOUT_SIZE + 2);
if (ret)
- return ret;
+ goto err;
if (reply != (u8) (packets & 0xff)) {
err("after downloading firmware, FW_CONFIRM expected 0x%x, received 0x%x", packets & 0xff, reply);
- return -EIO;
+ ret = -EIO;
+ goto err;
}
- ret = af9005_boot_packet(udev, FW_BOOT, &reply);
+ ret = af9005_boot_packet(udev, FW_BOOT, &reply, buf,
+ FW_BULKOUT_SIZE + 2);
if (ret)
- return ret;
- ret = af9005_boot_packet(udev, FW_CONFIG, &reply);
+ goto err;
+ ret = af9005_boot_packet(udev, FW_CONFIG, &reply, buf,
+ FW_BULKOUT_SIZE + 2);
if (ret)
- return ret;
+ goto err;
if (reply != 0x02) {
err("after downloading firmware, FW_CONFIG expected 0x02, received 0x%x", reply);
- return -EIO;
+ ret = -EIO;
+ goto err;
}
- return 0;
+err:
+ kfree(buf);
+ return ret;
}
@@ -823,9 +827,7 @@ static int af9005_rc_query(struct dvb_usb_device *d, u32 * event, int *state)
{
struct af9005_device_state *st = d->priv;
int ret, len;
-
- u8 obuf[5];
- u8 ibuf[256];
+ u8 seq;
*state = REMOTE_NO_KEY_PRESSED;
if (rc_decode == NULL) {
@@ -833,33 +835,33 @@ static int af9005_rc_query(struct dvb_usb_device *d, u32 * event, int *state)
return 0;
}
/* deb_info("rc_query\n"); */
- obuf[0] = 3; /* rest of packet length low */
- obuf[1] = 0; /* rest of packet lentgh high */
- obuf[2] = 0x40; /* read remote */
- obuf[3] = 1; /* rest of packet length */
- obuf[4] = st->sequence++; /* sequence number */
- ret = dvb_usb_generic_rw(d, obuf, 5, ibuf, 256, 0);
+ st->data[0] = 3; /* rest of packet length low */
+ st->data[1] = 0; /* rest of packet lentgh high */
+ st->data[2] = 0x40; /* read remote */
+ st->data[3] = 1; /* rest of packet length */
+ st->data[4] = seq = st->sequence++; /* sequence number */
+ ret = dvb_usb_generic_rw(d, st->data, 5, st->data, 256, 0);
if (ret) {
err("rc query failed");
return ret;
}
- if (ibuf[2] != 0x41) {
+ if (st->data[2] != 0x41) {
err("rc query bad header.");
return -EIO;
}
- if (ibuf[4] != obuf[4]) {
+ if (st->data[4] != seq) {
err("rc query bad sequence.");
return -EIO;
}
- len = ibuf[5];
+ len = st->data[5];
if (len > 246) {
err("rc query invalid length");
return -EIO;
}
if (len > 0) {
deb_rc("rc data (%d) ", len);
- debug_dump((ibuf + 6), len, deb_rc);
- ret = rc_decode(d, &ibuf[6], len, event, state);
+ debug_dump((st->data + 6), len, deb_rc);
+ ret = rc_decode(d, &st->data[6], len, event, state);
if (ret) {
err("rc_decode failed");
return ret;
@@ -953,10 +955,16 @@ static int af9005_identify_state(struct usb_device *udev,
int *cold)
{
int ret;
- u8 reply;
- ret = af9005_boot_packet(udev, FW_CONFIG, &reply);
+ u8 reply, *buf;
+
+ buf = kmalloc(FW_BULKOUT_SIZE + 2, GFP_KERNEL);
+ if (!buf)
+ return -ENOMEM;
+
+ ret = af9005_boot_packet(udev, FW_CONFIG, &reply,
+ buf, FW_BULKOUT_SIZE + 2);
if (ret)
- return ret;
+ goto err;
deb_info("result of FW_CONFIG in identify state %d\n", reply);
if (reply == 0x01)
*cold = 1;
@@ -965,7 +973,10 @@ static int af9005_identify_state(struct usb_device *udev,
else
return -EIO;
deb_info("Identify state cold = %d\n", *cold);
- return 0;
+
+err:
+ kfree(buf);
+ return ret;
}
static struct dvb_usb_device_properties af9005_properties;
Thanks,
Mauro
2016-10-05 11:04 GMT+02:00 Mauro Carvalho Chehab <[email protected]>:
> Em Wed, 5 Oct 2016 09:50:42 +0200 (CEST)
> Jiri Kosina <[email protected]> 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
On Wed, Oct 5, 2016 at 8:21 AM, Jörg Otte <[email protected]> wrote:
> 2016-10-05 11:04 GMT+02:00 Mauro Carvalho Chehab <[email protected]>:
>> Em Wed, 5 Oct 2016 09:50:42 +0200 (CEST)
>> Jiri Kosina <[email protected]> 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
2016-10-05 17:53 GMT+02:00 Andy Lutomirski <[email protected]>:
> On Wed, Oct 5, 2016 at 8:21 AM, Jörg Otte <[email protected]> wrote:
>> 2016-10-05 11:04 GMT+02:00 Mauro Carvalho Chehab <[email protected]>:
>>> Em Wed, 5 Oct 2016 09:50:42 +0200 (CEST)
>>> Jiri Kosina <[email protected]> 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
On Wed, Oct 5, 2016 at 9:45 AM, Jörg Otte <[email protected]> wrote:
> 2016-10-05 17:53 GMT+02:00 Andy Lutomirski <[email protected]>:
>> On Wed, Oct 5, 2016 at 8:21 AM, Jörg Otte <[email protected]> wrote:
>>> 2016-10-05 11:04 GMT+02:00 Mauro Carvalho Chehab <[email protected]>:
>>>> Em Wed, 5 Oct 2016 09:50:42 +0200 (CEST)
>>>> Jiri Kosina <[email protected]> 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
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
Hi Johannes,
Em Wed, 5 Oct 2016 20:29:45 +0200
Johannes Stezenbach <[email protected]> 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
2016-10-05 20:55 GMT+02:00 Mauro Carvalho Chehab <[email protected]>:
> Hi Johannes,
>
> Em Wed, 5 Oct 2016 20:29:45 +0200
> Johannes Stezenbach <[email protected]> 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
Em Thu, 6 Oct 2016 10:30:15 +0200
Jörg Otte <[email protected]> escreveu:
> 2016-10-05 20:55 GMT+02:00 Mauro Carvalho Chehab <[email protected]>:
> > Hi Johannes,
> >
> > Em Wed, 5 Oct 2016 20:29:45 +0200
> > Johannes Stezenbach <[email protected]> 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
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
Em Fri, 7 Oct 2016 09:52:56 +0200 (CEST)
Jiri Kosina <[email protected]> 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