| Message ID | 20231120084606.4083194-7-claudiu.beznea.uj@bp.renesas.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: ravb: Add suspend to RAM and runtime PM support for RZ/G3S | expand |
On 11/20/23 11:45 AM, Claudiu wrote: > From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > ravb_poll() initial code used to interrogate the first descriptor of the > RX queue in case gptp is false to know if ravb_rx() should be called. > This is done for non GPTP IPs. For GPTP IPs the driver PTP specific It's called gPTP, AFAIK. > information was used to know if receive function should be called. As > every IP has it's own receive function that interrogates the RX descriptor Its own. > list in the same way the ravb_poll() was doing there is no need to double > check this in ravb_poll(). Removing the code form ravb_poll() and From ravb_poll(). > adjusting ravb_rx_gbeth() leads to a cleaner code. > > Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > --- > drivers/net/ethernet/renesas/ravb_main.c | 18 ++++++------------ > 1 file changed, 6 insertions(+), 12 deletions(-) > > diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c > index 588e3be692d3..0fc9810c5e78 100644 > --- a/drivers/net/ethernet/renesas/ravb_main.c > +++ b/drivers/net/ethernet/renesas/ravb_main.c > @@ -771,12 +771,15 @@ static bool ravb_rx_gbeth(struct net_device *ndev, int *quota, int q) > int limit; > > entry = priv->cur_rx[q] % priv->num_rx_ring[q]; > + desc = &priv->gbeth_rx_ring[entry]; > + if (desc->die_dt == DT_FEMPTY) > + return false; > + I don't understand what this buys us, the following *while* loop will be skipped anyway, and the *for* loop as well, I think... And ravb_rx_rcar() doesn't have that anyway... > @@ -1279,25 +1282,16 @@ static int ravb_poll(struct napi_struct *napi, int budget) > struct net_device *ndev = napi->dev; > struct ravb_private *priv = netdev_priv(ndev); > const struct ravb_hw_info *info = priv->info; > - bool gptp = info->gptp || info->ccc_gac; > - struct ravb_rx_desc *desc; > unsigned long flags; > int q = napi - priv->napi; > int mask = BIT(q); > int quota = budget; > - unsigned int entry; > > - if (!gptp) { > - entry = priv->cur_rx[q] % priv->num_rx_ring[q]; > - desc = &priv->gbeth_rx_ring[entry]; > - } > /* Processing RX Descriptor Ring */ > /* Clear RX interrupt */ > ravb_write(ndev, ~(mask | RIS0_RESERVED), RIS0); > - if (gptp || desc->die_dt != DT_FEMPTY) { > - if (ravb_rx(ndev, "a, q)) > - goto out; > - } I don't really understand this code (despite I've AKCed it)... Biju, could you explain this (well, you tried already but I don't buy it anymore)? > + if (ravb_rx(ndev, "a, q)) > + goto out; This restores the behavior before: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=3d4e37df882b0f4f28b7223a42492650b71252c5 So does look correct. :-) MBR, Sergey
Hi Sergey Shtylyov, > Subject: Re: [PATCH 06/13] net: ravb: Let IP specific receive function to > interrogate descriptors > > On 11/20/23 11:45 AM, Claudiu wrote: > > > From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > > > ravb_poll() initial code used to interrogate the first descriptor of > > the RX queue in case gptp is false to know if ravb_rx() should be > called. > > This is done for non GPTP IPs. For GPTP IPs the driver PTP specific > > It's called gPTP, AFAIK. > > > information was used to know if receive function should be called. As > > every IP has it's own receive function that interrogates the RX > > descriptor > > Its own. > > > list in the same way the ravb_poll() was doing there is no need to > > double check this in ravb_poll(). Removing the code form ravb_poll() > > and > > From ravb_poll(). > > > adjusting ravb_rx_gbeth() leads to a cleaner code. > > > > Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > --- > > drivers/net/ethernet/renesas/ravb_main.c | 18 ++++++------------ > > 1 file changed, 6 insertions(+), 12 deletions(-) > > > > diff --git a/drivers/net/ethernet/renesas/ravb_main.c > > b/drivers/net/ethernet/renesas/ravb_main.c > > index 588e3be692d3..0fc9810c5e78 100644 > > --- a/drivers/net/ethernet/renesas/ravb_main.c > > +++ b/drivers/net/ethernet/renesas/ravb_main.c > > @@ -771,12 +771,15 @@ static bool ravb_rx_gbeth(struct net_device *ndev, > int *quota, int q) > > int limit; > > > > entry = priv->cur_rx[q] % priv->num_rx_ring[q]; > > + desc = &priv->gbeth_rx_ring[entry]; > > + if (desc->die_dt == DT_FEMPTY) > > + return false; > > + > > I don't understand what this buys us, the following *while* loop will > be skipped anyway, and the *for* loop as well, I think... And > ravb_rx_rcar() doesn't have that anyway... > > > @@ -1279,25 +1282,16 @@ static int ravb_poll(struct napi_struct *napi, > int budget) > > struct net_device *ndev = napi->dev; > > struct ravb_private *priv = netdev_priv(ndev); > > const struct ravb_hw_info *info = priv->info; > > - bool gptp = info->gptp || info->ccc_gac; > > - struct ravb_rx_desc *desc; > > unsigned long flags; > > int q = napi - priv->napi; > > int mask = BIT(q); > > int quota = budget; > > - unsigned int entry; > > > > - if (!gptp) { > > - entry = priv->cur_rx[q] % priv->num_rx_ring[q]; > > - desc = &priv->gbeth_rx_ring[entry]; > > - } > > /* Processing RX Descriptor Ring */ > > /* Clear RX interrupt */ > > ravb_write(ndev, ~(mask | RIS0_RESERVED), RIS0); > > - if (gptp || desc->die_dt != DT_FEMPTY) { > > - if (ravb_rx(ndev, "a, q)) > > - goto out; > > - } > > I don't really understand this code (despite I've AKCed it)... > Biju, could you explain this (well, you tried already but I don't buy it > anymore)? It was initial version of the driver. Now Claudiu optimized. Cheers, Biju > > > + if (ravb_rx(ndev, "a, q)) > > + goto out; > > This restores the behavior before: > > https://git.kern/ > el.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Ftorvalds%2Flinux.git%2Fcommit% > 2F%3Fid%3D3d4e37df882b0f4f28b7223a42492650b71252c5&data=05%7C01%7Cbiju.das > .jz%40bp.renesas.com%7C3c7d64ca68104738fb3f08dbec427e84%7C53d82571da1947e4 > 9cb4625a166a4a2a%7C0%7C0%7C638363542555838396%7CUnknown%7CTWFpbGZsb3d8eyJW > IjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7 > C%7C&sdata=lGBD8FdwY26OygYAV4Sd8bzIIO4rS7rNiYabQKaQAtA%3D&reserved=0 > > So does look correct. :-) > > MBR, Sergey
On 11/23/23 7:48 PM, Biju Das wrote: [...] >>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> >>> >>> ravb_poll() initial code used to interrogate the first descriptor of >>> the RX queue in case gptp is false to know if ravb_rx() should be >> called. >>> This is done for non GPTP IPs. For GPTP IPs the driver PTP specific >> >> It's called gPTP, AFAIK. >> >>> information was used to know if receive function should be called. As >>> every IP has it's own receive function that interrogates the RX >>> descriptor >> >> Its own. >> >>> list in the same way the ravb_poll() was doing there is no need to >>> double check this in ravb_poll(). Removing the code form ravb_poll() >>> and >> >> From ravb_poll(). >> >>> adjusting ravb_rx_gbeth() leads to a cleaner code. >>> >>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> >>> --- >>> drivers/net/ethernet/renesas/ravb_main.c | 18 ++++++------------ >>> 1 file changed, 6 insertions(+), 12 deletions(-) >>> >>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c >>> b/drivers/net/ethernet/renesas/ravb_main.c >>> index 588e3be692d3..0fc9810c5e78 100644 >>> --- a/drivers/net/ethernet/renesas/ravb_main.c >>> +++ b/drivers/net/ethernet/renesas/ravb_main.c >>> @@ -771,12 +771,15 @@ static bool ravb_rx_gbeth(struct net_device *ndev, >> int *quota, int q) >>> int limit; >>> >>> entry = priv->cur_rx[q] % priv->num_rx_ring[q]; >>> + desc = &priv->gbeth_rx_ring[entry]; >>> + if (desc->die_dt == DT_FEMPTY) >>> + return false; >>> + >> >> I don't understand what this buys us, the following *while* loop will >> be skipped anyway, and the *for* loop as well, I think... And >> ravb_rx_rcar() doesn't have that anyway... >> >>> @@ -1279,25 +1282,16 @@ static int ravb_poll(struct napi_struct *napi, >> int budget) >>> struct net_device *ndev = napi->dev; >>> struct ravb_private *priv = netdev_priv(ndev); >>> const struct ravb_hw_info *info = priv->info; >>> - bool gptp = info->gptp || info->ccc_gac; >>> - struct ravb_rx_desc *desc; >>> unsigned long flags; >>> int q = napi - priv->napi; >>> int mask = BIT(q); >>> int quota = budget; >>> - unsigned int entry; >>> >>> - if (!gptp) { >>> - entry = priv->cur_rx[q] % priv->num_rx_ring[q]; >>> - desc = &priv->gbeth_rx_ring[entry]; >>> - } >>> /* Processing RX Descriptor Ring */ >>> /* Clear RX interrupt */ >>> ravb_write(ndev, ~(mask | RIS0_RESERVED), RIS0); >>> - if (gptp || desc->die_dt != DT_FEMPTY) { >>> - if (ravb_rx(ndev, "a, q)) >>> - goto out; >>> - } >> >> I don't really understand this code (despite I've AKCed it)... >> Biju, could you explain this (well, you tried already but I don't buy it >> anymore)? > > It was initial version of the driver. Now Claudiu optimized. I fail to understand why you had (GBEth-specific) pre-conditions here to call ravb_rx()... and involving gPTP only further confused things... > Cheers, > Biju [...] MBR, Sergey
Hi Sergey Shtylyov, > Subject: Re: [PATCH 06/13] net: ravb: Let IP specific receive function to > interrogate descriptors > > On 11/23/23 7:48 PM, Biju Das wrote: > [...] > > >>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > >>> > >>> ravb_poll() initial code used to interrogate the first descriptor of > >>> the RX queue in case gptp is false to know if ravb_rx() should be > >> called. > >>> This is done for non GPTP IPs. For GPTP IPs the driver PTP specific > >> > >> It's called gPTP, AFAIK. > >> > >>> information was used to know if receive function should be called. > >>> As every IP has it's own receive function that interrogates the RX > >>> descriptor > >> > >> Its own. > >> > >>> list in the same way the ravb_poll() was doing there is no need to > >>> double check this in ravb_poll(). Removing the code form ravb_poll() > >>> and > >> > >> From ravb_poll(). > >> > >>> adjusting ravb_rx_gbeth() leads to a cleaner code. > >>> > >>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > >>> --- > >>> drivers/net/ethernet/renesas/ravb_main.c | 18 ++++++------------ > >>> 1 file changed, 6 insertions(+), 12 deletions(-) > >>> > >>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c > >>> b/drivers/net/ethernet/renesas/ravb_main.c > >>> index 588e3be692d3..0fc9810c5e78 100644 > >>> --- a/drivers/net/ethernet/renesas/ravb_main.c > >>> +++ b/drivers/net/ethernet/renesas/ravb_main.c > >>> @@ -771,12 +771,15 @@ static bool ravb_rx_gbeth(struct net_device > >>> *ndev, > >> int *quota, int q) > >>> int limit; > >>> > >>> entry = priv->cur_rx[q] % priv->num_rx_ring[q]; > >>> + desc = &priv->gbeth_rx_ring[entry]; > >>> + if (desc->die_dt == DT_FEMPTY) > >>> + return false; > >>> + > >> > >> I don't understand what this buys us, the following *while* loop > >> will be skipped anyway, and the *for* loop as well, I think... And > >> ravb_rx_rcar() doesn't have that anyway... > >> > >>> @@ -1279,25 +1282,16 @@ static int ravb_poll(struct napi_struct > >>> *napi, > >> int budget) > >>> struct net_device *ndev = napi->dev; > >>> struct ravb_private *priv = netdev_priv(ndev); > >>> const struct ravb_hw_info *info = priv->info; > >>> - bool gptp = info->gptp || info->ccc_gac; > >>> - struct ravb_rx_desc *desc; > >>> unsigned long flags; > >>> int q = napi - priv->napi; > >>> int mask = BIT(q); > >>> int quota = budget; > >>> - unsigned int entry; > >>> > >>> - if (!gptp) { > >>> - entry = priv->cur_rx[q] % priv->num_rx_ring[q]; > >>> - desc = &priv->gbeth_rx_ring[entry]; > >>> - } > >>> /* Processing RX Descriptor Ring */ > >>> /* Clear RX interrupt */ > >>> ravb_write(ndev, ~(mask | RIS0_RESERVED), RIS0); > >>> - if (gptp || desc->die_dt != DT_FEMPTY) { > >>> - if (ravb_rx(ndev, "a, q)) > >>> - goto out; > >>> - } > >> > >> I don't really understand this code (despite I've AKCed it)... > >> Biju, could you explain this (well, you tried already but I don't buy > >> it anymore)? > > > > It was initial version of the driver. Now Claudiu optimized. > > I fail to understand why you had (GBEth-specific) pre-conditions here > to call ravb_rx()... and involving gPTP only further confused things... Initial driver is based on a reference code/bsp driver and that code has this preconditions. Maybe they have faced some race condition in rx path involving ring buffer/descriptor. But so far we are not able to reproduce any race condition here. So it is safe to remove now. Cheers, Biju
On 23.11.2023 18:37, Sergey Shtylyov wrote: > On 11/20/23 11:45 AM, Claudiu wrote: > >> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> >> >> ravb_poll() initial code used to interrogate the first descriptor of the >> RX queue in case gptp is false to know if ravb_rx() should be called. >> This is done for non GPTP IPs. For GPTP IPs the driver PTP specific > > It's called gPTP, AFAIK. > >> information was used to know if receive function should be called. As >> every IP has it's own receive function that interrogates the RX descriptor > > Its own. > >> list in the same way the ravb_poll() was doing there is no need to double >> check this in ravb_poll(). Removing the code form ravb_poll() and > > From ravb_poll(). > >> adjusting ravb_rx_gbeth() leads to a cleaner code. >> >> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> >> --- >> drivers/net/ethernet/renesas/ravb_main.c | 18 ++++++------------ >> 1 file changed, 6 insertions(+), 12 deletions(-) >> >> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c >> index 588e3be692d3..0fc9810c5e78 100644 >> --- a/drivers/net/ethernet/renesas/ravb_main.c >> +++ b/drivers/net/ethernet/renesas/ravb_main.c >> @@ -771,12 +771,15 @@ static bool ravb_rx_gbeth(struct net_device *ndev, int *quota, int q) >> int limit; >> >> entry = priv->cur_rx[q] % priv->num_rx_ring[q]; >> + desc = &priv->gbeth_rx_ring[entry]; >> + if (desc->die_dt == DT_FEMPTY) >> + return false; >> + > > I don't understand what this buys us, the following *while* loop will > be skipped anyway, and the *for* loop as well, I think... And ravb_rx_rcar() Yes, I kept it because of the for loop and the update of the *quota. As commit specifies the code has been moved in IP specific RX function keeping the initial functionality. > doesn't have that anyway... > >> @@ -1279,25 +1282,16 @@ static int ravb_poll(struct napi_struct *napi, int budget) >> struct net_device *ndev = napi->dev; >> struct ravb_private *priv = netdev_priv(ndev); >> const struct ravb_hw_info *info = priv->info; >> - bool gptp = info->gptp || info->ccc_gac; >> - struct ravb_rx_desc *desc; >> unsigned long flags; >> int q = napi - priv->napi; >> int mask = BIT(q); >> int quota = budget; >> - unsigned int entry; >> >> - if (!gptp) { >> - entry = priv->cur_rx[q] % priv->num_rx_ring[q]; >> - desc = &priv->gbeth_rx_ring[entry]; >> - } >> /* Processing RX Descriptor Ring */ >> /* Clear RX interrupt */ >> ravb_write(ndev, ~(mask | RIS0_RESERVED), RIS0); >> - if (gptp || desc->die_dt != DT_FEMPTY) { >> - if (ravb_rx(ndev, "a, q)) >> - goto out; >> - } > > I don't really understand this code (despite I've AKCed it)... > Biju, could you explain this (well, you tried already but I don't > buy it anymore)? > >> + if (ravb_rx(ndev, "a, q)) >> + goto out; > > This restores the behavior before: > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=3d4e37df882b0f4f28b7223a42492650b71252c5 > > So does look correct. :-) > > MBR, Sergey
On 11/23/23 8:15 PM, claudiu beznea wrote: [...] >>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> >>> >>> ravb_poll() initial code used to interrogate the first descriptor of the >>> RX queue in case gptp is false to know if ravb_rx() should be called. >>> This is done for non GPTP IPs. For GPTP IPs the driver PTP specific >> >> It's called gPTP, AFAIK. >> >>> information was used to know if receive function should be called. As >>> every IP has it's own receive function that interrogates the RX descriptor >> >> Its own. >> >>> list in the same way the ravb_poll() was doing there is no need to double >>> check this in ravb_poll(). Removing the code form ravb_poll() and >> >> From ravb_poll(). >> >>> adjusting ravb_rx_gbeth() leads to a cleaner code. >>> >>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> >>> --- >>> drivers/net/ethernet/renesas/ravb_main.c | 18 ++++++------------ >>> 1 file changed, 6 insertions(+), 12 deletions(-) >>> >>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c >>> index 588e3be692d3..0fc9810c5e78 100644 >>> --- a/drivers/net/ethernet/renesas/ravb_main.c >>> +++ b/drivers/net/ethernet/renesas/ravb_main.c >>> @@ -771,12 +771,15 @@ static bool ravb_rx_gbeth(struct net_device *ndev, int *quota, int q) >>> int limit; >>> >>> entry = priv->cur_rx[q] % priv->num_rx_ring[q]; >>> + desc = &priv->gbeth_rx_ring[entry]; >>> + if (desc->die_dt == DT_FEMPTY) >>> + return false; >>> + >> >> I don't understand what this buys us, the following *while* loop will >> be skipped anyway, and the *for* loop as well, I think... And ravb_rx_rcar() > > Yes, I kept it because of the for loop and the update of the *quota. > > As commit specifies the code has been moved in IP specific RX function > keeping the initial functionality. Please pull this perplexed code out completely instead. It's not needed according to Biju. >> doesn't have that anyway... [...] MBR, Sergey
diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c index 588e3be692d3..0fc9810c5e78 100644 --- a/drivers/net/ethernet/renesas/ravb_main.c +++ b/drivers/net/ethernet/renesas/ravb_main.c @@ -771,12 +771,15 @@ static bool ravb_rx_gbeth(struct net_device *ndev, int *quota, int q) int limit; entry = priv->cur_rx[q] % priv->num_rx_ring[q]; + desc = &priv->gbeth_rx_ring[entry]; + if (desc->die_dt == DT_FEMPTY) + return false; + boguscnt = priv->dirty_rx[q] + priv->num_rx_ring[q] - priv->cur_rx[q]; stats = &priv->stats[q]; boguscnt = min(boguscnt, *quota); limit = boguscnt; - desc = &priv->gbeth_rx_ring[entry]; while (desc->die_dt != DT_FEMPTY) { /* Descriptor type must be checked before all other reads */ dma_rmb(); @@ -1279,25 +1282,16 @@ static int ravb_poll(struct napi_struct *napi, int budget) struct net_device *ndev = napi->dev; struct ravb_private *priv = netdev_priv(ndev); const struct ravb_hw_info *info = priv->info; - bool gptp = info->gptp || info->ccc_gac; - struct ravb_rx_desc *desc; unsigned long flags; int q = napi - priv->napi; int mask = BIT(q); int quota = budget; - unsigned int entry; - if (!gptp) { - entry = priv->cur_rx[q] % priv->num_rx_ring[q]; - desc = &priv->gbeth_rx_ring[entry]; - } /* Processing RX Descriptor Ring */ /* Clear RX interrupt */ ravb_write(ndev, ~(mask | RIS0_RESERVED), RIS0); - if (gptp || desc->die_dt != DT_FEMPTY) { - if (ravb_rx(ndev, "a, q)) - goto out; - } + if (ravb_rx(ndev, "a, q)) + goto out; /* Processing TX Descriptor Ring */ spin_lock_irqsave(&priv->lock, flags);