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: <20200319230002.GO27807@lunn.ch>
Date:   Fri, 20 Mar 2020 00:00:02 +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

Hi Tobias

> How about just mdio-mvusb?

Yes, i like that.

> 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.

I would be interested in knowing more.

> 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.

We don't have that at the moment. But so long as you only need
internal copper PHYs, it is possible to use a platform device and it
all just works.

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

And i've only ever written one which has been merged.

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

I would fixup the naming and repost. You can put whatever comments you
want under the --- marker. So say this driver should be merged via
netdev, but you would appreciate reviews of the USB parts from USB
maintainers. linux-usb@...r.kernel.org would be the correct list to
add.

     Andrew