| 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: <b5ea0128f9fb3b59058fd264eecc1c7f31fd6591.camel@microchip.com>
Date: Wed, 30 Oct 2024 11:48:11 +0530
From: Rakesh Babu Saladi <Saladi.Rakeshbabu@...rochip.com>
To: Fabian Benschuh <Fabi.Benschuh@....de>, Woojung Huh
<woojung.huh@...rochip.com>, <UNGLinuxDriver@...rochip.com>, Andrew Lunn
<andrew+netdev@...n.ch>, "David S. Miller" <davem@...emloft.net>, "Eric
Dumazet" <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, "Paolo
Abeni" <pabeni@...hat.com>, <netdev@...r.kernel.org>,
<linux-usb@...r.kernel.org>
Subject: Re: [PATCH] Add LAN78XX OTP_ACCESS flag support
Hi Fabian,
Please see the comments inline.
Thanks,
Rakesh.
On Sat, 2024-10-26 at 01:05 +0200, Fabian Benschuh wrote:
> With this flag we can now use ethtool to access the OTP:
> ethtool --set-priv-flags eth0 OTP_ACCESS on
> ethtool -e eth0 # this will read OTP if OTP_ACCESS is on, else
> EEPROM
>
> When writing to OTP we need to set OTP_ACCESS on and write with the
> correct magic 0x7873 for OTP
> ---
> drivers/net/usb/lan78xx.c | 55 ++++++++++++++++++++++++++++++++-----
> --
> 1 file changed, 45 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
> index 8adf77e3557e..2fc9b9b138b0 100644
> --- a/drivers/net/usb/lan78xx.c
> +++ b/drivers/net/usb/lan78xx.c
> @@ -85,6 +85,7 @@
> #define EEPROM_INDICATOR (0xA5)
> #define EEPROM_MAC_OFFSET (0x01)
> #define MAX_EEPROM_SIZE 512
> +#define MAX_OTP_SIZE 512
> #define OTP_INDICATOR_1 (0xF3)
> #define OTP_INDICATOR_2 (0xF7)
>
> @@ -172,6 +173,7 @@
> #define INT_EP_GPIO_2 (2)
> #define INT_EP_GPIO_1 (1)
> #define INT_EP_GPIO_0 (0)
> +#define LAN78XX_NET_FLAG_OTP BIT(0)
Please move this macro to other macros defined with prefix "LAN78XX_"
and probably please rename it to "LAN78XX_ADAPTER_ETHTOOL_FLAG_OTP".
>
> static const char lan78xx_gstrings[][ETH_GSTRING_LEN] = {
> "RX FCS Errors",
> @@ -446,6 +448,7 @@ struct lan78xx_net {
> unsigned int burst_cap;
>
> unsigned long flags;
> + u32 priv_flags;
It would be good to include variables of single type to be grouped
together. There are variables of type u32, please move this variable to
that group.
>
> wait_queue_head_t *wait;
> unsigned char suspend_count;
> @@ -1542,6 +1545,10 @@ static void lan78xx_status(struct lan78xx_net
> *dev, struct urb *urb)
>
> static int lan78xx_ethtool_get_eeprom_len(struct net_device *netdev)
> {
> + struct lan78xx_net *dev = netdev_priv(netdev);
> +
> + if (dev->priv_flags & LAN78XX_NET_FLAG_OTP)
> + return MAX_OTP_SIZE;
> return MAX_EEPROM_SIZE;
> }
>
> @@ -1555,9 +1562,10 @@ static int lan78xx_ethtool_get_eeprom(struct
> net_device *netdev,
> if (ret)
> return ret;
>
> - ee->magic = LAN78XX_EEPROM_MAGIC;
> -
> - ret = lan78xx_read_raw_eeprom(dev, ee->offset, ee->len, data);
> + if (dev->priv_flags & LAN78XX_NET_FLAG_OTP)
> + ret = lan78xx_read_raw_otp(dev, ee->offset, ee->len,
> data);
> + else
> + ret = lan78xx_read_raw_eeprom(dev, ee->offset, ee->len,
> data);
>
> usb_autopm_put_interface(dev->intf);
>
> @@ -1577,30 +1585,39 @@ static int lan78xx_ethtool_set_eeprom(struct
> net_device *netdev,
> /* Invalid EEPROM_INDICATOR at offset zero will result in a
> failure
> * to load data from EEPROM
> */
> - if (ee->magic == LAN78XX_EEPROM_MAGIC)
> - ret = lan78xx_write_raw_eeprom(dev, ee->offset, ee-
> >len, data);
> - else if ((ee->magic == LAN78XX_OTP_MAGIC) &&
> - (ee->offset == 0) &&
> - (ee->len == 512) &&
> - (data[0] == OTP_INDICATOR_1))
> - ret = lan78xx_write_raw_otp(dev, ee->offset, ee->len,
> data);
> + if (dev->priv_flags & LAN78XX_NET_FLAG_OTP) {
> + /* Beware! OTP is One Time Programming ONLY! */
> + if (ee->magic == LAN78XX_OTP_MAGIC)
> + ret = lan78xx_write_raw_otp(dev, ee->offset, ee-
> >len, data);
> + } else {
> + if (ee->magic == LAN78XX_EEPROM_MAGIC)
> + ret = lan78xx_write_raw_eeprom(dev, ee->offset, ee-
> >len, data);
In the function "lan78xx_write_raw_eeprom.otp", please add the
condition check if the offset + length are within the
EEPROM/OTP_MAX_SIZE limits and proceed further to write if the
condition is met.
> + }
>
> usb_autopm_put_interface(dev->intf);
>
> return ret;
> }
>
> +static const char lan78xx_priv_flags_strings[][ETH_GSTRING_LEN] = {
> + "OTP_ACCESS",
> +};
> +
> static void lan78xx_get_strings(struct net_device *netdev, u32
> stringset,
> u8 *data)
> {
> if (stringset == ETH_SS_STATS)
> memcpy(data, lan78xx_gstrings,
> sizeof(lan78xx_gstrings));
> + else if (stringset == ETH_SS_PRIV_FLAGS)
> + memcpy(data, lan78xx_priv_flags_strings,
> sizeof(lan78xx_priv_flags_strings));
> }
>
> static int lan78xx_get_sset_count(struct net_device *netdev, int
> sset)
> {
> if (sset == ETH_SS_STATS)
> return ARRAY_SIZE(lan78xx_gstrings);
> + else if (sset == ETH_SS_PRIV_FLAGS)
> + return ARRAY_SIZE(lan78xx_priv_flags_strings);
> else
> return -EOPNOTSUPP;
> }
> @@ -1617,6 +1634,22 @@ static void lan78xx_get_stats(struct
> net_device *netdev,
> mutex_unlock(&dev->stats.access_lock);
> }
>
> +static u32 lan78xx_ethtool_get_priv_flags(struct net_device *netdev)
> +{
> + struct lan78xx_net *dev = netdev_priv(netdev);
> +
> + return dev->priv_flags;
> +}
> +
> +static int lan78xx_ethtool_set_priv_flags(struct net_device *netdev,
> u32 flags)
> +{
> + struct lan78xx_net *dev = netdev_priv(netdev);
> +
> + dev->priv_flags = flags;
Instead of assigning flags directly, it would be good to add checks and
assign something like this
if (flags & LAN78XX_ADAPTER_ETHTOOL_FLAG_OTP)
dev->ethtool_flags |= LAN78XX_ADAPTER_ETHTOOL_FLAG_OTP;
else
dev->ethtool_flags &= ~LAN78XX_ADAPTER_ETHTOOL_FLAG_OTP;
> +
> + return 0;
> +}
> +
> static void lan78xx_get_wol(struct net_device *netdev,
> struct ethtool_wolinfo *wol)
> {
> @@ -1905,6 +1938,8 @@ static const struct ethtool_ops
> lan78xx_ethtool_ops = {
> .get_eeprom = lan78xx_ethtool_get_eeprom,
> .set_eeprom = lan78xx_ethtool_set_eeprom,
> .get_ethtool_stats = lan78xx_get_stats,
> + .get_priv_flags = lan78xx_ethtool_get_priv_flags,
> + .set_priv_flags = lan78xx_ethtool_set_priv_flags,
> .get_sset_count = lan78xx_get_sset_count,
> .get_strings = lan78xx_get_strings,
> .get_wol = lan78xx_get_wol,
Powered by blists - more mailing lists