From patchwork Wed Jun 1 13:29:13 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Arnd Bergmann X-Patchwork-Id: 102265 Delivered-To: patch@linaro.org Received: by 10.140.92.199 with SMTP id b65csp96368qge; Wed, 1 Jun 2016 06:28:50 -0700 (PDT) X-Received: by 10.98.28.148 with SMTP id c142mr8900670pfc.102.1464787730635; Wed, 01 Jun 2016 06:28:50 -0700 (PDT) Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id r68si51170429pfi.40.2016.06.01.06.28.50; Wed, 01 Jun 2016 06:28:50 -0700 (PDT) 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; 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 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1758257AbcFAN2s (ORCPT + 30 others); Wed, 1 Jun 2016 09:28:48 -0400 Received: from mout.kundenserver.de ([212.227.126.133]:54844 "EHLO mout.kundenserver.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753396AbcFAN2q (ORCPT ); Wed, 1 Jun 2016 09:28:46 -0400 Received: from wuerfel.localnet ([78.42.132.4]) by mrelayeu.kundenserver.de (mreue005) with ESMTPSA (Nemesis) id 0M21V9-1bN1UF0BVT-00u2ab; Wed, 01 Jun 2016 15:28:36 +0200 From: Arnd Bergmann To: Yuval Mintz Cc: David Miller , Manish Chopra , Sudarsana Kalluru , netdev , linux-kernel , Ariel Elior Subject: [PATCH v2] qed: fix qed_fill_link() error handling Date: Wed, 01 Jun 2016 15:29:13 +0200 Message-ID: <11393820.LSE4DkNrNd@wuerfel> User-Agent: KMail/5.1.3 (Linux/4.4.0-22-generic; KDE/5.18.0; x86_64; ; ) In-Reply-To: References: <1464623197-2084229-1-git-send-email-arnd@arndb.de> <8209894.K472MDjuEr@wuerfel> MIME-Version: 1.0 X-Provags-ID: V03:K0:R5p0YbecU9HEQbxuaIzvopnN/34DVG1dMhcCLgoInivoHXh3xau 6G6mQNBebkzo5mCNMaKTMLQjK2Qb6fHpq94L6OB/IigoUN1RLNBteqzWfnic+peAAIQhYz2 No/m0XlHbn8osEL5rD7CJM5dMBw2Fcx6QkVFSpaJMZkK8WNBY4Xm76RD8ow2uzYq7pG05A6 Xscq2THzBhKPU+ftZ1ZyA== X-UI-Out-Filterresults: notjunk:1; V01:K0:OVoD4VbRQyc=:elrIYw8ZM+cC082sHjgv2D pY/k3PvRXiCNgUMF9pKN+UwLu3dlp0l3UX6SgMQNYIWkMG/FwBRxcVQbMDblW4H9694zv4qOj biR7EpPqlTPKfC3hMynbzYcKYYR4XmWZRERmh1oaE+EBgs1msZYhjMTVPc5FGiOrK1Tf92N+V hHIPbJE4RbuY/vIgkc5oAxJLY8ut3Vzf2Xl6KJGVCp2/imIfntgq4VB89neC5haUm8TfJ3zqi QhKgHiXr77Ar19ecB5uFtftkuriMfHxQs1htX6vrAktZwHu0T5GJUvNYxqjA3TYXxenHcbUDt T/BSe79EX7EEXSskK2MsWFdI04tYuonS7Rc1yoEjwLRen9aJLx70NtqCPVR922noCvgJCRCtO zFy2WK7L6vI+rNHg8aK+Atw10TbA+afLilEq99hN/OdetrkRJXxBEF/qn+qEvPO7NzK6eLjmc lIpgKQ8LioX0bYjRmIyU5/r6kYYKTFixhxf7wpHpVCX7WMSP0fURJmmIEXgAbIijVsNsqlp75 5sFxtr3yNfcyvoRP9RSk7aW0Xye2atO8OWCWtbHG1WQ0t5JuWV+cN/FbvrfHc/L8MRuOrE2qn Va1Bs6IWaReP7mCRMx5Ernvj6yd9jnPnlr7C2HyPLaL8EgI1wFcF4FLB3KcvEc+tI/NEZNVee B2SsXM599looU3xWjPK3W6phyu8b4pTuY6cZ6LpSg2GfR8X+y/E/F9VUZAgqescFWFS06kubk 8e1TZIwFLimaHq1N Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org gcc warns about qed_fill_link possibly accessing uninitialized data: drivers/net/ethernet/qlogic/qed/qed_main.c: In function 'qed_fill_link': drivers/net/ethernet/qlogic/qed/qed_main.c:1170:35: error: 'link_caps' may be used uninitialized in this function [-Werror=maybe-uninitialized] While this warning is only about the specific case of CONFIG_QED_SRIOV being disabled but the function getting called for a VF (which should never happen), another possibility is that qed_mcp_get_*() fails without returning data. This rearranges the code so we bail out in either of the two cases and print a warning instead of accessing the uninitialized data. The qed_link_output structure remains untouched in this case, but all callers first call memset() on it, so at least we are not leaking stack data then. As discussed, we also use a compile-time check to ensure we never use any of the VF code if CONFIG_QED_SRIOV is disabled, and the PCI device table is updated to no longer bind to virtual functions in that configuration. Signed-off-by: Arnd Bergmann --- On Wednesday, June 1, 2016 11:10:30 AM CEST Yuval Mintz wrote: > Actually, I think VF probe should gracefully fail in that case, > as qed_vf_hw_prepare() would simply return -EINVAL. > But I can honestly say I've never tested this flow, and I agree there's > no reason to allow VF probe in case we're not supporting SRIOV. ok > So I guess removing the PCI ID and defining IS_PF to be true in case > CONFIG_QED_SRIOV isn't set is the right way to go. > Do you want to revise your patch, or do you want me to do it? I've done the patch below now, please either Ack or modify it the way you like and forward it. Thanks, Arnd diff --git a/drivers/net/ethernet/qlogic/qed/qed_main.c b/drivers/net/ethernet/qlogic/qed/qed_main.c index 753064679bde..61cc6869fa65 100644 --- a/drivers/net/ethernet/qlogic/qed/qed_main.c +++ b/drivers/net/ethernet/qlogic/qed/qed_main.c @@ -1105,6 +1105,39 @@ static int qed_get_port_type(u32 media_type) return port_type; } +static int qed_get_link_data(struct qed_hwfn *hwfn, + struct qed_mcp_link_params *params, + struct qed_mcp_link_state *link, + struct qed_mcp_link_capabilities *link_caps) +{ + void *p; + + if (!IS_PF(hwfn->cdev)) { + qed_vf_get_link_params(hwfn, params); + qed_vf_get_link_state(hwfn, link); + qed_vf_get_link_caps(hwfn, link_caps); + + return 0; + } + + p = qed_mcp_get_link_params(hwfn); + if (!p) + return -ENXIO; + memcpy(params, p, sizeof(*params)); + + p = qed_mcp_get_link_state(hwfn); + if (!p) + return -ENXIO; + memcpy(link, p, sizeof(*link)); + + p = qed_mcp_get_link_capabilities(hwfn); + if (!p) + return -ENXIO; + memcpy(link_caps, p, sizeof(*link_caps)); + + return 0; +} + static void qed_fill_link(struct qed_hwfn *hwfn, struct qed_link_output *if_link) { @@ -1116,15 +1149,9 @@ static void qed_fill_link(struct qed_hwfn *hwfn, memset(if_link, 0, sizeof(*if_link)); /* Prepare source inputs */ - if (IS_PF(hwfn->cdev)) { - memcpy(¶ms, qed_mcp_get_link_params(hwfn), sizeof(params)); - memcpy(&link, qed_mcp_get_link_state(hwfn), sizeof(link)); - memcpy(&link_caps, qed_mcp_get_link_capabilities(hwfn), - sizeof(link_caps)); - } else { - qed_vf_get_link_params(hwfn, ¶ms); - qed_vf_get_link_state(hwfn, &link); - qed_vf_get_link_caps(hwfn, &link_caps); + if (qed_get_link_data(hwfn, ¶ms, &link, &link_caps)) { + dev_warn(&hwfn->cdev->pdev->dev, "no link data available\n"); + return; } /* Set the link parameters to pass to protocol driver */ diff --git a/drivers/net/ethernet/qlogic/qed/qed_sriov.h b/drivers/net/ethernet/qlogic/qed/qed_sriov.h index c8667c65e685..c90b2b6ad969 100644 --- a/drivers/net/ethernet/qlogic/qed/qed_sriov.h +++ b/drivers/net/ethernet/qlogic/qed/qed_sriov.h @@ -12,11 +12,13 @@ #include "qed_vf.h" #define QED_VF_ARRAY_LENGTH (3) +#ifdef CONFIG_QED_SRIOV #define IS_VF(cdev) ((cdev)->b_is_vf) #define IS_PF(cdev) (!((cdev)->b_is_vf)) -#ifdef CONFIG_QED_SRIOV #define IS_PF_SRIOV(p_hwfn) (!!((p_hwfn)->cdev->p_iov_info)) #else +#define IS_VF(cdev) (0) +#define IS_PF(cdev) (1) #define IS_PF_SRIOV(p_hwfn) (0) #endif #define IS_PF_SRIOV_ALLOC(p_hwfn) (!!((p_hwfn)->pf_iov_info)) diff --git a/drivers/net/ethernet/qlogic/qede/qede_main.c b/drivers/net/ethernet/qlogic/qede/qede_main.c index 5d00d1404bfc..5733d1888223 100644 --- a/drivers/net/ethernet/qlogic/qede/qede_main.c +++ b/drivers/net/ethernet/qlogic/qede/qede_main.c @@ -87,7 +87,9 @@ static const struct pci_device_id qede_pci_tbl[] = { {PCI_VDEVICE(QLOGIC, PCI_DEVICE_ID_57980S_100), QEDE_PRIVATE_PF}, {PCI_VDEVICE(QLOGIC, PCI_DEVICE_ID_57980S_50), QEDE_PRIVATE_PF}, {PCI_VDEVICE(QLOGIC, PCI_DEVICE_ID_57980S_25), QEDE_PRIVATE_PF}, +#ifdef CONFIG_QED_SRIOV {PCI_VDEVICE(QLOGIC, PCI_DEVICE_ID_57980S_IOV), QEDE_PRIVATE_VF}, +#endif { 0 } };