linux-kernel - Re: [PATCH] staging: board: Remove macro board

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>] [day] [month] [year] [list]

Message-ID: <20210104143951.GS2809@kadam>
Date:   Mon, 4 Jan 2021 17:39:51 +0300
From:   Dan Carpenter <dan.carpenter@...cle.com>
To:     Song Chen <chensong_2000@....cn>
Cc:     greg@...ah.com, linux-kernel@...r.kernel.org,
        devel@...verdev.osuosl.org
Subject: Re: [PATCH] staging: board: Remove macro board_staging

On Fri, Dec 25, 2020 at 05:54:45PM +0800, Song Chen wrote:
> Macro is not supposed to have flow control in it's
> statement, remove.
> 

It took me a long time to figure out what this commit message is saying.
It turns out that it is inspired by checkpatch.  Forget all that nonsense
about "imperative tense" commit messages.  The only thing which matters
for drivers/staging/ is that the commit message is clear what the
problem is and how you are going to fix it.

  Checkpatch complains that macros should not have return statements in
  them.  "WARNING: Macros with flow control statements should be avoided"
  So I am removing the board_staging() macro and open coding it.

But in this case the checkpatch warning is a false positive.  The issue
that checkpatch is trying to fix is that we don't want return, break, or
goto statements in a macro.  Otherwise the code looks like this:

	frob();
	frob();
	frob();

It has three invisible return statements and we don't know what error
codes it is returning.

In this case, the board_staging() driver is implementing whole functions
so there are no hidden gotos.

That said, the board_staging() macro looks pretty rubbish.  It breaks
cscope.  It's not readable.  So I quite like the patch, it just needs a
new commit message.  It can be simple:

    It's cleaner and less code to delete the board_staging().

> Signed-off-by: Song Chen <chensong_2000@....cn>
> ---
>  drivers/staging/board/armadillo800eva.c | 10 ++++++----
>  drivers/staging/board/board.h           | 11 -----------
>  drivers/staging/board/kzm9d.c           | 18 ++++++++++--------
>  3 files changed, 16 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/staging/board/armadillo800eva.c b/drivers/staging/board/armadillo800eva.c
> index 0225234..a7e8487 100644
> --- a/drivers/staging/board/armadillo800eva.c
> +++ b/drivers/staging/board/armadillo800eva.c
> @@ -80,9 +80,11 @@ static const struct board_staging_dev armadillo800eva_devices[] __initconst = {
>  
>  static void __init armadillo800eva_init(void)

I think device_initcall() functions need to return int.  I am surprised
this even compiles.

>  {
> -	board_staging_gic_setup_xlate("arm,pl390", 32);
> -	board_staging_register_devices(armadillo800eva_devices,
> -				       ARRAY_SIZE(armadillo800eva_devices));
> +	if (of_machine_is_compatible("renesas,armadillo800eva")) {

Reverse this if statement.

	if (!of_machine_is_compatible("renesas,armadillo800eva"))
		return 0;

> +		board_staging_gic_setup_xlate("arm,pl390", 32);
> +		board_staging_register_devices(armadillo800eva_devices,
> +					       ARRAY_SIZE(armadillo800eva_devices));
> +	}
>  }
>  
> -board_staging("renesas,armadillo800eva", armadillo800eva_init);
> +device_initcall(armadillo800eva_init);
> diff --git a/drivers/staging/board/board.h b/drivers/staging/board/board.h
> index 5609daf..f1c233e 100644
> --- a/drivers/staging/board/board.h
> +++ b/drivers/staging/board/board.h
> @@ -32,15 +32,4 @@ int board_staging_register_device(const struct board_staging_dev *dev);
>  void board_staging_register_devices(const struct board_staging_dev *devs,
>  				    unsigned int ndevs);
>  
> -#define board_staging(str, fn)			\
> -static int __init runtime_board_check(void)	\
> -{						\
> -	if (of_machine_is_compatible(str))	\
> -		fn();				\
> -						\
> -	return 0;				\
> -}						\
> -						\
> -device_initcall(runtime_board_check)
> -
>  #endif /* __BOARD_H__ */
> diff --git a/drivers/staging/board/kzm9d.c b/drivers/staging/board/kzm9d.c
> index d449a83..72b1ad45 100644
> --- a/drivers/staging/board/kzm9d.c
> +++ b/drivers/staging/board/kzm9d.c
> @@ -12,15 +12,17 @@ static struct resource usbs1_res[] __initdata = {
>  
>  static void __init kzm9d_init(void)

Same.  return int.

>  {
> -	board_staging_gic_setup_xlate("arm,pl390", 32);
> +	if (of_machine_is_compatible("renesas,kzm9d")) {

Same reverse the if statement.

regards,
dan carpenter