netdev - Re: [PATCH net-next 1/5] net: txgbe: Identify PHY and SFP module

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <20221114153438.703209-1-alexandr.lobakin@intel.com>
Date:   Mon, 14 Nov 2022 16:34:38 +0100
From:   Alexander Lobakin <alexandr.lobakin@...el.com>
To:     Mengyuan Lou <mengyuanlou@...-swift.com>
Cc:     Alexander Lobakin <alexandr.lobakin@...el.com>,
        netdev@...r.kernel.org, jiawenwu@...stnetic.com
Subject: Re: [PATCH net-next 1/5] net: txgbe: Identify PHY and SFP module

From: Mengyuan Lou <mengyuanlou@...-swift.com>
Date: Tue,  8 Nov 2022 19:19:03 +0800

> From: Jiawen Wu <jiawenwu@...stnetic.com>
> 
> Add to get media type and physical layer module, support I2C access.
> 
> Signed-off-by: Jiawen Wu <jiawenwu@...stnetic.com>
> ---

[...]

> @@ -277,11 +647,30 @@ int txgbe_reset_hw(struct txgbe_hw *hw)
>  	struct wx_hw *wxhw = &hw->wxhw;
>  	int status;
>  
> +	u32 sr_pcs_ctl, sr_pma_mmd_ctl1, sr_an_mmd_ctl, sr_an_mmd_adv_reg2;
> +	u32 vr_xs_or_pcs_mmd_digi_ctl1, curr_vr_xs_or_pcs_mmd_digi_ctl1;
> +	u32 curr_sr_an_mmd_ctl, curr_sr_an_mmd_adv_reg2;
> +	u32 curr_sr_pcs_ctl, curr_sr_pma_mmd_ctl1;

Please merge this with the first declaration block, there must be
only one.
Also, are you sure you need all this simultaneously? Maybe reuse
some of them?

> +
>  	/* Call adapter stop to disable tx/rx and clear interrupts */
>  	status = wx_stop_adapter(wxhw);
>  	if (status != 0)
>  		return status;
>  
> +	/* Identify PHY and related function pointers */

[...]

> +	if (!hw->phy.orig_link_settings_stored) {
> +		hw->phy.orig_sr_pcs_ctl2 = sr_pcs_ctl;
> +		hw->phy.orig_sr_pma_mmd_ctl1 = sr_pma_mmd_ctl1;
> +		hw->phy.orig_sr_an_mmd_ctl = sr_an_mmd_ctl;
> +		hw->phy.orig_sr_an_mmd_adv_reg2 = sr_an_mmd_adv_reg2;
> +		hw->phy.orig_vr_xs_or_pcs_mmd_digi_ctl1 =
> +						vr_xs_or_pcs_mmd_digi_ctl1;
> +		hw->phy.orig_link_settings_stored = true;
> +	} else {
> +		hw->phy.orig_sr_pcs_ctl2 = curr_sr_pcs_ctl;
> +		hw->phy.orig_sr_pma_mmd_ctl1 = curr_sr_pma_mmd_ctl1;
> +		hw->phy.orig_sr_an_mmd_ctl = curr_sr_an_mmd_ctl;
> +		hw->phy.orig_sr_an_mmd_adv_reg2 =
> +					curr_sr_an_mmd_adv_reg2;
> +		hw->phy.orig_vr_xs_or_pcs_mmd_digi_ctl1 =
> +					curr_vr_xs_or_pcs_mmd_digi_ctl1;
> +	}
> +
> +	/*make sure phy power is up*/
> +	msleep(100);

Can we poll something to check if it is up already? 100 ms is a ton,
I would try to avoid such huge sleeps if possible.

> +
>  	/* Store the permanent mac address */
>  	wx_get_mac_addr(wxhw, wxhw->mac.perm_addr);

[...]

> -- 
> 2.38.1

Thanks,
Olek