Re: [PATCH net-next] net: lan966x: Improve the CPU TX bitrate.

[Andrew Lunn]

On Tue, Mar 08, 2022 at 11:30:00PM +0100, Horatiu Vultur wrote:
> The 03/08/2022 22:36, Andrew Lunn wrote:
> > 
> > >  static int lan966x_port_inj_ready(struct lan966x *lan966x, u8 grp)
> > >  {
> > > -     u32 val;
> > > +     unsigned long time = jiffies + usecs_to_jiffies(READL_TIMEOUT_US);
> > > +     int ret = 0;
> > >
> > > -     return readx_poll_timeout_atomic(lan966x_port_inj_status, lan966x, val,
> > > -                                      QS_INJ_STATUS_FIFO_RDY_GET(val) & BIT(grp),
> > > -                                      READL_SLEEP_US, READL_TIMEOUT_US);
> > > +     while (!(lan_rd(lan966x, QS_INJ_STATUS) &
> > > +              QS_INJ_STATUS_FIFO_RDY_SET(BIT(grp)))) {
> > > +             if (time_after(jiffies, time)) {
> > > +                     ret = -ETIMEDOUT;
> > > +                     break;
> > > +             }
> > 
> > Did you try setting READL_SLEEP_US to 0? readx_poll_timeout_atomic()
> > explicitly supports that.
> 
> I have tried but it didn't improve. It was the same as before.

The reason i recommend ipoll.h is that your implementation has the
usual bug, which iopoll does not have. Since you are using _atomic()
it is less of an issue, but it still exists.

     while (!(lan_rd(lan966x, QS_INJ_STATUS) &
              QS_INJ_STATUS_FIFO_RDY_SET(BIT(grp)))) {

Say you take an interrupt here

             if (time_after(jiffies, time)) {
                     ret = -ETIMEDOUT;
                     break;
             }


The interrupt takes a while, so that by the time you get to
time_after(), you have reached your timeout. So -ETIMEDOUT is
returned. But in fact, the hardware has done its thing, and if you
where to read the status the bit would be set, and you should actually
return O.K, not an error.

iopoll does another check of the status before deciding to return
-ETIMEDOUT or O.K.

If you decide to simply check the status directly after the write, i
suggest you then use readx_poll_timeout_atomic() if you do need to
poll.

	Andrew