Re: Changing the return value of xxx_attach() from void to int. (take 2)

[mlelstv%serpens.de@localhost (Michael van Elst)]

msaitoh%execsw.org@localhost (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: mlelstv%serpens.de@localhost
                                "A potential Snark may lurk in every tree."