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

[Andreas Noever]

On Mon, May 22, 2017 at 10:40 AM, Mika Westerberg
<mika.westerberg@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx>
>> > Reviewed-by: Yehezkel Bernat <yehezkel.bernat@xxxxxxxxx>
>> > Reviewed-by: Michael Jamet <michael.jamet@xxxxxxxxx>
>> > ---
>> >  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.