[email protected] (Masanobu SAITOH) writes: > 2) We can show a common error message when ca_attach() failed. > This change has some benefits.
We don't know why ca_attach failed. For that the driver has to emit another message. If you want to have common error message, you need to relay the error information to the framework. > 3) This change make possible not to call ca_detach() for > devices that the ca_attach() failed. In this patch, I don't think that this is a benefit. The driver has to keep global state for the success case anyway. This just replicates this into the framework (and only with a single bit, which IMHO is not enough). It also forbids the drivers to "partially attach", otherwise the "attach failed" bit isn't really correct. While a "partial" attachment can become a problem, the change requires that all drivers back out from such a condition (or you get resource leaks). >> b) The shutdown sequence calls xxx_shutdown() even if >> it's not really attached. It may causes panic. We have met >> this bugs many times. So you don't shut down drivers that fail to attach completely. The real benefit here comes from changing the drivers to back out from a partial attachment or from safely detaching. But this has nothing to do with the proposed API change. > Usually, author of a device driver doesn't expect to call ca_deach() >when ca_attach() failed. >They (include me) don't think ca_attach() >really fails. Our experience showed that it occurs. Some examples >are in bge_release_resources(it's called from both bge_attach() and >bge_detach()) and wm_detach(): ca_attach() doesn't fail and ca_detach() is always called. That's how it is defined currently. There is no distinction between a 'succesful attach' and a 'failed attach'. Of course many many drivers don't have a detach routine at all because hardware usually doesn't go away and was supposed to be quiescent when the last user closed the driver. That's a relict from the time where hardware was mostly static, but it is still present. > https://nxr.netbsd.org/xref/src/sys/dev/pci/if_bge.c#4132 > https://nxr.netbsd.org/xref/src/sys/dev/pci/if_wm.c#2595 >Some detach function expects that a pointer is initialized. On some >cases the pointer is not initialized and it occurred NULL pointer >reference and panicked. bge(4) and wm(4) are not only the case of >this problem. The very point is that no API change will fix that. Not calling detach isn't better than calling detach after a failed attachment and all gain is in making drivers fail (and back out) gracefully. -- -- Michael van Elst Internet: [email protected] "A potential Snark may lurk in every tree."
