Re: [PATCH 1/2] media: pxa_camera: don't deferenciate a NULL pointer

[Sakari Ailus <sakari.ailus@linux.intel.com>]

Hi Mauro,

On Fri, Dec 07, 2018 at 08:07:54AM -0500, Mauro Carvalho Chehab wrote:
> As warned by smatch:
> 	drivers/media/platform/pxa_camera.c:2400 pxa_camera_probe() error: we previously assumed 'pcdev->pdata' could be null (see line 2397)
> 
> It would be possible that neither DT nor platform data would be
> provided. This is a Kernel bug, so warn about that and bail.
> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>
> ---
>  drivers/media/platform/pxa_camera.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/media/platform/pxa_camera.c b/drivers/media/platform/pxa_camera.c
> index 5f930560eb30..f91f8fd424c4 100644
> --- a/drivers/media/platform/pxa_camera.c
> +++ b/drivers/media/platform/pxa_camera.c
> @@ -2396,6 +2396,9 @@ static int pxa_camera_probe(struct platform_device *pdev)
>  	pcdev->pdata = pdev->dev.platform_data;
>  	if (pdev->dev.of_node && !pcdev->pdata) {
>  		err = pxa_camera_pdata_from_dt(&pdev->dev, pcdev, &pcdev->asd);
> +	} else if (!pcdev->pdata) {

This fixes the issue, but the current checks remain a bit odd.

The driver seems to prefer platform data over OF. I wonder if that's
intentional or not.

In that case, I'd roughly write this as:

if (pcdev->pdata) {
	...;
} else if (pdev->dev.of_node) {
	...;
} else {
	return -ENODEV;
}

I'm not sure WARN_ON(1) is necessary. A lot of drivers simply do it this
way without WARN_ON().

> +		WARN_ON(1);
> +		return -ENODEV;
>  	} else {
>  		pcdev->platform_flags = pcdev->pdata->flags;
>  		pcdev->mclk = pcdev->pdata->mclk_10khz * 10000;

-- 
Regards,

Sakari Ailus
sakari.ailus@linux.intel.com