platform/chrome: cros_ec: Separate logic for getting panic info

Commit Message

Rob Barnes April 6, 2023, 4:14 p.m. UTC

Create a separate function called cros_ec_get_panicinfo for getting
panic info from EC.

Currently cros_ec_create_panicinfo is the only caller.

Signed-off-by: Rob Barnes <[e-mail]>
---

 drivers/platform/chrome/cros_ec_debugfs.c | 46 ++++++++++++++++++-----
 1 file changed, 36 insertions(+), 10 deletions(-)

Comments

Tzung-Bi Shih April 7, 2023, 2:33 a.m. UTC | #1

On Thu, Apr 06, 2023 at 04:14:14PM +0000, Rob Barnes wrote:
> -static int cros_ec_create_panicinfo(struct cros_ec_debugfs *debug_info)
> +/**
> + * Returns the size of the panicinfo data fetched from the EC
> + */
> +static int cros_ec_get_panicinfo(struct cros_ec_device *ec_dev, uint8_t *data,
> +				 int data_size)
>  {
> -	struct cros_ec_device *ec_dev = debug_info->ec->ec_dev;
> -	int ret;
> +	int ret = 0;

`ret` doesn't need to be initialized.  It will be overridden soon anyway.

> -	insize = ec_dev->max_response;
> +	if (!data || data_size <= 0)
> +		return -EINVAL;

How about `data_size > ec_dev->max_response`?

> -	msg = devm_kzalloc(debug_info->ec->dev,
> -			sizeof(*msg) + insize, GFP_KERNEL);
> +	msg = devm_kzalloc(ec_dev->dev, sizeof(*msg) + data_size, GFP_KERNEL);

In the case, it doesn't have to be devm anymore.

>  	ret = cros_ec_cmd_xfer_status(ec_dev, msg);
> +
> +	if (ret < 0)
> +		goto free;

If possible, please remove the extra blank line to make them look as a whole.

> +free:
> +	devm_kfree(ec_dev->dev, msg);

Given that the function controls the memory's lifecycle, it doesn't have to
be devm.

> +static int cros_ec_create_panicinfo(struct cros_ec_debugfs *debug_info)
> +{
> +	struct cros_ec_device *ec_dev = debug_info->ec->ec_dev;
[...]
> +	ret = cros_ec_get_panicinfo(ec_dev, data, ec_dev->max_response);
> +
>  	if (ret < 0) {
>  		ret = 0;
>  		goto free;

Ditto, please remove the extra blank line if possible.

Patch

diff --git a/drivers/platform/chrome/cros_ec_debugfs.c b/drivers/platform/chrome/cros_ec_debugfs.c
index a98c529d8c698..def7b2f2dd7f9 100644
--- a/drivers/platform/chrome/cros_ec_debugfs.c
+++ b/drivers/platform/chrome/cros_ec_debugfs.c
@@ -400,24 +400,50 @@  static void cros_ec_cleanup_console_log(struct cros_ec_debugfs *debug_info)
 	}
 }
 
-static int cros_ec_create_panicinfo(struct cros_ec_debugfs *debug_info)
+/**
+ * Returns the size of the panicinfo data fetched from the EC
+ */
+static int cros_ec_get_panicinfo(struct cros_ec_device *ec_dev, uint8_t *data,
+				 int data_size)
 {
-	struct cros_ec_device *ec_dev = debug_info->ec->ec_dev;
-	int ret;
+	int ret = 0;
 	struct cros_ec_command *msg;
-	int insize;
 
-	insize = ec_dev->max_response;
+	if (!data || data_size <= 0)
+		return -EINVAL;
 
-	msg = devm_kzalloc(debug_info->ec->dev,
-			sizeof(*msg) + insize, GFP_KERNEL);
+	msg = devm_kzalloc(ec_dev->dev, sizeof(*msg) + data_size, GFP_KERNEL);
 	if (!msg)
 		return -ENOMEM;
 
 	msg->command = EC_CMD_GET_PANIC_INFO;
-	msg->insize = insize;
+	msg->insize = data_size;
 
 	ret = cros_ec_cmd_xfer_status(ec_dev, msg);
+
+	if (ret < 0)
+		goto free;
+
+	memcpy(data, msg->data, data_size);
+
+free:
+	devm_kfree(ec_dev->dev, msg);
+	return ret;
+}
+
+static int cros_ec_create_panicinfo(struct cros_ec_debugfs *debug_info)
+{
+	struct cros_ec_device *ec_dev = debug_info->ec->ec_dev;
+	int ret;
+	void *data;
+
+	data = devm_kzalloc(debug_info->ec->dev, ec_dev->max_response,
+			    GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	ret = cros_ec_get_panicinfo(ec_dev, data, ec_dev->max_response);
+
 	if (ret < 0) {
 		ret = 0;
 		goto free;
@@ -427,7 +453,7 @@  static int cros_ec_create_panicinfo(struct cros_ec_debugfs *debug_info)
 	if (ret == 0)
 		goto free;
 
-	debug_info->panicinfo_blob.data = msg->data;
+	debug_info->panicinfo_blob.data = data;
 	debug_info->panicinfo_blob.size = ret;
 
 	debugfs_create_blob("panicinfo", S_IFREG | 0444, debug_info->dir,
@@ -436,7 +462,7 @@  static int cros_ec_create_panicinfo(struct cros_ec_debugfs *debug_info)
 	return 0;
 
 free:
-	devm_kfree(debug_info->ec->dev, msg);
+	devm_kfree(debug_info->ec->dev, data);
 	return ret;
 }