From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1424964AbcFHLMc (ORCPT <rfc822;w@1wt.eu>);
	Wed, 8 Jun 2016 07:12:32 -0400
Received: from mail-wm0-f65.google.com ([74.125.82.65]:33052 "EHLO
	mail-wm0-f65.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1422710AbcFHLM3 (ORCPT
	<rfc822;linux-kernel@vger.kernel.org>);
	Wed, 8 Jun 2016 07:12:29 -0400
Date: Wed, 8 Jun 2016 13:12:23 +0200
From: Ingo Molnar <mingo@kernel.org>
To: Joseph Thelen <jthelen@sgi.com>
Cc: linux-kernel@vger.kernel.org, Alex Thorlton <athorlton@sgi.com>,
        Matt Fleming <matt@codeblueprint.co.uk>,
        Thomas Gleixner <tglx@linutronix.de>, Ingo Molnar <mingo@redhat.com>,
        "H. Peter Anvin" <hpa@zytor.com>, x86@kernel.org,
        linux-efi@vger.kernel.org
Subject: Re: [PATCH] x86/efi: Auto enable EFI memmap on SGI UV systems
Message-ID: <20160608111223.GA5772@gmail.com>
References: <1464900635-11957-1-git-send-email-jthelen@sgi.com>
 <1464900635-11957-2-git-send-email-jthelen@sgi.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1464900635-11957-2-git-send-email-jthelen@sgi.com>
User-Agent: Mutt/1.5.24 (2015-08-30)
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


* Joseph Thelen <jthelen@sgi.com> wrote:

>  static int __init setup_add_efi_memmap(char *arg)
>  {
> +	static bool arg_as_bool;

Why hidden inside local variables as static?

> +	int ret = strtobool(arg, &arg_as_bool);
> +
> +	/* check for a non-existent arg, to maintain backward compatibility */
> +	if (!arg) {
> +		add_efi_memmap = EFI_MEMMAP_ENABLED;
> +	} else {
> +		if (ret) {
> +			/* a bad argument was passed... */
> +			return ret;
> +		} else {
> +			if (arg_as_bool)
> +				add_efi_memmap = EFI_MEMMAP_ENABLED;
> +			else
> +				add_efi_memmap = EFI_MEMMAP_DISABLED;
> +		}
> +	}
> +
>  	return 0;

And that's a really weird code flow!

How about something straightforward:

	int val = 0;
	int ret;

	/* Check for a non-existent arg, to maintain backward compatibility: */
	if (!arg) {
		add_efi_memmap = EFI_MEMMAP_ENABLED;
		return 0;
	}

	ret = strtobool(arg, &val);

	/* Was a bad argument passed? */
	if (ret)
		return ret;

	if (val)
		add_efi_memmap = EFI_MEMMAP_ENABLED;
	else
		add_efi_memmap = EFI_MEMMAP_DISABLED;

	return 0;

?

Also note the rename to 'val'.

Thanks,

	Ingo