netdev - Re: [net-next v3 13/15] iecm: Add ethtool

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]

Message-ID: <20200626121440.179db33c@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date:   Fri, 26 Jun 2020 12:14:40 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Jeff Kirsher <jeffrey.t.kirsher@...el.com>
Cc:     davem@...emloft.net, 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 13/15] iecm: Add ethtool

On Thu, 25 Jun 2020 19:07:35 -0700 Jeff Kirsher wrote:
> diff --git a/drivers/net/ethernet/intel/iecm/iecm_lib.c b/drivers/net/ethernet/intel/iecm/iecm_lib.c
> index d34834422049..a55151495e18 100644
> --- a/drivers/net/ethernet/intel/iecm/iecm_lib.c
> +++ b/drivers/net/ethernet/intel/iecm/iecm_lib.c
> @@ -765,7 +765,37 @@ static void iecm_deinit_task(struct iecm_adapter *adapter)
>  static enum iecm_status
>  iecm_init_hard_reset(struct iecm_adapter *adapter)
>  {
> -	/* stub */
> +	enum iecm_status err;
> +
> +	/* Prepare for reset */
> +	if (test_bit(__IECM_HR_FUNC_RESET, adapter->flags)) {
> +		iecm_deinit_task(adapter);
> +		adapter->dev_ops.reg_ops.trigger_reset(adapter,
> +						       __IECM_HR_FUNC_RESET);
> +		set_bit(__IECM_UP_REQUESTED, adapter->flags);
> +		clear_bit(__IECM_HR_FUNC_RESET, adapter->flags);
> +	} else if (test_bit(__IECM_HR_CORE_RESET, adapter->flags)) {
> +		if (adapter->state == __IECM_UP)
> +			set_bit(__IECM_UP_REQUESTED, adapter->flags);
> +		iecm_deinit_task(adapter);
> +		clear_bit(__IECM_HR_CORE_RESET, adapter->flags);
> +	} else if (test_and_clear_bit(__IECM_HR_DRV_LOAD, adapter->flags)) {
> +	/* Trigger reset */
> +	} else {
> +		dev_err(&adapter->pdev->dev, "Unhandled hard reset cause\n");
> +		err = IECM_ERR_PARAM;
> +		goto handle_err;
> +	}
> +
> +	/* Reset is complete and so start building the driver resources again */
> +	err = iecm_init_dflt_mbx(adapter);
> +	if (err) {
> +		dev_err(&adapter->pdev->dev, "Failed to initialize default mailbox: %d\n",
> +			err);
> +	}
> +handle_err:
> +	mutex_unlock(&adapter->reset_lock);
> +	return err;
>  }

Please limit the use of iecm_status to the absolute necessary minimum.

If FW reports those back, they should be converted to normal Linux
errors in the handler of FW communication and not leak all over the
driver like that.

Having had to modify i40e recently - I find it very frustrating to not 
be able to propagate normal errors throughout the driver. The driver-
-specific codes are a real PITA for people doing re-factoring work.