2023-12-28 07:57:03

by Markus Elfring

Subject: [PATCH] platform/chrome: cros_ec_i2c: Less function calls in cros_ec_cmd_xfer_i2c() after error detection

From: Markus Elfring <[email protected]>
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 <[email protected]>
---
drivers/platform/chrome/cros_ec_i2c.c | 13 ++++++++-----
1 file changed, 8 insertions(+), 5 deletions(-)

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

--
2.43.0

2024-01-03 03:34:28

by Tzung-Bi Shih

Subject: Re: [PATCH] platform/chrome: cros_ec_i2c: Less function calls in cros_ec_cmd_xfer_i2c() after error detection

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.

2024-01-03 07:58:05

by Markus Elfring

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

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