On Thu, 27 Nov 2008 10:58:43 +0200
Boaz Harrosh <bharrosh@xxxxxxxxxxx> wrote:
> FUJITA Tomonori wrote:
> > On Tue, 18 Nov 2008 16:24:58 -0500
> > James Smart <James.Smart@xxxxxxxxxx> wrote:
> >
> >> All,
> >>
> >> I've reworked Seokmann's patch for the following items:
> >> - Add an fchost interface for bsg requests
> >>
> >> - Formalized the request/response structures that I expect
> >> to have us stuff into the bsg cmd/sense data areas. These
> >> are now genericized so we can essentially pass any kind of
> >> transaction. It can be a request that has no transmit or
> >> receive payload, and simply returns a response.
> >>
> >> - 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.
> >>
> >> - I stripped out some things that were in the request
> >> structure that were actually LLD fields. Instead, I added
> >> a dd_bsgsize structure to the template, so the transport
> >> will allocate LLD work space along with the job structure.
> >> I expect the missing fields to move to this area.
> >>
> >> - I've made a strong attempt at ensuring that the request
> >> has all the information necessary for the LLD, so that
> >> there is no need to have the LLD remap the transmit payload
> >> to figure things out. Granted, this comes at the cost of
> >> replicating some data items.
> >>
> >> Sven, I've added the CT information you needed as part of this.
> >>
> >> - I've renamed things quite a bit, hoping to make it clarity
> >> better. The "service" struct is now a job. I still have
> >> headaches with "request" (is it the blk request, or the job
> >> request, or what..)
> >>
> >> - The CT/ELS response is a bit funky. I've noted that the
> >> way Emulex returns a response, vs Qlogic is a bit different,
> >> thus the 2 ways to indicate "reject".
> >>
> >> - fixed a couple of bugs in Seokmann's code, in the teardown,
> >> error flows, request que dma settings, etc.
> >>
> >> - I added a "vendor_id" field to the scsi_host_template to
> >> use when verifying that the recipient knows how to decode
> >> vendor-specific message. I didn't do this with the netlink
> >> things as I was prepping it to not break kabi in existing
> >> and older kernels. But, I believe this is a good time to
> >> add it.
> >>
> >> - I've started the Documentation/scsi/scsi_transport_fc.txt
> >> documentation, but punted finishing it in lieu of sending
> >> this RFC. I'm starting from Seokman's original emails and
> >> will be updating for this reformat.
> >>
> >> I'm only starting to debug this, so user beware.
> >>
> >> I could really use some code review from Fujita or Boaz, to
> >> make sure I'm calling the right blk_xx completion functions
> >> relative to the setup flow, and to ensure that the "goose"
> >> when I jump out while the rport is blocked is correct.
> >>
> >> Comments welcome
> >>
> >> -- james s
> >>
> >>
> >>
> >> Signed-off-by: James Smart <james.smart@xxxxxxxxxx>
> >>
> >> ---
> >>
> >> Documentation/scsi/scsi_fc_transport.txt | 11
> >> Documentation/scsi/scsi_mid_low_api.txt | 5
> >> drivers/scsi/scsi_transport_fc.c | 581 ++++++++++++++++++++++++++++++-
> >> include/scsi/scsi_bsg_fc.h | 291 +++++++++++++++
> >> include/scsi/scsi_host.h | 9
> >> include/scsi/scsi_transport_fc.h | 53 ++
> >> 6 files changed, 946 insertions(+), 4 deletions(-)
> >
> > (snip)
> >
> >> +/**
> >> + * 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;
> >> + struct request *rsp = req->next_rq;
> >> + unsigned long flags;
> >> + int err;
>
> + unsigned bytes_requested = 0;
>
> >> +
> >> + 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);
> >> + else
> >> + blk_end_request(job->req, err, blk_rq_bytes(job->req));
> >
> > I think that you can use blk_end_bidi_request() for non-bidi requests:
> >
> > blk_end_bidi_request(job->req, err, blk_rq_bytes(job->req),
> > rsp ?
> > job->reply->reply_payload_rcv_len : 0);
> >
> >
> > I guess that it would be better to have one function to complete a
> > request, instead of blk_end_bidi_request and blk_end_request.
> >
> >
>
> + /*
> + * tell blk layer all of the xmt data was sent.
> + * but set residual count to: requested - received
> + */
> +
> + if (rsp) {
> + bytes_requested = blk_rq_bytes(rsp);
> + rsp->data_len = bytes_requested - job->reply->reply_payload_rcv_len;
> + }
> +
> + blk_end_bidi_request(job->req, err, blk_rq_bytes(job->req), bytes_requested);
>
> The residual count is left in req->data_len. Does bsg have a way to return the
> residual to user-mode? It must, since Pete was using that for sure. Note that
> you are looking for the bidi_read residual count.
Yeah, bsg has. struct sg_io_v4 has:
__s32 din_resid; /* [o] din_xfer_len - actual_din_xfer_len */
__s32 dout_resid; /* [o] dout_xfer_len - actual_dout_xfer_len */
> As was said by people. You must complete ALL bytes on both sides. Residual information
> is passed through req->data_len. Other wise the request is still active.
>
> (And yes blk_end_request uses blk_end_bidi_request internally)
We always complete all bytes on both sides. So why we do something
like:
int blk_end_request(struct request *rq, int error, unsigned int nr_bytes)
{
unsigned int bidi_bytes = 0;
if (blk_bidi_rq(rq))
bidi_bytes = req->next_rq->data_len;
return blk_end_io(rq, error, nr_bytes, bidi_bytes, NULL);
}
The callers can do something like:
blk_end_request(rq, err, rq->data_len);
rq-->next_rq->data_len = resid;
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
