linux-kernel - RE: [PATCH 02/14] scsi: fnic: Add headers and definitions for FDLS

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID:
 <SJ0PR11MB5896CB425BF89696F52DF8E7C3C02@SJ0PR11MB5896.namprd11.prod.outlook.com>
Date: Wed, 12 Jun 2024 22:48:54 +0000
From: "Karan Tilak Kumar (kartilak)" <kartilak@...co.com>
To: Hannes Reinecke <hare@...e.de>,
        "Sesidhar Baddela (sebaddel)"
	<sebaddel@...co.com>
CC: "Arulprabhu Ponnusamy (arulponn)" <arulponn@...co.com>,
        "Dhanraj Jhawar
 (djhawar)" <djhawar@...co.com>,
        "Gian Carlo Boffa (gcboffa)"
	<gcboffa@...co.com>,
        "Masa Kai (mkai2)" <mkai2@...co.com>,
        "Satish Kharat
 (satishkh)" <satishkh@...co.com>,
        "jejb@...ux.ibm.com" <jejb@...ux.ibm.com>,
        "martin.petersen@...cle.com" <martin.petersen@...cle.com>,
        "linux-scsi@...r.kernel.org" <linux-scsi@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH 02/14] scsi: fnic: Add headers and definitions for FDLS

On Monday, June 10, 2024 11:54 PM, Hannes Reinecke <hare@...e.de> wrote:
>
> On 6/10/24 23:50, Karan Tilak Kumar wrote:
> > Fabric Discovery and Login Services (FDLS) provide functionality for
> > fnic to discover the fabric and target ports. It logs in to the target
> > ports from the initiator and creates target ports (tports).
> > This functionality is essential for port channel register state change
> > notification (PC-RSCN) handling and FC-NVME initiator role.
> >
> > This functionality is essential for eCPU hang or panic handling. In
> > cases where the eCPU in the UCS VIC (Unified Computing Services
> > Virtual Interface Card) hangs, a fabric log out is sent to the fabric.
> > Upon successful log out from the fabric, the IO path is failed over to
> > a new path.
> >
> Didn't you have the paragraph in the patch series description?
> Please don't duplicate it.

Yes, that's correct. Sure. I'll modify the patch description in the 
next version.

> Please tell me that 'OXID' doesn't mean what I think it does ...
> Hardcoded OXIDs for individual command types? Really?

The thinking during the design process was that each fabric related 
command needs only one OXID at a time. Since the calls to the fabric
are serial in nature, there may not be a need for a pool. In addition, 
debugging would be a little bit easier since we could just look at the 
OXID and immediately know what the command was, without the 
need for an FC trace to debug the issue.

However, based on your review feedback, we are evaluating a 
design for a pool-based approach.

> > +#define FC_CT_RFF_CMD          (0x1F02)
> > +
> > +#endif
>
> Duplicate macros for endian differences is quite error-prone, as you have to remember to always change both sides.
> I would prefer to have just one copy and use macros to access the values (eg always use get_unaligned_le16).

Sure. I'll fix this in the next version of changes.

> > +#define FC_ABTS_RCTL            0x81
> > +#define FNIC_ELS_ADISC_REQ      0x52
> > +#define FC_ELS_RJT_LOGICAL_BUSY 0x05
> > +#define FC_ELS_RJT_BUSY         0x09
> > +#define FC_ELS_RTV_REQ          0x0E
> > +
>
> There is already a copy of FC-LS definitions in include/uapi/scsi/fc/fc_els.h.
>
> Please ensure that you don't introduce duplicates.

Sure, makes sense. Thanks for pointing it out. I'll fix this in the next version of changes.

> htonll() macros are typically used in the network stack; please use get_unaligned_be64().

Sure. I'll fix this in the next version of changes.

> > +#define FNIC_FC_FRAME_UNSOLICITED(_fchdr)  (_fchdr->r_ctl == 0x22)
> > +#define FNIC_FC_FRAME_SOLICITED_DATA(_fchdr)    (_fchdr->r_ctl == 0x21)
> > +#define FNIC_FC_FRAME_SOLICITED_CTRL_REPLY(_fchdr)    (_fchdr->r_ctl == 0x23)
> > +#define FNIC_FC_FRAME_FCTL_LAST_END_SEQ(_fchdr)    (_fchdr->f_ctl == 0x98)
> > +#define FNIC_FC_FRAME_FCTL_LAST_END_SEQ_INT(_fchdr)    (_fchdr->f_ctl == 0x99)
> > +#define FNIC_FC_FRAME_FCTL_FIRST_LAST_SEQINIT(_fchdr)  (_fchdr->f_ctl == 0x29)
> > +#define FNIC_FC_FRAME_FC4_SCTL(_fchdr)    (_fchdr->r_ctl == 0x03)
> > +#define FNIC_FC_FRAME_TYPE_BLS(_fchdr) (_fchdr->type == 0x00) #define
> > +FNIC_FC_FRAME_TYPE_ELS(_fchdr) (_fchdr->type == 0x01) #define
> > +FNIC_FC_FRAME_TYPE_FC_GS(_fchdr) (_fchdr->type == 0x20) #define
> > +FNIC_FC_FRAME_CS_CTL(_fchdr) (_fchdr->cs_ctl == 0x00)
>
> Please use macro names instead of raw values here.

Sure. I'll fix this in the next version of changes.

> These seem to be standard FCoE frame definitions, for which we already have a copy in include/uapi/scsi/fc.
> Please use the definitions from there instead of introducing your own.

Sure. I'll use the standard definitions from the header files.

> > +#define fnic_del_fabric_timer_sync() {                                                     \
> > +   iport->fabric.del_timer_inprogress = 1;                                         \
> > +   spin_unlock_irqrestore(&fnic->fnic_lock, fnic->lock_flags);     \
> > +   del_timer_sync(&iport->fabric.retry_timer);                                     \
> > +   spin_lock_irqsave(&fnic->fnic_lock, fnic->lock_flags);          \
> > +   iport->fabric.del_timer_inprogress = 0;                                         \
> > +}
> > +
> Please, make this an inline function.
> > +#define fnic_del_tport_timer_sync() {                                                      \
> > +   tport->del_timer_inprogress = 1;                                                        \
> > +   spin_unlock_irqrestore(&fnic->fnic_lock, fnic->lock_flags);     \
> > +   del_timer_sync(&tport->retry_timer);                                            \
> > +   spin_lock_irqsave(&fnic->fnic_lock, fnic->lock_flags);          \
> > +   tport->del_timer_inprogress = 0;                                                        \
> > +}
> > +
> Same here.

Makes sense. I'll make these inline functions.

Regards,
Karan