| Message ID | 1477084253-1742-1-git-send-email-cleech@redhat.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
On Fri, 2016-10-21 at 14:10 -0700, Chris Leech wrote: > With the error message I added in "libfc: sanity check cpu number > extracted from xid" I didn't account for the fact that fc_exch_find is > called with FC_XID_UNKNOWN at the start of a new exchange if we are the > responder. > > It doesn't come up with the initiator much, but that's basically every > exchange for a target. By checking the xid for FC_XID_UNKNOWN first, we > not only prevent the erroneous error message, but skip the unnecessary > lookup attempt as well. > > Signed-off-by: Chris Leech <cleech@redhat.com> > --- > drivers/scsi/libfc/fc_exch.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/scsi/libfc/fc_exch.c b/drivers/scsi/libfc/fc_exch.c > index 16ca31a..42cc403 100644 > --- a/drivers/scsi/libfc/fc_exch.c > +++ b/drivers/scsi/libfc/fc_exch.c > @@ -910,6 +910,9 @@ static struct fc_exch *fc_exch_find(struct fc_exch_mgr *mp, u16 xid) > struct fc_exch *ep = NULL; > u16 cpu = xid & fc_cpu_mask; > > + if (xid == FC_XID_UNKNOWN) > + return NULL; > + > if (cpu >= nr_cpu_ids || !cpu_possible(cpu)) { > printk_ratelimited(KERN_ERR > "libfc: lookup request for XID = %d, " Reviewed-by: Ewan D. Milne <emilne@redhat.com> -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/21/2016 11:10 PM, Chris Leech wrote: > With the error message I added in "libfc: sanity check cpu number > extracted from xid" I didn't account for the fact that fc_exch_find is > called with FC_XID_UNKNOWN at the start of a new exchange if we are the > responder. > > It doesn't come up with the initiator much, but that's basically every > exchange for a target. By checking the xid for FC_XID_UNKNOWN first, we > not only prevent the erroneous error message, but skip the unnecessary > lookup attempt as well. > > Signed-off-by: Chris Leech <cleech@redhat.com> > --- > drivers/scsi/libfc/fc_exch.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/scsi/libfc/fc_exch.c b/drivers/scsi/libfc/fc_exch.c > index 16ca31a..42cc403 100644 > --- a/drivers/scsi/libfc/fc_exch.c > +++ b/drivers/scsi/libfc/fc_exch.c > @@ -910,6 +910,9 @@ static struct fc_exch *fc_exch_find(struct fc_exch_mgr *mp, u16 xid) > struct fc_exch *ep = NULL; > u16 cpu = xid & fc_cpu_mask; > > + if (xid == FC_XID_UNKNOWN) > + return NULL; > + > if (cpu >= nr_cpu_ids || !cpu_possible(cpu)) { > printk_ratelimited(KERN_ERR > "libfc: lookup request for XID = %d, " > Does that still apply with my libfc patchset? I was under the impression I've fixed it already ... Cheers, Hannes
On Mon, Oct 24, 2016 at 04:37:59PM +0200, Hannes Reinecke wrote: > On 10/21/2016 11:10 PM, Chris Leech wrote: > >With the error message I added in "libfc: sanity check cpu number > >extracted from xid" I didn't account for the fact that fc_exch_find is > >called with FC_XID_UNKNOWN at the start of a new exchange if we are the > >responder. > > > >It doesn't come up with the initiator much, but that's basically every > >exchange for a target. By checking the xid for FC_XID_UNKNOWN first, we > >not only prevent the erroneous error message, but skip the unnecessary > >lookup attempt as well. > > > >Signed-off-by: Chris Leech <cleech@redhat.com> > >--- > > drivers/scsi/libfc/fc_exch.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > >diff --git a/drivers/scsi/libfc/fc_exch.c b/drivers/scsi/libfc/fc_exch.c > >index 16ca31a..42cc403 100644 > >--- a/drivers/scsi/libfc/fc_exch.c > >+++ b/drivers/scsi/libfc/fc_exch.c > >@@ -910,6 +910,9 @@ static struct fc_exch *fc_exch_find(struct fc_exch_mgr *mp, u16 xid) > > struct fc_exch *ep = NULL; > > u16 cpu = xid & fc_cpu_mask; > > > >+ if (xid == FC_XID_UNKNOWN) > >+ return NULL; > >+ > > if (cpu >= nr_cpu_ids || !cpu_possible(cpu)) { > > printk_ratelimited(KERN_ERR > > "libfc: lookup request for XID = %d, " > > > Does that still apply with my libfc patchset? > I was under the impression I've fixed it already ... I believe so, although it might need refreshing to fix up the context change. It looks like you made a change specifically for REC, but in target mode with tcm_fc the first frame every new exchange comes in with RX_ID unknown until we assign one. Chris -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/25/2016 07:51 PM, Chris Leech wrote: > On Mon, Oct 24, 2016 at 04:37:59PM +0200, Hannes Reinecke wrote: >> On 10/21/2016 11:10 PM, Chris Leech wrote: >>> With the error message I added in "libfc: sanity check cpu number >>> extracted from xid" I didn't account for the fact that fc_exch_find is >>> called with FC_XID_UNKNOWN at the start of a new exchange if we are the >>> responder. >>> >>> It doesn't come up with the initiator much, but that's basically every >>> exchange for a target. By checking the xid for FC_XID_UNKNOWN first, we >>> not only prevent the erroneous error message, but skip the unnecessary >>> lookup attempt as well. >>> >>> Signed-off-by: Chris Leech <cleech@redhat.com> >>> --- >>> drivers/scsi/libfc/fc_exch.c | 3 +++ >>> 1 file changed, 3 insertions(+) >>> >>> diff --git a/drivers/scsi/libfc/fc_exch.c b/drivers/scsi/libfc/fc_exch.c >>> index 16ca31a..42cc403 100644 >>> --- a/drivers/scsi/libfc/fc_exch.c >>> +++ b/drivers/scsi/libfc/fc_exch.c >>> @@ -910,6 +910,9 @@ static struct fc_exch *fc_exch_find(struct fc_exch_mgr *mp, u16 xid) >>> struct fc_exch *ep = NULL; >>> u16 cpu = xid & fc_cpu_mask; >>> >>> + if (xid == FC_XID_UNKNOWN) >>> + return NULL; >>> + >>> if (cpu >= nr_cpu_ids || !cpu_possible(cpu)) { >>> printk_ratelimited(KERN_ERR >>> "libfc: lookup request for XID = %d, " >>> >> Does that still apply with my libfc patchset? >> I was under the impression I've fixed it already ... > > I believe so, although it might need refreshing to fix up the context > change. > > It looks like you made a change specifically for REC, but in target mode > with tcm_fc the first frame every new exchange comes in with RX_ID > unknown until we assign one. > Indeed; looks you're right. Reviewed-by: Hannes Reinecke <hare@suse.com> Cheers, Hannes
>>>>> "Chris" == Chris Leech <cleech@redhat.com> writes:
Chris> With the error message I added in "libfc: sanity check cpu number
Chris> extracted from xid" I didn't account for the fact that
Chris> fc_exch_find is called with FC_XID_UNKNOWN at the start of a new
Chris> exchange if we are the responder.
Chris> It doesn't come up with the initiator much, but that's basically
Chris> every exchange for a target. By checking the xid for
Chris> FC_XID_UNKNOWN first, we not only prevent the erroneous error
Chris> message, but skip the unnecessary lookup attempt as well.
Applied to 4.10/scsi-queue by hand as a result of all the libfc
changes. Please verify.
Thanks!
diff --git a/drivers/scsi/libfc/fc_exch.c b/drivers/scsi/libfc/fc_exch.c index 16ca31a..42cc403 100644 --- a/drivers/scsi/libfc/fc_exch.c +++ b/drivers/scsi/libfc/fc_exch.c @@ -910,6 +910,9 @@ static struct fc_exch *fc_exch_find(struct fc_exch_mgr *mp, u16 xid) struct fc_exch *ep = NULL; u16 cpu = xid & fc_cpu_mask; + if (xid == FC_XID_UNKNOWN) + return NULL; + if (cpu >= nr_cpu_ids || !cpu_possible(cpu)) { printk_ratelimited(KERN_ERR "libfc: lookup request for XID = %d, "
With the error message I added in "libfc: sanity check cpu number extracted from xid" I didn't account for the fact that fc_exch_find is called with FC_XID_UNKNOWN at the start of a new exchange if we are the responder. It doesn't come up with the initiator much, but that's basically every exchange for a target. By checking the xid for FC_XID_UNKNOWN first, we not only prevent the erroneous error message, but skip the unnecessary lookup attempt as well. Signed-off-by: Chris Leech <cleech@redhat.com> --- drivers/scsi/libfc/fc_exch.c | 3 +++ 1 file changed, 3 insertions(+)