linux-kernel - Re: [PATCH 1/3] tty: serial: 8250_omap: do not use RX DMA if pause is not supported

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]

Date:	Sat, 8 Aug 2015 08:40:01 -0700
From:	Greg KH <greg@...ah.com>
To:	Sebastian Andrzej Siewior <bigeasy@...utronix.de>
Cc:	Peter Hurley <peter@...leysoftware.com>,
	Vinod Koul <vinod.koul@...el.com>,
	Russell King <rmk+kernel@....linux.org.uk>,
	Dan Williams <dan.j.williams@...el.com>,
	dmaengine@...r.kernel.org, linux-kernel@...r.kernel.org,
	nsekhar@...com, linux-omap@...r.kernel.org,
	linux-serial@...r.kernel.org, john.ogness@...utronix.de,
	Peter Ujfalusi <peter.ujfalusi@...com>
Subject: Re: [PATCH 1/3] tty: serial: 8250_omap: do not use RX DMA if pause
 is not supported

On Sat, Aug 08, 2015 at 11:32:05AM +0200, Sebastian Andrzej Siewior wrote:
> On 08/08/2015 02:28 AM, Peter Hurley wrote:
> >> diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
> >> index 0340ee6ba970..07a11e0935e4 100644
> >> --- a/drivers/tty/serial/8250/8250_omap.c
> >> +++ b/drivers/tty/serial/8250/8250_omap.c
> >> @@ -769,7 +771,9 @@ static void omap_8250_rx_dma_flush(struct uart_8250_port *p)
> >>  		return;
> >>  	}
> >>  
> >> -	dmaengine_pause(dma->rxchan);
> >> +	ret = dmaengine_pause(dma->rxchan);
> >> +	if (WARN_ON_ONCE(ret))
> >> +		priv->rx_dma_broken = true;
> > 
> > No offense, Sebastian, but it boggles my mind that anyone could defend this
> > as solid api design. We're in the middle of an interrupt handler and the
> > slave dma driver is /just/ telling us now that it doesn't implement this
> > functionality?!!?
> 
> This is the best thing I could come up with. But to be fair: This
> happens once _very_ early in the process and is 100% reproducible. The
> WARN_ON should trigger user attention.

Never expect a _user_ to do anything with a WARN_ON except ignore it.

WARN_ON is for developers when something really bad went wrong that you
want to be notified so that you can fix the code to prevent that from
ever happening again.

So this isn't ok, sorry, a user would not know what to do with this at
all except email me asking why the driver is broken because a kernel
"oops" just happened, when in reality, their hardware is broken in a way
that you already know about when you wrote the patch.

You know better than this.

greg k-h
--
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/