On Mon, Aug 7, 2017 at 3:02 PM, Mika Westerberg
<mika.westerb...@linux.intel.com> 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
>> <kai.heng.f...@canonical.com> 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 <kai.heng.f...@canonical.com>
>> > ---
>> > 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?
