[PATCH 1/2] scsi_transport_fc: FC pass through support - revised II

[PATCH 1/2] scsi_transport_fc: FC pass through support - revised II

[Seokmann Ju]

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

This patch will add FC pass through support.
The FC pass through support is service request handling mechanism
for FC specification defined services including,
- Link Services (Basic LS, Extended LS)
- Generic Services (FC-CT - Common Transport)
The support utilize BSG (Block layer SCSI Generic) interface with
bidi (bi-directional) nature in handling the service requests.

This patch added following featues in the support
- FC service structure has defined to transport service requests
- Handles the service request in asynchronous manner - LLD
- Timeout capability
- Abort capability

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

diff --git a/drivers/scsi/scsi_transport_fc.c b/drivers/scsi/ 
scsi_transport_fc.c
index 1e71abf..e26e8e0 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,218 @@ 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);
+	}
+
+	service->req->errors = service->srv_reply.status;
+	service->req->next_rq->errors = service->srv_reply.status;
+
+	blk_end_bidi_request(service->req, service->srv_reply.status,
+	    blk_rq_bytes(service->req),
+	    service->req->next_rq ? blk_rq_bytes(service->req->next_rq) : 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 +2690,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 +2710,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

Re: [PATCH 1/2] scsi_transport_fc: FC pass through support - revised II

[Boaz Harrosh]

Seokmann Ju wrote:
>  From f32b386a61a23f408974f2289cd34200f59401e1 Mon Sep 17 00:00:00 2001
> From: root <root@linux-atl-00.qlogic.com>
> Date: Tue, 28 Oct 2008 19:27:15 -0700
> Subject: [PATCH] scsi_transport_fc: FC pass through support
> 
> This patch will add FC pass through support.
> The FC pass through support is service request handling mechanism
> for FC specification defined services including,
> - Link Services (Basic LS, Extended LS)
> - Generic Services (FC-CT - Common Transport)
> The support utilize BSG (Block layer SCSI Generic) interface with
> bidi (bi-directional) nature in handling the service requests.
> 
> This patch added following featues in the support
> - FC service structure has defined to transport service requests
> - Handles the service request in asynchronous manner - LLD
> - Timeout capability
> - Abort capability
> 
> Signed-off-by: Seokmann Ju <seokmann.ju@qlogic.com>
> ---
>   drivers/scsi/scsi_transport_fc.c |  216 +++++++++++++++++++++++++++++ 
> ++++++++-
>   include/scsi/scsi_transport_fc.h |   81 ++++++++++++++-
>   2 files changed, 294 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_transport_fc.c b/drivers/scsi/ 
> scsi_transport_fc.c
> index 1e71abf..e26e8e0 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,218 @@ 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);
> +	}
> +
> +	service->req->errors = service->srv_reply.status;
> +	service->req->next_rq->errors = service->srv_reply.status;
> +

Just nit-picking. next_rq should not carry any other information but
BIOs (sg-lists) and residual. Any global state information is carried
on the main request only.
-	service->req->next_rq->errors = service->srv_reply.status;

Jens TOMO?

I never understood the difference please explain:
What is the difference between "req->errors =" and the error passed
into blk_end_xxx_request(req, error,...)?

> +	blk_end_bidi_request(service->req, service->srv_reply.status,
> +	    blk_rq_bytes(service->req),
> +	    service->req->next_rq ? blk_rq_bytes(service->req->next_rq) : 0);
> +
> +	kfree(service->payload_dma);
> +	kfree(service->response_dma);
> +	kfree(service);
> +}
> +
<snip>

Thanks
Boaz

Re: [PATCH 1/2] scsi_transport_fc: FC pass through support - revised II

[FUJITA Tomonori]

On Wed, 29 Oct 2008 05:48:03 -0700
Seokmann Ju <seokmann.ju@qlogic.com> wrote:

>  From f32b386a61a23f408974f2289cd34200f59401e1 Mon Sep 17 00:00:00 2001
> From: root <root@linux-atl-00.qlogic.com>
> Date: Tue, 28 Oct 2008 19:27:15 -0700
> Subject: [PATCH] scsi_transport_fc: FC pass through support
> 
> This patch will add FC pass through support.
> The FC pass through support is service request handling mechanism
> for FC specification defined services including,
> - Link Services (Basic LS, Extended LS)
> - Generic Services (FC-CT - Common Transport)
> The support utilize BSG (Block layer SCSI Generic) interface with
> bidi (bi-directional) nature in handling the service requests.
> 
> This patch added following featues in the support
> - FC service structure has defined to transport service requests
> - Handles the service request in asynchronous manner - LLD
> - Timeout capability
> - Abort capability
> 
> Signed-off-by: Seokmann Ju <seokmann.ju@qlogic.com>
> ---
>   drivers/scsi/scsi_transport_fc.c |  216 +++++++++++++++++++++++++++++ 
> ++++++++-
>   include/scsi/scsi_transport_fc.h |   81 ++++++++++++++-
>   2 files changed, 294 insertions(+), 3 deletions(-)

First, the patch looks corrupt (I can't apply this). I guess that tabs
are converted to spaces.


> diff --git a/drivers/scsi/scsi_transport_fc.c b/drivers/scsi/ 
> scsi_transport_fc.c
> index 1e71abf..e26e8e0 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,218 @@ 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);
> +	}
> +
> +	service->req->errors = service->srv_reply.status;
> +	service->req->next_rq->errors = service->srv_reply.status;

We don't need to save service->srv_reply.status at two places
service->req is appropriate.


> +	blk_end_bidi_request(service->req, service->srv_reply.status,
> +	    blk_rq_bytes(service->req),
> +	    service->req->next_rq ? blk_rq_bytes(service->req->next_rq) : 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 *)

No needs to cast for kzalloc.

btw, tha name 'payload_dma' sounds a bit odd to me. Software FCoE
drivers doesn't do dma for data transfer, right?



> +	    kzalloc(sizeof (struct scatterlist) * req->nr_phys_segments,

Don't put space after sizeof. checkpatch.pl complains about this.


> +	    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);

You don't need service->payload. We use scatterlist for data transfer.


> +	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);

Ditto.


> +	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);

I'm not sure you need sg chaining for fc pass through.


> +	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 +2690,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 +2710,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
> 
> 
> --
> 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 - revised II

[Boaz Harrosh]

FUJITA Tomonori wrote:
> On Wed, 29 Oct 2008 05:48:03 -0700
> Seokmann Ju <seokmann.ju@qlogic.com> wrote:
> 
>>  From f32b386a61a23f408974f2289cd34200f59401e1 Mon Sep 17 00:00:00 2001
>> From: root <root@linux-atl-00.qlogic.com>
>> Date: Tue, 28 Oct 2008 19:27:15 -0700
>> Subject: [PATCH] scsi_transport_fc: FC pass through support
>>
>> This patch will add FC pass through support.
>> The FC pass through support is service request handling mechanism
>> for FC specification defined services including,
>> - Link Services (Basic LS, Extended LS)
>> - Generic Services (FC-CT - Common Transport)
>> The support utilize BSG (Block layer SCSI Generic) interface with
>> bidi (bi-directional) nature in handling the service requests.
>>
>> This patch added following featues in the support
>> - FC service structure has defined to transport service requests
>> - Handles the service request in asynchronous manner - LLD
>> - Timeout capability
>> - Abort capability
>>
>> Signed-off-by: Seokmann Ju <seokmann.ju@qlogic.com>
>> ---
>>   drivers/scsi/scsi_transport_fc.c |  216 +++++++++++++++++++++++++++++ 
>> ++++++++-
>>   include/scsi/scsi_transport_fc.h |   81 ++++++++++++++-
>>   2 files changed, 294 insertions(+), 3 deletions(-)
> 
> First, the patch looks corrupt (I can't apply this). I guess that tabs
> are converted to spaces.
> 
> 
>> diff --git a/drivers/scsi/scsi_transport_fc.c b/drivers/scsi/ 
>> scsi_transport_fc.c
>> index 1e71abf..e26e8e0 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,218 @@ 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);
>> +	}
>> +
>> +	service->req->errors = service->srv_reply.status;
>> +	service->req->next_rq->errors = service->srv_reply.status;
> 
> We don't need to save service->srv_reply.status at two places
> service->req is appropriate.
> 

TOMO Hi.
Do you know what is the difference between "req->error =" and the error
passed to blk_end_xxx_request(req, error, ...) ?

> 
>> +	blk_end_bidi_request(service->req, service->srv_reply.status,
>> +	    blk_rq_bytes(service->req),
>> +	    service->req->next_rq ? blk_rq_bytes(service->req->next_rq) : 0);
>> +
>> +	kfree(service->payload_dma);
>> +	kfree(service->response_dma);
>> +	kfree(service);
>> +}
>> +
<snip>

Boaz

Re: [PATCH 1/2] scsi_transport_fc: FC pass through support - revised II

[FUJITA Tomonori]

On Thu, 30 Oct 2008 09:51:40 +0200
Boaz Harrosh <bharrosh@panasas.com> wrote:

> Do you know what is the difference between "req->error =" and the error
> passed to blk_end_xxx_request(req, error, ...) ?

I guess that blk_end_xxx_request(req, error, ...) is common Linux
error code, -EIO, etc. req->error carries protocol specific errors;
SAM_STAT_*, DID_*, etc in the case of SCSI. But I'm not confident.

Re: [PATCH 1/2] scsi_transport_fc: FC pass through support - revised II

[Seokmann Ju]

On Oct 29, 2008, at 9:18 PM, FUJITA Tomonori wrote:

> On Wed, 29 Oct 2008 05:48:03 -0700
> Seokmann Ju <seokmann.ju@qlogic.com> wrote:
>
>> From f32b386a61a23f408974f2289cd34200f59401e1 Mon Sep 17 00:00:00  
>> 2001
>> From: root <root@linux-atl-00.qlogic.com>
>> Date: Tue, 28 Oct 2008 19:27:15 -0700
>> Subject: [PATCH] scsi_transport_fc: FC pass through support
>>
>> This patch will add FC pass through support.
>> The FC pass through support is service request handling mechanism
>> for FC specification defined services including,
>> - Link Services (Basic LS, Extended LS)
>> - Generic Services (FC-CT - Common Transport)
>> The support utilize BSG (Block layer SCSI Generic) interface with
>> bidi (bi-directional) nature in handling the service requests.
>>
>> This patch added following featues in the support
>> - FC service structure has defined to transport service requests
>> - Handles the service request in asynchronous manner - LLD
>> - Timeout capability
>> - Abort capability
>>
>> Signed-off-by: Seokmann Ju <seokmann.ju@qlogic.com>
>> ---
>>  drivers/scsi/scsi_transport_fc.c |  216 +++++++++++++++++++++++++++ 
>> ++
>> ++++++++-
>>  include/scsi/scsi_transport_fc.h |   81 ++++++++++++++-
>>  2 files changed, 294 insertions(+), 3 deletions(-)
>
> First, the patch looks corrupt (I can't apply this). I guess that tabs
> are converted to spaces.
Sorry, whenever I send patches, it makes me to worry about it... but,
I'm still struggling to find what causes the problem.
Sometimes it worked (guessing for shorter ones).
I will submit the patch along with attachment next time.
BTW: I use the Mail on my Mac... any comments on this problem would be
greatly appreciated...

>> diff --git a/drivers/scsi/scsi_transport_fc.c b/drivers/scsi/
>> scsi_transport_fc.c
>> index 1e71abf..e26e8e0 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,218 @@ 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);
>> +	}
>> +
>> +	service->req->errors = service->srv_reply.status;
>> +	service->req->next_rq->errors = service->srv_reply.status;
>
> We don't need to save service->srv_reply.status at two places
> service->req is appropriate.
OK. It is removed.

>> +	blk_end_bidi_request(service->req, service->srv_reply.status,
>> +	    blk_rq_bytes(service->req),
>> +	    service->req->next_rq ? blk_rq_bytes(service->req->next_rq) :  
>> 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 *)
>
> No needs to cast for kzalloc.
>
> btw, tha name 'payload_dma' sounds a bit odd to me. Software FCoE
> drivers doesn't do dma for data transfer, right?
OK. The type case has removed.
Any suggestion on the name change?
Please let me know if you have it. I will take it.

>> +	    kzalloc(sizeof (struct scatterlist) * req->nr_phys_segments,
>
> Don't put space after sizeof. checkpatch.pl complains about this.
OK. Done.

>> +	    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);
>
> You don't need service->payload. We use scatterlist for data transfer.
Yes. Removed.

>> +	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);
>
> Ditto.
OK.

>> +	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);
>
> I'm not sure you need sg chaining for fc pass through.
Yes, we need it for FC generic services which have more than one SGs.

>> +	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 +2690,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 +2710,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
>>
>>
>> --
>> 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 - revised II

[James Bottomley]

On Thu, 2008-10-30 at 17:29 +0900, FUJITA Tomonori wrote:
> On Thu, 30 Oct 2008 09:51:40 +0200
> Boaz Harrosh <bharrosh@panasas.com> wrote:
> 
> > Do you know what is the difference between "req->error =" and the error
> > passed to blk_end_xxx_request(req, error, ...) ?
> 
> I guess that blk_end_xxx_request(req, error, ...) is common Linux
> error code, -EIO, etc. req->error carries protocol specific errors;
> SAM_STAT_*, DID_*, etc in the case of SCSI. But I'm not confident.

Yes, that's it precisely.  The blk_end_request() carries a translated
UNIX error code for the user (usually -EIO).  req->error carries the
transport error in its full glory (although it may lack pieces that
won't fit into 32 bits).

James

Re: [PATCH 1/2] scsi_transport_fc: FC pass through support - revised II

[Boaz Harrosh]

Seokmann Ju wrote:
> On Oct 29, 2008, at 9:18 PM, FUJITA Tomonori wrote:
> 
>> On Wed, 29 Oct 2008 05:48:03 -0700
>> Seokmann Ju <seokmann.ju@qlogic.com> wrote:
>>
>>> From f32b386a61a23f408974f2289cd34200f59401e1 Mon Sep 17 00:00:00  
>>> 2001
>>> From: root <root@linux-atl-00.qlogic.com>
>>> Date: Tue, 28 Oct 2008 19:27:15 -0700
>>> Subject: [PATCH] scsi_transport_fc: FC pass through support
>>>
>>> This patch will add FC pass through support.
>>> The FC pass through support is service request handling mechanism
>>> for FC specification defined services including,
>>> - Link Services (Basic LS, Extended LS)
>>> - Generic Services (FC-CT - Common Transport)
>>> The support utilize BSG (Block layer SCSI Generic) interface with
>>> bidi (bi-directional) nature in handling the service requests.
>>>
>>> This patch added following featues in the support
>>> - FC service structure has defined to transport service requests
>>> - Handles the service request in asynchronous manner - LLD
>>> - Timeout capability
>>> - Abort capability
>>>
>>> Signed-off-by: Seokmann Ju <seokmann.ju@qlogic.com>
>>> ---
>>>  drivers/scsi/scsi_transport_fc.c |  216 +++++++++++++++++++++++++++ 
>>> ++
>>> ++++++++-
>>>  include/scsi/scsi_transport_fc.h |   81 ++++++++++++++-
>>>  2 files changed, 294 insertions(+), 3 deletions(-)
>> First, the patch looks corrupt (I can't apply this). I guess that tabs
>> are converted to spaces.
> Sorry, whenever I send patches, it makes me to worry about it... but,
> I'm still struggling to find what causes the problem.
> Sometimes it worked (guessing for shorter ones).
> I will submit the patch along with attachment next time.
> BTW: I use the Mail on my Mac... any comments on this problem would be
> greatly appreciated...
> 
>>> diff --git a/drivers/scsi/scsi_transport_fc.c b/drivers/scsi/
>>> scsi_transport_fc.c
>>> index 1e71abf..e26e8e0 100644
>>> --- a/drivers/scsi/scsi_transport_fc.c
>>> +++ b/drivers/scsi/scsi_transport_fc.c
<snip>
>>> +
>>> +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);
>>> +	}
>>> +
>>> +	service->req->errors = service->srv_reply.status;
>>> +	service->req->next_rq->errors = service->srv_reply.status;
>> We don't need to save service->srv_reply.status at two places
>> service->req is appropriate.
> OK. It is removed.
> 
>>> +	blk_end_bidi_request(service->req, service->srv_reply.status,
>>> +	    blk_rq_bytes(service->req),
>>> +	    service->req->next_rq ? blk_rq_bytes(service->req->next_rq) :  
>>> 0);

>From what James said I understand that you should do:
+	blk_end_bidi_request(service->req, service->req->errors ? -EIO : 0,
+		blk_rq_bytes(service->req),
+		service->req->next_rq ? blk_rq_bytes(service->req->next_rq) : 0);

>>> +
>>> +	kfree(service->payload_dma);
>>> +	kfree(service->response_dma);
>>> +	kfree(service);
>>> +}
>>> +
<snip>

Boaz

Re: [PATCH 1/2] scsi_transport_fc: FC pass through support - revised II

[Seokmann Ju]

On Oct 30, 2008, at 7:12 AM, Boaz Harrosh wrote:

> Seokmann Ju wrote:
>> On Oct 29, 2008, at 9:18 PM, FUJITA Tomonori wrote:
>>
>>> On Wed, 29 Oct 2008 05:48:03 -0700
>>> Seokmann Ju <seokmann.ju@qlogic.com> wrote:
>>>
>>>> From f32b386a61a23f408974f2289cd34200f59401e1 Mon Sep 17 00:00:00
>>>> 2001
>>>> From: root <root@linux-atl-00.qlogic.com>
>>>> Date: Tue, 28 Oct 2008 19:27:15 -0700
>>>> Subject: [PATCH] scsi_transport_fc: FC pass through support
>>>>
>>>> This patch will add FC pass through support.
>>>> The FC pass through support is service request handling mechanism
>>>> for FC specification defined services including,
>>>> - Link Services (Basic LS, Extended LS)
>>>> - Generic Services (FC-CT - Common Transport)
>>>> The support utilize BSG (Block layer SCSI Generic) interface with
>>>> bidi (bi-directional) nature in handling the service requests.
>>>>
>>>> This patch added following featues in the support
>>>> - FC service structure has defined to transport service requests
>>>> - Handles the service request in asynchronous manner - LLD
>>>> - Timeout capability
>>>> - Abort capability
>>>>
>>>> Signed-off-by: Seokmann Ju <seokmann.ju@qlogic.com>
>>>> ---
>>>> drivers/scsi/scsi_transport_fc.c |  216 +++++++++++++++++++++++++++
>>>> ++
>>>> ++++++++-
>>>> include/scsi/scsi_transport_fc.h |   81 ++++++++++++++-
>>>> 2 files changed, 294 insertions(+), 3 deletions(-)
>>> First, the patch looks corrupt (I can't apply this). I guess that  
>>> tabs
>>> are converted to spaces.
>> Sorry, whenever I send patches, it makes me to worry about it... but,
>> I'm still struggling to find what causes the problem.
>> Sometimes it worked (guessing for shorter ones).
>> I will submit the patch along with attachment next time.
>> BTW: I use the Mail on my Mac... any comments on this problem would  
>> be
>> greatly appreciated...
>>
>>>> diff --git a/drivers/scsi/scsi_transport_fc.c b/drivers/scsi/
>>>> scsi_transport_fc.c
>>>> index 1e71abf..e26e8e0 100644
>>>> --- a/drivers/scsi/scsi_transport_fc.c
>>>> +++ b/drivers/scsi/scsi_transport_fc.c
> <snip>
>>>> +
>>>> +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);
>>>> +	}
>>>> +
>>>> +	service->req->errors = service->srv_reply.status;
>>>> +	service->req->next_rq->errors = service->srv_reply.status;
>>> We don't need to save service->srv_reply.status at two places
>>> service->req is appropriate.
>> OK. It is removed.
>>
>>>> +	blk_end_bidi_request(service->req, service->srv_reply.status,
>>>> +	    blk_rq_bytes(service->req),
>>>> +	    service->req->next_rq ? blk_rq_bytes(service->req->next_rq) :
>>>> 0);
>
> From what James said I understand that you should do:
> +	blk_end_bidi_request(service->req, service->req->errors ? -EIO : 0,
> +		blk_rq_bytes(service->req),
> +		service->req->next_rq ? blk_rq_bytes(service->req->next_rq) : 0);
Yes, that is right. I will make the change accordingly.
Thanks for the comment.

>>>> +
>>>> +	kfree(service->payload_dma);
>>>> +	kfree(service->response_dma);
>>>> +	kfree(service);
>>>> +}
>>>> +
> <snip>
>
> Boaz

Re: [PATCH 1/2] scsi_transport_fc: FC pass through support - revised II

[FUJITA Tomonori]

On Thu, 30 Oct 2008 06:37:41 -0700
Seokmann Ju <seokmann.ju@qlogic.com> wrote:

> > btw, tha name 'payload_dma' sounds a bit odd to me. Software FCoE
> > drivers doesn't do dma for data transfer, right?
> OK. The type case has removed.
> Any suggestion on the name change?
> Please let me know if you have it. I will take it.

sg_req and sg_rsp or sg_in and sg_out?

Re: [PATCH 1/2] scsi_transport_fc: FC pass through support - revised II

[Seokmann Ju]

On Oct 30, 2008, at 6:37 AM, Seokmann Ju wrote:

>
> On Oct 29, 2008, at 9:18 PM, FUJITA Tomonori wrote:
[snip]
>
>
>>> +	    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);
>>
>> You don't need service->payload. We use scatterlist for data  
>> transfer.
> Yes. Removed.
I need a step back here.
I understand that modules get involved in handling pass through are  
treat
FC service requests as opaque data.
However, there are some case where LLDs need to retrieve the contents of
the service request and pull out some information - for example, opcode
of the ELS for the case of qla2xxx.
I guess there might be some other LLDs that need to pull out some
information from the service request for similar reasons.
For that purpose, I think it would be good to keep 'payload' and  
'response'
field instead.
Any comment?

>
>
>>> +	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);
>>
>> Ditto.
> OK.
Same here.

Seokmann

Re: [PATCH 1/2] scsi_transport_fc: FC pass through support - revised II

[FUJITA Tomonori]

On Thu, 30 Oct 2008 18:38:04 -0700
Seokmann Ju <seokmann.ju@qlogic.com> wrote:

> 
> On Oct 30, 2008, at 6:37 AM, Seokmann Ju wrote:
> 
> >
> > On Oct 29, 2008, at 9:18 PM, FUJITA Tomonori wrote:
> [snip]
> >
> >
> >>> +	    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);
> >>
> >> You don't need service->payload. We use scatterlist for data  
> >> transfer.
> > Yes. Removed.
> I need a step back here.
> I understand that modules get involved in handling pass through are  
> treat
> FC service requests as opaque data.
> However, there are some case where LLDs need to retrieve the contents of
> the service request and pull out some information - for example, opcode
> of the ELS for the case of qla2xxx.
> I guess there might be some other LLDs that need to pull out some
> information from the service request for similar reasons.
> For that purpose, I think it would be good to keep 'payload' and  
> 'response'
> field instead.
> Any comment?

Simply, you can't access to the data via bio_data(). You must access
to the scatter gather in a proper way.

For example, try to do the following way to allocate a request buffer
in user space:

struct sg_io_v4 sg_io;
char *p = valloc(8192);
struct fc_els_frame *els = (struct fc_els_frame *)(p + 4086);

sg_io.dout_xferp = (__u64) &els;


fc_els_frame crosses two page frames. bio_data() can't handle it.


If a driver wants the payload in a linear buffer, sg_copy_to_buffer()
is helpful.

Re: [PATCH 1/2] scsi_transport_fc: FC pass through support - revised II

[Seokmann Ju]

On Oct 30, 2008, at 6:57 PM, FUJITA Tomonori wrote:

> On Thu, 30 Oct 2008 18:38:04 -0700
> Seokmann Ju <seokmann.ju@qlogic.com> wrote:
>
>>
>> On Oct 30, 2008, at 6:37 AM, Seokmann Ju wrote:
>>
>>>
>>> On Oct 29, 2008, at 9:18 PM, FUJITA Tomonori wrote:
>> [snip]
>>>
>>>
>>>>> +	    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);
>>>>
>>>> You don't need service->payload. We use scatterlist for data
>>>> transfer.
>>> Yes. Removed.
>> I need a step back here.
>> I understand that modules get involved in handling pass through are
>> treat
>> FC service requests as opaque data.
>> However, there are some case where LLDs need to retrieve the  
>> contents of
>> the service request and pull out some information - for example,  
>> opcode
>> of the ELS for the case of qla2xxx.
>> I guess there might be some other LLDs that need to pull out some
>> information from the service request for similar reasons.
>> For that purpose, I think it would be good to keep 'payload' and
>> 'response'
>> field instead.
>> Any comment?
>
> Simply, you can't access to the data via bio_data(). You must access
> to the scatter gather in a proper way.
>
> For example, try to do the following way to allocate a request buffer
> in user space:
>
> struct sg_io_v4 sg_io;
> char *p = valloc(8192);
> struct fc_els_frame *els = (struct fc_els_frame *)(p + 4086);
>
> sg_io.dout_xferp = (__u64) &els;
>
>
> fc_els_frame crosses two page frames. bio_data() can't handle it.
I see..
This is a great tip to know. Thank you for explanation.

> If a driver wants the payload in a linear buffer, sg_copy_to_buffer()
> is helpful.
OK. Using the sg_copy_to_buffer() is the right way to access the
contents if needed.
I will keep the changes as initially committed.

Thank you,
Seokmann