linux-kernel - Re: [PATCH 03/20] bus: mhi: Make mhi_state_str[] array static const and move to common.h

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: <0d850b27-5684-2ecf-fc96-3258c0462d3d@ieee.org>
Date:   Wed, 5 Jan 2022 18:22:59 -0600
From:   Alex Elder <elder@...e.org>
To:     Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>,
        mhi@...ts.linux.dev
Cc:     hemantk@...eaurora.org, bbhatt@...eaurora.org,
        quic_jhugo@...cinc.com, vinod.koul@...aro.org,
        bjorn.andersson@...aro.org, dmitry.baryshkov@...aro.org,
        skananth@...eaurora.org, vpernami@...eaurora.org,
        vbadigan@...eaurora.org, linux-arm-msm@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 03/20] bus: mhi: Make mhi_state_str[] array static const
 and move to common.h

On 12/2/21 5:35 AM, Manivannan Sadhasivam wrote:
> mhi_state_str[] array could be used by MHI endpoint stack also. So let's
> make the array as "static const" and move it inside the "common.h" header
> so that the endpoint stack could also make use of it. Otherwise, the
> structure definition should be present in both host and endpoint stack and
> that'll result in duplication.
> 
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>

This result in common source code (which is good), but it will be
duplicated in everything that includes this file.

Do you have no common code, available to both the endpoint and host?
You could (in drivers/bus/mhi/common.c, for example).

If you don't, I have a different suggestion, below.  It does
basically the same thing you're doing here, but I much prefer
duplicating an inline function than a data structure.

> ---
>   drivers/bus/mhi/common.h    | 13 ++++++++++++-
>   drivers/bus/mhi/host/init.c | 12 ------------
>   2 files changed, 12 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/bus/mhi/common.h b/drivers/bus/mhi/common.h
> index 0f4f3b9f3027..2ea438205617 100644
> --- a/drivers/bus/mhi/common.h
> +++ b/drivers/bus/mhi/common.h
> @@ -174,7 +174,18 @@ struct mhi_cmd_ctxt {
>   	__u64 wp __packed __aligned(4);
>   };
>   
> -extern const char * const mhi_state_str[MHI_STATE_MAX];
> +static const char * const mhi_state_str[MHI_STATE_MAX] = {
> +	[MHI_STATE_RESET] = "RESET",
> +	[MHI_STATE_READY] = "READY",
> +	[MHI_STATE_M0] = "M0",
> +	[MHI_STATE_M1] = "M1",
> +	[MHI_STATE_M2] = "M2",
> +	[MHI_STATE_M3] = "M3",
> +	[MHI_STATE_M3_FAST] = "M3 FAST",
> +	[MHI_STATE_BHI] = "BHI",
> +	[MHI_STATE_SYS_ERR] = "SYS ERROR",
> +};
> +
>   #define TO_MHI_STATE_STR(state) ((state >= MHI_STATE_MAX || \
>   				  !mhi_state_str[state]) ? \
>   				"INVALID_STATE" : mhi_state_str[state])

You could easily and safely define this as an inline function instead.

#define MHI_STATE_CASE(x)	case MHI_STATE_ ## x: return #x
static inline const char *mhi_state_string(enum mhi_state state)
{
	switch(state) {
	MHI_STATE_CASE(RESET);
	MHI_STATE_CASE(READY);
	MHI_STATE_CASE(M0);
	MHI_STATE_CASE(M1);
	MHI_STATE_CASE(M2);
	MHI_STATE_CASE(M3_FAST);
	MHI_STATE_CASE(BHI);
	MHI_STATE_CASE(SYS_ERR);
	default: return "(unrecognized MHI state)";
	}
}
#undef MHI_STATE_CASE

					-Alex

> diff --git a/drivers/bus/mhi/host/init.c b/drivers/bus/mhi/host/init.c
> index 5aaca6d0f52b..fa904e7468d8 100644
> --- a/drivers/bus/mhi/host/init.c
> +++ b/drivers/bus/mhi/host/init.c
> @@ -44,18 +44,6 @@ const char * const dev_state_tran_str[DEV_ST_TRANSITION_MAX] = {
>   	[DEV_ST_TRANSITION_DISABLE] = "DISABLE",
>   };
>   
> -const char * const mhi_state_str[MHI_STATE_MAX] = {
> -	[MHI_STATE_RESET] = "RESET",
> -	[MHI_STATE_READY] = "READY",
> -	[MHI_STATE_M0] = "M0",
> -	[MHI_STATE_M1] = "M1",
> -	[MHI_STATE_M2] = "M2",
> -	[MHI_STATE_M3] = "M3",
> -	[MHI_STATE_M3_FAST] = "M3 FAST",
> -	[MHI_STATE_BHI] = "BHI",
> -	[MHI_STATE_SYS_ERR] = "SYS ERROR",
> -};
> -
>   const char * const mhi_ch_state_type_str[MHI_CH_STATE_TYPE_MAX] = {
>   	[MHI_CH_STATE_TYPE_RESET] = "RESET",
>   	[MHI_CH_STATE_TYPE_STOP] = "STOP",
>