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

Message ID aa54ca91f2310ecea413daa289ab882cf9f37245.1544188058.git.mchehab+samsung@kernel.org (mailing list archive)
State Superseded, archived
Delegated to: Mauro Carvalho Chehab
Headers

Commit Message

Mauro Carvalho Chehab Dec. 7, 2018, 1:07 p.m. UTC
  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(+)
  

Comments

Sakari Ailus Dec. 7, 2018, 9:11 p.m. UTC | #1
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;
  

Patch

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) {
+		WARN_ON(1);
+		return -ENODEV;
 	} else {
 		pcdev->platform_flags = pcdev->pdata->flags;
 		pcdev->mclk = pcdev->pdata->mclk_10khz * 10000;