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. > > 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 > 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). Marc > > Med venlig hilsen / kind regards > - Morten Br?rup > > -----Original Message----- > From: Thomas Monjalon [mailto:thomas.monjalon at 6wind.com] > Sent: 13. september 2015 23:19 > To: Marc Sune > Cc: N?lio Laranjeiro; dev at dpdk.org; Olga Shern; Adrien Mazarguil; Morten > Br?rup > Subject: Re: [dpdk-dev] [PATCH v4 0/2] ethdev: add port speed capability > bitmap > > 2015-09-13 21:14, Marc Sune: > > 2015-09-09 15:33 GMT+02:00 Thomas Monjalon <thomas.monjalon at 6wind.com>: > > > 2015-09-09 15:10, N?lio Laranjeiro: > > > > I think V2 is better, maybe you can add a function to convert a > > > > single bitmap value to the equivalent integer and get rid of > > > > ETH_SPEED_XXX > > > macros. > > > > > > > > Thomas what is your opinion? > > > > > > Your proposal looks good Nelio. > > > > I am confused, specially since you were the one advocating for having > > a unified set of constants for speeds (discussion in v2). > > Yes, my first thought was advocating an unification between capabilities > and negotiated link properties. > After I was convinced by Nelio's arguments: bitmap is good for > capabilities (especially to describe every capabilities in one field) but > integer is better for negotiated speed (especially for aggregated links). > Converting bitmap speed into integer should be easy to implement in a > function. > > > In any case, as I see it, if we want to address the comments of M. > Brorup: > > > > http://comments.gmane.org/gmane.comp.networking.dpdk.devel/19664 > > > > we need bitmaps for rte_eth_conf link_speed to set the advertised speeds. > > Yes I forgot this interesting comment. It is saying we need > 1/ capabilities > 2/ advertised modes (for auto-negotiation or fixed config) > 3/ negotiated mode > Previously we were focused only on 1/ and 3/. > 2/ was only limited to a mode configured without negotiation and was using > the same field as 3/. > Maybe we should really have 3 different fields. 1/ and 2/ would use a > bitmap. > > > Let me know if you really want to come back to v2 or not. > > It needs more discussion. What do you think of the above proposal? > What is the opinion of Nelio? Morten? > > >