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

[Nix <nix@esperi.org.uk>]

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)