There are multiple buses that pretend to be ofw compatible, e.g ofw_pci, mii_fdt.
For each of them we now need to provide an implementation of BUS_GET_PROPERTY.
Instead of modifying each of them it's better to just set up a default one that simply traverses up the device tree.
Remove the now unneeded implementation in mii_fdt.
Details
Details
Diff Detail
Diff Detail
- Repository
- rG FreeBSD src repository
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| sys/kern/bus_if.m | ||
|---|---|---|
| 79–80 | This should be in subr_bus.c and be named bus_generic_get_property to be consistant with the other non-null default implementations. | |
Comment Actions
I don't think that's right. We can't expect that all indirect descendants of simplebus were instantiated by simplebus. Typically all enumerable buses (e.g. pci or multifunction devices represented by single FDT node) are not derived from simplebus. You cannot use ofw_bus_get_node(child) on those.
IMHO we can have a single generic implementation of <foo>_get_property for a given bus , but it should be explicitly defined in the device_methods structure for all appropriate drives.
Comment Actions
That's exactly how I initially did this, take a look at https://reviews.freebsd.org/D33457?id=100979#inline-208849.
I don't really see a problem with using a default implementation though.
If the ofw_bus_get_node(child) is not implemented it will just return -1, causing the simplebus logic to fail.
If a bus has some sort of implementation, that doesn't support using the "child" as argument I'd expect it to fail in the same way as well.
The problem with defining things explicitly is that this will introduce yet another thing that has to be done when a new ofw-like bus is introduced.
Comment Actions
I see and I think that original implementation was more correct.
I don't really see a problem with using a default implementation though.
If the ofw_bus_get_node(child) is not implemented it will just return -1, causing the simplebus logic to fail.
If a bus has some sort of implementation, that doesn't support using the "child" as argument I'd expect it to fail in the same way as well.
This only applies to buses derived from simple/ofwbus, not to others (e.g. pci). For other device objects, the ofw_bus_get_node() function returns ENXIO (which may be a valid node ID). However, calling a class function on an object that is not derived from the given class should be considered an error.
The problem with defining things explicitly is that this will introduce yet another thing that has to be done when a new ofw-like bus is introduced.
Will adding this implementation as default to ofwbus solve this problem?
Comment Actions
This only applies to buses derived from simple/ofwbus, not to others (e.g. pci). For other device objects, the ofw_bus_get_node() function returns ENXIO (which may be a valid node ID). However, calling a class function on an object that is not derived from the given class should be considered an error.
Do you mean the case when kobj_lookup_method fails to find anything and we end up in kobj_error_method?
I don't think this is a possible scenario.
The default implementation - ofw_bus_default_get_node returns -1.
Note that BUS_GET_PROPERTY being resolved to simplebus_get_property implies that the bus was derived from ofw.
Because of that the OFW_BUS_GET_NODE will always have an implementation, possibly the default one.
The problem with defining things explicitly is that this will introduce yet another thing that has to be done when a new ofw-like bus is introduced.
Will adding this implementation as default to ofwbus solve this problem?
Unfortunately it won't. It has to be present on all buses with devices that will use the device_get_property API.
Comment Actions
Yes, I mean we end up in kobj_error_method. And that's a very real scenario. In newbus, the default method is only applied for the given class and its subclasses. *Not for all classes*
This means that all controllers (not derived from ofwbus), pci or other buses not derived from ofw/simplebus are subject to this error.
The problem with defining things explicitly is that this will introduce yet another thing that has to be done when a new ofw-like bus is introduced.
Will adding this implementation as default to ofwbus solve this problem?
Unfortunately it won't. It has to be present on all buses with devices that will use the device_get_property API.
Comment Actions
Yes, I mean we end up in kobj_error_method. And that's a very real scenario. In newbus, the default method is only applied for the given class and its subclasses. *Not for all classes*
This means that all controllers (not derived from ofwbus), pci or other buses not derived from ofw/simplebus are subject to this error.
Ok, but my point is that if simplebus_get_property gets called shouldn't that mean that the driver of this bus is a subclass of ofw?
In that case we should also get ofw_bus_default_get_node on the same bus.
Comment Actions
Hmm, I just did a simple test by adding a call to kdb_backtrace to ofw_bus_default_get_node and removing ofw_pci.c from the build.
The device in backtrace is attached to a standard pci bus that doesn't know anything about ofw:
pcib8: <Generic PCI host controller> mem 0x1f0000000-0x1f00fffff on simplebus0 (...) pci8: <PCI bus> on pcib8 (...) enetc0: <Freescale ENETC PCIe Gigabit Ethernet Controller> mem 0x1f8000000-0x1f803ffff,0x1f8160000-0x1f816ffff at device 0.0 on pci8
The backtrace dump is triggered by a call to OFW_BUS_GET_NODE(pci8, enetc0):
KDB: stack backtrace: db_trace_self() at db_trace_self db_trace_self_wrapper() at db_trace_self_wrapper+0x30 ofw_bus_default_get_node() at ofw_bus_default_get_node+0x8 simplebus_get_property() at simplebus_get_property+0x74 enetc_setup_phy() at enetc_setup_phy+0x30 enetc_attach_pre() at enetc_attach_pre+0x208 iflib_device_register() at iflib_device_register+0x144 iflib_device_attach() at iflib_device_attach+0xd0 device_attach() at device_attach+0x3f8 device_probe_and_attach() at device_probe_and_attach+0x7c bus_generic_attach() at bus_generic_attach+0x18 pci_attach() at pci_attach+0xe8 device_attach() at device_attach+0x3f8 device_probe_and_attach() at device_probe_and_attach+0x7c bus_generic_attach() at bus_generic_attach+0x18 device_attach() at device_attach+0x3f8 device_probe_and_attach() at device_probe_and_attach+0x7c bus_generic_new_pass() at bus_generic_new_pass+0xfc bus_generic_new_pass() at bus_generic_new_pass+0xac bus_generic_new_pass() at bus_generic_new_pass+0xac bus_generic_new_pass() at bus_generic_new_pass+0xac bus_set_pass() at bus_set_pass+0x4c mi_startup() at mi_startup+0x12c virtdone() at virtdone+0x7c
This means that all controllers (not derived from ofwbus), pci or other buses not derived from ofw/simplebus are subject to this error.
Is this the scenario where you expected ENXIO to be returned from ofw_bus_get_node?
I'm not sure if I understand you correctly here.
On a DT systems I'd expect all controllers to have a ofw/simplebus as an ancestor.
Comment Actions
Oups, my bad. I was 100% convinced that I had tested it earlier. Again, you're right and sorry for noise
On a DT systems I'd expect all controllers to have a ofw/simplebus as an ancestor.
It depends on which ancestor you mean.
If it is an ancestor in terms of the device tree hierarchy, then yes, we must have ofw/simplebus compatible parent/accessor.
If it is an ancestor in terms of device driver classes, then no, we have controllers/bus that are not a subclasses of ofw/simplebus.
For example, all PCI buses that are not directly connected to a PCI controller (behind a switch), and all connected devices,
MMC buses and all connected cards or so...