| Message ID | 1363271407.4853.52.camel@i7.infradead.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi David, > Occasionally the trackpad in my MacBookPro 8,3 stops working, spewing > 'bcm5974: bad trackpad package, length: 8' We have seen this on a few exemplars over the years. It could be a hardware problem. > I haven't been able to reproduce this on demand, but it's annoying when > it does happen. I'm *assuming* that it's somehow forgotten what mode > it's supposed to be in. The fix for this on suspend/resume was to switch > it back to Wellspring mode again, so let's try that... What do you mean by "fix for this on suspend/resume"? The driver always returns to normal mode at suspend, and sets wellspring mode at resume. > Untested, because my trackpad seems to *know* when I'm actually running > a kernel with this patch, and doesn't ever misbehave. I've played with > removing the *normal* mode switch in bcm5974_start_traffic() but can't > get it to produce the 'bad trackpad package' message at all in that > case, so the rest function doesn't get invoked. Being random, it really sounds like faulty hardware. As such, perhaps the problem can be detected in the usb layer? > > Signed-off-by: David Woodhouse <David.Woodhouse@intel.com> > > diff --git a/drivers/input/mouse/bcm5974.c b/drivers/input/mouse/bcm5974.c > index 2baff1b..db0b3ab 100644 > --- a/drivers/input/mouse/bcm5974.c > +++ b/drivers/input/mouse/bcm5974.c > @@ -244,6 +244,7 @@ struct bcm5974 { > struct bt_data *bt_data; /* button transferred data */ > struct urb *tp_urb; /* trackpad usb request block */ > u8 *tp_data; /* trackpad transferred data */ > + struct work_struct reset_work; /* reset to wellspring mode */ > const struct tp_finger *index[MAX_FINGERS]; /* finger index data */ > struct input_mt_pos pos[MAX_FINGERS]; /* position array */ > int slots[MAX_FINGERS]; /* slot assignments */ > @@ -676,9 +677,14 @@ static void bcm5974_irq_trackpad(struct urb *urb) > if (dev->tp_urb->actual_length == 2) > goto exit; > > - if (report_tp_state(dev, dev->tp_urb->actual_length)) > + if (report_tp_state(dev, dev->tp_urb->actual_length)) { > dprintk(1, "bcm5974: bad trackpad package, length: %d\n", > dev->tp_urb->actual_length); > + if (dev->tp_urb->actual_length == 8) { > + /* Hm. Make sure it's in wellspring mode... */ > + schedule_work(&dev->reset_work); > + } > + } > > exit: > error = usb_submit_urb(dev->tp_urb, GFP_ATOMIC); > @@ -815,6 +821,14 @@ static int bcm5974_resume(struct usb_interface *iface) > return error; > } > > +static void bcm5974_mode_workfn(struct work_struct *work) > +{ > + struct bcm5974 *dev = container_of(work, struct bcm5974, reset_work); > + > + dev_info(&dev->intf->dev, "Reset into wellspring mode...\n"); > + bcm5974_wellspring_mode(dev, true); > +} > + This looks racy. > static int bcm5974_probe(struct usb_interface *iface, > const struct usb_device_id *id) > { > @@ -840,6 +854,7 @@ static int bcm5974_probe(struct usb_interface *iface, > dev->input = input_dev; > dev->cfg = *cfg; > mutex_init(&dev->pm_mutex); > + INIT_WORK(&dev->reset_work, bcm5974_mode_workfn); > > /* setup urbs */ > if (cfg->tp_type == TYPE1) { > @@ -936,6 +951,7 @@ static void bcm5974_disconnect(struct usb_interface *iface) > dev->bt_data, dev->bt_urb->transfer_dma); > usb_free_urb(dev->tp_urb); > usb_free_urb(dev->bt_urb); > + cancel_work_sync(&dev->reset_work); > kfree(dev); > } > In general, It does not really make sense for the transaction mode to change under our feet without anything in the usb layer knowing about it. Maybe there is a reset state cycle which does not get handle properly in the driver? Thanks, Henrik -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, 2013-03-16 at 20:31 +0100, Henrik Rydberg wrote: > What do you mean by "fix for this on suspend/resume"? The driver > always returns to normal mode at suspend, and sets wellspring mode at > resume. Yes, that's exactly what I mean. And in the early days when the driver didn't have a resume method, users were seeing exactly this 'bad trackpad package, length: 8' message on resume. Adding the suspend/resume methods fixed that — and that's why I'm assuming that a switch back into wellspring mode is going to be sufficient to fix what I'm seeing too. Unloading and reloading the module certainly is. > > +static void bcm5974_mode_workfn(struct work_struct *work) > > +{ > > + struct bcm5974 *dev = container_of(work, struct bcm5974, reset_work); > > + > > + dev_info(&dev->intf->dev, "Reset into wellspring mode...\n"); > > + bcm5974_wellspring_mode(dev, true); > > +} > > + > > This looks racy. Racy with what? Oh, I see... we should probably move the cancel_work_sync() from bcm5974_disconnect() to bcm5974_pause_traffic()? > In general, It does not really make sense for the transaction mode to > change under our feet without anything in the usb layer knowing about > it. If hardware always made sense, the world would be a much better place :) > Maybe there is a reset state cycle which does not get handle > properly in the driver? I don't believe so. A USB reset would end up with the bcm5974_probe() method being called again, and everything would work fine. The device may have reset its mode, but the USB bus doesn't seem to notice anything. When it happens, there are no USB messages; it just starts spewing the 'bad trackpad package' messages.
Hi David, > On Sat, 2013-03-16 at 20:31 +0100, Henrik Rydberg wrote: > > What do you mean by "fix for this on suspend/resume"? The driver > > always returns to normal mode at suspend, and sets wellspring mode at > > resume. > > Yes, that's exactly what I mean. And in the early days when the driver > didn't have a resume method, users were seeing exactly this 'bad > trackpad package, length: 8' message on resume. Adding the > suspend/resume methods fixed that — and that's why I'm assuming that a > switch back into wellspring mode is going to be sufficient to fix what > I'm seeing too. Unloading and reloading the module certainly is. The driver always had suspend/resume, but it is true that the EFI booting did create a problem with the reset_resume method, so we went back to rebinding the device. In addition to that problem, there has been reports in the past that seem to indicate a few exemplars of faulty hardware. > > > > +static void bcm5974_mode_workfn(struct work_struct *work) > > > +{ > > > + struct bcm5974 *dev = container_of(work, struct bcm5974, reset_work); > > > + > > > + dev_info(&dev->intf->dev, "Reset into wellspring mode...\n"); > > > + bcm5974_wellspring_mode(dev, true); > > > +} > > > + > > > > This looks racy. > > Racy with what? Oh, I see... we should probably move the > cancel_work_sync() from bcm5974_disconnect() to bcm5974_pause_traffic()? I was thinking about the work function entering the wellspring_mode function without the pm lock, but your observation seems correct, too :-) > > In general, It does not really make sense for the transaction mode to > > change under our feet without anything in the usb layer knowing about > > it. > > If hardware always made sense, the world would be a much better place :) But it would be good to know why your machine is special. As you say, the patch is not really tested, so we don't even know if it will fix the problem. > > Maybe there is a reset state cycle which does not get handle > > properly in the driver? > > I don't believe so. A USB reset would end up with the bcm5974_probe() > method being called again, and everything would work fine. The device > may have reset its mode, but the USB bus doesn't seem to notice > anything. When it happens, there are no USB messages; it just starts > spewing the 'bad trackpad package' messages. Ok, if you manage to hit the problem with a second version of this patch, I will take it. Thanks, Henrik -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/input/mouse/bcm5974.c b/drivers/input/mouse/bcm5974.c index 2baff1b..db0b3ab 100644 --- a/drivers/input/mouse/bcm5974.c +++ b/drivers/input/mouse/bcm5974.c @@ -244,6 +244,7 @@ struct bcm5974 { struct bt_data *bt_data; /* button transferred data */ struct urb *tp_urb; /* trackpad usb request block */ u8 *tp_data; /* trackpad transferred data */ + struct work_struct reset_work; /* reset to wellspring mode */ const struct tp_finger *index[MAX_FINGERS]; /* finger index data */ struct input_mt_pos pos[MAX_FINGERS]; /* position array */ int slots[MAX_FINGERS]; /* slot assignments */ @@ -676,9 +677,14 @@ static void bcm5974_irq_trackpad(struct urb *urb) if (dev->tp_urb->actual_length == 2) goto exit; - if (report_tp_state(dev, dev->tp_urb->actual_length)) + if (report_tp_state(dev, dev->tp_urb->actual_length)) { dprintk(1, "bcm5974: bad trackpad package, length: %d\n", dev->tp_urb->actual_length); + if (dev->tp_urb->actual_length == 8) { + /* Hm. Make sure it's in wellspring mode... */ + schedule_work(&dev->reset_work); + } + } exit: error = usb_submit_urb(dev->tp_urb, GFP_ATOMIC); @@ -815,6 +821,14 @@ static int bcm5974_resume(struct usb_interface *iface) return error; } +static void bcm5974_mode_workfn(struct work_struct *work) +{ + struct bcm5974 *dev = container_of(work, struct bcm5974, reset_work); + + dev_info(&dev->intf->dev, "Reset into wellspring mode...\n"); + bcm5974_wellspring_mode(dev, true); +} + static int bcm5974_probe(struct usb_interface *iface, const struct usb_device_id *id) { @@ -840,6 +854,7 @@ static int bcm5974_probe(struct usb_interface *iface, dev->input = input_dev; dev->cfg = *cfg; mutex_init(&dev->pm_mutex); + INIT_WORK(&dev->reset_work, bcm5974_mode_workfn); /* setup urbs */ if (cfg->tp_type == TYPE1) { @@ -936,6 +951,7 @@ static void bcm5974_disconnect(struct usb_interface *iface) dev->bt_data, dev->bt_urb->transfer_dma); usb_free_urb(dev->tp_urb); usb_free_urb(dev->bt_urb); + cancel_work_sync(&dev->reset_work); kfree(dev); }
Occasionally the trackpad in my MacBookPro 8,3 stops working, spewing 'bcm5974: bad trackpad package, length: 8' I haven't been able to reproduce this on demand, but it's annoying when it does happen. I'm *assuming* that it's somehow forgotten what mode it's supposed to be in. The fix for this on suspend/resume was to switch it back to Wellspring mode again, so let's try that... Untested, because my trackpad seems to *know* when I'm actually running a kernel with this patch, and doesn't ever misbehave. I've played with removing the *normal* mode switch in bcm5974_start_traffic() but can't get it to produce the 'bad trackpad package' message at all in that case, so the rest function doesn't get invoked. Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>