[PATCH] intel_punit_ipc: add NULL check for input parameters

[PATCH] intel_punit_ipc: add NULL check for input parameters

[Qipeng Zha]

intel_punit_ipc_command() maybe called when in or out
data pointers are NULL.

Signed-off-by: Qipeng Zha <qipeng.zha@intel.com>
---
 drivers/platform/x86/intel_punit_ipc.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/platform/x86/intel_punit_ipc.c b/drivers/platform/x86/intel_punit_ipc.c
index 16685bc..bd87540 100644
--- a/drivers/platform/x86/intel_punit_ipc.c
+++ b/drivers/platform/x86/intel_punit_ipc.c
@@ -187,10 +187,12 @@ int intel_punit_ipc_command(u32 cmd, u32 para1, u32 para2, u32 *in, u32 *out)
 
 	reinit_completion(&ipcdev->cmd_complete);
 	type = (cmd & IPC_PUNIT_CMD_TYPE_MASK) >> IPC_TYPE_OFFSET;
-	ipc_write_data_low(ipcdev, type, *in);
 
-	if (type == GTDRIVER_IPC || type == ISPDRIVER_IPC)
-		ipc_write_data_high(ipcdev, type, *++in);
+	if (in) {
+		ipc_write_data_low(ipcdev, type, *in);
+		if (type == GTDRIVER_IPC || type == ISPDRIVER_IPC)
+			ipc_write_data_high(ipcdev, type, *++in);
+	}
 
 	val = cmd & ~IPC_PUNIT_CMD_TYPE_MASK;
 	val |= CMD_RUN | para2 << CMD_PARA2_SHIFT | para1 << CMD_PARA1_SHIFT;
@@ -199,10 +201,12 @@ int intel_punit_ipc_command(u32 cmd, u32 para1, u32 para2, u32 *in, u32 *out)
 	ret = intel_punit_ipc_check_status(ipcdev, type);
 	if (ret)
 		goto out;
-	*out = ipc_read_data_low(ipcdev, type);
 
-	if (type == GTDRIVER_IPC || type == ISPDRIVER_IPC)
-		*++out = ipc_read_data_high(ipcdev, type);
+	if (out) {
+		*out = ipc_read_data_low(ipcdev, type);
+		if (type == GTDRIVER_IPC || type == ISPDRIVER_IPC)
+			*++out = ipc_read_data_high(ipcdev, type);
+	}
 
 out:
 	mutex_unlock(&ipcdev->lock);
-- 
1.8.3.2

Re: [PATCH] intel_punit_ipc: add NULL check for input parameters

[One Thousand Gnomes]

On Fri,  8 Jan 2016 18:32:27 +0800
Qipeng Zha <qipeng.zha@intel.com> wrote:

> intel_punit_ipc_command() maybe called when in or out
> data pointers are NULL.

In which case what happens: the command is not issued but there seems to
be no way for the caller to discover this, and nohing is logged.

Alan

Re: [PATCH] intel_punit_ipc: add NULL check for input parameters

[Darren Hart]

On Fri, Jan 08, 2016 at 11:26:44AM +0000, One Thousand Gnomes wrote:
> On Fri,  8 Jan 2016 18:32:27 +0800
> Qipeng Zha <qipeng.zha@intel.com> wrote:
> 
> > intel_punit_ipc_command() maybe called when in or out
> > data pointers are NULL.
> 
> In which case what happens: the command is not issued but there seems to
> be no way for the caller to discover this, and nohing is logged.
> 
> Alan
> 

These functinos are not void, it seems an errcode or a message is in order.

Also, the commit message needs to document in what scenario
intel_punit_ipc_command() may be called with null pointers - and why we consider
that to be OK.

-- 
Darren Hart
Intel Open Source Technology Center

RE: [PATCH] intel_punit_ipc: add NULL check for input parameters

[Chakravarty, Souvik K]

> -----Original Message-----
> From: platform-driver-x86-owner@vger.kernel.org [mailto:platform-driver-
> x86-owner@vger.kernel.org] On Behalf Of Darren Hart
> Sent: Monday, January 11, 2016 11:22 PM
> To: One Thousand Gnomes <gnomes@lxorguk.ukuu.org.uk>
> Cc: Zha, Qipeng <qipeng.zha@intel.com>; platform-driver-
> x86@vger.kernel.org; andriy.shevchenko@linux.intel.com; Chakravarty,
> Souvik K <souvik.k.chakravarty@intel.com>
> Subject: Re: [PATCH] intel_punit_ipc: add NULL check for input parameters
> 
> On Fri, Jan 08, 2016 at 11:26:44AM +0000, One Thousand Gnomes wrote:
> > On Fri,  8 Jan 2016 18:32:27 +0800
> > Qipeng Zha <qipeng.zha@intel.com> wrote:
> >
> > > intel_punit_ipc_command() maybe called when in or out data pointers
> > > are NULL.
> >
> > In which case what happens: the command is not issued but there seems
> > to be no way for the caller to discover this, and nohing is logged.
> >
> > Alan
> >
> 
> These functinos are not void, it seems an errcode or a message is in order.
> 
> Also, the commit message needs to document in what scenario
> intel_punit_ipc_command() may be called with null pointers - and why we
> consider that to be OK.

Darren,
In Punit IPC, all of the commands do not need an Input parameter; similarly all cmds do not need an output parameter(eg setting events).
Telemetry uses all the flavors. (This fix is already there in our internal codebase for a long time, but somehow got missed during upstreaming).
So this is not an error condition & the API needs to handle all cases properly. 
> 
> --
> Darren Hart
> Intel Open Source Technology Center
> --
> To unsubscribe from this list: send the line "unsubscribe platform-driver-x86"
> in the body of a message to majordomo@vger.kernel.org More majordomo
> info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] intel_punit_ipc: add NULL check for input parameters

[Darren Hart]

On Fri, Jan 08, 2016 at 11:26:44AM +0000, One Thousand Gnomes wrote:
> On Fri,  8 Jan 2016 18:32:27 +0800
> Qipeng Zha <qipeng.zha@intel.com> wrote:
> 
> > intel_punit_ipc_command() maybe called when in or out
> > data pointers are NULL.
> 
> In which case what happens: the command is not issued but there seems to
> be no way for the caller to discover this, and nohing is logged.

My reading of this is different.

A command receives a type and a val. It may or may not also require data in the
"in" buffer, and the caller may or may not provide an out buffer.

The command is issued in any case.

I have merged this to avoid a crash, but we can certainly revisit this if I have
missed something.

-- 
Darren Hart
Intel Open Source Technology Center

Re: [PATCH] intel_punit_ipc: add NULL check for input parameters

[Darren Hart]

On Fri, Jan 08, 2016 at 06:32:27PM +0800, Qipeng Zha wrote:
> intel_punit_ipc_command() maybe called when in or out
> data pointers are NULL.
> 
> Signed-off-by: Qipeng Zha <qipeng.zha@intel.com>

Queued, thank you Qipeng.

-- 
Darren Hart
Intel Open Source Technology Center