Re: [PATCH net-next 3/3] net: eth: fbnic: Add led support

[Andrew Lunn]

On Thu, May 21, 2026 at 10:05:43AM -0400, Mike Marciniszyn wrote:
> On Wed, May 20, 2026 at 10:37:01PM +0200, Andrew Lunn wrote:
> > > +/**
> > > + * fbnic_set_phys_id - Used to strobe the MAC LEDs in a recognizable pattern
> > > + * @netdev: Interface/port to strobe the LEDs for
> > > + * @phys_id_state: State requested by the call
> > > + *
> > > + * This function can really be broken down into two parts. There are the
> > > + * ACTIVE/INACTIVE states which really are meant to be defining the start
> > > + * and stop of the LED strobing. There is also the ON/OFF states which are
> > > + * used to provide us with a way of telling us that we should be turning
> > > + * the LED on and/or off.
> > > + *
> > > + * We translate these calls and pass them off to the MAC layer. They will
> > > + * be used to initialize a strobe, then on and off will be used to cycle
> > > + * between the patterns, and finally we will restore the original LED state.
> > > + *
> > > + * We will return 2 when we are requested to go active. This will tell the
> > > + * call that it will need to call back to turn on/off the LED twice every
> > > + * second.
> > > + *
> > > + * Return: blink in half second intervals on success, negative value on failure
> > > + */
> >
> > Could you drop this for the moment. Ideally, we want the LED core to
> > be implementing this somehow, so that all devices using the core get
> > this functionality, rather than put it in every driver.
> >
> 
> The driver does exploit the LED core.

The driver does make use of the LED core, but it is not using the LED
subsystem to do phys_id. Ideally, we want either the LED core, or
maybe a netdev helper to expose a function which can be used as the
ethtool op. The driver itself does nothing, except expose its LEDs to
the core. We than have a function any driver with LEDs can use. A
generic solution.

> The actually interface to the
> LEDs are simple on/off settings.  The strobing follows the OCP spec
> to alternate between the "below" speed color (amber for fbnic) and the "at"
> speed color (blue for fbnic).

You appear to need a specific implementation of phys_id. What i'm
asking is that you first implement MAC LEDs as Linux currently defines
them. A simple clean implementation. Once that is merged, we can
figure out how to implement what OCP required, in a generic way that
any vendor can use.

> 
> The only complication is the the CSR bit for the activity LED is 1 implies
> off and zero implies on.  The activity LED flashes automatically
> based on actual network activity provides it is set to 0 (on).
> 
> The return to the LED core gives the count of the number of cycles per
> second and each cycle alternates between amber and blue.  The driver
> is a slave to the ETHTOOL_ID_* phys_id state from the LED core based
> on that timing.
> 
> I'm not sure what else would need to go into the LED core?

An LED driver should first expose an simple on/off method. If it
supports acceleration, it can then optionally expose its acceleration
capabilities. 


> > > +static int fbnic_led_hw_ctl_set(struct led_classdev *led_cdev,
> > > +				unsigned long flags)
> > > +{
> > > +	struct fbnic_led_cdev *ldev = led_classdev_to_fbnic_led(led_cdev);
> > > +	struct fbnic_dev *fbd = led_cdev_to_fbd(led_cdev);
> > > +	int led_idx = fbnic_led_get_idx(fbd, ldev);
> > > +	u32 supported_modes;
> > > +
> > > +	supported_modes = fbnic_led_get_supported_modes(fbd, led_idx);
> > > +
> > > +	if (flags & ~supported_modes)
> > > +		return -EINVAL;
> > > +
> > > +	/* Turn on activity LED only when both the TX and RX flags are set. */
> > > +	if (led_idx == FBNIC_LED_ACTIVITY && (flags & supported_modes) &&
> > > +	    flags != supported_modes) {
> > > +		dev_warn(fbd->dev,
> > > +			 "Invalid activity LED mode(s): 0x%lx\n", flags);
> > > +		return -EINVAL;
> >
> > How does this happen? It should be that the core will not ask you to
> > do something you cannot do, if you have answered -EOPNOTSUPP in
> > fbnic_led_hw_ctl_is_supported().
> >
> 
> Will need to investigate this.
> 
> > > +static int fbnic_led_brightness_set_blocking(struct led_classdev *led_cdev,
> > > +					     enum led_brightness brightness)
> > > +{
> > > +	struct fbnic_led_cdev *ldev = led_classdev_to_fbnic_led(led_cdev);
> > > +	struct fbnic_dev *fbd = led_cdev_to_fbd(led_cdev);
> > > +	int led_idx = fbnic_led_get_idx(fbd, ldev);
> > > +
> > > +	mutex_lock(&fbd->led_mutex);
> > > +	if (!brightness) {
> > > +		fbd->leds[led_idx].enabled_modes = 0;
> > > +		fbd->leds[led_idx].strobe_mode = 0;
> > > +	} else {
> > > +		u32 mode;
> > > +
> > > +		switch (led_idx) {
> > > +		case FBNIC_LED_ACTIVITY:
> > > +			fbd->leds[led_idx].enabled_modes =
> > > +				BIT(TRIGGER_NETDEV_TX) | BIT(TRIGGER_NETDEV_RX);
> > > +			break;
> > > +		default:
> > > +			mode = fbnic_led_get_link_speed_mode(fbd);
> > > +			fbd->leds[led_idx].enabled_modes = mode;
> > > +			break;
> > > +		}
> > > +	}
> >
> > This looks odd. led_brightness_set() is simply used to set the LED to
> > on or off. The netdev trigger will use it to software blink the LEDs
> > when asked to do something which cannot be offloaded to the hardware.
> >
> > And when the LED is being used to show NUMLOCK, or Morse code crash
> > dump, network activity should not intervene.
> >
> 
> Will need to investigate this.
> 
> > > diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_mac.c b/drivers/net/ethernet/meta/fbnic/fbnic_mac.c
> > > index 53b7a938b4c2..c6b1130f9159 100644
> > > --- a/drivers/net/ethernet/meta/fbnic/fbnic_mac.c
> > > +++ b/drivers/net/ethernet/meta/fbnic/fbnic_mac.c
> >
> >
> > > @@ -623,6 +623,53 @@ static bool fbnic_pmd_update_state(struct fbnic_dev *fbd, bool signal_detect)
> > >  	return false;
> > >  }
> > >
> > > +u32 fbnic_get_link_speed(u8 link_speed)
> > > +{
> > > +	switch (link_speed) {
> > > +	case FBNIC_FW_LINK_MODE_25CR:
> > > +		return SPEED_25000;
> > > +	case FBNIC_FW_LINK_MODE_50CR2:
> > > +	case FBNIC_FW_LINK_MODE_50CR:
> > > +		return SPEED_50000;
> > > +	case FBNIC_FW_LINK_MODE_100CR2:
> > > +		return SPEED_100000;
> > > +	default:
> > > +		return 0;
> > > +	}
> > > +}
> > > +
> > > +static void fbnic_set_led_state(struct fbnic_dev *fbd, int state)
> > > +{
> > > +	mutex_lock(&fbd->led_mutex);
> > > +
> > > +	/* alternating amber,blue IDs device every half second */
> > > +	switch (state) {
> > > +	case FBNIC_LED_OFF: /* amber on, blue off */
> > > +		fbd->leds[FBNIC_LED_LINK_AMBER].strobe_mode = FBNIC_STROBE_ON;
> > > +		fbd->leds[FBNIC_LED_LINK_BLUE].strobe_mode = FBNIC_STROBE_OFF;
> > > +		break;
> > > +	case FBNIC_LED_ON: /* amber off, blue on */
> > > +		fbd->leds[FBNIC_LED_LINK_AMBER].strobe_mode = FBNIC_STROBE_OFF;
> > > +		fbd->leds[FBNIC_LED_LINK_BLUE].strobe_mode = FBNIC_STROBE_ON;
> > > +		break;
> > > +	case FBNIC_LED_RESTORE:
> > > +		fbd->leds[FBNIC_LED_LINK_AMBER].strobe_mode =
> > > +			FBNIC_STROBE_DISABLED;
> > > +		fbd->leds[FBNIC_LED_LINK_BLUE].strobe_mode =
> > > +			FBNIC_STROBE_DISABLED;
> > > +		break;
> > > +	case FBNIC_LED_STROBE_INIT: /* a no-op */
> > > +		/* Initialization is a no-op; LED toggling happens on ON/OFF */
> > > +		goto out_unlock;
> > > +	default:
> > > +		goto out_unlock;
> > > +	}
> > > +
> > > +	fbnic_led_update_csr(fbd);
> > > +out_unlock:
> > > +	mutex_unlock(&fbd->led_mutex);
> > > +}
> > > +
> >
> > This appears to be something different to trig-netdev and sys class
> > LED control of the LED. Please could you move this out into another
> > patch. Add clean support for the LED first, and them mess it all up in
> > follow up patches adding all your vendor specific stuff. We can then
> > think about if some of that can be moved into the LED core/trig-netdev
> > somehow.
> >
> 
> This IS the slave to the LED core ID mechanism.
> 
> If I'm understanding your suggestion the ethtool id, this routine and
> the fbnic_led_update_csr() need to be a separarate patch?

I'm saying start KISS. All the LED core really needs is on/off. It
will then blink the LEDs as required.

The next step is to then expose whatever acceleration the hardware
provides, so that in some configurations software blinking is not
needed, the hardware does it.

Then we can think about whatever OCP requires and how we can do a
generic implementation.

	Andrew