netdev - Re: [PATCH] sis900: add support for ethtool's EEPROM dump

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] [day] [month] [year] [list]

Message-ID: <c26f0ca5ae49c46f67dad51d013c51fffe2ac7d1.camel@mellanox.com>
Date:   Fri, 26 Jul 2019 21:39:04 +0000
From:   Saeed Mahameed <saeedm@...lanox.com>
To:     "venza@...wnhat.org" <venza@...wnhat.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "sergej.benilov@...glemail.com" <sergej.benilov@...glemail.com>,
        "andrew@...n.ch" <andrew@...n.ch>
Subject: Re: [PATCH] sis900: add support for ethtool's EEPROM dump

On Thu, 2019-07-25 at 21:48 +0200, Sergej Benilov wrote:
> Implement ethtool's EEPROM dump command (ethtool -e|--eeprom-dump).
> 
> Thx to Andrew Lunn for comments.
> 
> Signed-off-by: Sergej Benilov <sergej.benilov@...glemail.com>
> ---
>  drivers/net/ethernet/sis/sis900.c | 68
> +++++++++++++++++++++++++++++++
>  1 file changed, 68 insertions(+)
> 
> diff --git a/drivers/net/ethernet/sis/sis900.c
> b/drivers/net/ethernet/sis/sis900.c
> index 6e07f5ebacfc..85eaccbbbac1 100644
> --- a/drivers/net/ethernet/sis/sis900.c
> +++ b/drivers/net/ethernet/sis/sis900.c
> @@ -191,6 +191,8 @@ struct sis900_private {
>  	unsigned int tx_full; /* The Tx queue is full. */
>  	u8 host_bridge_rev;
>  	u8 chipset_rev;
> +	/* EEPROM data */
> +	int eeprom_size;
>  };
>  
>  MODULE_AUTHOR("Jim Huang <cmhuang@....com.tw>, Ollie Lho <
> ollie@....com.tw>");
> @@ -475,6 +477,8 @@ static int sis900_probe(struct pci_dev *pci_dev,
>  	sis_priv->pci_dev = pci_dev;
>  	spin_lock_init(&sis_priv->lock);
>  
> +	sis_priv->eeprom_size = 24;
> + 

this value isn't changing across this patch, 
why do you need to store a constant value in private data ?

Just make a #define .. 

>  	pci_set_drvdata(pci_dev, net_dev);
>  
>  	ring_space = pci_alloc_consistent(pci_dev, TX_TOTAL_SIZE,
> &ring_dma);
> @@ -2122,6 +2126,68 @@ static void sis900_get_wol(struct net_device
> *net_dev, struct ethtool_wolinfo *w
>  	wol->supported = (WAKE_PHY | WAKE_MAGIC);
>  }
>  
> +static int sis900_get_eeprom_len(struct net_device *dev)
> +{
> +	struct sis900_private *sis_priv = netdev_priv(dev);
> +
> +	return sis_priv->eeprom_size;
> +}
> +
> +static int sis900_read_eeprom(struct net_device *net_dev, u8 *buf)
> +{
> +	struct sis900_private *sis_priv = netdev_priv(net_dev);
> +	void __iomem *ioaddr = sis_priv->ioaddr;
> +	int wait, ret = -EAGAIN;
> +	u16 signature;
> +	u16 *ebuf = (u16 *)buf;

reverse xmas tree.

> +	int i;
> +
> +	if (sis_priv->chipset_rev == SIS96x_900_REV) {
> +		sw32(mear, EEREQ);
> +		for (wait = 0; wait < 2000; wait++) {
> +			if (sr32(mear) & EEGNT) {
> +				/* read 16 bits, and index by 16 bits
> */
> +				for (i = 0; i < sis_priv->eeprom_size /
> 2; i++)
> +					ebuf[i] =
> (u16)read_eeprom(ioaddr, i);
> +				ret = 0;
> +				break;
> +			}
> +			udelay(1);
> +		}

cosmetic comment, too much indentations to my taste,
two ways to avoid this,
1) if !SIS96x_900_REV execute the else statement and early return 
2) "do while" to wait for (sr32(mear) & EEGNT) and then execute the for
loop which reads the eeprom outs side the wait loop.

> +		sw32(mear, EEDONE);
> +	} else {
> +		signature = (u16)read_eeprom(ioaddr, EEPROMSignature);
> +		if (signature != 0xffff && signature != 0x0000) {
> +			/* read 16 bits, and index by 16 bits */
> +			for (i = 0; i < sis_priv->eeprom_size / 2; i++)
> +				ebuf[i] = (u16)read_eeprom(ioaddr, i);
> +			ret = 0;
> +		}
> +	}
> +	return ret;
> +}
> +
> +#define SIS900_EEPROM_MAGIC	0xBABE
> +static int sis900_get_eeprom(struct net_device *dev, struct
> ethtool_eeprom *eeprom, u8 *data)
> +{
> +	struct sis900_private *sis_priv = netdev_priv(dev);
> +	u8 *eebuf;
> +	int res;
> +
> +	eebuf = kmalloc(sis_priv->eeprom_size, GFP_KERNEL);
> +	if (!eebuf)
> +		return -ENOMEM;
> +
> +	eeprom->magic = SIS900_EEPROM_MAGIC;
> +	spin_lock_irq(&sis_priv->lock);
> +	res = sis900_read_eeprom(dev, eebuf);
> +	spin_unlock_irq(&sis_priv->lock);
> +	if (!res)
> +		memcpy(data, eebuf + eeprom->offset, eeprom->len);
> +	kfree(eebuf);
> +	return res;
> +}
> +
>  static const struct ethtool_ops sis900_ethtool_ops = {
>  	.get_drvinfo 	= sis900_get_drvinfo,
>  	.get_msglevel	= sis900_get_msglevel,
> @@ -2132,6 +2198,8 @@ static const struct ethtool_ops
> sis900_ethtool_ops = {
>  	.set_wol	= sis900_set_wol,
>  	.get_link_ksettings = sis900_get_link_ksettings,
>  	.set_link_ksettings = sis900_set_link_ksettings,
> +	.get_eeprom_len = sis900_get_eeprom_len,
> +	.get_eeprom = sis900_get_eeprom,
>  };
>  
>  /**