Re: [BUGFIX 2/2] gdth: bugfix for the Timer at exit crash

[Boaz Harrosh <bharrosh@panasas.com>]

On Wed, Feb 13 2008 at 19:36 +0200, James Bottomley <James.Bottomley@HansenPartnership.com> wrote:
<snip>
>> ---
>> From: Boaz Harrosh <bharrosh@panasas.com>
>> Subject: [PATCH] gdth: bugfix for the at-exit problems
>>
>> gdth_exit would first remove all cards then stop the timer
>> and would not sync with the timer function. This caused a crash
>> in gdth_timer() when module was unloaded.
>> So del_timer_sync the timer before we delete the cards.
>>
>> also the reboot notifier function would crash. So unify
>> the exit and halt functions with a gdth_shutdown() that's
>> called by both.
>>
>> Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
>> ---
<snip>
>> +static struct notifier_block gdth_notifier = {
>> +    gdth_halt, NULL, 0
>> +};
>> +
>> +bool gdth_shutdown_done;
> 

right forgot the static. But I use it in gdth_init(), so it
must be external. Unless you promise me that gdth_init() will
never ever be called after a call to shutdown.
Any way the hot-plug patch changes all that. This is only
for 2.6.24 bugfixs.

> Static police alert!  Just make it static and move it into
> gdth_shutdown()
> 
>> +static void gdth_shutdown()
>> +{
>> +	gdth_ha_str *ha;
>> +	if (gdth_shutdown_done)
>> +		return;
>> +
>> +	gdth_shutdown_done = true;
>> +	unregister_chrdev(major,"gdth");
>> +	unregister_reboot_notifier(&gdth_notifier);
> 
> I'm not sure you can do this, aren't reboot notifiers called with the
> rwsem held?  In which case the unregister which also takes the rwsem
> will hang the system.
> 
humm, can't remove a notifier from within the notifier. Thanks James for 
the catch, it's what happens when you don't test your own patches.

I have moved unregister_reboot_notifier to gdth_exit.
> James
> 

Will send a new version for review. Please note that this is a bugfix patch
on top of 2.6.24. It is not needed for Jeff's hot-plug path.

There will be one more bugfix patch for a crash at the user-mode ioctl code.

Boaz