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

[Ajay Gupta <ajayg@nvidia.com>]

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