Re: [PATCH v4] ucsi_ccg: Refine the UCSI Interrupt handling

[Greg Kroah-Hartman]

On Tue, Feb 21, 2023 at 04:40:24PM +0000, Jon Hunter wrote:
> Hi Greg,
> 
> On 19/01/2023 12:28, Greg Kroah-Hartman wrote:
> > On Wed, Jan 18, 2023 at 02:15:23PM +0800, Haotien Hsu wrote:
> > > From: Sing-Han Chen <singhanc@xxxxxxxxxx>
> > > 
> > > For the CCGx, when the OPM field in the INTR_REG is cleared, then the
> > > CCI data in the PPM is reset.
> > > 
> > > To align with the CCGx UCSI interface guide, this patch updates the
> > > driver to copy CCI and MESSAGE_IN before clearing UCSI interrupt.
> > > When a new command is sent, the driver will clear the old CCI and
> > > MESSAGE_IN copy.
> > > 
> > > Finally, clear UCSI_READ_INT before calling complete() to ensure that
> > > the ucsi_ccg_sync_write() would wait for the interrupt handling to
> > > complete.
> > > It prevents the driver from resetting CCI prematurely.
> > > 
> > > Signed-off-by: Sing-Han Chen <singhanc@xxxxxxxxxx>
> > > Signed-off-by: Haotien Hsu <haotienh@xxxxxxxxxx>
> > > ---
> > > V1->V2
> > > - Fix uninitialized symbol 'cci'
> > > v2->v3
> > > - Remove misusing Reported-by tags
> > > v3->v4
> > > - Add comments for op_lock
> > > ---
> > >   drivers/usb/typec/ucsi/ucsi_ccg.c | 90 ++++++++++++++++++++++++++++---
> > >   1 file changed, 83 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/drivers/usb/typec/ucsi/ucsi_ccg.c b/drivers/usb/typec/ucsi/ucsi_ccg.c
> > > index eab3012e1b01..532813a32cc1 100644
> > > --- a/drivers/usb/typec/ucsi/ucsi_ccg.c
> > > +++ b/drivers/usb/typec/ucsi/ucsi_ccg.c
> > > @@ -192,6 +192,12 @@ struct ucsi_ccg_altmode {
> > >   	bool checked;
> > >   } __packed;
> > > +#define CCGX_MESSAGE_IN_MAX 4
> > > +struct op_region {
> > > +	u32 cci;
> > 
> > This is coming from hardware so you have to specify the endian-ness of
> > it, right?
> 
> The current driver reads the 'cci' state in the ccg_irq_handler and here we
> just pass a variable of type u32 for storing the state. We are just adding
> variable of the same type to save the state. This value is returned to the
> ucsi layer which does not specify the endian-ness either. I guess this
> driver like many assume little endian. What is the guidance here? Should we
> be adding __le32 here even if the upper layers don't?

Yes, set what you are reading from the hardware, and then do the proper
transformation to the cpu native types for the upper layers where
needed.

> > Or why even clean this out at all, what happens if you do not?
> 
> 
> I have been taking a look at this. If we don't clean the variable and
> buffer, then the previous state could be incorrectly read again after the
> next command has been sent.
> 
> Without this fix we occasionally see timeout errors such as ...
> 
>    ucsi_ccg 2-0008: error -ETIMEDOUT: PPM init failed (-110)
> 
> 
> I tried not doing this at all, but then we see these timeout issues are
> still seen.

Then that means someone is not properly handling errors, and assuming
that whatever data is in the buffer is correct?  Try fixing that bug :)

See my other comments about not handling errors, perhaps that is where
the problem is.

thanks,

greg k-h