From patchwork Sun Apr 9 13:09:42 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Mason X-Patchwork-Id: 9671539 X-Patchwork-Delegate: bhelgaas@google.com Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id C54BD601EA for ; Sun, 9 Apr 2017 13:10:11 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id A2F6C27F4B for ; Sun, 9 Apr 2017 13:10:11 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 8331A283FB; Sun, 9 Apr 2017 13:10:11 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.9 required=2.0 tests=BAYES_00,FREEMAIL_FROM, RCVD_IN_DNSWL_HI autolearn=ham version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 05CB527F4B for ; Sun, 9 Apr 2017 13:10:10 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752165AbdDINKJ (ORCPT ); Sun, 9 Apr 2017 09:10:09 -0400 Received: from smtp2-g21.free.fr ([212.27.42.2]:10598 "EHLO smtp2-g21.free.fr" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752150AbdDINKI (ORCPT ); Sun, 9 Apr 2017 09:10:08 -0400 Received: from [192.168.0.66] (unknown [88.191.210.51]) (Authenticated sender: shill) by smtp2-g21.free.fr (Postfix) with ESMTPSA id 45C64200348; Sun, 9 Apr 2017 15:09:43 +0200 (CEST) Subject: Re: [PATCH v3 2/2] PCI: Add tango PCIe host bridge support To: Bjorn Helgaas , linux-pci , Linux ARM Cc: Marc Gonzalez , Robin Murphy , Lorenzo Pieralisi , Liviu Dudau , David Laight , Thibaud Cornic , Phuong Nguyen , Prarit Bhargava References: <5309e718-5813-5b79-db57-9d702b50d0f9@sigmadesigns.com> <65114e62-7458-b6f7-327c-f07a5096a452@sigmadesigns.com> From: Mason Message-ID: Date: Sun, 9 Apr 2017 15:09:42 +0200 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:52.0) Gecko/20100101 Firefox/52.0 SeaMonkey/2.49 MIME-Version: 1.0 In-Reply-To: <65114e62-7458-b6f7-327c-f07a5096a452@sigmadesigns.com> Sender: linux-pci-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-pci@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On 29/03/2017 13:34, Marc Gonzalez wrote: > + /* > + * QUIRK #1 > + * Reads in configuration space outside devfn 0 return garbage. > + */ > + if (devfn != 0) { > + *val = ~0; /* Is this required even if we return an error? */ > + return PCIBIOS_FUNC_NOT_SUPPORTED; /* Error seems appropriate */ > + } AFAICT, the relevant caller is: bool pci_bus_read_dev_vendor_id(struct pci_bus *bus, int devfn, u32 *l, int crs_timeout) if (pci_bus_read_config_dword(bus, devfn, PCI_VENDOR_ID, l)) return false; Therefore, I believe updating *val is unnecessary. What matters is returning an error when appropriate. PCIBIOS_FUNC_NOT_SUPPORTED fits my purpose. > + > + /* > + * QUIRK #2 > + * The root complex advertizes a fake BAR, which is used to filter > + * bus-to-system requests. Hide it from Linux. > + */ > + if (where == PCI_BASE_ADDRESS_0 && bus->number == 0) { > + *val = 0; /* 0 or ~0 to hide the BAR from Linux? */ > + return PCIBIOS_SUCCESSFUL; /* Should we return error or success? */ > + } AFAICT, the relevant caller is: int __pci_read_base(struct pci_dev *dev, enum pci_bar_type type, struct resource *res, unsigned int pos) u32 l, sz, mask; pci_read_config_dword(dev, pos, &l); pci_write_config_dword(dev, pos, l | mask); pci_read_config_dword(dev, pos, &sz); pci_write_config_dword(dev, pos, l); Several things are note-worthy: 1) __pci_read_base() ignores the config accessors return value. Of the existing PCIBIOS errors, none seem to be a good fit for my use-case (hiding a non-standard BAR). Tangent: Maybe I should set dev->non_compliant_bars = true; instead of messing around in the accessor... I would likely set the flag in a PCI_FIXUP_EARLY function. /* Early fixups, before probing the BARs */ 1b) Perhaps a generic error could be added to the PCIBIOS_* error family, to signal that the requested operation was not completed, and *val remains unchanged. => Maybe PCIBIOS_FAILURE or PCIBIOS_NOP ? 2) Some drivers are not aware that *val needs to be updated for BAR accesses. e.g. drivers/pci/host/pcie-altera.c if altera_pcie_hide_rc_bar() is true, 'l' and 'sz' are not updated, and therefore contain garbage (uninitialized stack variables). I think we should apply the following trivial patch. Regards. --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c @@ -175,7 +175,7 @@ static inline unsigned long decode_bar(struct pci_dev *dev, u32 bar) int __pci_read_base(struct pci_dev *dev, enum pci_bar_type type, struct resource *res, unsigned int pos) { - u32 l, sz, mask; + u32 l = 0, sz = 0, mask; u64 l64, sz64, mask64; u16 orig_cmd; struct pci_bus_region region, inverted_region;