lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Message-ID: <CACRpkdYoqeThCHxmApmr2Dh6jcfwf2uEJgSeyPHGymcZ4DWo1Q@mail.gmail.com> Date: Sat, 30 Jun 2018 13:05:47 +0200 From: Linus Walleij <linus.walleij@...aro.org> To: Florian Fainelli <f.fainelli@...il.com> Cc: Andrew Lunn <andrew@...n.ch>, Vivien Didelot <vivien.didelot@...oirfairelinux.com>, netdev <netdev@...r.kernel.org>, OpenWrt Development List <openwrt-devel@...ts.openwrt.org>, LEDE Development List <lede-dev@...ts.infradead.org>, Gabor Juhos <juhosg@...nwrt.org> Subject: Re: [PATCH 3/3] net: dsa: Add Vitesse VSC73xx DSA router driver Hi Florian, Thanks for your review! will send a v2 shortly, just some minor feedbacks: On Thu, Jun 14, 2018 at 6:51 PM Florian Fainelli <f.fainelli@...il.com> wrote: > On 06/14/2018 05:35 AM, Linus Walleij wrote: > > This driver (currently) only takes control of the switch chip over > > SPI and configures it to route packages around when connected to a > > CPU port. The chip has embedded PHYs and VLAN support so we model it > > using DSA as a best fit so we can easily add VLAN support and maybe > > later also exploit the internal frame header to get more direct > > control over the switch. > > Yes having the internal frame header working would be really great, > DSA_TAG_PROTO_NONE is really difficult to use without knowing all the > DSA details which reminds that we should have the following action items: > > - document how DSA_TAG_PROTO_NONE behave differently with respect to > bridges/VLAN configuration and the DSA master device > > - possibly introduce DSA_TAG_PROTO_8021Q which would automatically > partition ports by allocating one VLAN ID per-port (e.g: from top to > bottom) that would effectively offer the same features/paradigms as what > a proper header would offer (Port separation, if nothing else) and it > could be made seemingly automatic from within DSA To me this makes a lot of sense. I haven't implemented VLAN yet for this switch chip, but for the Realtek RTL8366RB that I'm also working with, the vendor driver already does more or less exactly this by default. It would be great to have the kernel just put this in place so we have it under control. The RTL8366 chips does not even seem to have an internal frame format AFAICT, it seems they don't know what that is. So they will likely always have to use VLAN for this. > > The four built-in GPIO lines are exposed using a standard GPIO chip. > > What are those typically used for out of curiosity? Is this to connect > to an EEPROM? No, there are actually separate address and data lines for connecting an EEPROM and booting the 8051 CPU from it if need be. This is likely mostly for the stand-alone usecase. The VSC73xx and other Vitesse chips are only used with another CPU (like one running Linux) as an exception, IIUC. The usual usecase is to put the Vitesse chip with an EEPROM and SW from the vendor running on the 8051, and box it up as a rack-mounted or small switch or router. The vendor provides an SDK for the 8051 CPU including an SNMP implementation so it can be remotely managed. If you use this to build a rack-mounted or boxed product, you likely want to add some "reset" key or a few LEDs to indicate whatever, and then those 4 GPIO lines come in handy. With another OS, like Linux running, these lines can be used for whatever you want, just an extra GPIO expander essentially. > > + u16 chipid; > > + bool is_vsc7385; > > + bool is_vsc7388; > > + bool is_vsc7395; > > + bool is_vsc7398; > > How about having an u16/u32 chip_id instead? OK I already have the u16 chipid there. So I added some macros like IS_VSC7385() instead. > > + /* The switch internally uses a 8 byte header with length, > > + * source port, tag, LPA and priority. This is supposedly > > + * only accessible when operating the switch using the internal > > + * CPU or with an external CPU mapping the device in, but not > > + * when operating the switch over SPI and putting frames in/out > > + * on port 6 (the CPU port). So far we must assume that we > > + * cannot access the tag. (See "Internal frame header" section > > + * 3.9.1 in the manual.) > > I would be really good if we could get this to work even in SPI with the > CPU controlling the switch, I cannot really think of why the 8051 would > have to be involved, because having the 8051 means either the switch is > entirely standalone and runs off an EEPROM (which is additional cost on > your BOM), or the host, through SPI can entirely take over. If you think back on the original usecase in a switch or router it does make a bit of sense. Another OS is just in the back seat, the 8051 is supposed to do the heavy lifting here. This is different from devices without an internal CPU where some "outside OS" (Linux) is supposed to deal with the raw DSA-tagged frames. OpenWRT have a bunch of firmware from vendors being uploaded to the 8051 for different hardware: http://mirror2.openwrt.org/sources/vsc73x5-ucode.tar.bz2 These look very small, for the internal RAM. I think those are usually quite silly hacks and unnecessary, the only thing that really makes sense to load over there is the full SNMP stack that one can get from the vendor. I haven't disassembled their code though. That's why I think these devices are better off using this driver that just take control of the device over SPI. But it would be neat to figure out if we can get access to the raw frames. :/ > Is the datasheet public somehow? Partminer accidentally managed to download and archive it at one point, but it seems it disappeared from the link: https://www.buyparts.io/download.php?id=6263858&t=ih Since Microchips are much more liberal with datasheets I expect it to become public ASAP, I just got access to all datasheets when I asked. > > +static void vsc73xx_adjust_link(struct dsa_switch *ds, int port, > > + struct phy_device *phydev) > > +{ > > + struct vsc73xx *vsc = ds->priv; > > + u32 val; > > No is_pseudo_fixed_link() check, you really have to do all of this for > each front-panel port? That is really bad if that is the case... most > switches with front-panel built-in PHYs are at the very least capable of > re-configuring their internal MAC accordingly. Sadly this is needed. The manual explicitly says that the MAC has to be set up depending on speed immediately after the PHY comes up. I guess the ASIC engineer skipped any integration of the PHY and MAC and they are just connected, not really talking to each other. Luckily the .adjust_link() callback is handy for this. > > +static const struct of_device_id vsc73xx_of_match[] = { > > + { > > + .compatible = "vitesse,vsc7385", > > Would not you want to pass additional data here, like the possible port > layout/chip id to be reading? Since the chip is self-identifying (chip ID) it is not necessary. If I wanted to be overzealous I could check that the internal chip ID matches the compatible string (I have seen that some drivers do that.) Yours, Linus Walleij
Powered by blists - more mailing lists