lkml.org 
[lkml]   [2009]   [May]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [PATCH 03/11] block: add rq->resid_len
    From
    On Mon, 11 May 2009 09:18:20 -0500
    James Bottomley <James.Bottomley@HansenPartnership.com> wrote:

    > On Mon, 2009-05-11 at 14:49 +0900, FUJITA Tomonori wrote:
    > > On Mon, 11 May 2009 08:48:53 +0900
    > > Tejun Heo <tj@kernel.org> wrote:
    > >
    > > > Hello, Boaz.
    > > >
    > > > Boaz Harrosh wrote:
    > > >
    > > > > Hi Tejun, I've carefully reviewed these files which I know more
    > > > > about. The drivers/block files I've skipped, since I'm not familiar
    > > > > with this code.
    > > > >
    > > > > Except a small fallout, it looks very good. See some comments plus
    > > > > Ack/review below
    > > >
    > > > Thanks a lot for reviewing it closely. It's really nice to have
    > > > careful extra pair of eyes on the changes. :-)
    > > >
    > > > >> --- a/drivers/message/fusion/mptsas.c
    > > > >> +++ b/drivers/message/fusion/mptsas.c
    > > > >> @@ -1357,8 +1357,7 @@ static int mptsas_smp_handler(struct Scsi_Host *shost, struct sas_rphy *rphy,
    > > > >> smprep = (SmpPassthroughReply_t *)ioc->sas_mgmt.reply;
    > > > >> memcpy(req->sense, smprep, sizeof(*smprep));
    > > > >> req->sense_len = sizeof(*smprep);
    > > > >> - req->data_len = 0;
    > > > >> - rsp->data_len -= smprep->ResponseDataLength;
    > > > >> + rsp->resid_len = rsp->data_len - smprep->ResponseDataLength;
    > > > >> } else {
    > > > >> printk(MYIOC_s_ERR_FMT "%s: smp passthru reply failed to be returned\n",
    > > > >> ioc->name, __func__);
    > > > >
    > > > > I think original code was assuming full residual count on the else side
    > > > > (not MPT_IOCTL_STATUS_RF_VALID). So maybe add:
    > > > >
    > > > > + rsp->resid_len = rsp->data_len;
    > > >
    > > > Does resid_len make any sense w/ failed requests? I think we would be
    > > > better off with declaring residual count to be undefined on request
    > > > failure. Is there any place which depends on it?
    > >
    > > IIRC, I wrote the code. I think that this doesn't matter but it's
    > > better not to change the behavior unless Eric ack on this change
    > > (maybe LSI has some management binary that assume this behavior though
    > > it's unlikely).
    >
    > Actually, yes it does, for many possible reasons.
    >
    > The first being if the device is too stupid to report an actual sector
    > location the next best way of determining where the error occurred is
    > from the residual. We don't make use of this in kernel (perhaps we
    > should?) but some of the user space programs for CD/DVD burning do.

    This function is for SAS management protocol. Are there many possible
    reasons?


    \
     
     \ /
      Last update: 2009-05-11 17:07    [from the cache]
    ©2003-2011 Jasper Spaans