Re: [RFC] FC pass thru - Rev IV

[Sven Schuetz <sven@linux.vnet.ibm.com>]

James Smart wrote:

>- A new file was created, scsi_bsg_fc.h, which contains the
>  request/response data structures that should be shared
>  between the application and the kernel entities.

So we need to add this to include/scsi/Kbuild, right?

> +/**
> + * fc_bsg_jobdone - completion routine for bsg requests that the LLD has
> + *                  completed
> + * @job:	fc_bsg_job that is complete
> + */
> +static void
> +fc_bsg_jobdone(struct fc_bsg_job *job)
> +{
> +	struct request *req = job->req->next_rq;

Shouldn't this be just job->req?

> +	struct request *rsp = req->next_rq;
> +	unsigned long flags;
> +	int err;
> +
> +	spin_lock_irqsave(&job->job_lock, flags);
> +	job->state_flags |= FC_RQST_STATE_DONE;
> +	job->ref_cnt--;
> +	spin_unlock_irqrestore(&job->job_lock, flags);
> +
> +	err = job->req->errors = job->reply->result;
> +	if (err < 0)
> +		/* we're only returning the result field in the reply */
> +		job->req->sense_len = sizeof(uint32_t);
> +	else
> +		job->req->sense_len = job->reply_len;
> +
> +	/*
> +	 * we'll cheat: tell blk layer all of the xmt data was sent.
> +	 * but try to be honest about the amount of rcv data received
> +	 */
> +	if (rsp)
> +		blk_end_bidi_request(job->req, err, blk_rq_bytes(job->req),
> +	    			     job->reply->reply_payload_rcv_len);

Here is a problem when a LLD does not set job->reply->reply_payload_rcv_len, but
has created a response.
blk_end_bidi_request will be called with a length of 0 for the last parameter
and will thus return "1: still buffers pending for this request", which we do
not evaluate. A few lines later the job is destroyed. But a few seconds later
the timeout kicks in, leading to a crash (I suppose because the job has been
destroyed).
I see two options:
1. check if we have a response and see if a length has been set - but what do
for cases which have a response and no length set? throw the request away?
2. always use the blk_rq_bytes(job->req->next_rq) from Seokmanns initial patch
(which I prefer)

> +	else
> +		blk_end_request(job->req, err, blk_rq_bytes(job->req));
> +
> +	fc_destroy_bsgjob(job);
> +}
> +

<snip>

> +/**
> + * fc_bsg_rport_dispatch - process rport bsg requests and dispatch to LLDD
> + * @shost:	scsi host rport attached to
> + * @rport:	rport request destined to
> + * @job:	bsg job to be processed
> + */
> +static enum fc_dispatch_result
> +fc_bsg_rport_dispatch(struct request_queue *q, struct Scsi_Host *shost,
> +			 struct fc_rport *rport, struct fc_bsg_job *job)
> +{
> +	struct fc_internal *i = to_fc_internal(shost->transportt);
> +	int cmdlen = sizeof(uint32_t);	/* start with length of msgcode */
> +	int ret;
> +
> +	/* Validate the rport command */
> +	switch (job->request->msgcode) {
> +	case FC_BSG_RPT_ELS:
> +	case FC_BSG_RPT_CT:
> +		cmdlen += sizeof(struct fc_bsg_rport_els);

Why do we do the same here for ELS and CT? The struct fc_bsg_rport_ct is some
bytes larger than for the ELS and should get its own case.

> +		/* there better be a xmt and rcv payloads */
> +		if ((!job->request_payload.payload_len) ||
> +		    (!job->reply_payload.payload_len)) {
> +			ret = -EINVAL;
> +			goto fail_rport_msg;
> +		}
> +		break;
> +	default:
> +		ret = -EBADR;
> +		goto fail_rport_msg;
> +	}
> +
> +	/* check if we really have all the request data needed */
> +	if (job->request_len < cmdlen) {
> +		ret = -ENOMSG;
> +		goto fail_rport_msg;
> +	}
> +
> +	ret = i->f->bsg_request(job);
> +	if (ret) {
> +fail_rport_msg:
> +		/* return the errno failure code as the only status */
> +		BUG_ON(job->reply_len < sizeof(uint32_t));
> +		job->reply->result = ret;
> +		job->reply_len = sizeof(uint32_t);
> +		fc_bsg_jobdone(job);
> +		/* fall thru */
> +	}
> +
> +	return FC_DISPATCH_UNLOCKED;
> +}
> +
> +

<snip>
> +struct fc_bsg_job {
> +	struct Scsi_Host *shost;
> +	struct fc_rport *rport;
> +	struct device *dev;
> +	struct request *req;
> +	spinlock_t job_lock;
> +	unsigned int state_flags;
> +	unsigned int ref_cnt;
> +	void (*job_done)(struct fc_bsg_job *);
> +
> +	struct fc_bsg_request *request;
> +	struct fc_bsg_reply *reply;
> +	unsigned int request_len;
> +	unsigned int reply_len;
> +	/*
> +	 * On entry : reply_len indicates the buffer size allocated for
> +	 * the reply.
> +	 *
> +	 * Upon completion : the message handler must set reply_len
> +	 *  to indicates the size of the reply to be returned to the
> +	 *  caller.
> +	 */

Do we need this field (reply_len)? Both the fc_bsg_buffer struct (for the vector
i/o) as well as the fc_bsg_reply struct (for the sense data) contain fields for
the payload length.

> +
> +	/* DMA payloads for the request/response */
> +	struct fc_bsg_buffer request_payload;
> +	struct fc_bsg_buffer reply_payload;
> +
> +	void *dd_data;			/* Used for driver-specific storage */
> +};


Sven