[PATCH v2] x86/pmc_atom: Fix warning when CONFIG_DEBUG_FS=n

[PATCH v2] x86/pmc_atom: Fix warning when CONFIG_DEBUG_FS=n

[Martin Kelly]

When compiling with CONFIG_DEBUG_FS=n, gcc emits an unused variable
warning for pmc_atom.c because "ret" is used only within the
CONFIG_DEBUG_FS block. This patch adds a dummy #ifdef for
pmc_dbgfs_register when CONFIG_DEBUG_FS=n to simplify the code and
remove the warning.

Signed-off-by: Martin Kelly <martkell@amazon.com>
---
Changes in v2:
  - Implemented Ingo Molnar's suggestion to #ifdef the function rather
    than the lines.
---
 arch/x86/kernel/pmc_atom.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/pmc_atom.c b/arch/x86/kernel/pmc_atom.c
index 0c424a6..d06527a 100644
--- a/arch/x86/kernel/pmc_atom.c
+++ b/arch/x86/kernel/pmc_atom.c
@@ -235,6 +235,11 @@ err:
 	pmc_dbgfs_unregister(pmc);
 	return -ENODEV;
 }
+#else
+static int pmc_dbgfs_register(struct pmc_dev *pmc, struct pci_dev *pdev)
+{
+	return 0;
+}
 #endif /* CONFIG_DEBUG_FS */
 
 static int pmc_setup_dev(struct pci_dev *pdev)
@@ -262,13 +267,11 @@ static int pmc_setup_dev(struct pci_dev *pdev)
 	/* PMC hardware registers setup */
 	pmc_hw_reg_setup(pmc);
 
-#ifdef CONFIG_DEBUG_FS
 	ret = pmc_dbgfs_register(pmc, pdev);
 	if (ret) {
 		iounmap(pmc->regmap);
 		return ret;
 	}
-#endif /* CONFIG_DEBUG_FS */
 	return 0;
 }
 
-- 
2.1.0

Re: [PATCH v2] x86/pmc_atom: Fix warning when CONFIG_DEBUG_FS=n

[Li, Aubrey]

On 2014/9/17 8:49, Martin Kelly wrote:
> When compiling with CONFIG_DEBUG_FS=n, gcc emits an unused variable
> warning for pmc_atom.c because "ret" is used only within the
> CONFIG_DEBUG_FS block. This patch adds a dummy #ifdef for
> pmc_dbgfs_register when CONFIG_DEBUG_FS=n to simplify the code and
> remove the warning.
> 
> Signed-off-by: Martin Kelly <martkell@amazon.com>

Thanks to take care of this warning. How about this version?

diff --git a/arch/x86/kernel/pmc_atom.c b/arch/x86/kernel/pmc_atom.c
index 0c424a6..cd91b57 100644
--- a/arch/x86/kernel/pmc_atom.c
+++ b/arch/x86/kernel/pmc_atom.c
@@ -240,7 +240,7 @@ err:
 static int pmc_setup_dev(struct pci_dev *pdev)
 {
 	struct pmc_dev *pmc = &pmc_device;
-	int ret;
+	int ret = 0;
 
 	/* Obtain ACPI base address */
 	pci_read_config_dword(pdev, ACPI_BASE_ADDR_OFFSET, &acpi_base_addr);
@@ -269,7 +269,7 @@ static int pmc_setup_dev(struct pci_dev *pdev)
 		return ret;
 	}
 #endif /* CONFIG_DEBUG_FS */
-	return 0;
+	return ret;
 }
 
 /*

Thanks,
-Aubrey


> ---
> Changes in v2:
>   - Implemented Ingo Molnar's suggestion to #ifdef the function rather
>     than the lines.
> ---
>  arch/x86/kernel/pmc_atom.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kernel/pmc_atom.c b/arch/x86/kernel/pmc_atom.c
> index 0c424a6..d06527a 100644
> --- a/arch/x86/kernel/pmc_atom.c
> +++ b/arch/x86/kernel/pmc_atom.c
> @@ -235,6 +235,11 @@ err:
>  	pmc_dbgfs_unregister(pmc);
>  	return -ENODEV;
>  }
> +#else
> +static int pmc_dbgfs_register(struct pmc_dev *pmc, struct pci_dev *pdev)
> +{
> +	return 0;
> +}
>  #endif /* CONFIG_DEBUG_FS */
>  
>  static int pmc_setup_dev(struct pci_dev *pdev)
> @@ -262,13 +267,11 @@ static int pmc_setup_dev(struct pci_dev *pdev)
>  	/* PMC hardware registers setup */
>  	pmc_hw_reg_setup(pmc);
>  
> -#ifdef CONFIG_DEBUG_FS
>  	ret = pmc_dbgfs_register(pmc, pdev);
>  	if (ret) {
>  		iounmap(pmc->regmap);
>  		return ret;
>  	}
> -#endif /* CONFIG_DEBUG_FS */
>  	return 0;
>  }
>  
> 

Re: [PATCH v2] x86/pmc_atom: Fix warning when CONFIG_DEBUG_FS=n

[Martin Kelly]

On 09/16/2014 07:09 PM, Li, Aubrey wrote:
> 
> Thanks to take care of this warning. How about this version?
> 
> diff --git a/arch/x86/kernel/pmc_atom.c b/arch/x86/kernel/pmc_atom.c
> index 0c424a6..cd91b57 100644
> --- a/arch/x86/kernel/pmc_atom.c
> +++ b/arch/x86/kernel/pmc_atom.c
> @@ -240,7 +240,7 @@ err:
>  static int pmc_setup_dev(struct pci_dev *pdev)
>  {
>  	struct pmc_dev *pmc = &pmc_device;
> -	int ret;
> +	int ret = 0;
>  
>  	/* Obtain ACPI base address */
>  	pci_read_config_dword(pdev, ACPI_BASE_ADDR_OFFSET, &acpi_base_addr);
> @@ -269,7 +269,7 @@ static int pmc_setup_dev(struct pci_dev *pdev)
>  		return ret;
>  	}
>  #endif /* CONFIG_DEBUG_FS */
> -	return 0;
> +	return ret;
>  }
>  
>  /*
> 
> Thanks,
> -Aubrey
> 

Thanks for the suggestion, Aubrey. Although that version would also work, it still has inline #ifdef, which is harder to read and goes against general Linux conventions:

https://www.kernel.org/doc/Documentation/SubmittingPatches
(section 2, "#ifdefs are ugly")

Re: [PATCH v2] x86/pmc_atom: Fix warning when CONFIG_DEBUG_FS=n

[Li, Aubrey]

On 2014/9/17 11:20, Martin Kelly wrote:
> On 09/16/2014 07:09 PM, Li, Aubrey wrote:
>>
>> Thanks to take care of this warning. How about this version?
>>
>> diff --git a/arch/x86/kernel/pmc_atom.c b/arch/x86/kernel/pmc_atom.c
>> index 0c424a6..cd91b57 100644
>> --- a/arch/x86/kernel/pmc_atom.c
>> +++ b/arch/x86/kernel/pmc_atom.c
>> @@ -240,7 +240,7 @@ err:
>>  static int pmc_setup_dev(struct pci_dev *pdev)
>>  {
>>  	struct pmc_dev *pmc = &pmc_device;
>> -	int ret;
>> +	int ret = 0;
>>  
>>  	/* Obtain ACPI base address */
>>  	pci_read_config_dword(pdev, ACPI_BASE_ADDR_OFFSET, &acpi_base_addr);
>> @@ -269,7 +269,7 @@ static int pmc_setup_dev(struct pci_dev *pdev)
>>  		return ret;
>>  	}
>>  #endif /* CONFIG_DEBUG_FS */
>> -	return 0;
>> +	return ret;
>>  }
>>  
>>  /*
>>
>> Thanks,
>> -Aubrey
>>
> 
> Thanks for the suggestion, Aubrey. Although that version would also work, it still has inline #ifdef, which is harder to read and goes against general Linux conventions:

Why do you want to call pmc_dbgfs_register() anyway even if
CONFIG_DEBUG_FS=n?

Thanks,
-Aubrey

> 
> https://www.kernel.org/doc/Documentation/SubmittingPatches
> (section 2, "#ifdefs are ugly")
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 
> 

Re: [PATCH v2] x86/pmc_atom: Fix warning when CONFIG_DEBUG_FS=n

[Martin Kelly]

On 09/16/2014 08:44 PM, Li, Aubrey wrote:
> 
> Why do you want to call pmc_dbgfs_register() anyway even if
> CONFIG_DEBUG_FS=n?
> 
> Thanks,
> -Aubrey
> 

The compiler will optimize away the call when CONFIG_DEBUG_FS=n, as the function body is just "return 0". Since the line following the call is "if (ret)" and since ret will always be 0, the compiler will likely optimize away the branch as well.

The reasoning for doing it this way is that it gives you a uniform code flow, which is easier to follow and reason about than having to think about branching #ifdefs sprinkled throughout the functions. The link I pasted gives full detail, but here's a snippet from Linus:

"Code cluttered with ifdefs is difficult to read and maintain.  Don't do
it.  Instead, put your ifdefs in a header, and conditionally define
'static inline' functions, or macros, which are used in the code.
Let the compiler optimize away the "no-op" case."

https://www.kernel.org/doc/Documentation/SubmittingPatches
(section 2.2, "#ifdefs are ugly")

In addition, Ingo Molnar suggested that I revise the patch in this way.

Thanks,
Martin

Re: [PATCH v2] x86/pmc_atom: Fix warning when CONFIG_DEBUG_FS=n

[Ingo Molnar]

* Martin Kelly <martin@martingkelly.com> wrote:

> When compiling with CONFIG_DEBUG_FS=n, gcc emits an unused variable
> warning for pmc_atom.c because "ret" is used only within the
> CONFIG_DEBUG_FS block. This patch adds a dummy #ifdef for
> pmc_dbgfs_register when CONFIG_DEBUG_FS=n to simplify the code and
> remove the warning.
> 
> Signed-off-by: Martin Kelly <martkell@amazon.com>
> ---
> Changes in v2:
>   - Implemented Ingo Molnar's suggestion to #ifdef the function rather
>     than the lines.
> ---
>  arch/x86/kernel/pmc_atom.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kernel/pmc_atom.c b/arch/x86/kernel/pmc_atom.c
> index 0c424a6..d06527a 100644
> --- a/arch/x86/kernel/pmc_atom.c
> +++ b/arch/x86/kernel/pmc_atom.c
> @@ -235,6 +235,11 @@ err:
>  	pmc_dbgfs_unregister(pmc);
>  	return -ENODEV;
>  }
> +#else
> +static int pmc_dbgfs_register(struct pmc_dev *pmc, struct pci_dev *pdev)
> +{
> +	return 0;
> +}
>  #endif /* CONFIG_DEBUG_FS */

Ok, that part is good.

>  
>  static int pmc_setup_dev(struct pci_dev *pdev)
> @@ -262,13 +267,11 @@ static int pmc_setup_dev(struct pci_dev *pdev)
>  	/* PMC hardware registers setup */
>  	pmc_hw_reg_setup(pmc);
>  
> -#ifdef CONFIG_DEBUG_FS
>  	ret = pmc_dbgfs_register(pmc, pdev);
>  	if (ret) {
>  		iounmap(pmc->regmap);
>  		return ret;
>  	}
> -#endif /* CONFIG_DEBUG_FS */
>  	return 0;
>  }

Just to paint the bike shed a bit, this could be further 
simplified to something like:

	...

  	ret = pmc_dbgfs_register(pmc, pdev);
  	if (ret)
  		iounmap(pmc->regmap);

  	return ret;

and then your patch will be perfect! :-)

Thanks,

	Ingo

Re: [PATCH v2] x86/pmc_atom: Fix warning when CONFIG_DEBUG_FS=n

[Martin Kelly]

On 09/17/2014 05:24 AM, Ingo Molnar wrote:
> 
> Just to paint the bike shed a bit, this could be further 
> simplified to something like:
> 
> 	...
> 
>   	ret = pmc_dbgfs_register(pmc, pdev);
>   	if (ret)
>   		iounmap(pmc->regmap);
> 
>   	return ret;
> 
> and then your patch will be perfect! :-)
> 

Yes, good idea. I sent a v3 with that implemented.