[RFC PATCH] usb: typec: ucsi: ccg: Remove run_isr flag

[RFC PATCH] usb: typec: ucsi: ccg: Remove run_isr flag

[Heikki Krogerus]

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@linux.intel.com>
---
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. 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?

If the driver works fine without the flag, then let's just drop it.
The deadlock will need to be fixed in any case.

thanks,

---
 drivers/usb/typec/ucsi/ucsi_ccg.c | 40 ++-----------------------------
 1 file changed, 2 insertions(+), 38 deletions(-)

diff --git a/drivers/usb/typec/ucsi/ucsi_ccg.c b/drivers/usb/typec/ucsi/ucsi_ccg.c
index 907e20e1a71e..167cb6367198 100644
--- a/drivers/usb/typec/ucsi/ucsi_ccg.c
+++ b/drivers/usb/typec/ucsi/ucsi_ccg.c
@@ -195,7 +195,6 @@ struct ucsi_ccg {
 
 	/* fw build with vendor information */
 	u16 fw_build;
-	bool run_isr; /* flag to call ISR routine during resume */
 	struct work_struct pm_work;
 };
 
@@ -224,18 +223,6 @@ static int ccg_read(struct ucsi_ccg *uc, u16 rab, u8 *data, u32 len)
 	if (quirks && quirks->max_read_len)
 		max_read_len = quirks->max_read_len;
 
-	if (uc->fw_build == CCG_FW_BUILD_NVIDIA &&
-	    uc->fw_version <= CCG_OLD_FW_VERSION) {
-		mutex_lock(&uc->lock);
-		/*
-		 * Do not schedule pm_work to run ISR in
-		 * ucsi_ccg_runtime_resume() after pm_runtime_get_sync()
-		 * since we are already in ISR path.
-		 */
-		uc->run_isr = false;
-		mutex_unlock(&uc->lock);
-	}
-
 	pm_runtime_get_sync(uc->dev);
 	while (rem_len > 0) {
 		msgs[1].buf = &data[len - rem_len];
@@ -278,18 +265,6 @@ static int ccg_write(struct ucsi_ccg *uc, u16 rab, u8 *data, u32 len)
 	msgs[0].len = len + sizeof(rab);
 	msgs[0].buf = buf;
 
-	if (uc->fw_build == CCG_FW_BUILD_NVIDIA &&
-	    uc->fw_version <= CCG_OLD_FW_VERSION) {
-		mutex_lock(&uc->lock);
-		/*
-		 * Do not schedule pm_work to run ISR in
-		 * ucsi_ccg_runtime_resume() after pm_runtime_get_sync()
-		 * since we are already in ISR path.
-		 */
-		uc->run_isr = false;
-		mutex_unlock(&uc->lock);
-	}
-
 	pm_runtime_get_sync(uc->dev);
 	status = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
 	if (status < 0) {
@@ -1130,7 +1105,6 @@ static int ucsi_ccg_probe(struct i2c_client *client,
 	uc->ppm.sync = ucsi_ccg_sync;
 	uc->dev = dev;
 	uc->client = client;
-	uc->run_isr = true;
 	mutex_init(&uc->lock);
 	INIT_WORK(&uc->work, ccg_update_firmware);
 	INIT_WORK(&uc->pm_work, ccg_pm_workaround_work);
@@ -1229,7 +1203,6 @@ static int ucsi_ccg_runtime_resume(struct device *dev)
 {
 	struct i2c_client *client = to_i2c_client(dev);
 	struct ucsi_ccg *uc = i2c_get_clientdata(client);
-	bool schedule = true;
 
 	/*
 	 * Firmware version 3.1.10 or earlier, built for NVIDIA has known issue
@@ -1237,17 +1210,8 @@ static int ucsi_ccg_runtime_resume(struct device *dev)
 	 * Schedule a work to call ISR as a workaround.
 	 */
 	if (uc->fw_build == CCG_FW_BUILD_NVIDIA &&
-	    uc->fw_version <= CCG_OLD_FW_VERSION) {
-		mutex_lock(&uc->lock);
-		if (!uc->run_isr) {
-			uc->run_isr = true;
-			schedule = false;
-		}
-		mutex_unlock(&uc->lock);
-
-		if (schedule)
-			schedule_work(&uc->pm_work);
-	}
+	    uc->fw_version <= CCG_OLD_FW_VERSION)
+		schedule_work(&uc->pm_work);
 
 	return 0;
 }
-- 
2.23.0

RE: [RFC PATCH] usb: typec: ucsi: ccg: Remove run_isr flag

[Ajay Gupta]

Hi Heikki,

> -----Original Message-----
> From: linux-usb-owner@vger.kernel.org <linux-usb-owner@vger.kernel.org>
> On Behalf Of Heikki Krogerus
> Sent: Monday, September 23, 2019 6:31 AM
> To: Ajay Gupta <ajayg@nvidia.com>
> Cc: linux-usb@vger.kernel.org
> 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@linux.intel.com>
> ---
> 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.

> 
> If the driver works fine without the flag, then let's just drop it.
> The deadlock will need to be fixed in any case.

We need to protect read/write of run_isr flag from ccg_read()/ccg_write() and
ucsi_ccg_runtime_resume() since they can get called from interrupt and runtime
pm threads.

I propose to add new "uc->pm_lock" for this purpose and not use "uc->lock".
Please see the change below for this. I tested both FW flashing and runtime PM
and they both work with new pm_lock.

diff --git a/drivers/usb/typec/ucsi/ucsi_ccg.c b/drivers/usb/typec/ucsi/ucsi_ccg.c
index ed727b2..a79a475 100644
--- a/drivers/usb/typec/ucsi/ucsi_ccg.c
+++ b/drivers/usb/typec/ucsi/ucsi_ccg.c
@@ -206,6 +206,7 @@ struct ucsi_ccg {
        /* fw build with vendor information */
        u16 fw_build;
        bool run_isr; /* flag to call ISR routine during resume */
+       struct mutex pm_lock; /* to sync between io and system pm thread */
        struct work_struct pm_work;

        bool has_multiple_dp;
@@ -240,14 +241,14 @@ static int ccg_read(struct ucsi_ccg *uc, u16 rab, u8 *data, u32 len)

        if (uc->fw_build == CCG_FW_BUILD_NVIDIA &&
            uc->fw_version <= CCG_OLD_FW_VERSION) {
-               mutex_lock(&uc->lock);
+               mutex_lock(&uc->pm_lock);
                /*
                 * Do not schedule pm_work to run ISR in
                 * ucsi_ccg_runtime_resume() after pm_runtime_get_sync()
                 * since we are already in ISR path.
                 */
                uc->run_isr = false;
-               mutex_unlock(&uc->lock);
+               mutex_unlock(&uc->pm_lock);
        }

        pm_runtime_get_sync(uc->dev);
@@ -294,14 +295,14 @@ static int ccg_write(struct ucsi_ccg *uc, u16 rab, u8 *data, u32 len)

        if (uc->fw_build == CCG_FW_BUILD_NVIDIA &&
            uc->fw_version <= CCG_OLD_FW_VERSION) {
-               mutex_lock(&uc->lock);
+               mutex_lock(&uc->pm_lock);
                /*
                 * Do not schedule pm_work to run ISR in
                 * ucsi_ccg_runtime_resume() after pm_runtime_get_sync()
                 * since we are already in ISR path.
                 */
                uc->run_isr = false;
-               mutex_unlock(&uc->lock);
+               mutex_unlock(&uc->pm_lock);
        }

        pm_runtime_get_sync(uc->dev);
@@ -1323,6 +1324,7 @@ static int ucsi_ccg_probe(struct i2c_client *client,
        uc->client = client;
        uc->run_isr = true;
        mutex_init(&uc->lock);
+       mutex_init(&uc->pm_lock);
        INIT_WORK(&uc->work, ccg_update_firmware);
        INIT_WORK(&uc->pm_work, ccg_pm_workaround_work);
@@ -1434,12 +1436,12 @@ static int ucsi_ccg_runtime_resume(struct device *dev)
         */
        if (uc->fw_build == CCG_FW_BUILD_NVIDIA &&
            uc->fw_version <= CCG_OLD_FW_VERSION) {
-               mutex_lock(&uc->lock);
+               mutex_lock(&uc->pm_lock);
                if (!uc->run_isr) {
                        uc->run_isr = true;
                        schedule = false;
                }
-               mutex_unlock(&uc->lock);
+               mutex_unlock(&uc->pm_lock);

                if (schedule)
                        schedule_work(&uc->pm_work);


Thanks
> nvpublic
> thanks,
> 
> ---
>  drivers/usb/typec/ucsi/ucsi_ccg.c | 40 ++-----------------------------
>  1 file changed, 2 insertions(+), 38 deletions(-)
> 
> diff --git a/drivers/usb/typec/ucsi/ucsi_ccg.c
> b/drivers/usb/typec/ucsi/ucsi_ccg.c
> index 907e20e1a71e..167cb6367198 100644
> --- a/drivers/usb/typec/ucsi/ucsi_ccg.c
> +++ b/drivers/usb/typec/ucsi/ucsi_ccg.c
> @@ -195,7 +195,6 @@ struct ucsi_ccg {
> 
>  	/* fw build with vendor information */
>  	u16 fw_build;
> -	bool run_isr; /* flag to call ISR routine during resume */
>  	struct work_struct pm_work;
>  };
> 
> @@ -224,18 +223,6 @@ static int ccg_read(struct ucsi_ccg *uc, u16 rab, u8
> *data, u32 len)
>  	if (quirks && quirks->max_read_len)
>  		max_read_len = quirks->max_read_len;
> 
> -	if (uc->fw_build == CCG_FW_BUILD_NVIDIA &&
> -	    uc->fw_version <= CCG_OLD_FW_VERSION) {
> -		mutex_lock(&uc->lock);
> -		/*
> -		 * Do not schedule pm_work to run ISR in
> -		 * ucsi_ccg_runtime_resume() after pm_runtime_get_sync()
> -		 * since we are already in ISR path.
> -		 */
> -		uc->run_isr = false;
> -		mutex_unlock(&uc->lock);
> -	}
> -
>  	pm_runtime_get_sync(uc->dev);
>  	while (rem_len > 0) {
>  		msgs[1].buf = &data[len - rem_len];
> @@ -278,18 +265,6 @@ static int ccg_write(struct ucsi_ccg *uc, u16 rab, u8
> *data, u32 len)
>  	msgs[0].len = len + sizeof(rab);
>  	msgs[0].buf = buf;
> 
> -	if (uc->fw_build == CCG_FW_BUILD_NVIDIA &&
> -	    uc->fw_version <= CCG_OLD_FW_VERSION) {
> -		mutex_lock(&uc->lock);
> -		/*
> -		 * Do not schedule pm_work to run ISR in
> -		 * ucsi_ccg_runtime_resume() after pm_runtime_get_sync()
> -		 * since we are already in ISR path.
> -		 */
> -		uc->run_isr = false;
> -		mutex_unlock(&uc->lock);
> -	}
> -
>  	pm_runtime_get_sync(uc->dev);
>  	status = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
>  	if (status < 0) {
> @@ -1130,7 +1105,6 @@ static int ucsi_ccg_probe(struct i2c_client *client,
>  	uc->ppm.sync = ucsi_ccg_sync;
>  	uc->dev = dev;
>  	uc->client = client;
> -	uc->run_isr = true;
>  	mutex_init(&uc->lock);
>  	INIT_WORK(&uc->work, ccg_update_firmware);
>  	INIT_WORK(&uc->pm_work, ccg_pm_workaround_work); @@ -1229,7
> +1203,6 @@ static int ucsi_ccg_runtime_resume(struct device *dev)  {
>  	struct i2c_client *client = to_i2c_client(dev);
>  	struct ucsi_ccg *uc = i2c_get_clientdata(client);
> -	bool schedule = true;
> 
>  	/*
>  	 * Firmware version 3.1.10 or earlier, built for NVIDIA has known issue
> @@ -1237,17 +1210,8 @@ static int ucsi_ccg_runtime_resume(struct device
> *dev)
>  	 * Schedule a work to call ISR as a workaround.
>  	 */
>  	if (uc->fw_build == CCG_FW_BUILD_NVIDIA &&
> -	    uc->fw_version <= CCG_OLD_FW_VERSION) {
> -		mutex_lock(&uc->lock);
> -		if (!uc->run_isr) {
> -			uc->run_isr = true;
> -			schedule = false;
> -		}
> -		mutex_unlock(&uc->lock);
> -
> -		if (schedule)
> -			schedule_work(&uc->pm_work);
> -	}
> +	    uc->fw_version <= CCG_OLD_FW_VERSION)
> +		schedule_work(&uc->pm_work);
> 
>  	return 0;
>  }
> --
> 2.23.0

Re: [RFC PATCH] usb: typec: ucsi: ccg: Remove run_isr flag

[Heikki Krogerus]

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

Hi Ajay,

On Mon, Sep 23, 2019 at 06:15:59PM +0000, Ajay Gupta wrote:
> Hi Heikki,
> 
> > -----Original Message-----
> > From: linux-usb-owner@vger.kernel.org <linux-usb-owner@vger.kernel.org>
> > On Behalf Of Heikki Krogerus
> > Sent: Monday, September 23, 2019 6:31 AM
> > To: Ajay Gupta <ajayg@nvidia.com>
> > Cc: linux-usb@vger.kernel.org
> > 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@linux.intel.com>
> > ---
> > 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.

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,

-- 
heikki

[-- Attachment #2: 0001-usb-typec-ucsi-ccg-Remove-run_isr-flag.patch --]
[-- Type: text/plain, Size: 4143 bytes --]

From aced6041a9ff34cd1c10f60bbe8eaf0b55df60af Mon Sep 17 00:00:00 2001
From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Date: Mon, 23 Sep 2019 15:04:10 +0300
Subject: [PATCH] usb: typec: ucsi: ccg: Remove run_isr flag

The "run_isr" flag is used for preventing the driver from
calling the interrupt service routine in its runtime resume
callback when the driver is expecting completion to a
command, but what that basically does is that it hides the
real problem. The real problem is that the controller is
allowed to suspend in the middle of command execution.

As a more appropriate fix for the problem, using autosuspend
delay time that matches UCSI_TIMOUT_MS. That prevents the
controller from suspending while still in the middle of
executing a command.

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@linux.intel.com>
---
 drivers/usb/typec/ucsi/ucsi_ccg.c | 42 +++----------------------------
 1 file changed, 4 insertions(+), 38 deletions(-)

diff --git a/drivers/usb/typec/ucsi/ucsi_ccg.c b/drivers/usb/typec/ucsi/ucsi_ccg.c
index 907e20e1a71e..d772fce51905 100644
--- a/drivers/usb/typec/ucsi/ucsi_ccg.c
+++ b/drivers/usb/typec/ucsi/ucsi_ccg.c
@@ -195,7 +195,6 @@ struct ucsi_ccg {
 
 	/* fw build with vendor information */
 	u16 fw_build;
-	bool run_isr; /* flag to call ISR routine during resume */
 	struct work_struct pm_work;
 };
 
@@ -224,18 +223,6 @@ static int ccg_read(struct ucsi_ccg *uc, u16 rab, u8 *data, u32 len)
 	if (quirks && quirks->max_read_len)
 		max_read_len = quirks->max_read_len;
 
-	if (uc->fw_build == CCG_FW_BUILD_NVIDIA &&
-	    uc->fw_version <= CCG_OLD_FW_VERSION) {
-		mutex_lock(&uc->lock);
-		/*
-		 * Do not schedule pm_work to run ISR in
-		 * ucsi_ccg_runtime_resume() after pm_runtime_get_sync()
-		 * since we are already in ISR path.
-		 */
-		uc->run_isr = false;
-		mutex_unlock(&uc->lock);
-	}
-
 	pm_runtime_get_sync(uc->dev);
 	while (rem_len > 0) {
 		msgs[1].buf = &data[len - rem_len];
@@ -278,18 +265,6 @@ static int ccg_write(struct ucsi_ccg *uc, u16 rab, u8 *data, u32 len)
 	msgs[0].len = len + sizeof(rab);
 	msgs[0].buf = buf;
 
-	if (uc->fw_build == CCG_FW_BUILD_NVIDIA &&
-	    uc->fw_version <= CCG_OLD_FW_VERSION) {
-		mutex_lock(&uc->lock);
-		/*
-		 * Do not schedule pm_work to run ISR in
-		 * ucsi_ccg_runtime_resume() after pm_runtime_get_sync()
-		 * since we are already in ISR path.
-		 */
-		uc->run_isr = false;
-		mutex_unlock(&uc->lock);
-	}
-
 	pm_runtime_get_sync(uc->dev);
 	status = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
 	if (status < 0) {
@@ -1130,7 +1105,6 @@ static int ucsi_ccg_probe(struct i2c_client *client,
 	uc->ppm.sync = ucsi_ccg_sync;
 	uc->dev = dev;
 	uc->client = client;
-	uc->run_isr = true;
 	mutex_init(&uc->lock);
 	INIT_WORK(&uc->work, ccg_update_firmware);
 	INIT_WORK(&uc->pm_work, ccg_pm_workaround_work);
@@ -1188,6 +1162,8 @@ static int ucsi_ccg_probe(struct i2c_client *client,
 
 	pm_runtime_set_active(uc->dev);
 	pm_runtime_enable(uc->dev);
+	pm_runtime_use_autosuspend(uc->dev);
+	pm_runtime_set_autosuspend_delay(uc->dev, 5000);
 	pm_runtime_idle(uc->dev);
 
 	return 0;
@@ -1229,7 +1205,6 @@ static int ucsi_ccg_runtime_resume(struct device *dev)
 {
 	struct i2c_client *client = to_i2c_client(dev);
 	struct ucsi_ccg *uc = i2c_get_clientdata(client);
-	bool schedule = true;
 
 	/*
 	 * Firmware version 3.1.10 or earlier, built for NVIDIA has known issue
@@ -1237,17 +1212,8 @@ static int ucsi_ccg_runtime_resume(struct device *dev)
 	 * Schedule a work to call ISR as a workaround.
 	 */
 	if (uc->fw_build == CCG_FW_BUILD_NVIDIA &&
-	    uc->fw_version <= CCG_OLD_FW_VERSION) {
-		mutex_lock(&uc->lock);
-		if (!uc->run_isr) {
-			uc->run_isr = true;
-			schedule = false;
-		}
-		mutex_unlock(&uc->lock);
-
-		if (schedule)
-			schedule_work(&uc->pm_work);
-	}
+	    uc->fw_version <= CCG_OLD_FW_VERSION)
+		schedule_work(&uc->pm_work);
 
 	return 0;
 }
-- 
2.23.0

RE: [RFC PATCH] usb: typec: ucsi: ccg: Remove run_isr flag

[Ajay Gupta]

Hi Heikki,

> -----Original Message-----
> From: linux-usb-owner@vger.kernel.org <linux-usb-owner@vger.kernel.org>
> On Behalf Of Heikki Krogerus
> Sent: Tuesday, September 24, 2019 1:25 AM
> To: Ajay Gupta <ajayg@nvidia.com>
> Cc: linux-usb@vger.kernel.org
> 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@vger.kernel.org
> > > <linux-usb-owner@vger.kernel.org> On Behalf Of Heikki Krogerus
> > > Sent: Monday, September 23, 2019 6:31 AM
> > > To: Ajay Gupta <ajayg@nvidia.com>
> > > Cc: linux-usb@vger.kernel.org
> > > 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@linux.intel.com>
> > > ---
> > > 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