Re: [REGRESSION] Missing bcm5974 touchpad on Macbooks

From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
To: Javier Carrasco <javier.carrasco.cruz@gmail.com>
Cc: Takashi Iwai <tiwai@suse.de>,
	Javier Carrasco <javier.carrasco@wolfvision.net>,
	linux-input@vger.kernel.org, linux-kernel@vger.kernel.org,
	regressions@lists.linux.dev, Henrik Rydberg <rydberg@euromail.se>,
	John Horan <knasher@gmail.com>
Subject: Re: [REGRESSION] Missing bcm5974 touchpad on Macbooks
Date: Mon, 4 Mar 2024 17:15:39 -0800	[thread overview]
Message-ID: <ZeZyO7RUGyC8BRKP@google.com> (raw)
In-Reply-To: <449417ca-aae1-4868-a96f-a99ac5d187d6@gmail.com>

On Mon, Mar 04, 2024 at 09:21:19PM +0100, Javier Carrasco wrote:
> 
> There is indeed an interrupt endpoint with address 0x81, but the driver
> defines bInterfaceProtocol = 2 (Mouse), and the endpoint in that
> interface is 0x84:
> 
> #define BCM5974_DEVICE(prod) {					\
> 	.match_flags = (USB_DEVICE_ID_MATCH_DEVICE |		\
> 			USB_DEVICE_ID_MATCH_INT_CLASS |		\
> 			USB_DEVICE_ID_MATCH_INT_PROTOCOL),	\
> 	.idVendor = USB_VENDOR_ID_APPLE,			\
> 	.idProduct = (prod),					\
> 	.bInterfaceClass = USB_INTERFACE_CLASS_HID,		\
> 	.bInterfaceProtocol = USB_INTERFACE_PROTOCOL_MOUSE	\
> }
> 
> where USB_INTERFACE_PROTOCOL_MOUSE = 2.
> 
> 
> My interpretation is that the driver is checking if the endpoint with
> address 0x81 form the interface with bInterfaceProtocol = 2 (that is the
> last interface of the list, the one with bInterfaceNumber = 2), but it
> is not found, because its only endpoint has a different address (0x84).
> 
> Interestingly, 0x84 is the address given to the endpoint of the button
> interface. The button interface should not be relevant for Macbook 5,1
> (TYPE 2 in the driver), according to 43f482b48d03 ("Input: bcm5974 -
> only setup button urb for TYPE1 devices").
> 
> If that is true, does anyone know why bInterfaceProtocol is always set
> to USB_INTERFACE_PROTOCOL_MOUSE, and why the driver works anyway with
> bEndpointAddress = 0x81 for the trackpad? The urb setup for 0x84 is only
> executed for TYPE 1 devices, and the mouse interface does not have an
> endpoint with address 0x81. Or am I missing something?

The driver is naughty, it binds to the 3rd interface (bInterfaceNumber
2) but actually pokes into the 2nd interface with endpoint 0x84 without
actually claiming it. Your check expects that the endpoint belongs to
the interface that the driver binds to and thus fails.

> 
> We could revert the patch in question, but I see no reason why checking
> an expected interrupt endpoint should cause trouble. It looks like there
> is something fishy going on.

Yes, the driver needs to claim both interfaces and when checking use the
right one. I will revert the patch for now given that it causes
regression and we can try fixing it again.

Thanks.

-- 
Dmitry