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

Groups > linux.kernel > #1443994 > unrolled thread

Re: [PATCH 1/2] usb: typec: Add USB Power Delivery sink port support

Back to article view | Back to linux.kernel

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

Contents

  Re: [PATCH 1/2] usb: typec: Add USB Power Delivery sink port support Felipe Balbi <felipe.balbi@linux.intel.com> - 2016-07-15 09:30 +0200
    Re: [PATCH 1/2] usb: typec: Add USB Power Delivery sink port support Oliver Neukum <oneukum@suse.com> - 2016-07-15 10:50 +0200
      Re: [PATCH 1/2] usb: typec: Add USB Power Delivery sink port support Felipe Balbi <felipe.balbi@linux.intel.com> - 2016-07-15 12:40 +0200
        Re: [PATCH 1/2] usb: typec: Add USB Power Delivery sink port support Felipe Balbi <felipe.balbi@linux.intel.com> - 2016-07-15 15:20 +0200
    Re: [PATCH 1/2] usb: typec: Add USB Power Delivery sink port support Bin Gao <bin.gao@linux.intel.com> - 2016-07-16 01:50 +0200
      Re: [PATCH 1/2] usb: typec: Add USB Power Delivery sink port support Felipe Balbi <felipe.balbi@linux.intel.com> - 2016-07-18 09:10 +0200
        Re: [PATCH 1/2] usb: typec: Add USB Power Delivery sink port support Bin Gao <bin.gao@linux.intel.com> - 2016-07-19 07:40 +0200
          Re: [PATCH 1/2] usb: typec: Add USB Power Delivery sink port support Felipe Balbi <felipe.balbi@linux.intel.com> - 2016-07-19 10:40 +0200

#1443994 — Re: [PATCH 1/2] usb: typec: Add USB Power Delivery sink port support

[Multipart message — attachments visible in raw view] — view raw

Bin Gao <bin.gao@linux.intel.com> writes:

> This patch implements a simple USB Power Delivery sink port state machine.
> It assumes the hardware only handles PD packet transmitting and receiving
> over the CC line of the USB Type-C connector. The state transition is
> completely controlled by software. This patch only implement the sink port
> function and it doesn't support source port and port swap yet.
>
> This patch depends on these two patches:
> https://lkml.org/lkml/2016/6/29/349
> https://lkml.org/lkml/2016/6/29/350
>
> Signed-off-by: Bin Gao <bin.gao@intel.com>
> ---
>  drivers/usb/typec/Kconfig      |  13 +
>  drivers/usb/typec/Makefile     |   1 +
>  drivers/usb/typec/pd_sink.c    | 967 +++++++++++++++++++++++++++++++++++++++++
>  include/linux/usb/pd_message.h | 371 ++++++++++++++++
>  include/linux/usb/pd_sink.h    | 286 ++++++++++++
>  5 files changed, 1638 insertions(+)
>  create mode 100644 drivers/usb/typec/pd_sink.c
>  create mode 100644 include/linux/usb/pd_message.h
>  create mode 100644 include/linux/usb/pd_sink.h
>
> diff --git a/drivers/usb/typec/Kconfig b/drivers/usb/typec/Kconfig
> index 7a345a4..a04a900 100644
> --- a/drivers/usb/typec/Kconfig
> +++ b/drivers/usb/typec/Kconfig
> @@ -4,12 +4,25 @@ menu "USB PD and Type-C drivers"
>  config TYPEC
>  	tristate
>  
> +config USB_PD_SINK
> +	bool "USB Power Delivery Sink Port State Machine Driver"

tristate?

> +	select TYPEC

this should depend on TYPEC, not select it.

> +	help
> +	  Enable this to support USB PD(Power Delivery) Sink port.
> +	  This driver implements a simple USB PD sink state machine.
> +	  The underlying TypeC phy driver is responsible for cable
> +	  plug/unplug event, port orientation detection, transmitting
> +	  and receiving PD messages. This driver only process messages
> +	  received by the TypeC phy driver and maintain the sink port's
> +	  state machine.
> +
>  config TYPEC_WCOVE
>  	tristate "Intel WhiskeyCove PMIC USB Type-C PHY driver"
>  	depends on ACPI
>  	depends on INTEL_SOC_PMIC
>  	depends on INTEL_PMC_IPC
>  	select TYPEC
> +	select USB_PD_SINK

TYPEC without PD is valid, let user select PD support.

> diff --git a/drivers/usb/typec/pd_sink.c b/drivers/usb/typec/pd_sink.c
> new file mode 100644
> index 0000000..374bdef
> --- /dev/null
> +++ b/drivers/usb/typec/pd_sink.c
> @@ -0,0 +1,967 @@
> +/*
> + * pd_sink.c - USB PD (Power Delivery) sink port state machine driver
> + *
> + * This driver implements a simple USB PD sink port state machine.
> + * It assumes the upper layer, i.e. the user of this driver, handles
> + * the PD message receiving and transmitting. The upper layer receives
> + * PD messages from the Source, queues them to us, and when processing
> + * the received message we'll call upper layer's transmitting function
> + * to send PD messages to the source.
> + * The sink port state machine is maintained in this driver but we also
> + * broadcast some important PD messages to upper layer as events.
> + *
> + * Copyright (C) 2016 Intel Corporation
> + *
> + * Author: Bin Gao <bin.gao@intel.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/slab.h>
> +#include <linux/usb/pd_sink.h>
> +
> +#define MAKE_HEADER(port, header, msg, objs) \
> +do { \
> +	header->type = msg; \
> +	header->data_role = PD_DATA_ROLE_UFP; \
> +	header->revision = port->pd_rev; \
> +	header->power_role = PD_POWER_ROLE_SINK; \
> +	header->id = roll_msg_id(port); \
> +	header->nr_objs = objs; \
> +	header->extended = PD_MSG_NOT_EXTENDED; \
> +} while (0)
> +
> +static struct pd_sink_port *sink_ports[MAX_NR_SINK_PORTS];
> +static int nr_ports;
> +
> +BLOCKING_NOTIFIER_HEAD(pd_sink_notifier_list);
> +
> +static char *state_strings[] = {
> +	"WAIT_FOR_SOURCE_CAPABILITY",
> +	"REQUEST_SENT",
> +	"ACCEPT_RECEIVED",
> +	"POWER_SUPPLY_READY",
> +};
> +
> +/* Control messages */
> +static char *cmsg_strings[] = {
> +	"GOODCRC",			/* 1 */
> +	"GOTOMIN",			/* 2 */
> +	"ACCEPT",			/* 3 */
> +	"REJECT",			/* 4 */
> +	"PING",				/* 5 */
> +	"PS_RDY",			/* 6 */
> +	"GET_SRC_CAP",			/* 7 */
> +	"GET_SINK_CAP",			/* 8 */
> +	"DR_SWAP",			/* 9 */
> +	"PR_SWAP",			/* 10 */
> +	"VCONN_SWAP",			/* 11 */
> +	"WAIT",				/* 12 */
> +	"SOFT_RESET",			/* 13 */
> +	"RESERVED",			/* 14 */
> +	"RESERVED",			/* 15 */
> +	"NOT_SUPPORTED",		/* 16 */
> +	"GET_SOURCE_CAP_EXTENDED",	/* 17 */
> +	"GET_STATUS",			/* 18 */
> +	"FR_SWAP",			/* 19 */
> +	/* RESERVED			20 - 31 */
> +};
> +
> +/* Data messages */
> +static char *dmsg_strings[] = {
> +	"SOURCE_CAP",		/* 1 */
> +	"REQUEST",		/* 2 */
> +	"BIST",			/* 3 */
> +	"SINK_CAP",		/* 4 */
> +	"BATTERY_STATUS",	/* 5 */
> +	"ALERT",		/* 6 */
> +	"RESERVED",		/* 7 */
> +	"RESERVED",		/* 8 */
> +	"RESERVED",		/* 9 */
> +	"RESERVED",		/* 10 */
> +	"RESERVED",		/* 11 */
> +	"RESERVED",		/* 12 */
> +	"RESERVED",		/* 13 */
> +	"RESERVED",		/* 14 */
> +	"VENDOR_DEFINED",	/* 15 */
> +	/* RESERVED		16 - 31 */
> +};
> +
> +static char *state_to_string(enum pd_sink_state state)
> +{
> +	if (state < ARRAY_SIZE(state_strings))
> +		return state_strings[state];
> +	else
> +		return NULL;
> +}
> +
> +static char *msg_to_string(bool is_cmsg, u8 msg)
> +{
> +	int nr = is_cmsg ? ARRAY_SIZE(cmsg_strings) : ARRAY_SIZE(dmsg_strings);
> +
> +	if (msg <= nr)
> +		return is_cmsg ? cmsg_strings[msg - 1] : dmsg_strings[msg - 1];
> +	else
> +		return "RESERVED";
> +}
> +
> +static void print_message(int port, bool is_cmsg, u8 msg, bool recv)
> +{
> +	pr_info("sink port %d: %s message %s %s\n", port,
> +				is_cmsg ? "Control" : "Data",
> +				msg_to_string(is_cmsg, msg),
> +		 recv ? "received" : "sent(wait GOODCRC)");

looks like a debugging message to me. We don't want to spam dmesg with
every single message transmission.

> +static void start_timer(struct pd_sink_port *port, int timeout,
> +		enum hrtimer_restart (*f)(struct hrtimer *))
> +{
> +	if (hrtimer_active(&port->tx_timer)) {
> +		pr_err("Error: previous timer is still active\n");
> +		return;
> +	}
> +
> +	port->tx_timer.function = f;
> +	/* timeout comes with ms but ktime_set takes seconds and nanoseconds */
> +	hrtimer_start(&port->tx_timer, ktime_set(timeout / 1000,

I don't think you need HR timers here. A normal mod_timer() should do.

> +static enum hrtimer_restart goodcrc_timeout(struct hrtimer *timer)
> +{
> +	pr_err("GOODCRC message is not received in %d ms: timeout\n",
> +						PD_TIMEOUT_GOODCRC);
> +	return HRTIMER_NORESTART;
> +}
> +
> +/*
> + * For any message we send, we must get a GOODCRC message from the Source.
> + * The USB PD spec says the time should be measured between the last bit
> + * of the sending message's EOP has been transmitted and the last bit of
> + * the receiving GOODCRC message's EOP has been received. The allowed time
> + * is minimal 0.9 ms and maximal 1.1 ms. However, this measurement is
> + * performed in physical layer. When it reaches to the OS and this driver,
> + * the actual time is difficult to predict because of the scheduling,
> + * context switch, interrupt preemption and nesting, etc. So we only define
> + * a safe timeout value (PD_TIMEOUT_GOODCRC) which is large enough to take
> + * account of all software related latency.

that's true. But here's the thing. From 1ms to 500ms there are two
orders of magnitude. If we take that long to realize we received
GoodCRC, it's already too late. Remember that if we don't e.g. request
power within 30ms after reception of GoodCRC, Source will issue a
HardReset.

> + */
> +static int send_message(struct pd_sink_port *port, void *buf, int len,
> +			u8 msg, bool ctrl_msg, enum sop_type sop_type)
> +{
> +	if (ctrl_msg && msg == PD_CMSG_GOODCRC) {
> +		port->tx(port->port, port->tx_priv, buf, len, sop_type);
> +		print_message(port->port, true, msg, false);
> +		return 0;
> +	}
> +
> +	port->tx(port->port, port->tx_priv, buf, len, sop_type);
> +	print_message(port->port, ctrl_msg, msg, false);
> +
> +	if (!port->hw_goodcrc_rx) {
> +		port->waiting_goodcrc = true;
> +		start_timer(port, PD_TIMEOUT_GOODCRC, goodcrc_timeout);

Every port waits for goodcrc, it's just that some will receive it and
process it in SW, while others will just notify of its reception.

> +int pd_sink_queue_msg(struct pd_sink_msg *msg)
> +{
> +	unsigned long flags;
> +	struct pd_sink_port *port;
> +
> +	if (msg->port < 0 || msg->port >= MAX_NR_SINK_PORTS) {
> +		pr_err("Invalid port number\n");
> +		return -EINVAL;
> +	}
> +
> +	port = sink_ports[msg->port];
> +
> +	spin_lock_irqsave(&port->rx_lock, flags);
> +	list_add_tail(&msg->list, &port->rx_list);
> +	spin_unlock_irqrestore(&port->rx_lock, flags);
> +
> +	queue_work(port->rx_wq, &port->rx_work);

can we really queue several messages at a time? It seems unfeasible to
me. It's not like we can queue several power request in a role. Why do
you need this workqueue? Why don't you process message here, in place?

-- 
balbi

[toc] | [next] | [standalone]

#1444074

On Fri, 2016-07-15 at 10:25 +0300, Felipe Balbi wrote:
> > +int pd_sink_queue_msg(struct pd_sink_msg *msg)
> > +{
> > +     unsigned long flags;
> > +     struct pd_sink_port *port;
> > +
> > +     if (msg->port < 0 || msg->port >= MAX_NR_SINK_PORTS) {
> > +             pr_err("Invalid port number\n");
> > +             return -EINVAL;
> > +     }
> > +
> > +     port = sink_ports[msg->port];
> > +
> > +     spin_lock_irqsave(&port->rx_lock, flags);
> > +     list_add_tail(&msg->list, &port->rx_list);
> > +     spin_unlock_irqrestore(&port->rx_lock, flags);
> > +
> > +     queue_work(port->rx_wq, &port->rx_work);
> 
> can we really queue several messages at a time? It seems unfeasible to
> me. It's not like we can queue several power request in a role. Why do
> you need this workqueue? Why don't you process message here, in place?

A reset can come at any time.

	Regards
		Oliver

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

#1444160

[Multipart message — attachments visible in raw view] — view raw

Hi,

Oliver Neukum <oneukum@suse.com> writes:
> On Fri, 2016-07-15 at 10:25 +0300, Felipe Balbi wrote:
>> > +int pd_sink_queue_msg(struct pd_sink_msg *msg)
>> > +{
>> > +     unsigned long flags;
>> > +     struct pd_sink_port *port;
>> > +
>> > +     if (msg->port < 0 || msg->port >= MAX_NR_SINK_PORTS) {
>> > +             pr_err("Invalid port number\n");
>> > +             return -EINVAL;
>> > +     }
>> > +
>> > +     port = sink_ports[msg->port];
>> > +
>> > +     spin_lock_irqsave(&port->rx_lock, flags);
>> > +     list_add_tail(&msg->list, &port->rx_list);
>> > +     spin_unlock_irqrestore(&port->rx_lock, flags);
>> > +
>> > +     queue_work(port->rx_wq, &port->rx_work);
>> 
>> can we really queue several messages at a time? It seems unfeasible to
>> me. It's not like we can queue several power request in a role. Why do
>> you need this workqueue? Why don't you process message here, in place?
>
> A reset can come at any time.

right, but that's not how this is being used. IMHO, rx_work is a
misnomer. If you look at how typec_wcove (patch 2 in this series) uses
it, you'll see that pd_sink_queue_msg() is called to queue a reply to a
message that was *already* received. We can't have two replies, right?

In any case, this is a minor problem.

-- 
balbi

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

#1444309

[Multipart message — attachments visible in raw view] — view raw

Hi again,

Felipe Balbi <felipe.balbi@linux.intel.com> writes:
> Oliver Neukum <oneukum@suse.com> writes:
>> On Fri, 2016-07-15 at 10:25 +0300, Felipe Balbi wrote:
>>> > +int pd_sink_queue_msg(struct pd_sink_msg *msg)
>>> > +{
>>> > +     unsigned long flags;
>>> > +     struct pd_sink_port *port;
>>> > +
>>> > +     if (msg->port < 0 || msg->port >= MAX_NR_SINK_PORTS) {
>>> > +             pr_err("Invalid port number\n");
>>> > +             return -EINVAL;
>>> > +     }
>>> > +
>>> > +     port = sink_ports[msg->port];
>>> > +
>>> > +     spin_lock_irqsave(&port->rx_lock, flags);
>>> > +     list_add_tail(&msg->list, &port->rx_list);
>>> > +     spin_unlock_irqrestore(&port->rx_lock, flags);
>>> > +
>>> > +     queue_work(port->rx_wq, &port->rx_work);
>>> 
>>> can we really queue several messages at a time? It seems unfeasible to
>>> me. It's not like we can queue several power request in a role. Why do
>>> you need this workqueue? Why don't you process message here, in place?
>>
>> A reset can come at any time.
>
> right, but that's not how this is being used. IMHO, rx_work is a
> misnomer. If you look at how typec_wcove (patch 2 in this series) uses
> it, you'll see that pd_sink_queue_msg() is called to queue a reply to a
> message that was *already* received. We can't have two replies, right?
>
> In any case, this is a minor problem.

oh wait, it's not a minor problem. If CPU is busy, this workqueue might
take longer than 30ms to get scheduled. This is another problem I just
reproduced, even after changing that pr_info() in print_message() to a
pr_debug().

Everything worked fine when I called rx_msg_worker() directly, instead
of queueing it to the workqueue.

-- 
balbi

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

#1444673

On Fri, Jul 15, 2016 at 10:25:36AM +0300, Felipe Balbi wrote:
> Bin Gao <bin.gao@linux.intel.com> writes:
> 
> > This patch implements a simple USB Power Delivery sink port state machine.
> > It assumes the hardware only handles PD packet transmitting and receiving
> > over the CC line of the USB Type-C connector. The state transition is
> > completely controlled by software. This patch only implement the sink port
> > function and it doesn't support source port and port swap yet.
> >
> > This patch depends on these two patches:
> > https://lkml.org/lkml/2016/6/29/349
> > https://lkml.org/lkml/2016/6/29/350
> >
> > Signed-off-by: Bin Gao <bin.gao@intel.com>
> > ---
> >  drivers/usb/typec/Kconfig      |  13 +
> >  drivers/usb/typec/Makefile     |   1 +
> >  drivers/usb/typec/pd_sink.c    | 967 +++++++++++++++++++++++++++++++++++++++++
> >  include/linux/usb/pd_message.h | 371 ++++++++++++++++
> >  include/linux/usb/pd_sink.h    | 286 ++++++++++++
> >  5 files changed, 1638 insertions(+)
> >  create mode 100644 drivers/usb/typec/pd_sink.c
> >  create mode 100644 include/linux/usb/pd_message.h
> >  create mode 100644 include/linux/usb/pd_sink.h
> >
> > diff --git a/drivers/usb/typec/Kconfig b/drivers/usb/typec/Kconfig
> > index 7a345a4..a04a900 100644
> > --- a/drivers/usb/typec/Kconfig
> > +++ b/drivers/usb/typec/Kconfig
> > @@ -4,12 +4,25 @@ menu "USB PD and Type-C drivers"
> >  config TYPEC
> >  	tristate
> >  
> > +config USB_PD_SINK
> > +	bool "USB Power Delivery Sink Port State Machine Driver"
> 
> tristate?
> 
> > +	select TYPEC
> 
> this should depend on TYPEC, not select it.
> 
> > +	help
> > +	  Enable this to support USB PD(Power Delivery) Sink port.
> > +	  This driver implements a simple USB PD sink state machine.
> > +	  The underlying TypeC phy driver is responsible for cable
> > +	  plug/unplug event, port orientation detection, transmitting
> > +	  and receiving PD messages. This driver only process messages
> > +	  received by the TypeC phy driver and maintain the sink port's
> > +	  state machine.
> > +
> >  config TYPEC_WCOVE
> >  	tristate "Intel WhiskeyCove PMIC USB Type-C PHY driver"
> >  	depends on ACPI
> >  	depends on INTEL_SOC_PMIC
> >  	depends on INTEL_PMC_IPC
> >  	select TYPEC
> > +	select USB_PD_SINK
> 
> TYPEC without PD is valid, let user select PD support.
Yes will fix the Kconfig in next revision.

> > +static void print_message(int port, bool is_cmsg, u8 msg, bool recv)
> > +{
> > +	pr_info("sink port %d: %s message %s %s\n", port,
> > +				is_cmsg ? "Control" : "Data",
> > +				msg_to_string(is_cmsg, msg),
> > +		 recv ? "received" : "sent(wait GOODCRC)");
> 
> looks like a debugging message to me. We don't want to spam dmesg with
> every single message transmission.
This should be a pr_debug().

> 
> > +static void start_timer(struct pd_sink_port *port, int timeout,
> > +		enum hrtimer_restart (*f)(struct hrtimer *))
> > +{
> > +	if (hrtimer_active(&port->tx_timer)) {
> > +		pr_err("Error: previous timer is still active\n");
> > +		return;
> > +	}
> > +
> > +	port->tx_timer.function = f;
> > +	/* timeout comes with ms but ktime_set takes seconds and nanoseconds */
> > +	hrtimer_start(&port->tx_timer, ktime_set(timeout / 1000,
> 
> I don't think you need HR timers here. A normal mod_timer() should do.
When hrtimer is in place, the old timer becomes "legacy". And the old timer
APIs are implemented on top of hrtimer. It's no harm to use hrtimers anywhere
in the kernel and it would be encouraged in my opinion:-)

> 
> > +static enum hrtimer_restart goodcrc_timeout(struct hrtimer *timer)
> > +{
> > +	pr_err("GOODCRC message is not received in %d ms: timeout\n",
> > +						PD_TIMEOUT_GOODCRC);
> > +	return HRTIMER_NORESTART;
> > +}
> > +
> > +/*
> > + * For any message we send, we must get a GOODCRC message from the Source.
> > + * The USB PD spec says the time should be measured between the last bit
> > + * of the sending message's EOP has been transmitted and the last bit of
> > + * the receiving GOODCRC message's EOP has been received. The allowed time
> > + * is minimal 0.9 ms and maximal 1.1 ms. However, this measurement is
> > + * performed in physical layer. When it reaches to the OS and this driver,
> > + * the actual time is difficult to predict because of the scheduling,
> > + * context switch, interrupt preemption and nesting, etc. So we only define
> > + * a safe timeout value (PD_TIMEOUT_GOODCRC) which is large enough to take
> > + * account of all software related latency.
> 
> that's true. But here's the thing. From 1ms to 500ms there are two
> orders of magnitude. If we take that long to realize we received
> GoodCRC, it's already too late. Remember that if we don't e.g. request
> power within 30ms after reception of GoodCRC, Source will issue a
> HardReset.
This part is for sure to be revisited even in the beginning of the design.
The hardware(Type-C PHY) we tested on forces auto GOODCRC transmission
and reception and doesn't report to software so we actually have no way
to test it. But any way this timeout value will be rivisited. 

> 
> > + */
> > +static int send_message(struct pd_sink_port *port, void *buf, int len,
> > +			u8 msg, bool ctrl_msg, enum sop_type sop_type)
> > +{
> > +	if (ctrl_msg && msg == PD_CMSG_GOODCRC) {
> > +		port->tx(port->port, port->tx_priv, buf, len, sop_type);
> > +		print_message(port->port, true, msg, false);
> > +		return 0;
> > +	}
> > +
> > +	port->tx(port->port, port->tx_priv, buf, len, sop_type);
> > +	print_message(port->port, ctrl_msg, msg, false);
> > +
> > +	if (!port->hw_goodcrc_rx) {
> > +		port->waiting_goodcrc = true;
> > +		start_timer(port, PD_TIMEOUT_GOODCRC, goodcrc_timeout);
> 
> Every port waits for goodcrc, it's just that some will receive it and
> process it in SW, while others will just notify of its reception.
> 
> > +int pd_sink_queue_msg(struct pd_sink_msg *msg)
> > +{
> > +	unsigned long flags;
> > +	struct pd_sink_port *port;
> > +
> > +	if (msg->port < 0 || msg->port >= MAX_NR_SINK_PORTS) {
> > +		pr_err("Invalid port number\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	port = sink_ports[msg->port];
> > +
> > +	spin_lock_irqsave(&port->rx_lock, flags);
> > +	list_add_tail(&msg->list, &port->rx_list);
> > +	spin_unlock_irqrestore(&port->rx_lock, flags);
> > +
> > +	queue_work(port->rx_wq, &port->rx_work);
> 
> can we really queue several messages at a time? It seems unfeasible to
> me. It's not like we can queue several power request in a role. Why do
> you need this workqueue? Why don't you process message here, in place?
Some Type-C chargers send two messages in a short duration(less than 1 ms),
e.g. a SOURCE_CAPABILITY follows the previous SOURCE_CAPABILITY, or a
GET_SINK_CAPABILITY follows a previous SOURCE_CAPABILITY, etc. Queuing
message to PD stack by Type-C phy driver typically happens in a interrupt
context. So in this case a nested interrupt may happen. Our whole PD
stack while processing one message is not re-entrant so the nested
interrupt would cause a problem.
But I'll re-check and see if it will really happen.
Yes I agree the workqueue will introduce delay so we could miss the
tSenderResponse timeout deadline.
Most likey I'll remove the workqueue and directly process message one
bye one in the next revision.

> 
> -- 
> balbi
Thanks for your review.

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

#1445256

[Multipart message — attachments visible in raw view] — view raw

Hi,

Bin Gao <bin.gao@linux.intel.com> writes:
>> > +int pd_sink_queue_msg(struct pd_sink_msg *msg)
>> > +{
>> > +	unsigned long flags;
>> > +	struct pd_sink_port *port;
>> > +
>> > +	if (msg->port < 0 || msg->port >= MAX_NR_SINK_PORTS) {
>> > +		pr_err("Invalid port number\n");
>> > +		return -EINVAL;
>> > +	}
>> > +
>> > +	port = sink_ports[msg->port];
>> > +
>> > +	spin_lock_irqsave(&port->rx_lock, flags);
>> > +	list_add_tail(&msg->list, &port->rx_list);
>> > +	spin_unlock_irqrestore(&port->rx_lock, flags);
>> > +
>> > +	queue_work(port->rx_wq, &port->rx_work);
>> 
>> can we really queue several messages at a time? It seems unfeasible to
>> me. It's not like we can queue several power request in a role. Why do
>> you need this workqueue? Why don't you process message here, in place?
> Some Type-C chargers send two messages in a short duration(less than 1 ms),
> e.g. a SOURCE_CAPABILITY follows the previous SOURCE_CAPABILITY, or a
> GET_SINK_CAPABILITY follows a previous SOURCE_CAPABILITY, etc. Queuing
> message to PD stack by Type-C phy driver typically happens in a interrupt
> context. So in this case a nested interrupt may happen. Our whole PD
> stack while processing one message is not re-entrant so the nested
> interrupt would cause a problem.

keep interrupts masked for as long as necessary until your message is
processed.

-- 
balbi

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

#1446041

On Mon, Jul 18, 2016 at 10:07:24AM +0300, Felipe Balbi wrote:
> 
> Hi,
> 
> Bin Gao <bin.gao@linux.intel.com> writes:
> >> > +int pd_sink_queue_msg(struct pd_sink_msg *msg)
> >> > +{
> >> > +	unsigned long flags;
> >> > +	struct pd_sink_port *port;
> >> > +
> >> > +	if (msg->port < 0 || msg->port >= MAX_NR_SINK_PORTS) {
> >> > +		pr_err("Invalid port number\n");
> >> > +		return -EINVAL;
> >> > +	}
> >> > +
> >> > +	port = sink_ports[msg->port];
> >> > +
> >> > +	spin_lock_irqsave(&port->rx_lock, flags);
> >> > +	list_add_tail(&msg->list, &port->rx_list);
> >> > +	spin_unlock_irqrestore(&port->rx_lock, flags);
> >> > +
> >> > +	queue_work(port->rx_wq, &port->rx_work);
> >> 
> >> can we really queue several messages at a time? It seems unfeasible to
> >> me. It's not like we can queue several power request in a role. Why do
> >> you need this workqueue? Why don't you process message here, in place?
> > Some Type-C chargers send two messages in a short duration(less than 1 ms),
> > e.g. a SOURCE_CAPABILITY follows the previous SOURCE_CAPABILITY, or a
> > GET_SINK_CAPABILITY follows a previous SOURCE_CAPABILITY, etc. Queuing
> > message to PD stack by Type-C phy driver typically happens in a interrupt
> > context. So in this case a nested interrupt may happen. Our whole PD
> > stack while processing one message is not re-entrant so the nested
> > interrupt would cause a problem.
> 
> keep interrupts masked for as long as necessary until your message is
> processed.

Yes, that's a right way to go. 
We'll have to document this because there might be other Type-C
PHY drivers(other than Intel Whiskey Cove PMIC) to use the PD stack.

> 
> -- 
> balbi

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

#1446246

[Multipart message — attachments visible in raw view] — view raw

Hi,

Bin Gao <bin.gao@linux.intel.com> writes:
> On Mon, Jul 18, 2016 at 10:07:24AM +0300, Felipe Balbi wrote:
>> 
>> Hi,
>> 
>> Bin Gao <bin.gao@linux.intel.com> writes:
>> >> > +int pd_sink_queue_msg(struct pd_sink_msg *msg)
>> >> > +{
>> >> > +	unsigned long flags;
>> >> > +	struct pd_sink_port *port;
>> >> > +
>> >> > +	if (msg->port < 0 || msg->port >= MAX_NR_SINK_PORTS) {
>> >> > +		pr_err("Invalid port number\n");
>> >> > +		return -EINVAL;
>> >> > +	}
>> >> > +
>> >> > +	port = sink_ports[msg->port];
>> >> > +
>> >> > +	spin_lock_irqsave(&port->rx_lock, flags);
>> >> > +	list_add_tail(&msg->list, &port->rx_list);
>> >> > +	spin_unlock_irqrestore(&port->rx_lock, flags);
>> >> > +
>> >> > +	queue_work(port->rx_wq, &port->rx_work);
>> >> 
>> >> can we really queue several messages at a time? It seems unfeasible to
>> >> me. It's not like we can queue several power request in a role. Why do
>> >> you need this workqueue? Why don't you process message here, in place?
>> > Some Type-C chargers send two messages in a short duration(less than 1 ms),
>> > e.g. a SOURCE_CAPABILITY follows the previous SOURCE_CAPABILITY, or a
>> > GET_SINK_CAPABILITY follows a previous SOURCE_CAPABILITY, etc. Queuing
>> > message to PD stack by Type-C phy driver typically happens in a interrupt
>> > context. So in this case a nested interrupt may happen. Our whole PD
>> > stack while processing one message is not re-entrant so the nested
>> > interrupt would cause a problem.
>> 
>> keep interrupts masked for as long as necessary until your message is
>> processed.
>
> Yes, that's a right way to go. 
> We'll have to document this because there might be other Type-C
> PHY drivers(other than Intel Whiskey Cove PMIC) to use the PD stack.

that's a requirement for *any* driver. You _must_ keep interrupts masked
(or disabled) while you're processing the interrupts themselves.

-- 
balbi

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web