-
Notifications
You must be signed in to change notification settings - Fork 904
Add MaxLinear GSW1xx DSA driver support #1386
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
|
How do I create a dependency to a libpcap change? |
|
A Google search for make a github change depend on a change in a different github repository didn't turn up anything that looked easy to do. Presumably the libpcap change is the-tcpdump-group/libpcap#1578; given that this change won't build without that change, that's probably sufficient. |
|
It would be more useful to put the packet diagrams into tcpdump-htdocs. On this note, what are the two vertical lines to the right? One spans from a field boundary to a field boundary and another spans from a middle of a field to a field boundary. The labels next to them do not seems relevant. What is the reason the vendor is not allocating an EtherType value? |
I tried to fill in similar information as from the version it was based on.
I don't know, I am not affiliated with MaxLinear. It is currently not allocated with IANA. |
About the EtherType in linux kernel: |
IANA doesn't manage EtherTypes, the IEEE does. If this is referring to So perhaps that EtherType belonged to Infineon when it was first used for this purpose, or perhaps it's now owned by MaxLinear, or Infineon let their former division use it. |
Tnx! That make more sense. From the FAQ:
So, it is not out of the blue, it wont be any other users of the ethertype. |
For reading the packets in raw gsw1xx format.
|
Version 2. Using SpecialTag as name instead of EDSA. |
| if (egress) { | ||
| ND_PRINT("Egress Port %d,", GSW1XX_EG_IPN(p)); | ||
| if (ndo->ndo_eflag > 1) { | ||
| ND_PRINT("TTC %d,", GSW1XX_TTC(p)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regarding this and other fields: the convention is to have a space after a comma, the value is unsigned, so %u would be less confusing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also regarding this specific line, TTC does not exist in egress frames.
| #define GSW1XX_IG_TCE(tag) TOK(tag, 2, 0x40, 6) | ||
| #define GSW1XX_IG_TSE(tag) TOK(tag, 2, 0x20, 5) | ||
| #define GSW1XX_IG_FNL(tag) TOK(tag, 2, 0x10, 4) | ||
| #define GSW1XX_IG_SP(tag) TOK(tag, 2, 0x0F, 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as the packet diagram shows it, the offset for SP is 5, not 2.
| } | ||
| } else { | ||
| ND_PRINT("Ingress Port %d,", GSW1XX_IG_SP(p)); | ||
| ND_PRINT("MAP %d,", GSW1XX_MAP(p)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If DLPML is a bitmap rather than an integer, it should be printed using bittok2str(), please see commit 0b2041f for an example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correction: for ingress frames the port map is TEPML, but the other point still applies.
| #define GSW1XX_MAP_HIGH(tag) TOK(tag, 4, 0xFF, 0) | ||
| #define GSW1XX_MAP(tag) ((GSW1XX_MAP_HIGH(tag) << 8) + GSW1XX_MAP_LOW(tag)) | ||
| #define GSW1XX_LEN_LOW(tag) TOK(tag, 7, 0xFF, 0) | ||
| #define GSW1XX_LEN_HIGH(tag) TOK(tag, 6, 0x3F, 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any reason not to print the MI and KL2UM bits of byte 6?
| #define GSW1XX_EG_IPN(tag) TOK(tag, 2, 0x0F, 0) | ||
| #define GSW1XX_EG_TC(tag) TOK(tag, 2, 0xF0, 4) | ||
| #define GSW1XX_EG_POE(tag) TOK(tag, 2, 0x80, 7) | ||
| #define GSW1XX_EG_IV4(tag) TOK(tag, 2, 0x40, 6) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both PPPOE and IPV are in byte 3, not 2.
|
|
||
| #define GSW1XX_MAP_LOW(tag) TOK(tag, 3, 0xFF, 0) | ||
| #define GSW1XX_MAP_HIGH(tag) TOK(tag, 4, 0xFF, 0) | ||
| #define GSW1XX_MAP(tag) ((GSW1XX_MAP_HIGH(tag) << 8) + GSW1XX_MAP_LOW(tag)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DLPML is byte 4, not 3; DLPMR is byte 5, not 4. If DLPMR is reserved, it would be best to leave it alone instead of interpreting it as the high part of the port map.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or rather the actual problem here seems to be that both ingress and egress frames have a "port map" field, which for ingress is called TEPML and is the byte at offset 3, and for egress is called DLPML and is the byte at offset 4. This way, these should be two separate macros saying whether it is ingress or egress and which map it is (for example, GSW1XX_IG_TEPML and GSW1XX_EG_DLPML or something such as that). In any case, the reserved byte in both cases should be out of scope. Please correct me if I misunderstood this.
|
|
||
| #define GSW1XX_ET1(tag) TOK(tag, 0, 0xFF, 0) | ||
| #define GSW1XX_ET2(tag) TOK(tag, 1, 0xFF, 0) | ||
| #define GSW1XX_TTC(tag) TOK(tag, 2, 0x08, 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TTC is specific to ingress frames, in egress frames these bits are IPN.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So in practical terms this macro should be named GSW1XX_IG_TTC.
| if (GSW1XX_EG_IPO(p)) { | ||
| ND_PRINT("IV4 %d,", GSW1XX_EG_IV4(p)); | ||
| ND_PRINT("IPO %d,", GSW1XX_EG_IPO(p)); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After IPO and before the frame length it would be useful to print DLPML (using bittok2str(), as noted just below), MI and KL2UM.
| edsa-high-vid-e edsa-high-vid.pcap edsa-high-vid-e.out -e | ||
|
|
||
| # MaxLinear DSA tag tests | ||
| gsw1xx gsw1xx.pcap gsw1xx.out -n -e |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add
gsw1xx-ee gsw1xx.pcap gsw1xx-ee.out -n -e -e
to test the new code paths specific to -v. I am not sure whether -ee is the best way to hinge this, but whichever arrangement is in place, it should be tested.
| ND_PRINT("TTC %d,", GSW1XX_TTC(p)); | ||
| ND_PRINT("FNL %d,", GSW1XX_IG_FNL(p)); | ||
| ND_PRINT("IE %d,", GSW1XX_IG_IE(p)); | ||
| ND_PRINT("TSE %d,", GSW1XX_IG_TSE(p)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indentation is off at this line.
| ND_PRINT("GSW1XX "); | ||
| else | ||
| ND_PRINT("GSW1XX Unknown 0x%04x, ", sptag_etype); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the purpose of this differentiation based on ndo_eflag and/or sptag_value? Would it be sufficient just to print the EtherType in all cases? This could potentially be done using ether_type_print() if it is desirable to see the name, but that is a questionable problem to solve because nothing depends on the [per-packet] EtherType in this tag — the meaning for all packets is fixed in the DLT.
infrastation
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please address as noted, after that it should be easier to see if anything else requires attention before this is ready for merging.
This driver uses mdio port for it raw packets.
This is dependent on patch in libpcap.