linux-kernel - Re: [PATCH] HID: i2c-hid: wait for i2c touchpad deep-sleep to power-up transition

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date: Tue, 26 Mar 2024 06:43:21 -0700
From: Doug Anderson <dianders@...omium.org>
To: Łukasz Majczak <lma@...omium.org>
Cc: Jiri Kosina <jikos@...nel.org>, Dmitry Torokhov <dtor@...omium.org>, 
	Benjamin Tissoires <benjamin.tissoires@...hat.com>, Hans de Goede <hdegoede@...hat.com>, 
	Maxime Ripard <mripard@...nel.org>, Kai-Heng Feng <kai.heng.feng@...onical.com>, 
	Johan Hovold <johan+linaro@...nel.org>, linux-input@...r.kernel.org, 
	linux-kernel@...r.kernel.org, Radoslaw Biernacki <rad@...omium.org>
Subject: Re: [PATCH] HID: i2c-hid: wait for i2c touchpad deep-sleep to
 power-up transition

Hi,

On Tue, Mar 26, 2024 at 1:58 AM Łukasz Majczak <lma@...omium.org> wrote:
>
> > nit: checkpatch should have yelled at you saying that you should
> > specify a commit in the format:
> >
> > commit b3a81b6c4fc6 ("HID: i2c-hid: check if device is there before
> > really probing")
> >
> I will do it, but I did run the checkpatch (with --strict option) and
> it didn't complain about anything.

Weird that checkpatch didn't yell, but perhaps somehow your commit
message didn't trigger its regex. ;-)



> > nit: I believe your sign off should be last. It's also unclear why two
> > signoffs. Did Radoslaw author it and you changed it? ...or was it
> > Co-Developed-by, or ...? You'll probably need to adjust your tags a
> > bit depending on the answers.
> >
> Yes, we've discussed this patch together and the original
> investigation was done by Rad.

Sounds good. Probably the best way to tag is these tags in this order:

Co-developed-by: Radoslaw Biernacki <rad@...omium.org>
Signed-off-by: Radoslaw Biernacki <rad@...omium.org>
Signed-off-by: Lukasz Majczak <lma@...omium.org>


> > Having both ends of the usleep be 400 is iffy. In this case it's at
> > probe time so I wonder if udelay() is better? If not, maybe give at
> > least _some_ margin?
> >
> >
> > > +       } while (tries-- > 0 && ret < 0);
> >
> According to Documentation/timers/timers-howto.rst:
> " SLEEPING FOR ~USECS OR SMALL MSECS ( 10us - 20ms):
>                 * Use usleep_range"
> It was also pointed out by checkpath (when I initially used msleep).
> I think giving some margin (eg. 400,500) would be ok.

Yeah, usleep_range(400, 500) would be fine. udelay(400) would also be
OK. The later would be more "accurate" but also more wasteful of CPU
cycles. Given that this is at probe time and only run a small handful
of times, it probably doesn't matter lots though perhaps the sleeping
function would allow more parallelism of other running probes.


-Doug