[v4,0/3] Untie the host lock entanglement - part 2

Here is the 2nd part in the sequel, watering down the scsi host lock
usage in the ufs driver. This work is motivated by a comment made by
Bart [1], of the abuse of the scsi host lock in the ufs driver.  Its
Precursor [2] removed the host lock around some of the host register
accesses.

This part replaces the scsi host lock by dedicated locks serializing
access to the clock gating and clock scaling members.

Changes compared to v3:
 - Keep the host lock when checking ufshcd_state (Bean)

Changes compared to v2:
 - Use clang-format to fix formating (Bart)
 - Use guard() in ufshcd_clkgate_enable_store (Bart)
 - Elaborate commit log (Bart)

Changes compared to v1:
 - use the guard() & scoped_guard() macros (Bart)
 - re-order struct ufs_clk_scaling and struct ufs_clk_gating (Bart)

[1] https://lore.kernel.org/linux-scsi/0b031b8f-c07c-42ef-af93-7336439d3c37@acm.org/
[2] https://lore.kernel.org/linux-scsi/20241024075033.562562-1-avri.altman@wdc.com/

Avri Altman (3):
  scsi: ufs: core: Prepare to introduce a new clock_gating lock
  scsi: ufs: core: Introduce a new clock_gating lock
  scsi: ufs: core: Introduce a new clock_scaling lock

 drivers/ufs/core/ufshcd.c | 255 +++++++++++++++++++-------------------
 include/ufs/ufshcd.h      |  24 ++--
 2 files changed, 144 insertions(+), 135 deletions(-)

On 11/18/24 6:41 AM, Avri Altman wrote:
> Removed hba->clk_gating.active_reqs check from ufshcd_is_ufs_dev_busy
> function to separate clock gating logic from general device busy checks.
> 
> Signed-off-by: Avri Altman <avri.altman@wdc.com>
> ---
>   drivers/ufs/core/ufshcd.c | 20 ++++++++++++++------
>   1 file changed, 14 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index e338867bc96c..be5fe2407382 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -258,10 +258,15 @@ ufs_get_desired_pm_lvl_for_dev_link_state(enum ufs_dev_pwr_mode dev_state,
>   	return UFS_PM_LVL_0;
>   }
>   
> +static bool ufshcd_has_pending_tasks(struct ufs_hba *hba)
> +{
> +	return hba->outstanding_tasks || hba->active_uic_cmd ||
> +	       hba->uic_async_done;
> +}
> +
>   static bool ufshcd_is_ufs_dev_busy(struct ufs_hba *hba)
>   {
> -	return (hba->clk_gating.active_reqs || hba->outstanding_reqs || hba->outstanding_tasks ||
> -		hba->active_uic_cmd || hba->uic_async_done);
> +	return hba->outstanding_reqs || ufshcd_has_pending_tasks(hba);
>   }
>   
>   static const struct ufs_dev_quirk ufs_fixups[] = {
> @@ -1943,7 +1948,9 @@ static void ufshcd_gate_work(struct work_struct *work)
>   		goto rel_lock;
>   	}
>   
> -	if (ufshcd_is_ufs_dev_busy(hba) || hba->ufshcd_state != UFSHCD_STATE_OPERATIONAL)
> +	if (ufshcd_is_ufs_dev_busy(hba) ||
> +	    hba->ufshcd_state != UFSHCD_STATE_OPERATIONAL ||
> +	    hba->clk_gating.active_reqs)
>   		goto rel_lock;
>   
>   	spin_unlock_irqrestore(hba->host->host_lock, flags);
> @@ -1999,8 +2006,7 @@ static void __ufshcd_release(struct ufs_hba *hba)
>   
>   	if (hba->clk_gating.active_reqs || hba->clk_gating.is_suspended ||
>   	    hba->ufshcd_state != UFSHCD_STATE_OPERATIONAL ||
> -	    hba->outstanding_tasks || !hba->clk_gating.is_initialized ||
> -	    hba->active_uic_cmd || hba->uic_async_done ||
> +	    ufshcd_has_pending_tasks(hba) || !hba->clk_gating.is_initialized ||
>   	    hba->clk_gating.state == CLKS_OFF)
>   		return;
>   
> @@ -8221,7 +8227,9 @@ static void ufshcd_rtc_work(struct work_struct *work)
>   	hba = container_of(to_delayed_work(work), struct ufs_hba, ufs_rtc_update_work);
>   
>   	 /* Update RTC only when there are no requests in progress and UFSHCI is operational */
> -	if (!ufshcd_is_ufs_dev_busy(hba) && hba->ufshcd_state == UFSHCD_STATE_OPERATIONAL)
> +	if (!ufshcd_is_ufs_dev_busy(hba) &&
> +	    hba->ufshcd_state == UFSHCD_STATE_OPERATIONAL &&
> +	    !hba->clk_gating.active_reqs)
>   		ufshcd_update_rtc(hba);
>   
>   	if (ufshcd_is_ufs_dev_active(hba) && hba->dev_info.rtc_update_period)

Hi Avri,

I see two changes in this patch: introduction of the function
ufshcd_has_pending_tasks() and removal of hba->clk_gating.active_reqs
from ufshcd_is_ufs_dev_busy(). Shouldn't this patch be split into two
patches - one patch per change?

Thanks,

Bart.

> Hi Avri,
> 
> I see two changes in this patch: introduction of the function
> ufshcd_has_pending_tasks() and removal of hba->clk_gating.active_reqs from
> ufshcd_is_ufs_dev_busy(). Shouldn't this patch be split into two patches -
> one patch per change?
Done.

Thanks,
Avri

> 
> Thanks,
> 
> Bart.