From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757245AbcFALDN (ORCPT ); Wed, 1 Jun 2016 07:03:13 -0400 Received: from mout.kundenserver.de ([212.227.126.134]:53005 "EHLO mout.kundenserver.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750716AbcFALDL (ORCPT ); Wed, 1 Jun 2016 07:03:11 -0400 From: Arnd Bergmann To: Yuval Mintz Cc: David Miller , Manish Chopra , Sudarsana Kalluru , netdev , linux-kernel , Ariel Elior Subject: Re: [PATCH] qed: fix qed_fill_link() error handling Date: Wed, 01 Jun 2016 13:03:33 +0200 Message-ID: <8209894.K472MDjuEr@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> <5288285.fZuaAytaxX@wuerfel> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" X-Provags-ID: V03:K0:RxXPqVR5X08LzFYv9PbdbO3/CGo6qcafSdwXg/kFgnfG+5RNqxt SLVr92n9cbXw2wwjg63kbZ6VNjLo1Ie3oD9ERTTkN11tcA+1thHq5zk4AZQaAxCuv/kYUlK pOBG9pW3ZwsqlrbHSnmiQgWxNjpX++gNuiSHTC26EcOUkzPWZSutzi6FnYs8JG+CLZ7kZ3e k3gxN6dcX1oIC2s+j4Kdw== X-UI-Out-Filterresults: notjunk:1;V01:K0:mevXafQez+o=:4emi9S48Nhf5Ljk4vUWvNK Od7KzTB6sP6TWGRECdLBzebET90s89HauuXZRVx8Wf9+Tn+fjIqtUq/NsJIVKji8huXzwMp2L CqUp7gd5ydJEo8sDyCkd1PGRA2lKwmDuQnaQDXl2UepyxGlnFFXYzn6bU571e6DOiIRUEt5gi EvCOrmU9PPRYjS+6MfcuK6o+NtLAeTkRd5mX7ImmdjjN63lb/sTD/NhqwaEx1iOOUDoDITCmW X+a0UXzabtysnm2yMAUgwO6YecnXzFuPiS/Nh+2+skoebgCzO3rtx4mjjWYAZGuUCCpI+0ceI n+m42YwwjiuYzRa8jJMl2g7DPw20OTLp99eIeP8EOVF+YY7/6jWUPvkV7AAz0frIY03g2p0av C7VWj1ZX+vRruQpfhDKLKoKq5oUiDyumjNGn+a3Ixh40wuumkGYFG28A3kxvuDjNfHEhe97kW mfvVqCRBTgeNplJdEcDZJO8K7XPsUR13y3/HlYa4k0adA2ydz69DjQpGQo9KGe4uSYpC6D8YB lsKLAnBkYkIPXC0ta2a3AxiCKxikRBGF9EplV+/U4yE1HN1kKWPMvIukZvHcq+B1eCmRZ/oGu e80LHbWVj73sKvaYJOf3f5NvBMfUS+C5WNFakJ3P5Kbj8MQv60r/JnI1rTlVagBe/eBdihJot a820Tl4r/j5EP3Lsbnn1fOFcpnl+cvlHoVUql+5Z+iOx02ZuEbYe5r8TArl8ZShURsd0Z8YsV 4dn6v5CY/t5X7sXn Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wednesday, June 1, 2016 10:55:02 AM CEST Yuval Mintz wrote: > > > I think both solutions are equally valid/elegant. > > > > > > Arnd? > > > > I think we can just remove the IS_ENABLED() check there and define the > > IS_PF() macro conditionally to become 'true' if CONFIG_QED_SRIOV is not set, > > like some other drivers do > > > > diff --git a/drivers/net/ethernet/qlogic/qed/qed_main.c > > b/drivers/net/ethernet/qlogic/qed/qed_main.c > > index 287f61c20c19..756176525cf9 100644 > > --- a/drivers/net/ethernet/qlogic/qed/qed_main.c > > +++ b/drivers/net/ethernet/qlogic/qed/qed_main.c > > @@ -1110,7 +1110,7 @@ static int qed_get_link_data(struct qed_hwfn *hwfn, > > { > > void *p; > > > > - if (IS_ENABLED(CONFIG_QED_SRIOV) && !IS_PF(hwfn->cdev)) { > > + 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); 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)) > > > > I don't see why that isn't already the case actually. If this is ok, I'll send an > > updated patch. > > > > For the PF case, we still need to fix the qed_mcp_get_link_params() failure case, > > so the rest of my patch is needed anyway, regardless of how we address the > > warning. > > > > I think that would be unsafe with current qede - > qede currently publishes its VFs' PCI device-id as part its MODULE_DEVICE_TABLE, > even if CONFIG_QED_SRIOV isn't enabled [might be the wrong thing to do, but that > how it goes]. > Without changing this, if for some reason we'd have an assigned VF to a VM > whose kernel isn't compiled with CONFIG_QED_SRIOV [which is an odd config], > that VM is likely to miserably crash. Wouldn't it crash anyway if the code to handle VF devices is not present? E.g. the warning we got here tells us that qed_get_link_data() operates on uninitialized data when called on a VF device and SRIOV support is not built into the driver. I haven't looked if all the other functions handle that right, but my guess is that there are other functions with similar problems. Maybe it's best to remove the PCI IDs fort the virtual devices from the table if they are not supported by the configuration. Arnd