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@xxxxxxxxxxxx>
> > ---
> > 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
