netdev - Re: [PATCH v2 2/2] net: phy: marvell smi2usb mdio controller

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20200319223544.GA14699@wkz-x280>
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