[PATCH net-next v1] e1000: Initialize phy_data to avoid unexpected values

[PATCH net-next v1] e1000: Initialize phy_data to avoid unexpected values

[Rongguang Wei]

From: Rongguang Wei <weirongguang@kylinos.cn>

The phy_data variable is not initialized. If e1000_read_phy_reg
returns an error, phy_data will not point to a valid value from
the PHY register, which may cause the regs_buff array to be populated
with unexpected values.

Signed-off-by: Rongguang Wei <weirongguang@kylinos.cn>
Change-Id: I46071b3b21a566f8da650168d38d6968251b077d
---
 drivers/net/ethernet/intel/e1000/e1000_ethtool.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/e1000/e1000_ethtool.c b/drivers/net/ethernet/intel/e1000/e1000_ethtool.c
index 4dcbeabb3ad2..f068108c5004 100644
--- a/drivers/net/ethernet/intel/e1000/e1000_ethtool.c
+++ b/drivers/net/ethernet/intel/e1000/e1000_ethtool.c
@@ -327,7 +327,7 @@ static void e1000_get_regs(struct net_device *netdev, struct ethtool_regs *regs,
 	struct e1000_adapter *adapter = netdev_priv(netdev);
 	struct e1000_hw *hw = &adapter->hw;
 	u32 *regs_buff = p;
-	u16 phy_data;
+	u16 phy_data = 0;
 
 	memset(p, 0, E1000_REGS_LEN * sizeof(u32));
 
-- 
2.25.1

RE: [PATCH net-next v1] e1000: Initialize phy_data to avoid unexpected values

[Jagielski, Jedrzej]

From: Rongguang Wei <clementwei90@163.com> 
Sent: Friday, June 12, 2026 10:04 AM

>From: Rongguang Wei <weirongguang@kylinos.cn>
>
>The phy_data variable is not initialized. If e1000_read_phy_reg
>returns an error, phy_data will not point to a valid value from
>the PHY register, which may cause the regs_buff array to be populated
>with unexpected values.

Hi,

Sounds like a fix, but i believe we would like to have any real
scenario when the issue occurs and how it can be reproduced.
If such is provided please target the patch against net tree and
add fixes tag.

>
>Signed-off-by: Rongguang Wei <weirongguang@kylinos.cn>
>Change-Id: I46071b3b21a566f8da650168d38d6968251b077d


i doubt this is a correct kernel commit tag

>---
> drivers/net/ethernet/intel/e1000/e1000_ethtool.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/drivers/net/ethernet/intel/e1000/e1000_ethtool.c b/drivers/net/ethernet/intel/e1000/e1000_ethtool.c
>index 4dcbeabb3ad2..f068108c5004 100644
>--- a/drivers/net/ethernet/intel/e1000/e1000_ethtool.c
>+++ b/drivers/net/ethernet/intel/e1000/e1000_ethtool.c
>@@ -327,7 +327,7 @@ static void e1000_get_regs(struct net_device *netdev, struct ethtool_regs *regs,
> 	struct e1000_adapter *adapter = netdev_priv(netdev);
> 	struct e1000_hw *hw = &adapter->hw;
> 	u32 *regs_buff = p;
>-	u16 phy_data;
>+	u16 phy_data = 0;
> 
> 	memset(p, 0, E1000_REGS_LEN * sizeof(u32));
> 
>-- 
>2.25.1

Re: [PATCH net-next v1] e1000: Initialize phy_data to avoid unexpected values

[Rongguang Wei]

在 2026/6/12 16:58, Jagielski, Jedrzej 写道:
> From: Rongguang Wei <clementwei90@163.com> 
> Sent: Friday, June 12, 2026 10:04 AM
> 
>> From: Rongguang Wei <weirongguang@kylinos.cn>
>>
>> The phy_data variable is not initialized. If e1000_read_phy_reg
>> returns an error, phy_data will not point to a valid value from
>> the PHY register, which may cause the regs_buff array to be populated
>> with unexpected values.
> 
> Hi,
> 
> Sounds like a fix, but i believe we would like to have any real
> scenario when the issue occurs and how it can be reproduced.
> If such is provided please target the patch against net tree and
> add fixes tag.
> 
Hi,
I was not face a real scenario. I just found out there is no check for 
e1000_read_phy_reg return value when I reading the driver code. 
Maybe is better to initialized the value or check the return value of e1000_read_phy_reg.
>>
>> Signed-off-by: Rongguang Wei <weirongguang@kylinos.cn>
>> Change-Id: I46071b3b21a566f8da650168d38d6968251b077d
> 
> 
> i doubt this is a correct kernel commit tag
> 
>> ---
>> drivers/net/ethernet/intel/e1000/e1000_ethtool.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/ethernet/intel/e1000/e1000_ethtool.c b/drivers/net/ethernet/intel/e1000/e1000_ethtool.c
>> index 4dcbeabb3ad2..f068108c5004 100644
>> --- a/drivers/net/ethernet/intel/e1000/e1000_ethtool.c
>> +++ b/drivers/net/ethernet/intel/e1000/e1000_ethtool.c
>> @@ -327,7 +327,7 @@ static void e1000_get_regs(struct net_device *netdev, struct ethtool_regs *regs,
>> 	struct e1000_adapter *adapter = netdev_priv(netdev);
>> 	struct e1000_hw *hw = &adapter->hw;
>> 	u32 *regs_buff = p;
>> -	u16 phy_data;
>> +	u16 phy_data = 0;
>>
>> 	memset(p, 0, E1000_REGS_LEN * sizeof(u32));
>>
>> -- 
>> 2.25.1
> 

Re: [PATCH net-next v1] e1000: Initialize phy_data to avoid unexpected values

[Andrew Lunn]

On Fri, Jun 12, 2026 at 04:03:31PM +0800, Rongguang Wei wrote:
> From: Rongguang Wei <weirongguang@kylinos.cn>
> 
> The phy_data variable is not initialized. If e1000_read_phy_reg
> returns an error, phy_data will not point to a valid value from
> the PHY register, which may cause the regs_buff array to be populated
> with unexpected values.
> 
> Signed-off-by: Rongguang Wei <weirongguang@kylinos.cn>
> Change-Id: I46071b3b21a566f8da650168d38d6968251b077d

What does this Change-Id mean?

> ---
>  drivers/net/ethernet/intel/e1000/e1000_ethtool.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/intel/e1000/e1000_ethtool.c b/drivers/net/ethernet/intel/e1000/e1000_ethtool.c
> index 4dcbeabb3ad2..f068108c5004 100644
> --- a/drivers/net/ethernet/intel/e1000/e1000_ethtool.c
> +++ b/drivers/net/ethernet/intel/e1000/e1000_ethtool.c
> @@ -327,7 +327,7 @@ static void e1000_get_regs(struct net_device *netdev, struct ethtool_regs *regs,
>  	struct e1000_adapter *adapter = netdev_priv(netdev);
>  	struct e1000_hw *hw = &adapter->hw;
>  	u32 *regs_buff = p;
> -	u16 phy_data;
> +	u16 phy_data = 0;

	if (hw->phy_type == e1000_phy_igp) {
		e1000_write_phy_reg(hw, IGP01E1000_PHY_PAGE_SELECT,
				    IGP01E1000_PHY_AGC_A);
		e1000_read_phy_reg(hw, IGP01E1000_PHY_AGC_A &
				   IGP01E1000_PHY_PAGE_SELECT, &phy_data);
		regs_buff[13] = (u32)phy_data; /* cable length */

Isn't a cable length of 0 also unexpected?

How does this patch actually make the situation better?

    
    Andrew

---
pw-bot: cr

Re: [PATCH net-next v1] e1000: Initialize phy_data to avoid unexpected values

[Rongguang Wei]

在 2026/6/13 03:39, Andrew Lunn 写道:
> On Fri, Jun 12, 2026 at 04:03:31PM +0800, Rongguang Wei wrote:
>> From: Rongguang Wei <weirongguang@kylinos.cn>
>>
>> The phy_data variable is not initialized. If e1000_read_phy_reg
>> returns an error, phy_data will not point to a valid value from
>> the PHY register, which may cause the regs_buff array to be populated
>> with unexpected values.
>>
>> Signed-off-by: Rongguang Wei <weirongguang@kylinos.cn>
>> Change-Id: I46071b3b21a566f8da650168d38d6968251b077d
> 
> What does this Change-Id mean?
> 
Sorry, it just a auto generate id when I push this patch on my own repos.
I forget to delete.
>> ---
>>  drivers/net/ethernet/intel/e1000/e1000_ethtool.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/ethernet/intel/e1000/e1000_ethtool.c b/drivers/net/ethernet/intel/e1000/e1000_ethtool.c
>> index 4dcbeabb3ad2..f068108c5004 100644
>> --- a/drivers/net/ethernet/intel/e1000/e1000_ethtool.c
>> +++ b/drivers/net/ethernet/intel/e1000/e1000_ethtool.c
>> @@ -327,7 +327,7 @@ static void e1000_get_regs(struct net_device *netdev, struct ethtool_regs *regs,
>>  	struct e1000_adapter *adapter = netdev_priv(netdev);
>>  	struct e1000_hw *hw = &adapter->hw;
>>  	u32 *regs_buff = p;
>> -	u16 phy_data;
>> +	u16 phy_data = 0;
> 
> 	if (hw->phy_type == e1000_phy_igp) {
> 		e1000_write_phy_reg(hw, IGP01E1000_PHY_PAGE_SELECT,
> 				    IGP01E1000_PHY_AGC_A);
> 		e1000_read_phy_reg(hw, IGP01E1000_PHY_AGC_A &
> 				   IGP01E1000_PHY_PAGE_SELECT, &phy_data);
> 		regs_buff[13] = (u32)phy_data; /* cable length */
> 
> Isn't a cable length of 0 also unexpected?
> 
> How does this patch actually make the situation better?
> 
Uninitialized variables may be initialized to 0 by the system, explicit initialization
is performed to avoid accidents. 

The 0 is from e1000_read_phy_reg_ex in e1000_main.c and e1000_power_down_phy when use
e1000_read_phy_reg function the last paramenters is initialized 0. So I used this value.

There are many other function which use e1000_read_phy_reg also not initialize the last paramenters
eg. e1000_phy_reset_clk_and_crs. I can do it in V2.
>     
>     Andrew
> 
> ---
> pw-bot: cr

Re: [PATCH net-next v1] e1000: Initialize phy_data to avoid unexpected values

[Andrew Lunn]

> > Isn't a cable length of 0 also unexpected?
> > 
> > How does this patch actually make the situation better?
> > 
> Uninitialized variables may be initialized to 0 by the system, explicit initialization
> is performed to avoid accidents. 

That is still not an explanation why 0 is better.

Maybe you want to change this from a void function to something which
can return an error code?

But also consider the history of this driver. When did anybody do any
serious development work on it? Does it makes sense to do work on it?
I assume you have the hardware and can test the changes you are
making.

	Andrew