| 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
| ||
|
Date: Tue, 02 Feb 2010 15:27:11 -0600
From: Bob Rodgers <Robert_Rodgers@...l.com>
To: Linux-kernel <linux-kernel@...r.kernel.org>,
Matthew Garrett <mjg59@...f.ucam.org>, lenb@...nel.org,
rpurdie@...ys.net
Subject: Re: Re: [RFC] Dell activity led WMI driver
On Mon, Feb 01, 2010 at 04:44:36PM -0600, Bob Rodgers wrote:
> This has been internally reviewed, and we are ready for outside review
> and feedback. My colleagues have identified the dell-wmi module as a
> suitable container in lieu of a stand-alone module specifically for
> this driver, which makes sense, but we welcome advice. We are
> submitting it as a stand-alone module for now because that is how we
> developed and tested it. We would like this to be included upstream
> after it has been reviewed.
On Mon, Feb 01, 2010 at 5:02 PM, Matthew Garrett wrote:
> It uses a different GUID to the event interface used by dell-wmi,
> so right now there's no inherent reason to integrate it into that
> rather than keeping it as a separate driver. On the other hand,
> if the GUID is useful for other kinds of system control rather
> than just the LED then dell-wmi may want to make use of that
> functionality in the future. In that case we'd need it to be
> incorporated into the dell-wmi driver.
>
> So, really, it depends on the interface. If this GUID is specific to
LEDs,
> then keep it separate. Otherwise it should be integrated.
>
> I've got a few comments on the code...
>
> > // Error Result Codes:
>
> C99 style comments are usually discouraged in the kernel.
>
> > // Devide ID
>
> Typo?
>
> > // LED Commands
> > #define CMD_LED_ON 16
> > #define CMD_LED_OFF 17
> > #define CMD_LED_BLINK 18
>
> Use of whitespace isn't very consistent here.
>
> > struct bios_args {
> > unsigned char Length;
> > unsigned char ResultCode;
> > unsigned char DeviceId;
> > unsigned char Command;
> > unsigned char OnTime;
> > unsigned char OffTime;
> > unsigned char Reserved[122];
> > };
> Mm. We're also not usually big on CamelCasing in variable names -
> it'd be preferable to use underscores. That's true for the rest of
this, too.
>
> > // free the output ACPI object allocated by ACPI driver
>
> Probably don't need this comment.
>
> > static void led_on(void)
> > {
> > dell_led_perform_fn(3, // Length of command
> > INTERFACE_ERROR, // Init result code to INTERFACE_ERROR
> > DEVICE_ID_PANEL_BACK, // Device ID
> > CMD_LED_ON, // Command
> > 0, // not used
> > 0); // not used
> > }
>
> Whitespace is a bit odd here, again.
>
> Other than that, it looks good. You probably want to run it
> through Scripts/checkpatch.pl in the kernel tree to perform
> further style checks, but I can't see any functional issues.
> --
On Tue, Feb 02, 2010 at 6:15 AM, Dan Carpenter wrote:
> It would be better to not put the bios_return struct on the stack.
> Make it a pointer and use kmalloc().
>
> It's a pity the Makefile bits weren't included.
Thank you for all the feedback. We have reviewed the feedback and made
the recommended changes/corrections.
> So, really, it depends on the interface. If this GUID is specific to
LEDs,
> then keep it separate. Otherwise it should be integrated.
Since the GUID is specific to LEDs, we will keep the driver separate
rather than integrate it into the dell-wmi module.
> C99 style comments are usually discouraged in the kernel.
Removed.
> > // Devide ID
>
> Typo?
Yes. Fixed.
> Use of whitespace isn't very consistent here.
Fixed.
> Mm. We're also not usually big on CamelCasing in variable names -
> it'd be preferable to use underscores. That's true for the rest of
this, too.
Corrected. Changed to underscores.
> > // free the output ACPI object allocated by ACPI driver
>
> Probably don't need this comment.
Removed.
> > CMD_LED_ON, // Command
> > 0, // not used
> > 0); // not used
> > }
>
> Whitespace is a bit odd here, again.
Fixed.
> Other than that, it looks good. You probably want to run it
> through Scripts/checkpatch.pl in the kernel tree to perform
> further style checks, but I can't see any functional issues.
We ran the file through Scripts/checkpatch.pl and the script reported 0
errors and 0 warnings.
> It would be better to not put the bios_return struct on the stack.
> Make it a pointer and use kmalloc().
Changed to a pointer.
> It's a pity the Makefile bits weren't included.
The Makefile is now included.
The updated dell_led.c file and the Makefile are attached.
Regards,
Bob Rodgers
Louis Davis
View attachment "Makefile" of type "text/plain" (304 bytes)
View attachment "dell_led.c" of type "text/plain" (5116 bytes)
Powered by blists - more mailing lists