| Message ID | 1452249147-34712-1-git-send-email-qipeng.zha@intel.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
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 -- 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
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.
> -----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 -- 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
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.
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.
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);
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(-)