| Message ID | 20190131035411.53576-1-kyletso@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | usb: typec: tcpm: Export partner Source Capabilities | expand |
On Thu, Jan 31, 2019 at 11:54:11AM +0800, Kyle Tso wrote: > Provide a function to get the partner Source Capabilities. > > Signed-off-by: Kyle Tso <kyletso@google.com> > --- > drivers/usb/typec/tcpm/tcpm.c | 23 +++++++++++++++++++++++ > include/linux/usb/tcpm.h | 1 + > 2 files changed, 24 insertions(+) > > diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c > index f1d3e54210df..29cd84ba9960 100644 > --- a/drivers/usb/typec/tcpm/tcpm.c > +++ b/drivers/usb/typec/tcpm/tcpm.c > @@ -4494,6 +4494,29 @@ int tcpm_update_sink_capabilities(struct tcpm_port *port, const u32 *pdo, > } > EXPORT_SYMBOL_GPL(tcpm_update_sink_capabilities); > > +/* > + * Don't call this function in interrupt context. Caller needs to free the > + * memory itself. > + */ > +int tcpm_get_partner_src_caps(struct tcpm_port *port, u32 **src_pdo) > +{ > + unsigned int nr_pdo; > + > + if (port->nr_source_caps == 0) > + return -ENODATA; > + > + *src_pdo = kcalloc(port->nr_source_caps, sizeof(u32), GFP_KERNEL); > + if (!src_pdo) > + return -ENOMEM; > + > + mutex_lock(&port->lock); > + nr_pdo = tcpm_copy_pdos(*src_pdo, port->source_caps, > + port->nr_source_caps); > + mutex_unlock(&port->lock); > + return nr_pdo; > +} > +EXPORT_SYMBOL_GPL(tcpm_get_partner_src_caps); We don't add new functions that no one uses :(
On Thu, Jan 31, 2019 at 08:02:38AM +0100, Greg KH wrote: > On Thu, Jan 31, 2019 at 11:54:11AM +0800, Kyle Tso wrote: > > Provide a function to get the partner Source Capabilities. > > > > Signed-off-by: Kyle Tso <kyletso@google.com> > > --- > > drivers/usb/typec/tcpm/tcpm.c | 23 +++++++++++++++++++++++ > > include/linux/usb/tcpm.h | 1 + > > 2 files changed, 24 insertions(+) > > > > diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c > > index f1d3e54210df..29cd84ba9960 100644 > > --- a/drivers/usb/typec/tcpm/tcpm.c > > +++ b/drivers/usb/typec/tcpm/tcpm.c > > @@ -4494,6 +4494,29 @@ int tcpm_update_sink_capabilities(struct tcpm_port *port, const u32 *pdo, > > } > > EXPORT_SYMBOL_GPL(tcpm_update_sink_capabilities); > > > > +/* > > + * Don't call this function in interrupt context. Caller needs to free the > > + * memory itself. > > + */ > > +int tcpm_get_partner_src_caps(struct tcpm_port *port, u32 **src_pdo) > > +{ > > + unsigned int nr_pdo; > > + > > + if (port->nr_source_caps == 0) > > + return -ENODATA; > > + > > + *src_pdo = kcalloc(port->nr_source_caps, sizeof(u32), GFP_KERNEL); > > + if (!src_pdo) > > + return -ENOMEM; > > + > > + mutex_lock(&port->lock); > > + nr_pdo = tcpm_copy_pdos(*src_pdo, port->source_caps, > > + port->nr_source_caps); > > + mutex_unlock(&port->lock); The mutex use here suggests that the data can be updated while being copied. But that suggests that port->nr_source_caps can change as well and may no longer be current after mutex_lock(). > > + return nr_pdo; > > +} > > +EXPORT_SYMBOL_GPL(tcpm_get_partner_src_caps); > > We don't add new functions that no one uses :( > I am also concerned about the API itself; passing a pointer to be freed by the caller invites memory leaks. Guenter
On Thu, Jan 31, 2019 at 3:02 PM Greg KH <gregkh@linuxfoundation.org> wrote: > > On Thu, Jan 31, 2019 at 11:54:11AM +0800, Kyle Tso wrote: > > Provide a function to get the partner Source Capabilities. > > > > Signed-off-by: Kyle Tso <kyletso@google.com> > > --- > > drivers/usb/typec/tcpm/tcpm.c | 23 +++++++++++++++++++++++ > > include/linux/usb/tcpm.h | 1 + > > 2 files changed, 24 insertions(+) > > > > diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c > > index f1d3e54210df..29cd84ba9960 100644 > > --- a/drivers/usb/typec/tcpm/tcpm.c > > +++ b/drivers/usb/typec/tcpm/tcpm.c > > @@ -4494,6 +4494,29 @@ int tcpm_update_sink_capabilities(struct tcpm_port *port, const u32 *pdo, > > } > > EXPORT_SYMBOL_GPL(tcpm_update_sink_capabilities); > > > > +/* > > + * Don't call this function in interrupt context. Caller needs to free the > > + * memory itself. > > + */ > > +int tcpm_get_partner_src_caps(struct tcpm_port *port, u32 **src_pdo) > > +{ > > + unsigned int nr_pdo; > > + > > + if (port->nr_source_caps == 0) > > + return -ENODATA; > > + > > + *src_pdo = kcalloc(port->nr_source_caps, sizeof(u32), GFP_KERNEL); > > + if (!src_pdo) > > + return -ENOMEM; > > + > > + mutex_lock(&port->lock); > > + nr_pdo = tcpm_copy_pdos(*src_pdo, port->source_caps, > > + port->nr_source_caps); > > + mutex_unlock(&port->lock); > > + return nr_pdo; > > +} > > +EXPORT_SYMBOL_GPL(tcpm_get_partner_src_caps); > > We don't add new functions that no one uses :( > This function is useful if the PD Device Policy Manager is implemented outside of TCPM. In this situation, Device Policy Manager needs to know the partner capabilities to optimize the charging process. Take existing functions in TCPM for example: Function "tcpm_update_sink_capabilities" and "tcpm_update_source_capabilities" are exposed as well. And no one uses them now.
On Fri, Feb 1, 2019 at 2:22 AM Guenter Roeck <linux@roeck-us.net> wrote: > > On Thu, Jan 31, 2019 at 08:02:38AM +0100, Greg KH wrote: > > On Thu, Jan 31, 2019 at 11:54:11AM +0800, Kyle Tso wrote: > > > Provide a function to get the partner Source Capabilities. > > > > > > Signed-off-by: Kyle Tso <kyletso@google.com> > > > --- > > > drivers/usb/typec/tcpm/tcpm.c | 23 +++++++++++++++++++++++ > > > include/linux/usb/tcpm.h | 1 + > > > 2 files changed, 24 insertions(+) > > > > > > diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c > > > index f1d3e54210df..29cd84ba9960 100644 > > > --- a/drivers/usb/typec/tcpm/tcpm.c > > > +++ b/drivers/usb/typec/tcpm/tcpm.c > > > @@ -4494,6 +4494,29 @@ int tcpm_update_sink_capabilities(struct tcpm_port *port, const u32 *pdo, > > > } > > > EXPORT_SYMBOL_GPL(tcpm_update_sink_capabilities); > > > > > > +/* > > > + * Don't call this function in interrupt context. Caller needs to free the > > > + * memory itself. > > > + */ > > > +int tcpm_get_partner_src_caps(struct tcpm_port *port, u32 **src_pdo) > > > +{ > > > + unsigned int nr_pdo; > > > + > > > + if (port->nr_source_caps == 0) > > > + return -ENODATA; > > > + > > > + *src_pdo = kcalloc(port->nr_source_caps, sizeof(u32), GFP_KERNEL); > > > + if (!src_pdo) > > > + return -ENOMEM; > > > + > > > + mutex_lock(&port->lock); > > > + nr_pdo = tcpm_copy_pdos(*src_pdo, port->source_caps, > > > + port->nr_source_caps); > > > + mutex_unlock(&port->lock); > > The mutex use here suggests that the data can be updated while being > copied. But that suggests that port->nr_source_caps can change as > well and may no longer be current after mutex_lock(). > Thanks for pointing out the bug. I will fix this in the next version. > > > + return nr_pdo; > > > +} > > > +EXPORT_SYMBOL_GPL(tcpm_get_partner_src_caps); > > > > We don't add new functions that no one uses :( > > > > I am also concerned about the API itself; passing a pointer to be freed > by the caller invites memory leaks. > > Guenter What is the better way to implement this? How about providing a function (say "tcpm_put_partner_src_caps") for the caller to free the memory?
On Tue, Feb 12, 2019 at 06:29:39PM +0800, Kyle Tso wrote: > On Thu, Jan 31, 2019 at 3:02 PM Greg KH <gregkh@linuxfoundation.org> wrote: > > > On Thu, Jan 31, 2019 at 11:54:11AM +0800, Kyle Tso wrote: > > > Provide a function to get the partner Source Capabilities. > > > > > > Signed-off-by: Kyle Tso <kyletso@google.com> > > > --- > > > drivers/usb/typec/tcpm/tcpm.c | 23 +++++++++++++++++++++++ > > > include/linux/usb/tcpm.h | 1 + > > > 2 files changed, 24 insertions(+) > > > > > > diff --git a/drivers/usb/typec/tcpm/tcpm.c > > b/drivers/usb/typec/tcpm/tcpm.c > > > index f1d3e54210df..29cd84ba9960 100644 > > > --- a/drivers/usb/typec/tcpm/tcpm.c > > > +++ b/drivers/usb/typec/tcpm/tcpm.c > > > @@ -4494,6 +4494,29 @@ int tcpm_update_sink_capabilities(struct > > tcpm_port *port, const u32 *pdo, > > > } > > > EXPORT_SYMBOL_GPL(tcpm_update_sink_capabilities); > > > > > > +/* > > > + * Don't call this function in interrupt context. Caller needs to free > > the > > > + * memory itself. > > > + */ > > > +int tcpm_get_partner_src_caps(struct tcpm_port *port, u32 **src_pdo) > > > +{ > > > + unsigned int nr_pdo; > > > + > > > + if (port->nr_source_caps == 0) > > > + return -ENODATA; > > > + > > > + *src_pdo = kcalloc(port->nr_source_caps, sizeof(u32), GFP_KERNEL); > > > + if (!src_pdo) > > > + return -ENOMEM; > > > + > > > + mutex_lock(&port->lock); > > > + nr_pdo = tcpm_copy_pdos(*src_pdo, port->source_caps, > > > + port->nr_source_caps); > > > + mutex_unlock(&port->lock); > > > + return nr_pdo; > > > +} > > > +EXPORT_SYMBOL_GPL(tcpm_get_partner_src_caps); > > > > We don't add new functions that no one uses :( > > > > > This function is useful if the PD Device Policy Manager is implemented > outside of TCPM. > In this situation, Device Policy Manager needs to know the partner > capabilities to optimize > the charging process. And where is that code? > Take existing functions in TCPM for example: > Function "tcpm_update_sink_capabilities" and > "tcpm_update_source_capabilities" are exposed > as well. And no one uses them now. Great, let's go delete them now, we should not have apis that no one uses. This isn't a new thing... thanks, greg k-h
On 2/12/19 2:54 AM, Greg KH wrote: > On Tue, Feb 12, 2019 at 06:29:39PM +0800, Kyle Tso wrote: >> On Thu, Jan 31, 2019 at 3:02 PM Greg KH <gregkh@linuxfoundation.org> wrote: >> >>> On Thu, Jan 31, 2019 at 11:54:11AM +0800, Kyle Tso wrote: >>>> Provide a function to get the partner Source Capabilities. >>>> >>>> Signed-off-by: Kyle Tso <kyletso@google.com> >>>> --- >>>> drivers/usb/typec/tcpm/tcpm.c | 23 +++++++++++++++++++++++ >>>> include/linux/usb/tcpm.h | 1 + >>>> 2 files changed, 24 insertions(+) >>>> >>>> diff --git a/drivers/usb/typec/tcpm/tcpm.c >>> b/drivers/usb/typec/tcpm/tcpm.c >>>> index f1d3e54210df..29cd84ba9960 100644 >>>> --- a/drivers/usb/typec/tcpm/tcpm.c >>>> +++ b/drivers/usb/typec/tcpm/tcpm.c >>>> @@ -4494,6 +4494,29 @@ int tcpm_update_sink_capabilities(struct >>> tcpm_port *port, const u32 *pdo, >>>> } >>>> EXPORT_SYMBOL_GPL(tcpm_update_sink_capabilities); >>>> >>>> +/* >>>> + * Don't call this function in interrupt context. Caller needs to free >>> the >>>> + * memory itself. >>>> + */ >>>> +int tcpm_get_partner_src_caps(struct tcpm_port *port, u32 **src_pdo) >>>> +{ >>>> + unsigned int nr_pdo; >>>> + >>>> + if (port->nr_source_caps == 0) >>>> + return -ENODATA; >>>> + >>>> + *src_pdo = kcalloc(port->nr_source_caps, sizeof(u32), GFP_KERNEL); >>>> + if (!src_pdo) >>>> + return -ENOMEM; >>>> + >>>> + mutex_lock(&port->lock); >>>> + nr_pdo = tcpm_copy_pdos(*src_pdo, port->source_caps, >>>> + port->nr_source_caps); >>>> + mutex_unlock(&port->lock); >>>> + return nr_pdo; >>>> +} >>>> +EXPORT_SYMBOL_GPL(tcpm_get_partner_src_caps); >>> >>> We don't add new functions that no one uses :( >>> >>> >> This function is useful if the PD Device Policy Manager is implemented >> outside of TCPM. >> In this situation, Device Policy Manager needs to know the partner >> capabilities to optimize >> the charging process. > > And where is that code? > Agreed - that code should be sent upstream as well to let us see the entire context. >> Take existing functions in TCPM for example: >> Function "tcpm_update_sink_capabilities" and >> "tcpm_update_source_capabilities" are exposed >> as well. And no one uses them now. > > Great, let's go delete them now, we should not have apis that no one > uses. This isn't a new thing... > I sent a patch to do just that. Quite frankly I don't recal why I thought those functions might be needed. Guenter
On 12 February 2019 16:20, Guenter Roeck wrote: > On 2/12/19 2:54 AM, Greg KH wrote: > > On Tue, Feb 12, 2019 at 06:29:39PM +0800, Kyle Tso wrote: > >> On Thu, Jan 31, 2019 at 3:02 PM Greg KH <gregkh@linuxfoundation.org> > wrote: > >> > >>> On Thu, Jan 31, 2019 at 11:54:11AM +0800, Kyle Tso wrote: > >>>> Provide a function to get the partner Source Capabilities. > >>>> > >>>> Signed-off-by: Kyle Tso <kyletso@google.com> > >>>> --- > >>>> drivers/usb/typec/tcpm/tcpm.c | 23 +++++++++++++++++++++++ > >>>> include/linux/usb/tcpm.h | 1 + > >>>> 2 files changed, 24 insertions(+) > >>>> > >>>> diff --git a/drivers/usb/typec/tcpm/tcpm.c > >>> b/drivers/usb/typec/tcpm/tcpm.c > >>>> index f1d3e54210df..29cd84ba9960 100644 > >>>> --- a/drivers/usb/typec/tcpm/tcpm.c > >>>> +++ b/drivers/usb/typec/tcpm/tcpm.c > >>>> @@ -4494,6 +4494,29 @@ int tcpm_update_sink_capabilities(struct > >>> tcpm_port *port, const u32 *pdo, > >>>> } > >>>> EXPORT_SYMBOL_GPL(tcpm_update_sink_capabilities); > >>>> > >>>> +/* > >>>> + * Don't call this function in interrupt context. Caller needs to > >>>> +free > >>> the > >>>> + * memory itself. > >>>> + */ > >>>> +int tcpm_get_partner_src_caps(struct tcpm_port *port, u32 > >>>> +**src_pdo) { > >>>> + unsigned int nr_pdo; > >>>> + > >>>> + if (port->nr_source_caps == 0) > >>>> + return -ENODATA; > >>>> + > >>>> + *src_pdo = kcalloc(port->nr_source_caps, sizeof(u32), GFP_KERNEL); > >>>> + if (!src_pdo) > >>>> + return -ENOMEM; > >>>> + > >>>> + mutex_lock(&port->lock); > >>>> + nr_pdo = tcpm_copy_pdos(*src_pdo, port->source_caps, > >>>> + port->nr_source_caps); > >>>> + mutex_unlock(&port->lock); > >>>> + return nr_pdo; > >>>> +} > >>>> +EXPORT_SYMBOL_GPL(tcpm_get_partner_src_caps); > >>> > >>> We don't add new functions that no one uses :( > >>> > >>> > >> This function is useful if the PD Device Policy Manager is > >> implemented outside of TCPM. > >> In this situation, Device Policy Manager needs to know the partner > >> capabilities to optimize the charging process. > > > > And where is that code? > > > > Agreed - that code should be sent upstream as well to let us see the entire > context. > > >> Take existing functions in TCPM for example: > >> Function "tcpm_update_sink_capabilities" and > >> "tcpm_update_source_capabilities" are exposed as well. And no one > >> uses them now. > > > > Great, let's go delete them now, we should not have apis that no one > > uses. This isn't a new thing... > > > > I sent a patch to do just that. Quite frankly I don't recal why I thought those > functions might be needed. As a mind jog - https://lkml.org/lkml/2017/11/27/1256 :)
On Thu, Feb 14, 2019 at 02:17:14PM +0000, Adam Thomson wrote: > On 12 February 2019 16:20, Guenter Roeck wrote: > > > On 2/12/19 2:54 AM, Greg KH wrote: > > > On Tue, Feb 12, 2019 at 06:29:39PM +0800, Kyle Tso wrote: > > >> On Thu, Jan 31, 2019 at 3:02 PM Greg KH <gregkh@linuxfoundation.org> > > wrote: > > >> > > >>> On Thu, Jan 31, 2019 at 11:54:11AM +0800, Kyle Tso wrote: > > >>>> Provide a function to get the partner Source Capabilities. > > >>>> > > >>>> Signed-off-by: Kyle Tso <kyletso@google.com> > > >>>> --- > > >>>> drivers/usb/typec/tcpm/tcpm.c | 23 +++++++++++++++++++++++ > > >>>> include/linux/usb/tcpm.h | 1 + > > >>>> 2 files changed, 24 insertions(+) > > >>>> > > >>>> diff --git a/drivers/usb/typec/tcpm/tcpm.c > > >>> b/drivers/usb/typec/tcpm/tcpm.c > > >>>> index f1d3e54210df..29cd84ba9960 100644 > > >>>> --- a/drivers/usb/typec/tcpm/tcpm.c > > >>>> +++ b/drivers/usb/typec/tcpm/tcpm.c > > >>>> @@ -4494,6 +4494,29 @@ int tcpm_update_sink_capabilities(struct > > >>> tcpm_port *port, const u32 *pdo, > > >>>> } > > >>>> EXPORT_SYMBOL_GPL(tcpm_update_sink_capabilities); > > >>>> > > >>>> +/* > > >>>> + * Don't call this function in interrupt context. Caller needs to > > >>>> +free > > >>> the > > >>>> + * memory itself. > > >>>> + */ > > >>>> +int tcpm_get_partner_src_caps(struct tcpm_port *port, u32 > > >>>> +**src_pdo) { > > >>>> + unsigned int nr_pdo; > > >>>> + > > >>>> + if (port->nr_source_caps == 0) > > >>>> + return -ENODATA; > > >>>> + > > >>>> + *src_pdo = kcalloc(port->nr_source_caps, sizeof(u32), GFP_KERNEL); > > >>>> + if (!src_pdo) > > >>>> + return -ENOMEM; > > >>>> + > > >>>> + mutex_lock(&port->lock); > > >>>> + nr_pdo = tcpm_copy_pdos(*src_pdo, port->source_caps, > > >>>> + port->nr_source_caps); > > >>>> + mutex_unlock(&port->lock); > > >>>> + return nr_pdo; > > >>>> +} > > >>>> +EXPORT_SYMBOL_GPL(tcpm_get_partner_src_caps); > > >>> > > >>> We don't add new functions that no one uses :( > > >>> > > >>> > > >> This function is useful if the PD Device Policy Manager is > > >> implemented outside of TCPM. > > >> In this situation, Device Policy Manager needs to know the partner > > >> capabilities to optimize the charging process. > > > > > > And where is that code? > > > > > > > Agreed - that code should be sent upstream as well to let us see the entire > > context. > > > > >> Take existing functions in TCPM for example: > > >> Function "tcpm_update_sink_capabilities" and > > >> "tcpm_update_source_capabilities" are exposed as well. And no one > > >> uses them now. > > > > > > Great, let's go delete them now, we should not have apis that no one > > > uses. This isn't a new thing... > > > > > > > I sent a patch to do just that. Quite frankly I don't recal why I thought those > > functions might be needed. > > As a mind jog - https://lkml.org/lkml/2017/11/27/1256 :) Nice find :) Given that no one used these functions in over a year, we should be safe deleting them. If not, it's trivial to do 'git revert'. thanks, greg k-h
On 2/14/19 6:17 AM, Adam Thomson wrote: > On 12 February 2019 16:20, Guenter Roeck wrote: > >> On 2/12/19 2:54 AM, Greg KH wrote: >>> On Tue, Feb 12, 2019 at 06:29:39PM +0800, Kyle Tso wrote: >>>> On Thu, Jan 31, 2019 at 3:02 PM Greg KH <gregkh@linuxfoundation.org> >> wrote: >>>> >>>>> On Thu, Jan 31, 2019 at 11:54:11AM +0800, Kyle Tso wrote: >>>>>> Provide a function to get the partner Source Capabilities. >>>>>> >>>>>> Signed-off-by: Kyle Tso <kyletso@google.com> >>>>>> --- >>>>>> drivers/usb/typec/tcpm/tcpm.c | 23 +++++++++++++++++++++++ >>>>>> include/linux/usb/tcpm.h | 1 + >>>>>> 2 files changed, 24 insertions(+) >>>>>> >>>>>> diff --git a/drivers/usb/typec/tcpm/tcpm.c >>>>> b/drivers/usb/typec/tcpm/tcpm.c >>>>>> index f1d3e54210df..29cd84ba9960 100644 >>>>>> --- a/drivers/usb/typec/tcpm/tcpm.c >>>>>> +++ b/drivers/usb/typec/tcpm/tcpm.c >>>>>> @@ -4494,6 +4494,29 @@ int tcpm_update_sink_capabilities(struct >>>>> tcpm_port *port, const u32 *pdo, >>>>>> } >>>>>> EXPORT_SYMBOL_GPL(tcpm_update_sink_capabilities); >>>>>> >>>>>> +/* >>>>>> + * Don't call this function in interrupt context. Caller needs to >>>>>> +free >>>>> the >>>>>> + * memory itself. >>>>>> + */ >>>>>> +int tcpm_get_partner_src_caps(struct tcpm_port *port, u32 >>>>>> +**src_pdo) { >>>>>> + unsigned int nr_pdo; >>>>>> + >>>>>> + if (port->nr_source_caps == 0) >>>>>> + return -ENODATA; >>>>>> + >>>>>> + *src_pdo = kcalloc(port->nr_source_caps, sizeof(u32), GFP_KERNEL); >>>>>> + if (!src_pdo) >>>>>> + return -ENOMEM; >>>>>> + >>>>>> + mutex_lock(&port->lock); >>>>>> + nr_pdo = tcpm_copy_pdos(*src_pdo, port->source_caps, >>>>>> + port->nr_source_caps); >>>>>> + mutex_unlock(&port->lock); >>>>>> + return nr_pdo; >>>>>> +} >>>>>> +EXPORT_SYMBOL_GPL(tcpm_get_partner_src_caps); >>>>> >>>>> We don't add new functions that no one uses :( >>>>> >>>>> >>>> This function is useful if the PD Device Policy Manager is >>>> implemented outside of TCPM. >>>> In this situation, Device Policy Manager needs to know the partner >>>> capabilities to optimize the charging process. >>> >>> And where is that code? >>> >> >> Agreed - that code should be sent upstream as well to let us see the entire >> context. >> >>>> Take existing functions in TCPM for example: >>>> Function "tcpm_update_sink_capabilities" and >>>> "tcpm_update_source_capabilities" are exposed as well. And no one >>>> uses them now. >>> >>> Great, let's go delete them now, we should not have apis that no one >>> uses. This isn't a new thing... >>> >> >> I sent a patch to do just that. Quite frankly I don't recal why I thought those >> functions might be needed. > > As a mind jog - https://lkml.org/lkml/2017/11/27/1256 :) > Nice find. However, since the code using it was never implemented, I think it is prudent at this point to remove the code. It can be reintroduced if/when needed, and at that point be tested properly. Thanks, Guenter
On 14 February 2019 14:30, Greg KH wrote: > On Thu, Feb 14, 2019 at 02:17:14PM +0000, Adam Thomson wrote: > > On 12 February 2019 16:20, Guenter Roeck wrote: > > > > > On 2/12/19 2:54 AM, Greg KH wrote: > > > > On Tue, Feb 12, 2019 at 06:29:39PM +0800, Kyle Tso wrote: > > > >> On Thu, Jan 31, 2019 at 3:02 PM Greg KH > > > >> <gregkh@linuxfoundation.org> > > > wrote: > > > >> > > > >>> On Thu, Jan 31, 2019 at 11:54:11AM +0800, Kyle Tso wrote: > > > >>>> Provide a function to get the partner Source Capabilities. > > > >>>> > > > >>>> Signed-off-by: Kyle Tso <kyletso@google.com> > > > >>>> --- > > > >>>> drivers/usb/typec/tcpm/tcpm.c | 23 +++++++++++++++++++++++ > > > >>>> include/linux/usb/tcpm.h | 1 + > > > >>>> 2 files changed, 24 insertions(+) > > > >>>> > > > >>>> diff --git a/drivers/usb/typec/tcpm/tcpm.c > > > >>> b/drivers/usb/typec/tcpm/tcpm.c > > > >>>> index f1d3e54210df..29cd84ba9960 100644 > > > >>>> --- a/drivers/usb/typec/tcpm/tcpm.c > > > >>>> +++ b/drivers/usb/typec/tcpm/tcpm.c > > > >>>> @@ -4494,6 +4494,29 @@ int tcpm_update_sink_capabilities(struct > > > >>> tcpm_port *port, const u32 *pdo, > > > >>>> } > > > >>>> EXPORT_SYMBOL_GPL(tcpm_update_sink_capabilities); > > > >>>> > > > >>>> +/* > > > >>>> + * Don't call this function in interrupt context. Caller needs > > > >>>> +to free > > > >>> the > > > >>>> + * memory itself. > > > >>>> + */ > > > >>>> +int tcpm_get_partner_src_caps(struct tcpm_port *port, u32 > > > >>>> +**src_pdo) { > > > >>>> + unsigned int nr_pdo; > > > >>>> + > > > >>>> + if (port->nr_source_caps == 0) > > > >>>> + return -ENODATA; > > > >>>> + > > > >>>> + *src_pdo = kcalloc(port->nr_source_caps, sizeof(u32), > GFP_KERNEL); > > > >>>> + if (!src_pdo) > > > >>>> + return -ENOMEM; > > > >>>> + > > > >>>> + mutex_lock(&port->lock); > > > >>>> + nr_pdo = tcpm_copy_pdos(*src_pdo, port->source_caps, > > > >>>> + port->nr_source_caps); > > > >>>> + mutex_unlock(&port->lock); > > > >>>> + return nr_pdo; > > > >>>> +} > > > >>>> +EXPORT_SYMBOL_GPL(tcpm_get_partner_src_caps); > > > >>> > > > >>> We don't add new functions that no one uses :( > > > >>> > > > >>> > > > >> This function is useful if the PD Device Policy Manager is > > > >> implemented outside of TCPM. > > > >> In this situation, Device Policy Manager needs to know the > > > >> partner capabilities to optimize the charging process. > > > > > > > > And where is that code? > > > > > > > > > > Agreed - that code should be sent upstream as well to let us see the > > > entire context. > > > > > > >> Take existing functions in TCPM for example: > > > >> Function "tcpm_update_sink_capabilities" and > > > >> "tcpm_update_source_capabilities" are exposed as well. And no one > > > >> uses them now. > > > > > > > > Great, let's go delete them now, we should not have apis that no > > > > one uses. This isn't a new thing... > > > > > > > > > > I sent a patch to do just that. Quite frankly I don't recal why I > > > thought those functions might be needed. > > > > As a mind jog - https://lkml.org/lkml/2017/11/27/1256 :) > > Nice find :) > > Given that no one used these functions in over a year, we should be safe > deleting them. If not, it's trivial to do 'git revert'. Yep, agreed.
diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c index f1d3e54210df..29cd84ba9960 100644 --- a/drivers/usb/typec/tcpm/tcpm.c +++ b/drivers/usb/typec/tcpm/tcpm.c @@ -4494,6 +4494,29 @@ int tcpm_update_sink_capabilities(struct tcpm_port *port, const u32 *pdo, } EXPORT_SYMBOL_GPL(tcpm_update_sink_capabilities); +/* + * Don't call this function in interrupt context. Caller needs to free the + * memory itself. + */ +int tcpm_get_partner_src_caps(struct tcpm_port *port, u32 **src_pdo) +{ + unsigned int nr_pdo; + + if (port->nr_source_caps == 0) + return -ENODATA; + + *src_pdo = kcalloc(port->nr_source_caps, sizeof(u32), GFP_KERNEL); + if (!src_pdo) + return -ENOMEM; + + mutex_lock(&port->lock); + nr_pdo = tcpm_copy_pdos(*src_pdo, port->source_caps, + port->nr_source_caps); + mutex_unlock(&port->lock); + return nr_pdo; +} +EXPORT_SYMBOL_GPL(tcpm_get_partner_src_caps); + /* Power Supply access to expose source power information */ enum tcpm_psy_online_states { TCPM_PSY_OFFLINE = 0, diff --git a/include/linux/usb/tcpm.h b/include/linux/usb/tcpm.h index 50c74a77db55..fe56d759567c 100644 --- a/include/linux/usb/tcpm.h +++ b/include/linux/usb/tcpm.h @@ -173,5 +173,6 @@ void tcpm_pd_transmit_complete(struct tcpm_port *port, enum tcpm_transmit_status status); void tcpm_pd_hard_reset(struct tcpm_port *port); void tcpm_tcpc_reset(struct tcpm_port *port); +int tcpm_get_partner_src_caps(struct tcpm_port *port, u32 **pdo); #endif /* __LINUX_USB_TCPM_H */
Provide a function to get the partner Source Capabilities. Signed-off-by: Kyle Tso <kyletso@google.com> --- drivers/usb/typec/tcpm/tcpm.c | 23 +++++++++++++++++++++++ include/linux/usb/tcpm.h | 1 + 2 files changed, 24 insertions(+)