public inbox for linux-arm-kernel@lists.infradead.org
 help / color / mirror / Atom feed
From: Johan Hovold <johan@kernel.org>
To: Mark Rutland <mark.rutland@arm.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Guangshuo Li <lgs201920130244@gmail.com>,
	Will Deacon <will@kernel.org>,
	Anshuman Khandual <anshuman.khandual@arm.com>,
	linux-arm-kernel@lists.infradead.org,
	linux-perf-users@vger.kernel.org, linux-kernel@vger.kernel.org,
	stable@vger.kernel.org
Subject: Re: [PATCH] arm_pmu: acpi: fix reference leak on failed device registration
Date: Mon, 20 Apr 2026 09:28:47 +0200	[thread overview]
Message-ID: <aeXVr5enpjb3rfq7@hovoldconsulting.com> (raw)
In-Reply-To: <aeCsLy-45QyeCwGA@J2N7QTR9R3>

On Thu, Apr 16, 2026 at 10:30:23AM +0100, Mark Rutland wrote:
> On Thu, Apr 16, 2026 at 09:23:33AM +0200, Johan Hovold wrote:

> > It's not just the platform code as this directly reflects the behaviour
> > of device_register() as Mark pointed out.
> > 
> > It is indeed an unfortunate quirk of the driver model, but one can argue
> > that having a registration function that frees its argument on errors
> > would be even worse. And even more so when many (or most) users get this
> > right.
> 
> Ah, sorry; I had missed that the _put() step would actually free the
> object (and as you explain below, how that won't work for many callers).
> 
> > So if we want to change this, I think we would need to deprecate
> > device_register() in favour of explicit device_initialize() and
> > device_add().
> 
> Is is possible to have {platfom_,}device_uninitialize() functions that
> does everything except the ->release() call? If we had that, then we'd
> be able to have a flow along the lines of:
> 
> 	int some_init_function(void)
> 	{
> 		int err;
> 	
> 		platform_device_init(&static_pdev);
> 	
> 		err = platform_device_add(&static_pdev))
> 		if (err)
> 			goto out_uninit;
> 	
> 		return 0;
> 	
> 	out_uninit:
> 		platform_device_uninit(&static_pdev);
> 		return err;
> 	}
> 
> ... which I think would align with what people generally expect to have
> to do.

The issue here is that platform_device_add() allocates a device name and
such resources are not released until the last reference is dropped.

It's been this way since 2008, but some of the static platform devices
predates that and they both lack a release callback (explicitly required
since 2003) and are not cleaned up on registration failure.

Since registration would essentially only fail during development (e.g.
due to name collision or fault injection), this is hardly something to
worry about, but we could consider moving towards dynamic objects to
address both issues.

We have a few functions for allocating *and* registering platform
devices that could be used in many of these cases (and they already
clean up after themselves on errors):

	platform_device_register_simple()
	platform_device_register_data()
	platform_device_register_resndata()
	platform_device_register_full()

and where those do not fit (and cannot be extended) we have the
underlying:

	platform_device_alloc()
	platform_device_add_resources()
	platform_device_add_data()
	plaform_device_add()

But there are some 800 static platform devices left, mostly in legacy
platform code and board files that I assume few people care about.

Johan


  reply	other threads:[~2026-04-20  7:28 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-04-15 17:41 [PATCH] arm_pmu: acpi: fix reference leak on failed device registration Guangshuo Li
2026-04-15 18:19 ` Mark Rutland
2026-04-16  4:40   ` Greg Kroah-Hartman
2026-04-16  6:34     ` Guangshuo Li
2026-04-16  7:23     ` Johan Hovold
2026-04-16  8:59       ` Guangshuo Li
2026-04-16  9:50         ` Mark Rutland
2026-04-16  9:30       ` Mark Rutland
2026-04-20  7:28         ` Johan Hovold [this message]
2026-04-20  8:05           ` Greg Kroah-Hartman

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=aeXVr5enpjb3rfq7@hovoldconsulting.com \
    --to=johan@kernel.org \
    --cc=anshuman.khandual@arm.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=lgs201920130244@gmail.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-perf-users@vger.kernel.org \
    --cc=mark.rutland@arm.com \
    --cc=stable@vger.kernel.org \
    --cc=will@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox