Re: + hpsa-use-msleep-instead-of-schedule_timeout.patch added to -mm tree

[Jiri Slaby <jirislaby@gmail.com>]

On 12/08/2009 11:44 PM, Andrew Morton wrote:
> On Tue, 08 Dec 2009 23:25:05 +0100
> Jiri Slaby <jirislaby@gmail.com> wrote:
> 
>> On 12/08/2009 11:04 PM, akpm@linux-foundation.org wrote:
>>> Subject: hpsa: use msleep() instead of schedule_timeout
>>> From: Stephen M. Cameron <scameron@beardog.cce.hp.com>
>>>
>>> Use msleep() instead of schedule_timeout
>>
>> The patch does more than that and moreover in a wrong manner, see below.
>>
>>> @@ -3262,8 +3262,8 @@ static int hpsa_pci_init(struct ctlr_inf
>>>  		if (!(readl(h->vaddr + SA5_DOORBELL) & CFGTBL_ChangeReq))
>>>  			break;
>>>  		/* delay and try again */
>>> -		set_current_state(TASK_INTERRUPTIBLE);
>>> -		schedule_timeout(10);
>>> +		set_current_state(TASK_UNINTERRUPTIBLE);
>>> +		msleep(10);
>>
>> Why do you change interruptible sleep to uninterruptible?
> 
> Stealth bugfix ;)
> 
> If the calling process (called "modprobe") has signal_pending()
> (someone ^C'ed it) then the TASK_INTERRUPTIBLE sleep will be a no-op
> and the driver will probably go and screw things up.

Ok, then it should have been in the changelog. The changelog was just
"Use msleep() instead of schedule_timeout". Apart it doesn't mention why
it is changed (it might be obvious for some that it's a cleanup), it
doesn't do merely that.

>>> diff -puN drivers/scsi/hpsa.h~hpsa-use-msleep-instead-of-schedule_timeout drivers/scsi/hpsa.h
>>> --- a/drivers/scsi/hpsa.h~hpsa-use-msleep-instead-of-schedule_timeout
>>> +++ a/drivers/scsi/hpsa.h
>> ...
>>> @@ -139,7 +139,7 @@ struct ctlr_info {
>>>  #define HPSA_BOARD_READY_ITERATIONS \
>>>  	((HPSA_BOARD_READY_WAIT_SECS * 1000) / \
>>>  		HPSA_BOARD_READY_POLL_INTERVAL_MSECS)
>>> -#define HPSA_POST_RESET_PAUSE (30 * HZ)
>>> +#define HPSA_POST_RESET_PAUSE_MSECS (3000)
>>
>> Ehm?
> 
> 30 seconds would have sucked anyway ;)

Might be. But again, then it should be explained in the changelog. And
as this is totally separate thing, also in a separate patch.

At least I though this is the politics.

thanks,
-- 
js