Re: [PATCH v2 23/29] media: vimc: Release resources on media device release

[Sakari Ailus <sakari.ailus@linux.intel.com>]

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.

-- 
Regards,

Sakari Ailus