linux-kernel - Re: [PATCH 5/5] arm: omap: Proper cleanups for omap

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-next>] [day] [month] [year] [list]

Date:	Tue, 6 Aug 2013 17:33:56 +0800
From:	Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To:	Pantelis Antoniou <panto@...oniou-consulting.com>
Cc:	Tony Lindgren <tony@...mide.com>,
	Russell King <linux@....linux.org.uk>,
	Benoît Coussno <b-cousson@...com>,
	Paul Walmsley <paul@...an.com>,
	Sourav Poddar <sourav.poddar@...com>,
	Russ Dill <Russ.Dill@...com>, Felipe Balbi <balbi@...com>,
	Koen Kooi <koen@...cuitco.com>, linux-omap@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 5/5] arm: omap: Proper cleanups for omap_device

On Tue, Aug 06, 2013 at 10:53:44AM +0300, Pantelis Antoniou wrote:
> Removing any omap device always resulted in a crash; turns out
> BUS_NOTIFY_DEL_DEVICE is not the last notifier event sent in the
> course of removing the device, the correct event is
> BUS_NOTIFY_UNBOUND_DRIVER, which still is not the right place to
> perform the cleanup. A device callback handles that properly, as
> well as making sure the hwmods of the device are shutdown.
> 
> Signed-off-by: Pantelis Antoniou <panto@...oniou-consulting.com>
> ---
>  arch/arm/mach-omap2/omap_device.c | 34 ++++++++++++++++++++++++++++++++--
>  1 file changed, 32 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/mach-omap2/omap_device.c b/arch/arm/mach-omap2/omap_device.c
> index f33b40c..6dec521 100644
> --- a/arch/arm/mach-omap2/omap_device.c
> +++ b/arch/arm/mach-omap2/omap_device.c
> @@ -178,6 +178,32 @@ odbfd_exit:
>  	return ret;
>  }
>  
> +static void _omap_device_cleanup(struct device *dev)
> +{
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct omap_device *od;
> +	struct omap_hwmod *oh;
> +	int i;
> +
> +	od = pdev->archdata.od;
> +	if (!od)
> +		return;
> +
> +	for (i = 0; i < od->hwmods_cnt; i++) {
> +
> +		oh = od->hwmods[i];
> +
> +		/* shutdown hwmods */
> +		omap_hwmod_shutdown(oh);
> +
> +		/* we don't remove clocks cause there's no API to do so */
> +		/* no harm done, since they will not be created next time */
> +	}
> +
> +	/* cleanup the structure now */
> +	omap_device_delete(od);
> +}
> +
>  static int _omap_device_notifier_call(struct notifier_block *nb,
>  				      unsigned long event, void *dev)
>  {
> @@ -185,9 +211,13 @@ static int _omap_device_notifier_call(struct notifier_block *nb,
>  	struct omap_device *od;
>  
>  	switch (event) {
> -	case BUS_NOTIFY_DEL_DEVICE:
> +	case BUS_NOTIFY_UNBOUND_DRIVER:
> +		/* NOTIFY_DEL_DEVICE is not the right call...
> +		 * we use a callback here, to make sure no-one is going to
> +		 * try to use the omap_device data after they're deleted
> +		 */
>  		if (pdev->archdata.od)
> -			omap_device_delete(pdev->archdata.od);
> +			device_schedule_callback(dev, _omap_device_cleanup);

Really?  This is one sign that you are totally using the driver core
incorrectly.  You shouldn't have to rely on notifier callbacks to handle
device removals, your bus code should do that for you directly.

I don't like this at all, sorry.

And I was waiting for the day when people started to finally remove
platform devices from the system, I always thought it would never work
properly.  Good luck with this, I think you have a lot of work ahead of
yourself...

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/