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: <20200319154937.GB27807@lunn.ch>
Date:   Thu, 19 Mar 2020 16:49:37 +0100
From:   Andrew Lunn <andrew@...n.ch>
To:     Tobias Waldekranz <tobias@...dekranz.com>
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 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?

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?

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.

> +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?

> +	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?

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.

	   Andrew