Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <0056a39d-d7dc-34ea-3a71-6d5d3835c2d5@intel.com>
Date:   Tue, 23 Aug 2022 08:19:39 -0700
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.12.0
Subject: Re: [PATCH v1] drivers/net/ethernet: check return value of e1e_rphy()
Content-Language: en-US
To:     lily <floridsleeves@gmail.com>, <intel-wired-lan@lists.osuosl.org>,
        <netdev@vger.kernel.org>, <linux-kernel@vger.kernel.org>
CC:     <anthony.l.nguyen@intel.com>, <davem@davemloft.net>,
        <edumazet@google.com>, <kuba@kernel.org>, <pabeni@redhat.com>
References: <20220823060200.1452663-1-floridsleeves@gmail.com>
From:   Jesse Brandeburg <jesse.brandeburg@intel.com>
In-Reply-To: <20220823060200.1452663-1-floridsleeves@gmail.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?TTlnWEtMMmxtam16YzR1b1E4Z0x3UXNTcFlBaHNabE5ZdXBiWW1NOEtZbW95?=
 =?utf-8?B?K2VFUXVTUU1QcGprdGwxZUpPZ0V4WTJ0VDdXWTVaTEk1eGY0eDA2azR1aFg4?=
 =?utf-8?B?MzEyZWo4TGlDd3BNbzcxcitmWWQrM1d3d0w1VUFTYzkyUzhqTDFVd3hsenBv?=
 =?utf-8?B?YmxWcVZZRndiVWNvK051MVorNXllN2I2NDlkSnFEUVg2ak13dHpOSHJxUzN6?=
 =?utf-8?B?Ym44MHc5WjN3Ymc5aHc0U1MxWWRsUEVmaDdLcjNrWFhHeWRzZG1kekdOWkVk?=
 =?utf-8?B?NWdtWVc3Z3JQTTRlYzBPVWZHZzU0cjhGeXFaUm5hSWF6RTM4Q3ZQd1kzNVFu?=
 =?utf-8?B?TXJhb25HeU9pTVRsaDA4L3FLbkFjblR2b1k0VzhlaTY5TzBvTFdRc3RLL0Vo?=
 =?utf-8?B?MGRrRm5Ma1BRcExwOW5YVkdKMHZ3OUVKNlR4TlFoaXZ3MlB6VUJpSnBpRTZu?=
 =?utf-8?B?NUppK0NXalpKUDQxT3BiOFlnYi91eXhCYVhVVk4xcU9SLzN6RlBnSFZhMElF?=
 =?utf-8?B?NUZMaEprUEYzeVp0RmxGOTU2U21WekhDemhveGFVYzhieWc0aWZSTmcwQkUx?=
 =?utf-8?B?TmhxUHV1d2tmT2Myd01RY1JDc1pHMzZuQWROazU5ZENqcDk1cHh2aHc2Wi83?=
 =?utf-8?B?UFFQTjhqMlV0alF2TU53akpVQ0lVNUFSUkMxUk1aK3h2ZTNEb3RLKzVuSTVj?=
 =?utf-8?B?Mng2MGRVa1V2WGJpSzRKTlA3ZXJ6SGF3c3dyMUVVMHg5WHlvdmRtVzNHa1J6?=
 =?utf-8?B?ZTA0d3NCWUtnT1ZQbzJlWGNiYTRxV3cveG5RTmp1c1pXd1UwdU9Md3M4Kysx?=
 =?utf-8?B?dHhMbVFpd2wzUk44QmZ5bTUzZU5vSXl0cWJvaUZ4S2hRR2NrZWwzK0pmcGZa?=
 =?utf-8?B?THVWanJ6NW96ejFlL3J5TmJpZGo1djJoNWVFM0g5WWE4ODhqS25JbkxXRzdj?=
 =?utf-8?B?WHd4ZXFXa2ViamliYW5wS3B2dUIrVzRJMi8wYm5tQk5Gd25LNWZseSs1ZFhh?=
 =?utf-8?B?WWtnL1F1ckdjWWJYZm44Wk5ldGdvVHRqYTlGUlpVR0FTZ0lzcW5tSzRWcmU0?=
 =?utf-8?B?TjZpdDZxMUpQZFNiSXVWdnJEdXFzb29iQXUwM2hpYStNWVlPLzlzaGtaTUhQ?=
 =?utf-8?B?SDFoaHFOZXBJdzZvTkROR2V3VWM3VDV5Z1J4czJ1ZnBEREFsc0hEQjVMMXNW?=
 =?utf-8?B?Y2VnNXY0NmVQcWhBVjY0UTVCNUl0ZzlLa1ZTYW10MTF1TUVybmJxbDJlMGx5?=
 =?utf-8?B?RHQ3S3JHZENqR1doblBZakt2dVpUTmxTZ0tWdytNNXRDQ1dsOWZVTDVkNFV6?=
 =?utf-8?B?QWlaZnFka2NFeWRkNjFJSWZUODJTV2J3WnNpbDd6SGV6U2g4TFhUY3VsWDN2?=
 =?utf-8?B?V0wyb1hzMW1VVndvM2NvaG1rT05ZcDMxM1JyQS9pbXhjbG9XeHhLTllQQmdq?=
 =?utf-8?B?c2VCTGh0eHVCbHF1YzNxdFZCeFUzY0orT1ZtSjhpREVMNk1jcWhtOGhRS3Fx?=
 =?utf-8?B?Sm9VdDB1K1grenFsdUJqZjBuaThvQlJtRFJLQlBNb0ltYnpOcmpHcFlreFBN?=
 =?utf-8?B?UXRIQUE3bFR6QW1WdDZieFdRWko4QWtsVWdZd2YvRDQzOW1pTXptM0pjTVVa?=
 =?utf-8?B?MmNWVWd1R2RLSjB5TTExM0VZMkloNG9oaG5jb3lZWDNhcEdWV2x1dnp4cWRI?=
 =?utf-8?B?TEtVWVEyWnRCT1hoOHhXUThmcm8rUUZDR0dna1VRbHBwOUMvMnFsR0I4dTRp?=
 =?utf-8?B?TVZ1THVGeVFjQTY4Wm1SOUE3NUx4RVJSUWVBalZ3U2ZaSXVSV2FaNk1MLzlt?=
 =?utf-8?B?d3VLSWhKZThwZEpYUFBZYWdGcEQxd1lJZUpISFBzOWJSRWRndFlyZklaKzhD?=
 =?utf-8?B?bkRwOXdzeG12djhxT3JQRWdpcXc3SVNycHA4RU81VXUzRlFYdC9nSGdGaVha?=
 =?utf-8?B?M0hGb1FYbnhDdnVIdlZnazV3dUY3NGlBQS91U2RkYVZiM1d3T0d5N3ZXeUFn?=
 =?utf-8?B?aG5aYzdabmNOTkpMaTk1SjczZDdVSEptZGdsaWF1cytjQzFkVExCSXh6aGN0?=
 =?utf-8?B?OWYrcElVMFd5OVRNYnMzYWNHSkZWMHZyUkpxRmJ0YkVpVFhjd0RNeFVoSXhR?=
 =?utf-8?B?czhFNXRjZ0JPQTYxQ01uYVh2bDBiSm16R0x0c3pXb2pRMmV5QkNWRzB1ckFS?=
 =?utf-8?B?b3c9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 6bbf82a3-c1f3-47af-3360-08da851ae770
X-MS-Exchange-CrossTenant-AuthSource: CO1PR11MB4914.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 23 Aug 2022 15:19:42.6598
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: b65ZAVat7G4NVISd32MOOHpNKyKc6Fpf44+AixULED7XgvwEw0Sg/eeTgD8dexbYw7xdnWQOQ1+GOXLXuwcr/M9pMnPvAeCQw0ONCSrFBv8=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR11MB2714
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-7.1 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,
        RCVD_IN_DNSWL_HI,SPF_HELO_NONE,SPF_NONE,T_SCC_BODY_TEXT_LINE
        autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 8/22/2022 11:02 PM, lily wrote:
> e1e_rphy() could return error value, which need to be checked.

Thanks for having a look at the e1000e driver. Was there some bug you 
found or is this just a fix based on a tool or observation?

If a tool was used, what tool?

For networking patches please follow the guidance at 
https://www.kernel.org/doc/html/latest/process/maintainer-netdev.html


> Signed-off-by: Li Zhong <floridsleeves@gmail.com>
> ---
>   drivers/net/ethernet/intel/e1000e/phy.c | 14 +++++++++++---
>   1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/e1000e/phy.c b/drivers/net/ethernet/intel/e1000e/phy.c
> index fd07c3679bb1..15ac302fdee0 100644
> --- a/drivers/net/ethernet/intel/e1000e/phy.c
> +++ b/drivers/net/ethernet/intel/e1000e/phy.c
> @@ -2697,9 +2697,12 @@ static s32 e1000_access_phy_wakeup_reg_bm(struct e1000_hw *hw, u32 offset,
>   void e1000_power_up_phy_copper(struct e1000_hw *hw)
>   {
>   	u16 mii_reg = 0;
> +	int ret;
>   
>   	/* The PHY will retain its settings across a power down/up cycle */
> -	e1e_rphy(hw, MII_BMCR, &mii_reg);
> +	ret = e1e_rphy(hw, MII_BMCR, &mii_reg);
> +	if (ret)
> +		return ret;

Can't return value to a void declared function, did you even compile 
test this?

Maybe it should be like:
     if (ret) {
	// this is psuedo code
         dev_warn(..., "PHY read failed during power up\n");
         return;
     }

>   	mii_reg &= ~BMCR_PDOWN;
>   	e1e_wphy(hw, MII_BMCR, mii_reg);
>   }
> @@ -2715,9 +2718,12 @@ void e1000_power_up_phy_copper(struct e1000_hw *hw)
>   void e1000_power_down_phy_copper(struct e1000_hw *hw)
>   {
>   	u16 mii_reg = 0;
> +	int ret;
>   
>   	/* The PHY will retain its settings across a power down/up cycle */
> -	e1e_rphy(hw, MII_BMCR, &mii_reg);
> +	ret = e1e_rphy(hw, MII_BMCR, &mii_reg);
> +	if (ret)
> +		return ret;

same here.

>   	mii_reg |= BMCR_PDOWN;
>   	e1e_wphy(hw, MII_BMCR, mii_reg);
>   	usleep_range(1000, 2000);
> @@ -3037,7 +3043,9 @@ s32 e1000_link_stall_workaround_hv(struct e1000_hw *hw)
>   		return 0;
>   
>   	/* Do not apply workaround if in PHY loopback bit 14 set */
> -	e1e_rphy(hw, MII_BMCR, &data);
> +	ret_val = e1e_rphy(hw, MII_BMCR, &data);
> +	if (ret_val)
> +		return ret_val;
>   	if (data & BMCR_LOOPBACK)
>   		return 0;
>   

Did any of the callers of the above function care about the return code 
being an error value? This has been like this for a long time...