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] [thread-next>] [day] [month] [year] [list]

Date: Tue, 26 Mar 2024 09:58:12 +0100
From: Łukasz Majczak <lma@...omium.org>
To: Doug Anderson <dianders@...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

> 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.

>
> 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.

> 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.

> I'm not a huge fan of having to check "tries" and "ret" twice.
> Personally I'd rather see a "while (true)" loop and test the condition
> once to break out. AKA:
>
> while (true) {
>   ret = i2c_...
>   tries--;
>   if (tries == 0 || ret >= 0)
>     break;
>   udelay(400);
> }
>
> ...if you feel very strongly about the way you have coded it, though,
> I won't stand in your way.
I don't have emotional bond to this code ;), thanks.
>
> Pretty much all my comments are just nits and, since I'm not the
> maintainer here, my opinion is just an opinion. I'd wait at least a
> little while for the maintainers to comment before posting v2. I'm
> happy to give a Reviewed-by tag when some of the nits are fixed.
>
> -Doug
Thank you Doug for all your input.

Best regards,
Lukasz