Re: USB Type-C Port Manager API concern

[Mats Karrman <mats.dev.list@gmail.com>]

On 04/09/2017 05:16 PM, Guenter Roeck wrote:

> Hi Mats,
>
> On Sun, Apr 09, 2017 at 01:09:57AM +0200, Mats Karrman wrote:
>> I'm working on a tcpi driver and have some concern about the tcpm api.
>> The tcpm_register_port() is typically called from the probe function of
>> tcpi driver where the tcpm_port reference returned is stored in the
>> driver private data. The problem I ran into is that tcpm_register_port()
>> calls back to the not yet fully initialized tcpi driver, causing null-
>> pointer dereferences. This could of course be solved by extra logic in
>> the tcpi driver but I think it would be more elegant if the registration
>> of a port could be free of premature callbacks. E.g. it could be required
>> that the tcpi driver probe called tcpm_tcpc_reset() once it's done
>> initializing or the necessary data could be supplied in the call to
>> tcpm_register_port().
>> What do you think?
> Let me think about it. In theory it should be possible to avoid callbacks into
> the underlying driver until after the return from the registration code, but
> that would still be racy if the underlying driver is not ready.
>
> Basic problem seems to be that an API in general assumes that the caller is
> ready to serve it once it registers itself. It is kind of unusual to have two
> calls, one to register the driver and one to tell the infrastructure that it
> is ready (which I assume your reset call would do). Ultimately this means
> that the tcpm driver would have to have additional logic to identify if the
> underlying driver is ready to handle callbacks.
>
> Can you delay tcpm registration until after the underlying driver is ready,
> ie typically to the end of its probe function ? Or am I misunderstanding
> your problem ?

The problem I had was that I was trying to pull the same trick that you do ;)
I.e. the probe function calls tcpm_register_port() that is calling back
to the driver that was trying to call back to tcpm, just that the call to
tcpm_register_port() has not yet returned so the pointer to tcpm_port in the
driver data structure was still null.

I was able to fix the issue by commenting out the call to tcpm_init() at the
end of tcpm_register_port() and instead call ("your") tcpm_tcpc_reset(), that
currently does nothing but calling tcpm_init(), after I'm done.

Even though I'm not overly excited about the tcpm register function calling back
to the driver I don't think my fix is much better. I should live by my own words
and refrain from calling back to tcpm until registration has finished...

// Mats