Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Thu, 24 Jun 2021 12:31:53 +0300
From:   Sakari Ailus <sakari.ailus@iki.fi>
To:     Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
Cc:     linuxarm@huawei.com, mauro.chehab@huawei.com,
        Mauro Carvalho Chehab <mchehab@kernel.org>,
        linux-kernel@vger.kernel.org, linux-media@vger.kernel.org
Subject: Re: [PATCH 3/5] media: v4l2-flash-led-class: drop an useless check
Message-ID: <20210624093153.GJ3@valkosipuli.retiisi.eu>
References: <cover.1624276137.git.mchehab+huawei@kernel.org>
 <e1629ac223470630eed4096361965d154aff70b7.1624276138.git.mchehab+huawei@kernel.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <e1629ac223470630eed4096361965d154aff70b7.1624276138.git.mchehab+huawei@kernel.org>
User-Agent: Mutt/1.10.1 (2018-07-13)
Precedence: bulk

Hi Mauro,

Could you check if your mail client could be configured not to add junk to
To: field? It often leads anything in the Cc: field being dropped.

On Mon, Jun 21, 2021 at 01:56:47PM +0200, Mauro Carvalho Chehab wrote:
> As pointed by smatch:
> 	drivers/media/v4l2-core/v4l2-flash-led-class.c:264 v4l2_flash_s_ctrl() error: we previously assumed 'fled_cdev' could be null (see line 197)
> 
> It is too late to check if fled_cdev is NULL there. If such check is
> needed, it should be, instead, inside v4l2_flash_init().
> 
> On other words, if v4l2_flash->fled_cdev() is NULL at
> v4l2_flash_s_ctrl(), all led_*() function calls inside the function
> would try to de-reference a NULL pointer, as the logic won't prevent
> it.
> 
> So, remove the useless check.
> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> ---
>  drivers/media/v4l2-core/v4l2-flash-led-class.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-flash-led-class.c b/drivers/media/v4l2-core/v4l2-flash-led-class.c
> index 10ddcc48aa17..a1653c635d82 100644
> --- a/drivers/media/v4l2-core/v4l2-flash-led-class.c
> +++ b/drivers/media/v4l2-core/v4l2-flash-led-class.c
> @@ -194,7 +194,7 @@ static int v4l2_flash_s_ctrl(struct v4l2_ctrl *c)
>  {
>  	struct v4l2_flash *v4l2_flash = v4l2_ctrl_to_v4l2_flash(c);
>  	struct led_classdev_flash *fled_cdev = v4l2_flash->fled_cdev;
> -	struct led_classdev *led_cdev = fled_cdev ? &fled_cdev->led_cdev : NULL;
> +	struct led_classdev *led_cdev = &fled_cdev->led_cdev;

fled_cdev may be NULL here. The reason is that some controls are for flash
LEDs only but the same sub-device may also control an indicator. This is
covered when the controls are created, so that the NULL pointer isn't
dereferenced.

If you wish the false positive to be addressed while also improving the
implementation, that could be done by e.g. splitting the switch into two,
the part that needs fled_cdev and another that doesn't.

I can send a patch for that.

Please also cc me to V4L2 flash class patches. I noticed this one by
accident only.

>  	struct v4l2_ctrl **ctrls = v4l2_flash->ctrls;
>  	bool external_strobe;
>  	int ret = 0;

-- 
Kind regards,

Sakari Ailus