@@ -96,7 +96,6 @@ struct picolcd_data {u8*fb_vbitmap;/* local copy of what was sent to PicoLCD */u8*fb_bitmap;/* framebuffer */structfb_info*fb_info;-structfb_deferred_iofb_defio;#endif /* CONFIG_HID_PICOLCD_FB */#ifdef CONFIG_HID_PICOLCD_LCDstructlcd_device*lcd;
@@ -179,8 +178,6 @@ int picolcd_init_framebuffer(struct picolcd_data *data);voidpicolcd_exit_framebuffer(structpicolcd_data*data);-voidpicolcd_fb_unload(void);-voidpicolcd_fb_refresh(structpicolcd_data*data);#define picolcd_fbinfo(d) ((d)->fb_info)#else
@@ -195,9 +192,6 @@ static inline int picolcd_init_framebuffer(struct picolcd_data *data)staticinlinevoidpicolcd_exit_framebuffer(structpicolcd_data*data){}-staticinlinevoidpicolcd_fb_unload(void)-{-}staticinlinevoidpicolcd_fb_refresh(structpicolcd_data*data){}
@@ -327,24 +327,17 @@ static int picolcd_fb_blank(int blank, struct fb_info *info)staticvoidpicolcd_fb_destroy(structfb_info*info){-structpicolcd_data*data=info->par;-u32*ref_cnt=info->pseudo_palette;-intmay_release;+structpicolcd_data*data;+/* make sure no work is deferred */+cancel_delayed_work_sync(&info->deferred_work);+data=info->par;info->par=NULL;if(data)data->fb_info=NULL;-fb_deferred_io_cleanup(info);-ref_cnt--;-mutex_lock(&info->lock);-(*ref_cnt)--;-may_release=!*ref_cnt;-mutex_unlock(&info->lock);-if(may_release){-vfree((u8*)info->fix.smem_start);-framebuffer_release(info);-}+vfree((u8*)info->fix.smem_start);+framebuffer_release(info);}staticintpicolcd_fb_check_var(structfb_var_screeninfo*var,structfb_info*info)
@@ -414,77 +407,10 @@ static int picolcd_set_par(struct fb_info *info)return0;}-/* Do refcounting on our FB and cleanup per worker if FB is-*closedafterunplugofourdevice-*(fb_releaseholdsinfo->lockandstilltouchesinfoafter-*wereturnsowecan'treleaseitimmediately.-*/-structpicolcd_fb_cleanup_item{-structfb_info*info;-structpicolcd_fb_cleanup_item*next;-};-staticstructpicolcd_fb_cleanup_item*fb_pending;-staticDEFINE_SPINLOCK(fb_pending_lock);--staticvoidpicolcd_fb_do_cleanup(structwork_struct*data)-{-structpicolcd_fb_cleanup_item*item;-unsignedlongflags;--do{-spin_lock_irqsave(&fb_pending_lock,flags);-item=fb_pending;-fb_pending=item?item->next:NULL;-spin_unlock_irqrestore(&fb_pending_lock,flags);--if(item){-u8*fb=(u8*)item->info->fix.smem_start;-/* make sure we do not race against fb core when-*releasing*/-mutex_lock(&item->info->lock);-mutex_unlock(&item->info->lock);-framebuffer_release(item->info);-vfree(fb);-}-}while(item);-}--staticDECLARE_WORK(picolcd_fb_cleanup,picolcd_fb_do_cleanup);--staticintpicolcd_fb_open(structfb_info*info,intu)-{-u32*ref_cnt=info->pseudo_palette;-ref_cnt--;--(*ref_cnt)++;-return0;-}--staticintpicolcd_fb_release(structfb_info*info,intu)-{-u32*ref_cnt=info->pseudo_palette;-ref_cnt--;--(*ref_cnt)++;-if(!*ref_cnt){-unsignedlongflags;-structpicolcd_fb_cleanup_item*item=(structpicolcd_fb_cleanup_item*)ref_cnt;-item--;-spin_lock_irqsave(&fb_pending_lock,flags);-item->next=fb_pending;-fb_pending=item;-spin_unlock_irqrestore(&fb_pending_lock,flags);-schedule_work(&picolcd_fb_cleanup);-}-return0;-}-/* Note this can't be const because of struct fb_info definition */staticstructfb_opspicolcdfb_ops={.owner=THIS_MODULE,.fb_destroy=picolcd_fb_destroy,-.fb_open=picolcd_fb_open,-.fb_release=picolcd_fb_release,.fb_read=fb_sys_read,.fb_write=picolcd_fb_write,.fb_blank=picolcd_fb_blank,
@@ -580,25 +506,23 @@ int picolcd_init_framebuffer(struct picolcd_data *data)}data->fb_update_rate=PICOLCDFB_UPDATE_RATE_DEFAULT;-data->fb_defio=picolcd_fb_defio;/* The extra memory is:-*-structpicolcd_fb_cleanup_item-*-u32forref_count*-256*u32forpseudo_palette+*-structfb_deferred_io*/-info=framebuffer_alloc(257*sizeof(u32)+sizeof(structpicolcd_fb_cleanup_item),dev);+info=framebuffer_alloc(256*sizeof(u32)++sizeof(structfb_deferred_io),dev);if(info=NULL){dev_err(dev,"failed to allocate a framebuffer\n");gotoerr_nomem;}-palette=info->par+sizeof(structpicolcd_fb_cleanup_item);-*palette=1;-palette++;+info->fbdefio=info->par;+*info->fbdefio=picolcd_fb_defio;+palette=info->par+sizeof(structfb_deferred_io);for(i=0;i<256;i++)palette[i]=i>0&&i<16?0xff:0;info->pseudo_palette=palette;-info->fbdefio=&data->fb_defio;info->screen_base=(char__force__iomem*)fb_bitmap;info->fbops=&picolcdfb_ops;info->var=picolcdfb_var;
@@ -226,13 +226,16 @@ int picolcd_fb_reset(struct picolcd_data *data, int clear)}/* Update fb_vbitmap from the screen_base and send changed tiles to device */-staticvoidpicolcd_fb_update(structpicolcd_data*data)+staticvoidpicolcd_fb_update(structfb_info*info){intchip,tile,n;unsignedlongflags;+structpicolcd_data*data;+mutex_lock(&info->lock);+data=info->par;if(!data)-return;+gotoout;spin_lock_irqsave(&data->lock,flags);if(!(data->status&PICOLCD_READY_FB)){
@@ -256,17 +259,31 @@ static void picolcd_fb_update(struct picolcd_data *data)data->fb_bitmap,data->fb_bpp,chip,tile)||data->fb_force){n+=2;-if(data->status&PICOLCD_FAILED)-return;/* device lost! */if(n>=HID_OUTPUT_FIFO_SIZE/2){+mutex_unlock(&info->lock);usbhid_wait_io(data->hdev);+mutex_lock(&info->lock);+data=info->par;+if(!data)+gotoout;+spin_lock_irqsave(&data->lock,flags);+if(data->status&PICOLCD_FAILED){+spin_unlock_irqrestore(&data->lock,flags);+gotoout;+}+spin_unlock_irqrestore(&data->lock,flags);n=0;}picolcd_fb_send_tile(data->hdev,chip,tile);}data->fb_force=false;-if(n)+if(n){+mutex_unlock(&info->lock);usbhid_wait_io(data->hdev);+return;+}+out:+mutex_unlock(&info->lock);}/* Stub to call the system default and update the image on the picoLCD */
@@ -327,17 +344,12 @@ static int picolcd_fb_blank(int blank, struct fb_info *info)staticvoidpicolcd_fb_destroy(structfb_info*info){-structpicolcd_data*data;-/* make sure no work is deferred */-cancel_delayed_work_sync(&info->deferred_work);-data=info->par;-info->par=NULL;-if(data)-data->fb_info=NULL;+fb_deferred_io_cleanup(info);vfree((u8*)info->fix.smem_start);framebuffer_release(info);+printk(KERN_DEBUG"picolcd_fb_destroy(%p)\n",info);}staticintpicolcd_fb_check_var(structfb_var_screeninfo*var,structfb_info*info)
From: David Herrmann <hidden> Date: 2012-07-31 07:26:12
Hi Bruno
On Mon, Jul 30, 2012 at 9:36 PM, Bruno Prémont
[off-list ref] wrote:
Hi,
This series updates picoLCD driver:
- split the driver functions into separate files which get included
depending on Kconfig selection
(implementation for CIR using RC_CORE will follow later)
- drop private framebuffer refcounting in favor of refcounting added
to fb_info some time ago
- fix various bugs issues
- disabled firmware version checking in probe() as it does not work
anymore since commit 4ea5454203d991ec85264f64f89ca8855fce69b0
[HID: Fix race condition between driver core and ll-driver]
Note: I still get weird behavior on quick unbind/bind sequences
issued via sysfs (CONFIG_SMP=n system) that are triggered by framebuffer
support and apparently more specifically fb_defio part of it.
Unfortunately I'm out of ideas as to how to track down the problem which
shows either as SLAB corruption (detected with SLUB debugging, e.g.
[ 6383.521833] ======================================> [ 6383.530020] BUG kmalloc-64 (Not tainted): Object already free
[ 6383.530020] -----------------------------------------------------------------------------
[ 6383.530020]
[ 6383.530020] INFO: Slab 0xdde0ea20 objectsQ used@ fp=0xcef516e0 flags=0x40000080
[ 6383.530020] INFO: Object 0xcef51190 @offset@0 fp=0xcef51f50
[ 6383.530020]
[ 6383.530020] Bytes b4 cef51180: cc cc cc cc d0 12 f5 ce 5a 5a 5a 5a 5a 5a 5a 5a ........ZZZZZZZZ
[ 6383.530020] Object cef51190: 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b kkkkkkkkkkkkkkkk
[ 6383.530020] Object cef511a0: 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b kkkkkkkkkkkkkkkk
[ 6383.530020] Object cef511b0: 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b kkkkkkkkkkkkkkkk
[ 6383.530020] Object cef511c0: 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b a5 kkkkkkkkkkkkkkk.
[ 6383.530020] Redzone cef511d0: bb bb bb bb ....
[ 6383.530020] Padding cef511d8: 5a 5a 5a 5a 5a 5a 5a 5a ZZZZZZZZ
[ 6383.530020] Pid: 1922, comm: bash Not tainted 3.5.0-jupiter-00003-g8d858b1-dirty #2
[ 6383.530020] Call Trace:
[ 6383.530020] [<c10bd3cc>] print_trailer+0x11c/0x130
[ 6383.530020] [<c10bd415>] object_err+0x35/0x40
[ 6383.530020] [<c10be809>] free_debug_processing+0x99/0x200
[ 6383.530020] [<c10bf77e>] __slab_free+0x2e/0x280
[ 6383.530020] [<c1322284>] ? hid_submit_out+0xa4/0x120
[ 6383.530020] [<c1322870>] ? __usbhid_submit_report+0xc0/0x3c0
[ 6383.530020] [<c10bfbda>] ? kfree+0xfa/0x110
[ 6383.530020] [<de932aa4>] ? picolcd_debug_out_report+0x8c4/0x8e0 [hid_picolcd]
[ 6383.530020] [<c10bfbda>] kfree+0xfa/0x110
[ 6383.530020] [<c1322284>] ? hid_submit_out+0xa4/0x120
[ 6383.530020] [<c1322284>] ? hid_submit_out+0xa4/0x120
[ 6383.530020] [<c1322284>] ? hid_submit_out+0xa4/0x120
[ 6383.530020] [<c1322284>] hid_submit_out+0xa4/0x120
[ 6383.530020] [<c1322908>] __usbhid_submit_report+0x158/0x3c0
[ 6383.530020] [<c1322c2b>] usbhid_submit_report+0x1b/0x30
[ 6383.530020] [<de930789>] picolcd_fb_reset+0xb9/0x180 [hid_picolcd]
[ 6383.530020] [<de930f1d>] picolcd_init_framebuffer+0x20d/0x2e0 [hid_picolcd]
[ 6383.530020] [<de92fb9c>] picolcd_probe+0x3cc/0x580 [hid_picolcd]
[ 6383.530020] [<c1319147>] hid_device_probe+0x67/0xf0
[ 6383.530020] [<c1282f97>] ? driver_sysfs_add+0x57/0x80
[ 6383.530020] [<c128329d>] driver_probe_device+0xbd/0x1c0
[ 6383.530020] [<c1318a1b>] ? hid_match_device+0x7b/0x90
[ 6383.530020] [<c12821e5>] driver_bind+0x75/0xd0
[ 6383.530020] [<c1282170>] ? driver_unbind+0x90/0x90
[ 6383.530020] [<c12818b7>] drv_attr_store+0x27/0x30
[ 6383.530020] [<c1114aec>] sysfs_write_file+0xac/0xf0
[ 6383.530020] [<c10c794c>] vfs_write+0x9c/0x130
[ 6383.530020] [<c10d4a1f>] ? sys_dup3+0x11f/0x160
[ 6383.530020] [<c1114a40>] ? sysfs_poll+0x90/0x90
[ 6383.530020] [<c10c7bbd>] sys_write+0x3d/0x70
[ 6383.530020] [<c13f2557>] sysenter_do_call+0x12/0x26
[ 6383.530020] FIX kmalloc-64: Object at 0xcef51190 not freed
or worse spontaneous reboot of the system without any trace on netconsole or
serial console.
echo $devid > bind; echo $devid > unbind
or
echo $devid > bind; echo $devid > unbind; sleep 0.2; echo $devid > bind; echo $devid > unbind
is sufficient to trigger the above issue while waiting a few seconds between bind and unbind
shows no sign of trouble.
Suggestions as to how to debug this and fix it are welcome!
This is not directly related to this patchset, but did you fix the
locking issue with hid-core? It is still on my todo-list but I haven't
gotten around fixing it, yet. However, I plan on fixing it this
summer, but if picolcd does not require it, it's probably not worth
the work.
Regards
David
From: Bruno Prémont <bonbons@linux-vserver.org> Date: 2012-07-31 07:59:15
Hi David,
On Tue, 31 Jul 2012 09:26:07 David Herrmann wrote:
This is not directly related to this patchset, but did you fix the
locking issue with hid-core? It is still on my todo-list but I haven't
gotten around fixing it, yet. However, I plan on fixing it this
summer, but if picolcd does not require it, it's probably not worth
the work.
I worked around it with patch 6/7 as in disabling the version check.
The other probing steps don't expect a response from the hardware, thus
are not affected.
But it would definitely be helpful to be able to revert that one.
Understanding and fixing the FB-related misbehavior on quick bind/unbind
seems more important though.
Not related with this, getting a return value from
usbhid_submit_report() as in -EAGAIN or -ENODEV would help in order
to avoid the ugly
hid-picolcd 0003:04D8:C002.0003: usb_submit_urb(out) failed: -19
lines in kernel log and stopping interaction with hardware when it's
gone already or giving it time to consume its queue when there is lots
of data being sent.
Regards,
Bruno
From: Bruno Prémont <bonbons@linux-vserver.org> Date: 2012-08-09 18:10:31
Hi Tejun,
As you are working on workqueues and related code, could you have a look
at my usage of them in combination with db_defio?
The delayed memory corruptions or system reboots after unbinding/unplugging
the PicoLCD seem very much related to workqueue used to handle the deferred
IO to framebuffer.
I think things don't get cleaned-up as they should though I'm not sure
where the trouble lies.
For ease of reading, I'm inlineing below the framebuffer related code of
PicoLCD (for complete driver after this patch series apply the whole series
on top of 3.5 https://lkml.org/lkml/2012/7/30/375 )
Thanks,
Bruno
On Mon, 30 July 2012 Bruno Prémont [off-list ref] wrote:
This series updates picoLCD driver:
- split the driver functions into separate files which get included
depending on Kconfig selection
(implementation for CIR using RC_CORE will follow later)
- drop private framebuffer refcounting in favor of refcounting added
to fb_info some time ago
- fix various bugs issues
- disabled firmware version checking in probe() as it does not work
anymore since commit 4ea5454203d991ec85264f64f89ca8855fce69b0
[HID: Fix race condition between driver core and ll-driver]
Note: I still get weird behavior on quick unbind/bind sequences
issued via sysfs (CONFIG_SMP=n system) that are triggered by framebuffer
support and apparently more specifically fb_defio part of it.
Unfortunately I'm out of ideas as to how to track down the problem which
shows either as SLAB corruption (detected with SLUB debugging, e.g.
[ 6383.521833] ======================================> [ 6383.530020] BUG kmalloc-64 (Not tainted): Object already free
[ 6383.530020] -----------------------------------------------------------------------------
[ 6383.530020]
[ 6383.530020] INFO: Slab 0xdde0ea20 objectsQ used@ fp=0xcef516e0 flags=0x40000080
[ 6383.530020] INFO: Object 0xcef51190 @offset@0 fp=0xcef51f50
[ 6383.530020]
[ 6383.530020] Bytes b4 cef51180: cc cc cc cc d0 12 f5 ce 5a 5a 5a 5a 5a 5a 5a 5a ........ZZZZZZZZ
[ 6383.530020] Object cef51190: 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b kkkkkkkkkkkkkkkk
[ 6383.530020] Object cef511a0: 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b kkkkkkkkkkkkkkkk
[ 6383.530020] Object cef511b0: 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b kkkkkkkkkkkkkkkk
[ 6383.530020] Object cef511c0: 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b a5 kkkkkkkkkkkkkkk.
[ 6383.530020] Redzone cef511d0: bb bb bb bb ....
[ 6383.530020] Padding cef511d8: 5a 5a 5a 5a 5a 5a 5a 5a ZZZZZZZZ
[ 6383.530020] Pid: 1922, comm: bash Not tainted 3.5.0-jupiter-00003-g8d858b1-dirty #2
[ 6383.530020] Call Trace:
[ 6383.530020] [<c10bd3cc>] print_trailer+0x11c/0x130
[ 6383.530020] [<c10bd415>] object_err+0x35/0x40
[ 6383.530020] [<c10be809>] free_debug_processing+0x99/0x200
[ 6383.530020] [<c10bf77e>] __slab_free+0x2e/0x280
[ 6383.530020] [<c1322284>] ? hid_submit_out+0xa4/0x120
[ 6383.530020] [<c1322870>] ? __usbhid_submit_report+0xc0/0x3c0
[ 6383.530020] [<c10bfbda>] ? kfree+0xfa/0x110
[ 6383.530020] [<de932aa4>] ? picolcd_debug_out_report+0x8c4/0x8e0 [hid_picolcd]
[ 6383.530020] [<c10bfbda>] kfree+0xfa/0x110
[ 6383.530020] [<c1322284>] ? hid_submit_out+0xa4/0x120
[ 6383.530020] [<c1322284>] ? hid_submit_out+0xa4/0x120
[ 6383.530020] [<c1322284>] ? hid_submit_out+0xa4/0x120
[ 6383.530020] [<c1322284>] hid_submit_out+0xa4/0x120
[ 6383.530020] [<c1322908>] __usbhid_submit_report+0x158/0x3c0
[ 6383.530020] [<c1322c2b>] usbhid_submit_report+0x1b/0x30
[ 6383.530020] [<de930789>] picolcd_fb_reset+0xb9/0x180 [hid_picolcd]
[ 6383.530020] [<de930f1d>] picolcd_init_framebuffer+0x20d/0x2e0 [hid_picolcd]
[ 6383.530020] [<de92fb9c>] picolcd_probe+0x3cc/0x580 [hid_picolcd]
[ 6383.530020] [<c1319147>] hid_device_probe+0x67/0xf0
[ 6383.530020] [<c1282f97>] ? driver_sysfs_add+0x57/0x80
[ 6383.530020] [<c128329d>] driver_probe_device+0xbd/0x1c0
[ 6383.530020] [<c1318a1b>] ? hid_match_device+0x7b/0x90
[ 6383.530020] [<c12821e5>] driver_bind+0x75/0xd0
[ 6383.530020] [<c1282170>] ? driver_unbind+0x90/0x90
[ 6383.530020] [<c12818b7>] drv_attr_store+0x27/0x30
[ 6383.530020] [<c1114aec>] sysfs_write_file+0xac/0xf0
[ 6383.530020] [<c10c794c>] vfs_write+0x9c/0x130
[ 6383.530020] [<c10d4a1f>] ? sys_dup3+0x11f/0x160
[ 6383.530020] [<c1114a40>] ? sysfs_poll+0x90/0x90
[ 6383.530020] [<c10c7bbd>] sys_write+0x3d/0x70
[ 6383.530020] [<c13f2557>] sysenter_do_call+0x12/0x26
[ 6383.530020] FIX kmalloc-64: Object at 0xcef51190 not freed
or worse spontaneous reboot of the system without any trace on netconsole or
serial console.
echo $devid > bind; echo $devid > unbind
or
echo $devid > bind; echo $devid > unbind; sleep 0.2; echo $devid > bind; echo $devid > unbind
is sufficient to trigger the above issue while waiting a few seconds between bind and unbind
shows no sign of trouble.
Suggestions as to how to debug this and fix it are welcome!
========= driver/hid/hid-picolcd_fb.c ===============
/***************************************************************************
* Copyright (C) 2010-2012 by Bruno Prémont [off-list ref] *
* *
* Based on Logitech G13 driver (v0.4) *
* Copyright (C) 2009 by Rick L. Vinyard, Jr. [off-list ref] *
* *
* This program is free software: you can redistribute it and/or modify *
* it under the terms of the GNU General Public License as published by *
* the Free Software Foundation, version 2 of the License. *
* *
* This driver is distributed in the hope that it will be useful, but *
* WITHOUT ANY WARRANTY; without even the implied warranty of *
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU *
* General Public License for more details. *
* *
* You should have received a copy of the GNU General Public License *
* along with this software. If not see <http://www.gnu.org/licenses/>. *
***************************************************************************/
#include <linux/hid.h>
#include "usbhid/usbhid.h"
#include <linux/usb.h>
#include <linux/fb.h>
#include <linux/module.h>
#include "hid-picolcd.h"
/* Framebuffer
*
* The PicoLCD use a Topway LCD module of 256x64 pixel
* This display area is tiled over 4 controllers with 8 tiles
* each. Each tile has 8x64 pixel, each data byte representing
* a 1-bit wide vertical line of the tile.
*
* The display can be updated at a tile granularity.
*
* Chip 1 Chip 2 Chip 3 Chip 4
* +----------------+----------------+----------------+----------------+
* | Tile 1 | Tile 1 | Tile 1 | Tile 1 |
* +----------------+----------------+----------------+----------------+
* | Tile 2 | Tile 2 | Tile 2 | Tile 2 |
* +----------------+----------------+----------------+----------------+
* ...
* +----------------+----------------+----------------+----------------+
* | Tile 8 | Tile 8 | Tile 8 | Tile 8 |
* +----------------+----------------+----------------+----------------+
*/
#define PICOLCDFB_NAME "picolcdfb"
#define PICOLCDFB_WIDTH (256)
#define PICOLCDFB_HEIGHT (64)
#define PICOLCDFB_SIZE (PICOLCDFB_WIDTH * PICOLCDFB_HEIGHT / 8)
#define PICOLCDFB_UPDATE_RATE_LIMIT 10
#define PICOLCDFB_UPDATE_RATE_DEFAULT 2
/* Framebuffer visual structures */
static const struct fb_fix_screeninfo picolcdfb_fix = {
.id = PICOLCDFB_NAME,
.type = FB_TYPE_PACKED_PIXELS,
.visual = FB_VISUAL_MONO01,
.xpanstep = 0,
.ypanstep = 0,
.ywrapstep = 0,
.line_length = PICOLCDFB_WIDTH / 8,
.accel = FB_ACCEL_NONE,
};
static const struct fb_var_screeninfo picolcdfb_var = {
.xres = PICOLCDFB_WIDTH,
.yres = PICOLCDFB_HEIGHT,
.xres_virtual = PICOLCDFB_WIDTH,
.yres_virtual = PICOLCDFB_HEIGHT,
.width = 103,
.height = 26,
.bits_per_pixel = 1,
.grayscale = 1,
.red = {
.offset = 0,
.length = 1,
.msb_right = 0,
},
.green = {
.offset = 0,
.length = 1,
.msb_right = 0,
},
.blue = {
.offset = 0,
.length = 1,
.msb_right = 0,
},
.transp = {
.offset = 0,
.length = 0,
.msb_right = 0,
},
};
/* Send a given tile to PicoLCD */
static int picolcd_fb_send_tile(struct hid_device *hdev, int chip, int tile)
{
struct picolcd_data *data = hid_get_drvdata(hdev);
struct hid_report *report1 = picolcd_out_report(REPORT_LCD_CMD_DATA, hdev);
struct hid_report *report2 = picolcd_out_report(REPORT_LCD_DATA, hdev);
unsigned long flags;
u8 *tdata;
int i;
if (!report1 || report1->maxfield != 1 || !report2 || report2->maxfield != 1)
return -ENODEV;
spin_lock_irqsave(&data->lock, flags);
hid_set_field(report1->field[0], 0, chip << 2);
hid_set_field(report1->field[0], 1, 0x02);
hid_set_field(report1->field[0], 2, 0x00);
hid_set_field(report1->field[0], 3, 0x00);
hid_set_field(report1->field[0], 4, 0xb8 | tile);
hid_set_field(report1->field[0], 5, 0x00);
hid_set_field(report1->field[0], 6, 0x00);
hid_set_field(report1->field[0], 7, 0x40);
hid_set_field(report1->field[0], 8, 0x00);
hid_set_field(report1->field[0], 9, 0x00);
hid_set_field(report1->field[0], 10, 32);
hid_set_field(report2->field[0], 0, (chip << 2) | 0x01);
hid_set_field(report2->field[0], 1, 0x00);
hid_set_field(report2->field[0], 2, 0x00);
hid_set_field(report2->field[0], 3, 32);
tdata = data->fb_vbitmap + (tile * 4 + chip) * 64;
for (i = 0; i < 64; i++)
if (i < 32)
hid_set_field(report1->field[0], 11 + i, tdata[i]);
else
hid_set_field(report2->field[0], 4 + i - 32, tdata[i]);
usbhid_submit_report(data->hdev, report1, USB_DIR_OUT);
usbhid_submit_report(data->hdev, report2, USB_DIR_OUT);
spin_unlock_irqrestore(&data->lock, flags);
return 0;
}
/* Translate a single tile*/
static int picolcd_fb_update_tile(u8 *vbitmap, const u8 *bitmap, int bpp,
int chip, int tile)
{
int i, b, changed = 0;
u8 tdata[64];
u8 *vdata = vbitmap + (tile * 4 + chip) * 64;
if (bpp = 1) {
for (b = 7; b >= 0; b--) {
const u8 *bdata = bitmap + tile * 256 + chip * 8 + b * 32;
for (i = 0; i < 64; i++) {
tdata[i] <<= 1;
tdata[i] |= (bdata[i/8] >> (i % 8)) & 0x01;
}
}
} else if (bpp = 8) {
for (b = 7; b >= 0; b--) {
const u8 *bdata = bitmap + (tile * 256 + chip * 8 + b * 32) * 8;
for (i = 0; i < 64; i++) {
tdata[i] <<= 1;
tdata[i] |= (bdata[i] & 0x80) ? 0x01 : 0x00;
}
}
} else {
/* Oops, we should never get here! */
WARN_ON(1);
return 0;
}
for (i = 0; i < 64; i++)
if (tdata[i] != vdata[i]) {
changed = 1;
vdata[i] = tdata[i];
}
return changed;
}
void picolcd_fb_refresh(struct picolcd_data *data)
{
if (data->fb_info)
schedule_delayed_work(&data->fb_info->deferred_work, 0);
}
/* Reconfigure LCD display */
int picolcd_fb_reset(struct picolcd_data *data, int clear)
{
struct hid_report *report = picolcd_out_report(REPORT_LCD_CMD, data->hdev);
int i, j;
unsigned long flags;
static const u8 mapcmd[8] = { 0x00, 0x02, 0x00, 0x64, 0x3f, 0x00, 0x64, 0xc0 };
if (!report || report->maxfield != 1)
return -ENODEV;
spin_lock_irqsave(&data->lock, flags);
for (i = 0; i < 4; i++) {
for (j = 0; j < report->field[0]->maxusage; j++)
if (j = 0)
hid_set_field(report->field[0], j, i << 2);
else if (j < sizeof(mapcmd))
hid_set_field(report->field[0], j, mapcmd[j]);
else
hid_set_field(report->field[0], j, 0);
usbhid_submit_report(data->hdev, report, USB_DIR_OUT);
}
data->status |= PICOLCD_READY_FB;
spin_unlock_irqrestore(&data->lock, flags);
if (data->fb_bitmap) {
if (clear) {
memset(data->fb_vbitmap, 0, PICOLCDFB_SIZE);
memset(data->fb_bitmap, 0, PICOLCDFB_SIZE*data->fb_bpp);
}
data->fb_force = 1;
}
/* schedule first output of framebuffer */
picolcd_fb_refresh(data);
return 0;
}
/* Update fb_vbitmap from the screen_base and send changed tiles to device */
static void picolcd_fb_update(struct fb_info *info)
{
int chip, tile, n;
unsigned long flags;
struct picolcd_data *data;
mutex_lock(&info->lock);
data = info->par;
if (!data)
goto out;
spin_lock_irqsave(&data->lock, flags);
if (!(data->status & PICOLCD_READY_FB)) {
spin_unlock_irqrestore(&data->lock, flags);
picolcd_fb_reset(data, 0);
} else {
spin_unlock_irqrestore(&data->lock, flags);
}
/*
* Translate the framebuffer into the format needed by the PicoLCD.
* See display layout above.
* Do this one tile after the other and push those tiles that changed.
*
* Wait for our IO to complete as otherwise we might flood the queue!
*/
n = 0;
for (chip = 0; chip < 4; chip++)
for (tile = 0; tile < 8; tile++)
if (picolcd_fb_update_tile(data->fb_vbitmap,
data->fb_bitmap, data->fb_bpp, chip, tile) ||
data->fb_force) {
n += 2;
if (n >= HID_OUTPUT_FIFO_SIZE / 2) {
mutex_unlock(&info->lock);
usbhid_wait_io(data->hdev);
mutex_lock(&info->lock);
data = info->par;
if (!data)
goto out;
spin_lock_irqsave(&data->lock, flags);
if (data->status & PICOLCD_FAILED) {
spin_unlock_irqrestore(&data->lock, flags);
goto out;
}
spin_unlock_irqrestore(&data->lock, flags);
n = 0;
}
picolcd_fb_send_tile(data->hdev, chip, tile);
}
data->fb_force = false;
if (n) {
mutex_unlock(&info->lock);
usbhid_wait_io(data->hdev);
return;
}
out:
mutex_unlock(&info->lock);
}
/* Stub to call the system default and update the image on the picoLCD */
static void picolcd_fb_fillrect(struct fb_info *info,
const struct fb_fillrect *rect)
{
if (!info->par)
return;
sys_fillrect(info, rect);
schedule_delayed_work(&info->deferred_work, 0);
}
/* Stub to call the system default and update the image on the picoLCD */
static void picolcd_fb_copyarea(struct fb_info *info,
const struct fb_copyarea *area)
{
if (!info->par)
return;
sys_copyarea(info, area);
schedule_delayed_work(&info->deferred_work, 0);
}
/* Stub to call the system default and update the image on the picoLCD */
static void picolcd_fb_imageblit(struct fb_info *info, const struct fb_image *image)
{
if (!info->par)
return;
sys_imageblit(info, image);
schedule_delayed_work(&info->deferred_work, 0);
}
/*
* this is the slow path from userspace. they can seek and write to
* the fb. it's inefficient to do anything less than a full screen draw
*/
static ssize_t picolcd_fb_write(struct fb_info *info, const char __user *buf,
size_t count, loff_t *ppos)
{
ssize_t ret;
if (!info->par)
return -ENODEV;
ret = fb_sys_write(info, buf, count, ppos);
if (ret >= 0)
schedule_delayed_work(&info->deferred_work, 0);
return ret;
}
static int picolcd_fb_blank(int blank, struct fb_info *info)
{
if (!info->par)
return -ENODEV;
/* We let fb notification do this for us via lcd/backlight device */
return 0;
}
static void picolcd_fb_destroy(struct fb_info *info)
{
/* make sure no work is deferred */
fb_deferred_io_cleanup(info);
vfree((u8 *)info->fix.smem_start);
framebuffer_release(info);
printk(KERN_DEBUG "picolcd_fb_destroy(%p)\n", info);
}
static int picolcd_fb_check_var(struct fb_var_screeninfo *var, struct fb_info *info)
{
__u32 bpp = var->bits_per_pixel;
__u32 activate = var->activate;
/* only allow 1/8 bit depth (8-bit is grayscale) */
*var = picolcdfb_var;
var->activate = activate;
if (bpp >= 8) {
var->bits_per_pixel = 8;
var->red.length = 8;
var->green.length = 8;
var->blue.length = 8;
} else {
var->bits_per_pixel = 1;
var->red.length = 1;
var->green.length = 1;
var->blue.length = 1;
}
return 0;
}
static int picolcd_set_par(struct fb_info *info)
{
struct picolcd_data *data = info->par;
u8 *tmp_fb, *o_fb;
if (!data)
return -ENODEV;
if (info->var.bits_per_pixel = data->fb_bpp)
return 0;
/* switch between 1/8 bit depths */
if (info->var.bits_per_pixel != 1 && info->var.bits_per_pixel != 8)
return -EINVAL;
o_fb = data->fb_bitmap;
tmp_fb = kmalloc(PICOLCDFB_SIZE*info->var.bits_per_pixel, GFP_KERNEL);
if (!tmp_fb)
return -ENOMEM;
/* translate FB content to new bits-per-pixel */
if (info->var.bits_per_pixel = 1) {
int i, b;
for (i = 0; i < PICOLCDFB_SIZE; i++) {
u8 p = 0;
for (b = 0; b < 8; b++) {
p <<= 1;
p |= o_fb[i*8+b] ? 0x01 : 0x00;
}
tmp_fb[i] = p;
}
memcpy(o_fb, tmp_fb, PICOLCDFB_SIZE);
info->fix.visual = FB_VISUAL_MONO01;
info->fix.line_length = PICOLCDFB_WIDTH / 8;
} else {
int i;
memcpy(tmp_fb, o_fb, PICOLCDFB_SIZE);
for (i = 0; i < PICOLCDFB_SIZE * 8; i++)
o_fb[i] = tmp_fb[i/8] & (0x01 << (7 - i % 8)) ? 0xff : 0x00;
info->fix.visual = FB_VISUAL_DIRECTCOLOR;
info->fix.line_length = PICOLCDFB_WIDTH;
}
kfree(tmp_fb);
data->fb_bpp = info->var.bits_per_pixel;
return 0;
}
/* Note this can't be const because of struct fb_info definition */
static struct fb_ops picolcdfb_ops = {
.owner = THIS_MODULE,
.fb_destroy = picolcd_fb_destroy,
.fb_read = fb_sys_read,
.fb_write = picolcd_fb_write,
.fb_blank = picolcd_fb_blank,
.fb_fillrect = picolcd_fb_fillrect,
.fb_copyarea = picolcd_fb_copyarea,
.fb_imageblit = picolcd_fb_imageblit,
.fb_check_var = picolcd_fb_check_var,
.fb_set_par = picolcd_set_par,
};
/* Callback from deferred IO workqueue */
static void picolcd_fb_deferred_io(struct fb_info *info, struct list_head *pagelist)
{
picolcd_fb_update(info);
}
static const struct fb_deferred_io picolcd_fb_defio = {
.delay = HZ / PICOLCDFB_UPDATE_RATE_DEFAULT,
.deferred_io = picolcd_fb_deferred_io,
};
/*
* The "fb_update_rate" sysfs attribute
*/
static ssize_t picolcd_fb_update_rate_show(struct device *dev,
struct device_attribute *attr, char *buf)
{
struct picolcd_data *data = dev_get_drvdata(dev);
unsigned i, fb_update_rate = data->fb_update_rate;
size_t ret = 0;
for (i = 1; i <= PICOLCDFB_UPDATE_RATE_LIMIT; i++)
if (ret >= PAGE_SIZE)
break;
else if (i = fb_update_rate)
ret += snprintf(buf+ret, PAGE_SIZE-ret, "[%u] ", i);
else
ret += snprintf(buf+ret, PAGE_SIZE-ret, "%u ", i);
if (ret > 0)
buf[min(ret, (size_t)PAGE_SIZE)-1] = '\n';
return ret;
}
static ssize_t picolcd_fb_update_rate_store(struct device *dev,
struct device_attribute *attr, const char *buf, size_t count)
{
struct picolcd_data *data = dev_get_drvdata(dev);
int i;
unsigned u;
if (count < 1 || count > 10)
return -EINVAL;
i = sscanf(buf, "%u", &u);
if (i != 1)
return -EINVAL;
if (u > PICOLCDFB_UPDATE_RATE_LIMIT)
return -ERANGE;
else if (u = 0)
u = PICOLCDFB_UPDATE_RATE_DEFAULT;
data->fb_update_rate = u;
data->fb_info->fbdefio->delay = HZ / data->fb_update_rate;
return count;
}
static DEVICE_ATTR(fb_update_rate, 0666, picolcd_fb_update_rate_show,
picolcd_fb_update_rate_store);
/* initialize Framebuffer device */
int picolcd_init_framebuffer(struct picolcd_data *data)
{
struct device *dev = &data->hdev->dev;
struct fb_info *info = NULL;
int i, error = -ENOMEM;
u8 *fb_vbitmap = NULL;
u8 *fb_bitmap = NULL;
u32 *palette;
fb_bitmap = vmalloc(PICOLCDFB_SIZE*8);
if (fb_bitmap = NULL) {
dev_err(dev, "can't get a free page for framebuffer\n");
goto err_nomem;
}
fb_vbitmap = kmalloc(PICOLCDFB_SIZE, GFP_KERNEL);
if (fb_vbitmap = NULL) {
dev_err(dev, "can't alloc vbitmap image buffer\n");
goto err_nomem;
}
data->fb_update_rate = PICOLCDFB_UPDATE_RATE_DEFAULT;
/* The extra memory is:
* - 256*u32 for pseudo_palette
* - struct fb_deferred_io
*/
info = framebuffer_alloc(256 * sizeof(u32) +
sizeof(struct fb_deferred_io), dev);
if (info = NULL) {
dev_err(dev, "failed to allocate a framebuffer\n");
goto err_nomem;
}
info->fbdefio = info->par;
*info->fbdefio = picolcd_fb_defio;
palette = info->par + sizeof(struct fb_deferred_io);
for (i = 0; i < 256; i++)
palette[i] = i > 0 && i < 16 ? 0xff : 0;
info->pseudo_palette = palette;
info->screen_base = (char __force __iomem *)fb_bitmap;
info->fbops = &picolcdfb_ops;
info->var = picolcdfb_var;
info->fix = picolcdfb_fix;
info->fix.smem_len = PICOLCDFB_SIZE*8;
info->fix.smem_start = (unsigned long)fb_bitmap;
info->par = data;
info->flags = FBINFO_FLAG_DEFAULT;
data->fb_vbitmap = fb_vbitmap;
data->fb_bitmap = fb_bitmap;
data->fb_bpp = picolcdfb_var.bits_per_pixel;
error = picolcd_fb_reset(data, 1);
if (error) {
dev_err(dev, "failed to configure display\n");
goto err_cleanup;
}
error = device_create_file(dev, &dev_attr_fb_update_rate);
if (error) {
dev_err(dev, "failed to create sysfs attributes\n");
goto err_cleanup;
}
fb_deferred_io_init(info);
data->fb_info = info;
error = register_framebuffer(info);
if (error) {
dev_err(dev, "failed to register framebuffer\n");
goto err_sysfs;
}
/* schedule first output of framebuffer */
data->fb_force = 1;
schedule_delayed_work(&info->deferred_work, 0);
return 0;
err_sysfs:
fb_deferred_io_cleanup(info);
device_remove_file(dev, &dev_attr_fb_update_rate);
err_cleanup:
data->fb_vbitmap = NULL;
data->fb_bitmap = NULL;
data->fb_bpp = 0;
data->fb_info = NULL;
err_nomem:
framebuffer_release(info);
vfree(fb_bitmap);
kfree(fb_vbitmap);
return error;
}
void picolcd_exit_framebuffer(struct picolcd_data *data)
{
struct fb_info *info = data->fb_info;
u8 *fb_vbitmap = data->fb_vbitmap;
if (!info)
return;
device_remove_file(&data->hdev->dev, &dev_attr_fb_update_rate);
info->par = NULL;
unregister_framebuffer(info);
data->fb_vbitmap = NULL;
data->fb_bitmap = NULL;
data->fb_bpp = 0;
data->fb_info = NULL;
kfree(fb_vbitmap);
}
Hello,
On Thu, Aug 09, 2012 at 08:09:47PM +0200, Bruno Prémont wrote:
As you are working on workqueues and related code, could you have a look
at my usage of them in combination with db_defio?
The delayed memory corruptions or system reboots after unbinding/unplugging
the PicoLCD seem very much related to workqueue used to handle the deferred
IO to framebuffer.
I think things don't get cleaned-up as they should though I'm not sure
where the trouble lies.
For ease of reading, I'm inlineing below the framebuffer related code of
PicoLCD (for complete driver after this patch series apply the whole series
on top of 3.5 https://lkml.org/lkml/2012/7/30/375 )
As you are working on workqueues and related code, could you have a look
at my usage of them in combination with db_defio?
The delayed memory corruptions or system reboots after unbinding/unplugging
the PicoLCD seem very much related to workqueue used to handle the deferred
IO to framebuffer.
I think things don't get cleaned-up as they should though I'm not sure
where the trouble lies.
For ease of reading, I'm inlineing below the framebuffer related code of
PicoLCD (for complete driver after this patch series apply the whole series
on top of 3.5 https://lkml.org/lkml/2012/7/30/375 )
I'm kinda shooting in the dark but who flushes / cancels
fb_info->deferred_work?
fb_deferred_io_cleanup() does so and is called by destroy fbops
(when last reference to fb_info is returned):
static void picolcd_fb_destroy(struct fb_info *info)
{
/* make sure no work is deferred */
fb_deferred_io_cleanup(info);
vfree((u8 *)info->fix.smem_start);
framebuffer_release(info);
printk(KERN_DEBUG "picolcd_fb_destroy(%p)\n", info);
}
=== drivers/video/fb_defio.c ==void fb_deferred_io_cleanup(struct fb_info *info)
{
struct fb_deferred_io *fbdefio = info->fbdefio;
struct page *page;
int i;
BUG_ON(!fbdefio);
cancel_delayed_work_sync(&info->deferred_work);
/* clear out the mapping that we setup */
for (i = 0 ; i < info->fix.smem_len; i += PAGE_SIZE) {
page = fb_deferred_io_page(info, i);
page->mapping = NULL;
}
info->fbops->fb_mmap = NULL;
mutex_destroy(&fbdefio->lock);
}
Thanks,
Bruno
Commit 4ea5454203d991ec85264f64f89ca8855fce69b0
[HID: Fix race condition between driver core and ll-driver] introduced
new locking around proce/remove functions that prevent any report/reply
from hardware to reach driver until it returned from probe.
As such, the ask-reply way to checking picoLCD firmware version during
probe is bound to timeout and let probe fail.
Disabling the check lets driver sucessfully probe again.
Signed-off-by: Bruno Prémont <bonbons@linux-vserver.org>
---
drivers/hid/hid-picolcd_core.c | 8 ++++----
1 files changed, 4 insertions(+), 4 deletions(-)
@@ -478,13 +478,13 @@ static int picolcd_probe_lcd(struct hid_device *hdev, struct picolcd_data *data){interror;-error=picolcd_check_version(hdev);+/* error = picolcd_check_version(hdev);if(error)returnerror;if(data->version[0]!=0&&data->version[1]!=3)hid_info(hdev,"Device with untested firmware revision, please submit /sys/kernel/debug/hid/%s/rdesc for this device.\n",-dev_name(&hdev->dev));+dev_name(&hdev->dev));*/
Please just remove it altogether, I don't see a reason to keep the
commented-out code in the in-tree driver.
Once the locking mess is sorted out, we can re-introduce it again as
necessary.
Thanks.
--
Jiri Kosina
SUSE Labs
Hi,
This series updates picoLCD driver:
- split the driver functions into separate files which get included
depending on Kconfig selection
(implementation for CIR using RC_CORE will follow later)
- drop private framebuffer refcounting in favor of refcounting added
to fb_info some time ago
- fix various bugs issues
- disabled firmware version checking in probe() as it does not work
anymore since commit 4ea5454203d991ec85264f64f89ca8855fce69b0
[HID: Fix race condition between driver core and ll-driver]
I have now applied the series to my 'picolcd' branch, except for 6/7,
please see the comment I sent to it separately.
Note: I still get weird behavior on quick unbind/bind sequences
issued via sysfs (CONFIG_SMP=n system) that are triggered by framebuffer
support and apparently more specifically fb_defio part of it.
Unfortunately I'm out of ideas as to how to track down the problem which
shows either as SLAB corruption (detected with SLUB debugging, e.g.
Would be nice to have this sorted out before the next merge window indeed,
so that it can go in together with the rest of the changes.
So I am wondering whether the path this happens on is
if (!test_bit(HID_OUT_RUNNING, &usbhid->iofl)) {
usbhid_restart_out_queue(usbhid);
in __usbhid_submit_report(). It would then indicate perhaps some race with
iofl handling.
Could you please stick some printk() just before and after this
usbhid_restart_out_queue() call, so that we know that it's this triggering
it?
--
Jiri Kosina
SUSE Labs
Hi,
This series updates picoLCD driver:
- split the driver functions into separate files which get included
depending on Kconfig selection
(implementation for CIR using RC_CORE will follow later)
- drop private framebuffer refcounting in favor of refcounting added
to fb_info some time ago
- fix various bugs issues
- disabled firmware version checking in probe() as it does not work
anymore since commit 4ea5454203d991ec85264f64f89ca8855fce69b0
[HID: Fix race condition between driver core and ll-driver]
I have now applied the series to my 'picolcd' branch, except for 6/7,
please see the comment I sent to it separately.
Will respin that one soon
quoted
Note: I still get weird behavior on quick unbind/bind sequences
issued via sysfs (CONFIG_SMP=n system) that are triggered by framebuffer
support and apparently more specifically fb_defio part of it.
Unfortunately I'm out of ideas as to how to track down the problem which
shows either as SLAB corruption (detected with SLUB debugging, e.g.
Would be nice to have this sorted out before the next merge window indeed,
so that it can go in together with the rest of the changes.
So I am wondering whether the path this happens on is
if (!test_bit(HID_OUT_RUNNING, &usbhid->iofl)) {
usbhid_restart_out_queue(usbhid);
in __usbhid_submit_report(). It would then indicate perhaps some race with
iofl handling.
Huh, that specific test_bit hunk I can't find in __usbhid_submit_report,
is that 3.6 material?
I'm running my tests against 3.5...
The nearest I have is:
if (!test_bit(HID_OUT_RUNNING, &usbhid->iofl))
if (!irq_out_pump_restart(hid))
set_bit(HID_OUT_RUNNING, &usbhid->iofl);
Could you please stick some printk() just before and after this
usbhid_restart_out_queue() call, so that we know that it's this triggering
it?
So I am wondering whether the path this happens on is
if (!test_bit(HID_OUT_RUNNING, &usbhid->iofl)) {
usbhid_restart_out_queue(usbhid);
in __usbhid_submit_report(). It would then indicate perhaps some race with
iofl handling.
Huh, that specific test_bit hunk I can't find in __usbhid_submit_report,
is that 3.6 material?
I'm running my tests against 3.5...
I see. Alan Stern has fixed a huge pile of things in this area in 3.6-rc1.
I have expected all of those to actually be on theoretical problems not
ever having happened in the wild, but it might be that you are actually
chasing on of those.
Could you please retest with latest Linus' tree (or at least eb055fd0560b)
to see whether this hasn't actually been fixed already by Alan's series?
Thanks,
--
Jiri Kosina
SUSE Labs
From: Bruno Prémont <bonbons@linux-vserver.org> Date: 2012-08-15 15:17:20
Hi Jiri,
On Wed, 15 August 2012 Jiri Kosina [off-list ref] wrote:
I see. Alan Stern has fixed a huge pile of things in this area in 3.6-rc1.
I have expected all of those to actually be on theoretical problems not
ever having happened in the wild, but it might be that you are actually
chasing on of those.
Could you please retest with latest Linus' tree (or at least eb055fd0560b)
to see whether this hasn't actually been fixed already by Alan's series?
I've started trying that out, it seems Alan's work improved things.
For the first few attempts I have not seen SLAB corruptions, though after
a few rounds I hit accumulation of the following messages:
[ 297.174828] hid-picolcd 0003:04D8:C002.0003: output queue full
[ 297.181098] hid-picolcd 0003:04D8:C002.0003: output queue full
[ 297.187820] hid-picolcd 0003:04D8:C002.0003: output queue full
[ 297.194087] hid-picolcd 0003:04D8:C002.0003: output queue full
with sporadically in between:
[ 292.668019] hid-picolcd 0003:04D8:C002.0003: usb_submit_urb(out) failed: -1
At first glance I think the queue filling up and never draining is caused
by hid_hw_stop() stalling the queue and the time between both being just too
short.
Maybe me (un)binding on hid-picolcd driver level is the major cause of this
and (un)binding at lower usb level would kill the queue thus preventing
it just growing until full...
Is there a proper way to do rate-limiting or throttling when submitting
reports (which are the urbs deeper in the stack) and catch -ENODEV early
while remove() is delayed for locking reasons?
At least until 3.5 submitting reports provides no return value for driver
which could convey errors like -ENODEV, -EBUSY.
After a few bind-unbind iterations I have also hit a race in hid-picolcd
with regard to fbdefio.
That looks like chicken-egg release sequencing between hid-picolcd and
fb_info subdev -- though I'm wondering how that happens as the only framebuffer
user I know of - fbcon - should have stopped using it after
unregister_framebuffer() returned and released the last reference!
But that part can be improved rather easily with a new spinlock synchronizing
interoperation between hid side and framebuffer side.
Thanks,
Bruno
I see. Alan Stern has fixed a huge pile of things in this area in 3.6-rc1.
I have expected all of those to actually be on theoretical problems not
ever having happened in the wild, but it might be that you are actually
chasing on of those.
Could you please retest with latest Linus' tree (or at least eb055fd0560b)
to see whether this hasn't actually been fixed already by Alan's series?
I've started trying that out, it seems Alan's work improved things.
For the first few attempts I have not seen SLAB corruptions, though after
a few rounds I hit accumulation of the following messages:
[ 297.174828] hid-picolcd 0003:04D8:C002.0003: output queue full
[ 297.181098] hid-picolcd 0003:04D8:C002.0003: output queue full
[ 297.187820] hid-picolcd 0003:04D8:C002.0003: output queue full
[ 297.194087] hid-picolcd 0003:04D8:C002.0003: output queue full
with sporadically in between:
[ 292.668019] hid-picolcd 0003:04D8:C002.0003: usb_submit_urb(out) failed: -1
At first glance I think the queue filling up and never draining is caused
by hid_hw_stop() stalling the queue and the time between both being just too
short.
I don't really understand this explanation. Once usb_kill_urb() returns,
the URB should be available for future use (and therefore all queues
completely drained).
--
Jiri Kosina
SUSE Labs
I see. Alan Stern has fixed a huge pile of things in this area in 3.6-rc1.
I have expected all of those to actually be on theoretical problems not
ever having happened in the wild, but it might be that you are actually
chasing on of those.
Could you please retest with latest Linus' tree (or at least eb055fd0560b)
to see whether this hasn't actually been fixed already by Alan's series?
I've started trying that out, it seems Alan's work improved things.
For the first few attempts I have not seen SLAB corruptions, though after
a few rounds I hit accumulation of the following messages:
[ 297.174828] hid-picolcd 0003:04D8:C002.0003: output queue full
[ 297.181098] hid-picolcd 0003:04D8:C002.0003: output queue full
[ 297.187820] hid-picolcd 0003:04D8:C002.0003: output queue full
[ 297.194087] hid-picolcd 0003:04D8:C002.0003: output queue full
with sporadically in between:
[ 292.668019] hid-picolcd 0003:04D8:C002.0003: usb_submit_urb(out) failed: -1
At first glance I think the queue filling up and never draining is caused
by hid_hw_stop() stalling the queue and the time between both being just too
short.
I don't really understand this explanation. Once usb_kill_urb() returns,
the URB should be available for future use (and therefore all queues
completely drained).
I won't have time today to check, though my guess is that on each
echo $usb-id > bind; echo $usb-id > unbind
under /sys/bus/hid/drivers/hid-picolcd/ the USB urb queue fills a bit does
not get cleared.
Is usb_kill_urb() called when unbinding just the specific hid driver? If so
my short timing between bind/unbind must be triggering something else...
Otherwise I'm missing something as at first time I got no "output queue full"
messages, but as I repeated the bind/unbind sequences the prints per bind/unbind
iteration increased in number.
Anyhow, on Friday evening/week-end I will continue digging and report back with my
findings.
Thanks,
Bruno
I don't really understand this explanation. Once usb_kill_urb() returns,
the URB should be available for future use (and therefore all queues
completely drained).
I won't have time today to check, though my guess is that on each
echo $usb-id > bind; echo $usb-id > unbind
under /sys/bus/hid/drivers/hid-picolcd/ the USB urb queue fills a bit does
not get cleared.
Is usb_kill_urb() called when unbinding just the specific hid driver?
Yup, through hid_hw_stop() -> usbhid_stop().
If so my short timing between bind/unbind must be triggering something
else...
Otherwise I'm missing something as at first time I got no "output queue full"
messages, but as I repeated the bind/unbind sequences the prints per bind/unbind
iteration increased in number.
Anyhow, on Friday evening/week-end I will continue digging and report back with my
findings.
From: Bruno Prémont <bonbons@linux-vserver.org> Date: 2012-08-18 12:41:33
Hi Jiri,
[CCing Alan Stern]
On Thu, 16 August 2012 Jiri Kosina [off-list ref] wrote:
On Thu, 16 Aug 2012, Bruno Prémont wrote:
quoted
quoted
I don't really understand this explanation. Once usb_kill_urb() returns,
the URB should be available for future use (and therefore all queues
completely drained).
I won't have time today to check, though my guess is that on each
echo $usb-id > bind; echo $usb-id > unbind
under /sys/bus/hid/drivers/hid-picolcd/ the USB urb queue fills a bit does
not get cleared.
Is usb_kill_urb() called when unbinding just the specific hid driver?
Yup, through hid_hw_stop() -> usbhid_stop().
quoted
If so my short timing between bind/unbind must be triggering something
else...
Otherwise I'm missing something as at first time I got no "output queue full"
messages, but as I repeated the bind/unbind sequences the prints per bind/unbind
iteration increased in number.
Anyhow, on Friday evening/week-end I will continue digging and report back with my
findings.
Huh, after changing some of the hid-picolcd data in order to have less racy
coupling between hid and framebuffer I'm now dying way too often in _mmx_memcpy
and most of the time I don't get a (complete) trace...
The only full trace I got was:
[ 3857.426136] BUG: unable to handle kernel paging request at dbdf9000
[ 3857.432555] IP: [<c11e2be7>] _mmx_memcpy+0x27/0x16c
[ 3857.435906] *pde = 1bebb063 *pte = 1bdf9161
[ 3857.435906] Oops: 0003 [#1]
[ 3857.435906] Modules linked in: hid_picolcd fb_sys_fops sysimgblt sysfillrect syscopyarea drm_kms_helper nfs3 nfs_acl nfs lockd sunrpc
[ 3857.435906] Pid: 1935, comm: bash Not tainted 3.6.0-rc1-jupiter-00363-g0e8ccbc #1 NVIDIA Corporation. nFORCE-MCP/MS-6373
[ 3857.435906] EIP: 0060:[<c11e2be7>] EFLAGS: 00010013 CPU: 0
[ 3857.435906] EIP is at _mmx_memcpy+0x27/0x16c
[ 3857.435906] EAX: dd40a000 EBX: dd6ea010 ECX: 035af77b EDX: dbdf4230
[ 3857.435906] ESI: dbe0d1b0 EDI: dbdf9000 EBP: dd40bea8 ESP: dd40be8c
[ 3857.435906] DS: 007b ES: 007b FS: 0000 GS: 00e0 SS: 0068
[ 3857.435906] CR0: 8005003b CR2: dbdf9000 CR3: 1d7ee000 CR4: 000007d0
[ 3857.435906] DR0: 00000000 DR1: 00000000 DR2: 00000000 DR3: 00000000
[ 3857.435906] DR6: ffff0ff0 DR7: 00000400
[ 3857.435906] Process bash (pid: 1935, ti=dd40a000 task=dcdb6120 task.ti=dbce2000)
[ 3857.435906] Stack:
[ 3857.435906] 0d6d6d6f dd6eb49c dd40beb8 dbde0080 dd6ea010 dbde0080 dbd31010 dd40bed0
[ 3857.435906] c133b364 00000000 00000000 ddfaa0e0 dbd31010 dbdf4230 dd6ea010 00000046
[ 3857.435906] dbd31010 dd40bef0 c133bea0 00000000 dbd0c3f0 00000000 dbd0c3f0 00000000
[ 3857.435906] Call Trace:
[ 3857.435906] [<c133b364>] hid_submit_out+0xa4/0x130
[ 3857.435906] [<c133bea0>] hid_irq_out+0xa0/0x100
[ 3857.435906] [<c12e425e>] usb_hcd_giveback_urb+0x4e/0x90
[ 3857.435906] [<c12f9bd2>] finish_urb+0xb2/0xf0
[ 3857.435906] [<c12fa67d>] takeback_td+0x3d/0x100
[ 3857.435906] [<c12faccf>] dl_done_list+0x14f/0x180
[ 3857.435906] [<c12fc351>] ohci_irq+0x191/0x300
[ 3857.435906] [<c1077790>] ? unmask_irq+0x20/0x20
[ 3857.435906] [<c12e3ace>] usb_hcd_irq+0x1e/0x40
[ 3857.435906] [<c107549d>] handle_irq_event_percpu+0x6d/0x1c0
[ 3857.435906] [<c1077790>] ? unmask_irq+0x20/0x20
[ 3857.435906] [<c1075618>] handle_irq_event+0x28/0x40
[ 3857.435906] [<c107781a>] handle_fasteoi_irq+0x8a/0xe0
[ 3857.435906] <IRQ>
[ 3857.435906] [<c1003e0a>] ? do_IRQ+0x3a/0xb0
[ 3857.435906] [<c140c870>] ? common_interrupt+0x30/0x38
[ 3857.435906] [<c1035143>] ? __do_softirq+0x53/0x1c0
[ 3857.435906] [<c10350f0>] ? local_bh_enable_ip+0x80/0x80
[ 3857.435906] <IRQ>
[ 3857.435906] [<c103544e>] ? irq_exit+0x3e/0xa0
[ 3857.435906] [<c101dfde>] ? smp_apic_timer_interrupt+0x6e/0x80
[ 3857.435906] [<c140bee1>] ? apic_timer_interrupt+0x31/0x38
[ 3857.435906] [<c102fdba>] ? vprintk_emit+0x34a/0x390
[ 3857.435906] [<c1409f58>] ? printk+0x38/0x3a
[ 3857.435906] [<de94b021>] ? picolcd_init_framebuffer+0x261/0x350 [hid_picolcd]
[ 3857.435906] [<de949bd1>] ? picolcd_probe+0x3d1/0x5a0 [hid_picolcd]
[ 3857.435906] [<c13321f7>] ? hid_device_probe+0x67/0xf0
[ 3857.435906] [<c128ac17>] ? driver_sysfs_add+0x57/0x80
[ 3857.435906] [<c128af1d>] ? driver_probe_device+0xbd/0x1d0
[ 3857.435906] [<c1331aab>] ? hid_match_device+0x7b/0x90
[ 3857.435906] [<c1289e65>] ? driver_bind+0x75/0xd0
[ 3857.435906] [<c1289df0>] ? driver_unbind+0x90/0x90
[ 3857.435906] [<c1289537>] ? drv_attr_store+0x27/0x30
[ 3857.435906] [<c111a59c>] ? sysfs_write_file+0xac/0xf0
[ 3857.435906] [<c10cca0c>] ? vfs_write+0x9c/0x130
[ 3857.435906] [<c10da2ef>] ? sys_dup3+0x11f/0x160
[ 3857.435906] [<c111a4f0>] ? sysfs_poll+0x90/0x90
[ 3857.435906] [<c10ccc7d>] ? sys_write+0x3d/0x70
[ 3857.435906] [<c140c357>] ? sysenter_do_call+0x12/0x26
[ 3857.435906] Code: 90 90 90 90 55 89 e5 57 56 89 d6 53 83 ec 10 89 45 f0 89 4d e4 89 e0 25 00 e0 ff ff f7 40 14 00 ff ff 07 74 17 c1 e9 02 8b 7d f0 <f3> a5 8b 4d e4 83 e1 03 74 02 f3 a4 e9 27 01 00 00 8b 45 1
[ 3857.435906] EIP: [<c11e2be7>] _mmx_memcpy+0x27/0x16c SS:ESP 0068:dd40be8c
[ 3857.435906] CR2: 00000000dbdf9000
[ 3857.435906] ---[ end trace 10ae520e65e3b763 ]---
[ 3857.435906] Kernel panic - not syncing: Fatal exception in interrupt
It happens around the time of calling
device_create_file(dev, &dev_attr_fb_update_rate);
that is, after picolcd_fb_reset(data, 1) has returned but before
calling fb_deferred_io_init(info); according to printk() I put
around the various calls in picolcd_init_framebuffer().
The other case I got part of a trace it was again in _mmx_memcpy()
but the trace output stopped in the middle of register dumping.
The full trace happened after the following sequence:
- system boot
- hotplug of picolcd (triggering modprobe hid-picolcd)
- cd /sys/bus/hid/drivers/hid-picolcd/
- echo 0003:04D8:C002.0003 > unbind
- wait some time
- echo 0003:04D8:C002.0003 > bind
Any idea what it could be? It might be during processing of first
reports send in order to clear the picolcd display.
As (un)binding in hid-picolcd crashes I tried doing so a level above,
at usbhid driver on usb bus which resulted in the following:
- system boot
- hotplug of picolcd (triggering modprobe hid-picolcd)
- cd /sys/bus/usb/drivers/usbhid/
- echo 2-1:1.0 > unbind
- wait some time
- echo 2-1:1.0 > bind
- wait some time
- echo 2-1:1.0 > unbind
produced the following trace:
[ 1418.387466] BUG: unable to handle kernel NULL pointer dereference at 00000030
[ 1418.390013] IP: [<c1339c40>] hid_submit_ctrl+0x80/0x2a0
[ 1418.390013] *pde = 00000000
[ 1418.390013] Oops: 0002 [#1]
[ 1418.390013] Modules linked in: hid_picolcd fb_sys_fops sysimgblt sysfillrect syscopyarea drm_kms_helper nfs3 nfs_acl nfs lockd sunrpc
[ 1418.390013] Pid: 1994, comm: kworker/0:0 Not tainted 3.6.0-rc1-jupiter-00363-g0e8ccbc #1 NVIDIA Corporation. nFORCE-MCP/MS-6373
[ 1418.390013] EIP: 0060:[<c1339c40>] EFLAGS: 00010086 CPU: 0
[ 1418.390013] EIP is at hid_submit_ctrl+0x80/0x2a0
[ 1418.390013] EAX: 80000200 EBX: dc2d0000 ECX: dcfe2020 EDX: 00000000
[ 1418.390013] ESI: 00000040 EDI: dc1f8f00 EBP: dd743e2c ESP: dd743df4
[ 1418.390013] DS: 007b ES: 007b FS: 0000 GS: 00e0 SS: 0068
[ 1418.390013] CR0: 8005003b CR2: 00000030 CR3: 1cd42000 CR4: 000007d0
[ 1418.390013] DR0: 00000000 DR1: 00000000 DR2: 00000000 DR3: 00000000
[ 1418.390013] DR6: ffff0ff0 DR7: 00000400
[ 1418.390013] Process kworker/0:0 (pid: 1994, ti=dd742000 task=dcdd0d40 task.ti=dd742000)
[ 1418.390013] Stack:
[ 1418.390013] 0000003e dc2f9020 dcf64c7c 00000005 00000002 dd743e1c 00000002 dc1f8f00
[ 1418.390013] 00000040 dcfe2020 dc2cb4d0 dc2d0000 00000000 dcfe2020 dd743e44 c1339ef6
[ 1418.390013] 00000008 dc2d0000 dc2cb4d0 dcfe2020 dd743e74 c133bbeb 00000023 000000ff
[ 1418.390013] Call Trace:
[ 1418.390013] [<c1339ef6>] usbhid_restart_ctrl_queue+0x96/0xe0
[ 1418.390013] [<c133bbeb>] __usbhid_submit_report+0x29b/0x320
[ 1418.390013] [<c1330200>] ? hid_parser_local+0x260/0x2d0
[ 1418.390013] [<c133bd2b>] usbhid_submit_report+0x1b/0x30
[ 1418.390013] [<de996cb9>] picolcd_fb_deferred_io+0x3b9/0x4c0 [hid_picolcd]
[ 1418.390013] [<c120e118>] fb_deferred_io_work+0x78/0xc0
[ 1418.390013] [<c1045e3f>] process_one_work+0x1ff/0x380
[ 1418.390013] [<c120e0a0>] ? fb_deferred_io_fault+0xa0/0xa0
[ 1418.390013] [<c10467f7>] worker_thread+0x237/0x360
[ 1418.390013] [<c10465c0>] ? manage_workers.clone.25+0x250/0x250
[ 1418.390013] [<c10465c0>] ? manage_workers.clone.25+0x250/0x250
[ 1418.390013] [<c104a30d>] kthread+0x6d/0x80
[ 1418.390013] [<c104a2a0>] ? kthread_freezable_should_stop+0x70/0x70
[ 1418.390013] [<c140c87e>] kernel_thread_helper+0x6/0xd
[ 1418.390013] Code: 01 89 45 e8 0f 85 91 00 00 00 8b 4d ec 8b 53 1c 8b 7d e4 8b 75 e8 8b 81 68 0c 00 00 8b 00 8b 40 9c c1 e0 08 0d 00 00 00 80 85 ff <89> 42 30 8b 43 1c 89 70 54 0f 84 19 01 00 00 81 fe ff 01 0
[ 1418.390013] EIP: [<c1339c40>] hid_submit_ctrl+0x80/0x2a0 SS:ESP 0068:dd743df4
[ 1418.390013] CR2: 0000000000000030
[ 1418.390013] ---[ end trace 089e20ed16261a0a ]---
pretty early during unbind process, e.g. way before picolcd even started to
dispose framebuffer.
Either I'm doing something really wrong or I'm the first one to produce
lots of outbound reports thus exploring corner-cases...
PS: attached is a diff between the hid-picolcd from this patch series to what
I'm testing right now.
Thanks,
Bruno
From: Alan Stern <stern@rowland.harvard.edu> Date: 2012-08-18 13:19:15
On Sat, 18 Aug 2012, Bruno Prémont wrote:
Hi Jiri,
[CCing Alan Stern]
On Thu, 16 August 2012 Jiri Kosina [off-list ref] wrote:
quoted
On Thu, 16 Aug 2012, Bruno Prémont wrote:
quoted
quoted
I don't really understand this explanation. Once usb_kill_urb() returns,
the URB should be available for future use (and therefore all queues
completely drained).
I won't have time today to check, though my guess is that on each
echo $usb-id > bind; echo $usb-id > unbind
under /sys/bus/hid/drivers/hid-picolcd/ the USB urb queue fills a bit does
not get cleared.
Is usb_kill_urb() called when unbinding just the specific hid driver?
Yup, through hid_hw_stop() -> usbhid_stop().
quoted
If so my short timing between bind/unbind must be triggering something
else...
Otherwise I'm missing something as at first time I got no "output queue full"
messages, but as I repeated the bind/unbind sequences the prints per bind/unbind
iteration increased in number.
Anyhow, on Friday evening/week-end I will continue digging and report back with my
findings.
Huh, after changing some of the hid-picolcd data in order to have less racy
coupling between hid and framebuffer I'm now dying way too often in _mmx_memcpy
and most of the time I don't get a (complete) trace...
There was a similar problem reported recently. It turned out to be
caused by a __devinitconst annotation attached to a usb_device_id
table.
If there are any __devinit* annotations in the hid-picolcd driver, you
should see if removing them helps.
Alan Stern
From: Bruno Prémont <bonbons@linux-vserver.org> Date: 2012-08-18 13:49:18
On Sat, 18 August 2012 Alan Stern [off-list ref] wrote:
On Sat, 18 Aug 2012, Bruno Prémont wrote:
quoted
On Thu, 16 August 2012 Jiri Kosina [off-list ref] wrote:
quoted
On Thu, 16 Aug 2012, Bruno Prémont wrote:
quoted
quoted
I don't really understand this explanation. Once usb_kill_urb() returns,
the URB should be available for future use (and therefore all queues
completely drained).
I won't have time today to check, though my guess is that on each
echo $usb-id > bind; echo $usb-id > unbind
under /sys/bus/hid/drivers/hid-picolcd/ the USB urb queue fills a bit does
not get cleared.
Is usb_kill_urb() called when unbinding just the specific hid driver?
Yup, through hid_hw_stop() -> usbhid_stop().
quoted
If so my short timing between bind/unbind must be triggering something
else...
Otherwise I'm missing something as at first time I got no "output queue full"
messages, but as I repeated the bind/unbind sequences the prints per bind/unbind
iteration increased in number.
Anyhow, on Friday evening/week-end I will continue digging and report back with my
findings.
Huh, after changing some of the hid-picolcd data in order to have less racy
coupling between hid and framebuffer I'm now dying way too often in _mmx_memcpy
and most of the time I don't get a (complete) trace...
There was a similar problem reported recently. It turned out to be
caused by a __devinitconst annotation attached to a usb_device_id
table.
If there are any __devinit* annotations in the hid-picolcd driver, you
should see if removing them helps.
There is no such annotation around in hid-picolcd.
One thing I just though about, how does usbhid handle the calls to
usbhid_submit_report() when hid_hw_stop()/hid_hw_close() have already
been called?
I will attempt to see if it makes a difference to shortcut my
usbhid_submit_report() calls from the point on I have called hid_hw_close()...
Bruno
One thing I just though about, how does usbhid handle the calls to
usbhid_submit_report() when hid_hw_stop()/hid_hw_close() have already
been called?
I will attempt to see if it makes a difference to shortcut my
usbhid_submit_report() calls from the point on I have called hid_hw_close()...
I don't know if I'm progressing or just drawing circles...
I included a check right before calling usbhid_submit_report() to prevent
calling it if hid_hw_close()/hid_hw_stop() has been called and that part
works as expected, just that at some moment it explodes on a NULL-pointer
dereference.
Initially I had usbhid_submit_report macro:
#define usbhid_submit_report(a, b, c) \
do { \
picolcd_debug_out_report(d, a, b); \
usbhid_submit_report(a, b, c); \
} while (0)
Now to make sure no new reports get submitted once hid_hw_stop()
was called I changed it to:
#define usbhid_submit_report(a, b, c) \
do { \
struct picolcd_data *d = hid_get_drvdata(a); \
unsigned long flags; \
picolcd_debug_out_report(d, a, b); \
spin_lock_irqsave(&d->lock, flags); \
if (!(d->status & PICOLCD_FAILED)) \
usbhid_submit_report(a, b, c); \
else \
printk(KERN_INFO "Submitting report after hw_stop!\n"); \
spin_unlock_irqrestore(&d->lock, flags); \
} while (0)
with the printk() to spam dmesg :)
The NULL-pointer dereference I got inside picolcd_fb_deferred_io() was due
to me calling hid_set_drvdata(hdev, NULL) too early in remove(), fixed.
But with that fixed I still encounter the bad page, probably around
the time of hid_hw_stop(), though it always takes the HARD LOCK watchdog
to trigger for the trace to show (probably because or IRQ context):
It seems to happen during the bind/unbind following the unbind that
produces a
hid-picolcd 0003:04D8:C002.0003: usb_submit_urb(out) failed: -1
either a BUG or even as reboot and happend between hid_hw_close() and
hid_hw_stop() though my extra printks seem to reduce probability of
bad things to happen.
[ 1217.643017] BUG: unable to handle kernel paging request at c7cb0000
[ 1217.643017] IP: [<c11e2be7>] _mmx_memcpy+0x27/0x16c
[ 1217.643017] *pde = 19078063 *pte = 07cb0161
[ 1217.643017] Oops: 0003 [#1]
[ 1217.643017] Modules linked in: hid_picolcd fb_sys_fops sysimgblt sysfillrect syscopyarea drm_kms_helper nfs3 nfs_acl nfs lockd sunrpc
[ 1217.643017] Pid: 0, comm: swapper Not tainted 3.6.0-rc2-jupiter-00008-gd3c2b0c-dirty #2 NVIDIA Corporation. nFORCE-MCP/MS-6373
[ 1217.643017] EIP: 0060:[<c11e2be7>] EFLAGS: 00010013 CPU: 0
[ 1217.643017] EIP is at _mmx_memcpy+0x27/0x16c
[ 1217.643017] EAX: dd40a000 EBX: dd6ea010 ECX: 035af77b EDX: d9108c80
[ 1217.643017] ESI: d9121c00 EDI: c7cb0000 EBP: dd40bebc ESP: dd40bea0
[ 1217.643017] DS: 007b ES: 007b FS: 0000 GS: 00e0 SS: 0068
[ 1217.643017] CR0: 8005003b CR2: c7cb0000 CR3: 1cf1c000 CR4: 000007d0
[ 1217.643017] DR0: 00000000 DR1: 00000000 DR2: 00000000 DR3: 00000000
[ 1217.643017] DR6: ffff0ff0 DR7: 00000400
[ 1217.643017] Process swapper (pid: 0, tiÝ40a000 taskÁ5d24c0 task.tiÁ5c6000)
[ 1217.643017] Stack:
[ 1217.643017] 0d6d6d6f dd40beac c109bba4 c7c97080 dd6ea010 c7c97080 dceb3030 dd40bee4
[ 1217.643017] c133b364 c7d40000 c7d40000 dd40bed4 dceb3030 d9108c80 dd6ea010 00000046
[ 1217.643017] dceb3030 dd40bf04 c133bea0 0000d6d6 dbe901b0 fffffffe dbe901b0 fffffffe
[ 1217.643017] Call Trace:
[ 1217.643017] [<c109bba4>] ? free_compound_page+0x14/0x20
[ 1217.643017] [<c133b364>] hid_submit_out+0xa4/0x130
[ 1217.643017] [<c133bea0>] hid_irq_out+0xa0/0x100
[ 1217.643017] [<c12e425e>] usb_hcd_giveback_urb+0x4e/0x90
[ 1217.643017] [<c12f9bd2>] finish_urb+0xb2/0xf0
[ 1217.643017] [<c12fa8a8>] finish_unlinks+0x168/0x320
[ 1217.643017] [<c12fc43f>] ohci_irq+0x27f/0x300
[ 1217.643017] [<c1077790>] ? unmask_irq+0x20/0x20
[ 1217.643017] [<c12e3ace>] usb_hcd_irq+0x1e/0x40
[ 1217.643017] [<c107549d>] handle_irq_event_percpu+0x6d/0x1c0
[ 1217.643017] [<c1077790>] ? unmask_irq+0x20/0x20
[ 1217.643017] [<c1075618>] handle_irq_event+0x28/0x40
[ 1217.643017] [<c107781a>] handle_fasteoi_irq+0x8a/0xe0
[ 1217.643017] <IRQ>
[ 1217.643017] [<c1003e0a>] ? do_IRQ+0x3a/0xb0
[ 1217.643017] [<c140c870>] ? common_interrupt+0x30/0x38
[ 1217.643017] [<c1009028>] ? default_idle+0x68/0xd0
[ 1217.643017] [<c1009a0f>] ? cpu_idle+0x8f/0xc0
[ 1217.643017] [<c13ffe97>] ? rest_init+0x67/0x70
[ 1217.643017] [<c161b8b8>] ? start_kernel+0x2a3/0x2aa
[ 1217.643017] [<c161b3da>] ? repair_env_string+0x51/0x51
[ 1217.643017] [<c161b28e>] ? i386_start_kernel+0x44/0x46
[ 1217.643017] Code: 90 90 90 90 55 89 e5 57 56 89 d6 53 83 ec 10 89 45 f0 89 4d e4 89 e0 25 00 e0 ff ff f7 40 14 00 ff ff 07 74 17 c1 e9 02 8b 7d f0 <f3> a5 8b 4d e4 83 e1 03 74 02 f3 a4 e9 27 01 00 00 8b 45 1
[ 1217.643017] EIP: [<c11e2be7>] _mmx_memcpy+0x27/0x16c SS:ESP 0068:dd40bea0
[ 1217.643017] CR2: 00000000c7cb0000
[ 1217.643017] ---[ end trace fbaa4c5261e1a846 ]---
Bruno
One thing I just though about, how does usbhid handle the calls to
usbhid_submit_report() when hid_hw_stop()/hid_hw_close() have already
been called?
Looks like they aren't synchronized at all. That's a bug.
usbhid_submit_report() should check the HID_DISCONNECTED flag.
quoted
I will attempt to see if it makes a difference to shortcut my
usbhid_submit_report() calls from the point on I have called hid_hw_close()...
I don't know bout hid_hw_close(). Certainly no more reports should be
submitted following usbhid_stop().
Ok, I did just that, prevent new calls to usbhid_submit_report(), after
calling hid_hw_close(), fixed one bug in my code that triggers the NULL
pointer dereference (calling hid_set_drvdata(hdev, NULL) too early).
Now I'm still seeing the bad paging request in _mmx_memcpy(), though rather
sporadically.
The last ones I saw were during remove() around the time of calling hid_hw_close()
and hid_hw_stop(). Adding a printk() between the two (at least while picoLCD
is hosting fbcon) makes it very improbably for the bad page to happen.
It looks like low-level driver did free memory in hid_hw_close() for some
in-flight URB and thus things break in following USB interrupt.
From mapping trace information to source it seems:
usbhid/hid-core.c:
static int hid_submit_out(struct hid_device *hid)
{
struct hid_report *report;
char *raw_report;
struct usbhid_device *usbhid = hid->driver_data;
int r;
report = usbhid->out[usbhid->outtail].report;
raw_report = usbhid->out[usbhid->outtail].raw_report;
usbhid->urbout->transfer_buffer_length = ((report->size - 1) >> 3) +
1 + (report->id > 0);
usbhid->urbout->dev = hid_to_usb_dev(hid);
if (raw_report) {
memcpy(usbhid->outbuf, raw_report,
usbhid->urbout->transfer_buffer_length);
^^^^^^^^^^^^^^^_ this is exploding
kfree(raw_report);
usbhid->out[usbhid->outtail].raw_report = NULL;
}
dbg_hid("submitting out urb\n");
r = usb_submit_urb(usbhid->urbout, GFP_ATOMIC);
if (r < 0) {
hid_err(hid, "usb_submit_urb(out) failed: %d\n", r);
return r;
}
usbhid->last_out = jiffies;
return 0;
}
Bruno
@@ -478,14 +478,6 @@ static int picolcd_probe_lcd(struct hid_device *hdev, struct picolcd_data *data){interror;-error=picolcd_check_version(hdev);-if(error)-returnerror;--if(data->version[0]!=0&&data->version[1]!=3)-hid_info(hdev,"Device with untested firmware revision, please submit /sys/kernel/debug/hid/%s/rdesc for this device.\n",-dev_name(&hdev->dev));-/* Setup keypad input device */error=picolcd_init_keys(data,picolcd_in_report(REPORT_KEY_STATE,hdev));if(error)
@@ -534,16 +526,6 @@ err:staticintpicolcd_probe_bootloader(structhid_device*hdev,structpicolcd_data*data){-interror;--error=picolcd_check_version(hdev);-if(error)-returnerror;--if(data->version[0]!=1&&data->version[1]!=0)-hid_info(hdev,"Device with untested bootloader revision, please submit /sys/kernel/debug/hid/%s/rdesc for this device.\n",-dev_name(&hdev->dev));-picolcd_init_devfs(data,NULL,NULL,picolcd_out_report(REPORT_BL_READ_MEMORY,hdev),picolcd_out_report(REPORT_BL_WRITE_MEMORY,hdev),NULL);
From: Bruno Prémont <bonbons@linux-vserver.org> Date: 2012-08-19 17:29:49
Hi,
This series fixes some race conditions in picoLCD driver during remove()
and adds support for IR functionality.
Repeatedly binding/unbinding device at hid-picolcd driver level or at
usbhid level now works properly (except in rare occasions which trigger
a paging error in interrupt context (memcopy) and seem to come from
usbhid internals when usbhid gets interrupt between hid_hw_close() and
hid_hw_stop()).
CIR support is added using RC_CORE for the decoding and handling of
IR-event stream and works for a Sony and a JVC remote.
Bruno
@@ -90,11 +90,6 @@ struct picolcd_data {#ifdef CONFIG_HID_PICOLCD_FB/* Framebuffer stuff */-u8fb_update_rate;-u8fb_bpp;-u8fb_force;-u8*fb_vbitmap;/* local copy of what was sent to PicoLCD */-u8*fb_bitmap;/* framebuffer */structfb_info*fb_info;#endif /* CONFIG_HID_PICOLCD_FB */#ifdef CONFIG_HID_PICOLCD_LCD
@@ -119,9 +114,21 @@ struct picolcd_data {intstatus;#define PICOLCD_BOOTLOADER 1#define PICOLCD_FAILED 2-#define PICOLCD_READY_FB 4};+#ifdef CONFIG_HID_PICOLCD_FB+structpicolcd_fb_data{+/* Framebuffer stuff */+spinlock_tlock;+structpicolcd_data*picolcd;+u8update_rate;+u8bpp;+u8force;+u8ready;+u8*vbitmap;/* local copy of what was sent to PicoLCD */+u8*bitmap;/* framebuffer */+};+#endif /* CONFIG_HID_PICOLCD_FB *//* Find a given report */#define picolcd_in_report(id, dev) picolcd_report(id, dev, HID_INPUT_REPORT)
@@ -98,19 +98,26 @@ static const struct fb_var_screeninfo picolcdfb_var = {};/* Send a given tile to PicoLCD */-staticintpicolcd_fb_send_tile(structhid_device*hdev,intchip,inttile)+staticintpicolcd_fb_send_tile(structpicolcd_data*data,u8*vbitmap,+intchip,inttile){-structpicolcd_data*data=hid_get_drvdata(hdev);-structhid_report*report1=picolcd_out_report(REPORT_LCD_CMD_DATA,hdev);-structhid_report*report2=picolcd_out_report(REPORT_LCD_DATA,hdev);+structhid_report*report1,*report2;unsignedlongflags;u8*tdata;inti;-if(!report1||report1->maxfield!=1||!report2||report2->maxfield!=1)+report1=picolcd_out_report(REPORT_LCD_CMD_DATA,data->hdev);+if(!report1||report1->maxfield!=1)+return-ENODEV;+report2=picolcd_out_report(REPORT_LCD_DATA,data->hdev);+if(!report2||report2->maxfield!=1)return-ENODEV;spin_lock_irqsave(&data->lock,flags);+if((data->status&PICOLCD_FAILED)){+spin_unlock_irqrestore(&data->lock,flags);+return-ENODEV;+}hid_set_field(report1->field[0],0,chip<<2);hid_set_field(report1->field[0],1,0x02);hid_set_field(report1->field[0],2,0x00);
@@ -128,7 +135,7 @@ static int picolcd_fb_send_tile(struct hid_device *hdev, int chip, int tile)hid_set_field(report2->field[0],2,0x00);hid_set_field(report2->field[0],3,32);-tdata=data->fb_vbitmap+(tile*4+chip)*64;+tdata=vbitmap+(tile*4+chip)*64;for(i=0;i<64;i++)if(i<32)hid_set_field(report1->field[0],11+i,tdata[i]);
@@ -207,20 +215,19 @@ int picolcd_fb_reset(struct picolcd_data *data, int clear)hid_set_field(report->field[0],j,0);usbhid_submit_report(data->hdev,report,USB_DIR_OUT);}--data->status|=PICOLCD_READY_FB;spin_unlock_irqrestore(&data->lock,flags);-if(data->fb_bitmap){-if(clear){-memset(data->fb_vbitmap,0,PICOLCDFB_SIZE);-memset(data->fb_bitmap,0,PICOLCDFB_SIZE*data->fb_bpp);-}-data->fb_force=1;+if(clear){+memset(fbdata->vbitmap,0,PICOLCDFB_SIZE);+memset(fbdata->bitmap,0,PICOLCDFB_SIZE*fbdata->bpp);}+fbdata->force=1;/* schedule first output of framebuffer */-picolcd_fb_refresh(data);+if(fbdata->ready)+schedule_delayed_work(&data->fb_info->deferred_work,0);+else+fbdata->ready=1;return0;}
@@ -336,20 +344,22 @@ static ssize_t picolcd_fb_write(struct fb_info *info, const char __user *buf,staticintpicolcd_fb_blank(intblank,structfb_info*info){-if(!info->par)-return-ENODEV;/* We let fb notification do this for us via lcd/backlight device */return0;}staticvoidpicolcd_fb_destroy(structfb_info*info){+structpicolcd_fb_data*fbdata=info->par;+/* make sure no work is deferred */fb_deferred_io_cleanup(info);+/* No thridparty should ever unregister our framebuffer! */+WARN_ON(fbdata->picolcd!=NULL);+vfree((u8*)info->fix.smem_start);framebuffer_release(info);-printk(KERN_DEBUG"picolcd_fb_destroy(%p)\n",info);}staticintpicolcd_fb_check_var(structfb_var_screeninfo*var,structfb_info*info)
@@ -376,17 +386,15 @@ static int picolcd_fb_check_var(struct fb_var_screeninfo *var, struct fb_info *istaticintpicolcd_set_par(structfb_info*info){-structpicolcd_data*data=info->par;+structpicolcd_fb_data*fbdata=info->par;u8*tmp_fb,*o_fb;-if(!data)-return-ENODEV;-if(info->var.bits_per_pixel=data->fb_bpp)+if(info->var.bits_per_pixel=fbdata->bpp)return0;/* switch between 1/8 bit depths */if(info->var.bits_per_pixel!=1&&info->var.bits_per_pixel!=8)return-EINVAL;-o_fb=data->fb_bitmap;+o_fb=fbdata->bitmap;tmp_fb=kmalloc(PICOLCDFB_SIZE*info->var.bits_per_pixel,GFP_KERNEL);if(!tmp_fb)return-ENOMEM;
@@ -415,7 +423,7 @@ static int picolcd_set_par(struct fb_info *info)}kfree(tmp_fb);-data->fb_bpp=info->var.bits_per_pixel;+fbdata->bpp=info->var.bits_per_pixel;return0;}
@@ -500,30 +510,18 @@ int picolcd_init_framebuffer(struct picolcd_data *data){structdevice*dev=&data->hdev->dev;structfb_info*info=NULL;+structpicolcd_fb_data*fbdata=NULL;inti,error=-ENOMEM;-u8*fb_vbitmap=NULL;-u8*fb_bitmap=NULL;u32*palette;-fb_bitmap=vmalloc(PICOLCDFB_SIZE*8);-if(fb_bitmap=NULL){-dev_err(dev,"can't get a free page for framebuffer\n");-gotoerr_nomem;-}--fb_vbitmap=kmalloc(PICOLCDFB_SIZE,GFP_KERNEL);-if(fb_vbitmap=NULL){-dev_err(dev,"can't alloc vbitmap image buffer\n");-gotoerr_nomem;-}--data->fb_update_rate=PICOLCDFB_UPDATE_RATE_DEFAULT;/* The extra memory is:*-256*u32forpseudo_palette*-structfb_deferred_io*/info=framebuffer_alloc(256*sizeof(u32)+-sizeof(structfb_deferred_io),dev);+sizeof(structfb_deferred_io)++sizeof(structpicolcd_fb_data)++PICOLCDFB_SIZE,dev);if(info=NULL){dev_err(dev,"failed to allocate a framebuffer\n");gotoerr_nomem;
@@ -531,74 +529,86 @@ int picolcd_init_framebuffer(struct picolcd_data *data)info->fbdefio=info->par;*info->fbdefio=picolcd_fb_defio;-palette=info->par+sizeof(structfb_deferred_io);+info->par+=sizeof(structfb_deferred_io);+palette=info->par;+info->par+=256*sizeof(u32);for(i=0;i<256;i++)palette[i]=i>0&&i<16?0xff:0;info->pseudo_palette=palette;-info->screen_base=(char__force__iomem*)fb_bitmap;info->fbops=&picolcdfb_ops;info->var=picolcdfb_var;info->fix=picolcdfb_fix;info->fix.smem_len=PICOLCDFB_SIZE*8;-info->fix.smem_start=(unsignedlong)fb_bitmap;-info->par=data;info->flags=FBINFO_FLAG_DEFAULT;-data->fb_vbitmap=fb_vbitmap;-data->fb_bitmap=fb_bitmap;-data->fb_bpp=picolcdfb_var.bits_per_pixel;+fbdata=info->par;+spin_lock_init(&fbdata->lock);+fbdata->picolcd=data;+fbdata->update_rate=PICOLCDFB_UPDATE_RATE_DEFAULT;+fbdata->bpp=picolcdfb_var.bits_per_pixel;+fbdata->force=1;+fbdata->vbitmap=info->par+sizeof(structpicolcd_fb_data);+fbdata->bitmap=vmalloc(PICOLCDFB_SIZE*8);+if(fbdata->bitmap=NULL){+dev_err(dev,"can't get a free page for framebuffer\n");+gotoerr_nomem;+}+info->screen_base=(char__force__iomem*)fbdata->bitmap;+info->fix.smem_start=(unsignedlong)fbdata->bitmap;+memset(fbdata->vbitmap,0xff,PICOLCDFB_SIZE);+data->fb_info=info;+error=picolcd_fb_reset(data,1);if(error){dev_err(dev,"failed to configure display\n");gotoerr_cleanup;}+error=device_create_file(dev,&dev_attr_fb_update_rate);if(error){dev_err(dev,"failed to create sysfs attributes\n");gotoerr_cleanup;}+fb_deferred_io_init(info);-data->fb_info=info;error=register_framebuffer(info);if(error){dev_err(dev,"failed to register framebuffer\n");gotoerr_sysfs;}-/* schedule first output of framebuffer */-data->fb_force=1;-schedule_delayed_work(&info->deferred_work,0);return0;err_sysfs:-fb_deferred_io_cleanup(info);device_remove_file(dev,&dev_attr_fb_update_rate);+fb_deferred_io_cleanup(info);err_cleanup:-data->fb_vbitmap=NULL;-data->fb_bitmap=NULL;-data->fb_bpp=0;data->fb_info=NULL;err_nomem:+if(fbdata)+vfree(fbdata->bitmap);framebuffer_release(info);-vfree(fb_bitmap);-kfree(fb_vbitmap);returnerror;}voidpicolcd_exit_framebuffer(structpicolcd_data*data){structfb_info*info=data->fb_info;-u8*fb_vbitmap=data->fb_vbitmap;--if(!info)-return;+structpicolcd_fb_data*fbdata=info->par;+unsignedlongflags;device_remove_file(&data->hdev->dev,&dev_attr_fb_update_rate);-info->par=NULL;++/* disconnect framebuffer from HID dev */+spin_lock_irqsave(&fbdata->lock,flags);+fbdata->picolcd=NULL;+spin_unlock_irqrestore(&fbdata->lock,flags);++/* make sure there is no running update - thus that fbdata->picolcd+*onceobtainedunderlockisguaranteednottogetfree()under+*thefeetofthedeferredwork*/+flush_delayed_work_sync(&info->deferred_work);++data->fb_info=NULL;unregister_framebuffer(info);-data->fb_vbitmap=NULL;-data->fb_bitmap=NULL;-data->fb_bpp=0;-data->fb_info=NULL;-kfree(fb_vbitmap);}
I don't know bout hid_hw_close(). Certainly no more reports should be
submitted following usbhid_stop().
Ok, I did just that, prevent new calls to usbhid_submit_report(), after
calling hid_hw_close(), fixed one bug in my code that triggers the NULL
pointer dereference (calling hid_set_drvdata(hdev, NULL) too early).
Now I'm still seeing the bad paging request in _mmx_memcpy(), though rather
sporadically.
The last ones I saw were during remove() around the time of calling hid_hw_close()
and hid_hw_stop(). Adding a printk() between the two (at least while picoLCD
is hosting fbcon) makes it very improbably for the bad page to happen.
It looks like low-level driver did free memory in hid_hw_close() for some
in-flight URB and thus things break in following USB interrupt.
No, memory is not freed in usbhid_close(). It is freed in
usbhid_stop().
From mapping trace information to source it seems:
usbhid/hid-core.c:
static int hid_submit_out(struct hid_device *hid)
{
struct hid_report *report;
char *raw_report;
struct usbhid_device *usbhid = hid->driver_data;
int r;
report = usbhid->out[usbhid->outtail].report;
raw_report = usbhid->out[usbhid->outtail].raw_report;
usbhid->urbout->transfer_buffer_length = ((report->size - 1) >> 3) +
1 + (report->id > 0);
usbhid->urbout->dev = hid_to_usb_dev(hid);
if (raw_report) {
memcpy(usbhid->outbuf, raw_report,
usbhid->urbout->transfer_buffer_length);
^^^^^^^^^^^^^^^_ this is exploding
Right. Like I said, usbhid->outbuf is freed in hid_free_buffers(),
which is called from usbhid_stop().
Forget about usbhid_close(). The race is between hid_submit_out() and
usbhid_stop().
Alan Stern
From: Bruno Prémont <bonbons@linux-vserver.org> Date: 2012-09-17 18:22:07
Jiri,
You seem not to have applied this one yet (it doesn't show up in picolcd
branch).
Could you please apply it?
(or did I miss locking changes making it obsolete?)
Thanks,
Bruno
On Sun, 19 August 2012 Bruno Prémont [off-list ref] wrote:
Commit 4ea5454203d991ec85264f64f89ca8855fce69b0
[HID: Fix race condition between driver core and ll-driver] introduced
new locking around probe/remove functions that prevents any report/reply
from hardware to reach driver until it returned from probe.
As such, the ask-reply way to checking picoLCD firmware version during
probe is bound to timeout and let probe fail.
Drop the check to let driver successfully probe again (until locking issues
are resolved allowing to reinstate the check).
Signed-off-by: Bruno Prémont <bonbons@linux-vserver.org>
---
Changes since v1:
- drop version check during probe instead of commenting it out.
drivers/hid/hid-picolcd_core.c | 18 ------------------
1 files changed, 0 insertions(+), 18 deletions(-)
You seem not to have applied this one yet (it doesn't show up in picolcd
branch).
Could you please apply it?
(or did I miss locking changes making it obsolete?)
No, sorry, I lost that one. Applied now.
Have you and David made any progress with the locking?
--
Jiri Kosina
SUSE Labs