Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1619396 > unrolled thread

USB Type-C Port Manager API concern

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  USB Type-C Port Manager API concern Mats Karrman <mats.dev.list@gmail.com> - 2017-04-09 01:20 +0200
    Re: USB Type-C Port Manager API concern Guenter Roeck <linux@roeck-us.net> - 2017-04-09 17:20 +0200
      Re: USB Type-C Port Manager API concern Mats Karrman <mats.dev.list@gmail.com> - 2017-04-09 23:10 +0200
        Re: USB Type-C Port Manager API concern Guenter Roeck <linux@roeck-us.net> - 2017-04-14 05:00 +0200
          Re: USB Type-C Port Manager API concern Mats Karrman <mats.dev.list@gmail.com> - 2017-04-14 10:40 +0200

#1619396 — USB Type-C Port Manager API concern

Hi Guenter,

 From a previous thread:

On 03/08/2017 02:38 AM, Guenter Roeck wrote:
> On 03/07/2017 02:30 PM, Mats Karrman wrote:
> [ ... ]
>
>>
>> I'm still struggling to catch up on what you guys have been up to 
>> during the
>> last year or so :-) and came across some patches of Guenter from last 
>> October:
>>
>> http://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1243527.html
>>
>> What happened to them? Has there been any progress since then?
>>
>
> Updates to keep in sync with API changes, bug fixes, and minor 
> improvements,
> for the most part. I can post a current version if there is interest.
> The latest version is also available from
> https://chromium-review.googlesource.com/#/c/389917/

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?

BR,
Mats

[toc] | [next] | [standalone]

#1619509

Hi Mats,

On Sun, Apr 09, 2017 at 01:09:57AM +0200, Mats Karrman wrote:
> Hi Guenter,
> 
> From a previous thread:
> 
> On 03/08/2017 02:38 AM, Guenter Roeck wrote:
> >On 03/07/2017 02:30 PM, Mats Karrman wrote:
> >[ ... ]
> >
> >>
> >>I'm still struggling to catch up on what you guys have been up to during
> >>the
> >>last year or so :-) and came across some patches of Guenter from last
> >>October:
> >>
> >>http://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1243527.html
> >>
> >>What happened to them? Has there been any progress since then?
> >>
> >
> >Updates to keep in sync with API changes, bug fixes, and minor
> >improvements,
> >for the most part. I can post a current version if there is interest.
> >The latest version is also available from
> >https://chromium-review.googlesource.com/#/c/389917/
> 
> 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 ?

Thanks,
Guenter

[toc] | [prev] | [next] | [standalone]

#1619551

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

[toc] | [prev] | [next] | [standalone]

#1623486

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

[toc] | [prev] | [next] | [standalone]

#1623565

On 04/14/2017 04:57 AM, Guenter Roeck wrote:

> ... 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 ?

Yes, I have deferred my calls until after the registration function has 
returned which seems only logical. Thanks, Mats

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web