Re: [PATCH] firmware: arm_scmi: Convert u32 to unsigned long to align with arm_smccc_1_1_invoke()

From: Cristian Marussi <cristian.marussi@arm.com>
To: Sudeep Holla <sudeep.holla@arm.com>
Cc: linux-arm-kernel@lists.infradead.org, andersson@kernel.org,
	Nikunj Kela <quic_nkela@quicinc.com>
Subject: Re: [PATCH] firmware: arm_scmi: Convert u32 to unsigned long to align with arm_smccc_1_1_invoke()
Date: Mon, 9 Oct 2023 17:49:25 +0100	[thread overview]
Message-ID: <ZSQvFWpv7sR8kMpC@e120937-lin> (raw)
In-Reply-To: <20231009152049.1428872-1-sudeep.holla@arm.com>

On Mon, Oct 09, 2023 at 04:20:49PM +0100, Sudeep Holla wrote:
> All the parameters to arm_smccc_1_1_invoke() are unsigned long which
> aligns well on both 32-bit and 64-bit Arm based platforms. Let us store
> all the members in the structure scmi_smc used as the parameters to the
> arm_smccc_1_1_invoke() call as unsigned long.
> 
> Cc: Cristian Marussi <cristian.marussi@arm.com>
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
>  drivers/firmware/arm_scmi/smc.c | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/firmware/arm_scmi/smc.c b/drivers/firmware/arm_scmi/smc.c
> index c193516a254d..8eba60a41975 100644
> --- a/drivers/firmware/arm_scmi/smc.c
> +++ b/drivers/firmware/arm_scmi/smc.c
> @@ -60,9 +60,9 @@ struct scmi_smc {
>  	struct mutex shmem_lock;
>  #define INFLIGHT_NONE	MSG_TOKEN_MAX
>  	atomic_t inflight;
> -	u32 func_id;
> -	u32 param_page;
> -	u32 param_offset;
> +	unsigned long func_id;
> +	unsigned long param_page;
> +	unsigned long param_offset;
>  };
>  
>  static irqreturn_t smc_msg_done_isr(int irq, void *data)
> @@ -211,8 +211,6 @@ static int smc_send_message(struct scmi_chan_info *cinfo,
>  {
>  	struct scmi_smc *scmi_info = cinfo->transport_info;
>  	struct arm_smccc_res res;
> -	unsigned long page = scmi_info->param_page;
> -	unsigned long offset = scmi_info->param_offset;
>  
>  	/*
>  	 * Channel will be released only once response has been
> @@ -222,8 +220,8 @@ static int smc_send_message(struct scmi_chan_info *cinfo,
>  
>  	shmem_tx_prepare(scmi_info->shmem, xfer, cinfo);
>  
> -	arm_smccc_1_1_invoke(scmi_info->func_id, page, offset, 0, 0, 0, 0, 0,
> -			     &res);
> +	arm_smccc_1_1_invoke(scmi_info->func_id, scmi_info->param_page,
> +			     scmi_info->param_offset, 0, 0, 0, 0, 0, &res);
>  
>  	/* Only SMCCC_RET_NOT_SUPPORTED is valid error code */
>  	if (res.a0) {
> -- 

Looking at where scmi_info->param_page and scmi_info->param_offset
are set with the SHMEM_PAGE/OFFSET macros, I dont get what is the
supposed meaning of _UL macros in the original:

	#define SHMEM_SHIFT 12
	#define SHMEM_PAGE(x) (_UL((x) >> SHMEM_SHIFT))

given it is used as:

	scmi_info->param_page = SHMEM_PAGE(res.start);

which indeed looking at the pre-processor output yields (somehow) just a
simple:

	scmi_info->param_page = ((((res.start) >> 12UL)));

...this was introduced in a previous patch, though:

5f2ea10a808ae firmware: arm_scmi: Augment SMC/HVC to allow optional parameters

..not sure if there are other deeper intents here...

But anyway, with this patch any form of casting is probably not needed anymore
since you'll have a:

	unsigned long (u32 or u64) <<--- res.start (u32 or u64)

...not sure if static analyzers and/or compilers would complain/warn
these days.

Other than this, LGTM:

Reviewed-by: Cristian Marussi <cristian.marussi@arm.com>

Thanks,
Cristian

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel