| Message ID | 1526289360-3997-4-git-send-email-yoshihiro.shimoda.uh@renesas.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, May 14, 2018 at 06:15:59PM +0900, Yoshihiro Shimoda wrote: > This patch uses usb role switch API if the register suceeeded. > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > --- > drivers/usb/gadget/udc/renesas_usb3.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/drivers/usb/gadget/udc/renesas_usb3.c b/drivers/usb/gadget/udc/renesas_usb3.c > index c878449..bfb5803 100644 > --- a/drivers/usb/gadget/udc/renesas_usb3.c > +++ b/drivers/usb/gadget/udc/renesas_usb3.c > @@ -657,7 +657,11 @@ static void _usb3_set_mode(struct renesas_usb3 *usb3, bool host) > > static void usb3_set_mode(struct renesas_usb3 *usb3, bool host) > { > - _usb3_set_mode(usb3, host); > + if (usb3->role_sw) > + usb_role_switch_set_role(usb3->role_sw, host ? > + USB_ROLE_HOST : USB_ROLE_DEVICE); > + else > + _usb3_set_mode(usb3, host); > } > > static void usb3_vbus_out(struct renesas_usb3 *usb3, bool enable) > @@ -672,8 +676,8 @@ static void usb3_mode_config(struct renesas_usb3 *usb3, bool host, bool a_dev) > { > unsigned long flags; > > - spin_lock_irqsave(&usb3->lock, flags); > usb3_set_mode(usb3, host); > + spin_lock_irqsave(&usb3->lock, flags); Hi Shimoda-san, could you clarify why moving this lock is safe? > usb3_vbus_out(usb3, a_dev); > /* for A-Peripheral or forced B-device mode */ > if ((!host && a_dev) || > -- > 1.9.1 > -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Simon-san, Thank you for your review! > From: Simon Horman, Sent: Tuesday, May 15, 2018 4:54 PM > On Mon, May 14, 2018 at 06:15:59PM +0900, Yoshihiro Shimoda wrote: <snip> > > static void usb3_set_mode(struct renesas_usb3 *usb3, bool host) > > { > > - _usb3_set_mode(usb3, host); > > + if (usb3->role_sw) > > + usb_role_switch_set_role(usb3->role_sw, host ? > > + USB_ROLE_HOST : USB_ROLE_DEVICE); > > + else > > + _usb3_set_mode(usb3, host); > > } > > > > static void usb3_vbus_out(struct renesas_usb3 *usb3, bool enable) > > @@ -672,8 +676,8 @@ static void usb3_mode_config(struct renesas_usb3 *usb3, bool host, bool a_dev) > > { > > unsigned long flags; > > > > - spin_lock_irqsave(&usb3->lock, flags); > > usb3_set_mode(usb3, host); > > + spin_lock_irqsave(&usb3->lock, flags); > > Hi Shimoda-san, > > could you clarify why moving this lock is safe? The usb3_set_mode() is possible to call usb_role_switch_set_role() API and usb_role_switch_set_role() calls mutex_lock(). So, this moving this lock (especially avoiding irqsave) needs. Best regards, Yoshihiro Shimoda > > usb3_vbus_out(usb3, a_dev); > > /* for A-Peripheral or forced B-device mode */ > > if ((!host && a_dev) || > > -- > > 1.9.1 > > -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, May 15, 2018 at 08:02:00AM +0000, Yoshihiro Shimoda wrote: > Hi Simon-san, > > Thank you for your review! > > > From: Simon Horman, Sent: Tuesday, May 15, 2018 4:54 PM > > On Mon, May 14, 2018 at 06:15:59PM +0900, Yoshihiro Shimoda wrote: > <snip> > > > static void usb3_set_mode(struct renesas_usb3 *usb3, bool host) > > > { > > > - _usb3_set_mode(usb3, host); > > > + if (usb3->role_sw) > > > + usb_role_switch_set_role(usb3->role_sw, host ? > > > + USB_ROLE_HOST : USB_ROLE_DEVICE); > > > + else > > > + _usb3_set_mode(usb3, host); > > > } > > > > > > static void usb3_vbus_out(struct renesas_usb3 *usb3, bool enable) > > > @@ -672,8 +676,8 @@ static void usb3_mode_config(struct renesas_usb3 *usb3, bool host, bool a_dev) > > > { > > > unsigned long flags; > > > > > > - spin_lock_irqsave(&usb3->lock, flags); > > > usb3_set_mode(usb3, host); > > > + spin_lock_irqsave(&usb3->lock, flags); > > > > Hi Shimoda-san, > > > > could you clarify why moving this lock is safe? > > The usb3_set_mode() is possible to call usb_role_switch_set_role() API > and usb_role_switch_set_role() calls mutex_lock(). > So, this moving this lock (especially avoiding irqsave) needs. Thanks, that make sense. But usb3_set_mode() may also call _usb3_set_mode(). It is safe to call _usb3_set_mode() without holding the lock? > Best regards, > Yoshihiro Shimoda > > > > usb3_vbus_out(usb3, a_dev); > > > /* for A-Peripheral or forced B-device mode */ > > > if ((!host && a_dev) || > > > -- > > > 1.9.1 > > > > -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Simon-san, > From: Simon Horman, Sent: Tuesday, May 15, 2018 5:04 PM > > On Tue, May 15, 2018 at 08:02:00AM +0000, Yoshihiro Shimoda wrote: > > Hi Simon-san, > > > > Thank you for your review! > > > > > From: Simon Horman, Sent: Tuesday, May 15, 2018 4:54 PM > > > On Mon, May 14, 2018 at 06:15:59PM +0900, Yoshihiro Shimoda wrote: > > <snip> > > > > static void usb3_set_mode(struct renesas_usb3 *usb3, bool host) > > > > { > > > > - _usb3_set_mode(usb3, host); > > > > + if (usb3->role_sw) > > > > + usb_role_switch_set_role(usb3->role_sw, host ? > > > > + USB_ROLE_HOST : USB_ROLE_DEVICE); > > > > + else > > > > + _usb3_set_mode(usb3, host); > > > > } > > > > > > > > static void usb3_vbus_out(struct renesas_usb3 *usb3, bool enable) > > > > @@ -672,8 +676,8 @@ static void usb3_mode_config(struct renesas_usb3 *usb3, bool host, bool a_dev) > > > > { > > > > unsigned long flags; > > > > > > > > - spin_lock_irqsave(&usb3->lock, flags); > > > > usb3_set_mode(usb3, host); > > > > + spin_lock_irqsave(&usb3->lock, flags); > > > > > > Hi Shimoda-san, > > > > > > could you clarify why moving this lock is safe? > > > > The usb3_set_mode() is possible to call usb_role_switch_set_role() API > > and usb_role_switch_set_role() calls mutex_lock(). > > So, this moving this lock (especially avoiding irqsave) needs. > > Thanks, that make sense. > > But usb3_set_mode() may also call _usb3_set_mode(). > It is safe to call _usb3_set_mode() without holding the lock? Thank you for the pointed out. I am thinking it is possible to be unsafe without holding the lock. So, I'd like to improve this patch somehow (for example: Add new usb role switch APIs without mutex). Best regards, Yoshihiro Shimoda -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/usb/gadget/udc/renesas_usb3.c b/drivers/usb/gadget/udc/renesas_usb3.c index c878449..bfb5803 100644 --- a/drivers/usb/gadget/udc/renesas_usb3.c +++ b/drivers/usb/gadget/udc/renesas_usb3.c @@ -657,7 +657,11 @@ static void _usb3_set_mode(struct renesas_usb3 *usb3, bool host) static void usb3_set_mode(struct renesas_usb3 *usb3, bool host) { - _usb3_set_mode(usb3, host); + if (usb3->role_sw) + usb_role_switch_set_role(usb3->role_sw, host ? + USB_ROLE_HOST : USB_ROLE_DEVICE); + else + _usb3_set_mode(usb3, host); } static void usb3_vbus_out(struct renesas_usb3 *usb3, bool enable) @@ -672,8 +676,8 @@ static void usb3_mode_config(struct renesas_usb3 *usb3, bool host, bool a_dev) { unsigned long flags; - spin_lock_irqsave(&usb3->lock, flags); usb3_set_mode(usb3, host); + spin_lock_irqsave(&usb3->lock, flags); usb3_vbus_out(usb3, a_dev); /* for A-Peripheral or forced B-device mode */ if ((!host && a_dev) ||
This patch uses usb role switch API if the register suceeeded. Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> --- drivers/usb/gadget/udc/renesas_usb3.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)