Message-ID: <52838553-0c3f-3a79-2bd7-63dd388b1a5f@compro.net>
Date:   Thu, 11 Mar 2021 12:39:00 -0500
From:   Mark Hounschell <markh@...pro.net>
To:     Hans de Goede <hdegoede@...hat.com>,
        Filipe Laíns <lains@...hlinux.org>,
        Jiri Kosina <jkosina@...e.cz>, sashal@...nel.org,
        Linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: Logitech G602 wireless mouse kernel error messages in 5.10.11+
 kernels

On 3/10/21 4:48 PM, Hans de Goede wrote:
> Hi,
> 
> On 3/10/21 9:55 PM, Filipe Laíns wrote:
>> On Wed, 2021-03-10 at 15:24 -0500, Mark Hounschell wrote:
>>>
>>> That is correct, I don't have any buttons bound to keyboard events. With
>>> the original patch the G4(forward) and G5(Backward) buttons work in a
>>> browser. I guess G7, G8, and G9 buttons are programmable to keyboard events?
>>>
>>> However this patch does not seem to fix the messages I get.
>>>
>>> Regards
>>> Mark
>>
>> Those events belong to the USB HID button usage page and are sent by the
>> receiver in the HID device with the unnumbered report descriptor, so they are
>> not affected.
>>
>> Looking at the report descriptor for the other HID device, I see a report ID of
>> 128 (0x80) used for a vendor application, I am not really sure what it is used
>> for and can't seem to trigger my device to send it.
>>
>> I am gonna guess this is the device reporting the pressed buttons via vendor
>> reports or something like that. Speaking as the person who added support for
>> this device in libratbag, this report is very likely not something that we don't
>> need in our custom drivers and just likely something extra that Logitech built
>> to achieve something custom in the Windows driver. FWIW, this device is a very
>> weird one, it does not even follow Logitech's own spec :P
>>
>> Seeing this report the driver chugs.
>>
>> 	if (report > REPORT_TYPE_RFREPORT_LAST) {
>> 		hid_err(hdev, "Unexpected input report number %d\n", report);
>> 		return;
>> 	}
>>
>> Causing your
>>
>> [   36.471326] logitech-djreceiver 0003:046D:C537.0002: Unexpected input report number 128
>> [   36.565317] logitech-djreceiver 0003:046D:C537.0002: Unexpected input report number 128
>> [   42.390321] logitech-djreceiver 0003:046D:C537.0002: Unexpected input report number 128
>>
>> I feel like the correct fix for these cases is not to consume the report and not
>> forward it to device node, but rather to forward it to the receiver node.
>>
>> (looping in Hans)
>> Hans, you introduced this code, do you remember why? Where did
>> REPORT_TYPE_RFREPORT_LAST get its value from and what is the purpose of this
>> check?
>> Shouldn't we just keep forwarding unknown reports to the receiver node? Is there
>> any technical limitation to do that? I am not too familiar with this part of the
>> code.
> 
> The code used by the recvr_type_gaming_hidpp receivers is shared with all the
> other non-unifying receivers. Even though these receivers are not unifying the
> non gaming versions may still have multiple devices (typically a keyboard + a mouse)
> paired with them.
> 
> The standard HID interfaces which these devices emulate are usually split in
> at least 2 HID interfaces:
> 
> 1. A keyboard following the requirements of the "boot keyboard" subclass of the
> USB HID class, so that the keyboard works inside say the BIOS setup screen.
> This uses a single unnumbered HID report
> 
> 2. A mouse + media-keys interface, which delivers numbered reports, including the
> special Logitech HID++ reports for things like battery monitoring, but also some
> special keys, which have their own sub-addressing embedded inside the reports.
> 
> The driver asks the receiver for a list of paired devices and then builts a list
> of devices, which are then instantiated as child-HID devices which are
> handled by the drivers/hid/hid-logitech-hidpp.c driver.
> 
> Any input reports received by drivers/hid/hid-logitech-dj.c are then forwarded
> to the instantiated child devices, where they are actually processed.
> 
> The problem is that there is not a 1:1 relation between the interfaces and
> the instantiated child-devices, so the driver aggregates all input-reports
> from both interfaces together and then dispatches / forwards them to the
> child-devices using its own internal addressing.
> 
> This forwarding uses 2 different addressing schemes:
> 
> 1. If the report received is a special HID++ report, then it is forwarded to
> paired-dev child-dev matching the HID++ device-index which is embedded
> inside these special reports.
> 
> 2. If a normal (unnumbered or numbered) report is received then that report is
> forwarded based on the report-number.  What happens here is that each paired-dev
> which the hid-logitech-dj.c code instantiates has a bitmask associated with it
> which indicates which kind of reports it consumes. So e.g. a normal mouse will
> only consume mouse input-reports (STD_MOUSE, report-id 2) and a keyboard
> will consume all of:
> 
> #define STD_KEYBOARD                            BIT(1)
> #define MULTIMEDIA                              BIT(3)
> #define POWER_KEYS                              BIT(4)
> #define MEDIA_CENTER                            BIT(8)
> #define KBD_LEDS                                BIT(14)
> 
> When forwarding these normal (unnumbered or numbered) reports, the list of
> paired devices is searched and the report is forwarded to the first paired-dev
> which reports_supported bitmask includes the report-nr:
> 
> 	spin_lock_irqsave(&djrcv_dev->lock, flags);
>          for (i = 0; i < (DJ_MAX_PAIRED_DEVICES + DJ_DEVICE_INDEX_MIN); i++) {
>                  dj_dev = djrcv_dev->paired_dj_devices[i];
>                  if (dj_dev && (dj_dev->reports_supported & BIT(report))) {
>                          logi_dj_recv_forward_report(dj_dev, data, size);
>                          spin_unlock_irqrestore(&djrcv_dev->lock, flags);
>                          return;
>                  }
>          }
> 
> The:
> 
>          if (report > REPORT_TYPE_RFREPORT_LAST) {
>                  hid_err(hdev, "Unexpected input report number %d\n", report);
>                  return;
>          }
> 
> check happens before this to ensure that report can be represented
> as a bitmask, IOW to ensure that BIT(report) does what we expect it to do,
> without any wrapping BIT(128) cannot be represented in a 64 bit integer,
> so then we end up with undefined behavior. The result will likely be either
> 0x00 or 0x01, but it certainly will not do what we want.
> 
> I hope that helps explain why the check is there.
> 
> As for what to do about the errors, I agree with you that the code which is
> logging these errors should check for this new special input-reports with
> a report-number of 128 and just silently discard these.
> 
> Regards,
> 
> Hans
> 

I am unsubscribing from kernel.org mailing list. I only subscribed to 
report this issue. Please keep me CC'd to this thread.

Thanks
Mark

Powered by blists - more mailing lists