Re: [net PATCH] net: stmmac: No need to calculate speed divider when offload is disabled

[xiaolei wang]

On 6/15/24 22:47, Simon Horman wrote:
> CAUTION: This email comes from a non Wind River email account!
> Do not click links or open attachments unless you recognize the sender and know the content is safe.
>
> On Fri, Jun 14, 2024 at 04:19:16PM +0800, Xiaolei Wang wrote:
> > commit be27b8965297 ("net: stmmac: replace priv->speed with
> > the portTransmitRate from the tc-cbs parameters") introduced
> > a problem. When deleting, it prompts "Invalid portTransmitRate
> > 0 (idleSlope - sendSlope)" and exits. Add judgment on cbs.enable.
> > Only when offload is enabled, speed divider needs to be calculated.
> >
> > Fixes: be27b8965297 ("net: stmmac: replace priv->speed with the portTransmitRate from the tc-cbs parameters")
> > Signed-off-by: Xiaolei Wang <xiaolei.wang@xxxxxxxxxxxxx>
> > ---
> >   .../net/ethernet/stmicro/stmmac/stmmac_tc.c   | 38 ++++++++++---------
> >   1 file changed, 20 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
> > index 1562fbdd0a04..b0fd2d6e525e 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
> > @@ -358,24 +358,26 @@ static int tc_setup_cbs(struct stmmac_priv *priv,
> >
> >        port_transmit_rate_kbps = qopt->idleslope - qopt->sendslope;
> >
> > -     /* Port Transmit Rate and Speed Divider */
> > -     switch (div_s64(port_transmit_rate_kbps, 1000)) {
> > -     case SPEED_10000:
> > -     case SPEED_5000:
> > -             ptr = 32;
> > -             break;
> > -     case SPEED_2500:
> > -     case SPEED_1000:
> > -             ptr = 8;
> > -             break;
> > -     case SPEED_100:
> > -             ptr = 4;
> > -             break;
> > -     default:
> > -             netdev_err(priv->dev,
> > -                        "Invalid portTransmitRate %lld (idleSlope - sendSlope)\n",
> > -                        port_transmit_rate_kbps);
> > -             return -EINVAL;
> > +     if (qopt->enable) {
> > +             /* Port Transmit Rate and Speed Divider */
> > +             switch (div_s64(port_transmit_rate_kbps, 1000)) {
> > +             case SPEED_10000:
> > +             case SPEED_5000:
> > +                     ptr = 32;
> > +                     break;
> > +             case SPEED_2500:
> > +             case SPEED_1000:
> > +                     ptr = 8;
> > +                     break;
> > +             case SPEED_100:
> > +                     ptr = 4;
> > +                     break;
> > +             default:
> > +                     netdev_err(priv->dev,
> > +                                "Invalid portTransmitRate %lld (idleSlope - sendSlope)\n",
> > +                                port_transmit_rate_kbps);
> > +                     return -EINVAL;
> > +             }
> >        }
> >        mode_to_use = priv->plat->tx_queues_cfg[queue].mode_to_use;
> Hi Xiaolei Wang,
>
> The code following this function looks like this:
>
>          if (mode_to_use == MTL_QUEUE_DCB && qopt->enable) {
>                  ret = stmmac_dma_qmode(priv, priv->ioaddr, queue, MTL_QUEUE_AVB);
>                  if (ret)
>                          return ret;
>                  priv->plat->tx_queues_cfg[queue].mode_to_use = MTL_QUEUE_AVB;
>          } else if (!qopt->enable) {
>                  ret = stmmac_dma_qmode(priv, priv->ioaddr, queue,
>                                         MTL_QUEUE_DCB);
>                  if (ret)
>                          return ret;
>                  priv->plat->tx_queues_cfg[queue].mode_to_use = MTL_QUEUE_DCB;
>          }
>
>          /* Final adjustments for HW */
>          value = div_s64(qopt->idleslope * 1024ll * ptr, port_transmit_rate_kbps);
>          priv->plat->tx_queues_cfg[queue].idle_slope = value & GENMASK(31, 0);
>
>          value = div_s64(-qopt->sendslope * 1024ll * ptr, port_transmit_rate_kbps);
>          priv->plat->tx_queues_cfg[queue].send_slope = value & GENMASK(31, 0);
>
> And the div_s64() lines above appear to use
> ptr uninitialised in the !qopt->enable case.

Oh, when deleting the configuration, idleslope and sendslope are both 0, 
do you mean we also need to set ptr to 0?

thanks

xiaolei

> Flagged by Smatch.
>
> --
> pw-bot: changes-requested