Messages in this thread | | | Date | Sun, 22 Dec 2019 19:37:39 +0100 | From | Bruno Prémont <> | Subject | Re: [PATCH] hid: hid-picolcd: fix possible sleep-in-atomic-context bug |
| |
Hi Jia-Ju,
I've had a deeper look at the code (possibly also applies to hid-lg4ff).
The hdev->ll_driver->request (at least on USB bus which is the only one that matters for hid-picolcd) points to: usbhid_request() from drivers/hid/usbhid/hid-core.c
This one directly calls usbhid_submit_report() which then directly calls __usbhid_submit_report() under spinlock.
Thus for USB bus there is no possible sleep left.
Just moving the hid_hw_request() calls out of the spinlock is incorrect though as it would introduce the possibility of unexpected concurrent initialization/submissions of reports from the distinct sub-drivers. The report pointer used is not call-private but comes from feature description and is filled with data on each call within the spinlock.
The question could be whether the generic fallback in hid_hw_request() should be adjusted to be non-sleeping. It has been introduced rather more recently than both drivers you detected.
Best regards, Bruno Prémont
On Wed, 18 Dec 2019 20:11:47 Jia-Ju Bai wrote: > Thanks for the reply. > > On 2019/12/18 16:41, Bruno Prémont wrote: > > Hi Jia-Ju, > > > > Your checker has been looking at fallback implementation for > > the might-sleep hid_alloc_report_buf(GFP_KERNEL). > > > > Did you have a look at the low-lever bus-driver implementations: > > hdev->ll_driver->request > > ^^^^^^^ > > > > Are those all sleeping as well or maybe they don't sleep?\ > > In fact, I find that a function possibly-related to this function > pointer can sleep: > > drivers/hid/intel-ish-hid/ishtp-hid.c, 97: > kzalloc(GFP_KERNEL) in ishtp_hid_request > > But I am not quite sure whether this function is really referenced by > the function pointer, so I did not report it. > > > Best wishes, > Jia-Ju Bai
| |