Messages in this thread | | | Date | Wed, 17 Sep 2008 11:03:39 -0700 | From | Tejun Heo <> | Subject | Re: [PATCH 2/4 v2] libata: Implement disk shock protection support |
| |
Hello, Elias.
Looks generally good. Just a few points.
Elias Oltmanns wrote: > +static void ata_eh_pull_action(struct ata_link *link, struct ata_device *dev, > + unsigned int action) > +{ > + struct ata_port *ap = link->ap; > + struct ata_eh_info *ehi = &link->eh_info, *ehci = &link->eh_context.i; > + struct ata_device *tdev; > + unsigned int taction; > + unsigned long flags; > + > + spin_lock_irqsave(ap->lock, flags); > + > + if (dev) { > + taction = action & (ehi->action | ehi->dev_action[dev->devno]); > + ehci->dev_action[dev->devno] |= taction & ATA_EH_PERDEV_MASK; > + ehci->action |= taction & ~ATA_EH_PERDEV_MASK; > + } else { > + if (WARN_ON(action & ATA_EH_PERDEV_MASK)) > + action &= ~ATA_EH_PERDEV_MASK; > + ata_link_for_each_dev(tdev, link) > + taction |= ehi->dev_action[tdev->devno] & action;
taction seems to be being used uninitialized.
> + do { > + unsigned long now; > + > + deadline = jiffies; > + ata_port_for_each_link(link, ap) { > + ata_link_for_each_dev(dev, link) { > + struct ata_eh_info *ehi = &link->eh_context.i; > + > + if (dev->class != ATA_DEV_ATA) > + continue; > + > + ata_eh_pull_action(link, dev, ATA_EH_PARK); > + if (ehi->dev_action[dev->devno] & ATA_EH_PARK) { > + unsigned long tmp = > + dev->unpark_deadline; > + > + if (time_before(deadline, tmp)) > + deadline = tmp; > + else if (time_before_eq(tmp, jiffies)) > + continue; > + } > + > + ata_eh_park_issue_cmd(dev, 1); > + } > + } > + now = jiffies; > + if (time_before_eq(deadline, now)) > + break; > + prepare_to_wait(&ata_scsi_park_wq, &wait, TASK_UNINTERRUPTIBLE);
Doesn't prepare_to_wait() have to come before pull_action and timeout check? Which in turn means that it should be a completion instead of wait combined with INIT_COMPLETION because thread state can't be used to track wake up as ata_eh_park_issue_cmd() sleeps.
Thanks. :-)
-- tejun
| |