On Wed, Feb 13 2008 at 19:36 +0200, James Bottomley <[EMAIL PROTECTED]> wrote:
<snip>
>> ---
>> From: Boaz Harrosh <[EMAIL PROTECTED]>
>> 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 <[EMAIL PROTECTED]>
>> ---
<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
-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
