Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1526555 > unrolled thread

[PATCH 0/4] HID: fix few non-DMA capable HID transfers

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/4] HID: fix few non-DMA capable HID transfers Benjamin Tissoires <benjamin.tissoires@redhat.com> - 2016-11-21 11:50 +0100
    [PATCH 3/4] HID: magicmouse: make transfer buffers DMA capable Benjamin Tissoires <benjamin.tissoires@redhat.com> - 2016-11-21 11:50 +0100
    [PATCH 1/4] HID: cp2112: make transfer buffers DMA capable Benjamin Tissoires <benjamin.tissoires@redhat.com> - 2016-11-21 11:50 +0100
    [PATCH 2/4] HID: lg: make transfer buffers DMA capable Benjamin Tissoires <benjamin.tissoires@redhat.com> - 2016-11-21 11:50 +0100
      [PATCH] HID: lg: fix noderef.cocci warnings kbuild test robot <lkp@intel.com> - 2016-11-21 13:10 +0100
        Re: [PATCH] HID: lg: fix noderef.cocci warnings Benjamin Tissoires <benjamin.tissoires@redhat.com> - 2016-11-21 15:20 +0100
          Re: [PATCH] HID: lg: fix noderef.cocci warnings Jiri Kosina <jikos@kernel.org> - 2016-11-22 11:50 +0100
            Re: [PATCH] HID: lg: fix noderef.cocci warnings Fengguang Wu <lkp@intel.com> - 2016-11-23 07:00 +0100
              Re: [PATCH] HID: lg: fix noderef.cocci warnings Julia Lawall <julia.lawall@lip6.fr> - 2016-11-23 08:10 +0100
      Re: [PATCH 2/4] HID: lg: make transfer buffers DMA capable kbuild test robot <lkp@intel.com> - 2016-11-21 13:10 +0100
    [PATCH 4/4] HID: rmi: make transfer buffers DMA capable Benjamin Tissoires <benjamin.tissoires@redhat.com> - 2016-11-21 11:50 +0100
    Re: [PATCH 0/4] HID: fix few non-DMA capable HID transfers Jiri Kosina <jikos@kernel.org> - 2016-11-23 17:50 +0100

#1526555 — [PATCH 0/4] HID: fix few non-DMA capable HID transfers

Hi Jiri,

While playing a little bit with the CP2112 (for hid-rmi I must confess), I
realized that kernel v4.9 now enforces DMA capable buffers when calling
hid_hw_raw_request(). Kernel v4.8 works fine (but gives a stacktrace the first
time), but v4.9 doesn't.

So I gave a check of all the other drivers in the HID tree, and it looks like
only 4 drivers are not properly allocating their buffers.

I'd say this is v4.9 material but I was not able to test magicmouse and lg.
If this doesn't qualifies for 4.9-rc7, I think we should add the stable@ stamp,
given that the chance this will break hid-rmi is huge (cp2112 is not so much an
issue, given it's a devel board).

Cheers,
Benjamin



Benjamin Tissoires (4):
  HID: cp2112: make transfer buffers DMA capable
  HID: lg: make transfer buffers DMA capable
  HID: magicmouse: make transfer buffers DMA capable
  HID: rmi: make transfer buffers DMA capable

 drivers/hid/hid-cp2112.c     | 115 +++++++++++++++++++++++++++++--------------
 drivers/hid/hid-lg.c         |  12 +++--
 drivers/hid/hid-magicmouse.c |  12 ++++-
 drivers/hid/hid-rmi.c        |  10 +++-
 4 files changed, 106 insertions(+), 43 deletions(-)

-- 
2.7.4

[toc] | [next] | [standalone]

#1526557 — [PATCH 3/4] HID: magicmouse: make transfer buffers DMA capable

Kernel v4.9 strictly enforces DMA capable buffers, so we need to remove
buffers allocated on the stack.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/hid/hid-magicmouse.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/hid/hid-magicmouse.c b/drivers/hid/hid-magicmouse.c
index d6fa496..20b40ad 100644
--- a/drivers/hid/hid-magicmouse.c
+++ b/drivers/hid/hid-magicmouse.c
@@ -493,7 +493,8 @@ static int magicmouse_input_configured(struct hid_device *hdev,
 static int magicmouse_probe(struct hid_device *hdev,
 	const struct hid_device_id *id)
 {
-	__u8 feature[] = { 0xd7, 0x01 };
+	const u8 feature[] = { 0xd7, 0x01 };
+	u8 *buf;
 	struct magicmouse_sc *msc;
 	struct hid_report *report;
 	int ret;
@@ -544,6 +545,12 @@ static int magicmouse_probe(struct hid_device *hdev,
 	}
 	report->size = 6;
 
+	buf = kmemdup(feature, sizeof(feature), GFP_KERNEL);
+	if (!buf) {
+		ret = -ENOMEM;
+		goto err_stop_hw;
+	}
+
 	/*
 	 * Some devices repond with 'invalid report id' when feature
 	 * report switching it into multitouch mode is sent to it.
@@ -552,8 +559,9 @@ static int magicmouse_probe(struct hid_device *hdev,
 	 * but there seems to be no other way of switching the mode.
 	 * Thus the super-ugly hacky success check below.
 	 */
-	ret = hid_hw_raw_request(hdev, feature[0], feature, sizeof(feature),
+	ret = hid_hw_raw_request(hdev, buf[0], buf, sizeof(feature),
 				HID_FEATURE_REPORT, HID_REQ_SET_REPORT);
+	kfree(buf);
 	if (ret != -EIO && ret != sizeof(feature)) {
 		hid_err(hdev, "unable to request touch data (%d)\n", ret);
 		goto err_stop_hw;
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1526558 — [PATCH 1/4] HID: cp2112: make transfer buffers DMA capable

Kernel v4.9 strictly enforces DMA capable buffers, so we need to remove
buffers allocated on the stack.

Use a spinlock to prevent concurrent accesses to the buffer.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/hid/hid-cp2112.c | 115 ++++++++++++++++++++++++++++++++---------------
 1 file changed, 79 insertions(+), 36 deletions(-)

diff --git a/drivers/hid/hid-cp2112.c b/drivers/hid/hid-cp2112.c
index 086d8a5..60d3020 100644
--- a/drivers/hid/hid-cp2112.c
+++ b/drivers/hid/hid-cp2112.c
@@ -32,6 +32,11 @@
 #include <linux/usb/ch9.h>
 #include "hid-ids.h"
 
+#define CP2112_REPORT_MAX_LENGTH		64
+#define CP2112_GPIO_CONFIG_LENGTH		5
+#define CP2112_GPIO_GET_LENGTH			2
+#define CP2112_GPIO_SET_LENGTH			3
+
 enum {
 	CP2112_GPIO_CONFIG		= 0x02,
 	CP2112_GPIO_GET			= 0x03,
@@ -161,6 +166,8 @@ struct cp2112_device {
 	atomic_t read_avail;
 	atomic_t xfer_avail;
 	struct gpio_chip gc;
+	u8 *in_out_buffer;
+	spinlock_t lock;
 };
 
 static int gpio_push_pull = 0xFF;
@@ -171,62 +178,86 @@ static int cp2112_gpio_direction_input(struct gpio_chip *chip, unsigned offset)
 {
 	struct cp2112_device *dev = gpiochip_get_data(chip);
 	struct hid_device *hdev = dev->hdev;
-	u8 buf[5];
+	u8 *buf = dev->in_out_buffer;
+	unsigned long flags;
 	int ret;
 
+	spin_lock_irqsave(&dev->lock, flags);
+
 	ret = hid_hw_raw_request(hdev, CP2112_GPIO_CONFIG, buf,
-				       sizeof(buf), HID_FEATURE_REPORT,
-				       HID_REQ_GET_REPORT);
-	if (ret != sizeof(buf)) {
+				 CP2112_GPIO_CONFIG_LENGTH, HID_FEATURE_REPORT,
+				 HID_REQ_GET_REPORT);
+	if (ret != CP2112_GPIO_CONFIG_LENGTH) {
 		hid_err(hdev, "error requesting GPIO config: %d\n", ret);
-		return ret;
+		goto exit;
 	}
 
 	buf[1] &= ~(1 << offset);
 	buf[2] = gpio_push_pull;
 
-	ret = hid_hw_raw_request(hdev, CP2112_GPIO_CONFIG, buf, sizeof(buf),
-				 HID_FEATURE_REPORT, HID_REQ_SET_REPORT);
+	ret = hid_hw_raw_request(hdev, CP2112_GPIO_CONFIG, buf,
+				 CP2112_GPIO_CONFIG_LENGTH, HID_FEATURE_REPORT,
+				 HID_REQ_SET_REPORT);
 	if (ret < 0) {
 		hid_err(hdev, "error setting GPIO config: %d\n", ret);
-		return ret;
+		goto exit;
 	}
 
-	return 0;
+	ret = 0;
+
+exit:
+	spin_unlock_irqrestore(&dev->lock, flags);
+	return ret <= 0 ? ret : -EIO;
 }
 
 static void cp2112_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
 {
 	struct cp2112_device *dev = gpiochip_get_data(chip);
 	struct hid_device *hdev = dev->hdev;
-	u8 buf[3];
+	u8 *buf = dev->in_out_buffer;
+	unsigned long flags;
 	int ret;
 
+	spin_lock_irqsave(&dev->lock, flags);
+
 	buf[0] = CP2112_GPIO_SET;
 	buf[1] = value ? 0xff : 0;
 	buf[2] = 1 << offset;
 
-	ret = hid_hw_raw_request(hdev, CP2112_GPIO_SET, buf, sizeof(buf),
-				 HID_FEATURE_REPORT, HID_REQ_SET_REPORT);
+	ret = hid_hw_raw_request(hdev, CP2112_GPIO_SET, buf,
+				 CP2112_GPIO_SET_LENGTH, HID_FEATURE_REPORT,
+				 HID_REQ_SET_REPORT);
 	if (ret < 0)
 		hid_err(hdev, "error setting GPIO values: %d\n", ret);
+
+	spin_unlock_irqrestore(&dev->lock, flags);
 }
 
 static int cp2112_gpio_get(struct gpio_chip *chip, unsigned offset)
 {
 	struct cp2112_device *dev = gpiochip_get_data(chip);
 	struct hid_device *hdev = dev->hdev;
-	u8 buf[2];
+	u8 *buf = dev->in_out_buffer;
+	unsigned long flags;
 	int ret;
 
-	ret = hid_hw_raw_request(hdev, CP2112_GPIO_GET, buf, sizeof(buf),
-				       HID_FEATURE_REPORT, HID_REQ_GET_REPORT);
-	if (ret != sizeof(buf)) {
+	spin_lock_irqsave(&dev->lock, flags);
+
+	ret = hid_hw_raw_request(hdev, CP2112_GPIO_GET, buf,
+				 CP2112_GPIO_GET_LENGTH, HID_FEATURE_REPORT,
+				 HID_REQ_GET_REPORT);
+	if (ret != CP2112_GPIO_GET_LENGTH) {
 		hid_err(hdev, "error requesting GPIO values: %d\n", ret);
-		return ret;
+		ret = ret < 0 ? ret : -EIO;
+		goto exit;
 	}
 
-	return (buf[1] >> offset) & 1;
+	ret = (buf[1] >> offset) & 1;
+
+exit:
+	spin_unlock_irqrestore(&dev->lock, flags);
+
+	return ret;
 }
 
 static int cp2112_gpio_direction_output(struct gpio_chip *chip,
@@ -234,27 +265,33 @@ static int cp2112_gpio_direction_output(struct gpio_chip *chip,
 {
 	struct cp2112_device *dev = gpiochip_get_data(chip);
 	struct hid_device *hdev = dev->hdev;
-	u8 buf[5];
+	u8 *buf = dev->in_out_buffer;
+	unsigned long flags;
 	int ret;
 
+	spin_lock_irqsave(&dev->lock, flags);
+
 	ret = hid_hw_raw_request(hdev, CP2112_GPIO_CONFIG, buf,
-				       sizeof(buf), HID_FEATURE_REPORT,
-				       HID_REQ_GET_REPORT);
-	if (ret != sizeof(buf)) {
+				 CP2112_GPIO_CONFIG_LENGTH, HID_FEATURE_REPORT,
+				 HID_REQ_GET_REPORT);
+	if (ret != CP2112_GPIO_CONFIG_LENGTH) {
 		hid_err(hdev, "error requesting GPIO config: %d\n", ret);
-		return ret;
+		goto fail;
 	}
 
 	buf[1] |= 1 << offset;
 	buf[2] = gpio_push_pull;
 
-	ret = hid_hw_raw_request(hdev, CP2112_GPIO_CONFIG, buf, sizeof(buf),
-				 HID_FEATURE_REPORT, HID_REQ_SET_REPORT);
+	ret = hid_hw_raw_request(hdev, CP2112_GPIO_CONFIG, buf,
+				 CP2112_GPIO_CONFIG_LENGTH, HID_FEATURE_REPORT,
+				 HID_REQ_SET_REPORT);
 	if (ret < 0) {
 		hid_err(hdev, "error setting GPIO config: %d\n", ret);
-		return ret;
+		goto fail;
 	}
 
+	spin_unlock_irqrestore(&dev->lock, flags);
+
 	/*
 	 * Set gpio value when output direction is already set,
 	 * as specified in AN495, Rev. 0.2, cpt. 4.4
@@ -262,6 +299,10 @@ static int cp2112_gpio_direction_output(struct gpio_chip *chip,
 	cp2112_gpio_set(chip, offset, value);
 
 	return 0;
+
+fail:
+	spin_unlock_irqrestore(&dev->lock, flags);
+	return ret < 0 ? ret : -EIO;
 }
 
 static int cp2112_hid_get(struct hid_device *hdev, unsigned char report_number,
@@ -1007,6 +1048,17 @@ static int cp2112_probe(struct hid_device *hdev, const struct hid_device_id *id)
 	struct cp2112_smbus_config_report config;
 	int ret;
 
+	dev = devm_kzalloc(&hdev->dev, sizeof(*dev), GFP_KERNEL);
+	if (!dev)
+		return -ENOMEM;
+
+	dev->in_out_buffer = devm_kzalloc(&hdev->dev, CP2112_REPORT_MAX_LENGTH,
+					  GFP_KERNEL);
+	if (!dev->in_out_buffer)
+		return -ENOMEM;
+
+	spin_lock_init(&dev->lock);
+
 	ret = hid_parse(hdev);
 	if (ret) {
 		hid_err(hdev, "parse failed\n");
@@ -1063,12 +1115,6 @@ static int cp2112_probe(struct hid_device *hdev, const struct hid_device_id *id)
 		goto err_power_normal;
 	}
 
-	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
-	if (!dev) {
-		ret = -ENOMEM;
-		goto err_power_normal;
-	}
-
 	hid_set_drvdata(hdev, (void *)dev);
 	dev->hdev		= hdev;
 	dev->adap.owner		= THIS_MODULE;
@@ -1087,7 +1133,7 @@ static int cp2112_probe(struct hid_device *hdev, const struct hid_device_id *id)
 
 	if (ret) {
 		hid_err(hdev, "error registering i2c adapter\n");
-		goto err_free_dev;
+		goto err_power_normal;
 	}
 
 	hid_dbg(hdev, "adapter registered\n");
@@ -1123,8 +1169,6 @@ static int cp2112_probe(struct hid_device *hdev, const struct hid_device_id *id)
 	gpiochip_remove(&dev->gc);
 err_free_i2c:
 	i2c_del_adapter(&dev->adap);
-err_free_dev:
-	kfree(dev);
 err_power_normal:
 	hid_hw_power(hdev, PM_HINT_NORMAL);
 err_hid_close:
@@ -1149,7 +1193,6 @@ static void cp2112_remove(struct hid_device *hdev)
 	 */
 	hid_hw_close(hdev);
 	hid_hw_stop(hdev);
-	kfree(dev);
 }
 
 static int cp2112_raw_event(struct hid_device *hdev, struct hid_report *report,
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1526559 — [PATCH 2/4] HID: lg: make transfer buffers DMA capable

Kernel v4.9 strictly enforces DMA capable buffers, so we need to remove
buffers allocated on the stack.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/hid/hid-lg.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/hid/hid-lg.c b/drivers/hid/hid-lg.c
index 76f644d..d4c72a5 100644
--- a/drivers/hid/hid-lg.c
+++ b/drivers/hid/hid-lg.c
@@ -756,11 +756,16 @@ static int lg_probe(struct hid_device *hdev, const struct hid_device_id *id)
 
 	/* Setup wireless link with Logitech Wii wheel */
 	if (hdev->product == USB_DEVICE_ID_LOGITECH_WII_WHEEL) {
-		unsigned char buf[] = { 0x00, 0xAF,  0x01, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00 };
+		const unsigned char cbuf[] = { 0x00, 0xAF,  0x01, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00 };
+		u8 *buf = kmemdup(cbuf, sizeof(cbuf), GFP_KERNEL);
 
-		ret = hid_hw_raw_request(hdev, buf[0], buf, sizeof(buf),
-					HID_FEATURE_REPORT, HID_REQ_SET_REPORT);
+		if (!buf) {
+			ret = -ENOMEM;
+			goto err_free;
+		}
 
+		ret = hid_hw_raw_request(hdev, buf[0], buf, sizeof(cbuf),
+					HID_FEATURE_REPORT, HID_REQ_SET_REPORT);
 		if (ret >= 0) {
 			/* insert a little delay of 10 jiffies ~ 40ms */
 			wait_queue_head_t wait;
@@ -775,6 +780,7 @@ static int lg_probe(struct hid_device *hdev, const struct hid_device_id *id)
 			ret = hid_hw_raw_request(hdev, buf[0], buf, sizeof(buf),
 					HID_FEATURE_REPORT, HID_REQ_SET_REPORT);
 		}
+		kfree(buf);
 	}
 
 	if (drv_data->quirks & LG_FF)
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1526607 — [PATCH] HID: lg: fix noderef.cocci warnings

drivers/hid/hid-lg.c:780:47-53: ERROR: application of sizeof to pointer

 sizeof when applied to a pointer typed expression gives the size of
 the pointer

Generated by: scripts/coccinelle/misc/noderef.cocci

CC: Benjamin Tissoires <benjamin.tissoires@redhat.com>
Signed-off-by: Fengguang Wu <fengguang.wu@intel.com>
---

 hid-lg.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

--- a/drivers/hid/hid-lg.c
+++ b/drivers/hid/hid-lg.c
@@ -777,8 +777,10 @@ static int lg_probe(struct hid_device *h
 			buf[1] = 0xB2;
 			get_random_bytes(&buf[2], 2);
 
-			ret = hid_hw_raw_request(hdev, buf[0], buf, sizeof(buf),
-					HID_FEATURE_REPORT, HID_REQ_SET_REPORT);
+			ret = hid_hw_raw_request(hdev, buf[0], buf,
+						 sizeof(*buf),
+						 HID_FEATURE_REPORT,
+						 HID_REQ_SET_REPORT);
 		}
 		kfree(buf);
 	}

[toc] | [prev] | [next] | [standalone]

#1526701 — Re: [PATCH] HID: lg: fix noderef.cocci warnings

On Nov 21 2016 or thereabouts, kbuild test robot wrote:
> drivers/hid/hid-lg.c:780:47-53: ERROR: application of sizeof to pointer
> 
>  sizeof when applied to a pointer typed expression gives the size of
>  the pointer
> 
> Generated by: scripts/coccinelle/misc/noderef.cocci
> 
> CC: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> Signed-off-by: Fengguang Wu <fengguang.wu@intel.com>
> ---
> 
>  hid-lg.c |    6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> --- a/drivers/hid/hid-lg.c
> +++ b/drivers/hid/hid-lg.c
> @@ -777,8 +777,10 @@ static int lg_probe(struct hid_device *h
>  			buf[1] = 0xB2;
>  			get_random_bytes(&buf[2], 2);
>  
> -			ret = hid_hw_raw_request(hdev, buf[0], buf, sizeof(buf),
> -					HID_FEATURE_REPORT, HID_REQ_SET_REPORT);
> +			ret = hid_hw_raw_request(hdev, buf[0], buf,
> +						 sizeof(*buf),

This is wrong. I messed up and should have used "sizeof(cbuf)", but the
coccinelle script failed at detecting the correct solution (I guess it
couldn't).

Jiri, do you want me to send a v2 of the series or will you just amend
the patch while applying?

Cheers,
Benjamin

> +						 HID_FEATURE_REPORT,
> +						 HID_REQ_SET_REPORT);
>  		}
>  		kfree(buf);
>  	}

[toc] | [prev] | [next] | [standalone]

#1527396 — Re: [PATCH] HID: lg: fix noderef.cocci warnings

On Mon, 21 Nov 2016, Benjamin Tissoires wrote:

> > Generated by: scripts/coccinelle/misc/noderef.cocci
> > 
> > CC: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> > Signed-off-by: Fengguang Wu <fengguang.wu@intel.com>
> > ---
> > 
> >  hid-lg.c |    6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > --- a/drivers/hid/hid-lg.c
> > +++ b/drivers/hid/hid-lg.c
> > @@ -777,8 +777,10 @@ static int lg_probe(struct hid_device *h
> >  			buf[1] = 0xB2;
> >  			get_random_bytes(&buf[2], 2);
> >  
> > -			ret = hid_hw_raw_request(hdev, buf[0], buf, sizeof(buf),
> > -					HID_FEATURE_REPORT, HID_REQ_SET_REPORT);
> > +			ret = hid_hw_raw_request(hdev, buf[0], buf,
> > +						 sizeof(*buf),
> 
> This is wrong. I messed up and should have used "sizeof(cbuf)", but the
> coccinelle script failed at detecting the correct solution (I guess it
> couldn't).

Fengguang, is there anything that could be done to improve this?

> Jiri, do you want me to send a v2 of the series or will you just amend 
> the patch while applying?

I'll fix that up, no worries. Thanks,

-- 
Jiri Kosina
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1528140 — Re: [PATCH] HID: lg: fix noderef.cocci warnings

On Tue, Nov 22, 2016 at 11:44:34AM +0100, Jiri Kosina wrote:
>On Mon, 21 Nov 2016, Benjamin Tissoires wrote:
>
>> > Generated by: scripts/coccinelle/misc/noderef.cocci
>> >
>> > CC: Benjamin Tissoires <benjamin.tissoires@redhat.com>
>> > Signed-off-by: Fengguang Wu <fengguang.wu@intel.com>
>> > ---
>> >
>> >  hid-lg.c |    6 ++++--
>> >  1 file changed, 4 insertions(+), 2 deletions(-)
>> >
>> > --- a/drivers/hid/hid-lg.c
>> > +++ b/drivers/hid/hid-lg.c
>> > @@ -777,8 +777,10 @@ static int lg_probe(struct hid_device *h
>> >  			buf[1] = 0xB2;
>> >  			get_random_bytes(&buf[2], 2);
>> >
>> > -			ret = hid_hw_raw_request(hdev, buf[0], buf, sizeof(buf),
>> > -					HID_FEATURE_REPORT, HID_REQ_SET_REPORT);
>> > +			ret = hid_hw_raw_request(hdev, buf[0], buf,
>> > +						 sizeof(*buf),
>>
>> This is wrong. I messed up and should have used "sizeof(cbuf)", but the
>> coccinelle script failed at detecting the correct solution (I guess it
>> couldn't).
>
>Fengguang, is there anything that could be done to improve this?

CC Julie and Gilles. I'm not sure if the coccinelle script could be
made that smart. :)

>> Jiri, do you want me to send a v2 of the series or will you just amend
>> the patch while applying?
>
>I'll fix that up, no worries. Thanks,
>
>-- 
>Jiri Kosina
>SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1528154 — Re: [PATCH] HID: lg: fix noderef.cocci warnings

On Wed, 23 Nov 2016, Fengguang Wu wrote:

> On Tue, Nov 22, 2016 at 11:44:34AM +0100, Jiri Kosina wrote:
> > On Mon, 21 Nov 2016, Benjamin Tissoires wrote:
> >
> > > > Generated by: scripts/coccinelle/misc/noderef.cocci
> > > >
> > > > CC: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> > > > Signed-off-by: Fengguang Wu <fengguang.wu@intel.com>
> > > > ---
> > > >
> > > >  hid-lg.c |    6 ++++--
> > > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > > >
> > > > --- a/drivers/hid/hid-lg.c
> > > > +++ b/drivers/hid/hid-lg.c
> > > > @@ -777,8 +777,10 @@ static int lg_probe(struct hid_device *h
> > > >  			buf[1] = 0xB2;
> > > >  			get_random_bytes(&buf[2], 2);
> > > >
> > > > -			ret = hid_hw_raw_request(hdev, buf[0], buf,
> > > sizeof(buf),
> > > > -					HID_FEATURE_REPORT,
> > > HID_REQ_SET_REPORT);
> > > > +			ret = hid_hw_raw_request(hdev, buf[0], buf,
> > > > +						 sizeof(*buf),
> > >
> > > This is wrong. I messed up and should have used "sizeof(cbuf)", but the
> > > coccinelle script failed at detecting the correct solution (I guess it
> > > couldn't).
> >
> > Fengguang, is there anything that could be done to improve this?
>
> CC Julie and Gilles. I'm not sure if the coccinelle script could be
> made that smart. :)

Thanks for forwarding.  From looking at the code snippet, I have the
impression that if it were possible, it would require an interprocedural
analysis, and the cost would outweigh the benefit.  Basically, I don't see
any cbuf nearby.

julia


> > > Jiri, do you want me to send a v2 of the series or will you just amend
> > > the patch while applying?
> >
> > I'll fix that up, no worries. Thanks,
> >
> > --
> > Jiri Kosina
> > SUSE Labs
>

[toc] | [prev] | [next] | [standalone]

#1526608 — Re: [PATCH 2/4] HID: lg: make transfer buffers DMA capable

Hi Benjamin,

[auto build test WARNING on hid/for-next]
[also build test WARNING on v4.9-rc6 next-20161117]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Benjamin-Tissoires/HID-fix-few-non-DMA-capable-HID-transfers/20161121-185330
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jikos/hid.git for-next


coccinelle warnings: (new ones prefixed by >>)

>> drivers/hid/hid-lg.c:780:47-53: ERROR: application of sizeof to pointer

Please review and possibly fold the followup patch.

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[toc] | [prev] | [next] | [standalone]

#1526560 — [PATCH 4/4] HID: rmi: make transfer buffers DMA capable

Kernel v4.9 strictly enforces DMA capable buffers, so we need to remove
buffers allocated on the stack.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/hid/hid-rmi.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/hid/hid-rmi.c b/drivers/hid/hid-rmi.c
index 9cd2ca3..be89bcb 100644
--- a/drivers/hid/hid-rmi.c
+++ b/drivers/hid/hid-rmi.c
@@ -188,10 +188,16 @@ static int rmi_set_page(struct hid_device *hdev, u8 page)
 static int rmi_set_mode(struct hid_device *hdev, u8 mode)
 {
 	int ret;
-	u8 txbuf[2] = {RMI_SET_RMI_MODE_REPORT_ID, mode};
+	const u8 txbuf[2] = {RMI_SET_RMI_MODE_REPORT_ID, mode};
+	u8 *buf;
 
-	ret = hid_hw_raw_request(hdev, RMI_SET_RMI_MODE_REPORT_ID, txbuf,
+	buf = kmemdup(txbuf, sizeof(txbuf), GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
+
+	ret = hid_hw_raw_request(hdev, RMI_SET_RMI_MODE_REPORT_ID, buf,
 			sizeof(txbuf), HID_FEATURE_REPORT, HID_REQ_SET_REPORT);
+	kfree(buf);
 	if (ret < 0) {
 		dev_err(&hdev->dev, "unable to set rmi mode to %d (%d)\n", mode,
 			ret);
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1528581

On Mon, 21 Nov 2016, Benjamin Tissoires wrote:

> While playing a little bit with the CP2112 (for hid-rmi I must confess), I
> realized that kernel v4.9 now enforces DMA capable buffers when calling
> hid_hw_raw_request(). Kernel v4.8 works fine (but gives a stacktrace the first
> time), but v4.9 doesn't.
> 
> So I gave a check of all the other drivers in the HID tree, and it looks like
> only 4 drivers are not properly allocating their buffers.
> 
> I'd say this is v4.9 material but I was not able to test magicmouse and lg.
> If this doesn't qualifies for 4.9-rc7, I think we should add the stable@ stamp,
> given that the chance this will break hid-rmi is huge (cp2112 is not so much an
> issue, given it's a devel board).

Thanks for fixing this up. Applied to for-4.9/upstream-fixes.

-- 
Jiri Kosina
SUSE Labs

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web