linux-kernel - Re: [PATCH 10/13] eeepc-laptop: compare proper return values in get

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: <20140915214902.GB64909@vmdeb7>
Date:	Mon, 15 Sep 2014 14:49:02 -0700
From:	Darren Hart <dvhart@...radead.org>
To:	Frans Klaver <fransklaver@...il.com>
Cc:	Corentin Chary <corentin.chary@...il.com>,
	Rafael Wysocki <rafael.j.wysocki@...el.com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	acpi4asus-user@...ts.sourceforge.net,
	platform-driver-x86@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-acpi@...r.kernel.org, "H. Peter Anvin" <hpa@...ux.intel.com>
Subject: Re: [PATCH 10/13] eeepc-laptop: compare proper return values in
 get_cpufv

On Sat, Sep 13, 2014 at 01:06:49AM +0200, Frans Klaver wrote:
> In get_cpufv the return value of get_acpi is stored in the cpufv struct.
> Right before this value is checked for errors, it is and'ed with 0xff.
> This means c->cur can never be less than zero. Besides that, the actual
> error value is ignored.
> 
> c->num is also and'ed with 0xff, which means we can ignore values below
> zero.
> 
> Check the result of get_acpi() right away. While at it, propagate the
> error if we got one.
> 
> Signed-off-by: Frans Klaver <fransklaver@...il.com>
> ---
>  drivers/platform/x86/eeepc-laptop.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/x86/eeepc-laptop.c b/drivers/platform/x86/eeepc-laptop.c
> index 47488d3..828db56 100644
> --- a/drivers/platform/x86/eeepc-laptop.c
> +++ b/drivers/platform/x86/eeepc-laptop.c
> @@ -332,9 +332,12 @@ struct eeepc_cpufv {
>  static int get_cpufv(struct eeepc_laptop *eeepc, struct eeepc_cpufv *c)
>  {
>  	c->cur = get_acpi(eeepc, CM_ASL_CPUFV);
> +	if (c->cur < 0)
> +		return c->cur;
> +
>  	c->num = (c->cur >> 8) & 0xff;
>  	c->cur &= 0xff;
> -	if (c->cur < 0 || c->num <= 0 || c->num > 12)
> +	if (c->num == 0 || c->num > 12)
>  		return -ENODEV;
>  	return 0;

This patch is fine as is. However, Greg has supported propogating the error code
through to the sysfs interface (if I understand him correctly on an earlier post
to this list). This would require an addition change to this patch would
propogated the get_cpufv error code in show_available_cpuv(), show_cpuv(), and
store_cpuv(). As it is, we return -ENODEV on any failure, where an ACPI call
error should probably return -ENXIO as I understand it.

However, there was a rather famous change in error code handling in which pulse
audio broke and Linus was very upset with one of his maintainers.

How do we know when it is acceptable to change which error code is returned?

-- 
Darren Hart
Intel Open Source Technology Center
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/