netdev - RE: [PATCH net v2 06/10] ice: Flesh out implementation of support for SRIOV on bonded interface

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]

Date: Thu, 8 Jun 2023 01:08:01 +0000
From: "Ertman, David M" <david.m.ertman@...el.com>
To: Daniel Machon <daniel.machon@...rochip.com>
CC: "intel-wired-lan@...ts.osuosl.org" <intel-wired-lan@...ts.osuosl.org>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: RE: [PATCH net v2 06/10] ice: Flesh out implementation of support for
 SRIOV on bonded interface

> -----Original Message-----
> From: Daniel Machon <daniel.machon@...rochip.com>
> Sent: Wednesday, June 7, 2023 1:42 AM
> To: Ertman, David M <david.m.ertman@...el.com>
> Cc: intel-wired-lan@...ts.osuosl.org; netdev@...r.kernel.org
> Subject: Re: [PATCH net v2 06/10] ice: Flesh out implementation of support for
> SRIOV on bonded interface
> 
> 
> Mostly tiny nits here. But since you are respinning, you might as well
> get that in.
> 
> > Add in the function s that will allow a VF created on the primary interface
> 
> s/function s/functions/

Change made

> 
> > of a bond to "fail-over" to another PF interface in the bond and continue
> > to Tx and Rx.
> >


> > +/**
> > + * ice_lag_qbuf_recfg - generate a buffer of queues for a reconfigure
> command
> > + * @hw: HW struct that contains the queue contexts
> > + * @qbuf: pointer to buffer to populate
> > + * @vsi_num: index of the VSI in PF space
> > + * @numq: number of queues to search for
> > + * @tc: traffic class that contains the queues
> > + *
> > + * function returns the numnber of valid queues in buffer
> 
> s/numnber/number/

Change made

> 
> > + */
> > +static u16
> > +ice_lag_qbuf_recfg(struct ice_hw *hw, struct ice_aqc_cfg_txqs_buf *qbuf,


> > +
> >  /**
> >   * ice_lag_move_vf_node_tc - move scheduling nodes for one VF on one TC
> >   * @lag: lag info struct
> > @@ -245,6 +353,167 @@ static void
> >  ice_lag_move_vf_node_tc(struct ice_lag *lag, u8 oldport, u8 newport,
> >                         u16 vsi_num, u8 tc)
> >  {
> > +       struct ice_sched_node *n_prt, *tc_node, *aggnode;
> > +       u16 num_nodes[ICE_AQC_TOPO_MAX_LEVEL_NUM] = { 0 };
> 
> RXT.

Moved the u16 num_nodes line into first position.

> 
> > +       u16 numq, valq, buf_size, num_moved, qbuf_size;
> > +       struct device *dev = ice_pf_to_dev(lag->pf);
> > +       struct ice_aqc_cfg_txqs_buf *qbuf;
> > +       struct ice_aqc_move_elem *buf;
> > +       struct ice_hw *new_hw = NULL;
> > +       struct ice_port_info *pi;


> > +       aggnode = ice_sched_get_agg_node(pi, tc_node, ICE_DFLT_AGG_ID);
> > +       if (!aggnode) {
> > +               dev_warn(dev, "Failure to find aggreagte node in failover tree\n");
> 
> s/aggreagte/aggregate/

Change made

> 
> > +               goto resume_reclaim;
> > +       }
> > +


> > +       if (!buf) {
> > +               dev_err(ice_pf_to_dev(local_lag->pf), "-ENONMEM error setting
> SWID\n");
> 
> s/ENONMEM/ENOMEM/

Change made

> 
> > +               return;
> > +       }
> > +
> > +       buf->num_elems = cpu_to_le16(1);
> > +       buf->res_type = cpu_to_le16(ICE_AQC_RES_TYPE_SWID);
> > +       /* if unlinnking need to free the shared resource */
> > +       if (!link && local_lag->bond_swid) {
> > +               buf->elem[0].e.sw_resp = cpu_to_le16(local_lag->bond_swid);
> > +               status = ice_aq_alloc_free_res(&local_lag->pf->hw, 1, buf,
> > +                                              buf_len, ice_aqc_opc_free_res,
> > +                                              NULL);
> > +               if (status)
> > +                       dev_err(ice_pf_to_dev(local_lag->pf), "Error freeing SWID duing
> LAG unlink\n");
> 
> s/duing/during/
> 

Change made.

> > +               local_lag->bond_swid = 0;
> > +       }
> > +
> > +       if (link) {
> >
> > +       kfree(new_rcp);
> >         return rid;
> >  }
> 
> You return 0 on error here. In patch #5, rid is checked against zero and
> -EINVAL is returned. Maybe you could make rid a function argument and
> just return the actual error codes here. That way the error is passed down
> the callstack. Just a suggestion - your call.

Seemed like a reasonable change - passing the actual error could be helpful
In future debugging.  Change Made Changes coming in v3.

And thanks for the reviews!