[PATCH 5/6] wifi: mac80211: export ieee80211_tpt_led_trig_tx/rx for driver

[Ryder Lee]

On Fri, 2023-08-25 at 11:08 -0700, Jeff Johnson wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  On 8/25/2023 10:47 AM, Yi-Chia Hsieh wrote:
> > Whenever the H/W path is enabled and traffic is in the binding
> state,
> > mac80211 is not aware of the traffic. Consequently, the LED does
> not
> > blink for that reason.
> > 
> > The ieee80211_tpt_led_trig_tx/rx functions are exported for the
> driver
> > so that we can report the tx and rx bytes from the driver when
> > the H/W path is being used.
> > 
> > Signed-off-by: Yi-Chia Hsieh <yi-chia.hsieh at mediatek.com>
> > ---
> >   include/net/mac80211.h | 17 +++++++++++++++++
> >   net/mac80211/led.c     | 18 ++++++++++++++++++
> >   net/mac80211/led.h     | 18 ------------------
> >   net/mac80211/rx.c      |  2 +-
> >   net/mac80211/tx.c      |  4 ++--
> >   5 files changed, 38 insertions(+), 21 deletions(-)
> > 
> > diff --git a/include/net/mac80211.h b/include/net/mac80211.h
> > index 3a8a2d2c58c3..53804822dc8d 100644
> > --- a/include/net/mac80211.h
> > +++ b/include/net/mac80211.h
> > @@ -4729,6 +4729,8 @@ __ieee80211_create_tpt_led_trigger(struct
> ieee80211_hw *hw,
> >      unsigned int flags,
> >      const struct ieee80211_tpt_blink *blink_table,
> >      unsigned int blink_table_len);
> > +void __ieee80211_tpt_led_trig_tx(struct ieee80211_hw *hw, int
> bytes);
> > +void __ieee80211_tpt_led_trig_rx(struct ieee80211_hw *hw, int
> bytes);
> 
> suggest these should be in led.h since they are local to mac80211
> and 
> should not be directly invoked by clients of mac80211
> 
> >   #endif
> >   /**
> >    * ieee80211_get_tx_led_name - get name of TX LED
> > @@ -4839,6 +4841,21 @@ ieee80211_create_tpt_led_trigger(struct
> ieee80211_hw *hw, unsigned int flags,
> >   #endif
> >   }
> >   
> > +static inline void
> > +ieee80211_tpt_led_trig_tx(struct ieee80211_hw *hw, int bytes)
> > +{
> > +#ifdef CPTCFG_MAC80211_LEDS
> 
> why isn't this using CONFIG_MAC80211_LEDS??
> 

Will fix.
> > +__ieee80211_tpt_led_trig_tx(hw, bytes);
> > +#endif
> > +}
> > +
> > +static inline void
> > +ieee80211_tpt_led_trig_rx(struct ieee80211_hw *hw, int bytes)
> > +{
> > +#ifdef CPTCFG_MAC80211_LEDS
> > +__ieee80211_tpt_led_trig_rx(hw, bytes);
> > +#endif
> > +}
> >   /**
> >    * ieee80211_unregister_hw - Unregister a hardware device
> >    *
> > diff --git a/net/mac80211/led.c b/net/mac80211/led.c
> > index 2dc732147e85..af03a2ef5c6a 100644
> > --- a/net/mac80211/led.c
> > +++ b/net/mac80211/led.c
> > @@ -319,6 +319,24 @@ __ieee80211_create_tpt_led_trigger(struct
> ieee80211_hw *hw,
> >   }
> >   EXPORT_SYMBOL(__ieee80211_create_tpt_led_trigger);
> >   
> > +void __ieee80211_tpt_led_trig_tx(struct ieee80211_hw *hw, int
> bytes)
> > +{
> > +struct ieee80211_local *local = hw_to_local(hw);
> > +
> > +if (atomic_read(&local->tpt_led_active))
> > +local->tpt_led_trigger->tx_bytes += bytes;
> > +}
> > +EXPORT_SYMBOL(__ieee80211_tpt_led_trig_tx);
> > +
> > +void __ieee80211_tpt_led_trig_rx(struct ieee80211_hw *hw, int
> bytes)
> > +{
> > +struct ieee80211_local *local = hw_to_local(hw);
> > +
> > +if (atomic_read(&local->tpt_led_active))
> > +local->tpt_led_trigger->rx_bytes += bytes;
> > +}
> > +EXPORT_SYMBOL(__ieee80211_tpt_led_trig_rx);
> > +
> 
> why are you making these exported implementations instead of keeping 
> them as inline in led.h?
> 

We thought so. However, led.c has many same exported implementations,
including the local functions you suggested above. i.e.
__ieee80211_get_assoc_led_name() For the sake of consistency, we just
copy-pasted one of those functions. Do you want us to change this?

> >   static void ieee80211_start_tpt_led_trig(struct ieee80211_local
> *local)
> >   {
> >   struct tpt_led_trigger *tpt_trig = local->tpt_led_trigger;
> > diff --git a/net/mac80211/led.h b/net/mac80211/led.h
> > index d25f13346b82..98db4356d0de 100644
> > --- a/net/mac80211/led.h
> > +++ b/net/mac80211/led.h
> > @@ -66,21 +66,3 @@ static inline void
> ieee80211_mod_tpt_led_trig(struct ieee80211_local *local,
> >   {
> >   }
> >   #endif
> > -
> > -static inline void
> > -ieee80211_tpt_led_trig_tx(struct ieee80211_local *local, int
> bytes)
> > -{
> > -#ifdef CONFIG_MAC80211_LEDS
> > -if (atomic_read(&local->tpt_led_active))
> > -local->tpt_led_trigger->tx_bytes += bytes;
> > -#endif
> > -}
> > -
> > -static inline void
> > -ieee80211_tpt_led_trig_rx(struct ieee80211_local *local, int
> bytes)
> > -{
> > -#ifdef CONFIG_MAC80211_LEDS
> > -if (atomic_read(&local->tpt_led_active))
> > -local->tpt_led_trigger->rx_bytes += bytes;
> > -#endif
> > -}
> > diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
> > index 4f707d2a160f..5747d7dac4d7 100644
> > --- a/net/mac80211/rx.c
> > +++ b/net/mac80211/rx.c
> > @@ -5375,7 +5375,7 @@ void ieee80211_rx_list(struct ieee80211_hw
> *hw, struct ieee80211_sta *pubsta,
> >   if (skb) {
> >   if ((status->flag & RX_FLAG_8023) ||
> >   ieee80211_is_data_present(hdr->frame_control))
> > -ieee80211_tpt_led_trig_rx(local, skb->len);
> > +ieee80211_tpt_led_trig_rx(&local->hw, skb->len);
> 
> what is the rationale for changing the signature, especially given
> that 
> the first thing the implementations do is local = hw_to_local(hw)
> 

We want to make ieee80211_tpt_led_trig_rx() more generic so that it can
be called by driver as what he did in patch 6/6. But the
ieee80211_local can't be accessed other than mac80211 so he made this
change to use ieee80211_hw instead. Does that make sense?

Ryder