3.9 merge window kernels

Sat Feb 23 20:41:42 UTC 2013

Em Sat, 23 Feb 2013 22:30:57 +0200
Antti Palosaari <crope at iki.fi> escreveu:

> On 02/23/2013 03:30 AM, Mauro Carvalho Chehab wrote:
> > Em Sat, 23 Feb 2013 02:09:01 +0100
> > poma <pomidorabelisima at gmail.com> escreveu:
> >
> >> On 02/23/13 01:09, Mauro Carvalho Chehab wrote:
> 
> >> This one stayed;
> >> …
> >> ------------[ cut here ]------------
> >> WARNING: at lib/dma-debug.c:947 check_for_stack+0xa7/0xf0()
> >> Hardware name: M720-US3
> >> ehci-pci 0000:00:04.1: DMA-API: device driver maps memory fromstack
> >> [addr=ffff8801283d7be1]
> >> Modules linked in: dvb_usb_af9015(OF+)… dvb_usb_v2(OF)… dvb_core(OF)
> >> wm8775(OF) tda9887(OF) tda8290(OF)… tuner(OF)… cx25840(OF)… bttv(OF+)…
> >> rc_core(OF) btcx_risc(OF) ivtv(OF) snd_bt87x… videobuf_dma_sg(OF)
> >> videobuf_core(OF)… cx2341x(OF) tveeprom(OF) v4l2_common(OF)…
> >> videodev(OF)… video i2c_algo_bit… (irrelevant omitted)
> >> Pid: 38, comm: kworker/2:1 Tainted: GF          O
> >> 3.9.0-0.rc0.git5.1.fc18.x86_64 #1
> >> Call Trace:
> >> usb 4-2: pl2303 converter now attached to ttyUSB0
> >>   [<ffffffff81069b5f>] warn_slowpath_common+0x7f/0xc0
> >>   [<ffffffff81069c56>] warn_slowpath_fmt+0x46/0x50
> >>   [<ffffffff81386467>] check_for_stack+0xa7/0xf0
> >>   [<ffffffff81388228>] debug_dma_map_page+0x118/0x150
> >>   [<ffffffff814fa5d9>] usb_hcd_map_urb_for_dma+0x569/0x630
> >>   [<ffffffff811be091>] ? set_track+0x61/0x1b0
> >>   [<ffffffff814fa945>] usb_hcd_submit_urb+0x2a5/0x900
> >>   [<ffffffff810d814c>] ? lockdep_init_map+0xac/0x540
> >>   [<ffffffff814fc07f>] usb_submit_urb+0xff/0x3d0
> >>   [<ffffffff814fd042>] usb_start_wait_urb+0x82/0x1a0
> >>   [<ffffffff814fbdae>] ? usb_alloc_urb+0x1e/0x50
> >>   [<ffffffff814fd22c>] usb_bulk_msg+0xcc/0x180
> >>   [<ffffffffa05be7e8>] dvb_usbv2_generic_rw+0xd8/0x270 [dvb_usb_v2]
> >>   [<ffffffffa05cc5da>] af9015_ctrl_msg+0x1da/0x280 [dvb_usb_af9015]
> >>   [<ffffffff810ae7aa>] ? cpuacct_charge+0xfa/0x210
> >>   [<ffffffff810ae6c3>] ? cpuacct_charge+0x13/0x210
> >>   [<ffffffffa05ccb4c>] af9015_identify_state+0x3c/0x90 [dvb_usb_af9015]
> >>   [<ffffffffa05bcf18>] dvb_usbv2_init_work+0x68/0x11a0 [dvb_usb_v2]
> >>   [<ffffffff8108e36f>] ? process_one_work+0x19f/0x690
> >>   [<ffffffff8108e3db>] process_one_work+0x20b/0x690
> >>   [<ffffffff8108e36f>] ? process_one_work+0x19f/0x690
> >>   [<ffffffff8108ec10>] worker_thread+0x110/0x380
> >>   [<ffffffff8108eb00>] ? rescuer_thread+0x260/0x260
> >>   [<ffffffff8109569d>] kthread+0xed/0x100
> >>   [<ffffffff810955b0>] ? flush_kthread_worker+0x190/0x190
> >>   [<ffffffff8173882c>] ret_from_fork+0x7c/0xb0
> >>   [<ffffffff810955b0>] ? flush_kthread_worker+0x190/0x190
> >> ---[ end trace 4f4bc1c2abe300b5 ]---
> >> --
> >
> > Ah, you're running it on a non-x86 hardware, right? On (modern) x86 machines,
> > almost all memories can do DMA, including the area used by stack. That's
> > not true on other archs like arm.
> >
> > I suspect that Antti never tried to run this driver outside x86.
> 
> Yep, that's true! I will generally test only x86 64bit and let the 
> others test x86 32bit. Basically x86 is only which is tested by multiple 
> users. MIPS and ARM are something that could be nice to test frequently, 
> but I have no hw...
> 
> > Let me look into it...
> >
> > static int af9015_ctrl_msg(struct dvb_usb_device *d, struct req_t *req)
> > {
> > #define BUF_LEN 63
> > #define REQ_HDR_LEN 8 /* send header size */
> > #define ACK_HDR_LEN 2 /* rece header size */
> >          struct af9015_state *state = d_to_priv(d);
> >          int ret, wlen, rlen;
> >          u8 buf[BUF_LEN];
> > ...
> >          ret = dvb_usbv2_generic_rw(d, buf, wlen, buf, rlen);
> >
> >
> >
> > int dvb_usbv2_generic_rw(struct dvb_usb_device *d, u8 *wbuf, u16 wlen, u8 *rbuf,
> >                  u16 rlen)
> > {
> >          int ret, actual_length;
> >
> > ...
> >          ret = usb_bulk_msg(d->udev, usb_sndbulkpipe(d->udev,
> >                          d->props->generic_bulk_ctrl_endpoint), wbuf, wlen,
> >                          &actual_length, 2000);
> >
> >
> > That's the problem: "buf" is at stack, and it is passed directly to
> > usb_bulk_msg(). This doesn't work on -arm (and sometimes may cause
> > data to be lost even on x86).
> >
> > In order to fix it, the driver should be doing, instead, something like:
> >
> > static int af9015_ctrl_msg(struct dvb_usb_device *d, struct req_t *req)
> > {
> > ...
> > 	u8 *buf = kmalloc(BUF_LEN, GFP_ATOMIC);
> >
> > 	...
> >
> > 	kfree(buf);
> > ...
> > }
> 
> I remember patch set about two years ago which removes usb_control_msg() 
> stacked buffers from the all the /media/ USB drivers. At that time 
> stacked use of usb_bulk_msg() was left as it was OK(?).

No, that's not ok. Probably people just forgot usb_bulk_msg() when that
patch got submitted.

> > You shouldn't blame the merge windows kernel here. This never worked
> > on arm, even on older kernel versions ;)
> >
> > A more permanent fix would be to allocate a temporary 80 bytes buffer
> > embedded with dvb_usb_v2 structs, and change dvb_usbv2_generic_rw to
> > always use this temporary buffer for the control URB transfers.
> >
> > Antti,
> >
> > Could you please tale a look in it?
> 
> I am happy to alloc() all the buffers what are used by drivers I made, 
> if that is the reason of these bugs (or even if it is the correct way 
> despite that bug or not).

IMO, the better is to allocate it before. The only drawback is that a
mutex is needed to serialize the buffer usage, otherwise the remote control
and CA logic may race with usual ioctl handling that might try to access
the device's registers.

I would put that logic in the dvb-usb/dvb-usb-v2 core.

With this new 3.8 "pedantic" dma mode, people will start complain about it
anyway on all drivers.

Regards,
Mauro