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] [day] [month] [year] [list]
Message-ID:
 <SJ0PR11MB5896CE3F203F4904D3B0D20CC3BA2@SJ0PR11MB5896.namprd11.prod.outlook.com>
Date: Fri, 9 Aug 2024 16:36:04 +0000
From: "Karan Tilak Kumar (kartilak)" <kartilak@...co.com>
To: John Garry <john.g.garry@...cle.com>,
        "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 07/14] scsi: fnic: Add and integrate support for FIP

On Thursday, June 13, 2024 9:17 PM, John Garry <john.g.garry@...cle.com> wrote:
> 
> On 10/06/2024 22:50, Karan Tilak Kumar wrote:
> > Add and integrate support for FCoE Initialization
> > (protocol) FIP. This protocol will be exercised on
> > Cisco UCS rack servers.
> > Add support to specifically print FIP related
> > debug messages.
> > Replace existing definitions to handle new
> > data structures.
> > Clean up old and obsolete definitions.
> 
> > Reviewed-by: Sesidhar Baddela <sebaddel@...co.com>
> > Reviewed-by: Arulprabhu Ponnusamy <arulponn@...co.com>
> > Reviewed-by: Gian Carlo Boffa <gcboffa@...co.com>
> > Signed-off-by: Karan Tilak Kumar <kartilak@...co.com>
> > ---
> >   drivers/scsi/fnic/Makefile    |   1 +
> >   drivers/scsi/fnic/fip.c       | 875 +++++++++++++++++++++++++++++++++
> >   drivers/scsi/fnic/fip.h       | 341 +++++++++++++
> >   drivers/scsi/fnic/fnic.h      |  23 +-
> >   drivers/scsi/fnic/fnic_fcs.c  | 889 ++++++----------------------------
> >   drivers/scsi/fnic/fnic_main.c |  47 +-
> >   6 files changed, 1402 insertions(+), 774 deletions(-)
> >   create mode 100644 drivers/scsi/fnic/fip.c
> >   create mode 100644 drivers/scsi/fnic/fip.h
> > +
> > +/**
> > + * fnic_fcoe_process_vlan_resp
> > + *
> > + * Processes the vlan response from one FCF and populates VLAN list.
> > + * Will wait for responses from multiple FCFs until timeout.
> > + *
> > + * @param fnic driver instance
> > + * @param fiph received fip frame
> > + */
> > +
> > +void fnic_fcoe_process_vlan_resp(struct fnic *fnic,
> > +								 struct fip_header_s *fiph)
> > +{
> > +	struct fip_vlan_notif_s *vlan_notif = (struct fip_vlan_notif_s *) fiph;
> > +
> > +	struct fnic_stats *fnic_stats = &fnic->fnic_stats;
> > +	u16 vid;
> > +	int num_vlan = 0;
> > +	int cur_desc, desc_len;
> > +	struct fcoe_vlan *vlan;
> > +	struct fip_vlan_desc_s *vlan_desc;
> > +	unsigned long flags;
> > +
> > +	FNIC_FIP_DBG(KERN_INFO, fnic->lport->host, fnic->fnic_num,
> > +				 "fnic 0x%p got vlan resp\n", fnic);
> > +
> > +	desc_len = ntohs(vlan_notif->fip.desc_len);
> > +	FNIC_FIP_DBG(KERN_INFO, fnic->lport->host, fnic->fnic_num,
> > +				 "desc_len %d\n", desc_len);
> > +
> > +	spin_lock_irqsave(&fnic->vlans_lock, flags);
> > +
> > +	cur_desc = 0;
> > +	while (desc_len > 0) {
> > +		vlan_desc =
> > +			(struct fip_vlan_desc_s *) (((char *) vlan_notif->vlans_desc)
> > +										+ cur_desc * 4);
> > +
> > +		if (vlan_desc->type == FIP_TYPE_VLAN) {
> > +			if (vlan_desc->len != 1) {
> > +				FNIC_FIP_DBG(KERN_INFO, fnic->lport->host, fnic->fnic_num,
> > +					 "Invalid descriptor length(%x) in VLan response\n",
> > +					 vlan_desc->len);
> > +
> > +			}
> > +			num_vlan++;
> > +			vid = ntohs(vlan_desc->vlan);
> > +			FNIC_FIP_DBG(KERN_INFO, fnic->lport->host, fnic->fnic_num,
> > +						 "process_vlan_resp: FIP VLAN %d\n", vid);
> > +			vlan = kmalloc(sizeof(*vlan), GFP_ATOMIC);
> 
> 
> is this alloc under spinlock really required?

Yes John. There can be responses from multiple FCFs, with multiple vlans. 
Even though the responses are serialized via a frame queue,
it's not worth the effort of just dropping the lock for the
sake of allocating the vlan. Hope this clarifies.
Thanks again for your review.

> > +/**
> > + * fnic_handle_enode_ka_timer
> > + *
> > + * FIP node keep alive.
> > + *
> > + * @param data Opaque pointer to fnic struct
> > + */
> > +void fnic_handle_enode_ka_timer(struct timer_list *t)
> > +{
> > +	struct fnic *fnic = from_timer(fnic, t, enode_ka_timer);
> > +
> > +	struct fnic_iport_s *iport = &fnic->iport;
> > +	int fr_len;
> > +	struct fip_enode_ka_s enode_ka;
> > +	u64 enode_ka_tov;
> > +
> > +	if (iport->fip.state != FDLS_FIP_FLOGI_COMPLETE)
> > +		return;
> > +
> > +	if ((iport->selected_fcf.ka_disabled)
> > +		|| (iport->selected_fcf.fka_adv_period == 0)) {
> > +		return;
> > +	}
> > +
> > +	fr_len = sizeof(struct fip_enode_ka_s);
> > +
> > +	memcpy(&enode_ka, &fip_enode_ka_tmpl, fr_len);
> > +	memcpy(enode_ka.eth.smac, iport->hwmac, ETH_ALEN);
> > +	memcpy(enode_ka.eth.dmac, iport->selected_fcf.fcf_mac, ETH_ALEN);
> > +	memcpy(enode_ka.mac_desc.mac, iport->hwmac, ETH_ALEN);
> > +
> > +	fnic_send_fip_frame(iport, &enode_ka, fr_len);
> > +	enode_ka_tov = jiffies
> > +		+ msecs_to_jiffies(iport->selected_fcf.fka_adv_period);
> > +	mod_timer(&fnic->enode_ka_timer, round_jiffies(enode_ka_tov));
> > +}
> > +
> > +/**
> > + * fnic_handle_vn_ka_timer
> > + *
> > + * FIP virtual port keep alive.
> > + *
> > + * @param data Opaque pointer to fnic structure
> > + */
> > +
> > +void fnic_handle_vn_ka_timer(struct timer_list *t)
> > +{
> > +	struct fnic *fnic = from_timer(fnic, t, vn_ka_timer);
> > +
> > +	struct fnic_iport_s *iport = &fnic->iport;
> > +	int fr_len;
> > +	struct fip_vn_port_ka_s vn_port_ka;
> > +	u64 vn_ka_tov;
> > +	uint8_t fcid[3];
> > +
> > +	if (iport->fip.state != FDLS_FIP_FLOGI_COMPLETE)
> > +		return;
> > +
> > +	if ((iport->selected_fcf.ka_disabled)
> > +		|| (iport->selected_fcf.fka_adv_period == 0)) {
> > +		return;
> > +	}
> 
> exact same code is duplicated from fnic_handle_enode_ka_timer(), above

Thanks for this observation, John. 
The challenge with integrating them into one function using 
some arguments is that we would not know which timer elapsed.
Therefore, we decided to go with this approach of separate
timer functions. Hope this clarifies.

Regards,
Karan

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ