Re: [3.16.1 BISECTED REGRESSION]: Simtec Entropy Key (cdc-acm) broken in 3.16

From: Nix
Date: Tue Oct 14 2014 - 10:44:56 EST


On 14 Oct 2014, Johan Hovold spake thusly:

> On Sun, Oct 12, 2014 at 10:36:30PM +0100, Nix wrote:
>> On 12 Oct 2014, Johan Hovold verbalised:
>>
>> > On Sat, Oct 11, 2014 at 11:24:59PM +0100, Nix wrote:
>> >> On 11 Oct 2014, Paul Martin spake thusly:
>> >>
>> >> > Having been privy to the firmware of the eKey, it is very simplisting,
>> >> > with no implementation whatsoever of any flow control.
>> >>
>> >> That's what I thought. (Why would something that just provides data at a
>> >> constant rate way below that of even the slowest USB bus *need* flow
>> >> control?)
>> >>
>> >> One presumes therefore that the kernel suddenly trying to do flow
>> >> control on shutdown would fubar the firmware's internal state, leading
>> >> to the symptoms I see.
>> >
>> > The cdc-acm driver was dropping DTR/RTS on shutdown (close) also before
>> > the commit you refer to. One thing it did change however is that this is
>> > now only done if HUPCL is set. Might setting that flag be enough to
>> > prevent the device firmware from crashing?
>>
>> If I read the ekeyd 1.1.5 source code correctly, this is already
>> happening:
>>
>> ,----[ host/stream.c:estream_open() ]
>> | } else if (S_ISCHR(sbuf.st_mode)) {
>> | /* Open the file as a character device/tty */
>> | fd = open(uri, O_RDWR | O_NOCTTY);
>> | if ((fd != -1) && (isatty(fd))) {
>> | if (tcgetattr(fd, &settings) == 0 ) {
>> | settings.c_cflag &= ~(CSIZE | CSTOPB | PARENB | CLOCAL |
>> | CREAD | PARODD | CRTSCTS);
>> | settings.c_iflag &= ~(BRKINT | IGNPAR | PARMRK | INPCK |
>> | ISTRIP | INLCR | IGNCR | ICRNL | IXON |
>> | IXOFF | IXANY | IMAXBEL);
>> | settings.c_iflag |= IGNBRK;
>> | settings.c_oflag &= ~(OPOST | OCRNL | ONOCR | ONLRET);
>> | settings.c_lflag &= ~(ISIG | ICANON | IEXTEN | ECHO |
>> | ECHOE | ECHOK | ECHONL | NOFLSH |
>> | TOSTOP | ECHOPRT | ECHOCTL | ECHOKE);
>> | settings.c_cflag |= CS8 | HUPCL | CREAD | CLOCAL;
>> | #ifdef EKEY_FULL_TERMIOS
>> | settings.c_cflag &= ~(CBAUD);
>> | settings.c_iflag &= ~(IUTF8 | IUCLC);
>> | settings.c_oflag &= ~(OFILL | OFDEL | NLDLY | CRDLY | TABDLY |
>> | BSDLY | VTDLY | FFDLY | OLCUC );
>> | settings.c_oflag |= NL0 | CR0 | TAB0 | BS0 | VT0 | FF0;
>> | settings.c_lflag &= ~(XCASE);
>> | #endif
>> | settings.c_cflag |= B115200;
>> | if (tcsetattr(fd, TCSANOW, &settings) < 0) {
>> `----
>>
>> Note the HUPCL in there.
>>
>> I have checked: this code is being executed against a symlink that
>> points to /dev/ttyACM0, and the tcsetattr() succeeds. (At least, it's
>> succeeding on the kernel I'm running now, but of course that's 3.16.5
>> with this commit reverted...)
>
> You could verify that by enabling debugging in the cdc-acm driver and
> making sure that the corresponding control messages are indeed sent on
> close.

Yeah, good idea -- in the specific context of the system rebooting, in
particular (though I'll also see if just killing the daemon causes this
problem, something I haven't tested). I was assuming it would be hard to
see it before the reboot process cleared the screen -- but of course
this machine has a serial console so that's not important.

> But you haven't seen any fw crashes since you reverted the commit in
> question?

Not one.

> Another thing you could try is to add back the
>
> acm_set_control(acm, 0);
>
> just after the dev_info message in probe.

I'll try that tonight.

--
NULL && (void)
--
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/