linux-kernel - Re: [PATCH v2] HID: corsair-void: Add Corsair Void headset family driver

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]

Message-ID: <CALTg27mgOx3W3WENxFh0sEEeNYKEjrZCEQGoBi9=vjgiaZnZtQ@mail.gmail.com>
Date: Tue, 20 Aug 2024 01:21:40 +0100
From: Stuart <stuart.a.hayhurst@...il.com>
To: Markus Elfring <Markus.Elfring@....de>
Cc: linux-input@...r.kernel.org, LKML <linux-kernel@...r.kernel.org>, 
	Benjamin Tissoires <bentiss@...nel.org>, Jiri Kosina <jikos@...nel.org>
Subject: Re: [PATCH v2] HID: corsair-void: Add Corsair Void headset family driver

> * How do you think about to use the attribute “__free(kfree)” at more places accordingly?
>  https://elixir.bootlin.com/linux/v6.11-rc4/source/include/linux/slab.h#L282

Done, thanks

> * Would you like to reduce scopes for such local variables?

I thought the kernel programming style put the local variables at the
start of the function?

> Can condition checks be merged with the same return value (for less statements)?

Done

> Can such a size determination be explained better?

Well it allocates 3 bytes and immediately fills them, so I'm not sure
what I'd write except stating the obvious.
Unless you mean why it has to be allocated rather than just using the stack?

> I suggest to use another local variable for the previous return value
> so that such a reset can be avoided.

Done

> Is there a need to organise device attributes into separate subgroups?

If the driver ends up registering / deregistering the attributes
depending on if it's wired or wireless headset and the headset's
connection state, I'd say so.
Otherwise if the driver sticks with returning -ENODEV, then I don't
see a reason to split them up.

> I propose to omit the explicit initialisation for this local variable.

Done

> Please improve such a size determination.
>
> See also:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst?h=v6.11-rc4#n953

Assuming you just wanted me to use `sizeof(*drvdata)`, done

> Please apply the statement “return 0;” instead.

Done

> > +/*failed_after_hid_start:
> > +     hid_hw_stop(hid_dev);*/
>
> Please reconsider the need once more also for this information.

Removed it

Thanks,
Stuart