platform/chrome: cros_ec_i2c: Less function calls in cros_ec_cmd_xfer_i2c() after error detection

Commit Message

Markus Elfring Dec. 28, 2023, 7:56 a.m. UTC

From: Markus Elfring <[e-mail]>
Date: Thu, 28 Dec 2023 08:36:49 +0100

The kfree() function was called in up to two cases by
the cros_ec_cmd_xfer_i2c() function during error handling
even if the passed variable contained a null pointer.
This issue was detected by using the Coccinelle software.

* Adjust jump targets.

* Delete two initialisations which became unnecessary
  with this refactoring.

Signed-off-by: Markus Elfring <[e-mail]>
---
 drivers/platform/chrome/cros_ec_i2c.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

--
2.43.0

Comments

Tzung-Bi Shih Jan. 3, 2024, 3:34 a.m. UTC | #1

On Thu, Dec 28, 2023 at 08:56:38AM +0100, Markus Elfring wrote:
> The kfree() function was called in up to two cases by
> the cros_ec_cmd_xfer_i2c() function during error handling
> even if the passed variable contained a null pointer.
> This issue was detected by using the Coccinelle software.
> 
> * Adjust jump targets.
> 
> * Delete two initialisations which became unnecessary
>   with this refactoring.

The patch saves few instructions but makes the code less readable a bit.  I
would prefer to leave the code as is or wait for other reviewers' input.

Markus Elfring Jan. 3, 2024, 7:56 a.m. UTC | #2

>> The kfree() function was called in up to two cases by
>> the cros_ec_cmd_xfer_i2c() function during error handling
>> even if the passed variable contained a null pointer.
>> This issue was detected by using the Coccinelle software.
>>
>> * Adjust jump targets.
>>
>> * Delete two initialisations which became unnecessary
>>   with this refactoring.
>
> The patch saves few instructions but makes the code less readable a bit.

Do you find advices applicable from another information source
also for this function implementation?
https://wiki.sei.cmu.edu/confluence/display/c/MEM12-C.+Consider+using+a+goto+chain+when+leaving+a+function+on+error+when+using+and+releasing+resources

Regards,
Markus

Patch

diff --git a/drivers/platform/chrome/cros_ec_i2c.c b/drivers/platform/chrome/cros_ec_i2c.c
index e29c51cbfd71..2a6ec623e352 100644
--- a/drivers/platform/chrome/cros_ec_i2c.c
+++ b/drivers/platform/chrome/cros_ec_i2c.c
@@ -193,8 +193,7 @@  static int cros_ec_cmd_xfer_i2c(struct cros_ec_device *ec_dev,
 	int i;
 	int len;
 	int packet_len;
-	u8 *out_buf = NULL;
-	u8 *in_buf = NULL;
+	u8 *in_buf, *out_buf;
 	u8 sum;
 	struct i2c_msg i2c_msg[2];

@@ -210,7 +209,8 @@  static int cros_ec_cmd_xfer_i2c(struct cros_ec_device *ec_dev,
 	packet_len = msg->insize + 3;
 	in_buf = kzalloc(packet_len, GFP_KERNEL);
 	if (!in_buf)
-		goto done;
+		goto check_command;
+
 	i2c_msg[1].len = packet_len;
 	i2c_msg[1].buf = (char *)in_buf;

@@ -221,7 +221,8 @@  static int cros_ec_cmd_xfer_i2c(struct cros_ec_device *ec_dev,
 	packet_len = msg->outsize + 4;
 	out_buf = kzalloc(packet_len, GFP_KERNEL);
 	if (!out_buf)
-		goto done;
+		goto free_in_buf;
+
 	i2c_msg[0].len = packet_len;
 	i2c_msg[0].buf = (char *)out_buf;

@@ -278,8 +279,10 @@  static int cros_ec_cmd_xfer_i2c(struct cros_ec_device *ec_dev,

 	ret = len;
 done:
-	kfree(in_buf);
 	kfree(out_buf);
+free_in_buf:
+	kfree(in_buf);
+check_command:
 	if (msg->command == EC_CMD_REBOOT_EC)
 		msleep(EC_REBOOT_DELAY_MS);