The following series adds two apis which allow board definition
files to register bindings between device paths (like usb1/1-1/1-1.1)
and platform data. It's for use in the case that there are hardwired
chips on a board that need to be configured from the board definition
file, but the creation of the device is asynchronous to the boot
action. Beagle XM and Panda are both real-world examples where there
are hardwired chips on the PCB with fixed interconnect using USB
protocol that can benefit from platform_data for the usual reasons.
This patcheset is the platform part of a three part set which when
taken as a whole, allows the board definition file to configure
USB-wired onboard network interface assets.
---
Andy Green (4):
PLATFORM: Add some documentation to platform docs about async platform_data
PLATFORM: Introduce async platform_data attach api
PLATFORM: Introduce registration function for async platform data maps
PLATFORM: introduce structure to bind async platform data to a dev path name
Documentation/driver-model/platform.txt | 25 +++++++++
drivers/base/platform.c | 91 +++++++++++++++++++++++++++++++
include/linux/platform_device.h | 22 +++++++
3 files changed, 138 insertions(+), 0 deletions(-)
--
Signature
This structure allows tagging arbitrary platform_data that can't be attached
to a device until after it is probed, with the device path name that it is
to be attached to.
Signed-off-by: Andy Green <[email protected]>
---
include/linux/platform_device.h | 17 +++++++++++++++++
1 files changed, 17 insertions(+), 0 deletions(-)
diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h
index 2e700ec..d8c0ba9 100644
--- a/include/linux/platform_device.h
+++ b/include/linux/platform_device.h
@@ -193,4 +193,21 @@ static inline char *early_platform_driver_setup_func(void) \
}
#endif /* MODULE */
+/**
+ * platform_async_platform_data - maps a known bus + device name on to
+ * platform_data to be attached to that device
+ * when it is eventually instantiated. For use
+ * with onboard devices on buses that probe
+ * asynchronously. Device path fields must
+ * be separated with '/'.
+ * @device_path: bus / device path, eg, "usb1/1-1/1-1.1"
+ * @platform_data: platform_data to attach to device matching the
+ * device_path
+ */
+
+struct platform_async_platform_data {
+ const char *device_path;
+ void *platform_data;
+};
+
#endif /* _PLATFORM_DEVICE_H_ */
This adds a small platform API that lets a board definition file
register a mapping structure for asynchronously probed devices so
platform_data can be attached to them at probe time.
Signed-off-by: Andy Green <[email protected]>
---
drivers/base/platform.c | 21 +++++++++++++++++++++
include/linux/platform_device.h | 3 +++
2 files changed, 24 insertions(+), 0 deletions(-)
diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index f051cff..180e372 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -31,6 +31,12 @@ struct device platform_bus = {
};
EXPORT_SYMBOL_GPL(platform_bus);
+struct platform_async_platform_data *platform_async_platform_data_map;
+EXPORT_SYMBOL_GPL(platform_async_platform_data_map);
+
+int platform_async_platform_data_count;
+EXPORT_SYMBOL_GPL(platform_async_platform_data_count);
+
/**
* platform_get_resource - get a resource for a device
* @dev: platform device
@@ -1332,3 +1338,18 @@ void __init early_platform_cleanup(void)
}
}
+/**
+ * platform_async_platform_data_register - register an array of async-
+ * probed bus / device names mapping to
+ * plaform_data for that device.
+ * @map: the array of devname vs platform_data mapping structs
+ * @count: the count of structs in the @map array
+ */
+
+void platform_async_platform_data_register(
+ struct platform_async_platform_data *map, int count)
+{
+ platform_async_platform_data_map = map;
+ platform_async_platform_data_count = count;
+}
+EXPORT_SYMBOL_GPL(platform_async_platform_data_register);
diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h
index d8c0ba9..19ea497 100644
--- a/include/linux/platform_device.h
+++ b/include/linux/platform_device.h
@@ -210,4 +210,7 @@ struct platform_async_platform_data {
void *platform_data;
};
+extern void platform_async_platform_data_register(
+ struct platform_async_platform_data *map, int count);
+
#endif /* _PLATFORM_DEVICE_H_ */
This introduces a platform API so busses can allow platform_data to
be attached to any struct device they create from probing in one step.
The function checks through the async platform_data map if one was
previously registered, and checks the device's device path for itself
and its parents against the mapped device path names.
If it sees a match, it attaches the associated platform_data and sets
that map entry's device_path to NULL so no further time is spent trying
to match it.
Signed-off-by: Andy Green <[email protected]>
---
drivers/base/platform.c | 70 +++++++++++++++++++++++++++++++++++++++
include/linux/platform_device.h | 2 +
2 files changed, 72 insertions(+), 0 deletions(-)
diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index 180e372..534bf3a 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -1353,3 +1353,73 @@ void platform_async_platform_data_register(
platform_async_platform_data_count = count;
}
EXPORT_SYMBOL_GPL(platform_async_platform_data_register);
+
+/**
+ * platform_async_platform_data_attach - if there is any async devname map
+ * defined, go through it looking for a match with the
+ * device's path considering its parent device names.
+ * If a match is found, attach the corresponding
+ * platform_data and the entry in the map table set to
+ * NULL so it won't be looked for again.
+ *
+ * @dev: device to have data attched to if it matches any map entry
+ */
+void platform_async_platform_data_attach(struct device *dev)
+{
+ struct platform_async_platform_data *map;
+ const char *path;
+ int count;
+ const char *p;
+ int len;
+ struct device *devn;
+
+ map = platform_async_platform_data_map;
+ count = platform_async_platform_data_count;
+
+ while (count--) {
+
+ if (map->device_path == NULL) {
+ map++;
+ continue;
+ }
+
+ p = map->device_path + strlen(map->device_path);
+ devn = dev;
+
+ while (devn) {
+
+ path = dev_name(devn);
+ len = strlen(path);
+
+ if ((p - map->device_path) < len) {
+ devn = NULL;
+ continue;
+ }
+
+ p -= len;
+
+ if (strncmp(path, p, len)) {
+ devn = NULL;
+ continue;
+ }
+
+ devn = devn->parent;
+ if (p == map->device_path) {
+ dev_info(dev, "Attched async platform data\n");
+ dev->platform_data = map->platform_data;
+ map->device_path = NULL;
+ return;
+ }
+
+ if (devn != NULL && (p - map->device_path) < 2)
+ devn = NULL;
+
+ p--;
+ if (devn != NULL && *p != '/')
+ devn = NULL;
+ }
+
+ map++;
+ }
+}
+EXPORT_SYMBOL_GPL(platform_async_platform_data_attach);
diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h
index 19ea497..4b5fa12 100644
--- a/include/linux/platform_device.h
+++ b/include/linux/platform_device.h
@@ -213,4 +213,6 @@ struct platform_async_platform_data {
extern void platform_async_platform_data_register(
struct platform_async_platform_data *map, int count);
+extern void platform_async_platform_data_attach(struct device *dev);
+
#endif /* _PLATFORM_DEVICE_H_ */
Signed-off-by: Andy Green <[email protected]>
---
Documentation/driver-model/platform.txt | 25 +++++++++++++++++++++++++
1 files changed, 25 insertions(+), 0 deletions(-)
diff --git a/Documentation/driver-model/platform.txt b/Documentation/driver-model/platform.txt
index 41f4163..0c34156 100644
--- a/Documentation/driver-model/platform.txt
+++ b/Documentation/driver-model/platform.txt
@@ -96,6 +96,31 @@ System setup also associates those clocks with the device, so that that
calls to clk_get(&pdev->dev, clock_name) return them as needed.
+Providing platform_data to onboard devices on asynchronously probed buses
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+Some boards have fixed onboard assets that would normally be dealt with
+by using plaform_data set in the usual way in the board definition file,
+but cannot directly do so because they are on a bus that is probed
+asynchronously. For emebedded devices, often the broard definition
+file is adding platform devices for the buses involved, like USB host
+and gadget, in a fixed order using platform_add_devices(), so the device
+path of these fixed soldered-on-the-board assets is deterministic.
+
+For buses and devices that are named deterministically at boot-time,
+you can define platform_data that binds to these devices when they are
+probed by declaring a map of device paths to platform_data in your
+board definition file before adding the bus platform devices, using
+
+ void platform_register_async_platform_data(
+ struct platform_async_platform_data *map, int count);
+
+Buses that participate in this scheme will then check this mapping list
+for corresponding platform_data as they are probed and apply it
+automatically. An example device path mapping is
+
+ "usb1/1-1/1-1.1"
+
+
Legacy Drivers: Device Probing
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Some drivers are not fully converted to the driver model, because they take
Hi,
On Saturday, March 12, 2011, Andy Green wrote:
> This structure allows tagging arbitrary platform_data that can't be attached
> to a device until after it is probed, with the device path name that it is
> to be attached to.
>
> Signed-off-by: Andy Green <[email protected]>
> ---
>
> include/linux/platform_device.h | 17 +++++++++++++++++
> 1 files changed, 17 insertions(+), 0 deletions(-)
>
> diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h
> index 2e700ec..d8c0ba9 100644
> --- a/include/linux/platform_device.h
> +++ b/include/linux/platform_device.h
> @@ -193,4 +193,21 @@ static inline char *early_platform_driver_setup_func(void) \
> }
> #endif /* MODULE */
>
> +/**
> + * platform_async_platform_data - maps a known bus + device name on to
> + * platform_data to be attached to that device
> + * when it is eventually instantiated. For use
> + * with onboard devices on buses that probe
> + * asynchronously. Device path fields must
> + * be separated with '/'.
> + * @device_path: bus / device path, eg, "usb1/1-1/1-1.1"
> + * @platform_data: platform_data to attach to device matching the
> + * device_path
> + */
> +
> +struct platform_async_platform_data {
> + const char *device_path;
> + void *platform_data;
> +};
> +
> #endif /* _PLATFORM_DEVICE_H_ */
Using device paths for this purpose seems to be very fragile to me. Isn't
there any better solution?
Rafael
On 03/12/2011 11:29 PM, Somebody in the thread at some point said:
Hi -
> On Saturday, March 12, 2011, Andy Green wrote:
>> This structure allows tagging arbitrary platform_data that can't be attached
>> to a device until after it is probed, with the device path name that it is
>> to be attached to.
>> +struct platform_async_platform_data {
>> + const char *device_path;
>> + void *platform_data;
>> +};
>> +
>> #endif /* _PLATFORM_DEVICE_H_ */
>
> Using device paths for this purpose seems to be very fragile to me. Isn't
> there any better solution?
Given that this targets board definition files which commonly do the
platform_add_device for the USB bus controller synchronously, and the
bus-connected devices it is aimed at are soldered on to the board
connected to specific bus controllers, the bus paths are completely
deterministic.
-Andy
On Sat, Mar 12, 2011 at 10:32:27PM +0000, Andy Green wrote:
> This introduces a platform API so busses can allow platform_data to
> be attached to any struct device they create from probing in one step.
>
> The function checks through the async platform_data map if one was
> previously registered, and checks the device's device path for itself
> and its parents against the mapped device path names.
>
> If it sees a match, it attaches the associated platform_data and sets
> that map entry's device_path to NULL so no further time is spent trying
> to match it.
This _really_ should just use the device tree stuff, that is what it is
for, please don't duplicate it here in a not-as-flexible way.
Why wouldn't that work for you instead for this type of thing?
thanks,
greg k-h
On Sat, Mar 12, 2011 at 11:39:15PM +0000, Andy Green wrote:
> On 03/12/2011 11:29 PM, Somebody in the thread at some point said:
>
> Hi -
>
> >On Saturday, March 12, 2011, Andy Green wrote:
> >>This structure allows tagging arbitrary platform_data that can't be attached
> >>to a device until after it is probed, with the device path name that it is
> >>to be attached to.
>
> >>+struct platform_async_platform_data {
> >>+ const char *device_path;
> >>+ void *platform_data;
> >>+};
> >>+
> >> #endif /* _PLATFORM_DEVICE_H_ */
> >
> >Using device paths for this purpose seems to be very fragile to me. Isn't
> >there any better solution?
>
> Given that this targets board definition files which commonly do the
> platform_add_device for the USB bus controller synchronously, and
> the bus-connected devices it is aimed at are soldered on to the
> board connected to specific bus controllers, the bus paths are
> completely deterministic.
No they are not.
The physical layout is deterministic, but the bus number, and device
number, is not. You are using the bus number here in this path, so that
is not going to work, sorry.
thanks,
greg k-h
On Sunday, March 13, 2011, Greg KH wrote:
> On Sat, Mar 12, 2011 at 10:32:27PM +0000, Andy Green wrote:
> > This introduces a platform API so busses can allow platform_data to
> > be attached to any struct device they create from probing in one step.
> >
> > The function checks through the async platform_data map if one was
> > previously registered, and checks the device's device path for itself
> > and its parents against the mapped device path names.
> >
> > If it sees a match, it attaches the associated platform_data and sets
> > that map entry's device_path to NULL so no further time is spent trying
> > to match it.
>
> This _really_ should just use the device tree stuff, that is what it is
> for, please don't duplicate it here in a not-as-flexible way.
I agree.
@Andy: If it doesn't work for you for some reason, please let us know the
usage case that is not covered (in detail).
Thanks,
Rafael
On 03/13/2011 01:03 AM, Somebody in the thread at some point said:
>>> Using device paths for this purpose seems to be very fragile to me. Isn't
>>> there any better solution?
>>
>> Given that this targets board definition files which commonly do the
>> platform_add_device for the USB bus controller synchronously, and
>> the bus-connected devices it is aimed at are soldered on to the
>> board connected to specific bus controllers, the bus paths are
>> completely deterministic.
>
> No they are not.
>
> The physical layout is deterministic, but the bus number, and device
> number, is not. You are using the bus number here in this path, so that
> is not going to work, sorry.
Okay. This is not a PC we are talking about.
If the platform / board definition file is registering the USB hosts
synchronously at boot time, the driver is composed into the monolithic
kernel, there are no PCI busses or whatever on the SoC, the bus indexing
is totally deterministic. This is extremely common in the platform /
SoC case and is the case the patchset is targeted at. Even further, the
only time you'd use it is to reach a USB asset that is wired up the same
board permanently as well.
Anyway this seems moot by now.
-Andy
On 03/13/2011 10:41 AM, Somebody in the thread at some point said:
> On Sunday, March 13, 2011, Greg KH wrote:
>> On Sat, Mar 12, 2011 at 10:32:27PM +0000, Andy Green wrote:
>>> This introduces a platform API so busses can allow platform_data to
>>> be attached to any struct device they create from probing in one step.
>>>
>>> The function checks through the async platform_data map if one was
>>> previously registered, and checks the device's device path for itself
>>> and its parents against the mapped device path names.
>>>
>>> If it sees a match, it attaches the associated platform_data and sets
>>> that map entry's device_path to NULL so no further time is spent trying
>>> to match it.
>>
>> This _really_ should just use the device tree stuff, that is what it is
>> for, please don't duplicate it here in a not-as-flexible way.
>
> I agree.
>
> @Andy: If it doesn't work for you for some reason, please let us know the
> usage case that is not covered (in detail).
The device tree stuff does not yet exist in a workable way,
platform_data is established everywhere except USB bus. Device tree
brings in bootloader version as a dependency: this method doesn't.
Using platform / board definition files to define and configure the SoC
and most or all of its onboard assets is a technique widely deployed in
SoC board definition files. The patch just very lightly extends
platform_data to be workable with USB bus devices like it is the other
buses, within the goal of being able to configure onboard assets at
boot-time, so it's quite unremarkable from that POV.
However if you don't have that perspective on it, and think about it on
a PC where it is not intended to be used, no doubt the patchset's
approach is hard to understand as useful.
Device Tree will be a nice improvement in many ways when it is done, in
this particular case though presumably it'll have to patch usbnet and
smsc95xx in a similar way to offer similar configurability.
In the meanwhile, either Panda and Beagle will stay as they are for
these misfeatures or specific userlands will have to be stunk up with
/proc/cpuinfo-grepping board-specific ditro-by-distro quirk management.
Anyway, thanks for all the comments on this RFC.
-Andy
On Sunday, March 13, 2011, Andy Green wrote:
> On 03/13/2011 01:03 AM, Somebody in the thread at some point said:
>
> >>> Using device paths for this purpose seems to be very fragile to me. Isn't
> >>> there any better solution?
> >>
> >> Given that this targets board definition files which commonly do the
> >> platform_add_device for the USB bus controller synchronously, and
> >> the bus-connected devices it is aimed at are soldered on to the
> >> board connected to specific bus controllers, the bus paths are
> >> completely deterministic.
> >
> > No they are not.
> >
> > The physical layout is deterministic, but the bus number, and device
> > number, is not. You are using the bus number here in this path, so that
> > is not going to work, sorry.
>
> Okay. This is not a PC we are talking about.
>
> If the platform / board definition file is registering the USB hosts
> synchronously at boot time, the driver is composed into the monolithic
> kernel, there are no PCI busses or whatever on the SoC, the bus indexing
> is totally deterministic. This is extremely common in the platform /
> SoC case and is the case the patchset is targeted at. Even further, the
> only time you'd use it is to reach a USB asset that is wired up the same
> board permanently as well.
>
> Anyway this seems moot by now.
However, if you add a new infrastructure like this, it should be at least
usable on systems that you description doesn't apply to.
Thanks,
Rafael
On Sunday, March 13, 2011, Andy Green wrote:
> On 03/13/2011 10:41 AM, Somebody in the thread at some point said:
> > On Sunday, March 13, 2011, Greg KH wrote:
> >> On Sat, Mar 12, 2011 at 10:32:27PM +0000, Andy Green wrote:
> >>> This introduces a platform API so busses can allow platform_data to
> >>> be attached to any struct device they create from probing in one step.
> >>>
> >>> The function checks through the async platform_data map if one was
> >>> previously registered, and checks the device's device path for itself
> >>> and its parents against the mapped device path names.
> >>>
> >>> If it sees a match, it attaches the associated platform_data and sets
> >>> that map entry's device_path to NULL so no further time is spent trying
> >>> to match it.
> >>
> >> This _really_ should just use the device tree stuff, that is what it is
> >> for, please don't duplicate it here in a not-as-flexible way.
> >
> > I agree.
> >
> > @Andy: If it doesn't work for you for some reason, please let us know the
> > usage case that is not covered (in detail).
>
> The device tree stuff does not yet exist in a workable way,
> platform_data is established everywhere except USB bus. Device tree
> brings in bootloader version as a dependency: this method doesn't.
It is not the same device tree we are talking about. :-)
I mean device hierarchy (and I guess Greg meant the same).
Thanks,
Rafael
On 03/13/2011 12:53 PM, Somebody in the thread at some point said:
>>>> This _really_ should just use the device tree stuff, that is what it is
>>>> for, please don't duplicate it here in a not-as-flexible way.
>>>
>>> I agree.
>>>
>>> @Andy: If it doesn't work for you for some reason, please let us know the
>>> usage case that is not covered (in detail).
>>
>> The device tree stuff does not yet exist in a workable way,
>> platform_data is established everywhere except USB bus. Device tree
>> brings in bootloader version as a dependency: this method doesn't.
>
> It is not the same device tree we are talking about. :-)
>
> I mean device hierarchy (and I guess Greg meant the same).
I see. Elsewhere on the previous thread people were proposing to use
New Shiny Device Tree, hence the confusion.
I am using the old style device tree to walk the device's parent path.
What were you guys actually suggesting to do differently via the device
tree then that's cleaner?
-Andy
On 03/13/2011 12:51 PM, Somebody in the thread at some point said:
>> Okay. This is not a PC we are talking about.
>>
>> If the platform / board definition file is registering the USB hosts
>> synchronously at boot time, the driver is composed into the monolithic
>> kernel, there are no PCI busses or whatever on the SoC, the bus indexing
>> is totally deterministic. This is extremely common in the platform /
>> SoC case and is the case the patchset is targeted at. Even further, the
>> only time you'd use it is to reach a USB asset that is wired up the same
>> board permanently as well.
>>
>> Anyway this seems moot by now.
>
> However, if you add a new infrastructure like this, it should be at least
> usable on systems that you description doesn't apply to.
Sounds reasonable, except the platform data is coming from a
board-specific board definition file at boot-time to talk about assets
that are on fixed interfaces of a specific board. It's not really
applicable to wider generic bus use, just like platform_data usually
isn't and has to be targeted at "device at XYZ on I2C bus n" with
knowledge of what driver is bound to that device. Despite that
"impedence mismatch", it covers the SoC onboard USB asset case just fine
as it is.
If you mean though, that the patch series implements new configuration
options in usbnet that are anyway interesting to expose for the general
case, I can see the point but don't know enough about udev / usb
subsystem internals to suggest a way to expose the options nicely.
It's also commented the "right" thing to do is to have the driver set
the device up to wrong defaults like inappropriate interface name /
random MAC and have userland clean up the mess, rather than have the
board file ask the driver to set things appropriately.
-Andy
On Sun, Mar 13, 2011 at 11:22:40AM +0000, Andy Green wrote:
> On 03/13/2011 01:03 AM, Somebody in the thread at some point said:
>
> >>>Using device paths for this purpose seems to be very fragile to me. Isn't
> >>>there any better solution?
> >>
> >>Given that this targets board definition files which commonly do the
> >>platform_add_device for the USB bus controller synchronously, and
> >>the bus-connected devices it is aimed at are soldered on to the
> >>board connected to specific bus controllers, the bus paths are
> >>completely deterministic.
> >
> >No they are not.
> >
> >The physical layout is deterministic, but the bus number, and device
> >number, is not. You are using the bus number here in this path, so that
> >is not going to work, sorry.
>
> Okay. This is not a PC we are talking about.
I know that, and again, it doesn't matter.
You CAN NOT GUARANTEE the USB device ordering of bus numbers or device
numbers. It's that simple.
> If the platform / board definition file is registering the USB hosts
> synchronously at boot time, the driver is composed into the
> monolithic kernel, there are no PCI busses or whatever on the SoC,
> the bus indexing is totally deterministic.
Not true, it could change for a number of reasons, not the least being
your kernel version changed.
So again NEVER rely on this, bad things could happen in the field when
you least expect it.
thanks,
greg k-h
On Sun, Mar 13, 2011 at 01:21:20PM +0000, Andy Green wrote:
> On 03/13/2011 12:53 PM, Somebody in the thread at some point said:
>
> >>>>This _really_ should just use the device tree stuff, that is what it is
> >>>>for, please don't duplicate it here in a not-as-flexible way.
> >>>
> >>>I agree.
> >>>
> >>>@Andy: If it doesn't work for you for some reason, please let us know the
> >>>usage case that is not covered (in detail).
> >>
> >>The device tree stuff does not yet exist in a workable way,
> >>platform_data is established everywhere except USB bus. Device tree
> >>brings in bootloader version as a dependency: this method doesn't.
> >
> >It is not the same device tree we are talking about. :-)
> >
> >I mean device hierarchy (and I guess Greg meant the same).
>
> I see. Elsewhere on the previous thread people were proposing to
> use New Shiny Device Tree, hence the confusion.
Yes, I meant the "new shiny device tree" work from Grant, who in an
earlier message, said that this could all be done using that instead of
your proposal.
thanks,
greg k-h
On Sunday, March 13, 2011, Andy Green wrote:
> On 03/13/2011 12:51 PM, Somebody in the thread at some point said:
>
> >> Okay. This is not a PC we are talking about.
> >>
> >> If the platform / board definition file is registering the USB hosts
> >> synchronously at boot time, the driver is composed into the monolithic
> >> kernel, there are no PCI busses or whatever on the SoC, the bus indexing
> >> is totally deterministic. This is extremely common in the platform /
> >> SoC case and is the case the patchset is targeted at. Even further, the
> >> only time you'd use it is to reach a USB asset that is wired up the same
> >> board permanently as well.
> >>
> >> Anyway this seems moot by now.
> >
> > However, if you add a new infrastructure like this, it should be at least
> > usable on systems that you description doesn't apply to.
>
> Sounds reasonable, except the platform data is coming from a
> board-specific board definition file at boot-time to talk about assets
> that are on fixed interfaces of a specific board.
I'm not sure how this contradicts what I said above.
> It's not really applicable to wider generic bus use, just like platform_data
> usually isn't and has to be targeted at "device at XYZ on I2C bus n" with
> knowledge of what driver is bound to that device. Despite that
> "impedence mismatch", it covers the SoC onboard USB asset case just fine
> as it is.
So, you want to have a mechanism telling the driver "if the device
happens to have this particular path, use that platform data", right?
And it works because the initialization code kind of knows what path the
device is going to be at, so it can predict that and provide the mathing data.
Unfortunately, this relies on how device paths are constructed at the moment,
so if this approach is adopted in general, it will prevent us from changing
that way in the future (or at least it will make that very difficult).
Perhaps you could use some other kind of device identification here?
Rafael
On 03/13/2011 04:15 PM, Somebody in the thread at some point said:
> On Sun, Mar 13, 2011 at 01:21:20PM +0000, Andy Green wrote:
>> On 03/13/2011 12:53 PM, Somebody in the thread at some point said:
>>
>>>>>> This _really_ should just use the device tree stuff, that is what it is
>>>>>> for, please don't duplicate it here in a not-as-flexible way.
>>>>>
>>>>> I agree.
>>>>>
>>>>> @Andy: If it doesn't work for you for some reason, please let us know the
>>>>> usage case that is not covered (in detail).
>>>>
>>>> The device tree stuff does not yet exist in a workable way,
>>>> platform_data is established everywhere except USB bus. Device tree
>>>> brings in bootloader version as a dependency: this method doesn't.
>>>
>>> It is not the same device tree we are talking about. :-)
>>>
>>> I mean device hierarchy (and I guess Greg meant the same).
>>
>> I see. Elsewhere on the previous thread people were proposing to
>> use New Shiny Device Tree, hence the confusion.
>
> Yes, I meant the "new shiny device tree" work from Grant, who in an
> earlier message, said that this could all be done using that instead of
> your proposal.
That is what I took you to mean, since I already use oldstyle device
tree as far as I could see it was possible. So I have no idea what
Rafael thought you or he meant by strongly agreeing with you when he was
mistaken that thought you meant oldstyle device tree. Anyway never mind.
Well I never heard mentioned before that Device Tree targets
asynchronously probed device configuration. If it does, and can do the
same effective as the first patchset, then I guess that will (when it
exists) fulfil a similar job and that'd be fine.
But what this overall patch set does in panda.c, usbnet and smsc95xx
will need the same work done on it either way to deliver the same new
configuration features in the driver side, via Shiny New Device Tree or
whatever.
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?
-Andy
On 03/13/2011 04:58 PM, Somebody in the thread at some point said:
> So, you want to have a mechanism telling the driver "if the device
> happens to have this particular path, use that platform data", right?
Yeah.
> And it works because the initialization code kind of knows what path the
> device is going to be at, so it can predict that and provide the mathing data.
>
> Unfortunately, this relies on how device paths are constructed at the moment,
> so if this approach is adopted in general, it will prevent us from changing
> that way in the future (or at least it will make that very difficult).
>
> Perhaps you could use some other kind of device identification here?
I'm sorry what prevents you changing paths in the future?
Nothing does, if you change the bus tag from like usb1 to UsB_1 you just
fix up the strings in the board definition files at the same time, they
are sitting there in the same tree and
grep platform_async_platform_data arch/* -R
will show them all up in one hit. It's no different if you changed the
name of a driver, you'd patch the board definition files with devices
that need to bind to that driver name to uplevel them to the new name.
The board definition file for these SoC cases usually has access to a
pointer to the host controller directly since it instantiates them, if
this was the only stumbling point and you thought it helped something
the matching system can look for that particular pointer being a parent
of the candidate probed device.
-Andy
On 03/13/2011 04:14 PM, Somebody in the thread at some point said:
Hi -
> You CAN NOT GUARANTEE the USB device ordering of bus numbers or device
> numbers. It's that simple.
>
>> If the platform / board definition file is registering the USB hosts
>> synchronously at boot time, the driver is composed into the
>> monolithic kernel, there are no PCI busses or whatever on the SoC,
>> the bus indexing is totally deterministic.
>
> Not true, it could change for a number of reasons, not the least being
> your kernel version changed.
>
> So again NEVER rely on this, bad things could happen in the field when
> you least expect it.
Okay, I can't see how and you did not explain how, but let's agree with
what you are saying.
Unlike the Shiny Device Tree path where the binding device path string
is in the bootloader, with this patch series the binding device path is
in the board definition file, ie, part of the same kernel. If an
upgrade breaks it, the guy can look in /sys on his new broken kernel,
find the new path and uplevel it to that and he's consistent again. If
he goes back to an older kernel, it still works consistently (unlike if
he updated his bootloader that now only knows the new way).
However as I said to Rafael if he thought bus name part of this path was
too shaky, and you also think it is, it can be changed to use a pointer
to the host controller since that's also coming from platform or board
definition file directly in this kind of SoC implementation and is
available.
-Andy
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,
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.
thanks,
greg k-h
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.
> 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.
So, we don't need to discuss this any further: thanks again for your time.
-Andy
On Sunday, March 13, 2011, Andy Green wrote:
> On 03/13/2011 04:58 PM, Somebody in the thread at some point said:
>
> > So, you want to have a mechanism telling the driver "if the device
> > happens to have this particular path, use that platform data", right?
>
> Yeah.
>
> > And it works because the initialization code kind of knows what path the
> > device is going to be at, so it can predict that and provide the mathing data.
> >
> > Unfortunately, this relies on how device paths are constructed at the moment,
> > so if this approach is adopted in general, it will prevent us from changing
> > that way in the future (or at least it will make that very difficult).
> >
> > Perhaps you could use some other kind of device identification here?
>
> I'm sorry what prevents you changing paths in the future?
>
> Nothing does, if you change the bus tag from like usb1 to UsB_1 you just
> fix up the strings in the board definition files at the same time, they
> are sitting there in the same tree and
>
> grep platform_async_platform_data arch/* -R
>
> will show them all up in one hit. It's no different if you changed the
> name of a driver, you'd patch the board definition files with devices
> that need to bind to that driver name to uplevel them to the new name.
Well, I'm in the process of doing something similar with sysdevs and, trust me,
it is not fun. :-/
> The board definition file for these SoC cases usually has access to a
> pointer to the host controller directly since it instantiates them, if
> this was the only stumbling point and you thought it helped something
> the matching system can look for that particular pointer being a parent
> of the candidate probed device.
I think that would have been better than matching based on device paths.
Whether or not it's the only stumbling point depends on what other people
think.
Thanks,
Rafael
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.
> >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.
> 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?
confused,
greg k-h
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
On Mon, Mar 14, 2011 at 08:38:53AM +0000, Andy Green wrote:
> 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.
It's not up to the driver to "know" this at all.
> 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.
It really is. How do you know that I don't want to name this device
something else than you feel you should? Do you really want to tell me
to recompile my kernel just to change the name of the network device?
No, it has been determined a long time ago that network naming things
like this are to be done in userspace. It's an argument that has come
and gone many years ago, sorry. See all of the wonderful, and simple,
tools we have today in userspace to handle this type of thing. Distros
can use them how ever they see fit, and even better, users can configure
them! That means they don't have to rebuild their kernels, which is a
bit unreasonable, don't you think?
> 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");
Perhaps we should just always name these things 'eth%d'? Oh wait, as it
really is a USB device, they are supposed to be called 'usb%d' as
determined (again) a long time ago.
If a distro/board manufacturer wants to hide the fact that this really
is a usb device by renaming it to eth0, then again, it can. But don't
force the kernel to have that policy in it.
>
> 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.
Ick, well, I'm not saying that cleaning up this driver is not a bad idea
at all, feel free to submit patches for it that are self-contained.
> 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.
You can set MAC addresses from userspace, why not do that here for
systems in which the hardware designers messed things up so bad it takes
a programmer to fix their brokenness?
> 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.
Are you sure? Try me on one of the other "real" busses out there like
PCI and see how all of these same arguments are identical :)
The abuse of platform_data today does not give you the right to persist
in it. Look at the work of the device tree developers to fix this
problem as proof of this.
> >>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.
Again, because you can solve them in userspace! Why is that not a
correct solution? It should make you even happier as you don't have to
have any kernel patches that are not upstream, you can drop them
entirely and just set up some of your distro's config files and be done
with it.
thanks,
greg k-h
On Mon, 14 Mar 2011, Greg KH wrote:
> No, it has been determined a long time ago that network naming things
> like this are to be done in userspace. It's an argument that has come
> and gone many years ago, sorry. See all of the wonderful, and simple,
> tools we have today in userspace to handle this type of thing. Distros
> can use them how ever they see fit, and even better, users can configure
> them! That means they don't have to rebuild their kernels, which is a
> bit unreasonable, don't you think?
...
> Perhaps we should just always name these things 'eth%d'? Oh wait, as it
> really is a USB device, they are supposed to be called 'usb%d' as
> determined (again) a long time ago.
>
> If a distro/board manufacturer wants to hide the fact that this really
> is a usb device by renaming it to eth0, then again, it can. But don't
> force the kernel to have that policy in it.
This argument does sound contradictory. If network interface naming
should be left entirely up to userspace, then why doesn't the kernel
always generate names of the form "eth%d"? Why not rip all that stuff
about "usb%d" or "wlan%d" out of the driver entirely?
(Apart from the fact that this would be a user-visible change in kernel
policy and would break a large number of systems...)
Alan Stern
On Mon, Mar 14, 2011 at 01:54:32PM -0700, Greg KH wrote:
> On Mon, Mar 14, 2011 at 08:38:53AM +0000, Andy Green wrote:
> > 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.
> Are you sure? Try me on one of the other "real" busses out there like
> PCI and see how all of these same arguments are identical :)
You were asking for concrete examples up thread, and this brings one to
mind - I used to work on a system where we had to modify the PCI ethernet
controller driver to tell it that while it did have two PHYs attached to
the MDIO bus they were nothing to do with the data interface of the
ethernet controller which was instead directly wired to a fixed
configuration device. This sort of thing could be handled by the device
tree but...
> The abuse of platform_data today does not give you the right to persist
> in it. Look at the work of the device tree developers to fix this
> problem as proof of this.
...the problem with device tree is that it's only available on a limited
subset of architectures at the minute, with progress towards deploying
it more widely being somewhat slow.
On Mon, Mar 14, 2011 at 05:03:17PM -0400, Alan Stern wrote:
> On Mon, 14 Mar 2011, Greg KH wrote:
>
> > No, it has been determined a long time ago that network naming things
> > like this are to be done in userspace. It's an argument that has come
> > and gone many years ago, sorry. See all of the wonderful, and simple,
> > tools we have today in userspace to handle this type of thing. Distros
> > can use them how ever they see fit, and even better, users can configure
> > them! That means they don't have to rebuild their kernels, which is a
> > bit unreasonable, don't you think?
>
> ...
>
> > Perhaps we should just always name these things 'eth%d'? Oh wait, as it
> > really is a USB device, they are supposed to be called 'usb%d' as
> > determined (again) a long time ago.
> >
> > If a distro/board manufacturer wants to hide the fact that this really
> > is a usb device by renaming it to eth0, then again, it can. But don't
> > force the kernel to have that policy in it.
>
> This argument does sound contradictory. If network interface naming
> should be left entirely up to userspace, then why doesn't the kernel
> always generate names of the form "eth%d"? Why not rip all that stuff
> about "usb%d" or "wlan%d" out of the driver entirely?
>
> (Apart from the fact that this would be a user-visible change in kernel
> policy and would break a large number of systems...)
I think that is the only reason it is sticking around.
thanks,
greg k-h
On 03/14/2011 08:54 PM, Somebody in the thread at some point said:
>> 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.
>
> It's not up to the driver to "know" this at all.
Right, it's a matter for the board definition file to know about the
board. Not sure how many times I wrote that on this thread, but
evidently not enough. It can then send specific functional
configuration information to the subsystems on the board so they can
adapt automatically to that environment without stinking up everything
with private quirk information per-board in the drivers. That is the
idea of platform_data and it works really successfully.
It's pointless for you to talk about Device Tree when we'll allegedly
have the capability to deliver functional configuration data to USB
devices from board support files, but you claim it's forever evil to use
it, so what's the point of discussing it? I dunno why you're bothering
and it's getting less interesting the more distorted your argumentation
is becoming trying to hold the line against fixing the drivers.
>> 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.
>
> It really is. How do you know that I don't want to name this device
> something else than you feel you should? Do you really want to tell me
> to recompile my kernel just to change the name of the network device?
You know by default if it's going to do it, it ought to try to be
correct if it's possible, it sounds strange how lassaiz-faire you are
about these drivers doing the wrong thing. As Alan pointed out that you
should either do it well or not bother to do it and have all network
devices called xxxN until your "wonderful" and "simple" userland rename
is mandated. Never mind userland will have to hold a board quirk
database and query the driver to find out which is WLAN or whatever:
simplicity.
Nobody said about removing network interface naming, what we're talking
about is making the driver a touch better so it does the right thing
first time, adaptively to its platform.
Hey, did you see the smsc95xx driver is configuring its mac address from
an EEPROM if it's connected instead of letting userland do it, holy
crap. I bet those guys who get their MAC address automagically are
super upset the kernel is cheating userland of some simple wonderfulness.
Ha ha no, just kidding.
Thanks to the other guys for giving their opinions frankly -- I am
particularly appreciate Mark got the idea immediately -- and I hope
everyone has a nice evening.
-Andy