Subject: Re: USB Type-C Port Manager API concern
To: Guenter Roeck <linux@roeck-us.net>
References: <20170221142405.76299-3-heikki.krogerus@linux.intel.com>
 <ae5d8f3a-859b-c3fa-1427-c25920a82abe@gmail.com>
 <4b4bbffc-db02-3b54-04bc-e7de79b2d9ed@roeck-us.net>
 <07618170-d561-e7fe-08e0-91316c53d832@gmail.com>
 <20170303125940.GA6999@kuha.fi.intel.com>
 <6ddb2eac-03d5-127e-df1e-ad189968e6b2@gmail.com>
 <20170306131442.GC6999@kuha.fi.intel.com>
 <696552a7-c36a-1d73-9517-543907e9da39@gmail.com>
 <9b32fabf-3ff5-112f-4249-a7024f808b20@roeck-us.net>
 <4fe455d8-30d9-0c21-46aa-6d273cd24d50@gmail.com>
 <20170409151614.GA23984@roeck-us.net>
Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com>,
        Greg KH <gregkh@linuxfoundation.org>,
        Felipe Balbi <felipe.balbi@linux.intel.com>,
        linux-kernel@vger.kernel.org, linux-usb@vger.kernel.org
From: Mats Karrman <mats.dev.list@gmail.com>
Message-ID: <949bc063-7002-c12c-ba56-8b46f9ea178b@gmail.com>
Date: Sun, 9 Apr 2017 23:05:28 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101
 Thunderbird/45.8.0
MIME-Version: 1.0
In-Reply-To: <20170409151614.GA23984@roeck-us.net>
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2554
Lines: 47

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