All of lore.kernel.org
 help / color / mirror / Atom feed
* Re: [RFC PATCH v1 1/2] ufs: introduce callbacks to get command information
@ 2020-06-20 19:48 kernel test robot
  0 siblings, 0 replies; 12+ messages in thread
From: kernel test robot @ 2020-06-20 19:48 UTC (permalink / raw)
  To: kbuild

[-- Attachment #1: Type: text/plain, Size: 9470 bytes --]

CC: kbuild-all(a)lists.01.org
In-Reply-To: <1592635992-35619-1-git-send-email-kwmad.kim@samsung.com>
References: <1592635992-35619-1-git-send-email-kwmad.kim@samsung.com>
TO: Kiwoong Kim <kwmad.kim@samsung.com>

Hi Kiwoong,

[FYI, it's a private test report for your RFC patch.]
[auto build test WARNING on scsi/for-next]
[also build test WARNING on mkp-scsi/for-next v5.8-rc1 next-20200618]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use  as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Kiwoong-Kim/ufs-introduce-callbacks-to-get-command-information/20200620-150310
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jejb/scsi.git for-next
:::::: branch date: 13 hours ago
:::::: commit date: 13 hours ago
config: x86_64-randconfig-m001-20200620 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-13) 9.3.0

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>

smatch warnings:
drivers/scsi/ufs/ufshcd.c:2548 ufshcd_queuecommand() warn: variable dereferenced before check 'cmd' (see line 2475)

# https://github.com/0day-ci/linux/commit/cff1afd5a0773e6da9106e953721996a56a9332c
git remote add linux-review https://github.com/0day-ci/linux
git remote update linux-review
git checkout cff1afd5a0773e6da9106e953721996a56a9332c
vim +/cmd +2548 drivers/scsi/ufs/ufshcd.c

4d2b8d40dd754e Bart Van Assche    2020-01-22  2457  
7a3e97b0dc4bba Santosh Yaraganavi 2012-02-29  2458  /**
7a3e97b0dc4bba Santosh Yaraganavi 2012-02-29  2459   * ufshcd_queuecommand - main entry point for SCSI requests
8aa29f192ca675 Bart Van Assche    2018-03-01  2460   * @host: SCSI host pointer
7a3e97b0dc4bba Santosh Yaraganavi 2012-02-29  2461   * @cmd: command from SCSI Midlayer
7a3e97b0dc4bba Santosh Yaraganavi 2012-02-29  2462   *
7a3e97b0dc4bba Santosh Yaraganavi 2012-02-29  2463   * Returns 0 for success, non-zero in case of failure
7a3e97b0dc4bba Santosh Yaraganavi 2012-02-29  2464   */
7a3e97b0dc4bba Santosh Yaraganavi 2012-02-29  2465  static int ufshcd_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *cmd)
7a3e97b0dc4bba Santosh Yaraganavi 2012-02-29  2466  {
7a3e97b0dc4bba Santosh Yaraganavi 2012-02-29  2467  	struct ufshcd_lrb *lrbp;
7a3e97b0dc4bba Santosh Yaraganavi 2012-02-29  2468  	struct ufs_hba *hba;
7a3e97b0dc4bba Santosh Yaraganavi 2012-02-29  2469  	unsigned long flags;
7a3e97b0dc4bba Santosh Yaraganavi 2012-02-29  2470  	int tag;
7a3e97b0dc4bba Santosh Yaraganavi 2012-02-29  2471  	int err = 0;
7a3e97b0dc4bba Santosh Yaraganavi 2012-02-29  2472  
7a3e97b0dc4bba Santosh Yaraganavi 2012-02-29  2473  	hba = shost_priv(host);
7a3e97b0dc4bba Santosh Yaraganavi 2012-02-29  2474  
7a3e97b0dc4bba Santosh Yaraganavi 2012-02-29 @2475  	tag = cmd->request->tag;
14497328b6a628 Yaniv Gardi        2016-02-01  2476  	if (!ufshcd_valid_tag(hba, tag)) {
14497328b6a628 Yaniv Gardi        2016-02-01  2477  		dev_err(hba->dev,
14497328b6a628 Yaniv Gardi        2016-02-01  2478  			"%s: invalid command tag %d: cmd=0x%p, cmd->request=0x%p",
14497328b6a628 Yaniv Gardi        2016-02-01  2479  			__func__, tag, cmd, cmd->request);
14497328b6a628 Yaniv Gardi        2016-02-01  2480  		BUG();
14497328b6a628 Yaniv Gardi        2016-02-01  2481  	}
7a3e97b0dc4bba Santosh Yaraganavi 2012-02-29  2482  
a3cd5ec55f6c72 Subhash Jadavani   2017-02-03  2483  	if (!down_read_trylock(&hba->clk_scaling_lock))
a3cd5ec55f6c72 Subhash Jadavani   2017-02-03  2484  		return SCSI_MLQUEUE_HOST_BUSY;
a3cd5ec55f6c72 Subhash Jadavani   2017-02-03  2485  
3441da7ddbdedf Sujit Reddy Thumma 2014-05-26  2486  	spin_lock_irqsave(hba->host->host_lock, flags);
3441da7ddbdedf Sujit Reddy Thumma 2014-05-26  2487  	switch (hba->ufshcd_state) {
3441da7ddbdedf Sujit Reddy Thumma 2014-05-26  2488  	case UFSHCD_STATE_OPERATIONAL:
3441da7ddbdedf Sujit Reddy Thumma 2014-05-26  2489  		break;
141f81651037ea Zang Leigang       2016-11-16  2490  	case UFSHCD_STATE_EH_SCHEDULED:
3441da7ddbdedf Sujit Reddy Thumma 2014-05-26  2491  	case UFSHCD_STATE_RESET:
7a3e97b0dc4bba Santosh Yaraganavi 2012-02-29  2492  		err = SCSI_MLQUEUE_HOST_BUSY;
3441da7ddbdedf Sujit Reddy Thumma 2014-05-26  2493  		goto out_unlock;
3441da7ddbdedf Sujit Reddy Thumma 2014-05-26  2494  	case UFSHCD_STATE_ERROR:
3441da7ddbdedf Sujit Reddy Thumma 2014-05-26  2495  		set_host_byte(cmd, DID_ERROR);
3441da7ddbdedf Sujit Reddy Thumma 2014-05-26  2496  		cmd->scsi_done(cmd);
3441da7ddbdedf Sujit Reddy Thumma 2014-05-26  2497  		goto out_unlock;
3441da7ddbdedf Sujit Reddy Thumma 2014-05-26  2498  	default:
3441da7ddbdedf Sujit Reddy Thumma 2014-05-26  2499  		dev_WARN_ONCE(hba->dev, 1, "%s: invalid state %d\n",
3441da7ddbdedf Sujit Reddy Thumma 2014-05-26  2500  				__func__, hba->ufshcd_state);
3441da7ddbdedf Sujit Reddy Thumma 2014-05-26  2501  		set_host_byte(cmd, DID_BAD_TARGET);
3441da7ddbdedf Sujit Reddy Thumma 2014-05-26  2502  		cmd->scsi_done(cmd);
3441da7ddbdedf Sujit Reddy Thumma 2014-05-26  2503  		goto out_unlock;
7a3e97b0dc4bba Santosh Yaraganavi 2012-02-29  2504  	}
53c12d0ef6fcb7 Yaniv Gardi        2016-02-01  2505  
53c12d0ef6fcb7 Yaniv Gardi        2016-02-01  2506  	/* if error handling is in progress, don't issue commands */
53c12d0ef6fcb7 Yaniv Gardi        2016-02-01  2507  	if (ufshcd_eh_in_progress(hba)) {
53c12d0ef6fcb7 Yaniv Gardi        2016-02-01  2508  		set_host_byte(cmd, DID_ERROR);
53c12d0ef6fcb7 Yaniv Gardi        2016-02-01  2509  		cmd->scsi_done(cmd);
53c12d0ef6fcb7 Yaniv Gardi        2016-02-01  2510  		goto out_unlock;
53c12d0ef6fcb7 Yaniv Gardi        2016-02-01  2511  	}
3441da7ddbdedf Sujit Reddy Thumma 2014-05-26  2512  	spin_unlock_irqrestore(hba->host->host_lock, flags);
7a3e97b0dc4bba Santosh Yaraganavi 2012-02-29  2513  
7fabb77b3aa016 Gilad Broner       2017-02-03  2514  	hba->req_abort_count = 0;
7fabb77b3aa016 Gilad Broner       2017-02-03  2515  
1ab27c9cf8b63d Sahitya Tummala    2014-09-25  2516  	err = ufshcd_hold(hba, true);
1ab27c9cf8b63d Sahitya Tummala    2014-09-25  2517  	if (err) {
1ab27c9cf8b63d Sahitya Tummala    2014-09-25  2518  		err = SCSI_MLQUEUE_HOST_BUSY;
1ab27c9cf8b63d Sahitya Tummala    2014-09-25  2519  		goto out;
1ab27c9cf8b63d Sahitya Tummala    2014-09-25  2520  	}
1ab27c9cf8b63d Sahitya Tummala    2014-09-25  2521  	WARN_ON(hba->clk_gating.state != CLKS_ON);
1ab27c9cf8b63d Sahitya Tummala    2014-09-25  2522  
7a3e97b0dc4bba Santosh Yaraganavi 2012-02-29  2523  	lrbp = &hba->lrb[tag];
7a3e97b0dc4bba Santosh Yaraganavi 2012-02-29  2524  
5a0b0cb9bee767 Sujit Reddy Thumma 2013-07-30  2525  	WARN_ON(lrbp->cmd);
7a3e97b0dc4bba Santosh Yaraganavi 2012-02-29  2526  	lrbp->cmd = cmd;
09a5a24ff36f90 Avri Altman        2018-11-22  2527  	lrbp->sense_bufflen = UFS_SENSE_SIZE;
7a3e97b0dc4bba Santosh Yaraganavi 2012-02-29  2528  	lrbp->sense_buffer = cmd->sense_buffer;
7a3e97b0dc4bba Santosh Yaraganavi 2012-02-29  2529  	lrbp->task_tag = tag;
0ce147d48a3e33 Subhash Jadavani   2014-09-25  2530  	lrbp->lun = ufshcd_scsi_to_upiu_lun(cmd->device->lun);
b852190e589abe Yaniv Gardi        2015-05-17  2531  	lrbp->intr_cmd = !ufshcd_is_intr_aggr_allowed(hba) ? true : false;
e0b299e36004f5 Gilad Broner       2017-02-03  2532  	lrbp->req_abort_skip = false;
7a3e97b0dc4bba Santosh Yaraganavi 2012-02-29  2533  
300bb13f5c7b1d Joao Pinto         2016-05-11  2534  	ufshcd_comp_scsi_upiu(hba, lrbp);
300bb13f5c7b1d Joao Pinto         2016-05-11  2535  
75b1cc4ad63afa Kiwoong Kim        2016-11-22  2536  	err = ufshcd_map_sg(hba, lrbp);
5a0b0cb9bee767 Sujit Reddy Thumma 2013-07-30  2537  	if (err) {
5a0b0cb9bee767 Sujit Reddy Thumma 2013-07-30  2538  		lrbp->cmd = NULL;
17c7d35f141ef6 Can Guo            2019-12-05  2539  		ufshcd_release(hba);
7a3e97b0dc4bba Santosh Yaraganavi 2012-02-29  2540  		goto out;
5a0b0cb9bee767 Sujit Reddy Thumma 2013-07-30  2541  	}
ad1a1b9cd67a4b Gilad Broner       2016-10-17  2542  	/* Make sure descriptors are ready before ringing the doorbell */
ad1a1b9cd67a4b Gilad Broner       2016-10-17  2543  	wmb();
7a3e97b0dc4bba Santosh Yaraganavi 2012-02-29  2544  
7a3e97b0dc4bba Santosh Yaraganavi 2012-02-29  2545  	/* issue command to the controller */
7a3e97b0dc4bba Santosh Yaraganavi 2012-02-29  2546  	spin_lock_irqsave(hba->host->host_lock, flags);
5905d4648e7ec2 Bart Van Assche    2020-01-22  2547  	ufshcd_vops_setup_xfer_req(hba, tag, true);
cff1afd5a0773e Kiwoong Kim        2020-06-20 @2548  	if (cmd)
cff1afd5a0773e Kiwoong Kim        2020-06-20  2549  		ufshcd_vops_cmd_log(hba, cmd, 1);
7a3e97b0dc4bba Santosh Yaraganavi 2012-02-29  2550  	ufshcd_send_command(hba, tag);
3441da7ddbdedf Sujit Reddy Thumma 2014-05-26  2551  out_unlock:
7a3e97b0dc4bba Santosh Yaraganavi 2012-02-29  2552  	spin_unlock_irqrestore(hba->host->host_lock, flags);
7a3e97b0dc4bba Santosh Yaraganavi 2012-02-29  2553  out:
a3cd5ec55f6c72 Subhash Jadavani   2017-02-03  2554  	up_read(&hba->clk_scaling_lock);
7a3e97b0dc4bba Santosh Yaraganavi 2012-02-29  2555  	return err;
7a3e97b0dc4bba Santosh Yaraganavi 2012-02-29  2556  }
7a3e97b0dc4bba Santosh Yaraganavi 2012-02-29  2557  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 32367 bytes --]

^ permalink raw reply	[flat|nested] 12+ messages in thread
* RE: [RFC PATCH v1 1/2] ufs: introduce callbacks to get command information
  2020-06-26 12:29       ` Stanley Chu
  2020-06-29  7:01         ` Kiwoong Kim
@ 2020-06-29  7:11         ` Kiwoong Kim
  1 sibling, 0 replies; 12+ messages in thread
From: Kiwoong Kim @ 2020-06-29  7:11 UTC (permalink / raw)
  To: linux-scsi, 'Stanley Chu'
  Cc: 'Alim Akhtar', 'Avri Altman',
	'James E.J. Bottomley', 'Martin K. Petersen',
	'Bean Huo (beanhuo)', 'Asutosh Das',
	cang, bvanassche

> Hi Kiwoong,
> 
> Kiwoong Kim <kwmad.kim@samsung.com> 於 2020年6月26日 週五 下午7:42寫道：
> >
> > > Hi Kiwoong,
> > >
> > > Kiwoong Kim <kwmad.kim@samsung.com> 於 2020年6月20日 週六 下午3:00寫道：
> > > >
> > > > Some SoC specific might need command history for various reasons,
> such
> > > > as stacking command contexts in system memory to check for debugging
> > > > in the future or scaling some DVFS knobs to boost IO throughput.
> > > >
> > > > What you would do with the information could be variant per SoC
> > > > vendor.
> > > >
> > > > Signed-off-by: Kiwoong Kim <kwmad.kim@samsung.com>
> > > > ---
> > > >  drivers/scsi/ufs/ufshcd.c | 4 ++++
> > > >  drivers/scsi/ufs/ufshcd.h | 8 ++++++++
> > > >  2 files changed, 12 insertions(+)
> > > >
> > > > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> > > > index 52abe82..0eae3ce 100644
> > > > --- a/drivers/scsi/ufs/ufshcd.c
> > > > +++ b/drivers/scsi/ufs/ufshcd.c
> > > > @@ -2545,6 +2545,8 @@ static int ufshcd_queuecommand(struct
> Scsi_Host
> > > *host, struct scsi_cmnd *cmd)
> > > >         /* issue command to the controller */
> > > >         spin_lock_irqsave(hba->host->host_lock, flags);
> > > >         ufshcd_vops_setup_xfer_req(hba, tag, true);
> > > > +       if (cmd)
> > > > +               ufshcd_vops_cmd_log(hba, cmd, 1);
> > > >         ufshcd_send_command(hba, tag);
> > > >  out_unlock:
> > > >         spin_unlock_irqrestore(hba->host->host_lock, flags); @@
> > > > -4890,6 +4892,8 @@ static void __ufshcd_transfer_req_compl(struct
> > > ufs_hba *hba,
> > > >                         /* Mark completed command as NULL in LRB */
> > > >                         lrbp->cmd = NULL;
> > > >                         lrbp->compl_time_stamp = ktime_get();
> > > > +                       ufshcd_vops_cmd_log(hba, cmd, 2);
> > > > +
> > > >                         /* Do not touch lrbp after scsi done */
> > > >                         cmd->scsi_done(cmd);
> > > >                         __ufshcd_release(hba);
> > >
> > > If your cmd_log vop callbacks are only existed in
> "ufshcd_queuecommand"
> > > and "ufshcd_transfer_req_compl", perhaps you could re-use
> > > "ufshcd_vops_setup_xfer_req()" and an added "ufshcd_vops_compl_req()"
> > > instead of a brand new "ufshcd_vops_cmd_log()" ?
> > >
> > > Thanks,
> > > Stanley Chu
> >
> > Currently, ufshcd_vops_setup_xfer_req doesn't get scsi_cmnd variable.
> 
> You could get scsi_cmnd by hba->lrb[tag].cmd if is_scsi_cmd is true in
> your ufshcd_vops_setup_xfer_req vendor implementation.
> 
> > Actually, when introduced this callback first, I was willing to make it
> do that
> > but someone gave me another idea. Then do you agree to change argument
> set of the function?
> >
> > And I can't find ufshcd_vops_compl_req in 5.9/scsi-queue. Could you let
> me know where to find?
> >
> 
> Sorry to not describing clearly. What I mean is that you could "add"
> ufshcd_vops_compl_xfer_req vop callback in your patch.
> 
> Thanks,
> Stanley Chu 

Got it and I'll refer to what you said.

Thanks.
Kiwoong Kim



^ permalink raw reply	[flat|nested] 12+ messages in thread
* RE: [RFC PATCH v1 1/2] ufs: introduce callbacks to get command information
  2020-06-26 12:29       ` Stanley Chu
@ 2020-06-29  7:01         ` Kiwoong Kim
  2020-06-29  7:11         ` Kiwoong Kim
  1 sibling, 0 replies; 12+ messages in thread
From: Kiwoong Kim @ 2020-06-29  7:01 UTC (permalink / raw)
  To: 'Stanley Chu'
  Cc: linux-scsi, 'Alim Akhtar', 'Avri Altman',
	'James E.J. Bottomley', 'Martin K. Petersen',
	'Bean Huo (beanhuo)', 'Asutosh Das',
	cang, bvanassche

> Hi Kiwoong,
> 
> Kiwoong Kim <kwmad.kim@samsung.com> 於 2020年6月26日 週五 下午7:42寫道：
> >
> > > Hi Kiwoong,
> > >
> > > Kiwoong Kim <kwmad.kim@samsung.com> 於 2020年6月20日 週六 下午3:00寫道：
> > > >
> > > > Some SoC specific might need command history for various reasons,
> such
> > > > as stacking command contexts in system memory to check for debugging
> > > > in the future or scaling some DVFS knobs to boost IO throughput.
> > > >
> > > > What you would do with the information could be variant per SoC
> > > > vendor.
> > > >
> > > > Signed-off-by: Kiwoong Kim <kwmad.kim@samsung.com>
> > > > ---
> > > >  drivers/scsi/ufs/ufshcd.c | 4 ++++
> > > >  drivers/scsi/ufs/ufshcd.h | 8 ++++++++
> > > >  2 files changed, 12 insertions(+)
> > > >
> > > > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> > > > index 52abe82..0eae3ce 100644
> > > > --- a/drivers/scsi/ufs/ufshcd.c
> > > > +++ b/drivers/scsi/ufs/ufshcd.c
> > > > @@ -2545,6 +2545,8 @@ static int ufshcd_queuecommand(struct
> Scsi_Host
> > > *host, struct scsi_cmnd *cmd)
> > > >         /* issue command to the controller */
> > > >         spin_lock_irqsave(hba->host->host_lock, flags);
> > > >         ufshcd_vops_setup_xfer_req(hba, tag, true);
> > > > +       if (cmd)
> > > > +               ufshcd_vops_cmd_log(hba, cmd, 1);
> > > >         ufshcd_send_command(hba, tag);
> > > >  out_unlock:
> > > >         spin_unlock_irqrestore(hba->host->host_lock, flags); @@
> > > > -4890,6 +4892,8 @@ static void __ufshcd_transfer_req_compl(struct
> > > ufs_hba *hba,
> > > >                         /* Mark completed command as NULL in LRB */
> > > >                         lrbp->cmd = NULL;
> > > >                         lrbp->compl_time_stamp = ktime_get();
> > > > +                       ufshcd_vops_cmd_log(hba, cmd, 2);
> > > > +
> > > >                         /* Do not touch lrbp after scsi done */
> > > >                         cmd->scsi_done(cmd);
> > > >                         __ufshcd_release(hba);
> > >
> > > If your cmd_log vop callbacks are only existed in
> "ufshcd_queuecommand"
> > > and "ufshcd_transfer_req_compl", perhaps you could re-use
> > > "ufshcd_vops_setup_xfer_req()" and an added "ufshcd_vops_compl_req()"
> > > instead of a brand new "ufshcd_vops_cmd_log()" ?
> > >
> > > Thanks,
> > > Stanley Chu
> >
> > Currently, ufshcd_vops_setup_xfer_req doesn't get scsi_cmnd variable.
> 
> You could get scsi_cmnd by hba->lrb[tag].cmd if is_scsi_cmd is true in
> your ufshcd_vops_setup_xfer_req vendor implementation.
> 
> > Actually, when introduced this callback first, I was willing to make it
> do that
> > but someone gave me another idea. Then do you agree to change argument
> set of the function?
> >
> > And I can't find ufshcd_vops_compl_req in 5.9/scsi-queue. Could you let
> me know where to find?
> >
> 
> Sorry to not describing clearly. What I mean is that you could "add"
> ufshcd_vops_compl_xfer_req vop callback in your patch.
> 
> Thanks,
> Stanley Chu

Got it and I'll refer to what you said.
Thanks.
Kiwoong Kim




^ permalink raw reply	[flat|nested] 12+ messages in thread
* Re: [RFC PATCH v1 1/2] ufs: introduce callbacks to get command information
  2020-06-26 11:42     ` Kiwoong Kim
@ 2020-06-26 12:29       ` Stanley Chu
  2020-06-29  7:01         ` Kiwoong Kim
  2020-06-29  7:11         ` Kiwoong Kim
  0 siblings, 2 replies; 12+ messages in thread
From: Stanley Chu @ 2020-06-26 12:29 UTC (permalink / raw)
  To: Kiwoong Kim
  Cc: linux-scsi, Alim Akhtar, Avri Altman, James E.J. Bottomley,
	Martin K. Petersen, Bean Huo (beanhuo),
	Asutosh Das, cang, bvanassche

Hi Kiwoong,

Kiwoong Kim <kwmad.kim@samsung.com> 於 2020年6月26日 週五 下午7:42寫道：
>
> > Hi Kiwoong,
> >
> > Kiwoong Kim <kwmad.kim@samsung.com> 於 2020年6月20日 週六 下午3:00寫道：
> > >
> > > Some SoC specific might need command history for various reasons, such
> > > as stacking command contexts in system memory to check for debugging
> > > in the future or scaling some DVFS knobs to boost IO throughput.
> > >
> > > What you would do with the information could be variant per SoC
> > > vendor.
> > >
> > > Signed-off-by: Kiwoong Kim <kwmad.kim@samsung.com>
> > > ---
> > >  drivers/scsi/ufs/ufshcd.c | 4 ++++
> > >  drivers/scsi/ufs/ufshcd.h | 8 ++++++++
> > >  2 files changed, 12 insertions(+)
> > >
> > > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> > > index 52abe82..0eae3ce 100644
> > > --- a/drivers/scsi/ufs/ufshcd.c
> > > +++ b/drivers/scsi/ufs/ufshcd.c
> > > @@ -2545,6 +2545,8 @@ static int ufshcd_queuecommand(struct Scsi_Host
> > *host, struct scsi_cmnd *cmd)
> > >         /* issue command to the controller */
> > >         spin_lock_irqsave(hba->host->host_lock, flags);
> > >         ufshcd_vops_setup_xfer_req(hba, tag, true);
> > > +       if (cmd)
> > > +               ufshcd_vops_cmd_log(hba, cmd, 1);
> > >         ufshcd_send_command(hba, tag);
> > >  out_unlock:
> > >         spin_unlock_irqrestore(hba->host->host_lock, flags); @@
> > > -4890,6 +4892,8 @@ static void __ufshcd_transfer_req_compl(struct
> > ufs_hba *hba,
> > >                         /* Mark completed command as NULL in LRB */
> > >                         lrbp->cmd = NULL;
> > >                         lrbp->compl_time_stamp = ktime_get();
> > > +                       ufshcd_vops_cmd_log(hba, cmd, 2);
> > > +
> > >                         /* Do not touch lrbp after scsi done */
> > >                         cmd->scsi_done(cmd);
> > >                         __ufshcd_release(hba);
> >
> > If your cmd_log vop callbacks are only existed in "ufshcd_queuecommand"
> > and "ufshcd_transfer_req_compl", perhaps you could re-use
> > "ufshcd_vops_setup_xfer_req()" and an added "ufshcd_vops_compl_req()"
> > instead of a brand new "ufshcd_vops_cmd_log()" ?
> >
> > Thanks,
> > Stanley Chu
>
> Currently, ufshcd_vops_setup_xfer_req doesn't get scsi_cmnd variable.

You could get scsi_cmnd by hba->lrb[tag].cmd if is_scsi_cmd is true in
your ufshcd_vops_setup_xfer_req vendor implementation.

> Actually, when introduced this callback first, I was willing to make it do that
> but someone gave me another idea. Then do you agree to change argument set of the function?
>
> And I can't find ufshcd_vops_compl_req in 5.9/scsi-queue. Could you let me know where to find?
>

Sorry to not describing clearly. What I mean is that you could "add"
ufshcd_vops_compl_xfer_req vop callback in your patch.

Thanks,
Stanley Chu

^ permalink raw reply	[flat|nested] 12+ messages in thread
* RE: [RFC PATCH v1 1/2] ufs: introduce callbacks to get command information
  2020-06-25 14:05   ` Stanley Chu
@ 2020-06-26 11:42     ` Kiwoong Kim
  2020-06-26 12:29       ` Stanley Chu
  0 siblings, 1 reply; 12+ messages in thread
From: Kiwoong Kim @ 2020-06-26 11:42 UTC (permalink / raw)
  To: 'Stanley Chu'
  Cc: linux-scsi, 'Alim Akhtar', 'Avri Altman',
	'James E.J. Bottomley', 'Martin K. Petersen',
	'Bean Huo (beanhuo)', 'Asutosh Das',
	cang, bvanassche

> Hi Kiwoong,
> 
> Kiwoong Kim <kwmad.kim@samsung.com> 於 2020年6月20日 週六 下午3:00寫道：
> >
> > Some SoC specific might need command history for various reasons, such
> > as stacking command contexts in system memory to check for debugging
> > in the future or scaling some DVFS knobs to boost IO throughput.
> >
> > What you would do with the information could be variant per SoC
> > vendor.
> >
> > Signed-off-by: Kiwoong Kim <kwmad.kim@samsung.com>
> > ---
> >  drivers/scsi/ufs/ufshcd.c | 4 ++++
> >  drivers/scsi/ufs/ufshcd.h | 8 ++++++++
> >  2 files changed, 12 insertions(+)
> >
> > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> > index 52abe82..0eae3ce 100644
> > --- a/drivers/scsi/ufs/ufshcd.c
> > +++ b/drivers/scsi/ufs/ufshcd.c
> > @@ -2545,6 +2545,8 @@ static int ufshcd_queuecommand(struct Scsi_Host
> *host, struct scsi_cmnd *cmd)
> >         /* issue command to the controller */
> >         spin_lock_irqsave(hba->host->host_lock, flags);
> >         ufshcd_vops_setup_xfer_req(hba, tag, true);
> > +       if (cmd)
> > +               ufshcd_vops_cmd_log(hba, cmd, 1);
> >         ufshcd_send_command(hba, tag);
> >  out_unlock:
> >         spin_unlock_irqrestore(hba->host->host_lock, flags); @@
> > -4890,6 +4892,8 @@ static void __ufshcd_transfer_req_compl(struct
> ufs_hba *hba,
> >                         /* Mark completed command as NULL in LRB */
> >                         lrbp->cmd = NULL;
> >                         lrbp->compl_time_stamp = ktime_get();
> > +                       ufshcd_vops_cmd_log(hba, cmd, 2);
> > +
> >                         /* Do not touch lrbp after scsi done */
> >                         cmd->scsi_done(cmd);
> >                         __ufshcd_release(hba);
> 
> If your cmd_log vop callbacks are only existed in "ufshcd_queuecommand"
> and "ufshcd_transfer_req_compl", perhaps you could re-use
> "ufshcd_vops_setup_xfer_req()" and an added "ufshcd_vops_compl_req()"
> instead of a brand new "ufshcd_vops_cmd_log()" ?
> 
> Thanks,
> Stanley Chu

Currently, ufshcd_vops_setup_xfer_req doesn't get scsi_cmnd variable.
Actually, when introduced this callback first, I was willing to make it do that
but someone gave me another idea. Then do you agree to change argument set of the function?

And I can't find ufshcd_vops_compl_req in 5.9/scsi-queue. Could you let me know where to find?

Thank you for your opinions.




^ permalink raw reply	[flat|nested] 12+ messages in thread
* Re: [RFC PATCH v1 1/2] ufs: introduce callbacks to get command information
  2020-06-20  6:53 ` Kiwoong Kim
  2020-06-20 16:33   ` Bart Van Assche
  2020-06-22 11:35     ` Dan Carpenter
@ 2020-06-25 14:05   ` Stanley Chu
  2020-06-26 11:42     ` Kiwoong Kim
  2 siblings, 1 reply; 12+ messages in thread
From: Stanley Chu @ 2020-06-25 14:05 UTC (permalink / raw)
  To: Kiwoong Kim
  Cc: linux-scsi, Alim Akhtar, Avri Altman, James E.J. Bottomley,
	Martin K. Petersen, Bean Huo (beanhuo),
	Asutosh Das, cang, bvanassche

Hi Kiwoong,

Kiwoong Kim <kwmad.kim@samsung.com> 於 2020年6月20日 週六 下午3:00寫道：
>
> Some SoC specific might need command history for
> various reasons, such as stacking command contexts
> in system memory to check for debugging in the future
> or scaling some DVFS knobs to boost IO throughput.
>
> What you would do with the information could be
> variant per SoC vendor.
>
> Signed-off-by: Kiwoong Kim <kwmad.kim@samsung.com>
> ---
>  drivers/scsi/ufs/ufshcd.c | 4 ++++
>  drivers/scsi/ufs/ufshcd.h | 8 ++++++++
>  2 files changed, 12 insertions(+)
>
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 52abe82..0eae3ce 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -2545,6 +2545,8 @@ static int ufshcd_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *cmd)
>         /* issue command to the controller */
>         spin_lock_irqsave(hba->host->host_lock, flags);
>         ufshcd_vops_setup_xfer_req(hba, tag, true);
> +       if (cmd)
> +               ufshcd_vops_cmd_log(hba, cmd, 1);
>         ufshcd_send_command(hba, tag);
>  out_unlock:
>         spin_unlock_irqrestore(hba->host->host_lock, flags);
> @@ -4890,6 +4892,8 @@ static void __ufshcd_transfer_req_compl(struct ufs_hba *hba,
>                         /* Mark completed command as NULL in LRB */
>                         lrbp->cmd = NULL;
>                         lrbp->compl_time_stamp = ktime_get();
> +                       ufshcd_vops_cmd_log(hba, cmd, 2);
> +
>                         /* Do not touch lrbp after scsi done */
>                         cmd->scsi_done(cmd);
>                         __ufshcd_release(hba);

If your cmd_log vop callbacks are only existed in
"ufshcd_queuecommand" and "ufshcd_transfer_req_compl", perhaps you
could re-use "ufshcd_vops_setup_xfer_req()" and an added
"ufshcd_vops_compl_req()" instead of a brand new
"ufshcd_vops_cmd_log()" ?

Thanks,
Stanley Chu

^ permalink raw reply	[flat|nested] 12+ messages in thread
* Re: [RFC PATCH v1 1/2] ufs: introduce callbacks to get command information
  2020-06-23  2:28     ` Kiwoong Kim
@ 2020-06-24  2:53       ` Bart Van Assche
  0 siblings, 0 replies; 12+ messages in thread
From: Bart Van Assche @ 2020-06-24  2:53 UTC (permalink / raw)
  To: Kiwoong Kim, linux-scsi, alim.akhtar, avri.altman, jejb,
	martin.petersen, beanhuo, asutoshd, cang,
	'Christoph Hellwig'

On 2020-06-22 19:28, Kiwoong Kim wrote:
> If you could get the information, Many would exploit it for their respective purposes.
> But, it's important for the information to contain accurate timestamps when the driver
> hooks it, if you're trying to figure out something wrong.
> 
> As for scaling DVFS knobs to boost UFS throughput, locating in the ufs driver is more
> Beneficial because SoC vendors have their own power domains and thus lead to make
> different way of what to scale up to boost. If it's populated in block layer, there is
> no way to introduce boosting per SoC.

Thanks for the clarification. Unfortunately we do not yet have an API
for sharing information between I/O schedulers and block drivers ...

Bart.



^ permalink raw reply	[flat|nested] 12+ messages in thread
* RE: [RFC PATCH v1 1/2] ufs: introduce callbacks to get command information
  2020-06-20 16:33   ` Bart Van Assche
@ 2020-06-23  2:28     ` Kiwoong Kim
  2020-06-24  2:53       ` Bart Van Assche
  0 siblings, 1 reply; 12+ messages in thread
From: Kiwoong Kim @ 2020-06-23  2:28 UTC (permalink / raw)
  To: 'Bart Van Assche',
	linux-scsi, alim.akhtar, avri.altman, jejb, martin.petersen,
	beanhuo, asutoshd, cang, 'Christoph Hellwig'

> On 2020-06-19 23:53, Kiwoong Kim wrote:
> > Some SoC specific might need command history for various reasons, such
> > as stacking command contexts in system memory to check for debugging
> > in the future or scaling some DVFS knobs to boost IO throughput.
> >
> > What you would do with the information could be variant per SoC
> > vendor.
> 
> Isn't this something that should be done in an I/O scheduler instead of in
> a SCSI LLD? I don't like the idea behind this patch series at all.
> 
> Bart.

If you could get the information, Many would exploit it for their respective purposes.
But, it's important for the information to contain accurate timestamps when the driver
hooks it, if you're trying to figure out something wrong.

As for scaling DVFS knobs to boost UFS throughput, locating in the ufs driver is more
Beneficial because SoC vendors have their own power domains and thus lead to make
different way of what to scale up to boost. If it's populated in block layer, there is
no way to introduce boosting per SoC.


^ permalink raw reply	[flat|nested] 12+ messages in thread
* Re: [RFC PATCH v1 1/2] ufs: introduce callbacks to get command information
  2020-06-20  6:53 ` Kiwoong Kim
@ 2020-06-22 11:35     ` Dan Carpenter
  2020-06-22 11:35     ` Dan Carpenter
  2020-06-25 14:05   ` Stanley Chu
  2 siblings, 0 replies; 12+ messages in thread
From: Dan Carpenter @ 2020-06-22 11:35 UTC (permalink / raw)
  To: kbuild

[-- Attachment #1: Type: text/plain, Size: 8382 bytes --]

Hi Kiwoong,

url:    https://github.com/0day-ci/linux/commits/Kiwoong-Kim/ufs-introduce-callbacks-to-get-command-information/20200620-150310
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jejb/scsi.git for-next
config: x86_64-randconfig-m001-20200620 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-13) 9.3.0

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>

smatch warnings:
drivers/scsi/ufs/ufshcd.c:2548 ufshcd_queuecommand() warn: variable dereferenced before check 'cmd' (see line 2475)

# https://github.com/0day-ci/linux/commit/cff1afd5a0773e6da9106e953721996a56a9332c
git remote add linux-review https://github.com/0day-ci/linux
git remote update linux-review
git checkout cff1afd5a0773e6da9106e953721996a56a9332c
vim +/cmd +2548 drivers/scsi/ufs/ufshcd.c

7a3e97b0dc4bba Santosh Yaraganavi 2012-02-29  2465  static int ufshcd_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *cmd)
7a3e97b0dc4bba Santosh Yaraganavi 2012-02-29  2466  {
7a3e97b0dc4bba Santosh Yaraganavi 2012-02-29  2467  	struct ufshcd_lrb *lrbp;
7a3e97b0dc4bba Santosh Yaraganavi 2012-02-29  2468  	struct ufs_hba *hba;
7a3e97b0dc4bba Santosh Yaraganavi 2012-02-29  2469  	unsigned long flags;
7a3e97b0dc4bba Santosh Yaraganavi 2012-02-29  2470  	int tag;
7a3e97b0dc4bba Santosh Yaraganavi 2012-02-29  2471  	int err = 0;
7a3e97b0dc4bba Santosh Yaraganavi 2012-02-29  2472  
7a3e97b0dc4bba Santosh Yaraganavi 2012-02-29  2473  	hba = shost_priv(host);
7a3e97b0dc4bba Santosh Yaraganavi 2012-02-29  2474  
7a3e97b0dc4bba Santosh Yaraganavi 2012-02-29 @2475  	tag = cmd->request->tag;
                                                              ^^^^^^^^^^^^^^^^^
Dereference.

14497328b6a628 Yaniv Gardi        2016-02-01  2476  	if (!ufshcd_valid_tag(hba, tag)) {
14497328b6a628 Yaniv Gardi        2016-02-01  2477  		dev_err(hba->dev,
14497328b6a628 Yaniv Gardi        2016-02-01  2478  			"%s: invalid command tag %d: cmd=0x%p, cmd->request=0x%p",
14497328b6a628 Yaniv Gardi        2016-02-01  2479  			__func__, tag, cmd, cmd->request);
14497328b6a628 Yaniv Gardi        2016-02-01  2480  		BUG();
14497328b6a628 Yaniv Gardi        2016-02-01  2481  	}
7a3e97b0dc4bba Santosh Yaraganavi 2012-02-29  2482  
a3cd5ec55f6c72 Subhash Jadavani   2017-02-03  2483  	if (!down_read_trylock(&hba->clk_scaling_lock))
a3cd5ec55f6c72 Subhash Jadavani   2017-02-03  2484  		return SCSI_MLQUEUE_HOST_BUSY;
a3cd5ec55f6c72 Subhash Jadavani   2017-02-03  2485  
3441da7ddbdedf Sujit Reddy Thumma 2014-05-26  2486  	spin_lock_irqsave(hba->host->host_lock, flags);
3441da7ddbdedf Sujit Reddy Thumma 2014-05-26  2487  	switch (hba->ufshcd_state) {
3441da7ddbdedf Sujit Reddy Thumma 2014-05-26  2488  	case UFSHCD_STATE_OPERATIONAL:
3441da7ddbdedf Sujit Reddy Thumma 2014-05-26  2489  		break;
141f81651037ea Zang Leigang       2016-11-16  2490  	case UFSHCD_STATE_EH_SCHEDULED:
3441da7ddbdedf Sujit Reddy Thumma 2014-05-26  2491  	case UFSHCD_STATE_RESET:
7a3e97b0dc4bba Santosh Yaraganavi 2012-02-29  2492  		err = SCSI_MLQUEUE_HOST_BUSY;
3441da7ddbdedf Sujit Reddy Thumma 2014-05-26  2493  		goto out_unlock;
3441da7ddbdedf Sujit Reddy Thumma 2014-05-26  2494  	case UFSHCD_STATE_ERROR:
3441da7ddbdedf Sujit Reddy Thumma 2014-05-26  2495  		set_host_byte(cmd, DID_ERROR);
3441da7ddbdedf Sujit Reddy Thumma 2014-05-26  2496  		cmd->scsi_done(cmd);
3441da7ddbdedf Sujit Reddy Thumma 2014-05-26  2497  		goto out_unlock;
3441da7ddbdedf Sujit Reddy Thumma 2014-05-26  2498  	default:
3441da7ddbdedf Sujit Reddy Thumma 2014-05-26  2499  		dev_WARN_ONCE(hba->dev, 1, "%s: invalid state %d\n",
3441da7ddbdedf Sujit Reddy Thumma 2014-05-26  2500  				__func__, hba->ufshcd_state);
3441da7ddbdedf Sujit Reddy Thumma 2014-05-26  2501  		set_host_byte(cmd, DID_BAD_TARGET);
3441da7ddbdedf Sujit Reddy Thumma 2014-05-26  2502  		cmd->scsi_done(cmd);
3441da7ddbdedf Sujit Reddy Thumma 2014-05-26  2503  		goto out_unlock;
7a3e97b0dc4bba Santosh Yaraganavi 2012-02-29  2504  	}
53c12d0ef6fcb7 Yaniv Gardi        2016-02-01  2505  
53c12d0ef6fcb7 Yaniv Gardi        2016-02-01  2506  	/* if error handling is in progress, don't issue commands */
53c12d0ef6fcb7 Yaniv Gardi        2016-02-01  2507  	if (ufshcd_eh_in_progress(hba)) {
53c12d0ef6fcb7 Yaniv Gardi        2016-02-01  2508  		set_host_byte(cmd, DID_ERROR);
53c12d0ef6fcb7 Yaniv Gardi        2016-02-01  2509  		cmd->scsi_done(cmd);
53c12d0ef6fcb7 Yaniv Gardi        2016-02-01  2510  		goto out_unlock;
53c12d0ef6fcb7 Yaniv Gardi        2016-02-01  2511  	}
3441da7ddbdedf Sujit Reddy Thumma 2014-05-26  2512  	spin_unlock_irqrestore(hba->host->host_lock, flags);
7a3e97b0dc4bba Santosh Yaraganavi 2012-02-29  2513  
7fabb77b3aa016 Gilad Broner       2017-02-03  2514  	hba->req_abort_count = 0;
7fabb77b3aa016 Gilad Broner       2017-02-03  2515  
1ab27c9cf8b63d Sahitya Tummala    2014-09-25  2516  	err = ufshcd_hold(hba, true);
1ab27c9cf8b63d Sahitya Tummala    2014-09-25  2517  	if (err) {
1ab27c9cf8b63d Sahitya Tummala    2014-09-25  2518  		err = SCSI_MLQUEUE_HOST_BUSY;
1ab27c9cf8b63d Sahitya Tummala    2014-09-25  2519  		goto out;
1ab27c9cf8b63d Sahitya Tummala    2014-09-25  2520  	}
1ab27c9cf8b63d Sahitya Tummala    2014-09-25  2521  	WARN_ON(hba->clk_gating.state != CLKS_ON);
1ab27c9cf8b63d Sahitya Tummala    2014-09-25  2522  
7a3e97b0dc4bba Santosh Yaraganavi 2012-02-29  2523  	lrbp = &hba->lrb[tag];
7a3e97b0dc4bba Santosh Yaraganavi 2012-02-29  2524  
5a0b0cb9bee767 Sujit Reddy Thumma 2013-07-30  2525  	WARN_ON(lrbp->cmd);
7a3e97b0dc4bba Santosh Yaraganavi 2012-02-29  2526  	lrbp->cmd = cmd;
09a5a24ff36f90 Avri Altman        2018-11-22  2527  	lrbp->sense_bufflen = UFS_SENSE_SIZE;
7a3e97b0dc4bba Santosh Yaraganavi 2012-02-29  2528  	lrbp->sense_buffer = cmd->sense_buffer;
7a3e97b0dc4bba Santosh Yaraganavi 2012-02-29  2529  	lrbp->task_tag = tag;
0ce147d48a3e33 Subhash Jadavani   2014-09-25  2530  	lrbp->lun = ufshcd_scsi_to_upiu_lun(cmd->device->lun);
b852190e589abe Yaniv Gardi        2015-05-17  2531  	lrbp->intr_cmd = !ufshcd_is_intr_aggr_allowed(hba) ? true : false;
e0b299e36004f5 Gilad Broner       2017-02-03  2532  	lrbp->req_abort_skip = false;
7a3e97b0dc4bba Santosh Yaraganavi 2012-02-29  2533  
300bb13f5c7b1d Joao Pinto         2016-05-11  2534  	ufshcd_comp_scsi_upiu(hba, lrbp);
300bb13f5c7b1d Joao Pinto         2016-05-11  2535  
75b1cc4ad63afa Kiwoong Kim        2016-11-22  2536  	err = ufshcd_map_sg(hba, lrbp);
5a0b0cb9bee767 Sujit Reddy Thumma 2013-07-30  2537  	if (err) {
5a0b0cb9bee767 Sujit Reddy Thumma 2013-07-30  2538  		lrbp->cmd = NULL;
17c7d35f141ef6 Can Guo            2019-12-05  2539  		ufshcd_release(hba);
7a3e97b0dc4bba Santosh Yaraganavi 2012-02-29  2540  		goto out;
5a0b0cb9bee767 Sujit Reddy Thumma 2013-07-30  2541  	}
ad1a1b9cd67a4b Gilad Broner       2016-10-17  2542  	/* Make sure descriptors are ready before ringing the doorbell */
ad1a1b9cd67a4b Gilad Broner       2016-10-17  2543  	wmb();
7a3e97b0dc4bba Santosh Yaraganavi 2012-02-29  2544  
7a3e97b0dc4bba Santosh Yaraganavi 2012-02-29  2545  	/* issue command to the controller */
7a3e97b0dc4bba Santosh Yaraganavi 2012-02-29  2546  	spin_lock_irqsave(hba->host->host_lock, flags);
5905d4648e7ec2 Bart Van Assche    2020-01-22  2547  	ufshcd_vops_setup_xfer_req(hba, tag, true);
cff1afd5a0773e Kiwoong Kim        2020-06-20 @2548  	if (cmd)
                                                            ^^^
If "cmd" is NULL then we would have already crashed.

cff1afd5a0773e Kiwoong Kim        2020-06-20  2549  		ufshcd_vops_cmd_log(hba, cmd, 1);
7a3e97b0dc4bba Santosh Yaraganavi 2012-02-29  2550  	ufshcd_send_command(hba, tag);
3441da7ddbdedf Sujit Reddy Thumma 2014-05-26  2551  out_unlock:
7a3e97b0dc4bba Santosh Yaraganavi 2012-02-29  2552  	spin_unlock_irqrestore(hba->host->host_lock, flags);
7a3e97b0dc4bba Santosh Yaraganavi 2012-02-29  2553  out:
a3cd5ec55f6c72 Subhash Jadavani   2017-02-03  2554  	up_read(&hba->clk_scaling_lock);
7a3e97b0dc4bba Santosh Yaraganavi 2012-02-29  2555  	return err;
7a3e97b0dc4bba Santosh Yaraganavi 2012-02-29  2556  }

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 32367 bytes --]

^ permalink raw reply	[flat|nested] 12+ messages in thread
* Re: [RFC PATCH v1 1/2] ufs: introduce callbacks to get command information
@ 2020-06-22 11:35     ` Dan Carpenter
  0 siblings, 0 replies; 12+ messages in thread
From: Dan Carpenter @ 2020-06-22 11:35 UTC (permalink / raw)
  To: kbuild-all

[-- Attachment #1: Type: text/plain, Size: 8382 bytes --]

Hi Kiwoong,

url:    https://github.com/0day-ci/linux/commits/Kiwoong-Kim/ufs-introduce-callbacks-to-get-command-information/20200620-150310
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jejb/scsi.git for-next
config: x86_64-randconfig-m001-20200620 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-13) 9.3.0

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>

smatch warnings:
drivers/scsi/ufs/ufshcd.c:2548 ufshcd_queuecommand() warn: variable dereferenced before check 'cmd' (see line 2475)

# https://github.com/0day-ci/linux/commit/cff1afd5a0773e6da9106e953721996a56a9332c
git remote add linux-review https://github.com/0day-ci/linux
git remote update linux-review
git checkout cff1afd5a0773e6da9106e953721996a56a9332c
vim +/cmd +2548 drivers/scsi/ufs/ufshcd.c

7a3e97b0dc4bba Santosh Yaraganavi 2012-02-29  2465  static int ufshcd_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *cmd)
7a3e97b0dc4bba Santosh Yaraganavi 2012-02-29  2466  {
7a3e97b0dc4bba Santosh Yaraganavi 2012-02-29  2467  	struct ufshcd_lrb *lrbp;
7a3e97b0dc4bba Santosh Yaraganavi 2012-02-29  2468  	struct ufs_hba *hba;
7a3e97b0dc4bba Santosh Yaraganavi 2012-02-29  2469  	unsigned long flags;
7a3e97b0dc4bba Santosh Yaraganavi 2012-02-29  2470  	int tag;
7a3e97b0dc4bba Santosh Yaraganavi 2012-02-29  2471  	int err = 0;
7a3e97b0dc4bba Santosh Yaraganavi 2012-02-29  2472  
7a3e97b0dc4bba Santosh Yaraganavi 2012-02-29  2473  	hba = shost_priv(host);
7a3e97b0dc4bba Santosh Yaraganavi 2012-02-29  2474  
7a3e97b0dc4bba Santosh Yaraganavi 2012-02-29 @2475  	tag = cmd->request->tag;
                                                              ^^^^^^^^^^^^^^^^^
Dereference.

14497328b6a628 Yaniv Gardi        2016-02-01  2476  	if (!ufshcd_valid_tag(hba, tag)) {
14497328b6a628 Yaniv Gardi        2016-02-01  2477  		dev_err(hba->dev,
14497328b6a628 Yaniv Gardi        2016-02-01  2478  			"%s: invalid command tag %d: cmd=0x%p, cmd->request=0x%p",
14497328b6a628 Yaniv Gardi        2016-02-01  2479  			__func__, tag, cmd, cmd->request);
14497328b6a628 Yaniv Gardi        2016-02-01  2480  		BUG();
14497328b6a628 Yaniv Gardi        2016-02-01  2481  	}
7a3e97b0dc4bba Santosh Yaraganavi 2012-02-29  2482  
a3cd5ec55f6c72 Subhash Jadavani   2017-02-03  2483  	if (!down_read_trylock(&hba->clk_scaling_lock))
a3cd5ec55f6c72 Subhash Jadavani   2017-02-03  2484  		return SCSI_MLQUEUE_HOST_BUSY;
a3cd5ec55f6c72 Subhash Jadavani   2017-02-03  2485  
3441da7ddbdedf Sujit Reddy Thumma 2014-05-26  2486  	spin_lock_irqsave(hba->host->host_lock, flags);
3441da7ddbdedf Sujit Reddy Thumma 2014-05-26  2487  	switch (hba->ufshcd_state) {
3441da7ddbdedf Sujit Reddy Thumma 2014-05-26  2488  	case UFSHCD_STATE_OPERATIONAL:
3441da7ddbdedf Sujit Reddy Thumma 2014-05-26  2489  		break;
141f81651037ea Zang Leigang       2016-11-16  2490  	case UFSHCD_STATE_EH_SCHEDULED:
3441da7ddbdedf Sujit Reddy Thumma 2014-05-26  2491  	case UFSHCD_STATE_RESET:
7a3e97b0dc4bba Santosh Yaraganavi 2012-02-29  2492  		err = SCSI_MLQUEUE_HOST_BUSY;
3441da7ddbdedf Sujit Reddy Thumma 2014-05-26  2493  		goto out_unlock;
3441da7ddbdedf Sujit Reddy Thumma 2014-05-26  2494  	case UFSHCD_STATE_ERROR:
3441da7ddbdedf Sujit Reddy Thumma 2014-05-26  2495  		set_host_byte(cmd, DID_ERROR);
3441da7ddbdedf Sujit Reddy Thumma 2014-05-26  2496  		cmd->scsi_done(cmd);
3441da7ddbdedf Sujit Reddy Thumma 2014-05-26  2497  		goto out_unlock;
3441da7ddbdedf Sujit Reddy Thumma 2014-05-26  2498  	default:
3441da7ddbdedf Sujit Reddy Thumma 2014-05-26  2499  		dev_WARN_ONCE(hba->dev, 1, "%s: invalid state %d\n",
3441da7ddbdedf Sujit Reddy Thumma 2014-05-26  2500  				__func__, hba->ufshcd_state);
3441da7ddbdedf Sujit Reddy Thumma 2014-05-26  2501  		set_host_byte(cmd, DID_BAD_TARGET);
3441da7ddbdedf Sujit Reddy Thumma 2014-05-26  2502  		cmd->scsi_done(cmd);
3441da7ddbdedf Sujit Reddy Thumma 2014-05-26  2503  		goto out_unlock;
7a3e97b0dc4bba Santosh Yaraganavi 2012-02-29  2504  	}
53c12d0ef6fcb7 Yaniv Gardi        2016-02-01  2505  
53c12d0ef6fcb7 Yaniv Gardi        2016-02-01  2506  	/* if error handling is in progress, don't issue commands */
53c12d0ef6fcb7 Yaniv Gardi        2016-02-01  2507  	if (ufshcd_eh_in_progress(hba)) {
53c12d0ef6fcb7 Yaniv Gardi        2016-02-01  2508  		set_host_byte(cmd, DID_ERROR);
53c12d0ef6fcb7 Yaniv Gardi        2016-02-01  2509  		cmd->scsi_done(cmd);
53c12d0ef6fcb7 Yaniv Gardi        2016-02-01  2510  		goto out_unlock;
53c12d0ef6fcb7 Yaniv Gardi        2016-02-01  2511  	}
3441da7ddbdedf Sujit Reddy Thumma 2014-05-26  2512  	spin_unlock_irqrestore(hba->host->host_lock, flags);
7a3e97b0dc4bba Santosh Yaraganavi 2012-02-29  2513  
7fabb77b3aa016 Gilad Broner       2017-02-03  2514  	hba->req_abort_count = 0;
7fabb77b3aa016 Gilad Broner       2017-02-03  2515  
1ab27c9cf8b63d Sahitya Tummala    2014-09-25  2516  	err = ufshcd_hold(hba, true);
1ab27c9cf8b63d Sahitya Tummala    2014-09-25  2517  	if (err) {
1ab27c9cf8b63d Sahitya Tummala    2014-09-25  2518  		err = SCSI_MLQUEUE_HOST_BUSY;
1ab27c9cf8b63d Sahitya Tummala    2014-09-25  2519  		goto out;
1ab27c9cf8b63d Sahitya Tummala    2014-09-25  2520  	}
1ab27c9cf8b63d Sahitya Tummala    2014-09-25  2521  	WARN_ON(hba->clk_gating.state != CLKS_ON);
1ab27c9cf8b63d Sahitya Tummala    2014-09-25  2522  
7a3e97b0dc4bba Santosh Yaraganavi 2012-02-29  2523  	lrbp = &hba->lrb[tag];
7a3e97b0dc4bba Santosh Yaraganavi 2012-02-29  2524  
5a0b0cb9bee767 Sujit Reddy Thumma 2013-07-30  2525  	WARN_ON(lrbp->cmd);
7a3e97b0dc4bba Santosh Yaraganavi 2012-02-29  2526  	lrbp->cmd = cmd;
09a5a24ff36f90 Avri Altman        2018-11-22  2527  	lrbp->sense_bufflen = UFS_SENSE_SIZE;
7a3e97b0dc4bba Santosh Yaraganavi 2012-02-29  2528  	lrbp->sense_buffer = cmd->sense_buffer;
7a3e97b0dc4bba Santosh Yaraganavi 2012-02-29  2529  	lrbp->task_tag = tag;
0ce147d48a3e33 Subhash Jadavani   2014-09-25  2530  	lrbp->lun = ufshcd_scsi_to_upiu_lun(cmd->device->lun);
b852190e589abe Yaniv Gardi        2015-05-17  2531  	lrbp->intr_cmd = !ufshcd_is_intr_aggr_allowed(hba) ? true : false;
e0b299e36004f5 Gilad Broner       2017-02-03  2532  	lrbp->req_abort_skip = false;
7a3e97b0dc4bba Santosh Yaraganavi 2012-02-29  2533  
300bb13f5c7b1d Joao Pinto         2016-05-11  2534  	ufshcd_comp_scsi_upiu(hba, lrbp);
300bb13f5c7b1d Joao Pinto         2016-05-11  2535  
75b1cc4ad63afa Kiwoong Kim        2016-11-22  2536  	err = ufshcd_map_sg(hba, lrbp);
5a0b0cb9bee767 Sujit Reddy Thumma 2013-07-30  2537  	if (err) {
5a0b0cb9bee767 Sujit Reddy Thumma 2013-07-30  2538  		lrbp->cmd = NULL;
17c7d35f141ef6 Can Guo            2019-12-05  2539  		ufshcd_release(hba);
7a3e97b0dc4bba Santosh Yaraganavi 2012-02-29  2540  		goto out;
5a0b0cb9bee767 Sujit Reddy Thumma 2013-07-30  2541  	}
ad1a1b9cd67a4b Gilad Broner       2016-10-17  2542  	/* Make sure descriptors are ready before ringing the doorbell */
ad1a1b9cd67a4b Gilad Broner       2016-10-17  2543  	wmb();
7a3e97b0dc4bba Santosh Yaraganavi 2012-02-29  2544  
7a3e97b0dc4bba Santosh Yaraganavi 2012-02-29  2545  	/* issue command to the controller */
7a3e97b0dc4bba Santosh Yaraganavi 2012-02-29  2546  	spin_lock_irqsave(hba->host->host_lock, flags);
5905d4648e7ec2 Bart Van Assche    2020-01-22  2547  	ufshcd_vops_setup_xfer_req(hba, tag, true);
cff1afd5a0773e Kiwoong Kim        2020-06-20 @2548  	if (cmd)
                                                            ^^^
If "cmd" is NULL then we would have already crashed.

cff1afd5a0773e Kiwoong Kim        2020-06-20  2549  		ufshcd_vops_cmd_log(hba, cmd, 1);
7a3e97b0dc4bba Santosh Yaraganavi 2012-02-29  2550  	ufshcd_send_command(hba, tag);
3441da7ddbdedf Sujit Reddy Thumma 2014-05-26  2551  out_unlock:
7a3e97b0dc4bba Santosh Yaraganavi 2012-02-29  2552  	spin_unlock_irqrestore(hba->host->host_lock, flags);
7a3e97b0dc4bba Santosh Yaraganavi 2012-02-29  2553  out:
a3cd5ec55f6c72 Subhash Jadavani   2017-02-03  2554  	up_read(&hba->clk_scaling_lock);
7a3e97b0dc4bba Santosh Yaraganavi 2012-02-29  2555  	return err;
7a3e97b0dc4bba Santosh Yaraganavi 2012-02-29  2556  }

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 32367 bytes --]

^ permalink raw reply	[flat|nested] 12+ messages in thread
* Re: [RFC PATCH v1 1/2] ufs: introduce callbacks to get command information
  2020-06-20  6:53 ` Kiwoong Kim
@ 2020-06-20 16:33   ` Bart Van Assche
  2020-06-23  2:28     ` Kiwoong Kim
  2020-06-22 11:35     ` Dan Carpenter
  2020-06-25 14:05   ` Stanley Chu
  2 siblings, 1 reply; 12+ messages in thread
From: Bart Van Assche @ 2020-06-20 16:33 UTC (permalink / raw)
  To: Kiwoong Kim, linux-scsi, alim.akhtar, avri.altman, jejb,
	martin.petersen, beanhuo, asutoshd, cang, Christoph Hellwig

On 2020-06-19 23:53, Kiwoong Kim wrote:
> Some SoC specific might need command history for
> various reasons, such as stacking command contexts
> in system memory to check for debugging in the future
> or scaling some DVFS knobs to boost IO throughput.
> 
> What you would do with the information could be
> variant per SoC vendor.

Isn't this something that should be done in an I/O scheduler instead of
in a SCSI LLD? I don't like the idea behind this patch series at all.

Bart.

^ permalink raw reply	[flat|nested] 12+ messages in thread
* [RFC PATCH v1 1/2] ufs: introduce callbacks to get command information
       [not found] <CGME20200620070044epcas2p269e3c266c86c65dd0e894d8188036a30@epcas2p2.samsung.com>
@ 2020-06-20  6:53 ` Kiwoong Kim
  2020-06-20 16:33   ` Bart Van Assche
                     ` (2 more replies)
  0 siblings, 3 replies; 12+ messages in thread
From: Kiwoong Kim @ 2020-06-20  6:53 UTC (permalink / raw)
  To: linux-scsi, alim.akhtar, avri.altman, jejb, martin.petersen,
	beanhuo, asutoshd, cang, bvanassche
  Cc: Kiwoong Kim

Some SoC specific might need command history for
various reasons, such as stacking command contexts
in system memory to check for debugging in the future
or scaling some DVFS knobs to boost IO throughput.

What you would do with the information could be
variant per SoC vendor.

Signed-off-by: Kiwoong Kim <kwmad.kim@samsung.com>
---
 drivers/scsi/ufs/ufshcd.c | 4 ++++
 drivers/scsi/ufs/ufshcd.h | 8 ++++++++
 2 files changed, 12 insertions(+)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 52abe82..0eae3ce 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -2545,6 +2545,8 @@ static int ufshcd_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *cmd)
 	/* issue command to the controller */
 	spin_lock_irqsave(hba->host->host_lock, flags);
 	ufshcd_vops_setup_xfer_req(hba, tag, true);
+	if (cmd)
+		ufshcd_vops_cmd_log(hba, cmd, 1);
 	ufshcd_send_command(hba, tag);
 out_unlock:
 	spin_unlock_irqrestore(hba->host->host_lock, flags);
@@ -4890,6 +4892,8 @@ static void __ufshcd_transfer_req_compl(struct ufs_hba *hba,
 			/* Mark completed command as NULL in LRB */
 			lrbp->cmd = NULL;
 			lrbp->compl_time_stamp = ktime_get();
+			ufshcd_vops_cmd_log(hba, cmd, 2);
+
 			/* Do not touch lrbp after scsi done */
 			cmd->scsi_done(cmd);
 			__ufshcd_release(hba);
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index c774012..80c4f0d 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -307,6 +307,7 @@ struct ufs_hba_variant_ops {
 	void	(*config_scaling_param)(struct ufs_hba *hba,
 					struct devfreq_dev_profile *profile,
 					void *data);
+	void	(*cmd_log)(struct ufs_hba *hba, struct scsi_cmnd *cmd, int enter);
 };
 
 /* clock gating state  */
@@ -1137,6 +1138,13 @@ static inline void ufshcd_vops_config_scaling_param(struct ufs_hba *hba,
 		hba->vops->config_scaling_param(hba, profile, data);
 }
 
+static inline void ufshcd_vops_cmd_log(struct ufs_hba *hba,
+					 struct scsi_cmnd *cmd, int enter)
+{
+	if (hba->vops && hba->vops->cmd_log)
+		hba->vops->cmd_log(hba, cmd, enter);
+}
+
 extern struct ufs_pm_lvl_states ufs_pm_lvl_states[];
 
 /*
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 12+ messages in thread
end of thread, other threads:[~2020-06-29 21:23 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-20 19:48 [RFC PATCH v1 1/2] ufs: introduce callbacks to get command information kernel test robot
     [not found] <CGME20200620070044epcas2p269e3c266c86c65dd0e894d8188036a30@epcas2p2.samsung.com>
2020-06-20  6:53 ` Kiwoong Kim
2020-06-20 16:33   ` Bart Van Assche
2020-06-23  2:28     ` Kiwoong Kim
2020-06-24  2:53       ` Bart Van Assche
2020-06-22 11:35   ` Dan Carpenter
2020-06-22 11:35     ` Dan Carpenter
2020-06-25 14:05   ` Stanley Chu
2020-06-26 11:42     ` Kiwoong Kim
2020-06-26 12:29       ` Stanley Chu
2020-06-29  7:01         ` Kiwoong Kim
2020-06-29  7:11         ` Kiwoong Kim

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.