Comments inline, marked MB>. Med venlig hilsen / kind regards - Morten Br?rup
Marc Sune <marcdevel at gmail.com> on 14. september 2015 23:34 wrote: 2015-09-14 12:52 GMT+02:00 Morten Br?rup <mb at 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
