linux-kernel - Re: [PATCH] i2c-hix5hd2: Add more debug info when transfer fail.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20230610140151.dghq3g2xw5wx6g67@intel.intel>
Date:   Sat, 10 Jun 2023 16:01:51 +0200
From:   Andi Shyti <andi.shyti@...nel.org>
To:     Tao Lan <taolan@...wei.com>
Cc:     linux-i2c@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] i2c-hix5hd2: Add more debug info when transfer fail.

Hi Taolan,

On Fri, Sep 30, 2022 at 01:44:05AM +0000, Tao Lan wrote:
> From: taolan <taolan@...wei.com>
> 
> The transfer result is checked, classified, and printed, which
> facilitates debugging.

you are missing your Signed-off-by here.

> ---
>  drivers/i2c/busses/i2c-hix5hd2.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/busses/i2c-hix5hd2.c b/drivers/i2c/busses/i2c-hix5hd2.c
> index 61ae58f57047..e6ab8b20cd01 100644
> --- a/drivers/i2c/busses/i2c-hix5hd2.c
> +++ b/drivers/i2c/busses/i2c-hix5hd2.c
> @@ -366,8 +366,17 @@ static int hix5hd2_i2c_xfer(struct i2c_adapter *adap,
>  			goto out;
>  	}
>  
> -	ret = num;
> +	if (i == num) {
> +		ret = num;
> +	} else {

it can't be, because if you are exiting normally from the for
loop, then i is equal to num.

You might want to put this inside the out label, or, even better:

                    stop = (i == num - 1);
                    ret = hix5hd2_i2c_xfer_msg(priv, msgs, stop);
                    if (ret < 0)
    -                       goto out;
    +                       break;
            }
     
    -       ret = num;
    +       if (i == num) {
    +               ret = num;
    +       } else {
    +               /* Only one message, cannot access the device */
                    ...
    +       }
     
    -out:

What do you think?

> +		/* Only one message, cannot access the device */
> +		if (i == 1)
> +			ret = -EREMOTEIO;
> +		else
> +			ret = i;
>  
> +		pr_warning("xfer message failed\n");

please use dev_warn here.

BTW, is it warning or error?

Andi

> +	}
>  out:
>  	pm_runtime_mark_last_busy(priv->dev);
>  	pm_runtime_put_autosuspend(priv->dev);
> -- 
> 2.22.0
>