[PATCH 0/4] 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(-)

-- 
2.14.1

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 <redacted>
---
Untested
---
 drivers/hid/hid-alps.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/hid/hid-alps.c b/drivers/hid/hid-alps.c
index b1eeb4839bfc..925396fdf0d9 100644
--- a/drivers/hid/hid-alps.c
+++ b/drivers/hid/hid-alps.c

@@ -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);

-- 
2.14.1

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 <redacted>
---
 drivers/hid/hid-alps.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/hid/hid-alps.c b/drivers/hid/hid-alps.c
index 925396fdf0d9..fe8a0624d5e4 100644
--- a/drivers/hid/hid-alps.c
+++ b/drivers/hid/hid-alps.c

@@ -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;

-- 
2.14.1

Better indent the code to improve readability.

Signed-off-by: Christophe JAILLET <redacted>
---
 drivers/hid/hid-alps.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/hid/hid-alps.c b/drivers/hid/hid-alps.c
index 137b963779c6..60d3950692d9 100644
--- a/drivers/hid/hid-alps.c
+++ b/drivers/hid/hid-alps.c

@@ -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;
 		}
 

-- 
2.14.1

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 <redacted>
---
 drivers/hid/hid-alps.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/hid/hid-alps.c b/drivers/hid/hid-alps.c
index fe8a0624d5e4..137b963779c6 100644
--- a/drivers/hid/hid-alps.c
+++ b/drivers/hid/hid-alps.c

@@ -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;
 }
 

-- 
2.14.1

On Mon, 19 Mar 2018, Christophe JAILLET wrote:

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(-)

Masaki-san,

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

Thanks,

-- 
Jiri Kosina
SUSE Labs

On Tue, 27 Mar 2018, Jiri Kosina wrote:

quoted

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(-)

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,

-- 
Jiri Kosina
SUSE Labs

On Mon, 19 Mar 2018, Christophe JAILLET wrote:

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(-)

Queued for 4.18, thanks.

-- 
Jiri Kosina
SUSE Labs