Re: [PATCH] igb: Add support for firmware update

[Jacob Keller <jacob.e.keller@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.