Marc, here's a couple of details that I missed in my email below:
Correction: 1000BASE-T and 1000BASE-X also have half duplex, so full/half
duplex is relevant for 10, 100 and 1000 Mbit/s speeds.
The bitmap in 3/ (result) probably also needs a bit ("NO_MEDIA" or whatever) to
indicate if a media module (e.g. an SFP+ module) is present or missing (i.e.
the SFP+ cage is empty); e.g. refer to the "Media Available" row in table 3-55
in Intel's XL710 Ethernet Controller datasheet
(xl710-10-40-controller-datasheet.pdf). Alternatively, this can be indicated by
having 1/ (capabilities) returning an empty set of capabilities when no media
module has been installed.
Furthermore, the 1/ (capabilities) or 3/ (result) also needs a means to
indicate which physical port of a dual-personality port is being used. And by
dual-personality ports, I mean a PHY with both an RJ45 copper port and an SFP
cage, where only one of them can be active at any time.
Med venlig hilsen / kind regards
- Morten Br?rup
-----Original Message-----
From: Morten Br?rup
Sent: 15. september 2015 00:50
To: 'Marc Sune'
Cc: Thomas Monjalon; N?lio Laranjeiro; dev at dpdk.org; Olga Shern; Adrien
Mazarguil
Subject: RE: [dpdk-dev] [PATCH v4 0/2] ethdev: add port speed capability bitmap
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
