Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20221220055954.11197-1-zhouruihai@huaqin.corp-partner.google.com>
 <CABXOdTeJuFn-23OiTshJTQWMBvkiqOXUh6S8z354eEPzj+q4Ew@mail.gmail.com> <CAFUSabk3Lut1sHXOQi=ArtS9BvyqZQcTawKBVHupRJt+zOVaMg@mail.gmail.com>
In-Reply-To: <CAFUSabk3Lut1sHXOQi=ArtS9BvyqZQcTawKBVHupRJt+zOVaMg@mail.gmail.com>
From:   Guenter Roeck <groeck@google.com>
Date:   Tue, 20 Dec 2022 20:10:42 -0800
Message-ID: <CABXOdTdt+iW5e6X1ZAKfqWdkdZOrQ0-5bLoJA8s8vBmSi7f9zg@mail.gmail.com>
Subject: Re: [PATCH] platform/chrome: cros_ec_typec: deferred probe when typec
 count mismatch
To:     Ruihai Zhou <zhouruihai@huaqin.corp-partner.google.com>
Cc:     pmalani@chromium.org, bleung@chromium.org, groeck@chromium.org,
        knoxchiou@chromium.org, weishunc@chromium.org,
        chrome-platform@lists.linux.dev, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Tue, Dec 20, 2022 at 6:10 PM Ruihai Zhou
<zhouruihai@huaqin.corp-partner.google.com> wrote:
>>
>> I think that is problematic. It might as well be that nports >
>> EC_USB_PD_MAX_PORTS.
>>
>  Yes, you're right. so we should consider it's a invalid argument and return -EINVAL if nports > EC_USB_PD_MAX_PORTS. right?

Why ? While this would be a bug, it should hopefully be found early in
development and never hit the field. I don't see a reason for changing
the code.

>>
>> Is this really seen in the field ? The EC should never report a wrong
>> (random) number of ports. If it is not ready, there should be _some_
>> indication that it isn't ready. Does it really report a more or less
>> random number in this case ?
>
>  Yes, I saw this on corsola board. The EC report a wrong number(not random), because corsola emulates HDMI MUX over the current
>  type-c mux stack. The ec has to fake a type-c port to pass the MUX info. But the task are not initiated on starting up, and increase the
>  type-c port counts after the tasks finished. In this case, I saw the typec->num_ports = 1, but the nports = 2, which will be probe failed and
>  block the HDMI mux function.
>
>  I will send v2 patch, if nports > EC_USB_PD_MAX_PORTS, then return -EINVAL, but if nports > typec->num_ports, we consider wait a second
>  to ec task increase the type-c port counts if there're a HDMI DB attach, then return -EPROBE_DEFER

The current code just reduces nports if it is larger than
EC_USB_PD_MAX_PORTS. Again, I don't see a reason to change that.

Anyway, I am not sure if the above will work reliably. I am not sure
what "HDMI DB" refers to, but if it is an external connector its
status could change anytime. In that situation, no amount of waiting
would help. Either case, the EC on corsola should really not change
the number of ports it supports. Either it is a constant that should
not change, or it is dynamic and the EC would need to tell the host if
the number of ports changes (up or down). Trying to fix this in
cros_ec_typec without well defined protocol exchange with the EC is at
best a kludge, but not a real solution.

Thanks,
Guenter