linux-kernel - Re: [PATCH 03/16] media: i2c: rdacm20: Replace goto with a loop

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 for Android: free password hash cracker in your pocket

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

Message-ID: <c95022bc-3841-4d0a-653c-6d6974e20355@ideasonboard.com>
Date:   Wed, 17 Feb 2021 13:01:26 +0000
From:   Kieran Bingham <kieran.bingham+renesas@...asonboard.com>
To:     Jacopo Mondi <jacopo+renesas@...ndi.org>,
        laurent.pinchart+renesas@...asonboard.com,
        niklas.soderlund+renesas@...natech.se, geert@...ux-m68k.org
Cc:     Mauro Carvalho Chehab <mchehab@...nel.org>,
        linux-media@...r.kernel.org, linux-renesas-soc@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 03/16] media: i2c: rdacm20: Replace goto with a loop

Hi Jacopo,

On 16/02/2021 17:41, Jacopo Mondi wrote:
> During the camera module initialization the image sensor PID is read to
> verify it can correctly be identified. The current implementation is
> rather confused and uses a loop implemented with a label and a goto.
> 
> Replace it with a more compact for() loop.
> 
> No functional changes intended.

I think there is a functional change in here, but I almost like it.

Before, if the read was successful, it would check to see if the
OV10635_PID == OV10635_VERSION, and if not it would print that the read
was successful but a mismatch.

Now - it will retry again instead, and if at the end of the retries it
still fails then it's a failure.

This means we perhaps don't get told if the device id is not correct in
the same way, but it also means that if the VERSION was not correct
because of a read error (which I believe i've seen occur), it will retry.

Because there is a functional change you might want to update the
commit, but I still think this is a good change overall.

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@...asonboard.com>

> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@...ndi.org>
> ---
>  drivers/media/i2c/rdacm20.c | 27 ++++++++++-----------------
>  1 file changed, 10 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c
> index 4d9bac87cba8..6504ed0bd3bc 100644
> --- a/drivers/media/i2c/rdacm20.c
> +++ b/drivers/media/i2c/rdacm20.c
> @@ -59,6 +59,8 @@
>   */
>  #define OV10635_PIXEL_RATE		(44000000)
>  
> +#define OV10635_PID_TIMEOUT		3
> +
>  static const struct ov10635_reg {
>  	u16	reg;
>  	u8	val;
> @@ -452,7 +454,7 @@ static const struct v4l2_subdev_ops rdacm20_subdev_ops = {
>  
>  static int rdacm20_initialize(struct rdacm20_device *dev)
>  {
> -	unsigned int retry = 3;
> +	unsigned int i;
>  	int ret;
>  
>  	/* Verify communication with the MAX9271: ping to wakeup. */
> @@ -501,23 +503,14 @@ static int rdacm20_initialize(struct rdacm20_device *dev)
>  		return ret;
>  	usleep_range(10000, 15000);
>  
> -again:
> -	ret = ov10635_read16(dev, OV10635_PID);
> -	if (ret < 0) {
> -		if (retry--)
> -			goto again;
> -
> -		dev_err(dev->dev, "OV10635 ID read failed (%d)\n",
> -			ret);
> -		return -ENXIO;
> +	for (i = 0; i < OV10635_PID_TIMEOUT; ++i) {
> +		ret = ov10635_read16(dev, OV10635_PID);
> +		if (ret == OV10635_VERSION)
> +			break;
> +		usleep_range(1000, 2000);
>  	}
> -
> -	if (ret != OV10635_VERSION) {
> -		if (retry--)
> -			goto again;
> -
> -		dev_err(dev->dev, "OV10635 ID mismatch (0x%04x)\n",
> -			ret);
> +	if (i == OV10635_PID_TIMEOUT) {
> +		dev_err(dev->dev, "OV10635 ID read failed (%d)\n", ret);
>  		return -ENXIO;
>  	}
>  
>