Received-SPF: pass (google.com: domain of linux-kernel+bounces-208978-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <4f2ec2fb-5d31-4a90-9ef6-a036d16a5cb4@intel.com>
Date: Mon, 10 Jun 2024 15:04:06 -0700
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] igb: Add support for firmware update
To: Richard chien <m8809301@gmail.com>, <davem@davemloft.net>,
	<edumazet@google.com>, <kuba@kernel.org>, <pabeni@redhat.com>
CC: <jesse.brandeburg@intel.com>, <anthony.l.nguyen@intel.com>,
	<intel-wired-lan@lists.osuosl.org>, <netdev@vger.kernel.org>,
	<linux-kernel@vger.kernel.org>, Richard chien <richard.chien@hpe.com>
References: <20240609081526.5621-1-richard.chien@hpe.com>
Content-Language: en-US
From: Jacob Keller <jacob.e.keller@intel.com>
In-Reply-To: <20240609081526.5621-1-richard.chien@hpe.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?QXZMbHhIK3dTV01vbStWMDBicnZmNWhoQ010ejhCWTRCd2tlbnpUakc1WWlZ?=
 =?utf-8?B?SlZwQytzZWJrdW53UWtFTnlUalU5ZW8vMUVmN2w4UERyWVVyL0w4UnR4WXd2?=
 =?utf-8?B?eStEU3I1Mm81SHdqYlhSckRRSFZlYTRUT2NuM3hUeUQrbWFLM25SbG9NVXk0?=
 =?utf-8?B?V0pBLzhZWkhjb1VGczZaU0pIaXo2U3czTk43Rjg3YjJBSXl3Q012TnZQV1dh?=
 =?utf-8?B?SlhqV0ZTS2x2RDQ0NkNabzJlSmV3MWhVSjFINGVkdkNGNC93LzNDR0NHU2hN?=
 =?utf-8?B?dXBFL3VoWTlmOTdFL1dzRnplbmF0OW5BTmp3Y2dNMHArZ2JySmN3bVVrUlpN?=
 =?utf-8?B?QkZhRExMNDVralRycithaDhBNjl3eVppK01FdE1HQ29KM3ZlMW9XbmhucjZD?=
 =?utf-8?B?WVMvYUM3WnlPOTRwTDdrRnd4QUYwZVJUNWs4RWV2b0dDeTJGVDdBdmpBd3l6?=
 =?utf-8?B?U0J2aEY4WUwzRkVuR1hOUDRxYkVwZkRId09HRlBWNTBmTjJFcHJDcU1NMjRT?=
 =?utf-8?B?ZmxMSHQ5dmxnWmtIUkJsSFBDeGhJbDNxTm53aFZ5TGgxZ1VLeEY4eEdMUzQ0?=
 =?utf-8?B?V2o5U1Zvd0lwVnFNekNwVHlaZXpQOVpML1JVUDlQK3Q0NTB0ZURuSEt6OFlq?=
 =?utf-8?B?R3dlWjRuRWdYZmpRVjY3UWNXMG8yYkVMTzRVbTdkZTlneURGTHAzSGdVNzJD?=
 =?utf-8?B?SGFFWkJXU21ISlpObnR6TU9oWlpGWFpwNlZNU2RXa0JkdDR2Q0xibVZrVFlF?=
 =?utf-8?B?dVlJRDVrU3UzWHUyUE9OaXlZYk1pYWVsRXA0MVZZbzJlRlp5ZmFOeVhzN0RD?=
 =?utf-8?B?REtUN1A1c0prTXlBT0s2eVBvL1RwWW1VbVIzbGJvaHFidnRuenR0Z2NiUVFq?=
 =?utf-8?B?Z1ZMZnhaeGlJMUQvd2V2NHZPZEhGTmFqVFZYZEdCa2pwL1NlZVpOZEhYLzFx?=
 =?utf-8?B?ZytYRHQ2Z3VzcmpEWldYa2JwZ3NQaWtKUWZMeWVhMENPMEdrRlU5aHgvdWVV?=
 =?utf-8?B?ZmtXY0ozb3Riemd3b3owRzlRYkhybVA5RVBoQ1RsdG9nRnRSU3dBZEJPMXNr?=
 =?utf-8?B?ZzZzeXNXSlg5QWN1c2g0M0JRYVI3U2kySnE5SVNtczA0VEtFcUZNWEd2eEhC?=
 =?utf-8?B?OThWNTZrSEJoSExlZU1ZeTdZQk5BVG9vVE5KRi9wdi9qMHFoNHJRT2NBWkFz?=
 =?utf-8?B?dDkrR1RRNUdCVFFyeHlkK2pLLzZEWGlUQTNsRUVnaVJ3V3NVeSsrR2VuV04z?=
 =?utf-8?B?Z1YyK3RPanZ3ODV4OXlsUXcwbG5vSjM5WElpMk93ZTdJdEZmQTFVdUljWXE0?=
 =?utf-8?B?ckFDbklsZnN5S0s2d2JWVnVsak8vc2J2V1pETkk2WWtRb1RRMlF2K0syTmVJ?=
 =?utf-8?B?WmhhaWNCZCs1akVoaFVFcGxTdDFjSDJIK0NDV0l0MHZtNUxFNW40SUU4aVR0?=
 =?utf-8?B?S2RsQ1hvdDczT0REMmxBcmJmUGNRcjd0OHZwbnE4VG1zaTJYeU1leWZ2eEVK?=
 =?utf-8?B?U2Ftc0M3cStmeEdrdkFOSFhOQnJzRW1iL25MSEJXek5WRE9FdExpK0tSVk5v?=
 =?utf-8?B?N25OdjVva1hTRCtRUEJIbXRBQlF2VE92ckQ3YTZrR1R3QjFDVHJ6bWVXbUgx?=
 =?utf-8?B?U3JvME04MnltWnZXMjhtSDZLZGREV2RWUzBLbGJsUlkvUTJWRXZIYTRKM2Vv?=
 =?utf-8?B?bTJubHNrN3RqTGlvSHd1d0hyN0hTWXJZd1NnQkF1QkM5S0VPTnlRWmlhWng5?=
 =?utf-8?B?Z240UFExenhveHNiZ1RLY1pabnpjbmxDazFYMVgyU29NN3VCdXJLdStIVTlW?=
 =?utf-8?B?SFNFZUw4QWl2dE5iSk9wUWF0cFN1OC82SmpnV3FNd3NtWVdCRWtRZWlCWG01?=
 =?utf-8?B?cGZKVzVFbFRqOXRmYUJ5MmFzWFNubkhXOTZDNXJ6Yk5IMCsyZU1aS0pjKy9P?=
 =?utf-8?B?MGpQelJETmZ4cVM4RXlVbVlXZU9YY085dnMwOWhqbXh0aHA1QlFWN1hZUmhN?=
 =?utf-8?B?TzJLeDUyQ3BManp5VEF6VmJ4dXFyT1lJZlYrUkFKQ3dIbm5uSVl5YnQ4TjUv?=
 =?utf-8?B?TmtUVExpSXYzcjEvazVhcnN6WlBRZkVYTGtkN0ZPNDYyTEhUUm5nR3pmOTJ4?=
 =?utf-8?B?citRVzNiczA4dy9RS3F4MWpPQUx5QmlzSVlZcm1XV2Q2akNuM0hQbzVnYUl3?=
 =?utf-8?B?Qmc9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 5b6d6ee6-194f-4298-f01b-08dc89994002
X-MS-Exchange-CrossTenant-AuthSource: CO1PR11MB5089.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 10 Jun 2024 22:04:07.8076
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: FAT47FQ7xyG6eYbbXpRhZxNrtegCyXjgxthnjUxN89Qkf2coaguay4gQMzEDAKFSYqkzaDdB5PSsjhMpXDE2LS/kIfDxXmHhqxKG7PiusUI=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM4PR11MB7349
X-OriginatorOrg: intel.com


On 6/9/2024 1:15 AM, Richard chien wrote:
> This patch adds support for firmware update to the in-tree igb driver and it is actually a port from the out-of-tree igb driver.
> In-band firmware update is one of the essential system maintenance tasks. To simplify this task, the Intel online firmware update
> utility provides a common interface that works across different Intel NICs running the igb/ixgbe/i40e drivers. Unfortunately, the
> in-tree igb and ixgbe drivers are unable to support this firmware update utility, causing problems for enterprise users who do not
> or cannot use out-of-distro drivers due to security and various other reasons (e.g. commercial Linux distros do not provide technical
> support for out-of-distro drivers). As a result, getting this feature into the in-tree igb driver is highly desirable.
> 
> Signed-off-by: Richard chien <richard.chien@hpe.com>

The motivation to support updating flash with in-kernel drivers is good.

>  static int igb_set_eeprom(struct net_device *netdev,
> +                          struct ethtool_eeprom *eeprom, u8 *bytes)
> +{
> +        struct igb_adapter *adapter = netdev_priv(netdev);
> +        struct e1000_hw *hw = &adapter->hw;
> +        u16 *eeprom_buff;
> +        void *ptr;
> +        int max_len, first_word, last_word, ret_val = 0;
> +        struct e1000_nvm_access *nvm;
> +        u32 magic;
> +        u16 i;
> +
> +        if (eeprom->len == 0)
> +                return -EOPNOTSUPP;
> +
> +        magic = hw->vendor_id | (hw->device_id << 16);
> +        if (eeprom->magic && eeprom->magic != magic) {
> +                nvm = (struct e1000_nvm_access *)eeprom;
> +                ret_val = igb_nvmupd_command(hw, nvm, bytes);
> +                return ret_val;
> +        }
> +
However, this implementation is wrong. It is exposing the
ETHTOOL_GEEPROM and ETHTOOL_SEEPROM interface and abusing it to
implement a non-standard interface that is custom to the out-of-tree
Intel drivers to support the flash update utility.

This implementation was widely rejected when discovered in i40e and in
submissions for the  ice driver. It abuses the ETHTOOL_GEEPROM and
ETHTOOL_SEEPROM interface in order to allow tools to access the
hardware. The use violates the documented behavior of the ethtool
interface and breaks the intended functionality of ETHTOOL_GEEPROM and
ETHTOOL_SEEPROM.

The correct way to implement flash update is via the devlink dev flash
interface, using request_firmware, and implementing the entire update
process in the driver. The common portions of this could be done in a
shared module.

Attempting to support the broken legacy update that is supported by the
out-of-tree drivers is a non-starter for upstream. We (Intel) have known
this for some time, and this is why the patches and support have never
been published.