* [PATCH] usb: typec: tcpm: Export partner Source Capabilities @ 2019-01-31 3:54 Kyle Tso 2019-01-31 7:02 ` Greg KH 0 siblings, 1 reply; 11+ messages in thread From: Kyle Tso @ 2019-01-31 3:54 UTC (permalink / raw) To: linux, heikki.krogerus, gregkh; +Cc: badhri, linux-usb, linux-kernel, 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] usb: typec: tcpm: Export partner Source Capabilities 2019-01-31 3:54 [PATCH] usb: typec: tcpm: Export partner Source Capabilities Kyle Tso @ 2019-01-31 7:02 ` Greg KH 2019-01-31 18:22 ` Guenter Roeck ` (2 more replies) 0 siblings, 3 replies; 11+ messages in thread From: Greg KH @ 2019-01-31 7:02 UTC (permalink / raw) To: Kyle Tso; +Cc: linux, heikki.krogerus, badhri, linux-usb, linux-kernel 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 :( ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] usb: typec: tcpm: Export partner Source Capabilities 2019-01-31 7:02 ` Greg KH @ 2019-01-31 18:22 ` Guenter Roeck 2019-02-12 10:33 ` Kyle Tso 2019-02-12 10:32 ` Kyle Tso [not found] ` <CAGZ6i=07EOHqd3r9y1MpYn_e1nFeYGt_wp9arhMZWWeTBm-udA@mail.gmail.com> 2 siblings, 1 reply; 11+ messages in thread From: Guenter Roeck @ 2019-01-31 18:22 UTC (permalink / raw) To: Greg KH; +Cc: Kyle Tso, heikki.krogerus, badhri, linux-usb, linux-kernel 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] usb: typec: tcpm: Export partner Source Capabilities 2019-01-31 18:22 ` Guenter Roeck @ 2019-02-12 10:33 ` Kyle Tso 0 siblings, 0 replies; 11+ messages in thread From: Kyle Tso @ 2019-02-12 10:33 UTC (permalink / raw) To: Guenter Roeck Cc: Greg KH, Heikki Krogerus, Badhri Jagan Sridharan, linux-usb, linux-kernel 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? ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] usb: typec: tcpm: Export partner Source Capabilities 2019-01-31 7:02 ` Greg KH 2019-01-31 18:22 ` Guenter Roeck @ 2019-02-12 10:32 ` Kyle Tso [not found] ` <CAGZ6i=07EOHqd3r9y1MpYn_e1nFeYGt_wp9arhMZWWeTBm-udA@mail.gmail.com> 2 siblings, 0 replies; 11+ messages in thread From: Kyle Tso @ 2019-02-12 10:32 UTC (permalink / raw) To: Greg KH Cc: Guenter Roeck, Heikki Krogerus, Badhri Jagan Sridharan, linux-usb, linux-kernel 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. ^ permalink raw reply [flat|nested] 11+ messages in thread
[parent not found: <CAGZ6i=07EOHqd3r9y1MpYn_e1nFeYGt_wp9arhMZWWeTBm-udA@mail.gmail.com>]
* Re: [PATCH] usb: typec: tcpm: Export partner Source Capabilities [not found] ` <CAGZ6i=07EOHqd3r9y1MpYn_e1nFeYGt_wp9arhMZWWeTBm-udA@mail.gmail.com> @ 2019-02-12 10:54 ` Greg KH 2019-02-12 16:19 ` Guenter Roeck 0 siblings, 1 reply; 11+ messages in thread From: Greg KH @ 2019-02-12 10:54 UTC (permalink / raw) To: Kyle Tso Cc: Guenter Roeck, Heikki Krogerus, Badhri Jagan Sridharan, linux-usb, linux-kernel 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] usb: typec: tcpm: Export partner Source Capabilities 2019-02-12 10:54 ` Greg KH @ 2019-02-12 16:19 ` Guenter Roeck 2019-02-14 14:17 ` Adam Thomson 0 siblings, 1 reply; 11+ messages in thread From: Guenter Roeck @ 2019-02-12 16:19 UTC (permalink / raw) To: Greg KH, Kyle Tso Cc: Heikki Krogerus, Badhri Jagan Sridharan, linux-usb, linux-kernel 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: [PATCH] usb: typec: tcpm: Export partner Source Capabilities 2019-02-12 16:19 ` Guenter Roeck @ 2019-02-14 14:17 ` Adam Thomson 2019-02-14 14:30 ` Greg KH 2019-02-14 14:31 ` Guenter Roeck 0 siblings, 2 replies; 11+ messages in thread From: Adam Thomson @ 2019-02-14 14:17 UTC (permalink / raw) To: Guenter Roeck, Greg KH, Kyle Tso Cc: Heikki Krogerus, Badhri Jagan Sridharan, linux-usb, linux-kernel 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 :) ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] usb: typec: tcpm: Export partner Source Capabilities 2019-02-14 14:17 ` Adam Thomson @ 2019-02-14 14:30 ` Greg KH 2019-02-14 15:28 ` Adam Thomson 2019-02-14 14:31 ` Guenter Roeck 1 sibling, 1 reply; 11+ messages in thread From: Greg KH @ 2019-02-14 14:30 UTC (permalink / raw) To: Adam Thomson Cc: Guenter Roeck, Kyle Tso, Heikki Krogerus, Badhri Jagan Sridharan, linux-usb, linux-kernel 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: [PATCH] usb: typec: tcpm: Export partner Source Capabilities 2019-02-14 14:30 ` Greg KH @ 2019-02-14 15:28 ` Adam Thomson 0 siblings, 0 replies; 11+ messages in thread From: Adam Thomson @ 2019-02-14 15:28 UTC (permalink / raw) To: Greg KH, Adam Thomson Cc: Guenter Roeck, Kyle Tso, Heikki Krogerus, Badhri Jagan Sridharan, linux-usb, linux-kernel 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. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] usb: typec: tcpm: Export partner Source Capabilities 2019-02-14 14:17 ` Adam Thomson 2019-02-14 14:30 ` Greg KH @ 2019-02-14 14:31 ` Guenter Roeck 1 sibling, 0 replies; 11+ messages in thread From: Guenter Roeck @ 2019-02-14 14:31 UTC (permalink / raw) To: Adam Thomson, Greg KH, Kyle Tso Cc: Heikki Krogerus, Badhri Jagan Sridharan, linux-usb, linux-kernel 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2019-02-14 15:29 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-01-31 3:54 [PATCH] usb: typec: tcpm: Export partner Source Capabilities Kyle Tso
2019-01-31 7:02 ` Greg KH
2019-01-31 18:22 ` Guenter Roeck
2019-02-12 10:33 ` Kyle Tso
2019-02-12 10:32 ` Kyle Tso
[not found] ` <CAGZ6i=07EOHqd3r9y1MpYn_e1nFeYGt_wp9arhMZWWeTBm-udA@mail.gmail.com>
2019-02-12 10:54 ` Greg KH
2019-02-12 16:19 ` Guenter Roeck
2019-02-14 14:17 ` Adam Thomson
2019-02-14 14:30 ` Greg KH
2019-02-14 15:28 ` Adam Thomson
2019-02-14 14:31 ` Guenter Roeck
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).