[0/2] Fix the UFS driver hibernation code

Hi Martin,

While testing the UFS driver we noticed that the UFS host controller does not
always enter hibernation when it should enter hibernation. This patch series
fixes this. Please consider this patch series for the next merge window.

Thanks,

Bart.

Bart Van Assche (2):
  scsi: ufs: core: Make ufshcd_uic_cmd_compl() easier to read
  scsi: ufs: core: Fix the code for entering hibernation

 drivers/ufs/core/ufshcd.c | 37 +++++++++++++------------------------
 include/ufs/ufshcd.h      |  7 ++++---
 2 files changed, 17 insertions(+), 27 deletions(-)

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;
>  }

On 8/21/24 10:34 PM, Peter Wang (王信友) wrote:
> For example, consider the following complex conditional expression:
> 
> if (a == b && c > d || e < f && g != h)

Hi Peter,

As you may know, the above is not allowed in Linux kernel code. I'm
not sure this has been written down anywhere formally. The C compilers
supported by the Linux kernel emit a warning about expressions like the
above and Linux kernel code should be warning-free.

I agree with you that code readability is important. However, I think
that it's important for Linux kernel developers to make themselves
familiar with expressions like (a & b && ...) since this style is common
in the Linux kernel. Do you agree that the data below shows that not
surrounding binary and expressions with parentheses is common in Linux
kernel code?

$ git grep -nHE 'if.*&&[^()&]*&[^&]|if.*[^&]&[^&()]*&&' | wc -l
    2548

Thanks,

Bart.

On Thu, 2024-08-22 at 10:02 -0700, Bart Van Assche wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  On 8/21/24 10:34 PM, Peter Wang (王信友) wrote:
> > For example, consider the following complex conditional expression:
> > 
> > if (a == b && c > d || e < f && g != h)
> 
> Hi Peter,
> 
> As you may know, the above is not allowed in Linux kernel code. I'm
> not sure this has been written down anywhere formally. The C
> compilers
> supported by the Linux kernel emit a warning about expressions like
> the
> above and Linux kernel code should be warning-free.
> 
> I agree with you that code readability is important. However, I think
> that it's important for Linux kernel developers to make themselves
> familiar with expressions like (a & b && ...) since this style is
> common
> in the Linux kernel. Do you agree that the data below shows that not
> surrounding binary and expressions with parentheses is common in
> Linux
> kernel code?
> 
> $ git grep -nHE 'if.*&&[^()&]*&[^&]|if.*[^&]&[^&()]*&&' | wc -l
>     2548
> 
> Thanks,
> 
> Bart.


Hi Bart,

I agree that it's important for Linux kernel developers to make
themselves familiar with expressions like (a & b && ...)
I have no further comments.

Thanks
Peter

Reviewed-by: Peter Wang <peter.wang@mediatek.com>