3.9 merge window kernels

Sat Feb 23 23:07:08 UTC 2013

On 02/23/2013 10:41 PM, Mauro Carvalho Chehab wrote:
> 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.

It was that patch set which did that fix for usb_control_msg()

https://lkml.org/lkml/2011/4/3/67
https://bugzilla.kernel.org/show_bug.cgi?id=15977

So now same for the usb_bulk_msg()

>>> 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.

I would like to put those transfer buffers to the driver state. For the 
DVB USB it was not possible as there was no pointer to state early 
enough, like identify state & download firmware phases. For the DVB USB 
V2 there is and it is very simply to move buffers to the state.

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

regards
Antti

-- 
http://palosaari.fi/