| 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: Wed, 31 Aug 2016 17:34:56 -0700
From: Darren Hart <dvhart@...radead.org>
To: Azael Avalos <coproscefalo@...il.com>
Cc: platform-driver-x86@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/3] toshiba_acpi: Clean up variable declaration
On Mon, Aug 29, 2016 at 09:37:34AM -0600, Azael Avalos wrote:
> This patch moves all the multiple line variable declaration to a
> single line declaration (except variables being initialized)
> following the reverse tree order, to conform to the practices
> of the kernel.
So... I don't really want to spend a lot of time on this patch :-) but... there
are some inconsistencies with the stated intent....
>
> Signed-off-by: Azael Avalos <coproscefalo@...il.com>
> ---
> drivers/platform/x86/toshiba_acpi.c | 33 +++++++++++++++------------------
> 1 file changed, 15 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/platform/x86/toshiba_acpi.c b/drivers/platform/x86/toshiba_acpi.c
> index 9d60a40..54dea64 100644
> --- a/drivers/platform/x86/toshiba_acpi.c
> +++ b/drivers/platform/x86/toshiba_acpi.c
> @@ -321,10 +321,9 @@ static int write_acpi_int(const char *methodName, int val)
> static acpi_status tci_raw(struct toshiba_acpi_dev *dev,
> const u32 in[TCI_WORDS], u32 out[TCI_WORDS])
> {
> + union acpi_object in_objs[TCI_WORDS], out_objs[TCI_WORDS + 1];
> struct acpi_object_list params;
> - union acpi_object in_objs[TCI_WORDS];
> struct acpi_buffer results;
> - union acpi_object out_objs[TCI_WORDS + 1];
> acpi_status status;
> int i;
>
> @@ -387,9 +386,8 @@ static int sci_open(struct toshiba_acpi_dev *dev)
> {
> u32 in[TCI_WORDS] = { SCI_OPEN, 0, 0, 0, 0, 0 };
> u32 out[TCI_WORDS];
> - acpi_status status;
> + acpi_status status = tci_raw(dev, in, out);
>
> - status = tci_raw(dev, in, out);
> if (ACPI_FAILURE(status)) {
> pr_err("ACPI call to open SCI failed\n");
> return 0;
> @@ -425,9 +423,8 @@ static void sci_close(struct toshiba_acpi_dev *dev)
> {
> u32 in[TCI_WORDS] = { SCI_CLOSE, 0, 0, 0, 0, 0 };
> u32 out[TCI_WORDS];
> - acpi_status status;
> + acpi_status status = tci_raw(dev, in, out);
>
> - status = tci_raw(dev, in, out);
> if (ACPI_FAILURE(status)) {
> pr_err("ACPI call to close SCI failed\n");
> return;
> @@ -509,7 +506,8 @@ static enum led_brightness toshiba_illumination_get(struct led_classdev *cdev)
> {
> struct toshiba_acpi_dev *dev = container_of(cdev,
> struct toshiba_acpi_dev, led_dev);
> - u32 state, result;
> + u32 result;
> + u32 state;
>
> /* First request : initialize communication. */
> if (!sci_open(dev))
> @@ -672,9 +670,9 @@ static int toshiba_touchpad_get(struct toshiba_acpi_dev *dev, u32 *state)
> /* Eco Mode support */
> static void toshiba_eco_mode_available(struct toshiba_acpi_dev *dev)
> {
> - acpi_status status;
> u32 in[TCI_WORDS] = { HCI_GET, HCI_ECO_MODE, 0, 0, 0, 0 };
> u32 out[TCI_WORDS];
> + acpi_status status;
>
> dev->eco_supported = 0;
> dev->eco_led_registered = false;
> @@ -1282,9 +1280,9 @@ static struct proc_dir_entry *toshiba_proc_dir;
> /* LCD Brightness */
> static int __get_lcd_brightness(struct toshiba_acpi_dev *dev)
> {
> + int brightness = 0;
> u32 result;
> u32 value;
> - int brightness = 0;
>
> if (dev->tr_backlight_supported) {
> int ret = get_tr_backlight_status(dev, &value);
> @@ -1377,7 +1375,7 @@ static ssize_t lcd_proc_write(struct file *file, const char __user *buf,
> struct toshiba_acpi_dev *dev = PDE_DATA(file_inode(file));
> char cmd[42];
> size_t len;
> - int levels = dev->backlight_dev->props.max_brightness + 1;
> + int levels;
> int value;
>
> len = min(count, sizeof(cmd) - 1);
> @@ -1385,6 +1383,7 @@ static ssize_t lcd_proc_write(struct file *file, const char __user *buf,
> return -EFAULT;
> cmd[len] = '\0';
>
> + levels = dev->backlight_dev->props.max_brightness + 1;
> if (sscanf(cmd, " brightness : %i", &value) != 1 &&
> value < 0 && value > levels)
> return -EINVAL;
> @@ -1447,10 +1446,8 @@ static ssize_t video_proc_write(struct file *file, const char __user *buf,
> struct toshiba_acpi_dev *dev = PDE_DATA(file_inode(file));
> char *buffer;
> char *cmd;
> + int lcd_out, crt_out, tv_out;
> int remain = count;
> - int lcd_out = -1;
> - int crt_out = -1;
> - int tv_out = -1;
> int value;
> int ret;
> u32 video_out;
> @@ -1486,6 +1483,7 @@ static ssize_t video_proc_write(struct file *file, const char __user *buf,
>
> kfree(cmd);
>
> + lcd_out = crt_out = tv_out = -1;
> ret = get_video_status(dev, &video_out);
> if (!ret) {
> unsigned int new_video_out = video_out;
> @@ -1980,9 +1978,9 @@ static ssize_t usb_sleep_charge_store(struct device *dev,
> const char *buf, size_t count)
> {
> struct toshiba_acpi_dev *toshiba = dev_get_drvdata(dev);
> - u32 mode;
> int state;
> int ret;
> + u32 mode;
This is not consistent with the stated changes in the changelog, u32 mode is
longer than int ret, and should come before.
>
> ret = kstrtoint(buf, 0, &state);
> if (ret)
> @@ -2021,11 +2019,10 @@ static ssize_t sleep_functions_on_battery_show(struct device *dev,
> char *buf)
> {
> struct toshiba_acpi_dev *toshiba = dev_get_drvdata(dev);
> - u32 state;
> - int bat_lvl;
> - int status;
> + int bat_lvl, status;
> int ret;
> int tmp;
> + u32 state;
Here too (I'd just drop this change)
>
> ret = toshiba_sleep_functions_status_get(toshiba, &state);
> if (ret < 0)
> @@ -3015,8 +3012,8 @@ static int toshiba_acpi_add(struct acpi_device *acpi_dev)
> {
> struct toshiba_acpi_dev *dev;
> const char *hci_method;
> - u32 dummy;
> int ret = 0;
> + u32 dummy;
And here, per your exception for initialization.
So, what I've done is fixed the last three instances, and queued this to
for-next to save both our time on back and forth. Let me know if you object.
--
Darren Hart
Intel Open Source Technology Center
Powered by blists - more mailing lists