On Thu, May 17, 2018 at 05:39:21AM +0200, Florian Zumbiehl wrote:
> Hi,
>
> > > Note that the patch has only been fully tested with kernel 4.9.28, and
> > > test-compiled against 4.16.8. Tested only with the pl2303 adapters I have
> > > available (which seem to be type 0 if I interpret the code correctly), I
> > > don't have a clue whether this works with other versions of the chip.
> >
> > What's the usb-devices (or lsusb -v) output for your device?
>
> | T: Bus=06 Lev=01 Prnt=01 Port=02 Cnt=02 Dev#= 4 Spd=12 MxCh= 0
> | D: Ver= 1.10 Cls=00(>ifc ) Sub=00 Prot=00 MxPS=64 #Cfgs= 1
> | P: Vendor=067b ProdID=2303 Rev=03.00
> | S: Manufacturer=Prolific Technology Inc.
> | S: Product=USB-Serial Controller
> | C: #Ifs= 1 Cfg#= 1 Atr=80 MxPwr=100mA
> | I: If#= 0 Alt= 0 #EPs= 3 Cls=ff(vend.) Sub=00 Prot=00 Driver=pl2303
That looks like an HX device according to the current way we identify
these types (which may be wrong).
I have a similar device here but with Rev 04.00 and I can confirm that
your patch works with that one too.
> > > --- linux-4.16.8/drivers/usb/serial/pl2303.c.orig 2018-05-09 09:53:14.000000000 +0200
> > > +++ linux-4.16.8/drivers/usb/serial/pl2303.c 2018-05-14 04:32:27.860780856 +0200
> > > @@ -544,8 +544,12 @@ static void pl2303_set_termios(struct tt
> > > int ret;
> > > u8 control;
> > >
> > > - if (old_termios && !tty_termios_hw_change(&tty->termios, old_termios))
> > > - return;
> > > + if (old_termios &&
> > > + !(tty_termios_hw_change(&tty->termios, old_termios) ||
> > > + ((tty->termios.c_iflag ^ old_termios->c_iflag) & (IXON | IXANY)) ||
> > > + tty->termios.c_cc[VSTOP] != old_termios->c_cc[VSTOP] ||
> > > + tty->termios.c_cc[VSTART] != old_termios->c_cc[VSTART]))
> >
> > Use a helper function for this (if everything is indeed needed, see
> > below).
> >
> > > + return;
> >
> > Odd indentation (which checkpatch would have caught).
>
> Well, it did, but it didn't exactly offer a suggestion as to how to
> reformat things that would still be readable, nor does the style guide,
> AFAICS, so I kept it as it is.
You used double indentation (two tabs further) for the inner block
(return statement) instead of one.
Continuation lines (e.g. the broken if statement) should be indented
substantially to the right, which some subsystems interpret as at least
two tabs further (you used just one).
if (........................................... &&
...) {
return;
}
> (Also, I doubt that had I encountered this
> condition moved out of line into a separate function that would have made
> it easier for me to follow what's going on.)
Depends on how you name your helper, I'd say. Another option could be to
use an intermediate bool. Either way, the above is hardly readable and
must be fixed.
> > > buf = kzalloc(7, GFP_KERNEL);
> > > if (!buf) {
> > > @@ -662,6 +666,9 @@ static void pl2303_set_termios(struct tt
> > > pl2303_vendor_write(serial, 0x0, 0x41);
> > > else
> > > pl2303_vendor_write(serial, 0x0, 0x61);
> > > + } else if (I_IXON(tty) && !I_IXANY(tty) &&
> > > + START_CHAR(tty) == 0x11 && STOP_CHAR(tty) == 0x13) {
> >
> > If the device does not support IXANY, I think you should just clear that
> > bit to indicate lack of support.
> >
> > And the start and stop characters cannot be modified (as far as you
> > know)? Then we should probably set these in the termios whenever IXON is
> > set as well since usbserial does not support falling back to the
> > line-discipline IXON implementation.
>
> I don't know how to enable IXANY or change the control characters, so I am
> working with what I do know (also, I guess the screenshot of the "ROM
> programming tool" in the "datasheet" suggests that none of that is
> supported).
Fair enough, that's why I asked. My device exhibits the same behaviour
with regards to IXANY by the way.
> Now, I agree that signaling lack of support would be better in general, but
> it does change what the code does for (still) unsupported cases. After all,
> usbserial in general does not signal that it doesn't support IXON/IXANY,
> but just pretends that it does. So, if there is a good reason why usbserial
> ignores POSIX on this point, I would think that would still apply for the
> remaining unsupported cases after partial support is implemented?!
There are historical reasons for a lot of things, but that's not
necessarily a reason to continue taking shortcuts.
> However, in any case, the part of setting particular control characters
> only when IXON is set seems like a very bad idea, in that it potentially
> changes settings where no change was requested, so userspace code cannot
> really be expected to check for that (and certainly not based on POSIX,
> which explicitly states that other fields have to stay unchanged). So, if
> we want to go with indicating lack of support, I think the correct strategy
> would be to always force the control characters to what the hardware
> supports.
You're right, but since the defaults termios control characters match
what the device uses, you can still reset them whenever user space tries
to change them to indicate lack of support (instead of silently
disabling sw flow control while leaving IXON set).
> > > + pl2303_vendor_write(serial, 0x0, 0xc0);
> >
> > Odd indentation again.
>
> See above.
You too. ;)
Thanks,
Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
