Skip to contentSkip to conversation

HID: alps: Fix some bugs and improve code around 't4_read_write_register()'

PATCH Applied Christophe JAILLET 2 participants 8 messages Mar 19, 2018 – Apr 26, 2018

Subsystems (2)
Kernel janitors HID
lore
Share & tools
lore

Christophe JAILLET

author
#0 lore raw

cover HID: alps: Fix some bugs and improve code around 't4_read_write_register()'

All is said in the subject and below.

These patches are untested. Especially, patch 1 slightly changes the behavior of 't4_read_write_register()'.
This looks logical to me, but please, review it carefully.

Christophe JAILLET (4):
HID: alps: Report an error if we receive invalid data in

    't4_read_write_register()'

HID: alps: Save a memory allocation in 't4_read_write_register()' when
writing data
HID: alps: Check errors returned by 't4_read_write_register()'
HID: alps: Fix some style in 't4_read_write_register()'

 drivers/hid/hid-alps.c | 27 ++++++++++++++++++++++-----
 1 file changed, 22 insertions(+), 5 deletions(-)
Signature · 1 line
2.14.1
author
#1 lore raw

PATCH 1/4 HID: alps: Report an error if we receive invalid data in 't4_read_write_register()'

If the data received is not what is expected, we should return an error.

Otherwise, we return 0 or a positive value which will be interpreted as success, but '*read_val' has not been updated.

Fixes 73196ebe134d ("HID: alps: add support for Alps T4 Touchpad device")
Signed-off-by Christophe JAILLET <[email protected]>

---
Untested

---
 drivers/hid/hid-alps.c | 2 ++
 1 file changed, 2 insertions(+)
drivers/hid/hid-alps.c +2 -0
index b1eeb4839bfc..925396fdf0d9 100644
@@ -219,6 +219,8 @@ static int t4_read_write_register(struct hid_device *hdev, u32 address,
 			goto exit_readbuf;
 		}
 
+		ret = -EINVAL;
+
 		if (*(u32 *)&readbuf[6] != address) {
 			dev_err(&hdev->dev, "read register address error (%x,%x)\n",
 			*(u32 *)&readbuf[6], address);
Signature · 1 line
2.14.1
author
#2 lore raw

PATCH 2/4 HID: alps: Save a memory allocation in 't4_read_write_register()' when writing data

In reply to Christophe JAILLET

if 'read_flag' is false, there is no need to allocate and free memory. We can simply avoid the memory allocation and pass NULL to kfree.

Signed-off-by Christophe JAILLET <[email protected]>
---
 drivers/hid/hid-alps.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/hid/hid-alps.c +2 -2
index 925396fdf0d9..fe8a0624d5e4 100644
@@ -171,7 +171,7 @@ static int t4_read_write_register(struct hid_device *hdev, u32 address,
 	int ret;
 	u16 check_sum;
 	u8 *input;
-	u8 *readbuf;
+	u8 *readbuf = NULL;
 
 	input = kzalloc(T4_FEATURE_REPORT_LEN, GFP_KERNEL);
 	if (!input)

@@ -204,8 +204,8 @@ static int t4_read_write_register(struct hid_device *hdev, u32 address,
 		goto exit;
 	}
 
-	readbuf = kzalloc(T4_FEATURE_REPORT_LEN, GFP_KERNEL);
 	if (read_flag) {
+		readbuf = kzalloc(T4_FEATURE_REPORT_LEN, GFP_KERNEL);
 		if (!readbuf) {
 			ret = -ENOMEM;
 			goto exit;
Signature · 1 line
2.14.1
author
#3 lore raw

PATCH 3/4 HID: alps: Check errors returned by 't4_read_write_register()'

In reply to Christophe JAILLET

If only the first 't4_read_write_register()' call fails, the error code will be overwritten and lost.
Directly report the error instead.

While at it, log some errors if 't4_read_write_register()' fails, as done in the rest of the driver.

Signed-off-by Christophe JAILLET <[email protected]>
---
 drivers/hid/hid-alps.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)
drivers/hid/hid-alps.c +18 -0
index fe8a0624d5e4..137b963779c6 100644
@@ -460,17 +460,35 @@ static int __maybe_unused alps_post_reset(struct hid_device *hdev)
 	case T4:
 		ret = t4_read_write_register(hdev, T4_PRM_FEED_CONFIG_1,
 			NULL, T4_I2C_ABS, false);
+		if (ret < 0) {
+			dev_err(&hdev->dev, "failed T4_PRM_FEED_CONFIG_1 (%d)\n",
+				ret);
+			goto exit;
+		}
+
 		ret = t4_read_write_register(hdev, T4_PRM_FEED_CONFIG_4,
 			NULL, T4_FEEDCFG4_ADVANCED_ABS_ENABLE, false);
+		if (ret < 0) {
+			dev_err(&hdev->dev, "failed T4_PRM_FEED_CONFIG_4 (%d)\n",
+				ret);
+			goto exit;
+		}
 		break;
 	case U1:
 		ret = u1_read_write_register(hdev,
 			ADDRESS_U1_DEV_CTRL_1, NULL,
 			U1_TP_ABS_MODE | U1_SP_ABS_MODE, false);
+		if (ret < 0) {
+			dev_err(&hdev->dev, "failed to change TP mode (%d)\n",
+				ret);
+			goto exit;
+		}
 		break;
 	default:
 		break;
 	}
+
+exit:
 	return ret;
 }
Signature · 1 line
2.14.1
author
#4 lore raw

PATCH 4/4 HID: alps: Fix some style in 't4_read_write_register()'

In reply to Christophe JAILLET

Better indent the code to improve readability.

Signed-off-by Christophe JAILLET <[email protected]>
---
 drivers/hid/hid-alps.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
drivers/hid/hid-alps.c +3 -3
index 137b963779c6..60d3950692d9 100644
@@ -223,20 +223,20 @@ static int t4_read_write_register(struct hid_device *hdev, u32 address,
 
 		if (*(u32 *)&readbuf[6] != address) {
 			dev_err(&hdev->dev, "read register address error (%x,%x)\n",
-			*(u32 *)&readbuf[6], address);
+				*(u32 *)&readbuf[6], address);
 			goto exit_readbuf;
 		}
 
 		if (*(u16 *)&readbuf[10] != 1) {
 			dev_err(&hdev->dev, "read register size error (%x)\n",
-			*(u16 *)&readbuf[10]);
+				*(u16 *)&readbuf[10]);
 			goto exit_readbuf;
 		}
 
 		check_sum = t4_calc_check_sum(readbuf, 6, 7);
 		if (*(u16 *)&readbuf[13] != check_sum) {
 			dev_err(&hdev->dev, "read register checksum error (%x,%x)\n",
-			*(u16 *)&readbuf[13], check_sum);
+				*(u16 *)&readbuf[13], check_sum);
 			goto exit_readbuf;
 		}
Signature · 1 line
2.14.1
maintainer
#5 lore raw
In reply to Christophe JAILLET
Quoted 8 lines · Christophe JAILLET

All is said in the subject and below.

These patches are untested. Especially, patch 1 slightly changes the behavior of 't4_read_write_register()'.
This looks logical to me, but please, review it carefully.

Christophe JAILLET (4):
HID: alps: Report an error if we receive invalid data in

Quoted 8 lines
    't4_read_write_register()'

HID: alps: Save a memory allocation in 't4_read_write_register()' when
writing data
HID: alps: Check errors returned by 't4_read_write_register()'
HID: alps: Fix some style in 't4_read_write_register()'

 drivers/hid/hid-alps.c | 27 ++++++++++++++++++++++-----
 1 file changed, 22 insertions(+), 5 deletions(-)

Masaki-san,

do you have any comments to Christophe's patchset please?

Thanks,

Signature · 2 lines
Jiri Kosina
SUSE Labs
maintainer
#6 lore raw
Quoted 12 lines · Jiri Kosina
Quoted 12 lines

These patches are untested. Especially, patch 1 slightly changes the behavior of 't4_read_write_register()'.
This looks logical to me, but please, review it carefully.

Christophe JAILLET (4):
HID: alps: Report an error if we receive invalid data in

    't4_read_write_register()'

HID: alps: Save a memory allocation in 't4_read_write_register()' when
writing data
HID: alps: Check errors returned by 't4_read_write_register()'
HID: alps: Fix some style in 't4_read_write_register()'

Quoted 6 lines
Quoted 2 lines
 drivers/hid/hid-alps.c | 27 ++++++++++++++++++++++-----
 1 file changed, 22 insertions(+), 5 deletions(-)

Masaki-san,

do you have any comments to Christophe's patchset please?

If there is no feedback, I'll queue the set for 4.18.

Thanks,

Signature · 2 lines
Jiri Kosina
SUSE Labs
maintainer Applied
#7 lore raw
In reply to Christophe JAILLET
Quoted 8 lines · Christophe JAILLET

All is said in the subject and below.

These patches are untested. Especially, patch 1 slightly changes the behavior of 't4_read_write_register()'.
This looks logical to me, but please, review it carefully.

Christophe JAILLET (4):
HID: alps: Report an error if we receive invalid data in

Quoted 8 lines
    't4_read_write_register()'

HID: alps: Save a memory allocation in 't4_read_write_register()' when
writing data
HID: alps: Check errors returned by 't4_read_write_register()'
HID: alps: Fix some style in 't4_read_write_register()'

 drivers/hid/hid-alps.c | 27 ++++++++++++++++++++++-----
 1 file changed, 22 insertions(+), 5 deletions(-)

Queued for 4.18, thanks.

Signature · 2 lines
Jiri Kosina
SUSE Labs