linux-kernel - Re: + hpsa-use-msleep-instead-of-schedule

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <4B1F7324.9000402@gmail.com>
Date:	Wed, 09 Dec 2009 10:51:32 +0100
From:	Jiri Slaby <jirislaby@...il.com>
To:	Andrew Morton <akpm@...ux-foundation.org>
CC:	linux-kernel@...r.kernel.org, scameron@...rdog.cce.hp.com,
	James.Bottomley@...senPartnership.com, achiang@...com,
	jens.axboe@...cle.com, mikem@...rdog.cce.hp.com
Subject: Re: + 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@...il.com> wrote:
> 
>> On 12/08/2009 11:04 PM, akpm@...ux-foundation.org wrote:
>>> Subject: hpsa: use msleep() instead of schedule_timeout
>>> From: Stephen M. Cameron <scameron@...rdog.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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/