Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1644614 > unrolled thread

[PATCH 02/24] thunderbolt: Do not try to read UID if DROM offset is read as 0

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 02/24] thunderbolt: Do not try to read UID if DROM offset is read as 0 Mika Westerberg <mika.westerberg@linux.intel.com> - 2017-05-18 16:50 +0200
    Re: [PATCH 02/24] thunderbolt: Do not try to read UID if DROM offset  is read as 0 Andreas Noever <andreas.noever@gmail.com> - 2017-05-21 15:50 +0200
      Re: [PATCH 02/24] thunderbolt: Do not try to read UID if DROM offset  is read as 0 Mika Westerberg <mika.westerberg@linux.intel.com> - 2017-05-22 10:50 +0200
        Re: [PATCH 02/24] thunderbolt: Do not try to read UID if DROM offset  is read as 0 Andreas Noever <andreas.noever@gmail.com> - 2017-05-22 20:50 +0200
          Re: [PATCH 02/24] thunderbolt: Do not try to read UID if DROM offset  is read as 0 Mika Westerberg <mika.westerberg@linux.intel.com> - 2017-05-22 22:50 +0200
            Re: [PATCH 02/24] thunderbolt: Do not try to read UID if DROM offset  is read as 0 Andreas Noever <andreas.noever@gmail.com> - 2017-05-22 23:00 +0200

#1644614 — [PATCH 02/24] thunderbolt: Do not try to read UID if DROM offset is read as 0

At least Falcon Ridge when in host mode does not have any kind of DROM
available and reading DROM offset returns 0 for these. Do not try to
read DROM any further in that case.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Reviewed-by: Yehezkel Bernat <yehezkel.bernat@intel.com>
Reviewed-by: Michael Jamet <michael.jamet@intel.com>
---
 drivers/thunderbolt/eeprom.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/thunderbolt/eeprom.c b/drivers/thunderbolt/eeprom.c
index 6392990c984d..e4e64b130514 100644
--- a/drivers/thunderbolt/eeprom.c
+++ b/drivers/thunderbolt/eeprom.c
@@ -276,6 +276,9 @@ int tb_drom_read_uid_only(struct tb_switch *sw, u64 *uid)
 	if (res)
 		return res;
 
+	if (drom_offset == 0)
+		return -ENODEV;
+
 	/* read uid */
 	res = tb_eeprom_read_n(sw, drom_offset, data, 9);
 	if (res)
-- 
2.11.0

[toc] | [next] | [standalone]

#1646356 — Re: [PATCH 02/24] thunderbolt: Do not try to read UID if DROM offset is read as 0

On Thu, May 18, 2017 at 4:38 PM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> At least Falcon Ridge when in host mode does not have any kind of DROM
> available and reading DROM offset returns 0 for these. Do not try to
> read DROM any further in that case.
>
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> Reviewed-by: Yehezkel Bernat <yehezkel.bernat@intel.com>
> Reviewed-by: Michael Jamet <michael.jamet@intel.com>
> ---
>  drivers/thunderbolt/eeprom.c | 3 +++
>  1 file changed, 3 insertions(+)
>

Hi Mika,

nice work, it is nice to see Intel contribute to the Thunderbolt
driver (I can second Lukas's 'jaw drop' comment)!

I will try to read through everything today, but maybe the last few
patches will get pushed back to next weekend.

> diff --git a/drivers/thunderbolt/eeprom.c b/drivers/thunderbolt/eeprom.c
> index 6392990c984d..e4e64b130514 100644
> --- a/drivers/thunderbolt/eeprom.c
> +++ b/drivers/thunderbolt/eeprom.c
> @@ -276,6 +276,9 @@ int tb_drom_read_uid_only(struct tb_switch *sw, u64 *uid)
>         if (res)
>                 return res;
>
> +       if (drom_offset == 0)
> +               return -ENODEV;
> +
I think that this will make tb_switch_resume bail out on the root
switch, which is not good. Since the uid is only used to detect
whether a different device was plugged in while the system was
suspended I think that we can safely ignore the uid on the root
switch:
 - don't read it in tb_drom_read (route == 0 is already special cased anyways)
 - add a special case for the root switch to tb_switch_resume and
don't read the uid - just assume that it did not change (should be
impossible anyways)

What do you think?

Andreas

>         /* read uid */
>         res = tb_eeprom_read_n(sw, drom_offset, data, 9);
>         if (res)
> --
> 2.11.0
>

[toc] | [prev] | [next] | [standalone]

#1646614 — Re: [PATCH 02/24] thunderbolt: Do not try to read UID if DROM offset is read as 0

On Sun, May 21, 2017 at 03:46:20PM +0200, Andreas Noever wrote:
> On Thu, May 18, 2017 at 4:38 PM, Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> > At least Falcon Ridge when in host mode does not have any kind of DROM
> > available and reading DROM offset returns 0 for these. Do not try to
> > read DROM any further in that case.
> >
> > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > Reviewed-by: Yehezkel Bernat <yehezkel.bernat@intel.com>
> > Reviewed-by: Michael Jamet <michael.jamet@intel.com>
> > ---
> >  drivers/thunderbolt/eeprom.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> 
> Hi Mika,
> 
> nice work, it is nice to see Intel contribute to the Thunderbolt
> driver (I can second Lukas's 'jaw drop' comment)!
> 
> I will try to read through everything today, but maybe the last few
> patches will get pushed back to next weekend.

Thanks :)

> > diff --git a/drivers/thunderbolt/eeprom.c b/drivers/thunderbolt/eeprom.c
> > index 6392990c984d..e4e64b130514 100644
> > --- a/drivers/thunderbolt/eeprom.c
> > +++ b/drivers/thunderbolt/eeprom.c
> > @@ -276,6 +276,9 @@ int tb_drom_read_uid_only(struct tb_switch *sw, u64 *uid)
> >         if (res)
> >                 return res;
> >
> > +       if (drom_offset == 0)
> > +               return -ENODEV;
> > +
> I think that this will make tb_switch_resume bail out on the root
> switch, which is not good. Since the uid is only used to detect
> whether a different device was plugged in while the system was
> suspended I think that we can safely ignore the uid on the root
> switch:
>  - don't read it in tb_drom_read (route == 0 is already special cased anyways)
>  - add a special case for the root switch to tb_switch_resume and
> don't read the uid - just assume that it did not change (should be
> impossible anyways)
> 
> What do you think?

I think there actually is such check already in tb_switch_resume() where
we special case the root switch ignoring its UID. Unless I'm missing
something.

I'm testing this on a Mac with Cactus Ridge and the root switch resume
does not fail :)

[toc] | [prev] | [next] | [standalone]

#1647263 — Re: [PATCH 02/24] thunderbolt: Do not try to read UID if DROM offset is read as 0

On Mon, May 22, 2017 at 10:40 AM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> On Sun, May 21, 2017 at 03:46:20PM +0200, Andreas Noever wrote:
>> On Thu, May 18, 2017 at 4:38 PM, Mika Westerberg
>> <mika.westerberg@linux.intel.com> wrote:
>> > At least Falcon Ridge when in host mode does not have any kind of DROM
>> > available and reading DROM offset returns 0 for these. Do not try to
>> > read DROM any further in that case.
>> >
>> > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
>> > Reviewed-by: Yehezkel Bernat <yehezkel.bernat@intel.com>
>> > Reviewed-by: Michael Jamet <michael.jamet@intel.com>
>> > ---
>> >  drivers/thunderbolt/eeprom.c | 3 +++
>> >  1 file changed, 3 insertions(+)
>> >
>>
>> Hi Mika,
>>
>> nice work, it is nice to see Intel contribute to the Thunderbolt
>> driver (I can second Lukas's 'jaw drop' comment)!
>>
>> I will try to read through everything today, but maybe the last few
>> patches will get pushed back to next weekend.
>
> Thanks :)
>
>> > diff --git a/drivers/thunderbolt/eeprom.c b/drivers/thunderbolt/eeprom.c
>> > index 6392990c984d..e4e64b130514 100644
>> > --- a/drivers/thunderbolt/eeprom.c
>> > +++ b/drivers/thunderbolt/eeprom.c
>> > @@ -276,6 +276,9 @@ int tb_drom_read_uid_only(struct tb_switch *sw, u64 *uid)
>> >         if (res)
>> >                 return res;
>> >
>> > +       if (drom_offset == 0)
>> > +               return -ENODEV;
>> > +
>> I think that this will make tb_switch_resume bail out on the root
>> switch, which is not good. Since the uid is only used to detect
>> whether a different device was plugged in while the system was
>> suspended I think that we can safely ignore the uid on the root
>> switch:
>>  - don't read it in tb_drom_read (route == 0 is already special cased anyways)
>>  - add a special case for the root switch to tb_switch_resume and
>> don't read the uid - just assume that it did not change (should be
>> impossible anyways)
>>
>> What do you think?
>
> I think there actually is such check already in tb_switch_resume() where
> we special case the root switch ignoring its UID. Unless I'm missing
> something.
>
> I'm testing this on a Mac with Cactus Ridge and the root switch resume
> does not fail :)

Yes there is a check for the root switch, but also one that checks the
return code of tb_drom_read_uid_only :)

err = tb_drom_read_uid_only(sw, &uid);
if (err) {
    tb_sw_warn(sw, "uid read failed\n");
    return err;
}
if (sw != sw->tb->root_switch && sw->uid != uid) {


The reason it works on the Mac is because drom_offset is not 0, so the
new branch in tb_drom_read_uid_only is not taken. Probably this is the
case for all Cactus Ridge models and Alpine Ridge doesn't go there
since it uses the ICM? Still it wouldn't hurt to only read the uid if
sw != root_switch, the value is not used if sw == root_switch.

[toc] | [prev] | [next] | [standalone]

#1647374 — Re: [PATCH 02/24] thunderbolt: Do not try to read UID if DROM offset is read as 0

On Mon, May 22, 2017 at 08:41:22PM +0200, Andreas Noever wrote:
> Yes there is a check for the root switch, but also one that checks the
> return code of tb_drom_read_uid_only :)
> 
> err = tb_drom_read_uid_only(sw, &uid);
> if (err) {
>     tb_sw_warn(sw, "uid read failed\n");
>     return err;
> }
> if (sw != sw->tb->root_switch && sw->uid != uid) {
> 
> 
> The reason it works on the Mac is because drom_offset is not 0, so the
> new branch in tb_drom_read_uid_only is not taken. Probably this is the
> case for all Cactus Ridge models and Alpine Ridge doesn't go there
> since it uses the ICM? 

Yes in case of ICM we don't call the function at all.

> Still it wouldn't hurt to only read the uid if
> sw != root_switch, the value is not used if sw == root_switch.

I agree. I'll update the code so that it will only read and check UID
when we are not dealing with the root switch.

[toc] | [prev] | [next] | [standalone]

#1647380 — Re: [PATCH 02/24] thunderbolt: Do not try to read UID if DROM offset is read as 0

On Mon, May 22, 2017 at 10:38 PM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> On Mon, May 22, 2017 at 08:41:22PM +0200, Andreas Noever wrote:
>> Yes there is a check for the root switch, but also one that checks the
>> return code of tb_drom_read_uid_only :)
>>
>> err = tb_drom_read_uid_only(sw, &uid);
>> if (err) {
>>     tb_sw_warn(sw, "uid read failed\n");
>>     return err;
>> }
>> if (sw != sw->tb->root_switch && sw->uid != uid) {
>>
>>
>> The reason it works on the Mac is because drom_offset is not 0, so the
>> new branch in tb_drom_read_uid_only is not taken. Probably this is the
>> case for all Cactus Ridge models and Alpine Ridge doesn't go there
>> since it uses the ICM?
>
> Yes in case of ICM we don't call the function at all.
>
>> Still it wouldn't hurt to only read the uid if
>> sw != root_switch, the value is not used if sw == root_switch.
>
> I agree. I'll update the code so that it will only read and check UID
> when we are not dealing with the root switch.
Thanks,
Andreas

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web