From mboxrd@z Thu Jan 1 00:00:00 1970 From: Heiner Kallweit Subject: Re: [PATCH v2] hid: hid-led: fix issue with transfer buffer not being dma capable Date: Fri, 7 Oct 2016 20:40:39 +0200 Message-ID: References: <237d1c6f-933a-e218-bbda-04920bbf3a2a@gmail.com> <20161007162203.GG30411@mail.corp.redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Return-path: Received: from mail-wm0-f67.google.com ([74.125.82.67]:33885 "EHLO mail-wm0-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932239AbcJGSkz (ORCPT ); Fri, 7 Oct 2016 14:40:55 -0400 In-Reply-To: <20161007162203.GG30411@mail.corp.redhat.com> Sender: linux-input-owner@vger.kernel.org List-Id: linux-input@vger.kernel.org To: Benjamin Tissoires Cc: Jiri Kosina , linux-input@vger.kernel.org, Linux USB Mailing List , Alan Stern Am 07.10.2016 um 18:22 schrieb Benjamin Tissoires: > On Oct 03 2016 or thereabouts, Heiner Kallweit wrote: >> The hid-led driver works fine under 4.8.0, however with the next >> kernel from today I get this: >> >> ------------[ cut here ]------------ >> WARNING: CPU: 0 PID: 2578 at drivers/usb/core/hcd.c:1584 usb_hcd_map_urb_for_dma+0x373/0x550 [usbcore] >> transfer buffer not dma capable >> Modules linked in: hid_led(+) usbhid vfat fat ir_sony_decoder iwlmvm led_class mac80211 snd_hda_codec_realtek snd_hda_codec_generic x86_pkg_temp_thermal iwlwifi crc32c_intel snd_hda_codec_hdmi i2c_i801 i2c_smbus snd_hda_intel cfg80211 snd_hda_codec snd_hda_core snd_pcm r8169 snd_timer mei_me mii snd mei ir_lirc_codec lirc_dev nuvoton_cir rc_core btusb btintel bluetooth rfkill usb_storage efivarfs ipv6 ehci_pci ehci_hcd xhci_pci xhci_hcd usbcore usb_common ext4 jbd2 mbcache ahci libahci libata >> CPU: 0 PID: 2578 Comm: systemd-udevd Not tainted 4.8.0-rc8-next-20161003 #1 >> Hardware name: ZOTAC ZBOX-CI321NANO/ZBOX-CI321NANO, BIOS B246P105 06/01/2015 >> ffffc90003dbb7e0 ffffffff81280425 ffffc90003dbb830 0000000000000000 >> ffffc90003dbb820 ffffffff8105b086 0000063003dbb800 ffff88006f374480 >> 0000000000000000 0000000000000000 0000000000000001 ffff880079544000 >> Call Trace: >> [] dump_stack+0x68/0x93 >> [] __warn+0xc6/0xe0 >> [] warn_slowpath_fmt+0x4a/0x50 >> [] usb_hcd_map_urb_for_dma+0x373/0x550 [usbcore] >> [] usb_hcd_submit_urb+0x316/0x9c0 [usbcore] >> [] ? rcu_read_lock_sched_held+0x40/0x80 >> [] ? module_assert_mutex_or_preempt+0x13/0x50 >> [] ? __module_address+0x27/0xf0 >> [] usb_submit_urb+0x2c4/0x520 [usbcore] >> [] usb_start_wait_urb+0x5a/0xe0 [usbcore] >> [] usb_control_msg+0xbc/0xf0 [usbcore] >> [] ? __module_address+0x27/0xf0 >> [] usbhid_raw_request+0xa4/0x180 [usbhid] >> [] hidled_recv+0x71/0xe0 [hid_led] >> [] thingm_init+0x2d/0x50 [hid_led] >> [] hidled_probe+0xcb/0x24a [hid_led] >> [] hid_device_probe+0xd2/0x150 >> [] driver_probe_device+0x1fd/0x2c0 >> [] __driver_attach+0x9a/0xa0 >> [] ? driver_probe_device+0x2c0/0x2c0 >> [] bus_for_each_dev+0x5d/0x90 >> [] driver_attach+0x19/0x20 >> [] bus_add_driver+0x11f/0x220 >> [] ? 0xffffffffa07ac000 >> [] driver_register+0x5b/0xd0 >> [] ? 0xffffffffa07ac000 >> [] __hid_register_driver+0x61/0xa0 >> [] hidled_driver_init+0x1e/0x20 [hid_led] >> [] do_one_initcall+0x38/0x150 >> [] ? rcu_read_lock_sched_held+0x40/0x80 >> [] ? kmem_cache_alloc_trace+0x1d0/0x230 >> [] do_init_module+0x5a/0x1cb >> [] load_module+0x1e42/0x2530 >> [] ? __symbol_put+0x50/0x50 >> [] ? show_coresize+0x30/0x30 >> [] ? kernel_read_file+0x100/0x190 >> [] ? kernel_read_file_from_fd+0x44/0x70 >> [] SYSC_finit_module+0xba/0xc0 >> [] SyS_finit_module+0x9/0x10 >> [] entry_SYSCALL_64_fastpath+0x18/0xad >> ---[ end trace c9e6ea27003ecf9e ]--- >> >> Fix this by using a kmalloc'ed buffer when calling hid_hw_raw_request. >> >> Signed-off-by: Heiner Kallweit >> --- >> v2: >> - Based on a comment from Alan Stern allocate the buffer to be provided to >> hid_hw_raw_request separately (and not as part of struct hidled_device). >> Alternative would have been to allocate the buffer dynamically in each >> function calling hidled_send/_recv. However this would mean more overhead >> IMHO, and we'd need an error path in callers to free the buffer in case >> of an error. >> In addition we have better control that a proper buffer is used in case >> the driver is extended by somebody for supporting another LED device. >> --- > > Looks like the receive function is only called from .probe(), so this > should be safe. > However, for the send function, is there a chance there can be a > concurrent access of the buffer? (like 2 userspace processes writing a > different values at the same time). > Buffer ldev->buf is accessed with mutex ldev->lock held in hidled_send. So the locking exists already. > If so, then you'll need to add a locking mechanism (can't recall if the > LED API provide one for us or not), or just alloc/free the buffers > directly. > > If no, the patch is: > Reviewed-by: Benjamin Tissoires > > Cheers, > Benjamin > >> drivers/hid/hid-led.c | 23 +++++++++++++++++++---- >> 1 file changed, 19 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/hid/hid-led.c b/drivers/hid/hid-led.c >> index d8d55f3..555c88e 100644 >> --- a/drivers/hid/hid-led.c >> +++ b/drivers/hid/hid-led.c >> @@ -100,6 +100,7 @@ struct hidled_device { >> const struct hidled_config *config; >> struct hid_device *hdev; >> struct hidled_rgb *rgb; >> + u8 *buf; >> struct mutex lock; >> }; >> >> @@ -118,13 +119,19 @@ static int hidled_send(struct hidled_device *ldev, __u8 *buf) >> >> mutex_lock(&ldev->lock); >> >> + /* >> + * buffer provided to hid_hw_raw_request must not be on the stack >> + * and must not be part of a data structure >> + */ >> + memcpy(ldev->buf, buf, ldev->config->report_size); >> + >> if (ldev->config->report_type == RAW_REQUEST) >> - ret = hid_hw_raw_request(ldev->hdev, buf[0], buf, >> + ret = hid_hw_raw_request(ldev->hdev, buf[0], ldev->buf, >> ldev->config->report_size, >> HID_FEATURE_REPORT, >> HID_REQ_SET_REPORT); >> else if (ldev->config->report_type == OUTPUT_REPORT) >> - ret = hid_hw_output_report(ldev->hdev, buf, >> + ret = hid_hw_output_report(ldev->hdev, ldev->buf, >> ldev->config->report_size); >> else >> ret = -EINVAL; >> @@ -147,17 +154,21 @@ static int hidled_recv(struct hidled_device *ldev, __u8 *buf) >> >> mutex_lock(&ldev->lock); >> >> - ret = hid_hw_raw_request(ldev->hdev, buf[0], buf, >> + memcpy(ldev->buf, buf, ldev->config->report_size); >> + >> + ret = hid_hw_raw_request(ldev->hdev, buf[0], ldev->buf, >> ldev->config->report_size, >> HID_FEATURE_REPORT, >> HID_REQ_SET_REPORT); >> if (ret < 0) >> goto err; >> >> - ret = hid_hw_raw_request(ldev->hdev, buf[0], buf, >> + ret = hid_hw_raw_request(ldev->hdev, buf[0], ldev->buf, >> ldev->config->report_size, >> HID_FEATURE_REPORT, >> HID_REQ_GET_REPORT); >> + >> + memcpy(buf, ldev->buf, ldev->config->report_size); >> err: >> mutex_unlock(&ldev->lock); >> >> @@ -447,6 +458,10 @@ static int hidled_probe(struct hid_device *hdev, const struct hid_device_id *id) >> if (!ldev) >> return -ENOMEM; >> >> + ldev->buf = devm_kmalloc(&hdev->dev, MAX_REPORT_SIZE, GFP_KERNEL); >> + if (!ldev->buf) >> + return -ENOMEM; >> + >> ret = hid_parse(hdev); >> if (ret) >> return ret; >> -- >> 2.10.0 >> >> >