From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-1.0 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_PASS,UNPARSEABLE_RELAY autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 97071C282C4 for ; Tue, 5 Feb 2019 00:39:00 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 607EC206DD for ; Tue, 5 Feb 2019 00:39:00 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727882AbfBEAi7 (ORCPT ); Mon, 4 Feb 2019 19:38:59 -0500 Received: from belmont79srvr.owm.bell.net ([184.150.200.79]:46831 "EHLO mtlfep01.bell.net" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1725888AbfBEAi7 (ORCPT ); Mon, 4 Feb 2019 19:38:59 -0500 Received: from bell.net mtlfep01 184.150.200.30 by mtlfep01.bell.net with ESMTP id <20190205003857.SYCF28651.mtlfep01.bell.net@mtlspm02.bell.net> for ; Mon, 4 Feb 2019 19:38:57 -0500 Received: from [192.168.2.49] (really [70.53.62.20]) by mtlspm02.bell.net with ESMTP id <20190205003857.PUYE26679.mtlspm02.bell.net@[192.168.2.49]>; Mon, 4 Feb 2019 19:38:57 -0500 Subject: Re: [PATCH v2] net: dsa: mv88e6xxx: Revise irq setup ordering To: Andrew Lunn Cc: Russell King , Vivien Didelot , Florian Fainelli , netdev@vger.kernel.org References: <20190123002240.GF3634@lunn.ch> <20190130172818.GJ21904@lunn.ch> <2ea9fd81-f92d-9505-dd0b-bdd0f67d8ce7@bell.net> <20190130223846.GB30115@lunn.ch> <9415d82e-965b-7777-0ad0-f23d6c9f177e@bell.net> <53b49df8-53ed-704f-9197-230b18d83090@bell.net> <824d011b-3692-69c3-5e2c-58e950a80abf@bell.net> <20190204231410.GG3397@lunn.ch> From: John David Anglin Openpgp: preference=signencrypt Message-ID: <8d8123cc-60f0-e236-e496-0aacf735fceb@bell.net> Date: Mon, 4 Feb 2019 19:38:57 -0500 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:60.0) Gecko/20100101 Thunderbird/60.5.0 MIME-Version: 1.0 In-Reply-To: <20190204231410.GG3397@lunn.ch> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Content-Language: en-US X-Cloudmark-Analysis: v=2.2 cv=QLaHfkDL c=1 sm=0 tr=0 a=O1s9vCNsZHvkjbFmGvGkaA==:17 a=IkcTkHD0fZMA:10 a=CFTnQlWoA9kA:10 a=FBHGMhGWAAAA:8 a=rtm8CetPjXpvHzFXdTYA:9 a=QEXdDO2ut3YA:10 a=9gvnlMMaQFpL9xblJ6ne:22 Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org On 2019-02-04 6:14 p.m., Andrew Lunn wrote: > On Mon, Feb 04, 2019 at 04:59:13PM -0500, John David Anglin wrote: >> This change fixes a race condition in the setup of hardware irqs and the >> code enabling PHY link detection in the mv88e6xxx driver. >> >> This race was observed on the espressobin board where the GPIO interrupt >> controller only supports edge interrupts.  If the INTn output pin goes low >> before the GPIO interrupt is enabled, PHY link interrupts are not detected. >> >> With this change, we >> 1) force INTn high by clearing all interrupt enables in global 1 control 1, >> 2) setup the hardware irq, and then >> 3) perform the remaining common setup. >> >> This simplifies the setup and allows some unnecessary code to be removed. > Hi Dave > > I took a closer look now. I don't actually see why the current code is > wrong. The problem is INTn can go low before the interrupt handler for it is registered and enabled. As a result, interrupts never occur if link happens to come up before the interrupt handler completes being enabled. > > mv88e6xxx_g1_irq_setup() calls mv88e6xxx_g1_irq_setup_common() and > then registers the interrupt handler. > > mv88e6xxx_g1_irq_setup_common() does what you want, it masks all > interrupts in the hardware and clears any pending interrupts which can > be cleared. > > The change you made is actually dangerous. As soon as you request the > interrupt, it is live, it can fire, and call > mv88e6xxx_g1_irq_thread_work(). That needs the irq domain. But the > change you made defers the creating of the domain until after the > interrupt is registered. So we can de-refernece a NULL pointer in the > interrupt handler. This can't happen.  The domain is setup immediately after registering the GPIO interrupt. The interrupt can't fire until one of the enables is set.  These are set by mv88e6xxx_g2_irq_setup(), mv88e6xxx_g1_atu_prob_irq_setup() and mv88e6xxx_g1_vtu_prob_irq_setup().  These irqs are setup after mv88e6xxx_g1_irq_setup()/mv88e6xxx_irq_poll_setup() is called.  Thus, the irq domain is setup before the GPIO interrupt can fire. I have tested both hardware and polled interrupts using espressobin with v4.20.6 and networking starts correctly. Dave -- John David Anglin dave.anglin@bell.net