Messages in this thread | | | Date | Wed, 9 Mar 2022 23:05:16 +0100 | From | Horatiu Vultur <> | Subject | Re: [PATCH net-next] net: lan966x: Improve the CPU TX bitrate. |
| |
The 03/09/2022 14:11, Andrew Lunn wrote: > > 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.
That is a good catch and really nice explanation! Then if I add also the check at the end, then it should be also OK.
> > 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
-- /Horatiu
| |