On 10/1/20 9:38 AM, Greg KH wrote:
On Thu, Oct 01, 2020 at 11:33:34AM -0300, Jason Gunthorpe wrote:
On Thu, Oct 01, 2020 at 02:14:23PM +0200, Greg KH wrote:
On Thu, Oct 01, 2020 at 08:58:47AM -0300, Jason Gunthorpe wrote:
On Thu, Oct 01, 2020 at 01:05:51PM +0200, Greg KH wrote:
You have to be _VERY_ careful after calling
ancillary_device_initialize(), as now you can not just free up the
memory if something goes wrong before ancillary_device_add() is called,
right?
I've looked at way too many versions of this patch and related. This
is the only one so far that I didn't find various bugs on the error
cases.
But you haven't seen the callers of this function. Without this
documented, you will have problems.
I've seen the Intel irdma, both versions of the SOF stuff and an
internal mlx5 patch..
Look at the SOF example, it has perfectly paired error unwinds. Each
function has unwind that cleans up exactly what it creates. Every
'free' unwind is paired with an 'alloc' in the same function. Simple.
Easy to audit. Easy to correctly enhance down the road.
This is the common kernel goto error design pattern.
But that's where people get this wrong. Once device_initialize() is
called, the "free" can not be called, something else must be,
device_put().
Tricky, yes. Messy, yes. Sorry.
Why is this two-step process even needed here?
Initializing the refcount close to the allocation is a common design
pattern as is initializing it close to registration. Both options are
tricky, both have various common subtle bugs, both have awkward
elements.
At the end of the day, after something like 20 iterations, this is the
first series that actually doesn't have error unwind bugs.
Can we empower Dave to make this choice? It is not like it is wild or
weird, the driver core already exposes _initialize and _add functions
that work in exactly the same way.
Sure, but without a real user that _NEEDS_ this two-step process, let's
not include it. Why bake complexity into the system from the start that
is never used?
Iteration and evolution is fine, it's not like this is going to be
set-in-stone for forever.
We initially had a single ancillary_device_register(). At some point,
there was an ask to simplify the error handling by moving some of it to
the caller, and an ask to let the IDAs be managed at the parent level to
avoid possible discontinuities in the numbers allocated.
Both changes made it hard to deal with errors flow on the caller side.
As you describe it above, we had to either free memory if the error
happened before device_initialize() was called (e.g. missing .release
callback, etc), or use put_device() afterwards.
Splitting the two appeared to be the only way to make sure the resources
are released in the right way, with a single function we had several
cases where the caller couldn't figure out whether to free memory or
call put_device().
