linux-kernel - Re: [PATCH v2 2/8] serial: sh-sci: Check if TX data was written to device in .tx

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: <d19d12e7-aabb-460c-a37c-6cbd3fe4e459@tuxon.dev>
Date: Tue, 12 Nov 2024 10:35:13 +0200
From: Claudiu Beznea <claudiu.beznea@...on.dev>
To: Jiri Slaby <jirislaby@...nel.org>, geert+renesas@...der.be,
 magnus.damm@...il.com, robh@...nel.org, krzk+dt@...nel.org,
 conor+dt@...nel.org, mturquette@...libre.com, sboyd@...nel.org,
 gregkh@...uxfoundation.org, p.zabel@...gutronix.de, g.liakhovetski@....de,
 lethal@...ux-sh.org
Cc: linux-renesas-soc@...r.kernel.org, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-clk@...r.kernel.org,
 linux-serial@...r.kernel.org,
 Claudiu Beznea <claudiu.beznea.uj@...renesas.com>, stable@...r.kernel.org
Subject: Re: [PATCH v2 2/8] serial: sh-sci: Check if TX data was written to
 device in .tx_empty()

Hi, Jiri,

On 08.11.2024 14:19, Claudiu Beznea wrote:
>>> @@ -885,6 +887,7 @@ static void sci_transmit_chars(struct uart_port *port)
>>>           }
>>>             sci_serial_out(port, SCxTDR, c);
>>> +        s->first_time_tx = true;
>>>             port->icount.tx++;
>>>       } while (--count > 0);
>>> @@ -1241,6 +1244,8 @@ static void sci_dma_tx_complete(void *arg)
>>>       if (kfifo_len(&tport->xmit_fifo) < WAKEUP_CHARS)
>>>           uart_write_wakeup(port);
>>>   +    s->first_time_tx = true;
>> This is too late IMO. The first in-flight dma won't be accounted in
>> sci_tx_empty(). From DMA submit up to now.
> If it's in-flight we can't determine it's status anyway with one variable.
> We can set this variable later but it wouldn't tell the truth as the TX
> might be in progress anyway or may have been finished?
> 
> The hardware might help with this though the TEND bit. According to the HW
> manual, the TEND bit has the following meaning:
> 
> 0: Transmission is in the waiting state or in progress.
> 1: Transmission is completed.
> 
> But the problem, from my point of view, is that the 0 has double meaning.
> 
> I noticed the tx_empty() is called in kernel multiple times before
> declaring TX is empty or not. E.g., uart_suspend_port() call it 3 times,
> uart_wait_until_sent() call it in a while () look with a timeout. There is
> the uart_ioctl() which calls it though uart_get_lsr_info() only one time
> but I presumed the user space might implement the same multiple trials
> approach before declaring it empty.
> 
> Because of this I considered it wouldn't be harmful for the scenario you
> described "The first in-flight dma won't be accounted in sci_tx_empty()"
> as the user may try again later to check the status. For this reason I also
> chose to have no extra locking around this variable.
> 
> Please let me know if you consider otherwise.

With the above explanation, can you please let me know if you still
consider I should change the approach for this patch?

Thank you,
Claudiu Beznea