Re: [dpdk-dev] [PATCH v4 0/2] ethdev: add port speed capability bitmap

["Morten Brørup" <mb@smartsharesystems.com>]

Comments inline, marked MB>.

Med venlig hilsen / kind regards
- Morten Brørup

Marc Sune <marcdevel@gmail.com> on 14. september 2015 23:34 wrote:

2015-09-14 12:52 GMT+02:00 Morten Brørup <mb@smartsharesystems.com>:
> It is important to consider that a multipath link (bonding etc.) is not a physical link, but a logical link (built on top of multiple physical links). Regardless whether it is a Layer2 link aggregate (IEEE 802.1ad, Ethernet bonding, EtherChannel, DSL pair bonding, etc.) or a Layer3 multipath link (e.g. simultaneously using Wi-Fi and mobile networks). So it doesn't make sense trying to impose physical link properties on a purely logical link. Likewise, it doesn't make sense to impose logical link properties on physical links. In other words: Don't consider bonding or any other logical link types when designing the PHY API.

+1
 

> I think there is consensus that 1/ (PHY capabilities) and 2/ (PHY advertisements) should use the same definitions, specifically a bitmap field. And when you disregard bonding, I don't see any reason to use different definitions for 3/ (PHY negotiation result). This makes it one unified API for all three purposes.

Agree.
 

> Nelio suggested adding a support function to convert the bitmap field to a speed value as an integer. I strongly support this, because you cannot expect the bitmap to be ordered by speed. 

Agree with Nelio&you. This is useful.
 
> This support function will be able to determine which speed is higher when exotic speeds are added to the bitmap. Please extend this conversion function to give three output parameters: speed, full/half duplex, auto negotiation/non-auto negotiation, or add two separate functions to get the duplex and auto-negotiation.

Since, Full/Half duplex is for legacy 10/100Mbps only (afaik), I have my doubts on using a bit for all speeds. I would suggest to define (unroll) 100M (or 100M_FD) and 100M_HD, and the same 10Mbps/1gbps, as Thomas was suggesting some mails ago.

This was done in v4 (implicitely 100M == 100M_FD). See below.
 
MB> I didn't intend two bits to be allocated in the bitmap for all speeds to support full/half duplex, only for the relevant speeds. Since full duplex is dominant, I agree with the previous decision (originally suggested by Thomas, I think) to make full duplex implicit unless half duplex is explicitly specified. E.g. 10M_HD, 10M (alias 10M_FD), 100M_HD, 100M (alias 100M_FD), 1000M (or 1G), 2500M, 10G, 40G, 100G, etc.


> I haven't read the suggested code, but there should be some means in 2/ (advertisements) to disable auto negotiation, e.g. a single bit in the bitmap to indicate if the speed/duplex-indicating bits in the bitmap means forced speed/duplex (in which case only a single speed/duplex-bit should be set) or auto negotiation advertised speed/duplex (in which case multiple speed/duplex-bits can be set). 

Agree.

v3/4 of this patch adds the bitmap in the advertised, as per discussed, to select a group of speeds This is not implemented by drivers yet (!).

So, as of v4 of this patch, there could be: a) autoneg any supported speed (=> bitmap == 0) b) autoneg over group of speeds (=> more than one bit set in the bitmap) c) forced speed (one and only one set in the bitmap).

I think this is precisely what you meant + b) as a bonus

MB> This was not what I meant, but it wasn't very clearly written, so I'll try again: Add an additional single bit "NO_AUTONEG" (or whatever you want to name it) to the 2/ (advertisements) bitmap that explicitly turns off auto negotiation and tries to force the selected speed/duplex (i.e. only one other bit can be set in the bitmap when the NO_AUTONEG bit is set). Your c) makes it impossible to use auto negotiation to advertise a specific speed/duplex, e.g. 100M_FD. My suggested NO_AUTONEG bit can also be used in 3/ (result) to indicate that the speed was a result of Parallel Detection, i.e. that auto negotiation failed or was disabled in either end of the link.

MB> However, I like your suggestion a).

 
> And some means in 3/ (result) and maybe 2/ (advertisements) to select and/or indicate physical interface in dual-personality ports (e.g. ports where the PHY has both an SFP and a RJ45 connector, but only one of the two can be used at any time).

For rte_eth_link_get() I don't have such a strong opinion. You either

* encode the link speed and duplex as of now, separating duplex and numeric speed. I would suggest to add the encoded speed+duplex bitmap flag for consistency (although redundant).
* or you return a single value, the bitmap with a single flag set of the unrolled speeds, and then have the helpers int rte_eth_speed_from_bm(int val_bm) and bool rte_eth_duplex_from_bm(int val_bm).

MB> I prefer the latter of the two, only because it makes 3/ (result) consistent with 1/ (capabilities) and 2/ (advertisements).


Marc