linux-kernel - Re: [PATCH 1/3] serial: 8250: Handle UART without interrupt on TEMT using em485

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>] [day] [month] [year] [list]

Date:   Fri, 29 Jan 2021 11:22:03 -0500
From:   Eric Tremblay <etremblay@...tech-controls.com>
To:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc:     gregkh@...uxfoundation.org, jslaby@...e.com,
        matwey.kornilov@...il.com, giulio.benetti@...ronovasrl.com,
        lukas@...ner.de, linux-serial@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        christoph.muellner@...obroma-systems.com, heiko@...ech.de,
        heiko.stuebner@...obroma-systems.com
Subject: Re: [PATCH 1/3] serial: 8250: Handle UART without interrupt on TEMT
 using em485

On 2021-01-29 6:22 a.m., Andy Shevchenko wrote:
> On Thu, Jan 28, 2021 at 06:36:27PM -0500, Eric Tremblay wrote:
>> The patch introduce the UART_CAP_TEMT capability which is by default
>> assigned to all 8250 UART since the code assume that device has the
>> interrupt on TEMT
> You have missed periods in the sentences here and there. Please, check the
> grammar and punctuation everywhere.
>
>> In the case where the device does not support it, we calculate the
>> maximum of time it could take for the transmitter to empty the
> maximum time
>
>> shift register. When we get in the situation where we get the
>> THRE interrupt but the TEMT bit is not set we start the timer
>> and recall __stop_tx after the delay
> __stop_tx()

I will review the grammar and spelling, thanks for mentioning it

>
> ...
>
>>  	/* initialize data */
>> -	up.capabilities = UART_CAP_FIFO | UART_CAP_MINI;
>> +	data->uart.capabilities = UART_CAP_FIFO | UART_CAP_MINI | UART_CAP_TEMT;
> I didn't get, if you state that CAP_TEMT is default on all UARTs, why you have
> this?

It's a merge mistake, sorry for that. The next version will use the 
reverse capability like Jiri Slaby suggested, there will be no needs to
modify other driver.

>
>> -	up.capabilities = UART_CAP_FIFO;
>> +	up.capabilities = UART_CAP_FIFO | UART_CAP_TEMT;
> And so this?
>
> ...
>
>> +static inline void serial8250_em485_update_temt_delay(struct uart_8250_port *p,
>> +			unsigned int cflag, unsigned int baud)
>> +{
>> +	unsigned int bits;
>> +
>> +	if (!p->em485)
>> +		return;
>> +
>> +	/* byte size and parity */
>> +	switch (cflag & CSIZE) {
>> +	case CS5:
>> +		bits = 7;
>> +		break;
>> +	case CS6:
>> +		bits = 8;
>> +		break;
>> +	case CS7:
>> +		bits = 9;
>> +		break;
>> +	default:
>> +		bits = 10;
>> +		break; /* CS8 */
>> +	}
>> +
>> +	if (cflag & CSTOPB)
>> +		bits++;
>> +	if (cflag & PARENB)
>> +		bits++;
> This is repetition of uart_update_timeout(). Find a way to deduplicate.
>
>> +	p->em485->no_temt_delay = bits*1000000/baud;
> Use spaces.
> Is this magic should be defined as HZ_PER_MHZ?
>
>> +}
> ...
>
>> +static void start_hrtimer_us(struct hrtimer *hrt, unsigned long usec)
>> +{
>> +	long sec = usec / 1000000;
>> +	long nsec = (usec % 1000000) * 1000;
>
> USEC_PER_SEC
> NSEC_PER_USEC
>
>> +	ktime_t t = ktime_set(sec, nsec);
>> +
>> +	hrtimer_start(hrt, t, HRTIMER_MODE_REL);
>> +}
> ...
>
>> +		if ((lsr & BOTH_EMPTY) != BOTH_EMPTY) {
>> +			/*
>> +			 * On devices with no interrupt on TEMT available
> "with no TEMT interrupt available"
>
>> +			 * start a timer for a byte time, the timer will recall
>> +			 * __stop_tx
> __stop_tx().
>
>> +			 */
>> +			if (!(p->capabilities & UART_CAP_TEMT) && (lsr & UART_LSR_THRE)) {
>> +				em485->active_timer = &em485->no_temt_timer;
>> +				start_hrtimer_us(&em485->no_temt_timer, em485->no_temt_delay);
>> +			}
> Perhaps
> 			if ((p->capabilities & UART_CAP_TEMT) && (lsr & UART_LSR_THRE))
> 				return;
>
> 			em485->active_timer = &em485->no_temt_timer;
> 			start_hrtimer_us(&em485->no_temt_timer, em485->no_temt_delay);
>
> ?

I also prefer that form, I will apply it in next version

>
>>  			return;
>> +		}