Re: [PATCH] ACPI: AGDI: Improve error reporting for problems during .remove()

From: Will Deacon <will@kernel.org>
To: Lorenzo Pieralisi <lpieralisi@kernel.org>
Cc: "James Morse" <james.morse@arm.com>,
	"Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>,
	"Hanjun Guo" <guohanjun@huawei.com>,
	"Sudeep Holla" <sudeep.holla@arm.com>,
	"Rafael J. Wysocki" <rafael@kernel.org>,
	"Len Brown" <lenb@kernel.org>,
	linux-acpi@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
	kernel@pengutronix.de, catalin.marinas@arm.com
Subject: Re: [PATCH] ACPI: AGDI: Improve error reporting for problems during .remove()
Date: Thu, 13 Apr 2023 15:48:03 +0100	[thread overview]
Message-ID: <20230413144802.GB26421@willie-the-truck> (raw)
In-Reply-To: <ZDe8FpBlHRbQkS7m@lpieralisi>

On Thu, Apr 13, 2023 at 10:23:50AM +0200, Lorenzo Pieralisi wrote:
> [+Catalin, Will: ACPI arm64 changes are sent through arm64 tree]
> 
> On Wed, Oct 26, 2022 at 05:09:40PM +0100, James Morse wrote:
> > Hi guys,
> > 
> > On 18/10/2022 10:35, Lorenzo Pieralisi wrote:
> > > On Fri, Oct 14, 2022 at 06:06:23PM +0200, Uwe Kleine-Kï¿½nig wrote:
> > >> Returning an error value in a platform driver's remove callback results in
> > >> a generic error message being emitted by the driver core, but otherwise it
> > >> doesn't make a difference. The device goes away anyhow.
> > >>
> > >> So instead of triggering the generic platform error message, emit a more
> > >> helpful message if a problem occurs and return 0 to suppress the generic
> > >> message.
> > >>
> > >> This patch is a preparation for making platform remove callbacks return
> > >> void.
> > > 
> > > If that's the plan - I don't have anything against this patch.
> > > 
> > >> Signed-off-by: Uwe Kleine-Kï¿½nig <u.kleine-koenig@pengutronix.de>
> > >> ---
> > >> Hello,
> > >>
> > >> note that in the situations where the driver returned an error before
> > >> and now emits a message, there is a resource leak. Someone who knows
> > >> more about this driver and maybe even can test stuff, might want to
> > >> address this. This might not only be about non-freed memory, the device
> > >> disappears but it is kept in sdei_list and so might be used after being
> > >> gone.
> > 
> > > I'd need James' input on this. I guess we may ignore
> > > sdei_event_disable() return value and continue anyway in agdi_remove(),
> > > whether that's the right thing to do it is a different question.
> > 
> > The unregister stuff is allowed to fail if the event is 'in progress' on another CPU.
> > Given the handler panic()s the machine, if an event is in progress, the resource leak
> > isn't something worth worrying about. The real problem is that the handler code may be
> > free()d while another CPU is still executing it, which is only a problem for modules.
> > 
> > As this thing can't be built as a module, and the handler panic()s the machine, I don't
> > think there is going to be a problem here.
> 
> Thanks James, I think though that's something we may want to handle in a
> separate patch.
> 
> This one looks fine to merge to me:
> 
> Reviewed-by: Lorenzo Pieralisi <lpieralisi@kernel.org>

Cheers, Lorenzo. I'll pick this one up.

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel