On Thursday, June 13, 2024 9:17 PM, John Garry <john.g.garry@xxxxxxxxxx> 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@xxxxxxxxx>
> > Reviewed-by: Arulprabhu Ponnusamy <arulponn@xxxxxxxxx>
> > Reviewed-by: Gian Carlo Boffa <gcboffa@xxxxxxxxx>
> > Signed-off-by: Karan Tilak Kumar <kartilak@xxxxxxxxx>
> > ---
> > 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
