RE: [PATCH v2 2/6] wifi: mt76: mt7925: add EHT control support based on the CLC data

[Ping-Ke Shih <pkshih@realtek.com>]

Mingyen Hsieh (謝明諺) <Mingyen.Hsieh@mediatek.com> wrote:
> On Wed, 2025-03-05 at 01:21 +0000, Ping-Ke Shih wrote:
> > 
> > External email : Please do not click links or open attachments until
> > you have verified the sender or the content.
> > 
> > 
> > Mingyen Hsieh (謝明諺) <mailto:Mingyen.Hsieh@mediatek.com>  wrote:
> > > On Tue, 2025-03-04 at 09:10 +0000, Ping-Ke Shih wrote:
> > > > Mingyen Hsieh <mailto:mingyen.hsieh@mediatek.com> wrote:
> > > > 
> > > > [...]
> > > > 
> > > > > 
> > > > > +void mt7925_regd_be_ctrl(struct mt792x_dev *dev, u8 *alpha2)
> > > > > +{
> > > > > +       struct mt792x_phy *phy = &dev->phy;
> > > > > +       struct mt7925_clc_rule_v2 *rule;
> > > > > +       struct mt7925_clc *clc;
> > > > > +       bool old = dev->has_eht, new = true;
> > > > > +       u8 *pos;
> > > > > +
> > > > > +       if (!phy->clc[MT792x_CLC_BE_CTRL])
> > > > > +               goto out;
> > > > > +
> > > > > +       clc = (struct mt7925_clc *)phy-
> > > > > >clc[MT792x_CLC_BE_CTRL];
> > > > > +       pos = clc->data;
> > > > > +
> > > > > +       while (1) {
> > > > 
> > > > while (1) could lead infinite loop unexpectedly.
> > > > Adding a checking of clc->len would be safer.
> > > > 
> > > > > +               rule = (struct mt7925_clc_rule_v2 *)pos;
> > > > > +
> > > > > +               if (rule->alpha2[0] == alpha2[0] &&
> > > > > +                   rule->alpha2[1] == alpha2[1]) {
> > > > > +                       new = false;
> > > > > +                       break;
> > > > > +               }
> > > > > +
> > > > > +               /* Check the last one */
> > > > > +               if (rule->flag && BIT(0))
> > > > > +                       break;
> > > > > +
> > > 
> > > Hi Ping-Ke,
> > > 
> > > I designed a flag to prevent the infinite loop.
> > 
> > I knew.
> > 
> > People believe their code is safe, but somehow `while (1)` get
> > unexpected
> > result. An alternative way is to use for-loop with limit range, like
> > 
> > clc_end = &clc->data[clc->len];
> > 
> > for (pos = clc->data; pos < clc_end; pos += sizeof(*rule))
> > 
> > Just FYR.
> > 
> 
> Ok. I got it. Thanks for the suggestions.
> Perhaps i will modify it in the next version.
> 

Good. :-)

By the way, could you check if you replied in plain text mode?
From my outlook, it looks like in html mode, but patchwork can
recognize it well [1].

[1] https://patchwork.kernel.org/project/linux-wireless/patch/20250304062854.829194-2-mingyen.hsieh@mediatek.com/

> > > 
> > > > > +               pos += sizeof(*rule);
> > > > > +       }
> > > > > +
> > > > > +out:
> > > > > +       if (old == new)
> > > > > +               return;
> > > > > +
> > > > > +       dev->has_eht = new;
> > > > > +       mt7925_set_stream_he_eht_caps(phy);
> > > > > +}
> > > > > +
> > > > 
> > > > [...]
> > > > 
> > > 
>