Hi Heikki, > -----Original Message----- > From: linux-usb-owner@xxxxxxxxxxxxxxx <linux-usb-owner@xxxxxxxxxxxxxxx> > On Behalf Of Heikki Krogerus > Sent: Tuesday, September 24, 2019 1:25 AM > To: Ajay Gupta <ajayg@xxxxxxxxxx> > Cc: linux-usb@xxxxxxxxxxxxxxx > Subject: Re: [RFC PATCH] usb: typec: ucsi: ccg: Remove run_isr flag > > Hi Ajay, > > On Mon, Sep 23, 2019 at 06:15:59PM +0000, Ajay Gupta wrote: > > Hi Heikki, > > > > > -----Original Message----- > > > From: linux-usb-owner@xxxxxxxxxxxxxxx > > > <linux-usb-owner@xxxxxxxxxxxxxxx> On Behalf Of Heikki Krogerus > > > Sent: Monday, September 23, 2019 6:31 AM > > > To: Ajay Gupta <ajayg@xxxxxxxxxx> > > > Cc: linux-usb@xxxxxxxxxxxxxxx > > > Subject: [RFC PATCH] usb: typec: ucsi: ccg: Remove run_isr flag > > > > > > There is no need to try to prevent the extra ucsi_notify() that > > > runtime resuming the device will cause. > > > > > > This fixes potential deadlock. Both ccg_read() and > > > ccg_write() are called with the mutex already taken at least from > > > ccg_send_command(). In ccg_read() and ccg_write, the mutex is only > > > acquired so that run_isr flag can be set. > > > > > > Signed-off-by: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx> > > > --- > > > Hi Ajay, > > > > > > Before going forward with this I would like to get confirmation from > > > you that it is OK, and that I'm not missing anything. > > Thanks for this. I mixed up firmware flashing and IO path by using same lock. > > > > >I did not see any real purpose for that run_isr flag. > > > The only thing that I can see it preventing is an extra > > >ucsi_notify() call caused by the waking of the controller, but that should > not be a problem. > > > Is there any other reason why the flag is there? > > ucsi_ccg_runtime_resume() will get called after > > pm_runtime_get_sync(uc->dev); in ccg_read() and ccg_write(). If we > > allow extra ucsi_notify() then ccg_read() and > > ccg_write() will get called again from ucsi_notfiy() through > > ucsi_sync(). This will keep on happening in a loop and the controller > > will remain in D0 always and runtime pm will never be done. > > OK, so the problem is actually that we are allowing the controller to suspend > while we are still in the middle of waiting for completion to a command, right? > That should not be allowed to happen. True. > Instead of hiding the symptom from the problem with that flag, the driver > needs to initially define autosuspend delay that is as long as > UCSI_TIMEOUT_MS (or longer). That should be enough to fix the problem, > though it still leaves a small change of hitting it in some corner cases. After I'm > done with the I/O rewrite, we can guarantee that the controller stays in D0 for > as long as it takes to process the whole command. > > But as the initial fix, let's just use the autosuspend delay. > > Can you test the attached patch? Thanks for the patch. I tested both runtime PM and firmware flashing on GPUs with old firmware and latest firmware and it works fine. Thanks >nvpublic > > thanks, > > -- > heikki
