linux-kernel - RE: [PATCH 2/2] tty: serial: fsl_lpuart: writing a 1 and then a 0 to trigger a break character

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <AS8PR04MB8404B8E3EB0FFCEE8ADDA283928B9@AS8PR04MB8404.eurprd04.prod.outlook.com>
Date:   Fri, 15 Jul 2022 09:18:31 +0000
From:   Sherry Sun <sherry.sun@....com>
To:     Michael Walle <michael@...le.cc>
CC:     "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
        "jirislaby@...nel.org" <jirislaby@...nel.org>,
        "linux-serial@...r.kernel.org" <linux-serial@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        dl-linux-imx <linux-imx@....com>
Subject: RE: [PATCH 2/2] tty: serial: fsl_lpuart: writing a 1 and then a 0 to
 trigger a break character


> Subject: Re: [PATCH 2/2] tty: serial: fsl_lpuart: writing a 1 and then a 0 to
> trigger a break character
> 
> Hi,
> 
> Am 2022-07-15 09:20, schrieb Sherry Sun:
> >> Subject: Re: [PATCH 2/2] tty: serial: fsl_lpuart: writing a 1 and
> >> then a 0 to trigger a break character
> >>
> >> Hi,
> >>
> >> Am 2022-07-15 04:59, schrieb Sherry Sun:
> >> > According to the lpuart reference manual, need to writing a 1 and
> >> > then a
> >> > 0 to the UARTCTRL_SBK field queues a break character in the
> >> > transmit data stream. Only writing a 1 cannot trigger the break
> >> > character, so fix it.
> >>
> >> I don't think this is correct. The tty core will already call this:
> >>    .break_ctl(port, 1)
> >>    usleep()
> >>    .break_ctl(port, 0)
> >>
> >> So you'll have your 1->0 transition.
> >>
> >> My RM from the LS1028A says the following:
> >>
> >> | Writing a 1 and then a 0 to SBK queues a break character in the
> >> | transmit data stream. Additional break characters of 10 to 13, or
> >> | 13 to 16 if LPUART_STATBRK13] is set, bit times of logic 0 are
> >> | queued
> >> as
> >> | long as SBK is set. Depending on the timing of the set and clear of
> >> | SBK relative to the information currently being transmitted, a
> >> second
> >> | break character may be queued before software clears SBK.
> >>
> >> To me it seems that setting the SBK bit just pulls the TX line low
> >> and releasing it will return to normal transmitter mode.
> >>
> >
> > Hi Michael,
> >
> > Actually set break_ctl(tty, -1) then break_ctl(tty, 0) is only done in
> > the send_break() function.
> > If we call TIOCSBRK from user space, it will only set break_ctl(tty,
> > -1) without break_ctl(tty, 0).
> 
> That is expected. no? There is also the TIOCCBRK which will clear the
> break. TIOCSBRK will just turn the break on.
> 
> I'm not sure if the break is already transmitted when the SBK bit
> is set, though. Is that your problem here? I'd need to check that
> on the real hardware.
> 

Hi Michael,

Seems we have the different understanding of the break_ctl(port,ctl) callback. The original break_ctl(tty,-1) in lpuart will not send the break signal until we call break_ctl(tty,0). Per my understanding of "If ctl is nonzero, the break signal should be transmitted", call break_ctl(tty,-1) is enough the send one break signal, now my patch makes the behavior align with my understanding. 

And my understanding of break_ctl(tty,0) is that it will terminate the break signal send requirement which has not been done instead of cooperate with break_ctl(tty,-1) to finish one break character send behavior.

   | break_ctl(port,ctl)
   | Control the transmission of a break signal.  If ctl is
   | nonzero, the break signal should be transmitted.  The signal
   | should be terminated when another call is made with a zero
   | ctl.

Best regards
Sherry


> > And from the definition of .break_ctl(port,ctl), the callback is used
> > to Control the transmission of a break
> > signal(Documentation/driver-api/serial/driver.rst), if ctl is nonzero,
> > it should queues a break character. I don't think it is reasonable to
> > call break_ctl() twice in order to send one break signal.
> 
> Maybe Gred can correct me, but to me it seems like the .break_ctl()
> will set the *state* according to the argument, that is either
> turning it on or turning it off (Except if TTY_DRIVER_HARDWARE_BREAK
> is set, but that doesn't seem to be supported by the ioctl interface.)
> 
> > Also I have tried other uart IP, such as drivers/tty/serial/imx.c, it
> > also queues a break character if we call break_ctl() once. So I
> > believe the break_ctl() in lpuart driver should be fixed.
> 
> -michael