All of lore.kernel.org
 help / color / mirror / Atom feed
From: Bastien Nocera <hadess@hadess.net>
To: Linus Torvalds <torvalds@linux-foundation.org>,
	Linux regressions mailing list <regressions@lists.linux.dev>
Cc: "Benjamin Tissoires" <benjamin.tissoires@redhat.com>,
	"Bagas Sanjaya" <bagasdotme@gmail.com>,
	"Filipe Laíns" <lains@riseup.net>,
	"Jiri Kosina" <jikos@kernel.org>,
	"open list:HID CORE LAYER" <linux-input@vger.kernel.org>,
	LKML <linux-kernel@vger.kernel.org>,
	guy.b@bluewin.ch
Subject: Re: [regression] Since kernel 6.3.1 logitech unify receiver not working properly
Date: Wed, 31 May 2023 10:46:02 +0200	[thread overview]
Message-ID: <6d911cceaaf894754a1183a449d6d3deaf354bd8.camel@hadess.net> (raw)
In-Reply-To: <CAHk-=whvhkSk6m8_AidhofgR9nq0Md+HbNad5r1RE69tZgbv6Q@mail.gmail.com>

On Mon, 2023-05-22 at 11:23 -0700, Linus Torvalds wrote:
> On Mon, May 22, 2023 at 5:38 AM Linux regression tracking (Thorsten
> Leemhuis) <regressions@leemhuis.info> wrote:
> > 
> > FWIW, in case anybody is interested in a status update: one
> > reporter
> > bisected the problem down to 586e8fede79 ("HID: logitech-hidpp:
> > Retry
> > commands when device is busy"); reverting that commit on-top of 6.3
> > fixes the problem for that reporter. For that reporter things also
> > work
> > on 6.4-rc; but for someone else that is affected that's not the
> > case.
> 
> Hmm. It's likely timing-dependent.
> 
> But that code is clearly buggy.
> 
> If the wait_event_timeout() returns early, the device hasn't replied,
> but the code does
> 
>                 if (!wait_event_timeout(hidpp->wait, hidpp-
> >answer_available,
>                                         5*HZ)) {
>                         dbg_hid("%s:timeout waiting for response\n",
> __func__);
>                         memset(response, 0, sizeof(struct
> hidpp_report));
>                         ret = -ETIMEDOUT;
>                 }
> 
> and then continues to look at the response _anyway_.
> 
> Now, depending on out hardening options, that response may have been
> initialized by the compiler, or may just be random stack contents.

It's kzalloc()'ed in the 2 places it's used, hidpp_send_message_sync().

> That bug is pre-existing (ie the problem was not introduced by that
> commit), but who knows if the retry makes things worse (ie if it then
> triggers on a retry, the response data will be the *previous*
> response).
> 
> The whole "goto exit" games should be removed too, because we're in a
> for-loop, and instead of "goto exit" it should just do "break".
> 
> IOW, something like this might be worth testing.
> 
> That said, while I think the code is buggy, I doubt this is the
> actual
> cause of the problem people are reporting. But it would be lovely to
> hear if the attached patch makes any difference, and I think this is
> fixing a real - but unlikely - problem anyway.
> 
> And obviously it might be helpful to actually enable those dbg_hid()
> messages, but I didn't look at what the magic config option to do so
> was.

Thomas Weißschuh's patch ("HID: use standard debug APIs") linked all
those debug calls to the dynamic debugging system, so something like
this will work after boot:
echo 'file hid-logitech-hidpp.c +p' > /sys/kernel/debug/dynamic_debug/control

Adding this to the kernel command-line to get some debug during boot
should work:
dyndbg="file hid-logitech-hidpp.c +p"

In both cases, check it's enabled and that the messages can be printed
with:
grep -i hidpp /sys/kernel/debug/dynamic_debug/control

> NOTE! Patch below *ENTIRELY* untested. I just looked at the code when
> that commit was mentioned, and went "that's not right"...

I sent a similar patch before seeing your version, in answer to a
separate report I was sent. It doesn't change the style of the code,
and just fixes that one omission:
https://patchwork.kernel.org/project/linux-input/patch/20230531082428.21763-1-hadess@hadess.net/

Cheers

> 
>                      Linus


  parent reply	other threads:[~2023-05-31  8:57 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-05-08  9:55 [regression] Since kernel 6.3.1 logitech unify receiver not working properly Linux regression tracking (Thorsten Leemhuis)
2023-05-11 11:58 ` Thorsten Leemhuis
2023-05-16 13:24   ` Bagas Sanjaya
2023-05-16 13:34     ` Benjamin Tissoires
2023-05-22 12:38       ` Linux regression tracking (Thorsten Leemhuis)
2023-05-22 18:23         ` Linus Torvalds
2023-05-23 12:31           ` Jiri Kosina
2023-05-24 15:27             ` Benjamin Tissoires
2023-05-25 11:10               ` Jiri Kosina
2023-05-25 11:19                 ` Benjamin Tissoires
2023-05-26 18:41                 ` Jiri Kosina
2023-05-28 12:21                   ` Thorsten Leemhuis
2023-05-31  8:43                     ` Jiri Kosina
2023-05-31  8:46           ` Bastien Nocera [this message]
2023-05-16 13:53     ` Thorsten Leemhuis
2023-05-17 13:17   ` Bagas Sanjaya

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=6d911cceaaf894754a1183a449d6d3deaf354bd8.camel@hadess.net \
    --to=hadess@hadess.net \
    --cc=bagasdotme@gmail.com \
    --cc=benjamin.tissoires@redhat.com \
    --cc=guy.b@bluewin.ch \
    --cc=jikos@kernel.org \
    --cc=lains@riseup.net \
    --cc=linux-input@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=regressions@lists.linux.dev \
    --cc=torvalds@linux-foundation.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.