[PATCH] s5p-fimc: Fix platform entities registration

[PATCH] s5p-fimc: Fix platform entities registration

[Sylwester Nawrocki]

Make sure there is no v4l2_device_unregister_subdev() call
on a subdev which wasn't registered.

Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/media/platform/s5p-fimc/fimc-mdevice.c |   25 ++++++++++++------------
 1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/drivers/media/platform/s5p-fimc/fimc-mdevice.c b/drivers/media/platform/s5p-fimc/fimc-mdevice.c
index 715b258..a69f053 100644
--- a/drivers/media/platform/s5p-fimc/fimc-mdevice.c
+++ b/drivers/media/platform/s5p-fimc/fimc-mdevice.c
@@ -345,24 +345,23 @@ static int fimc_register_callback(struct device *dev, void *p)
 	struct fimc_dev *fimc = dev_get_drvdata(dev);
 	struct v4l2_subdev *sd = &fimc->vid_cap.subdev;
 	struct fimc_md *fmd = p;
-	int ret = 0;
+	int ret;
 
-	if (!fimc || !fimc->pdev)
+	if (fimc == NULL)
 		return 0;
 
-	if (fimc->pdev->id < 0 || fimc->pdev->id >= FIMC_MAX_DEVS)
+	if (fimc->id >= FIMC_MAX_DEVS)
 		return 0;
 
 	fimc->pipeline_ops = &fimc_pipeline_ops;
-	fmd->fimc[fimc->pdev->id] = fimc;
 	sd->grp_id = FIMC_GROUP_ID;
 
 	ret = v4l2_device_register_subdev(&fmd->v4l2_dev, sd);
-	if (ret) {
+	if (!ret)
+		fmd->fimc[fimc->id] = fimc;
+	else
 		v4l2_err(&fmd->v4l2_dev, "Failed to register FIMC.%d (%d)\n",
 			 fimc->id, ret);
-	}
-
 	return ret;
 }
 
@@ -380,15 +379,15 @@ static int fimc_lite_register_callback(struct device *dev, void *p)
 		return 0;
 
 	fimc->pipeline_ops = &fimc_pipeline_ops;
-	fmd->fimc_lite[fimc->index] = fimc;
 	sd->grp_id = FLITE_GROUP_ID;
 
 	ret = v4l2_device_register_subdev(&fmd->v4l2_dev, sd);
-	if (ret) {
+	if (!ret)
+		fmd->fimc_lite[fimc->index] = fimc;
+	else
 		v4l2_err(&fmd->v4l2_dev,
 			 "Failed to register FIMC-LITE.%d (%d)\n",
 			 fimc->index, ret);
-	}
 	return ret;
 }
 
@@ -407,10 +406,12 @@ static int csis_register_callback(struct device *dev, void *p)
 	v4l2_info(sd, "csis%d sd: %s\n", pdev->id, sd->name);
 
 	id = pdev->id < 0 ? 0 : pdev->id;
-	fmd->csis[id].sd = sd;
+
 	sd->grp_id = CSIS_GROUP_ID;
 	ret = v4l2_device_register_subdev(&fmd->v4l2_dev, sd);
-	if (ret)
+	if (!ret)
+		fmd->csis[id].sd = sd;
+	else
 		v4l2_err(&fmd->v4l2_dev,
 			 "Failed to register CSIS subdevice: %d\n", ret);
 	return ret;
-- 
1.7.9.5

Re: [PATCH] s5p-fimc: Fix platform entities registration

[Laurent Pinchart]

Hi Sylwester,

On Thursday 25 October 2012 11:06:56 Sylwester Nawrocki wrote:
> Make sure there is no v4l2_device_unregister_subdev() call
> on a subdev which wasn't registered.

I'm not implying that this fix is bad, but doesn't the V4L2 core already 
handle this ? v4l2_device_unregister_subdev() returns immediately without 
doing anything if the subdev hasn't been registered.

> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
>  drivers/media/platform/s5p-fimc/fimc-mdevice.c |   25  ++++++++++----------
>  1 file changed, 13 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/media/platform/s5p-fimc/fimc-mdevice.c
> b/drivers/media/platform/s5p-fimc/fimc-mdevice.c index 715b258..a69f053
> 100644
> --- a/drivers/media/platform/s5p-fimc/fimc-mdevice.c
> +++ b/drivers/media/platform/s5p-fimc/fimc-mdevice.c
> @@ -345,24 +345,23 @@ static int fimc_register_callback(struct device *dev,
> void *p) struct fimc_dev *fimc = dev_get_drvdata(dev);
>  	struct v4l2_subdev *sd = &fimc->vid_cap.subdev;
>  	struct fimc_md *fmd = p;
> -	int ret = 0;
> +	int ret;
> 
> -	if (!fimc || !fimc->pdev)
> +	if (fimc == NULL)
>  		return 0;
> 
> -	if (fimc->pdev->id < 0 || fimc->pdev->id >= FIMC_MAX_DEVS)
> +	if (fimc->id >= FIMC_MAX_DEVS)
>  		return 0;
> 
>  	fimc->pipeline_ops = &fimc_pipeline_ops;
> -	fmd->fimc[fimc->pdev->id] = fimc;
>  	sd->grp_id = FIMC_GROUP_ID;
> 
>  	ret = v4l2_device_register_subdev(&fmd->v4l2_dev, sd);
> -	if (ret) {
> +	if (!ret)
> +		fmd->fimc[fimc->id] = fimc;
> +	else
>  		v4l2_err(&fmd->v4l2_dev, "Failed to register FIMC.%d (%d)\n",
>  			 fimc->id, ret);
> -	}
> -
>  	return ret;
>  }
> 
> @@ -380,15 +379,15 @@ static int fimc_lite_register_callback(struct device
> *dev, void *p) return 0;
> 
>  	fimc->pipeline_ops = &fimc_pipeline_ops;
> -	fmd->fimc_lite[fimc->index] = fimc;
>  	sd->grp_id = FLITE_GROUP_ID;
> 
>  	ret = v4l2_device_register_subdev(&fmd->v4l2_dev, sd);
> -	if (ret) {
> +	if (!ret)
> +		fmd->fimc_lite[fimc->index] = fimc;
> +	else
>  		v4l2_err(&fmd->v4l2_dev,
>  			 "Failed to register FIMC-LITE.%d (%d)\n",
>  			 fimc->index, ret);
> -	}
>  	return ret;
>  }
> 
> @@ -407,10 +406,12 @@ static int csis_register_callback(struct device *dev,
> void *p) v4l2_info(sd, "csis%d sd: %s\n", pdev->id, sd->name);
> 
>  	id = pdev->id < 0 ? 0 : pdev->id;
> -	fmd->csis[id].sd = sd;
> +
>  	sd->grp_id = CSIS_GROUP_ID;
>  	ret = v4l2_device_register_subdev(&fmd->v4l2_dev, sd);
> -	if (ret)
> +	if (!ret)
> +		fmd->csis[id].sd = sd;
> +	else
>  		v4l2_err(&fmd->v4l2_dev,
>  			 "Failed to register CSIS subdevice: %d\n", ret);
>  	return ret;
-- 
Regards,

Laurent Pinchart

Re: [PATCH] s5p-fimc: Fix platform entities registration

[Sylwester Nawrocki]

Hi Laurent,

On 10/25/2012 01:35 PM, Laurent Pinchart wrote:
> On Thursday 25 October 2012 11:06:56 Sylwester Nawrocki wrote:
>> Make sure there is no v4l2_device_unregister_subdev() call
>> on a subdev which wasn't registered.
> 
> I'm not implying that this fix is bad, but doesn't the V4L2 core already 
> handle this ? v4l2_device_unregister_subdev() returns immediately without 
> doing anything if the subdev hasn't been registered.

Indeed, the patch summary might be a bit misleading and incomplete.
I of course wanted to make sure the platform subdevs are not treated
as registered when any part of v4l2_device_register_subdev() fails.


Looking at function v4l2_device_register_subdev(), I'm wondering whether
line
 159         sd->v4l2_dev = v4l2_dev;

shouldn't be moved right before

 190         spin_lock(&v4l2_dev->lock);

so sd->v4l2_dev is set only if we return 0 in this function ?

Since in function v4l2_device_unregister_subdev() there is a check like

 259         /* return if it isn't registered */
 260         if (sd == NULL || sd->v4l2_dev == NULL)
 261                 return;

i.e. if subdev is not really registered, e.g. internal .registered
op fails, it should be NULL.

In my case sd wasn't null since this structure was embedded in
other one.

--

Thanks,
Sylwester

[PATCH] s5p-fimc: Fix platform entities registration

[Sylwester Nawrocki]

Make sure the platform sub-devices are registered to the media
device driver only when v4l2_device_unregister_subdev() succeeds.

Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/media/platform/s5p-fimc/fimc-mdevice.c |   33 ++++++++++++------------
 1 file changed, 16 insertions(+), 17 deletions(-)

diff --git a/drivers/media/platform/s5p-fimc/fimc-mdevice.c b/drivers/media/platform/s5p-fimc/fimc-mdevice.c
index 715b258..0ffde5b 100644
--- a/drivers/media/platform/s5p-fimc/fimc-mdevice.c
+++ b/drivers/media/platform/s5p-fimc/fimc-mdevice.c
@@ -345,25 +345,23 @@ static int fimc_register_callback(struct device *dev, void *p)
 	struct fimc_dev *fimc = dev_get_drvdata(dev);
 	struct v4l2_subdev *sd = &fimc->vid_cap.subdev;
 	struct fimc_md *fmd = p;
-	int ret = 0;
-
-	if (!fimc || !fimc->pdev)
-		return 0;
+	int ret;
 
-	if (fimc->pdev->id < 0 || fimc->pdev->id >= FIMC_MAX_DEVS)
+	if (fimc == NULL || fimc->id >= FIMC_MAX_DEVS)
 		return 0;
 
-	fimc->pipeline_ops = &fimc_pipeline_ops;
-	fmd->fimc[fimc->pdev->id] = fimc;
 	sd->grp_id = FIMC_GROUP_ID;
 
 	ret = v4l2_device_register_subdev(&fmd->v4l2_dev, sd);
 	if (ret) {
 		v4l2_err(&fmd->v4l2_dev, "Failed to register FIMC.%d (%d)\n",
 			 fimc->id, ret);
+		return ret;
 	}
 
-	return ret;
+	fimc->pipeline_ops = &fimc_pipeline_ops;
+	fmd->fimc[fimc->id] = fimc;
+	return 0;
 }
 
 static int fimc_lite_register_callback(struct device *dev, void *p)
@@ -373,14 +371,9 @@ static int fimc_lite_register_callback(struct device *dev, void *p)
 	struct fimc_md *fmd = p;
 	int ret;
 
-	if (fimc == NULL)
+	if (fimc == NULL || fimc->index >= FIMC_LITE_MAX_DEVS)
 		return 0;
 
-	if (fimc->index >= FIMC_LITE_MAX_DEVS)
-		return 0;
-
-	fimc->pipeline_ops = &fimc_pipeline_ops;
-	fmd->fimc_lite[fimc->index] = fimc;
 	sd->grp_id = FLITE_GROUP_ID;
 
 	ret = v4l2_device_register_subdev(&fmd->v4l2_dev, sd);
@@ -388,8 +381,12 @@ static int fimc_lite_register_callback(struct device *dev, void *p)
 		v4l2_err(&fmd->v4l2_dev,
 			 "Failed to register FIMC-LITE.%d (%d)\n",
 			 fimc->index, ret);
+		return ret;
 	}
-	return ret;
+
+	fimc->pipeline_ops = &fimc_pipeline_ops;
+	fmd->fimc_lite[fimc->index] = fimc;
+	return 0;
 }
 
 static int csis_register_callback(struct device *dev, void *p)
@@ -407,10 +404,12 @@ static int csis_register_callback(struct device *dev, void *p)
 	v4l2_info(sd, "csis%d sd: %s\n", pdev->id, sd->name);
 
 	id = pdev->id < 0 ? 0 : pdev->id;
-	fmd->csis[id].sd = sd;
 	sd->grp_id = CSIS_GROUP_ID;
+
 	ret = v4l2_device_register_subdev(&fmd->v4l2_dev, sd);
-	if (ret)
+	if (!ret)
+		fmd->csis[id].sd = sd;
+	else
 		v4l2_err(&fmd->v4l2_dev,
 			 "Failed to register CSIS subdevice: %d\n", ret);
 	return ret;
-- 
1.7.9.5

Re: [PATCH] s5p-fimc: Fix platform entities registration

[Sylwester Nawrocki]

On 10/25/2012 02:42 PM, Sylwester Nawrocki wrote:
> Hi Laurent,
> 
> On 10/25/2012 01:35 PM, Laurent Pinchart wrote:
>> On Thursday 25 October 2012 11:06:56 Sylwester Nawrocki wrote:
>>> Make sure there is no v4l2_device_unregister_subdev() call
>>> on a subdev which wasn't registered.
>>
>> I'm not implying that this fix is bad, but doesn't the V4L2 core already
>> handle this ? v4l2_device_unregister_subdev() returns immediately without
>> doing anything if the subdev hasn't been registered.
> 
> Indeed, the patch summary might be a bit misleading and incomplete.
> I of course wanted to make sure the platform subdevs are not treated
> as registered when any part of v4l2_device_register_subdev() fails.
> 
> 
> Looking at function v4l2_device_register_subdev(), I'm wondering whether
> line
>   159         sd->v4l2_dev = v4l2_dev;
> 
> shouldn't be moved right before
> 
>   190         spin_lock(&v4l2_dev->lock);
> 
> so sd->v4l2_dev is set only if we return 0 in this function ?

Hmm, no, that would be wrong. Since sd->v4l2_dev needs to be initialized
for sd->internal_ops->registered() and sd->internal_ops->registered() ops.

Still, it is possible that a subdev has the v4l2_dev field initialized and 
is not added to the v4l2_device list of subdevs (v4l2_dev->subdevs). Then 
function v4l2_device_unregister_subdev() checks for valid sd->v4l2_dev and
attempts to remove (not yet added) subdev from v4l2_dev->subdevs.

This subdev (un)registration code seems buggy, unless I'm missing something...

> Since in function v4l2_device_unregister_subdev() there is a check like
> 
>   259         /* return if it isn't registered */
>   260         if (sd == NULL || sd->v4l2_dev == NULL)
>   261                 return;
> 
> i.e. if subdev is not really registered, e.g. internal .registered
> op fails, it should be NULL.
> 
> In my case sd wasn't null since this structure was embedded in
> other one.

--
Regards,
Sylwester