Re: [PATCH] s5p-fimc: Fix platform entities registration
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 -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] s5p-fimc: Fix platform entities registration
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.nawro...@samsung.com
Signed-off-by: Kyungmin Park kyungmin.p...@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
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] s5p-fimc: Fix platform entities registration
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 -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
