| Message ID | 20231220103713.113386-24-sakari.ailus@linux.intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Media device lifetime management | expand |
On 20/12/2023 11:37, Sakari Ailus wrote: > Release all the resources when the media device is related, moving away > form the struct v4l2_device used for that purpose. form -> from > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > --- > drivers/media/test-drivers/vimc/vimc-core.c | 15 +++++++++------ > 1 file changed, 9 insertions(+), 6 deletions(-) > > diff --git a/drivers/media/test-drivers/vimc/vimc-core.c b/drivers/media/test-drivers/vimc/vimc-core.c > index af127476e920..3e59f8c256c7 100644 > --- a/drivers/media/test-drivers/vimc/vimc-core.c > +++ b/drivers/media/test-drivers/vimc/vimc-core.c > @@ -264,13 +264,12 @@ static int vimc_add_subdevs(struct vimc_device *vimc) > return 0; > } > > -static void vimc_v4l2_dev_release(struct v4l2_device *v4l2_dev) > +static void vimc_mdev_release(struct media_device *mdev) > { > struct vimc_device *vimc = > - container_of(v4l2_dev, struct vimc_device, v4l2_dev); > + container_of_const(mdev, struct vimc_device, mdev); Why this change? > > vimc_release_subdevs(vimc); > - media_device_cleanup(&vimc->mdev); > kfree(vimc->ent_devs); > kfree(vimc); > } > @@ -336,6 +335,10 @@ static int vimc_register_devices(struct vimc_device *vimc) > return ret; > } > > +static const struct media_device_ops vimc_mdev_ops = { > + .release = vimc_mdev_release, > +}; > + > static int vimc_probe(struct platform_device *pdev) > { > const struct font_desc *font = find_font("VGA8x16"); > @@ -369,12 +372,12 @@ static int vimc_probe(struct platform_device *pdev) > snprintf(vimc->mdev.bus_info, sizeof(vimc->mdev.bus_info), > "platform:%s", VIMC_PDEV_NAME); > vimc->mdev.dev = &pdev->dev; > + vimc->mdev.ops = &vimc_mdev_ops; > media_device_init(&vimc->mdev); > > ret = vimc_register_devices(vimc); > if (ret) { > - media_device_cleanup(&vimc->mdev); > - kfree(vimc); > + media_device_put(&vimc->mdev); > return ret; > } > /* > @@ -382,7 +385,6 @@ static int vimc_probe(struct platform_device *pdev) > * if the registration fails, we release directly from probe > */ > > - vimc->v4l2_dev.release = vimc_v4l2_dev_release; > platform_set_drvdata(pdev, vimc); > return 0; > } > @@ -397,6 +399,7 @@ static void vimc_remove(struct platform_device *pdev) > media_device_unregister(&vimc->mdev); > v4l2_device_unregister(&vimc->v4l2_dev); > v4l2_device_put(&vimc->v4l2_dev); > + media_device_put(&vimc->mdev); > } > > static void vimc_dev_release(struct device *dev) Regards, Hans
On Mon, Feb 05, 2024 at 04:02:24PM +0100, Hans Verkuil wrote: > On 20/12/2023 11:37, Sakari Ailus wrote: > > Release all the resources when the media device is related, moving away s/related/released/ > > form the struct v4l2_device used for that purpose. > > form -> from Please explain *why* in the commit message. > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > --- > > drivers/media/test-drivers/vimc/vimc-core.c | 15 +++++++++------ > > 1 file changed, 9 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/media/test-drivers/vimc/vimc-core.c b/drivers/media/test-drivers/vimc/vimc-core.c > > index af127476e920..3e59f8c256c7 100644 > > --- a/drivers/media/test-drivers/vimc/vimc-core.c > > +++ b/drivers/media/test-drivers/vimc/vimc-core.c > > @@ -264,13 +264,12 @@ static int vimc_add_subdevs(struct vimc_device *vimc) > > return 0; > > } > > > > -static void vimc_v4l2_dev_release(struct v4l2_device *v4l2_dev) > > +static void vimc_mdev_release(struct media_device *mdev) > > { > > struct vimc_device *vimc = > > - container_of(v4l2_dev, struct vimc_device, v4l2_dev); > > + container_of_const(mdev, struct vimc_device, mdev); > > Why this change? > > > > > vimc_release_subdevs(vimc); > > - media_device_cleanup(&vimc->mdev); > > kfree(vimc->ent_devs); > > kfree(vimc); > > } > > @@ -336,6 +335,10 @@ static int vimc_register_devices(struct vimc_device *vimc) > > return ret; > > } > > > > +static const struct media_device_ops vimc_mdev_ops = { > > + .release = vimc_mdev_release, > > +}; > > + > > static int vimc_probe(struct platform_device *pdev) > > { > > const struct font_desc *font = find_font("VGA8x16"); > > @@ -369,12 +372,12 @@ static int vimc_probe(struct platform_device *pdev) > > snprintf(vimc->mdev.bus_info, sizeof(vimc->mdev.bus_info), > > "platform:%s", VIMC_PDEV_NAME); > > vimc->mdev.dev = &pdev->dev; > > + vimc->mdev.ops = &vimc_mdev_ops; > > media_device_init(&vimc->mdev); > > > > ret = vimc_register_devices(vimc); > > if (ret) { > > - media_device_cleanup(&vimc->mdev); > > - kfree(vimc); > > + media_device_put(&vimc->mdev); > > return ret; > > } > > /* > > @@ -382,7 +385,6 @@ static int vimc_probe(struct platform_device *pdev) > > * if the registration fails, we release directly from probe > > */ > > > > - vimc->v4l2_dev.release = vimc_v4l2_dev_release; > > platform_set_drvdata(pdev, vimc); > > return 0; > > } > > @@ -397,6 +399,7 @@ static void vimc_remove(struct platform_device *pdev) > > media_device_unregister(&vimc->mdev); > > v4l2_device_unregister(&vimc->v4l2_dev); > > v4l2_device_put(&vimc->v4l2_dev); > > + media_device_put(&vimc->mdev); > > } > > > > static void vimc_dev_release(struct device *dev)
Hi Hans, On Mon, Feb 05, 2024 at 04:02:24PM +0100, Hans Verkuil wrote: > On 20/12/2023 11:37, Sakari Ailus wrote: > > Release all the resources when the media device is related, moving away s/related/released/ > > form the struct v4l2_device used for that purpose. > > form -> from Yes. > > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > --- > > drivers/media/test-drivers/vimc/vimc-core.c | 15 +++++++++------ > > 1 file changed, 9 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/media/test-drivers/vimc/vimc-core.c b/drivers/media/test-drivers/vimc/vimc-core.c > > index af127476e920..3e59f8c256c7 100644 > > --- a/drivers/media/test-drivers/vimc/vimc-core.c > > +++ b/drivers/media/test-drivers/vimc/vimc-core.c > > @@ -264,13 +264,12 @@ static int vimc_add_subdevs(struct vimc_device *vimc) > > return 0; > > } > > > > -static void vimc_v4l2_dev_release(struct v4l2_device *v4l2_dev) > > +static void vimc_mdev_release(struct media_device *mdev) > > { > > struct vimc_device *vimc = > > - container_of(v4l2_dev, struct vimc_device, v4l2_dev); > > + container_of_const(mdev, struct vimc_device, mdev); > > Why this change? I changed the line already. There's no reason to continue using container_of() instead of container_of_const() that takes const-ness into account, too. > > > > > vimc_release_subdevs(vimc); > > - media_device_cleanup(&vimc->mdev); > > kfree(vimc->ent_devs); > > kfree(vimc); > > } > > @@ -336,6 +335,10 @@ static int vimc_register_devices(struct vimc_device *vimc) > > return ret; > > } > > > > +static const struct media_device_ops vimc_mdev_ops = { > > + .release = vimc_mdev_release, > > +}; > > + > > static int vimc_probe(struct platform_device *pdev) > > { > > const struct font_desc *font = find_font("VGA8x16"); > > @@ -369,12 +372,12 @@ static int vimc_probe(struct platform_device *pdev) > > snprintf(vimc->mdev.bus_info, sizeof(vimc->mdev.bus_info), > > "platform:%s", VIMC_PDEV_NAME); > > vimc->mdev.dev = &pdev->dev; > > + vimc->mdev.ops = &vimc_mdev_ops; > > media_device_init(&vimc->mdev); > > > > ret = vimc_register_devices(vimc); > > if (ret) { > > - media_device_cleanup(&vimc->mdev); > > - kfree(vimc); > > + media_device_put(&vimc->mdev); > > return ret; > > } > > /* > > @@ -382,7 +385,6 @@ static int vimc_probe(struct platform_device *pdev) > > * if the registration fails, we release directly from probe > > */ > > > > - vimc->v4l2_dev.release = vimc_v4l2_dev_release; > > platform_set_drvdata(pdev, vimc); > > return 0; > > } > > @@ -397,6 +399,7 @@ static void vimc_remove(struct platform_device *pdev) > > media_device_unregister(&vimc->mdev); > > v4l2_device_unregister(&vimc->v4l2_dev); > > v4l2_device_put(&vimc->v4l2_dev); > > + media_device_put(&vimc->mdev); > > } > > > > static void vimc_dev_release(struct device *dev)
Hi Laurent, On Wed, Feb 07, 2024 at 04:38:05PM +0200, Laurent Pinchart wrote: > On Mon, Feb 05, 2024 at 04:02:24PM +0100, Hans Verkuil wrote: > > On 20/12/2023 11:37, Sakari Ailus wrote: > > > Release all the resources when the media device is related, moving away > > s/related/released/ > > > > form the struct v4l2_device used for that purpose. > > > > form -> from > > Please explain *why* in the commit message. Just to show how the media device release callback is used. I'll add that to the commit message.
On Wed, Feb 21, 2024 at 10:53:43AM +0000, Sakari Ailus wrote: > Hi Hans, > > On Mon, Feb 05, 2024 at 04:02:24PM +0100, Hans Verkuil wrote: > > On 20/12/2023 11:37, Sakari Ailus wrote: > > > Release all the resources when the media device is related, moving away > > s/related/released/ > > > > form the struct v4l2_device used for that purpose. > > > > form -> from > > Yes. > > > > > > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > > --- > > > drivers/media/test-drivers/vimc/vimc-core.c | 15 +++++++++------ > > > 1 file changed, 9 insertions(+), 6 deletions(-) > > > > > > diff --git a/drivers/media/test-drivers/vimc/vimc-core.c b/drivers/media/test-drivers/vimc/vimc-core.c > > > index af127476e920..3e59f8c256c7 100644 > > > --- a/drivers/media/test-drivers/vimc/vimc-core.c > > > +++ b/drivers/media/test-drivers/vimc/vimc-core.c > > > @@ -264,13 +264,12 @@ static int vimc_add_subdevs(struct vimc_device *vimc) > > > return 0; > > > } > > > > > > -static void vimc_v4l2_dev_release(struct v4l2_device *v4l2_dev) > > > +static void vimc_mdev_release(struct media_device *mdev) > > > { > > > struct vimc_device *vimc = > > > - container_of(v4l2_dev, struct vimc_device, v4l2_dev); > > > + container_of_const(mdev, struct vimc_device, mdev); > > > > Why this change? > > I changed the line already. There's no reason to continue using > container_of() instead of container_of_const() that takes const-ness into > account, too. It should then be at least mentioned in the commit message. Any plan to switch to container_of_const() globally in the subsystem ? > > > > > > vimc_release_subdevs(vimc); > > > - media_device_cleanup(&vimc->mdev); > > > kfree(vimc->ent_devs); > > > kfree(vimc); > > > } > > > @@ -336,6 +335,10 @@ static int vimc_register_devices(struct vimc_device *vimc) > > > return ret; > > > } > > > > > > +static const struct media_device_ops vimc_mdev_ops = { > > > + .release = vimc_mdev_release, > > > +}; > > > + > > > static int vimc_probe(struct platform_device *pdev) > > > { > > > const struct font_desc *font = find_font("VGA8x16"); > > > @@ -369,12 +372,12 @@ static int vimc_probe(struct platform_device *pdev) > > > snprintf(vimc->mdev.bus_info, sizeof(vimc->mdev.bus_info), > > > "platform:%s", VIMC_PDEV_NAME); > > > vimc->mdev.dev = &pdev->dev; > > > + vimc->mdev.ops = &vimc_mdev_ops; > > > media_device_init(&vimc->mdev); > > > > > > ret = vimc_register_devices(vimc); > > > if (ret) { > > > - media_device_cleanup(&vimc->mdev); > > > - kfree(vimc); > > > + media_device_put(&vimc->mdev); > > > return ret; > > > } > > > /* > > > @@ -382,7 +385,6 @@ static int vimc_probe(struct platform_device *pdev) > > > * if the registration fails, we release directly from probe > > > */ > > > > > > - vimc->v4l2_dev.release = vimc_v4l2_dev_release; > > > platform_set_drvdata(pdev, vimc); > > > return 0; > > > } > > > @@ -397,6 +399,7 @@ static void vimc_remove(struct platform_device *pdev) > > > media_device_unregister(&vimc->mdev); > > > v4l2_device_unregister(&vimc->v4l2_dev); > > > v4l2_device_put(&vimc->v4l2_dev); > > > + media_device_put(&vimc->mdev); > > > } > > > > > > static void vimc_dev_release(struct device *dev)
On 21/02/2024 11:53, Sakari Ailus wrote: > Hi Hans, > > On Mon, Feb 05, 2024 at 04:02:24PM +0100, Hans Verkuil wrote: >> On 20/12/2023 11:37, Sakari Ailus wrote: >>> Release all the resources when the media device is related, moving away > > s/related/released/ > >>> form the struct v4l2_device used for that purpose. >> >> form -> from > > Yes. > >> >>> >>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> >>> --- >>> drivers/media/test-drivers/vimc/vimc-core.c | 15 +++++++++------ >>> 1 file changed, 9 insertions(+), 6 deletions(-) >>> >>> diff --git a/drivers/media/test-drivers/vimc/vimc-core.c b/drivers/media/test-drivers/vimc/vimc-core.c >>> index af127476e920..3e59f8c256c7 100644 >>> --- a/drivers/media/test-drivers/vimc/vimc-core.c >>> +++ b/drivers/media/test-drivers/vimc/vimc-core.c >>> @@ -264,13 +264,12 @@ static int vimc_add_subdevs(struct vimc_device *vimc) >>> return 0; >>> } >>> >>> -static void vimc_v4l2_dev_release(struct v4l2_device *v4l2_dev) >>> +static void vimc_mdev_release(struct media_device *mdev) >>> { >>> struct vimc_device *vimc = >>> - container_of(v4l2_dev, struct vimc_device, v4l2_dev); >>> + container_of_const(mdev, struct vimc_device, mdev); >> >> Why this change? > > I changed the line already. There's no reason to continue using > container_of() instead of container_of_const() that takes const-ness into > account, too. But neither vimc nor mdev can be const anyway, that would make no sense here. Regards, Hans > >> >>> >>> vimc_release_subdevs(vimc); >>> - media_device_cleanup(&vimc->mdev); >>> kfree(vimc->ent_devs); >>> kfree(vimc); >>> } >>> @@ -336,6 +335,10 @@ static int vimc_register_devices(struct vimc_device *vimc) >>> return ret; >>> } >>> >>> +static const struct media_device_ops vimc_mdev_ops = { >>> + .release = vimc_mdev_release, >>> +}; >>> + >>> static int vimc_probe(struct platform_device *pdev) >>> { >>> const struct font_desc *font = find_font("VGA8x16"); >>> @@ -369,12 +372,12 @@ static int vimc_probe(struct platform_device *pdev) >>> snprintf(vimc->mdev.bus_info, sizeof(vimc->mdev.bus_info), >>> "platform:%s", VIMC_PDEV_NAME); >>> vimc->mdev.dev = &pdev->dev; >>> + vimc->mdev.ops = &vimc_mdev_ops; >>> media_device_init(&vimc->mdev); >>> >>> ret = vimc_register_devices(vimc); >>> if (ret) { >>> - media_device_cleanup(&vimc->mdev); >>> - kfree(vimc); >>> + media_device_put(&vimc->mdev); >>> return ret; >>> } >>> /* >>> @@ -382,7 +385,6 @@ static int vimc_probe(struct platform_device *pdev) >>> * if the registration fails, we release directly from probe >>> */ >>> >>> - vimc->v4l2_dev.release = vimc_v4l2_dev_release; >>> platform_set_drvdata(pdev, vimc); >>> return 0; >>> } >>> @@ -397,6 +399,7 @@ static void vimc_remove(struct platform_device *pdev) >>> media_device_unregister(&vimc->mdev); >>> v4l2_device_unregister(&vimc->v4l2_dev); >>> v4l2_device_put(&vimc->v4l2_dev); >>> + media_device_put(&vimc->mdev); >>> } >>> >>> static void vimc_dev_release(struct device *dev) >
Hi Laurent, On Wed, Feb 21, 2024 at 01:02:22PM +0200, Laurent Pinchart wrote: > On Wed, Feb 21, 2024 at 10:53:43AM +0000, Sakari Ailus wrote: > > Hi Hans, > > > > On Mon, Feb 05, 2024 at 04:02:24PM +0100, Hans Verkuil wrote: > > > On 20/12/2023 11:37, Sakari Ailus wrote: > > > > Release all the resources when the media device is related, moving away > > > > s/related/released/ > > > > > > form the struct v4l2_device used for that purpose. > > > > > > form -> from > > > > Yes. > > > > > > > > > > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > > > --- > > > > drivers/media/test-drivers/vimc/vimc-core.c | 15 +++++++++------ > > > > 1 file changed, 9 insertions(+), 6 deletions(-) > > > > > > > > diff --git a/drivers/media/test-drivers/vimc/vimc-core.c b/drivers/media/test-drivers/vimc/vimc-core.c > > > > index af127476e920..3e59f8c256c7 100644 > > > > --- a/drivers/media/test-drivers/vimc/vimc-core.c > > > > +++ b/drivers/media/test-drivers/vimc/vimc-core.c > > > > @@ -264,13 +264,12 @@ static int vimc_add_subdevs(struct vimc_device *vimc) > > > > return 0; > > > > } > > > > > > > > -static void vimc_v4l2_dev_release(struct v4l2_device *v4l2_dev) > > > > +static void vimc_mdev_release(struct media_device *mdev) > > > > { > > > > struct vimc_device *vimc = > > > > - container_of(v4l2_dev, struct vimc_device, v4l2_dev); > > > > + container_of_const(mdev, struct vimc_device, mdev); > > > > > > Why this change? > > > > I changed the line already. There's no reason to continue using > > container_of() instead of container_of_const() that takes const-ness into > > account, too. > > It should then be at least mentioned in the commit message. I can add that. > > Any plan to switch to container_of_const() globally in the subsystem ? This should of course be done. I can post patches at some point unless someone gets there first. I can't promise a quick schedule though.
Hi Hans, On Wed, Feb 21, 2024 at 12:19:21PM +0100, Hans Verkuil wrote: > On 21/02/2024 11:53, Sakari Ailus wrote: > > Hi Hans, > > > > On Mon, Feb 05, 2024 at 04:02:24PM +0100, Hans Verkuil wrote: > >> On 20/12/2023 11:37, Sakari Ailus wrote: > >>> Release all the resources when the media device is related, moving away > > > > s/related/released/ > > > >>> form the struct v4l2_device used for that purpose. > >> > >> form -> from > > > > Yes. > > > >> > >>> > >>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > >>> --- > >>> drivers/media/test-drivers/vimc/vimc-core.c | 15 +++++++++------ > >>> 1 file changed, 9 insertions(+), 6 deletions(-) > >>> > >>> diff --git a/drivers/media/test-drivers/vimc/vimc-core.c b/drivers/media/test-drivers/vimc/vimc-core.c > >>> index af127476e920..3e59f8c256c7 100644 > >>> --- a/drivers/media/test-drivers/vimc/vimc-core.c > >>> +++ b/drivers/media/test-drivers/vimc/vimc-core.c > >>> @@ -264,13 +264,12 @@ static int vimc_add_subdevs(struct vimc_device *vimc) > >>> return 0; > >>> } > >>> > >>> -static void vimc_v4l2_dev_release(struct v4l2_device *v4l2_dev) > >>> +static void vimc_mdev_release(struct media_device *mdev) > >>> { > >>> struct vimc_device *vimc = > >>> - container_of(v4l2_dev, struct vimc_device, v4l2_dev); > >>> + container_of_const(mdev, struct vimc_device, mdev); > >> > >> Why this change? > > > > I changed the line already. There's no reason to continue using > > container_of() instead of container_of_const() that takes const-ness into > > account, too. > > But neither vimc nor mdev can be const anyway, that would make no sense > here. Neither is const, true. Yet container_of_const() is preferred over container_of(), due to the fact that it does take const-ness into account. container_of() should really be avoided. I'll add this to the commit message as Laurent suggested.
On 21/02/2024 12:40, Sakari Ailus wrote: > Hi Hans, > > On Wed, Feb 21, 2024 at 12:19:21PM +0100, Hans Verkuil wrote: >> On 21/02/2024 11:53, Sakari Ailus wrote: >>> Hi Hans, >>> >>> On Mon, Feb 05, 2024 at 04:02:24PM +0100, Hans Verkuil wrote: >>>> On 20/12/2023 11:37, Sakari Ailus wrote: >>>>> Release all the resources when the media device is related, moving away >>> >>> s/related/released/ >>> >>>>> form the struct v4l2_device used for that purpose. >>>> >>>> form -> from >>> >>> Yes. >>> >>>> >>>>> >>>>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> >>>>> --- >>>>> drivers/media/test-drivers/vimc/vimc-core.c | 15 +++++++++------ >>>>> 1 file changed, 9 insertions(+), 6 deletions(-) >>>>> >>>>> diff --git a/drivers/media/test-drivers/vimc/vimc-core.c b/drivers/media/test-drivers/vimc/vimc-core.c >>>>> index af127476e920..3e59f8c256c7 100644 >>>>> --- a/drivers/media/test-drivers/vimc/vimc-core.c >>>>> +++ b/drivers/media/test-drivers/vimc/vimc-core.c >>>>> @@ -264,13 +264,12 @@ static int vimc_add_subdevs(struct vimc_device *vimc) >>>>> return 0; >>>>> } >>>>> >>>>> -static void vimc_v4l2_dev_release(struct v4l2_device *v4l2_dev) >>>>> +static void vimc_mdev_release(struct media_device *mdev) >>>>> { >>>>> struct vimc_device *vimc = >>>>> - container_of(v4l2_dev, struct vimc_device, v4l2_dev); >>>>> + container_of_const(mdev, struct vimc_device, mdev); >>>> >>>> Why this change? >>> >>> I changed the line already. There's no reason to continue using >>> container_of() instead of container_of_const() that takes const-ness into >>> account, too. >> >> But neither vimc nor mdev can be const anyway, that would make no sense >> here. > > Neither is const, true. Yet container_of_const() is preferred over Says who? It makes sense in generic defines that use it, e.g.: drivers/base/firmware_loader/sysfs.h:#define to_fw_sysfs(__dev) container_of_const(__dev, struct fw_sysfs, dev) That way it can handle both const and non-const __dev pointers. In cases where this doesn't come into play I think there is no need to make code changes. Perhaps when writing new code it might make sense to use it, but changing it in existing code, esp. as part of a patch that deals with something else entirely, seems just unnecessary churn. I won't block this, but I recommend just dropping this change in this patch. Regards, Hans > container_of(), due to the fact that it does take const-ness into account. > container_of() should really be avoided. > > I'll add this to the commit message as Laurent suggested. >
Hi Hans, On Wed, Feb 21, 2024 at 12:48:51PM +0100, Hans Verkuil wrote: > On 21/02/2024 12:40, Sakari Ailus wrote: > > Hi Hans, > > > > On Wed, Feb 21, 2024 at 12:19:21PM +0100, Hans Verkuil wrote: > >> On 21/02/2024 11:53, Sakari Ailus wrote: > >>> Hi Hans, > >>> > >>> On Mon, Feb 05, 2024 at 04:02:24PM +0100, Hans Verkuil wrote: > >>>> On 20/12/2023 11:37, Sakari Ailus wrote: > >>>>> Release all the resources when the media device is related, moving away > >>> > >>> s/related/released/ > >>> > >>>>> form the struct v4l2_device used for that purpose. > >>>> > >>>> form -> from > >>> > >>> Yes. > >>> > >>>> > >>>>> > >>>>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > >>>>> --- > >>>>> drivers/media/test-drivers/vimc/vimc-core.c | 15 +++++++++------ > >>>>> 1 file changed, 9 insertions(+), 6 deletions(-) > >>>>> > >>>>> diff --git a/drivers/media/test-drivers/vimc/vimc-core.c b/drivers/media/test-drivers/vimc/vimc-core.c > >>>>> index af127476e920..3e59f8c256c7 100644 > >>>>> --- a/drivers/media/test-drivers/vimc/vimc-core.c > >>>>> +++ b/drivers/media/test-drivers/vimc/vimc-core.c > >>>>> @@ -264,13 +264,12 @@ static int vimc_add_subdevs(struct vimc_device *vimc) > >>>>> return 0; > >>>>> } > >>>>> > >>>>> -static void vimc_v4l2_dev_release(struct v4l2_device *v4l2_dev) > >>>>> +static void vimc_mdev_release(struct media_device *mdev) > >>>>> { > >>>>> struct vimc_device *vimc = > >>>>> - container_of(v4l2_dev, struct vimc_device, v4l2_dev); > >>>>> + container_of_const(mdev, struct vimc_device, mdev); > >>>> > >>>> Why this change? > >>> > >>> I changed the line already. There's no reason to continue using > >>> container_of() instead of container_of_const() that takes const-ness into > >>> account, too. > >> > >> But neither vimc nor mdev can be const anyway, that would make no sense > >> here. > > > > Neither is const, true. Yet container_of_const() is preferred over > > Says who? container_of() documentation comes with a big, fat warning on this issue. I can post a patch to add an explicit recommentation, too. > > It makes sense in generic defines that use it, e.g.: > > drivers/base/firmware_loader/sysfs.h:#define to_fw_sysfs(__dev) container_of_const(__dev, struct fw_sysfs, dev) > > That way it can handle both const and non-const __dev pointers. > > In cases where this doesn't come into play I think there is no need to > make code changes. Perhaps when writing new code it might make sense to > use it, but changing it in existing code, esp. as part of a patch that > deals with something else entirely, seems just unnecessary churn. > > I won't block this, but I recommend just dropping this change in this patch.
diff --git a/drivers/media/test-drivers/vimc/vimc-core.c b/drivers/media/test-drivers/vimc/vimc-core.c index af127476e920..3e59f8c256c7 100644 --- a/drivers/media/test-drivers/vimc/vimc-core.c +++ b/drivers/media/test-drivers/vimc/vimc-core.c @@ -264,13 +264,12 @@ static int vimc_add_subdevs(struct vimc_device *vimc) return 0; } -static void vimc_v4l2_dev_release(struct v4l2_device *v4l2_dev) +static void vimc_mdev_release(struct media_device *mdev) { struct vimc_device *vimc = - container_of(v4l2_dev, struct vimc_device, v4l2_dev); + container_of_const(mdev, struct vimc_device, mdev); vimc_release_subdevs(vimc); - media_device_cleanup(&vimc->mdev); kfree(vimc->ent_devs); kfree(vimc); } @@ -336,6 +335,10 @@ static int vimc_register_devices(struct vimc_device *vimc) return ret; } +static const struct media_device_ops vimc_mdev_ops = { + .release = vimc_mdev_release, +}; + static int vimc_probe(struct platform_device *pdev) { const struct font_desc *font = find_font("VGA8x16"); @@ -369,12 +372,12 @@ static int vimc_probe(struct platform_device *pdev) snprintf(vimc->mdev.bus_info, sizeof(vimc->mdev.bus_info), "platform:%s", VIMC_PDEV_NAME); vimc->mdev.dev = &pdev->dev; + vimc->mdev.ops = &vimc_mdev_ops; media_device_init(&vimc->mdev); ret = vimc_register_devices(vimc); if (ret) { - media_device_cleanup(&vimc->mdev); - kfree(vimc); + media_device_put(&vimc->mdev); return ret; } /* @@ -382,7 +385,6 @@ static int vimc_probe(struct platform_device *pdev) * if the registration fails, we release directly from probe */ - vimc->v4l2_dev.release = vimc_v4l2_dev_release; platform_set_drvdata(pdev, vimc); return 0; } @@ -397,6 +399,7 @@ static void vimc_remove(struct platform_device *pdev) media_device_unregister(&vimc->mdev); v4l2_device_unregister(&vimc->v4l2_dev); v4l2_device_put(&vimc->v4l2_dev); + media_device_put(&vimc->mdev); } static void vimc_dev_release(struct device *dev)
Release all the resources when the media device is related, moving away form the struct v4l2_device used for that purpose. Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> --- drivers/media/test-drivers/vimc/vimc-core.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-)