netdev - Re: [net-next PATCH v2 11/15] eth: fbnic: Add link detection

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]

Date: Tue, 25 Jun 2024 17:25:16 +0200
From: Andrew Lunn <andrew@...n.ch>
To: Alexander Duyck <alexander.duyck@...il.com>
Cc: netdev@...r.kernel.org, Alexander Duyck <alexanderduyck@...com>,
	kuba@...nel.org, davem@...emloft.net, pabeni@...hat.com
Subject: Re: [net-next PATCH v2 11/15] eth: fbnic: Add link detection

> +	/* Tri-state value indicating state of link.
> +	 *  0 - Up
> +	 *  1 - Down
> +	 *  2 - Event - Requires checking as link state may have changed
> +	 */
> +	s8 link_state;

Maybe add an enum?

> +static irqreturn_t fbnic_mac_msix_intr(int __always_unused irq, void *data)
> +{
> +	struct fbnic_dev *fbd = data;
> +	struct fbnic_net *fbn;
> +
> +	if (!fbd->mac->get_link_event(fbd)) {
> +		fbnic_wr32(fbd, FBNIC_INTR_MASK_CLEAR(0),
> +			   1u << FBNIC_MAC_MSIX_ENTRY);
> +		return IRQ_HANDLED;
> +	}
> +
> +	fbd->link_state = FBNIC_LINK_EVENT;
> +	fbn = netdev_priv(fbd->netdev);
> +
> +	phylink_mac_change(fbn->phylink, fbd->link_state == FBNIC_LINK_UP);

Can fbd->link_state == FBNIC_LINK_UP given that you have just done:
    fbd->link_state = FBNIC_LINK_EVENT ? 

> +static u32 __fbnic_mac_config_asic(struct fbnic_dev *fbd)
> +{
> +	/* Enable MAC Promiscuous mode and Tx padding */
> +	u32 command_config = FBNIC_MAC_COMMAND_CONFIG_TX_PAD_EN |
> +			     FBNIC_MAC_COMMAND_CONFIG_PROMISC_EN;
> +	struct fbnic_net *fbn = netdev_priv(fbd->netdev);
> +	u32 rxb_pause_ctrl;
> +
> +	/* Set class 0 Quanta and refresh */
> +	wr32(fbd, FBNIC_MAC_CL01_PAUSE_QUANTA, 0xffff);
> +	wr32(fbd, FBNIC_MAC_CL01_QUANTA_THRESH, 0x7fff);
> +
> +	/* Enable generation of pause frames if enabled */
> +	rxb_pause_ctrl = rd32(fbd, FBNIC_RXB_PAUSE_DROP_CTRL);
> +	rxb_pause_ctrl &= ~FBNIC_RXB_PAUSE_DROP_CTRL_PAUSE_ENABLE;
> +	if (!fbn->tx_pause)
> +		command_config |= FBNIC_MAC_COMMAND_CONFIG_TX_PAUSE_DIS;
> +	else
> +		rxb_pause_ctrl |=
> +			FIELD_PREP(FBNIC_RXB_PAUSE_DROP_CTRL_PAUSE_ENABLE,
> +				   FBNIC_PAUSE_EN_MASK);
> +	wr32(fbd, FBNIC_RXB_PAUSE_DROP_CTRL, rxb_pause_ctrl);
> +
> +	if (!fbn->rx_pause)
> +		command_config |= FBNIC_MAC_COMMAND_CONFIG_RX_PAUSE_DIS;

Everybody gets pause wrong. To try to combat that it has mostly been
moved into phylink. When phylink calls your mac_config() callback it
passes const struct phylink_link_state *state. Within state is the
pause member. That tells you how to configure the hardware. phylink
will then deal with the differences between forced pause configuration
and negotiated pause configuration, etc. Your current mac_config() is
empty...

	Andrew