Re: [patch 2/2] zfcp: Add FC pass-through support

[James Smart <James.Smart@Emulex.Com>]

Christof Schmitt wrote:
--- a/drivers/s390/scsi/zfcp_fc.c       2009-04-06 17:23:25.000000000 +0200
+++ b/drivers/s390/scsi/zfcp_fc.c       2009-04-06 17:24:30.000000000 +0200
...
> +static void zfcp_fc_generic_els_handler(unsigned long data)
> +{
> +       struct zfcp_els_fc_job *els_fc_job = (struct zfcp_els_fc_job *) data;
> +       struct fc_bsg_job *job = els_fc_job->job;
> +       struct fc_bsg_reply *reply = job->reply;
> +
> +       if (els_fc_job->els.status) {
> +               /* request rejected or timed out */
> +               reply->reply_data.ctels_reply.status = FC_CTELS_STATUS_REJECT;
> +               goto out;
> +       }
> +
> +       reply->reply_data.ctels_reply.status = FC_CTELS_STATUS_OK;
> +       reply->reply_payload_rcv_len = blk_rq_bytes(job->req->next_rq);

My intent was to not have the LLDD reference (or even know about) the
request structures and that bsg exists underneath.

Thus, for the last line above, it should be:
           reply->reply_payload_rcv_len = job->reply_payload->data_len;

Although, I'm a little surprised that your els interface doesn't supply the
received data length to you, rather than the global assumption that you sized 
the reply buffer for exactly what you always get.


> +struct zfcp_ct_fc_job {
> +       struct zfcp_send_ct ct;
> +       struct fc_bsg_job *job;
> +};
> +
> +static void zfcp_fc_generic_ct_handler(unsigned long data)
> +{
> +       struct zfcp_ct_fc_job *ct_fc_job = (struct zfcp_ct_fc_job *) data;
> +       struct fc_bsg_job *job = ct_fc_job->job;
> +
> +       job->reply->reply_data.ctels_reply.status = ct_fc_job->ct.status ?
> +                               FC_CTELS_STATUS_REJECT : FC_CTELS_STATUS_OK;
> +       job->state_flags = FC_RQST_STATE_DONE;
> +       job->reply->reply_payload_rcv_len = blk_rq_bytes(job->req->next_rq);

same comment as above.

> +       job->job_done(job);
> +
> +       zfcp_wka_port_put(ct_fc_job->ct.wka_port);
> +
> +       kfree(ct_fc_job);
> +}

Are you going to add support for the ELS/CT request timing out (e.g. for
bsg_timeout callback) ?

Everything else looks good, and looks like it wasn't much effort to support.
Great.

-- james s