Re: [RFC PATCH 3/4] PLATFORM: Introduce async platform_data attachapi

[Andy Green]

On 03/13/2011 11:26 PM, Somebody in the thread at some point said:
> On Sun, Mar 13, 2011 at 06:13:07PM +0000, Andy Green wrote:
> > On 03/13/2011 05:48 PM, Somebody in the thread at some point said:
> > > On Sun, Mar 13, 2011 at 05:13:31PM +0000, Andy Green wrote:
> > > > So when there's a bit more of Device Tree in evidence, are you going
> > > > to accept Device-tree based patches in usbnet etc along these lines,
> > > > or does that trigger the "do it in userspace" response, in which
> > > > case we are both wasting each others' time continuing to discuss
> > > > this at all?
> > >
> > > As mentioned numerous times before, network naming stuff happens in
> > > userspace, not in the kernel, so no matter what infrastructure is added,
> >
> > The naming of that network interface happens in smsc95xx, in kernel:
> > it uses a dodgy heuristic to choose between usb%d eth%d and others
> > and gets it wrong in this case due to stuff outside its view.
> > There's nothing golden and wonderful about that which needs
> > protecting from outside hints.
>
> Then why not always do this in userspace correctly?  It's the _exact_
> same problem that the server companies have in naming their network
> devices in a proper manner (i.e. the port with the 0 label on it wants
> to always be eth0).  We have the tools today to solve this issue, in a
> consistant and proper manner, please use them.

It is not at all the same problem.

Here, the usbnet driver tries to figure out what kind of device label it 
should use, the numbering is then dependent on the label and there's no 
problem coming from the numbering, nor do the patches touch that.

The first issue for usbnet is right now, it has no real insight into 
what the appropriate interface name is because it is out of its view if 
the USB Ethernet bridge is soldered onboard or not.  You know, it is 
also not in a generic userspace's view what is soldered on that board 
either, so I can only read your pushing of that 'solution' as "get this 
out of my hair".  The one place that can properly know it right now is 
the board definition file in the kernel, maybe Device Tree too in the 
future.

To do it what you are calling the "correct" way in userland, you are 
okay with the driver selecting the wrong interface name, condemning 
userland to regenerate board-specific knowledge from grepping 
/proc/cpuinfo, inferring in userland what can easily and justly be known 
in the board definition file in kernel, and overriding the wrong 
interface name.  In all that, Caesar's hands are clean alright, but 
don't tell me this is a good job and the correct solution.

Despite not having the information, the driver does have go at a 
heuristic, but because net->dev_addr[] is always coming from 
random_ether_addr(), which rightly forces [0] & 2 to 1 indicating it's a 
private namespace MAC address, the heuristic is broken and never selects 
eth%d.

-               // heuristic:  "usb%d" for links we know are two-host,
-               // else "eth%d" when there's reasonable doubt.  userspace
-               // can rename the link if it knows better.
-               if ((dev->driver_info->flags & FLAG_ETHER) != 0 &&
-                   (net->dev_addr [0] & 0x02) == 0)
+               /*
+                * heuristic:  "usb%d" for links we know are two-host,
+                * else "eth%d" when there's reasonable doubt.  userspace
+                * can rename the link if it knows better.  Async
+                * platform_data can also override this if it knows better
+                * based on knowledge of what this link is wired up to.
+                */
+               if ((((dev->driver_info->flags & FLAG_ETHER) != 0 &&
+                   (net->dev_addr[0] & 0x02) == 0)) ||
+                   (pdata && pdata->flags &
+ 
USBNET_PLATDATA_FLAG__FORCE_ETH_IFNAME))
                        strcpy (net->name, "eth%d");


static inline void random_ether_addr(u8 *addr)
{
        get_random_bytes (addr, ETH_ALEN);
        addr [0] &= 0xfe;       /* clear multicast bit */
        addr [0] |= 0x02;       /* set local assignment bit (IEEE802) */
}

If you look further down, it is making an attempt to choose the right 
name at interface creation time by taking information from other sources 
already

                /* WLAN devices should always be named "wlan%d" */
                if ((dev->driver_info->flags & FLAG_WLAN) != 0)

My patch just has it also look if there is a pdata->flags to see if the 
board definition file is guiding it to choose eth%d.

The second issue for usbnet which the patchset solves is that right now, 
it will always give a random MAC address that differs each boot, driving 
dhpcd insane.

Now once again, the existing code will take information from other 
sources to guide it about that too.  In smsc95xx usbnet driver, it looks 
to see if there is an EEPROM attached to the chip and will replace the 
random MAC address with the one from there if there is.  The patch 
extends that to check usbnet platform_data to see if a MAC address has 
been sent up from the board definition file.

 static void smsc95xx_init_mac_address(struct usbnet *dev)
 {
+       struct usbnet_platform_data *pdata = dev->udev->dev.platform_data;
+
+       /*
+        * if netdev platform data has taken responsibility for forcing
+        * the MAC then nothing to do here
+        */
+
+       if (pdata && pdata->flags & USBNET_PLATDATA_FLAG__USE_MAC)
+               return;
+
        /* try reading mac address from EEPROM */
        if (smsc95xx_read_eeprom(dev, EEPROM_MAC_OFFSET, ETH_ALEN,
                        dev->net->dev_addr) == 0) {

So given what it already does in a broken way for these valid 
implementations of smsc95xx with no EEPROM, I really do not understand 
what sanctity of these drivers is being violated by having it do the 
same things better using a couple of extra lines with platform_data.

For sure, in any other bus, we will never have this discussion about if 
the device driver should use relevant platform_data to be guided about 
what to do, that is what platform_data is for and it is very widely used 
indeed.

> > > insisting on naming the network device 'eth0' is not going to happen
> > > within the kernel.  Please use the tools we have today to do this with
> > > no kernel changes.
> > >
> > > As for other changes, it all depends on what you need to accomplish,
> > > right?  Those will be gladly reviewed on a case-by-case basis.
> >
> > The immediate thing I would have liked to accomplish is to help
> > smsc95xx make the right decision about naming and to use the MAC
> > address in platform_data.  It seemed this was a general issue
> > though, so I generalized how it was done.  But I don't have examples
> > on my desk of boards with soldered-on USB other than smsc95xx
> > Ethernet bridge.
> >
> > At least, I am grateful we get down to brass tacks now and you
> > clearly reject allowing the driver to take guidance to act more
> > appropriately for its environment in the first place.  Therefore,
> > this won't get fixed by Device Tree either.
>
> I beg to differ.  I'm stating that USB drivers can not use their
> numbering to do anything with them as that is never going to be
> deterministic.
>
> That's all.  You seem to disagree with this.

Actually as I indicated I am willing to pretend to agree it applies to 
these SoC cases (no argument it is certainly so in a non-SoC PC type 
situation where the board definition file / platform_data is not used), 
and offered to adapt the bus selection to use a usb host interface 
pointer from the board definition file that should solve that objection.

> > So, we don't need to discuss this any further: thanks again for your
> > time.
>
> Ok, so does this mean that you are convinced that I am correct here, or
> are just giving up and going to keep your patches outside of the kernel,
> or something else?

It means I acknowledge you are the guy in the way of solving these 
issues in usbnet, and that these patches will be discarded without your 
approval.  Normally, for me offering patches upstream is a side issue 
because I am trying to deliver some product, so with this level of 
pushback I would just shrug and hold it in my tree.  In this case though 
I am being paid to help TI solve issues upstream, the patches do solve 
real issues for them, but they're trying to eliminate BSP tree concept 
in favour of everything upstream, so there is no other place for them.

I'm willing to put in effort as I have in this thread explaining things 
more clearly when the proposal was not understood, and writing patches 
so it is unambiguous, adapting to feedback that improves things or even 
explains why the proposal isn't usable, but I also understand that where 
you are wrong and are simply filtering out the arguments to the contrary 
due to dogma, I am wasting my time, your time, you will stay wrong and 
the driver will stay broken.  In the meanwhile, I will sleep badly and 
my wife will be unhappy with me for burning a weekend on it, so if 
there's no useful path forward let's not protract the agony for either 
of us.

-Andy
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/