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

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

On 7/10/2013 6:58 PM, Seungwon Jeon wrote:
> On Tue, July 09, 2013, Sujit Reddy Thumma wrote:
>> As part of device initialization sequence, sending NOP OUT UPIU and
>> waiting for NOP IN UPIU response is mandatory. This confirms that the
>> device UFS Transport (UTP) layer is functional and the host can configure
>> the device with further commands. Add support for sending NOP OUT UPIU to
>> check the device connection path and test whether the UTP layer on the
>> device side is functional during initialization.
>>
>> A tag is acquired from the SCSI tag map space in order to send the device
>> management command. When the tag is acquired by internal command the scsi
>> command is rejected with host busy flag in order to requeue the request.
>> To avoid frequent collisions between internal commands and scsi commands
>> the device management command tag is allocated in the opposite direction
>> w.r.t block layer tag allocation.
>>
>> Signed-off-by: Sujit Reddy Thumma <sthumma@codeaurora.org>
>> Signed-off-by: Dolev Raviv <draviv@codeaurora.org>
>> ---
>>   drivers/scsi/ufs/ufs.h    |   43 +++-
>>   drivers/scsi/ufs/ufshcd.c |  596 +++++++++++++++++++++++++++++++++++++--------
>>   drivers/scsi/ufs/ufshcd.h |   29 ++-
>>   3 files changed, 552 insertions(+), 116 deletions(-)
>>
>> diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h
>> index 139bc06..14c0a4e 100644
>> --- a/drivers/scsi/ufs/ufs.h
>> +++ b/drivers/scsi/ufs/ufs.h
>> @@ -36,10 +36,16 @@
>>   #ifndef _UFS_H
>>   #define _UFS_H
>>
>> +#include <linux/mutex.h>
>> +#include <linux/types.h>
>> +
>>   #define MAX_CDB_SIZE	16
>> +#define GENERAL_UPIU_REQUEST_SIZE 32
>> +#define UPIU_HEADER_DATA_SEGMENT_MAX_SIZE	((ALIGNED_UPIU_SIZE) - \
>> +					(GENERAL_UPIU_REQUEST_SIZE))
>>
>>   #define UPIU_HEADER_DWORD(byte3, byte2, byte1, byte0)\
>> -			((byte3 << 24) | (byte2 << 16) |\
>> +			cpu_to_be32((byte3 << 24) | (byte2 << 16) |\
>>   			 (byte1 << 8) | (byte0))
>>
>>   /*
>> @@ -73,6 +79,7 @@ enum {
>>   	UPIU_TRANSACTION_TASK_RSP	= 0x24,
>>   	UPIU_TRANSACTION_READY_XFER	= 0x31,
>>   	UPIU_TRANSACTION_QUERY_RSP	= 0x36,
>> +	UPIU_TRANSACTION_REJECT_UPIU	= 0x3F,
>>   };
>>
>>   /* UPIU Read/Write flags */
>> @@ -110,6 +117,12 @@ enum {
>>   	UPIU_COMMAND_SET_TYPE_QUERY	= 0x2,
>>   };
>>
>> +/* UTP Transfer Request Command Offset */
>> +#define UPIU_COMMAND_TYPE_OFFSET	28
>> +
>> +/* Offset of the response code in the UPIU header */
>> +#define UPIU_RSP_CODE_OFFSET		8
>> +
>>   enum {
>>   	MASK_SCSI_STATUS	= 0xFF,
>>   	MASK_TASK_RESPONSE	= 0xFF00,
>> @@ -138,26 +151,32 @@ struct utp_upiu_header {
>>
>>   /**
>>    * struct utp_upiu_cmd - Command UPIU structure
>> - * @header: UPIU header structure DW-0 to DW-2
>>    * @data_transfer_len: Data Transfer Length DW-3
>>    * @cdb: Command Descriptor Block CDB DW-4 to DW-7
>>    */
>>   struct utp_upiu_cmd {
>> -	struct utp_upiu_header header;
>>   	u32 exp_data_transfer_len;
>>   	u8 cdb[MAX_CDB_SIZE];
>>   };
>>
>>   /**
>> - * struct utp_upiu_rsp - Response UPIU structure
>> - * @header: UPIU header DW-0 to DW-2
>> + * struct utp_upiu_req - general upiu request structure
>> + * @header:UPIU header structure DW-0 to DW-2
>> + * @sc: fields structure for scsi command DW-3 to DW-7
>> + */
>> +struct utp_upiu_req {
>> +	struct utp_upiu_header header;
>> +	struct utp_upiu_cmd sc;
>> +};
>> +
>> +/**
>> + * struct utp_cmd_rsp - Response UPIU structure
>>    * @residual_transfer_count: Residual transfer count DW-3
>>    * @reserved: Reserved double words DW-4 to DW-7
>>    * @sense_data_len: Sense data length DW-8 U16
>>    * @sense_data: Sense data field DW-8 to DW-12
>>    */
>> -struct utp_upiu_rsp {
>> -	struct utp_upiu_header header;
>> +struct utp_cmd_rsp {
>>   	u32 residual_transfer_count;
>>   	u32 reserved[4];
>>   	u16 sense_data_len;
>> @@ -165,6 +184,16 @@ struct utp_upiu_rsp {
>>   };
>>
>>   /**
>> + * struct utp_upiu_rsp - general upiu response structure
>> + * @header: UPIU header structure DW-0 to DW-2
>> + * @sr: fields structure for scsi command DW-3 to DW-12
>> + */
>> +struct utp_upiu_rsp {
>> +	struct utp_upiu_header header;
>> +	struct utp_cmd_rsp sr;
>> +};
>> +
>> +/**
>>    * struct utp_upiu_task_req - Task request UPIU structure
>>    * @header - UPIU header structure DW0 to DW-2
>>    * @input_param1: Input parameter 1 DW-3
>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>> index b743bd6..3f482b6 100644
>> --- a/drivers/scsi/ufs/ufshcd.c
>> +++ b/drivers/scsi/ufs/ufshcd.c
>> @@ -43,6 +43,11 @@
>>   /* UIC command timeout, unit: ms */
>>   #define UIC_CMD_TIMEOUT	500
>>
>> +/* NOP OUT retries waiting for NOP IN response */
>> +#define NOP_OUT_RETRIES    10
>> +/* Timeout after 30 msecs if NOP OUT hangs without response */
>> +#define NOP_OUT_TIMEOUT    30 /* msecs */
>> +
>>   enum {
>>   	UFSHCD_MAX_CHANNEL	= 0,
>>   	UFSHCD_MAX_ID		= 1,
>> @@ -71,6 +76,47 @@ enum {
>>   	INT_AGGR_CONFIG,
>>   };
>>
>> +/*
>> + * 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.

>          /* 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.

>                          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;
>> +}
>> +
>>   /**
>>    * ufshcd_get_intr_mask - Get the interrupt bit mask
>>    * @hba - Pointer to adapter instance
>> @@ -191,18 +237,13 @@ static inline int ufshcd_get_uic_cmd_result(struct ufs_hba *hba)
>>   }
>>
>>   /**
>> - * ufshcd_is_valid_req_rsp - checks if controller TR response is valid
>> + * ufshcd_get_req_rsp - returns the TR response transaction type
>>    * @ucd_rsp_ptr: pointer to response UPIU
>> - *
>> - * This function checks the response UPIU for valid transaction type in
>> - * response field
>> - * Returns 0 on success, non-zero on failure
>>    */
>>   static inline int
>> -ufshcd_is_valid_req_rsp(struct utp_upiu_rsp *ucd_rsp_ptr)
>> +ufshcd_get_req_rsp(struct utp_upiu_rsp *ucd_rsp_ptr)
>>   {
>> -	return ((be32_to_cpu(ucd_rsp_ptr->header.dword_0) >> 24) ==
>> -		 UPIU_TRANSACTION_RESPONSE) ? 0 : DID_ERROR << 16;
>> +	return be32_to_cpu(ucd_rsp_ptr->header.dword_0) >> 24;
>>   }
>>
>>   /**
>> @@ -299,9 +340,9 @@ static inline void ufshcd_copy_sense_data(struct ufshcd_lrb *lrbp)
>>   {
>>   	int len;
>>   	if (lrbp->sense_buffer) {
>> -		len = be16_to_cpu(lrbp->ucd_rsp_ptr->sense_data_len);
>> +		len = be16_to_cpu(lrbp->ucd_rsp_ptr->sr.sense_data_len);
>>   		memcpy(lrbp->sense_buffer,
>> -			lrbp->ucd_rsp_ptr->sense_data,
>> +			lrbp->ucd_rsp_ptr->sr.sense_data,
>>   			min_t(int, len, SCSI_SENSE_BUFFERSIZE));
>>   	}
>>   }
>> @@ -519,76 +560,128 @@ static void ufshcd_disable_intr(struct ufs_hba *hba, u32 intrs)
>>   }
>>
>>   /**
>> + * ufshcd_prepare_req_desc_hdr() - Fills the requests header
>> + * descriptor according to request
>> + * @lrbp: pointer to local reference block
>> + * @upiu_flags: flags required in the header
>> + * @cmd_dir: requests data direction
>> + */
>> +static void ufshcd_prepare_req_desc_hdr(struct ufshcd_lrb *lrbp,
>> +		u32 *upiu_flags, enum dma_data_direction cmd_dir)
>> +{
>> +	struct utp_transfer_req_desc *req_desc = lrbp->utr_descriptor_ptr;
>> +	u32 data_direction;
>> +	u32 dword_0;
>> +
>> +	if (cmd_dir == DMA_FROM_DEVICE) {
>> +		data_direction = UTP_DEVICE_TO_HOST;
>> +		*upiu_flags = UPIU_CMD_FLAGS_READ;
>> +	} else if (cmd_dir == DMA_TO_DEVICE) {
>> +		data_direction = UTP_HOST_TO_DEVICE;
>> +		*upiu_flags = UPIU_CMD_FLAGS_WRITE;
>> +	} else {
>> +		data_direction = UTP_NO_DATA_TRANSFER;
>> +		*upiu_flags = UPIU_CMD_FLAGS_NONE;
>> +	}
>> +
>> +	dword_0 = data_direction | (lrbp->command_type
>> +				<< UPIU_COMMAND_TYPE_OFFSET);
>> +	if (lrbp->intr_cmd)
>> +		dword_0 |= UTP_REQ_DESC_INT_CMD;
>> +
>> +	/* Transfer request descriptor header fields */
>> +	req_desc->header.dword_0 = cpu_to_le32(dword_0);
>> +
>> +	/*
>> +	 * assigning invalid value for command status. Controller
>> +	 * updates OCS on command completion, with the command
>> +	 * status
>> +	 */
>> +	req_desc->header.dword_2 =
>> +		cpu_to_le32(OCS_INVALID_COMMAND_STATUS);
>> +}
>> +
>> +/**
>> + * ufshcd_prepare_utp_scsi_cmd_upiu() - fills the utp_transfer_req_desc,
>> + * for scsi commands
>> + * @lrbp - local reference block pointer
>> + * @upiu_flags - flags
>> + */
>> +static
>> +void ufshcd_prepare_utp_scsi_cmd_upiu(struct ufshcd_lrb *lrbp, u32 upiu_flags)
>> +{
>> +	struct utp_upiu_req *ucd_req_ptr = lrbp->ucd_req_ptr;
>> +
>> +	/* command descriptor fields */
>> +	ucd_req_ptr->header.dword_0 = UPIU_HEADER_DWORD(
>> +				UPIU_TRANSACTION_COMMAND, upiu_flags,
>> +				lrbp->lun, lrbp->task_tag);
>> +	ucd_req_ptr->header.dword_1 = UPIU_HEADER_DWORD(
>> +				UPIU_COMMAND_SET_TYPE_SCSI, 0, 0, 0);
>> +
>> +	/* Total EHS length and Data segment length will be zero */
>> +	ucd_req_ptr->header.dword_2 = 0;
>> +
>> +	ucd_req_ptr->sc.exp_data_transfer_len =
>> +		cpu_to_be32(lrbp->cmd->sdb.length);
>> +
>> +	memcpy(ucd_req_ptr->sc.cdb, lrbp->cmd->cmnd,
>> +		(min_t(unsigned short, lrbp->cmd->cmd_len, MAX_CDB_SIZE)));
>> +}
>> +
>> +static inline void ufshcd_prepare_utp_nop_upiu(struct ufshcd_lrb *lrbp)
>> +{
>> +	struct utp_upiu_req *ucd_req_ptr = lrbp->ucd_req_ptr;
>> +
>> +	memset(ucd_req_ptr, 0, sizeof(struct utp_upiu_req));
>> +
>> +	/* command descriptor fields */
>> +	ucd_req_ptr->header.dword_0 =
>> +		UPIU_HEADER_DWORD(
>> +			UPIU_TRANSACTION_NOP_OUT, 0, 0, lrbp->task_tag);
>> +}
>> +
>> +/**
>>    * ufshcd_compose_upiu - form UFS Protocol Information Unit(UPIU)
>> + * @hba - per adapter instance
>>    * @lrb - pointer to local reference block
>>    */
>> -static void ufshcd_compose_upiu(struct ufshcd_lrb *lrbp)
>> +static int ufshcd_compose_upiu(struct ufs_hba *hba, struct ufshcd_lrb *lrbp)
>>   {
>> -	struct utp_transfer_req_desc *req_desc;
>> -	struct utp_upiu_cmd *ucd_cmd_ptr;
>> -	u32 data_direction;
>>   	u32 upiu_flags;
>> -
>> -	ucd_cmd_ptr = lrbp->ucd_cmd_ptr;
>> -	req_desc = lrbp->utr_descriptor_ptr;
>> +	int ret = 0;
>>
>>   	switch (lrbp->command_type) {
>>   	case UTP_CMD_TYPE_SCSI:
>> -		if (lrbp->cmd->sc_data_direction == DMA_FROM_DEVICE) {
>> -			data_direction = UTP_DEVICE_TO_HOST;
>> -			upiu_flags = UPIU_CMD_FLAGS_READ;
>> -		} else if (lrbp->cmd->sc_data_direction == DMA_TO_DEVICE) {
>> -			data_direction = UTP_HOST_TO_DEVICE;
>> -			upiu_flags = UPIU_CMD_FLAGS_WRITE;
>> +		if (likely(lrbp->cmd)) {
>> +			ufshcd_prepare_req_desc_hdr(lrbp, &upiu_flags,
>> +					lrbp->cmd->sc_data_direction);
>> +			ufshcd_prepare_utp_scsi_cmd_upiu(lrbp, upiu_flags);
>>   		} else {
>> -			data_direction = UTP_NO_DATA_TRANSFER;
>> -			upiu_flags = UPIU_CMD_FLAGS_NONE;
>> +			ret = -EINVAL;
>>   		}
>> -
>> -		/* Transfer request descriptor header fields */
>> -		req_desc->header.dword_0 =
>> -			cpu_to_le32(data_direction | UTP_SCSI_COMMAND);
>> -
>> -		/*
>> -		 * assigning invalid value for command status. Controller
>> -		 * updates OCS on command completion, with the command
>> -		 * status
>> -		 */
>> -		req_desc->header.dword_2 =
>> -			cpu_to_le32(OCS_INVALID_COMMAND_STATUS);
>> -
>> -		/* command descriptor fields */
>> -		ucd_cmd_ptr->header.dword_0 =
>> -			cpu_to_be32(UPIU_HEADER_DWORD(UPIU_TRANSACTION_COMMAND,
>> -						      upiu_flags,
>> -						      lrbp->lun,
>> -						      lrbp->task_tag));
>> -		ucd_cmd_ptr->header.dword_1 =
>> -			cpu_to_be32(
>> -				UPIU_HEADER_DWORD(UPIU_COMMAND_SET_TYPE_SCSI,
>> -						  0,
>> -						  0,
>> -						  0));
>> -
>> -		/* Total EHS length and Data segment length will be zero */
>> -		ucd_cmd_ptr->header.dword_2 = 0;
>> -
>> -		ucd_cmd_ptr->exp_data_transfer_len =
>> -			cpu_to_be32(lrbp->cmd->sdb.length);
>> -
>> -		memcpy(ucd_cmd_ptr->cdb,
>> -		       lrbp->cmd->cmnd,
>> -		       (min_t(unsigned short,
>> -			      lrbp->cmd->cmd_len,
>> -			      MAX_CDB_SIZE)));
>>   		break;
>>   	case UTP_CMD_TYPE_DEV_MANAGE:
>> -		/* For query function implementation */
>> +		ufshcd_prepare_req_desc_hdr(lrbp, &upiu_flags, DMA_NONE);
>> +		if (hba->dev_cmd.type == DEV_CMD_TYPE_NOP)
>> +			ufshcd_prepare_utp_nop_upiu(lrbp);
>> +		else
>> +			ret = -EINVAL;
>>   		break;
>>   	case UTP_CMD_TYPE_UFS:
>>   		/* For UFS native command implementation */
>> +		ret = -ENOTSUPP;
>> +		dev_err(hba->dev, "%s: UFS native command are not supported\n",
>> +			__func__);
>> +		break;
>> +	default:
>> +		ret = -ENOTSUPP;
>> +		dev_err(hba->dev, "%s: unknown command type: 0x%x\n",
>> +				__func__, lrbp->command_type);
>>   		break;
>>   	} /* end of switch */
>> +
>> +	return ret;
>>   }
>>
>>   /**
>> @@ -615,21 +708,37 @@ static int ufshcd_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *cmd)
>>   		goto out;
>>   	}
>>
>> +	/* acquire the tag to make sure device cmds don't use it */
>> +	if (test_and_set_bit_lock(tag, &hba->lrb_in_use)) {
>> +		/*
>> +		 * Dev manage command in progress, requeue the command.
>> +		 * Requeuing the command helps in cases where the request *may*
>> +		 * find different tag instead of waiting for dev manage command
>> +		 * completion.
>> +		 */
>> +		err = SCSI_MLQUEUE_HOST_BUSY;
>> +		goto out;
>> +	}
>> +
>>   	lrbp = &hba->lrb[tag];
>>
>> +	WARN_ON(lrbp->cmd);
>>   	lrbp->cmd = cmd;
>>   	lrbp->sense_bufflen = SCSI_SENSE_BUFFERSIZE;
>>   	lrbp->sense_buffer = cmd->sense_buffer;
>>   	lrbp->task_tag = tag;
>>   	lrbp->lun = cmd->device->lun;
>> -
>> +	lrbp->intr_cmd = false;
>>   	lrbp->command_type = UTP_CMD_TYPE_SCSI;
>>
>>   	/* form UPIU before issuing the command */
>> -	ufshcd_compose_upiu(lrbp);
>> +	ufshcd_compose_upiu(hba, lrbp);
>>   	err = ufshcd_map_sg(lrbp);
>> -	if (err)
>> +	if (err) {
>> +		lrbp->cmd = NULL;
>> +		clear_bit_unlock(tag, &hba->lrb_in_use);
>>   		goto out;
>> +	}
>>
>>   	/* issue command to the controller */
>>   	spin_lock_irqsave(hba->host->host_lock, flags);
>> @@ -639,6 +748,198 @@ out:
>>   	return err;
>>   }
>>
>> +static int ufshcd_compose_dev_cmd(struct ufs_hba *hba,
>> +		struct ufshcd_lrb *lrbp, enum dev_cmd_type cmd_type, int tag)
>> +{
>> +	lrbp->cmd = NULL;
>> +	lrbp->sense_bufflen = 0;
>> +	lrbp->sense_buffer = NULL;
>> +	lrbp->task_tag = tag;
>> +	lrbp->lun = 0; /* device management cmd is not specific to any LUN */
>> +	lrbp->command_type = UTP_CMD_TYPE_DEV_MANAGE;
>> +	lrbp->intr_cmd = true; /* No interrupt aggregation */
>> +	hba->dev_cmd.type = cmd_type;
>> +
>> +	return ufshcd_compose_upiu(hba, lrbp);
>> +}
>> +
>> +static int ufshcd_wait_for_dev_cmd(struct ufs_hba *hba,
>> +		struct ufshcd_lrb *lrbp, int max_timeout)
>> +{
>> +	int err = 0;
>> +	unsigned long timeout;
>> +	unsigned long flags;
>> +
>> +	timeout = wait_for_completion_timeout(hba->dev_cmd.complete,
>> +			msecs_to_jiffies(max_timeout));
>> +
>> +	spin_lock_irqsave(hba->host->host_lock, flags);
>> +	hba->dev_cmd.complete = NULL;
>> +	if (timeout)
>> +		err = ufshcd_get_tr_ocs(lrbp);
>> +	else
>> +		err = -ETIMEDOUT;
>> +	spin_unlock_irqrestore(hba->host->host_lock, flags);
>> +
>> +	return err;
>> +}
>> +
>> +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(...);

> 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.
> 
>> +
>> +	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.

> 
> Thanks,
> Seungwon Jeon
> 
>>   }
>>
>>   /**
>> @@ -1432,10 +1795,10 @@ ufshcd_issue_tm_cmd(struct ufs_hba *hba,
>>   	task_req_upiup =
>>   		(struct utp_upiu_task_req *) task_req_descp->task_req_upiu;
>>   	task_req_upiup->header.dword_0 =
>> -		cpu_to_be32(UPIU_HEADER_DWORD(UPIU_TRANSACTION_TASK_REQ, 0,
>> -					      lrbp->lun, lrbp->task_tag));
>> +		UPIU_HEADER_DWORD(UPIU_TRANSACTION_TASK_REQ, 0,
>> +					      lrbp->lun, lrbp->task_tag);
>>   	task_req_upiup->header.dword_1 =
>> -	cpu_to_be32(UPIU_HEADER_DWORD(0, tm_function, 0, 0));
>> +		UPIU_HEADER_DWORD(0, tm_function, 0, 0);
>>
>>   	task_req_upiup->input_param1 = lrbp->lun;
>>   	task_req_upiup->input_param1 =
>> @@ -1502,9 +1865,11 @@ static int ufshcd_device_reset(struct scsi_cmnd *cmd)
>>   			if (hba->lrb[pos].cmd) {
>>   				scsi_dma_unmap(hba->lrb[pos].cmd);
>>   				hba->lrb[pos].cmd->result =
>> -						DID_ABORT << 16;
>> +					DID_ABORT << 16;
>>   				hba->lrb[pos].cmd->scsi_done(cmd);
>>   				hba->lrb[pos].cmd = NULL;
>> +				clear_bit_unlock(pos, &hba->lrb_in_use);
>> +				wake_up(&hba->dev_cmd.tag_wq);
>>   			}
>>   		}
>>   	} /* end of for */
>> @@ -1572,6 +1937,9 @@ static int ufshcd_abort(struct scsi_cmnd *cmd)
>>   	__clear_bit(tag, &hba->outstanding_reqs);
>>   	hba->lrb[tag].cmd = NULL;
>>   	spin_unlock_irqrestore(host->host_lock, flags);
>> +
>> +	clear_bit_unlock(tag, &hba->lrb_in_use);
>> +	wake_up(&hba->dev_cmd.tag_wq);
>>   out:
>>   	return err;
>>   }
>> @@ -1587,8 +1955,16 @@ static void ufshcd_async_scan(void *data, async_cookie_t cookie)
>>   	int ret;
>>
>>   	ret = ufshcd_link_startup(hba);
>> -	if (!ret)
>> -		scsi_scan_host(hba->host);
>> +	if (ret)
>> +		goto out;
>> +
>> +	ret = ufshcd_validate_dev_connection(hba);
>> +	if (ret)
>> +		goto out;
>> +
>> +	scsi_scan_host(hba->host);
>> +out:
>> +	return;
>>   }
>>
>>   static struct scsi_host_template ufshcd_driver_template = {
>> @@ -1744,6 +2120,12 @@ int ufshcd_init(struct device *dev, struct ufs_hba **hba_handle,
>>   	/* Initialize UIC command mutex */
>>   	mutex_init(&hba->uic_cmd_mutex);
>>
>> +	/* Initialize mutex for device management commands */
>> +	mutex_init(&hba->dev_cmd.lock);
>> +
>> +	/* Initialize device management tag acquire wait queue */
>> +	init_waitqueue_head(&hba->dev_cmd.tag_wq);
>> +
>>   	/* IRQ registration */
>>   	err = devm_request_irq(dev, irq, ufshcd_intr, IRQF_SHARED, UFSHCD, hba);
>>   	if (err) {
>> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
>> index 49590ee..c750a90 100644
>> --- a/drivers/scsi/ufs/ufshcd.h
>> +++ b/drivers/scsi/ufs/ufshcd.h
>> @@ -68,6 +68,10 @@
>>   #define UFSHCD "ufshcd"
>>   #define UFSHCD_DRIVER_VERSION "0.2"
>>
>> +enum dev_cmd_type {
>> +	DEV_CMD_TYPE_NOP		= 0x0,
>> +};
>> +
>>   /**
>>    * struct uic_command - UIC command structure
>>    * @command: UIC command
>> @@ -91,7 +95,7 @@ struct uic_command {
>>   /**
>>    * struct ufshcd_lrb - local reference block
>>    * @utr_descriptor_ptr: UTRD address of the command
>> - * @ucd_cmd_ptr: UCD address of the command
>> + * @ucd_req_ptr: UCD address of the command
>>    * @ucd_rsp_ptr: Response UPIU address for this command
>>    * @ucd_prdt_ptr: PRDT address of the command
>>    * @cmd: pointer to SCSI command
>> @@ -101,10 +105,11 @@ struct uic_command {
>>    * @command_type: SCSI, UFS, Query.
>>    * @task_tag: Task tag of the command
>>    * @lun: LUN of the command
>> + * @intr_cmd: Interrupt command (doesn't participate in interrupt aggregation)
>>    */
>>   struct ufshcd_lrb {
>>   	struct utp_transfer_req_desc *utr_descriptor_ptr;
>> -	struct utp_upiu_cmd *ucd_cmd_ptr;
>> +	struct utp_upiu_req *ucd_req_ptr;
>>   	struct utp_upiu_rsp *ucd_rsp_ptr;
>>   	struct ufshcd_sg_entry *ucd_prdt_ptr;
>>
>> @@ -116,8 +121,22 @@ struct ufshcd_lrb {
>>   	int command_type;
>>   	int task_tag;
>>   	unsigned int lun;
>> +	bool intr_cmd;
>>   };
>>
>> +/**
>> + * struct ufs_dev_cmd - all assosiated fields with device management commands
>> + * @type: device management command type - Query, NOP OUT
>> + * @lock: lock to allow one command at a time
>> + * @complete: internal commands completion
>> + * @tag_wq: wait queue until free command slot is available
>> + */
>> +struct ufs_dev_cmd {
>> +	enum dev_cmd_type type;
>> +	struct mutex lock;
>> +	struct completion *complete;
>> +	wait_queue_head_t tag_wq;
>> +};
>>
>>   /**
>>    * struct ufs_hba - per adapter private structure
>> @@ -131,6 +150,7 @@ struct ufshcd_lrb {
>>    * @host: Scsi_Host instance of the driver
>>    * @dev: device handle
>>    * @lrb: local reference block
>> + * @lrb_in_use: lrb in use
>>    * @outstanding_tasks: Bits representing outstanding task requests
>>    * @outstanding_reqs: Bits representing outstanding transfer requests
>>    * @capabilities: UFS Controller Capabilities
>> @@ -146,6 +166,7 @@ struct ufshcd_lrb {
>>    * @intr_mask: Interrupt Mask Bits
>>    * @feh_workq: Work queue for fatal controller error handling
>>    * @errors: HBA errors
>> + * @dev_cmd: ufs device management command information
>>    */
>>   struct ufs_hba {
>>   	void __iomem *mmio_base;
>> @@ -164,6 +185,7 @@ struct ufs_hba {
>>   	struct device *dev;
>>
>>   	struct ufshcd_lrb *lrb;
>> +	unsigned long lrb_in_use;
>>
>>   	unsigned long outstanding_tasks;
>>   	unsigned long outstanding_reqs;
>> @@ -188,6 +210,9 @@ struct ufs_hba {
>>
>>   	/* HBA Errors */
>>   	u32 errors;
>> +
>> +	/* Device management request data */
>> +	struct ufs_dev_cmd dev_cmd;
>>   };
>>
>>   #define ufshcd_writel(hba, val, reg)	\
>> --
>> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
>> of Code Aurora Forum, hosted by The Linux Foundation.
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

-- 
Regards,
Sujit