From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id D2CC4A0545; Tue, 20 Dec 2022 16:48:01 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 7E2CE40395; Tue, 20 Dec 2022 16:48:01 +0100 (CET) Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by mails.dpdk.org (Postfix) with ESMTP id 4716840143 for ; Tue, 20 Dec 2022 16:48:00 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1671551279; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=cA3eZCFmOLp+3yQypi1wGGZKvJLRuGb+dqTzdHyyCZs=; b=KGRAhoFZrpOKM40Fgk1hZw0wsfQm9qSDx/1Us2U26i3S2O28qXlQlV3RsYXKSyZfshGjCv SYMdtURXsiQ/me8yO0USpG+JKDMgcpl26K0cYtDw7noqYIwYFkmn6X7BaH0kW5AA0BTJIb 99erTnizDCOZiZ9AUg0Vw8sp031rgPU= Received: from mail-wm1-f72.google.com (mail-wm1-f72.google.com [209.85.128.72]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_128_GCM_SHA256) id us-mta-14-vySocKF7Paul28ITyXyQhw-1; Tue, 20 Dec 2022 10:47:55 -0500 X-MC-Unique: vySocKF7Paul28ITyXyQhw-1 Received: by mail-wm1-f72.google.com with SMTP id i187-20020a1c3bc4000000b003d634aca337so582353wma.1 for ; Tue, 20 Dec 2022 07:47:55 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:in-reply-to:subject:content-language :references:cc:to:from:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=cA3eZCFmOLp+3yQypi1wGGZKvJLRuGb+dqTzdHyyCZs=; b=xQMDm5xFoZrZvMw5k5tvV6ugTADCcxq+HRaMx4pv+v/BNqXzWLrz5+r7R46c094r67 HR7UX0K+sQDtgFi8lZ4/oiG/xrEgVJt09zSFbMpKGpQHRlUXbdhg5MRutaNBWy9RGXzb VM3IA3SSsqxeVstzSnSWciE4SCy5gZTvW/NlkiDI8urxc1vfFsOJna9lA5GRvU7sJS0P yGkGk2ECBdwk3a9lKBcKkPyjQ3DN5g4E1Z1pm6g+lS2IWmPjW3u8n9NIc8gWGg9DEb46 Z6/avg9U1o32dpYpAiRojI2p/dqS2Tszjy+Qhy3cICMMTenG1zOQBJy7jPiiA9GX0LYG AufQ== X-Gm-Message-State: ANoB5pm4gwvvZPl7cyKBDWDmKeqU30LU6VTdKrHgoIrGV+1fkxnFKtsH g6ko3Ljptw8P25KgnMnPmoztyzBkBmHIpDXr6yUx8VNmsdmNZ97y6dX8vLUUHMAYLNF1eamwVs8 isjI= X-Received: by 2002:a5d:61c5:0:b0:242:2a13:bbd0 with SMTP id q5-20020a5d61c5000000b002422a13bbd0mr26392172wrv.65.1671551274536; Tue, 20 Dec 2022 07:47:54 -0800 (PST) X-Google-Smtp-Source: AA0mqf6608QOgqTfi2mFRYuqy5S37+0LSZP1fIzQy8CHrkRBDp5rwynSSCSCQU4slRLXLfL/Nj3lhg== X-Received: by 2002:a5d:61c5:0:b0:242:2a13:bbd0 with SMTP id q5-20020a5d61c5000000b002422a13bbd0mr26392165wrv.65.1671551274366; Tue, 20 Dec 2022 07:47:54 -0800 (PST) Received: from [192.168.0.36] ([78.17.251.111]) by smtp.gmail.com with ESMTPSA id cl4-20020a5d5f04000000b00242209dd1ffsm13244648wrb.41.2022.12.20.07.47.53 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 20 Dec 2022 07:47:53 -0800 (PST) Message-ID: <6f5133e1-ec40-dd70-9081-d42bfee2ee71@redhat.com> Date: Tue, 20 Dec 2022 15:47:53 +0000 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.5.1 From: Kevin Traynor To: "Mah, Yock Gen" , IOTG DPDK Ref App , "Zhang, Qi Z" , "Taripin, Samuel" Cc: "dev@dpdk.org" References: <20220829081436.29945-1-iotg.dpdk.ref.app@intel.com> <20220831225139.78201-1-iotg.dpdk.ref.app@intel.com> <45165da1-25af-b735-f80d-a6b42592cf6c@redhat.com> <31c280bd-2d69-e302-8ec6-cbff21badd4a@redhat.com> <44a7bd84-ef82-1d69-b591-06c61f5e2ec7@redhat.com> Subject: Re: [PATCH v2] IGC: Remove I225_I_PHY_ID checking In-Reply-To: X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org On 19/10/2022 09:34, Kevin Traynor wrote: > On 18/10/2022 23:45, Mah, Yock Gen wrote: >> >> >> -----Original Message----- >> From: Kevin Traynor >> Sent: Tuesday, 18 October, 2022 8:54 PM >> To: Mah, Yock Gen ; IOTG DPDK Ref App ; Zhang, Qi Z >> Cc: dev@dpdk.org >> Subject: Re: [PATCH v2] IGC: Remove I225_I_PHY_ID checking >> >> On 12/10/2022 09:39, Kevin Traynor wrote: >>> On 12/10/2022 08:45, Mah, Yock Gen wrote: >>>> >>>> >>>> -----Original Message----- >>>> From: Kevin Traynor >>>> Sent: Tuesday, 11 October, 2022 7:05 PM >>>> To: IOTG DPDK Ref App ; Zhang, Qi Z >>>> >>>> Cc: dev@dpdk.org >>>> Subject: Re: [PATCH v2] IGC: Remove I225_I_PHY_ID checking >>>> >>>> On 31/08/2022 23:51, iotg.dpdk.ref.app@intel.com wrote: >>>>> From: NSWE SWS DPDK Dev >>>>> >>>>> i225 devices have only one PHY vendor. There is unnecessary to check >>>>> _I_PHY_ID during the link establishment and auto-negotiation >>>>> process, the checking also caused devices like i225-IT failed. This >>>>> patch is to remove the mentioned unnecessary checking. >>>>> >>>>> Cc: stable@dpdk.org >>>>> Signed-off-by: NSWE SWS DPDK Dev >>>>> --- >>>>> drivers/net/igc/base/igc_i225.c | 15 ++------------- >>>>> drivers/net/igc/base/igc_phy.c | 6 ++---- >>>>> 2 files changed, 4 insertions(+), 17 deletions(-) >>>>> >>>>> diff --git a/drivers/net/igc/base/igc_i225.c >>>>> b/drivers/net/igc/base/igc_i225.c index 5f3d535490..af26602afb >>>>> 100644 >>>>> --- a/drivers/net/igc/base/igc_i225.c >>>>> +++ b/drivers/net/igc/base/igc_i225.c >>>>> @@ -173,19 +173,8 @@ static s32 igc_init_phy_params_i225(struct igc_hw *hw) >>>>> phy->ops.write_reg = igc_write_phy_reg_gpy; >>>>> >>>>> ret_val = igc_get_phy_id(hw); >>>>> - /* Verify phy id and set remaining function pointers */ >>>>> - switch (phy->id) { >>>>> - case I225_I_PHY_ID: >>>>> - case I226_LM_PHY_ID: >>>>> - phy->type = igc_phy_i225; >>>>> - phy->ops.set_d0_lplu_state = igc_set_d0_lplu_state_i225; >>>>> - phy->ops.set_d3_lplu_state = igc_set_d3_lplu_state_i225; >>>> >>>>> - The commit log says it is removing a check on the ID, but it does not say why these function pointers are being removed. >>>> >>>>> - Why are they removed, were they not needed? >>>> >>>> >>>> i225 devices have only one PHY vendor. There is no point checking _I_PHY_ID during the link establishment and auto-negotiation process. >>>> >>> >>> Right, that's clear about the vendor ID check. But it's not clear to >>> me why the the resulting code like this: >>> >>> phy->type = igc_phy_i225; >>> >>> and not like this: >>> >>> phy->type = igc_phy_i225; >>> phy->ops.set_d0_lplu_state = igc_set_d0_lplu_state_i225; >>> phy->ops.set_d3_lplu_state = igc_set_d3_lplu_state_i225; >>> >>> So it is using dummy null functions instead: >>> https://git.dpdk.org/dpdk/tree/drivers/net/igc/base/igc_phy.c#n61 >>> >>> Do the device registers not need to be set anymore? >>> >> >>> For main branch, it would be nice to have an answer to above. >> >>> It only adds a small readability benefit by removing some code branches, but does change functionality which adds risk, so I don't think it's a good candidate for stable branches. >> >> This is not only added readability, but to fix real world issue, we were experiencing i225-IT not runnable issue without patching another case checking as below: >> >> +++ b/drivers/net/igc/base/igc_phy.c >> @@ -1881,6 +1881,7 @@ s32 igc_phy_force_speed_duplex_m88(struct igc_hw *hw) >> case I210_I_PHY_ID: >> /* fall-through */ >> case I225_I_PHY_ID: >> + case I225_IT_PHY_ID: >> >> However, cleaner solution is to remove those unnecessary checking completely as it does in kernel also https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=7c496de538eebd8212dc2a3c9a468386b264d0d4 >> > > ok, you are correct that it is more than readability. I can apply but > first it would be good if you can answer the original question and > explain why the functionality is changed for I225_I_PHY_ID by removing > the function pointers that set the registers. > Hi, I didn't apply this to DPDK 21.11.3 because there was no explanation of why the registers were changing after asking multiple times. If it is required for a later 21.11 LTS release, please send a backport to stable mailing list with an updated commit message explaining the changes. thanks, Kevin. >>>>> - /* TODO - complete with GPY PHY information */ >>>>> - break; >>>>> - default: >>>>> - ret_val = -IGC_ERR_PHY; >>>>> - goto out; >>>>> - } >>>>> + phy->type = igc_phy_i225; >>>>> + >>>>> >>>>> out: >>>>> return ret_val; >>>>> diff --git a/drivers/net/igc/base/igc_phy.c >>>>> b/drivers/net/igc/base/igc_phy.c index 43bbe69bca..2906bae21a 100644 >>>>> --- a/drivers/net/igc/base/igc_phy.c >>>>> +++ b/drivers/net/igc/base/igc_phy.c >>>>> @@ -1474,8 +1474,7 @@ s32 igc_phy_setup_autoneg(struct igc_hw *hw) >>>>> return ret_val; >>>>> } >>>>> >>>>> - if ((phy->autoneg_mask & ADVERTISE_2500_FULL) && >>>>> - hw->phy.id == I225_I_PHY_ID) { >>>>> + if (phy->autoneg_mask & ADVERTISE_2500_FULL) { >>>>> /* Read the MULTI GBT AN Control Register - reg 7.32 */ >>>>> ret_val = phy->ops.read_reg(hw, (STANDARD_AN_REG_MASK << >>>>> MMD_DEVADDR_SHIFT) | >>>>> @@ -1615,8 +1614,7 @@ s32 igc_phy_setup_autoneg(struct igc_hw *hw) >>>>> ret_val = phy->ops.write_reg(hw, PHY_1000T_CTRL, >>>>> mii_1000t_ctrl_reg); >>>>> >>>>> - if ((phy->autoneg_mask & ADVERTISE_2500_FULL) && >>>>> - hw->phy.id == I225_I_PHY_ID) >>>>> + if (phy->autoneg_mask & ADVERTISE_2500_FULL) >>>>> ret_val = phy->ops.write_reg(hw, >>>>> (STANDARD_AN_REG_MASK << >>>>> MMD_DEVADDR_SHIFT) | >>>> >>> >> >