Re: [PATCH] USB: serial: cp210x: Adding tx_empty() to avoid cp2108 failure

[Johan Hovold <johan@kernel.org>]

On Fri, Oct 16, 2015 at 08:48:39AM -0500, Konstantin Shkolnyy wrote:
> On Fri, Oct 16, 2015 at 7:55 AM, Johan Hovold <johan@kernel.org> wrote:
> > On Thu, Oct 15, 2015 at 05:07:08PM -0500, Konstantin Shkolnyy wrote:
> >> Occasionally, writing data and immediately closing the port makes cp2108
> >> stop responding. The device had to be unplugged to clear the error.
> >> The failure is induced by shutting down the device while its Tx queue still has
> >> unsent data. Reporting the correct amount of those data avoids the problem.
> >> Adding tx_empty() has no adverse effect on other cp210x devices.
> [...]
> >
> > While tx_empty is a nice feature to have this does not seem to fully
> > address the problem you have identified. Specifically, you also need to
> > consider what happens if flow control is enabled. Then the TX buffer may
> > never drain, and you'd end up in the same situation.
> >
> > Could you first see if a simple purge command (0x12) to clear the tx
> > fifo from close is enough to fix the problem you're seeing? If so that
> > fix would be preferred as it is both more general and makes for smaller
> > patch more suitable for backporting.
> 
> I did consider the purge, but didn't want to kill bytes that could
> otherwise be successfully sent. By the description of tx_empty(), it
> sounded like something that just simply reports the status of the
> queue. It shouldn't cause any harm, if implemented. And, conveniently,
> it got rid of the failure I was seeing.

You're reasoning is sound, but if using purge works, it would be a more
generic (handling flow control) and less intrusive change, and as such
would be more suitable for backporting to stable.

We'd still want tx_empty (the basis for wait_until_sent) to avoid
unnecessarily dropping any data. But note that this would be a new
feature of the driver (and as such is not stable material) and should be
implemented on top of the fix.

> I'm new to this code. I don't know how flow control interacts with the
> rest of the logic. If it gets close() called even if there are still
> data in the Tx queue, then the purge may indeed be needed in close().
> Is this how it works?

Exactly. If you implement tx_empty the generic wait-until-sent
implementation will wait for the buffer to drain, but there would still
be a timeout in case the connection has stalled (e.g. due to flow
control). After that close() will always be called.

> [...]
> 
> >> +static bool cp210x_tx_empty(struct usb_serial_port *port)
> >> +{
> >> +     int err;
> >> +
> >> +     /* get_config needs "array of integers large enough", so pad to 0x14 bytes */
> >> +     struct cp210x_comm_status_container {
> >> +             struct cp210x_comm_status  sts; /* 0x13 bytes */
> >> +             u8                         pad_to_0x14_bytes;
> >> +     } comm_sts_cont;
> >> +
> >> +     err = cp210x_get_config(port, CP210X_GET_COMM_STATUS, (unsigned int *) &comm_sts_cont, 0x13);
> >
> > You should not use cp210x_get_config here at all and rather add a new
> > helper to read out the status that you call here after allocating a
> > buffer to store the result. Then use le32_to_cpu() to access the field
> > you're interested in.
> >
> > The byte fields at the end of the message will also be incorrectly
> > ordered otherwise.
> [...]
> 
> Do you mean that I should call cp210x_get_config from the helper?

No, just do the usb_control_msg call directly from your helper. We can
refactor that into a generic helper later (and kill off get_config).

Don't hesitate to ask if you have any further questions.

Thanks,
Johan