Received: by 2002:a25:8b91:0:0:0:0:0 with SMTP id j17csp3291413ybl; Fri, 20 Dec 2019 07:00:52 -0800 (PST) X-Google-Smtp-Source: APXvYqwMMN26FGA6yKGDpvJbBvz3SkzjZx7dVVLEDLPXIj5Ro8mnH4sGUTtgmDkstIeruWC02wzd X-Received: by 2002:a05:6830:689:: with SMTP id q9mr8094272otr.285.1576854052625; Fri, 20 Dec 2019 07:00:52 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1576854052; cv=none; d=google.com; s=arc-20160816; b=l2ssz0dzyNGYxfJhzhjWcv7wavFrhPkV7INQ6IZcIf6vudw4nvd+AC2UfX/z3hmtmp xvDSHLao1n1mOgqO0Kb48hXITmUIX+33oL4DPcRtjjJlkkljfn9X+EgKqZmbuKIq/Uhp 7P0IUlTgAzxESu+Xh4+lt8IZzE3aNaaGFmQmYQG6iDmBFfWc0V5nal8TpOKu5Mh4ab60 HsohJzHMC4/cADvUcknSd/4Y442NaT5ATNJcmbHqqM4blfLEmR8W2Zo6mlspXxayWP5Z SwULGmsBhlLqMe7U0LI7EYjszamCA0j4UcncBaeuV/FfepH97fZTYDXAMwKCvrOge5PX rhrw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:user-agent:in-reply-to :content-disposition:mime-version:message-id:subject:cc:to:from:date :dkim-signature; bh=BwimSFJsLM251EY7y9anyQXlyuUYCkMvHPVF48j06Ac=; b=PbnL4YDQfp9G8LPdyWCqXtNe5DH87OsafUmjEt81+RAiP7wn+WObLioYwy9Laix7L/ QvID60wP/XgJu/iglpX/EPen3g3qVfyYo0KjNBcrc8MGzWuZ9C2uIN+uDZpGCDpjkKmM dR999fT8h5dNQHWOrclscKslhfXfjCciLpHoke1xGWHtepj8aGdrpby+6p1Y0y9cZFEm w9WcwCqmUdShugCgf+/eBMivHt3o4tC8iV6dBAH/irX1Pc3k0LhlRQz7XfSGfEVbeXxm ThiYwrr62DG+pvWWFb4ZaJ0dqmMItd84/e/JHcwSujiklqriE0TZ/C1rzAsMoGX9THAx i58g== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@kernel.org header.s=default header.b=xaZvrMbY; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=kernel.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id f190si5021757oig.229.2019.12.20.07.00.38; Fri, 20 Dec 2019 07:00:52 -0800 (PST) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; dkim=pass header.i=@kernel.org header.s=default header.b=xaZvrMbY; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727451AbfLTO6f (ORCPT + 99 others); Fri, 20 Dec 2019 09:58:35 -0500 Received: from mail.kernel.org ([198.145.29.99]:49156 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727233AbfLTO6f (ORCPT ); Fri, 20 Dec 2019 09:58:35 -0500 Received: from localhost (mobile-166-170-223-177.mycingular.net [166.170.223.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id 478E021655; Fri, 20 Dec 2019 14:58:34 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1576853914; bh=m6fiKtTDXZuHPTihN9gMIJKc7nsDJK8i2aTkUAulxHY=; h=Date:From:To:Cc:Subject:In-Reply-To:From; b=xaZvrMbYpPIvyW61vg8KopAcqFXj0EyBVBdpHctX9Wl8WWu8TZAFr6cBVy/VtBtb2 GVU6Zq1oLxYcXvcXHsLeHIBiCqzS2dNbebMWaKzzU4Zn3aSpenGXU1TBOz1J+gY5wB v/w2VQoKtjzQNi7til2Za1g10efjSr3o0DDxBqiI= Date: Fri, 20 Dec 2019 08:58:32 -0600 From: Bjorn Helgaas To: Bharat Kumar Gogada Cc: linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org, rgummal@xilinx.com, Michal Simek , Ley Foon Tan , rfi@lists.rocketboards.org, Jingoo Han , Gustavo Pimentel Subject: Re: [PATCH 2/2] PCI: Versal CPM: Add support for Versal CPM Root Port driver Message-ID: <20191220145832.GA93984@google.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1576842072-32027-3-git-send-email-bharat.kumar.gogada@xilinx.com> User-Agent: Mutt/1.10.1 (2018-07-13) Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org [+cc other drivers that use the broken "is link up" test in config access] On Fri, Dec 20, 2019 at 05:11:12PM +0530, Bharat Kumar Gogada wrote: > - Adding support for versal CPM as Root Port. > - The Versal ACAP devices include CCIX-PCIe Module (CPM). The integrated > block for CPM along with the integrated bridge can function > as PCIe Root Port. > - Versal CPM uses GICv3 ITS feature for assigning MSI/MSI-X > vectors and handling MSI/MSI-X interrupts. > - Bridge error and legacy interrupts in versal CPM are handled using > versal CPM specific MISC interrupt line. "Versal" appears to be a brand name and should be capitalized consistently. > +#define INTX_NUM 4 Don't we have a generic PCI core definition for this? > +static inline bool cpm_pcie_link_is_up(struct xilinx_cpm_pcie_port *port) Please follow the example of other drivers and name this "cpm_pcie_link_up()". > +{ > + return (pcie_read(port, XILINX_CPM_PCIE_REG_PSCR) & > + XILINX_CPM_PCIE_REG_PSCR_LNKUP) ? 1 : 0; > +} > +/** > + * xilinx_cpm_pcie_valid_device - Check if a valid device is present on bus > + * @bus: PCI Bus structure > + * @devfn: device/function > + * > + * Return: 'true' on success and 'false' if invalid device is found > + */ > +static bool xilinx_cpm_pcie_valid_device(struct pci_bus *bus, > + unsigned int devfn) > +{ > + struct xilinx_cpm_pcie_port *port = bus->sysdata; > + > + /* Check if link is up when trying to access downstream ports */ > + if (bus->number != port->root_busno) > + if (!cpm_pcie_link_is_up(port)) > + return false; This check for the link being up is racy and should be removed. The link may go down after the check and before the config access. A config access to a device where the link is down is an error, but it's an error we expect to happen because of enumeration, surprise unplug, or electrical issue. It's impossible to avoid so we must be able to handle and recover from it. I know there are other drivers that do this (dwc, altera, xilinix-nwl, xilinx), and I've pointed this out many times in the past. This needs to be fixed. > + > + /* Only one device down on each root port */ > + if (bus->number == port->root_busno && devfn > 0) > + return false; > + > + return true; > +} > +static irqreturn_t xilinx_cpm_pcie_intr_handler(int irq, void *data) > +{ > + struct xilinx_cpm_pcie_port *port = > + (struct xilinx_cpm_pcie_port *)data; struct device *dev = port->dev; to reduce repetition of "port->dev" below. > + u32 val, mask, status, bit; > + unsigned long intr_val; > + > + /* Read interrupt decode and mask registers */ > + val = pcie_read(port, XILINX_CPM_PCIE_REG_IDR); > + mask = pcie_read(port, XILINX_CPM_PCIE_REG_IMR); > + > + status = val & mask; > + if (!status) > + return IRQ_NONE; > + > + if (status & XILINX_CPM_PCIE_INTR_LINK_DOWN) > + dev_warn(port->dev, "Link Down\n"); > + > + if (status & XILINX_CPM_PCIE_INTR_HOT_RESET) > + dev_info(port->dev, "Hot reset\n"); > ...