From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tomasz Wroblewski Subject: Re: [PATCH] PCI uart: fix boot hang, and second S3 resume inactive timer list corruption Date: Mon, 26 Aug 2013 13:39:04 +0200 Message-ID: <521B3E58.5020206@citrix.com> References: <521B1D24.1060903@citrix.com> <521B557002000078000EE575@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta3.messagelabs.com ([195.245.230.39]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1VDv9G-0002QT-HO for xen-devel@lists.xenproject.org; Mon, 26 Aug 2013 11:39:54 +0000 In-Reply-To: <521B557002000078000EE575@nat28.tlf.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: xen-devel List-Id: xen-devel@lists.xenproject.org On 08/26/2013 01:17 PM, Jan Beulich wrote: >>>> On 26.08.13 at 11:17, Tomasz Wroblewski wrote: >> - fix occasional xen boot hang whilst using PCI uart. Dom0 kernel disables ioport responses >> during PCI system initialisation, causing xen hang if __ns16550_poll() routine happens to >> be scheduled during that time. Detect and exit. Amended ns16550_ioport_invalid function >> to only check IER register, which contains three reservered (always 0) bits, therefore >> it's sufficient for this test. > And this was observed with 4.4-unstable? I'm asking because I > would at a first glance have thought that taking care of this > ought to be a desirable side effect of calling pci_hide_device(). This was observed with stable 4.3 - it seems to be doing the pci_hide_device as well, so I don't think this affects, or was it bugfixed later? I'm not entirely sure how is pci_hide_device supposed to work though - in my dom0, on 4.3, I am seeing the pci serial card used by xen console, so maybe it is bugged? (or i misunderstand it). >> +static int ns16550_ioport_invalid(struct ns16550 *uart) >> +{ >> + return (((unsigned char)ns_read_reg(uart, UART_IER)) == 0xff); >> +} > Why checking just one register is sufficient when originally > >> -static int ns16550_ioport_invalid(struct ns16550 *uart) >> -{ >> - return ((((unsigned char)ns_read_reg(uart, UART_LSR)) == 0xff)&& >> - (((unsigned char)ns_read_reg(uart, UART_MCR)) == 0xff)&& >> - (((unsigned char)ns_read_reg(uart, UART_IER)) == 0xff)&& >> - (((unsigned char)ns_read_reg(uart, UART_IIR)) == 0xff)&& >> - (((unsigned char)ns_read_reg(uart, UART_LCR)) == 0xff)); >> -} > we checked five also needs some better explanation. I believe it's enough to test IER register since it contains 3 reserved bits which are always 0 during normal operation, therefore the condition will never hit then. Made this as a mini optimisation since this function would now be called more frequently. > Jan >