* Re: [PATCH] thunderbolt: icm: Remove Apple check for Alpine Ridge
[not found] <20170807044912.18146-1-kai.heng.feng@canonical.com>
@ 2017-08-07 6:50 ` Kai-Heng Feng
2017-08-07 7:02 ` Mika Westerberg
0 siblings, 1 reply; 6+ messages in thread
From: Kai-Heng Feng @ 2017-08-07 6:50 UTC (permalink / raw)
To: LKML
Cc: andreas.noever, michael.jamet, yehezkel.bernat, Kai-Heng Feng,
mika.westerberg
On Mon, Aug 7, 2017 at 12:49 PM, Kai-Heng Feng
<kai.heng.feng@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.
>
> Signed-off-by: Kai-Heng Feng <kai.heng.feng@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;
> -
> - /*
> * Find the upstream PCIe port in case we need to do reset
> * through its vendor specific registers.
> */
> --
> 2.13.4
>
Forgot to CC LKML...
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] thunderbolt: icm: Remove Apple check for Alpine Ridge
2017-08-07 6:50 ` [PATCH] thunderbolt: icm: Remove Apple check for Alpine Ridge Kai-Heng Feng
@ 2017-08-07 7:02 ` Mika Westerberg
2017-08-07 7:20 ` Kai-Heng Feng
0 siblings, 1 reply; 6+ messages in thread
From: Mika Westerberg @ 2017-08-07 7:02 UTC (permalink / raw)
To: Kai-Heng Feng; +Cc: LKML, andreas.noever, michael.jamet, yehezkel.bernat
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@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.
Is there an actual issue you are trying to solve here?
> > Signed-off-by: Kai-Heng Feng <kai.heng.feng@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?
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] thunderbolt: icm: Remove Apple check for Alpine Ridge
2017-08-07 7:02 ` Mika Westerberg
@ 2017-08-07 7:20 ` Kai-Heng Feng
2017-08-07 7:51 ` Mika Westerberg
0 siblings, 1 reply; 6+ messages in thread
From: Kai-Heng Feng @ 2017-08-07 7:20 UTC (permalink / raw)
To: Mika Westerberg; +Cc: LKML, andreas.noever, michael.jamet, yehezkel.bernat
On Mon, Aug 7, 2017 at 3:02 PM, Mika Westerberg
<mika.westerberg@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.feng@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.feng@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?
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] thunderbolt: icm: Remove Apple check for Alpine Ridge
2017-08-07 7:20 ` Kai-Heng Feng
@ 2017-08-07 7:51 ` Mika Westerberg
2017-08-07 8:00 ` Kai-Heng Feng
0 siblings, 1 reply; 6+ messages in thread
From: Mika Westerberg @ 2017-08-07 7:51 UTC (permalink / raw)
To: Kai-Heng Feng; +Cc: LKML, andreas.noever, michael.jamet, yehezkel.bernat
On Mon, Aug 07, 2017 at 03:20:40PM +0800, Kai-Heng Feng wrote:
> On Mon, Aug 7, 2017 at 3:02 PM, Mika Westerberg
> <mika.westerberg@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.feng@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()
At this point we should find out that the ICM is already running and the
function never calls pci2cio_write().
The reason why it is not happening needs to be resolved.
> 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
I would like to understand what the actual problem is here because in
normal cases we should not end up starting ICM firmware in the first
place.
So no, let's not fix it like this until we know the root cause.
I'll be participating the discussion on the above bug in hopes we could
figure out the root cause.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] thunderbolt: icm: Remove Apple check for Alpine Ridge
2017-08-07 7:51 ` Mika Westerberg
@ 2017-08-07 8:00 ` Kai-Heng Feng
2017-08-07 8:06 ` Mika Westerberg
0 siblings, 1 reply; 6+ messages in thread
From: Kai-Heng Feng @ 2017-08-07 8:00 UTC (permalink / raw)
To: Mika Westerberg; +Cc: LKML, andreas.noever, michael.jamet, yehezkel.bernat
On Mon, Aug 7, 2017 at 3:51 PM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> At this point we should find out that the ICM is already running and the
> function never calls pci2cio_write().
I guess you mean this code section:
/* Check if the ICM firmware is already running */
val = ioread32(nhi->iobase + REG_FW_STS);
if (val & REG_FW_STS_ICM_EN)
return 0;
>
> The reason why it is not happening needs to be resolved.
>
>> 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
>
> I would like to understand what the actual problem is here because in
> normal cases we should not end up starting ICM firmware in the first
> place.
>
> So no, let's not fix it like this until we know the root cause.
>
> I'll be participating the discussion on the above bug in hopes we could
> figure out the root cause.
Thanks for the information and explanation.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] thunderbolt: icm: Remove Apple check for Alpine Ridge
2017-08-07 8:00 ` Kai-Heng Feng
@ 2017-08-07 8:06 ` Mika Westerberg
0 siblings, 0 replies; 6+ messages in thread
From: Mika Westerberg @ 2017-08-07 8:06 UTC (permalink / raw)
To: Kai-Heng Feng; +Cc: LKML, andreas.noever, michael.jamet, yehezkel.bernat
On Mon, Aug 07, 2017 at 04:00:06PM +0800, Kai-Heng Feng wrote:
> On Mon, Aug 7, 2017 at 3:51 PM, Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> > At this point we should find out that the ICM is already running and the
> > function never calls pci2cio_write().
>
> I guess you mean this code section:
>
> /* Check if the ICM firmware is already running */
> val = ioread32(nhi->iobase + REG_FW_STS);
> if (val & REG_FW_STS_ICM_EN)
> return 0;
Yes, that's correct.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-08-07 8:06 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <20170807044912.18146-1-kai.heng.feng@canonical.com>
2017-08-07 6:50 ` [PATCH] thunderbolt: icm: Remove Apple check for Alpine Ridge Kai-Heng Feng
2017-08-07 7:02 ` Mika Westerberg
2017-08-07 7:20 ` Kai-Heng Feng
2017-08-07 7:51 ` Mika Westerberg
2017-08-07 8:00 ` Kai-Heng Feng
2017-08-07 8:06 ` Mika Westerberg
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).