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.
>
> 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
