| Message ID | 20200211132603.73509-3-mika.westerberg@linux.intel.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | platform/x86: Rework intel_scu_ipc and intel_pmc_ipc drivers | expand |
On Tue, Feb 11, 2020 at 04:25:47PM +0300, Mika Westerberg wrote: > Currently we only log an error if the command times out which makes it > hard to figure out the failing command. This changes the driver to log > command and subcommand with the error code which should make debugging > easier. This also allows us to simplify the callers as they don't need > to log these errors themselves. > Thanks, my comments below. ... > scu = ipcdev; > - ipc_command(scu, sub << 12 | cmd); > + cmdval = sub << 12 | cmd; > + ipc_command(scu, cmdval); > err = intel_scu_ipc_check_status(scu); > + if (err) > + dev_err(&scu->dev, "IPC command %#x failed with %d\n", cmdval, > + err); We may move it out of the mutex, right? Also, please keep it one line. > mutex_unlock(&ipclock); > return err; ... > mutex_lock(&ipclock); > + cmdval = (inlen << 16) | (sub << 12) | cmd; > + ipc_command(scu, cmdval); > err = intel_scu_ipc_check_status(scu); > - > - if (!err) { > + if (err) { > + dev_err(&scu->dev, "IPC command %#x failed with %d\n", cmdval, > + err); > + } else { > for (i = 0; i < outlen; i++) > *out++ = ipc_data_readl(scu, 4 * i); > } It's not visible in this context but it looks like above applies here as well.
On Tue, Feb 11, 2020 at 05:41:38PM +0200, Andy Shevchenko wrote: > On Tue, Feb 11, 2020 at 04:25:47PM +0300, Mika Westerberg wrote: > > Currently we only log an error if the command times out which makes it > > hard to figure out the failing command. This changes the driver to log > > command and subcommand with the error code which should make debugging > > easier. This also allows us to simplify the callers as they don't need > > to log these errors themselves. > > > > Thanks, my comments below. > > ... > > > scu = ipcdev; > > - ipc_command(scu, sub << 12 | cmd); > > + cmdval = sub << 12 | cmd; > > + ipc_command(scu, cmdval); > > err = intel_scu_ipc_check_status(scu); > > > + if (err) > > + dev_err(&scu->dev, "IPC command %#x failed with %d\n", cmdval, > > + err); > > We may move it out of the mutex, right? Also, please keep it one line. Yup. > > mutex_unlock(&ipclock); > > return err; > > ... > > > mutex_lock(&ipclock); > > > + cmdval = (inlen << 16) | (sub << 12) | cmd; > > + ipc_command(scu, cmdval); > > err = intel_scu_ipc_check_status(scu); > > - > > - if (!err) { > > + if (err) { > > + dev_err(&scu->dev, "IPC command %#x failed with %d\n", cmdval, > > + err); > > + } else { > > for (i = 0; i < outlen; i++) > > *out++ = ipc_data_readl(scu, 4 * i); > > } > > It's not visible in this context but it looks like above applies here as well. You mean move outside of the lock? This one calls ipc_data_readl() which should be under the lock.
On Wed, Feb 12, 2020 at 01:36:49PM +0200, Mika Westerberg wrote: > On Tue, Feb 11, 2020 at 05:41:38PM +0200, Andy Shevchenko wrote: > > On Tue, Feb 11, 2020 at 04:25:47PM +0300, Mika Westerberg wrote: ... > > > mutex_lock(&ipclock); > > > > > + cmdval = (inlen << 16) | (sub << 12) | cmd; > > > + ipc_command(scu, cmdval); > > > err = intel_scu_ipc_check_status(scu); > > > - > > > - if (!err) { > > > + if (err) { > > > + dev_err(&scu->dev, "IPC command %#x failed with %d\n", cmdval, > > > + err); > > > + } else { > > > for (i = 0; i < outlen; i++) > > > *out++ = ipc_data_readl(scu, 4 * i); > > > } > > > > It's not visible in this context but it looks like above applies here as well. > > You mean move outside of the lock? This one calls ipc_data_readl() > which should be under the lock. I meant to move dev_err() out of the lock. The rest of course requires to stay under it.
On Wed, Feb 12, 2020 at 01:54:42PM +0200, Andy Shevchenko wrote: > > You mean move outside of the lock? This one calls ipc_data_readl() > > which should be under the lock. > > I meant to move dev_err() out of the lock. The rest of course requires to stay > under it. OK, got it :)
diff --git a/drivers/platform/x86/intel_scu_ipc.c b/drivers/platform/x86/intel_scu_ipc.c index 19c2cc41fb05..1fc52b63d984 100644 --- a/drivers/platform/x86/intel_scu_ipc.c +++ b/drivers/platform/x86/intel_scu_ipc.c @@ -147,7 +147,6 @@ static inline int busy_loop(struct intel_scu_ipc_dev *scu) usleep_range(50, 100); } while (time_before(jiffies, end)); - dev_err(&scu->dev, "IPC timed out"); return -ETIMEDOUT; } @@ -156,10 +155,8 @@ static inline int ipc_wait_for_interrupt(struct intel_scu_ipc_dev *scu) { int status; - if (!wait_for_completion_timeout(&scu->cmd_complete, IPC_TIMEOUT)) { - dev_err(&scu->dev, "IPC timed out\n"); + if (!wait_for_completion_timeout(&scu->cmd_complete, IPC_TIMEOUT)) return -ETIMEDOUT; - } status = ipc_read_status(scu); if (status & IPC_STATUS_ERR) @@ -331,6 +328,7 @@ EXPORT_SYMBOL(intel_scu_ipc_update_register); int intel_scu_ipc_simple_command(int cmd, int sub) { struct intel_scu_ipc_dev *scu; + u32 cmdval; int err; mutex_lock(&ipclock); @@ -339,8 +337,12 @@ int intel_scu_ipc_simple_command(int cmd, int sub) return -ENODEV; } scu = ipcdev; - ipc_command(scu, sub << 12 | cmd); + cmdval = sub << 12 | cmd; + ipc_command(scu, cmdval); err = intel_scu_ipc_check_status(scu); + if (err) + dev_err(&scu->dev, "IPC command %#x failed with %d\n", cmdval, + err); mutex_unlock(&ipclock); return err; } @@ -362,6 +364,7 @@ int intel_scu_ipc_command(int cmd, int sub, u32 *in, int inlen, u32 *out, int outlen) { struct intel_scu_ipc_dev *scu; + u32 cmdval; int i, err; mutex_lock(&ipclock); @@ -374,10 +377,13 @@ int intel_scu_ipc_command(int cmd, int sub, u32 *in, int inlen, for (i = 0; i < inlen; i++) ipc_data_writel(scu, *in++, 4 * i); - ipc_command(scu, (inlen << 16) | (sub << 12) | cmd); + cmdval = (inlen << 16) | (sub << 12) | cmd; + ipc_command(scu, cmdval); err = intel_scu_ipc_check_status(scu); - - if (!err) { + if (err) { + dev_err(&scu->dev, "IPC command %#x failed with %d\n", cmdval, + err); + } else { for (i = 0; i < outlen; i++) *out++ = ipc_data_readl(scu, 4 * i); }
Currently we only log an error if the command times out which makes it hard to figure out the failing command. This changes the driver to log command and subcommand with the error code which should make debugging easier. This also allows us to simplify the callers as they don't need to log these errors themselves. Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com> --- drivers/platform/x86/intel_scu_ipc.c | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-)