Messages in this thread | | | Date | Wed, 09 Dec 2009 10:51:32 +0100 | From | Jiri Slaby <> | 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@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
| |