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 15:25:24 +0200 Message-ID: <521B5744.1070608@citrix.com> References: <521B1D24.1060903@citrix.com> <521B557002000078000EE575@nat28.tlf.novell.com> <521B3E58.5020206@citrix.com> <521B6C1E02000078000EE640@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 1VDwoF-00055Y-1L for xen-devel@lists.xenproject.org; Mon, 26 Aug 2013 13:26:19 +0000 In-Reply-To: <521B6C1E02000078000EE640@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 >>> 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). > Wait, yes, pci_ro_device() is what would be needed to drop > Dom0 writes to the device's config space. But we don't want > this if at all possible, as there may be other devices (more > serial ports and/or one or more parallel ports) on the same > card, and we want to allow Dom0 to drive those. > > Nevertheless, the approach of your patch in simply giving up > the device (even if only termporarily) looks questionable to me > We'd rather need to restore full access to it I would think. But > yes, this hypervisor and Dom0 playing with the same device is > sort of a gray area. Restore ioport access at the start of poll routine (if not on) and disable it again at the end (if was not on)? I might do that (if you really prefer), but intuitively that seems more likely to produce side effects in dom0 kernel than skipping a poll in xen >>>> +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. > I assumed it was something like this. But that needs to be said in > the patch description. Yeah, I did mention it in the desc though, to quote: "Amended ns16550_ioport_invalid function to only check IER register, which contains three reservered (always 0) bits, therefore it's sufficient for this test", thought that was enough > Jan >