Re: USB Type-C Port Manager API concern
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
Re: USB Type-C Port Manager API concern
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
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
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
Re: USB Type-C Port Manager API concern
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
Re: USB Type-C Port Manager API concern
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
Re: USB Type-C Port Manager API concern
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
Re: USB Type-C Port Manager API concern
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