Messages in this thread | | | Date | Tue, 8 Dec 2009 14:44:19 -0800 | From | Andrew Morton <> | Subject | Re: + hpsa-use-msleep-instead-of-schedule_timeout.patch added to -mm tree |
| |
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 ;)
| |