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> <20221221070122.4167-1-zhouruihai@huaqin.corp-partner.google.com>
In-Reply-To: <20221221070122.4167-1-zhouruihai@huaqin.corp-partner.google.com>
From:   Prashant Malani <pmalani@chromium.org>
Date:   Wed, 21 Dec 2022 01:20:47 -0800
Message-ID: <CACeCKaeVMDWNCmatKBk+V60K_Cqct1gGT+iDk6Et08dY-BCdWQ@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:     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 11:01 PM Ruihai Zhou
<zhouruihai@huaqin.corp-partner.google.com> wrote:
>
> >>> 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.
>
> Thank you for the reply. The "HDMI DB" refer to the daughter board
> attached to the mainboard, which don't change anytime. In fact,
> the corsola EC increase the port count dynamically with some delay
> (no see a standard yet) during bootup. There is the EC code to
> increase the port count [1]. If the cros-ec-typec get the type-c
> port counts from EC before the EC increase the port counts, then
> will probe failed if return -EINVAL. I think the cros-ec-typec
> is replying on the fragile delay in EC, cros-ec-typec module will
> not get the fake(HDMI) type-c port counts once the EC deferred
> call is later than the driver probe. That is why I make this
> change. For the verification, the driver always probe failed
> when EC reboot without the patch. But the driver probe passed with
> the patch after a few times deferred probe.

Sorry, but I agree with Guenter; I don't think this is justification to add a
hack for 1 board in this driver.

In what situation does the EC task take so long after reboot that it
occurs after a module probe (which occurs shortly after system boot up)?

Why is the "corsola" board increasing the Type-C port count dynamically
after some delay, and what is that delay amount? Why can't this be set
via CBI/DT or its equivalent on your ARM platform?

This seems like fragile code in the EC and should be fixed there. The
number of Type-C ports reported by the EC really should be invariant
across the boot lifecycle. I don't think this patch can be accepted.

Frankly, I'm concerned that doing this might be introducing some subtle
bugs in the EC Type-C code too (I'm certain there are assumptions
about port count being static there too).

If you really need to change the number of ports dynamically in the EC,
add a board specific-hook to not respond to the EC_CMD_USB_PD_PORTS
host command until you have "set" the number of ports.

Best regards,

- Prashant