From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
        id S1756385AbcKKM22 (ORCPT <rfc822;w@1wt.eu>);
        Fri, 11 Nov 2016 07:28:28 -0500
Received: from bombadil.infradead.org ([198.137.202.9]:33521 "EHLO
        bombadil.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S1754170AbcKKM21 (ORCPT
        <rfc822;linux-kernel@vger.kernel.org>);
        Fri, 11 Nov 2016 07:28:27 -0500
Date: Fri, 11 Nov 2016 13:28:24 +0100
From: Peter Zijlstra <peterz@infradead.org>
To: Lu Baolu <baolu.lu@linux.intel.com>
Cc: Thomas Gleixner <tglx@linutronix.de>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Mathias Nyman <mathias.nyman@linux.intel.com>,
        Ingo Molnar <mingo@redhat.com>, linux-usb@vger.kernel.org,
        x86@kernel.org, LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v4 1/4] usb: dbc: early driver for xhci debug capability
Message-ID: <20161111122824.GH3117@twins.programming.kicks-ass.net>
References: <1477976354-13291-1-git-send-email-baolu.lu@linux.intel.com>
 <1477976354-13291-2-git-send-email-baolu.lu@linux.intel.com>
 <alpine.DEB.2.20.1611091030171.3501@nanos>
 <5823CB63.7090603@linux.intel.com>
 <alpine.DEB.2.20.1611100946190.3501@nanos>
 <20161110114445.GW3142@twins.programming.kicks-ass.net>
 <58254A19.8030003@linux.intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <58254A19.8030003@linux.intel.com>
User-Agent: Mutt/1.5.23.1 (2014-03-12)
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Fri, Nov 11, 2016 at 12:33:29PM +0800, Lu Baolu wrote:

> Things become complicated when it comes to USB debug port.
> But it's still addressable.
> 
> At this time, we can do it like this.
> 
> write()
> {
> 	if (in_nmi() && raw_spin_is_locked(&lock))
> 		return;
> 
> 	raw_spinlock_irqsave(&lock, flags);
> 	....
> 

Please use raw_spin_trlock_irqsave() instead, spin_is_locked() is fairly
icky.

Also, there's a bunch of exception contexts that do not set in_nmi().
That is in_nmi() is really only set for #NM. #MC and #DB and
others do not set this.

> This will filter some messages from NMI handler in case that
> another thread is holding the spinlock. I have no idea about
> how much chance could a debug user faces this. But it might
> further be fixed with below enhancement.
> 
> write()
> {
> 	if (in_nmi() && raw_spin_is_locked(&lock)) {
> 		produce_a_pending_item_in_ring();
> 		return;
> 	}
> 
> 	raw_spinlock_irqsave(&lock, flags);
> 
> 	while (!pending_item_ring_is_empty)
> 		consume_a_pending_item_in_ring();
> 
> 	....
> 
> 
> We can design the pending item ring in a producer-consumer
> model. It's easy to avoid race between the producer and
> consumer.

Problem is that the consumer might never happen, those are the fun most
bugs.

Not being able to deal with random nested exception context really
reduces the utility of this thing.

Again, a UART rules. Make a virtual UART in hardware, that'd be totally
awesome. This thing, I'm not convinced its worth having.