| 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
| ||
|
Message-ID: <MW3PR11MB452247A68D61F34FE745358B8F930@MW3PR11MB4522.namprd11.prod.outlook.com>
Date: Fri, 26 Jun 2020 17:38:53 +0000
From: "Brady, Alan" <alan.brady@...el.com>
To: Joe Perches <joe@...ches.com>,
"Kirsher, Jeffrey T" <jeffrey.t.kirsher@...el.com>,
"davem@...emloft.net" <davem@...emloft.net>
CC: "Michael, Alice" <alice.michael@...el.com>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
"nhorman@...hat.com" <nhorman@...hat.com>,
"sassmann@...hat.com" <sassmann@...hat.com>,
"Burra, Phani R" <phani.r.burra@...el.com>,
"Hay, Joshua A" <joshua.a.hay@...el.com>,
"Chittim, Madhu" <madhu.chittim@...el.com>,
"Linga, Pavan Kumar" <pavan.kumar.linga@...el.com>,
"Skidmore, Donald C" <donald.c.skidmore@...el.com>,
"Brandeburg, Jesse" <jesse.brandeburg@...el.com>,
"Samudrala, Sridhar" <sridhar.samudrala@...el.com>
Subject: RE: [net-next v3 05/15] iecm: Add basic netdevice functionality
> -----Original Message-----
> From: Joe Perches <joe@...ches.com>
> Sent: Thursday, June 25, 2020 7:39 PM
> To: Kirsher, Jeffrey T <jeffrey.t.kirsher@...el.com>; davem@...emloft.net
> Cc: Michael, Alice <alice.michael@...el.com>; netdev@...r.kernel.org;
> nhorman@...hat.com; sassmann@...hat.com; Brady, Alan
> <alan.brady@...el.com>; Burra, Phani R <phani.r.burra@...el.com>; Hay,
> Joshua A <joshua.a.hay@...el.com>; Chittim, Madhu
> <madhu.chittim@...el.com>; Linga, Pavan Kumar
> <pavan.kumar.linga@...el.com>; Skidmore, Donald C
> <donald.c.skidmore@...el.com>; Brandeburg, Jesse
> <jesse.brandeburg@...el.com>; Samudrala, Sridhar
> <sridhar.samudrala@...el.com>
> Subject: Re: [net-next v3 05/15] iecm: Add basic netdevice functionality
>
> On Thu, 2020-06-25 at 19:07 -0700, Jeff Kirsher wrote:
> > From: Alice Michael <alice.michael@...el.com>
> >
> > This implements probe, interface up/down, and netdev_ops.
>
> trivial notes:
>
> > diff --git a/drivers/net/ethernet/intel/iecm/iecm_lib.c
> > b/drivers/net/ethernet/intel/iecm/iecm_lib.c
> []
> > @@ -194,7 +298,24 @@ static int iecm_vport_rel(struct iecm_vport *vport)
> > */
> > static void iecm_vport_rel_all(struct iecm_adapter *adapter) {
> > - /* stub */
> > + int err, i;
> > +
> > + if (!adapter->vports)
> > + return;
> > +
> > + for (i = 0; i < adapter->num_alloc_vport; i++) {
> > + if (!adapter->vports[i])
> > + continue;
> > +
> > + err = iecm_vport_rel(adapter->vports[i]);
> > + if (err)
> > + dev_dbg(&adapter->pdev->dev,
> > + "Failed to release adapter->vport[%d], err
> %d,\n",
>
> odd comma
Will fix.
>
> > + i, err);
> > + else
> > + adapter->vports[i] = NULL;
> > + }
> > + adapter->num_alloc_vport = 0;
>
> If one of these fails to release, why always set num_alloc_vport to 0?
>
Basically because if the other side fails to release a vport for us, there's not much the driver can do. After some internal discussion, we think it's better to change iecm_vport_rel to return void and report any issues in that function (we should still report in dmesg something appears to have gone wrong). This function will otherwise assume it succeeded since there's not much the driver can do if it fails anyway.
> > @@ -273,7 +483,40 @@ static void iecm_init_task(struct work_struct *work)
> > */
> > static int iecm_api_init(struct iecm_adapter *adapter) {
> > - /* stub */
> > + struct iecm_reg_ops *reg_ops = &adapter->dev_ops.reg_ops;
> > + struct pci_dev *pdev = adapter->pdev;
> > +
> > + if (!adapter->dev_ops.reg_ops_init) {
> > + dev_err(&pdev->dev, "Invalid device, register API init not
> > +defined.\n");
>
> inconsistent uses of periods after logging messages.
>
Good catch, will fix.
> > + return -EINVAL;
> > + }
> > + adapter->dev_ops.reg_ops_init(adapter);
> > + if (!(reg_ops->ctlq_reg_init && reg_ops->vportq_reg_init &&
> > + reg_ops->intr_reg_init && reg_ops->mb_intr_reg_init &&
> > + reg_ops->reset_reg_init && reg_ops->trigger_reset)) {
> > + dev_err(&pdev->dev, "Invalid device, missing one or more
> register
> > +functions\n");
>
> Most are without period.
Yes agreed, will remove ones with periods.
>
> > + return -EINVAL;
> > + }
> > +
> > + if (adapter->dev_ops.vc_ops_init) {
> > + struct iecm_virtchnl_ops *vc_ops;
> > +
> > + adapter->dev_ops.vc_ops_init(adapter);
> > + vc_ops = &adapter->dev_ops.vc_ops;
> > + if (!(vc_ops->core_init && vc_ops->vport_init &&
> > + vc_ops->vport_queue_ids_init && vc_ops->get_caps &&
> > + vc_ops->config_queues && vc_ops->enable_queues &&
> > + vc_ops->disable_queues && vc_ops->irq_map_unmap &&
> > + vc_ops->get_set_rss_lut && vc_ops->get_set_rss_hash &&
> > + vc_ops->adjust_qs && vc_ops->get_ptype)) {
>
> style trivia:
>
> Sometimes it's clearer for human readers if all the tests are separated on
> individual lines.
>
Agreed, will fix.
> > diff --git a/drivers/net/ethernet/intel/iecm/iecm_virtchnl.c
> > b/drivers/net/ethernet/intel/iecm/iecm_virtchnl.c
> []
> > @@ -594,6 +642,25 @@ static bool iecm_is_capability_ena(struct
> iecm_adapter *adapter, u64 flag)
> > */
> > void iecm_vc_ops_init(struct iecm_adapter *adapter) {
> > - /* stub */
>
> Maybe add a temporary for adapter->dev_ops.vc_ops to reduce visual clutter?
>
> > + adapter->dev_ops.vc_ops.core_init = iecm_vc_core_init;
> > + adapter->dev_ops.vc_ops.vport_init = iecm_vport_init;
> > + adapter->dev_ops.vc_ops.vport_queue_ids_init =
> > + iecm_vport_queue_ids_init;
> > + adapter->dev_ops.vc_ops.get_caps = iecm_send_get_caps_msg;
> > + adapter->dev_ops.vc_ops.is_cap_ena = iecm_is_capability_ena;
Not opposed to this one either, will fix.
Alan
Powered by blists - more mailing lists