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]

Message-ID: <d81a0f1d-1051-3160-a9bc-dbbc645be857@pensando.io>
Date:   Thu, 11 Jul 2019 12:10:32 -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/8/19 3:04 PM, Andrew Lunn wrote:

>> +	case XCVR_PID_SFP_10GBASE_ER:
>> +		ethtool_link_ksettings_add_link_mode(ks, supported,
>> +						     10000baseER_Full);
>> +		break;
> I don't know these link modes too well. But only setting a single bit
> seems odd. What i do know is that an SFP which supports 2500BaseX
> should also be able to support 1000BaseX. So should a 100G SFP also
> support 40G, 25G, 10G etc? The SERDES just runs a slower bitstream
> over the basic bitpipe?

Yes, but in this initial release we're not supporting changes to the 
modes yet.  That flexibility will come later.

>
>> +	case XCVR_PID_QSFP_100G_ACC:
>> +	case XCVR_PID_QSFP_40GBASE_ER4:
>> +	case XCVR_PID_SFP_25GBASE_LR:
>> +	case XCVR_PID_SFP_25GBASE_ER:
>> +		dev_info(lif->ionic->dev, "no decode bits for xcvr type pid=%d / 0x%x\n",
>> +			 idev->port_info->status.xcvr.pid,
>> +			 idev->port_info->status.xcvr.pid);
>> +		break;
> Why not add them?

Yes, this has been mentioned before.  I might in the future, but I have 
my hands full at the moment.

>
>
>> +	memcpy(ks->link_modes.advertising, ks->link_modes.supported,
>> +	       sizeof(ks->link_modes.advertising));
> bitmap_copy() would be a better way to do this. You could consider
> adding a helper to ethtool.h.

Sure.

Thanks for your comments, and sorry I haven't responded as quickly as 
I'd like... I'll be going through these and your other comments over the 
next few days.

sln