Date:   Thu, 13 Apr 2017 19:57:19 -0700
From:   Guenter Roeck <linux@...ck-us.net>
To:     Mats Karrman <mats.dev.list@...il.com>
Cc:     Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
        Greg KH <gregkh@...uxfoundation.org>,
        Felipe Balbi <felipe.balbi@...ux.intel.com>,
        linux-kernel@...r.kernel.org, linux-usb@...r.kernel.org
Subject: Re: USB Type-C Port Manager API concern

On 04/09/2017 02:05 PM, Mats Karrman wrote:
> 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...
>
Problem is that I can't even defer the call to tcpm_init(), or for that matter any
other call into the low level driver code, into the worker, since there is still
no guarantee that the low level driver is "done" with its initialization. The only
real solution I can think of is that the low level driver should not use the pointer
to tcpm_port in any of its callback functions.

Overall I think there is an assumption in any API that any callback functions provided
in a registration call can immediately be called. Otherwise any API would be in trouble.
Can you modify your code to not require the port pointer in its callback functions ?

Thanks,
Guenter

Powered by blists - more mailing lists