From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755895AbbJGUC1 (ORCPT ); Wed, 7 Oct 2015 16:02:27 -0400 Received: from mout.kundenserver.de ([212.227.126.130]:50029 "EHLO mout.kundenserver.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751509AbbJGUCZ (ORCPT ); Wed, 7 Oct 2015 16:02:25 -0400 From: Arnd Bergmann To: Krzysztof =?utf-8?q?Ha=C5=82asa?= Subject: Re: [PATCH] ARM: cns3xxx: pci: avoid potential stack overflow Date: Wed, 7 Oct 2015 22:01:49 +0200 User-Agent: KMail/1.12.2 (Linux/3.19.0-27-generic; KDE/4.3.2; x86_64; ; ) Cc: linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org References: <2273145.0Ls8cProhs@wuerfel> In-Reply-To: MIME-Version: 1.0 Content-Type: Text/Plain; charset="utf-8" Content-Transfer-Encoding: 8bit Message-Id: <201510072201.51697.arnd@arndb.de> X-Provags-ID: V03:K0:F3H4VnxvTkdeowK6kgU63IHl+3ZSIIzNm0RHgOeErY8Ip+nNmew tV2c5GxjTl2tW4OpGcvk75yxBvE/dm8YXBUlYMv4pX/eOd6radQL6qaukgE5yN8RqwTp+ie DxeSrb8aUhrH/RT/R1KWTb54/qL0OM3TXWOI6CLqIX06zo/OKvNE5U93L36NOh0x9RppTZO FnlmKZqF0FnIQUNZ4QUbQ== X-UI-Out-Filterresults: notjunk:1;V01:K0:Cq/PsLdwcbU=:KNqNg5GY9FBu9HyKSi/+jj 9LLAZ04/Ldj6r8HYqclLN02txA0XsMz6Hkv0WK1CZXYYEG11GBzMi1cQUDwjTHSeLLKeOS7Y9 tISjQcq96W7+9MgBpPO/mwnNd9cfvekfkcxoBFv9+/pJl9YzX3pF7neBaOQesqiBwoBLpELL5 tZOaHjfnlTYUr+xuG9xAsx4yVcONIFWIm2ZmUoWg5ISa3kZZy2tzdNimhdZnq9nVQRVjyBYUC qJk0gE/fKZ1Vn989km3eJatDDmm4B3epcrJAju1JefTGEc+gX3X+guNGisRelFiDjh7V497Ha pse4aFpE+6gLtUaqnVAL0qV6F485ZS4k5BNseWzxN1BAwU6d1JJdXqvn/2XEhFyPGY783ZdKJ o2LnHe9pptGutKnmSGjDPB7Ejdtyl1F2q33uql1iGu0CadiHZ3a3Bc+GtuPAkrOyXzBd9aWfW 6Xa+xN2UkhUO6THZDyRmfMJJ3VMbj2Ucz2xtsDoTkJP3PiDgAha4Y6zw9+oUgTCP8iD//R0Bt J3x6VAqAinHN/SOCI0ysvkrJcLY22znxqR/RvsVvRnM8ujk0++IfGt/P72qTCVMdmTiaeG/t2 pJahaUiF1Kgkz6FIaUEr4+vShGLR8hq6C39Cs+hLKIfe80LmOzgZFfLD2hq9zlXPqWdG+sjFz GGyq2n/GbyTdhFunQu2ZfQHu8SzrOxM6ZaNHodS2MnAZBanN9Unqs2xIYFl/8R0CZlZO629zM tGdzhuP0YM4s1TeS Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wednesday 27 May 2015, Krzysztof Hałasa wrote: > Arnd Bergmann writes: > > > The cns3xxx_pcie_hw_init function uses excessive kernel > > stack space because of a hack that puts a fake struct > > pci_sys_data and struct pci_bus on the stack in order to > > call the generic pci_bus_read_config accessors, which causes > > a warning in ARM allmodconfig builds: > > > > arch/arm/mach-cns3xxx/pcie.c:266:1: warning: the frame size of 1080 bytes is larger than 1024 bytes > > > > This rewrites the code in question to use a private > > implementation of the config space access for the same > > purpose, getting rid of the local variables and the > > warning in the process. As part of this, we have to > > use an open-coded version of pci_bus_find_capability(), > > which unfortunately complicates the implementation. > > Wouldn't it be better to simply use static structs for this purpose? > The hack isn't pretty, though. Hi Krzysztof, sorry for the late reply. I sent the patch shortly before my parental leave and have only now picked up this work again. I've looked at the driver once more and have come up with a modified approach that should hopefully address all the concerns. > > + regs = cnspci->cfg0_regs + (PCI_DEVFN(1, 0) << 12); > > + > > Some comment about would be helpful. Such as this: > > > > - bus.number = 1; /* directly connected PCIe device */ > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Agreed. I think I've managed to get rid of this part completely though. > > + pos = cns3xxx_pci_raw_read_config(regs, PCI_CAPABILITY_LIST, 1); > > + while (cns3xxx_pci_raw_read_config(regs, pos, 1) != PCI_CAP_ID_EXP) > > + pos = cns3xxx_pci_raw_read_config(regs, pos + PCI_CAP_LIST_NEXT, 1); > > + > > I wonder if this can fail (i.e., no PCI_CAP_ID_EXP capability). This is now gone too, so we no longer have to worry about it. I was mistakingly assuming that these were registers inside of the SoC rather than in the device that gets attached. > > + dc = cns3xxx_pci_raw_read_config(regs, pos + PCI_EXP_DEVCTL, 2); > > + dc &= ~(0x3 << 12); /* Clear Device Control Register [14:12] */ > > + cns3xxx_pci_raw_write_config(regs, pos + PCI_EXP_DEVCTL, 2, dc); > > + dc = cns3xxx_pci_raw_read_config(regs, pos + PCI_EXP_DEVCTL, 2); > > + if (!(dc & (0x3 << 12))) > > + pr_info("PCIe: Set Device Max_Read_Request_Size to 128 byte\n"); > > + > > This seems to revert 367dc4b75f4349d5363bc3ebdc030939db944786. Why do > you want to do it? My mistake. Gone too now. Thanks for your careful review back then! Arnd