[REGRESSION] Missing bcm5974 touchpad on Macbooks

[REGRESSION] Missing bcm5974 touchpad on Macbooks

[Takashi Iwai]

Hi,

we've received a few regression reports for openSUSE Leap about the
missing touchpad on Macbooks.  After debugging, this turned out to be
the backport of the commit 2b9c3eb32a699acdd4784d6b93743271b4970899
    Input: bcm5974 - check endpoint type before starting traffic

And, the same regression was confirmed on the upstream 6.8-rc6
kernel.

Reverting the commit above fixes the problem, the touchpad reappears.

The detailed hardware info is found at:
  https://bugzilla.suse.com/show_bug.cgi?id=1220030

Feel free to join the bugzilla above, or let me know if you need
something for debugging, then I'll delegate on the bugzilla.


thanks,

Takashi

Re: [REGRESSION] Missing bcm5974 touchpad on Macbooks

[Javier Carrasco]

On 04.03.24 09:35, Takashi Iwai wrote:
> Hi,
> 
> we've received a few regression reports for openSUSE Leap about the
> missing touchpad on Macbooks.  After debugging, this turned out to be
> the backport of the commit 2b9c3eb32a699acdd4784d6b93743271b4970899
>     Input: bcm5974 - check endpoint type before starting traffic
> 
> And, the same regression was confirmed on the upstream 6.8-rc6
> kernel.
> 
> Reverting the commit above fixes the problem, the touchpad reappears.
> 
> The detailed hardware info is found at:
>   https://bugzilla.suse.com/show_bug.cgi?id=1220030
> 
> Feel free to join the bugzilla above, or let me know if you need
> something for debugging, then I'll delegate on the bugzilla.
> 
> 
> thanks,
> 
> Takashi
> 

Hi Takashi,

The commit adds a check to ensure that the endpoint type is interrupt.

According to that report, the issue arose with a MacBook Pro 5.1 (no
button, only trackpad endpoint), so the check on the tp_ep address
(0x81) returns false. I assume that you see an error message
("Unexpected non-int endpoint) and  the probe function fails returning
-ENODEV.

Do you see any warning in the logs when you revert the commit? It was
added to prevent using wrong endpoint types, which will display the
following warning: "BOGUS urb xfer, pipe "some_number" != type
"another_number""

I am just wondering if for some reason the check on interrupt type is
wrong here.


Best regards,
Javier Carrasco

Re: [REGRESSION] Missing bcm5974 touchpad on Macbooks

[Linux regression tracking #adding (Thorsten Leemhuis)]

[TLDR: I'm adding this report to the list of tracked Linux kernel
regressions; the text you find below is based on a few templates
paragraphs you might have encountered already in similar form.
See link in footer if these mails annoy you.]

On 04.03.24 09:35, Takashi Iwai wrote:
> 
> we've received a few regression reports for openSUSE Leap about the
> missing touchpad on Macbooks.  After debugging, this turned out to be
> the backport of the commit 2b9c3eb32a699acdd4784d6b93743271b4970899
>     Input: bcm5974 - check endpoint type before starting traffic
> 
> And, the same regression was confirmed on the upstream 6.8-rc6
> kernel.
> 
> Reverting the commit above fixes the problem, the touchpad reappears.
> 
> The detailed hardware info is found at:
>   https://bugzilla.suse.com/show_bug.cgi?id=1220030

Thanks for the report. To be sure the issue doesn't fall through the
cracks unnoticed, I'm adding it to regzbot, the Linux kernel regression
tracking bot:

#regzbot ^introduced 2b9c3eb32a699ac
#regzbot title Input: missing bcm5974 touchpad on Macbooks
#regzbot duplicate: https://bugzilla.suse.com/show_bug.cgi?id=1220030
#regzbot ignore-activity

Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
--
Everything you wanna know about Linux kernel regression tracking:
https://linux-regtracking.leemhuis.info/about/#tldr
That page also explains what to do if mails like this annoy you.

Re: [REGRESSION] Missing bcm5974 touchpad on Macbooks

[Takashi Iwai]

On Mon, 04 Mar 2024 12:26:48 +0100,
Javier Carrasco wrote:
> 
> On 04.03.24 09:35, Takashi Iwai wrote:
> > Hi,
> > 
> > we've received a few regression reports for openSUSE Leap about the
> > missing touchpad on Macbooks.  After debugging, this turned out to be
> > the backport of the commit 2b9c3eb32a699acdd4784d6b93743271b4970899
> >     Input: bcm5974 - check endpoint type before starting traffic
> > 
> > And, the same regression was confirmed on the upstream 6.8-rc6
> > kernel.
> > 
> > Reverting the commit above fixes the problem, the touchpad reappears.
> > 
> > The detailed hardware info is found at:
> >   https://bugzilla.suse.com/show_bug.cgi?id=1220030
> > 
> > Feel free to join the bugzilla above, or let me know if you need
> > something for debugging, then I'll delegate on the bugzilla.
> > 
> > 
> > thanks,
> > 
> > Takashi
> > 
> 
> Hi Takashi,
> 
> The commit adds a check to ensure that the endpoint type is interrupt.
> 
> According to that report, the issue arose with a MacBook Pro 5.1 (no
> button, only trackpad endpoint), so the check on the tp_ep address
> (0x81) returns false. I assume that you see an error message
> ("Unexpected non-int endpoint) and  the probe function fails returning
> -ENODEV.

Right, there is the message.

> Do you see any warning in the logs when you revert the commit? It was
> added to prevent using wrong endpoint types, which will display the
> following warning: "BOGUS urb xfer, pipe "some_number" != type
> "another_number""

The revert was tested on the downstream kernel, but it has also the
check of bogus pipe, and there was no such warning, as far as I see
the report.

> I am just wondering if for some reason the check on interrupt type is
> wrong here.

I'll ask reporters to give the lsusb -v output so that we can take a
deeper look.  Also, I'm building a test kernel based on 6.8-rc7 with
the revert, and ask reporters to test with it, just to be sure.


thanks,

Takashi

Re: [REGRESSION] Missing bcm5974 touchpad on Macbooks

[Javier Carrasco]

On 04.03.24 13:45, Takashi Iwai wrote:
> On Mon, 04 Mar 2024 12:26:48 +0100,
> Javier Carrasco wrote:
>>
>> On 04.03.24 09:35, Takashi Iwai wrote:
>>> Hi,
>>>
>>> we've received a few regression reports for openSUSE Leap about the
>>> missing touchpad on Macbooks.  After debugging, this turned out to be
>>> the backport of the commit 2b9c3eb32a699acdd4784d6b93743271b4970899
>>>     Input: bcm5974 - check endpoint type before starting traffic
>>>
>>> And, the same regression was confirmed on the upstream 6.8-rc6
>>> kernel.
>>>
>>> Reverting the commit above fixes the problem, the touchpad reappears.
>>>
>>> The detailed hardware info is found at:
>>>   https://bugzilla.suse.com/show_bug.cgi?id=1220030
>>>
>>> Feel free to join the bugzilla above, or let me know if you need
>>> something for debugging, then I'll delegate on the bugzilla.
>>>
>>>
>>> thanks,
>>>
>>> Takashi
>>>
>>
>> Hi Takashi,
>>
>> The commit adds a check to ensure that the endpoint type is interrupt.
>>
>> According to that report, the issue arose with a MacBook Pro 5.1 (no
>> button, only trackpad endpoint), so the check on the tp_ep address
>> (0x81) returns false. I assume that you see an error message
>> ("Unexpected non-int endpoint) and  the probe function fails returning
>> -ENODEV.
> 
> Right, there is the message.
> 
>> Do you see any warning in the logs when you revert the commit? It was
>> added to prevent using wrong endpoint types, which will display the
>> following warning: "BOGUS urb xfer, pipe "some_number" != type
>> "another_number""
> 
> The revert was tested on the downstream kernel, but it has also the
> check of bogus pipe, and there was no such warning, as far as I see
> the report.
> 
>> I am just wondering if for some reason the check on interrupt type is
>> wrong here.
> 
> I'll ask reporters to give the lsusb -v output so that we can take a
> deeper look.  Also, I'm building a test kernel based on 6.8-rc7 with
> the revert, and ask reporters to test with it, just to be sure.
> 
> 
> thanks,
> 
> Takashi


Getting the output of lsusb would be awesome, thank you.

The bcm9547 driver has always made the assumption that the endpoint type
is interrupt, and the expected output from lsusb would be something like

bEndpointAddress     0x81  EP 1 IN
bmAttributes         3
Transfer Type        Interrupt

which is what the reverted commit checks.

I don't have the specific piece of hardware the report mentions, but I
triggered the probe with the endpoint type = interrupt and the check was
fine i.e. the probe did not fail. That made me think that the endpoint
type could be different, but I am dubious about that.

I will keep an eye on the bugzilla you linked, in case we get feedback
quickly.

Best regards,
Javier Carrasco

Re: [REGRESSION] Missing bcm5974 touchpad on Macbooks

[Javier Carrasco]

On 04.03.24 13:45, Takashi Iwai wrote:
> On Mon, 04 Mar 2024 12:26:48 +0100,
> Javier Carrasco wrote:
>>
>> On 04.03.24 09:35, Takashi Iwai wrote:
>>> Hi,
>>>
>>> we've received a few regression reports for openSUSE Leap about the
>>> missing touchpad on Macbooks.  After debugging, this turned out to be
>>> the backport of the commit 2b9c3eb32a699acdd4784d6b93743271b4970899
>>>     Input: bcm5974 - check endpoint type before starting traffic
>>>
>>> And, the same regression was confirmed on the upstream 6.8-rc6
>>> kernel.
>>>
>>> Reverting the commit above fixes the problem, the touchpad reappears.
>>>
>>> The detailed hardware info is found at:
>>>   https://bugzilla.suse.com/show_bug.cgi?id=1220030
>>>
>>> Feel free to join the bugzilla above, or let me know if you need
>>> something for debugging, then I'll delegate on the bugzilla.
>>>
>>>
>>> thanks,
>>>
>>> Takashi
>>>
>>
>> Hi Takashi,
>>
>> The commit adds a check to ensure that the endpoint type is interrupt.
>>
>> According to that report, the issue arose with a MacBook Pro 5.1 (no
>> button, only trackpad endpoint), so the check on the tp_ep address
>> (0x81) returns false. I assume that you see an error message
>> ("Unexpected non-int endpoint) and  the probe function fails returning
>> -ENODEV.
> 
> Right, there is the message.
> 
>> Do you see any warning in the logs when you revert the commit? It was
>> added to prevent using wrong endpoint types, which will display the
>> following warning: "BOGUS urb xfer, pipe "some_number" != type
>> "another_number""
> 
> The revert was tested on the downstream kernel, but it has also the
> check of bogus pipe, and there was no such warning, as far as I see
> the report.
> 
>> I am just wondering if for some reason the check on interrupt type is
>> wrong here.
> 
> I'll ask reporters to give the lsusb -v output so that we can take a
> deeper look.  Also, I'm building a test kernel based on 6.8-rc7 with
> the revert, and ask reporters to test with it, just to be sure.
> 
> 
> thanks,
> 
> Takashi

I retrieved the relevant node from the report that was uploaded a few
minutes ago:

Bus 003 Device 003: ID 05ac:0237 Apple, Inc. Internal Keyboard/Trackpad
(ISO)
Device Descriptor:
  bLength                18
  bDescriptorType         1
  bcdUSB               2.00
  bDeviceClass            0
  bDeviceSubClass         0
  bDeviceProtocol         0
  bMaxPacketSize0         8
  idVendor           0x05ac Apple, Inc.
  idProduct          0x0237 Internal Keyboard/Trackpad (ISO)
  bcdDevice            0.77
  iManufacturer           1 Apple, Inc.
  iProduct                2 Apple Internal Keyboard / Trackpad
  iSerial                 0
  bNumConfigurations      1
  Configuration Descriptor:
    bLength                 9
    bDescriptorType         2
    wTotalLength       0x0054
    bNumInterfaces          3
    bConfigurationValue     1
    iConfiguration          0
    bmAttributes         0xa0
      (Bus Powered)
      Remote Wakeup
    MaxPower               40mA
    Interface Descriptor:
      bLength                 9
      bDescriptorType         4
      bInterfaceNumber        0
      bAlternateSetting       0
      bNumEndpoints           1
      bInterfaceClass         3 Human Interface Device
      bInterfaceSubClass      1 Boot Interface Subclass
      bInterfaceProtocol      1 Keyboard
      iInterface              3 Apple Internal Keyboard
        HID Device Descriptor:
          bLength                 9
          bDescriptorType        33
          bcdHID               1.11
          bCountryCode           13 International (ISO)
          bNumDescriptors         1
          bDescriptorType        34 Report
          wDescriptorLength     156
         Report Descriptors:
           ** UNAVAILABLE **
      Endpoint Descriptor:
        bLength                 7
        bDescriptorType         5
        bEndpointAddress     0x83  EP 3 IN
        bmAttributes            3
          Transfer Type            Interrupt
          Synch Type               None
          Usage Type               Data
        wMaxPacketSize     0x000a  1x 10 bytes
        bInterval               8
    Interface Descriptor:
      bLength                 9
      bDescriptorType         4
      bInterfaceNumber        1
      bAlternateSetting       0
      bNumEndpoints           1
      bInterfaceClass         3 Human Interface Device
      bInterfaceSubClass      0
      bInterfaceProtocol      0
      iInterface              4 Touchpad
        HID Device Descriptor:
          bLength                 9
          bDescriptorType        33
          bcdHID               1.11
          bCountryCode            0 Not supported
          bNumDescriptors         1
          bDescriptorType        34 Report
          wDescriptorLength      27
         Report Descriptors:
           ** UNAVAILABLE **
      Endpoint Descriptor:
        bLength                 7
        bDescriptorType         5
        bEndpointAddress     0x81  EP 1 IN
        bmAttributes            3
          Transfer Type            Interrupt
          Synch Type               None
          Usage Type               Data
        wMaxPacketSize     0x0040  1x 64 bytes
        bInterval               2
    Interface Descriptor:
      bLength                 9
      bDescriptorType         4
      bInterfaceNumber        2
      bAlternateSetting       0
      bNumEndpoints           1
      bInterfaceClass         3 Human Interface Device
      bInterfaceSubClass      1 Boot Interface Subclass
      bInterfaceProtocol      2 Mouse
      iInterface              4 Touchpad
        HID Device Descriptor:
          bLength                 9
          bDescriptorType        33
          bcdHID               1.11
          bCountryCode            0 Not supported
          bNumDescriptors         1
          bDescriptorType        34 Report
          wDescriptorLength      52
         Report Descriptors:
           ** UNAVAILABLE **
      Endpoint Descriptor:
        bLength                 7
        bDescriptorType         5
        bEndpointAddress     0x84  EP 4 IN
        bmAttributes            3
          Transfer Type            Interrupt
          Synch Type               None
          Usage Type               Data
        wMaxPacketSize     0x0008  1x 8 bytes
        bInterval               8
Device Status:     0x0000
  (Bus Powered)


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?

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.

Best regards,
Javier Carrasco

Re: [REGRESSION] Missing bcm5974 touchpad on Macbooks

[Dmitry Torokhov]

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