Create initial review for the automatic PNP work.
ClosedPublic
Actions

Authored by imp on Aug 22 2015, 3:03 AM.

Details

Reviewers

jhb
• hselasky

Group Reviewers

transport

Commits

rS287775: Update TSO limits to include all headers.

Summary

changes all together for first round of reviews, will be committed in smaller chunks

Create the MDT_PNP_INFO metadata record to communicate PNP info about
modules. External agents may use this data to automatically load those
modules.

Create a generic PCCARD_PNP_INFO from the MODULE_PNP_INFO building
block. Use it in all the PNP drivers to export either the current PNP
table. For uart, create a custom table and export it using
MODULE_PNP_INFO since it's the only one that matches on function
number.

Create a USB_PNP_INFO and use it to export the existing PNP
tables. Some drivers needed some slight re-arrangement of declarations
to accommodate this. Change the USB pnp tables slightly to allow
better compatibility with the system by moving linux driver info from
start of each entry to the end. All other PNP tables in the system
have the per-device flags and such at the end of the elements rather
that at the beginning.

Augment kldxref to find the new MODULE_PNP_INFO records now in
modules, simplify them into a more normal form and write them to
linker.hints.

Add PNP info for ISA and PCI to the ed driver to prove design.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 195
Build 195: arc lint + arc unit

Event Timeline

imp updated this revision to Diff 8121.Aug 22 2015, 3:03 AM

imp retitled this revision from to Create initial review for the automatic PNP work..

imp updated this object.

imp edited the test plan for this revision. (Show Details)

imp updated this object.Aug 22 2015, 3:07 AM

imp added a reviewer: jhb.

imp added a reviewer: • hselasky.Aug 22 2015, 3:34 AM

Review D3461 has the 'base' of this change: the kldxref and modules.h stuff.

Traditionally USB ID's are split in HOST and DEVICE ID's. So that means all structs marked with:

STRUCT_USB_HOST_ID should be in their own table. I suggest:

USB_HOST_IDS()
USB_DEVICE_IDS()
USB_DUAL_IDS()

Instead of only:

USB_PNP_IDS()

--HPS

In D3458#70629, @hselasky wrote:

Traditionally USB ID's are split in HOST and DEVICE ID's. So that means all structs marked with:

STRUCT_USB_HOST_ID should be in their own table. I suggest:

USB_HOST_IDS()
USB_DEVICE_IDS()
USB_DUAL_IDS()

Instead of only:

USB_PNP_IDS()

What benefit would there be calling these out separately? Do they need to be marked differently somehow so that external agent can do different things with them? Or is it just eye candy?

If you want to use the exported data for loading modules there is one piece missing, which is not encoded in the ID tables, and that is the USB mode the device ID's apply to. Usually drivers are only loaded for host mode, but if you have a USB target mode device, USB drivers should be automatically loaded too. Currently I use three different sections of device ID's to solve that.

What is the purpose of USB_PNP_IDS() ?

In D3458#70631, @hselasky wrote:

If you want to use the exported data for loading modules there is one piece missing, which is not encoded in the ID tables, and that is the USB mode the device ID's apply to. Usually drivers are only loaded for host mode, but if you have a USB target mode device, USB drivers should be automatically loaded too. Currently I use three different sections of device ID's to solve that.

So we do need to tag things differently for the different flavors somehow. This was not anticipated in the design, but I think it can be accommodated.

What is the purpose of USB_PNP_IDS() ?

Short answer: export enough information to replace the boatload of generated devd.conf scripts that you are currently generating. But to do it dynamically at run time, maybe even from the boot loader. Or the kernel even...

Longer answer: they provide a generic way of expressing pnp tables that scales to the entire system where pnp tables aren't as uniform as the USB system.

I'm curious, in target mode, how do IDs matter? I thought the target driver said what the ID of the emulated device was? Or am I missing something again?

I'm curious, in target mode, how do IDs matter? I thought the target driver said what the ID of the emulated device was? Or am I missing something again?

For example we have umass.ko for host side and ustorage_fs.ko for device side. Both attach to the same ID's but the drivers work completely opposite. The current USB auto load rules will generate output for matching the "mode" parameter based on which section it finds the ID's in.

nomatch 32 {

match "bus" "uhub[0-9]+";
match "mode" "host";
match "vendor" "0x03e8";
match "product" "0x0008";
action "kldload -n if_kue";

};

nomatch 32 {

match "bus" "uhub[0-9]+";
match "mode" "(host|device)";
match "intclass" "0x02";
match "intsubclass" "0x06";
action "kldload -n if_cdce";

};

If this can be preserved somehow, would be great.

It might also be possible I can extend the matching containing a usb_mode bit, so that the PNP info contains this information too.

What do you think?

--HPS

In D3458#70633, @hselasky wrote:

It might also be possible I can extend the matching containing a usb_mode bit, so that the PNP info contains this information too.

What do you think?

I could extend my language with a K clause. Kmode=host;Kmode=device; could encode the dual. I don't think it would be a big deal. If the tables are already separate, that's likely good enough and would be a O(1) way to encode it.

I'm still not understanding where the IDs come from for device mode. I don't need to know for this work, but it would plug a hole in my understanding.

Update the USB stuff after confersation with Hans...

Create the MDT_PNP_INFO metadata record to communicate PNP info about
Augment kldxref to find the new MODULE_PNP_INFO records now in
Create a generic PCCARD_PNP_INFO from the MODULE_PNP_INFO building
Create a USB_PNP_INFO and use it to export the existing PNP
Add PNP info for ISA and PCI to the ed driver to prove design.

Hi,

Right now there aren't so many ID's for the device side. I think I'll encode this into the USB device ID structure. Where 0, USB host mode, is the default. Your approach would then be fine.

static const STRUCT_USB_DUAL_ID cdce_dual_devs[] = {

{USB_IF_CSI(UICLASS_CDC, UISUBCLASS_ETHERNET_NETWORKING_CONTROL_MODEL, 0)},
{USB_IF_CSI(UICLASS_CDC, UISUBCLASS_MOBILE_DIRECT_LINE_MODEL, 0)},
{USB_IF_CSI(UICLASS_CDC, UISUBCLASS_NETWORK_CONTROL_MODEL, 0)},

};

In D3458#70702, @hselasky wrote:
Hi,

Right now there aren't so many ID's for the device side. I think I'll encode this into the USB device ID structure. Where 0, USB host mode, is the default. Your approach would then be fine.

static const STRUCT_USB_DUAL_ID cdce_dual_devs[] = {
{USB_IF_CSI(UICLASS_CDC, UISUBCLASS_ETHERNET_NETWORKING_CONTROL_MODEL, 0)},
{USB_IF_CSI(UICLASS_CDC, UISUBCLASS_MOBILE_DIRECT_LINE_MODEL, 0)},
{USB_IF_CSI(UICLASS_CDC, UISUBCLASS_NETWORK_CONTROL_MODEL, 0)},
};

That's not needed at all. And in fact, makes it harder since I would have to grow a special case to interpret this structure (which I really don't want to do). The updated diff shows that it's trivial to do at the PNP level.

One question: Does the new PNP framework support loading multiple drivers for the same match? Or is only the first matching driver loaded?

At the moment, we export information. This information may indicate that multiple drivers are loaded based on a set of facts we get back from the bus' nomatch event. likely will happen is that this causes multiple drivers to be loaded. It is a quite hard problem, given the current state of newbus, to only load the driver that will eventually claim the device. It is only less hard to load multiple drivers and then unload the ones that didn't match.

So it's safe to assume that if multiple drivers match a nomatch event, based on the exported PNP tables, multiple drivers will be loaded.

Looks good!

sys/dev/usb/usbdi.h
245	If you want you can expand STRUCT_USB_HOST_ID, STRUCT_USB_DEVICE_ID and STRUCT_USB_DUAL_ID all into "struct usb_device_id", because they will no longer be needed and remove the whole #if USB_HAVE_ID_SECTION for -current. Or just leave the latter section after #else for legacy support. Also you might want to cleanup the "tools/tools/bus_autoconf"