Message-ID: <ZheAahcOoGemxGgw@cae.in-ulm.de>
Date: Thu, 11 Apr 2024 08:17:14 +0200
From: "Christian A. Ehrhardt" <lk@...e.de>
To: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
Cc: Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Neil Armstrong <neil.armstrong@...aro.org>,
	linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-arm-msm@...r.kernel.org
Subject: Re: [PATCH v2 3/3] usb: typec: ucsi_glink: drop special handling for
 CCI_BUSY


Hi Dmitry,

On Thu, Apr 11, 2024 at 12:09:31AM +0300, Dmitry Baryshkov wrote:
> On Wed, Apr 10, 2024 at 09:41:39AM +0200, Christian A. Ehrhardt wrote:
> > 
> > Hi Dmitry,
> > 
> > On Wed, Apr 10, 2024 at 01:58:58AM +0300, Dmitry Baryshkov wrote:
> > > On Tue, 9 Apr 2024 at 22:26, Christian A. Ehrhardt <lk@...e.de> wrote:
> > > >
> > > >
> > > > Hi Dmitry,
> > > >
> > > > On Tue, Apr 09, 2024 at 06:29:18PM +0300, Dmitry Baryshkov wrote:
> > > > > Newer Qualcomm platforms (sm8450+) successfully handle busy state and
> > > > > send the Command Completion after sending the Busy state. Older devices
> > > > > have firmware bug and can not continue after sending the CCI_BUSY state,
> > > > > but the command that leads to CCI_BUSY is already forbidden by the
> > > > > NO_PARTNER_PDOS quirk.
> > > > >
> > > > > Follow other UCSI glue drivers and drop special handling for CCI_BUSY
> > > > > event. Let the UCSI core properly handle this state.
> > > > >
> > > > > Reviewed-by: Heikki Krogerus <heikki.krogerus@...ux.intel.com>
> > > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
> > > > > ---
> > > > >  drivers/usb/typec/ucsi/ucsi_glink.c | 10 ++++------
> > > > >  1 file changed, 4 insertions(+), 6 deletions(-)
> > > > >
> > > > > diff --git a/drivers/usb/typec/ucsi/ucsi_glink.c b/drivers/usb/typec/ucsi/ucsi_glink.c
> > > > > index 9ffea20020e7..fe9b951f5228 100644
> > > > > --- a/drivers/usb/typec/ucsi/ucsi_glink.c
> > > > > +++ b/drivers/usb/typec/ucsi/ucsi_glink.c
> > > > > @@ -176,7 +176,8 @@ static int pmic_glink_ucsi_sync_write(struct ucsi *__ucsi, unsigned int offset,
> > > > >       left = wait_for_completion_timeout(&ucsi->sync_ack, 5 * HZ);
> > > > >       if (!left) {
> > > > >               dev_err(ucsi->dev, "timeout waiting for UCSI sync write response\n");
> > > > > -             ret = -ETIMEDOUT;
> > > > > +             /* return 0 here and let core UCSI code handle the CCI_BUSY */
> > > > > +             ret = 0;
> > > > >       } else if (ucsi->sync_val) {
> > > > >               dev_err(ucsi->dev, "sync write returned: %d\n", ucsi->sync_val);
> > > > >       }
> > > > > @@ -243,11 +244,8 @@ static void pmic_glink_ucsi_notify(struct work_struct *work)
> > > > >               ucsi_connector_change(ucsi->ucsi, con_num);
> > > > >       }
> > > > >
> > > > > -     if (ucsi->sync_pending && cci & UCSI_CCI_BUSY) {
> > > > > -             ucsi->sync_val = -EBUSY;
> > > > > -             complete(&ucsi->sync_ack);
> > > > > -     } else if (ucsi->sync_pending &&
> > > > > -                (cci & (UCSI_CCI_ACK_COMPLETE | UCSI_CCI_COMMAND_COMPLETE))) {
> > > > > +     if (ucsi->sync_pending &&
> > > > > +         (cci & (UCSI_CCI_ACK_COMPLETE | UCSI_CCI_COMMAND_COMPLETE))) {
> > > > >               complete(&ucsi->sync_ack);
> > > > >       }
> > > > >  }
> > > >
> > > > This handling of the command completion turned out to be racy in
> > > > the ACPI case: If a normal command was sent one should wait for
> > > > UCSI_CCI_COMMAND_COMPLETE only. In case of an UCSI_ACK_CC_CI
> > > > command the completion is indicated by UCSI_CCI_ACK_COMPLETE.
> > > >
> > > > While not directly related, a port of this
> > > >     https://lore.kernel.org/all/20240121204123.275441-3-lk@c--e.de/
> > > > would nicely fit into this series.
> > > 
> > > Ack, I'll take a look.
> > 
> > Thanks.
> > 
> > > However... I can not stop but notice that CCG and STM32 glue drivers
> > > use the same old approach as we do. Which probably means that they
> > > might have the same issue.
> > 
> > I did ping the ccg people wrt. this but they have a different
> > workaround that saves them at least most of the time, so I let
> > this drop.
> > 
> > > Could you please consider pulling up that
> > > code into the UCSI driver? Maybe the low-level code really should just
> > > read/write the messages, leaving all completions and CCI parsing to
> > > the core layer?
> > 
> > I did consider that but one of the ideas behind the new API for
> > UCSI backends was that backends can send commands (e.g. as part of
> > a quirk) even in the middle of a ->sync_write() call. Currently,
> > I don't really see how to combine this with completion handling
> > in the UCSI core.
> > 
> > > > I don't have the hardware to do this myself.
> > 
> > I did propose other changes to the API with little respone here:
> >     https://lore.kernel.org/all/20240218222039.822040-1-lk@c--e.de/
> > That could possibly be extended to achieve this. But again, that
> > would require testers for all the backends.
> 
> Well, I think that the patchset is too intrusive and (from the
> pmic-glink perspective) is too low-level.

Point taken.

> I'd start by pulling the sync_write() into the core layer, leaving just
> async_write in the glue layer. The async_write() then can be renamed to
> something like send_cmd(). Once required we can add the data pointer to
> this callback.

Fine with me.

However, this basically looks to me what we had before the transition
from the old to the new API wrt. command completion. So it basically
used to be that way and it was changed bei Heikki for a reason.
See below.

> I liked the idea of getting the CCI from the notification (in case of
> pmic-glink it works this way on all platforms except sc8180x).

Yeah. ACPI has a quirk to do just this for some platforms that won't
work properly otherwise. Additionally, strange things can happen if
CCI changes between command completion and the re-read in the UCSI
core. E.g. a command that ran into a timeout could be completed by
then or a busy flag seen in the event handler could be gone.

What's also nagging at me is the not so obvious fact that the
notification handling can but doesn't have to happen under the PPM
lock. So whatever we do there may still happend in the middle of a
->sync_write(). But maybe we should think about this later.

> So what about having a really simple interface:
> 
> sruct ucsi_operations {
> 	/*
> 	 * send the command without waiting for the result
> 	 * can be extended with u8 *data, size_t data_len once
> 	 * necessary.
> 	 * maybe use u8 control[8] instead of u64 control.
> 	 */
> 	int send_command(struct ucsi *, u64 control);
> 
> 	int read_data(struct ucsi *, void *buf, size_t len);
> 	int read_version(struct ucsi *, u16 *version);
> 	/* to be used only for reset handling */
> 	int read_cci(struct ucsi *, u32 cci);
> 
> 	// other ops like update_altmode, as is
> };

In the Dell quirk for ACPI the ->sync_write operation (at one point)
did the following (simplified):

- Send the command to the PPM and wait for the result
- If the command was an ACK for a connector change send another
  ack and wait for the result.
- Report the result of the original command to the UCSI core.

I.e. the quirk post-processes the result of the original command,
possibly sending other commands in the process.

The command completion was pushed into the backends when the new
API was introduced to allow this type of thing and keep it contained
in the backends and not in the UCSI core. See
   https://lore.kernel.org/all/20190926100727.71117-10-heikki.krogerus@linux.intel.com/
or the commit message of bdc62f2bae8fb0e8e99574de5232f0a3c54a27df
in mainline.

> /* to be called by the glue driver once it gets the notification from
>  * PPM */
> void ucsi_notify(struct ucsi *ucsi, u32 cci);
> 
> This way we can pull all the common ACK/connection_changed/completion
> code into the core, while keeping glue layers flexible enough.

I'm not saying that we shouldn't do this. But I'm pointing out that
doing it differently seems to have been a deliberate decision...


Best regards,
Christian

Powered by blists - more mailing lists