linux-kernel - Re: [05/12] watchdog: sp5100_tco: Clean up sp5100_tco

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: <1516132557.18904.12.camel@redhat.com>
Date:   Tue, 16 Jan 2018 14:55:57 -0500
From:   Lyude Paul <lyude@...hat.com>
To:     Guenter Roeck <linux@...ck-us.net>,
        Wim Van Sebroeck <wim@...ana.be>
Cc:     linux-watchdog@...r.kernel.org, linux-kernel@...r.kernel.org,
        Zoltán Böszörményi <zboszor@...hu>
Subject: Re: [05/12] watchdog: sp5100_tco: Clean up sp5100_tco_setupdevice

Thank you for this cleanup, the gotos that were in this code are really
confusing to read through! I'd recommend one very small change described
below. Assuming you add that in the next version:

Reviewed-by: Lyude Paul <lyude@...hat.com>

On Sun, 2017-12-24 at 13:04 -0800, Guenter Roeck wrote:
> There are too many unnecessary goto statements in sp5100_tco_setupdevice().
> Rearrange the code and limit goto statements to error handling.
> 
> Cc: Zoltán Böszörményi <zboszor@...hu>
> Signed-off-by: Guenter Roeck <linux@...ck-us.net>
> ---
>  drivers/watchdog/sp5100_tco.c | 62 ++++++++++++++++++++------------------
> -----
>  1 file changed, 29 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/watchdog/sp5100_tco.c b/drivers/watchdog/sp5100_tco.c
> index 0e816f2cdb07..5a13ab483c50 100644
> --- a/drivers/watchdog/sp5100_tco.c
> +++ b/drivers/watchdog/sp5100_tco.c
> @@ -396,48 +396,44 @@ static int sp5100_tco_setupdevice(void)
>  	pr_debug("Got 0x%04x from indirect I/O\n", val);
>  
>  	/* Check MMIO address conflict */
> -	if (request_mem_region_exclusive(val, SP5100_WDT_MEM_MAP_SIZE,
> -								dev_name))
> -		goto setup_wdt;
> -	else
> +	if (!request_mem_region_exclusive(val, SP5100_WDT_MEM_MAP_SIZE,
> +					  dev_name)) {
>  		pr_debug("MMIO address 0x%04x already in use\n", val);
> +		/*
> +		 * Secondly, Find the watchdog timer MMIO address
> +		 * from SBResource_MMIO register.
> +		 */
> +		if (tco_has_sp5100_reg_layout(sp5100_tco_pci)) {
> +			/* Read SBResource_MMIO from PCI config(PCI_Reg:
> 9Ch) */
> +			pci_read_config_dword(sp5100_tco_pci,
> +					      SP5100_SB_RESOURCE_MMIO_BASE,
> +					      &val);
> +		} else {
> +			/* Read SBResource_MMIO from AcpiMmioEn(PM_Reg:
> 24h) */
> +			val =
> sp5100_tco_read_pm_reg32(SB800_PM_ACPI_MMIO_EN);
> +		}
>  
> -	/*
> -	 * Secondly, Find the watchdog timer MMIO address
> -	 * from SBResource_MMIO register.
> -	 */
> -	if (tco_has_sp5100_reg_layout(sp5100_tco_pci)) {
> -		/* Read SBResource_MMIO from PCI config(PCI_Reg: 9Ch) */
> -		pci_read_config_dword(sp5100_tco_pci,
> -				      SP5100_SB_RESOURCE_MMIO_BASE, &val);
> -	} else {
> -		/* Read SBResource_MMIO from AcpiMmioEn(PM_Reg: 24h) */
> -		val = sp5100_tco_read_pm_reg32(SB800_PM_ACPI_MMIO_EN);
> -	}
> -
> -	/* The SBResource_MMIO is enabled and mapped memory space? */
> -	if ((val & (SB800_ACPI_MMIO_DECODE_EN | SB800_ACPI_MMIO_SEL)) ==
> +		/* The SBResource_MMIO is enabled and mapped memory space?
> */
> +		if ((val & (SB800_ACPI_MMIO_DECODE_EN |
> SB800_ACPI_MMIO_SEL)) !=
>  						  SB800_ACPI_MMIO_DECODE_EN
Re-align this line since you're changing the code around here anyway

> ) {
> +			pr_notice("failed to find MMIO address, giving
> up.\n");
> +			ret = -ENODEV;
> +			goto unreg_region;
> +		}
>  		/* Clear unnecessary the low twelve bits */
>  		val &= ~0xFFF;
>  		/* Add the Watchdog Timer offset to base address. */
>  		val += SB800_PM_WDT_MMIO_OFFSET;
>  		/* Check MMIO address conflict */
> -		if (request_mem_region_exclusive(val,
> SP5100_WDT_MEM_MAP_SIZE,
> -								   dev_name
> )) {
> -			pr_debug("Got 0x%04x from SBResource_MMIO
> register\n",
> -				val);
> -			goto setup_wdt;
> -		} else
> +		if (!request_mem_region_exclusive(val,
> SP5100_WDT_MEM_MAP_SIZE,
> +						  dev_name)) {
>  			pr_debug("MMIO address 0x%04x already in use\n",
> val);
> -	} else
> -		pr_debug("SBResource_MMIO is disabled(0x%04x)\n", val);
> -
> -	pr_notice("failed to find MMIO address, giving up.\n");
> -	ret = -ENODEV;
> -	goto  unreg_region;
> +			ret = -EBUSY;
> +			goto unreg_region;
> +		}
> +		pr_debug("Got 0x%04x from SBResource_MMIO register\n",
> val);
> +	}
>  
> -setup_wdt:
>  	tcobase_phys = val;
>  
>  	tcobase = ioremap(val, SP5100_WDT_MEM_MAP_SIZE);
> @@ -472,7 +468,7 @@ static int sp5100_tco_setupdevice(void)
>  	tco_timer_stop();
>  
>  	release_region(SP5100_IO_PM_INDEX_REG, SP5100_PM_IOPORTS_SIZE);
> -	/* Done */
> +
>  	return 0;
>  
>  unreg_mem_region:
-- 
Cheers,
	Lyude Paul