Re: [PATCH] platform/chrome: cros_ec: Separate logic for getting panic info

[Tzung-Bi Shih <tzungbi@kernel.org>]

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.