linux-kernel - Re: [PATCH] AT91 Serial: update the powersave handler to match serial core

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]

Message-ID: <20080919150938.08b2c7be@hskinnemo-gx745.norway.atmel.com>
Date:	Fri, 19 Sep 2008 15:09:38 +0200
From:	Haavard Skinnemoen <haavard.skinnemoen@...el.com>
To:	Anti Sullin <anti.sullin@...ecdesign.ee>
Cc:	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] AT91 Serial: update the powersave handler to match
 serial core

Anti Sullin <anti.sullin@...ecdesign.ee> wrote:
> This problem seems to be unnoticed so far:
> 
> Patch http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=b3b708fa2780cd2b5d8266a8f0c3a1cab364d4d2
> has changed the serial core behavior to not to suspend the port if the device is enabled as a wakeup source. If the
> AT91 system goes to slow clock mode, the port should be suspended always and the clocks should be switched off. 
> The patch attached updates the atmel_serial driver to match the changes in serial core.

Right...I guess this might also help get rid of the warning about
unbalanced irqwake that I've been meaning to look at for a while now...

> Also, the interrupts are disabled when the clock is disabled. If we disable the clock with interrupts enabled, an interrupt
> may get stuck. If this is the DBGU interrupt, this blocks the OR logic at system controller and thus all other sysc interrupts.

Not good. I suspect we need to stop DMA as well though...or wait until
it's drained.

> The patch is against 2.6.25.3 + at91 patchset at maxim.org.za. 

It doesn't apply to latest mainline, but I'll try to cram it in somehow.

A few questions though...

> diff -purN linux-2.6.25.3/drivers/serial/atmel_serial.c linux-2.6.25.3_ok/drivers/serial/atmel_serial.c
> --- linux-2.6.25.3/drivers/serial/atmel_serial.c	2008-05-10 07:48:50.000000000 +0300
> +++ linux-2.6.25.3_ok/drivers/serial/atmel_serial.c	2008-09-08 12:35:35.000000000 +0300
> @@ -132,7 +132,8 @@ struct atmel_uart_char {
>  struct atmel_uart_port {
>  	struct uart_port	uart;		/* uart */
>  	struct clk		*clk;		/* uart clock */
> -	unsigned short		suspended;	/* is port suspended? */
> +	int			may_wakeup;		/* cached value of device_may_wakeup for times we need to disable it */
> +	int			backup_imr;		/* back up the IMR during suspend */

Since this is a hardware register, shouldn't it be u32?

> @@ -1264,6 +1273,8 @@ void __init atmel_register_uart_fns(stru
>  #ifdef CONFIG_SERIAL_ATMEL_CONSOLE
>  static void atmel_console_putchar(struct uart_port *port, int ch)
>  {
> +	if (port->suspended) return;
> +
>  	while (!(UART_GET_CSR(port) & ATMEL_US_TXRDY))
>  		cpu_relax();
>  	UART_PUT_CHAR(port, ch);
> @@ -1278,6 +1289,8 @@ static void atmel_console_write(struct c
>  	unsigned int status, imr;
>  	unsigned int pdc_tx;
>  
> +	if (port->suspended) return;
> +

Are both of these checks necessary? It doesn't look like
atmel_console_putchar() can be called from anywhere else than
atmel_console_write().

Also, does the no_console_suspend parameter still work after this?

Haavard
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/