On Sat, Sep 24, 2011 at 12:00:19AM +0300, Dan Carpenter wrote:
> On Fri, Sep 23, 2011 at 06:38:05PM +0200, Julian Andres Klode wrote:
> > +struct nvec_msg *nvec_write_sync(struct nvec_chip *nvec,
> > + const unsigned char *data, short size)
> > +{
> > + mutex_lock(&nvec->sync_write_mutex);
> > +
> > + nvec->sync_write_pending = (data[1] << 8) + data[0];
> > + nvec_write_async(nvec, data, size);
> > +
> > + dev_dbg(nvec->dev, "nvec_sync_write: 0x%04x\n",
> > + nvec->sync_write_pending);
> > + if (!(wait_for_completion_timeout(&nvec->sync_write,
> > + msecs_to_jiffies(2000)))) {
> > + dev_warn(nvec->dev, "timeout waiting for sync write to complete\n");
> > + mutex_unlock(&nvec->sync_write_mutex);
> > + return NULL;
> > + }
> > +
> > + dev_dbg(nvec->dev, "nvec_sync_write: pong!\n");
> > +
> > + mutex_unlock(&nvec->sync_write_mutex);
> > +
> > + return nvec->last_sync_msg;
>
> The ->sync_write_mutex protects the ->last_sync_msg member but you're
> using it outside the lock here. (Save a local of msg pointer before
> you release the lock).
Yep, seems fairly obvious now that you point it out.
>
> > +}
> > +EXPORT_SYMBOL(nvec_write_sync);
>
> > +
> > +/* TX worker */
> > static void nvec_request_master(struct work_struct *work)
> > {
> > struct nvec_chip *nvec = container_of(work, struct nvec_chip, tx_work);
> > + unsigned long flags;
> > + struct nvec_msg *msg;
> >
> > - if (!list_empty(&nvec->tx_data))
> > - gpio_set_value(nvec->gpio, 0);
> > + spin_lock_irqsave(&nvec->tx_lock, flags);
> > + while (!list_empty(&nvec->tx_data)) {
> > + msg = list_first_entry(&nvec->tx_data, struct nvec_msg, node);
> > + spin_unlock_irqrestore(&nvec->tx_lock, flags);
> > + nvec_gpio_set_value(nvec, 0);
> > + if (!(wait_for_completion_interruptible_timeout(
> > + &nvec->ec_transfer, msecs_to_jiffies(5000)))) {
> > + dev_warn(nvec->dev, "timeout waiting for ec transfer\n");
> > + nvec_gpio_set_value(nvec, 1);
> > + msg->pos = 0;
> > + } else {
> > + list_del_init(&msg->node);
> > + nvec_msg_free(nvec, msg);
>
> wait_for_completion_interruptible_timeout() returns a negative error
> code if it gets interrupted so that case should be handled as well.
How'd you handle this? If we get interrupted after 4900 ms we
probably do not want to start waiting for another 5000 ms,
right? I forgot to add that to the commit message, but that
part of the patch is not mine, Marc wrote it, he knows best.
I just squashed Marc's rewrite and mine into one patch, as
having to review to rewrites of the interrupt and workers
seems a bit stupid.
--
Julian Andres Klode - Debian Developer, Ubuntu Member
See http://wiki.debian.org/JulianAndresKlode and http://jak-linux.org/.
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/devel
