Re: [PATCH] bcm5974: recover from failed mode switch

[Dmitry Torokhov]

Hi Liam,

On Sat, Feb 07, 2026 at 06:16:26PM +0100, Liam Mitchell wrote:
> Mode switches sent before control response are ignored.
> On receiving unknown 8-byte packets, assume that mode switch was ignored
> and reset by switching to normal mode, waiting then switching back to
> wellspring mode.
> 
> ---
> This patch addresses an issue where the bcm5974 driver switches modes
> before the device is ready, resulting in an unresponsive trackpad and
> "bcm5974: bad trackpad package, length: 8" repeated in logs.
> 
> Discussion of issue in the thread:
> https://lore.kernel.org/linux-input/CAOQ1CL4+DP1TuLAGNsz5GdFBTHvnTg=5q=Dr2Z1OQc6RXydSYA@xxxxxxxxxxxxxx/
> 
> This fix is conservative, avoiding changing existing mode-switch
> behavior because I cannot test all variations of hardware.
> 
> On receiving an unknown 8-byte packet, we assume the device is not in
> wellspring mode and schedule an asynchronous mode reset.
> 
> Signed-off-by: Liam Mitchell <mitchell.liam@xxxxxxxxx>
> ---
>  drivers/input/mouse/bcm5974.c | 43 ++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 42 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/input/mouse/bcm5974.c b/drivers/input/mouse/bcm5974.c
> index dfdfb59cc8b5..85ddd65f2603 100644
> --- a/drivers/input/mouse/bcm5974.c
> +++ b/drivers/input/mouse/bcm5974.c
> @@ -286,6 +286,8 @@ struct bcm5974 {
>  	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 */
> +	struct work_struct mode_reset_work;
> +	unsigned long last_mode_reset;
>  };
>  
>  /* trackpad finger block data, le16-aligned */
> @@ -696,6 +698,34 @@ static int bcm5974_wellspring_mode(struct bcm5974 *dev, bool on)
>  	return retval;
>  }
>  
> +/*
> + * Mode switches sent before the control response are ignored.
> + * Fixing this state requires switching to normal mode and waiting
> + * 1ms before switching back to wellspring mode.
> + */
> +static void bcm5974_mode_reset_work(struct work_struct *work)
> +{
> +	int error;
> +	struct bcm5974 *dev = container_of(work, struct bcm5974, mode_reset_work);
> +
> +	mutex_lock(&dev->pm_mutex);

	guard(mutex)(&dev->pm_mutex);

> +	dev->last_mode_reset = jiffies;
> +
> +	error = bcm5974_wellspring_mode(dev, false);
> +	if (error) {
> +		dprintk(1, "bcm5974: reset to normal mode failed\n");

		dev_err(...);

> +		goto out;

		return;
> +	}
> +
> +	msleep(1);

This duration is too short for msleep(). Use usleep_range() of fsleep().

> +
> +	error = bcm5974_wellspring_mode(dev, true);
> +	if (error)
> +		dprintk(1, "bcm5974: mode switch after reset failed\n");

		dev_err(...);
> + out:
> +	mutex_unlock(&dev->pm_mutex);

Explicit unlock is not needed with guard().

> +}
> +
>  static void bcm5974_irq_button(struct urb *urb)
>  {
>  	struct bcm5974 *dev = urb->context;
> @@ -752,10 +782,18 @@ 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);
>  
> +		/* HID packet means we aren't in wellspring mode */
> +		/* If we haven't tried a reset in the last second, try now */
> +		if (dev->tp_urb->actual_length == 8 &&
> +		    time_after(jiffies, dev->last_mode_reset + msecs_to_jiffies(1000))) {
> +			schedule_work(&dev->mode_reset_work);
> +		}
> +	}
> +
>  exit:
>  	error = usb_submit_urb(dev->tp_urb, GFP_ATOMIC);
>  	if (error)
> @@ -906,6 +944,8 @@ static int bcm5974_probe(struct usb_interface *iface,
>  	dev->intf = iface;
>  	dev->input = input_dev;
>  	dev->cfg = *cfg;
> +	dev->last_mode_reset = 0;

No need to initialize with 0, kzalloc() takes care of this.

> +	INIT_WORK(&dev->mode_reset_work, bcm5974_mode_reset_work);
>  	mutex_init(&dev->pm_mutex);
>  
>  	/* setup urbs */
> @@ -998,6 +1038,7 @@ static void bcm5974_disconnect(struct usb_interface *iface)
>  {
>  	struct bcm5974 *dev = usb_get_intfdata(iface);
>  
> +	cancel_work_sync(&dev->mode_reset_work);

Use disable_delayed_work_sync() instead to ensure it will not get
re-triggered.

>  	usb_set_intfdata(iface, NULL);
>  
>  	input_unregister_device(dev->input);

Thanks.

-- 
Dmitry