Re: [Open-FCoE] [PATCH RFC 2/5] qedf: Add QLogic FastLinQ offload FCoE driver framework.

[Hannes Reinecke]

On 01/09/2017 05:45 PM, Chad Dupuis wrote:
> 
> On Wed, 28 Dec 2016, 9:00am -, Hannes Reinecke wrote:
> 
>> On 12/23/2016 08:17 PM, Dupuis, Chad wrote:
>>> From: "Dupuis, Chad" 
>>>
>>> The QLogic FastLinQ Driver for FCoE (qedf) is the FCoE specific module
>>> for 41000 Series Converged Network Adapters by QLogic.
>>>
>>> This patch consists of following changes:
>>>   - MAINTAINERS Makefile and Kconfig changes for qedf
>>>   - PCI driver registration
>>>   - libfc/fcoe host level initialization
>>>   - SCSI host template initialization and callbacks
>>>   - Debugfs and log level infrastructure
>>>   - Link handling
>>>   - Firmware interface structures
>>>   - QED core module initialization
>>>   - Light L2 interface callbacks
>>>
>>> Signed-off-by: Nilesh Javali 
>>> Signed-off-by: Manish Rangankar 
>>> Signed-off-by: Saurav Kashyap 
>>> Signed-off-by: Chad Dupuis 
>>> ---
>>>  MAINTAINERS  |6 +
>>>  drivers/scsi/Kconfig |1 +
>>>  drivers/scsi/qedf/Kconfig|   11 +
>>>  drivers/scsi/qedf/Makefile   |5 +
>>>  drivers/scsi/qedf/qedf.h |  555 ++
>>>  drivers/scsi/qedf/qedf_attr.c|  165 ++
>>>  drivers/scsi/qedf/qedf_dbg.c |  192 +++
>>>  drivers/scsi/qedf/qedf_dbg.h |  153 ++
>>>  drivers/scsi/qedf/qedf_debugfs.c |  472 +
>>>  drivers/scsi/qedf/qedf_main.c| 3519 
>>> ++
>>>  drivers/scsi/qedf/qedf_version.h |   15 +
>>>  11 files changed, 5094 insertions(+)
>>>  create mode 100644 drivers/scsi/qedf/Kconfig
>>>  create mode 100644 drivers/scsi/qedf/Makefile
>>>  create mode 100644 drivers/scsi/qedf/qedf.h
>>>  create mode 100644 drivers/scsi/qedf/qedf_attr.c
>>>  create mode 100644 drivers/scsi/qedf/qedf_dbg.c
>>>  create mode 100644 drivers/scsi/qedf/qedf_dbg.h
>>>  create mode 100644 drivers/scsi/qedf/qedf_debugfs.c
>>>  create mode 100644 drivers/scsi/qedf/qedf_main.c
>>>  create mode 100644 drivers/scsi/qedf/qedf_version.h
>>>
>> [ .. ]
>>> +/* Returns true if we have a valid vlan, false otherwise */
>>> +static bool qedf_initiate_fipvlan_req(struct qedf_ctx *qedf)
>>> +{
>>> +   int rc;
>>> +
>>> +   if (atomic_read(>link_state) != QEDF_LINK_UP) {
>>> +   QEDF_ERR(&(qedf->dbg_ctx), "Link not up.\n");
>>> +   return  false;
>>> +   }
>>> +
>>> +   while (qedf->fipvlan_retries--) {
>>> +   if (qedf->vlan_id > 0)
>>> +   return true;
>> Some weird FCoE bridges (most notably HP VirtualConnect) return a VLAN
>> ID of '0'. Shouldn't you rather test for '>= 0' here?
> 
> Will look into this but isn't a VLAN ID of 0 not valid?
> 
Well, a VLAN ID of '0' indicates no VLAN at all but rather use the base
interface.
But you still will be seeing a VLAN ID '0' in the FIP VLAN response.


Cheers,

Hannes
-- 
Dr. Hannes ReineckeTeamlead Storage & Networking
h...@suse.de   +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Open-FCoE] [PATCH RFC 2/5] qedf: Add QLogic FastLinQ offload FCoE driver framework.

[Chad Dupuis]

On Wed, 28 Dec 2016, 9:00am -, Hannes Reinecke wrote:

> On 12/23/2016 08:17 PM, Dupuis, Chad wrote:
> > From: "Dupuis, Chad" 
> > 
> > The QLogic FastLinQ Driver for FCoE (qedf) is the FCoE specific module
> > for 41000 Series Converged Network Adapters by QLogic.
> > 
> > This patch consists of following changes:
> >   - MAINTAINERS Makefile and Kconfig changes for qedf
> >   - PCI driver registration
> >   - libfc/fcoe host level initialization
> >   - SCSI host template initialization and callbacks
> >   - Debugfs and log level infrastructure
> >   - Link handling
> >   - Firmware interface structures
> >   - QED core module initialization
> >   - Light L2 interface callbacks
> > 
> > Signed-off-by: Nilesh Javali 
> > Signed-off-by: Manish Rangankar 
> > Signed-off-by: Saurav Kashyap 
> > Signed-off-by: Chad Dupuis 
> > ---
> >  MAINTAINERS  |6 +
> >  drivers/scsi/Kconfig |1 +
> >  drivers/scsi/qedf/Kconfig|   11 +
> >  drivers/scsi/qedf/Makefile   |5 +
> >  drivers/scsi/qedf/qedf.h |  555 ++
> >  drivers/scsi/qedf/qedf_attr.c|  165 ++
> >  drivers/scsi/qedf/qedf_dbg.c |  192 +++
> >  drivers/scsi/qedf/qedf_dbg.h |  153 ++
> >  drivers/scsi/qedf/qedf_debugfs.c |  472 +
> >  drivers/scsi/qedf/qedf_main.c| 3519 
> > ++
> >  drivers/scsi/qedf/qedf_version.h |   15 +
> >  11 files changed, 5094 insertions(+)
> >  create mode 100644 drivers/scsi/qedf/Kconfig
> >  create mode 100644 drivers/scsi/qedf/Makefile
> >  create mode 100644 drivers/scsi/qedf/qedf.h
> >  create mode 100644 drivers/scsi/qedf/qedf_attr.c
> >  create mode 100644 drivers/scsi/qedf/qedf_dbg.c
> >  create mode 100644 drivers/scsi/qedf/qedf_dbg.h
> >  create mode 100644 drivers/scsi/qedf/qedf_debugfs.c
> >  create mode 100644 drivers/scsi/qedf/qedf_main.c
> >  create mode 100644 drivers/scsi/qedf/qedf_version.h
> > 
> [ .. ]
> > +/* Returns true if we have a valid vlan, false otherwise */
> > +static bool qedf_initiate_fipvlan_req(struct qedf_ctx *qedf)
> > +{
> > +   int rc;
> > +
> > +   if (atomic_read(>link_state) != QEDF_LINK_UP) {
> > +   QEDF_ERR(&(qedf->dbg_ctx), "Link not up.\n");
> > +   return  false;
> > +   }
> > +
> > +   while (qedf->fipvlan_retries--) {
> > +   if (qedf->vlan_id > 0)
> > +   return true;
> Some weird FCoE bridges (most notably HP VirtualConnect) return a VLAN
> ID of '0'. Shouldn't you rather test for '>= 0' here?

Will look into this but isn't a VLAN ID of 0 not valid?

> 
> [ .. ]
> > +
> > +static void qedf_flogi_resp(struct fc_seq *seq, struct fc_frame *fp,
> > +   void *arg)
> > +{
> > +   struct fc_exch *exch = fc_seq_exch(seq);
> > +   struct fc_lport *lport = exch->lp;
> > +   struct qedf_ctx *qedf = lport_priv(lport);
> > +
> > +   if (!qedf) {
> > +   QEDF_ERR(NULL, "qedf is NULL.\n");
> > +   return;
> > +   }
> > +
> > +   /*
> > +* If ERR_PTR is set then don't try to stat anything as it will cause
> > +* a crash when we access fp.
> > +*/
> > +   if (fp == ERR_PTR(-FC_EX_TIMEOUT) ||
> > +   fp == ERR_PTR(-FC_EX_CLOSED)) {
> > +   QEDF_INFO(&(qedf->dbg_ctx), QEDF_LOG_ELS,
> > +   "fp has ERR_PTR() set.\n");
> > +   goto skip_stat;
> > +   }
> 
> Please use
> 
> if (IS_ERR(fp)) {
> 
> here instead of checking for individual error codes; if 'fp' has a
> different error value you'll continue with an invalid fp from here on.
>

Will fix up.
 
> [ .. ]
> 
> > +/**
> > + * qedf_xmit - qedf FCoE frame transmit function
> > + *
> > + */
> > +static int qedf_xmit(struct fc_lport *lport, struct fc_frame *fp)
> > +{
> > +   struct fc_lport *base_lport;
> > +   struct qedf_ctx *qedf;
> > +   struct ethhdr   *eh;
> > +   struct fcoe_crc_eof *cp;
> > +   struct sk_buff  *skb;
> > +   struct fc_frame_header  *fh;
> > +   struct fcoe_hdr *hp;
> > +   u8  sof, eof;
> > +   u32 crc;
> > +   unsigned inthlen, tlen, elen;
> > +   int wlen;
> > +   struct fc_stats *stats;
> > +   struct fc_lport *tmp_lport;
> > +   struct fc_lport *vn_port = NULL;
> > +   struct qedf_rport *fcport;
> > +   int rc;
> > +   u16 vlan_tci = 0;
> > +   unsigned long flags;
> > +
> > +   qedf = (struct qedf_ctx *)lport_priv(lport);
> > +
> > +   fh = fc_frame_header_get(fp);
> > +   skb = fp_skb(fp);
> > +
> > +   /* Filter out traffic to other NPIV ports on the same host */
> > +   if (lport->vport)
> > +   base_lport = shost_priv(vport_to_shost(lport->vport));
> > +   else
> > +   base_lport = lport;
> > +
> > +   /* Flag if the destination is the base port */
> > +   if (base_lport->port_id == ntoh24(fh->fh_d_id)) {
> > +   vn_port = 

Re: [Open-FCoE] [PATCH RFC 2/5] qedf: Add QLogic FastLinQ offload FCoE driver framework.

[Hannes Reinecke]

On 12/23/2016 08:17 PM, Dupuis, Chad wrote:
> From: "Dupuis, Chad" 
> 
> The QLogic FastLinQ Driver for FCoE (qedf) is the FCoE specific module
> for 41000 Series Converged Network Adapters by QLogic.
> 
> This patch consists of following changes:
>   - MAINTAINERS Makefile and Kconfig changes for qedf
>   - PCI driver registration
>   - libfc/fcoe host level initialization
>   - SCSI host template initialization and callbacks
>   - Debugfs and log level infrastructure
>   - Link handling
>   - Firmware interface structures
>   - QED core module initialization
>   - Light L2 interface callbacks
> 
> Signed-off-by: Nilesh Javali 
> Signed-off-by: Manish Rangankar 
> Signed-off-by: Saurav Kashyap 
> Signed-off-by: Chad Dupuis 
> ---
>  MAINTAINERS  |6 +
>  drivers/scsi/Kconfig |1 +
>  drivers/scsi/qedf/Kconfig|   11 +
>  drivers/scsi/qedf/Makefile   |5 +
>  drivers/scsi/qedf/qedf.h |  555 ++
>  drivers/scsi/qedf/qedf_attr.c|  165 ++
>  drivers/scsi/qedf/qedf_dbg.c |  192 +++
>  drivers/scsi/qedf/qedf_dbg.h |  153 ++
>  drivers/scsi/qedf/qedf_debugfs.c |  472 +
>  drivers/scsi/qedf/qedf_main.c| 3519 
> ++
>  drivers/scsi/qedf/qedf_version.h |   15 +
>  11 files changed, 5094 insertions(+)
>  create mode 100644 drivers/scsi/qedf/Kconfig
>  create mode 100644 drivers/scsi/qedf/Makefile
>  create mode 100644 drivers/scsi/qedf/qedf.h
>  create mode 100644 drivers/scsi/qedf/qedf_attr.c
>  create mode 100644 drivers/scsi/qedf/qedf_dbg.c
>  create mode 100644 drivers/scsi/qedf/qedf_dbg.h
>  create mode 100644 drivers/scsi/qedf/qedf_debugfs.c
>  create mode 100644 drivers/scsi/qedf/qedf_main.c
>  create mode 100644 drivers/scsi/qedf/qedf_version.h
> 
[ .. ]
> +/* Returns true if we have a valid vlan, false otherwise */
> +static bool qedf_initiate_fipvlan_req(struct qedf_ctx *qedf)
> +{
> + int rc;
> +
> + if (atomic_read(>link_state) != QEDF_LINK_UP) {
> + QEDF_ERR(&(qedf->dbg_ctx), "Link not up.\n");
> + return  false;
> + }
> +
> + while (qedf->fipvlan_retries--) {
> + if (qedf->vlan_id > 0)
> + return true;
Some weird FCoE bridges (most notably HP VirtualConnect) return a VLAN
ID of '0'. Shouldn't you rather test for '>= 0' here?

[ .. ]
> +
> +static void qedf_flogi_resp(struct fc_seq *seq, struct fc_frame *fp,
> + void *arg)
> +{
> + struct fc_exch *exch = fc_seq_exch(seq);
> + struct fc_lport *lport = exch->lp;
> + struct qedf_ctx *qedf = lport_priv(lport);
> +
> + if (!qedf) {
> + QEDF_ERR(NULL, "qedf is NULL.\n");
> + return;
> + }
> +
> + /*
> +  * If ERR_PTR is set then don't try to stat anything as it will cause
> +  * a crash when we access fp.
> +  */
> + if (fp == ERR_PTR(-FC_EX_TIMEOUT) ||
> + fp == ERR_PTR(-FC_EX_CLOSED)) {
> + QEDF_INFO(&(qedf->dbg_ctx), QEDF_LOG_ELS,
> + "fp has ERR_PTR() set.\n");
> + goto skip_stat;
> + }

Please use

if (IS_ERR(fp)) {

here instead of checking for individual error codes; if 'fp' has a
different error value you'll continue with an invalid fp from here on.

[ .. ]

> +/**
> + * qedf_xmit - qedf FCoE frame transmit function
> + *
> + */
> +static int qedf_xmit(struct fc_lport *lport, struct fc_frame *fp)
> +{
> + struct fc_lport *base_lport;
> + struct qedf_ctx *qedf;
> + struct ethhdr   *eh;
> + struct fcoe_crc_eof *cp;
> + struct sk_buff  *skb;
> + struct fc_frame_header  *fh;
> + struct fcoe_hdr *hp;
> + u8  sof, eof;
> + u32 crc;
> + unsigned inthlen, tlen, elen;
> + int wlen;
> + struct fc_stats *stats;
> + struct fc_lport *tmp_lport;
> + struct fc_lport *vn_port = NULL;
> + struct qedf_rport *fcport;
> + int rc;
> + u16 vlan_tci = 0;
> + unsigned long flags;
> +
> + qedf = (struct qedf_ctx *)lport_priv(lport);
> +
> + fh = fc_frame_header_get(fp);
> + skb = fp_skb(fp);
> +
> + /* Filter out traffic to other NPIV ports on the same host */
> + if (lport->vport)
> + base_lport = shost_priv(vport_to_shost(lport->vport));
> + else
> + base_lport = lport;
> +
> + /* Flag if the destination is the base port */
> + if (base_lport->port_id == ntoh24(fh->fh_d_id)) {
> + vn_port = base_lport;
> + } else {
> + /* Got through the list of vports attached to the base_lport
> +  * and see if we have a match with the destination address.
> +  */
> + list_for_each_entry(tmp_lport, _lport->vports, list) {
> +