Re: [PATCH v4 06/11] net: ethernet: mtk-eth-mac: new driver

[Bartosz Golaszewski <brgl@bgdev.pl>]

śr., 20 maj 2020 o 23:23 Arnd Bergmann <arnd@arndb.de> napisał(a):
>
> On Wed, May 20, 2020 at 7:35 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > śr., 20 maj 2020 o 16:37 Arnd Bergmann <arnd@arndb.de> napisał(a):
>
> > > I just noticed how the naming of NET_MEDIATEK_MAC and NET_MEDIATEK_SOC
> > > for two different drivers doing the same thing is really confusing.
> > >
> > > Maybe someone can come up with a better name, such as one
> > > based on the soc it first showed up in.
> > >
> >
> > This has been discussed under one of the previous submissions.
> > MediaTek wants to use this IP on future designs as well and it's
> > already used on multiple SoCs so they want the name to be generic. I
> > also argued that this is a driver strongly tied to a specific
> > platform(s) so if someone wants to compile it - they probably know
> > what they're doing.
> >
> > That being said: I verified with MediaTek and the name of the IP I can
> > use is "star" so they proposed "mtk-star-eth". I would personally
> > maybe go with "mtk-star-mac". How about those two?
>
> Both seem fine to me. If this was previously discussed, I don't want
> do further bike-shedding and I'd trust you to pick a sensible name
> based on the earlier discussions.
>
> > >  +               /* One of the counters reached 0x8000000 - update stats and
> > > > +                * reset all counters.
> > > > +                */
> > > > +               if (unlikely(status & MTK_MAC_REG_INT_STS_MIB_CNT_TH)) {
> > > > +                       mtk_mac_intr_disable_stats(priv);
> > > > +                       schedule_work(&priv->stats_work);
> > > > +               }
> > > > + befor
> > > > +               mtk_mac_intr_ack_all(priv);
> > >
> > > The ack here needs to be dropped, otherwise you can get further
> > > interrupts before the bottom half has had a chance to run.
> > >
> >
> > My thinking was this: if I mask the relevant interrupt (TX/RX
> > complete) and ack it right away, the status bit will be asserted on
> > the next packet received/sent but the process won't get interrupted
> > and when I unmask it, it will fire right away and I won't have to
> > recheck the status register. I noticed that if I ack it at the end of
> > napi poll callback, I end up missing certain TX complete interrupts
> > and end up seeing a lot of retransmissions even if I reread the status
> > register. I'm not yet sure where this race happens.
>
> Right, I see. If you just ack at the end of the poll function, you need
> to check the rings again to ensure you did not miss an interrupt
> between checking observing both rings to be empty and the irq-ack.
>
> I suspect it's still cheaper to check the two rings with an uncached
> read from memory than to to do the read-modify-write on the mmio,
> but you'd have to measure that to be sure.
>

Unfortunately the PHY on the board I have is 100Mbps which is the
limiting factor in benchmarking this driver. :(

If you're fine with this - I'd like to fix the minor issues you
pointed out and stick with the current approach for now. We can always
fix the implementation in the future once a board with a Gigabit PHY
is out. Most ethernet drivers don't use such fine-grained interrupt
control anyway. I expect the performance differences to be miniscule
really.

Bart