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

[Henrik Rydberg <rydberg@bitmath.se>]

Hi Liam,

thanks for seeing this through! Looks good to me.

Acked-by: Henrik Rydberg <rydberg@bitmath.org>

On 2/13/26 10:25 AM, 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@mail.gmail.com/
> 
> 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@gmail.com>
> Link: https://lore.kernel.org/linux-input/CAOQ1CL4+DP1TuLAGNsz5GdFBTHvnTg=5q=Dr2Z1OQc6RXydSYA@mail.gmail.com/
> ---
> Changes in v2:
> - mutex_lock -> guard(mutex)
> - dprintk -> dev_err
> - msleep -> fsleep
> - removed 0 init
> - cancel_work_sync -> disable_delayed_work_sync
> - work_struct -> delayed_work
> - Link to v1: https://lore.kernel.org/r/20260207-bcm5974-reset-v1-1-af7163903fa6@gmail.com
> ---
>   drivers/input/mouse/bcm5974.c | 40 +++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 39 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/input/mouse/bcm5974.c b/drivers/input/mouse/bcm5974.c
> index dfdfb59cc8b5..6ee766ed8402 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 delayed_work mode_reset_work;
> +	unsigned long last_mode_reset;
>   };
>   
>   /* trackpad finger block data, le16-aligned */
> @@ -696,6 +698,32 @@ 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
> + * about 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.work);
> +
> +	guard(mutex)(&dev->pm_mutex);
> +	dev->last_mode_reset = jiffies;
> +
> +	error = bcm5974_wellspring_mode(dev, false);
> +	if (error) {
> +		dev_err(&dev->intf->dev, "reset to normal mode failed\n");
> +		return;
> +	}
> +
> +	fsleep(1000);
> +
> +	error = bcm5974_wellspring_mode(dev, true);
> +	if (error)
> +		dev_err(&dev->intf->dev, "mode switch after reset failed\n");
> +}
> +
>   static void bcm5974_irq_button(struct urb *urb)
>   {
>   	struct bcm5974 *dev = urb->context;
> @@ -752,10 +780,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_delayed_work(&dev->mode_reset_work, 0);
> +		}
> +	}
> +
>   exit:
>   	error = usb_submit_urb(dev->tp_urb, GFP_ATOMIC);
>   	if (error)
> @@ -906,6 +942,7 @@ static int bcm5974_probe(struct usb_interface *iface,
>   	dev->intf = iface;
>   	dev->input = input_dev;
>   	dev->cfg = *cfg;
> +	INIT_DELAYED_WORK(&dev->mode_reset_work, bcm5974_mode_reset_work);
>   	mutex_init(&dev->pm_mutex);
>   
>   	/* setup urbs */
> @@ -998,6 +1035,7 @@ static void bcm5974_disconnect(struct usb_interface *iface)
>   {
>   	struct bcm5974 *dev = usb_get_intfdata(iface);
>   
> +	disable_delayed_work_sync(&dev->mode_reset_work);
>   	usb_set_intfdata(iface, NULL);
>   
>   	input_unregister_device(dev->input);
> 
> ---
> base-commit: 7d0a66e4bb9081d75c82ec4957c50034cb0ea449
> change-id: 20260207-bcm5974-reset-85ccdfca9641
> 
> Best regards,