netdev - Re: [PATCH v3 net-next 13/19] ionic: Add initial ethtool support

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]

Date:   Thu, 18 Jul 2019 10:05:23 -0700
From:   Shannon Nelson <snelson@...sando.io>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     netdev@...r.kernel.org
Subject: Re: [PATCH v3 net-next 13/19] ionic: Add initial ethtool support

On 7/17/19 8:21 PM, Andrew Lunn wrote:
> On Tue, Jul 09, 2019 at 03:42:39PM -0700, Shannon Nelson wrote:
>> On 7/8/19 7:27 PM, Andrew Lunn wrote:
>>>> +static int ionic_get_module_eeprom(struct net_device *netdev,
>>>> +				   struct ethtool_eeprom *ee,
>>>> +				   u8 *data)
>>>> +{
>>>> +	struct lif *lif = netdev_priv(netdev);
>>>> +	struct ionic_dev *idev = &lif->ionic->idev;
>>>> +	struct xcvr_status *xcvr;
>>>> +	u32 len;
>>>> +
>>>> +	/* copy the module bytes into data */
>>>> +	xcvr = &idev->port_info->status.xcvr;
>>>> +	len = min_t(u32, sizeof(xcvr->sprom), ee->len);
>>>> +	memcpy(data, xcvr->sprom, len);
>>> Hi Shannon
>>>
>>> This also looks odd. Where is the call into the firmware to get the
>>> eeprom contents? Even though it is called 'eeprom', the data is not
>>> static. It contains real time diagnostic values, temperature, transmit
>>> power, receiver power, voltages etc.
>> idev->port_info is a memory mapped space that the device keeps up-to-date.
> Hi Shannon
>
> It at least needs a comment. How frequently does the device update
> this chunk of memory? It would be good to comment about that as
> well. Or do MMIO reads block while i2c operations occur to update the
> memory?

The device keeps this updated when changes happen internally so that 
there is no need to block on MMIO read.  Sure, I'll add a little more 
commentary here and perhaps in a couple other similar places.

>
>>>> +
>>>> +	dev_dbg(&lif->netdev->dev, "notifyblock eid=0x%llx link_status=0x%x link_speed=0x%x link_down_cnt=0x%x\n",
>>>> +		lif->info->status.eid,
>>>> +		lif->info->status.link_status,
>>>> +		lif->info->status.link_speed,
>>>> +		lif->info->status.link_down_count);
>>>> +	dev_dbg(&lif->netdev->dev, "  port_status id=0x%x status=0x%x speed=0x%x\n",
>>>> +		idev->port_info->status.id,
>>>> +		idev->port_info->status.status,
>>>> +		idev->port_info->status.speed);
>>>> +	dev_dbg(&lif->netdev->dev, "    xcvr status state=0x%x phy=0x%x pid=0x%x\n",
>>>> +		xcvr->state, xcvr->phy, xcvr->pid);
>>>> +	dev_dbg(&lif->netdev->dev, "  port_config state=0x%x speed=0x%x mtu=0x%x an_enable=0x%x fec_type=0x%x pause_type=0x%x loopback_mode=0x%x\n",
>>>> +		idev->port_info->config.state,
>>>> +		idev->port_info->config.speed,
>>>> +		idev->port_info->config.mtu,
>>>> +		idev->port_info->config.an_enable,
>>>> +		idev->port_info->config.fec_type,
>>>> +		idev->port_info->config.pause_type,
>>>> +		idev->port_info->config.loopback_mode);
>>> It is a funny place to have all the debug code.
>> Someone wanted a hook for getting some link info on the fly on request.
> I would suggest deleting it all. Most of it is already available via
> ethtool.
>
Sure.

sln