Re: [PATCH 5/5] arm: omap: Proper cleanups for omap_device

[Pantelis Antoniou]

Hi Greg,

On Aug 6, 2013, at 12:33 PM, Greg Kroah-Hartman wrote:

> 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@xxxxxxxxxxxxxxxxxxxxxxx>
>> ---
>> 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.
> 

Don't shoot the messenger please...

This is all about fixing a crash without messing too many things.

> 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...
> 

I know.

Platform device removal is the wild-wild west...

If I had to make a wish, would be to kill platform_device completely and
integrate all it's functionality in the the vanilla device.

What exactly is a platform device anyway?

> greg k-h

Regards

-- Pantelis

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