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

[Andrew Lunn <andrew@lunn.ch>]

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@vger.kernel.org would be the correct list to
add.

     Andrew