Message ID | 20231220103713.113386-24-sakari.ailus@linux.intel.com (mailing list archive) |
---|---|
State | Changes Requested |
Delegated to: | Sakari Ailus |
Headers |
Received: from ny.mirrors.kernel.org ([147.75.199.223]) by www.linuxtv.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from <linux-media+bounces-2784-patchwork=linuxtv.org@vger.kernel.org>) id 1rFtyY-00H4GJ-P2 for patchwork@linuxtv.org; Wed, 20 Dec 2023 10:38:55 +0000 Received: from smtp.subspace.kernel.org (wormhole.subspace.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ny.mirrors.kernel.org (Postfix) with ESMTPS id B8B091C21346 for <patchwork@linuxtv.org>; Wed, 20 Dec 2023 10:38:51 +0000 (UTC) Received: from localhost.localdomain (localhost.localdomain [127.0.0.1]) by smtp.subspace.kernel.org (Postfix) with ESMTP id E676522096; Wed, 20 Dec 2023 10:37:33 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=intel.com header.i=@intel.com header.b="HI98N/Yv" X-Original-To: linux-media@vger.kernel.org Received: from mgamail.intel.com (mgamail.intel.com [198.175.65.10]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 6559E208C9 for <linux-media@vger.kernel.org>; Wed, 20 Dec 2023 10:37:32 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linux.intel.com Authentication-Results: smtp.subspace.kernel.org; spf=none smtp.mailfrom=linux.intel.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1703068652; x=1734604652; h=from:to:cc:subject:date:message-id:in-reply-to: references:mime-version:content-transfer-encoding; bh=4vQ1P4LGqxbJ7uxsk6Jqsh18rVWDSKFSxKOC9Nf9ryM=; b=HI98N/YvWPaOa9C+P6JpTbs4iASLPFA0kd0WTBX+TF5c3DBCqF6n9s/M GfQ4KQHBbyikDH6wDjm9AqY9ci3LbyG3kgqABfpDMwFe1Np1mrpLIIr/T ihjRMJOq14HOEr39vxH+AeXbP0AA17lFvg/CfSJ0GAkgikp22mmK1J5DE qtdu6DdeNZo9Bv3G+S1SfDiqbs5Mm3/fdAMrYO5FHLzw55TmR6qWILN2G lKh6hpcm8GgLtsVWyIIELoB2clwGRI8rKEOe2be+8dFJ12oqbnuytK8VP KyD/Lp+7CxtpX60HblJ5efM3ID/yO5U5Y3Us3s8ufqls0RNCozBglwfKf g==; X-IronPort-AV: E=McAfee;i="6600,9927,10929"; a="9174392" X-IronPort-AV: E=Sophos;i="6.04,291,1695711600"; d="scan'208";a="9174392" Received: from orsmga007.jf.intel.com ([10.7.209.58]) by orvoesa102.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 20 Dec 2023 02:37:32 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6600,9927,10929"; a="769544274" X-IronPort-AV: E=Sophos;i="6.04,291,1695711600"; d="scan'208";a="769544274" Received: from turnipsi.fi.intel.com (HELO kekkonen.fi.intel.com) ([10.237.72.44]) by orsmga007-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 20 Dec 2023 02:37:31 -0800 Received: from svinhufvud.ger.corp.intel.com (localhost [IPv6:::1]) by kekkonen.fi.intel.com (Postfix) with ESMTP id 7922D11FC49; Wed, 20 Dec 2023 12:37:28 +0200 (EET) From: Sakari Ailus <sakari.ailus@linux.intel.com> To: linux-media@vger.kernel.org Cc: laurent.pinchart@ideasonboard.com, Hans Verkuil <hverkuil-cisco@xs4all.nl> Subject: [PATCH v2 23/29] media: vimc: Release resources on media device release Date: Wed, 20 Dec 2023 12:37:07 +0200 Message-Id: <20231220103713.113386-24-sakari.ailus@linux.intel.com> X-Mailer: git-send-email 2.39.2 In-Reply-To: <20231220103713.113386-1-sakari.ailus@linux.intel.com> References: <20231220103713.113386-1-sakari.ailus@linux.intel.com> Precedence: bulk X-Mailing-List: linux-media@vger.kernel.org List-Id: <linux-media.vger.kernel.org> List-Subscribe: <mailto:linux-media+subscribe@vger.kernel.org> List-Unsubscribe: <mailto:linux-media+unsubscribe@vger.kernel.org> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-LSpam-Score: -4.7 (----) X-LSpam-Report: No, score=-4.7 required=5.0 tests=BAYES_00=-1.9,DKIMWL_WL_HIGH=0.001,DKIM_SIGNED=0.1,DKIM_VALID=-0.1,HEADER_FROM_DIFFERENT_DOMAINS=0.5,MAILING_LIST_MULTI=-1,RCVD_IN_DNSWL_MED=-2.3 autolearn=ham autolearn_force=no |
Series |
Media device lifetime management
|
|
Commit Message
Sakari Ailus
Dec. 20, 2023, 10:37 a.m. UTC
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(-)
Comments
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)