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

[Konstantin Shkolnyy]

On Fri, Oct 16, 2015 at 7:55 AM, Johan Hovold <johan@xxxxxxxxxx> 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.

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?
[...]

>> +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?

Thanks,
Konstantin
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/