[PATCH] usb: typec: tcpm: Export partner Source Capabilities

[PATCH] usb: typec: tcpm: Export partner Source Capabilities

[Kyle Tso]

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);
+
 /* 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 */
-- 
2.20.1.495.gaa96b0ce6b-goog

Re: [PATCH] usb: typec: tcpm: Export partner Source Capabilities

[Greg KH]

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 :(

Re: [PATCH] usb: typec: tcpm: Export partner Source Capabilities

[Guenter Roeck]

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

Re: [PATCH] usb: typec: tcpm: Export partner Source Capabilities

[Kyle Tso]

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.

Re: [PATCH] usb: typec: tcpm: Export partner Source Capabilities

[Kyle Tso]

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?

Re: [PATCH] usb: typec: tcpm: Export partner Source Capabilities

[Greg KH]

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

Re: [PATCH] usb: typec: tcpm: Export partner Source Capabilities

[Guenter Roeck]

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

RE: [PATCH] usb: typec: tcpm: Export partner Source Capabilities

[Adam Thomson]

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 :)

Re: [PATCH] usb: typec: tcpm: Export partner Source Capabilities

[Greg KH]

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

Re: [PATCH] usb: typec: tcpm: Export partner Source Capabilities

[Guenter Roeck]

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

RE: [PATCH] usb: typec: tcpm: Export partner Source Capabilities

[Adam Thomson]

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.