RE: [PATCH] usb: dwc2: gadget: Fix issue in dwc2_gadget_start_isoc()
Hi Minas:
>-Original Message-
>From: linux-usb-ow...@vger.kernel.org
>[mailto:linux-usb-ow...@vger.kernel.org] On Behalf Of Minas Harutyunyan
>Sent: Tuesday, June 12, 2018 4:37 PM
>To: Felipe Balbi ; Greg Kroah-Hartman
>; Minas Harutyunyan
>; linux-usb@vger.kernel.org
>Cc: John Youn
>Subject: [PATCH] usb: dwc2: gadget: Fix issue in dwc2_gadget_start_isoc()
>
>In case of requests queue is empty reset EP target_frame to initial value.
>
>This allow restarting ISOC traffic in case when function driver queued
>requests with interruptions.
>
>Signed-off-by: Minas Harutyunyan
>---
> drivers/usb/dwc2/gadget.c | 1 +
> 1 file changed, 1 insertion(+)
>
>diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c index
>7a5e265f48f7..24d23025be77 100644
>--- a/drivers/usb/dwc2/gadget.c
>+++ b/drivers/usb/dwc2/gadget.c
>@@ -891,6 +891,7 @@ static void dwc2_gadget_start_isoc_ddma(struct
>dwc2_hsotg_ep *hs_ep)
> struct dwc2_dma_desc *desc;
>
> if (list_empty(&hs_ep->queue)) {
>+ hs_ep->target_frame = TARGET_FRAME_INITIAL;
> dev_dbg(hsotg->dev, "%s: No requests in queue\n", __func__);
> return;
> }
>--
Tested-by: Zeng Tao
I have tested it on my platform using webcam gadget application.
Regards
Zengtao
>2.11.0
>
>--
>To unsubscribe from this list: send the line "unsubscribe linux-usb" in the
>body of a message to majord...@vger.kernel.org More majordomo info at
>http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] USB: cdc-wdm: don't enable interrupts in USB-giveback
On 2018-06-13 19:43:55 [+0200], Oliver Neukum wrote:
> On Mi, 2018-06-13 at 18:28 +0200, Sebastian Andrzej Siewior wrote:
> > The function service_outstanding_interrupt() will unconditionally enable
> > interrupts during unlock and invoke usb_submit_urb() with GFP_KERNEL.
> > If the HCD completes in BH (like ehci does) then the context remains
> > atomic due local_bh_disable() and enabling interrupts does not change
> > this.
>
> Hi,
Hi Oliver,
> I am just looking at your patch and I am wondering why
> wdm_in_callback() won't just call service_outstanding_interrupt()
> again and again? OK, maybe I am dense and it may well be present now,
> but it just looks to me that way.
But this part didn't change, did it? The user blocks in wdmw_read()
waiting for the WDM_READ to be set and a wakeup. After rhe wakeup it
clears the ->rerr.
| static ssize_t wdm_read
| (struct file *file, char __user *buffer, size_t count, loff_t *ppos)
| {
|…
|rv = wait_event_interruptible(desc->wait,
| test_bit(WDM_READ, &desc->flags));
|…
|
|if (desc->rerr) { /* read completed, error happened */
| rv = usb_translate_errors(desc->rerr);
| desc->rerr = 0;
| spin_unlock_irq(&desc->iuspin);
| goto err;
| }
Maybe we should delay the WDM_READ flag in the error case until the
worker is done (before the wakeup).
> Regards
> Oliver
Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] USB: cdc-wdm: don't enable interrupts in USB-giveback
On Mi, 2018-06-13 at 18:28 +0200, Sebastian Andrzej Siewior wrote: > The function service_outstanding_interrupt() will unconditionally enable > interrupts during unlock and invoke usb_submit_urb() with GFP_KERNEL. > If the HCD completes in BH (like ehci does) then the context remains > atomic due local_bh_disable() and enabling interrupts does not change > this. Hi, I am just looking at your patch and I am wondering why wdm_in_callback() won't just call service_outstanding_interrupt() again and again? OK, maybe I am dense and it may well be present now, but it just looks to me that way. Regards Oliver -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2] USB: cdc-wdm: don't enable interrupts in USB-giveback
In the code path
__usb_hcd_giveback_urb()
-> wdm_in_callback()
-> service_outstanding_interrupt()
The function service_outstanding_interrupt() will unconditionally enable
interrupts during unlock and invoke usb_submit_urb() with GFP_KERNEL.
If the HCD completes in BH (like ehci does) then the context remains
atomic due local_bh_disable() and enabling interrupts does not change
this.
Defer the error case handling to a workqueue as suggested by Oliver
Neukum. In case of an error the worker performs the read out and wakes
the user.
Fixes: c1da59dad0eb ("cdc-wdm: Clear read pipeline in case of error")
Cc: Robert Foss
Cc: Oliver Neukum
Signed-off-by: Sebastian Andrzej Siewior
---
On 2018-06-13 10:30:18 [+0200], Oliver Neukum wrote:
> It seems to me that the core of the problem is handling an error
> in irq context potentially. How about shifting it to a work queue?
Something like this then?
drivers/usb/class/cdc-wdm.c | 25 +++--
1 file changed, 19 insertions(+), 6 deletions(-)
diff --git a/drivers/usb/class/cdc-wdm.c b/drivers/usb/class/cdc-wdm.c
index a0d284ef3f40..ddf55f93d4ca 100644
--- a/drivers/usb/class/cdc-wdm.c
+++ b/drivers/usb/class/cdc-wdm.c
@@ -96,6 +96,7 @@ struct wdm_device {
struct mutexrlock;
wait_queue_head_t wait;
struct work_struct rxwork;
+ struct work_struct service_outs_intr;
int werr;
int rerr;
int resp_count;
@@ -151,9 +152,6 @@ static void wdm_out_callback(struct urb *urb)
wake_up(&desc->wait);
}
-/* forward declaration */
-static int service_outstanding_interrupt(struct wdm_device *desc);
-
static void wdm_in_callback(struct urb *urb)
{
struct wdm_device *desc = urb->context;
@@ -210,7 +208,6 @@ static void wdm_in_callback(struct urb *urb)
}
skip_error:
set_bit(WDM_READ, &desc->flags);
- wake_up(&desc->wait);
if (desc->rerr) {
/*
@@ -219,9 +216,10 @@ static void wdm_in_callback(struct urb *urb)
* We should respond to further attempts from the device to send
* data, so that we can get unstuck.
*/
- service_outstanding_interrupt(desc);
+ schedule_work(&desc->service_outs_intr);
+ } else {
+ wake_up(&desc->wait);
}
-
spin_unlock(&desc->iuspin);
}
@@ -758,6 +756,18 @@ static void wdm_rxwork(struct work_struct *work)
}
}
+static void service_interrupt_work(struct work_struct *work)
+{
+ struct wdm_device *desc;
+
+ desc = container_of(work, struct wdm_device, service_outs_intr);
+
+ spin_lock_irq(&desc->iuspin);
+ service_outstanding_interrupt(desc);
+ wake_up(&desc->wait);
+ spin_unlock_irq(&desc->iuspin);
+}
+
/* --- hotplug --- */
static int wdm_create(struct usb_interface *intf, struct
usb_endpoint_descriptor *ep,
@@ -779,6 +789,7 @@ static int wdm_create(struct usb_interface *intf, struct
usb_endpoint_descriptor
desc->inum =
cpu_to_le16((u16)intf->cur_altsetting->desc.bInterfaceNumber);
desc->intf = intf;
INIT_WORK(&desc->rxwork, wdm_rxwork);
+ INIT_WORK(&desc->service_outs_intr, service_interrupt_work);
rv = -EINVAL;
if (!usb_endpoint_is_int_in(ep))
@@ -964,6 +975,7 @@ static void wdm_disconnect(struct usb_interface *intf)
mutex_lock(&desc->wlock);
kill_urbs(desc);
cancel_work_sync(&desc->rxwork);
+ cancel_work_sync(&desc->service_outs_intr);
mutex_unlock(&desc->wlock);
mutex_unlock(&desc->rlock);
@@ -1065,6 +1077,7 @@ static int wdm_pre_reset(struct usb_interface *intf)
mutex_lock(&desc->wlock);
kill_urbs(desc);
cancel_work_sync(&desc->rxwork);
+ cancel_work_sync(&desc->service_outs_intr);
return 0;
}
--
2.17.1
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: USB role switches, usb-connector, typec and device trees
On Tue, Jun 12, 2018 at 07:16:03PM +0200, Mats Karrman wrote: > On 2018-06-07 14:01, Heikki Krogerus wrote: > > > On Thu, Jun 07, 2018 at 09:22:56AM +0200, Hans de Goede wrote: > > > Hi, > > > > > > On 06-06-18 23:36, Mats Karrman wrote: > > > > Hello Gentlemen, > > > > > > > > I'm trying to get my head around USB role switches in connection with > > > > Type-C ports > > > > and device-trees. So far I have not found much documentation, e.g. > > > > there are no > > > > device-tree bindings documented and really no good examples in existing > > > > device > > > > trees, although there has been some attempts, e.g. [1] and [2]. Anyway, > > > > so I send > > > > you a couple of questions instead: > > > > > > > > 1) tcpm uses the port device struct to find a single usb_role_switch > > > > but there is > > > > room for three USB busses in the Type-C connector; one high speed and > > > > two (?) super- > > > > speed. These would not all come from the same controller (there might > > > > even be > > > > separate controllers for host and device mode for each bus). > > I believe USB 3.2 spec in practice says that the two superspeed > > "lanes" must to go to the same controller. Only one will be used for > > link training etc. The second one is pulled in after certain state of > > enumeration has been passed. > > > > So we may theoretically have two controllers to deal with, one for > > USB2 and another for USB3, but not three. > > > > But in any case, let's not try to fix theoretical problems that may > > never exist. > > > > > AFAIK the 2nd superspeed USB bus is never used as such. There really is > > > only 1 > > > USB bus on the Type-C connector, the combined USB-2 + the 1st superspeed > > > bus, > > > physically these are 2 separate busses but that is purely for > > > compatibility > > > reasons, logically there really only is 1 bus, just like a superspeed > > > Type-A > > > connector has both busses physically but logically represents a single > > > bus/port. > > > > > > > The case I am working on now only have a single USB2 otg controller so > > > > it should > > > > be possible to make that driver register a role switch but for other > > > > cases? > > > I guess theoretically a device could use separate role switches / muxes > > > for > > > the USB-2 and USB-3 busses, but that would be weird. So lets cross that > > > bridge > > > when we reach it. > > > > > > > I imagine it would be possible to create a composite driver as a proxy > > > > for all role > > > > switches but that would probably be different for every > > > > platform/product - not > > > > very elegant. Could the role switch infrastructure be extended to > > > > handle arbitrary > > > > sets of coordinated switches? > > > As said lets cross that bridge when we reach it. > > Agreed. > > OK, I'm buying what you're saying. After reading some manuals for USB3 > controllers > and USB3 equipped SoC's I realize that most USB3 controllers seem to also > support > USB2. Makes things easier, although not as flexible as I imagined. > One thing that led me astray was the comment for usb_role_switch_register() > and the > separate device struct pointers for usb2/usb3/udc in the usb_role_switch_desc > struct. > Still not totally clear to me what they are for? They are not really used for anything. Well, not yet at least. Even if there is only a single physical controller, in case of host, there will be separate logical devices representing the USB2 bus the USB3 bus. UDC is just UDC, the device is the physical controller. I added those usb2/usb3/udc handles to the structure because a long time a go somebody requested that we should have a way in user space to see the USB buses we are dealing with somehow. So the idea was never to actually use those in the code. That's why I'm talking about symlinks in the TODO comment. But if those are not useful, or confusing, I'm happy to drop them. > > > > 2) How should the connection between the Type-C port and the switches > > > > best be > > > > expressed in a device tree? Using graph I presume, but should it be > > > > mixed into the > > > > existing "usb-connector" or should this be a separate block? > > I don't know? > > > > I'm not super comfortable proposing things for these bindings because > > my knowledge of DT is a bit limited. I mostly work with ACPI based > > platforms. > > > > I'm not even completely sure device graph is usable in our case, > > though I pretty sure it is. > > > > > > I think it is unfortunate that the graph use numeric addresses that > > > > need to be > > > > fixed by documentation and already I see problems with the current > > > > assignment > > > > (0=HS, 1=SS, 2=SBU), e.g. if the host and device mode are handled by > > > > different > > > > controllers. Graph do support multiple endpoints for one port but then > > > > we have > > > > another level of magic numbers which does not exactly make things easier > > > > (e.g. 0=dual or host controll
Re: [PATCH v3] storage: Widen bcdDevice range for SanDisk SDDR-31 quirk
The SanDisk SDDR-31 needs the US_FL_FIX_CAPACITY quirk. Previously that was only applied for bcdDevice 0x0009, but later firmware which reports bcdDevice 0x0022 needs it too. Signed-off-by: Mark Knibbs --- v4: Max. bcdDevice is 0x0022 again, removed extraneous text v3: Fixed tabs, but accidentally reverted max. bcdDevice to 0x00FF v2: Changed maximum bcdDevice to 0x0022 diff --git a/drivers/usb/storage/unusual_devs.h b/drivers/usb/storage/unusual_devs.h index 747d3a9..dfcceaf 100644 --- a/drivers/usb/storage/unusual_devs.h +++ b/drivers/usb/storage/unusual_devs.h @@ -1044,7 +1044,7 @@ UNUSUAL_DEV( 0x0781, 0x0001, 0x0200, 0x0200, USB_SC_SCSI, USB_PR_CB, NULL, US_FL_SINGLE_LUN ), -UNUSUAL_DEV( 0x0781, 0x0002, 0x0009, 0x0009, +UNUSUAL_DEV( 0x0781, 0x0002, 0x, 0x0022, "SanDisk Corporation", "ImageMate CompactFlash USB", USB_SC_DEVICE, USB_PR_DEVICE, NULL, -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v6 12/15] usb: typec: tcpm: set cc for drp toggling attach
Hi
> -Original Message-
> From: Jun Li
> Sent: 2018年6月13日 19:07
> To: Guenter Roeck ; Heikki Krogerus
> ; shufan_...@richtek.com
> Cc: robh...@kernel.org; gre...@linuxfoundation.org; cw00.c...@samsung.com;
> a.ha...@samsung.com; Peter Chen ;
> garsi...@embeddedor.com; gso...@gmail.com; linux-usb@vger.kernel.org;
> devicet...@vger.kernel.org; dl-linux-imx
> Subject: RE: [PATCH v6 12/15] usb: typec: tcpm: set cc for drp toggling attach
>
> Hi,
> > -Original Message-
> > From: Guenter Roeck [mailto:groe...@gmail.com] On Behalf Of Guenter
> > Roeck
> > Sent: 2018年6月11日 21:35
> > To: Heikki Krogerus ; Jun Li
> >
> > Cc: robh...@kernel.org; gre...@linuxfoundation.org;
> > cw00.c...@samsung.com; a.ha...@samsung.com; shufan_...@richtek.com;
> > Peter Chen ; garsi...@embeddedor.com;
> > gso...@gmail.com; linux-usb@vger.kernel.org;
> > devicet...@vger.kernel.org; dl-linux-imx
> > Subject: Re: [PATCH v6 12/15] usb: typec: tcpm: set cc for drp
> > toggling attach
> >
> > On 06/11/2018 05:29 AM, Heikki Krogerus wrote:
> > > On Mon, May 28, 2018 at 10:52:44AM +0800, Li Jun wrote:
> > >> In case of drp toggling, we may need set correct cc value for role
> > >> control after attach as it may never been set.
> > >
> > > Is this something that should be considered as a fix?
> > >
> >
> > The problem with this patch is that it hides a problem. CC should have
> > been set by the time a port reaches the attached state. The patch
> > means that there is a state machine path where this does not happen.
> > I'd rather understand that path and fix the problem where it happens.
>
> Here is my previous explanation about this state machine path[1]
>
> [1]https://www.spinics.net/lists/linux-usb/msg167467.html
>
> The physical CC line state is correct, just the Role Control register value
> may
> mismatch on some(e.g. shufan's) HW, if the common change in tcpm is not the
> right way, I have to ask shufan to try resolve this in his custom rt1711h
> driver.
>
> I understand the concern of setting cc at attached state is late, should be
> done
> before that, how about I move this in cc_change like below:
>
> @@ -3492,10 +3492,13 @@ static void _tcpm_cc_change(struct tcpm_port *port,
> enum typec_cc_status cc1,
> switch (port->state) {
> case DRP_TOGGLING:
> if (tcpm_port_is_debug(port) || tcpm_port_is_audio(port) ||
> - tcpm_port_is_source(port))
> + tcpm_port_is_source(port)) {
> + tcpm_set_cc(port, tcpm_rp_cc(port));
> tcpm_set_state(port, SRC_ATTACH_WAIT, 0);
> - else if (tcpm_port_is_sink(port))
> + } else if (tcpm_port_is_sink(port)) {
> + if (port->cc_req == TYPEC_CC_OPEN)
Sorry, here should be:
+ } else if (tcpm_port_is_sink(port)) {
+ tcpm_set_cc(port, TYPEC_CC_RD);
tcpm_set_state(port, SNK_ATTACH_WAIT, 0);
+ }
> tcpm_set_state(port, SNK_ATTACH_WAIT, 0);
> + }
> break;
> case SRC_UNATTACHED:
> case ACC_UNATTACHED:
>
> thanks
> Li Jun
>
> >
> > Guenter
> >
> > >> Signed-off-by: Li Jun
> > >> ---
> > >> drivers/usb/typec/tcpm.c | 5 +
> > >> 1 file changed, 5 insertions(+)
> > >>
> > >> diff --git a/drivers/usb/typec/tcpm.c b/drivers/usb/typec/tcpm.c
> > >> index d885bff..98ea916 100644
> > >> --- a/drivers/usb/typec/tcpm.c
> > >> +++ b/drivers/usb/typec/tcpm.c
> > >> @@ -2599,6 +2599,7 @@ static void tcpm_reset_port(struct tcpm_port
> *port)
> > >> tcpm_set_attached_state(port, false);
> > >> port->try_src_count = 0;
> > >> port->try_snk_count = 0;
> > >> +port->cc_req = TYPEC_CC_OPEN;
> > >> port->supply_voltage = 0;
> > >> port->current_limit = 0;
> > >> port->usb_type = POWER_SUPPLY_USB_TYPE_C; @@ -2831,6 +2832,8
> > @@
> > >> static void run_state_machine(struct tcpm_port *port)
> > >> break;
> > >>
> > >> case SRC_ATTACHED:
> > >> +if (port->cc_req == TYPEC_CC_OPEN)
> > >> +tcpm_set_cc(port, tcpm_rp_cc(port));
> > >> ret = tcpm_src_attach(port);
> > >> tcpm_set_state(port, SRC_UNATTACHED,
> > >> ret < 0 ? 0 : PD_T_PS_SOURCE_ON); @@
> > >> -3004,6
> > +3007,8 @@
> > >> static void run_state_machine(struct tcpm_port *port)
> > >> tcpm_set_state(port, SNK_UNATTACHED,
> > PD_T_PD_DEBOUNCE);
> > >> break;
> > >> case SNK_ATTACHED:
> > >> +if (port->cc_req == TYPEC_CC_OPEN)
> > >> +tcpm_set_cc(port, TYPEC_CC_RD);
> > >> ret = tcpm_snk_attach(port);
> > >> if (ret < 0)
> > >> tcpm_set_state(port, SNK_UNATTACHED, 0);
> > >
RE: [PATCH v6 12/15] usb: typec: tcpm: set cc for drp toggling attach
Hi,
> -Original Message-
> From: Guenter Roeck [mailto:groe...@gmail.com] On Behalf Of Guenter Roeck
> Sent: 2018年6月11日 21:35
> To: Heikki Krogerus ; Jun Li
> Cc: robh...@kernel.org; gre...@linuxfoundation.org; cw00.c...@samsung.com;
> a.ha...@samsung.com; shufan_...@richtek.com; Peter Chen
> ; garsi...@embeddedor.com; gso...@gmail.com;
> linux-usb@vger.kernel.org; devicet...@vger.kernel.org; dl-linux-imx
>
> Subject: Re: [PATCH v6 12/15] usb: typec: tcpm: set cc for drp toggling attach
>
> On 06/11/2018 05:29 AM, Heikki Krogerus wrote:
> > On Mon, May 28, 2018 at 10:52:44AM +0800, Li Jun wrote:
> >> In case of drp toggling, we may need set correct cc value for role
> >> control after attach as it may never been set.
> >
> > Is this something that should be considered as a fix?
> >
>
> The problem with this patch is that it hides a problem. CC should have been
> set
> by the time a port reaches the attached state. The patch means that there is a
> state machine path where this does not happen. I'd rather understand that path
> and fix the problem where it happens.
Here is my previous explanation about this state machine path[1]
[1]https://www.spinics.net/lists/linux-usb/msg167467.html
The physical CC line state is correct, just the Role Control register value
may mismatch on some(e.g. shufan's) HW, if the common change in tcpm
is not the right way, I have to ask shufan to try resolve this in his custom
rt1711h driver.
I understand the concern of setting cc at attached state is late, should be
done before that, how about I move this in cc_change like below:
@@ -3492,10 +3492,13 @@ static void _tcpm_cc_change(struct tcpm_port *port,
enum typec_cc_status cc1,
switch (port->state) {
case DRP_TOGGLING:
if (tcpm_port_is_debug(port) || tcpm_port_is_audio(port) ||
- tcpm_port_is_source(port))
+ tcpm_port_is_source(port)) {
+ tcpm_set_cc(port, tcpm_rp_cc(port));
tcpm_set_state(port, SRC_ATTACH_WAIT, 0);
- else if (tcpm_port_is_sink(port))
+ } else if (tcpm_port_is_sink(port)) {
+ if (port->cc_req == TYPEC_CC_OPEN)
tcpm_set_state(port, SNK_ATTACH_WAIT, 0);
+ }
break;
case SRC_UNATTACHED:
case ACC_UNATTACHED:
thanks
Li Jun
>
> Guenter
>
> >> Signed-off-by: Li Jun
> >> ---
> >> drivers/usb/typec/tcpm.c | 5 +
> >> 1 file changed, 5 insertions(+)
> >>
> >> diff --git a/drivers/usb/typec/tcpm.c b/drivers/usb/typec/tcpm.c
> >> index d885bff..98ea916 100644
> >> --- a/drivers/usb/typec/tcpm.c
> >> +++ b/drivers/usb/typec/tcpm.c
> >> @@ -2599,6 +2599,7 @@ static void tcpm_reset_port(struct tcpm_port *port)
> >>tcpm_set_attached_state(port, false);
> >>port->try_src_count = 0;
> >>port->try_snk_count = 0;
> >> + port->cc_req = TYPEC_CC_OPEN;
> >>port->supply_voltage = 0;
> >>port->current_limit = 0;
> >>port->usb_type = POWER_SUPPLY_USB_TYPE_C; @@ -2831,6 +2832,8
> @@
> >> static void run_state_machine(struct tcpm_port *port)
> >>break;
> >>
> >>case SRC_ATTACHED:
> >> + if (port->cc_req == TYPEC_CC_OPEN)
> >> + tcpm_set_cc(port, tcpm_rp_cc(port));
> >>ret = tcpm_src_attach(port);
> >>tcpm_set_state(port, SRC_UNATTACHED,
> >> ret < 0 ? 0 : PD_T_PS_SOURCE_ON); @@ -3004,6
> +3007,8 @@
> >> static void run_state_machine(struct tcpm_port *port)
> >>tcpm_set_state(port, SNK_UNATTACHED,
> PD_T_PD_DEBOUNCE);
> >>break;
> >>case SNK_ATTACHED:
> >> + if (port->cc_req == TYPEC_CC_OPEN)
> >> + tcpm_set_cc(port, TYPEC_CC_RD);
> >>ret = tcpm_snk_attach(port);
> >>if (ret < 0)
> >>tcpm_set_state(port, SNK_UNATTACHED, 0);
> >
N�r��y���b�X��ǧv�^�){.n�+{�^n�r��z���h&�� �G���h�(�階�ݢj"���m�z�ޖ���f���h���~�m�
Re: Regarding usb: gadget: ffs: Fix BUG when userland exits with submitted AIO transfers
Hello, On Sat, 2 Jun 2018 01:02:24 +0300, Sam Protsenko wrote: > Hi Vincent, > > The issue you mentioned in your RFC patch is also reproducing on TI > X15 board (dwc3) with Android master, when doing "adb root" command. > Your patch fixes it just fine, for which I really thankful. Thanks a lot for digging the archives and finding my patch, and happy it could help ! > Do you know is there anything preventing it to be merged? I have X15 > (dwc3) and BeagleBone Black (I guess it has MUSB UDC controller on > it) which I can run some tests on, if it's a blocker for merge. Looking back at the mails I sent, I realise I did not resend the patch *without* an [RFC] subject prefix and without sign-off. My very bad. On the testing side, any testing is very welcome: I could test it on a DWC3 (intel edison) and DWC2 (raspberry pi zero), and have not heard of other tests until your mail. I reapplied my patch over Torvalds' master, cleaned the commit message and added a sign-off. I CC'ed you on the patch submission in case you want to add "tested-by". Regards, -- Vincent Pelletier -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
usb: gadget: ffs: Fix BUG when userland exits with submitted AIO transfers
This bug happens only when the UDC needs to sleep during usb_ep_dequeue,
as is the case for (at least) dwc3.
[ 382.200896] BUG: scheduling while atomic: screen/1808/0x0100
[ 382.207124] 4 locks held by screen/1808:
[ 382.211266] #0: (rcu_callback){}, at: []
rcu_process_callbacks+0x260/0x440
[ 382.219949] #1: (rcu_read_lock_sched){}, at: []
percpu_ref_switch_to_atomic_rcu+0xb0/0x130
[ 382.230034] #2: (&(&ctx->ctx_lock)->rlock){}, at: []
free_ioctx_users+0x23/0xd0
[ 382.230096] #3: (&(&ffs->eps_lock)->rlock){}, at: []
ffs_aio_cancel+0x20/0x60 [usb_f_fs]
[ 382.230160] Modules linked in: usb_f_fs libcomposite configfs bnep btsdio
bluetooth ecdh_generic brcmfmac brcmutil intel_powerclamp coretemp dwc3
kvm_intel ulpi udc_core kvm irqbypass crc32_pclmul crc32c_intel pcbc dwc3_pci
aesni_intel aes_i586 crypto_simd cryptd ehci_pci ehci_hcd gpio_keys usbcore
basincove_gpadc industrialio usb_common
[ 382.230407] CPU: 1 PID: 1808 Comm: screen Not tainted 4.14.0-edison+ #117
[ 382.230416] Hardware name: Intel Corporation Merrifield/BODEGA BAY, BIOS 542
2015.01.21:18.19.48
[ 382.230425] Call Trace:
[ 382.230438]
[ 382.230466] dump_stack+0x47/0x62
[ 382.230498] __schedule_bug+0x61/0x80
[ 382.230522] __schedule+0x43/0x7a0
[ 382.230587] schedule+0x5f/0x70
[ 382.230625] dwc3_gadget_ep_dequeue+0x14c/0x270 [dwc3]
[ 382.230669] ? do_wait_intr_irq+0x70/0x70
[ 382.230724] usb_ep_dequeue+0x19/0x90 [udc_core]
[ 382.230770] ffs_aio_cancel+0x37/0x60 [usb_f_fs]
[ 382.230798] kiocb_cancel+0x31/0x40
[ 382.230822] free_ioctx_users+0x4d/0xd0
[ 382.230858] percpu_ref_switch_to_atomic_rcu+0x10a/0x130
[ 382.230881] ? percpu_ref_exit+0x40/0x40
[ 382.230904] rcu_process_callbacks+0x2b3/0x440
[ 382.230965] __do_softirq+0xf8/0x26b
[ 382.231011] ? __softirqentry_text_start+0x8/0x8
[ 382.231033] do_softirq_own_stack+0x22/0x30
[ 382.231042]
[ 382.231071] irq_exit+0x45/0xc0
[ 382.231089] smp_apic_timer_interrupt+0x13c/0x150
[ 382.231118] apic_timer_interrupt+0x35/0x3c
[ 382.231132] EIP: __copy_user_ll+0xe2/0xf0
[ 382.231142] EFLAGS: 00210293 CPU: 1
[ 382.231154] EAX: bfd4508c EBX: 0004 ECX: 0003 EDX: f3d8fe50
[ 382.231165] ESI: f3d8fe51 EDI: bfd4508d EBP: f3d8fe14 ESP: f3d8fe08
[ 382.231176] DS: 007b ES: 007b FS: 00d8 GS: 0033 SS: 0068
[ 382.231265] core_sys_select+0x25f/0x320
[ 382.231346] ? __wake_up_common_lock+0x62/0x80
[ 382.231399] ? tty_ldisc_deref+0x13/0x20
[ 382.231438] ? ldsem_up_read+0x1b/0x40
[ 382.231459] ? tty_ldisc_deref+0x13/0x20
[ 382.231479] ? tty_write+0x29f/0x2e0
[ 382.231514] ? n_tty_ioctl+0xe0/0xe0
[ 382.231541] ? tty_write_unlock+0x30/0x30
[ 382.231566] ? __vfs_write+0x22/0x110
[ 382.231604] ? security_file_permission+0x2f/0xd0
[ 382.231635] ? rw_verify_area+0xac/0x120
[ 382.231677] ? vfs_write+0x103/0x180
[ 382.231711] SyS_select+0x87/0xc0
[ 382.231739] ? SyS_write+0x42/0x90
[ 382.231781] do_fast_syscall_32+0xd6/0x1a0
[ 382.231836] entry_SYSENTER_32+0x47/0x71
[ 382.231848] EIP: 0xb7f75b05
[ 382.231857] EFLAGS: 0246 CPU: 1
[ 382.231868] EAX: ffda EBX: 0400 ECX: bfd4508c EDX: bfd4510c
[ 382.231878] ESI: EDI: EBP: ESP: bfd45020
[ 382.231889] DS: 007b ES: 007b FS: GS: 0033 SS: 007b
[ 382.232281] softirq: huh, entered softirq 9 RCU c10b4d90 with preempt_count
0100, exited with ?
Signed-off-by: Vincent Pelletier
---
drivers/usb/gadget/function/f_fs.c | 26 ++
1 file changed, 18 insertions(+), 8 deletions(-)
diff --git a/drivers/usb/gadget/function/f_fs.c
b/drivers/usb/gadget/function/f_fs.c
index 199d25700050..01841fdb3144 100644
--- a/drivers/usb/gadget/function/f_fs.c
+++ b/drivers/usb/gadget/function/f_fs.c
@@ -215,6 +215,7 @@ struct ffs_io_data {
struct mm_struct *mm;
struct work_struct work;
+ struct work_struct cancellation_work;
struct usb_ep *ep;
struct usb_request *req;
@@ -1072,22 +1073,31 @@ ffs_epfile_open(struct inode *inode, struct file *file)
return 0;
}
+static void ffs_aio_cancel_worker(struct work_struct *work)
+{
+ struct ffs_io_data *io_data = container_of(work, struct ffs_io_data,
+ cancellation_work);
+
+ ENTER();
+
+ usb_ep_dequeue(io_data->ep, io_data->req);
+}
+
static int ffs_aio_cancel(struct kiocb *kiocb)
{
struct ffs_io_data *io_data = kiocb->private;
- struct ffs_epfile *epfile = kiocb->ki_filp->private_data;
+ struct ffs_data *ffs = io_data->ffs;
int value;
ENTER();
- spin_lock_irq(&epfile->ffs->eps_lock);
-
- if (likely(io_data && io_data->ep && io_data->req))
- value = usb_ep_dequeue(io_data->ep, io_data->req);
- else
+ if (likely(io_data && io_data->ep && io_data->req)) {
+ INIT_WORK(&io_data->cancellation_work, ffs_aio_cancel_worker);
+ queue_work
Re: [PATCH] usb: dwc3: of-simple: fix use-after-free on remove
On Wed, Jun 13, 2018 at 12:39:18PM +0300, Felipe Balbi wrote:
> Roger Quadros writes:
> > I'm still trying to get my head around this.
> >
> > in probe() we do
> > {
> > enable all clocks;
> > pm_runtime_set_active(dev);
> > pm_runtime_enable(dev);
> > pm_runtime_get_sync(dev);
> > }
> >
> > How will runtime suspend work at all?
> > We're holding a positive RPM count in probe().
>
> echo auto > /path/to/dwc3/power/control
That makes no difference, user space cannot modify the always-active
behaviour given that probe returns with a positive usage count.
> Granted, that get_sync() would've been better as a pm_runtime_forbid()
Yeah, that would allow user space some control, albeit in a way that
may override a user space configuration (as the platform device would
already have been registered).
Why are you trying to prevent runtime pm in the first place? Shouldn't
the device be allowed to suspend when it has no active child by default?
Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v6 05/15] usb: typec: add API to get typec basic port power and data config
Hi Heikki,
> -Original Message-
> From: Heikki Krogerus [mailto:heikki.kroge...@linux.intel.com]
> Sent: 2018年6月11日 19:09
> To: Jun Li
> Cc: robh...@kernel.org; gre...@linuxfoundation.org; li...@roeck-us.net;
> cw00.c...@samsung.com; a.ha...@samsung.com; shufan_...@richtek.com;
> Peter Chen ; garsi...@embeddedor.com;
> gso...@gmail.com; linux-usb@vger.kernel.org; devicet...@vger.kernel.org;
> dl-linux-imx
> Subject: Re: [PATCH v6 05/15] usb: typec: add API to get typec basic port
> power
> and data config
>
> Hi Jun,
>
> On Mon, May 28, 2018 at 10:52:37AM +0800, Li Jun wrote:
> > This patch adds 3 APIs to get the typec port power and data type, and
> > preferred power role by its name string.
> >
> > Signed-off-by: Li Jun
> > ---
> > drivers/usb/typec/class.c | 50
> > +++
> > include/linux/usb/typec.h | 3 +++
> > 2 files changed, 53 insertions(+)
> >
> > diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
> > index 53df10d..4c7d18c 100644
> > --- a/drivers/usb/typec/class.c
> > +++ b/drivers/usb/typec/class.c
> > @@ -802,6 +802,12 @@ static const char * const typec_port_types[] = {
> > [TYPEC_PORT_DRP] = "dual",
> > };
> >
> > +static const char * const typec_data_caps[] = {
> > + [TYPEC_PORT_DFP] = "host",
> > + [TYPEC_PORT_UFP] = "device",
> > + [TYPEC_PORT_DRD] = "dual",
> > +};
>
> Since I guess you need to fix this patch in any case, could you rename that to
> "typec_port_data_roles".
OK.
>
> And while at it, how about using this as an opportunity to rename
> typec_port_types to typec_port_power_roles?
>
> So this just a suggestion, no need to actually change it :-) :
Also OK for me, I can rename it by this chance.
>
> diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c index
> abbd33939109..97f7eb0e9879 100644
> --- a/drivers/usb/typec/class.c
> +++ b/drivers/usb/typec/class.c
> @@ -918,12 +918,18 @@ static const char * const typec_data_roles[] = {
> [TYPEC_HOST]= "host",
> };
>
> -static const char * const typec_port_types[] = {
> +static const char * const typec_port_power_roles[] = {
> [TYPEC_PORT_SRC] = "source",
> [TYPEC_PORT_SNK] = "sink",
> [TYPEC_PORT_DRP] = "dual",
> };
>
> +static const char * const typec_port_data_roles[] = {
> + [TYPEC_PORT_DFP] = "host",
> + [TYPEC_PORT_UFP] = "device",
> + [TYPEC_PORT_DRD] = "dual",
> +};
> +
> static const char * const typec_port_types_drp[] = {
> [TYPEC_PORT_SRC] = "dual [source] sink",
> [TYPEC_PORT_SNK] = "dual source [sink]", @@ -1054,7 +1060,7 @@
> static ssize_t power_role_store(struct device *dev,
> mutex_lock(&port->port_type_lock);
> if (port->port_type != TYPEC_PORT_DRP) {
> dev_dbg(dev, "port type fixed at \"%s\"",
> -typec_port_types[port->port_type]);
> + typec_port_power_roles[port->port_type]);
> ret = -EOPNOTSUPP;
> goto unlock_and_ret;
> }
> @@ -1095,7 +1101,7 @@ port_type_store(struct device *dev, struct
> device_attribute *attr,
> return -EOPNOTSUPP;
> }
>
> - ret = sysfs_match_string(typec_port_types, buf);
> + ret = sysfs_match_string(typec_port_power_roles, buf);
> if (ret < 0)
> return ret;
>
> @@ -1129,7 +1135,7 @@ port_type_show(struct device *dev, struct
> device_attribute *attr,
> return sprintf(buf, "%s\n",
>typec_port_types_drp[port->port_type]);
>
> - return sprintf(buf, "[%s]\n", typec_port_types[port->cap->type]);
> + return sprintf(buf, "[%s]\n",
> + typec_port_power_roles[port->cap->type]);
> }
> static DEVICE_ATTR_RW(port_type);
>
>
> Thanks,
>
> --
> heikki
Re: [PATCH] usb: dwc3: of-simple: fix use-after-free on remove
Roger Quadros writes:
> On 13/06/18 11:05, Felipe Balbi wrote:
>> Roger Quadros writes:
>>
>>> Hi Johan,
>>>
>>> On 31/05/18 17:45, Johan Hovold wrote:
The clocks have already been explicitly disabled and put as part of
remove() so the runtime suspend callback must not be run when balancing
the runtime PM usage count before returning.
Fixes: 16adc674d0d6 ("usb: dwc3: add generic OF glue layer")
Signed-off-by: Johan Hovold
---
Changes in v2
- balance usage count only after disabling runtime PM to avoid racing
with pm_runtime_suspend() as suggested by Alan
drivers/usb/dwc3/dwc3-of-simple.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/dwc3/dwc3-of-simple.c
b/drivers/usb/dwc3/dwc3-of-simple.c
index cb2ee96fd3e8..048922d549dd 100644
--- a/drivers/usb/dwc3/dwc3-of-simple.c
+++ b/drivers/usb/dwc3/dwc3-of-simple.c
@@ -165,8 +165,9 @@ static int dwc3_of_simple_remove(struct
platform_device *pdev)
reset_control_put(simple->resets);
- pm_runtime_put_sync(dev);
>>>
>>> Wasn't this call there to balance out the pm_runtime_get_sync() call in
>>> probe()?
>>> The pm_runtime_get_sync() call is still there in probe().
>>
>> that's now balanced by put_noidle below
>>
>>
>
> OK.
>
> I'm still trying to get my head around this.
>
> in probe() we do
> {
> enable all clocks;
> pm_runtime_set_active(dev);
> pm_runtime_enable(dev);
> pm_runtime_get_sync(dev);
> }
>
> How will runtime suspend work at all?
> We're holding a positive RPM count in probe().
echo auto > /path/to/dwc3/power/control
Granted, that get_sync() would've been better as a pm_runtime_forbid()
--
balbi
signature.asc
Description: PGP signature
Re: [PATCH] USB: cdc-wdm: don't enable interrupts in USB-giveback
On Di, 2018-06-12 at 21:57 +0200, Sebastian Andrzej Siewior wrote:
> > Yes, the atomic case should be rare. It will only happen on errors, and
> > IIUC that's only to work around issues caused by reporting errors back
> > to userspace without actually wanting to err out anyway.
>
> Yup. The missing part is if this was done to workaround a specific
> userland application or most/all of them.
Yes, If possible we should not regress in that regard.
> > I believe it would be better to decide one an error policy and stick to
> > that. Then you could just simplify away that whole mess, by either
> > ignoring the error and continue or bailing out and die.
>
> "Bailing out and die" would be a revert of commit c1da59dad0eb
> ("cdc-wdm: Clear read pipeline in case of error")?
> And ignoring the error would be "not updating rerr" in
> wdm_in_callback().
> I don't care either way. I can do whatever works for you/users best.
It seems to me that the core of the problem is handling an error
in irq context potentially. How about shifting it to a work queue?
Regards
Oliver
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: dwc3: of-simple: fix use-after-free on remove
On 13/06/18 11:05, Felipe Balbi wrote:
> Roger Quadros writes:
>
>> Hi Johan,
>>
>> On 31/05/18 17:45, Johan Hovold wrote:
>>> The clocks have already been explicitly disabled and put as part of
>>> remove() so the runtime suspend callback must not be run when balancing
>>> the runtime PM usage count before returning.
>>>
>>> Fixes: 16adc674d0d6 ("usb: dwc3: add generic OF glue layer")
>>> Signed-off-by: Johan Hovold
>>> ---
>>>
>>> Changes in v2
>>> - balance usage count only after disabling runtime PM to avoid racing
>>>with pm_runtime_suspend() as suggested by Alan
>>>
>>>
>>> drivers/usb/dwc3/dwc3-of-simple.c | 3 ++-
>>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/usb/dwc3/dwc3-of-simple.c
>>> b/drivers/usb/dwc3/dwc3-of-simple.c
>>> index cb2ee96fd3e8..048922d549dd 100644
>>> --- a/drivers/usb/dwc3/dwc3-of-simple.c
>>> +++ b/drivers/usb/dwc3/dwc3-of-simple.c
>>> @@ -165,8 +165,9 @@ static int dwc3_of_simple_remove(struct platform_device
>>> *pdev)
>>>
>>> reset_control_put(simple->resets);
>>>
>>> - pm_runtime_put_sync(dev);
>>
>> Wasn't this call there to balance out the pm_runtime_get_sync() call in
>> probe()?
>> The pm_runtime_get_sync() call is still there in probe().
>
> that's now balanced by put_noidle below
>
>
OK.
I'm still trying to get my head around this.
in probe() we do
{
enable all clocks;
pm_runtime_set_active(dev);
pm_runtime_enable(dev);
pm_runtime_get_sync(dev);
}
How will runtime suspend work at all?
We're holding a positive RPM count in probe().
This was pointed out by Johan as well in [1]
[1] https://lkml.org/lkml/2018/5/28/1705
--
cheers,
-roger
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: dwc3: of-simple: fix use-after-free on remove
Roger Quadros writes:
> Hi Johan,
>
> On 31/05/18 17:45, Johan Hovold wrote:
>> The clocks have already been explicitly disabled and put as part of
>> remove() so the runtime suspend callback must not be run when balancing
>> the runtime PM usage count before returning.
>>
>> Fixes: 16adc674d0d6 ("usb: dwc3: add generic OF glue layer")
>> Signed-off-by: Johan Hovold
>> ---
>>
>> Changes in v2
>> - balance usage count only after disabling runtime PM to avoid racing
>>with pm_runtime_suspend() as suggested by Alan
>>
>>
>> drivers/usb/dwc3/dwc3-of-simple.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/dwc3/dwc3-of-simple.c
>> b/drivers/usb/dwc3/dwc3-of-simple.c
>> index cb2ee96fd3e8..048922d549dd 100644
>> --- a/drivers/usb/dwc3/dwc3-of-simple.c
>> +++ b/drivers/usb/dwc3/dwc3-of-simple.c
>> @@ -165,8 +165,9 @@ static int dwc3_of_simple_remove(struct platform_device
>> *pdev)
>>
>> reset_control_put(simple->resets);
>>
>> -pm_runtime_put_sync(dev);
>
> Wasn't this call there to balance out the pm_runtime_get_sync() call in
> probe()?
> The pm_runtime_get_sync() call is still there in probe().
that's now balanced by put_noidle below
--
balbi
signature.asc
Description: PGP signature
Re: [PATCH] usb: dwc3: of-simple: fix use-after-free on remove
Hi Johan,
On 31/05/18 17:45, Johan Hovold wrote:
> The clocks have already been explicitly disabled and put as part of
> remove() so the runtime suspend callback must not be run when balancing
> the runtime PM usage count before returning.
>
> Fixes: 16adc674d0d6 ("usb: dwc3: add generic OF glue layer")
> Signed-off-by: Johan Hovold
> ---
>
> Changes in v2
> - balance usage count only after disabling runtime PM to avoid racing
>with pm_runtime_suspend() as suggested by Alan
>
>
> drivers/usb/dwc3/dwc3-of-simple.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/dwc3/dwc3-of-simple.c
> b/drivers/usb/dwc3/dwc3-of-simple.c
> index cb2ee96fd3e8..048922d549dd 100644
> --- a/drivers/usb/dwc3/dwc3-of-simple.c
> +++ b/drivers/usb/dwc3/dwc3-of-simple.c
> @@ -165,8 +165,9 @@ static int dwc3_of_simple_remove(struct platform_device
> *pdev)
>
> reset_control_put(simple->resets);
>
> - pm_runtime_put_sync(dev);
Wasn't this call there to balance out the pm_runtime_get_sync() call in probe()?
The pm_runtime_get_sync() call is still there in probe().
> pm_runtime_disable(dev);
> + pm_runtime_put_noidle(dev);
> + pm_runtime_set_suspended(dev);
>
> return 0;
> }
>
--
cheers,
-roger
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: USB role switches, usb-connector, typec and device trees
On 12.06.2018 19:33, Mats Karrman wrote: > Hi Andrzej, > > On 2018-06-07 13:40, Andrzej Hajda wrote: > >> On 06.06.2018 23:36, Mats Karrman wrote: >>> Hello Gentlemen, >>> >>> I'm trying to get my head around USB role switches in connection with >>> Type-C ports >>> and device-trees. So far I have not found much documentation, e.g. there >>> are no >>> device-tree bindings documented and really no good examples in existing >>> device >>> trees, although there has been some attempts, e.g. [1] and [2]. Anyway, so >>> I send >>> you a couple of questions instead: >>> >>> 1) tcpm uses the port device struct to find a single usb_role_switch but >>> there is >>> room for three USB busses in the Type-C connector; one high speed and two >>> (?) super- >>> speed. These would not all come from the same controller (there might even >>> be >>> separate controllers for host and device mode for each bus). >>> The case I am working on now only have a single USB2 otg controller so it >>> should >>> be possible to make that driver register a role switch but for other cases? >>> I imagine it would be possible to create a composite driver as a proxy for >>> all role >>> switches but that would probably be different for every platform/product - >>> not >>> very elegant. Could the role switch infrastructure be extended to handle >>> arbitrary >>> sets of coordinated switches? >>> >>> 2) How should the connection between the Type-C port and the switches best >>> be >>> expressed in a device tree? Using graph I presume, but should it be mixed >>> into the >>> existing "usb-connector" or should this be a separate block? >>> I think it is unfortunate that the graph use numeric addresses that need to >>> be >>> fixed by documentation >> I also do not like port numbers, I even have argued for using names >> during multiple discussions, but the discussion ended as is :( >> >>> and already I see problems with the current assignment >>> (0=HS, 1=SS, 2=SBU), e.g. if the host and device mode are handled by >>> different >>> controllers. Graph do support multiple endpoints for one port but then we >>> have >>> another level of magic numbers which does not exactly make things easier >>> (e.g. 0=dual or host controller, 1=device controller, 2=mode switch). >> Where do you want to use these magic numbers? To describe endpoints? I > For some hypothetic worst case scenario where a switch needs to handle > different USB controllers for port host and device mode. However I think > Heikki and Hans are correct in saying we should wait to worry about that > until we have an actual use-case. > >> guess there should be controllable mux/switch behind the port, it could >> be standalone, or a part of bigger IP. >> Anyway it should be described by device node, probably parent of USB-C > child? > >> device-node, in such case only links from USB connector going to >> different IPs should be described using OF graphs. >> And in such case drivers of these devices should >> ask/determine/change/communicate their role using in-kernel frameworks >> (for example extcon). >> >> There are many different configurations of USB-C ports >> InterfaceControllers, Muxes/Switches, USB related devices, Alternate >> Mode devices, PMICs, USB-PDs, >> Could you describe particular one you have problem with, to make the >> discussion more specific. > My use-case now is simple. I have a typec port that I need to connect to > a dual-role USB controller, probably extended with a usb-role-switch > device and I want to express that connection in the device-tree. I was looking > for a documented description or example of how to do this but I realize that > that is not yet available. USB connector bindings [1] are not enough? Did you look at the example? I do not know what is exactly your usb-role-switch. Can you provide some schematics? Or describe which physical lines of USB-C connector are connected to which chips (including muxes/switches). Two rules of thumb, I think quite simple: 1. Do you have USB interface controller? Ie chip which is connected to CC lines of USB-C and performs cable detection, maybe PD negotiations, etc. This device should be the parent of USB connector node. 2. Connections between connector and other chips should be described using OF graph. [1]: https://www.kernel.org/doc/Documentation/devicetree/bindings/connector/usb-connector.txt Regards Andrzej > > // Mats > > > > -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
