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
| ||
|
Date: Thu, 19 Mar 2020 23:35:44 +0100 From: Tobias Waldekranz <tobias@...dekranz.com> To: Andrew Lunn <andrew@...n.ch> Cc: netdev@...r.kernel.org, f.fainelli@...il.com, hkallweit1@...il.com Subject: Re: [PATCH v2 2/2] net: phy: marvell smi2usb mdio controller On Thu, Mar 19, 2020 at 04:49:37PM +0100, Andrew Lunn wrote: > On Thu, Mar 19, 2020 at 02:59:52PM +0100, Tobias Waldekranz wrote: > > An MDIO controller present on development boards for Marvell switches > > from the Link Street (88E6xxx) family. > > > > Using this module, you can use the following setup as a development > > platform for switchdev and DSA related work. > > > > .-------. .-----------------. > > | USB----USB | > > | SoC | | 88E6390X-DB ETH1-10 > > | ETH----ETH0 | > > '-------' '-----------------' > > > > Signed-off-by: Tobias Waldekranz <tobias@...dekranz.com> > > --- > > > > v1->v2: > > - Reverse christmas tree ordering of local variables. > > > > --- > > MAINTAINERS | 1 + > > drivers/net/phy/Kconfig | 7 ++ > > drivers/net/phy/Makefile | 1 + > > drivers/net/phy/mdio-smi2usb.c | 137 +++++++++++++++++++++++++++++++++ > > 4 files changed, 146 insertions(+) > > create mode 100644 drivers/net/phy/mdio-smi2usb.c > > Hi Tobias > > Where does the name mii2usb come from? To me, it seems to be the wrong > way around, it is USB to MII. I suppose the Marvell Switch team could > of given it this name, for them the switch is the centre of their > world, and things connect to it? The name is indeed coming from Marvell. They use the term SMI over MDIO in most of their software and documentation. I had the same reaction to the name regarding the ordering of the terms, but felt it was best to go with the vendor's choice. > I'm just wondering if we should actually ignore Marvell and call it > usb2mii? > > I also think there should be a marvell prefix in the name, since were > could be other implementations of USB/MII. mvusb2mii? You're absolutely right that there should be an mv prefix in there. Calling it usb2mii seems like a misnomer though. At least for me, MII relates more to the data interface between a MAC and a PHY, whereas MDIO or SMI refers to the control interface (MDC/MDIO). How about just mdio-mvusb? > Do you know how this is implemented? Is it a product you can purchase? > Or a microcontroller on the board which implements this? It would be > an interesting product, especially on x86 machines which generally end > up doing bit-banging because of the lack of drivers using kernel MDIO. On the 88E6390X-DB, I know that there is a chip by the USB port that is probably either an MCU or a small FPGA. I can have a closer look at it when I'm at the office tomorrow if you'd like. I also remember seeing some docs from Marvell which seemed to indicate that they have a standalone product providing only the USB-to-MDIO functionality. The x86 use-case is interesting. It would be even more so if there was some way of loading a DSA DT fragment so that you could hook it up to your machine's Ethernet port. > > +static int smi2usb_probe(struct usb_interface *interface, > > + const struct usb_device_id *id) > > +{ > > + struct device *dev = &interface->dev; > > + struct mii_bus *mdio; > > + struct smi2usb *smi; > > + int err = -ENOMEM; > > + > > + mdio = devm_mdiobus_alloc_size(dev, sizeof(*smi)); > > + if (!mdio) > > + goto err; > > + > > ... > > > > +static void smi2usb_disconnect(struct usb_interface *interface) > > +{ > > + struct smi2usb *smi; > > + > > + smi = usb_get_intfdata(interface); > > + mdiobus_unregister(smi->mdio); > > + usb_set_intfdata(interface, NULL); > > + > > + usb_put_intf(interface); > > + usb_put_dev(interface_to_usbdev(interface)); > > +} > > I don't know enough about USB. Does disconnect have the same semantics > remove()? You used devm_mdiobus_alloc_size() to allocate the bus > structure. Will it get freed after disconnect? I've had USB devices > connected via flaky USB hubs and they have repeatedly disappeared and > reappeared. I wonder if in that case you are leaking memory if > disconnect does not release the memory? Disclaimer: This is my first ever USB driver. I assumed that since we're removing 'interface', 'interface->dev' will be removed as well and thus calling all devm hooks. > > + usb_put_intf(interface); > > + usb_put_dev(interface_to_usbdev(interface)); > > +} > > Another USB novice question. Is this safe? Could the put of interface > cause it to be destroyed? Then interface_to_usbdev() is called on > invalid memory? That does indeed look scary. I inverted the order of the calls to the _get_ functions, which I got from the USB skeleton driver. I'll try to review some other drivers to see if I can figure this out. > Maybe this should be cross posted to a USB mailing list, so we can get > the USB aspects reviewed. The MDIO bits seem good to me. Good idea. Any chance you can help an LKML rookie out? How does one go about that? Do I simply reply to this thread and add the USB list, or do I post the patches again as a new series? Any special tags? Is there any documentation available? > Andrew
Powered by blists - more mailing lists