linux-kernel - Re: [PATCH 2/3] toshiba_acpi: Change error checking logic from TCI functions

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:   Mon, 29 Aug 2016 09:32:17 -0600
From:   Azael Avalos <coproscefalo@...il.com>
To:     Darren Hart <dvhart@...radead.org>
Cc:     "platform-driver-x86@...r.kernel.org" 
        <platform-driver-x86@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 2/3] toshiba_acpi: Change error checking logic from TCI functions

Hi Darren,

2016-08-28 10:56 GMT-06:00 Darren Hart <dvhart@...radead.org>:
> On Tue, Aug 16, 2016 at 12:06:16PM -0600, Azael Avalos wrote:
>> Currently the success/error checking logic is intermixed, making the
>> code a bit cumbersome to understand.
>>
>> This patch changes the affected functions to first check for errors
>> and take appropriate actions, then check for the supported features.
>>
>> This patch also separates the error check from the acpi_status and
>> the tci_raw function call error check, as those two are completely
>> unrelated and were nested in if/else statements.
>
> Thanks, this is a good improvement. One questions below...
>
>>
>> Signed-off-by: Azael Avalos <coproscefalo@...il.com>
>> ---
>>  drivers/platform/x86/toshiba_acpi.c | 222 ++++++++++++++++++++++--------------
>>  1 file changed, 135 insertions(+), 87 deletions(-)
>>
>> diff --git a/drivers/platform/x86/toshiba_acpi.c b/drivers/platform/x86/toshiba_acpi.c
>> index c6fc5cc..2256cf5 100644
>> --- a/drivers/platform/x86/toshiba_acpi.c
>> +++ b/drivers/platform/x86/toshiba_acpi.c
>> @@ -476,10 +476,15 @@ static void toshiba_illumination_available(struct toshiba_acpi_dev *dev)
>>
>>       status = tci_raw(dev, in, out);
>>       sci_close(dev);
>> -     if (ACPI_FAILURE(status))
>> +     if (ACPI_FAILURE(status)) {
>>               pr_err("ACPI call to query Illumination support failed\n");
>> -     else if (out[0] == TOS_SUCCESS)
>> -             dev->illumination_supported = 1;
>> +             return;
>> +     }
>> +
>> +     if (out[0] != TOS_SUCCESS)
>
> Does this condition not merit a pr_err message? It reads like an error...
>
> There are several similar situations below which are equally silent. Is this a
> deliberate decision?

This was on purpose, since we are querying for all the supported features,
the kernel log will contain a lot of error strings for not supported features,
as not all laptop models support all of them.

>
> --
> Darren Hart
> Intel Open Source Technology Center
> --
> To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
-- El mundo apesta y vosotros apestais tambien --