[PATCH 1/2] scsi_transport_fc: FC pass through support via bsg interface - revised

[PATCH 1/2] scsi_transport_fc: FC pass through support via bsg interface - revised

[Seokmann Ju]

Signed-off-by: Seokmann Ju <seokmann.ju@qlogic.com>
---
diff --git a/drivers/scsi/qla2xxx/qla_attr.c b/drivers/scsi/qla2xxx/ 
qla_attr.c
index 45e7dcb..d240da9 100644
--- a/drivers/scsi/qla2xxx/qla_attr.c
+++ b/drivers/scsi/qla2xxx/qla_attr.c
@@ -1230,6 +1230,234 @@ qla24xx_vport_disable(struct fc_vport  
*fc_vport, bool disable)
  	return 0;
  }

+static int
+qla2x00_execute_fc_service(struct fc_service *service)
+{
+	struct fc_rport *rport = service->rport;
+	fc_port_t *fcport = *(fc_port_t **) rport->dd_data;
+	struct Scsi_Host *host = rport_to_shost(rport);
+	scsi_qla_host_t *ha = shost_priv(host);
+	struct els_entry_24xx *els_iocb;
+	struct ct_entry_24xx *ct_iocb;
+	request_t *iocb;
+	srb_t *sp;
+	unsigned long flags;
+	uint32_t  handle;
+	int  index, sg_cnt;
+	uint8_t  *els_pkt = service->payload;
+	uint16_t avail_dsds;
+	uint32_t *cur_dsd;
+
+	if (rport->port_state != FC_PORTSTATE_ONLINE || !fcport)
+		goto pt_error0;
+
+	if ((service->srv_request->request_type != FC_FRAME_TYPE_ELS) &&
+	    (service->srv_request->request_type != FC_FRAME_TYPE_FC_CT))
+		goto pt_error0;
+
+	/* ELS doesn't support multiple SG */
+	if ((service->srv_request->request_type == FC_FRAME_TYPE_ELS) &&
+	    (service->payload_sg_cnt > 1 || service->response_sg_cnt > 1)) {
+		printk(KERN_WARNING "ERROR: multiple SG of payload/response "
+		    "[%u/%u]  are not supported for ELS services\n",
+		    service->payload_sg_cnt, service->response_sg_cnt);
+		service->srv_reply.status = FC_SERVICE_UNSUPPORTED;
+		goto pt_error0;
+	}
+
+	sp = mempool_alloc(ha->srb_mempool, GFP_ATOMIC);
+	if (!sp)
+		goto pt_error0;
+
+	sp->ha = ha;
+	sp->fcport = fcport;
+	sp->cmd = (struct scsi_cmnd *)service;
+	sp->flags = SRB_ELS_CT;
+
+	spin_lock_irqsave(&ha->hardware_lock, flags);
+	iocb = qla2x00_req_pkt(ha);
+	if (iocb == NULL) {
+		qla_printk(KERN_WARNING, ha,
+		    "Passthru request failed to get request packet\n");
+		goto pt_error1;
+	}
+
+	/* Check for room in outstanding command list. */
+	handle = ha->current_outstanding_cmd;
+	for (index = 1; index < MAX_OUTSTANDING_COMMANDS; index++) {
+		handle++;
+		if (handle == MAX_OUTSTANDING_COMMANDS)
+			handle = 1;
+		if (!ha->outstanding_cmds[handle])
+			break;
+	}
+
+	ha->current_outstanding_cmd = handle;
+	ha->outstanding_cmds[handle] = (srb_t *) sp;
+
+	iocb->handle = handle;
+
+	service->payload_sg_cnt = dma_map_sg(&rport->dev,
+	    service->payload_dma, service->payload_sg_cnt, DMA_TO_DEVICE);
+	service->response_sg_cnt = dma_map_sg(&rport->dev,
+	    service->response_dma, service->response_sg_cnt, DMA_FROM_DEVICE);
+
+	if (service->srv_request->request_type == FC_FRAME_TYPE_ELS) {
+		els_iocb = (struct els_entry_24xx *)iocb;
+		els_iocb->entry_type = ELS_IOCB_TYPE;
+		els_iocb->entry_count = 1;
+		els_iocb->sys_define = 0;
+		els_iocb->entry_status = 0;
+		els_iocb->nport_handle = cpu_to_le16(fcport->loop_id);
+		els_iocb->tx_dsd_count = __constant_cpu_to_le16(1);
+		els_iocb->vp_index = ha->vp_idx;
+		els_iocb->sof_type = EST_SOFI3;
+		els_iocb->rx_dsd_count = __constant_cpu_to_le16(1);
+		els_iocb->opcode = els_pkt[0];
+		els_iocb->port_id[0] = fcport->d_id.b.al_pa;
+		els_iocb->port_id[1] = fcport->d_id.b.area;
+		els_iocb->port_id[2] = fcport->d_id.b.domain;
+		els_iocb->control_flags = 0;
+		els_iocb->rx_byte_count = cpu_to_le32(service->response_size);
+		els_iocb->tx_byte_count = cpu_to_le32(service->payload_size);
+		els_iocb->tx_address[0] = cpu_to_le32(LSD(sg_dma_address
+		    (&service->payload_dma[0])));
+		els_iocb->tx_address[1] = cpu_to_le32(MSD(sg_dma_address
+		    (&service->payload_dma[0])));
+
+		els_iocb->tx_len = cpu_to_le32(sg_dma_len
+		    (&service->payload_dma[0]));
+
+		els_iocb->rx_address[0] = cpu_to_le32(LSD(sg_dma_address
+		    (&service->response_dma[0])));
+		els_iocb->rx_address[1] = cpu_to_le32(MSD(sg_dma_address
+		    (&service->response_dma[0])));
+
+		els_iocb->rx_len = cpu_to_le32(sg_dma_len
+		    (&service->response_dma[0]));
+	
+		/* lld_pkt is only for ELS services in case of abort */
+		service->lld_pkt = (void *) els_iocb;
+	} else {
+		ct_iocb = (struct ct_entry_24xx *)iocb;
+		ct_iocb->entry_type = CT_IOCB_TYPE;
+		ct_iocb->entry_count = 1;
+		ct_iocb->entry_status = 0;
+		ct_iocb->comp_status = __constant_cpu_to_le16(0);
+		if (IS_FWI2_CAPABLE(ha))
+			ct_iocb->nport_handle = cpu_to_le16(NPH_SNS);
+		else
+			ct_iocb->nport_handle =
+			    cpu_to_le16(SIMPLE_NAME_SERVER);
+		ct_iocb->cmd_dsd_count =
+		    __constant_cpu_to_le16(service->payload_sg_cnt);
+		ct_iocb->vp_index = ha->vp_idx;
+		ct_iocb->timeout =
+		    __constant_cpu_to_le16(service->timer.expires);
+		ct_iocb->rsp_dsd_count =
+		    __constant_cpu_to_le16(service->response_sg_cnt);
+		ct_iocb->rsp_byte_count =
+		    cpu_to_le32(service->response_size);
+		ct_iocb->cmd_byte_count =
+		    cpu_to_le32(service->payload_size);
+		ct_iocb->dseg_0_address[0] = cpu_to_le32(LSD(sg_dma_address
+		    (&service->payload_dma[0])));
+		ct_iocb->dseg_0_address[1] = cpu_to_le32(MSD(sg_dma_address
+		    (&service->payload_dma[0])));
+
+		ct_iocb->dseg_0_len = cpu_to_le32(sg_dma_len
+		    (&service->payload_dma[0]));
+
+		sg_cnt = service->response_sg_cnt;
+		avail_dsds = 1;
+		cur_dsd = (uint32_t *)ct_iocb->dseg_1_address;
+		index = 0;
+		while (sg_cnt) {
+			cont_a64_entry_t *cont_pkt;
+
+			/* Allocate additional continuation packets? */
+			if (avail_dsds == 0) {
+				/*
+				 * Five DSDs are available in the Continuation
+				 * Type 1 IOCB.
+				 */
+				cont_pkt = qla2x00_prep_cont_type1_iocb(ha);
+				cur_dsd =
+				    (uint32_t *) cont_pkt->dseg_0_address;
+				avail_dsds = 5;
+			}
+
+			*cur_dsd++ = cpu_to_le32(LSD(sg_dma_address
+			    (&service->response_dma[index])));
+			*cur_dsd++ = cpu_to_le32(MSD(sg_dma_address
+			    (&service->response_dma[index])));
+			*cur_dsd++ = cpu_to_le32(sg_dma_len
+			    (&service->response_dma[index]));
+			avail_dsds--;
+			sg_cnt--;
+			index++;
+		}
+{
+	int i;
+	ushort *tmp = ct_iocb;
+	for (i=0;i<sizeof (struct ct_entry_24xx) / 2; i++)
+		printk("%4x\n", tmp[i]);
+}
+	}
+
+	wmb();
+	qla2x00_isp_cmd(ha);
+
+	spin_unlock_irqrestore(&ha->hardware_lock, flags);
+	return 0;
+pt_error1:
+	spin_unlock_irqrestore(&ha->hardware_lock, flags);
+pt_error0:
+	qla_printk(KERN_WARNING, ha, "Passthru ELS failed\n");
+
+	return -EINVAL;
+}
+
+static int
+qla2x00_abort_fc_service(struct fc_service *service)
+{
+	unsigned long flags;
+	struct fc_rport *rport = service->rport;
+	struct Scsi_Host *host = rport_to_shost(rport);
+	scsi_qla_host_t *ha = shost_priv(host);
+	struct abort_entry_24xx *abort_iocb;
+	struct els_entry_24xx *els_iocb = service->lld_pkt;
+
+	printk("ELS/CT: DEBUG: %s is called...\n", __func__);
+	if (service->srv_request->request_type != FC_FRAME_TYPE_ELS) {
+		printk(KERN_ERR "abort frame is not supported\n");
+		return -EINVAL;
+	}
+
+	spin_lock_irqsave(&ha->hardware_lock, flags);
+	abort_iocb = (struct abort_entry_24xx *)qla2x00_req_pkt(ha);
+
+	abort_iocb->entry_type = ABORT_IOCB_TYPE;
+	abort_iocb->entry_count = 1;
+	abort_iocb->handle_count = 0;
+	abort_iocb->entry_status = 0;
+	abort_iocb->options = 0;
+	abort_iocb->handle_to_abort = els_iocb->handle;
+	abort_iocb->port_id[0] = els_iocb->port_id[0];
+	abort_iocb->port_id[1] = els_iocb->port_id[1];
+	abort_iocb->port_id[2] = els_iocb->port_id[2];
+	abort_iocb->vp_index = ha->vp_idx;
+
+	wmb();
+	qla2x00_isp_cmd(ha);
+
+	spin_unlock_irqrestore(&ha->hardware_lock, flags);
+
+	service->service_state_flag = FC_SERVICE_STATE_ABORTED;
+
+	return 0;
+}
+
  struct fc_function_template qla2xxx_transport_functions = {

  	.show_host_node_name = 1,
@@ -1273,6 +1501,9 @@ struct fc_function_template  
qla2xxx_transport_functions = {
  	.vport_create = qla24xx_vport_create,
  	.vport_disable = qla24xx_vport_disable,
  	.vport_delete = qla24xx_vport_delete,
+
+	.execute_fc_service = qla2x00_execute_fc_service,
+	.abort_fc_service = qla2x00_abort_fc_service,
  };

  struct fc_function_template qla2xxx_transport_vport_functions = {
@@ -1313,6 +1544,9 @@ struct fc_function_template  
qla2xxx_transport_vport_functions = {
  	.dev_loss_tmo_callbk = qla2x00_dev_loss_tmo_callbk,
  	.terminate_rport_io = qla2x00_terminate_rport_io,
  	.get_fc_host_stats = qla2x00_get_fc_host_stats,
+
+	.execute_fc_service = qla2x00_execute_fc_service,
+	.abort_fc_service = qla2x00_abort_fc_service,
  };

  void
diff --git a/drivers/scsi/qla2xxx/qla_def.h b/drivers/scsi/qla2xxx/ 
qla_def.h
index 431c19e..c5510db 100644
--- a/drivers/scsi/qla2xxx/qla_def.h
+++ b/drivers/scsi/qla2xxx/qla_def.h
@@ -93,6 +93,10 @@
  #define LSD(x)	((uint32_t)((uint64_t)(x)))
  #define MSD(x)	((uint32_t)((((uint64_t)(x)) >> 16) >> 16))

+/* ELS PT request buffer = 32 bytes */
+#define EXT_ELS_PT_REQ_WWPN_VALID	0x1
+#define EXT_ELS_PT_REQ_WWNN_VALID	0x2
+#define EXT_ELS_PT_REQ_PID_VALID	0x4

  /*
   * I/O register
@@ -213,6 +217,7 @@ typedef struct srb {
  #define SRB_FO_CANCEL		BIT_9	/* Command don't need to do failover */
  #define SRB_IOCTL		BIT_10	/* IOCTL command. */
  #define SRB_TAPE		BIT_11	/* FCP2 (Tape) command. */
+#define SRB_ELS_CT		BIT_12	/* Link/Generic Service requests */

  /*
   * ISP I/O Register Set structure definitions.
@@ -2422,6 +2427,8 @@ typedef struct scsi_qla_host {
  	/* SNS command interfaces for 2200. */
  	struct sns_cmd_pkt	*sns_cmd;
  	dma_addr_t		sns_cmd_dma;
+	char			*pass_thru;
+	dma_addr_t		pass_thru_dma;

  #define SFP_DEV_SIZE	256
  #define SFP_BLOCK_SIZE	64
@@ -2463,6 +2470,7 @@ typedef struct scsi_qla_host {
  	struct mutex vport_lock;	/* Virtual port synchronization */
  	struct completion mbx_cmd_comp;	/* Serialize mbx access */
  	struct completion mbx_intr_comp;  /* Used for completion  
notification */
+	struct completion pass_thru_intr_comp;

  	uint32_t	mbx_flags;
  #define  MBX_IN_PROGRESS	BIT_0
diff --git a/drivers/scsi/qla2xxx/qla_fw.h b/drivers/scsi/qla2xxx/ 
qla_fw.h
index cf19451..481ab56 100644
--- a/drivers/scsi/qla2xxx/qla_fw.h
+++ b/drivers/scsi/qla2xxx/qla_fw.h
@@ -625,6 +625,41 @@ struct els_entry_24xx {
  	uint32_t rx_len;		/* Data segment 1 length. */
  };

+struct els_status_24xx {
+	uint8_t entry_type;		/* Entry type. */
+	uint8_t entry_count;		/* Entry count. */
+	uint8_t sys_define;		/* System Defined. */
+	uint8_t entry_status;		/* Entry Status. */
+
+	uint32_t handle;		/* System handle. */
+
+	uint16_t comp_status;
+
+	uint16_t nport_handle;		/* N_PORT handle. */
+
+	uint16_t tx_dsd_count;
+
+	uint8_t vp_index;
+	uint8_t sof_type;
+
+	uint32_t rx_xchg_address;	/* Receive exchange address. */
+	uint16_t rx_dsd_count;
+
+	uint8_t opcode;
+	uint8_t reserved_2;
+
+	uint8_t port_id[3];
+	uint8_t reserved_3;
+
+	uint16_t reserved_4;
+
+	uint16_t control_flags;		/* Control flags. */
+
+	uint32_t total_byte_count;
+	uint32_t err_subcode1;
+	uint32_t err_subcode2;
+};
+
  /*
   * ISP queue - Mailbox Command entry structure definition.
   */
diff --git a/drivers/scsi/qla2xxx/qla_gbl.h b/drivers/scsi/qla2xxx/ 
qla_gbl.h
index 0b15673..0295d39 100644
--- a/drivers/scsi/qla2xxx/qla_gbl.h
+++ b/drivers/scsi/qla2xxx/qla_gbl.h
@@ -121,6 +121,9 @@ extern int qla2x00_start_scsi(srb_t *sp);
  extern int qla24xx_start_scsi(srb_t *sp);
  int qla2x00_marker(scsi_qla_host_t *, uint16_t, uint16_t, uint8_t);
  int __qla2x00_marker(scsi_qla_host_t *, uint16_t, uint16_t, uint8_t);
+extern request_t * qla2x00_req_pkt(scsi_qla_host_t *);
+extern void qla2x00_isp_cmd(scsi_qla_host_t *ha);
+extern cont_a64_entry_t *qla2x00_prep_cont_type1_iocb(scsi_qla_host_t  
*);

  /*
   * Global Function Prototypes in qla_mbx.c source file.
@@ -345,6 +348,7 @@ extern int qla2x00_fdmi_register(scsi_qla_host_t *);
  extern int qla2x00_gfpn_id(scsi_qla_host_t *, sw_info_t *);
  extern int qla2x00_gpsc(scsi_qla_host_t *, sw_info_t *);
  extern void qla2x00_get_sym_node_name(scsi_qla_host_t *, uint8_t *);
+extern int qla2x00_mgmt_svr_login(scsi_qla_host_t *);

  /*
   * Global Function Prototypes in qla_attr.c source file.
diff --git a/drivers/scsi/qla2xxx/qla_gs.c b/drivers/scsi/qla2xxx/ 
qla_gs.c
index c2a4bfb..ceef231 100644
--- a/drivers/scsi/qla2xxx/qla_gs.c
+++ b/drivers/scsi/qla2xxx/qla_gs.c
@@ -1093,7 +1093,7 @@ qla2x00_sns_rnn_id(scsi_qla_host_t *ha)
   *
   * Returns 0 on success.
   */
-static int
+int
  qla2x00_mgmt_svr_login(scsi_qla_host_t *ha)
  {
  	int ret;
diff --git a/drivers/scsi/qla2xxx/qla_init.c b/drivers/scsi/qla2xxx/ 
qla_init.c
index ee89ddd..4956a9b 100644
--- a/drivers/scsi/qla2xxx/qla_init.c
+++ b/drivers/scsi/qla2xxx/qla_init.c
@@ -2386,6 +2386,18 @@ qla2x00_configure_fabric(scsi_qla_host_t *ha)
  			return (QLA_SUCCESS);
  		}

+		/* allocate fc_port_t structure for name/directory server */
+		fcptemp = qla2x00_alloc_fcport(ha, GFP_KERNEL);
+		fcptemp->flags |= (FCF_FABRIC_DEVICE | FCF_LOGIN_NEEDED);
+		fcptemp->loop_id = loop_id;
+		fcptemp->d_id.b.domain = 0xff;
+		fcptemp->d_id.b.area = 0xff;
+		fcptemp->d_id.b.al_pa = 0xfc;
+		list_add_tail(&fcptemp->list, &pha->fcports);
+		atomic_set(&fcptemp->state, FCS_ONLINE);
+		qla2x00_reg_remote_port(ha, fcptemp);
+
+
  		if (test_and_clear_bit(REGISTER_FC4_NEEDED, &ha->dpc_flags)) {
  			if (qla2x00_rft_id(ha)) {
  				/* EMPTY */
diff --git a/drivers/scsi/qla2xxx/qla_iocb.c b/drivers/scsi/qla2xxx/ 
qla_iocb.c
index d57669a..45e227e 100644
--- a/drivers/scsi/qla2xxx/qla_iocb.c
+++ b/drivers/scsi/qla2xxx/qla_iocb.c
@@ -11,8 +11,6 @@

  #include <scsi/scsi_tcq.h>

-static request_t *qla2x00_req_pkt(scsi_qla_host_t *ha);
-static void qla2x00_isp_cmd(scsi_qla_host_t *ha);

  /**
   * qla2x00_get_cmd_direction() - Determine control_flag data  
direction.
@@ -114,7 +112,7 @@ qla2x00_prep_cont_type0_iocb(scsi_qla_host_t *ha)
   *
   * Returns a pointer to the continuation type 1 IOCB packet.
   */
-static inline cont_a64_entry_t *
+inline cont_a64_entry_t *
  qla2x00_prep_cont_type1_iocb(scsi_qla_host_t *ha)
  {
  	cont_a64_entry_t *cont_pkt;
@@ -471,7 +469,7 @@ qla2x00_marker(scsi_qla_host_t *ha, uint16_t  
loop_id, uint16_t lun,
   *
   * Returns NULL if function failed, else, a pointer to the request  
packet.
   */
-static request_t *
+request_t *
  qla2x00_req_pkt(scsi_qla_host_t *ha)
  {
  	device_reg_t __iomem *reg = ha->iobase;
@@ -541,7 +539,7 @@ qla2x00_req_pkt(scsi_qla_host_t *ha)
   *
   * Note: The caller must hold the hardware lock before calling this  
routine.
   */
-static void
+void
  qla2x00_isp_cmd(scsi_qla_host_t *ha)
  {
  	device_reg_t __iomem *reg = ha->iobase;
diff --git a/drivers/scsi/qla2xxx/qla_isr.c b/drivers/scsi/qla2xxx/ 
qla_isr.c
index 45a3b93..1b11358 100644
--- a/drivers/scsi/qla2xxx/qla_isr.c
+++ b/drivers/scsi/qla2xxx/qla_isr.c
@@ -1398,6 +1398,9 @@ qla24xx_process_response_queue(struct  
scsi_qla_host *ha)
  {
  	struct device_reg_24xx __iomem *reg = &ha->iobase->isp24;
  	struct sts_entry_24xx *pkt;
+	struct els_status_24xx *els_status;
+	struct fc_service *service;
+	srb_t *sp;

  	if (!ha->flags.online)
  		return;
@@ -1430,10 +1433,72 @@ qla24xx_process_response_queue(struct  
scsi_qla_host *ha)
  		case STATUS_CONT_TYPE:
  			qla2x00_status_cont_entry(ha, (sts_cont_entry_t *)pkt);
  			break;
+ 		case MS_IOCB_TYPE:
+		case ELS_IOCB_TYPE:
+			if (pkt->handle < MAX_OUTSTANDING_COMMANDS) {
+				sp = ha->outstanding_cmds[pkt->handle];
+				service = (struct fc_service *) sp->cmd;
+				ha->outstanding_cmds[pkt->handle] = NULL;
+			} else {
+				service = NULL;
+				break;
+			}
+
+			service->service_state_flag = FC_SERVICE_STATE_DONE;
+			service->srv_reply.status = FC_SERVICE_COMPLETE;
+			service->srv_reply.error_code = pkt->comp_status;
+
+			if (pkt->comp_status) {
+				printk("ELS/CT: comp_status = %x\n", pkt->comp_status);
+				els_status = (struct els_status_24xx *)pkt;
+				service->srv_reply.residual = 0;
+				if (pkt->comp_status == CS_DATA_UNDERRUN)
+					service->srv_reply.residual =
+					    els_status->total_byte_count;
+				else if (pkt->comp_status == CS_ABORTED) {
+					service->service_state_flag =
+					    FC_SERVICE_STATE_ABORTED;
+					service->srv_reply.status =
+					    FC_SERVICE_ABORT;
+				} else
+				service->srv_reply.status = FC_SERVICE_ERROR;
+			}
+
+			dma_unmap_sg(&ha->pdev->dev, service->payload_dma,
+			    service->payload_sg_cnt, DMA_TO_DEVICE);
+			dma_unmap_sg(&ha->pdev->dev, service->response_dma,
+			    service->response_sg_cnt, DMA_FROM_DEVICE);
+
+			mempool_free(sp, ha->srb_mempool);
+			service->service_done(service);
+ 			break;
  		case VP_RPT_ID_IOCB_TYPE:
  			qla24xx_report_id_acquisition(ha,
  			    (struct vp_rpt_id_entry_24xx *)pkt);
  			break;
+		case ABORT_IOCB_TYPE:
+			if (pkt->comp_status) {
+				if (pkt->handle < MAX_OUTSTANDING_COMMANDS) {
+					sp = ha->outstanding_cmds[pkt->handle];
+					ha->outstanding_cmds[pkt->handle] =
+					    NULL;
+					if (sp->flags & SRB_ELS_CT) {
+						service = (struct fc_service *)
+						    sp->cmd;
+						qla_printk(KERN_ERR, ha,
+						    "failed to abort"
+						    " FC service %p\n",
+						    service);
+
+					} else
+						qla_printk(KERN_ERR, ha,
+						    "failed to abort"
+						    " SCSI CMD[0] = %2x\n",
+						    sp->cmd->cmnd[0]);
+				}
+			}
+			
+			break;
  		default:
  			/* Type Not Supported. */
  			DEBUG4(printk(KERN_WARNING
diff --git a/drivers/scsi/qla2xxx/qla_os.c b/drivers/scsi/qla2xxx/ 
qla_os.c
index 26afe44..7e1f027 100644
--- a/drivers/scsi/qla2xxx/qla_os.c
+++ b/drivers/scsi/qla2xxx/qla_os.c
@@ -1672,6 +1672,8 @@ qla2x00_probe_one(struct pci_dev *pdev, const  
struct pci_device_id *id)
  	init_completion(&ha->mbx_cmd_comp);
  	complete(&ha->mbx_cmd_comp);
  	init_completion(&ha->mbx_intr_comp);
+	init_completion(&ha->pass_thru_intr_comp);
+

  	INIT_LIST_HEAD(&ha->list);
  	INIT_LIST_HEAD(&ha->fcports);
@@ -2020,6 +2022,8 @@ qla2x00_mem_alloc(scsi_qla_host_t *ha)
  		    sizeof(struct ct_sns_pkt), &ha->ct_sns_dma, GFP_KERNEL);
  		if (!ha->ct_sns)
  			goto fail_free_ms_iocb;
+		ha->pass_thru = dma_alloc_coherent(&ha->pdev->dev,
+		    PAGE_SIZE, &ha->pass_thru_dma, GFP_KERNEL);
  	}

  	return 0;
---

Re: [PATCH 1/2] scsi_transport_fc: FC pass through support via bsg interface - revised

[Seokmann Ju]

On Oct 13, 2008, at 10:53 AM, Seokmann Ju wrote:

> Signed-off-by: Seokmann Ju <seokmann.ju@qlogic.com>
> ---
By mistake, I placed patches so that it won't match with the subject.
Sorry for the shortcomings.
Here is the patch for scsi_transport_fc module.

Signed-off-by: Seokmann Ju <seokmann.ju@qlogic.com>
---
diff --git a/drivers/scsi/scsi_transport_fc.c b/drivers/scsi/ 
scsi_transport_fc.c
index cb971f0..dda12e0 100644
--- a/drivers/scsi/scsi_transport_fc.c
+++ b/drivers/scsi/scsi_transport_fc.c
@@ -43,6 +43,11 @@ static void fc_vport_sched_delete(struct  
work_struct *work);
  static int fc_vport_setup(struct Scsi_Host *shost, int channel,
  	struct device *pdev, struct fc_vport_identifiers  *ids,
  	struct fc_vport **vport);
+static void fc_service_timeout(unsigned long);
+static void fc_service_done(struct fc_service *);
+static int fc_service_handler(struct Scsi_Host *, struct fc_rport *,
+    struct request *, struct request_queue *);
+static void fc_bsg_remove(struct Scsi_Host *, struct fc_rport *);

  /*
   * Redefine so that we can have same named attributes in the
@@ -2406,12 +2411,194 @@ fc_rport_final_delete(struct work_struct *work)

  	transport_remove_device(dev);
  	device_del(dev);
+	fc_bsg_remove(shost, rport);
  	transport_destroy_device(dev);
  	put_device(&shost->shost_gendev);	/* for fc_host->rport list */
  	put_device(dev);			/* for self-reference */
  }


+static void fc_service_timeout(unsigned long _service)
+{
+	struct fc_service *service = (void *) _service;
+	unsigned long flags;
+
+	spin_lock_irqsave(&service->service_state_lock, flags);
+	if (!(service->service_state_flag & FC_SERVICE_STATE_DONE))
+		service->service_state_flag |= FC_SERVICE_STATE_TIMEOUT;
+	spin_unlock_irqrestore(&service->service_state_lock, flags);
+}
+
+static void fc_service_done(struct fc_service *service)
+{
+	if (service->service_state_flag != FC_SERVICE_STATE_DONE) {
+		if (service->service_state_flag == FC_SERVICE_STATE_TIMEOUT) {
+			printk(KERN_ERR "ERROR: FC service timed out\n");
+			/* expect abort to be issued by the application */
+			return;
+		} else
+			printk(KERN_ERR "ERROR: FC service not finished\n");
+	}
+
+	if (service->srv_reply.status != FC_SERVICE_COMPLETE) {
+		printk(KERN_ERR "ERROR: FC service to rport %p failed with"
+		    " status 0x%x\n", service->rport,
+		    service->srv_reply.status);
+	}
+
+	if (service->srv_reply.residual) {
+		service->req->data_len = 0;
+		service->req->next_rq->data_len = service->srv_reply.residual;
+	} else {
+		service->req->data_len = 0;
+		service->req->next_rq->data_len = 0;
+	}
+
+	service->req->end_io(service->req, 0);
+	kfree(service);
+}
+
+static int
+issue_fc_service(struct fc_rport *rport, struct request *req,
+    struct request *rsp, struct request_queue *q)
+{
+	int res = -ECOMM;
+	struct fc_service *service;
+	struct Scsi_Host *shost = rport_to_shost(rport);
+	struct fc_internal *i = to_fc_internal(shost->transportt);
+	struct fc_service_request *srv_request = (struct fc_service_request *)
+	    req->cmd;
+
+	service = kzalloc(sizeof (struct fc_service), GFP_KERNEL);
+	if (!service)
+		return -ENOMEM;
+
+	service->rport = rport;
+	service->req = req;
+	service->q = q;
+	service->srv_request = srv_request;
+
+	sg_init_table(service->payload_dma, FC_SERVICE_MAX_SG);
+	service->payload_sg_cnt =
+	    blk_rq_map_sg(q, req, service->payload_dma);
+	service->payload = (void *) bio_data(req->bio);
+	service->payload_size = req->data_len;
+
+	sg_init_table(service->response_dma, FC_SERVICE_MAX_SG);
+	service->response_sg_cnt =
+	    blk_rq_map_sg(q, rsp, service->response_dma);
+	service->response = bio_data(rsp->bio);
+	service->response_size = rsp->data_len;
+
+	/* sense area of the request structure */
+	service->reply_seq = req->sense;
+	service->service_done = fc_service_done;
+	service->service_state_flag = FC_SERVICE_STATE_PENDING;
+
+	if (i->f->execute_fc_service)
+		res = i->f->execute_fc_service(service);
+
+	if (res) {
+		printk(KERN_ERR "ERROR: issuing FC service to the LLD "
+		"failed with status %d\n", res);
+		fc_service_done(service);
+		return res;
+	}
+
+	return 0;
+}
+
+static int
+fc_service_handler(struct Scsi_Host *shost, struct fc_rport *rport,
+			  struct request *req, struct request_queue *q)
+{
+	int ret;
+	struct request *rsp = req->next_rq;
+
+	if (!rsp) {
+		printk(KERN_ERR "ERROR: space for a FC service"
+		   " response is missing\n");
+		return -EINVAL;
+	}
+
+	if ((req->bio->bi_vcnt > FC_SERVICE_MAX_SG) ||
+	    (rsp->bio->bi_vcnt > FC_SERVICE_MAX_SG)) {
+		printk(KERN_ERR "ERROR: a FC service"
+		    " supports no more than %d SGs\n", FC_SERVICE_MAX_SG);
+		return -EINVAL;
+	}
+
+	ret = issue_fc_service(rport, req, rsp, q);
+
+	return ret;
+}
+
+static void fc_service_dispatch(struct request_queue *q)
+{
+	struct request *req;
+	struct fc_rport *rport = q->queuedata;
+	struct Scsi_Host *shost = rport_to_shost(rport);
+
+	while (!blk_queue_plugged(q)) {
+		req = elv_next_request(q);
+		if (!req)
+			break;
+
+		blkdev_dequeue_request(req);
+		spin_unlock_irq(q->queue_lock);
+
+		fc_service_handler(shost, rport, req, q);
+
+		spin_lock_irq(q->queue_lock);
+	}
+}
+
+static int
+fc_bsg_initialize(struct Scsi_Host *shost, struct fc_rport *rport)
+{
+	struct request_queue *q;
+	int error;
+	struct device *dev;
+	const char *name;
+	void (*release)(struct device *);
+
+	if (!rport) {
+		printk(KERN_ERR "ERROR: rport is NULL\n");
+		return -ENOMEM;
+	}
+
+	q = blk_init_queue(fc_service_dispatch, NULL);
+	if (!q)
+		return -ENOMEM;
+
+	dev = &rport->dev;
+	name = dev->bus_id;
+	release = NULL;
+
+	error = bsg_register_queue(q, dev, name, release);
+	if (error) {
+		blk_cleanup_queue(q);
+		return -ENOMEM;
+	}
+
+	rport->q = q;
+	q->queuedata = rport;
+	queue_flag_set_unlocked(QUEUE_FLAG_BIDI, q);
+
+	return 0;
+}
+
+static void
+fc_bsg_remove(struct Scsi_Host *shost, struct fc_rport *rport)
+{
+	struct request_queue *q = rport->q;
+
+	if (!q)
+		return;
+
+	bsg_unregister_queue(q);
+}
+
  /**
   * fc_rport_create - allocates and creates a remote FC port.
   * @shost:	scsi host the remote port is connected to.
@@ -2471,8 +2658,8 @@ fc_rport_create(struct Scsi_Host *shost, int  
channel,
  	else
  		rport->scsi_target_id = -1;
  	list_add_tail(&rport->peers, &fc_host->rports);
-	get_device(&shost->shost_gendev);	/* for fc_host->rport list */

+	get_device(&shost->shost_gendev);	/* for fc_host->rport list */
  	spin_unlock_irqrestore(shost->host_lock, flags);

  	dev = &rport->dev;
@@ -2491,6 +2678,8 @@ fc_rport_create(struct Scsi_Host *shost, int  
channel,
  	transport_add_device(dev);
  	transport_configure_device(dev);

+	fc_bsg_initialize(shost, rport);
+
  	if (rport->roles & FC_PORT_ROLE_FCP_TARGET) {
  		/* initiate a scan of the target */
  		rport->flags |= FC_RPORT_SCAN_PENDING;
diff --git a/include/scsi/scsi_transport_fc.h b/include/scsi/ 
scsi_transport_fc.h
index 21018a4..6c6809a 100644
--- a/include/scsi/scsi_transport_fc.h
+++ b/include/scsi/scsi_transport_fc.h
@@ -33,7 +33,6 @@

  struct scsi_transport_template;

-
  /*
   * FC Port definitions - Following FC HBAAPI guidelines
   *
@@ -352,6 +351,7 @@ struct fc_rport {	/* aka fc_starget_attrs */
   	struct delayed_work fail_io_work;
   	struct work_struct stgt_delete_work;
  	struct work_struct rport_delete_work;
+	struct request_queue *q;
  } __attribute__((aligned(sizeof(unsigned long))));

  /* bit field values for struct fc_rport "flags" field: */
@@ -514,6 +514,81 @@ struct fc_host_attrs {
  	struct workqueue_struct *devloss_work_q;
  };

+#define FC_STATUS_BUF_SIZE 96
+
+enum fc_frame_type {
+	FC_FRAME_TYPE_BS,
+	FC_FRAME_TYPE_ELS,
+	FC_FRAME_TYPE_IEC = 4,
+	FC_FRAME_TYPE_IP,
+	FC_FRAME_TYPE_FCP = 8,
+	FC_FRAME_TYPE_GPP,
+	FC_FRAME_TYPE_FC_CT = 0x20,
+};
+
+enum fc_service_status {
+	FC_SERVICE_COMPLETE,
+	FC_SERVICE_TIMEOUT,
+	FC_SERVICE_ABORT,
+	FC_SERVICE_UNSUPPORTED,
+	FC_SERVICE_ERROR = -1,
+};
+
+#define FC_SERVICE_STATE_PENDING      1
+#define FC_SERVICE_STATE_DONE         2
+#define FC_SERVICE_STATE_ABORTED      4
+#define FC_SERVICE_STATE_TIMEOUT      8
+
+#define FC_SERVICE_TIMEOUT 10
+#define FC_SERVICE_MAX_SG             16
+
+/* request (CDB) structure of the sg_io_v4 */
+struct fc_service_request {
+	u8   request_type;
+	u8   timeout;
+	u8   reserved0;
+	u8   reserved1;
+};
+
+/* response  (request sense data) structure of the sg_io_v4 */
+struct fc_service_reply {
+	enum fc_service_status status;
+	u16  error_code;
+	u16  additional_error_code;
+	u32  residual;
+};
+
+struct fc_service {
+	struct fc_rport  *rport;
+	struct list_head list;
+
+	spinlock_t   service_state_lock;
+	unsigned     service_state_flag;
+
+	struct request *req;
+	struct request_queue *q;
+
+	/* Used by the discovery code. */
+	struct timer_list  timer;
+	struct completion  completion;
+
+	struct fc_service_request *srv_request;
+	void  *payload;
+	struct scatterlist payload_dma[FC_SERVICE_MAX_SG];
+	int   payload_sg_cnt;
+	int   payload_size;
+	void  *response;
+	struct scatterlist response_dma[FC_SERVICE_MAX_SG];
+	int   response_sg_cnt;
+	int   response_size;
+
+	struct fc_service_reply srv_reply;
+	void  *reply_seq;   /* pointer to sense area of request */
+	void  (*service_done)(struct fc_service *);
+
+	void *lld_pkt;
+};
+
  #define shost_to_fc_host(x) \
  	((struct fc_host_attrs *)(x)->shost_data)

@@ -612,6 +687,10 @@ struct fc_function_template {
  	int     (* tsk_mgmt_response)(struct Scsi_Host *, u64, u64, int);
  	int     (* it_nexus_response)(struct Scsi_Host *, u64, int);

+	/* fc service handler */
+	int	(*execute_fc_service)(struct fc_service *);
+	int	(*abort_fc_service)(struct fc_service *);
+
  	/* allocation lengths for host-specific data */
  	u32	 			dd_fcrport_size;
  	u32	 			dd_fcvport_size;
@@ -732,7 +811,6 @@ fc_vport_set_state(struct fc_vport *vport, enum  
fc_vport_state new_state)
  	vport->vport_state = new_state;
  }

-
  struct scsi_transport_template *fc_attach_transport(
  			struct fc_function_template *);
  void fc_release_transport(struct scsi_transport_template *);
---

Re: [PATCH 1/2] scsi_transport_fc: FC pass through support via bsg interface - revised

[FUJITA Tomonori]

On Mon, 13 Oct 2008 11:14:56 -0700
Seokmann Ju <seokmann.ju@qlogic.com> wrote:

> 
> On Oct 13, 2008, at 10:53 AM, Seokmann Ju wrote:
> 
> > Signed-off-by: Seokmann Ju <seokmann.ju@qlogic.com>
> > ---
> By mistake, I placed patches so that it won't match with the subject.
> Sorry for the shortcomings.
> Here is the patch for scsi_transport_fc module.
> 
> Signed-off-by: Seokmann Ju <seokmann.ju@qlogic.com>
> ---
> diff --git a/drivers/scsi/scsi_transport_fc.c b/drivers/scsi/ 
> scsi_transport_fc.c
> index cb971f0..dda12e0 100644
> --- a/drivers/scsi/scsi_transport_fc.c
> +++ b/drivers/scsi/scsi_transport_fc.c
> @@ -43,6 +43,11 @@ static void fc_vport_sched_delete(struct  
> work_struct *work);
>   static int fc_vport_setup(struct Scsi_Host *shost, int channel,
>   	struct device *pdev, struct fc_vport_identifiers  *ids,
>   	struct fc_vport **vport);
> +static void fc_service_timeout(unsigned long);
> +static void fc_service_done(struct fc_service *);
> +static int fc_service_handler(struct Scsi_Host *, struct fc_rport *,
> +    struct request *, struct request_queue *);
> +static void fc_bsg_remove(struct Scsi_Host *, struct fc_rport *);
> 
>   /*
>    * Redefine so that we can have same named attributes in the
> @@ -2406,12 +2411,194 @@ fc_rport_final_delete(struct work_struct *work)
> 
>   	transport_remove_device(dev);
>   	device_del(dev);
> +	fc_bsg_remove(shost, rport);
>   	transport_destroy_device(dev);
>   	put_device(&shost->shost_gendev);	/* for fc_host->rport list */
>   	put_device(dev);			/* for self-reference */
>   }
> 
> 
> +static void fc_service_timeout(unsigned long _service)
> +{
> +	struct fc_service *service = (void *) _service;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&service->service_state_lock, flags);
> +	if (!(service->service_state_flag & FC_SERVICE_STATE_DONE))
> +		service->service_state_flag |= FC_SERVICE_STATE_TIMEOUT;
> +	spin_unlock_irqrestore(&service->service_state_lock, flags);
> +}

Hmm, this function is not used? Using rq_timed_out_fn is todo?


> +static void fc_service_done(struct fc_service *service)
> +{
> +	if (service->service_state_flag != FC_SERVICE_STATE_DONE) {
> +		if (service->service_state_flag == FC_SERVICE_STATE_TIMEOUT) {
> +			printk(KERN_ERR "ERROR: FC service timed out\n");
> +			/* expect abort to be issued by the application */
> +			return;
> +		} else
> +			printk(KERN_ERR "ERROR: FC service not finished\n");
> +	}
> +
> +	if (service->srv_reply.status != FC_SERVICE_COMPLETE) {
> +		printk(KERN_ERR "ERROR: FC service to rport %p failed with"
> +		    " status 0x%x\n", service->rport,
> +		    service->srv_reply.status);
> +	}
> +
> +	if (service->srv_reply.residual) {
> +		service->req->data_len = 0;
> +		service->req->next_rq->data_len = service->srv_reply.residual;
> +	} else {
> +		service->req->data_len = 0;
> +		service->req->next_rq->data_len = 0;
> +	}
> +
> +	service->req->end_io(service->req, 0);
> +	kfree(service);
> +}
> +
> +static int
> +issue_fc_service(struct fc_rport *rport, struct request *req,
> +    struct request *rsp, struct request_queue *q)
> +{
> +	int res = -ECOMM;
> +	struct fc_service *service;
> +	struct Scsi_Host *shost = rport_to_shost(rport);
> +	struct fc_internal *i = to_fc_internal(shost->transportt);
> +	struct fc_service_request *srv_request = (struct fc_service_request *)
> +	    req->cmd;
> +
> +	service = kzalloc(sizeof (struct fc_service), GFP_KERNEL);
> +	if (!service)
> +		return -ENOMEM;
> +
> +	service->rport = rport;
> +	service->req = req;
> +	service->q = q;
> +	service->srv_request = srv_request;
> +
> +	sg_init_table(service->payload_dma, FC_SERVICE_MAX_SG);
> +	service->payload_sg_cnt =
> +	    blk_rq_map_sg(q, req, service->payload_dma);
> +	service->payload = (void *) bio_data(req->bio);

You use scatter-gather for data transfer so bio_data should not be
used.


> +	service->payload_size = req->data_len;
> +
> +	sg_init_table(service->response_dma, FC_SERVICE_MAX_SG);
> +	service->response_sg_cnt =
> +	    blk_rq_map_sg(q, rsp, service->response_dma);
> +	service->response = bio_data(rsp->bio);
> +	service->response_size = rsp->data_len;
> +
> +	/* sense area of the request structure */
> +	service->reply_seq = req->sense;
> +	service->service_done = fc_service_done;
> +	service->service_state_flag = FC_SERVICE_STATE_PENDING;
> +
> +	if (i->f->execute_fc_service)
> +		res = i->f->execute_fc_service(service);
> +
> +	if (res) {
> +		printk(KERN_ERR "ERROR: issuing FC service to the LLD "
> +		"failed with status %d\n", res);
> +		fc_service_done(service);
> +		return res;
> +	}
> +
> +	return 0;
> +}
> +
> +static int
> +fc_service_handler(struct Scsi_Host *shost, struct fc_rport *rport,
> +			  struct request *req, struct request_queue *q)
> +{
> +	int ret;
> +	struct request *rsp = req->next_rq;
> +
> +	if (!rsp) {
> +		printk(KERN_ERR "ERROR: space for a FC service"
> +		   " response is missing\n");
> +		return -EINVAL;
> +	}
> +
> +	if ((req->bio->bi_vcnt > FC_SERVICE_MAX_SG) ||
> +	    (rsp->bio->bi_vcnt > FC_SERVICE_MAX_SG)) {
> +		printk(KERN_ERR "ERROR: a FC service"
> +		    " supports no more than %d SGs\n", FC_SERVICE_MAX_SG);
> +		return -EINVAL;
> +	}

This doesn't look correct. bi_vcnt is not related with the number of
sg. You use scatter-gather for large data transfer. You don't need to
worry about bi_vcnt.


> +	ret = issue_fc_service(rport, req, rsp, q);
> +
> +	return ret;
> +}
> +
> +static void fc_service_dispatch(struct request_queue *q)
> +{
> +	struct request *req;
> +	struct fc_rport *rport = q->queuedata;
> +	struct Scsi_Host *shost = rport_to_shost(rport);
> +
> +	while (!blk_queue_plugged(q)) {
> +		req = elv_next_request(q);
> +		if (!req)
> +			break;
> +
> +		blkdev_dequeue_request(req);
> +		spin_unlock_irq(q->queue_lock);
> +
> +		fc_service_handler(shost, rport, req, q);
> +
> +		spin_lock_irq(q->queue_lock);
> +	}
> +}
> +
> +static int
> +fc_bsg_initialize(struct Scsi_Host *shost, struct fc_rport *rport)
> +{
> +	struct request_queue *q;
> +	int error;
> +	struct device *dev;
> +	const char *name;
> +	void (*release)(struct device *);
> +
> +	if (!rport) {
> +		printk(KERN_ERR "ERROR: rport is NULL\n");
> +		return -ENOMEM;
> +	}
> +
> +	q = blk_init_queue(fc_service_dispatch, NULL);
> +	if (!q)
> +		return -ENOMEM;
> +
> +	dev = &rport->dev;
> +	name = dev->bus_id;
> +	release = NULL;
> +
> +	error = bsg_register_queue(q, dev, name, release);
> +	if (error) {
> +		blk_cleanup_queue(q);
> +		return -ENOMEM;
> +	}

I think that you need to call blk_queue_max_hw_segments and
blk_queue_max_phys_segments here with FC_SERVICE_MAX_SG.


> +
> +	rport->q = q;
> +	q->queuedata = rport;
> +	queue_flag_set_unlocked(QUEUE_FLAG_BIDI, q);
> +
> +	return 0;
> +}
> +
> +static void
> +fc_bsg_remove(struct Scsi_Host *shost, struct fc_rport *rport)
> +{
> +	struct request_queue *q = rport->q;
> +
> +	if (!q)
> +		return;
> +
> +	bsg_unregister_queue(q);
> +}
> +
>   /**
>    * fc_rport_create - allocates and creates a remote FC port.
>    * @shost:	scsi host the remote port is connected to.
> @@ -2471,8 +2658,8 @@ fc_rport_create(struct Scsi_Host *shost, int  
> channel,
>   	else
>   		rport->scsi_target_id = -1;
>   	list_add_tail(&rport->peers, &fc_host->rports);
> -	get_device(&shost->shost_gendev);	/* for fc_host->rport list */
> 
> +	get_device(&shost->shost_gendev);	/* for fc_host->rport list */
>   	spin_unlock_irqrestore(shost->host_lock, flags);
> 
>   	dev = &rport->dev;
> @@ -2491,6 +2678,8 @@ fc_rport_create(struct Scsi_Host *shost, int  
> channel,
>   	transport_add_device(dev);
>   	transport_configure_device(dev);
> 
> +	fc_bsg_initialize(shost, rport);
> +
>   	if (rport->roles & FC_PORT_ROLE_FCP_TARGET) {
>   		/* initiate a scan of the target */
>   		rport->flags |= FC_RPORT_SCAN_PENDING;
> diff --git a/include/scsi/scsi_transport_fc.h b/include/scsi/ 
> scsi_transport_fc.h
> index 21018a4..6c6809a 100644
> --- a/include/scsi/scsi_transport_fc.h
> +++ b/include/scsi/scsi_transport_fc.h
> @@ -33,7 +33,6 @@
> 
>   struct scsi_transport_template;
> 
> -
>   /*
>    * FC Port definitions - Following FC HBAAPI guidelines
>    *
> @@ -352,6 +351,7 @@ struct fc_rport {	/* aka fc_starget_attrs */
>    	struct delayed_work fail_io_work;
>    	struct work_struct stgt_delete_work;
>   	struct work_struct rport_delete_work;
> +	struct request_queue *q;
>   } __attribute__((aligned(sizeof(unsigned long))));
> 
>   /* bit field values for struct fc_rport "flags" field: */
> @@ -514,6 +514,81 @@ struct fc_host_attrs {
>   	struct workqueue_struct *devloss_work_q;
>   };
> 
> +#define FC_STATUS_BUF_SIZE 96
> +
> +enum fc_frame_type {
> +	FC_FRAME_TYPE_BS,
> +	FC_FRAME_TYPE_ELS,
> +	FC_FRAME_TYPE_IEC = 4,
> +	FC_FRAME_TYPE_IP,
> +	FC_FRAME_TYPE_FCP = 8,
> +	FC_FRAME_TYPE_GPP,
> +	FC_FRAME_TYPE_FC_CT = 0x20,
> +};
> +
> +enum fc_service_status {
> +	FC_SERVICE_COMPLETE,
> +	FC_SERVICE_TIMEOUT,
> +	FC_SERVICE_ABORT,
> +	FC_SERVICE_UNSUPPORTED,
> +	FC_SERVICE_ERROR = -1,
> +};
> +
> +#define FC_SERVICE_STATE_PENDING      1
> +#define FC_SERVICE_STATE_DONE         2
> +#define FC_SERVICE_STATE_ABORTED      4
> +#define FC_SERVICE_STATE_TIMEOUT      8
> +
> +#define FC_SERVICE_TIMEOUT 10
> +#define FC_SERVICE_MAX_SG             16
> +
> +/* request (CDB) structure of the sg_io_v4 */
> +struct fc_service_request {
> +	u8   request_type;
> +	u8   timeout;
> +	u8   reserved0;
> +	u8   reserved1;
> +};
> +
> +/* response  (request sense data) structure of the sg_io_v4 */
> +struct fc_service_reply {
> +	enum fc_service_status status;
> +	u16  error_code;
> +	u16  additional_error_code;
> +	u32  residual;
> +};
> +
> +struct fc_service {
> +	struct fc_rport  *rport;
> +	struct list_head list;
> +
> +	spinlock_t   service_state_lock;
> +	unsigned     service_state_flag;
> +
> +	struct request *req;
> +	struct request_queue *q;
> +
> +	/* Used by the discovery code. */
> +	struct timer_list  timer;
> +	struct completion  completion;
> +
> +	struct fc_service_request *srv_request;
> +	void  *payload;
> +	struct scatterlist payload_dma[FC_SERVICE_MAX_SG];
> +	int   payload_sg_cnt;
> +	int   payload_size;
> +	void  *response;
> +	struct scatterlist response_dma[FC_SERVICE_MAX_SG];
> +	int   response_sg_cnt;
> +	int   response_size;
> +
> +	struct fc_service_reply srv_reply;
> +	void  *reply_seq;   /* pointer to sense area of request */
> +	void  (*service_done)(struct fc_service *);
> +
> +	void *lld_pkt;
> +};
> +
>   #define shost_to_fc_host(x) \
>   	((struct fc_host_attrs *)(x)->shost_data)
> 
> @@ -612,6 +687,10 @@ struct fc_function_template {
>   	int     (* tsk_mgmt_response)(struct Scsi_Host *, u64, u64, int);
>   	int     (* it_nexus_response)(struct Scsi_Host *, u64, int);
> 
> +	/* fc service handler */
> +	int	(*execute_fc_service)(struct fc_service *);
> +	int	(*abort_fc_service)(struct fc_service *);
> +
>   	/* allocation lengths for host-specific data */
>   	u32	 			dd_fcrport_size;
>   	u32	 			dd_fcvport_size;
> @@ -732,7 +811,6 @@ fc_vport_set_state(struct fc_vport *vport, enum  
> fc_vport_state new_state)
>   	vport->vport_state = new_state;
>   }
> 
> -
>   struct scsi_transport_template *fc_attach_transport(
>   			struct fc_function_template *);
>   void fc_release_transport(struct scsi_transport_template *);
> ---
> --
> 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

Re: [PATCH 1/2] scsi_transport_fc: FC pass through support via bsg interface - revised

[Seokmann Ju]

On Oct 13, 2008, at 7:22 PM, FUJITA Tomonori wrote:

> On Mon, 13 Oct 2008 11:14:56 -0700
> Seokmann Ju <seokmann.ju@qlogic.com> wrote:
>
>>
>> On Oct 13, 2008, at 10:53 AM, Seokmann Ju wrote:
>>
>>> Signed-off-by: Seokmann Ju <seokmann.ju@qlogic.com>
>>> ---
>> By mistake, I placed patches so that it won't match with the subject.
>> Sorry for the shortcomings.
>> Here is the patch for scsi_transport_fc module.
>>
>> Signed-off-by: Seokmann Ju <seokmann.ju@qlogic.com>
>> ---
>> diff --git a/drivers/scsi/scsi_transport_fc.c b/drivers/scsi/
>> scsi_transport_fc.c
>> index cb971f0..dda12e0 100644
>> --- a/drivers/scsi/scsi_transport_fc.c
>> +++ b/drivers/scsi/scsi_transport_fc.c
>> @@ -43,6 +43,11 @@ static void fc_vport_sched_delete(struct
>> work_struct *work);
>>  static int fc_vport_setup(struct Scsi_Host *shost, int channel,
>>  	struct device *pdev, struct fc_vport_identifiers  *ids,
>>  	struct fc_vport **vport);
>> +static void fc_service_timeout(unsigned long);
>> +static void fc_service_done(struct fc_service *);
>> +static int fc_service_handler(struct Scsi_Host *, struct fc_rport *,
>> +    struct request *, struct request_queue *);
>> +static void fc_bsg_remove(struct Scsi_Host *, struct fc_rport *);
>>
>>  /*
>>   * Redefine so that we can have same named attributes in the
>> @@ -2406,12 +2411,194 @@ fc_rport_final_delete(struct work_struct  
>> *work)
>>
>>  	transport_remove_device(dev);
>>  	device_del(dev);
>> +	fc_bsg_remove(shost, rport);
>>  	transport_destroy_device(dev);
>>  	put_device(&shost->shost_gendev);	/* for fc_host->rport list */
>>  	put_device(dev);			/* for self-reference */
>>  }
>>
>>
>> +static void fc_service_timeout(unsigned long _service)
>> +{
>> +	struct fc_service *service = (void *) _service;
>> +	unsigned long flags;
>> +
>> +	spin_lock_irqsave(&service->service_state_lock, flags);
>> +	if (!(service->service_state_flag & FC_SERVICE_STATE_DONE))
>> +		service->service_state_flag |= FC_SERVICE_STATE_TIMEOUT;
>> +	spin_unlock_irqrestore(&service->service_state_lock, flags);
>> +}
>
> Hmm, this function is not used? Using rq_timed_out_fn is todo?
Yes, timeout is not in place at this time.
That is what I'm working on.

>
>
>
>> +static void fc_service_done(struct fc_service *service)
>> +{
>> +	if (service->service_state_flag != FC_SERVICE_STATE_DONE) {
>> +		if (service->service_state_flag == FC_SERVICE_STATE_TIMEOUT) {
>> +			printk(KERN_ERR "ERROR: FC service timed out\n");
>> +			/* expect abort to be issued by the application */
>> +			return;
>> +		} else
>> +			printk(KERN_ERR "ERROR: FC service not finished\n");
>> +	}
>> +
>> +	if (service->srv_reply.status != FC_SERVICE_COMPLETE) {
>> +		printk(KERN_ERR "ERROR: FC service to rport %p failed with"
>> +		    " status 0x%x\n", service->rport,
>> +		    service->srv_reply.status);
>> +	}
>> +
>> +	if (service->srv_reply.residual) {
>> +		service->req->data_len = 0;
>> +		service->req->next_rq->data_len = service->srv_reply.residual;
>> +	} else {
>> +		service->req->data_len = 0;
>> +		service->req->next_rq->data_len = 0;
>> +	}
>> +
>> +	service->req->end_io(service->req, 0);
>> +	kfree(service);
>> +}
>> +
>> +static int
>> +issue_fc_service(struct fc_rport *rport, struct request *req,
>> +    struct request *rsp, struct request_queue *q)
>> +{
>> +	int res = -ECOMM;
>> +	struct fc_service *service;
>> +	struct Scsi_Host *shost = rport_to_shost(rport);
>> +	struct fc_internal *i = to_fc_internal(shost->transportt);
>> +	struct fc_service_request *srv_request = (struct  
>> fc_service_request *)
>> +	    req->cmd;
>> +
>> +	service = kzalloc(sizeof (struct fc_service), GFP_KERNEL);
>> +	if (!service)
>> +		return -ENOMEM;
>> +
>> +	service->rport = rport;
>> +	service->req = req;
>> +	service->q = q;
>> +	service->srv_request = srv_request;
>> +
>> +	sg_init_table(service->payload_dma, FC_SERVICE_MAX_SG);
>> +	service->payload_sg_cnt =
>> +	    blk_rq_map_sg(q, req, service->payload_dma);
>> +	service->payload = (void *) bio_data(req->bio);
>
> You use scatter-gather for data transfer so bio_data should not be
> used.
OK. Will be removed.

>
>
>
>> +	service->payload_size = req->data_len;
>> +
>> +	sg_init_table(service->response_dma, FC_SERVICE_MAX_SG);
>> +	service->response_sg_cnt =
>> +	    blk_rq_map_sg(q, rsp, service->response_dma);
>> +	service->response = bio_data(rsp->bio);
>> +	service->response_size = rsp->data_len;
>> +
>> +	/* sense area of the request structure */
>> +	service->reply_seq = req->sense;
>> +	service->service_done = fc_service_done;
>> +	service->service_state_flag = FC_SERVICE_STATE_PENDING;
>> +
>> +	if (i->f->execute_fc_service)
>> +		res = i->f->execute_fc_service(service);
>> +
>> +	if (res) {
>> +		printk(KERN_ERR "ERROR: issuing FC service to the LLD "
>> +		"failed with status %d\n", res);
>> +		fc_service_done(service);
>> +		return res;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int
>> +fc_service_handler(struct Scsi_Host *shost, struct fc_rport *rport,
>> +			  struct request *req, struct request_queue *q)
>> +{
>> +	int ret;
>> +	struct request *rsp = req->next_rq;
>> +
>> +	if (!rsp) {
>> +		printk(KERN_ERR "ERROR: space for a FC service"
>> +		   " response is missing\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	if ((req->bio->bi_vcnt > FC_SERVICE_MAX_SG) ||
>> +	    (rsp->bio->bi_vcnt > FC_SERVICE_MAX_SG)) {
>> +		printk(KERN_ERR "ERROR: a FC service"
>> +		    " supports no more than %d SGs\n", FC_SERVICE_MAX_SG);
>> +		return -EINVAL;
>> +	}
>
> This doesn't look correct. bi_vcnt is not related with the number of
> sg. You use scatter-gather for large data transfer. You don't need to
> worry about bi_vcnt.
I see...
Is there is a way to check, then, how many SG entries the service  
needs before the blk_rq_map_sg()?
Having an 'payload_dma/response_dma' as array type seems not right,  
too..

>
>
>
>> +	ret = issue_fc_service(rport, req, rsp, q);
>> +
>> +	return ret;
>> +}
>> +
>> +static void fc_service_dispatch(struct request_queue *q)
>> +{
>> +	struct request *req;
>> +	struct fc_rport *rport = q->queuedata;
>> +	struct Scsi_Host *shost = rport_to_shost(rport);
>> +
>> +	while (!blk_queue_plugged(q)) {
>> +		req = elv_next_request(q);
>> +		if (!req)
>> +			break;
>> +
>> +		blkdev_dequeue_request(req);
>> +		spin_unlock_irq(q->queue_lock);
>> +
>> +		fc_service_handler(shost, rport, req, q);
>> +
>> +		spin_lock_irq(q->queue_lock);
>> +	}
>> +}
>> +
>> +static int
>> +fc_bsg_initialize(struct Scsi_Host *shost, struct fc_rport *rport)
>> +{
>> +	struct request_queue *q;
>> +	int error;
>> +	struct device *dev;
>> +	const char *name;
>> +	void (*release)(struct device *);
>> +
>> +	if (!rport) {
>> +		printk(KERN_ERR "ERROR: rport is NULL\n");
>> +		return -ENOMEM;
>> +	}
>> +
>> +	q = blk_init_queue(fc_service_dispatch, NULL);
>> +	if (!q)
>> +		return -ENOMEM;
>> +
>> +	dev = &rport->dev;
>> +	name = dev->bus_id;
>> +	release = NULL;
>> +
>> +	error = bsg_register_queue(q, dev, name, release);
>> +	if (error) {
>> +		blk_cleanup_queue(q);
>> +		return -ENOMEM;
>> +	}
>
> I think that you need to call blk_queue_max_hw_segments and
> blk_queue_max_phys_segments here with FC_SERVICE_MAX_SG.
>
>
>> +
>> +	rport->q = q;
>> +	q->queuedata = rport;
>> +	queue_flag_set_unlocked(QUEUE_FLAG_BIDI, q);
>> +
>> +	return 0;
>> +}
>> +
>> +static void
>> +fc_bsg_remove(struct Scsi_Host *shost, struct fc_rport *rport)
>> +{
>> +	struct request_queue *q = rport->q;
>> +
>> +	if (!q)
>> +		return;
>> +
>> +	bsg_unregister_queue(q);
>> +}
>> +
>>  /**
>>   * fc_rport_create - allocates and creates a remote FC port.
>>   * @shost:	scsi host the remote port is connected to.
>> @@ -2471,8 +2658,8 @@ fc_rport_create(struct Scsi_Host *shost, int
>> channel,
>>  	else
>>  		rport->scsi_target_id = -1;
>>  	list_add_tail(&rport->peers, &fc_host->rports);
>> -	get_device(&shost->shost_gendev);	/* for fc_host->rport list */
>>
>> +	get_device(&shost->shost_gendev);	/* for fc_host->rport list */
>>  	spin_unlock_irqrestore(shost->host_lock, flags);
>>
>>  	dev = &rport->dev;
>> @@ -2491,6 +2678,8 @@ fc_rport_create(struct Scsi_Host *shost, int
>> channel,
>>  	transport_add_device(dev);
>>  	transport_configure_device(dev);
>>
>> +	fc_bsg_initialize(shost, rport);
>> +
>>  	if (rport->roles & FC_PORT_ROLE_FCP_TARGET) {
>>  		/* initiate a scan of the target */
>>  		rport->flags |= FC_RPORT_SCAN_PENDING;
>> diff --git a/include/scsi/scsi_transport_fc.h b/include/scsi/
>> scsi_transport_fc.h
>> index 21018a4..6c6809a 100644
>> --- a/include/scsi/scsi_transport_fc.h
>> +++ b/include/scsi/scsi_transport_fc.h
>> @@ -33,7 +33,6 @@
>>
>>  struct scsi_transport_template;
>>
>> -
>>  /*
>>   * FC Port definitions - Following FC HBAAPI guidelines
>>   *
>> @@ -352,6 +351,7 @@ struct fc_rport {	/* aka fc_starget_attrs */
>>   	struct delayed_work fail_io_work;
>>   	struct work_struct stgt_delete_work;
>>  	struct work_struct rport_delete_work;
>> +	struct request_queue *q;
>>  } __attribute__((aligned(sizeof(unsigned long))));
>>
>>  /* bit field values for struct fc_rport "flags" field: */
>> @@ -514,6 +514,81 @@ struct fc_host_attrs {
>>  	struct workqueue_struct *devloss_work_q;
>>  };
>>
>> +#define FC_STATUS_BUF_SIZE 96
>> +
>> +enum fc_frame_type {
>> +	FC_FRAME_TYPE_BS,
>> +	FC_FRAME_TYPE_ELS,
>> +	FC_FRAME_TYPE_IEC = 4,
>> +	FC_FRAME_TYPE_IP,
>> +	FC_FRAME_TYPE_FCP = 8,
>> +	FC_FRAME_TYPE_GPP,
>> +	FC_FRAME_TYPE_FC_CT = 0x20,
>> +};
>> +
>> +enum fc_service_status {
>> +	FC_SERVICE_COMPLETE,
>> +	FC_SERVICE_TIMEOUT,
>> +	FC_SERVICE_ABORT,
>> +	FC_SERVICE_UNSUPPORTED,
>> +	FC_SERVICE_ERROR = -1,
>> +};
>> +
>> +#define FC_SERVICE_STATE_PENDING      1
>> +#define FC_SERVICE_STATE_DONE         2
>> +#define FC_SERVICE_STATE_ABORTED      4
>> +#define FC_SERVICE_STATE_TIMEOUT      8
>> +
>> +#define FC_SERVICE_TIMEOUT 10
>> +#define FC_SERVICE_MAX_SG             16
>> +
>> +/* request (CDB) structure of the sg_io_v4 */
>> +struct fc_service_request {
>> +	u8   request_type;
>> +	u8   timeout;
>> +	u8   reserved0;
>> +	u8   reserved1;
>> +};
>> +
>> +/* response  (request sense data) structure of the sg_io_v4 */
>> +struct fc_service_reply {
>> +	enum fc_service_status status;
>> +	u16  error_code;
>> +	u16  additional_error_code;
>> +	u32  residual;
>> +};
>> +
>> +struct fc_service {
>> +	struct fc_rport  *rport;
>> +	struct list_head list;
>> +
>> +	spinlock_t   service_state_lock;
>> +	unsigned     service_state_flag;
>> +
>> +	struct request *req;
>> +	struct request_queue *q;
>> +
>> +	/* Used by the discovery code. */
>> +	struct timer_list  timer;
>> +	struct completion  completion;
>> +
>> +	struct fc_service_request *srv_request;
>> +	void  *payload;
>> +	struct scatterlist payload_dma[FC_SERVICE_MAX_SG];
>> +	int   payload_sg_cnt;
>> +	int   payload_size;
>> +	void  *response;
>> +	struct scatterlist response_dma[FC_SERVICE_MAX_SG];
>> +	int   response_sg_cnt;
>> +	int   response_size;
>> +
>> +	struct fc_service_reply srv_reply;
>> +	void  *reply_seq;   /* pointer to sense area of request */
>> +	void  (*service_done)(struct fc_service *);
>> +
>> +	void *lld_pkt;
>> +};
>> +
>>  #define shost_to_fc_host(x) \
>>  	((struct fc_host_attrs *)(x)->shost_data)
>>
>> @@ -612,6 +687,10 @@ struct fc_function_template {
>>  	int     (* tsk_mgmt_response)(struct Scsi_Host *, u64, u64, int);
>>  	int     (* it_nexus_response)(struct Scsi_Host *, u64, int);
>>
>> +	/* fc service handler */
>> +	int	(*execute_fc_service)(struct fc_service *);
>> +	int	(*abort_fc_service)(struct fc_service *);
>> +
>>  	/* allocation lengths for host-specific data */
>>  	u32	 			dd_fcrport_size;
>>  	u32	 			dd_fcvport_size;
>> @@ -732,7 +811,6 @@ fc_vport_set_state(struct fc_vport *vport, enum
>> fc_vport_state new_state)
>>  	vport->vport_state = new_state;
>>  }
>>
>> -
>>  struct scsi_transport_template *fc_attach_transport(
>>  			struct fc_function_template *);
>>  void fc_release_transport(struct scsi_transport_template *);
>> ---
>> --
>> 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

Re: [PATCH 1/2] scsi_transport_fc: FC pass through support via bsg interface - revised

[FUJITA Tomonori]

On Tue, 14 Oct 2008 04:44:17 -0700
Seokmann Ju <seokmann.ju@qlogic.com> wrote:

> >> +static int
> >> +fc_service_handler(struct Scsi_Host *shost, struct fc_rport *rport,
> >> +			  struct request *req, struct request_queue *q)
> >> +{
> >> +	int ret;
> >> +	struct request *rsp = req->next_rq;
> >> +
> >> +	if (!rsp) {
> >> +		printk(KERN_ERR "ERROR: space for a FC service"
> >> +		   " response is missing\n");
> >> +		return -EINVAL;
> >> +	}
> >> +
> >> +	if ((req->bio->bi_vcnt > FC_SERVICE_MAX_SG) ||
> >> +	    (rsp->bio->bi_vcnt > FC_SERVICE_MAX_SG)) {
> >> +		printk(KERN_ERR "ERROR: a FC service"
> >> +		    " supports no more than %d SGs\n", FC_SERVICE_MAX_SG);
> >> +		return -EINVAL;
> >> +	}
> >
> > This doesn't look correct. bi_vcnt is not related with the number of
> > sg. You use scatter-gather for large data transfer. You don't need to
> > worry about bi_vcnt.
> I see...
> Is there is a way to check, then, how many SG entries the service  
> needs before the blk_rq_map_sg()?

As I wrote in the previous mail, via blk_queue_max_hw_segments() and
blk_queue_max_phys_segments(), you can tell the block layer the number
of sg segments you can handle.

Re: [PATCH 1/2] scsi_transport_fc: FC pass through support via bsg interface - revised

[Seokmann Ju]

On Oct 14, 2008, at 6:34 AM, FUJITA Tomonori wrote:

> On Tue, 14 Oct 2008 04:44:17 -0700
> Seokmann Ju <seokmann.ju@qlogic.com> wrote:
>
>>>> +static int
>>>> +fc_service_handler(struct Scsi_Host *shost, struct fc_rport  
>>>> *rport,
>>>> +			  struct request *req, struct request_queue *q)
>>>> +{
>>>> +	int ret;
>>>> +	struct request *rsp = req->next_rq;
>>>> +
>>>> +	if (!rsp) {
>>>> +		printk(KERN_ERR "ERROR: space for a FC service"
>>>> +		   " response is missing\n");
>>>> +		return -EINVAL;
>>>> +	}
>>>> +
>>>> +	if ((req->bio->bi_vcnt > FC_SERVICE_MAX_SG) ||
>>>> +	    (rsp->bio->bi_vcnt > FC_SERVICE_MAX_SG)) {
>>>> +		printk(KERN_ERR "ERROR: a FC service"
>>>> +		    " supports no more than %d SGs\n", FC_SERVICE_MAX_SG);
>>>> +		return -EINVAL;
>>>> +	}
>>>
>>> This doesn't look correct. bi_vcnt is not related with the number of
>>> sg. You use scatter-gather for large data transfer. You don't need  
>>> to
>>> worry about bi_vcnt.
>> I see...
>> Is there is a way to check, then, how many SG entries the service
>> needs before the blk_rq_map_sg()?
>
> As I wrote in the previous mail, via blk_queue_max_hw_segments() and
> blk_queue_max_phys_segments(), you can tell the block layer the number
> of sg segments you can handle.
OK. I've got it. Thanks.

Seokmann

Re: [PATCH 1/2] scsi_transport_fc: FC pass through support via bsg interface - revised

[Seokmann Ju]

On Oct 14, 2008, at 7:13 AM, Seokmann Ju wrote:

>
> On Oct 14, 2008, at 6:34 AM, FUJITA Tomonori wrote:
>
>> On Tue, 14 Oct 2008 04:44:17 -0700
>> Seokmann Ju <seokmann.ju@qlogic.com> wrote:
>>
>>>>> +static int
>>>>> +fc_service_handler(struct Scsi_Host *shost, struct fc_rport  
>>>>> *rport,
>>>>> +			  struct request *req, struct request_queue *q)
>>>>> +{
>>>>> +	int ret;
>>>>> +	struct request *rsp = req->next_rq;
>>>>> +
>>>>> +	if (!rsp) {
>>>>> +		printk(KERN_ERR "ERROR: space for a FC service"
>>>>> +		   " response is missing\n");
>>>>> +		return -EINVAL;
>>>>> +	}
>>>>> +
>>>>> +	if ((req->bio->bi_vcnt > FC_SERVICE_MAX_SG) ||
>>>>> +	    (rsp->bio->bi_vcnt > FC_SERVICE_MAX_SG)) {
>>>>> +		printk(KERN_ERR "ERROR: a FC service"
>>>>> +		    " supports no more than %d SGs\n", FC_SERVICE_MAX_SG);
>>>>> +		return -EINVAL;
>>>>> +	}
>>>>
>>>> This doesn't look correct. bi_vcnt is not related with the number  
>>>> of
>>>> sg. You use scatter-gather for large data transfer. You don't  
>>>> need to
>>>> worry about bi_vcnt.
>>> I see...
>>> Is there is a way to check, then, how many SG entries the service
>>> needs before the blk_rq_map_sg()?
>>
>> As I wrote in the previous mail, via blk_queue_max_hw_segments() and
>> blk_queue_max_phys_segments(), you can tell the block layer the  
>> number
>> of sg segments you can handle.
One more question here...
With addition of a timer into the FC transport layer for fc_service  
handing, the
system seems getting locked up, as below trace shows.
I've been trying to figure out the reasons but not able to do.
It seems like that there is something that I don't know, fundamentally  
about
the timer usage...
Could you please comments on what might be the things causing the  
problem?
---
...
Oct 17 11:53:58 linux-atl-00 kernel: SysRq : Show Regs
Oct 17 11:53:58 linux-atl-00 kernel: CPU 0:
Oct 17 11:53:58 linux-atl-00 kernel: Modules linked in: qla2xxx  
scsi_transport_fc ext3 jbd ipv6 button battery ac loop dm_mod floppy  
e1000 parport_pc lp parport reiserfs piix edd fan thermal processor  
thermal_sys sg sr_mod cdrom ata_piix libata dock sd_mod scsi_mod  
ide_disk ide_core [last unloaded: scsi_transport_fc]
Oct 17 11:53:58 linux-atl-00 kernel: Pid: 4284, comm: a.out Not  
tainted 2.6.27-rc4-smp #9
Oct 17 11:53:58 linux-atl-00 kernel: RIP: 0010:[<ffffffff8023ab34>]   
[<ffffffff8023ab34>] lock_timer_base+0x10/0x4b
Oct 17 11:53:58 linux-atl-00 kernel: RSP: 0018:ffff88012a481c48   
EFLAGS: 00000246
Oct 17 11:53:58 linux-atl-00 kernel: RAX: 0000000000000039 RBX:  
0000000000000000 RCX: ffffffff80507588
Oct 17 11:53:58 linux-atl-00 kernel: RDX: ffffffff80507588 RSI:  
ffff88012a481c80 RDI: ffff88012c4c58f0
Oct 17 11:53:58 linux-atl-00 kernel: RBP: ffff88012ac5e1c0 R08:  
ffffffff80507570 R09: ffff88002803cf10
Oct 17 11:53:58 linux-atl-00 kernel: R10: 0000000000000001 R11:  
0000000000000000 R12: ffff88012c4c58f0
Oct 17 11:53:58 linux-atl-00 kernel: R13: ffff88012c9c6198 R14:  
ffff88012c4c58c0 R15: 0000000100000000
Oct 17 11:53:58 linux-atl-00 kernel: FS:  00007fe1c2a966d0(0000)  
GS:ffffffff80652a00(0000) knlGS:0000000000000000
Oct 17 11:53:58 linux-atl-00 kernel: CS:  0010 DS: 0000 ES: 0000 CR0:  
000000008005003b
Oct 17 11:53:58 linux-atl-00 kernel: CR2: 00007fe1c2812d70 CR3:  
000000012e553000 CR4: 00000000000006e0
Oct 17 11:53:59 linux-atl-00 kernel: DR0: 0000000000000000 DR1:  
0000000000000000 DR2: 0000000000000000
Oct 17 11:53:59 linux-atl-00 kernel: DR3: 0000000000000000 DR6:  
00000000ffff0ff0 DR7: 0000000000000400
Oct 17 11:53:59 linux-atl-00 kernel:
Oct 17 11:53:59 linux-atl-00 kernel: Call Trace:
Oct 17 11:53:59 linux-atl-00 kernel:  [<ffffffff8023ac09>] ?  
__mod_timer+0x2a/0xbf
Oct 17 11:53:59 linux-atl-00 kernel:  [<ffffffffa00cbc36>] ?  
fc_service_dispatch+0x2ad/0x323 [scsi_transport_fc]
Oct 17 11:53:59 linux-atl-00 kernel:  [<ffffffff802fa0f1>] ? elv_insert 
+0xf2/0x220
Oct 17 11:53:59 linux-atl-00 kernel:  [<ffffffff802b0ac4>] ?  
bio_copy_user_iov+0x16f/0x219
Oct 17 11:53:59 linux-atl-00 kernel:  [<ffffffff802fe804>] ?  
blk_execute_rq_nowait+0x5d/0x83
Oct 17 11:53:59 linux-atl-00 kernel:  [<ffffffff802fe8ba>] ?  
blk_execute_rq+0x90/0xba
Oct 17 11:53:59 linux-atl-00 kernel:  [<ffffffff80302edc>] ? bsg_ioctl 
+0x141/0x188
Oct 17 11:53:59 linux-atl-00 kernel:  [<ffffffff80298f85>] ? vfs_ioctl 
+0x21/0x6b
Oct 17 11:53:59 linux-atl-00 kernel:  [<ffffffff8029921e>] ?  
do_vfs_ioctl+0x24f/0x268
Oct 17 11:53:59 linux-atl-00 kernel:  [<ffffffff80299288>] ? sys_ioctl 
+0x51/0x71
Oct 17 11:53:59 linux-atl-00 kernel:  [<ffffffff8020bdfb>] ?  
system_call_fastpath+0x16/0x1b
Oct 17 11:53:59 linux-atl-00 kernel:
...
---

Following is patch containing the changes since last submit.
- modified abort mechanism - it aborts the timed out services  
automatically
- added timeout <-- this is the part that above question refers...
- modified/updated base on comments for previous patch including DMA  
mapping
   handling
---
diff --git a/drivers/scsi/qla2xxx/qla_attr.c b/drivers/scsi/qla2xxx/ 
qla_attr.c
index d240da9..a5d531c 100644
--- a/drivers/scsi/qla2xxx/qla_attr.c
+++ b/drivers/scsi/qla2xxx/qla_attr.c
@@ -1399,7 +1399,7 @@ qla2x00_execute_fc_service(struct fc_service  
*service)
  		}
  {
  	int i;
-	ushort *tmp = ct_iocb;
+	ushort *tmp = (ushort *) ct_iocb;
  	for (i=0;i<sizeof (struct ct_entry_24xx) / 2; i++)
  		printk("%4x\n", tmp[i]);
  }
diff --git a/drivers/scsi/scsi_transport_fc.c b/drivers/scsi/ 
scsi_transport_fc.c
index dda12e0..3a50675 100644
--- a/drivers/scsi/scsi_transport_fc.c
+++ b/drivers/scsi/scsi_transport_fc.c
@@ -2420,23 +2420,42 @@ fc_rport_final_delete(struct work_struct *work)

  static void fc_service_timeout(unsigned long _service)
  {
+	int res = 0;
  	struct fc_service *service = (void *) _service;
+	struct Scsi_Host *shost = rport_to_shost(service->rport);
+	struct fc_internal *i = to_fc_internal(shost->transportt);
  	unsigned long flags;

  	spin_lock_irqsave(&service->service_state_lock, flags);
  	if (!(service->service_state_flag & FC_SERVICE_STATE_DONE))
  		service->service_state_flag |= FC_SERVICE_STATE_TIMEOUT;
  	spin_unlock_irqrestore(&service->service_state_lock, flags);
+
+	if (i->f->abort_fc_service)
+		res = i->f->abort_fc_service(service);
+
+	if (res) {
+		printk(KERN_ERR "ERROR: issuing FC service to the LLD "
+		"failed with status %d\n", res);
+		fc_service_done(service);
+		return;
+	}
  }

  static void fc_service_done(struct fc_service *service)
  {
+	// if (!del_timer_sync(&service->timer)) {
+	if (!del_timer(&service->timer)) {
+		printk(KERN_ERR "ERROR: failed to delete timer\n");
+		return;
+	}
+
  	if (service->service_state_flag != FC_SERVICE_STATE_DONE) {
-		if (service->service_state_flag == FC_SERVICE_STATE_TIMEOUT) {
+		if (service->service_state_flag == FC_SERVICE_STATE_TIMEOUT)
  			printk(KERN_ERR "ERROR: FC service timed out\n");
-			/* expect abort to be issued by the application */
-			return;
-		} else
+		else if (service->service_state_flag == FC_SERVICE_STATE_ABORTED)
+			printk(KERN_ERR "ERROR: FC service aborted\n");
+		else
  			printk(KERN_ERR "ERROR: FC service not finished\n");
  	}

@@ -2455,6 +2474,8 @@ static void fc_service_done(struct fc_service  
*service)
  	}

  	service->req->end_io(service->req, 0);
+	kfree(service->payload_dma);
+	kfree(service->response_dma);
  	kfree(service);
  }

@@ -2478,13 +2499,19 @@ issue_fc_service(struct fc_rport *rport,  
struct request *req,
  	service->q = q;
  	service->srv_request = srv_request;

-	sg_init_table(service->payload_dma, FC_SERVICE_MAX_SG);
+	service->payload_dma = (struct scatterlist *)
+	    kzalloc(sizeof (struct scatterlist) * req->nr_phys_segments,
+	    GFP_KERNEL);
+	sg_init_table(service->payload_dma, req->nr_phys_segments);
  	service->payload_sg_cnt =
  	    blk_rq_map_sg(q, req, service->payload_dma);
  	service->payload = (void *) bio_data(req->bio);
  	service->payload_size = req->data_len;

-	sg_init_table(service->response_dma, FC_SERVICE_MAX_SG);
+	service->response_dma = (struct scatterlist *)
+	    kzalloc(sizeof (struct scatterlist) * rsp->nr_phys_segments,
+	    GFP_KERNEL);
+	sg_init_table(service->response_dma, rsp->nr_phys_segments);
  	service->response_sg_cnt =
  	    blk_rq_map_sg(q, rsp, service->response_dma);
  	service->response = bio_data(rsp->bio);
@@ -2495,10 +2522,20 @@ issue_fc_service(struct fc_rport *rport,  
struct request *req,
  	service->service_done = fc_service_done;
  	service->service_state_flag = FC_SERVICE_STATE_PENDING;

+	service->timer.data = (unsigned long) service;
+	// service->timer.expires = req->timeout * HZ;
+	service->timer.expires = 30 * HZ;
+	service->timer.function = fc_service_timeout;
+
+	printk("ELS/CT: debug: timer prepared... timeout value = %d\n",  
service->timer.expires);
+	add_timer(&service->timer);
+	printk("ELS/CT: debug: timer added...\n");
+
  	if (i->f->execute_fc_service)
  		res = i->f->execute_fc_service(service);

  	if (res) {
+		del_timer(&service->timer);
  		printk(KERN_ERR "ERROR: issuing FC service to the LLD "
  		"failed with status %d\n", res);
  		fc_service_done(service);
@@ -2521,13 +2558,6 @@ fc_service_handler(struct Scsi_Host *shost,  
struct fc_rport *rport,
  		return -EINVAL;
  	}

-	if ((req->bio->bi_vcnt > FC_SERVICE_MAX_SG) ||
-	    (rsp->bio->bi_vcnt > FC_SERVICE_MAX_SG)) {
-		printk(KERN_ERR "ERROR: a FC service"
-		    " supports no more than %d SGs\n", FC_SERVICE_MAX_SG);
-		return -EINVAL;
-	}
-
  	ret = issue_fc_service(rport, req, rsp, q);

  	return ret;
@@ -2562,6 +2592,8 @@ fc_bsg_initialize(struct Scsi_Host *shost,  
struct fc_rport *rport)
  	const char *name;
  	void (*release)(struct device *);

+	printk("ELS/CT: debug: %s LONG_MAX = %lx sizeof long = %ld\n",  
__func__, LONG_MAX, sizeof (long));
+
  	if (!rport) {
  		printk(KERN_ERR "ERROR: rport is NULL\n");
  		return -ENOMEM;
@@ -2581,6 +2613,9 @@ fc_bsg_initialize(struct Scsi_Host *shost,  
struct fc_rport *rport)
  		return -ENOMEM;
  	}

+	blk_queue_max_hw_segments(q, shost->sg_tablesize);
+	blk_queue_max_phys_segments(q, SCSI_MAX_SG_CHAIN_SEGMENTS);
+
  	rport->q = q;
  	q->queuedata = rport;
  	queue_flag_set_unlocked(QUEUE_FLAG_BIDI, q);
diff --git a/include/scsi/scsi_transport_fc.h b/include/scsi/ 
scsi_transport_fc.h
index 6c6809a..4b7a8da 100644
--- a/include/scsi/scsi_transport_fc.h
+++ b/include/scsi/scsi_transport_fc.h
@@ -540,7 +540,6 @@ enum fc_service_status {
  #define FC_SERVICE_STATE_TIMEOUT      8

  #define FC_SERVICE_TIMEOUT 10
-#define FC_SERVICE_MAX_SG             16

  /* request (CDB) structure of the sg_io_v4 */
  struct fc_service_request {
@@ -574,11 +573,11 @@ struct fc_service {

  	struct fc_service_request *srv_request;
  	void  *payload;
-	struct scatterlist payload_dma[FC_SERVICE_MAX_SG];
+	struct scatterlist *payload_dma;
  	int   payload_sg_cnt;
  	int   payload_size;
  	void  *response;
-	struct scatterlist response_dma[FC_SERVICE_MAX_SG];
+	struct scatterlist *response_dma;
  	int   response_sg_cnt;
  	int   response_size;

---

Re: [PATCH 1/2] scsi_transport_fc: FC pass through support via bsg interface - revised

[FUJITA Tomonori]

On Mon, 20 Oct 2008 03:59:18 -0700
Seokmann Ju <seokmann.ju@qlogic.com> wrote:

> 
> On Oct 14, 2008, at 7:13 AM, Seokmann Ju wrote:
> 
> >
> > On Oct 14, 2008, at 6:34 AM, FUJITA Tomonori wrote:
> >
> >> On Tue, 14 Oct 2008 04:44:17 -0700
> >> Seokmann Ju <seokmann.ju@qlogic.com> wrote:
> >>
> >>>>> +static int
> >>>>> +fc_service_handler(struct Scsi_Host *shost, struct fc_rport  
> >>>>> *rport,
> >>>>> +			  struct request *req, struct request_queue *q)
> >>>>> +{
> >>>>> +	int ret;
> >>>>> +	struct request *rsp = req->next_rq;
> >>>>> +
> >>>>> +	if (!rsp) {
> >>>>> +		printk(KERN_ERR "ERROR: space for a FC service"
> >>>>> +		   " response is missing\n");
> >>>>> +		return -EINVAL;
> >>>>> +	}
> >>>>> +
> >>>>> +	if ((req->bio->bi_vcnt > FC_SERVICE_MAX_SG) ||
> >>>>> +	    (rsp->bio->bi_vcnt > FC_SERVICE_MAX_SG)) {
> >>>>> +		printk(KERN_ERR "ERROR: a FC service"
> >>>>> +		    " supports no more than %d SGs\n", FC_SERVICE_MAX_SG);
> >>>>> +		return -EINVAL;
> >>>>> +	}
> >>>>
> >>>> This doesn't look correct. bi_vcnt is not related with the number  
> >>>> of
> >>>> sg. You use scatter-gather for large data transfer. You don't  
> >>>> need to
> >>>> worry about bi_vcnt.
> >>> I see...
> >>> Is there is a way to check, then, how many SG entries the service
> >>> needs before the blk_rq_map_sg()?
> >>
> >> As I wrote in the previous mail, via blk_queue_max_hw_segments() and
> >> blk_queue_max_phys_segments(), you can tell the block layer the  
> >> number
> >> of sg segments you can handle.
> One more question here...
> With addition of a timer into the FC transport layer for fc_service  
> handing, the
> system seems getting locked up, as below trace shows.
> I've been trying to figure out the reasons but not able to do.
> It seems like that there is something that I don't know, fundamentally  
> about
> the timer usage...
> Could you please comments on what might be the things causing the  
> problem?

Hmm, why do you need to invent the own timeout mechanism?

I think that you can use the block layer timeout feature. It doesn't
work for fc transport pass through? You can just remove struct
timer_list in struct fc_service. Check out how scsi-ml uses
blk_queue_rq_timed_out and blk_queue_rq_timeout.

Re: [PATCH 1/2] scsi_transport_fc: FC pass through support via bsg interface - revised

[Seokmann Ju]

On Oct 20, 2008, at 4:45 AM, FUJITA Tomonori wrote:

> On Mon, 20 Oct 2008 03:59:18 -0700
> Seokmann Ju <seokmann.ju@qlogic.com> wrote:
>
>>
>> On Oct 14, 2008, at 7:13 AM, Seokmann Ju wrote:
>>
>>>
>>> On Oct 14, 2008, at 6:34 AM, FUJITA Tomonori wrote:
>>>
>>>> On Tue, 14 Oct 2008 04:44:17 -0700
>>>> Seokmann Ju <seokmann.ju@qlogic.com> wrote:
>>>>
>>>>>>> +static int
>>>>>>> +fc_service_handler(struct Scsi_Host *shost, struct fc_rport
>>>>>>> *rport,
>>>>>>> +			  struct request *req, struct request_queue *q)
>>>>>>> +{
>>>>>>> +	int ret;
>>>>>>> +	struct request *rsp = req->next_rq;
>>>>>>> +
>>>>>>> +	if (!rsp) {
>>>>>>> +		printk(KERN_ERR "ERROR: space for a FC service"
>>>>>>> +		   " response is missing\n");
>>>>>>> +		return -EINVAL;
>>>>>>> +	}
>>>>>>> +
>>>>>>> +	if ((req->bio->bi_vcnt > FC_SERVICE_MAX_SG) ||
>>>>>>> +	    (rsp->bio->bi_vcnt > FC_SERVICE_MAX_SG)) {
>>>>>>> +		printk(KERN_ERR "ERROR: a FC service"
>>>>>>> +		    " supports no more than %d SGs\n", FC_SERVICE_MAX_SG);
>>>>>>> +		return -EINVAL;
>>>>>>> +	}
>>>>>>
>>>>>> This doesn't look correct. bi_vcnt is not related with the number
>>>>>> of
>>>>>> sg. You use scatter-gather for large data transfer. You don't
>>>>>> need to
>>>>>> worry about bi_vcnt.
>>>>> I see...
>>>>> Is there is a way to check, then, how many SG entries the service
>>>>> needs before the blk_rq_map_sg()?
>>>>
>>>> As I wrote in the previous mail, via blk_queue_max_hw_segments()  
>>>> and
>>>> blk_queue_max_phys_segments(), you can tell the block layer the
>>>> number
>>>> of sg segments you can handle.
>> One more question here...
>> With addition of a timer into the FC transport layer for fc_service
>> handing, the
>> system seems getting locked up, as below trace shows.
>> I've been trying to figure out the reasons but not able to do.
>> It seems like that there is something that I don't know,  
>> fundamentally
>> about
>> the timer usage...
>> Could you please comments on what might be the things causing the
>> problem?
>
> Hmm, why do you need to invent the own timeout mechanism?
I was trying to have similar timeout mechanism as in the SCSI I/O,  
just like 'scsi_add_timer()' in scsi.c:scsi_dispatch_cmd()....
I assume this is not right approach...?

> I think that you can use the block layer timeout feature. It doesn't
> work for fc transport pass through? You can just remove struct
> timer_list in struct fc_service. Check out how scsi-ml uses
> blk_queue_rq_timed_out and blk_queue_rq_timeout.
OK. Thanks, I will try out this.
What are the exact name of those APIs, again?
It seems like that none of them are available in the source tree....

Seokmann

Re: [PATCH 1/2] scsi_transport_fc: FC pass through support via bsg interface - revised

[FUJITA Tomonori]

On Mon, 20 Oct 2008 05:46:22 -0700
Seokmann Ju <seokmann.ju@qlogic.com> wrote:

> 
> On Oct 20, 2008, at 4:45 AM, FUJITA Tomonori wrote:
> 
> > On Mon, 20 Oct 2008 03:59:18 -0700
> > Seokmann Ju <seokmann.ju@qlogic.com> wrote:
> >
> >>
> >> On Oct 14, 2008, at 7:13 AM, Seokmann Ju wrote:
> >>
> >>>
> >>> On Oct 14, 2008, at 6:34 AM, FUJITA Tomonori wrote:
> >>>
> >>>> On Tue, 14 Oct 2008 04:44:17 -0700
> >>>> Seokmann Ju <seokmann.ju@qlogic.com> wrote:
> >>>>
> >>>>>>> +static int
> >>>>>>> +fc_service_handler(struct Scsi_Host *shost, struct fc_rport
> >>>>>>> *rport,
> >>>>>>> +			  struct request *req, struct request_queue *q)
> >>>>>>> +{
> >>>>>>> +	int ret;
> >>>>>>> +	struct request *rsp = req->next_rq;
> >>>>>>> +
> >>>>>>> +	if (!rsp) {
> >>>>>>> +		printk(KERN_ERR "ERROR: space for a FC service"
> >>>>>>> +		   " response is missing\n");
> >>>>>>> +		return -EINVAL;
> >>>>>>> +	}
> >>>>>>> +
> >>>>>>> +	if ((req->bio->bi_vcnt > FC_SERVICE_MAX_SG) ||
> >>>>>>> +	    (rsp->bio->bi_vcnt > FC_SERVICE_MAX_SG)) {
> >>>>>>> +		printk(KERN_ERR "ERROR: a FC service"
> >>>>>>> +		    " supports no more than %d SGs\n", FC_SERVICE_MAX_SG);
> >>>>>>> +		return -EINVAL;
> >>>>>>> +	}
> >>>>>>
> >>>>>> This doesn't look correct. bi_vcnt is not related with the number
> >>>>>> of
> >>>>>> sg. You use scatter-gather for large data transfer. You don't
> >>>>>> need to
> >>>>>> worry about bi_vcnt.
> >>>>> I see...
> >>>>> Is there is a way to check, then, how many SG entries the service
> >>>>> needs before the blk_rq_map_sg()?
> >>>>
> >>>> As I wrote in the previous mail, via blk_queue_max_hw_segments()  
> >>>> and
> >>>> blk_queue_max_phys_segments(), you can tell the block layer the
> >>>> number
> >>>> of sg segments you can handle.
> >> One more question here...
> >> With addition of a timer into the FC transport layer for fc_service
> >> handing, the
> >> system seems getting locked up, as below trace shows.
> >> I've been trying to figure out the reasons but not able to do.
> >> It seems like that there is something that I don't know,  
> >> fundamentally
> >> about
> >> the timer usage...
> >> Could you please comments on what might be the things causing the
> >> problem?
> >
> > Hmm, why do you need to invent the own timeout mechanism?
> I was trying to have similar timeout mechanism as in the SCSI I/O,  
> just like 'scsi_add_timer()' in scsi.c:scsi_dispatch_cmd()....
> I assume this is not right approach...?
> 
> > I think that you can use the block layer timeout feature. It doesn't
> > work for fc transport pass through? You can just remove struct
> > timer_list in struct fc_service. Check out how scsi-ml uses
> > blk_queue_rq_timed_out and blk_queue_rq_timeout.
> OK. Thanks, I will try out this.
> What are the exact name of those APIs, again?
> It seems like that none of them are available in the source tree....

You need the latest git kernel. The block layer timeout feature was
introduced post 2.6.27. scsi_add_timer() has gone.

Re: [PATCH 1/2] scsi_transport_fc: FC pass through support via bsg interface - revised

[Seokmann Ju]

On Oct 20, 2008, at 6:36 AM, FUJITA Tomonori wrote:

> On Mon, 20 Oct 2008 05:46:22 -0700
> Seokmann Ju <seokmann.ju@qlogic.com> wrote:
>
>>
>> On Oct 20, 2008, at 4:45 AM, FUJITA Tomonori wrote:
>>
>>> On Mon, 20 Oct 2008 03:59:18 -0700
>>> Seokmann Ju <seokmann.ju@qlogic.com> wrote:
>>>
>>>>
>>>> On Oct 14, 2008, at 7:13 AM, Seokmann Ju wrote:
>>>>
>>>>>
>>>>> On Oct 14, 2008, at 6:34 AM, FUJITA Tomonori wrote:
>>>>>
>>>>>> On Tue, 14 Oct 2008 04:44:17 -0700
>>>>>> Seokmann Ju <seokmann.ju@qlogic.com> wrote:
>>>>>>
>>>>>>>>> +static int
>>>>>>>>> +fc_service_handler(struct Scsi_Host *shost, struct fc_rport
>>>>>>>>> *rport,
>>>>>>>>> +			  struct request *req, struct request_queue *q)
>>>>>>>>> +{
>>>>>>>>> +	int ret;
>>>>>>>>> +	struct request *rsp = req->next_rq;
>>>>>>>>> +
>>>>>>>>> +	if (!rsp) {
>>>>>>>>> +		printk(KERN_ERR "ERROR: space for a FC service"
>>>>>>>>> +		   " response is missing\n");
>>>>>>>>> +		return -EINVAL;
>>>>>>>>> +	}
>>>>>>>>> +
>>>>>>>>> +	if ((req->bio->bi_vcnt > FC_SERVICE_MAX_SG) ||
>>>>>>>>> +	    (rsp->bio->bi_vcnt > FC_SERVICE_MAX_SG)) {
>>>>>>>>> +		printk(KERN_ERR "ERROR: a FC service"
>>>>>>>>> +		    " supports no more than %d SGs\n", FC_SERVICE_MAX_SG);
>>>>>>>>> +		return -EINVAL;
>>>>>>>>> +	}
>>>>>>>>
>>>>>>>> This doesn't look correct. bi_vcnt is not related with the  
>>>>>>>> number
>>>>>>>> of
>>>>>>>> sg. You use scatter-gather for large data transfer. You don't
>>>>>>>> need to
>>>>>>>> worry about bi_vcnt.
>>>>>>> I see...
>>>>>>> Is there is a way to check, then, how many SG entries the  
>>>>>>> service
>>>>>>> needs before the blk_rq_map_sg()?
>>>>>>
>>>>>> As I wrote in the previous mail, via blk_queue_max_hw_segments()
>>>>>> and
>>>>>> blk_queue_max_phys_segments(), you can tell the block layer the
>>>>>> number
>>>>>> of sg segments you can handle.
>>>> One more question here...
>>>> With addition of a timer into the FC transport layer for fc_service
>>>> handing, the
>>>> system seems getting locked up, as below trace shows.
>>>> I've been trying to figure out the reasons but not able to do.
>>>> It seems like that there is something that I don't know,
>>>> fundamentally
>>>> about
>>>> the timer usage...
>>>> Could you please comments on what might be the things causing the
>>>> problem?
>>>
>>> Hmm, why do you need to invent the own timeout mechanism?
>> I was trying to have similar timeout mechanism as in the SCSI I/O,
>> just like 'scsi_add_timer()' in scsi.c:scsi_dispatch_cmd()....
>> I assume this is not right approach...?
>>
>>> I think that you can use the block layer timeout feature. It doesn't
>>> work for fc transport pass through? You can just remove struct
>>> timer_list in struct fc_service. Check out how scsi-ml uses
>>> blk_queue_rq_timed_out and blk_queue_rq_timeout.
>> OK. Thanks, I will try out this.
>> What are the exact name of those APIs, again?
>> It seems like that none of them are available in the source tree....
>
> You need the latest git kernel. The block layer timeout feature was
> introduced post 2.6.27. scsi_add_timer() has gone.
With this approach, I'm getting panic as below...
---
[  995.673318] qla2xxx 0000:0a:00.0: Cable is unplugged...
[  996.005310] qla2xxx 0000:0a:00.1: Cable is unplugged...
[ 1017.853786] ELS/CT: comp_status = 15
[ 1017.867789] general protection fault: 0000 [#1] SMP
[ 1017.871766] last sysfs file: /sys/block/sda/sda2/stat
[ 1017.877376] CPU 3
[ 1017.879586] Modules linked in: qla2xxx scsi_transport_fc
[ 1017.883579] Pid: 5717, comm: X Not tainted 2.6.27 #3
[ 1017.883579] RIP: 0010:[<ffffffff80348a2f>]  [<ffffffff80348a2f>]  
blk_rq_timed
_out_timer+0x63/0x13d
...

GNU gdb 6.6
Copyright (C) 2006 Free Software Foundation, Inc.
GDB is free software, covered by the GNU General Public License, and  
you are
welcome to change it and/or distribute copies of it under certain  
conditions.
Type "show copying" to see the conditions.
There is absolutely no warranty for GDB.  Type "show warranty" for  
details.
This GDB was configured as "x86_64-suse-linux"...
Using host libthread_db library "/lib64/libthread_db.so.1".
(gdb) l **blk_rq_timed_out_timer+0x63
0x1bf is in blk_rq_timed_out_timer (block/blk-timeout.c:126).
121             unsigned long flags, uninitialized_var(next), next_set  
= 0;
122             struct request *rq, *tmp;
123
124             spin_lock_irqsave(q->queue_lock, flags);
125
126             list_for_each_entry_safe(rq, tmp, &q->timeout_list,  
timeout_list) {
127                     if (time_after_eq(jiffies, rq->deadline)) {
128                             list_del_init(&rq->timeout_list);
129
130                             /*
(gdb)
---

And it seems like that the panic is happening due to the fact that  
blk_delete_timer() is not called upon having completion of the service.
In other words, the block layer calls blk_add_timer() prior to  
dispatch the service but, it doesn't call blk_delete_timer() when it  
returned.
Just for heck of it, I've tried out by adding blk_delete_timer() in  
the ~/block/blk-exec.c:blk_end_sync_rq() and it seems fixes the problem.
Seems like that there are APIs in the block layer that are call the  
blk_delete_timer(), including,
- blk_end_io()
- __blk_end_request()

Could you guide me what is right way to fix the problem?

Seokmann

Re: [PATCH 1/2] scsi_transport_fc: FC pass through support via bsg interface - revised

[FUJITA Tomonori]

CC'ed Jens,

On Wed, 22 Oct 2008 19:27:35 -0700
Seokmann Ju <seokmann.ju@qlogic.com> wrote:

> And it seems like that the panic is happening due to the fact that  
> blk_delete_timer() is not called upon having completion of the service.
> In other words, the block layer calls blk_add_timer() prior to  
> dispatch the service but, it doesn't call blk_delete_timer() when it  
> returned.

Yeah, we need to call blk_delete_timer somewhere.


> Just for heck of it, I've tried out by adding blk_delete_timer() in  
> the ~/block/blk-exec.c:blk_end_sync_rq() and it seems fixes the problem.

I think blk_end_sync_rq() is not the good place. From the perspective
of bsg, we need to handle both blk_execute_rq_nowait and
blk_execute_rq.


> Seems like that there are APIs in the block layer that are call the  
> blk_delete_timer(), including,
> - blk_end_io()
> - __blk_end_request()
> 
> Could you guide me what is right way to fix the problem?

Exporting blk_delete_timer is one option, but it doesn't look very
nice (since the block layer doesn't export any details about its timer
infrastructure), I think. Modifying blk_end_io() to make it usable for
requests via something like bsg might be better.

Anyway, we need to ask Jens.

Jens, fc people have working on fc pass through support via bsg, which
hooks bsg's request queue on fc transport objects (We did the similar
thing for sas transport).

We want the timeout feature for fc pass through and I think that it's
nice to use the block layer timeout feature for it. But the users of
bsg request queue don't need (or call) APIs such as
end_that_request_last to call blk_delete_timer internally. How should
these users call blk_delete_timer?

Re: [PATCH 1/2] scsi_transport_fc: FC pass through support via bsg interface - revised

[Boaz Harrosh]

FUJITA Tomonori wrote:
> CC'ed Jens,
> 
> On Wed, 22 Oct 2008 19:27:35 -0700
> Seokmann Ju <seokmann.ju@qlogic.com> wrote:
> 
>> And it seems like that the panic is happening due to the fact that  
>> blk_delete_timer() is not called upon having completion of the service.
>> In other words, the block layer calls blk_add_timer() prior to  
>> dispatch the service but, it doesn't call blk_delete_timer() when it  
>> returned.
> 
> Yeah, we need to call blk_delete_timer somewhere.
> 
> 
>> Just for heck of it, I've tried out by adding blk_delete_timer() in  
>> the ~/block/blk-exec.c:blk_end_sync_rq() and it seems fixes the problem.
> 
> I think blk_end_sync_rq() is not the good place. From the perspective
> of bsg, we need to handle both blk_execute_rq_nowait and
> blk_execute_rq.
> 
> 
>> Seems like that there are APIs in the block layer that are call the  
>> blk_delete_timer(), including,
>> - blk_end_io()
>> - __blk_end_request()
>>
>> Could you guide me what is right way to fix the problem?
> 
> Exporting blk_delete_timer is one option, but it doesn't look very
> nice (since the block layer doesn't export any details about its timer
> infrastructure), I think. Modifying blk_end_io() to make it usable for
> requests via something like bsg might be better.
> 
> Anyway, we need to ask Jens.
> 
> Jens, fc people have working on fc pass through support via bsg, which
> hooks bsg's request queue on fc transport objects (We did the similar
> thing for sas transport).
> 
> We want the timeout feature for fc pass through and I think that it's
> nice to use the block layer timeout feature for it. But the users of
> bsg request queue don't need (or call) APIs such as
> end_that_request_last to call blk_delete_timer internally. How should
> these users call blk_delete_timer?

TOMO Hi
If a command is queued by bsg to a scsi device, which is posible. Then
blk_end_request() is called by scsi-ml. So it does work.

I think that all block-queue consumers should call one of
blk_end_request(), there are lots to choose from. We don't need
a new API. It will work with or without data, and it does what
you want.

Just my $0.017
Thanks Boaz

Re: [PATCH 1/2] scsi_transport_fc: FC pass through support via bsg interface - revised

[FUJITA Tomonori]

On Sun, 26 Oct 2008 11:38:04 +0200
Boaz Harrosh <bharrosh@panasas.com> wrote:

> FUJITA Tomonori wrote:
> > CC'ed Jens,
> > 
> > On Wed, 22 Oct 2008 19:27:35 -0700
> > Seokmann Ju <seokmann.ju@qlogic.com> wrote:
> > 
> >> And it seems like that the panic is happening due to the fact that  
> >> blk_delete_timer() is not called upon having completion of the service.
> >> In other words, the block layer calls blk_add_timer() prior to  
> >> dispatch the service but, it doesn't call blk_delete_timer() when it  
> >> returned.
> > 
> > Yeah, we need to call blk_delete_timer somewhere.
> > 
> > 
> >> Just for heck of it, I've tried out by adding blk_delete_timer() in  
> >> the ~/block/blk-exec.c:blk_end_sync_rq() and it seems fixes the problem.
> > 
> > I think blk_end_sync_rq() is not the good place. From the perspective
> > of bsg, we need to handle both blk_execute_rq_nowait and
> > blk_execute_rq.
> > 
> > 
> >> Seems like that there are APIs in the block layer that are call the  
> >> blk_delete_timer(), including,
> >> - blk_end_io()
> >> - __blk_end_request()
> >>
> >> Could you guide me what is right way to fix the problem?
> > 
> > Exporting blk_delete_timer is one option, but it doesn't look very
> > nice (since the block layer doesn't export any details about its timer
> > infrastructure), I think. Modifying blk_end_io() to make it usable for
> > requests via something like bsg might be better.
> > 
> > Anyway, we need to ask Jens.
> > 
> > Jens, fc people have working on fc pass through support via bsg, which
> > hooks bsg's request queue on fc transport objects (We did the similar
> > thing for sas transport).
> > 
> > We want the timeout feature for fc pass through and I think that it's
> > nice to use the block layer timeout feature for it. But the users of
> > bsg request queue don't need (or call) APIs such as
> > end_that_request_last to call blk_delete_timer internally. How should
> > these users call blk_delete_timer?
> 
> TOMO Hi
> If a command is queued by bsg to a scsi device, which is posible. Then
> blk_end_request() is called by scsi-ml. So it does work.

It doesn't work for bsg's scsi transport pass through stuff such as
SMP (sas management protocol, we already support) and FC. Virtually,
they don't use scsi-ml.


> I think that all block-queue consumers should call one of
> blk_end_request(),

This is kinda what I suggested in the previous mail but as I wrote,
some of them don't now.


> there are lots to choose from. We don't need
> a new API. It will work with or without data, and it does what
> you want.

Re: [PATCH 1/2] scsi_transport_fc: FC pass through support via bsg interface - revised

[Boaz Harrosh]

FUJITA Tomonori wrote:
> On Sun, 26 Oct 2008 11:38:04 +0200
> Boaz Harrosh <bharrosh@panasas.com> wrote:
> 
>> FUJITA Tomonori wrote:
>>> CC'ed Jens,
>>>
>>> On Wed, 22 Oct 2008 19:27:35 -0700
>>> Seokmann Ju <seokmann.ju@qlogic.com> wrote:
>>>
>>>> And it seems like that the panic is happening due to the fact that  
>>>> blk_delete_timer() is not called upon having completion of the service.
>>>> In other words, the block layer calls blk_add_timer() prior to  
>>>> dispatch the service but, it doesn't call blk_delete_timer() when it  
>>>> returned.
>>> Yeah, we need to call blk_delete_timer somewhere.
>>>
>>>
>>>> Just for heck of it, I've tried out by adding blk_delete_timer() in  
>>>> the ~/block/blk-exec.c:blk_end_sync_rq() and it seems fixes the problem.
>>> I think blk_end_sync_rq() is not the good place. From the perspective
>>> of bsg, we need to handle both blk_execute_rq_nowait and
>>> blk_execute_rq.
>>>
>>>
>>>> Seems like that there are APIs in the block layer that are call the  
>>>> blk_delete_timer(), including,
>>>> - blk_end_io()
>>>> - __blk_end_request()
>>>>
>>>> Could you guide me what is right way to fix the problem?
>>> Exporting blk_delete_timer is one option, but it doesn't look very
>>> nice (since the block layer doesn't export any details about its timer
>>> infrastructure), I think. Modifying blk_end_io() to make it usable for
>>> requests via something like bsg might be better.
>>>
>>> Anyway, we need to ask Jens.
>>>
>>> Jens, fc people have working on fc pass through support via bsg, which
>>> hooks bsg's request queue on fc transport objects (We did the similar
>>> thing for sas transport).
>>>
>>> We want the timeout feature for fc pass through and I think that it's
>>> nice to use the block layer timeout feature for it. But the users of
>>> bsg request queue don't need (or call) APIs such as
>>> end_that_request_last to call blk_delete_timer internally. How should
>>> these users call blk_delete_timer?
>> TOMO Hi
>> If a command is queued by bsg to a scsi device, which is posible. Then
>> blk_end_request() is called by scsi-ml. So it does work.
> 
> It doesn't work for bsg's scsi transport pass through stuff such as
> SMP (sas management protocol, we already support) and FC. Virtually,
> they don't use scsi-ml.
> 

Right, I know that, that's why I say.

> 
>> I think that all block-queue consumers should call one of
>> blk_end_request(),
> 
> This is kinda what I suggested in the previous mail but as I wrote,
> some of them don't now.
> 

I think they should, specially if they're going to use the timer.
The way I see it they must. It's kind of a block layer API thing.
Someone calls blk_execute_xx then eventually someone needs to call
blk_end_request. You could call it from bsg but only temporary until
all are fixed. (because you will need an ugly check to see if request
was not already ended)

> 
>> there are lots to choose from. We don't need
>> a new API. It will work with or without data, and it does what
>> you want.

Boaz

Re: [PATCH 1/2] scsi_transport_fc: FC pass through support via bsg interface - revised

[FUJITA Tomonori]

On Mon, 27 Oct 2008 10:20:21 +0200
Boaz Harrosh <bharrosh@panasas.com> wrote:

> FUJITA Tomonori wrote:
> > On Sun, 26 Oct 2008 11:38:04 +0200
> > Boaz Harrosh <bharrosh@panasas.com> wrote:
> > 
> >> FUJITA Tomonori wrote:
> >>> CC'ed Jens,
> >>>
> >>> On Wed, 22 Oct 2008 19:27:35 -0700
> >>> Seokmann Ju <seokmann.ju@qlogic.com> wrote:
> >>>
> >>>> And it seems like that the panic is happening due to the fact that  
> >>>> blk_delete_timer() is not called upon having completion of the service.
> >>>> In other words, the block layer calls blk_add_timer() prior to  
> >>>> dispatch the service but, it doesn't call blk_delete_timer() when it  
> >>>> returned.
> >>> Yeah, we need to call blk_delete_timer somewhere.
> >>>
> >>>
> >>>> Just for heck of it, I've tried out by adding blk_delete_timer() in  
> >>>> the ~/block/blk-exec.c:blk_end_sync_rq() and it seems fixes the problem.
> >>> I think blk_end_sync_rq() is not the good place. From the perspective
> >>> of bsg, we need to handle both blk_execute_rq_nowait and
> >>> blk_execute_rq.
> >>>
> >>>
> >>>> Seems like that there are APIs in the block layer that are call the  
> >>>> blk_delete_timer(), including,
> >>>> - blk_end_io()
> >>>> - __blk_end_request()
> >>>>
> >>>> Could you guide me what is right way to fix the problem?
> >>> Exporting blk_delete_timer is one option, but it doesn't look very
> >>> nice (since the block layer doesn't export any details about its timer
> >>> infrastructure), I think. Modifying blk_end_io() to make it usable for
> >>> requests via something like bsg might be better.
> >>>
> >>> Anyway, we need to ask Jens.
> >>>
> >>> Jens, fc people have working on fc pass through support via bsg, which
> >>> hooks bsg's request queue on fc transport objects (We did the similar
> >>> thing for sas transport).
> >>>
> >>> We want the timeout feature for fc pass through and I think that it's
> >>> nice to use the block layer timeout feature for it. But the users of
> >>> bsg request queue don't need (or call) APIs such as
> >>> end_that_request_last to call blk_delete_timer internally. How should
> >>> these users call blk_delete_timer?
> >> TOMO Hi
> >> If a command is queued by bsg to a scsi device, which is posible. Then
> >> blk_end_request() is called by scsi-ml. So it does work.
> > 
> > It doesn't work for bsg's scsi transport pass through stuff such as
> > SMP (sas management protocol, we already support) and FC. Virtually,
> > they don't use scsi-ml.
> > 
> 
> Right, I know that, that's why I say.
> 
> > 
> >> I think that all block-queue consumers should call one of
> >> blk_end_request(),
> > 
> > This is kinda what I suggested in the previous mail but as I wrote,
> > some of them don't now.
> > 
> 
> I think they should, specially if they're going to use the timer.
> The way I see it they must. It's kind of a block layer API thing.
> Someone calls blk_execute_xx then eventually someone needs to call
> blk_end_request.

As I said, with the current API, we don't need to do that. But it's
kinda what I suggested in that mail.


> You could call it from bsg but only temporary until
> all are fixed. (because you will need an ugly check to see if request
> was not already ended)

Note that now nothing in mainline is broken with regard to this
issue. SMP doesn't have the timeout feature. FC pass though is the
first one that wants to do this. So no need to have any temporary fix.

Re: [PATCH 1/2] scsi_transport_fc: FC pass through support via bsg interface - revised

[Seokmann Ju]

On Oct 27, 2008, at 1:20 AM, Boaz Harrosh wrote:

> FUJITA Tomonori wrote:
>> On Sun, 26 Oct 2008 11:38:04 +0200
>> Boaz Harrosh <bharrosh@panasas.com> wrote:
>>
>>> FUJITA Tomonori wrote:
>>>> CC'ed Jens,
>>>>
>>>> On Wed, 22 Oct 2008 19:27:35 -0700
>>>> Seokmann Ju <seokmann.ju@qlogic.com> wrote:
>>>>
>>>>> And it seems like that the panic is happening due to the fact that
>>>>> blk_delete_timer() is not called upon having completion of the  
>>>>> service.
>>>>> In other words, the block layer calls blk_add_timer() prior to
>>>>> dispatch the service but, it doesn't call blk_delete_timer()  
>>>>> when it
>>>>> returned.
>>>> Yeah, we need to call blk_delete_timer somewhere.
>>>>
>>>>
>>>>> Just for heck of it, I've tried out by adding blk_delete_timer()  
>>>>> in
>>>>> the ~/block/blk-exec.c:blk_end_sync_rq() and it seems fixes the  
>>>>> problem.
>>>> I think blk_end_sync_rq() is not the good place. From the  
>>>> perspective
>>>> of bsg, we need to handle both blk_execute_rq_nowait and
>>>> blk_execute_rq.
>>>>
>>>>
>>>>> Seems like that there are APIs in the block layer that are call  
>>>>> the
>>>>> blk_delete_timer(), including,
>>>>> - blk_end_io()
>>>>> - __blk_end_request()
>>>>>
>>>>> Could you guide me what is right way to fix the problem?
>>>> Exporting blk_delete_timer is one option, but it doesn't look very
>>>> nice (since the block layer doesn't export any details about its  
>>>> timer
>>>> infrastructure), I think. Modifying blk_end_io() to make it  
>>>> usable for
>>>> requests via something like bsg might be better.
>>>>
>>>> Anyway, we need to ask Jens.
>>>>
>>>> Jens, fc people have working on fc pass through support via bsg,  
>>>> which
>>>> hooks bsg's request queue on fc transport objects (We did the  
>>>> similar
>>>> thing for sas transport).
>>>>
>>>> We want the timeout feature for fc pass through and I think that  
>>>> it's
>>>> nice to use the block layer timeout feature for it. But the users  
>>>> of
>>>> bsg request queue don't need (or call) APIs such as
>>>> end_that_request_last to call blk_delete_timer internally. How  
>>>> should
>>>> these users call blk_delete_timer?
>>> TOMO Hi
>>> If a command is queued by bsg to a scsi device, which is posible.  
>>> Then
>>> blk_end_request() is called by scsi-ml. So it does work.
>>
>> It doesn't work for bsg's scsi transport pass through stuff such as
>> SMP (sas management protocol, we already support) and FC. Virtually,
>> they don't use scsi-ml.
>>
>
> Right, I know that, that's why I say.
>
>>
>>> I think that all block-queue consumers should call one of
>>> blk_end_request(),
>>
>> This is kinda what I suggested in the previous mail but as I wrote,
>> some of them don't now.
>>
>
> I think they should, specially if they're going to use the timer.
> The way I see it they must. It's kind of a block layer API thing.
> Someone calls blk_execute_xx then eventually someone needs to call
> blk_end_request. You could call it from bsg but only temporary until
> all are fixed. (because you will need an ugly check to see if request
> was not already ended)
I made following changes but, it seems not helpful for the issue.
It, eventually, got failed to call blk_delete_timer() as ~/block/blk- 
core.c:__end_that_request_first() returns non-zero.
Inside of the __end_that_reqeust_first(), it detected 'nbytes' is  
bigger than 'nr_bytes' in case of bidi (where req->next_rq is not NULL).
I'm not sure whether we need to have chains of function calls  
initiated by the blk_end_request() or blk_end_bidi_request().
Would it create any problems if we directly call 'blk_delete_timer()'?

Seokmann
---
diff -Naurp a/blk-exec.c b/blk-exec.c
--- a/blk-exec.c	2008-10-27 09:33:14.000000000 -0700
+++ b/blk-exec.c	2008-10-27 09:34:08.000000000 -0700
@@ -22,6 +22,13 @@ static void blk_end_sync_rq(struct reque
  {
  	struct completion *waiting = rq->end_io_data;

+	if (rq->next_rq) {
+		blk_end_bidi_request(rq, error, rq->data_len,
+		    rq->next_rq->data_len);
+		// blk_end_request(rq);
+		// blk_delete_timer(rq);
+	}
+
  	rq->end_io_data = NULL;
  	__blk_put_request(rq->q, rq);
---
>
>
>>
>>> there are lots to choose from. We don't need
>>> a new API. It will work with or without data, and it does what
>>> you want.
>
> Boaz

Re: [PATCH 1/2] scsi_transport_fc: FC pass through support via bsg interface - revised

[Boaz Harrosh]

Seokmann Ju wrote:
> On Oct 27, 2008, at 1:20 AM, Boaz Harrosh wrote:
> 
>> FUJITA Tomonori wrote:
>>> On Sun, 26 Oct 2008 11:38:04 +0200
>>> Boaz Harrosh <bharrosh@panasas.com> wrote:
>>>
>>>> FUJITA Tomonori wrote:
>>>>> CC'ed Jens,
>>>>>
>>>>> On Wed, 22 Oct 2008 19:27:35 -0700
>>>>> Seokmann Ju <seokmann.ju@qlogic.com> wrote:
>>>>>
>>>>>> And it seems like that the panic is happening due to the fact that
>>>>>> blk_delete_timer() is not called upon having completion of the  
>>>>>> service.
>>>>>> In other words, the block layer calls blk_add_timer() prior to
>>>>>> dispatch the service but, it doesn't call blk_delete_timer()  
>>>>>> when it
>>>>>> returned.
>>>>> Yeah, we need to call blk_delete_timer somewhere.
>>>>>
>>>>>
>>>>>> Just for heck of it, I've tried out by adding blk_delete_timer()  
>>>>>> in
>>>>>> the ~/block/blk-exec.c:blk_end_sync_rq() and it seems fixes the  
>>>>>> problem.
>>>>> I think blk_end_sync_rq() is not the good place. From the  
>>>>> perspective
>>>>> of bsg, we need to handle both blk_execute_rq_nowait and
>>>>> blk_execute_rq.
>>>>>
>>>>>
>>>>>> Seems like that there are APIs in the block layer that are call  
>>>>>> the
>>>>>> blk_delete_timer(), including,
>>>>>> - blk_end_io()
>>>>>> - __blk_end_request()
>>>>>>
>>>>>> Could you guide me what is right way to fix the problem?
>>>>> Exporting blk_delete_timer is one option, but it doesn't look very
>>>>> nice (since the block layer doesn't export any details about its  
>>>>> timer
>>>>> infrastructure), I think. Modifying blk_end_io() to make it  
>>>>> usable for
>>>>> requests via something like bsg might be better.
>>>>>
>>>>> Anyway, we need to ask Jens.
>>>>>
>>>>> Jens, fc people have working on fc pass through support via bsg,  
>>>>> which
>>>>> hooks bsg's request queue on fc transport objects (We did the  
>>>>> similar
>>>>> thing for sas transport).
>>>>>
>>>>> We want the timeout feature for fc pass through and I think that  
>>>>> it's
>>>>> nice to use the block layer timeout feature for it. But the users  
>>>>> of
>>>>> bsg request queue don't need (or call) APIs such as
>>>>> end_that_request_last to call blk_delete_timer internally. How  
>>>>> should
>>>>> these users call blk_delete_timer?
>>>> TOMO Hi
>>>> If a command is queued by bsg to a scsi device, which is posible.  
>>>> Then
>>>> blk_end_request() is called by scsi-ml. So it does work.
>>> It doesn't work for bsg's scsi transport pass through stuff such as
>>> SMP (sas management protocol, we already support) and FC. Virtually,
>>> they don't use scsi-ml.
>>>
>> Right, I know that, that's why I say.
>>
>>>> I think that all block-queue consumers should call one of
>>>> blk_end_request(),
>>> This is kinda what I suggested in the previous mail but as I wrote,
>>> some of them don't now.
>>>
>> I think they should, specially if they're going to use the timer.
>> The way I see it they must. It's kind of a block layer API thing.
>> Someone calls blk_execute_xx then eventually someone needs to call
>> blk_end_request. You could call it from bsg but only temporary until
>> all are fixed. (because you will need an ugly check to see if request
>> was not already ended)
> I made following changes but, it seems not helpful for the issue.
> It, eventually, got failed to call blk_delete_timer() as ~/block/blk- 
> core.c:__end_that_request_first() returns non-zero.
> Inside of the __end_that_reqeust_first(), it detected 'nbytes' is  
> bigger than 'nr_bytes' in case of bidi (where req->next_rq is not NULL).
> I'm not sure whether we need to have chains of function calls  
> initiated by the blk_end_request() or blk_end_bidi_request().
> Would it create any problems if we directly call 'blk_delete_timer()'?
> 

Dear Seokmann. You miss understud me. What I'm saying is that you must
call blk_end_bidi_request at the FC end, just after you have finished
to consume the request, and before you return it upstream. it can be
some thing like:

+	blk_end_bidi_request(rq, 0, blk_rq_bytes(rq),
+	                     rq->next_rq ?  blk_rq_bytes(rq->next_rq) : 0);

In this case __end_that_reqeust_first should never return non-zero.

> Seokmann
> ---
> diff -Naurp a/blk-exec.c b/blk-exec.c
> --- a/blk-exec.c	2008-10-27 09:33:14.000000000 -0700
> +++ b/blk-exec.c	2008-10-27 09:34:08.000000000 -0700
> @@ -22,6 +22,13 @@ static void blk_end_sync_rq(struct reque
>   {
>   	struct completion *waiting = rq->end_io_data;
> 
> +	if (rq->next_rq) {
> +		blk_end_bidi_request(rq, error, rq->data_len,
> +		    rq->next_rq->data_len);
> +		// blk_end_request(rq);
> +		// blk_delete_timer(rq);
> +	}
> +
>   	rq->end_io_data = NULL;
>   	__blk_put_request(rq->q, rq);
> ---

Please don't change blk_end_sync_rq at blk-exec.c it is too
delicate here. At this stage rq->data_len is already holding
the residual count in most cases, and touching it would
be a bug

>>
>>>> there are lots to choose from. We don't need
>>>> a new API. It will work with or without data, and it does what
>>>> you want.
>> Boaz
> 

Do you have this code on a public git-web somewhere? I need
to look at the complete code, if you need that I advise you
where in the FC code to call blk_end_bidi_request()

Thanks
Boaz

Re: [PATCH 1/2] scsi_transport_fc: FC pass through support via bsg interface - revised

[Seokmann Ju]

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


On Oct 28, 2008, at 12:57 AM, Boaz Harrosh wrote:

> Seokmann Ju wrote:
>> On Oct 27, 2008, at 1:20 AM, Boaz Harrosh wrote:
>>
>>> FUJITA Tomonori wrote:
>>>> On Sun, 26 Oct 2008 11:38:04 +0200
>>>> Boaz Harrosh <bharrosh@panasas.com> wrote:
>>>>
>>>>> FUJITA Tomonori wrote:
>>>>>> CC'ed Jens,
>>>>> I think that all block-queue consumers should call one of
>>>>> blk_end_request(),
>>>> This is kinda what I suggested in the previous mail but as I wrote,
>>>> some of them don't now.
>>>>
>>> I think they should, specially if they're going to use the timer.
>>> The way I see it they must. It's kind of a block layer API thing.
>>> Someone calls blk_execute_xx then eventually someone needs to call
>>> blk_end_request. You could call it from bsg but only temporary until
>>> all are fixed. (because you will need an ugly check to see if  
>>> request
>>> was not already ended)
>> I made following changes but, it seems not helpful for the issue.
>> It, eventually, got failed to call blk_delete_timer() as ~/block/blk-
>> core.c:__end_that_request_first() returns non-zero.
>> Inside of the __end_that_reqeust_first(), it detected 'nbytes' is
>> bigger than 'nr_bytes' in case of bidi (where req->next_rq is not  
>> NULL).
>> I'm not sure whether we need to have chains of function calls
>> initiated by the blk_end_request() or blk_end_bidi_request().
>> Would it create any problems if we directly call  
>> 'blk_delete_timer()'?
>>
>
> Dear Seokmann. You miss understud me. What I'm saying is that you must
> call blk_end_bidi_request at the FC end, just after you have finished
> to consume the request, and before you return it upstream. it can be
> some thing like:
>
> +	blk_end_bidi_request(rq, 0, blk_rq_bytes(rq),
> +	                     rq->next_rq ?  blk_rq_bytes(rq->next_rq) : 0);
>
> In this case __end_that_reqeust_first should never return non-zero.
Hello Boaz,
Thank you for the clarification.
I made the changes accordingly and tested it, but the problem is still
there - same result of getting non-zero returns from
__end_that_request_first().
I guess that, either, I still get confused about the location or, there
is something else going on...

Sorry, I don't have public git-web.
Here is snaptshot of the FC transport layer changes.
The fc_service_done() is the callback that the FC transport layer
provides. And that is the callback called by LLD before returning.

Please let me know for any comments.

[-- Attachment #2: 0001-scsi_transport_fc-FC-pass-through-support.patch --]
[-- Type: application/octet-stream, Size: 11573 bytes --]

From ea2dc821bb75abcec3d0861e8d7ae9c80196f759 Mon Sep 17 00:00:00 2001
From: root <root@linux-atl-00.qlogic.com>
Date: Tue, 28 Oct 2008 06:49:51 -0700
Subject: [PATCH] scsi_transport_fc: FC pass through support

Signed-off-by: Seokmann Ju <seokmann.ju@qlogic.com>
---
 drivers/scsi/scsi_transport_fc.c |  220 +++++++++++++++++++++++++++++++++++++-
 include/scsi/scsi_transport_fc.h |   81 ++++++++++++++-
 2 files changed, 298 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/scsi_transport_fc.c b/drivers/scsi/scsi_transport_fc.c
index 1e71abf..d7eebe6 100644
--- a/drivers/scsi/scsi_transport_fc.c
+++ b/drivers/scsi/scsi_transport_fc.c
@@ -43,6 +43,11 @@ static void fc_vport_sched_delete(struct work_struct *work);
 static int fc_vport_setup(struct Scsi_Host *shost, int channel,
 	struct device *pdev, struct fc_vport_identifiers  *ids,
 	struct fc_vport **vport);
+static enum blk_eh_timer_return fc_service_timeout(struct request *req);
+static void fc_service_done(struct fc_service *);
+static int fc_service_handler(struct Scsi_Host *, struct fc_rport *,
+    struct request *, struct request_queue *);
+static void fc_bsg_remove(struct Scsi_Host *, struct fc_rport *);
 
 /*
  * Redefine so that we can have same named attributes in the
@@ -2413,11 +2418,222 @@ fc_rport_final_delete(struct work_struct *work)
 
 	transport_remove_device(dev);
 	device_del(dev);
+	fc_bsg_remove(shost, rport);
 	transport_destroy_device(dev);
 	put_device(&shost->shost_gendev);	/* for fc_host->rport list */
 	put_device(dev);			/* for self-reference */
 }
 
+static enum blk_eh_timer_return fc_service_timeout(struct request *req)
+{
+	struct fc_service *service = (void *) req->special;
+	struct Scsi_Host *shost = rport_to_shost(service->rport);
+	struct fc_internal *i = to_fc_internal(shost->transportt);
+	unsigned long flags;
+	int res = 0;
+
+	if (service->rport->port_state == FC_PORTSTATE_BLOCKED)
+		return BLK_EH_RESET_TIMER;
+
+	spin_lock_irqsave(&service->service_state_lock, flags);
+	if (!(service->service_state_flag & FC_SERVICE_STATE_DONE))
+		service->service_state_flag |= FC_SERVICE_STATE_TIMEOUT;
+	spin_unlock_irqrestore(&service->service_state_lock, flags);
+
+	if (i->f->abort_fc_service)
+		res = i->f->abort_fc_service(service);
+
+	if (res) {
+		printk(KERN_ERR "ERROR: issuing FC service to the LLD "
+		"failed with status %d\n", res);
+		fc_service_done(service);
+	}
+
+	/* the blk_end_sync_io() doesn't check the error */
+	return BLK_EH_NOT_HANDLED;
+}
+
+static void fc_service_done(struct fc_service *service)
+{
+
+	if (service->service_state_flag != FC_SERVICE_STATE_DONE) {
+		if (service->service_state_flag == FC_SERVICE_STATE_TIMEOUT)
+			printk(KERN_ERR "ERROR: FC service timed out\n");
+		else if (service->service_state_flag ==
+		    FC_SERVICE_STATE_ABORTED)
+			printk(KERN_ERR "ERROR: FC service aborted\n");
+		else
+			printk(KERN_ERR "ERROR: FC service not finished\n");
+	}
+
+	if (service->srv_reply.status != FC_SERVICE_COMPLETE) {
+		printk(KERN_ERR "ERROR: FC service to rport %p failed with"
+		    " status 0x%x\n", service->rport,
+		    service->srv_reply.status);
+	}
+
+	if (service->srv_reply.residual) {
+		service->req->data_len = 0;
+		service->req->next_rq->data_len = service->srv_reply.residual;
+	} else {
+		service->req->data_len = 0;
+		service->req->next_rq->data_len = 0;
+	}
+
+	blk_end_bidi_request(service->req, 0, blk_rq_bytes(service->req),
+	    service->req->next_rq ? blk_rq_bytes(service->req->next_rq) : 0);
+	service->req->end_io(service->req, 0);
+	kfree(service->payload_dma);
+	kfree(service->response_dma);
+	kfree(service);
+}
+
+static int
+issue_fc_service(struct fc_rport *rport, struct request *req,
+    struct request *rsp, struct request_queue *q)
+{
+	int res = -ECOMM;
+	struct fc_service *service;
+	struct Scsi_Host *shost = rport_to_shost(rport);
+	struct fc_internal *i = to_fc_internal(shost->transportt);
+	struct fc_service_request *srv_request = (struct fc_service_request *)
+	    req->cmd;
+
+	service = kzalloc(sizeof (struct fc_service), GFP_KERNEL);
+	if (!service)
+		return -ENOMEM;
+
+	req->special = service;
+	service->rport = rport;
+	service->req = req;
+	service->q = q;
+	service->srv_request = srv_request;
+	service->timeout = req->timeout;
+
+	service->payload_dma = (struct scatterlist *)
+	    kzalloc(sizeof (struct scatterlist) * req->nr_phys_segments,
+	    GFP_KERNEL);
+	sg_init_table(service->payload_dma, req->nr_phys_segments);
+	service->payload_sg_cnt =
+	    blk_rq_map_sg(q, req, service->payload_dma);
+	service->payload = (void *) bio_data(req->bio);
+	service->payload_size = req->data_len;
+
+	service->response_dma = (struct scatterlist *)
+	    kzalloc(sizeof (struct scatterlist) * rsp->nr_phys_segments,
+	    GFP_KERNEL);
+	sg_init_table(service->response_dma, rsp->nr_phys_segments);
+	service->response_sg_cnt =
+	    blk_rq_map_sg(q, rsp, service->response_dma);
+	service->response = bio_data(rsp->bio);
+	service->response_size = rsp->data_len;
+
+	/* sense area of the request structure */
+	service->reply_seq = req->sense;
+	service->service_done = fc_service_done;
+	service->service_state_flag = FC_SERVICE_STATE_PENDING;
+
+	if (i->f->execute_fc_service)
+		res = i->f->execute_fc_service(service);
+
+	if (res) {
+		printk(KERN_ERR "ERROR: issuing FC service to the LLD "
+		"failed with status %d\n", res);
+		fc_service_done(service);
+		return res;
+	}
+
+	return 0;
+}
+
+static int
+fc_service_handler(struct Scsi_Host *shost, struct fc_rport *rport,
+			  struct request *req, struct request_queue *q)
+{
+	int ret;
+	struct request *rsp = req->next_rq;
+
+	if (!rsp) {
+		printk(KERN_ERR "ERROR: space for a FC service"
+		   " response is missing\n");
+		return -EINVAL;
+	}
+
+	ret = issue_fc_service(rport, req, rsp, q);
+
+	return ret;
+}
+
+static void fc_service_dispatch(struct request_queue *q)
+{
+	struct request *req;
+	struct fc_rport *rport = q->queuedata;
+	struct Scsi_Host *shost = rport_to_shost(rport);
+
+	while (!blk_queue_plugged(q)) {
+		req = elv_next_request(q);
+		if (!req)
+			break;
+
+		blkdev_dequeue_request(req);
+		spin_unlock_irq(q->queue_lock);
+
+		fc_service_handler(shost, rport, req, q);
+
+		spin_lock_irq(q->queue_lock);
+	}
+}
+
+static int
+fc_bsg_initialize(struct Scsi_Host *shost, struct fc_rport *rport)
+{
+	struct request_queue *q;
+	int error;
+	struct device *dev;
+	const char *name;
+	void (*release)(struct device *);
+
+	if (!rport) {
+		printk(KERN_ERR "ERROR: rport is NULL\n");
+		return -ENOMEM;
+	}
+
+	q = blk_init_queue(fc_service_dispatch, NULL);
+	if (!q)
+		return -ENOMEM;
+
+	dev = &rport->dev;
+	name = dev->bus_id;
+	release = NULL;
+
+	error = bsg_register_queue(q, dev, name, release);
+	if (error) {
+		blk_cleanup_queue(q);
+		return -ENOMEM;
+	}
+
+	blk_queue_max_hw_segments(q, shost->sg_tablesize);
+	blk_queue_max_phys_segments(q, SCSI_MAX_SG_CHAIN_SEGMENTS);
+	blk_queue_rq_timed_out(q, fc_service_timeout);
+	blk_queue_rq_timeout(q, BLK_DEFAULT_SG_TIMEOUT);
+
+	rport->q = q;
+	q->queuedata = rport;
+	queue_flag_set_unlocked(QUEUE_FLAG_BIDI, q);
+
+	return 0;
+}
+
+static void
+fc_bsg_remove(struct Scsi_Host *shost, struct fc_rport *rport)
+{
+	struct request_queue *q = rport->q;
+
+	if (!q)
+		return;
+
+	bsg_unregister_queue(q);
+}
 
 /**
  * fc_rport_create - allocates and creates a remote FC port.
@@ -2478,8 +2694,8 @@ fc_rport_create(struct Scsi_Host *shost, int channel,
 	else
 		rport->scsi_target_id = -1;
 	list_add_tail(&rport->peers, &fc_host->rports);
-	get_device(&shost->shost_gendev);	/* for fc_host->rport list */
 
+	get_device(&shost->shost_gendev);	/* for fc_host->rport list */
 	spin_unlock_irqrestore(shost->host_lock, flags);
 
 	dev = &rport->dev;
@@ -2498,6 +2714,8 @@ fc_rport_create(struct Scsi_Host *shost, int channel,
 	transport_add_device(dev);
 	transport_configure_device(dev);
 
+	fc_bsg_initialize(shost, rport);
+
 	if (rport->roles & FC_PORT_ROLE_FCP_TARGET) {
 		/* initiate a scan of the target */
 		rport->flags |= FC_RPORT_SCAN_PENDING;
diff --git a/include/scsi/scsi_transport_fc.h b/include/scsi/scsi_transport_fc.h
index 49d8913..68104f5 100644
--- a/include/scsi/scsi_transport_fc.h
+++ b/include/scsi/scsi_transport_fc.h
@@ -33,7 +33,6 @@
 
 struct scsi_transport_template;
 
-
 /*
  * FC Port definitions - Following FC HBAAPI guidelines
  *
@@ -352,6 +351,7 @@ struct fc_rport {	/* aka fc_starget_attrs */
  	struct delayed_work fail_io_work;
  	struct work_struct stgt_delete_work;
 	struct work_struct rport_delete_work;
+	struct request_queue *q;
 } __attribute__((aligned(sizeof(unsigned long))));
 
 /* bit field values for struct fc_rport "flags" field: */
@@ -515,6 +515,80 @@ struct fc_host_attrs {
 	struct workqueue_struct *devloss_work_q;
 };
 
+#define FC_STATUS_BUF_SIZE 96
+
+enum fc_frame_type {
+	FC_FRAME_TYPE_BS,
+	FC_FRAME_TYPE_ELS,
+	FC_FRAME_TYPE_IEC = 4,
+	FC_FRAME_TYPE_IP,
+	FC_FRAME_TYPE_FCP = 8,
+	FC_FRAME_TYPE_GPP,
+	FC_FRAME_TYPE_FC_CT = 0x20,
+};
+
+enum fc_service_status {
+	FC_SERVICE_COMPLETE,
+	FC_SERVICE_TIMEOUT,
+	FC_SERVICE_ABORT,
+	FC_SERVICE_UNSUPPORTED,
+	FC_SERVICE_ERROR = -1,
+};
+
+#define FC_SERVICE_STATE_PENDING      1
+#define FC_SERVICE_STATE_DONE         2
+#define FC_SERVICE_STATE_ABORTED      4
+#define FC_SERVICE_STATE_TIMEOUT      8
+
+#define FC_SERVICE_TIMEOUT 10
+
+/* request (CDB) structure of the sg_io_v4 */
+struct fc_service_request {
+	u8   request_type;
+	u8   timeout;
+	u8   reserved0;
+	u8   reserved1;
+};
+
+/* response  (request sense data) structure of the sg_io_v4 */
+struct fc_service_reply {
+	enum fc_service_status status;
+	u16  error_code;
+	u16  additional_error_code;
+	u32  residual;
+};
+
+struct fc_service {
+	struct fc_rport  *rport;
+	struct list_head list;
+
+	spinlock_t   service_state_lock;
+	unsigned     service_state_flag;
+
+	struct request *req;
+	struct request_queue *q;
+
+	/* Used by the discovery code. */
+	struct completion  completion;
+	int  timeout;
+
+	struct fc_service_request *srv_request;
+	void  *payload;
+	struct scatterlist *payload_dma;
+	int   payload_sg_cnt;
+	int   payload_size;
+	void  *response;
+	struct scatterlist *response_dma;
+	int   response_sg_cnt;
+	int   response_size;
+
+	struct fc_service_reply srv_reply;
+	void  *reply_seq;   /* pointer to sense area of request */
+	void  (*service_done)(struct fc_service *);
+
+	void *lld_pkt;
+};
+
 #define shost_to_fc_host(x) \
 	((struct fc_host_attrs *)(x)->shost_data)
 
@@ -613,6 +687,10 @@ struct fc_function_template {
 	int     (* tsk_mgmt_response)(struct Scsi_Host *, u64, u64, int);
 	int     (* it_nexus_response)(struct Scsi_Host *, u64, int);
 
+	/* fc service handler */
+	int	(*execute_fc_service)(struct fc_service *);
+	int	(*abort_fc_service)(struct fc_service *);
+
 	/* allocation lengths for host-specific data */
 	u32	 			dd_fcrport_size;
 	u32	 			dd_fcvport_size;
@@ -736,7 +814,6 @@ fc_vport_set_state(struct fc_vport *vport, enum fc_vport_state new_state)
 	vport->vport_state = new_state;
 }
 
-
 struct scsi_transport_template *fc_attach_transport(
 			struct fc_function_template *);
 void fc_release_transport(struct scsi_transport_template *);
-- 
1.6.0.2


[-- Attachment #3: Type: text/plain, Size: 21 bytes --]



Thank you,
Seokmann

Re: [PATCH 1/2] scsi_transport_fc: FC pass through support via bsg interface - revised

[Boaz Harrosh]

Seokmann Ju wrote:
> On Oct 28, 2008, at 12:57 AM, Boaz Harrosh wrote:
> 
>> Seokmann Ju wrote:
>>> On Oct 27, 2008, at 1:20 AM, Boaz Harrosh wrote:
>>>
>>>> FUJITA Tomonori wrote:
>>>>> On Sun, 26 Oct 2008 11:38:04 +0200
>>>>> Boaz Harrosh <bharrosh@panasas.com> wrote:
>>>>>
>>>>>> FUJITA Tomonori wrote:
>>>>>>> CC'ed Jens,
>>>>>> I think that all block-queue consumers should call one of
>>>>>> blk_end_request(),
>>>>> This is kinda what I suggested in the previous mail but as I wrote,
>>>>> some of them don't now.
>>>>>
>>>> I think they should, specially if they're going to use the timer.
>>>> The way I see it they must. It's kind of a block layer API thing.
>>>> Someone calls blk_execute_xx then eventually someone needs to call
>>>> blk_end_request. You could call it from bsg but only temporary until
>>>> all are fixed. (because you will need an ugly check to see if  
>>>> request
>>>> was not already ended)
>>> I made following changes but, it seems not helpful for the issue.
>>> It, eventually, got failed to call blk_delete_timer() as ~/block/blk-
>>> core.c:__end_that_request_first() returns non-zero.
>>> Inside of the __end_that_reqeust_first(), it detected 'nbytes' is
>>> bigger than 'nr_bytes' in case of bidi (where req->next_rq is not  
>>> NULL).
>>> I'm not sure whether we need to have chains of function calls
>>> initiated by the blk_end_request() or blk_end_bidi_request().
>>> Would it create any problems if we directly call  
>>> 'blk_delete_timer()'?
>>>
>> Dear Seokmann. You miss understud me. What I'm saying is that you must
>> call blk_end_bidi_request at the FC end, just after you have finished
>> to consume the request, and before you return it upstream. it can be
>> some thing like:
>>
>> +	blk_end_bidi_request(rq, 0, blk_rq_bytes(rq),
>> +	                     rq->next_rq ?  blk_rq_bytes(rq->next_rq) : 0);
>>
>> In this case __end_that_reqeust_first should never return non-zero.
> Hello Boaz,
> Thank you for the clarification.
> I made the changes accordingly and tested it, but the problem is still
> there - same result of getting non-zero returns from
> __end_that_request_first().
> I guess that, either, I still get confused about the location or, there
> is something else going on...
> 
> Sorry, I don't have public git-web.
> Here is snaptshot of the FC transport layer changes.
> The fc_service_done() is the callback that the FC transport layer
> provides. And that is the callback called by LLD before returning.
> 
> Please let me know for any comments.
> 
> Thank you,
> Seokmann

if the attached file is the code you tested then it is wrong look here:

> +
> +	if (service->srv_reply.residual) {
> +		service->req->data_len = 0;
> +		service->req->next_rq->data_len = service->srv_reply.residual;
> +	} else {
> +		service->req->data_len = 0;
> +		service->req->next_rq->data_len = 0;
> +	}
> +

Move above to after the blk_end_bidi_request call

> +	blk_end_bidi_request(service->req, 0, blk_rq_bytes(service->req),
> +	    service->req->next_rq ? blk_rq_bytes(service->req->next_rq) : 0);

You must call blk_end_bidi_request before you change service->req->data_len
to hold the residual (or 0). Otherwise you damage the request.

> +	service->req->end_io(service->req, 0);
> +	kfree(service->payload_dma);
> +	kfree(service->response_dma);
> +	kfree(service);

With bsg the request is held by one more reference count in bsg, but in
general after the call to blk_end_bidi_request one/both request(s) may
die. In that case you need a code like:

	unsigned int dlen = blk_rq_bytes(req);
	unsigned int next_dlen = req->next_rq ? blk_rq_bytes(req->next_rq) : 0;

	req->data_len = resid;
	if (req->next_rq)
		req->next_rq->data_len = bidi_resid;

	/* The req and req->next_rq have not been completed */
	BUG_ON(blk_end_bidi_request(req, 0, dlen, next_dlen));

Boaz
 

Re: [PATCH 1/2] scsi_transport_fc: FC pass through support via bsg interface - revised

[Boaz Harrosh]

Boaz Harrosh wrote:
> Seokmann Ju wrote:
>> On Oct 28, 2008, at 12:57 AM, Boaz Harrosh wrote:
>>
>>> Seokmann Ju wrote:
>>>> On Oct 27, 2008, at 1:20 AM, Boaz Harrosh wrote:
>>>>
>>>>> FUJITA Tomonori wrote:
>>>>>> On Sun, 26 Oct 2008 11:38:04 +0200
>>>>>> Boaz Harrosh <bharrosh@panasas.com> wrote:
>>>>>>
>>>>>>> FUJITA Tomonori wrote:
>>>>>>>> CC'ed Jens,
>>>>>>> I think that all block-queue consumers should call one of
>>>>>>> blk_end_request(),
>>>>>> This is kinda what I suggested in the previous mail but as I wrote,
>>>>>> some of them don't now.
>>>>>>
>>>>> I think they should, specially if they're going to use the timer.
>>>>> The way I see it they must. It's kind of a block layer API thing.
>>>>> Someone calls blk_execute_xx then eventually someone needs to call
>>>>> blk_end_request. You could call it from bsg but only temporary until
>>>>> all are fixed. (because you will need an ugly check to see if  
>>>>> request
>>>>> was not already ended)
>>>> I made following changes but, it seems not helpful for the issue.
>>>> It, eventually, got failed to call blk_delete_timer() as ~/block/blk-
>>>> core.c:__end_that_request_first() returns non-zero.
>>>> Inside of the __end_that_reqeust_first(), it detected 'nbytes' is
>>>> bigger than 'nr_bytes' in case of bidi (where req->next_rq is not  
>>>> NULL).
>>>> I'm not sure whether we need to have chains of function calls
>>>> initiated by the blk_end_request() or blk_end_bidi_request().
>>>> Would it create any problems if we directly call  
>>>> 'blk_delete_timer()'?
>>>>
>>> Dear Seokmann. You miss understud me. What I'm saying is that you must
>>> call blk_end_bidi_request at the FC end, just after you have finished
>>> to consume the request, and before you return it upstream. it can be
>>> some thing like:
>>>
>>> +	blk_end_bidi_request(rq, 0, blk_rq_bytes(rq),
>>> +	                     rq->next_rq ?  blk_rq_bytes(rq->next_rq) : 0);
>>>
>>> In this case __end_that_reqeust_first should never return non-zero.
>> Hello Boaz,
>> Thank you for the clarification.
>> I made the changes accordingly and tested it, but the problem is still
>> there - same result of getting non-zero returns from
>> __end_that_request_first().
>> I guess that, either, I still get confused about the location or, there
>> is something else going on...
>>
>> Sorry, I don't have public git-web.
>> Here is snaptshot of the FC transport layer changes.
>> The fc_service_done() is the callback that the FC transport layer
>> provides. And that is the callback called by LLD before returning.
>>
>> Please let me know for any comments.
>>
>> Thank you,
>> Seokmann
> 
> if the attached file is the code you tested then it is wrong look here:
> 
>> +
>> +	if (service->srv_reply.residual) {
>> +		service->req->data_len = 0;
>> +		service->req->next_rq->data_len = service->srv_reply.residual;
>> +	} else {
>> +		service->req->data_len = 0;
>> +		service->req->next_rq->data_len = 0;
>> +	}
>> +
> 
> Move above to after the blk_end_bidi_request call
> 
>> +	blk_end_bidi_request(service->req, 0, blk_rq_bytes(service->req),
>> +	    service->req->next_rq ? blk_rq_bytes(service->req->next_rq) : 0);
> 
> You must call blk_end_bidi_request before you change service->req->data_len
> to hold the residual (or 0). Otherwise you damage the request.
> 
>> +	service->req->end_io(service->req, 0);

Hmm, on re inspection req->end_io(...) called here has the same problem.
Are you sure it's needed?

>> +	kfree(service->payload_dma);
>> +	kfree(service->response_dma);
>> +	kfree(service);
> 
> With bsg the request is held by one more reference count in bsg, but in
> general after the call to blk_end_bidi_request one/both request(s) may
> die. In that case you need a code like:
> 
> 	unsigned int dlen = blk_rq_bytes(req);
> 	unsigned int next_dlen = req->next_rq ? blk_rq_bytes(req->next_rq) : 0;
> 
> 	req->data_len = resid;
> 	if (req->next_rq)
> 		req->next_rq->data_len = bidi_resid;
> 
> 	/* The req and req->next_rq have not been completed */
> 	BUG_ON(blk_end_bidi_request(req, 0, dlen, next_dlen));
> 
> Boaz
>  
> --
> 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

Re: [PATCH 1/2] scsi_transport_fc: FC pass through support via bsg interface - revised

[Boaz Harrosh]

Boaz Harrosh wrote:
> Boaz Harrosh wrote:
>> Seokmann Ju wrote:
>>> On Oct 28, 2008, at 12:57 AM, Boaz Harrosh wrote:
>>>
>>>> Seokmann Ju wrote:
>>>>> On Oct 27, 2008, at 1:20 AM, Boaz Harrosh wrote:
>>>>>
>>>>>> FUJITA Tomonori wrote:
>>>>>>> On Sun, 26 Oct 2008 11:38:04 +0200
>>>>>>> Boaz Harrosh <bharrosh@panasas.com> wrote:
>>>>>>>
>>>>>>>> FUJITA Tomonori wrote:
>>>>>>>>> CC'ed Jens,
>>>>>>>> I think that all block-queue consumers should call one of
>>>>>>>> blk_end_request(),
>>>>>>> This is kinda what I suggested in the previous mail but as I wrote,
>>>>>>> some of them don't now.
>>>>>>>
>>>>>> I think they should, specially if they're going to use the timer.
>>>>>> The way I see it they must. It's kind of a block layer API thing.
>>>>>> Someone calls blk_execute_xx then eventually someone needs to call
>>>>>> blk_end_request. You could call it from bsg but only temporary until
>>>>>> all are fixed. (because you will need an ugly check to see if  
>>>>>> request
>>>>>> was not already ended)
>>>>> I made following changes but, it seems not helpful for the issue.
>>>>> It, eventually, got failed to call blk_delete_timer() as ~/block/blk-
>>>>> core.c:__end_that_request_first() returns non-zero.
>>>>> Inside of the __end_that_reqeust_first(), it detected 'nbytes' is
>>>>> bigger than 'nr_bytes' in case of bidi (where req->next_rq is not  
>>>>> NULL).
>>>>> I'm not sure whether we need to have chains of function calls
>>>>> initiated by the blk_end_request() or blk_end_bidi_request().
>>>>> Would it create any problems if we directly call  
>>>>> 'blk_delete_timer()'?
>>>>>
>>>> Dear Seokmann. You miss understud me. What I'm saying is that you must
>>>> call blk_end_bidi_request at the FC end, just after you have finished
>>>> to consume the request, and before you return it upstream. it can be
>>>> some thing like:
>>>>
>>>> +	blk_end_bidi_request(rq, 0, blk_rq_bytes(rq),
>>>> +	                     rq->next_rq ?  blk_rq_bytes(rq->next_rq) : 0);
>>>>
>>>> In this case __end_that_reqeust_first should never return non-zero.
>>> Hello Boaz,
>>> Thank you for the clarification.
>>> I made the changes accordingly and tested it, but the problem is still
>>> there - same result of getting non-zero returns from
>>> __end_that_request_first().
>>> I guess that, either, I still get confused about the location or, there
>>> is something else going on...
>>>
>>> Sorry, I don't have public git-web.
>>> Here is snaptshot of the FC transport layer changes.
>>> The fc_service_done() is the callback that the FC transport layer
>>> provides. And that is the callback called by LLD before returning.
>>>
>>> Please let me know for any comments.
>>>
>>> Thank you,
>>> Seokmann
>> if the attached file is the code you tested then it is wrong look here:
>>
>>> +
>>> +	if (service->srv_reply.residual) {
>>> +		service->req->data_len = 0;
>>> +		service->req->next_rq->data_len = service->srv_reply.residual;
>>> +	} else {
>>> +		service->req->data_len = 0;
>>> +		service->req->next_rq->data_len = 0;
>>> +	}
>>> +
>> Move above to after the blk_end_bidi_request call
>>
>>> +	blk_end_bidi_request(service->req, 0, blk_rq_bytes(service->req),
>>> +	    service->req->next_rq ? blk_rq_bytes(service->req->next_rq) : 0);
>> You must call blk_end_bidi_request before you change service->req->data_len
>> to hold the residual (or 0). Otherwise you damage the request.
>>
>>> +	service->req->end_io(service->req, 0);
> 
> Hmm, on re inspection req->end_io(...) called here has the same problem.
> Are you sure it's needed?
> 

No you do not call req->end_io(..) directly. It eventual gets called
by blk_end_bidi_request() inside end_that_request_last(). (Once
all byte are completed)

<snip>

Boaz

Re: [PATCH 1/2] scsi_transport_fc: FC pass through support via bsg interface - revised

[Seokmann Ju]

On Oct 28, 2008, at 7:59 AM, Boaz Harrosh wrote:

> Boaz Harrosh wrote:
>> Boaz Harrosh wrote:
...
[snip]
...
>>> if the attached file is the code you tested then it is wrong look  
>>> here:
>>>
>>>> +
>>>> +	if (service->srv_reply.residual) {
>>>> +		service->req->data_len = 0;
>>>> +		service->req->next_rq->data_len = service->srv_reply.residual;
>>>> +	} else {
>>>> +		service->req->data_len = 0;
>>>> +		service->req->next_rq->data_len = 0;
>>>> +	}
>>>> +
>>> Move above to after the blk_end_bidi_request call
>>>
>>>> +	blk_end_bidi_request(service->req, 0, blk_rq_bytes(service->req),
>>>> +	    service->req->next_rq ? blk_rq_bytes(service->req- 
>>>> >next_rq) : 0);
>>> You must call blk_end_bidi_request before you change service->req- 
>>> >data_len
>>> to hold the residual (or 0). Otherwise you damage the request.
>>>
>>>> +	service->req->end_io(service->req, 0);
>>
>> Hmm, on re inspection req->end_io(...) called here has the same  
>> problem.
>> Are you sure it's needed?
>>
>
> No you do not call req->end_io(..) directly. It eventual gets called
> by blk_end_bidi_request() inside end_that_request_last(). (Once
> all byte are completed)
Yes, that is correct. Thank you for the correction - end_io() call has  
removed.
Other than setting req->errors field and req->next_rq->errors, there  
are not
many things to do in the callback prior to calling  
blk_end_bidi_request().

With the clean-up, updated patch will be followed.

Thank you,
Seokmann