Changeset View
Standalone View
sys/kern/device_if.m
Context not available. | |||||
# Default implementations of some methods. | # Default implementations of some methods. | ||||
# | # | ||||
CODE { | CODE { | ||||
static bool null_delay_attach(device_t dev) | |||||
imp: int returns FALSE?
This is a bad interface. It's supposed to return an ERRNO to detect when… | |||||
Not Done Inline ActionsThe ideal interface would be to trigger this when DEVICE_ATTACH() returns EAGAIN. However, device_attach() does enough setup that that is non-trivial and I don't see a way around some function like this. In general, living in this world, I don't care about the interface. For this suggestion in particular, since DEVICE_ATTACH() should keep all the error detection/reporting logic, I'm a bit hesitant to give this shim function more options than a boolean and do not see the advantage of 0/EAGAIN vs. TRUE/FALSE (the updated patch changes it to a bool). Is there one? nwhitehorn: The ideal interface would be to trigger this when DEVICE_ATTACH() returns EAGAIN. However… | |||||
Not Done Inline ActionsI'm a bit torn on this one. The kobj interface assumes that kobj methods always return an int (errno) value, and the default global fallback if a method isn't found is to fail with ENXIO. (kobj_error_method) That said, that would basically require treating ENXIO as 'no method installed so go ahead and call DEVICE_ATTACH' which is somewhat odd. Also, a default implementation really should ensure that kobj_error_method is never called (you have to really try hard to have a kernel that would have the 'cookie' symbol used to identify the method but not have the default method since both things live in the same auto-generated device_if.c). I think the fact that in this case any real error handling can (and should) be done in DEVICE_ATTACH probably does make it ok to just do a bool (bool uses 'true/false' and not 'TRUE/FALSE' btw). jhb: I'm a bit torn on this one.
The kobj interface assumes that kobj methods always return an int… | |||||
Not Done Inline ActionsI don't have any strong opinions here so long as any actual error reporting is deferred to DEVICE_ATTACH(). nwhitehorn: I don't have any strong opinions here so long as any actual error reporting is deferred to… | |||||
{ | |||||
return FALSE; | |||||
} | |||||
static int null_shutdown(device_t dev) | static int null_shutdown(device_t dev) | ||||
{ | { | ||||
return 0; | return 0; | ||||
Context not available. | |||||
}; | }; | ||||
/** | /** | ||||
* @brief See if a device's dependencies are present. | |||||
* If not, delay attach for the future but continue claiming the device. | |||||
* To include this method in a device driver, use a line like this | |||||
* in the driver's method list: | |||||
* | |||||
* @code | |||||
* KOBJMETHOD(device_delay_attach, foo_delay_attach) | |||||
Not Done Inline Actionswe usually don't have this doc for each function. I'd omit it. imp: we usually don't have this doc for each function. I'd omit it. | |||||
Not Done Inline ActionsEvery other function in this file has it. It seemed weird, but I wanted to match existing style. nwhitehorn: Every other function in this file has it. It seemed weird, but I wanted to match existing style. | |||||
Not Done Inline ActionsIt is also wrong, but it does seem consistently wrong in the file. A separate commit to fix these to use DEVMETHOD instead would be good. jhb: It is also wrong, but it does seem consistently wrong in the file. A separate commit to fix… | |||||
Not Done Inline ActionsOK, I'd like to do this after this commit though. nwhitehorn: OK, I'd like to do this after this commit though. | |||||
* @endcode | |||||
* | |||||
* @param dev the device to probe | |||||
* | |||||
* @retval TRUE postpone attachment | |||||
* @retval FALSE attach now | |||||
* @see DEVICE_ATTACH() | |||||
*/ | |||||
METHOD bool delay_attach { | |||||
device_t dev; | |||||
} DEFAULT null_delay_attach; | |||||
/** | |||||
* @brief Attach a device to a device driver | * @brief Attach a device to a device driver | ||||
* | * | ||||
* Normally only called via device_probe_and_attach(), this is called | * Normally only called via device_probe_and_attach(), this is called | ||||
Context not available. |
int returns FALSE?
This is a bad interface. It's supposed to return an ERRNO to detect when it's not implemented.
Instead, have it return 0 for no delay or EAGAIN for a delay.