diff mbox series

[v4,3/4] platform/x86: intel_scu_ipc: Don't override scu in intel_scu_ipc_dev_simple_command()

Message ID 20230913212723.3055315-4-swboyd@chromium.org
State New
Headers
Series platform/x86: intel_scu_ipc: Timeout fixes |

Commit Message

Stephen Boyd Sept. 13, 2023, 9:27 p.m. UTC 
  Andy discovered this bug during patch review. The 'scu' argument to this
function shouldn't be overridden by the function itself. It doesn't make
any sense. Looking at the commit history, we see that commit
f57fa18583f5 ("platform/x86: intel_scu_ipc: Introduce new SCU IPC API")
removed the setting of the scu to ipcdev in other functions, but not
this one. That was an oversight. Remove this line so that we stop
overriding the scu instance that is used by this function.

Reported-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Closes: https://lore.kernel.org/r/ZPjdZ3xNmBEBvNiS@smile.fi.intel.com
Cc: Prashant Malani <pmalani@chromium.org>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Fixes: f57fa18583f5 ("platform/x86: intel_scu_ipc: Introduce new SCU IPC API")
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---
 drivers/platform/x86/intel_scu_ipc.c | 1 -
 1 file changed, 1 deletion(-)

Comments

Ilpo Järvinen Sept. 15, 2023, 2:45 p.m. UTC | #1 
On Wed, 13 Sep 2023, Stephen Boyd wrote:

> Andy discovered this bug during patch review. The 'scu' argument to this
> function shouldn't be overridden by the function itself. It doesn't make
> any sense. Looking at the commit history, we see that commit
> f57fa18583f5 ("platform/x86: intel_scu_ipc: Introduce new SCU IPC API")
> removed the setting of the scu to ipcdev in other functions, but not
> this one. That was an oversight. Remove this line so that we stop
> overriding the scu instance that is used by this function.
> 
> Reported-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Closes: https://lore.kernel.org/r/ZPjdZ3xNmBEBvNiS@smile.fi.intel.com

This looks somewhat unusual way to tag it. I'd just drop the Closes tag 
as the email list is not a bug tracter.

Other than that,
Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Andy Shevchenko Sept. 16, 2023, 11:14 a.m. UTC | #2 
On Fri, Sep 15, 2023 at 05:45:42PM +0300, Ilpo Järvinen wrote:
> On Wed, 13 Sep 2023, Stephen Boyd wrote:
> 
> > Andy discovered this bug during patch review. The 'scu' argument to this
> > function shouldn't be overridden by the function itself. It doesn't make
> > any sense. Looking at the commit history, we see that commit
> > f57fa18583f5 ("platform/x86: intel_scu_ipc: Introduce new SCU IPC API")
> > removed the setting of the scu to ipcdev in other functions, but not
> > this one. That was an oversight. Remove this line so that we stop
> > overriding the scu instance that is used by this function.
> > 
> > Reported-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > Closes: https://lore.kernel.org/r/ZPjdZ3xNmBEBvNiS@smile.fi.intel.com
> 
> This looks somewhat unusual way to tag it. I'd just drop the Closes tag 
> as the email list is not a bug tracter.

This is a new requirement enforced by checkpatch.pl. If commit message has
the Reported-by: tag it should have Closes: one as well.
diff mbox series

Patch

diff --git a/drivers/platform/x86/intel_scu_ipc.c b/drivers/platform/x86/intel_scu_ipc.c
index 299c15312acb..3271f81a9c00 100644
--- a/drivers/platform/x86/intel_scu_ipc.c
+++ b/drivers/platform/x86/intel_scu_ipc.c
@@ -443,7 +443,6 @@  int intel_scu_ipc_dev_simple_command(struct intel_scu_ipc_dev *scu, int cmd,
 		mutex_unlock(&ipclock);
 		return -ENODEV;
 	}
-	scu = ipcdev;
 	cmdval = sub << 12 | cmd;
 	ipc_command(scu, cmdval);
 	err = intel_scu_ipc_check_status(scu);