On Thu, Oct 7, 2010 at 3:38 AM, Robert Love <[email protected]> wrote:
> On Wed, 2010-10-06 at 12:17 +0800, Hillf Danton wrote:
>> On Tue, Oct 5, 2010 at 7:16 AM, Robert Love <[email protected]> wrote:
>> > On Sun, 2010-09-26 at 07:06 -0700, Hillf Danton wrote:
>> >> The operation related to fr_seq(fp) seems unnecessary, since it is
>> >> also modified in fc_fill_hdr().
>> >> Or fr_seq(fp) should be computed after calling fc_fill_hdr().
>> >>
>> >> Signed-off-by: Hillf Danton <[email protected]>
>> >> ---
>> >>
>> >> --- o/linux-2.6.36-rc4/drivers/scsi/libfc/fc_libfc.c 2010-09-13
>> >> 07:07:38.000000000 +0800
>> >> +++ m/linux-2.6.36-rc4/drivers/scsi/libfc/fc_libfc.c 2010-09-26
>> >> 20:54:14.000000000 +0800
>> >> @@ -202,11 +202,6 @@ EXPORT_SYMBOL(fc_fill_hdr);
>> >> void fc_fill_reply_hdr(struct fc_frame *fp, const struct fc_frame *in_fp,
>> >> enum fc_rctl r_ctl, u32 parm_offset)
>> >> {
>> >> - struct fc_seq *sp;
>> >> -
>> >> - sp = fr_seq(in_fp);
>> >> - if (sp)
>> >> - fr_seq(fp) = fr_dev(in_fp)->tt.seq_start_next(sp);
>
> This assignment makes it so the sequence that is in in_fp is the same
> one in fp, so the old frame and the new frame actually share the same
> sequence.
>
>> >> fc_fill_hdr(fp, in_fp, r_ctl, FC_FCTL_RESP, 0, parm_offset);
>> >> }
>> >> EXPORT_SYMBOL(fc_fill_reply_hdr);
>> >
>> > Hi Hillf,
>> >
>> > I don't think you can remove this code. In the default case
>> > lport->tt.seq_start_next() will point to fc_seq_start_next() and will
>> > increment the seq_id. It will then be inserted into the new frame header
>> > in fc_fill_hdr().
>>
>> It seems what seq_start_next does is not inserted in fc_fill_hdr() but
>> replaced with fr_seq(in_fp), if fr_seq(in_fp) is not false.
>>
>
> Since the sequence is shared between fp and in_fp there is no
> replacement. The sequence with the incremented seq_id is used by the new
> fp.
>
Thank you for patience and guiding me to right direction, but
the incremented seq_id is not used by the new fp, as shown by
the following (in diff format for clear),
static struct fc_seq *fc_seq_start_next_locked(struct fc_seq *sp)
{
struct fc_exch *ep = fc_seq_exch(sp);
- sp = fc_seq_alloc(ep, ep->seq_id++);
+ sp = fc_seq_alloc(ep, ++ep->seq_id);
FC_EXCH_DBG(ep, "f_ctl %6x seq %2x\n",
ep->f_ctl, sp->id);
return sp;
}
and after calling fc_fill_hdr() it seems that the seq_id of
the new fp still has to be updated, if the incremented has to
be inserted.
> Also, keep in mind that fc_seq_start_next()/fc_seq_alloc() doesn't
> really _allocate_ a new sequence. It takes the exchange from the
> sequence (using container_of) and then updates the sequence within that
> exchange (the same sequence we started with).
>
> This confused me too when I was reviewing your patch. I think there is
> room for improvement to make the code more straight-forward. At least a
> comment would be nice.
>
> //Rob
>
>
>
_______________________________________________
devel mailing list
[email protected]
http://www.open-fcoe.org/mailman/listinfo/devel
