From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752065AbdHGHUn (ORCPT ); Mon, 7 Aug 2017 03:20:43 -0400 Received: from mail-wm0-f41.google.com ([74.125.82.41]:37837 "EHLO mail-wm0-f41.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751247AbdHGHUm (ORCPT ); Mon, 7 Aug 2017 03:20:42 -0400 MIME-Version: 1.0 In-Reply-To: <20170807070254.GL2369@lahna.fi.intel.com> References: <20170807044912.18146-1-kai.heng.feng@canonical.com> <20170807070254.GL2369@lahna.fi.intel.com> From: Kai-Heng Feng Date: Mon, 7 Aug 2017 15:20:40 +0800 Message-ID: Subject: Re: [PATCH] thunderbolt: icm: Remove Apple check for Alpine Ridge To: Mika Westerberg Cc: LKML , andreas.noever@gmail.com, michael.jamet@intel.com, yehezkel.bernat@intel.com Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Aug 7, 2017 at 3:02 PM, Mika Westerberg wrote: > On Mon, Aug 07, 2017 at 02:50:49PM +0800, Kai-Heng Feng wrote: >> On Mon, Aug 7, 2017 at 12:49 PM, Kai-Heng Feng >> wrote: >> > In icm_ar_is_supported(), icm->upstream_port will be uninitialized if >> > the hardware is not an Apple one. >> > >> > The uninitialized icm->upstream_port will later be dereferenced in >> > pcie2cio_write(), causes a NULL pointer dereference issue. >> > >> > Commit f67cf491175a ("thunderbolt: Add support for Internal Connection >> > Manager (ICM)") states that all Alpine Ridge will use ICM, so I guess >> > it's safe to remove the Apple check. > > Yes, Alpine Ridge uses ICM but on Apple systems we need to additional > steps to get it up and running. That's why the check is there. So no it > cannot be removed. If that's the case, it probably should be like this: diff --git a/drivers/thunderbolt/icm.c b/drivers/thunderbolt/icm.c index bdaac1ff00a5..95c255996ff0 100644 --- a/drivers/thunderbolt/icm.c +++ b/drivers/thunderbolt/icm.c @@ -888,9 +888,11 @@ static int icm_driver_ready(struct tb *tb) struct icm *icm = tb_priv(tb); int ret; - ret = icm_firmware_init(tb); - if (ret) - return ret; + if (is_apple()) { + ret = icm_firmware_init(tb); + if (ret) + return ret; + } if (icm->safe_mode) { tb_info(tb, "Thunderbolt host controller is in safe mode.\n"); --- The uninitialized icm->upstream_port, will be used at here: icm_firmware_init() icm_firmware_start() icm_firmware_reset() pcie2cio_write() pci_write_config_dword(pdev, vnd_cap + PCIE2CIO_WRDATA, data); > Is there an actual issue you are trying to solve here? Yes, please take a look at [1]. Although both the patch I sent and the diff above still failed to probe the device But there are no more NULL pointer dereference. [1] https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1708043/comments/11 > >> > Signed-off-by: Kai-Heng Feng >> > --- >> > drivers/thunderbolt/icm.c | 7 ------- >> > 1 file changed, 7 deletions(-) >> > >> > diff --git a/drivers/thunderbolt/icm.c b/drivers/thunderbolt/icm.c >> > index bdaac1ff00a5..2ab25aac5446 100644 >> > --- a/drivers/thunderbolt/icm.c >> > +++ b/drivers/thunderbolt/icm.c >> > @@ -514,13 +514,6 @@ static bool icm_ar_is_supported(struct tb *tb) >> > struct icm *icm = tb_priv(tb); >> > >> > /* >> > - * Starting from Alpine Ridge we can use ICM on Apple machines >> > - * as well. We just need to reset and re-enable it first. >> > - */ >> > - if (!is_apple()) >> > - return true; >> > - >> > - /* > > How did you test this?