diff mbox series

[4.19.y,13/17] usb: gadget: udc: renesas_usb3: add a safety connection way for forced_b_device

Message ID 1556126440-27978-14-git-send-email-fabrizio.castro@bp.renesas.com (mailing list archive)
State Accepted
Headers show
Series Backport USB support for RZ/G2E | expand

Commit Message

Fabrizio Castro April 24, 2019, 5:20 p.m. UTC 
From: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>

commit ceb94bc52c437463f0903e61060a94a2226fb672 upstream.

This patch adds a safety connection way for "forced_b_device" with
"workaround_for_vbus" like below:

< Example for R-Car E3 Ebisu >
 # modprobe <any usb gadget driver>
 # echo 1 > /sys/kernel/debug/ee020000.usb/b_device
 (connect a usb cable to host side.)
 # echo 2 > /sys/kernel/debug/ee020000.usb/b_device

Previous code should have connected a usb cable before the "b_device"
is set to 1 on the Ebisu board. However, if xHCI driver on the board
is probed, it causes some troubles:
 - Conflicts USB VBUS/signals between the board and another host.
 - "Cannot enable. Maybe the USB cable is bad?" might happen on
   both the board and another host with a usb hub.
 - Cannot enumerate a usb gadget correctly because an interruption
   of VBUS change happens unexpectedly.

Reported-by: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com>
Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>
Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
---
 drivers/usb/gadget/udc/renesas_usb3.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

Comments

Pavel Machek April 25, 2019, 8:40 a.m. UTC | #1 
Hi!

> From: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> 
> commit ceb94bc52c437463f0903e61060a94a2226fb672 upstream.
> 
> This patch adds a safety connection way for "forced_b_device" with
> "workaround_for_vbus" like below:

I don't understand what "safety connection way" is.

> @@ -2432,7 +2433,11 @@ static ssize_t renesas_usb3_b_device_write(struct file *file,
>  	if (copy_from_user(&buf, ubuf, min_t(size_t, sizeof(buf) - 1, count)))
>  		return -EFAULT;
>  
> -	if (!strncmp(buf, "1", 1))
> +	usb3->start_to_connect = false;
> +	if (usb3->workaround_for_vbus && usb3->forced_b_device &&
> +	    !strncmp(buf, "2", 1))
> +		usb3->start_to_connect = true;
> +	else if (!strncmp(buf, "1", 1))
>  		usb3->forced_b_device = true;
>  	else
>  		usb3->forced_b_device = false;

I don't think this is correct. If user writes "12" to the file,
(!strncmp(buf, "1", 1)) will still be true, AFAICT. Similar problem
exists elsewhere in the file, with (!strncmp(buf, "host",
strlen("host"))).

This is adding new option to kernel interface. ("2" to sysfs file). I
believe that should be documented in Documentation/ABI.

Anyway, I'll most likely apply the patch, as problem needs to be fixed
in the mainline, first.

Best regards,
								Pavel
diff mbox series

Patch

diff --git a/drivers/usb/gadget/udc/renesas_usb3.c b/drivers/usb/gadget/udc/renesas_usb3.c
index cdffbd1..6e34f95 100644
--- a/drivers/usb/gadget/udc/renesas_usb3.c
+++ b/drivers/usb/gadget/udc/renesas_usb3.c
@@ -358,6 +358,7 @@  struct renesas_usb3 {
 	bool extcon_host;		/* check id and set EXTCON_USB_HOST */
 	bool extcon_usb;		/* check vbus and set EXTCON_USB */
 	bool forced_b_device;
+	bool start_to_connect;
 };
 
 #define gadget_to_renesas_usb3(_gadget)	\
@@ -476,7 +477,8 @@  static void usb3_init_axi_bridge(struct renesas_usb3 *usb3)
 static void usb3_init_epc_registers(struct renesas_usb3 *usb3)
 {
 	usb3_write(usb3, ~0, USB3_USB_INT_STA_1);
-	usb3_enable_irq_1(usb3, USB_INT_1_VBUS_CNG);
+	if (!usb3->workaround_for_vbus)
+		usb3_enable_irq_1(usb3, USB_INT_1_VBUS_CNG);
 }
 
 static bool usb3_wakeup_usb2_phy(struct renesas_usb3 *usb3)
@@ -700,8 +702,7 @@  static void usb3_mode_config(struct renesas_usb3 *usb3, bool host, bool a_dev)
 	usb3_set_mode_by_role_sw(usb3, host);
 	usb3_vbus_out(usb3, a_dev);
 	/* for A-Peripheral or forced B-device mode */
-	if ((!host && a_dev) ||
-	    (usb3->workaround_for_vbus && usb3->forced_b_device))
+	if ((!host && a_dev) || usb3->start_to_connect)
 		usb3_connect(usb3);
 	spin_unlock_irqrestore(&usb3->lock, flags);
 }
@@ -2432,7 +2433,11 @@  static ssize_t renesas_usb3_b_device_write(struct file *file,
 	if (copy_from_user(&buf, ubuf, min_t(size_t, sizeof(buf) - 1, count)))
 		return -EFAULT;
 
-	if (!strncmp(buf, "1", 1))
+	usb3->start_to_connect = false;
+	if (usb3->workaround_for_vbus && usb3->forced_b_device &&
+	    !strncmp(buf, "2", 1))
+		usb3->start_to_connect = true;
+	else if (!strncmp(buf, "1", 1))
 		usb3->forced_b_device = true;
 	else
 		usb3->forced_b_device = false;
@@ -2440,7 +2445,7 @@  static ssize_t renesas_usb3_b_device_write(struct file *file,
 	if (usb3->workaround_for_vbus)
 		usb3_disconnect(usb3);
 
-	/* Let this driver call usb3_connect() anyway */
+	/* Let this driver call usb3_connect() if needed */
 	usb3_check_id(usb3);
 
 	return count;