Re: [PATCH] thunderbolt: icm: Remove Apple check for Alpine Ridge

[Kai-Heng Feng]

On Mon, Aug 7, 2017 at 3:02 PM, Mika Westerberg
<mika.westerberg@xxxxxxxxxxxxxxx> 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.feng@xxxxxxxxxxxxx> 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.feng@xxxxxxxxxxxxx>
>> > ---
>> >  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?