Re: [PATCH v2] ufs: adjust queue settings to PRDT limitations

[Subhash Jadavani]

Looks good to me.

Reviewed-by: Subhash Jadavani 

On 9/25/2013 7:17 PM, Akinobu Mita wrote:

The data byte count field of PRDT indicates the length of data block
which is a segment of data transfer for SCSI commands.  The value of
this field shall have Dword granularity and the the maximum of length
is 256KB.

This adjusts dma pad mask and max segment size to the above-mentioned
PRDT limitations.

Signed-off-by: Akinobu Mita 
Cc: Subhash Jadavani 
Cc: Vinayak Holikatti 
Cc: Santosh Y 
Cc: "James E.J. Bottomley" 
Cc: linux-scsi@vger.kernel.org
---
* Change from v1
- Add comments for PRDT limitations as suggested by Subhash Jadavani

  drivers/scsi/ufs/ufshcd.c | 15 +++
  drivers/scsi/ufs/ufshci.h |  5 +
  2 files changed, 20 insertions(+)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index a2abe9a..660792e 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -1918,6 +1918,20 @@ static int ufshcd_slave_alloc(struct scsi_device *sdev)
  }
  
  /**

+ * ufshcd_slave_configure - adjust SCSI device configurations
+ * @sdev: pointer to SCSI device
+ */
+static int ufshcd_slave_configure(struct scsi_device *sdev)
+{
+   struct request_queue *q = sdev->request_queue;
+
+   blk_queue_update_dma_pad(q, PRDT_DATA_BYTE_COUNT_PAD - 1);
+   blk_queue_max_segment_size(q, PRDT_DATA_BYTE_COUNT_MAX);
+
+   return 0;
+}
+
+/**
   * ufshcd_slave_destroy - remove SCSI device configurations
   * @sdev: pointer to SCSI device
   */
@@ -2748,6 +2762,7 @@ static struct scsi_host_template ufshcd_driver_template = 
{
.proc_name  = UFSHCD,
.queuecommand   = ufshcd_queuecommand,
.slave_alloc= ufshcd_slave_alloc,
+   .slave_configure= ufshcd_slave_configure,
.slave_destroy  = ufshcd_slave_destroy,
.eh_abort_handler   = ufshcd_abort,
.eh_device_reset_handler = ufshcd_device_reset,
diff --git a/drivers/scsi/ufs/ufshci.h b/drivers/scsi/ufs/ufshci.h
index 0475c66..b2b7004 100644
--- a/drivers/scsi/ufs/ufshci.h
+++ b/drivers/scsi/ufs/ufshci.h
@@ -296,6 +296,11 @@ enum {
MASK_OCS= 0x0F,
  };
  
+/* The maximum length of the data byte count field in the PRDT is 256KB */

+#define PRDT_DATA_BYTE_COUNT_MAX   (256 * 1024)
+/* The granularity of the data byte count field in the PRDT is 32-bit */
+#define PRDT_DATA_BYTE_COUNT_PAD   4
+
  /**
   * struct ufshcd_sg_entry - UFSHCI PRD Entry
   * @base_addr: Lower 32bit physical address DW-0


--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V2 06/10] pm80xx: Print SAS address of IO failed device.

[Jack Wang]

On 09/26/2013 07:37 AM, Anand wrote:
> From 5ddec5ef3033b4fee08dcdc7ba8b434425453e9d Mon Sep 17 00:00:00 2001
> From: Anand Kumar Santhanam 
> Date: Tue, 17 Sep 2013 16:47:21 +0530
> Subject: [PATCH V2 06/10] pm80xx: Print SAS address of IO failed device.
> 
> Signed-off-by: anandkumar.santha...@pmcs.com
> 
> ---
>  drivers/scsi/pm8001/pm8001_hwi.c |   65 
> ++
>  drivers/scsi/pm8001/pm80xx_hwi.c |   65 
> ++
>  2 files changed, 130 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/scsi/pm8001/pm8001_hwi.c 
> b/drivers/scsi/pm8001/pm8001_hwi.c
> index 2fd8c38..c9cc744 100644
> --- a/drivers/scsi/pm8001/pm8001_hwi.c
> +++ b/drivers/scsi/pm8001/pm8001_hwi.c
> @@ -1868,6 +1868,13 @@ mpi_ssp_completion(struct pm8001_hba_info *pm8001_ha , 
> void *piomb)
>   if (unlikely(!t || !t->lldd_task || !t->dev))
>   return;
>   ts = &t->task_status;
> + /* Print sas address of IO failed device */
> + if ((status != IO_SUCCESS) && (status != IO_OVERFLOW) &&
> + (status != IO_UNDERFLOW))
> + PM8001_FAIL_DBG(pm8001_ha,
> + pm8001_printk("SAS Address of IO Failure Drive:"
> + "%016llx", SAS_ADDR(t->dev->sas_addr)-1));
> +
>   switch (status) {
>   case IO_SUCCESS:
>   PM8001_IO_DBG(pm8001_ha, pm8001_printk("IO_SUCCESS"
> @@ -2276,6 +2283,11 @@ mpi_sata_completion(struct pm8001_hba_info *pm8001_ha, 
> void *piomb)
>   u32 param;
>   u32 status;
>   u32 tag;
> + int i, j;
> + u8 sata_addr_low[4];
> + u32 temp_sata_addr_low;
> + u8 sata_addr_hi[4];
> + u32 temp_sata_addr_hi;
>   struct sata_completion_resp *psataPayload;
>   struct task_status_struct *ts;
>   struct ata_task_resp *resp ;
> @@ -2325,7 +2337,60 @@ mpi_sata_completion(struct pm8001_hba_info *pm8001_ha, 
> void *piomb)
>   pm8001_printk("ts null\n"));
>   return;
>   }
> + /* Print sas address of IO failed device */
> + if ((status != IO_SUCCESS) && (status != IO_OVERFLOW) &&
> + (status != IO_UNDERFLOW)) {
> + if (!((t->dev->parent) &&
> + (DEV_IS_EXPANDER(t->dev->parent->dev_type {
> + for (i = 0 , j = 4 ; j <= 7 && i <= 3 ; i++ , j++)
> + sata_addr_low[i] = pm8001_ha->sas_addr[j];
> + for (i = 0 , j = 0; j <= 3 && i <= 3; i++ , j++)
> + sata_addr_hi[i] = pm8001_ha->sas_addr[j];
> + memcpy(&temp_sata_addr_low, sata_addr_low,
> + sizeof(sata_addr_low));
> + memcpy(&temp_sata_addr_hi, sata_addr_hi,
> + sizeof(sata_addr_hi));
> + temp_sata_addr_hi = (((temp_sata_addr_hi >> 24) & 0xff)
> + |((temp_sata_addr_hi << 8) &
> + 0xff) |
> + ((temp_sata_addr_hi >> 8)
> + & 0xff00) |
> + ((temp_sata_addr_hi << 24) &
> + 0xff00));
> + temp_sata_addr_low = temp_sata_addr_low >> 24)
> + & 0xff) |
> + ((temp_sata_addr_low << 8)
> + & 0xff) |
> + ((temp_sata_addr_low >> 8)
> + & 0xff00) |
> + ((temp_sata_addr_low << 24)
> + & 0xff00)) +
> + pm8001_dev->attached_phy +
> + 0x10);
> + PM8001_FAIL_DBG(pm8001_ha,
> + pm8001_printk("SAS Address of IO Failure Drive:"
> + "%08x%08x", temp_sata_addr_hi,
> + temp_sata_addr_low));
> + } else {
> + for (i = 0 , j = 4 ; j <= 7 && i <= 3; i++ , j++)
> + sata_addr_low[i] = t->dev->sas_addr[j];
> + for (i = 0 , j = 0 ; j <= 3 && i <= 3 ; i++ , j++)
> + sata_addr_hi[i] = t->dev->sas_addr[j];
> + temp_sata_addr_low = (sata_addr_low[0] << 24) |
> + (sata_addr_low[1] << 16) |
> + (sata_addr_low[2] << 8) |
> + (sata_addr_low[3]);
> + temp_sata_addr_hi =  (sata_addr_hi[0] << 24)|
> + (sata_addr_hi[1] << 16)|
>

Re: [PATCH V2 07/10] pm80xx: Set device state response logic fix.

[Jack Wang]

On 09/26/2013 07:39 AM, Anand wrote:
> From edf018a942e0da4da12bad21e5bf0d48621a18de Mon Sep 17 00:00:00 2001
> From: Anand Kumar Santhanam 
> Date: Tue, 17 Sep 2013 16:58:10 +0530
> Subject: [PATCH V2 07/10] pm80xx: Set device state response logic fix.
> 
> Signed-off-by: anandkumar.santha...@pmcs.com
> 

Reviewed-by: Jack Wang 

> ---
>  drivers/scsi/pm8001/pm8001_hwi.c |6 --
>  drivers/scsi/pm8001/pm8001_sas.c |9 -
>  2 files changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/scsi/pm8001/pm8001_hwi.c 
> b/drivers/scsi/pm8001/pm8001_hwi.c
> index c9cc744..502c7d6 100644
> --- a/drivers/scsi/pm8001/pm8001_hwi.c
> +++ b/drivers/scsi/pm8001/pm8001_hwi.c
> @@ -3152,8 +3152,8 @@ void pm8001_mpi_set_dev_state_resp(struct 
> pm8001_hba_info *pm8001_ha,
>   struct pm8001_device *pm8001_dev = ccb->device;
>   u32 status = le32_to_cpu(pPayload->status);
>   u32 device_id = le32_to_cpu(pPayload->device_id);
> - u8 pds = le32_to_cpu(pPayload->pds_nds) | PDS_BITS;
> - u8 nds = le32_to_cpu(pPayload->pds_nds) | NDS_BITS;
> + u8 pds = le32_to_cpu(pPayload->pds_nds) & PDS_BITS;
> + u8 nds = le32_to_cpu(pPayload->pds_nds) & NDS_BITS;
>   PM8001_MSG_DBG(pm8001_ha, pm8001_printk("Set device id = 0x%x state "
>   "from 0x%x to 0x%x status = 0x%x!\n",
>   device_id, pds, nds, status));
> @@ -4765,6 +4765,8 @@ int pm8001_chip_ssp_tm_req(struct pm8001_hba_info 
> *pm8001_ha,
>   sspTMCmd.tmf = cpu_to_le32(tmf->tmf);
>   memcpy(sspTMCmd.lun, task->ssp_task.LUN, 8);
>   sspTMCmd.tag = cpu_to_le32(ccb->ccb_tag);
> + if (pm8001_ha->chip_id != chip_8001)
> + sspTMCmd.ds_ads_m = 0x08;
>   circularQ = &pm8001_ha->inbnd_q_tbl[0];
>   ret = pm8001_mpi_build_cmd(pm8001_ha, circularQ, opc, &sspTMCmd, 0);
>   return ret;
> diff --git a/drivers/scsi/pm8001/pm8001_sas.c 
> b/drivers/scsi/pm8001/pm8001_sas.c
> index a85d73d..f4eb18e 100644
> --- a/drivers/scsi/pm8001/pm8001_sas.c
> +++ b/drivers/scsi/pm8001/pm8001_sas.c
> @@ -447,7 +447,6 @@ static int pm8001_task_exec(struct sas_task *task, const 
> int num,
>   break;
>   case SAS_PROTOCOL_SATA:
>   case SAS_PROTOCOL_STP:
> - case SAS_PROTOCOL_SATA | SAS_PROTOCOL_STP:
>   rc = pm8001_task_prep_ata(pm8001_ha, ccb);
>   break;
>   default:
> @@ -704,6 +703,8 @@ static int pm8001_exec_internal_tmf_task(struct 
> domain_device *dev,
>   int res, retry;
>   struct sas_task *task = NULL;
>   struct pm8001_hba_info *pm8001_ha = pm8001_find_ha_by_dev(dev);
> + struct pm8001_device *pm8001_dev = dev->lldd_dev;
> + DECLARE_COMPLETION_ONSTACK(completion_setstate);
>  
>   for (retry = 0; retry < 3; retry++) {
>   task = sas_alloc_slow_task(GFP_KERNEL);
> @@ -729,6 +730,12 @@ static int pm8001_exec_internal_tmf_task(struct 
> domain_device *dev,
>   goto ex_err;
>   }
>   wait_for_completion(&task->slow_task->completion);
> + if (pm8001_ha->chip_id != chip_8001) {
> + pm8001_dev->setds_completion = &completion_setstate;
> + PM8001_CHIP_DISP->set_dev_state_req(pm8001_ha,
> + pm8001_dev, 0x01);
> + wait_for_completion(&completion_setstate);
> + }
>   res = -TMF_RESP_FUNC_FAILED;
>   /* Even TMF timed out, return direct. */
>   if ((task->task_state_flags & SAS_TASK_STATE_ABORTED)) {
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V2 08/10] pm80xx: Queue rotation logic for inbound and outbound queues

[Jack Wang]

On 09/26/2013 07:40 AM, Anand wrote:
> From 678d8085ace7c471dc140420c41dc4ad70300d60 Mon Sep 17 00:00:00 2001
> From: Anand Kumar Santhanam 
> Date: Wed, 18 Sep 2013 11:12:59 +0530
> Subject: [PATCH V2 08/10] pm80xx: Queue rotation logic for inbound and 
> outbound queues.
> 
> Signed-off-by: anandkumar.santha...@pmcs.com
> 
I'd like to know the number of this improved for performance?

Jack

> ---
>  drivers/scsi/pm8001/pm80xx_hwi.c |   31 +--
>  1 files changed, 13 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/scsi/pm8001/pm80xx_hwi.c 
> b/drivers/scsi/pm8001/pm80xx_hwi.c
> index 80a10aa..ce59d0d 100644
> --- a/drivers/scsi/pm8001/pm80xx_hwi.c
> +++ b/drivers/scsi/pm8001/pm80xx_hwi.c
> @@ -3715,8 +3715,7 @@ static int pm80xx_chip_ssp_io_req(struct 
> pm8001_hba_info *pm8001_ha,
>   int ret;
>   u64 phys_addr;
>   struct inbound_queue_table *circularQ;
> - static u32 inb;
> - static u32 outb;
> + u32 q_index;
>   u32 opc = OPC_INB_SSPINIIOSTART;
>   memset(&ssp_cmd, 0, sizeof(ssp_cmd));
>   memcpy(ssp_cmd.ssp_iu.lun, task->ssp_task.LUN, 8);
> @@ -3735,7 +3734,8 @@ static int pm80xx_chip_ssp_io_req(struct 
> pm8001_hba_info *pm8001_ha,
>   ssp_cmd.ssp_iu.efb_prio_attr |= (task->ssp_task.task_attr & 7);
>   memcpy(ssp_cmd.ssp_iu.cdb, task->ssp_task.cmd->cmnd,
>  task->ssp_task.cmd->cmd_len);
> - circularQ = &pm8001_ha->inbnd_q_tbl[0];
> + q_index = (u32) (pm8001_dev->id & 0x00ff) % PM8001_MAX_INB_NUM;
> + circularQ = &pm8001_ha->inbnd_q_tbl[q_index];
>  
>   /* Check if encryption is set */
>   if (pm8001_ha->chip->encrypt &&
> @@ -3783,7 +3783,7 @@ static int pm80xx_chip_ssp_io_req(struct 
> pm8001_hba_info *pm8001_ha,
>   } else {
>   PM8001_IO_DBG(pm8001_ha, pm8001_printk(
>   "Sending Normal SAS command 0x%x inb q %x\n",
> - task->ssp_task.cmd->cmnd[0], inb));
> + task->ssp_task.cmd->cmnd[0], q_index));
>   /* fill in PRD (scatter/gather) table, if any */
>   if (task->num_scatter > 1) {
>   pm8001_chip_make_sg(task->scatter, ccb->n_elem,
> @@ -3809,11 +3809,9 @@ static int pm80xx_chip_ssp_io_req(struct 
> pm8001_hba_info *pm8001_ha,
>   ssp_cmd.esgl = 0;
>   }
>   }
> - ret = pm8001_mpi_build_cmd(pm8001_ha, circularQ, opc, &ssp_cmd, outb++);
> -
> - /* rotate the outb queue */
> - outb = outb%PM8001_MAX_SPCV_OUTB_NUM;
> -
> + q_index = (u32) (pm8001_dev->id & 0x00ff) % PM8001_MAX_OUTB_NUM;
> + ret = pm8001_mpi_build_cmd(pm8001_ha, circularQ, opc,
> + &ssp_cmd, q_index);
>   return ret;
>  }
>  
> @@ -3825,8 +3823,7 @@ static int pm80xx_chip_sata_req(struct pm8001_hba_info 
> *pm8001_ha,
>   struct pm8001_device *pm8001_ha_dev = dev->lldd_dev;
>   u32 tag = ccb->ccb_tag;
>   int ret;
> - static u32 inb;
> - static u32 outb;
> + u32 q_index;
>   struct sata_start_req sata_cmd;
>   u32 hdr_tag, ncg_tag = 0;
>   u64 phys_addr;
> @@ -3836,7 +3833,8 @@ static int pm80xx_chip_sata_req(struct pm8001_hba_info 
> *pm8001_ha,
>   unsigned long flags;
>   u32 opc = OPC_INB_SATA_HOST_OPSTART;
>   memset(&sata_cmd, 0, sizeof(sata_cmd));
> - circularQ = &pm8001_ha->inbnd_q_tbl[0];
> + q_index = (u32) (pm8001_ha_dev->id & 0x00ff) % PM8001_MAX_INB_NUM;
> + circularQ = &pm8001_ha->inbnd_q_tbl[q_index];
>  
>   if (task->data_dir == PCI_DMA_NONE) {
>   ATAP = 0x04; /* no data*/
> @@ -3917,7 +3915,7 @@ static int pm80xx_chip_sata_req(struct pm8001_hba_info 
> *pm8001_ha,
>   } else {
>   PM8001_IO_DBG(pm8001_ha, pm8001_printk(
>   "Sending Normal SATA command 0x%x inb %x\n",
> - sata_cmd.sata_fis.command, inb));
> + sata_cmd.sata_fis.command, q_index));
>   /* dad (bit 0-1) is 0 */
>   sata_cmd.ncqtag_atap_dir_m_dad =
>   cpu_to_le32(((ncg_tag & 0xff)<<16) |
> @@ -4014,12 +4012,9 @@ static int pm80xx_chip_sata_req(struct pm8001_hba_info 
> *pm8001_ha,
>   }
>   }
>   }
> -
> + q_index = (u32) (pm8001_ha_dev->id & 0x00ff) % PM8001_MAX_OUTB_NUM;
>   ret = pm8001_mpi_build_cmd(pm8001_ha, circularQ, opc,
> - &sata_cmd, outb++);
> -
> - /* rotate the outb queue */
> - outb = outb%PM8001_MAX_SPCV_OUTB_NUM;
> + &sata_cmd, q_index);
>   return ret;
>  }
>  
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V2 09/10] pm80xx: 4G boundary fix

[Jack Wang]

On 09/26/2013 07:42 AM, Anand wrote:
> From 4715339743d45a6ef862bc0f5d5ec404b4667f94 Mon Sep 17 00:00:00 2001
> From: Anand Kumar Santhanam 
> Date: Wed, 18 Sep 2013 11:14:54 +0530
> Subject: [PATCH V2 09/10] pm80xx: 4G boundary fix.
> 
> Signed-off-by: anandkumar.santha...@pmcs.com
> 
Please give more description why you do this change?

Thanks
Jack

> ---
>  drivers/scsi/pm8001/pm80xx_hwi.c |  103 
> +-
>  1 files changed, 101 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/pm8001/pm80xx_hwi.c 
> b/drivers/scsi/pm8001/pm80xx_hwi.c
> index ce59d0d..94de2ed 100644
> --- a/drivers/scsi/pm8001/pm80xx_hwi.c
> +++ b/drivers/scsi/pm8001/pm80xx_hwi.c
> @@ -3713,7 +3713,8 @@ static int pm80xx_chip_ssp_io_req(struct 
> pm8001_hba_info *pm8001_ha,
>   struct ssp_ini_io_start_req ssp_cmd;
>   u32 tag = ccb->ccb_tag;
>   int ret;
> - u64 phys_addr;
> + u64 phys_addr, start_addr, end_addr;
> + u32 end_addr_high, end_addr_low;
>   struct inbound_queue_table *circularQ;
>   u32 q_index;
>   u32 opc = OPC_INB_SSPINIIOSTART;
> @@ -3767,6 +3768,30 @@ static int pm80xx_chip_ssp_io_req(struct 
> pm8001_hba_info *pm8001_ha,
>   cpu_to_le32(upper_32_bits(dma_addr));
>   ssp_cmd.enc_len = cpu_to_le32(task->total_xfer_len);
>   ssp_cmd.enc_esgl = 0;
> + /* Check 4G Boundary */
> + start_addr = cpu_to_le64(dma_addr);
> + end_addr = (start_addr + ssp_cmd.enc_len) - 1;
> + end_addr_low = cpu_to_le32(lower_32_bits(end_addr));
> + end_addr_high = cpu_to_le32(upper_32_bits(end_addr));
> + if (end_addr_high != ssp_cmd.enc_addr_high) {
> + PM8001_FAIL_DBG(pm8001_ha,
> + pm8001_printk("The sg list address "
> + "start_addr=0x%016llx data_len=0x%x "
> + "end_addr_high=0x%08x end_addr_low="
> + "0x%08x has crossed 4G boundary\n",
> + start_addr, ssp_cmd.enc_len,
> + end_addr_high, end_addr_low));
> + pm8001_chip_make_sg(task->scatter, 1,
> + ccb->buf_prd);
> + phys_addr = ccb->ccb_dma_handle +
> + offsetof(struct pm8001_ccb_info,
> + buf_prd[0]);
> + ssp_cmd.enc_addr_low =
> + cpu_to_le32(lower_32_bits(phys_addr));
> + ssp_cmd.enc_addr_high =
> + cpu_to_le32(upper_32_bits(phys_addr));
> + ssp_cmd.enc_esgl = cpu_to_le32(1<<31);
> + }
>   } else if (task->num_scatter == 0) {
>   ssp_cmd.enc_addr_low = 0;
>   ssp_cmd.enc_addr_high = 0;
> @@ -3802,6 +3827,30 @@ static int pm80xx_chip_ssp_io_req(struct 
> pm8001_hba_info *pm8001_ha,
>   cpu_to_le32(upper_32_bits(dma_addr));
>   ssp_cmd.len = cpu_to_le32(task->total_xfer_len);
>   ssp_cmd.esgl = 0;
> + /* Check 4G Boundary */
> + start_addr = cpu_to_le64(dma_addr);
> + end_addr = (start_addr + ssp_cmd.len) - 1;
> + end_addr_low = cpu_to_le32(lower_32_bits(end_addr));
> + end_addr_high = cpu_to_le32(upper_32_bits(end_addr));
> + if (end_addr_high != ssp_cmd.addr_high) {
> + PM8001_FAIL_DBG(pm8001_ha,
> + pm8001_printk("The sg list address "
> + "start_addr=0x%016llx data_len=0x%x "
> + "end_addr_high=0x%08x end_addr_low="
> + "0x%08x has crossed 4G boundary\n",
> +  start_addr, ssp_cmd.len,
> +  end_addr_high, end_addr_low));
> + pm8001_chip_make_sg(task->scatter, 1,
> + ccb->buf_prd);
> + phys_addr = ccb->ccb_dma_handle +
> + offsetof(struct pm8001_ccb_info,
> +  buf_prd[0]);
> + ssp_cmd.addr_low =
> + cpu_to_le32(lower_32_bits(phys_addr));
> + ssp_cmd.addr_high =
> + cpu_to_le32(upper_32_bits(phys_addr));
> + ssp_cmd.e

Re: [PATCH V2 10/10] pm80xx: Firmware logging support

[Jack Wang]

On 09/26/2013 07:43 AM, Anand wrote:
> From ab1b030500a835eacda3d86e5570366099b21311 Mon Sep 17 00:00:00 2001
> From: Anand Kumar Santhanam 
> Date: Wed, 4 Sep 2013 12:57:00 +0530
> Subject: [PATCH V2 10/10] pm80xx: Firmware logging support.
> 
> Supports below logging facilities,
> Inbound outbound queues dump.
> Non fatal dump in case of IO failures.
> Fatal dump in case of firmware failure.
> 
> Signed-off-by: anandkumar.santha...@pmcs.com

 Reviewed-by: Jack Wang 

> ---
>  drivers/scsi/pm8001/pm8001_ctl.c  |  129 +
>  drivers/scsi/pm8001/pm8001_ctl.h  |4 +
>  drivers/scsi/pm8001/pm8001_defs.h |3 +-
>  drivers/scsi/pm8001/pm8001_hwi.c  |   83 ++
>  drivers/scsi/pm8001/pm8001_hwi.h  |3 +
>  drivers/scsi/pm8001/pm8001_init.c |4 +
>  drivers/scsi/pm8001/pm8001_sas.h  |   68 +++
>  drivers/scsi/pm8001/pm80xx_hwi.c  |  225 
> +
>  drivers/scsi/pm8001/pm80xx_hwi.h  |2 +
>  9 files changed, 520 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/scsi/pm8001/pm8001_ctl.c 
> b/drivers/scsi/pm8001/pm8001_ctl.c
> index 5a19e19..2ca79a5 100644
> --- a/drivers/scsi/pm8001/pm8001_ctl.c
> +++ b/drivers/scsi/pm8001/pm8001_ctl.c
> @@ -309,6 +309,94 @@ static ssize_t pm8001_ctl_aap_log_show(struct device 
> *cdev,
>  }
>  static DEVICE_ATTR(aap_log, S_IRUGO, pm8001_ctl_aap_log_show, NULL);
>  /**
> + * pm8001_ctl_ib_queue_log_show - Out bound Queue log
> + * @cdev:pointer to embedded class device
> + * @buf: the buffer returned
> + * A sysfs 'read-only' shost attribute.
> + */
> +static ssize_t pm8001_ctl_ib_queue_log_show(struct device *cdev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct Scsi_Host *shost = class_to_shost(cdev);
> + struct sas_ha_struct *sha = SHOST_TO_SAS_HA(shost);
> + struct pm8001_hba_info *pm8001_ha = sha->lldd_ha;
> + int offset;
> + char *str = buf;
> + int start = 0;
> +#define IB_MEMMAP(c) \
> + (*(u32 *)((u8 *)pm8001_ha-> \
> + memoryMap.region[IB].virt_ptr + \
> + pm8001_ha->evtlog_ib_offset + (c)))
> +
> +#define IB_MEMMAP_SPC(c) \
> + (*(u32 *)((u8 *)pm8001_ha-> \
> + memoryMap.region[IB].virt_ptr + \
> + pm8001_ha->evtlog_ib_offset + (c)))
> +
> + for (offset = 0; offset < IB_OB_READ_TIMES; offset++) {
> + if (pm8001_ha->chip_id != chip_8001)
> + str += sprintf(str, "0x%08x\n", IB_MEMMAP(start));
> + else
> + str += sprintf(str, "0x%08x\n", IB_MEMMAP_SPC(start));
> + start = start + 4;
> + }
> + pm8001_ha->evtlog_ib_offset += SYSFS_OFFSET;
> + if pm8001_ha->evtlog_ib_offset) % (PM80XX_IB_OB_QUEUE_SIZE)) == 0)
> + && (pm8001_ha->chip_id != chip_8001))
> + pm8001_ha->evtlog_ib_offset = 0;
> + if pm8001_ha->evtlog_ib_offset) % (PM8001_IB_OB_QUEUE_SIZE)) == 0)
> + && (pm8001_ha->chip_id == chip_8001))
> + pm8001_ha->evtlog_ib_offset = 0;
> +
> + return str - buf;
> +}
> +
> +static DEVICE_ATTR(ib_log, S_IRUGO, pm8001_ctl_ib_queue_log_show, NULL);
> +/**
> + * pm8001_ctl_ob_queue_log_show - Out bound Queue log
> + * @cdev:pointer to embedded class device
> + * @buf: the buffer returned
> + * A sysfs 'read-only' shost attribute.
> + */
> +
> +static ssize_t pm8001_ctl_ob_queue_log_show(struct device *cdev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct Scsi_Host *shost = class_to_shost(cdev);
> + struct sas_ha_struct *sha = SHOST_TO_SAS_HA(shost);
> + struct pm8001_hba_info *pm8001_ha = sha->lldd_ha;
> + int offset;
> + char *str = buf;
> + int start = 0;
> +#define OB_MEMMAP(c) \
> + (*(u32 *)((u8 *)pm8001_ha-> \
> + memoryMap.region[OB].virt_ptr + \
> + pm8001_ha->evtlog_ob_offset + (c)))
> +
> +#define OB_MEMMAP_SPC(c) \
> + (*(u32 *)((u8 *)pm8001_ha-> \
> + memoryMap.region[OB].virt_ptr + \
> + pm8001_ha->evtlog_ob_offset + (c)))
> +
> + for (offset = 0; offset < IB_OB_READ_TIMES; offset++) {
> + if (pm8001_ha->chip_id != chip_8001)
> + str += sprintf(str, "0x%08x\n", OB_MEMMAP(start));
> + else
> + str += sprintf(str, "0x%08x\n", OB_MEMMAP_SPC(start));
> + start = start + 4;
> + }
> + pm8001_ha->evtlog_ob_offset += SYSFS_OFFSET;
> + if pm8001_ha->evtlog_ob_offset) % (PM80XX_IB_OB_QUEUE_SIZE)) == 0)
> + && (pm8001_ha->chip_id != chip_8001))
> + pm8001_ha->evtlog_ob_offset = 0;
> + if pm8001_ha->evtlog_ob_offset) % (PM8001_IB_OB_QUEUE_SIZE)) == 0)
> + && (pm8001_ha->chip_id == chip_8001))
> + pm8001_ha->evtlog_ob_offset = 0;
> +
> +

RE: [PATCH V2 05/10] pm80xx: Phy settings support for motherboard controller.

[Sangeetha Gnanasekaran]

Phy settings support is only for Motherboard controller. Firmware will
only initialize default values for all PHY in case of motherboard
controller. Hence, this is mandatory to add this support in all
motherboard controllers to configure their own settings. 

-Original Message-
From: Jack Wang [mailto:xjtu...@gmail.com] 
Sent: Thursday, September 26, 2013 12:27 PM
To: Anand Kumar Santhanam
Cc: linux-scsi@vger.kernel.org; Sangeetha Gnanasekaran; Nikith
Ganigarakoppal; Viswas G
Subject: Re: [PATCH V2 05/10] pm80xx: Phy settings support for
motherboard controller.

snip
>  #ifdef PM8001_USE_MSIX
>  /**
>   * pm8001_setup_msix - enable MSI-X interrupt @@ -847,6 +872,9 @@ 
> static int pm8001_pci_probe(struct pci_dev *pdev,
>   }
>  
>   pm8001_init_sas_add(pm8001_ha);
> + /* phy setting support for motherboard controller */
> + if (pdev->subsystem_vendor != PCI_VENDOR_ID_ADAPTEC2)
> + pm8001_get_phy_settings_info(pm8001_ha);

Are you sure about this, have you checked all controller except device
with subsystem_vendorid is PCI_VENDOR_ID_ADAPTEC2 all support this
get_phy_setting_info funcion?

Jack
>   pm8001_post_sas_ha_init(shost, chip);
>   rc = sas_register_ha(SHOST_TO_SAS_HA(shost));
>   if (rc)
> diff --git a/drivers/scsi/pm8001/pm8001_sas.h 
> b/drivers/scsi/pm8001/pm8001_sas.h
> index 68e1147..cbde11a 100644
> --- a/drivers/scsi/pm8001/pm8001_sas.h
> +++ b/drivers/scsi/pm8001/pm8001_sas.h
> @@ -632,7 +632,8 @@ struct pm8001_device *pm8001_find_dev(struct 
> pm8001_hba_info *pm8001_ha,  int pm80xx_set_thermal_config(struct 
> pm8001_hba_info *pm8001_ha);
>  
>  int pm8001_bar4_shift(struct pm8001_hba_info *pm8001_ha, u32 
> shiftValue);
> -
> +void pm8001_set_phy_profile(struct pm8001_hba_info *pm8001_ha,
> + u32 length, u8 *buf);
>  /* ctl shared API */
>  extern struct device_attribute *pm8001_host_attrs[];
>  
> diff --git a/drivers/scsi/pm8001/pm80xx_hwi.c 
> b/drivers/scsi/pm8001/pm80xx_hwi.c
> index 99cec5f..e1ab320 100644
> --- a/drivers/scsi/pm8001/pm80xx_hwi.c
> +++ b/drivers/scsi/pm8001/pm80xx_hwi.c
> @@ -3131,9 +3131,27 @@ static int mpi_flash_op_ext_resp(struct 
> pm8001_hba_info *pm8001_ha, void *piomb)  static int
mpi_set_phy_profile_resp(struct pm8001_hba_info *pm8001_ha,
>   void *piomb)
>  {
> - PM8001_MSG_DBG(pm8001_ha,
> - pm8001_printk("
pm80xx_addition_functionality\n"));
> + u8 page_code;
> + struct set_phy_profile_resp *pPayload =
> + (struct set_phy_profile_resp *)(piomb + 4);
> + u32 ppc_phyid = le32_to_cpu(pPayload->ppc_phyid);
> + u32 status = le32_to_cpu(pPayload->status);
>  
> + page_code = (u8)((ppc_phyid & 0xFF00) >> 8);
> + if (status) {
> + /* status is FAILED */
> + PM8001_FAIL_DBG(pm8001_ha,
> + pm8001_printk("PhyProfile command failed  with
status "
> + "0x%08X \n", status));
> + return -1;
> + } else {
> + if (page_code != SAS_PHY_ANALOG_SETTINGS_PAGE) {
> + PM8001_FAIL_DBG(pm8001_ha,
> + pm8001_printk("Invalid page code
0x%X\n",
> + page_code));
> + return -1;
> + }
> + }
>   return 0;
>  }
>  
> @@ -4128,6 +4146,45 @@ pm80xx_chip_isr(struct pm8001_hba_info
*pm8001_ha, u8 vec)
>   return IRQ_HANDLED;
>  }
>  
> +void mpi_set_phy_profile_req(struct pm8001_hba_info *pm8001_ha,
> + u32 operation, u32 phyid, u32 length, u32 *buf) {
> + u32 tag , i, j = 0;
> + int rc;
> + struct set_phy_profile_req payload;
> + struct inbound_queue_table *circularQ;
> + u32 opc = OPC_INB_SET_PHY_PROFILE;
> +
> + memset(&payload, 0, sizeof(payload));
> + rc = pm8001_tag_alloc(pm8001_ha, &tag);
> + if (rc)
> + PM8001_FAIL_DBG(pm8001_ha, pm8001_printk("Invalid
tag\n"));
> + circularQ = &pm8001_ha->inbnd_q_tbl[0];
> + payload.tag = cpu_to_le32(tag);
> + payload.ppc_phyid = (((operation & 0xF) << 8) | (phyid  &
0xFF));
> + PM8001_INIT_DBG(pm8001_ha,
> + pm8001_printk(" phy profile command for phy %x ,length
is %d\n",
> + payload.ppc_phyid, length));
> + for (i = length ; i < (length + PHY_DWORD_LENGTH - 1) ; i++) {
> + payload.reserved[j] =  cpu_to_le32(*((u32 *)buf + i));
> + j++;
> + }
> + pm8001_mpi_build_cmd(pm8001_ha, circularQ, opc, &payload, 0); }
> +
> +void pm8001_set_phy_profile(struct pm8001_hba_info *pm8001_ha,
> + u32 length, u8 *buf)
> +{
> + u32 page_code, i;
> +
> + page_code = SAS_PHY_ANALOG_SETTINGS_PAGE;
> + for (i = 0 ; i < pm8001_ha->chip->n_phy ; i++) {
> + mpi_set_phy_profile_req(pm8001_ha,
> + SAS_PHY_ANALOG_SETTINGS_PAGE, i, length, (u32
*)buf);
> + length = length + PHY_DWORD_LENGTH;
> + }
> + PM8001_INIT_DBG(pm8001_ha, pm8001

RE: [PATCH V2 06/10] pm80xx: Print SAS address of IO failed device.

[Sangeetha Gnanasekaran]

We will resend the updated patch with this change.

-Original Message-
From: Jack Wang [mailto:xjtu...@gmail.com] 
Sent: Thursday, September 26, 2013 12:33 PM
To: Anand Kumar Santhanam
Cc: linux-scsi@vger.kernel.org; Sangeetha Gnanasekaran; Nikith
Ganigarakoppal; Viswas G
Subject: Re: [PATCH V2 06/10] pm80xx: Print SAS address of IO failed
device.

On 09/26/2013 07:37 AM, Anand wrote:
> From 5ddec5ef3033b4fee08dcdc7ba8b434425453e9d Mon Sep 17 00:00:00 2001
> From: Anand Kumar Santhanam 
> Date: Tue, 17 Sep 2013 16:47:21 +0530
> Subject: [PATCH V2 06/10] pm80xx: Print SAS address of IO failed
device.
> 
> Signed-off-by: anandkumar.santha...@pmcs.com
> 
> ---
>  drivers/scsi/pm8001/pm8001_hwi.c |   65
++
>  drivers/scsi/pm8001/pm80xx_hwi.c |   65
++
>  2 files changed, 130 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/scsi/pm8001/pm8001_hwi.c 
> b/drivers/scsi/pm8001/pm8001_hwi.c
> index 2fd8c38..c9cc744 100644
> --- a/drivers/scsi/pm8001/pm8001_hwi.c
> +++ b/drivers/scsi/pm8001/pm8001_hwi.c
> @@ -1868,6 +1868,13 @@ mpi_ssp_completion(struct pm8001_hba_info
*pm8001_ha , void *piomb)
>   if (unlikely(!t || !t->lldd_task || !t->dev))
>   return;
>   ts = &t->task_status;
> + /* Print sas address of IO failed device */
> + if ((status != IO_SUCCESS) && (status != IO_OVERFLOW) &&
> + (status != IO_UNDERFLOW))
> + PM8001_FAIL_DBG(pm8001_ha,
> + pm8001_printk("SAS Address of IO Failure Drive:"
> + "%016llx", SAS_ADDR(t->dev->sas_addr)-1));
> +
>   switch (status) {
>   case IO_SUCCESS:
>   PM8001_IO_DBG(pm8001_ha, pm8001_printk("IO_SUCCESS"
> @@ -2276,6 +2283,11 @@ mpi_sata_completion(struct pm8001_hba_info
*pm8001_ha, void *piomb)
>   u32 param;
>   u32 status;
>   u32 tag;
> + int i, j;
> + u8 sata_addr_low[4];
> + u32 temp_sata_addr_low;
> + u8 sata_addr_hi[4];
> + u32 temp_sata_addr_hi;
>   struct sata_completion_resp *psataPayload;
>   struct task_status_struct *ts;
>   struct ata_task_resp *resp ;
> @@ -2325,7 +2337,60 @@ mpi_sata_completion(struct pm8001_hba_info
*pm8001_ha, void *piomb)
>   pm8001_printk("ts null\n"));
>   return;
>   }
> + /* Print sas address of IO failed device */
> + if ((status != IO_SUCCESS) && (status != IO_OVERFLOW) &&
> + (status != IO_UNDERFLOW)) {
> + if (!((t->dev->parent) &&
> + (DEV_IS_EXPANDER(t->dev->parent->dev_type {
> + for (i = 0 , j = 4 ; j <= 7 && i <= 3 ; i++ ,
j++)
> + sata_addr_low[i] =
pm8001_ha->sas_addr[j];
> + for (i = 0 , j = 0; j <= 3 && i <= 3; i++ , j++)
> + sata_addr_hi[i] =
pm8001_ha->sas_addr[j];
> + memcpy(&temp_sata_addr_low, sata_addr_low,
> + sizeof(sata_addr_low));
> + memcpy(&temp_sata_addr_hi, sata_addr_hi,
> + sizeof(sata_addr_hi));
> + temp_sata_addr_hi = (((temp_sata_addr_hi >> 24)
& 0xff)
> + |((temp_sata_addr_hi <<
8) &
> + 0xff) |
> + ((temp_sata_addr_hi >>
8)
> + & 0xff00) |
> + ((temp_sata_addr_hi <<
24) &
> + 0xff00));
> + temp_sata_addr_low = temp_sata_addr_low >>
24)
> + & 0xff) |
> + ((temp_sata_addr_low <<
8)
> + & 0xff) |
> + ((temp_sata_addr_low >>
8)
> + & 0xff00) |
> + ((temp_sata_addr_low <<
24)
> + & 0xff00)) +
> + pm8001_dev->attached_phy
+
> + 0x10);
> + PM8001_FAIL_DBG(pm8001_ha,
> + pm8001_printk("SAS Address of IO Failure
Drive:"
> + "%08x%08x", temp_sata_addr_hi,
> + temp_sata_addr_low));
> + } else {
> + for (i = 0 , j = 4 ; j <= 7 && i <= 3; i++ ,
j++)
> + sata_addr_low[i] = t->dev->sas_addr[j];
> + for (i = 0 , j = 0 ; j <= 3 && i <= 3 ; i++ ,
j++)
> + sata_addr_hi[i] = t->dev->sas_addr[j];
> + temp_sata_addr_low = (sata_addr_low[0] << 24) |
> +  

RE: [PATCH V2 09/10] pm80xx: 4G boundary fix

[Sangeetha Gnanasekaran]

We will add description and resend the updated patch.

-Original Message-
From: Jack Wang [mailto:xjtu...@gmail.com] 
Sent: Thursday, September 26, 2013 12:40 PM
To: Anand Kumar Santhanam
Cc: linux-scsi@vger.kernel.org; Sangeetha Gnanasekaran; Nikith
Ganigarakoppal; Viswas G
Subject: Re: [PATCH V2 09/10] pm80xx: 4G boundary fix

On 09/26/2013 07:42 AM, Anand wrote:
> From 4715339743d45a6ef862bc0f5d5ec404b4667f94 Mon Sep 17 00:00:00 2001
> From: Anand Kumar Santhanam 
> Date: Wed, 18 Sep 2013 11:14:54 +0530
> Subject: [PATCH V2 09/10] pm80xx: 4G boundary fix.
> 
> Signed-off-by: anandkumar.santha...@pmcs.com
> 
Please give more description why you do this change?

Thanks
Jack

> ---
>  drivers/scsi/pm8001/pm80xx_hwi.c |  103 
> +-
>  1 files changed, 101 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/pm8001/pm80xx_hwi.c 
> b/drivers/scsi/pm8001/pm80xx_hwi.c
> index ce59d0d..94de2ed 100644
> --- a/drivers/scsi/pm8001/pm80xx_hwi.c
> +++ b/drivers/scsi/pm8001/pm80xx_hwi.c
> @@ -3713,7 +3713,8 @@ static int pm80xx_chip_ssp_io_req(struct
pm8001_hba_info *pm8001_ha,
>   struct ssp_ini_io_start_req ssp_cmd;
>   u32 tag = ccb->ccb_tag;
>   int ret;
> - u64 phys_addr;
> + u64 phys_addr, start_addr, end_addr;
> + u32 end_addr_high, end_addr_low;
>   struct inbound_queue_table *circularQ;
>   u32 q_index;
>   u32 opc = OPC_INB_SSPINIIOSTART;
> @@ -3767,6 +3768,30 @@ static int pm80xx_chip_ssp_io_req(struct
pm8001_hba_info *pm8001_ha,
>   cpu_to_le32(upper_32_bits(dma_addr));
>   ssp_cmd.enc_len =
cpu_to_le32(task->total_xfer_len);
>   ssp_cmd.enc_esgl = 0;
> + /* Check 4G Boundary */
> + start_addr = cpu_to_le64(dma_addr);
> + end_addr = (start_addr + ssp_cmd.enc_len) - 1;
> + end_addr_low =
cpu_to_le32(lower_32_bits(end_addr));
> + end_addr_high =
cpu_to_le32(upper_32_bits(end_addr));
> + if (end_addr_high != ssp_cmd.enc_addr_high) {
> + PM8001_FAIL_DBG(pm8001_ha,
> + pm8001_printk("The sg list
address "
> + "start_addr=0x%016llx
data_len=0x%x "
> + "end_addr_high=0x%08x
end_addr_low="
> + "0x%08x has crossed 4G
boundary\n",
> + start_addr,
ssp_cmd.enc_len,
> + end_addr_high,
end_addr_low));
> + pm8001_chip_make_sg(task->scatter, 1,
> + ccb->buf_prd);
> + phys_addr = ccb->ccb_dma_handle +
> + offsetof(struct pm8001_ccb_info,
> + buf_prd[0]);
> + ssp_cmd.enc_addr_low =
> +
cpu_to_le32(lower_32_bits(phys_addr));
> + ssp_cmd.enc_addr_high =
> +
cpu_to_le32(upper_32_bits(phys_addr));
> + ssp_cmd.enc_esgl = cpu_to_le32(1<<31);
> + }
>   } else if (task->num_scatter == 0) {
>   ssp_cmd.enc_addr_low = 0;
>   ssp_cmd.enc_addr_high = 0;
> @@ -3802,6 +3827,30 @@ static int pm80xx_chip_ssp_io_req(struct
pm8001_hba_info *pm8001_ha,
>   cpu_to_le32(upper_32_bits(dma_addr));
>   ssp_cmd.len = cpu_to_le32(task->total_xfer_len);
>   ssp_cmd.esgl = 0;
> + /* Check 4G Boundary */
> + start_addr = cpu_to_le64(dma_addr);
> + end_addr = (start_addr + ssp_cmd.len) - 1;
> + end_addr_low =
cpu_to_le32(lower_32_bits(end_addr));
> + end_addr_high =
cpu_to_le32(upper_32_bits(end_addr));
> + if (end_addr_high != ssp_cmd.addr_high) {
> + PM8001_FAIL_DBG(pm8001_ha,
> + pm8001_printk("The sg list
address "
> + "start_addr=0x%016llx
data_len=0x%x "
> + "end_addr_high=0x%08x
end_addr_low="
> + "0x%08x has crossed 4G
boundary\n",
> +  start_addr,
ssp_cmd.len,
> +  end_addr_high,
end_addr_low));
> + pm8001_chip_make_sg(task->scatter, 1,
> + ccb->buf_prd);
> + phys_addr = ccb->ccb_dma_handle +
> + offsetof(struct pm8001_ccb_info,
> +  buf_prd[0]);
> + ssp_cmd.addr

RE: [PATCH V2 01/10] pm80xx: Device id changes to support series 8 controllers.

[Sangeetha Gnanasekaran]

We will modify and send updated patch.

-Original Message-
From: Jack Wang [mailto:xjtu...@gmail.com] 
Sent: Thursday, September 26, 2013 12:10 PM
To: Anand Kumar Santhanam
Cc: linux-scsi@vger.kernel.org; Sangeetha Gnanasekaran; Nikith
Ganigarakoppal; Viswas G
Subject: Re: [PATCH V2 01/10] pm80xx: Device id changes to support
series 8 controllers.

snip
>*/
>   payload.ase_sh_lm_slr_phyid = cpu_to_le32(SPINHOLD_DISABLE |
>   LINKMODE_AUTO | LINKRATE_15 |
> - LINKRATE_30 | LINKRATE_60 | phy_id);
> + LINKRATE_30 | LINKRATE_60 | LINKRATE_120 |
phy_id);
Is it safe to set 12g linkrate support also on 6g, if so, then :

Reviewed-by: Jack Wang 

>   /* SSC Disable and SAS Analog ST configuration */
>   /**
>   payload.ase_sh_lm_slr_phyid =
> diff --git a/drivers/scsi/pm8001/pm80xx_hwi.h
b/drivers/scsi/pm8001/pm80xx_hwi.h
> index 2b760ba..9a9116d 100644
> --- a/drivers/scsi/pm8001/pm80xx_hwi.h
> +++ b/drivers/scsi/pm8001/pm80xx_hwi.h
> @@ -168,6 +168,7 @@
>  #define LINKRATE_15  (0x01 << 8)
>  #define LINKRATE_30  (0x02 << 8)
>  #define LINKRATE_60  (0x06 << 8)
> +#define LINKRATE_120 (0x08 << 8)
>  
>  /* Thermal related */
>  #define  THERMAL_ENABLE  0x1
> @@ -1223,10 +1224,10 @@ typedef struct SASProtocolTimerConfig
SASProtocolTimerConfig_t;
>  
>  /* MSGU CONFIGURATION TABLE*/
>  
> -#define SPCv_MSGU_CFG_TABLE_UPDATE   0x01
> -#define SPCv_MSGU_CFG_TABLE_RESET0x02
> -#define SPCv_MSGU_CFG_TABLE_FREEZE   0x04
> -#define SPCv_MSGU_CFG_TABLE_UNFREEZE 0x08
> +#define SPCv_MSGU_CFG_TABLE_UPDATE   0x001
> +#define SPCv_MSGU_CFG_TABLE_RESET0x002
> +#define SPCv_MSGU_CFG_TABLE_FREEZE   0x004
> +#define SPCv_MSGU_CFG_TABLE_UNFREEZE 0x008
>  #define MSGU_IBDB_SET0x00
>  #define MSGU_HOST_INT_STATUS 0x08
>  #define MSGU_HOST_INT_MASK   0x0C
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH V2 08/10] pm80xx: Queue rotation logic for inbound and outbound queues

[Sangeetha Gnanasekaran]

We have increased performance by setting inbound to 16 and outbound to
4. We will send patch for it.

-Original Message-
From: Jack Wang [mailto:xjtu...@gmail.com] 
Sent: Thursday, September 26, 2013 12:38 PM
To: Anand Kumar Santhanam
Cc: linux-scsi@vger.kernel.org; Sangeetha Gnanasekaran; Nikith
Ganigarakoppal; Viswas G
Subject: Re: [PATCH V2 08/10] pm80xx: Queue rotation logic for inbound
and outbound queues

On 09/26/2013 07:40 AM, Anand wrote:
> From 678d8085ace7c471dc140420c41dc4ad70300d60 Mon Sep 17 00:00:00 2001
> From: Anand Kumar Santhanam 
> Date: Wed, 18 Sep 2013 11:12:59 +0530
> Subject: [PATCH V2 08/10] pm80xx: Queue rotation logic for inbound and
outbound queues.
> 
> Signed-off-by: anandkumar.santha...@pmcs.com
> 
I'd like to know the number of this improved for performance?

Jack

> ---
>  drivers/scsi/pm8001/pm80xx_hwi.c |   31
+--
>  1 files changed, 13 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/scsi/pm8001/pm80xx_hwi.c 
> b/drivers/scsi/pm8001/pm80xx_hwi.c
> index 80a10aa..ce59d0d 100644
> --- a/drivers/scsi/pm8001/pm80xx_hwi.c
> +++ b/drivers/scsi/pm8001/pm80xx_hwi.c
> @@ -3715,8 +3715,7 @@ static int pm80xx_chip_ssp_io_req(struct
pm8001_hba_info *pm8001_ha,
>   int ret;
>   u64 phys_addr;
>   struct inbound_queue_table *circularQ;
> - static u32 inb;
> - static u32 outb;
> + u32 q_index;
>   u32 opc = OPC_INB_SSPINIIOSTART;
>   memset(&ssp_cmd, 0, sizeof(ssp_cmd));
>   memcpy(ssp_cmd.ssp_iu.lun, task->ssp_task.LUN, 8); @@ -3735,7 
> +3734,8 @@ static int pm80xx_chip_ssp_io_req(struct pm8001_hba_info
*pm8001_ha,
>   ssp_cmd.ssp_iu.efb_prio_attr |= (task->ssp_task.task_attr & 7);
>   memcpy(ssp_cmd.ssp_iu.cdb, task->ssp_task.cmd->cmnd,
>  task->ssp_task.cmd->cmd_len);
> - circularQ = &pm8001_ha->inbnd_q_tbl[0];
> + q_index = (u32) (pm8001_dev->id & 0x00ff) %
PM8001_MAX_INB_NUM;
> + circularQ = &pm8001_ha->inbnd_q_tbl[q_index];
>  
>   /* Check if encryption is set */
>   if (pm8001_ha->chip->encrypt &&
> @@ -3783,7 +3783,7 @@ static int pm80xx_chip_ssp_io_req(struct
pm8001_hba_info *pm8001_ha,
>   } else {
>   PM8001_IO_DBG(pm8001_ha, pm8001_printk(
>   "Sending Normal SAS command 0x%x inb q %x\n",
> - task->ssp_task.cmd->cmnd[0], inb));
> + task->ssp_task.cmd->cmnd[0], q_index));
>   /* fill in PRD (scatter/gather) table, if any */
>   if (task->num_scatter > 1) {
>   pm8001_chip_make_sg(task->scatter, ccb->n_elem,
@@ -3809,11 
> +3809,9 @@ static int pm80xx_chip_ssp_io_req(struct pm8001_hba_info
*pm8001_ha,
>   ssp_cmd.esgl = 0;
>   }
>   }
> - ret = pm8001_mpi_build_cmd(pm8001_ha, circularQ, opc, &ssp_cmd,
outb++);
> -
> - /* rotate the outb queue */
> - outb = outb%PM8001_MAX_SPCV_OUTB_NUM;
> -
> + q_index = (u32) (pm8001_dev->id & 0x00ff) %
PM8001_MAX_OUTB_NUM;
> + ret = pm8001_mpi_build_cmd(pm8001_ha, circularQ, opc,
> + &ssp_cmd, q_index);
>   return ret;
>  }
>  
> @@ -3825,8 +3823,7 @@ static int pm80xx_chip_sata_req(struct
pm8001_hba_info *pm8001_ha,
>   struct pm8001_device *pm8001_ha_dev = dev->lldd_dev;
>   u32 tag = ccb->ccb_tag;
>   int ret;
> - static u32 inb;
> - static u32 outb;
> + u32 q_index;
>   struct sata_start_req sata_cmd;
>   u32 hdr_tag, ncg_tag = 0;
>   u64 phys_addr;
> @@ -3836,7 +3833,8 @@ static int pm80xx_chip_sata_req(struct
pm8001_hba_info *pm8001_ha,
>   unsigned long flags;
>   u32 opc = OPC_INB_SATA_HOST_OPSTART;
>   memset(&sata_cmd, 0, sizeof(sata_cmd));
> - circularQ = &pm8001_ha->inbnd_q_tbl[0];
> + q_index = (u32) (pm8001_ha_dev->id & 0x00ff) %
PM8001_MAX_INB_NUM;
> + circularQ = &pm8001_ha->inbnd_q_tbl[q_index];
>  
>   if (task->data_dir == PCI_DMA_NONE) {
>   ATAP = 0x04; /* no data*/
> @@ -3917,7 +3915,7 @@ static int pm80xx_chip_sata_req(struct
pm8001_hba_info *pm8001_ha,
>   } else {
>   PM8001_IO_DBG(pm8001_ha, pm8001_printk(
>   "Sending Normal SATA command 0x%x inb %x\n",
> - sata_cmd.sata_fis.command, inb));
> + sata_cmd.sata_fis.command, q_index));
>   /* dad (bit 0-1) is 0 */
>   sata_cmd.ncqtag_atap_dir_m_dad =
>   cpu_to_le32(((ncg_tag & 0xff)<<16) | @@ -4014,12
+4012,9 @@ static 
> int pm80xx_chip_sata_req(struct pm8001_hba_info *pm8001_ha,
>   }
>   }
>   }
> -
> + q_index = (u32) (pm8001_ha_dev->id & 0x00ff) % 
> +PM8001_MAX_OUTB_NUM;
>   ret = pm8001_mpi_build_cmd(pm8001_ha, circularQ, opc,
> - &sata_cmd, outb++);
> -
> - /* rotate the outb queue */
> - outb = outb%PM8001_MA

Re: [PATCH V2 05/10] pm80xx: Phy settings support for motherboard controller.

[Jack Wang]

On 09/26/2013 11:04 AM, Sangeetha Gnanasekaran wrote:
> Phy settings support is only for Motherboard controller. Firmware will
> only initialize default values for all PHY in case of motherboard
> controller. Hence, this is mandatory to add this support in all
> motherboard controllers to configure their own settings. 

I still think you'd better add a list for devices know to support phy
setting similar as IS_SPC12G, there are some devices in the driver
support list do not have default setting I suspect.

Jack
> 
> -Original Message-
> From: Jack Wang [mailto:xjtu...@gmail.com] 
> Sent: Thursday, September 26, 2013 12:27 PM
> To: Anand Kumar Santhanam
> Cc: linux-scsi@vger.kernel.org; Sangeetha Gnanasekaran; Nikith
> Ganigarakoppal; Viswas G
> Subject: Re: [PATCH V2 05/10] pm80xx: Phy settings support for
> motherboard controller.
> 
> snip
>>  #ifdef PM8001_USE_MSIX
>>  /**
>>   * pm8001_setup_msix - enable MSI-X interrupt @@ -847,6 +872,9 @@ 
>> static int pm8001_pci_probe(struct pci_dev *pdev,
>>  }
>>  
>>  pm8001_init_sas_add(pm8001_ha);
>> +/* phy setting support for motherboard controller */
>> +if (pdev->subsystem_vendor != PCI_VENDOR_ID_ADAPTEC2)
>> +pm8001_get_phy_settings_info(pm8001_ha);
> 
> Are you sure about this, have you checked all controller except device
> with subsystem_vendorid is PCI_VENDOR_ID_ADAPTEC2 all support this
> get_phy_setting_info funcion?
> 
> Jack
>>  pm8001_post_sas_ha_init(shost, chip);
>>  rc = sas_register_ha(SHOST_TO_SAS_HA(shost));
>>  if (rc)
>> diff --git a/drivers/scsi/pm8001/pm8001_sas.h 
>> b/drivers/scsi/pm8001/pm8001_sas.h
>> index 68e1147..cbde11a 100644
>> --- a/drivers/scsi/pm8001/pm8001_sas.h
>> +++ b/drivers/scsi/pm8001/pm8001_sas.h
>> @@ -632,7 +632,8 @@ struct pm8001_device *pm8001_find_dev(struct 
>> pm8001_hba_info *pm8001_ha,  int pm80xx_set_thermal_config(struct 
>> pm8001_hba_info *pm8001_ha);
>>  
>>  int pm8001_bar4_shift(struct pm8001_hba_info *pm8001_ha, u32 
>> shiftValue);
>> -
>> +void pm8001_set_phy_profile(struct pm8001_hba_info *pm8001_ha,
>> +u32 length, u8 *buf);
>>  /* ctl shared API */
>>  extern struct device_attribute *pm8001_host_attrs[];
>>  
>> diff --git a/drivers/scsi/pm8001/pm80xx_hwi.c 
>> b/drivers/scsi/pm8001/pm80xx_hwi.c
>> index 99cec5f..e1ab320 100644
>> --- a/drivers/scsi/pm8001/pm80xx_hwi.c
>> +++ b/drivers/scsi/pm8001/pm80xx_hwi.c
>> @@ -3131,9 +3131,27 @@ static int mpi_flash_op_ext_resp(struct 
>> pm8001_hba_info *pm8001_ha, void *piomb)  static int
> mpi_set_phy_profile_resp(struct pm8001_hba_info *pm8001_ha,
>>  void *piomb)
>>  {
>> -PM8001_MSG_DBG(pm8001_ha,
>> -pm8001_printk("
> pm80xx_addition_functionality\n"));
>> +u8 page_code;
>> +struct set_phy_profile_resp *pPayload =
>> +(struct set_phy_profile_resp *)(piomb + 4);
>> +u32 ppc_phyid = le32_to_cpu(pPayload->ppc_phyid);
>> +u32 status = le32_to_cpu(pPayload->status);
>>  
>> +page_code = (u8)((ppc_phyid & 0xFF00) >> 8);
>> +if (status) {
>> +/* status is FAILED */
>> +PM8001_FAIL_DBG(pm8001_ha,
>> +pm8001_printk("PhyProfile command failed  with
> status "
>> +"0x%08X \n", status));
>> +return -1;
>> +} else {
>> +if (page_code != SAS_PHY_ANALOG_SETTINGS_PAGE) {
>> +PM8001_FAIL_DBG(pm8001_ha,
>> +pm8001_printk("Invalid page code
> 0x%X\n",
>> +page_code));
>> +return -1;
>> +}
>> +}
>>  return 0;
>>  }
>>  
>> @@ -4128,6 +4146,45 @@ pm80xx_chip_isr(struct pm8001_hba_info
> *pm8001_ha, u8 vec)
>>  return IRQ_HANDLED;
>>  }
>>  
>> +void mpi_set_phy_profile_req(struct pm8001_hba_info *pm8001_ha,
>> +u32 operation, u32 phyid, u32 length, u32 *buf) {
>> +u32 tag , i, j = 0;
>> +int rc;
>> +struct set_phy_profile_req payload;
>> +struct inbound_queue_table *circularQ;
>> +u32 opc = OPC_INB_SET_PHY_PROFILE;
>> +
>> +memset(&payload, 0, sizeof(payload));
>> +rc = pm8001_tag_alloc(pm8001_ha, &tag);
>> +if (rc)
>> +PM8001_FAIL_DBG(pm8001_ha, pm8001_printk("Invalid
> tag\n"));
>> +circularQ = &pm8001_ha->inbnd_q_tbl[0];
>> +payload.tag = cpu_to_le32(tag);
>> +payload.ppc_phyid = (((operation & 0xF) << 8) | (phyid  &
> 0xFF));
>> +PM8001_INIT_DBG(pm8001_ha,
>> +pm8001_printk(" phy profile command for phy %x ,length
> is %d\n",
>> +payload.ppc_phyid, length));
>> +for (i = length ; i < (length + PHY_DWORD_LENGTH - 1) ; i++) {
>> +payload.reserved[j] =  cpu_to_le32(*((u32 *)buf + i));
>> +j++;
>> +}
>> +pm8001_mpi_build_cmd(pm8001_ha, circularQ, opc, &payload, 0); }
>> +
>> +void pm8001_set_phy_profile(struct pm8001_hba_info *pm8001_ha,
>> +u32 length, u8 *buf)
>> +{
>> +u32 page_code

RE: [PATCH V2 05/10] pm80xx: Phy settings support for motherboard controller.

[Sangeetha Gnanasekaran]

Ok. We will add the device list and send updated patch.

-Original Message-
From: Jack Wang [mailto:xjtu...@gmail.com] 
Sent: Thursday, September 26, 2013 2:43 PM
To: Sangeetha Gnanasekaran
Cc: Anand Kumar Santhanam; linux-scsi@vger.kernel.org; Nikith
Ganigarakoppal; Viswas G
Subject: Re: [PATCH V2 05/10] pm80xx: Phy settings support for
motherboard controller.

On 09/26/2013 11:04 AM, Sangeetha Gnanasekaran wrote:
> Phy settings support is only for Motherboard controller. Firmware will

> only initialize default values for all PHY in case of motherboard 
> controller. Hence, this is mandatory to add this support in all 
> motherboard controllers to configure their own settings.

I still think you'd better add a list for devices know to support phy
setting similar as IS_SPC12G, there are some devices in the driver
support list do not have default setting I suspect.

Jack
> 
> -Original Message-
> From: Jack Wang [mailto:xjtu...@gmail.com]
> Sent: Thursday, September 26, 2013 12:27 PM
> To: Anand Kumar Santhanam
> Cc: linux-scsi@vger.kernel.org; Sangeetha Gnanasekaran; Nikith 
> Ganigarakoppal; Viswas G
> Subject: Re: [PATCH V2 05/10] pm80xx: Phy settings support for 
> motherboard controller.
> 
> snip
>>  #ifdef PM8001_USE_MSIX
>>  /**
>>   * pm8001_setup_msix - enable MSI-X interrupt @@ -847,6 +872,9 @@ 
>> static int pm8001_pci_probe(struct pci_dev *pdev,
>>  }
>>  
>>  pm8001_init_sas_add(pm8001_ha);
>> +/* phy setting support for motherboard controller */
>> +if (pdev->subsystem_vendor != PCI_VENDOR_ID_ADAPTEC2)
>> +pm8001_get_phy_settings_info(pm8001_ha);
> 
> Are you sure about this, have you checked all controller except device

> with subsystem_vendorid is PCI_VENDOR_ID_ADAPTEC2 all support this 
> get_phy_setting_info funcion?
> 
> Jack
>>  pm8001_post_sas_ha_init(shost, chip);
>>  rc = sas_register_ha(SHOST_TO_SAS_HA(shost));
>>  if (rc)
>> diff --git a/drivers/scsi/pm8001/pm8001_sas.h
>> b/drivers/scsi/pm8001/pm8001_sas.h
>> index 68e1147..cbde11a 100644
>> --- a/drivers/scsi/pm8001/pm8001_sas.h
>> +++ b/drivers/scsi/pm8001/pm8001_sas.h
>> @@ -632,7 +632,8 @@ struct pm8001_device *pm8001_find_dev(struct 
>> pm8001_hba_info *pm8001_ha,  int pm80xx_set_thermal_config(struct 
>> pm8001_hba_info *pm8001_ha);
>>  
>>  int pm8001_bar4_shift(struct pm8001_hba_info *pm8001_ha, u32 
>> shiftValue);
>> -
>> +void pm8001_set_phy_profile(struct pm8001_hba_info *pm8001_ha,
>> +u32 length, u8 *buf);
>>  /* ctl shared API */
>>  extern struct device_attribute *pm8001_host_attrs[];
>>  
>> diff --git a/drivers/scsi/pm8001/pm80xx_hwi.c
>> b/drivers/scsi/pm8001/pm80xx_hwi.c
>> index 99cec5f..e1ab320 100644
>> --- a/drivers/scsi/pm8001/pm80xx_hwi.c
>> +++ b/drivers/scsi/pm8001/pm80xx_hwi.c
>> @@ -3131,9 +3131,27 @@ static int mpi_flash_op_ext_resp(struct 
>> pm8001_hba_info *pm8001_ha, void *piomb)  static int
> mpi_set_phy_profile_resp(struct pm8001_hba_info *pm8001_ha,
>>  void *piomb)
>>  {
>> -PM8001_MSG_DBG(pm8001_ha,
>> -pm8001_printk("
> pm80xx_addition_functionality\n"));
>> +u8 page_code;
>> +struct set_phy_profile_resp *pPayload =
>> +(struct set_phy_profile_resp *)(piomb + 4);
>> +u32 ppc_phyid = le32_to_cpu(pPayload->ppc_phyid);
>> +u32 status = le32_to_cpu(pPayload->status);
>>  
>> +page_code = (u8)((ppc_phyid & 0xFF00) >> 8);
>> +if (status) {
>> +/* status is FAILED */
>> +PM8001_FAIL_DBG(pm8001_ha,
>> +pm8001_printk("PhyProfile command failed  with
> status "
>> +"0x%08X \n", status));
>> +return -1;
>> +} else {
>> +if (page_code != SAS_PHY_ANALOG_SETTINGS_PAGE) {
>> +PM8001_FAIL_DBG(pm8001_ha,
>> +pm8001_printk("Invalid page code
> 0x%X\n",
>> +page_code));
>> +return -1;
>> +}
>> +}
>>  return 0;
>>  }
>>  
>> @@ -4128,6 +4146,45 @@ pm80xx_chip_isr(struct pm8001_hba_info
> *pm8001_ha, u8 vec)
>>  return IRQ_HANDLED;
>>  }
>>  
>> +void mpi_set_phy_profile_req(struct pm8001_hba_info *pm8001_ha,
>> +u32 operation, u32 phyid, u32 length, u32 *buf) {
>> +u32 tag , i, j = 0;
>> +int rc;
>> +struct set_phy_profile_req payload;
>> +struct inbound_queue_table *circularQ;
>> +u32 opc = OPC_INB_SET_PHY_PROFILE;
>> +
>> +memset(&payload, 0, sizeof(payload));
>> +rc = pm8001_tag_alloc(pm8001_ha, &tag);
>> +if (rc)
>> +PM8001_FAIL_DBG(pm8001_ha, pm8001_printk("Invalid
> tag\n"));
>> +circularQ = &pm8001_ha->inbnd_q_tbl[0];
>> +payload.tag = cpu_to_le32(tag);
>> +payload.ppc_phyid = (((operation & 0xF) << 8) | (phyid  &
> 0xFF));
>> +PM8001_INIT_DBG(pm8001_ha,
>> +pm8001_printk(" phy profile command for phy %x ,length
> is %d\n",
>> +payload.ppc_phyid, length)

Re: [PATCH 39/51] DMA-API: others: use dma_set_coherent_mask()

[Archit Taneja]

Hi,

On Friday 20 September 2013 04:41 AM, Russell King wrote:

The correct way for a driver to specify the coherent DMA mask is
not to directly access the field in the struct device, but to use
dma_set_coherent_mask().  Only arch and bus code should access this
member directly.

Convert all direct write accesses to using the correct API.

Signed-off-by: Russell King 
---
  drivers/ata/pata_ixp4xx_cf.c |5 -
  drivers/gpu/drm/exynos/exynos_drm_drv.c  |6 +-
  drivers/gpu/drm/omapdrm/omap_dmm_tiler.c |5 +++--
  3 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/ata/pata_ixp4xx_cf.c b/drivers/ata/pata_ixp4xx_cf.c
index 1ec53f8..ddf470c 100644
--- a/drivers/ata/pata_ixp4xx_cf.c
+++ b/drivers/ata/pata_ixp4xx_cf.c
@@ -144,6 +144,7 @@ static int ixp4xx_pata_probe(struct platform_device *pdev)
struct ata_host *host;
struct ata_port *ap;
struct ixp4xx_pata_data *data = dev_get_platdata(&pdev->dev);
+   int ret;

cs0 = platform_get_resource(pdev, IORESOURCE_MEM, 0);
cs1 = platform_get_resource(pdev, IORESOURCE_MEM, 1);
@@ -157,7 +158,9 @@ static int ixp4xx_pata_probe(struct platform_device *pdev)
return -ENOMEM;

/* acquire resources and fill host */
-   pdev->dev.coherent_dma_mask = DMA_BIT_MASK(32);
+   ret = dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(32));
+   if (ret)
+   return ret;

data->cs0 = devm_ioremap(&pdev->dev, cs0->start, 0x1000);
data->cs1 = devm_ioremap(&pdev->dev, cs1->start, 0x1000);
diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c 
b/drivers/gpu/drm/exynos/exynos_drm_drv.c
index bb82ef7..81192d0 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
@@ -286,7 +286,11 @@ static struct drm_driver exynos_drm_driver = {

  static int exynos_drm_platform_probe(struct platform_device *pdev)
  {
-   pdev->dev.coherent_dma_mask = DMA_BIT_MASK(32);
+   int ret;
+
+   ret = dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(32));
+   if (ret)
+   return ret;

return drm_platform_init(&exynos_drm_driver, pdev);
  }
diff --git a/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c 
b/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
index acf6678..701c4c1 100644
--- a/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
+++ b/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
@@ -664,8 +664,9 @@ static int omap_dmm_probe(struct platform_device *dev)
}

/* set dma mask for device */
-   /* NOTE: this is a workaround for the hwmod not initializing properly */
-   dev->dev.coherent_dma_mask = DMA_BIT_MASK(32);
+   ret = dma_set_coherent_mask(&dev->dev, DMA_BIT_MASK(32));
+   if (ret)
+   goto fail;


Tested with omapdrm on omap4 panda es board.

Thanks,
Archit

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: SCSI's heuristics for enabling WRITE SAME still need work [was: dm mpath: disable WRITE SAME if it fails]

[Bernd Schubert]

On 09/26/2013 07:39 AM, Douglas Gilbert wrote:

On 13-09-25 08:44 PM, Martin K. Petersen wrote:

"Bernd" == Bernd Schubert  writes:


Hey Bernd,

Bernd> I'm afraid we have another problem. I'm currently working on to
Bernd> get discard working for our LSI2008 HBAs with attached sata-SSDs
Bernd> and the heuristics in sd_read_write_same with based on VPD page
Bernd> 0x89 is not correct for this HBA - its SATL supports write-same

This has nothing to do with the WRITE SAME heuristics.

It's true that depending on wind and whether we might issue WRITE
SAME(10) or (16) with the UNMAP bit set to perform discard operations on
the low level device. But we use a set of different (and somewhat more
reliable) heuristics to decide which command to send down for that
purpose.

For discards to a SATA device to work you need a recent phase LSI
firmware. And you need the target mode firmware (IT). There is no
UNMAP->DSM TRIM translation in the RAID (IR) firmware.

If your SATA SSDs reports DSM TRIM support, the LSI firmware will set
LBPME=1 in READ CAPACITY(16) and the LOGICAL BLOCK PROVISIONING VPD page
will indicate a preference for the UNMAP command (LBPU=1).

Also, LSI firmware is well-behaved in general and will report ILLEGAL
REQUEST when you send down a command that can't be handled.


An example with a LSI 9212-4i4e running the latest firmware
(P17) connected to a SATA SSD (via an expander):

# sg_vpd /dev/sg1 -p sinq
standard INQUIRY:
   PQual=0  Device_type=0  RMB=0  version=0x06  [SPC-4]
   [AERC=0]  [TrmTsk=0]  NormACA=0  HiSUP=1  Resp_data_format=2
   SCCS=0  ACC=0  TPGS=0  3PC=0  Protect=0  [BQue=0]
   EncServ=0  MultiP=0  [MChngr=0]  [ACKREQQ=0]  Addr16=0
   [RelAdr=0]  WBus16=0  Sync=0  Linked=0  [TranDis=0]  CmdQue=1
   Vendor_identification: ATA
   Product_identification: INTEL SSDSA2M080
   Product_revision_level: 02M3

# sg_vpd /dev/sg1 -p bl
Block limits VPD page (SBC):
   Write same no zero (WSNZ): 0
   Maximum compare and write length: 0 blocks
   Optimal transfer length granularity: 0 blocks
   Maximum transfer length: 0 blocks
   Optimal transfer length: 0 blocks
   Maximum prefetch length: 0 blocks
   Maximum unmap LBA count: 4194303
   Maximum unmap block descriptor count: 32
   Optimal unmap granularity: 1
   Unmap granularity alignment valid: 0
   Unmap granularity alignment: 0
   Maximum write same length: 0x0 blocks

# sg_vpd /dev/sg1 -p lbpv
Logical block provisioning VPD page (SBC):
   Unmap command supported (LBPU): 1
   Write same (16) with unmap bit supported (LBWS): 1
   Write same (10) with unmap bit supported (LBWS10): 0
   Logical block provisioning read zeros (LBPRZ): 0
   Anchored LBAs supported (ANC_SUP): 1
   Threshold exponent: 0
   Descriptor present (DP): 0
   Provisioning type: 0

# sg_opcodes -n /dev/sg1
Report supported operation codes: operation not supported


Room for improvement there. It also supports a useful set
of mode pages (including some chageable fields) and two
log pages.



Both types of systems we have in-house neither block limits vpd nor 
READ_CAP16 return anything that would indicate discard is supported. But 
UNMAP and WRITE SAME unmap(*) just work fine.


I certainly don't want to cause any more write-same trouble, but as all 
layers initially have to assume write same is supported anyway and need 
to dynamically disable it if it fails, can't we also enable discard by 
default with WRITE SAME16 unmap?

I'm going to send a PoC patch later on.

The older system I can play with for a few days has an Intel510 
(SSDSC2MH25) connected to an LSI SAS9211-8i via a sas enclosure.
Ignoring identificication string and revision level, sg_vdp output is 
almost the same here, but with the exception of


(wheezy)node02:~# sg_vpd /dev/sdb -p bl
Block limits VPD page (SBC):
[...]
  Maximum unmap LBA count: 0
  Maximum unmap block descriptor count: 0
  Optimal unmap granularity: 0
[...]


I think interesting for discard is also read cap 16:


(wheezy)node02:~# sg_readcap --16 /dev/sda
Read Capacity results:
   Protection: prot_en=0, p_type=0, p_i_exponent=0
   Logical block provisioning: lbpme=0, lbprz=0
   Last logical block address=490350671 (0x1d3a284f), Number of logical 
blocks=490350672
   Logical block length=512 bytes
   Logical blocks per physical block exponent=0
   Lowest aligned logical block address=0
Hence:
   Device size: 251059544064 bytes, 239429.0 MiB, 251.06 GB



So again no indication of discard support.


We also long ago flushed  the IT fw and just recently updated to fw 
version 17

mpt2sas0: LSISAS2008: FWVersion(17.00.01.00), ChipRevision(0x03), 
BiosVersion(07.33.00.00)
mpt2sas0: Protocol=(Initiator,Target), Capabilities=(TLR,EEDP,Snapshot 
Buffer,Diag Trace Buffer,Task Set Full,NCQ)



Thanks,
Bernd


PS: LSI SATL with FWv17 seems to have an unmap bug - I cannot unmap the 
last sector:



(wheezy)node02:~# cat /sys/block/sdb/size
488397168



(wheezy)node02:~# sg_write_same --16 --unmap --verbose --lba=488397167 --num=1 
/dev/sdb
Default data-

Re: esp_scsi QTAG in FAS216 (was Re: [PATCH 0/2] Experimental Amiga Zorro ESP driver)

[Michael Schmitz]

Tuomas,

I might still have a copy of the manual somewhere from way back, if you 
haven't found it anywhere. Would be on an old disk or even hardcopy in 
storage, so please confirm you still need it.


Cheers,

 Michael



Am 13.09.2013 um 03:36 schrieb Tuomas Vainikka:


On 09/11/2013 05:48 PM, Geert Uytterhoeven wrote:
On Wed, Sep 11, 2013 at 12:12 PM, Michael Schmitz 
 wrote:
problem. Using PIO, only the first byte of the tag message comes 
through. It
might not be esp_scsi's fault, but there seems to be an assumption 
that all
devices support TCQ. Also, no SCSI-2 features seem to be enabled in 
the chip

by esp_scsi.
I'd have to check with DaveM, but such an assumption might in fact 
exist.

BTW, it would be nice to start CCing linux-scsi@vger.kernel.org and
David S. Miller  on future discussions.

Gr{oetje,eeting}s,

 Geert



Hello,

Does anyone have the register descriptions for the FAS216 chip? It 
would seem that receiving only one byte during reconnect is perfectly 
normal [1] unless SCSI-2 features are explicitly enabled (which 
esp_scsi doesn't seem to be doing).


Also, looking at the timeout formulae in the old NCR53C9x.c driver, 
the values would be different for FAS216. Why was this dropped from 
the modern esp_scsi?


1. Page 2 in 
http://www.datasheetarchive.com/dl/Scans-030/ScansU9X12569.pdf


-Tuomas


--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Race condition between "read CFQ stats" and "block device shutdown"

[Tejun Heo]

Hello, (cc'ing linux-scsi)

On Wed, Sep 25, 2013 at 01:37:51PM -0700, Anatol Pomozov wrote:
> Hi
> 
> On Wed, Sep 4, 2013 at 9:07 AM, Tejun Heo  wrote:
> > Hello,
> >
> > On Wed, Sep 04, 2013 at 08:45:33AM -0700, Anatol Pomozov wrote:
> >> I am not an expect in block code, so I have a few questions here:
> >>
> >> - are we sure that this operation is atomic? What if blkg->q becomes
> >> dead right after we checked it, and blkg->q->queue_lock got invalid so
> >> we have the same crash as before?
> >
> > request_queue lock switching is something inherently broken in block
> > layer.  It's unsalvageable.
> 
> Fully agree. The problem that request_queue->queue_lock is a shared
> resource that concurrently modified/accessed. In this case (when one
> thread changes, another thread access it) we need synchronization to
> prevent race conditions. So we need a spin_lock to access queue_lock
> spin_lock, otherwise we have a crash like one above...
> 
> >  Maybe we can drop lock switching once blk-mq is fully merged.
> 
> Could you please provide more information about it? What is the timeline?

I have no idea.  Hopefully, not too far out.  Jens would have better
idea.

> If there is an easy way to fix the race condition I would like to
> help. Please give me some pointer what direction I should move.

The first step would be identifying who are actually making use of
lock switching, why and how much difference it would make for them to
not do that.

> PS Just a little bit of context why I care about this bug. We test a
> large farm that actively uses iscsi. We are going to have a lot of
> iscsi device startup/shutdown. I am testing whether this codepath has
> race conditions and I found one above.

Thanks.

-- 
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Race condition between "read CFQ stats" and "block device shutdown"

[Hannes Reinecke]

On 09/26/2013 03:54 PM, Tejun Heo wrote:
> Hello, (cc'ing linux-scsi)
> 
> On Wed, Sep 25, 2013 at 01:37:51PM -0700, Anatol Pomozov wrote:
>> Hi
>>
>> On Wed, Sep 4, 2013 at 9:07 AM, Tejun Heo  wrote:
>>> Hello,
>>>
>>> On Wed, Sep 04, 2013 at 08:45:33AM -0700, Anatol Pomozov wrote:
 I am not an expect in block code, so I have a few questions here:

 - are we sure that this operation is atomic? What if blkg->q becomes
 dead right after we checked it, and blkg->q->queue_lock got invalid so
 we have the same crash as before?
>>>
>>> request_queue lock switching is something inherently broken in block
>>> layer.  It's unsalvageable.
>>
>> Fully agree. The problem that request_queue->queue_lock is a shared
>> resource that concurrently modified/accessed. In this case (when one
>> thread changes, another thread access it) we need synchronization to
>> prevent race conditions. So we need a spin_lock to access queue_lock
>> spin_lock, otherwise we have a crash like one above...
>>
>>>  Maybe we can drop lock switching once blk-mq is fully merged.
>>
>> Could you please provide more information about it? What is the timeline?
> 
> I have no idea.  Hopefully, not too far out.  Jens would have better
> idea.
> 
>> If there is an easy way to fix the race condition I would like to
>> help. Please give me some pointer what direction I should move.
> 
> The first step would be identifying who are actually making use of
> lock switching, why and how much difference it would make for them to
> not do that.
> 
Typically, the lock is being used by the block drivers to
synchronize access between some internal data structures and the
request queue itself. You don't actually _need_ to do it that way,
but removing the lock switching would involve quite some redesign of
these drivers.
Give that most of the are rather oldish I really wouldn't want to
touch them.

However, none of the modern devices should be using this lock
switching, so I would just ignore it.
EG SCSI most definitely doesn't use it.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke   zSeries & Storage
h...@suse.de  +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Race condition between "read CFQ stats" and "block device shutdown"

[Tejun Heo]

Hey, Hannes.

On Thu, Sep 26, 2013 at 04:18:34PM +0200, Hannes Reinecke wrote:
> However, none of the modern devices should be using this lock
> switching, so I would just ignore it.
> EG SCSI most definitely doesn't use it.

The kernel is crashing from it, so I don't think ignoring is an
acceptable strategy here.

Thanks.

-- 
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: SCSI's heuristics for enabling WRITE SAME still need work [was: dm mpath: disable WRITE SAME if it fails]

[Martin K. Petersen]

> "Bernd" == Bernd Schubert  writes:

Bernd,

Bernd> Both types of systems we have in-house neither block limits vpd
Bernd> nor READ_CAP16 return anything that would indicate discard is
Bernd> supported. But UNMAP and WRITE SAME unmap(*) just work fine.

I have a collection of different SSDs in a tray connected to an LSI
SAS2008 ASIC. The 510 is the only drive that does not have LBPME=1.
Chances are it's because DRAT and RZAT are not set but it could also be
that the 510 is blacklisted.


Bernd> I certainly don't want to cause any more write-same trouble, but
Bernd> as all layers initially have to assume write same is supported
Bernd> anyway and need to dynamically disable it if it fails, can't we
Bernd> also enable discard by default with WRITE SAME16 unmap?  

No thanks :)

But you can force discards on by doing a:

 # echo -n unmap > /sys/class/scsi_disk/x:y:z:n/provisioning_mode

or

 # echo -n writesame_16 > /sys/class/scsi_disk/x:y:z:n/provisioning_mode

Create a udev rule if you like.

In any case I wouldn't recommend using TRIM on that drive...


Bernd> PS: LSI SATL with FWv17 seems to have an unmap bug - I cannot
Bernd> unmap the last sector:

Yes, it appears there's an off-by-one bug in the UNMAP translation.
Sumit, is this something you guys can look into?

-- 
Martin K. Petersen  Oracle Linux Engineering
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: SCSI's heuristics for enabling WRITE SAME still need work [was: dm mpath: disable WRITE SAME if it fails]

[Bernd Schubert]

On 09/26/2013 04:42 PM, Martin K. Petersen wrote:

"Bernd" == Bernd Schubert  writes:


Bernd,

Bernd> Both types of systems we have in-house neither block limits vpd
Bernd> nor READ_CAP16 return anything that would indicate discard is
Bernd> supported. But UNMAP and WRITE SAME unmap(*) just work fine.

I have a collection of different SSDs in a tray connected to an LSI
SAS2008 ASIC. The 510 is the only drive that does not have LBPME=1.
Chances are it's because DRAT and RZAT are not set but it could also be
that the 510 is blacklisted.


Bernd> I certainly don't want to cause any more write-same trouble, but
Bernd> as all layers initially have to assume write same is supported
Bernd> anyway and need to dynamically disable it if it fails, can't we
Bernd> also enable discard by default with WRITE SAME16 unmap?

No thanks :)

But you can force discards on by doing a:

  # echo -n unmap > /sys/class/scsi_disk/x:y:z:n/provisioning_mode

or

  # echo -n writesame_16 > /sys/class/scsi_disk/x:y:z:n/provisioning_mode


D'oh, I didn't know about this parameter! THANKS a lot!



Create a udev rule if you like.


Definitely, we have rules anyway.



In any case I wouldn't recommend using TRIM on that drive...


Hmm yeah, the problem is that that these SSDs are used in our compute 
nodes (but we as file system group can also sometimes use it for 
benchmarking) and these SSDs slow down rather quickly without trim. The 
current procedure is to remove everything, to sg_unmap the full device 
and to re-create anything. So anything that would allow us to do it on 
the fly is very welcome. I don't think we need DRAT and RZAT, as we only 
use raid0 for these nodes.


So thanks a lot for your hint about the provisioning_mode!




Bernd> PS: LSI SATL with FWv17 seems to have an unmap bug - I cannot
Bernd> unmap the last sector:

Yes, it appears there's an off-by-one bug in the UNMAP translation.
Sumit, is this something you guys can look into?




Thanks again,
Bernd


--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: SCSI's heuristics for enabling WRITE SAME still need work [was: dm mpath: disable WRITE SAME if it fails]

[Douglas Gilbert]

On 13-09-26 10:42 AM, Martin K. Petersen wrote:

"Bernd" == Bernd Schubert  writes:


Bernd,

Bernd> Both types of systems we have in-house neither block limits vpd
Bernd> nor READ_CAP16 return anything that would indicate discard is
Bernd> supported. But UNMAP and WRITE SAME unmap(*) just work fine.

I have a collection of different SSDs in a tray connected to an LSI
SAS2008 ASIC. The 510 is the only drive that does not have LBPME=1.
Chances are it's because DRAT and RZAT are not set but it could also be
that the 510 is blacklisted.


Bernd> I certainly don't want to cause any more write-same trouble, but
Bernd> as all layers initially have to assume write same is supported
Bernd> anyway and need to dynamically disable it if it fails, can't we
Bernd> also enable discard by default with WRITE SAME16 unmap?

No thanks :)

But you can force discards on by doing a:

  # echo -n unmap > /sys/class/scsi_disk/x:y:z:n/provisioning_mode

or

  # echo -n writesame_16 > /sys/class/scsi_disk/x:y:z:n/provisioning_mode

Create a udev rule if you like.

In any case I wouldn't recommend using TRIM on that drive...


Bernd> PS: LSI SATL with FWv17 seems to have an unmap bug - I cannot
Bernd> unmap the last sector:

Yes, it appears there's an off-by-one bug in the UNMAP translation.
Sumit, is this something you guys can look into?


Hi Sumit,
While you are looking at the HBA firmware could you fix
this minor annoyance:

# lsscsi -H -t
.
[6]mpt3sas   sas:0x500605b006d3b510

# smp_rep_general /dev/mpt3ctl -s 0x500605b006d3b510
Report general response:
  expander change count: 0
  expander route indexes: 0
  long response: 0
  number of phys: 0
  zone configuring: 0
  self configuring: 0
  externally configurable route table: 0
  initiates SSP close: 0
  enclosure logical identifier (hex): b005065010b5d306

So that is a SMP REPORT GENERAL (RG) directed at the HBA itself.
So either my code is incorrectly decoding the "enclosure logical
identifier" or ... . That field was defined in SAS 1.1 so no
excuses on that front. 0x0 would be better than a shuffled
version of the HBA's own SAS address.

Same bug/feature in all versions of SAS-2 and now SAS-3 firmware
that I have tried.

Doug Gilbert


BTW The bsg driver can send a RG to the HBA like this:
 smp_rep_general /dev/bsg/sas_host6
So the intent is clearer and the response is the same.




--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH RESEND] iscsi_tcp: consider session state in iscsi_sw_sk_state_check

[Chris Leech]

It seems some iSCSI targets (including the Linux kernel target) close
the TCP connection from the target side immediately after processing a
session logout.

When a TCP FIN comes in right after the iSCSI logout response,
iscsi_sw_sk_state_check sees the local socket as not yet being in
CLOSE_WAIT or CLOSE and logs an error.  But the initiator would close
the connection right after processing the logout response anyway, and
the error is confusing to admins who just requested that the session be
shut down.

This adds a check of the session state, and suppresses the error if we
are in the process of logging out.

Signed-off-by: Chris Leech 
Reviewed-by: Mike Christie 
---
 drivers/scsi/iscsi_tcp.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/scsi/iscsi_tcp.c b/drivers/scsi/iscsi_tcp.c
index 9e2588a..add6d15 100644
--- a/drivers/scsi/iscsi_tcp.c
+++ b/drivers/scsi/iscsi_tcp.c
@@ -116,6 +116,7 @@ static inline int iscsi_sw_sk_state_check(struct sock *sk)
struct iscsi_conn *conn = sk->sk_user_data;
 
if ((sk->sk_state == TCP_CLOSE_WAIT || sk->sk_state == TCP_CLOSE) &&
+   (conn->session->state != ISCSI_STATE_LOGGING_OUT) &&
!atomic_read(&sk->sk_rmem_alloc)) {
ISCSI_SW_TCP_DBG(conn, "TCP_CLOSE|TCP_CLOSE_WAIT\n");
iscsi_conn_failure(conn, ISCSI_ERR_TCP_CONN_CLOSE);
-- 
1.8.3.1

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Race condition between "read CFQ stats" and "block device shutdown"

[Anatol Pomozov]

Hi

On Thu, Sep 26, 2013 at 7:18 AM, Hannes Reinecke  wrote:
> On 09/26/2013 03:54 PM, Tejun Heo wrote:
>> Hello, (cc'ing linux-scsi)
>>
>> On Wed, Sep 25, 2013 at 01:37:51PM -0700, Anatol Pomozov wrote:
>>> Hi
>>>
>>> On Wed, Sep 4, 2013 at 9:07 AM, Tejun Heo  wrote:
 Hello,

 On Wed, Sep 04, 2013 at 08:45:33AM -0700, Anatol Pomozov wrote:
> I am not an expect in block code, so I have a few questions here:
>
> - are we sure that this operation is atomic? What if blkg->q becomes
> dead right after we checked it, and blkg->q->queue_lock got invalid so
> we have the same crash as before?

 request_queue lock switching is something inherently broken in block
 layer.  It's unsalvageable.
>>>
>>> Fully agree. The problem that request_queue->queue_lock is a shared
>>> resource that concurrently modified/accessed. In this case (when one
>>> thread changes, another thread access it) we need synchronization to
>>> prevent race conditions. So we need a spin_lock to access queue_lock
>>> spin_lock, otherwise we have a crash like one above...
>>>
  Maybe we can drop lock switching once blk-mq is fully merged.
>>>
>>> Could you please provide more information about it? What is the timeline?
>>
>> I have no idea.  Hopefully, not too far out.  Jens would have better
>> idea.
>>
>>> If there is an easy way to fix the race condition I would like to
>>> help. Please give me some pointer what direction I should move.
>>
>> The first step would be identifying who are actually making use of
>> lock switching, why and how much difference it would make for them to
>> not do that.
>>
> Typically, the lock is being used by the block drivers to
> synchronize access between some internal data structures and the
> request queue itself. You don't actually _need_ to do it that way,
> but removing the lock switching would involve quite some redesign of
> these drivers.
> Give that most of the are rather oldish I really wouldn't want to
> touch them.

Thanks for the info.

We use modified version of "sbull" block device driver from GKH book.
We use it for testing block device startup/shutdown path + CFQ
manipulation.

The sbull driver uses function
blk_init_queue(..., &dev->qlock);

it passes lock as a second parameter and function makes the lock
swapping. According to your information passing lock to
blk_init_queue() considered as an "old non recommended way" so we
should modify our driver and avoid doing this. I'll take a look.

Thanks.


>
> However, none of the modern devices should be using this lock
> switching, so I would just ignore it.
> EG SCSI most definitely doesn't use it.
>
> Cheers,
>
> Hannes
> --
> Dr. Hannes Reinecke   zSeries & Storage
> h...@suse.de  +49 911 74053 688
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
> GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Race condition between "read CFQ stats" and "block device shutdown"

[Tejun Heo]

Hello,

On Thu, Sep 26, 2013 at 09:23:19AM -0700, Anatol Pomozov wrote:
> We use modified version of "sbull" block device driver from GKH book.
> We use it for testing block device startup/shutdown path + CFQ
> manipulation.
> 
> The sbull driver uses function
> blk_init_queue(..., &dev->qlock);
> 
> it passes lock as a second parameter and function makes the lock
> swapping. According to your information passing lock to
> blk_init_queue() considered as an "old non recommended way" so we
> should modify our driver and avoid doing this. I'll take a look.

I wonder whether we can make request_queue record the passed in lock
and grab it in addition to the queue lock instead of replacing it when
invoking the request function.  This would mean more overhead for the
broken drivers but given how legacy they are at this point, I don't
think that's gonna be a big problem.  if that's likely to work, we may
as well just push all the extra locking to the legacy drivers so that
their request functions take the extra lock and then we can remove the
spinlock argument completely.  I'm not sure whether the locking in
request path is the only thing necessary tho.

Thanks.

-- 
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] sg: fix memory leaks

[Douglas Gilbert]

This patch plugs two memory leaks in the sg driver. The
first was reported by Vegard Nossum and the second was
found by checking for the same pattern.

ChangeLog:
   - close two heap memory leaks. The first is an error
 path; the second is associated with getting debug
 information from procfs and sysfs.

The patch is generated from lk 3.12.0-rc2 .


Signed-off-by: Douglas Gilbert 

diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index 5cbc4bb..aa86276 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -2060,6 +2060,7 @@ sg_add_sfp(Sg_device * sdp, int dev)
 	spin_lock_irqsave(&sdp->sfd_lock, iflags);
 	if (sdp->detached) {
 		spin_unlock_irqrestore(&sdp->sfd_lock, iflags);
+		kfree(sfp);
 		return ERR_PTR(-ENODEV);
 	}
 	list_add_tail(&sfp->sfd_siblings, &sdp->sfds);
@@ -2433,8 +2434,10 @@ static void * dev_seq_start(struct seq_file *s, loff_t *pos)
 
 	it->index = *pos;
 	it->max = sg_last_dev();
-	if (it->index >= it->max)
+	if (it->index >= it->max) {
+		kfree(it);
 		return NULL;
+	}
 	return it;
 }
 

RE: SCSI's heuristics for enabling WRITE SAME still need work [was: dm mpath: disable WRITE SAME if it fails]

[Saxena, Sumit]

>-Original Message-
>From: Douglas Gilbert [mailto:dgilb...@interlog.com]
>Sent: Thursday, September 26, 2013 9:17 PM
>To: Martin K. Petersen; Bernd Schubert
>Cc: Mike Snitzer; Hannes Reinecke; emi...@redhat.com; device-mapper
>development; linux-scsi@vger.kernel.org; Saxena, Sumit
>Subject: Re: SCSI's heuristics for enabling WRITE SAME still need work
>[was: dm mpath: disable WRITE SAME if it fails]
>
>On 13-09-26 10:42 AM, Martin K. Petersen wrote:
>>> "Bernd" == Bernd Schubert  writes:
>>
>> Bernd,
>>
>> Bernd> Both types of systems we have in-house neither block limits vpd
>> Bernd> nor READ_CAP16 return anything that would indicate discard is
>> Bernd> supported. But UNMAP and WRITE SAME unmap(*) just work fine.
>>
>> I have a collection of different SSDs in a tray connected to an LSI
>> SAS2008 ASIC. The 510 is the only drive that does not have LBPME=1.
>> Chances are it's because DRAT and RZAT are not set but it could also
>> be that the 510 is blacklisted.
>>
>>
>> Bernd> I certainly don't want to cause any more write-same trouble,
>> Bernd> but as all layers initially have to assume write same is
>> Bernd> supported anyway and need to dynamically disable it if it
>> Bernd> fails, can't we also enable discard by default with WRITE
>SAME16 unmap?
>>
>> No thanks :)
>>
>> But you can force discards on by doing a:
>>
>>   # echo -n unmap > /sys/class/scsi_disk/x:y:z:n/provisioning_mode
>>
>> or
>>
>>   # echo -n writesame_16 >
>> /sys/class/scsi_disk/x:y:z:n/provisioning_mode
>>
>> Create a udev rule if you like.
>>
>> In any case I wouldn't recommend using TRIM on that drive...
>>
>>
>> Bernd> PS: LSI SATL with FWv17 seems to have an unmap bug - I cannot
>> Bernd> unmap the last sector:
>>
>> Yes, it appears there's an off-by-one bug in the UNMAP translation.
>> Sumit, is this something you guys can look into?
>
>Hi Sumit,
>While you are looking at the HBA firmware could you fix this minor
>annoyance:
>

Thibash, Can you please look at this?

Sumit 

># lsscsi -H -t
>.
>[6]mpt3sas   sas:0x500605b006d3b510
>
># smp_rep_general /dev/mpt3ctl -s 0x500605b006d3b510 Report general
>response:
>   expander change count: 0
>   expander route indexes: 0
>   long response: 0
>   number of phys: 0
>   zone configuring: 0
>   self configuring: 0
>   externally configurable route table: 0
>   initiates SSP close: 0
>   enclosure logical identifier (hex): b005065010b5d306
>
>So that is a SMP REPORT GENERAL (RG) directed at the HBA itself.
>So either my code is incorrectly decoding the "enclosure logical
>identifier" or ... . That field was defined in SAS 1.1 so no excuses on
>that front. 0x0 would be better than a shuffled version of the HBA's own
>SAS address.
>
>Same bug/feature in all versions of SAS-2 and now SAS-3 firmware that I
>have tried.
>
>Doug Gilbert
>
>
>BTW The bsg driver can send a RG to the HBA like this:
>  smp_rep_general /dev/bsg/sas_host6
> So the intent is clearer and the response is the same.
>
>
>
>


--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 00/51] DMA mask changes

[Rafał Miłecki]

2013/9/19 Russell King - ARM Linux :
> This email is only being sent to the mailing lists in question, not to
> anyone personally.  The list of individuals is far to great to do that.
> I'm hoping no mailing lists reject the patches based on the number of
> recipients.

Huh, I think it was enough to send only 3 patches to the b43-dev@. Like:
[PATCH 01/51] DMA-API: provide a helper to set both DMA and coherent DMA masks
[PATCH 14/51] DMA-API: net: b43: (...)
[PATCH 15/51] DMA-API: net: b43legacy: (...)
;)

I believe Joe has some nice script for doing it that way. When fixing
some coding style / formatting, he sends only related patches to the
given ML.

-- 
Rafał
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 31/51] DMA-API: media: omap3isp: use dma_coerce_mask_and_coherent()

[Laurent Pinchart]

Hi Russell,

Thank you for the patch.

On Thursday 19 September 2013 22:56:02 Russell King wrote:
> The code sequence:
>   isp->raw_dmamask = DMA_BIT_MASK(32);
>   isp->dev->dma_mask = &isp->raw_dmamask;
>   isp->dev->coherent_dma_mask = DMA_BIT_MASK(32);
> bypasses the architectures check on the DMA mask.  It can be replaced
> with dma_coerce_mask_and_coherent(), avoiding the direct initialization
> of this mask.
> 
> Signed-off-by: Russell King 

Acked-by: Laurent Pinchart 

> ---
>  drivers/media/platform/omap3isp/isp.c |6 +++---
>  drivers/media/platform/omap3isp/isp.h |3 ---
>  2 files changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/platform/omap3isp/isp.c
> b/drivers/media/platform/omap3isp/isp.c index df3a0ec..1c36080 100644
> --- a/drivers/media/platform/omap3isp/isp.c
> +++ b/drivers/media/platform/omap3isp/isp.c
> @@ -2182,9 +2182,9 @@ static int isp_probe(struct platform_device *pdev)
>   isp->pdata = pdata;
>   isp->ref_count = 0;
> 
> - isp->raw_dmamask = DMA_BIT_MASK(32);
> - isp->dev->dma_mask = &isp->raw_dmamask;
> - isp->dev->coherent_dma_mask = DMA_BIT_MASK(32);
> + ret = dma_coerce_mask_and_coherent(isp->dev, DMA_BIT_MASK(32));
> + if (ret)
> + return ret;
> 
>   platform_set_drvdata(pdev, isp);
> 
> diff --git a/drivers/media/platform/omap3isp/isp.h
> b/drivers/media/platform/omap3isp/isp.h index cd3eff4..ce65d3a 100644
> --- a/drivers/media/platform/omap3isp/isp.h
> +++ b/drivers/media/platform/omap3isp/isp.h
> @@ -152,7 +152,6 @@ struct isp_xclk {
>   * @mmio_base_phys: Array with physical L4 bus addresses for ISP register
>   *  regions.
>   * @mmio_size: Array with ISP register regions size in bytes.
> - * @raw_dmamask: Raw DMA mask
>   * @stat_lock: Spinlock for handling statistics
>   * @isp_mutex: Mutex for serializing requests to ISP.
>   * @crashed: Bitmask of crashed entities (indexed by entity ID)
> @@ -190,8 +189,6 @@ struct isp_device {
>   unsigned long mmio_base_phys[OMAP3_ISP_IOMEM_LAST];
>   resource_size_t mmio_size[OMAP3_ISP_IOMEM_LAST];
> 
> - u64 raw_dmamask;
> -
>   /* ISP Obj */
>   spinlock_t stat_lock;   /* common lock for statistic drivers */
>   struct mutex isp_mutex; /* For handling ref_count field */
-- 
Regards,

Laurent Pinchart

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Race condition between "read CFQ stats" and "block device shutdown"

[Hannes Reinecke]

On 09/26/2013 06:23 PM, Anatol Pomozov wrote:
> Hi
> 
> On Thu, Sep 26, 2013 at 7:18 AM, Hannes Reinecke  wrote:
>> On 09/26/2013 03:54 PM, Tejun Heo wrote:
>>> Hello, (cc'ing linux-scsi)
>>>
>>> On Wed, Sep 25, 2013 at 01:37:51PM -0700, Anatol Pomozov wrote:
 Hi

 On Wed, Sep 4, 2013 at 9:07 AM, Tejun Heo  wrote:
> Hello,
>
> On Wed, Sep 04, 2013 at 08:45:33AM -0700, Anatol Pomozov wrote:
>> I am not an expect in block code, so I have a few questions here:
>>
>> - are we sure that this operation is atomic? What if blkg->q becomes
>> dead right after we checked it, and blkg->q->queue_lock got invalid so
>> we have the same crash as before?
>
> request_queue lock switching is something inherently broken in block
> layer.  It's unsalvageable.

 Fully agree. The problem that request_queue->queue_lock is a shared
 resource that concurrently modified/accessed. In this case (when one
 thread changes, another thread access it) we need synchronization to
 prevent race conditions. So we need a spin_lock to access queue_lock
 spin_lock, otherwise we have a crash like one above...

>  Maybe we can drop lock switching once blk-mq is fully merged.

 Could you please provide more information about it? What is the timeline?
>>>
>>> I have no idea.  Hopefully, not too far out.  Jens would have better
>>> idea.
>>>
 If there is an easy way to fix the race condition I would like to
 help. Please give me some pointer what direction I should move.
>>>
>>> The first step would be identifying who are actually making use of
>>> lock switching, why and how much difference it would make for them to
>>> not do that.
>>>
>> Typically, the lock is being used by the block drivers to
>> synchronize access between some internal data structures and the
>> request queue itself. You don't actually _need_ to do it that way,
>> but removing the lock switching would involve quite some redesign of
>> these drivers.
>> Give that most of the are rather oldish I really wouldn't want to
>> touch them.
> 
> Thanks for the info.
> 
> We use modified version of "sbull" block device driver from GKH book.
> We use it for testing block device startup/shutdown path + CFQ
> manipulation.
> 
> The sbull driver uses function
> blk_init_queue(..., &dev->qlock);
> 
> it passes lock as a second parameter and function makes the lock
> swapping. According to your information passing lock to
> blk_init_queue() considered as an "old non recommended way" so we
> should modify our driver and avoid doing this. I'll take a look.
> 
Yep. I would recommend to use the queue_lock directly whenever you
need to pull requests off the request_queue, and use your own lock
to protect any internal structures associated with handling the
request internally.

Yes, this _might_ involve some lock dancing between the queue_lock
and the internal lock, but has the advantage that the internal lock
typically it used far more often than the queue_lock itself.
So you might even get some speed advantage here as you reduce
contention on the queue_lock.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke   zSeries & Storage
h...@suse.de  +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Bypass block layer and Fill SCSI lower layer driver queue

[Vladislav Bolkhovitin]

Douglas Gilbert, on 09/18/2013 07:07 AM wrote:
> On 13-09-18 03:58 AM, Jack Wang wrote:
>> On 09/18/2013 08:41 AM, Alireza Haghdoost wrote:
>>> Hi
>>>
>>> I am working on a high throughput and low latency application which
>>> does not tolerate block layer overhead to send IO request directly to
>>> fiber channel lower layer SCSI driver. I used to work with libaio but
>>> currently I am looking for a way to by pass the block layer and send
>>> SCSI commands from the application layer directly to the SCSI driver
>>> using /dev/sgX device and ioctl() system call.
>>>
>>> I have noticed that sending IO request through sg device even with
>>> nonblocking and direct IO flags is quite slow and does not fill up
>>> lower layer SCSI driver TCQ queue. i.e IO depth or
>>> /sys/block/sdX/in_flight is always ZERO. Therefore the application
>>> throughput is even lower that sending IO request through block layer
>>> with libaio and io_submit() system call. In both cases I used only one
>>> IO context (or fd) and single threaded.
>>>
>> Hi Alireza,
>>
>> I think what you want is in_flight command scsi dispatch to low level
>> device.
>> I submit a simple patch to export device_busy
>>
>> http://www.spinics.net/lists/linux-scsi/msg68697.html
>>
>> I also notice fio sg engine will not fill queue properly, but haven't
>> look into deeper.
>>
>> Cheers
>> Jack
>>
>>> I have noticed that some well known benchmarking tools like fio does
>>> not support IO depth for sg devices as well. Therefore, I was
>>> wondering if it is feasible to bypass block layer and achieve higher
>>> throughput and lower latency (for sending IO request only).
>>>
>>>
>>> Any comment on my issue is highly appreciated.
> 
> I'm not sure if this is relevant to your problem but by
> default both the bsg and sg drivers "queue at head"
> when they inject SCSI commands into the block layer.
> 
> The bsg driver has a BSG_FLAG_Q_AT_TAIL flag to change
> that queueing to what may be preferable for your purposes.
> The sg driver could, but does not, support that flag.

Just curious, for how long this counterproductive insert in head is going to 
stay? I
guess, now (almost) nobody can recall why it is so. This behavior makes sg 
interface,
basically, unusable for anything bigger, than sg-utils.

Vlad
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html