On Tue, Jan 29, 2019 at 2:09 AM Mathias Thore <mathias.th...@infinera.com> wrote: > > Is there a scenario where we are clearing the TX ring but don't want to reset > the BQL TX queue?
Right now the function is also used on interface open/close, driver removal and driver resumption besides the timeout situation. I think the reseting BQL queue is either not neccessary or already called explicitly for the other scenarios. > > I think it makes sense to keep it in ucc_geth_free_tx since the reason it is > needed isn't the timeout per se, but rather the clearing of the TX ring. This > way, it will be performed no matter why the driver ends up calling this > function. I don't see a consensus on when netdev_reset_queue() should be called among existing drivers. Doing it on buffer ring cleanup probably can be future-proofing. But if we want to use this approach, we can remove the redundent netdev_reset_queue() calls in open/close functions. Regards, Leo > > -----Original Message----- > From: Li Yang [mailto:leoyang...@nxp.com] > Sent: Monday, 28 January 2019 22:37 > To: Mathias Thore <mathias.th...@infinera.com> > Cc: Christophe Leroy <christophe.le...@c-s.fr>; netdev@vger.kernel.org; > linuxppc-...@lists.ozlabs.org; David Gounaris <david.gouna...@infinera.com>; > Joakim Tjernlund <joakim.tjernl...@infinera.com> > Subject: Re: [PATCH] ucc_geth: Reset BQL queue when stopping device > > On Mon, Jan 28, 2019 at 8:37 AM Mathias Thore <mathias.th...@infinera.com> > wrote: > > > > Hi, > > > > > > This is what we observed: there was a storm on the medium so that our > > controller could not do its TX, resulting in timeout. When timeout occurs, > > the driver clears all descriptors from the TX queue. The function called in > > this patch is used to reflect this clearing also in the BQL layer. Without > > it, the controller would get stuck, unable to perform TX, even several > > minutes after the storm had ended. Bringing the device down and then up > > again would solve the problem, but this patch also solves it automatically. > > The explanation makes sense. So this should only be required in the timeout > scenario instead of other clean up scenarios like device shutdown? If so, it > probably it will be better to be done in ucc_geth_timeout_work()? > > > > > > > Some other drivers do the same, for example e1000e driver calls > > netdev_reset_queue in its e1000_clean_tx_ring function. It is possible that > > other drivers should do the same; I have no way of verifying this. > > > > > > Regards, > > > > Mathias > > > > -- > > > > > > From: Christophe Leroy <christophe.le...@c-s.fr> > > Sent: Monday, January 28, 2019 10:48 AM > > To: Mathias Thore; leoyang...@nxp.com; netdev@vger.kernel.org; > > linuxppc-...@lists.ozlabs.org; David Gounaris; Joakim Tjernlund > > Subject: Re: [PATCH] ucc_geth: Reset BQL queue when stopping device > > > > > > CAUTION: This email originated from outside of the organization. Do not > > click links or open attachments unless you recognize the sender and know > > the content is safe. > > > > > > Hi, > > > > Le 28/01/2019 à 10:07, Mathias Thore a écrit : > > > After a timeout event caused by for example a broadcast storm, when > > > the MAC and PHY are reset, the BQL TX queue needs to be reset as > > > well. Otherwise, the device will exhibit severe performance issues > > > even after the storm has ended. > > > > What are the symptomns ? > > > > Is this reset needed on any network driver in that case, or is it > > something particular for the ucc_geth ? > > For instance, the freescale fs_enet doesn't have that reset. Should it > > have it too ? > > > > Christophe > > > > > > > > Co-authored-by: David Gounaris <david.gouna...@infinera.com> > > > Signed-off-by: Mathias Thore <mathias.th...@infinera.com> > > > --- > > > drivers/net/ethernet/freescale/ucc_geth.c | 2 ++ > > > 1 file changed, 2 insertions(+) > > > > > > diff --git a/drivers/net/ethernet/freescale/ucc_geth.c > > > b/drivers/net/ethernet/freescale/ucc_geth.c > > > index c3d539e209ed..eb3e65e8868f 100644 > > > --- a/drivers/net/ethernet/freescale/ucc_geth.c > > > +++ b/drivers/net/ethernet/freescale/ucc_geth.c > > > @@ -1879,6 +1879,8 @@ static void ucc_geth_free_tx(struct > > > ucc_geth_private *ugeth) > > > u16 i, j; > > > u8 __iomem *bd; > > > > > > + netdev_reset_queue(ugeth->ndev); > > > + > > > ug_info = ugeth->ug_info; > > > uf_info = &ug_info->uf_info; > > > > > > > >
