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.

In D34031#776851, @mmel wrote:

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.

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.

In D34031#776866, @mindal_semihalf.com wrote:

That's exactly how I initially did this, take a look at https://reviews.freebsd.org/D33457?id=100979#inline-208849.

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?

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.

In D34031#777672, @mindal_semihalf.com wrote:

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.

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.

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.

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.

In D34031#777724, @mindal_semihalf.com wrote:

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.
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.

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...