Re: [PATCH 4/5] scsi: ufs: rework link start-up process

[Sujit Reddy Thumma <sthumma@codeaurora.org>]

On 4/30/2013 12:03 PM, Seungwon Jeon wrote:
> On Monday, April 29, 2013, Sujit Reddy Thumma wrote:
>> On 4/29/2013 3:54 PM, Seungwon Jeon wrote:
>>> On Monday, April 29, 2013, Sujit Reddy Thumma wrote:
>>>> On 4/26/2013 10:44 AM, Seungwon Jeon wrote:
>>>>> On Thursday, April 25, 2013 , Sujit Reddy Thumma wrote:
>>>>>> On 4/24/2013 9:36 PM, Seungwon Jeon wrote:
>>>>>>> Link start-up requires long time with multiphase handshakes
>>>>>>> between UFS host and device. This affects driver's probe time.
>>>>>>> This patch let link start-up run asynchronously.
>>>>>>> And completion time of uic command is defined to avoid a
>>>>>>> permanent wait.
>>>>>>
>>>>>> I have similar patch posted few days back "scsi: ufs: Generalize UFS
>>>>>> Interconnect Layer (UIC) command support" which does a bit more (mutex,
>>>>>> error handling) than what is done here. Can that be used/improved?
>>>>> I completed to check your patch to compare it now.
>>>>> Though it's just my thought, the patch I sent is more intuitive on the whole.
>>>>> Considering other dme operations which I have introduced, it looks like matched.
>>>>
>>>> There are lot of code duplications you might want to minimize building a
>>>> DME command.
>>>>
>>>>> Of course, you may disagree.
>>>>> But I think the part of mutex is needed. It's a good point.
>>>>> In case of error handling, I didn't catch nothing special.
>>>>> Rather, handling link lost case is not proper.
>>>>> When ufs host meets link lost status, it should start with dme_reset not retried dme_linkstartup.
>>>>
>>>> In section 7.2.1 (Host Controller Initialization) of JESD223A UFS HCI
>>>> v1.1  specification I find this -
>>>>
>>>> 6. Sent DME_LINKSTARTUP command to start the link startup procedure
>>>> 9. Check value of HCS.DP and make sure that there is a device attached
>>>> to the Link. If presence of a device is detected, go to step 10;
>>>> otherwise, resend the DME_LINKSTARTUP command after IS.ULLS has been set
>>>> to 1 (Go to step 6). IS.ULLS equal 1 indicates that the UFS Device is
>>>> ready for a link startup.
>>>>
>>>> Going by the spec. just retrying with DME_LINKSTARTUP is correct.
>>> Yes, as you quoted above, HCI standard mentions that.
>>> Also, the following is mentioned.
>>> UIC Link Lost Status (ULLS) corresponds to the UniPro DME_LINKLOST.ind
>>> I just referred unipro specification.
>>> When DME_LINKLOST.ind is generated, this affects the Link is put in the LinkLost state.
>>> Unipro spec says that DME User must apply a DME_RESET to redo the boot sequence.
>>> If there is misunderstood meaning and I have something to miss, we can discuss more.
>>> Please let me know.
>>
>> Yes, it looks like the two specs. are conflicting each other. I guess we
>> need to take this to Jedec for clarification. Meanwhile, to be on safe
>> side can we add a retry mechanism that does ufshcd_hba_enable() before
>> sending DME_LINKSTARTUP again? This way we can be sure that the
>> DME_RESET and DME_ENABLE is taken care by the host reset itself.
> Yes, If the latter case is applied, 'ufshcd_hba_enable' will be start entry for retry.
> Further, IS.ULLS could be handled through the interrupt instead of polling for retry mechanism?

Agree, but the interrupt handling will be tailored for two things - 1) 
bootup case where scsi_scan_host is not yet called. 2) the case where 
link lost occurred after a long time after bootup where there is no need 
to do scsi_scan_host again.

>
>>
>>>
>>>>
>>>> In addition, it doesn't say what happens if IS.ULLS never sets to 1.
>>>> Probably, the case which never happens.
>>>>
>>>>> And it would be good if link start-up procedure is done in separate process, not in driver probe.
>>>> True.
>>>>
>>>>> If it's all right with you, I'd like to update lock mechanism for uic command.
>>>>> I can add your signed-off. Please let me know your opinion.
>>>> I would like to get a third opinion as both the patches needs modifications.
>>>>
>>>> Some comments below:
>>>>
>>>>>>
>>>>>>>
>>>>>>> Signed-off-by: Seungwon Jeon <tgih.jun@samsung.com>
>>>>>>> ---
>>>>>>>      drivers/scsi/ufs/ufshcd.c |  114 +++++++++++++++++++++++++++++++++-----------
>>>>>>>      drivers/scsi/ufs/ufshcd.h |    6 ++-
>>>>>>>      2 files changed, 89 insertions(+), 31 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>>>>>>> index efe2256..76ff332 100644
>>>>>>> --- a/drivers/scsi/ufs/ufshcd.c
>>>>>>> +++ b/drivers/scsi/ufs/ufshcd.c
>>>>>>> @@ -38,6 +38,7 @@
>>>>>>>      #define UFSHCD_ENABLE_INTRS	(UTP_TRANSFER_REQ_COMPL |\
>>>>>>>      				 UTP_TASK_REQ_COMPL |\
>>>>>>>      				 UFSHCD_ERROR_MASK)
>>>>>>> +#define UIC_CMD_TIMEOUT	100
>>>>>>>
>>>>>>>      enum {
>>>>>>>      	UFSHCD_MAX_CHANNEL	= 0,
>>>>>>> @@ -357,13 +358,15 @@ static inline void ufshcd_hba_capabilities(struct ufs_hba *hba)
>>>>>>>      }
>>>>>>>
>>>>>>>      /**
>>>>>>> - * ufshcd_send_uic_command - Send UIC commands to unipro layers
>>>>>>> + * ufshcd_dispatch_uic_cmd - Dispatch UIC commands to unipro layers
>>>>>>>       * @hba: per adapter instance
>>>>>>>       * @uic_command: UIC command
>>>>>>>       */
>>>>>>>      static inline void
>>>>>>> -ufshcd_send_uic_command(struct ufs_hba *hba, struct uic_command *uic_cmnd)
>>>>>>> +ufshcd_dispatch_uic_cmd(struct ufs_hba *hba, struct uic_command *uic_cmnd)
>>>>>>>      {
>>>>>>> +	init_completion(&uic_cmnd->done);
>>>>>>> +
>>>>>>>      	/* Write Args */
>>>>>>>      	ufshcd_writel(hba, REG_UIC_COMMAND_ARG_1, uic_cmnd->argument1);
>>>>>>>      	ufshcd_writel(hba, REG_UIC_COMMAND_ARG_2, uic_cmnd->argument2);
>>>>>>> @@ -375,6 +378,45 @@ ufshcd_send_uic_command(struct ufs_hba *hba, struct uic_command *uic_cmnd)
>>>>>>>      }
>>>>>>>
>>>>>>>      /**
>>>>>>> + * ufshcd_wait_for_uic_cmd - Wait complectioin of UIC command
>>>>>>> + * @hba: per adapter instance
>>>>>>> + * @uic_command: UIC command
>>>>>>> + *
>>>>>>> + * Returns 0 only if success.
>>>>>>> + */
>>>>>>> +static int ufshcd_wait_for_uic_cmd(struct ufs_hba *hba)
>>>>>>> +{
>>>>>>> +	struct uic_command *uic_cmd = &hba->active_uic_cmd;
>>>>>>> +	int ret;
>>>>>>> +
>>>>>>> +	if (wait_for_completion_timeout(&uic_cmd->done,
>>>>>>> +					msecs_to_jiffies(UIC_CMD_TIMEOUT)))
>>>>>>> +		ret = ufshcd_get_uic_cmd_result(hba);
>>>>>>> +	else
>>>>>>> +		ret = -ETIMEDOUT;
>>>>>>> +
>>>>>>> +	return ret;
>>>>>>> +}
>>>>>>> +
>>>>>>> +/**
>>>>>>> + * ufshcd_ready_uic_cmd - Check if controller is ready
>>>>>>> + *                        to accept UIC commands
>>>>>>> + * @hba: per adapter instance
>>>>>>> + * Return true on success, else false
>>>>>>> + */
>>>>>>> +static inline bool ufshcd_ready_uic_cmd(struct ufs_hba *hba)
>>>>>>> +{
>>>>>>> +	if (ufshcd_readl(hba, REG_CONTROLLER_STATUS) & UIC_COMMAND_READY) {
>>>>>>> +		return true;
>>>>>>> +	} else {
>>>>>>> +		dev_err(hba->dev,
>>>>>>> +				"Controller not ready"
>>>>>>> +				" to accept UIC commands\n");
>>>>>>> +		return false;
>>>>>>> +	}
>>>>>>> +}
>>>>>>> +
>>>>>>> +/**
>>>>>>>       * ufshcd_map_sg - Map scatter-gather list to prdt
>>>>>>>       * @lrbp - pointer to local reference block
>>>>>>>       *
>>>>>>> @@ -735,15 +777,10 @@ static int ufshcd_dme_link_startup(struct ufs_hba *hba)
>>>>>>>      {
>>>>>>>      	struct uic_command *uic_cmd;
>>>>>>>      	unsigned long flags;
>>>>>>> +	int ret;
>>>>>>>
>>>>>>> -	/* check if controller is ready to accept UIC commands */
>>>>>>> -	if (((ufshcd_readl(hba, REG_CONTROLLER_STATUS)) &
>>>>>>> -	    UIC_COMMAND_READY) == 0x0) {
>>>>>>> -		dev_err(hba->dev,
>>>>>>> -			"Controller not ready"
>>>>>>> -			" to accept UIC commands\n");
>>>>>>> +	if (!ufshcd_ready_uic_cmd(hba))
>>>>>>>      		return -EIO;
>>>>>>> -	}
>>>>>>>
>>>>>>>      	spin_lock_irqsave(hba->host->host_lock, flags);
>>>>>>>
>>>>>>> @@ -754,13 +791,16 @@ static int ufshcd_dme_link_startup(struct ufs_hba *hba)
>>>>>>>      	uic_cmd->argument2 = 0;
>>>>>>>      	uic_cmd->argument3 = 0;
>>>>>>>
>>>>>>> -	/* enable UIC related interrupts */
>>>>>>> -	ufshcd_enable_intr(hba, UIC_COMMAND_COMPL);
>>>>>>> +	/* Dispatching UIC commands to controller */
>>>>>>> +	ufshcd_dispatch_uic_cmd(hba, uic_cmd);
>>>>>>>
>>>>>>> -	/* sending UIC commands to controller */
>>>>>>> -	ufshcd_send_uic_command(hba, uic_cmd);
>>>>>>>      	spin_unlock_irqrestore(hba->host->host_lock, flags);
>>>>>>> -	return 0;
>>>>>>> +
>>>>>>> +	ret = ufshcd_wait_for_uic_cmd(hba);
>>>>
>>>> Error code is incorrect. only -ETIMEDOUT is valid others are just DME
>>>> errors.
>>> Only success returns '0', other positive value from dme and -ETIMEDOUT mean failure.
>>> Error code can be reused purely, not being redefined.
>>> I am seeing that -EINVAL represents from 01h to 07h in your handling.
>>> It looks like error's detail is disappear. Exact return might be needed from DME.
>> okay.
>>
>>>
>>>>
>>>> Also, spec. clearly mentions a retry mechanism which means that there
>>>> could be some timing issues anticipated where the UIC layer cannot
>>>> respond properly.
>>> Sorry, I didn't catch your meaning fully. Where can I refer to it?
>>
>> I meant the same retry mechanism mentioned in the section 7.2.1 (Host
>> Controller Initialization) of JESD223A UFS HCI v1.1.
>>
>>>
>>>>
>>>>
>>>>>>> +	if (ret)
>>>>>>> +		dev_err(hba->dev, "link startup: error code %d returned\n", ret);
>>>>>>> +
>>>>>>> +	return ret;
>>>>>>>      }
>>>>>>>
>>>>>>>      /**
>>>>>>> @@ -898,6 +938,9 @@ static int ufshcd_initialize_hba(struct ufs_hba *hba)
>>>>>>>      	if (ufshcd_hba_enable(hba))
>>>>>>>      		return -EIO;
>>>>>>>
>>>>>>> +	/* enable UIC related interrupts */
>>>>>>> +	ufshcd_enable_intr(hba, UIC_COMMAND_COMPL | UIC_ERROR);
>>>>
>>>> The recovery when UIC_ERROR happens is broken because of re-entrancy to
>>>> dme_link_startup from ufshcd_fatal_err_handler(). So better handle with
>>>> timeout than allowing controller to raise a UIC_ERROR until that is fixed?
>>> I also recognize error handling should be done further.
>>> Ok, I agree with you.
>>>
>>>>
>>>>>>> +
>>>>>>>      	/* Configure UTRL and UTMRL base address registers */
>>>>>>>      	ufshcd_writel(hba, REG_UTP_TRANSFER_REQ_LIST_BASE_L,
>>>>>>>      		      lower_32_bits(hba->utrdl_dma_addr));
>>>>>>> @@ -909,7 +952,9 @@ static int ufshcd_initialize_hba(struct ufs_hba *hba)
>>>>>>>      		      upper_32_bits(hba->utmrdl_dma_addr));
>>>>>>>
>>>>>>>      	/* Initialize unipro link startup procedure */
>>>>>>> -	return ufshcd_dme_link_startup(hba);
>>>>>>> +	schedule_work(&hba->link_startup_wq);
>>>>>>> +
>>>>>>> +	return 0;
>>>>>>>      }
>>>>>>>
>>>>>>>      /**
>>>>>>> @@ -1186,6 +1231,16 @@ ufshcd_transfer_rsp_status(struct ufs_hba *hba, struct ufshcd_lrb *lrbp)
>>>>>>>      }
>>>>>>>
>>>>>>>      /**
>>>>>>> + * ufshcd_uic_cmd_compl - handle completion of uic command
>>>>>>> + * @hba: per adapter instance
>>>>>>> + */
>>>>>>> +static void ufshcd_uic_cmd_compl(struct ufs_hba *hba, u32 intr_status)
>>>>>>> +{
>>>>>>> +	if (intr_status & UIC_COMMAND_COMPL)
>>>>
>>>> why this redundant check if it is already checked in ufshcd_sl_intr()?
>>> Yes, it's currently not needed.
>>> It will be used to identify several uic command. ([PATCH 5/5] scsi: ufs: add dme operations)
>>> Anyway, it's better to be removed here.
>>>
>>>>
>>>>>>> +		complete(&hba->active_uic_cmd.done);
>>>>>>> +}
>>>>>>> +
>>>>>>> +/**
>>>>>>>       * ufshcd_transfer_req_compl - handle SCSI and query command completion
>>>>>>>       * @hba: per adapter instance
>>>>>>>       */
>>>>>>> @@ -1225,25 +1280,26 @@ static void ufshcd_transfer_req_compl(struct ufs_hba *hba)
>>>>>>>      }
>>>>>>>
>>>>>>>      /**
>>>>>>> - * ufshcd_uic_cc_handler - handle UIC command completion
>>>>>>> + * ufshcd_link_startup - link initialization
>>>>>>>       * @work: pointer to a work queue structure
>>>>>>> - *
>>>>>>> - * Returns 0 on success, non-zero value on failure
>>>>>>>       */
>>>>>>> -static void ufshcd_uic_cc_handler (struct work_struct *work)
>>>>>>> +static void ufshcd_link_startup(struct work_struct *work)
>>>>>>>      {
>>>>>>>      	struct ufs_hba *hba;
>>>>>>> +	int ret;
>>>>>>>
>>>>>>> -	hba = container_of(work, struct ufs_hba, uic_workq);
>>>>>>> +	hba = container_of(work, struct ufs_hba, link_startup_wq);
>>>>>>>
>>>>>>> -	if ((hba->active_uic_cmd.command == UIC_CMD_DME_LINK_STARTUP) &&
>>>>>>> -	    !(ufshcd_get_uic_cmd_result(hba))) {
>>>>>>> +	ret = ufshcd_dme_link_startup(hba);
>>>>>>> +	if (ret)
>>>>>>> +		goto out;
>>>>>>>
>>>>>>> -		if (ufshcd_make_hba_operational(hba))
>>>>>>> -			dev_err(hba->dev,
>>>>>>> -				"cc: hba not operational state\n");
>>>>>>> -		return;
>>>>>>> -	}
>>>>>>> +	ret = ufshcd_make_hba_operational(hba);
>>>>>>> +	if (ret)
>>>>>>> +		goto out;
>>>>>>> +	return;
>>>>>>> +out:
>>>>>>> +	dev_err(hba->dev, "link startup failed %d\n", ret);
>>>>>>>      }
>>>>>>>
>>>>>>>      /**
>>>>>>> @@ -1307,7 +1363,7 @@ static void ufshcd_sl_intr(struct ufs_hba *hba, u32 intr_status)
>>>>>>>      		ufshcd_err_handler(hba);
>>>>>>>
>>>>>>>      	if (intr_status & UIC_COMMAND_COMPL)
>>>>>>> -		schedule_work(&hba->uic_workq);
>>>>>>> +		ufshcd_uic_cmd_compl(hba, intr_status);
>>>>>>>
>>>>>>>      	if (intr_status & UTP_TASK_REQ_COMPL)
>>>>>>>      		ufshcd_tmc_handler(hba);
>>>>>>> @@ -1694,7 +1750,7 @@ int ufshcd_init(struct device *dev, struct ufs_hba **hba_handle,
>>>>>>>      	init_waitqueue_head(&hba->ufshcd_tm_wait_queue);
>>>>>>>
>>>>>>>      	/* Initialize work queues */
>>>>>>> -	INIT_WORK(&hba->uic_workq, ufshcd_uic_cc_handler);
>>>>>>> +	INIT_WORK(&hba->link_startup_wq, ufshcd_link_startup);
>>>>
>>>> Can we use async function calls kernel/async.c instead of having work
>>>> queues as this is only used during boot up?
>>> As we know, both probe and resume are sensitive to execution time.
>>> I guess link startup procedure will also be activated in driver's resume.
>>> Do you have any specific reason for async function?
>>>
>>
>> I guess most UFS devices currently are embedded and have rootfs on them.
>> If we use schedule_work there is no synchronization mechanism to check
>> whether the device link startup is completed and device is available for
>> the userspace to mount the partitions. Having async mechanism in place,
>> the prepare_namespace() does wait for such async probes to be completed
>> before mounting the rootfs.
> I understand your meaning.
> If you are considering that, I think 'scsi_scan_host' has a role for that.
> 'scsi_scan_host' will be the conclusion and be done in async subsystem.
> 'scsi_scan_host' is actually called, after finishing link startup procedure.

with this patch the scsi_scan_host() is called after the work is 
scheduled. Which means that link_startup_wq might be scheduled after 
prepare_namespace().

>
>>
>> I agree that the resume is sensitive to execute time but during resume
>> you can't schedule link startup work because the fs/block/scsi layer
>> expects the device availability as soon as resume operation is
>> completed. So ufshcd_link_startup() should be called in the resume
>> context itself or implement a synchronization mechanism like blocking
>> scsi layer queuing requests until link startup is completed.
> 'scsi_block_requests' and  'scsi_unblock_requests' can be used during suspend/resume.
> After the link startup is finished and host is ready, 'scsi_unblock_requests' will be called.
>
> Thanks,
> Seungwon Jeon
>>
>> Would following implementation looks better?
>>
>>
>> ufshcd_async_probe()
>> {
>> ...
>>     ufshcd_link_startup(hba);
>> ...
>> }
>>
>> ufshcd_resume()
>> {
>> ...
>> ufshcd_link_startup(hba);
>> ...
>> }
>>
>> ufshcd_init()
>> {
>> ...
>> async_schedule(ufshcd_async_probe, hba);
>> ...
>> }
>>
>>
>> --

-- 
Regards,
Sujit