netdev - Re: [net-next v3 05/15] iecm: Add basic netdevice functionality

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <fbe69fcb9714123f36b6ed6d03873b0e47f23500.camel@perches.com>
Date:   Thu, 25 Jun 2020 19:39:03 -0700
From:   Joe Perches <joe@...ches.com>
To:     Jeff Kirsher <jeffrey.t.kirsher@...el.com>, davem@...emloft.net
Cc:     Alice Michael <alice.michael@...el.com>, netdev@...r.kernel.org,
        nhorman@...hat.com, sassmann@...hat.com,
        Alan Brady <alan.brady@...el.com>,
        Phani Burra <phani.r.burra@...el.com>,
        Joshua Hay <joshua.a.hay@...el.com>,
        Madhu Chittim <madhu.chittim@...el.com>,
        Pavan Kumar Linga <pavan.kumar.linga@...el.com>,
        Donald Skidmore <donald.c.skidmore@...el.com>,
        Jesse Brandeburg <jesse.brandeburg@...el.com>,
        Sridhar Samudrala <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

> +				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?

> @@ -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.

> +		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.

> +		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.

> 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;