Re: [PATCH V3 1/2] scsi: ufs: Add support for sending NOP OUT UPIU

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

>>>> + * ufshcd_wait_for_register - wait for register value to change
>>>> + * @hba - per-adapter interface
>>>> + * @reg - mmio register offset
>>>> + * @mask - mask to apply to read register value
>>>> + * @val - wait condition
>>>> + * @interval_us - polling interval in microsecs
>>>> + * @timeout_ms - timeout in millisecs
>>>> + *
>>>> + * Returns final register value after iteration
>>>> + */
>>>> +static u32 ufshcd_wait_for_register(struct ufs_hba *hba, u32 reg, u32 mask,
>>>> +		u32 val, unsigned long interval_us, unsigned long timeout_ms)
>>> I feel like this function's role is to wait for clearing register (specially, doorbells).
>>> If you really don't intend to apply other all register, I think it would better to change the
>> function name.
>>> ex> ufshcd_wait_for_clear_doorbell or ufshcd_wait_for_clear_reg?
>>
>> Although, this API is introduced in this patch and used only for
>> clearing the doorbell, I have intentionally made it generic to avoid
>> duplication of wait condition code in future (not just clearing but
>> also for setting a bit). For example, when we write to HCE.ENABLE we
>> wait for it turn to 1.
>>
>>
>>> And if you like it, it could be more simple like below
>>>
>>> static bool ufshcd_wait_for_clear_reg(struct ufs_hba *hba, u32 reg, u32 mask,
>>>                                            unsigned long interval_us,
>>>                                            unsigned int timeout_ms)
>>> {
>>>           unsigned long timeout = jiffies + msecs_to_jiffies(timeout_ms);
>>
>> Jiffies on 100Hz systems have minimum granularity of 10ms. So we really
>> wait for more 10ms even though the timeout_ms < 10ms.
> Yes. Real timeout depends on system.
> But normally actual wait will be maintained until 'ufshcd_readl' is done.
> Timeout is valid for failure case.  If we don't need to apply a strict timeout value, it's not bad.

Hmm.. make sense. Will take care in the next patchset.

>
>>
>>>           /* wakeup within 50us of expiry */
>>>           const unsigned int expiry = 50;
>>>
>>>           while (ufshcd_readl(hba, reg) & mask) {
>>>                   usleep_range(interval_us, interval_us + expiry);
>>>                   if (time_after(jiffies, timeout)) {
>>>                           if (ufshcd_readl(hba, reg) & mask)
>>>                                   return false;
>>
>> I really want the caller to decide on what to do after the timeout.
>> This helps in error handling cases where we try to clear off the entire
>> door-bell register but only a few of the bits are cleared after timeout.
> I don't know what we can do with the report of the partial success on clearing bit.
> It's just failure case. Isn't enough with false/true?

The point is, if a bit can't be cleared it can be regarded as a
permanent failure (only for that request), otherwise, it can be
retried with the same tag value.

>
>>
>>>                           else
>>>                                   return true;
>>>                   }
>>>           }
>>>
>>>           return true;
>>> }
>>>> +{
>>>> +	u32 tmp;
>>>> +	ktime_t start;
>>>> +	unsigned long diff;
>>>> +
>>>> +	tmp = ufshcd_readl(hba, reg);
>>>> +
>>>> +	if ((val & mask) != val) {
>>>> +		dev_err(hba->dev, "%s: Invalid wait condition 0x%x\n",
>>>> +				__func__, val);
>>>> +		goto out;
>>>> +	}
>>>> +
>>>> +	start = ktime_get();
>>>> +	while ((tmp & mask) != val) {
>>>> +		/* wakeup within 50us of expiry */
>>>> +		usleep_range(interval_us, interval_us + 50);
>>>> +		tmp = ufshcd_readl(hba, reg);
>>>> +		diff = ktime_to_ms(ktime_sub(ktime_get(), start));
>>>> +		if (diff > timeout_ms) {
>>>> +			tmp = ufshcd_readl(hba, reg);
>>>> +			break;
>>>> +		}
>>>> +	}
>>>> +out:
>>>> +	return tmp;
>>>> +}
>>>> +
..
>>>> +static int
>>>> +ufshcd_clear_cmd(struct ufs_hba *hba, int tag)
>>>> +{
>>>> +	int err = 0;
>>>> +	unsigned long flags;
>>>> +	u32 reg;
>>>> +	u32 mask = 1 << tag;
>>>> +
>>>> +	/* clear outstanding transaction before retry */
>>>> +	spin_lock_irqsave(hba->host->host_lock, flags);
>>>> +	ufshcd_utrl_clear(hba, tag);
>>>> +	spin_unlock_irqrestore(hba->host->host_lock, flags);
>>>> +
>>>> +	/*
>>>> +	 * wait for for h/w to clear corresponding bit in door-bell.
>>>> +	 * max. wait is 1 sec.
>>>> +	 */
>>>> +	reg = ufshcd_wait_for_register(hba,
>>>> +			REG_UTP_TRANSFER_REQ_DOOR_BELL,
>>>> +			mask, 0, 1000, 1000);
>>> 4th argument should be (~mask) instead of '0', right?
>>
>> True, but not really for this implementation. It breaks the check in
>> in wait_for_register -
>> if ((val & mask) != val)
>>              dev_err(...);
> Hmm, it seems complicated to use.
> Ok. Is right the following about val as 4th argument?
> - clear: val  should be '0' regardless corresponding bit.
> - set: val should be same with mask.
> If the related comment is added, it will be helpful.

Thinking again it looks like it is complicated. How about changing
the check to -

val = val & mask; /* ignore the bits we don't intend to wait on */
while (ufshcd_readl() & mask != val) {
  sleep
}

Usage will be ~mask for clearing the bits, mask for setting the bits
in the fourth argument.

>
>>
>>> Actually, mask value involves the corresponding bit to be cleared.
>>> So, 4th argument may be unnecessary.
>>
>> As I described above, the wait_for_register can also be used to
>> check if the value is set or not. In which case, we need 4th argument.
>>
>>>
>>>> +	if ((reg & mask) == mask)
>>>> +		err = -ETIMEDOUT;
>>> Also, checking the result can be involved in ufshcd_wait_for_register.
> Any comment?

Sorry I missed this. But the point was the same. To make
wait_for_register() just to wait a definite time and not return any
error condition when the bits don't turn as expected.

>
>>>
>>>> +
>>>> +	return err;
>>>> +}
>>>> +
>>>> +/**
>>>> + * ufshcd_dev_cmd_completion() - handles device management command responses
>>>> + * @hba: per adapter instance
>>>> + * @lrbp: pointer to local reference block
>>>> + */
>>>> +static int
>>>> +ufshcd_dev_cmd_completion(struct ufs_hba *hba, struct ufshcd_lrb *lrbp)
>>>> +{
>>>> +	int resp;
>>>> +	int err = 0;
>>>> +
>>>> +	resp = ufshcd_get_req_rsp(lrbp->ucd_rsp_ptr);
>>>> +
>>>> +	switch (resp) {
>>>> +	case UPIU_TRANSACTION_NOP_IN:
>>>> +		if (hba->dev_cmd.type != DEV_CMD_TYPE_NOP) {
>>>> +			err = -EINVAL;
>>>> +			dev_err(hba->dev, "%s: unexpected response %x\n",
>>>> +					__func__, resp);
>>>> +		}
>>>> +		break;
>>>> +	case UPIU_TRANSACTION_REJECT_UPIU:
>>>> +		/* TODO: handle Reject UPIU Response */
>>>> +		err = -EPERM;
>>>> +		dev_err(hba->dev, "%s: Reject UPIU not fully implemented\n",
>>>> +				__func__);
>>>> +		break;
>>>> +	default:
>>>> +		err = -EINVAL;
>>>> +		dev_err(hba->dev, "%s: Invalid device management cmd response: %x\n",
>>>> +				__func__, resp);
>>>> +		break;
>>>> +	}
>>>> +
>>>> +	return err;
>>>> +}
>>>> +
>>>> +/**
>>>> + * ufshcd_get_dev_cmd_tag - Get device management command tag
>>>> + * @hba: per-adapter instance
>>>> + * @tag: pointer to variable with available slot value
>>>> + *
>>>> + * Get a free slot and lock it until device management command
>>>> + * completes.
>>>> + *
>>>> + * Returns false if free slot is unavailable for locking, else
>>>> + * return true with tag value in @tag.
>>>> + */
>>>> +static bool ufshcd_get_dev_cmd_tag(struct ufs_hba *hba, int *tag_out)
>>>> +{
>>>> +	int tag;
>>>> +	bool ret = false;
>>>> +	unsigned long tmp;
>>>> +
>>>> +	if (!tag_out)
>>>> +		goto out;
>>>> +
>>>> +	do {
>>>> +		tmp = ~hba->lrb_in_use;
>>>> +		tag = find_last_bit(&tmp, hba->nutrs);
>>>> +		if (tag >= hba->nutrs)
>>>> +			goto out;
>>>> +	} while (test_and_set_bit_lock(tag, &hba->lrb_in_use));
>>>> +
>>>> +	*tag_out = tag;
>>>> +	ret = true;
>>>> +out:
>>>> +	return ret;
>>>> +}
>>>> +
>>>> +static inline void ufshcd_put_dev_cmd_tag(struct ufs_hba *hba, int tag)
>>>> +{
>>>> +	clear_bit_unlock(tag, &hba->lrb_in_use);
>>>> +}
>>>> +
>>>> +/**
>>>> + * ufshcd_exec_dev_cmd - API for sending device management requests
>>>> + * @hba - UFS hba
>>>> + * @cmd_type - specifies the type (NOP, Query...)
>>>> + * @timeout - time in seconds
>>>> + *
>>>> + * NOTE: There is only one available tag for device management commands. Thus
>>>> + * synchronisation is the responsibilty of the user.
>>>> + */
>>>> +static int ufshcd_exec_dev_cmd(struct ufs_hba *hba,
>>>> +		enum dev_cmd_type cmd_type, int timeout)
>>>> +{
>>>> +	struct ufshcd_lrb *lrbp;
>>>> +	int err;
>>>> +	int tag;
>>>> +	struct completion wait;
>>>> +	unsigned long flags;
>>>> +
>>>> +	/*
>>>> +	 * Get free slot, sleep if slots are unavailable.
>>>> +	 * Even though we use wait_event() which sleeps indefinitely,
>>>> +	 * the maximum wait time is bounded by SCSI request timeout.
>>>> +	 */
>>>> +	wait_event(hba->dev_cmd.tag_wq, ufshcd_get_dev_cmd_tag(hba, &tag));
>>>> +
>>>> +	init_completion(&wait);
>>>> +	lrbp = &hba->lrb[tag];
>>>> +	WARN_ON(lrbp->cmd);
>>>> +	err = ufshcd_compose_dev_cmd(hba, lrbp, cmd_type, tag);
>>>> +	if (unlikely(err))
>>>> +		goto out_put_tag;
>>>> +
>>>> +	hba->dev_cmd.complete = &wait;
>>>> +
>>>> +	spin_lock_irqsave(hba->host->host_lock, flags);
>>>> +	ufshcd_send_command(hba, tag);
>>>> +	spin_unlock_irqrestore(hba->host->host_lock, flags);
>>>> +
>>>> +	err = ufshcd_wait_for_dev_cmd(hba, lrbp, timeout);
>>>> +
>>> <snip>
>>>> +	if (err == -ETIMEDOUT) {
>>>> +		if (!ufshcd_clear_cmd(hba, tag))
>>>> +			err = -EAGAIN;
>>>> +	} else if (!err) {
>>>> +		spin_lock_irqsave(hba->host->host_lock, flags);
>>>> +		err = ufshcd_dev_cmd_completion(hba, lrbp);
>>>> +		spin_unlock_irqrestore(hba->host->host_lock, flags);
>>>> +	}
>>> </snip>
>>> I think sniped part can be involved in ufshcd_wait_for_dev_cmd.
>>> How do you think about that?
>>
>> Yes, it can be moved.
>>
>>>
>>>> +
>>>> +out_put_tag:
>>>> +	ufshcd_put_dev_cmd_tag(hba, tag);
>>>> +	wake_up(&hba->dev_cmd.tag_wq);
>>>> +	return err;
>>>> +}
>>>> +
>>>>    /**
>>>>     * ufshcd_memory_alloc - allocate memory for host memory space data structures
>>>>     * @hba: per adapter instance
>>>> @@ -774,8 +1075,8 @@ static void ufshcd_host_memory_configure(struct ufs_hba *hba)
>>>>    				cpu_to_le16(ALIGNED_UPIU_SIZE >> 2);
>>>>
>>>>    		hba->lrb[i].utr_descriptor_ptr = (utrdlp + i);
>>>> -		hba->lrb[i].ucd_cmd_ptr =
>>>> -			(struct utp_upiu_cmd *)(cmd_descp + i);
>>>> +		hba->lrb[i].ucd_req_ptr =
>>>> +			(struct utp_upiu_req *)(cmd_descp + i);
>>>>    		hba->lrb[i].ucd_rsp_ptr =
>>>>    			(struct utp_upiu_rsp *)cmd_descp[i].response_upiu;
>>>>    		hba->lrb[i].ucd_prdt_ptr =
>>>> @@ -961,6 +1262,38 @@ out:
>>>>    }
>>>>
>>>>    /**
>>>> + * ufshcd_validate_dev_connection() - Check device connection status
>>>> + * @hba: per-adapter instance
>>>> + *
>>>> + * Send NOP OUT UPIU and wait for NOP IN response to check whether the
>>>> + * device Transport Protocol (UTP) layer is ready after a reset.
>>>> + * If the UTP layer at the device side is not initialized, it may
>>>> + * not respond with NOP IN UPIU within timeout of %NOP_OUT_TIMEOUT
>>>> + * and we retry sending NOP OUT for %NOP_OUT_RETRIES iterations.
>>>> + */
>>>> +static int ufshcd_validate_dev_connection(struct ufs_hba *hba)
>>> I think ufshcd_verify_dev_init is close to standard description.
>>>
>>
>> Okay.
>>
>>>> +{
>>>> +	int err = 0;
>>>> +	int retries;
>>>> +
>>>> +	mutex_lock(&hba->dev_cmd.lock);
>>>> +	for (retries = NOP_OUT_RETRIES; retries > 0; retries--) {
>>>> +		err = ufshcd_exec_dev_cmd(hba, DEV_CMD_TYPE_NOP,
>>>> +					       NOP_OUT_TIMEOUT);
>>>> +
>>>> +		if (!err || err == -ETIMEDOUT)
>>>> +			break;
>>>> +
>>>> +		dev_dbg(hba->dev, "%s: error %d retrying\n", __func__, err);
>>>> +	}
>>>> +	mutex_unlock(&hba->dev_cmd.lock);
>>>> +
>>>> +	if (err)
>>>> +		dev_err(hba->dev, "%s: NOP OUT failed %d\n", __func__, err);
>>>> +	return err;
>>>> +}
>>>> +
>>>> +/**
>>>>     * ufshcd_do_reset - reset the host controller
>>>>     * @hba: per adapter instance
>>>>     *
>>>> @@ -986,13 +1319,20 @@ static int ufshcd_do_reset(struct ufs_hba *hba)
>>>>    	for (tag = 0; tag < hba->nutrs; tag++) {
>>>>    		if (test_bit(tag, &hba->outstanding_reqs)) {
>>>>    			lrbp = &hba->lrb[tag];
>>>> -			scsi_dma_unmap(lrbp->cmd);
>>>> -			lrbp->cmd->result = DID_RESET << 16;
>>>> -			lrbp->cmd->scsi_done(lrbp->cmd);
>>>> -			lrbp->cmd = NULL;
>>>> +			if (lrbp->cmd) {
>>>> +				scsi_dma_unmap(lrbp->cmd);
>>>> +				lrbp->cmd->result = DID_RESET << 16;
>>>> +				lrbp->cmd->scsi_done(lrbp->cmd);
>>>> +				lrbp->cmd = NULL;
>>>> +				clear_bit_unlock(tag, &hba->lrb_in_use);
>>>> +			}
>>> I know above.
>>> But there is no relation to this patch.
>>> Can be it moved to scsi: ufs: Fix device and host reset methods?
>>
>> Yes, but it is basically to avoid NULL pointer derefernce in case
>> someone picks this patch but not the other one.
>>
>>>
>>>>    		}
>>>>    	}
>>>>
>>>> +	/* complete device management command */
>>>> +	if (hba->dev_cmd.complete)
>>>> +		complete(hba->dev_cmd.complete);
>>>> +
>>>>    	/* clear outstanding request/task bit maps */
>>>>    	hba->outstanding_reqs = 0;
>>>>    	hba->outstanding_tasks = 0;
>>>> @@ -1199,27 +1539,37 @@ ufshcd_transfer_rsp_status(struct ufs_hba *hba, struct ufshcd_lrb *lrbp)
>>>>
>>>>    	switch (ocs) {
>>>>    	case OCS_SUCCESS:
>>>> -
>>>>    		/* check if the returned transfer response is valid */
>>> As replaced with new function, comment isn't valid.
>>> Remove or "get the TR response transaction type" seems proper.
>>
>> Okay.
>>
>>>
>>>> -		result = ufshcd_is_valid_req_rsp(lrbp->ucd_rsp_ptr);
>>>> -		if (result) {
>>>> +		result = ufshcd_get_req_rsp(lrbp->ucd_rsp_ptr);
>>>> +
>>>> +		switch (result) {
>>>> +		case UPIU_TRANSACTION_RESPONSE:
>>>> +			/*
>>>> +			 * get the response UPIU result to extract
>>>> +			 * the SCSI command status
>>>> +			 */
>>>> +			result = ufshcd_get_rsp_upiu_result(lrbp->ucd_rsp_ptr);
>>>> +
>>>> +			/*
>>>> +			 * get the result based on SCSI status response
>>>> +			 * to notify the SCSI midlayer of the command status
>>>> +			 */
>>>> +			scsi_status = result & MASK_SCSI_STATUS;
>>>> +			result = ufshcd_scsi_cmd_status(lrbp, scsi_status);
>>>> +			break;
>>>> +		case UPIU_TRANSACTION_REJECT_UPIU:
>>>> +			/* TODO: handle Reject UPIU Response */
>>>> +			result = DID_ERROR << 16;
>>>> +			dev_err(hba->dev,
>>>> +				"Reject UPIU not fully implemented\n");
>>>> +			break;
>>>> +		default:
>>>> +			result = DID_ERROR << 16;
>>>>    			dev_err(hba->dev,
>>>> -				"Invalid response = %x\n", result);
>>>> +				"Unexpected request response code = %x\n",
>>>> +				result);
>>>>    			break;
>>>>    		}
>>>> -
>>>> -		/*
>>>> -		 * get the response UPIU result to extract
>>>> -		 * the SCSI command status
>>>> -		 */
>>>> -		result = ufshcd_get_rsp_upiu_result(lrbp->ucd_rsp_ptr);
>>>> -
>>>> -		/*
>>>> -		 * get the result based on SCSI status response
>>>> -		 * to notify the SCSI midlayer of the command status
>>>> -		 */
>>>> -		scsi_status = result & MASK_SCSI_STATUS;
>>>> -		result = ufshcd_scsi_cmd_status(lrbp, scsi_status);
>>>>    		break;
>>>>    	case OCS_ABORTED:
>>>>    		result |= DID_ABORT << 16;
>>>> @@ -1259,28 +1609,37 @@ static void ufshcd_uic_cmd_compl(struct ufs_hba *hba)
>>>>     */
>>>>    static void ufshcd_transfer_req_compl(struct ufs_hba *hba)
>>>>    {
>>>> -	struct ufshcd_lrb *lrb;
>>>> +	struct ufshcd_lrb *lrbp;
>>>> +	struct scsi_cmnd *cmd;
>>>>    	unsigned long completed_reqs;
>>>>    	u32 tr_doorbell;
>>>>    	int result;
>>>>    	int index;
>>>> +	bool int_aggr_reset = false;
>>>>
>>>> -	lrb = hba->lrb;
>>>>    	tr_doorbell = ufshcd_readl(hba, REG_UTP_TRANSFER_REQ_DOOR_BELL);
>>>>    	completed_reqs = tr_doorbell ^ hba->outstanding_reqs;
>>>>
>>>>    	for (index = 0; index < hba->nutrs; index++) {
>>>>    		if (test_bit(index, &completed_reqs)) {
>>>> -
>>>> -			result = ufshcd_transfer_rsp_status(hba, &lrb[index]);
>>>> -
>>>> -			if (lrb[index].cmd) {
>>>> -				scsi_dma_unmap(lrb[index].cmd);
>>>> -				lrb[index].cmd->result = result;
>>>> -				lrb[index].cmd->scsi_done(lrb[index].cmd);
>>>> -
>>>> +			lrbp = &hba->lrb[index];
>>>> +			cmd = lrbp->cmd;
>>>> +			/* Don't reset counters for interrupt cmd */
>>>> +			int_aggr_reset |= !lrbp->intr_cmd;
>>>> +
>>>> +			if (cmd) {
>>>> +				result = ufshcd_transfer_rsp_status(hba, lrbp);
>>>> +				scsi_dma_unmap(cmd);
>>>> +				cmd->result = result;
>>>>    				/* Mark completed command as NULL in LRB */
>>>> -				lrb[index].cmd = NULL;
>>>> +				lrbp->cmd = NULL;
>>>> +				clear_bit_unlock(index, &hba->lrb_in_use);
>>>> +				/* Do not touch lrbp after scsi done */
>>>> +				cmd->scsi_done(cmd);
>>>> +			} else if (lrbp->command_type ==
>>>> +					UTP_CMD_TYPE_DEV_MANAGE) {
>>>> +				if (hba->dev_cmd.complete)
>>>> +					complete(hba->dev_cmd.complete);
>>>>    			}
>>>>    		} /* end of if */
>>>>    	} /* end of for */
>>>> @@ -1288,8 +1647,12 @@ static void ufshcd_transfer_req_compl(struct ufs_hba *hba)
>>>>    	/* clear corresponding bits of completed commands */
>>>>    	hba->outstanding_reqs ^= completed_reqs;
>>>>
>>>> +	/* we might have free'd some tags above */
>>>> +	wake_up(&hba->dev_cmd.tag_wq);
>>>> +
>>>>    	/* Reset interrupt aggregation counters */
>>>> -	ufshcd_config_int_aggr(hba, INT_AGGR_RESET);
>>>> +	if (int_aggr_reset)
>>>> +		ufshcd_config_int_aggr(hba, INT_AGGR_RESET);
>>> Do you assume that interrupt command(device management command) is completed alone?
>>> Of course, interrupt command is not counted in aggregation unlike regular command.
>>> We need to consider that interrupt command comes along with regular command?
>>> If right, ufshcd_config_int_aggr should not be skipped.
>>
>> True. We are not skipping it. int_aggr_reset will be false only when
>> there is single interrupt command completed.
>>
>> Check the above part which reads -
>> for_all_completed_commands() {
>>    int_aggr_reset |= !lrbp->intr_cmd;
>> }
>>
>> Even if one of the command in the iteration is not interrupt command
>> (lrbp->intr_cmd is false) then int_aggr_reset is always true.
> Clear!
> But I confused it with your comment line. (/* Don't reset counters for interrupt cmd */)
> How about changing like below?
> 'Don't skip to reset counters if a regular command is present'

Makes sense.

-- 
Regards,
Sujit