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

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

[akpm]

The patch titled
     hpsa: use msleep() instead of schedule_timeout
has been added to the -mm tree.  Its filename is
     hpsa-use-msleep-instead-of-schedule_timeout.patch

Before you just go and hit "reply", please:
   a) Consider who else should be cc'ed
   b) Prefer to cc a suitable mailing list as well
   c) Ideally: find the original patch on the mailing list and do a
      reply-to-all to that, adding suitable additional cc's

*** Remember to use Documentation/SubmitChecklist when testing your code ***

See http://userweb.kernel.org/~akpm/stuff/added-to-mm.txt to find
out what to do about this

The current -mm tree may be found at http://userweb.kernel.org/~akpm/mmotm/

------------------------------------------------------
Subject: hpsa: use msleep() instead of schedule_timeout
From: Stephen M. Cameron <scameron@beardog.cce.hp.com>

Use msleep() instead of schedule_timeout

Signed-off-by: Stephen M. Cameron <scameron@beardog.cce.hp.com>
Cc: Mike Miller <mikem@beardog.cce.hp.com>
Cc: Jens Axboe <jens.axboe@oracle.com>
Cc: James Bottomley <James.Bottomley@HansenPartnership.com>
Cc: Alex Chiang <achiang@hp.com>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 drivers/scsi/hpsa.c |   27 ++++++++++++++-------------
 drivers/scsi/hpsa.h |    4 ++--
 2 files changed, 16 insertions(+), 15 deletions(-)

diff -puN drivers/scsi/hpsa.c~hpsa-use-msleep-instead-of-schedule_timeout drivers/scsi/hpsa.c
--- a/drivers/scsi/hpsa.c~hpsa-use-msleep-instead-of-schedule_timeout
+++ a/drivers/scsi/hpsa.c
@@ -1933,7 +1933,7 @@ static int wait_for_device_to_become_rea
 {
 	int rc = 0;
 	int count = 0;
-	int waittime = HZ;
+	int waittime = 1; /* seconds */
 	struct CommandList *c;
 
 	c = cmd_special_alloc(h);
@@ -1950,11 +1950,11 @@ static int wait_for_device_to_become_rea
 		 * the TUR right away, the reset will just abort it.
 		 */
 		set_current_state(TASK_UNINTERRUPTIBLE);
-		schedule_timeout(waittime);
+		msleep(1000 * waittime);
 		count++;
 
 		/* Increase wait time with each try, up to a point. */
-		if (waittime < (HZ * HPSA_MAX_WAIT_INTERVAL_SECS))
+		if (waittime < HPSA_MAX_WAIT_INTERVAL_SECS)
 			waittime = waittime * 2;
 
 		/* Send the Test Unit Ready */
@@ -1972,7 +1972,7 @@ static int wait_for_device_to_become_rea
 			break;
 
 		dev_warn(&h->pdev->dev, "waiting %d secs "
-			"for device to become ready.\n", waittime / HZ);
+			"for device to become ready.\n", waittime);
 		rc = 1; /* device not ready. */
 	}
 
@@ -2838,8 +2838,8 @@ static __devinit int hpsa_message(struct
 		tag = readl(vaddr + SA5_REPLY_PORT_OFFSET);
 		if (HPSA_TAG_DISCARD_ERROR_BITS(tag) == paddr32)
 			break;
-		schedule_timeout_uninterruptible(
-			HPSA_MSG_SEND_RETRY_INTERVAL_SECS * HZ);
+		set_current_state(TASK_UNINTERRUPTIBLE);
+		msleep(HPSA_MSG_SEND_RETRY_INTERVAL_MSECS);
 	}
 
 	iounmap(vaddr);
@@ -2953,7 +2953,7 @@ static __devinit int hpsa_hard_reset_con
 	pci_write_config_word(pdev, pos + PCI_PM_CTRL, pmcsr);
 
 	set_current_state(TASK_UNINTERRUPTIBLE);
-	schedule_timeout(HZ >> 1);
+	msleep(500);
 
 	/* enter the D0 power management state */
 	pmcsr &= ~PCI_PM_CTRL_STATE_MASK;
@@ -2961,7 +2961,7 @@ static __devinit int hpsa_hard_reset_con
 	pci_write_config_word(pdev, pos + PCI_PM_CTRL, pmcsr);
 
 	set_current_state(TASK_UNINTERRUPTIBLE);
-	schedule_timeout(HZ >> 1);
+	msleep(500);
 
 	/* Restore the PCI configuration space.  The Open CISS
 	 * Specification says, "Restore the PCI Configuration
@@ -3187,8 +3187,8 @@ static int hpsa_pci_init(struct ctlr_inf
 		scratchpad = readl(h->vaddr + SA5_SCRATCHPAD_OFFSET);
 		if (scratchpad == HPSA_FIRMWARE_READY)
 			break;
-		set_current_state(TASK_INTERRUPTIBLE);
-		schedule_timeout(HPSA_BOARD_READY_POLL_INTERVAL);
+		set_current_state(TASK_UNINTERRUPTIBLE);
+		msleep(HPSA_BOARD_READY_POLL_INTERVAL_MSECS);
 	}
 	if (scratchpad != HPSA_FIRMWARE_READY) {
 		dev_warn(&pdev->dev, "board not ready, timed out.\n");
@@ -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);
 	}
 
 #ifdef HPSA_DEBUG
@@ -3302,7 +3302,8 @@ static int __devinit hpsa_init_one(struc
 
 		/* Some devices (notably the HP Smart Array 5i Controller)
 		   need a little pause here */
-		schedule_timeout_uninterruptible(HPSA_POST_RESET_PAUSE);
+		set_current_state(TASK_UNINTERRUPTIBLE);
+		msleep(HPSA_POST_RESET_PAUSE_MSECS);
 
 		/* Now try to get the controller to respond to a no-op */
 		for (i = 0; i < HPSA_POST_RESET_NOOP_RETRIES; i++) {
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
@@ -108,7 +108,7 @@ struct ctlr_info {
 #define HPSA_BUS_RESET_MSG 2
 #define HPSA_HOST_RESET_MSG 3
 #define HPSA_MSG_SEND_RETRY_LIMIT 10
-#define HPSA_MSG_SEND_RETRY_INTERVAL_SECS 1
+#define HPSA_MSG_SEND_RETRY_INTERVAL_MSECS 1000
 
 /* Maximum time in seconds driver will wait for command completions
  * when polling before giving up.
@@ -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)
 #define HPSA_POST_RESET_NOOP_RETRIES (12)
 
 /*  Defining the diffent access_menthods */
_

Patches currently in -mm which might be from scameron@beardog.cce.hp.com are

origin.patch
scsi-add-hpsa-driver-for-hp-smart-array-controllers.patch
hpsa-use-msleep-instead-of-schedule_timeout.patch
hpsa-use-msleep-instead-of-schedule_timeout-fix.patch
hpsa-rename-too-generic-variable-names.patch
hpsa-return-scsi_mlqueue_host_busy-on-command-allocation-failure.patch
hpsa-fix-incorrect-scsi-status-reporting.patch
hpsa-suppress-messages-due-to-unsupport-scsi-report_luns.patch

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

[Jiri Slaby]

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? And you
intermix jiffies with msecs. Use schedule_timeout_interruptible(10).

> @@ -3302,7 +3302,8 @@ static int __devinit hpsa_init_one(struc
>  
>  		/* Some devices (notably the HP Smart Array 5i Controller)
>  		   need a little pause here */
> -		schedule_timeout_uninterruptible(HPSA_POST_RESET_PAUSE);
> +		set_current_state(TASK_UNINTERRUPTIBLE);
> +		msleep(HPSA_POST_RESET_PAUSE_MSECS);

Hmm, setting the state is superfluous, as msleep does the job itself.

> 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?

-- 
js

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

[Jiri Slaby]

On 12/08/2009 11:25 PM, Jiri Slaby wrote:
>> @@ -3302,7 +3302,8 @@ static int __devinit hpsa_init_one(struc
>>  
>>  		/* Some devices (notably the HP Smart Array 5i Controller)
>>  		   need a little pause here */
>> -		schedule_timeout_uninterruptible(HPSA_POST_RESET_PAUSE);
>> +		set_current_state(TASK_UNINTERRUPTIBLE);
>> +		msleep(HPSA_POST_RESET_PAUSE_MSECS);
> 
> Hmm, setting the state is superfluous, as msleep does the job itself.

(FWIW addressed by Andrew already.)

-- 
js

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

[scameron]

On Tue, Dec 08, 2009 at 11:25:05PM +0100, Jiri Slaby 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? And you

Because the I think the interruptible was wrong, we aren't interested
in signals at this point.

> intermix jiffies with msecs. Use schedule_timeout_interruptible(10).

Probably it should have been msecs all along, not jiffies.  Looking at that particular
part of the code, it's very old, and might even be superflous given the controllers
this driver supports (and those it does not support.)  I'm thinking that
schedule_timeout(10) was written back when HZ was commonly 100, so that would
have been 100 msecs, not 10.  Considering the comment above this code describing
the circumstances that this is for, I doubt that code has ever run, actually.
You have to replace a failed drive just as driver is loading.

I will talk to the firmware guys and see if I can figure out what the original
case that lead to this code being put in is still relevant.  We might be
able to get rid of this section altogether.

> 
> > @@ -3302,7 +3302,8 @@ static int __devinit hpsa_init_one(struc
> >  
> >  		/* Some devices (notably the HP Smart Array 5i Controller)
> >  		   need a little pause here */
> > -		schedule_timeout_uninterruptible(HPSA_POST_RESET_PAUSE);
> > +		set_current_state(TASK_UNINTERRUPTIBLE);
> > +		msleep(HPSA_POST_RESET_PAUSE_MSECS);
> 
> Hmm, setting the state is superfluous, as msleep does the job itself.

Yeah, Andrew pointed that out and sent me a patch.  Thanks.

> 
> > 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?

Jeez, you'd think I could multiply by 1000 without screwing up.  Guess not.

> 
> -- 
> js

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

[Andrew Morton]

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.

> And you
> intermix jiffies with msecs. Use schedule_timeout_interruptible(10).
> 
> > @@ -3302,7 +3302,8 @@ static int __devinit hpsa_init_one(struc
> >  
> >  		/* Some devices (notably the HP Smart Array 5i Controller)
> >  		   need a little pause here */
> > -		schedule_timeout_uninterruptible(HPSA_POST_RESET_PAUSE);
> > +		set_current_state(TASK_UNINTERRUPTIBLE);
> > +		msleep(HPSA_POST_RESET_PAUSE_MSECS);
> 
> Hmm, setting the state is superfluous, as msleep does the job itself.

yup, I fixed those.

> > 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 ;)

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

[Jiri Slaby]

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

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

[scameron]

On Wed, Dec 09, 2009 at 10:51:32AM +0100, Jiri Slaby wrote:
> 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.

Sorry.  I'll try to do better.  FWIW, these all get rolled up into one
big patch on account of this being a new driver not yet in Linus's
tree, so those change log entries wouldn't go into git anyway, so
far as I know.

> 
> >>> 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 ;)
> 

BTW, I talked to a firmware guy familiar with the original problem
and he looked through the firmware code for the the controllers this
driver supports and said he couldn't see any reason it should take more
than a couple seconds, so, three seconds in the driver seems reasonable.
If it turns out that people start complaining that the boards aren't
getting initialized in time, we can revisit it.  It's also my own
experience that this section takes next to no time at all.

> 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