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

["Russell King (Oracle)" <linux@armlinux.org.uk>]

Thanks for the patch - this is a review mainly from the phylink
perspective.

On Tue, Jul 02, 2024 at 08:00:22AM -0700, Alexander Duyck wrote:
> +static irqreturn_t fbnic_pcs_msix_intr(int __always_unused irq, void *data)
> +{
> +	struct fbnic_dev *fbd = data;
> +	struct fbnic_net *fbn;
> +	bool link_up;
> +
> +	if (!fbd->mac->pcs_get_link_event(fbd)) {
> +		fbnic_wr32(fbd, FBNIC_INTR_MASK_CLEAR(0),
> +			   1u << FBNIC_PCS_MSIX_ENTRY);
> +		return IRQ_HANDLED;
> +	}
> +
> +	link_up = fbd->link_state == FBNIC_LINK_UP;
> +
> +	fbd->link_state = FBNIC_LINK_EVENT;
> +	fbn = netdev_priv(fbd->netdev);
> +
> +	phylink_pcs_change(&fbn->phylink_pcs, link_up);

fbd->link_state seems to be set to FBNIC_LINK_UP when the
mac_link_up(), more specifically fbnic_mac_link_up_asic() gets called.
No, never report back to phylink what phylink asked the MAC/PCS to do!

If you don't know what happened to the link here, then report that the
link went down - in other words, always pass "false" to
phylink_pcs_change() which ensures that phylink will never miss a
link-down event (because it will assume that the link went down.)

I think you could even do:

	struct fbnic_dev *fbd = data;
	struct fbnic_net *fbn;
	int status;

	status = fbd->mac->pcs_get_link_event(fbd);
	if (status == 0) {
		fbnic_wr32(fbd, FBNIC_INTR_MASK_CLEAR(0),
			   1u << FBNIC_PCS_MSIX_ENTRY);
	} else {
		fbn = netdiev_priv(fbd->netdev);

		phylink_pcs_change(&fbn->phylink_pcs, status > 0);
	}

	return IRQ_HANDLED;


> +/**
> + * fbnic_mac_enable - Configure the MAC to enable it to advertise link
> + * @fbd: Pointer to device to initialize
> + *
> + * This function provides basic bringup for the CMAC and sets the link
> + * state to FBNIC_LINK_EVENT which tells the link state check that the
> + * current state is unknown and that interrupts must be enabled after the
> + * check is completed.
> + *
> + * Return: non-zero on failure.
> + **/
> +int fbnic_mac_enable(struct fbnic_dev *fbd)
> +{
> +	struct fbnic_net *fbn = netdev_priv(fbd->netdev);
> +	u32 vector = fbd->pcs_msix_vector;
> +	int err;
> +
> +	/* Request the IRQ for MAC link vector.
> +	 * Map MAC cause to it, and unmask it
> +	 */
> +	err = request_irq(vector, &fbnic_pcs_msix_intr, 0,
> +			  fbd->netdev->name, fbd);
> +	if (err)
> +		return err;
> +
> +	fbnic_wr32(fbd, FBNIC_INTR_MSIX_CTRL(FBNIC_INTR_MSIX_CTRL_PCS_IDX),
> +		   FBNIC_PCS_MSIX_ENTRY | FBNIC_INTR_MSIX_CTRL_ENABLE);
> +
> +	phylink_start(fbn->phylink);
> +
> +	fbnic_wr32(fbd, FBNIC_INTR_SET(0), 1u << FBNIC_PCS_MSIX_ENTRY);

If this is enabling the interrupt, ideally that should be before
phylink_start().

> +void fbnic_mac_disable(struct fbnic_dev *fbd)
> +{
> +	struct fbnic_net *fbn = netdev_priv(fbd->netdev);
> +
> +	/* Nothing to do if link is already disabled */
> +	if (fbd->link_state == FBNIC_LINK_DISABLED)
> +		return;
> +
> +	phylink_stop(fbn->phylink);

Why is this conditional? If you've called phylink_start(), and the
network interface is being taken down administratively, then
phylink_stop() needs to be called no matter what. If the link was
up at that point, phylink will call your mac_link_down() as part
of that. Moreover, the networking layers guarantee that .ndo_stop
won't be called unless .ndo_open has been successfully called.

> +static int fbnic_pcs_get_link_event_asic(struct fbnic_dev *fbd)
> +{
> +	u32 pcs_intr_mask = rd32(fbd, FBNIC_SIG_PCS_INTR_STS);
> +
> +	if (pcs_intr_mask & FBNIC_SIG_PCS_INTR_LINK_DOWN)
> +		return -1;
> +
> +	return (pcs_intr_mask & FBNIC_SIG_PCS_INTR_LINK_UP) ? 1 : 0;
> +}

I think an enum/#define of some symbolic names would be useful both
here and in the interrupt handler so we have something descriptive
instead of -1, 0, 1.

> +static bool fbnic_pcs_get_link_asic(struct fbnic_dev *fbd)
> +{
> +	int link_direction;
> +	bool link;
> +
> +	/* If disabled do not update link_state nor change settings */
> +	if (fbd->link_state == FBNIC_LINK_DISABLED)
> +		return false;

If phylink_stop() has been called (one of the places you set
link_state to FBNIC_LINK_DISABLED) then phylink will force the link
down and will disregard state read from the .pcs_get_state() method.

The other place is when fbnic_pcs_disable_asic() has been called,
which I think is hooked into the .pcs_disable method. Well, if
phylink has called the .pcs_disable method, then it is disconnecting
from this PCS, and won't be calling .pcs_get_state anyway.

So all in all, I think this check is unnecessary and should be removed.

> +
> +	/* In an interrupt driven setup we can just skip the check if
> +	 * the link is up as the interrupt should toggle it to the EVENT
> +	 * state if the link has changed state at any time since the last
> +	 * check.
> +	 */
> +	if (fbd->link_state == FBNIC_LINK_UP)
> +		return true;

Again, don't feed back to phylink what phylink asked you to do!

> +
> +	link_direction = fbnic_pcs_get_link_event_asic(fbd);
> +
> +	/* Clear interrupt state due to recent changes. */
> +	wr32(fbd, FBNIC_SIG_PCS_INTR_STS,
> +	     FBNIC_SIG_PCS_INTR_LINK_DOWN | FBNIC_SIG_PCS_INTR_LINK_UP);
> +
> +	/* If link bounced down clear the PCS_STS bit related to link */
> +	if (link_direction < 0) {
> +		wr32(fbd, FBNIC_SIG_PCS_OUT0, FBNIC_SIG_PCS_OUT0_LINK |
> +					 FBNIC_SIG_PCS_OUT0_BLOCK_LOCK |
> +					 FBNIC_SIG_PCS_OUT0_AMPS_LOCK);
> +		wr32(fbd, FBNIC_SIG_PCS_OUT1, FBNIC_SIG_PCS_OUT1_FCFEC_LOCK);
> +	}

If the link "bounces" down, then phylink needs to know - but that
would be covered if, when you receive an interrupt, you always
call phylink_pcs_change(..., false). Still, phylink can deal with
latched-low clear-on-read link statuses. I think you want to read
the link status, and if it's indicating link-failed, then clear
the latched link-failed state.

> +
> +	link = fbnic_mac_get_pcs_link_status(fbd);
> +
> +	if (link_direction)
> +		wr32(fbd, FBNIC_SIG_PCS_INTR_MASK,
> +		     link ?  ~FBNIC_SIG_PCS_INTR_LINK_DOWN :
> +			     ~FBNIC_SIG_PCS_INTR_LINK_UP);

Why do you need to change the interrupt mask? Can't you just leave
both enabled and let the hardware tell you when something changes?

> +static int fbnic_pcs_enable_asic(struct fbnic_dev *fbd)
> +{
> +	/* Mask and clear the PCS interrupt, will be enabled by link handler */
> +	wr32(fbd, FBNIC_SIG_PCS_INTR_MASK, ~0);
> +	wr32(fbd, FBNIC_SIG_PCS_INTR_STS, ~0);
> +
> +	/* Pull in settings from FW */
> +	fbnic_pcs_get_fw_settings(fbd);
> +
> +	/* Flush any stale link status info */
> +	wr32(fbd, FBNIC_SIG_PCS_OUT0, FBNIC_SIG_PCS_OUT0_LINK |
> +				 FBNIC_SIG_PCS_OUT0_BLOCK_LOCK |
> +				 FBNIC_SIG_PCS_OUT0_AMPS_LOCK);

If the link went down, it's better to allow phylink to see that.

> +static void fbnic_mac_link_down_asic(struct fbnic_dev *fbd)
> +{
> +	u32 cmd_cfg, mac_ctrl;
> +
> +	if (fbd->link_state == FBNIC_LINK_DOWN)
> +		return;

You shouldn't need this.

> +static void fbnic_mac_link_up_asic(struct fbnic_dev *fbd)
> +{
> +	u32 cmd_cfg, mac_ctrl;
> +
> +	if (fbd->link_state == FBNIC_LINK_UP)
> +		return;

You shouldn't need this.

> +/* Treat the FEC bits as a bitmask laid out as follows:
> + * Bit 0: RS Enabled
> + * Bit 1: BASER(Firecode) Enabled
> + * Bit 2: Autoneg FEC
> + */
> +enum {
> +	FBNIC_FEC_OFF		= 0,
> +	FBNIC_FEC_RS		= 1,
> +	FBNIC_FEC_BASER		= 2,
> +	FBNIC_FEC_AUTO		= 4,
> +};
> +
> +#define FBNIC_FEC_MODE_MASK	(FBNIC_FEC_AUTO - 1)
> +
> +/* Treat the link modes as a set of moldulation/lanes bitmask:

Spelling: modulation

> @@ -22,9 +23,19 @@ struct fbnic_net {
>  
>  	u16 num_napi;
>  
> +	struct phylink *phylink;
> +	struct phylink_config phylink_config;
> +	struct phylink_pcs phylink_pcs;
> +
> +	u8 tx_pause;
> +	u8 rx_pause;

If you passed these flags into your .link_up method, then you don't need
to store them.

> +	u8 fec;
> +	u8 link_mode;

I think "link_mode" can be entirely removed.

> +static void
> +fbnic_phylink_pcs_get_state(struct phylink_pcs *pcs,
> +			    struct phylink_link_state *state)
> +{
> +	struct fbnic_net *fbn = fbnic_pcs_to_net(pcs);
> +	struct fbnic_dev *fbd = fbn->fbd;
> +
> +	/* For now we use hard-coded defaults and FW config to determine
> +	 * the current values. In future patches we will add support for
> +	 * reconfiguring these values and changing link settings.
> +	 */
> +	switch (fbd->fw_cap.link_speed) {
> +	case FBNIC_FW_LINK_SPEED_25R1:
> +		state->speed = SPEED_25000;
> +		break;
> +	case FBNIC_FW_LINK_SPEED_50R2:
> +		state->speed = SPEED_50000;
> +		break;
> +	case FBNIC_FW_LINK_SPEED_100R2:
> +		state->speed = SPEED_100000;
> +		break;
> +	default:
> +		state->speed = SPEED_UNKNOWN;
> +		break;
> +	}
> +
> +	state->pause |= MLO_PAUSE_RX;
> +	state->duplex = DUPLEX_FULL;
> +	state->interface = PHY_INTERFACE_MODE_XLGMII;

Please don't set state->interface, and please read the documentation
for this method:

 * Read the current inband link state from the MAC PCS, reporting the
 * current speed in @state->speed, duplex mode in @state->duplex, pause
 * mode in @state->pause using the %MLO_PAUSE_RX and %MLO_PAUSE_TX bits,
 * negotiation completion state in @state->an_complete, and link up state
 * in @state->link. If possible, @state->lp_advertising should also be
 * populated.

Note that it doesn't say that state->interface should be set (it
shouldn't.)

> +int fbnic_phylink_init(struct net_device *netdev)
> +{
> +	struct fbnic_net *fbn = netdev_priv(netdev);
> +	struct phylink *phylink;
> +
> +	fbn->phylink_pcs.ops = &fbnic_phylink_pcs_ops;

Please also set phylink_pcs.pcs_neg_mode = true (required for modern
drivers), especially as you call the argument "neg_mode" in your
pcs_config function.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!