lkml.org 
[lkml]   [2009]   [Dec]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: + hpsa-use-msleep-instead-of-schedule_timeout.patch added to -mm tree
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


\
 
 \ /
  Last update: 2009-12-09 10:53    [W:0.041 / U:0.700 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site