On Fri, Oct 31, 2025 at 04:17:23PM -0700, Jakub Kicinski wrote:
> On Wed, 29 Oct 2025 03:37:52 -0700 Erni Sri Satya Vennela wrote:
> > diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c
> > b/drivers/net/ethernet/microsoft/mana/mana_en.c
> > index 009e869ef296..48df44889f05 100644
> > --- a/drivers/net/ethernet/microsoft/mana/mana_en.c
> > +++ b/drivers/net/ethernet/microsoft/mana/mana_en.c
> > @@ -494,6 +494,11 @@ static void mana_get_stats64(struct net_device *ndev,
> >
> > netdev_stats_to_stats64(st, &ndev->stats);
> >
> > + if (apc->ac->hwc_timeout_occurred)
> > + netdev_warn_once(ndev, "HWC timeout occurred\n");
>
> I don't think there's much value in this print.
This print is added because, if the user tries to run
"ip -s link interface" for multiple times, he should be warned about
the time out. I will add more info into the print to let the user
know that it is from this action.
>
> > +#define MANA_GF_STATS_PERIOD (2 * HZ)
> > +
> > +static void mana_gf_stats_work_handler(struct work_struct *work)
> > +{
> > + struct mana_context *ac =
> > + container_of(to_delayed_work(work), struct mana_context,
> > gf_stats_work);
> > + int err;
> > +
> > + err = mana_query_gf_stats(ac);
> > + if (err == -ETIMEDOUT) {
> > + /* HWC timeout detected - reset stats and stop rescheduling */
> > + ac->hwc_timeout_occurred = true;
> > + memset(&ac->hc_stats, 0, sizeof(ac->hc_stats));
>
> Not sure I've seen another device using this approach but I can't
> really tell what's the best strategy. The device is unusable if it
> can't provide stats..
In the case where the HWC becomes unresponsive,
there will be a brief interval needed for the driver to recover.
During this period, if users request ethtool stats, they would
receive outdated information. To address this, we proactively
reset the stats to zero, ensuring users do not see stale data.
>
> > + return;
> > + }
> > + queue_delayed_work(ac->gf_stats_wq, &ac->gf_stats_work,
> > MANA_GF_STATS_PERIOD);
> > +}
> > +
> > int mana_probe(struct gdma_dev *gd, bool resuming)
> > {
> > struct gdma_context *gc = gd->gdma_context;
> > @@ -3478,6 +3503,15 @@ int mana_probe(struct gdma_dev *gd, bool resuming)
> > }
> >
> > err = add_adev(gd, "eth");
> > + ac->gf_stats_wq = create_singlethread_workqueue("mana_gf_stats");
>
> Why are you creating a workqueue? You can use system queues.
Thankyou for the suggestion. I will integrate it in the next version.
>
> > + queue_delayed_work(ac->gf_stats_wq, &ac->gf_stats_work,
> > MANA_GF_STATS_PERIOD);
>
> ls wrap long lines at 80 chars.
I will make this change for the next verison.
>
> > diff --git a/drivers/net/ethernet/microsoft/mana/mana_ethtool.c
> > b/drivers/net/ethernet/microsoft/mana/mana_ethtool.c
> > index 3dfd96146424..99e811208683 100644
> > --- a/drivers/net/ethernet/microsoft/mana/mana_ethtool.c
> > +++ b/drivers/net/ethernet/microsoft/mana/mana_ethtool.c
> > @@ -213,8 +213,6 @@ static void mana_get_ethtool_stats(struct net_device
> > *ndev,
> >
> > if (!apc->port_is_up)
> > return;
> > - /* we call mana function to update stats from GDMA */
> > - mana_query_gf_stats(apc->ac);
>
> Why delete this? We can get fresh stats for the user in this context.
We want to prevent some user that run ethtool too frequently, like
thousands of times / sec, to overwhelm the HW channel.
