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 04316A0573; Wed, 19 Oct 2022 10:34:12 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 979FC40A8B; Wed, 19 Oct 2022 10:34:12 +0200 (CEST) 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 78B494069C for ; Wed, 19 Oct 2022 10:34:11 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1666168450; 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=FoAz2sQLJX0cWHjNeliOHynWfUjx1fywMjsi24i3ptU=; b=PZRn5dcPFLkxIomLBW2fqTmySX0y3xamG1XkZ99khOWVORfzKP6DtmLbn3ud+AigjLaZGY tVdlbQmJz1o3+BkEtVtB6+gBC9uH0LbeyZ8UQw0DPY466vKLpRY1uXCVg9a1ksuyQ8cGoh QlA+KrvPs/6zXifpNsFRxD5FwDoCH6c= Received: from mail-wm1-f69.google.com (mail-wm1-f69.google.com [209.85.128.69]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_128_GCM_SHA256) id us-mta-416-E1R8wxCXPEqE5tLJJvkVbQ-1; Wed, 19 Oct 2022 04:34:09 -0400 X-MC-Unique: E1R8wxCXPEqE5tLJJvkVbQ-1 Received: by mail-wm1-f69.google.com with SMTP id 133-20020a1c028b000000b003c5e6b44ebaso12592077wmc.9 for ; Wed, 19 Oct 2022 01:34:09 -0700 (PDT) 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:from:references:cc:to :content-language:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=FoAz2sQLJX0cWHjNeliOHynWfUjx1fywMjsi24i3ptU=; b=dyC3fea8zObVDKq+j544SKpC+rAwzSHIn/lX01NsS6eb5YG8CCkpWTxYvU8pjFuLv2 TlMqHt6wTzkFwSiI9KO0TRqf59yk7omsxFz4RXj7FdL+fEzIILatrUiUm2IROp6B6Qvx U6dvB5jsjB6aPNjMdHt32h2ke86p8bM0PHiXU95hiEoROkg6NosetO7rXFjS5r9Dbgo2 LKnRoNYR2bhhld6+hWPIMPxPN3/Xi4dnslR1oyU7/R664JcaVAWKGzQVfaJxDwJb6OVX jHyQP6k+bAf/Cfby8KVCvvGwVJ0ulhkvTqkVhF3YLlw6u5w/sgcRg4uxwJugSHXq0tBi oL4A== X-Gm-Message-State: ACrzQf2IUt+YuC9Abz7Z7Tx3GdOZv9ck7GFwnraaMDnKQwLdn5T0Dm4s Fa1mwUfNLR0wq4SZPjm4WlA0sSDsdkjFnHFNz8feliR0//va/x7N4KJ0JfFMaGo+NUC1rvvybuB a+3Q= X-Received: by 2002:a5d:69c6:0:b0:22c:dbb2:d84 with SMTP id s6-20020a5d69c6000000b0022cdbb20d84mr4536345wrw.190.1666168448654; Wed, 19 Oct 2022 01:34:08 -0700 (PDT) X-Google-Smtp-Source: AMsMyM7ssGZd8nSBYhJ//bE5MhJGn4Zk/DaxVQ7qADYsECEnkjg9HOQdJB8QNOReYDiO/qQjD5fKqw== X-Received: by 2002:a5d:69c6:0:b0:22c:dbb2:d84 with SMTP id s6-20020a5d69c6000000b0022cdbb20d84mr4536337wrw.190.1666168448389; Wed, 19 Oct 2022 01:34:08 -0700 (PDT) Received: from [192.168.0.36] ([78.19.110.164]) by smtp.gmail.com with ESMTPSA id d5-20020adfe845000000b0022b1d74dc56sm13099950wrn.79.2022.10.19.01.34.07 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 19 Oct 2022 01:34:07 -0700 (PDT) Message-ID: Date: Wed, 19 Oct 2022 09:34:07 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.3.1 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> From: Kevin Traynor 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 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. >>>> - /* 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) | >>> >> >