Message-ID: <CAL_jEz_LAjgZrPZHiHg6mKCwq7pfCh5fP_LN=NYc+oNH-Fwa4w@mail.gmail.com>
Date:	Tue, 20 Oct 2015 12:27:57 -0500
From:	Konstantin Shkolnyy <konstantin.shkolnyy@...il.com>
To:	Johan Hovold <johan@...nel.org>
Cc:	linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] USB: serial: cp210x: Implement 16-bit register access functions

On Tue, Oct 20, 2015 at 11:22 AM, Johan Hovold <johan@...nel.org> wrote:
> On Tue, Oct 20, 2015 at 09:19:05AM -0500, Konstantin Shkolnyy wrote:
>> On Tue, Oct 20, 2015 at 8:02 AM, Johan Hovold <johan@...nel.org> wrote:
>> > On Tue, Oct 20, 2015 at 07:52:31AM -0500, Konstantin Shkolnyy wrote:
>> >> On Tue, Oct 20, 2015 at 2:45 AM, Johan Hovold <johan@...nel.org> wrote:
>> >> [...]
>> >> > Instead of adding the new helpers to read u16 as a prerequisite for
>> >> > fixing the broken cp2108 support, just reuse the current config register
>> >> > helpers for now (in order to keep the fixes minimal and potentially
>> >> > backportable). Once the fixes are in place, feel free to clean up the
>> >> > remaining register accesses.
>> >>
>> >> The current helpers take "port" as a parameter. You pointed out
>> >> previously that port shouldn't be used in probe(). That made me
>> >> implement new helpers cp210x_write_u16_reg and cp210x_read_u16_reg
>> >> that don't use port. Probe() now calls cp210x_activate_workarounds
>> >> which in turn calls these new helpers.
>> >
>> > Oh, that's right.
>> >
>> >> An alternative would be to call usb_control_msg from
>> >> cp210x_activate_workarounds, but I think it would make it look pretty
>> >> ugly.
>> >
>> > Or you move the quirk-detect (and private data allocation) to port_probe
>> > instead (and remove startup/release). These devices have exactly one
>> > port per interface, so you wouldn't introduce any redundancy.
>>
>> OK, how about this:
>> patch #1
>> - attach/release replaced with port_probe/port_release;
>> - changes in the code using the "private" data since it has moved from
>> serial to port;
>> - cp210x_activate_workarounds in the port_probe, using the current helpers;
>> - a new helper cp210x_get_line_ctl that calls the current helper and
>> swaps the bytes.
>> patch #2
>> - sending "purge" from close, using the current helper.
>>
>> However, the Tx queue bug fixed by the "purge" doesn't have a
>> detection method. We only know that it exists in the same cp2108 that
>> has the "byte swap" bug. So the "purge" is activated by the same "byte
>> swap" flag and  patch #2 depends on patch #1.
>
> So you only see the hang after close with data in fifo on cp2108? Is
> this something that might get fixed as well in later revisions?
>
> But doing a purge as part of close should not be a problem on the other
> products, right? I think doing this unconditionally would be best (just
> like you eventually add tx_empty for all devices). No need to depend on
> the broken-lcr-quirk.

Theoretically it shouldn't be a problem, but since it hasn't been
there for other devices, not causing any complaints, I'm reluctant to
do this.

>> Also, there is no reason
>> to only apply one of the patches. Do you think we can merge these
>> patches?
>
> No, again you want one logical change per patch.
>
> I suggest you split it as follows
>
>  - fix hang with non-empty tx fifo
>  - use port data for private data
>  - fix broken cp2108 lcr

Yes, this looks OK for an "unconditional purge" option.
Otherwise it would be
- use port data for private data
- fix both bugs

Since the purge workaround is almost a one-liner, it won't noticeably
increase the diff, and nobody would ever want to apply just one
workaround of the 2 and continue to experience device hangs.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists