linux-kernel - Re: [PATCH 1/4] media: nuvoton: Fix reference handling of ece

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CANiDSCsMCSJMEsY3R=pnZ4XUTiEYuPz-N1kEX7y13yTzE6Dm5w@mail.gmail.com>
Date: Sun, 23 Feb 2025 19:34:30 +0100
From: Ricardo Ribalda <ribalda@...omium.org>
To: Hans Verkuil <hverkuil@...all.nl>
Cc: Joseph Liu <kwliu@...oton.com>, Marvin Lin <kflin@...oton.com>, 
	Mauro Carvalho Chehab <mchehab@...nel.org>, Philipp Zabel <p.zabel@...gutronix.de>, 
	Marvin Lin <milkfafa@...il.com>, linux-media@...r.kernel.org, openbmc@...ts.ozlabs.org, 
	linux-kernel@...r.kernel.org, stable@...r.kernel.org
Subject: Re: [PATCH 1/4] media: nuvoton: Fix reference handling of ece_pdev

On Fri, 21 Feb 2025 at 10:18, Hans Verkuil <hverkuil@...all.nl> wrote:
>
> On 21/02/2025 10:04, Hans Verkuil wrote:
> > Hi Ricardo,
> >
> > On 21/01/2025 22:14, Ricardo Ribalda wrote:
> >> When we obtain a reference to of a platform_device, we need to release
> >> it via put_device.
> >>
> >> Found by cocci:
> >> ./platform/nuvoton/npcm-video.c:1677:3-9: ERROR: missing put_device; call of_find_device_by_node on line 1667, but without a corresponding object release within this function.
> >> ./platform/nuvoton/npcm-video.c:1684:3-9: ERROR: missing put_device; call of_find_device_by_node on line 1667, but without a corresponding object release within this function.
> >> ./platform/nuvoton/npcm-video.c:1690:3-9: ERROR: missing put_device; call of_find_device_by_node on line 1667, but without a corresponding object release within this function.
> >> ./platform/nuvoton/npcm-video.c:1694:1-7: ERROR: missing put_device; call of_find_device_by_node on line 1667, but without a corresponding object release within this function.
> >
> > This driver uses this construct:
> >
> >                 struct device *ece_dev __free(put_device) = &ece_pdev->dev;
> >
> > to automatically call put_device. So this patch would 'put' the device twice.
> >
> > Does cocci understand constructs like this? If I hadn't looked closely at the
> > code first, I would just have merged it.
>
> Oh wait, now that I am reading the following patches I see that it was those later
> patches that add the __free code.
>
> This is far too confusing. Please post a v2 that just combines the 'fix references'
> and 'use cleanup.h macros' in a single patch. It makes no sense to have this two-phase
> approach.

I believe this is discouraged.

cleanup.h macros does not exist in old kernel versions, so makes it
impossible to backport the fix to them.

This is an example of other series following this policy:
https://lore.kernel.org/lkml/173608125422.1253657.3732758016133408588.stgit@devnote2/

They also mention the same here:
https://hackerbikepacker.com/kernel-auto-cleanup-1 .... I am pretty
sure that I read the policy in a more official location... but I
cannot find it right now :)


>
> Regards,
>
>         Hans
>
> >
> > Regards,
> >
> >       Hans
> >
> >>
> >> Cc: stable@...r.kernel.org
> >> Fixes: 46c15a4ff1f4 ("media: nuvoton: Add driver for NPCM video capture and encoding engine")
> >> Signed-off-by: Ricardo Ribalda <ribalda@...omium.org>
> >> ---
> >>  drivers/media/platform/nuvoton/npcm-video.c | 3 +++
> >>  1 file changed, 3 insertions(+)
> >>
> >> diff --git a/drivers/media/platform/nuvoton/npcm-video.c b/drivers/media/platform/nuvoton/npcm-video.c
> >> index 024cd8ee1709..7b4c23dbe709 100644
> >> --- a/drivers/media/platform/nuvoton/npcm-video.c
> >> +++ b/drivers/media/platform/nuvoton/npcm-video.c
> >> @@ -1673,6 +1673,7 @@ static int npcm_video_ece_init(struct npcm_video *video)
> >>
> >>              regs = devm_platform_ioremap_resource(ece_pdev, 0);
> >>              if (IS_ERR(regs)) {
> >> +                    put_device(&ece_pdev->dev);
> >>                      dev_err(dev, "Failed to parse ECE reg in DTS\n");
> >>                      return PTR_ERR(regs);
> >>              }
> >> @@ -1680,11 +1681,13 @@ static int npcm_video_ece_init(struct npcm_video *video)
> >>              video->ece.regmap = devm_regmap_init_mmio(dev, regs,
> >>                                                        &npcm_video_ece_regmap_cfg);
> >>              if (IS_ERR(video->ece.regmap)) {
> >> +                    put_device(&ece_pdev->dev);
> >>                      dev_err(dev, "Failed to initialize ECE regmap\n");
> >>                      return PTR_ERR(video->ece.regmap);
> >>              }
> >>
> >>              video->ece.reset = devm_reset_control_get(&ece_pdev->dev, NULL);
> >> +            put_device(&ece_pdev->dev);
> >>              if (IS_ERR(video->ece.reset)) {
> >>                      dev_err(dev, "Failed to get ECE reset control in DTS\n");
> >>                      return PTR_ERR(video->ece.reset);
> >>
> >
> >
>


-- 
Ricardo Ribalda