Re: [PATCH v3 1/2] usb: dwc2/gadget: add mutex to serialize init/deinit calls

[Felipe Balbi <balbi@ti.com>]

[-- Attachment #1: Type: text/plain, Size: 1836 bytes --]

Hi,

On Fri, Nov 14, 2014 at 07:43:23PM +0000, Paul Zimmerman wrote:
> > >>>> @@ -3699,6 +3717,8 @@ static int s3c_hsotg_resume(struct platform_device *pdev)
> > >>>>    	s3c_hsotg_core_connect(hsotg);
> > >>>>    	spin_unlock_irqrestore(&hsotg->lock, flags);
> > >>>>
> > >>>> +	mutex_unlock(&hsotg->init_mutex);
> > >>>> +
> > >>>>    	return ret;
> > >>>>    }
> > >>>>
> > >>> Hmm. I can't find any other UDC driver that uses a mutex in its
> > >>> suspend/resume functions. Can you explain why this is needed only
> > >>> for dwc2?
> > >> I've posted this version because I thought you were not convinced that
> > >> the patch
> > >> "usb: dwc2/gadget: rework suspend/resume code to correctly restore
> > >> gadget state"
> > >> can add code for initialization and deinitialization in suspend/resume
> > >> paths.
> > > My problem with that patch was that you were checking the ->enabled
> > > flag outside of the spinlock. To address that, you only need to move
> > > the check inside of the spinlock. I don't see why a mutex is needed.
> > 
> > It is not that simple. I can add spin_lock() before checking enabled,
> > but then
> > I would need to spin_unlock() to call regulator_bulk_enable() and
> > phy_enable(),
> > because both cannot be called from atomic context. This means that the
> > spinlock
> > in such case will not protect anything and is simply useless.
> 
> Ah, OK. So you're using the mutex instead of the ->enabled flag that you
> proposed in the "rework suspend/resume code" patch. So this patch is a
> replacement for that one. Somehow I was thinking this patch was on top
> of that one.
> 
> So I guess this is OK, but I would like to get Felipe's opinion about
> it before we apply this.
> 
> Felipe?

I can't think of a better way, I'm afraid :-(

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]