linux-kernel - Re: [PATCH] media: adv7180: Fix cppcheck warnings and errors

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:
 <MAZPR01MB695752E4ADB0110443EA695CF2432@MAZPR01MB6957.INDPRD01.PROD.OUTLOOK.COM>
Date: Thu, 1 Feb 2024 12:13:25 +0000
From: Bhavin Sharma <bhavin.sharma@...iconsignals.io>
To: Kieran Bingham <kieran.bingham@...asonboard.com>, "lars@...afoo.de"
	<lars@...afoo.de>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "linux-media@...r.kernel.org"
	<linux-media@...r.kernel.org>, "mchehab@...nel.org" <mchehab@...nel.org>
Subject: Re: [PATCH] media: adv7180: Fix cppcheck warnings and errors

Hi,

> Quoting Bhavin Sharma (2023-12-29 13:37:14)
>> Thanks for the reply,�Kieran
>>
>> >> WARNING: Missing a blank line after declarations
>> >> ERROR: else should follow close brace '}'
>> >>
>> >> Signed-off-by: Bhavin Sharma <bhavin.sharma@...iconsignals.io>
>> >>
>> >> diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
>> >> index 54134473186b..91756116eff7 100644
>> >> --- a/drivers/media/i2c/adv7180.c
>> >> +++ b/drivers/media/i2c/adv7180.c
>> >> @@ -357,6 +357,7 @@ static int adv7180_querystd(struct v4l2_subdev *sd, v4l2_std_id *std)
>> >>� {
>> >>�������� struct adv7180_state *state = to_state(sd);
>>
>> >>Personally, I would keep the if (err) hugging the line it's associated
>> with.
>>
>> If we follow the code base pattern for this diver, we are getting same online space in conditional if statements.
>> So, we need to make changes there also.

> If there are multiple places in a file for the same fixup, then indeed -
> make them all in a single patch as a single cleanup.


>> >>�������� int err = mutex_lock_interruptible(&state->mutex);
>> >> +
>> >>�������� if (err)
>> >>���������������� return err;
>> >>�
>> >> @@ -411,6 +412,7 @@ static int adv7180_g_input_status(struct v4l2_subdev *sd, u32 *status)
>> >>� {
>> >>�������� struct adv7180_state *state = to_state(sd);
>> >>�������� int ret = mutex_lock_interruptible(&state->mutex);
>> >> +
>> >>�������� if (ret)
>> >>���������������� return ret;
>> >>�
>> >> @@ -1046,8 +1048,7 @@ static int adv7182_init(struct adv7180_state *state)
>> >>���������������������������������������������� ADV7180_REG_EXTENDED_OUTPUT_CONTROL,
>> >>���������������������������������������������� 0x17);
>> >>������������������������ }
>> >> -�������������� }
>> >> -�������������� else
>> >> +�������������� } else
>>
>> >>I think kernel code style requires an else clause following a multiline
>> scope to also have its scope enclosed in braces even if it's a single
>> statement.
>>
>> On many places in driver there is single statement after else without closing�
>> So, we have to make changes in those places also.
>>
>> So, better I should make changes in all places and make version V2 patch.

> Yes, but you should probably tackle both cleanups as two patches
> covering the whole file for each cleanup.

I have submitted two different patches each for separate cleanups. 

Any update on those patches?

Thank you,
Bhavin Sharma