Re: [PATCHv2 10/13] usb: dwc2: initialize the spin_lock for both host and gadget
On 8/22/14, 3:57 PM, Felipe Balbi wrote: Hi, On Fri, Aug 22, 2014 at 08:52:23PM +, Paul Zimmerman wrote: From: dingu...@altera.com [mailto:dingu...@altera.com] Sent: Wednesday, July 30, 2014 8:21 AM Move spin_lock_init to common location for both host and gadget. Signed-off-by: Dinh Nguyen dingu...@altera.com --- drivers/usb/dwc2/hcd.c |1 - drivers/usb/dwc2/platform.c |1 + 2 files changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c index 07a7bcd..c6778d9 100644 --- a/drivers/usb/dwc2/hcd.c +++ b/drivers/usb/dwc2/hcd.c @@ -2824,7 +2824,6 @@ int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq, hcd-has_tt = 1; - spin_lock_init(hsotg-lock); ((struct wrapper_priv_data *) hcd-hcd_priv)-hsotg = hsotg; hsotg-priv = hcd; diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c index eb2a131..4898268 100644 --- a/drivers/usb/dwc2/platform.c +++ b/drivers/usb/dwc2/platform.c @@ -197,6 +197,7 @@ static int dwc2_driver_probe(struct platform_device *dev) } platform_set_drvdata(dev, hsotg); + spin_lock_init(hsotg-lock); return retval; } Hi Dinh, I don't have a copy of your v3 patches in my mailbox anymore, so I am replying to the v2 one instead. Are you absolutely sure that no code that takes the spinlock can be called before this point? This is the last line in the probe() function, so I have a hard time believing it is safe to initialize the spinlock this late. In particular, the IRQ has already been attached, and usb_add_gadget_udc() has already been called. So it seems entirely possible that some other entity could try to access the driver before this point. you're right with this comment. request_irq() enables the IRQ line and it could be that we already have a pending event to handle which fires as soon as we enable that IRQ line. Yes, thanks for catching this. I will move the call up in v4. Dinh -- 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: [PATCHv2 10/13] usb: dwc2: initialize the spin_lock for both host and gadget
From: Dinh Nguyen [mailto:dinh.li...@gmail.com] Sent: Monday, August 25, 2014 3:58 AM On 8/22/14, 3:57 PM, Felipe Balbi wrote: Hi, On Fri, Aug 22, 2014 at 08:52:23PM +, Paul Zimmerman wrote: From: dingu...@altera.com [mailto:dingu...@altera.com] Sent: Wednesday, July 30, 2014 8:21 AM Move spin_lock_init to common location for both host and gadget. Signed-off-by: Dinh Nguyen dingu...@altera.com --- drivers/usb/dwc2/hcd.c |1 - drivers/usb/dwc2/platform.c |1 + 2 files changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c index 07a7bcd..c6778d9 100644 --- a/drivers/usb/dwc2/hcd.c +++ b/drivers/usb/dwc2/hcd.c @@ -2824,7 +2824,6 @@ int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq, hcd-has_tt = 1; - spin_lock_init(hsotg-lock); ((struct wrapper_priv_data *) hcd-hcd_priv)-hsotg = hsotg; hsotg-priv = hcd; diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c index eb2a131..4898268 100644 --- a/drivers/usb/dwc2/platform.c +++ b/drivers/usb/dwc2/platform.c @@ -197,6 +197,7 @@ static int dwc2_driver_probe(struct platform_device *dev) } platform_set_drvdata(dev, hsotg); + spin_lock_init(hsotg-lock); return retval; } Hi Dinh, I don't have a copy of your v3 patches in my mailbox anymore, so I am replying to the v2 one instead. Are you absolutely sure that no code that takes the spinlock can be called before this point? This is the last line in the probe() function, so I have a hard time believing it is safe to initialize the spinlock this late. In particular, the IRQ has already been attached, and usb_add_gadget_udc() has already been called. So it seems entirely possible that some other entity could try to access the driver before this point. you're right with this comment. request_irq() enables the IRQ line and it could be that we already have a pending event to handle which fires as soon as we enable that IRQ line. Yes, thanks for catching this. I will move the call up in v4. OK, I think that was the last issue I had with the series. So you can add my acked-by to any other patches in the series that I haven't acked yet. Except I want to see the changes for spin_lock_init() before I ack those. And again, thanks for all your work on this. -- Paul -- 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: [PATCHv2 10/13] usb: dwc2: initialize the spin_lock for both host and gadget
From: dingu...@altera.com [mailto:dingu...@altera.com] Sent: Wednesday, July 30, 2014 8:21 AM Move spin_lock_init to common location for both host and gadget. Signed-off-by: Dinh Nguyen dingu...@altera.com --- drivers/usb/dwc2/hcd.c |1 - drivers/usb/dwc2/platform.c |1 + 2 files changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c index 07a7bcd..c6778d9 100644 --- a/drivers/usb/dwc2/hcd.c +++ b/drivers/usb/dwc2/hcd.c @@ -2824,7 +2824,6 @@ int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq, hcd-has_tt = 1; - spin_lock_init(hsotg-lock); ((struct wrapper_priv_data *) hcd-hcd_priv)-hsotg = hsotg; hsotg-priv = hcd; diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c index eb2a131..4898268 100644 --- a/drivers/usb/dwc2/platform.c +++ b/drivers/usb/dwc2/platform.c @@ -197,6 +197,7 @@ static int dwc2_driver_probe(struct platform_device *dev) } platform_set_drvdata(dev, hsotg); + spin_lock_init(hsotg-lock); return retval; } Hi Dinh, I don't have a copy of your v3 patches in my mailbox anymore, so I am replying to the v2 one instead. Are you absolutely sure that no code that takes the spinlock can be called before this point? This is the last line in the probe() function, so I have a hard time believing it is safe to initialize the spinlock this late. In particular, the IRQ has already been attached, and usb_add_gadget_udc() has already been called. So it seems entirely possible that some other entity could try to access the driver before this point. The same comment applies to your Update pci portion of the dwc2 driver patch. -- Paul -- 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: [PATCHv2 10/13] usb: dwc2: initialize the spin_lock for both host and gadget
Hi, On Fri, Aug 22, 2014 at 08:52:23PM +, Paul Zimmerman wrote: From: dingu...@altera.com [mailto:dingu...@altera.com] Sent: Wednesday, July 30, 2014 8:21 AM Move spin_lock_init to common location for both host and gadget. Signed-off-by: Dinh Nguyen dingu...@altera.com --- drivers/usb/dwc2/hcd.c |1 - drivers/usb/dwc2/platform.c |1 + 2 files changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c index 07a7bcd..c6778d9 100644 --- a/drivers/usb/dwc2/hcd.c +++ b/drivers/usb/dwc2/hcd.c @@ -2824,7 +2824,6 @@ int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq, hcd-has_tt = 1; - spin_lock_init(hsotg-lock); ((struct wrapper_priv_data *) hcd-hcd_priv)-hsotg = hsotg; hsotg-priv = hcd; diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c index eb2a131..4898268 100644 --- a/drivers/usb/dwc2/platform.c +++ b/drivers/usb/dwc2/platform.c @@ -197,6 +197,7 @@ static int dwc2_driver_probe(struct platform_device *dev) } platform_set_drvdata(dev, hsotg); + spin_lock_init(hsotg-lock); return retval; } Hi Dinh, I don't have a copy of your v3 patches in my mailbox anymore, so I am replying to the v2 one instead. Are you absolutely sure that no code that takes the spinlock can be called before this point? This is the last line in the probe() function, so I have a hard time believing it is safe to initialize the spinlock this late. In particular, the IRQ has already been attached, and usb_add_gadget_udc() has already been called. So it seems entirely possible that some other entity could try to access the driver before this point. you're right with this comment. request_irq() enables the IRQ line and it could be that we already have a pending event to handle which fires as soon as we enable that IRQ line. -- balbi signature.asc Description: Digital signature
