Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   Stanislav Nijnikov <Stanislav.Nijnikov@wdc.com>
To:     Asutosh Das <asutoshd@codeaurora.org>,
        "subhashj@codeaurora.org" <subhashj@codeaurora.org>,
        "cang@codeaurora.org" <cang@codeaurora.org>,
        "vivek.gautam@codeaurora.org" <vivek.gautam@codeaurora.org>,
        "rnayak@codeaurora.org" <rnayak@codeaurora.org>,
        "vinholikatti@gmail.com" <vinholikatti@gmail.com>,
        "jejb@linux.vnet.ibm.com" <jejb@linux.vnet.ibm.com>,
        "martin.petersen@oracle.com" <martin.petersen@oracle.com>
CC:     "linux-scsi@vger.kernel.org" <linux-scsi@vger.kernel.org>,
        open list <linux-kernel@vger.kernel.org>
Subject: RE: [PATCH 5/9] scsi: ufs: add reference counting for scsi block
 requests
Thread-Topic: [PATCH 5/9] scsi: ufs: add reference counting for scsi block
 requests
Thread-Index: AQHTqtF3nCgA49p9yEi7LuXFGC4iE6Ou04rw
Date:   Wed, 21 Feb 2018 13:18:33 +0000
Message-ID: <MWHPR04MB1137BE3E48766435570460689ACE0@MWHPR04MB1137.namprd04.prod.outlook.com>
References: <cover.1519120988.git.asutoshd@codeaurora.org>
 <ff8f06512eca0c6275d3d486af0d430d8111c451.1519120988.git.asutoshd@codeaurora.org>
In-Reply-To: <ff8f06512eca0c6275d3d486af0d430d8111c451.1519120988.git.asutoshd@codeaurora.org>
Accept-Language: en-US
Content-Language: en-US
wdcipoutbound: EOP-TRUE
spamdiagnosticoutput: 1:99
spamdiagnosticmetadata: NSPM
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 86006cd3-4bf8-454e-ab07-08d5792d9ba7
X-MS-Exchange-CrossTenant-originalarrivaltime: 21 Feb 2018 13:18:33.3020
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: b61c8803-16f3-4c35-9b17-6f65f441df86
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MWHPR04MB1070
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


> -----Original Message-----
> From: linux-scsi-owner@vger.kernel.org [mailto:linux-scsi-
> owner@vger.kernel.org] On Behalf Of Asutosh Das
> Sent: Wednesday, February 21, 2018 6:57 AM
> To: subhashj@codeaurora.org; cang@codeaurora.org;
> vivek.gautam@codeaurora.org; rnayak@codeaurora.org;
> vinholikatti@gmail.com; jejb@linux.vnet.ibm.com;
> martin.petersen@oracle.com
> Cc: linux-scsi@vger.kernel.org; Asutosh Das <asutoshd@codeaurora.org>;
> open list <linux-kernel@vger.kernel.org>
> Subject: [PATCH 5/9] scsi: ufs: add reference counting for scsi block req=
uests
>=20
> From: Subhash Jadavani <subhashj@codeaurora.org>
>=20
> Currently we call the scsi_block_requests()/scsi_unblock_requests()
> whenever we want to block/unblock scsi requests but as there is no
> reference counting, nesting of these calls could leave us in undesired st=
ate
> sometime. Consider following call flow sequence:
> 1. func1() calls scsi_block_requests() but calls func2() before
>    calling scsi_unblock_requests()
> 2. func2() calls scsi_block_requests()
> 3. func2() calls scsi_unblock_requests() 4. func1() calls
> scsi_unblock_requests()
>=20
> As there is no reference counting, we will have scsi requests unblocked a=
fter
> #3 instead of it to be unblocked only after #4. Though we may not have
> failures seen with this, we might run into some failures in future.
> Better solution would be to fix this by adding reference counting.
>=20
> Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org>
> Signed-off-by: Can Guo <cang@codeaurora.org>
> Signed-off-by: Asutosh Das <asutoshd@codeaurora.org>
> ---
>  drivers/scsi/ufs/ufshcd.c | 44
> +++++++++++++++++++++++++++++++++++++-------
>  drivers/scsi/ufs/ufshcd.h |  5 +++++
>  2 files changed, 42 insertions(+), 7 deletions(-)
>=20
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c index
> 7a4df95..987b81b 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -264,6 +264,36 @@ static inline void ufshcd_disable_irq(struct ufs_hba
> *hba)
>  	}
>  }
>=20
> +void ufshcd_scsi_unblock_requests(struct ufs_hba *hba) {
> +	unsigned long flags;
> +	bool unblock =3D false;
> +
> +	spin_lock_irqsave(hba->host->host_lock, flags);
> +	hba->scsi_block_reqs_cnt--;
> +	unblock =3D !hba->scsi_block_reqs_cnt;
> +	spin_unlock_irqrestore(hba->host->host_lock, flags);
> +	if (unblock)
> +		scsi_unblock_requests(hba->host);
> +}
> +EXPORT_SYMBOL(ufshcd_scsi_unblock_requests);
> +
> +static inline void __ufshcd_scsi_block_requests(struct ufs_hba *hba) {
> +	if (!hba->scsi_block_reqs_cnt++)
> +		scsi_block_requests(hba->host);
> +}
> +
> +void ufshcd_scsi_block_requests(struct ufs_hba *hba) {
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(hba->host->host_lock, flags);
> +	__ufshcd_scsi_block_requests(hba);
> +	spin_unlock_irqrestore(hba->host->host_lock, flags); }
> +EXPORT_SYMBOL(ufshcd_scsi_block_requests);
> +
>  /* replace non-printable or non-ASCII characters with spaces */  static =
inline
> void ufshcd_remove_non_printable(char *val)  { @@ -1079,12 +1109,12 @@
> static int ufshcd_clock_scaling_prepare(struct ufs_hba *hba)
>  	 * make sure that there are no outstanding requests when
>  	 * clock scaling is in progress
>  	 */
> -	scsi_block_requests(hba->host);
> +	ufshcd_scsi_block_requests(hba);
>  	down_write(&hba->clk_scaling_lock);
>  	if (ufshcd_wait_for_doorbell_clr(hba, DOORBELL_CLR_TOUT_US)) {
>  		ret =3D -EBUSY;
>  		up_write(&hba->clk_scaling_lock);
> -		scsi_unblock_requests(hba->host);
> +		ufshcd_scsi_unblock_requests(hba);
>  	}
>=20
>  	return ret;
> @@ -1093,7 +1123,7 @@ static int ufshcd_clock_scaling_prepare(struct
> ufs_hba *hba)  static void ufshcd_clock_scaling_unprepare(struct ufs_hba
> *hba)  {
>  	up_write(&hba->clk_scaling_lock);
> -	scsi_unblock_requests(hba->host);
> +	ufshcd_scsi_unblock_requests(hba);
>  }
>=20
>  /**
> @@ -1413,7 +1443,7 @@ static void ufshcd_ungate_work(struct work_struct
> *work)
>  		hba->clk_gating.is_suspended =3D false;
>  	}
>  unblock_reqs:
> -	scsi_unblock_requests(hba->host);
> +	ufshcd_scsi_unblock_requests(hba);
>  }
>=20
>  /**
> @@ -1469,7 +1499,7 @@ int ufshcd_hold(struct ufs_hba *hba, bool async)
>  		 * work and to enable clocks.
>  		 */
>  	case CLKS_OFF:
> -		scsi_block_requests(hba->host);
> +		__ufshcd_scsi_block_requests(hba);
>  		hba->clk_gating.state =3D REQ_CLKS_ON;
>  		trace_ufshcd_clk_gating(dev_name(hba->dev),
>  					hba->clk_gating.state);
> @@ -5197,7 +5227,7 @@ static void ufshcd_err_handler(struct work_struct
> *work)
>=20
>  out:
>  	spin_unlock_irqrestore(hba->host->host_lock, flags);
> -	scsi_unblock_requests(hba->host);
> +	ufshcd_scsi_unblock_requests(hba);
>  	ufshcd_release(hba);
>  	pm_runtime_put_sync(hba->dev);
>  }
> @@ -5299,7 +5329,7 @@ static void ufshcd_check_errors(struct ufs_hba
> *hba)
>  		/* handle fatal errors only when link is functional */
>  		if (hba->ufshcd_state =3D=3D UFSHCD_STATE_OPERATIONAL) {
>  			/* block commands from scsi mid-layer */
> -			scsi_block_requests(hba->host);
> +			__ufshcd_scsi_block_requests(hba);
>=20
>  			hba->ufshcd_state =3D
> UFSHCD_STATE_EH_SCHEDULED;
>=20
> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h index
> 7a2dad3..4385741 100644
> --- a/drivers/scsi/ufs/ufshcd.h
> +++ b/drivers/scsi/ufs/ufshcd.h
> @@ -498,6 +498,7 @@ struct ufs_stats {
>   * @urgent_bkops_lvl: keeps track of urgent bkops level for device
>   * @is_urgent_bkops_lvl_checked: keeps track if the urgent bkops level f=
or
>   *  device is known or not.
> + * @scsi_block_reqs_cnt: reference counting for scsi block requests
>   */
>  struct ufs_hba {
>  	void __iomem *mmio_base;
> @@ -690,6 +691,7 @@ struct ufs_hba {
>=20
>  	struct rw_semaphore clk_scaling_lock;
>  	struct ufs_desc_size desc_size;
> +	int scsi_block_reqs_cnt;
>  };
>=20
>  /* Returns true if clocks can be gated. Otherwise false */ @@ -862,6 +86=
4,9
> @@ int ufshcd_map_desc_id_to_length(struct ufs_hba *hba, enum
> desc_idn desc_id,
>=20
>  u32 ufshcd_get_local_unipro_ver(struct ufs_hba *hba);
>=20
> +void ufshcd_scsi_block_requests(struct ufs_hba *hba); void
> +ufshcd_scsi_unblock_requests(struct ufs_hba *hba);
> +
>  /* Wrapper functions for safely calling variant operations */  static in=
line
> const char *ufshcd_get_var_name(struct ufs_hba *hba)  {
> --
> Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center,
> Inc.
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a
> Linux Foundation Collaborative Project.

1. The atomic variable and operations could be used for the reference count=
ing.
     This will allow to avoid usage of the locks.
2. Why are the ufshcd_scsi_block_requests/ ufshcd_scsi_unblock_requests=20
    functions not defined as static? They are not used outside ufshcd.c.

Regards
Stanislav