Re: [PATCH 1/2] scsi: ufs: core: Make ufshcd_uic_cmd_compl() easier to read

["Peter Wang (王信友)" <peter.wang@mediatek.com>]

On Wed, 2024-08-21 at 11:29 -0700, Bart Van Assche wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  Introduce a local variable for the expression hba->active_uic_cmd.
> Remove superfluous parentheses.
> 
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>  drivers/ufs/core/ufshcd.c | 21 ++++++++++-----------
>  1 file changed, 10 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index 0dd26059f5d7..d0ae6e50becc 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -5464,31 +5464,30 @@ static bool
> ufshcd_is_auto_hibern8_error(struct ufs_hba *hba,
>  static irqreturn_t ufshcd_uic_cmd_compl(struct ufs_hba *hba, u32
> intr_status)
>  {
>  	irqreturn_t retval = IRQ_NONE;
> +	struct uic_command *cmd;
>  
>  	spin_lock(hba->host->host_lock);
> +	cmd = hba->active_uic_cmd;
>  	if (ufshcd_is_auto_hibern8_error(hba, intr_status))
>  		hba->errors |= (UFSHCD_UIC_HIBERN8_MASK & intr_status);
>  
> -	if ((intr_status & UIC_COMMAND_COMPL) && hba->active_uic_cmd) {
> -		hba->active_uic_cmd->argument2 |=
> -			ufshcd_get_uic_cmd_result(hba);
> -		hba->active_uic_cmd->argument3 =
> -			ufshcd_get_dme_attr_val(hba);
> +	if (intr_status & UIC_COMMAND_COMPL && cmd) {
> 

Hi Bart,

in C language, when conditional expressions become complex, 
using parentheses can help improve the readability of the 
code and clearly specify the precedence of operators. 
This is very important because different operators, 
such as logical operators && and ||, comparison operators 
==, !=, <, >, etc., have different levels of precedence. 
Without using parentheses to clarify,  it could lead to 
unexpected results.

For example, consider the following complex conditional expression:

if (a == b && c > d || e < f && g != h)

While this conditional expression is valid, its order of 
operations might not be immediately clear. Using parentheses 
can help others reading the code (including your future self) 
to understand your intent more easily:

if ((a == b && c > d) || (e < f && g != h))

In this example, the parentheses clearly indicate that the 
&& operations are to be evaluated first, followed by the || 
operation. This avoids confusion caused by operator precedence 
and makes the logic of the code more explicit. Therefore, 
it is a good practice to use parentheses when dealing with 
complex conditional expressions.

Thanks.
Peter



> +		cmd->argument2 |= ufshcd_get_uic_cmd_result(hba);
> +		cmd->argument3 = ufshcd_get_dme_attr_val(hba);
>  		if (!hba->uic_async_done)
> -			hba->active_uic_cmd->cmd_active = 0;
> -		complete(&hba->active_uic_cmd->done);
> +			cmd->cmd_active = 0;
> +		complete(&cmd->done);
>  		retval = IRQ_HANDLED;
>  	}
>  
> -	if ((intr_status & UFSHCD_UIC_PWR_MASK) && hba->uic_async_done) 
> {
> -		hba->active_uic_cmd->cmd_active = 0;
> +	if (intr_status & UFSHCD_UIC_PWR_MASK && hba->uic_async_done) {
> +		cmd->cmd_active = 0;
>  		complete(hba->uic_async_done);
>  		retval = IRQ_HANDLED;
>  	}
>  
>  	if (retval == IRQ_HANDLED)
> -		ufshcd_add_uic_command_trace(hba, hba->active_uic_cmd,
> -					     UFS_CMD_COMP);
> +		ufshcd_add_uic_command_trace(hba, cmd, UFS_CMD_COMP);
>  	spin_unlock(hba->host->host_lock);
>  	return retval;
>  }