| Message ID | 1351156016-10970-1-git-send-email-s.nawrocki@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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;
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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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;