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

[John Nemeth <jnemeth%cue.bc.ca@localhost>]

On Jun 23,  7:40pm, Masanobu SAITOH wrote:
} 
}   As you know, the return value of device driver's attach function is void.
} I've thought that we should change it to int for many years. I believe I'm
} not the only person.

     I've been meaning to get back to this one for some time...

}   xxx_attach() may fail the following cases:
} 
} 	got unexpected behavior.
} 
} 	resource allocation error.
} 
} 	driver is really broken.
} 
} 	some others.

     I believe the interesting cases are:  device is broken, resource
allocation failure, can't find firmware, etc. (i.e. no chance of
attaching device); driver decides that it can't handle device even
though it matched (i.e. try another driver); and success.

}   xxx_attach() is void, so the caller can't know the fail. It makes some
} problems:
} 
} 	a) The OS may touch the broken device while the OS is running.
} 	   It may causes a panic.
} 
} 	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.
} 
} 	c) To prevent b), we have added extra code into xxx_shutdown().
} 	   It wastes our time and the code become big and complex.
} 
} 	d) Resource leak. For example, A device_t structure is allocated
} 	   by caller side.
} 
}   If we change the return value to int or any other type's value, we can
} do the following things.
} 
} 	a) Don't register failed device to avoid problem.
} 
} 	b) If we don't do a), we can add code to not to call xxx_shutdown()
} 	   for broken devices instead.
} 
} 	c) We can add the code to fallback to lower priority device driver
} 	  like ukphy(4), ugen(4) or others.
} 
} 	d) (add some other good features?)
} 
}   What do you think about this change? If you're OK, I'll change _ALL_ device
} drivers' attach function first by the following way:
} 
} 	0) Change return value to int.
} 
} 	1) Change "return;" to "return 0;" or "return -1;"
} 	   (or bool value or Exxx. See below)
} 
} 	2) I won't modify the caller side for the time being.

        3) Add "return 0;" to end of match routine to catch any
           that just "fall off" the end of the function (perfectly
           legal for void functions).

} In this way, it won't break anything because the caller doesn't check the
} value. Even if I mistakenly modified the return value and we add code to
} check the return value, it won't be a big problem because almost all drivers
} don't go into the failure paths. So, I think it's not required to use new
} CFATTACH_XXXX() because it makes both the driver and the caller side be
} complex.
} 
}   Now I'm wondering if I should return "-1" or Exxx. FreeBSD returns Exxx
} (e.g. ENOMEN, EIO, ENXIO, etc.). I don't know which one is better.
} 
} 	Changing to -1 is easier than Exxx because it's not required
} 	to wonder what Exxx I should choose.
} 
} 	Changing to Exxx may be good if we check the error code and
} 	do something depend on the value though I can't imagine anything now.
} 
} 	-1 and Exxx can be mixed because both are int. We can detect
} 	error with "if (rv != 0)" even if it's mixed.

     I would just have a sequence of numeric values.  I don't think
we need formal Exxx values.  I would recommend 0 for success, 1
for impossible, 2 for try another driver, etc.  There is only one
caller, autoconf, so it isn't like we have to prepare for a bunch
of random callers.

}   Any objection, advice or idea?

     I like the idea.  On the first pass, you could simply have
all drivers return "success" unconditionally (this would maintain
status quo).  Then, figure out what autoconf should do with the
various errors, then modify drivers one at a time.

     A number of people have expressed reservation (bring up memories
of device_t and how long that took to settle out) indicating that
this should be done on a branch or something.  Personally, I don't
see the need to do so.  The issue with the device_t change was that
it involved actual code changes.  This does not, it is simply search
and replace, which is a much less dangerous thing to do.  Some have
suggested doing other changes at the same time.  That would certainly
increase the risk.

}-- End of excerpt from Masanobu SAITOH