RE: [PATCH/RFC v3 3/4] usb: gadget: udc: renesas_usb3: use usb role switch API
Hi Simon-san, > From: Simon Horman, Sent: Tuesday, May 15, 2018 5:04 PM > > On Tue, May 15, 2018 at 08:02:00AM +, 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: > > > > > > 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC v3 3/4] usb: gadget: udc: renesas_usb3: use usb role switch API
On Tue, May 15, 2018 at 08:02:00AM +, 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: > > > > 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH/RFC v3 3/4] usb: gadget: udc: renesas_usb3: use usb role switch API
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: > > 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC v3 3/4] usb: gadget: udc: renesas_usb3: use usb role switch API
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 > --- > 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html