Re: [PATCH v9 04/11] HID: asus: fortify keyboard handshake

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 5797 bytes --]

On Thu, 20 Nov 2025, Antheas Kapenekakis wrote:

> On Thu, 20 Nov 2025 at 17:41, Ilpo Järvinen
> <ilpo.jarvinen@linux.intel.com> wrote:
> >
> > On Thu, 20 Nov 2025, Antheas Kapenekakis wrote:
> >
> > > On Thu, 20 Nov 2025 at 15:15, Denis Benato <benato.denis96@gmail.com> wrote:
> > > >
> > > >
> > > > On 11/20/25 10:46, Antheas Kapenekakis wrote:
> > > > > Handshaking with an Asus device involves sending it a feature report
> > > > > with the string "ASUS Tech.Inc." and then reading it back to verify the
> > > > > handshake was successful, under the feature ID the interaction will
> > > > > take place.
> > > > >
> > > > > Currently, the driver only does the first part. Add the readback to
> > > > > verify the handshake was successful. As this could cause breakages,
> > > > > allow the verification to fail with a dmesg error until we verify
> > > > > all devices work with it (they seem to).
> > > > >
> > > > > Since the response is more than 16 bytes, increase the buffer size
> > > > > to 64 as well to avoid overflow errors.
> > > > >
> > > > > Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
> > > > > ---
> > > > >  drivers/hid/hid-asus.c | 32 +++++++++++++++++++++++++++++---
> > > > >  1 file changed, 29 insertions(+), 3 deletions(-)
> > > > >
> > > > > diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> > > > > index 6de402d215d0..5149dc7edfc5 100644
> > > > > --- a/drivers/hid/hid-asus.c
> > > > > +++ b/drivers/hid/hid-asus.c
> > > > > @@ -48,7 +48,7 @@ MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad");
> > > > >  #define FEATURE_REPORT_ID 0x0d
> > > > >  #define INPUT_REPORT_ID 0x5d
> > > > >  #define FEATURE_KBD_REPORT_ID 0x5a
> > > > > -#define FEATURE_KBD_REPORT_SIZE 16
> > > > > +#define FEATURE_KBD_REPORT_SIZE 64
> > > > >  #define FEATURE_KBD_LED_REPORT_ID1 0x5d
> > > > >  #define FEATURE_KBD_LED_REPORT_ID2 0x5e
> > > > >
> > > > > @@ -394,14 +394,40 @@ static int asus_kbd_set_report(struct hid_device *hdev, const u8 *buf, size_t bu
> > > > >
> > > > >  static int asus_kbd_init(struct hid_device *hdev, u8 report_id)
> > > > >  {
> > > > > +     /*
> > > > > +      * The handshake is first sent as a set_report, then retrieved
> > > > > +      * from a get_report. They should be equal.
> > > > > +      */
> > > > >       const u8 buf[] = { report_id, 0x41, 0x53, 0x55, 0x53, 0x20, 0x54,
> > > > >                    0x65, 0x63, 0x68, 0x2e, 0x49, 0x6e, 0x63, 0x2e, 0x00 };
> > > > > +     u8 *readbuf;
> > > > >       int ret;
> > > > >
> > > > >       ret = asus_kbd_set_report(hdev, buf, sizeof(buf));
> > > > > -     if (ret < 0)
> > > > > -             hid_err(hdev, "Asus failed to send init command: %d\n", ret);
> > > > > +     if (ret < 0) {
> > > > > +             hid_err(hdev, "Asus failed to send handshake: %d\n", ret);
> > > > > +             return ret;
> > > > > +     }
> > > > > +
> > > > > +     readbuf = kzalloc(FEATURE_KBD_REPORT_SIZE, GFP_KERNEL);
> > > > I see my suggestion to use __free here didn't materialize in code using
> > > > it even after Ilpo kindly wrote how to correctly use it.
> > > >
> > > > I think you can move the readbuf assignment right below buf and
> > > > take into account what Ilpo said.
> > > >
> > > > I don't expect new variables will be added here ever again,
> >
> > It's also about always doing the right thing so others will pick up the
> > pattern (for the cases when it's needed).
> >
> > > > but I agree with Ilpo that it's a good idea here to write code
> > > > accounting for that possibility.
> > > >
> > > > It is my understanding that who proposes patches is expected to
> > > > resolve discussions when changes are proposed or to take into
> > > > account requested changes and submit a modified version.
> > >
> > > It was ambiguous. I interpreted Ilpo's email as a dismissal
> >
> > I tried to explain how to use it, not to suggest cleanup.h shouldn't be
> > used.
> 
> Ok, I'll wait a few days and do another revision, doing some rewording
> as well. Hopefully that will cover everything. To that extent, try to
> finish reviewing the latter part of the series before that revision.
> 
> I'm a bit concerned with introducing kfree here because I do not know
> how to use it and it might regress, but it should be ok.

You probably meant to say __free(), there are other things that could be 
put inside __free() beyond kfree (no need to ack if that was the case).

It's not rocket science, basically the compiler performs the freeing 
function when the scope the variable is in goes away. For error handling 
rollback, this avoids need to do the cleanup call manually and no path is 
left accidently without cleanup.

For cases, where you want to preserve a pointer to the allocated thing,
there are additional wrappers that one uses when returning the pointer
out of function (only gotcha here is that it will NULL the local 
variable in question, so the variable cannot be dereferenced after that 
point; been there, done that and had to debug a boot issue :-)).

> I'd rather push the init down instead of pulling it up. Referencing
> other code samples for kfree it is acceptable to push the variable
> definition down, right?

Yes.

Mid-function definitions used to be forbidding on the compiler level 
but it was changed exactly because adding this auto cleanup framework. So 
while top definition is still the general requirement, using __free() is an 
exception to that rule and should be defined at the spot where we make 
the "alloc" to ensure the teardown order is exactly reverse of the alloc 
order (the order you introduce variables is the reverse of the order in 
which the compiler will perform each tear down).

-- 
 i.