* [PATCH] net: fec: fix the error to get the previous BD entry
@ 2013-08-30 4:35 Fugang Duan
2013-08-30 21:59 ` David Miller
0 siblings, 1 reply; 21+ messages in thread
From: Fugang Duan @ 2013-08-30 4:35 UTC (permalink / raw)
To: davem, b20596; +Cc: netdev, b38611, bhutchings, l.stach
When the current BD is the first BD, the previous BD entry
must be the last BD, not "bdp - 1".
Add BD entry check to get the previous BD entry correctly.
Signed-off-by: Fugang Duan <B38611@freescale.com>
---
drivers/net/ethernet/freescale/fec_main.c | 10 +++++++++-
1 files changed, 9 insertions(+), 1 deletions(-)
diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index 4ea1555..b13746f 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -380,7 +380,15 @@ fec_enet_start_xmit(struct sk_buff *skb, struct net_device *ndev)
}
}
- bdp_pre = fec_enet_get_prevdesc(bdp, fep->bufdesc_ex);
+ if (bdp == fep->tx_bd_base) {
+ if (fep->bufdesc_ex)
+ bdp_pre = (struct bufdesc *)((void *)bdp +
+ (sizeof(struct bufdesc_ex) * (TX_RING_SIZE - 1)));
+ else
+ bdp_pre = bdp + TX_RING_SIZE - 1;
+ } else {
+ bdp_pre = fec_enet_get_prevdesc(bdp, fep->bufdesc_ex);
+ }
if ((id_entry->driver_data & FEC_QUIRK_ERR006358) &&
!(bdp_pre->cbd_sc & BD_ENET_TX_READY)) {
fep->delay_work.trig_tx = true;
--
1.7.1
^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH] net: fec: fix the error to get the previous BD entry
2013-08-30 4:35 [PATCH] net: fec: fix the error to get the previous BD entry Fugang Duan
@ 2013-08-30 21:59 ` David Miller
2013-09-02 1:37 ` Duan Fugang-B38611
0 siblings, 1 reply; 21+ messages in thread
From: David Miller @ 2013-08-30 21:59 UTC (permalink / raw)
To: B38611; +Cc: b20596, netdev, bhutchings, l.stach
From: Fugang Duan <B38611@freescale.com>
Date: Fri, 30 Aug 2013 12:35:25 +0800
> When the current BD is the first BD, the previous BD entry
> must be the last BD, not "bdp - 1".
>
> Add BD entry check to get the previous BD entry correctly.
>
> Signed-off-by: Fugang Duan <B38611@freescale.com>
The correct fix is to put this logic into fec_enet_get_prevdesc().
With this current limitation, the function is completely useless.
^ permalink raw reply [flat|nested] 21+ messages in thread
* RE: [PATCH] net: fec: fix the error to get the previous BD entry
2013-08-30 21:59 ` David Miller
@ 2013-09-02 1:37 ` Duan Fugang-B38611
2013-09-02 2:09 ` Li Frank-B20596
0 siblings, 1 reply; 21+ messages in thread
From: Duan Fugang-B38611 @ 2013-09-02 1:37 UTC (permalink / raw)
To: David Miller; +Cc: Li Frank-B20596, netdev, bhutchings, l.stach
From: David Miller <davem@davemloft.net>
Data: Saturday, August 31, 2013 5:59 AM
> To: Duan Fugang-B38611
> Cc: Li Frank-B20596; netdev@vger.kernel.org; bhutchings@solarflare.com;
> l.stach@pengutronix.de
> Subject: Re: [PATCH] net: fec: fix the error to get the previous BD entry
>
> From: Fugang Duan <B38611@freescale.com>
> Date: Fri, 30 Aug 2013 12:35:25 +0800
>
> > When the current BD is the first BD, the previous BD entry must be the
> > last BD, not "bdp - 1".
> >
> > Add BD entry check to get the previous BD entry correctly.
> >
> > Signed-off-by: Fugang Duan <B38611@freescale.com>
>
> The correct fix is to put this logic into fec_enet_get_prevdesc().
> With this current limitation, the function is completely useless.
Due to get tx & rx previous description use the same function fec_enet_get_prevdesc(). If put the logic into fec_enet_get_prevdesc(),
I think it is better to split the function to fec_enet_get_tx_prevdesc(),fec_enet_get_rx_prevdesc().
If you agree it, I will submit one patch to do it.
Thanks,
Andy
^ permalink raw reply [flat|nested] 21+ messages in thread
* RE: [PATCH] net: fec: fix the error to get the previous BD entry
2013-09-02 1:37 ` Duan Fugang-B38611
@ 2013-09-02 2:09 ` Li Frank-B20596
2013-09-02 2:23 ` Duan Fugang-B38611
0 siblings, 1 reply; 21+ messages in thread
From: Li Frank-B20596 @ 2013-09-02 2:09 UTC (permalink / raw)
To: Duan Fugang-B38611, David Miller; +Cc: netdev, bhutchings, l.stach
>
> Due to get tx & rx previous description use the same function
> fec_enet_get_prevdesc(). If put the logic into fec_enet_get_prevdesc(), I
> think it is better to split the function to
> fec_enet_get_tx_prevdesc(),fec_enet_get_rx_prevdesc().
>
> If you agree it, I will submit one patch to do it.
It doesn't looks good to split to two functions.
Can we use if (p == fep->tx_base || p == fep->rx_base) to check if get first entry of queue?
Best regards
Frank Li
>
> Thanks,
> Andy
^ permalink raw reply [flat|nested] 21+ messages in thread
* RE: [PATCH] net: fec: fix the error to get the previous BD entry
2013-09-02 2:09 ` Li Frank-B20596
@ 2013-09-02 2:23 ` Duan Fugang-B38611
2013-09-02 2:25 ` Li Frank-B20596
0 siblings, 1 reply; 21+ messages in thread
From: Duan Fugang-B38611 @ 2013-09-02 2:23 UTC (permalink / raw)
To: Li Frank-B20596, David Miller; +Cc: netdev, bhutchings, l.stach
From: Li Frank-B20596
Data: Monday, September 02, 2013 10:10 AM + 800
> To: Duan Fugang-B38611; David Miller
> Cc: netdev@vger.kernel.org; bhutchings@solarflare.com;
> l.stach@pengutronix.de
> Subject: RE: [PATCH] net: fec: fix the error to get the previous BD entry
>
> >
> > Due to get tx & rx previous description use the same function
> > fec_enet_get_prevdesc(). If put the logic into
> > fec_enet_get_prevdesc(), I think it is better to split the function to
> > fec_enet_get_tx_prevdesc(),fec_enet_get_rx_prevdesc().
> >
> > If you agree it, I will submit one patch to do it.
>
> It doesn't looks good to split to two functions.
> Can we use if (p == fep->tx_base || p == fep->rx_base) to check if get
> first entry of queue?
Yes, this check can get the first entry of queue, but the ring size is different for rx and tx ring.
In current driver, there have no tx/rx ring size variable define in struct fec_enet_private.
And, if change fec_enet_get_prevdesc(), the fec_enet_get_nextdesc() may need to do related logic change for nicety code.
> Best regards
> Frank Li
^ permalink raw reply [flat|nested] 21+ messages in thread
* RE: [PATCH] net: fec: fix the error to get the previous BD entry
2013-09-02 2:23 ` Duan Fugang-B38611
@ 2013-09-02 2:25 ` Li Frank-B20596
0 siblings, 0 replies; 21+ messages in thread
From: Li Frank-B20596 @ 2013-09-02 2:25 UTC (permalink / raw)
To: Duan Fugang-B38611, David Miller; +Cc: netdev, bhutchings, l.stach
> > > Due to get tx & rx previous description use the same function
> > > fec_enet_get_prevdesc(). If put the logic into
> > > fec_enet_get_prevdesc(), I think it is better to split the function
> > > to fec_enet_get_tx_prevdesc(),fec_enet_get_rx_prevdesc().
> > >
> > > If you agree it, I will submit one patch to do it.
> >
> > It doesn't looks good to split to two functions.
> > Can we use if (p == fep->tx_base || p == fep->rx_base) to check if get
> > first entry of queue?
>
> Yes, this check can get the first entry of queue, but the ring size is
> different for rx and tx ring.
> In current driver, there have no tx/rx ring size variable define in struct
> fec_enet_private.
>
> And, if change fec_enet_get_prevdesc(), the fec_enet_get_nextdesc() may need
> to do related logic change for nicety code.
Yes, it should be better handle this logical in get_prevdesc and get_nextdesc.
So we have unified place to handle wrap.
>
> > Best regards
> > Frank Li
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] net: fec: fix the error to get the previous BD entry
2013-09-03 2:41 Fugang Duan
2013-09-03 3:00 ` Li Frank-B20596
@ 2013-09-04 18:13 ` David Miller
1 sibling, 0 replies; 21+ messages in thread
From: David Miller @ 2013-09-04 18:13 UTC (permalink / raw)
To: B38611; +Cc: b20596, netdev, bhutchings, stephen
From: Fugang Duan <B38611@freescale.com>
Date: Tue, 3 Sep 2013 10:41:18 +0800
> Bug: error to get the previous BD entry. When the current BD
> is the first BD, the previous BD entry must be the last BD,
> not "bdp - 1" in current logic.
...
> Reviewed-by: Li Frank <B20596@freescale.com>
> Signed-off-by: Fugang Duan <B38611@freescale.com>
Applied.
^ permalink raw reply [flat|nested] 21+ messages in thread
* RE: [PATCH] net: fec: fix the error to get the previous BD entry
2013-09-03 15:14 ` Fabio Estevam
@ 2013-09-03 15:20 ` Li Frank-B20596
0 siblings, 0 replies; 21+ messages in thread
From: Li Frank-B20596 @ 2013-09-03 15:20 UTC (permalink / raw)
To: Fabio Estevam
Cc: David Miller, Duan Fugang-B38611, netdev, bhutchings, stephen
> -----Original Message-----
> From: Fabio Estevam [mailto:festevam@gmail.com]
> Sent: Tuesday, September 03, 2013 10:15 AM
> To: Li Frank-B20596
> Cc: David Miller; Duan Fugang-B38611; netdev@vger.kernel.org;
> bhutchings@solarflare.com; stephen@networkplumber.org
> Subject: Re: [PATCH] net: fec: fix the error to get the previous BD entry
>
> On Tue, Sep 3, 2013 at 12:10 PM, Li Frank-B20596 <B20596@freescale.com>
> wrote:
> >
> >> > Acked
> >>
> >> This is not the correct way to ACK a patch, the correct format is:
> >>
> >> Acked-by: David S. Miller <davem@davemloft.net>
> >
> >
> > Thanks
> >
> > Acked-by: Frank Li <frank.li@freescale.net>
>
> I thought your email was freescale.com ;-)
Yes, I am wrong.
Sorry.
Acked-by: Frank Li <frank.li@freescale.com>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] net: fec: fix the error to get the previous BD entry
2013-09-03 15:10 ` Li Frank-B20596
@ 2013-09-03 15:14 ` Fabio Estevam
2013-09-03 15:20 ` Li Frank-B20596
0 siblings, 1 reply; 21+ messages in thread
From: Fabio Estevam @ 2013-09-03 15:14 UTC (permalink / raw)
To: Li Frank-B20596
Cc: David Miller, Duan Fugang-B38611, netdev, bhutchings, stephen
On Tue, Sep 3, 2013 at 12:10 PM, Li Frank-B20596 <B20596@freescale.com> wrote:
>
>> > Acked
>>
>> This is not the correct way to ACK a patch, the correct format is:
>>
>> Acked-by: David S. Miller <davem@davemloft.net>
>
>
> Thanks
>
> Acked-by: Frank Li <frank.li@freescale.net>
I thought your email was freescale.com ;-)
^ permalink raw reply [flat|nested] 21+ messages in thread
* RE: [PATCH] net: fec: fix the error to get the previous BD entry
2013-09-03 3:35 ` David Miller
@ 2013-09-03 15:10 ` Li Frank-B20596
2013-09-03 15:14 ` Fabio Estevam
0 siblings, 1 reply; 21+ messages in thread
From: Li Frank-B20596 @ 2013-09-03 15:10 UTC (permalink / raw)
To: David Miller; +Cc: Duan Fugang-B38611, netdev, bhutchings, stephen
> > Acked
>
> This is not the correct way to ACK a patch, the correct format is:
>
> Acked-by: David S. Miller <davem@davemloft.net>
Thanks
Acked-by: Frank Li <frank.li@freescale.net>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] net: fec: fix the error to get the previous BD entry
2013-09-03 3:00 ` Li Frank-B20596
@ 2013-09-03 3:35 ` David Miller
2013-09-03 15:10 ` Li Frank-B20596
0 siblings, 1 reply; 21+ messages in thread
From: David Miller @ 2013-09-03 3:35 UTC (permalink / raw)
To: B20596; +Cc: B38611, netdev, bhutchings, stephen
From: Li Frank-B20596 <B20596@freescale.com>
Date: Tue, 3 Sep 2013 03:00:48 +0000
> Acked
This is not the correct way to ACK a patch, the correct format is:
Acked-by: David S. Miller <davem@davemloft.net>
^ permalink raw reply [flat|nested] 21+ messages in thread
* RE: [PATCH] net: fec: fix the error to get the previous BD entry
2013-09-03 2:41 Fugang Duan
@ 2013-09-03 3:00 ` Li Frank-B20596
2013-09-03 3:35 ` David Miller
2013-09-04 18:13 ` David Miller
1 sibling, 1 reply; 21+ messages in thread
From: Li Frank-B20596 @ 2013-09-03 3:00 UTC (permalink / raw)
To: Duan Fugang-B38611, davem; +Cc: netdev, bhutchings, stephen
>
> Bug: error to get the previous BD entry. When the current BD is the first BD,
> the previous BD entry must be the last BD, not "bdp - 1" in current logic.
>
> V4:
> * Optimize fec_enet_get_nextdesc() for code clean.
> Replace "ex_new_bd - ring_size" with "ex_base".
> Replace "new_bd - ring_size" with "base".
>
> V3:
> * Restore the API name because David suggest to use fec_enet_
> prefix for all function in fec driver.
> So, change next_bd() -> fec_enet_get_nextdesc()
> change pre_bd() -> fec_enet_get_prevdesc()
> * Reduce the two APIs parameters for easy to call.
>
> V2:
> * Add tx_ring_size and rx_ring_size to struct fec_enet_private.
> * Replace api fec_enet_get_nextdesc() with next_bd().
> Replace api fec_enet_get_prevdesc() with pre_bd().
>
> * Move all ring size check logic to next_bd() and pre_bd(), which
> simplifies the code redundancy.
>
> V1:
> * Add BD ring size check to get the previous BD entry in correctly.
>
> Reviewed-by: Li Frank <B20596@freescale.com>
> Signed-off-by: Fugang Duan <B38611@freescale.com>
Acked
> ---
> drivers/net/ethernet/freescale/fec.h | 3 +
> drivers/net/ethernet/freescale/fec_main.c | 120 ++++++++++++++++++---------
> --
> 2 files changed, 77 insertions(+), 46 deletions(-)
>
> diff --git a/drivers/net/ethernet/freescale/fec.h
> b/drivers/net/ethernet/freescale/fec.h
> index ae23600..0120217 100644
> --- a/drivers/net/ethernet/freescale/fec.h
> +++ b/drivers/net/ethernet/freescale/fec.h
> @@ -296,6 +296,9 @@ struct fec_enet_private {
> /* The ring entries to be free()ed */
> struct bufdesc *dirty_tx;
>
> + unsigned short tx_ring_size;
> + unsigned short rx_ring_size;
> +
> struct platform_device *pdev;
>
> int opened;
> diff --git a/drivers/net/ethernet/freescale/fec_main.c
> b/drivers/net/ethernet/freescale/fec_main.c
> index 4ea1555..1364a6f 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -239,22 +239,57 @@ MODULE_PARM_DESC(macaddr, "FEC Ethernet MAC address");
>
> static int mii_cnt;
>
> -static struct bufdesc *fec_enet_get_nextdesc(struct bufdesc *bdp, int is_ex)
> +static inline
> +struct bufdesc *fec_enet_get_nextdesc(struct bufdesc *bdp, struct
> +fec_enet_private *fep)
> {
> - struct bufdesc_ex *ex = (struct bufdesc_ex *)bdp;
> - if (is_ex)
> - return (struct bufdesc *)(ex + 1);
> + struct bufdesc *new_bd = bdp + 1;
> + struct bufdesc_ex *ex_new_bd = (struct bufdesc_ex *)bdp + 1;
> + struct bufdesc_ex *ex_base;
> + struct bufdesc *base;
> + int ring_size;
> +
> + if (bdp >= fep->tx_bd_base) {
> + base = fep->tx_bd_base;
> + ring_size = fep->tx_ring_size;
> + ex_base = (struct bufdesc_ex *)fep->tx_bd_base;
> + } else {
> + base = fep->rx_bd_base;
> + ring_size = fep->rx_ring_size;
> + ex_base = (struct bufdesc_ex *)fep->rx_bd_base;
> + }
> +
> + if (fep->bufdesc_ex)
> + return (struct bufdesc *)((ex_new_bd >= (ex_base + ring_size)) ?
> + ex_base : ex_new_bd);
> else
> - return bdp + 1;
> + return (new_bd >= (base + ring_size)) ?
> + base : new_bd;
> }
>
> -static struct bufdesc *fec_enet_get_prevdesc(struct bufdesc *bdp, int is_ex)
> +static inline
> +struct bufdesc *fec_enet_get_prevdesc(struct bufdesc *bdp, struct
> +fec_enet_private *fep)
> {
> - struct bufdesc_ex *ex = (struct bufdesc_ex *)bdp;
> - if (is_ex)
> - return (struct bufdesc *)(ex - 1);
> + struct bufdesc *new_bd = bdp - 1;
> + struct bufdesc_ex *ex_new_bd = (struct bufdesc_ex *)bdp - 1;
> + struct bufdesc_ex *ex_base;
> + struct bufdesc *base;
> + int ring_size;
> +
> + if (bdp >= fep->tx_bd_base) {
> + base = fep->tx_bd_base;
> + ring_size = fep->tx_ring_size;
> + ex_base = (struct bufdesc_ex *)fep->tx_bd_base;
> + } else {
> + base = fep->rx_bd_base;
> + ring_size = fep->rx_ring_size;
> + ex_base = (struct bufdesc_ex *)fep->rx_bd_base;
> + }
> +
> + if (fep->bufdesc_ex)
> + return (struct bufdesc *)((ex_new_bd < ex_base) ?
> + (ex_new_bd + ring_size) : ex_new_bd);
> else
> - return bdp - 1;
> + return (new_bd < base) ? (new_bd + ring_size) : new_bd;
> }
>
> static void *swap_buffer(void *bufaddr, int len) @@ -380,7 +415,7 @@
> fec_enet_start_xmit(struct sk_buff *skb, struct net_device *ndev)
> }
> }
>
> - bdp_pre = fec_enet_get_prevdesc(bdp, fep->bufdesc_ex);
> + bdp_pre = fec_enet_get_prevdesc(bdp, fep);
> if ((id_entry->driver_data & FEC_QUIRK_ERR006358) &&
> !(bdp_pre->cbd_sc & BD_ENET_TX_READY)) {
> fep->delay_work.trig_tx = true;
> @@ -389,10 +424,7 @@ fec_enet_start_xmit(struct sk_buff *skb, struct
> net_device *ndev)
> }
>
> /* If this was the last BD in the ring, start at the beginning again.
> */
> - if (status & BD_ENET_TX_WRAP)
> - bdp = fep->tx_bd_base;
> - else
> - bdp = fec_enet_get_nextdesc(bdp, fep->bufdesc_ex);
> + bdp = fec_enet_get_nextdesc(bdp, fep);
>
> fep->cur_tx = bdp;
>
> @@ -417,18 +449,18 @@ static void fec_enet_bd_init(struct net_device *dev)
>
> /* Initialize the receive buffer descriptors. */
> bdp = fep->rx_bd_base;
> - for (i = 0; i < RX_RING_SIZE; i++) {
> + for (i = 0; i < fep->rx_ring_size; i++) {
>
> /* Initialize the BD for every fragment in the page. */
> if (bdp->cbd_bufaddr)
> bdp->cbd_sc = BD_ENET_RX_EMPTY;
> else
> bdp->cbd_sc = 0;
> - bdp = fec_enet_get_nextdesc(bdp, fep->bufdesc_ex);
> + bdp = fec_enet_get_nextdesc(bdp, fep);
> }
>
> /* Set the last buffer to wrap */
> - bdp = fec_enet_get_prevdesc(bdp, fep->bufdesc_ex);
> + bdp = fec_enet_get_prevdesc(bdp, fep);
> bdp->cbd_sc |= BD_SC_WRAP;
>
> fep->cur_rx = fep->rx_bd_base;
> @@ -436,7 +468,7 @@ static void fec_enet_bd_init(struct net_device *dev)
> /* ...and the same for transmit */
> bdp = fep->tx_bd_base;
> fep->cur_tx = bdp;
> - for (i = 0; i < TX_RING_SIZE; i++) {
> + for (i = 0; i < fep->tx_ring_size; i++) {
>
> /* Initialize the BD for every fragment in the page. */
> bdp->cbd_sc = 0;
> @@ -445,11 +477,11 @@ static void fec_enet_bd_init(struct net_device *dev)
> fep->tx_skbuff[i] = NULL;
> }
> bdp->cbd_bufaddr = 0;
> - bdp = fec_enet_get_nextdesc(bdp, fep->bufdesc_ex);
> + bdp = fec_enet_get_nextdesc(bdp, fep);
> }
>
> /* Set the last buffer to wrap */
> - bdp = fec_enet_get_prevdesc(bdp, fep->bufdesc_ex);
> + bdp = fec_enet_get_prevdesc(bdp, fep);
> bdp->cbd_sc |= BD_SC_WRAP;
> fep->dirty_tx = bdp;
> }
> @@ -510,10 +542,10 @@ fec_restart(struct net_device *ndev, int duplex)
> writel(fep->bd_dma, fep->hwp + FEC_R_DES_START);
> if (fep->bufdesc_ex)
> writel((unsigned long)fep->bd_dma + sizeof(struct bufdesc_ex)
> - * RX_RING_SIZE, fep->hwp + FEC_X_DES_START);
> + * fep->rx_ring_size, fep->hwp + FEC_X_DES_START);
> else
> writel((unsigned long)fep->bd_dma + sizeof(struct bufdesc)
> - * RX_RING_SIZE, fep->hwp + FEC_X_DES_START);
> + * fep->rx_ring_size, fep->hwp + FEC_X_DES_START);
>
>
> for (i = 0; i <= TX_RING_MOD_MASK; i++) { @@ -727,10 +759,7 @@
> fec_enet_tx(struct net_device *ndev)
> bdp = fep->dirty_tx;
>
> /* get next bdp of dirty_tx */
> - if (bdp->cbd_sc & BD_ENET_TX_WRAP)
> - bdp = fep->tx_bd_base;
> - else
> - bdp = fec_enet_get_nextdesc(bdp, fep->bufdesc_ex);
> + bdp = fec_enet_get_nextdesc(bdp, fep);
>
> while (((status = bdp->cbd_sc) & BD_ENET_TX_READY) == 0) {
>
> @@ -800,10 +829,7 @@ fec_enet_tx(struct net_device *ndev)
> fep->dirty_tx = bdp;
>
> /* Update pointer to next buffer descriptor to be transmitted */
> - if (status & BD_ENET_TX_WRAP)
> - bdp = fep->tx_bd_base;
> - else
> - bdp = fec_enet_get_nextdesc(bdp, fep->bufdesc_ex);
> + bdp = fec_enet_get_nextdesc(bdp, fep);
>
> /* Since we have freed up a buffer, the ring is no longer full
> */
> @@ -994,10 +1020,8 @@ rx_processing_done:
> }
>
> /* Update BD pointer to next entry */
> - if (status & BD_ENET_RX_WRAP)
> - bdp = fep->rx_bd_base;
> - else
> - bdp = fec_enet_get_nextdesc(bdp, fep->bufdesc_ex);
> + bdp = fec_enet_get_nextdesc(bdp, fep);
> +
> /* Doing this here will keep the FEC running while we process
> * incoming frames. On a heavily loaded network, we should be
> * able to keep up at the expense of system resources.
> @@ -1663,7 +1687,7 @@ static void fec_enet_free_buffers(struct net_device
> *ndev)
> struct bufdesc *bdp;
>
> bdp = fep->rx_bd_base;
> - for (i = 0; i < RX_RING_SIZE; i++) {
> + for (i = 0; i < fep->rx_ring_size; i++) {
> skb = fep->rx_skbuff[i];
>
> if (bdp->cbd_bufaddr)
> @@ -1671,11 +1695,11 @@ static void fec_enet_free_buffers(struct net_device
> *ndev)
> FEC_ENET_RX_FRSIZE, DMA_FROM_DEVICE);
> if (skb)
> dev_kfree_skb(skb);
> - bdp = fec_enet_get_nextdesc(bdp, fep->bufdesc_ex);
> + bdp = fec_enet_get_nextdesc(bdp, fep);
> }
>
> bdp = fep->tx_bd_base;
> - for (i = 0; i < TX_RING_SIZE; i++)
> + for (i = 0; i < fep->tx_ring_size; i++)
> kfree(fep->tx_bounce[i]);
> }
>
> @@ -1687,7 +1711,7 @@ static int fec_enet_alloc_buffers(struct net_device
> *ndev)
> struct bufdesc *bdp;
>
> bdp = fep->rx_bd_base;
> - for (i = 0; i < RX_RING_SIZE; i++) {
> + for (i = 0; i < fep->rx_ring_size; i++) {
> skb = netdev_alloc_skb(ndev, FEC_ENET_RX_FRSIZE);
> if (!skb) {
> fec_enet_free_buffers(ndev);
> @@ -1704,15 +1728,15 @@ static int fec_enet_alloc_buffers(struct net_device
> *ndev)
> ebdp->cbd_esc = BD_ENET_RX_INT;
> }
>
> - bdp = fec_enet_get_nextdesc(bdp, fep->bufdesc_ex);
> + bdp = fec_enet_get_nextdesc(bdp, fep);
> }
>
> /* Set the last buffer to wrap. */
> - bdp = fec_enet_get_prevdesc(bdp, fep->bufdesc_ex);
> + bdp = fec_enet_get_prevdesc(bdp, fep);
> bdp->cbd_sc |= BD_SC_WRAP;
>
> bdp = fep->tx_bd_base;
> - for (i = 0; i < TX_RING_SIZE; i++) {
> + for (i = 0; i < fep->tx_ring_size; i++) {
> fep->tx_bounce[i] = kmalloc(FEC_ENET_TX_FRSIZE, GFP_KERNEL);
>
> bdp->cbd_sc = 0;
> @@ -1723,11 +1747,11 @@ static int fec_enet_alloc_buffers(struct net_device
> *ndev)
> ebdp->cbd_esc = BD_ENET_TX_INT;
> }
>
> - bdp = fec_enet_get_nextdesc(bdp, fep->bufdesc_ex);
> + bdp = fec_enet_get_nextdesc(bdp, fep);
> }
>
> /* Set the last buffer to wrap. */
> - bdp = fec_enet_get_prevdesc(bdp, fep->bufdesc_ex);
> + bdp = fec_enet_get_prevdesc(bdp, fep);
> bdp->cbd_sc |= BD_SC_WRAP;
>
> return 0;
> @@ -1967,13 +1991,17 @@ static int fec_enet_init(struct net_device *ndev)
> /* Get the Ethernet address */
> fec_get_mac(ndev);
>
> + /* init the tx & rx ring size */
> + fep->tx_ring_size = TX_RING_SIZE;
> + fep->rx_ring_size = RX_RING_SIZE;
> +
> /* Set receive and transmit descriptor base. */
> fep->rx_bd_base = cbd_base;
> if (fep->bufdesc_ex)
> fep->tx_bd_base = (struct bufdesc *)
> - (((struct bufdesc_ex *)cbd_base) + RX_RING_SIZE);
> + (((struct bufdesc_ex *)cbd_base) + fep->rx_ring_size);
> else
> - fep->tx_bd_base = cbd_base + RX_RING_SIZE;
> + fep->tx_bd_base = cbd_base + fep->rx_ring_size;
>
> /* The FEC Ethernet specific entries in the device structure */
> ndev->watchdog_timeo = TX_TIMEOUT;
> --
> 1.7.1
^ permalink raw reply [flat|nested] 21+ messages in thread
* RE: [PATCH] net: fec: fix the error to get the previous BD entry
2013-09-03 2:33 ` Li Frank-B20596
@ 2013-09-03 2:49 ` Duan Fugang-B38611
0 siblings, 0 replies; 21+ messages in thread
From: Duan Fugang-B38611 @ 2013-09-03 2:49 UTC (permalink / raw)
To: Li Frank-B20596, davem; +Cc: netdev, bhutchings, stephen
From: Li Frank-B20596
Data: Tuesday, September 03, 2013 10:34 AM
> To: Duan Fugang-B38611; davem@davemloft.net
> Cc: netdev@vger.kernel.org; bhutchings@solarflare.com;
> stephen@networkplumber.org
> Subject: RE: [PATCH] net: fec: fix the error to get the previous BD entry
>
>
> > Bug: error to get the previous BD entry. When the current BD is the
> > first BD, the previous BD entry must be the last BD, not "bdp - 1" in
> current logic.
> >
> > V3:
> > * Restore the API name because David suggest to use fec_enet_
> > prefix for all function in fec driver.
> > So, change next_bd() -> fec_enet_get_nextdesc()
> > change pre_bd() -> fec_enet_get_prevdesc()
> > * Reduce the two APIs parameters for easy to call.
> >
> > V2:
> > * Add tx_ring_size and rx_ring_size to struct fec_enet_private.
> > * Replace api fec_enet_get_nextdesc() with next_bd().
> > Replace api fec_enet_get_prevdesc() with pre_bd().
> >
> > * Move all ring size check logic to next_bd() and pre_bd(), which
> > simplifies the code redundancy.
> >
> > V1:
> > * Add BD ring size check to get the previous BD entry in correctly.
> >
> > Reviewed-by: Li Frank <B20596@freescale.com>
> > Signed-off-by: Fugang Duan <B38611@freescale.com>
> > ---
> > drivers/net/ethernet/freescale/fec.h | 3 +
> > drivers/net/ethernet/freescale/fec_main.c | 120
> > ++++++++++++++++++---------
> > --
> > 2 files changed, 77 insertions(+), 46 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/freescale/fec.h
> > b/drivers/net/ethernet/freescale/fec.h
> > index ae23600..0120217 100644
> > --- a/drivers/net/ethernet/freescale/fec.h
> > +++ b/drivers/net/ethernet/freescale/fec.h
> > @@ -296,6 +296,9 @@ struct fec_enet_private {
> > /* The ring entries to be free()ed */
> > struct bufdesc *dirty_tx;
> >
> > + unsigned short tx_ring_size;
> > + unsigned short rx_ring_size;
> > +
> > struct platform_device *pdev;
> >
> > int opened;
> > diff --git a/drivers/net/ethernet/freescale/fec_main.c
> > b/drivers/net/ethernet/freescale/fec_main.c
> > index 4ea1555..d5d8984 100644
> > --- a/drivers/net/ethernet/freescale/fec_main.c
> > +++ b/drivers/net/ethernet/freescale/fec_main.c
> > @@ -239,22 +239,57 @@ MODULE_PARM_DESC(macaddr, "FEC Ethernet MAC
> > address");
> >
> > static int mii_cnt;
> >
> > -static struct bufdesc *fec_enet_get_nextdesc(struct bufdesc *bdp, int
> > is_ex)
> > +static inline
> > +struct bufdesc *fec_enet_get_nextdesc(struct bufdesc *bdp, struct
> > +fec_enet_private *fep)
> > {
> > - struct bufdesc_ex *ex = (struct bufdesc_ex *)bdp;
> > - if (is_ex)
> > - return (struct bufdesc *)(ex + 1);
> > + struct bufdesc *new_bd = bdp + 1;
> > + struct bufdesc_ex *ex_new_bd = (struct bufdesc_ex *)bdp + 1;
> > + struct bufdesc_ex *ex_base;
> > + struct bufdesc *base;
> > + int ring_size;
> > +
> > + if (bdp >= fep->tx_bd_base) {
> > + base = fep->tx_bd_base;
> > + ring_size = fep->tx_ring_size;
> > + ex_base = (struct bufdesc_ex *)fep->tx_bd_base;
> > + } else {
> > + base = fep->rx_bd_base;
> > + ring_size = fep->rx_ring_size;
> > + ex_base = (struct bufdesc_ex *)fep->rx_bd_base;
> > + }
> > +
> > + if (fep->bufdesc_ex)
> > + return (struct bufdesc *)((ex_new_bd >= (ex_base +
> ring_size)) ?
> > + (ex_new_bd - ring_size) : ex_new_bd);
>
> I think "exbase:ex_new_bd" is better
>
> > else
> > - return bdp + 1;
> > + return (new_bd >= (base + ring_size)) ?
> > + (new_bd - ring_size) : new_bd;
>
> I think "base:new_bd" is better.
>
Yes, it is more clean for code review. I have sent the V4 version.
Thanks.
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH] net: fec: fix the error to get the previous BD entry
@ 2013-09-03 2:41 Fugang Duan
2013-09-03 3:00 ` Li Frank-B20596
2013-09-04 18:13 ` David Miller
0 siblings, 2 replies; 21+ messages in thread
From: Fugang Duan @ 2013-09-03 2:41 UTC (permalink / raw)
To: b20596, davem; +Cc: netdev, bhutchings, stephen
Bug: error to get the previous BD entry. When the current BD
is the first BD, the previous BD entry must be the last BD,
not "bdp - 1" in current logic.
V4:
* Optimize fec_enet_get_nextdesc() for code clean.
Replace "ex_new_bd - ring_size" with "ex_base".
Replace "new_bd - ring_size" with "base".
V3:
* Restore the API name because David suggest to use fec_enet_
prefix for all function in fec driver.
So, change next_bd() -> fec_enet_get_nextdesc()
change pre_bd() -> fec_enet_get_prevdesc()
* Reduce the two APIs parameters for easy to call.
V2:
* Add tx_ring_size and rx_ring_size to struct fec_enet_private.
* Replace api fec_enet_get_nextdesc() with next_bd().
Replace api fec_enet_get_prevdesc() with pre_bd().
* Move all ring size check logic to next_bd() and pre_bd(), which
simplifies the code redundancy.
V1:
* Add BD ring size check to get the previous BD entry in correctly.
Reviewed-by: Li Frank <B20596@freescale.com>
Signed-off-by: Fugang Duan <B38611@freescale.com>
---
drivers/net/ethernet/freescale/fec.h | 3 +
drivers/net/ethernet/freescale/fec_main.c | 120 ++++++++++++++++++-----------
2 files changed, 77 insertions(+), 46 deletions(-)
diff --git a/drivers/net/ethernet/freescale/fec.h b/drivers/net/ethernet/freescale/fec.h
index ae23600..0120217 100644
--- a/drivers/net/ethernet/freescale/fec.h
+++ b/drivers/net/ethernet/freescale/fec.h
@@ -296,6 +296,9 @@ struct fec_enet_private {
/* The ring entries to be free()ed */
struct bufdesc *dirty_tx;
+ unsigned short tx_ring_size;
+ unsigned short rx_ring_size;
+
struct platform_device *pdev;
int opened;
diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index 4ea1555..1364a6f 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -239,22 +239,57 @@ MODULE_PARM_DESC(macaddr, "FEC Ethernet MAC address");
static int mii_cnt;
-static struct bufdesc *fec_enet_get_nextdesc(struct bufdesc *bdp, int is_ex)
+static inline
+struct bufdesc *fec_enet_get_nextdesc(struct bufdesc *bdp, struct fec_enet_private *fep)
{
- struct bufdesc_ex *ex = (struct bufdesc_ex *)bdp;
- if (is_ex)
- return (struct bufdesc *)(ex + 1);
+ struct bufdesc *new_bd = bdp + 1;
+ struct bufdesc_ex *ex_new_bd = (struct bufdesc_ex *)bdp + 1;
+ struct bufdesc_ex *ex_base;
+ struct bufdesc *base;
+ int ring_size;
+
+ if (bdp >= fep->tx_bd_base) {
+ base = fep->tx_bd_base;
+ ring_size = fep->tx_ring_size;
+ ex_base = (struct bufdesc_ex *)fep->tx_bd_base;
+ } else {
+ base = fep->rx_bd_base;
+ ring_size = fep->rx_ring_size;
+ ex_base = (struct bufdesc_ex *)fep->rx_bd_base;
+ }
+
+ if (fep->bufdesc_ex)
+ return (struct bufdesc *)((ex_new_bd >= (ex_base + ring_size)) ?
+ ex_base : ex_new_bd);
else
- return bdp + 1;
+ return (new_bd >= (base + ring_size)) ?
+ base : new_bd;
}
-static struct bufdesc *fec_enet_get_prevdesc(struct bufdesc *bdp, int is_ex)
+static inline
+struct bufdesc *fec_enet_get_prevdesc(struct bufdesc *bdp, struct fec_enet_private *fep)
{
- struct bufdesc_ex *ex = (struct bufdesc_ex *)bdp;
- if (is_ex)
- return (struct bufdesc *)(ex - 1);
+ struct bufdesc *new_bd = bdp - 1;
+ struct bufdesc_ex *ex_new_bd = (struct bufdesc_ex *)bdp - 1;
+ struct bufdesc_ex *ex_base;
+ struct bufdesc *base;
+ int ring_size;
+
+ if (bdp >= fep->tx_bd_base) {
+ base = fep->tx_bd_base;
+ ring_size = fep->tx_ring_size;
+ ex_base = (struct bufdesc_ex *)fep->tx_bd_base;
+ } else {
+ base = fep->rx_bd_base;
+ ring_size = fep->rx_ring_size;
+ ex_base = (struct bufdesc_ex *)fep->rx_bd_base;
+ }
+
+ if (fep->bufdesc_ex)
+ return (struct bufdesc *)((ex_new_bd < ex_base) ?
+ (ex_new_bd + ring_size) : ex_new_bd);
else
- return bdp - 1;
+ return (new_bd < base) ? (new_bd + ring_size) : new_bd;
}
static void *swap_buffer(void *bufaddr, int len)
@@ -380,7 +415,7 @@ fec_enet_start_xmit(struct sk_buff *skb, struct net_device *ndev)
}
}
- bdp_pre = fec_enet_get_prevdesc(bdp, fep->bufdesc_ex);
+ bdp_pre = fec_enet_get_prevdesc(bdp, fep);
if ((id_entry->driver_data & FEC_QUIRK_ERR006358) &&
!(bdp_pre->cbd_sc & BD_ENET_TX_READY)) {
fep->delay_work.trig_tx = true;
@@ -389,10 +424,7 @@ fec_enet_start_xmit(struct sk_buff *skb, struct net_device *ndev)
}
/* If this was the last BD in the ring, start at the beginning again. */
- if (status & BD_ENET_TX_WRAP)
- bdp = fep->tx_bd_base;
- else
- bdp = fec_enet_get_nextdesc(bdp, fep->bufdesc_ex);
+ bdp = fec_enet_get_nextdesc(bdp, fep);
fep->cur_tx = bdp;
@@ -417,18 +449,18 @@ static void fec_enet_bd_init(struct net_device *dev)
/* Initialize the receive buffer descriptors. */
bdp = fep->rx_bd_base;
- for (i = 0; i < RX_RING_SIZE; i++) {
+ for (i = 0; i < fep->rx_ring_size; i++) {
/* Initialize the BD for every fragment in the page. */
if (bdp->cbd_bufaddr)
bdp->cbd_sc = BD_ENET_RX_EMPTY;
else
bdp->cbd_sc = 0;
- bdp = fec_enet_get_nextdesc(bdp, fep->bufdesc_ex);
+ bdp = fec_enet_get_nextdesc(bdp, fep);
}
/* Set the last buffer to wrap */
- bdp = fec_enet_get_prevdesc(bdp, fep->bufdesc_ex);
+ bdp = fec_enet_get_prevdesc(bdp, fep);
bdp->cbd_sc |= BD_SC_WRAP;
fep->cur_rx = fep->rx_bd_base;
@@ -436,7 +468,7 @@ static void fec_enet_bd_init(struct net_device *dev)
/* ...and the same for transmit */
bdp = fep->tx_bd_base;
fep->cur_tx = bdp;
- for (i = 0; i < TX_RING_SIZE; i++) {
+ for (i = 0; i < fep->tx_ring_size; i++) {
/* Initialize the BD for every fragment in the page. */
bdp->cbd_sc = 0;
@@ -445,11 +477,11 @@ static void fec_enet_bd_init(struct net_device *dev)
fep->tx_skbuff[i] = NULL;
}
bdp->cbd_bufaddr = 0;
- bdp = fec_enet_get_nextdesc(bdp, fep->bufdesc_ex);
+ bdp = fec_enet_get_nextdesc(bdp, fep);
}
/* Set the last buffer to wrap */
- bdp = fec_enet_get_prevdesc(bdp, fep->bufdesc_ex);
+ bdp = fec_enet_get_prevdesc(bdp, fep);
bdp->cbd_sc |= BD_SC_WRAP;
fep->dirty_tx = bdp;
}
@@ -510,10 +542,10 @@ fec_restart(struct net_device *ndev, int duplex)
writel(fep->bd_dma, fep->hwp + FEC_R_DES_START);
if (fep->bufdesc_ex)
writel((unsigned long)fep->bd_dma + sizeof(struct bufdesc_ex)
- * RX_RING_SIZE, fep->hwp + FEC_X_DES_START);
+ * fep->rx_ring_size, fep->hwp + FEC_X_DES_START);
else
writel((unsigned long)fep->bd_dma + sizeof(struct bufdesc)
- * RX_RING_SIZE, fep->hwp + FEC_X_DES_START);
+ * fep->rx_ring_size, fep->hwp + FEC_X_DES_START);
for (i = 0; i <= TX_RING_MOD_MASK; i++) {
@@ -727,10 +759,7 @@ fec_enet_tx(struct net_device *ndev)
bdp = fep->dirty_tx;
/* get next bdp of dirty_tx */
- if (bdp->cbd_sc & BD_ENET_TX_WRAP)
- bdp = fep->tx_bd_base;
- else
- bdp = fec_enet_get_nextdesc(bdp, fep->bufdesc_ex);
+ bdp = fec_enet_get_nextdesc(bdp, fep);
while (((status = bdp->cbd_sc) & BD_ENET_TX_READY) == 0) {
@@ -800,10 +829,7 @@ fec_enet_tx(struct net_device *ndev)
fep->dirty_tx = bdp;
/* Update pointer to next buffer descriptor to be transmitted */
- if (status & BD_ENET_TX_WRAP)
- bdp = fep->tx_bd_base;
- else
- bdp = fec_enet_get_nextdesc(bdp, fep->bufdesc_ex);
+ bdp = fec_enet_get_nextdesc(bdp, fep);
/* Since we have freed up a buffer, the ring is no longer full
*/
@@ -994,10 +1020,8 @@ rx_processing_done:
}
/* Update BD pointer to next entry */
- if (status & BD_ENET_RX_WRAP)
- bdp = fep->rx_bd_base;
- else
- bdp = fec_enet_get_nextdesc(bdp, fep->bufdesc_ex);
+ bdp = fec_enet_get_nextdesc(bdp, fep);
+
/* Doing this here will keep the FEC running while we process
* incoming frames. On a heavily loaded network, we should be
* able to keep up at the expense of system resources.
@@ -1663,7 +1687,7 @@ static void fec_enet_free_buffers(struct net_device *ndev)
struct bufdesc *bdp;
bdp = fep->rx_bd_base;
- for (i = 0; i < RX_RING_SIZE; i++) {
+ for (i = 0; i < fep->rx_ring_size; i++) {
skb = fep->rx_skbuff[i];
if (bdp->cbd_bufaddr)
@@ -1671,11 +1695,11 @@ static void fec_enet_free_buffers(struct net_device *ndev)
FEC_ENET_RX_FRSIZE, DMA_FROM_DEVICE);
if (skb)
dev_kfree_skb(skb);
- bdp = fec_enet_get_nextdesc(bdp, fep->bufdesc_ex);
+ bdp = fec_enet_get_nextdesc(bdp, fep);
}
bdp = fep->tx_bd_base;
- for (i = 0; i < TX_RING_SIZE; i++)
+ for (i = 0; i < fep->tx_ring_size; i++)
kfree(fep->tx_bounce[i]);
}
@@ -1687,7 +1711,7 @@ static int fec_enet_alloc_buffers(struct net_device *ndev)
struct bufdesc *bdp;
bdp = fep->rx_bd_base;
- for (i = 0; i < RX_RING_SIZE; i++) {
+ for (i = 0; i < fep->rx_ring_size; i++) {
skb = netdev_alloc_skb(ndev, FEC_ENET_RX_FRSIZE);
if (!skb) {
fec_enet_free_buffers(ndev);
@@ -1704,15 +1728,15 @@ static int fec_enet_alloc_buffers(struct net_device *ndev)
ebdp->cbd_esc = BD_ENET_RX_INT;
}
- bdp = fec_enet_get_nextdesc(bdp, fep->bufdesc_ex);
+ bdp = fec_enet_get_nextdesc(bdp, fep);
}
/* Set the last buffer to wrap. */
- bdp = fec_enet_get_prevdesc(bdp, fep->bufdesc_ex);
+ bdp = fec_enet_get_prevdesc(bdp, fep);
bdp->cbd_sc |= BD_SC_WRAP;
bdp = fep->tx_bd_base;
- for (i = 0; i < TX_RING_SIZE; i++) {
+ for (i = 0; i < fep->tx_ring_size; i++) {
fep->tx_bounce[i] = kmalloc(FEC_ENET_TX_FRSIZE, GFP_KERNEL);
bdp->cbd_sc = 0;
@@ -1723,11 +1747,11 @@ static int fec_enet_alloc_buffers(struct net_device *ndev)
ebdp->cbd_esc = BD_ENET_TX_INT;
}
- bdp = fec_enet_get_nextdesc(bdp, fep->bufdesc_ex);
+ bdp = fec_enet_get_nextdesc(bdp, fep);
}
/* Set the last buffer to wrap. */
- bdp = fec_enet_get_prevdesc(bdp, fep->bufdesc_ex);
+ bdp = fec_enet_get_prevdesc(bdp, fep);
bdp->cbd_sc |= BD_SC_WRAP;
return 0;
@@ -1967,13 +1991,17 @@ static int fec_enet_init(struct net_device *ndev)
/* Get the Ethernet address */
fec_get_mac(ndev);
+ /* init the tx & rx ring size */
+ fep->tx_ring_size = TX_RING_SIZE;
+ fep->rx_ring_size = RX_RING_SIZE;
+
/* Set receive and transmit descriptor base. */
fep->rx_bd_base = cbd_base;
if (fep->bufdesc_ex)
fep->tx_bd_base = (struct bufdesc *)
- (((struct bufdesc_ex *)cbd_base) + RX_RING_SIZE);
+ (((struct bufdesc_ex *)cbd_base) + fep->rx_ring_size);
else
- fep->tx_bd_base = cbd_base + RX_RING_SIZE;
+ fep->tx_bd_base = cbd_base + fep->rx_ring_size;
/* The FEC Ethernet specific entries in the device structure */
ndev->watchdog_timeo = TX_TIMEOUT;
--
1.7.1
^ permalink raw reply related [flat|nested] 21+ messages in thread
* RE: [PATCH] net: fec: fix the error to get the previous BD entry
2013-09-03 2:18 Fugang Duan
@ 2013-09-03 2:33 ` Li Frank-B20596
2013-09-03 2:49 ` Duan Fugang-B38611
0 siblings, 1 reply; 21+ messages in thread
From: Li Frank-B20596 @ 2013-09-03 2:33 UTC (permalink / raw)
To: Duan Fugang-B38611, davem; +Cc: netdev, bhutchings, stephen
> Bug: error to get the previous BD entry. When the current BD is the first BD,
> the previous BD entry must be the last BD, not "bdp - 1" in current logic.
>
> V3:
> * Restore the API name because David suggest to use fec_enet_
> prefix for all function in fec driver.
> So, change next_bd() -> fec_enet_get_nextdesc()
> change pre_bd() -> fec_enet_get_prevdesc()
> * Reduce the two APIs parameters for easy to call.
>
> V2:
> * Add tx_ring_size and rx_ring_size to struct fec_enet_private.
> * Replace api fec_enet_get_nextdesc() with next_bd().
> Replace api fec_enet_get_prevdesc() with pre_bd().
>
> * Move all ring size check logic to next_bd() and pre_bd(), which
> simplifies the code redundancy.
>
> V1:
> * Add BD ring size check to get the previous BD entry in correctly.
>
> Reviewed-by: Li Frank <B20596@freescale.com>
> Signed-off-by: Fugang Duan <B38611@freescale.com>
> ---
> drivers/net/ethernet/freescale/fec.h | 3 +
> drivers/net/ethernet/freescale/fec_main.c | 120 ++++++++++++++++++---------
> --
> 2 files changed, 77 insertions(+), 46 deletions(-)
>
> diff --git a/drivers/net/ethernet/freescale/fec.h
> b/drivers/net/ethernet/freescale/fec.h
> index ae23600..0120217 100644
> --- a/drivers/net/ethernet/freescale/fec.h
> +++ b/drivers/net/ethernet/freescale/fec.h
> @@ -296,6 +296,9 @@ struct fec_enet_private {
> /* The ring entries to be free()ed */
> struct bufdesc *dirty_tx;
>
> + unsigned short tx_ring_size;
> + unsigned short rx_ring_size;
> +
> struct platform_device *pdev;
>
> int opened;
> diff --git a/drivers/net/ethernet/freescale/fec_main.c
> b/drivers/net/ethernet/freescale/fec_main.c
> index 4ea1555..d5d8984 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -239,22 +239,57 @@ MODULE_PARM_DESC(macaddr, "FEC Ethernet MAC address");
>
> static int mii_cnt;
>
> -static struct bufdesc *fec_enet_get_nextdesc(struct bufdesc *bdp, int is_ex)
> +static inline
> +struct bufdesc *fec_enet_get_nextdesc(struct bufdesc *bdp, struct
> +fec_enet_private *fep)
> {
> - struct bufdesc_ex *ex = (struct bufdesc_ex *)bdp;
> - if (is_ex)
> - return (struct bufdesc *)(ex + 1);
> + struct bufdesc *new_bd = bdp + 1;
> + struct bufdesc_ex *ex_new_bd = (struct bufdesc_ex *)bdp + 1;
> + struct bufdesc_ex *ex_base;
> + struct bufdesc *base;
> + int ring_size;
> +
> + if (bdp >= fep->tx_bd_base) {
> + base = fep->tx_bd_base;
> + ring_size = fep->tx_ring_size;
> + ex_base = (struct bufdesc_ex *)fep->tx_bd_base;
> + } else {
> + base = fep->rx_bd_base;
> + ring_size = fep->rx_ring_size;
> + ex_base = (struct bufdesc_ex *)fep->rx_bd_base;
> + }
> +
> + if (fep->bufdesc_ex)
> + return (struct bufdesc *)((ex_new_bd >= (ex_base + ring_size)) ?
> + (ex_new_bd - ring_size) : ex_new_bd);
I think "exbase:ex_new_bd" is better
> else
> - return bdp + 1;
> + return (new_bd >= (base + ring_size)) ?
> + (new_bd - ring_size) : new_bd;
I think "base:new_bd" is better.
> }
>
> -static struct bufdesc *fec_enet_get_prevdesc(struct bufdesc *bdp, int is_ex)
> +static inline
> +struct bufdesc *fec_enet_get_prevdesc(struct bufdesc *bdp, struct
> +fec_enet_private *fep)
> {
> - struct bufdesc_ex *ex = (struct bufdesc_ex *)bdp;
> - if (is_ex)
> - return (struct bufdesc *)(ex - 1);
> + struct bufdesc *new_bd = bdp - 1;
> + struct bufdesc_ex *ex_new_bd = (struct bufdesc_ex *)bdp - 1;
> + struct bufdesc_ex *ex_base;
> + struct bufdesc *base;
> + int ring_size;
> +
> + if (bdp >= fep->tx_bd_base) {
> + base = fep->tx_bd_base;
> + ring_size = fep->tx_ring_size;
> + ex_base = (struct bufdesc_ex *)fep->tx_bd_base;
> + } else {
> + base = fep->rx_bd_base;
> + ring_size = fep->rx_ring_size;
> + ex_base = (struct bufdesc_ex *)fep->rx_bd_base;
> + }
> +
> + if (fep->bufdesc_ex)
> + return (struct bufdesc *)((ex_new_bd < ex_base) ?
> + (ex_new_bd + ring_size) : ex_new_bd);
> else
> - return bdp - 1;
> + return (new_bd < base) ? (new_bd + ring_size) : new_bd;
> }
>
> static void *swap_buffer(void *bufaddr, int len) @@ -380,7 +415,7 @@
> fec_enet_start_xmit(struct sk_buff *skb, struct net_device *ndev)
> }
> }
>
> - bdp_pre = fec_enet_get_prevdesc(bdp, fep->bufdesc_ex);
> + bdp_pre = fec_enet_get_prevdesc(bdp, fep);
> if ((id_entry->driver_data & FEC_QUIRK_ERR006358) &&
> !(bdp_pre->cbd_sc & BD_ENET_TX_READY)) {
> fep->delay_work.trig_tx = true;
> @@ -389,10 +424,7 @@ fec_enet_start_xmit(struct sk_buff *skb, struct
> net_device *ndev)
> }
>
> /* If this was the last BD in the ring, start at the beginning again.
> */
> - if (status & BD_ENET_TX_WRAP)
> - bdp = fep->tx_bd_base;
> - else
> - bdp = fec_enet_get_nextdesc(bdp, fep->bufdesc_ex);
> + bdp = fec_enet_get_nextdesc(bdp, fep);
>
> fep->cur_tx = bdp;
>
> @@ -417,18 +449,18 @@ static void fec_enet_bd_init(struct net_device *dev)
>
> /* Initialize the receive buffer descriptors. */
> bdp = fep->rx_bd_base;
> - for (i = 0; i < RX_RING_SIZE; i++) {
> + for (i = 0; i < fep->rx_ring_size; i++) {
>
> /* Initialize the BD for every fragment in the page. */
> if (bdp->cbd_bufaddr)
> bdp->cbd_sc = BD_ENET_RX_EMPTY;
> else
> bdp->cbd_sc = 0;
> - bdp = fec_enet_get_nextdesc(bdp, fep->bufdesc_ex);
> + bdp = fec_enet_get_nextdesc(bdp, fep);
> }
>
> /* Set the last buffer to wrap */
> - bdp = fec_enet_get_prevdesc(bdp, fep->bufdesc_ex);
> + bdp = fec_enet_get_prevdesc(bdp, fep);
> bdp->cbd_sc |= BD_SC_WRAP;
>
> fep->cur_rx = fep->rx_bd_base;
> @@ -436,7 +468,7 @@ static void fec_enet_bd_init(struct net_device *dev)
> /* ...and the same for transmit */
> bdp = fep->tx_bd_base;
> fep->cur_tx = bdp;
> - for (i = 0; i < TX_RING_SIZE; i++) {
> + for (i = 0; i < fep->tx_ring_size; i++) {
>
> /* Initialize the BD for every fragment in the page. */
> bdp->cbd_sc = 0;
> @@ -445,11 +477,11 @@ static void fec_enet_bd_init(struct net_device *dev)
> fep->tx_skbuff[i] = NULL;
> }
> bdp->cbd_bufaddr = 0;
> - bdp = fec_enet_get_nextdesc(bdp, fep->bufdesc_ex);
> + bdp = fec_enet_get_nextdesc(bdp, fep);
> }
>
> /* Set the last buffer to wrap */
> - bdp = fec_enet_get_prevdesc(bdp, fep->bufdesc_ex);
> + bdp = fec_enet_get_prevdesc(bdp, fep);
> bdp->cbd_sc |= BD_SC_WRAP;
> fep->dirty_tx = bdp;
> }
> @@ -510,10 +542,10 @@ fec_restart(struct net_device *ndev, int duplex)
> writel(fep->bd_dma, fep->hwp + FEC_R_DES_START);
> if (fep->bufdesc_ex)
> writel((unsigned long)fep->bd_dma + sizeof(struct bufdesc_ex)
> - * RX_RING_SIZE, fep->hwp + FEC_X_DES_START);
> + * fep->rx_ring_size, fep->hwp + FEC_X_DES_START);
> else
> writel((unsigned long)fep->bd_dma + sizeof(struct bufdesc)
> - * RX_RING_SIZE, fep->hwp + FEC_X_DES_START);
> + * fep->rx_ring_size, fep->hwp + FEC_X_DES_START);
>
>
> for (i = 0; i <= TX_RING_MOD_MASK; i++) { @@ -727,10 +759,7 @@
> fec_enet_tx(struct net_device *ndev)
> bdp = fep->dirty_tx;
>
> /* get next bdp of dirty_tx */
> - if (bdp->cbd_sc & BD_ENET_TX_WRAP)
> - bdp = fep->tx_bd_base;
> - else
> - bdp = fec_enet_get_nextdesc(bdp, fep->bufdesc_ex);
> + bdp = fec_enet_get_nextdesc(bdp, fep);
>
> while (((status = bdp->cbd_sc) & BD_ENET_TX_READY) == 0) {
>
> @@ -800,10 +829,7 @@ fec_enet_tx(struct net_device *ndev)
> fep->dirty_tx = bdp;
>
> /* Update pointer to next buffer descriptor to be transmitted */
> - if (status & BD_ENET_TX_WRAP)
> - bdp = fep->tx_bd_base;
> - else
> - bdp = fec_enet_get_nextdesc(bdp, fep->bufdesc_ex);
> + bdp = fec_enet_get_nextdesc(bdp, fep);
>
> /* Since we have freed up a buffer, the ring is no longer full
> */
> @@ -994,10 +1020,8 @@ rx_processing_done:
> }
>
> /* Update BD pointer to next entry */
> - if (status & BD_ENET_RX_WRAP)
> - bdp = fep->rx_bd_base;
> - else
> - bdp = fec_enet_get_nextdesc(bdp, fep->bufdesc_ex);
> + bdp = fec_enet_get_nextdesc(bdp, fep);
> +
> /* Doing this here will keep the FEC running while we process
> * incoming frames. On a heavily loaded network, we should be
> * able to keep up at the expense of system resources.
> @@ -1663,7 +1687,7 @@ static void fec_enet_free_buffers(struct net_device
> *ndev)
> struct bufdesc *bdp;
>
> bdp = fep->rx_bd_base;
> - for (i = 0; i < RX_RING_SIZE; i++) {
> + for (i = 0; i < fep->rx_ring_size; i++) {
> skb = fep->rx_skbuff[i];
>
> if (bdp->cbd_bufaddr)
> @@ -1671,11 +1695,11 @@ static void fec_enet_free_buffers(struct net_device
> *ndev)
> FEC_ENET_RX_FRSIZE, DMA_FROM_DEVICE);
> if (skb)
> dev_kfree_skb(skb);
> - bdp = fec_enet_get_nextdesc(bdp, fep->bufdesc_ex);
> + bdp = fec_enet_get_nextdesc(bdp, fep);
> }
>
> bdp = fep->tx_bd_base;
> - for (i = 0; i < TX_RING_SIZE; i++)
> + for (i = 0; i < fep->tx_ring_size; i++)
> kfree(fep->tx_bounce[i]);
> }
>
> @@ -1687,7 +1711,7 @@ static int fec_enet_alloc_buffers(struct net_device
> *ndev)
> struct bufdesc *bdp;
>
> bdp = fep->rx_bd_base;
> - for (i = 0; i < RX_RING_SIZE; i++) {
> + for (i = 0; i < fep->rx_ring_size; i++) {
> skb = netdev_alloc_skb(ndev, FEC_ENET_RX_FRSIZE);
> if (!skb) {
> fec_enet_free_buffers(ndev);
> @@ -1704,15 +1728,15 @@ static int fec_enet_alloc_buffers(struct net_device
> *ndev)
> ebdp->cbd_esc = BD_ENET_RX_INT;
> }
>
> - bdp = fec_enet_get_nextdesc(bdp, fep->bufdesc_ex);
> + bdp = fec_enet_get_nextdesc(bdp, fep);
> }
>
> /* Set the last buffer to wrap. */
> - bdp = fec_enet_get_prevdesc(bdp, fep->bufdesc_ex);
> + bdp = fec_enet_get_prevdesc(bdp, fep);
> bdp->cbd_sc |= BD_SC_WRAP;
>
> bdp = fep->tx_bd_base;
> - for (i = 0; i < TX_RING_SIZE; i++) {
> + for (i = 0; i < fep->tx_ring_size; i++) {
> fep->tx_bounce[i] = kmalloc(FEC_ENET_TX_FRSIZE, GFP_KERNEL);
>
> bdp->cbd_sc = 0;
> @@ -1723,11 +1747,11 @@ static int fec_enet_alloc_buffers(struct net_device
> *ndev)
> ebdp->cbd_esc = BD_ENET_TX_INT;
> }
>
> - bdp = fec_enet_get_nextdesc(bdp, fep->bufdesc_ex);
> + bdp = fec_enet_get_nextdesc(bdp, fep);
> }
>
> /* Set the last buffer to wrap. */
> - bdp = fec_enet_get_prevdesc(bdp, fep->bufdesc_ex);
> + bdp = fec_enet_get_prevdesc(bdp, fep);
> bdp->cbd_sc |= BD_SC_WRAP;
>
> return 0;
> @@ -1967,13 +1991,17 @@ static int fec_enet_init(struct net_device *ndev)
> /* Get the Ethernet address */
> fec_get_mac(ndev);
>
> + /* init the tx & rx ring size */
> + fep->tx_ring_size = TX_RING_SIZE;
> + fep->rx_ring_size = RX_RING_SIZE;
> +
> /* Set receive and transmit descriptor base. */
> fep->rx_bd_base = cbd_base;
> if (fep->bufdesc_ex)
> fep->tx_bd_base = (struct bufdesc *)
> - (((struct bufdesc_ex *)cbd_base) + RX_RING_SIZE);
> + (((struct bufdesc_ex *)cbd_base) + fep->rx_ring_size);
> else
> - fep->tx_bd_base = cbd_base + RX_RING_SIZE;
> + fep->tx_bd_base = cbd_base + fep->rx_ring_size;
>
> /* The FEC Ethernet specific entries in the device structure */
> ndev->watchdog_timeo = TX_TIMEOUT;
> --
> 1.7.1
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH] net: fec: fix the error to get the previous BD entry
@ 2013-09-03 2:18 Fugang Duan
2013-09-03 2:33 ` Li Frank-B20596
0 siblings, 1 reply; 21+ messages in thread
From: Fugang Duan @ 2013-09-03 2:18 UTC (permalink / raw)
To: b20596, davem; +Cc: netdev, bhutchings, stephen
Bug: error to get the previous BD entry. When the current BD
is the first BD, the previous BD entry must be the last BD,
not "bdp - 1" in current logic.
V3:
* Restore the API name because David suggest to use fec_enet_
prefix for all function in fec driver.
So, change next_bd() -> fec_enet_get_nextdesc()
change pre_bd() -> fec_enet_get_prevdesc()
* Reduce the two APIs parameters for easy to call.
V2:
* Add tx_ring_size and rx_ring_size to struct fec_enet_private.
* Replace api fec_enet_get_nextdesc() with next_bd().
Replace api fec_enet_get_prevdesc() with pre_bd().
* Move all ring size check logic to next_bd() and pre_bd(), which
simplifies the code redundancy.
V1:
* Add BD ring size check to get the previous BD entry in correctly.
Reviewed-by: Li Frank <B20596@freescale.com>
Signed-off-by: Fugang Duan <B38611@freescale.com>
---
drivers/net/ethernet/freescale/fec.h | 3 +
drivers/net/ethernet/freescale/fec_main.c | 120 ++++++++++++++++++-----------
2 files changed, 77 insertions(+), 46 deletions(-)
diff --git a/drivers/net/ethernet/freescale/fec.h b/drivers/net/ethernet/freescale/fec.h
index ae23600..0120217 100644
--- a/drivers/net/ethernet/freescale/fec.h
+++ b/drivers/net/ethernet/freescale/fec.h
@@ -296,6 +296,9 @@ struct fec_enet_private {
/* The ring entries to be free()ed */
struct bufdesc *dirty_tx;
+ unsigned short tx_ring_size;
+ unsigned short rx_ring_size;
+
struct platform_device *pdev;
int opened;
diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index 4ea1555..d5d8984 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -239,22 +239,57 @@ MODULE_PARM_DESC(macaddr, "FEC Ethernet MAC address");
static int mii_cnt;
-static struct bufdesc *fec_enet_get_nextdesc(struct bufdesc *bdp, int is_ex)
+static inline
+struct bufdesc *fec_enet_get_nextdesc(struct bufdesc *bdp, struct fec_enet_private *fep)
{
- struct bufdesc_ex *ex = (struct bufdesc_ex *)bdp;
- if (is_ex)
- return (struct bufdesc *)(ex + 1);
+ struct bufdesc *new_bd = bdp + 1;
+ struct bufdesc_ex *ex_new_bd = (struct bufdesc_ex *)bdp + 1;
+ struct bufdesc_ex *ex_base;
+ struct bufdesc *base;
+ int ring_size;
+
+ if (bdp >= fep->tx_bd_base) {
+ base = fep->tx_bd_base;
+ ring_size = fep->tx_ring_size;
+ ex_base = (struct bufdesc_ex *)fep->tx_bd_base;
+ } else {
+ base = fep->rx_bd_base;
+ ring_size = fep->rx_ring_size;
+ ex_base = (struct bufdesc_ex *)fep->rx_bd_base;
+ }
+
+ if (fep->bufdesc_ex)
+ return (struct bufdesc *)((ex_new_bd >= (ex_base + ring_size)) ?
+ (ex_new_bd - ring_size) : ex_new_bd);
else
- return bdp + 1;
+ return (new_bd >= (base + ring_size)) ?
+ (new_bd - ring_size) : new_bd;
}
-static struct bufdesc *fec_enet_get_prevdesc(struct bufdesc *bdp, int is_ex)
+static inline
+struct bufdesc *fec_enet_get_prevdesc(struct bufdesc *bdp, struct fec_enet_private *fep)
{
- struct bufdesc_ex *ex = (struct bufdesc_ex *)bdp;
- if (is_ex)
- return (struct bufdesc *)(ex - 1);
+ struct bufdesc *new_bd = bdp - 1;
+ struct bufdesc_ex *ex_new_bd = (struct bufdesc_ex *)bdp - 1;
+ struct bufdesc_ex *ex_base;
+ struct bufdesc *base;
+ int ring_size;
+
+ if (bdp >= fep->tx_bd_base) {
+ base = fep->tx_bd_base;
+ ring_size = fep->tx_ring_size;
+ ex_base = (struct bufdesc_ex *)fep->tx_bd_base;
+ } else {
+ base = fep->rx_bd_base;
+ ring_size = fep->rx_ring_size;
+ ex_base = (struct bufdesc_ex *)fep->rx_bd_base;
+ }
+
+ if (fep->bufdesc_ex)
+ return (struct bufdesc *)((ex_new_bd < ex_base) ?
+ (ex_new_bd + ring_size) : ex_new_bd);
else
- return bdp - 1;
+ return (new_bd < base) ? (new_bd + ring_size) : new_bd;
}
static void *swap_buffer(void *bufaddr, int len)
@@ -380,7 +415,7 @@ fec_enet_start_xmit(struct sk_buff *skb, struct net_device *ndev)
}
}
- bdp_pre = fec_enet_get_prevdesc(bdp, fep->bufdesc_ex);
+ bdp_pre = fec_enet_get_prevdesc(bdp, fep);
if ((id_entry->driver_data & FEC_QUIRK_ERR006358) &&
!(bdp_pre->cbd_sc & BD_ENET_TX_READY)) {
fep->delay_work.trig_tx = true;
@@ -389,10 +424,7 @@ fec_enet_start_xmit(struct sk_buff *skb, struct net_device *ndev)
}
/* If this was the last BD in the ring, start at the beginning again. */
- if (status & BD_ENET_TX_WRAP)
- bdp = fep->tx_bd_base;
- else
- bdp = fec_enet_get_nextdesc(bdp, fep->bufdesc_ex);
+ bdp = fec_enet_get_nextdesc(bdp, fep);
fep->cur_tx = bdp;
@@ -417,18 +449,18 @@ static void fec_enet_bd_init(struct net_device *dev)
/* Initialize the receive buffer descriptors. */
bdp = fep->rx_bd_base;
- for (i = 0; i < RX_RING_SIZE; i++) {
+ for (i = 0; i < fep->rx_ring_size; i++) {
/* Initialize the BD for every fragment in the page. */
if (bdp->cbd_bufaddr)
bdp->cbd_sc = BD_ENET_RX_EMPTY;
else
bdp->cbd_sc = 0;
- bdp = fec_enet_get_nextdesc(bdp, fep->bufdesc_ex);
+ bdp = fec_enet_get_nextdesc(bdp, fep);
}
/* Set the last buffer to wrap */
- bdp = fec_enet_get_prevdesc(bdp, fep->bufdesc_ex);
+ bdp = fec_enet_get_prevdesc(bdp, fep);
bdp->cbd_sc |= BD_SC_WRAP;
fep->cur_rx = fep->rx_bd_base;
@@ -436,7 +468,7 @@ static void fec_enet_bd_init(struct net_device *dev)
/* ...and the same for transmit */
bdp = fep->tx_bd_base;
fep->cur_tx = bdp;
- for (i = 0; i < TX_RING_SIZE; i++) {
+ for (i = 0; i < fep->tx_ring_size; i++) {
/* Initialize the BD for every fragment in the page. */
bdp->cbd_sc = 0;
@@ -445,11 +477,11 @@ static void fec_enet_bd_init(struct net_device *dev)
fep->tx_skbuff[i] = NULL;
}
bdp->cbd_bufaddr = 0;
- bdp = fec_enet_get_nextdesc(bdp, fep->bufdesc_ex);
+ bdp = fec_enet_get_nextdesc(bdp, fep);
}
/* Set the last buffer to wrap */
- bdp = fec_enet_get_prevdesc(bdp, fep->bufdesc_ex);
+ bdp = fec_enet_get_prevdesc(bdp, fep);
bdp->cbd_sc |= BD_SC_WRAP;
fep->dirty_tx = bdp;
}
@@ -510,10 +542,10 @@ fec_restart(struct net_device *ndev, int duplex)
writel(fep->bd_dma, fep->hwp + FEC_R_DES_START);
if (fep->bufdesc_ex)
writel((unsigned long)fep->bd_dma + sizeof(struct bufdesc_ex)
- * RX_RING_SIZE, fep->hwp + FEC_X_DES_START);
+ * fep->rx_ring_size, fep->hwp + FEC_X_DES_START);
else
writel((unsigned long)fep->bd_dma + sizeof(struct bufdesc)
- * RX_RING_SIZE, fep->hwp + FEC_X_DES_START);
+ * fep->rx_ring_size, fep->hwp + FEC_X_DES_START);
for (i = 0; i <= TX_RING_MOD_MASK; i++) {
@@ -727,10 +759,7 @@ fec_enet_tx(struct net_device *ndev)
bdp = fep->dirty_tx;
/* get next bdp of dirty_tx */
- if (bdp->cbd_sc & BD_ENET_TX_WRAP)
- bdp = fep->tx_bd_base;
- else
- bdp = fec_enet_get_nextdesc(bdp, fep->bufdesc_ex);
+ bdp = fec_enet_get_nextdesc(bdp, fep);
while (((status = bdp->cbd_sc) & BD_ENET_TX_READY) == 0) {
@@ -800,10 +829,7 @@ fec_enet_tx(struct net_device *ndev)
fep->dirty_tx = bdp;
/* Update pointer to next buffer descriptor to be transmitted */
- if (status & BD_ENET_TX_WRAP)
- bdp = fep->tx_bd_base;
- else
- bdp = fec_enet_get_nextdesc(bdp, fep->bufdesc_ex);
+ bdp = fec_enet_get_nextdesc(bdp, fep);
/* Since we have freed up a buffer, the ring is no longer full
*/
@@ -994,10 +1020,8 @@ rx_processing_done:
}
/* Update BD pointer to next entry */
- if (status & BD_ENET_RX_WRAP)
- bdp = fep->rx_bd_base;
- else
- bdp = fec_enet_get_nextdesc(bdp, fep->bufdesc_ex);
+ bdp = fec_enet_get_nextdesc(bdp, fep);
+
/* Doing this here will keep the FEC running while we process
* incoming frames. On a heavily loaded network, we should be
* able to keep up at the expense of system resources.
@@ -1663,7 +1687,7 @@ static void fec_enet_free_buffers(struct net_device *ndev)
struct bufdesc *bdp;
bdp = fep->rx_bd_base;
- for (i = 0; i < RX_RING_SIZE; i++) {
+ for (i = 0; i < fep->rx_ring_size; i++) {
skb = fep->rx_skbuff[i];
if (bdp->cbd_bufaddr)
@@ -1671,11 +1695,11 @@ static void fec_enet_free_buffers(struct net_device *ndev)
FEC_ENET_RX_FRSIZE, DMA_FROM_DEVICE);
if (skb)
dev_kfree_skb(skb);
- bdp = fec_enet_get_nextdesc(bdp, fep->bufdesc_ex);
+ bdp = fec_enet_get_nextdesc(bdp, fep);
}
bdp = fep->tx_bd_base;
- for (i = 0; i < TX_RING_SIZE; i++)
+ for (i = 0; i < fep->tx_ring_size; i++)
kfree(fep->tx_bounce[i]);
}
@@ -1687,7 +1711,7 @@ static int fec_enet_alloc_buffers(struct net_device *ndev)
struct bufdesc *bdp;
bdp = fep->rx_bd_base;
- for (i = 0; i < RX_RING_SIZE; i++) {
+ for (i = 0; i < fep->rx_ring_size; i++) {
skb = netdev_alloc_skb(ndev, FEC_ENET_RX_FRSIZE);
if (!skb) {
fec_enet_free_buffers(ndev);
@@ -1704,15 +1728,15 @@ static int fec_enet_alloc_buffers(struct net_device *ndev)
ebdp->cbd_esc = BD_ENET_RX_INT;
}
- bdp = fec_enet_get_nextdesc(bdp, fep->bufdesc_ex);
+ bdp = fec_enet_get_nextdesc(bdp, fep);
}
/* Set the last buffer to wrap. */
- bdp = fec_enet_get_prevdesc(bdp, fep->bufdesc_ex);
+ bdp = fec_enet_get_prevdesc(bdp, fep);
bdp->cbd_sc |= BD_SC_WRAP;
bdp = fep->tx_bd_base;
- for (i = 0; i < TX_RING_SIZE; i++) {
+ for (i = 0; i < fep->tx_ring_size; i++) {
fep->tx_bounce[i] = kmalloc(FEC_ENET_TX_FRSIZE, GFP_KERNEL);
bdp->cbd_sc = 0;
@@ -1723,11 +1747,11 @@ static int fec_enet_alloc_buffers(struct net_device *ndev)
ebdp->cbd_esc = BD_ENET_TX_INT;
}
- bdp = fec_enet_get_nextdesc(bdp, fep->bufdesc_ex);
+ bdp = fec_enet_get_nextdesc(bdp, fep);
}
/* Set the last buffer to wrap. */
- bdp = fec_enet_get_prevdesc(bdp, fep->bufdesc_ex);
+ bdp = fec_enet_get_prevdesc(bdp, fep);
bdp->cbd_sc |= BD_SC_WRAP;
return 0;
@@ -1967,13 +1991,17 @@ static int fec_enet_init(struct net_device *ndev)
/* Get the Ethernet address */
fec_get_mac(ndev);
+ /* init the tx & rx ring size */
+ fep->tx_ring_size = TX_RING_SIZE;
+ fep->rx_ring_size = RX_RING_SIZE;
+
/* Set receive and transmit descriptor base. */
fep->rx_bd_base = cbd_base;
if (fep->bufdesc_ex)
fep->tx_bd_base = (struct bufdesc *)
- (((struct bufdesc_ex *)cbd_base) + RX_RING_SIZE);
+ (((struct bufdesc_ex *)cbd_base) + fep->rx_ring_size);
else
- fep->tx_bd_base = cbd_base + RX_RING_SIZE;
+ fep->tx_bd_base = cbd_base + fep->rx_ring_size;
/* The FEC Ethernet specific entries in the device structure */
ndev->watchdog_timeo = TX_TIMEOUT;
--
1.7.1
^ permalink raw reply related [flat|nested] 21+ messages in thread
* RE: [PATCH] net: fec: fix the error to get the previous BD entry
2013-09-02 14:30 ` Li Frank-B20596
@ 2013-09-03 1:39 ` Duan Fugang-B38611
0 siblings, 0 replies; 21+ messages in thread
From: Duan Fugang-B38611 @ 2013-09-03 1:39 UTC (permalink / raw)
To: Li Frank-B20596, davem; +Cc: netdev, bhutchings, stephen
From: Li Frank-B20596
Data: Monday, September 02, 2013 10:30 PM
> To: Duan Fugang-B38611; davem@davemloft.net
> Cc: netdev@vger.kernel.org; bhutchings@solarflare.com;
> stephen@networkplumber.org
> Subject: RE: [PATCH] net: fec: fix the error to get the previous BD entry
>
>
>
> > -----Original Message-----
> > From: Duan Fugang-B38611
> > Sent: Monday, September 02, 2013 6:36 AM
> > To: Li Frank-B20596; davem@davemloft.net
> > Cc: netdev@vger.kernel.org; bhutchings@solarflare.com;
> > stephen@networkplumber.org
> > Subject: [PATCH] net: fec: fix the error to get the previous BD entry
> >
> > Bug: error to get the previous BD entry. When the current BD is the
> > first BD, the previous BD entry must be the last BD, not "bdp - 1" in
> current logic.
> >
> > V2:
> > Add tx_ring_size and rx_ring_size to struct fec_enet_private.
> > Replace api fec_enet_get_nextdesc() with next_bd().
> > Replace api fec_enet_get_prevdesc() with pre_bd().
> >
> > Move all ring size check logic to next_bd() and pre_bd, which
> > simplifies the code redundancy.
> >
> > V1:
> > Add BD ring size check to get the previous BD entry in correctly.
> >
> > Signed-off-by: Fugang Duan <B38611@freescale.com>
> > ---
> > drivers/net/ethernet/freescale/fec.h | 3 +
> > drivers/net/ethernet/freescale/fec_main.c | 94 +++++++++++++++-------
> -----
> > -
> > 2 files changed, 53 insertions(+), 44 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/freescale/fec.h
> > b/drivers/net/ethernet/freescale/fec.h
> > index ae23600..0120217 100644
> > --- a/drivers/net/ethernet/freescale/fec.h
> > +++ b/drivers/net/ethernet/freescale/fec.h
> > @@ -296,6 +296,9 @@ struct fec_enet_private {
> > /* The ring entries to be free()ed */
> > struct bufdesc *dirty_tx;
> >
> > + unsigned short tx_ring_size;
> > + unsigned short rx_ring_size;
> > +
> > struct platform_device *pdev;
> >
> > int opened;
> > diff --git a/drivers/net/ethernet/freescale/fec_main.c
> > b/drivers/net/ethernet/freescale/fec_main.c
> > index 4ea1555..9894dd3 100644
> > --- a/drivers/net/ethernet/freescale/fec_main.c
> > +++ b/drivers/net/ethernet/freescale/fec_main.c
> > @@ -239,22 +239,35 @@ MODULE_PARM_DESC(macaddr, "FEC Ethernet MAC
> > address");
> >
> > static int mii_cnt;
> >
> > -static struct bufdesc *fec_enet_get_nextdesc(struct bufdesc *bdp, int
> > is_ex)
> > +static inline
> > +struct bufdesc *next_bd(struct bufdesc *bdp, struct bufdesc *base,
> > + int ring_size, int is_ex)
>
> David suggest use fec_enet_ prefix for all function in fec driver when I
> upstream it.
>
>
> How about use
> Fec_enet_next_bd(struct bufdesc *bdp, *fep)
> The parameter will become little and easy to call.
>
> > {
> > - struct bufdesc_ex *ex = (struct bufdesc_ex *)bdp;
> > + struct bufdesc *new_bd = bdp + 1;
> > + struct bufdesc_ex *ex_new_bd = (struct bufdesc_ex *)bdp + 1;
> > + struct bufdesc_ex *ex_base = (struct bufdesc_ex *)base;
> > +
>
> If pass down fep,
> If( bdp > fep->rx_base) {
> Ex_base = fep->rx_base;
> Ring_size = fep->rx_ring_size;
> }
> Else {
> Ex_base = fep->tx_base.
> Ring_size = fep->tx_ring_size;
> }
>
> Because RX always behind of TX.
>
Good idea, I will send patch V3. Thanks.
^ permalink raw reply [flat|nested] 21+ messages in thread
* RE: [PATCH] net: fec: fix the error to get the previous BD entry
2013-09-02 11:35 Fugang Duan
@ 2013-09-02 14:30 ` Li Frank-B20596
2013-09-03 1:39 ` Duan Fugang-B38611
0 siblings, 1 reply; 21+ messages in thread
From: Li Frank-B20596 @ 2013-09-02 14:30 UTC (permalink / raw)
To: Duan Fugang-B38611, davem; +Cc: netdev, bhutchings, stephen
> -----Original Message-----
> From: Duan Fugang-B38611
> Sent: Monday, September 02, 2013 6:36 AM
> To: Li Frank-B20596; davem@davemloft.net
> Cc: netdev@vger.kernel.org; bhutchings@solarflare.com;
> stephen@networkplumber.org
> Subject: [PATCH] net: fec: fix the error to get the previous BD entry
>
> Bug: error to get the previous BD entry. When the current BD is the first BD,
> the previous BD entry must be the last BD, not "bdp - 1" in current logic.
>
> V2:
> Add tx_ring_size and rx_ring_size to struct fec_enet_private.
> Replace api fec_enet_get_nextdesc() with next_bd().
> Replace api fec_enet_get_prevdesc() with pre_bd().
>
> Move all ring size check logic to next_bd() and pre_bd, which
> simplifies the code redundancy.
>
> V1:
> Add BD ring size check to get the previous BD entry in correctly.
>
> Signed-off-by: Fugang Duan <B38611@freescale.com>
> ---
> drivers/net/ethernet/freescale/fec.h | 3 +
> drivers/net/ethernet/freescale/fec_main.c | 94 +++++++++++++++------------
> -
> 2 files changed, 53 insertions(+), 44 deletions(-)
>
> diff --git a/drivers/net/ethernet/freescale/fec.h
> b/drivers/net/ethernet/freescale/fec.h
> index ae23600..0120217 100644
> --- a/drivers/net/ethernet/freescale/fec.h
> +++ b/drivers/net/ethernet/freescale/fec.h
> @@ -296,6 +296,9 @@ struct fec_enet_private {
> /* The ring entries to be free()ed */
> struct bufdesc *dirty_tx;
>
> + unsigned short tx_ring_size;
> + unsigned short rx_ring_size;
> +
> struct platform_device *pdev;
>
> int opened;
> diff --git a/drivers/net/ethernet/freescale/fec_main.c
> b/drivers/net/ethernet/freescale/fec_main.c
> index 4ea1555..9894dd3 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -239,22 +239,35 @@ MODULE_PARM_DESC(macaddr, "FEC Ethernet MAC address");
>
> static int mii_cnt;
>
> -static struct bufdesc *fec_enet_get_nextdesc(struct bufdesc *bdp, int is_ex)
> +static inline
> +struct bufdesc *next_bd(struct bufdesc *bdp, struct bufdesc *base,
> + int ring_size, int is_ex)
David suggest use fec_enet_ prefix for all function in fec driver when
I upstream it.
How about use
Fec_enet_next_bd(struct bufdesc *bdp, *fep)
The parameter will become little and easy to call.
> {
> - struct bufdesc_ex *ex = (struct bufdesc_ex *)bdp;
> + struct bufdesc *new_bd = bdp + 1;
> + struct bufdesc_ex *ex_new_bd = (struct bufdesc_ex *)bdp + 1;
> + struct bufdesc_ex *ex_base = (struct bufdesc_ex *)base;
> +
If pass down fep,
If( bdp > fep->rx_base) {
Ex_base = fep->rx_base;
Ring_size = fep->rx_ring_size;
}
Else {
Ex_base = fep->tx_base.
Ring_size = fep->tx_ring_size;
}
Because RX always behind of TX.
> if (is_ex)
> - return (struct bufdesc *)(ex + 1);
> + return (struct bufdesc *)((ex_new_bd >= (ex_base + ring_size)) ?
> + (ex_new_bd - ring_size) : ex_new_bd);
> else
> - return bdp + 1;
> + return (new_bd >= (base + ring_size)) ?
> + (new_bd - ring_size) : new_bd;
> }
>
> -static struct bufdesc *fec_enet_get_prevdesc(struct bufdesc *bdp, int is_ex)
> +static inline
> +struct bufdesc *pre_bd(struct bufdesc *bdp, struct bufdesc *base,
> + int ring_size, int is_ex)
> {
> - struct bufdesc_ex *ex = (struct bufdesc_ex *)bdp;
> + struct bufdesc *new_bd = bdp - 1;
> + struct bufdesc_ex *ex_new_bd = (struct bufdesc_ex *)bdp - 1;
> + struct bufdesc_ex *ex_base = (struct bufdesc_ex *)base;
> +
> if (is_ex)
> - return (struct bufdesc *)(ex - 1);
> + return (struct bufdesc *)((ex_new_bd < ex_base) ?
> + (ex_new_bd + ring_size) : ex_new_bd);
> else
> - return bdp - 1;
> + return (new_bd < base) ? (new_bd + ring_size) : new_bd;
> }
>
> static void *swap_buffer(void *bufaddr, int len) @@ -380,7 +393,7 @@
> fec_enet_start_xmit(struct sk_buff *skb, struct net_device *ndev)
> }
> }
>
> - bdp_pre = fec_enet_get_prevdesc(bdp, fep->bufdesc_ex);
> + bdp_pre = pre_bd(bdp, fep->tx_bd_base, fep->tx_ring_size,
> +fep->bufdesc_ex);
> if ((id_entry->driver_data & FEC_QUIRK_ERR006358) &&
> !(bdp_pre->cbd_sc & BD_ENET_TX_READY)) {
> fep->delay_work.trig_tx = true;
> @@ -389,10 +402,7 @@ fec_enet_start_xmit(struct sk_buff *skb, struct
> net_device *ndev)
> }
>
> /* If this was the last BD in the ring, start at the beginning again.
> */
> - if (status & BD_ENET_TX_WRAP)
> - bdp = fep->tx_bd_base;
> - else
> - bdp = fec_enet_get_nextdesc(bdp, fep->bufdesc_ex);
> + bdp = next_bd(bdp, fep->tx_bd_base, fep->tx_ring_size,
> +fep->bufdesc_ex);
>
> fep->cur_tx = bdp;
>
> @@ -417,18 +427,18 @@ static void fec_enet_bd_init(struct net_device *dev)
>
> /* Initialize the receive buffer descriptors. */
> bdp = fep->rx_bd_base;
> - for (i = 0; i < RX_RING_SIZE; i++) {
> + for (i = 0; i < fep->rx_ring_size; i++) {
>
> /* Initialize the BD for every fragment in the page. */
> if (bdp->cbd_bufaddr)
> bdp->cbd_sc = BD_ENET_RX_EMPTY;
> else
> bdp->cbd_sc = 0;
> - bdp = fec_enet_get_nextdesc(bdp, fep->bufdesc_ex);
> + bdp = next_bd(bdp, fep->rx_bd_base, fep->rx_ring_size,
> +fep->bufdesc_ex);
> }
>
> /* Set the last buffer to wrap */
> - bdp = fec_enet_get_prevdesc(bdp, fep->bufdesc_ex);
> + bdp = pre_bd(bdp, fep->rx_bd_base, fep->rx_ring_size,
> +fep->bufdesc_ex);
> bdp->cbd_sc |= BD_SC_WRAP;
>
> fep->cur_rx = fep->rx_bd_base;
> @@ -436,7 +446,7 @@ static void fec_enet_bd_init(struct net_device *dev)
> /* ...and the same for transmit */
> bdp = fep->tx_bd_base;
> fep->cur_tx = bdp;
> - for (i = 0; i < TX_RING_SIZE; i++) {
> + for (i = 0; i < fep->tx_ring_size; i++) {
>
> /* Initialize the BD for every fragment in the page. */
> bdp->cbd_sc = 0;
> @@ -445,11 +455,11 @@ static void fec_enet_bd_init(struct net_device *dev)
> fep->tx_skbuff[i] = NULL;
> }
> bdp->cbd_bufaddr = 0;
> - bdp = fec_enet_get_nextdesc(bdp, fep->bufdesc_ex);
> + bdp = next_bd(bdp, fep->tx_bd_base, fep->tx_ring_size,
> +fep->bufdesc_ex);
> }
>
> /* Set the last buffer to wrap */
> - bdp = fec_enet_get_prevdesc(bdp, fep->bufdesc_ex);
> + bdp = pre_bd(bdp, fep->tx_bd_base, fep->tx_ring_size,
> +fep->bufdesc_ex);
> bdp->cbd_sc |= BD_SC_WRAP;
> fep->dirty_tx = bdp;
> }
> @@ -510,10 +520,10 @@ fec_restart(struct net_device *ndev, int duplex)
> writel(fep->bd_dma, fep->hwp + FEC_R_DES_START);
> if (fep->bufdesc_ex)
> writel((unsigned long)fep->bd_dma + sizeof(struct bufdesc_ex)
> - * RX_RING_SIZE, fep->hwp + FEC_X_DES_START);
> + * fep->rx_ring_size, fep->hwp + FEC_X_DES_START);
> else
> writel((unsigned long)fep->bd_dma + sizeof(struct bufdesc)
> - * RX_RING_SIZE, fep->hwp + FEC_X_DES_START);
> + * fep->rx_ring_size, fep->hwp + FEC_X_DES_START);
>
>
> for (i = 0; i <= TX_RING_MOD_MASK; i++) { @@ -727,10 +737,7 @@
> fec_enet_tx(struct net_device *ndev)
> bdp = fep->dirty_tx;
>
> /* get next bdp of dirty_tx */
> - if (bdp->cbd_sc & BD_ENET_TX_WRAP)
> - bdp = fep->tx_bd_base;
> - else
> - bdp = fec_enet_get_nextdesc(bdp, fep->bufdesc_ex);
> + bdp = next_bd(bdp, fep->tx_bd_base, fep->tx_ring_size,
> +fep->bufdesc_ex);
>
> while (((status = bdp->cbd_sc) & BD_ENET_TX_READY) == 0) {
>
> @@ -800,10 +807,7 @@ fec_enet_tx(struct net_device *ndev)
> fep->dirty_tx = bdp;
>
> /* Update pointer to next buffer descriptor to be transmitted */
> - if (status & BD_ENET_TX_WRAP)
> - bdp = fep->tx_bd_base;
> - else
> - bdp = fec_enet_get_nextdesc(bdp, fep->bufdesc_ex);
> + bdp = next_bd(bdp, fep->tx_bd_base, fep->tx_ring_size,
> +fep->bufdesc_ex);
>
> /* Since we have freed up a buffer, the ring is no longer full
> */
> @@ -994,10 +998,8 @@ rx_processing_done:
> }
>
> /* Update BD pointer to next entry */
> - if (status & BD_ENET_RX_WRAP)
> - bdp = fep->rx_bd_base;
> - else
> - bdp = fec_enet_get_nextdesc(bdp, fep->bufdesc_ex);
> + bdp = next_bd(bdp, fep->rx_bd_base, fep->rx_ring_size,
> +fep->bufdesc_ex);
> +
> /* Doing this here will keep the FEC running while we process
> * incoming frames. On a heavily loaded network, we should be
> * able to keep up at the expense of system resources.
> @@ -1663,7 +1665,7 @@ static void fec_enet_free_buffers(struct net_device
> *ndev)
> struct bufdesc *bdp;
>
> bdp = fep->rx_bd_base;
> - for (i = 0; i < RX_RING_SIZE; i++) {
> + for (i = 0; i < fep->rx_ring_size; i++) {
> skb = fep->rx_skbuff[i];
>
> if (bdp->cbd_bufaddr)
> @@ -1671,11 +1673,11 @@ static void fec_enet_free_buffers(struct net_device
> *ndev)
> FEC_ENET_RX_FRSIZE, DMA_FROM_DEVICE);
> if (skb)
> dev_kfree_skb(skb);
> - bdp = fec_enet_get_nextdesc(bdp, fep->bufdesc_ex);
> + bdp = next_bd(bdp, fep->rx_bd_base, fep->rx_ring_size,
> +fep->bufdesc_ex);
> }
>
> bdp = fep->tx_bd_base;
> - for (i = 0; i < TX_RING_SIZE; i++)
> + for (i = 0; i < fep->tx_ring_size; i++)
> kfree(fep->tx_bounce[i]);
> }
>
> @@ -1687,7 +1689,7 @@ static int fec_enet_alloc_buffers(struct net_device
> *ndev)
> struct bufdesc *bdp;
>
> bdp = fep->rx_bd_base;
> - for (i = 0; i < RX_RING_SIZE; i++) {
> + for (i = 0; i < fep->rx_ring_size; i++) {
> skb = netdev_alloc_skb(ndev, FEC_ENET_RX_FRSIZE);
> if (!skb) {
> fec_enet_free_buffers(ndev);
> @@ -1704,15 +1706,15 @@ static int fec_enet_alloc_buffers(struct net_device
> *ndev)
> ebdp->cbd_esc = BD_ENET_RX_INT;
> }
>
> - bdp = fec_enet_get_nextdesc(bdp, fep->bufdesc_ex);
> + bdp = next_bd(bdp, fep->rx_bd_base, fep->rx_ring_size,
> +fep->bufdesc_ex);
> }
>
> /* Set the last buffer to wrap. */
> - bdp = fec_enet_get_prevdesc(bdp, fep->bufdesc_ex);
> + bdp = pre_bd(bdp, fep->rx_bd_base, fep->rx_ring_size,
> +fep->bufdesc_ex);
> bdp->cbd_sc |= BD_SC_WRAP;
>
> bdp = fep->tx_bd_base;
> - for (i = 0; i < TX_RING_SIZE; i++) {
> + for (i = 0; i < fep->tx_ring_size; i++) {
> fep->tx_bounce[i] = kmalloc(FEC_ENET_TX_FRSIZE, GFP_KERNEL);
>
> bdp->cbd_sc = 0;
> @@ -1723,11 +1725,11 @@ static int fec_enet_alloc_buffers(struct net_device
> *ndev)
> ebdp->cbd_esc = BD_ENET_TX_INT;
> }
>
> - bdp = fec_enet_get_nextdesc(bdp, fep->bufdesc_ex);
> + bdp = next_bd(bdp, fep->tx_bd_base, fep->tx_ring_size,
> +fep->bufdesc_ex);
> }
>
> /* Set the last buffer to wrap. */
> - bdp = fec_enet_get_prevdesc(bdp, fep->bufdesc_ex);
> + bdp = pre_bd(bdp, fep->tx_bd_base, fep->tx_ring_size,
> +fep->bufdesc_ex);
> bdp->cbd_sc |= BD_SC_WRAP;
>
> return 0;
> @@ -1967,13 +1969,17 @@ static int fec_enet_init(struct net_device *ndev)
> /* Get the Ethernet address */
> fec_get_mac(ndev);
>
> + /* init the tx & rx ring size */
> + fep->tx_ring_size = TX_RING_SIZE;
> + fep->rx_ring_size = RX_RING_SIZE;
> +
> /* Set receive and transmit descriptor base. */
> fep->rx_bd_base = cbd_base;
> if (fep->bufdesc_ex)
> fep->tx_bd_base = (struct bufdesc *)
> - (((struct bufdesc_ex *)cbd_base) + RX_RING_SIZE);
> + (((struct bufdesc_ex *)cbd_base) + fep->rx_ring_size);
> else
> - fep->tx_bd_base = cbd_base + RX_RING_SIZE;
> + fep->tx_bd_base = cbd_base + fep->rx_ring_size;
>
> /* The FEC Ethernet specific entries in the device structure */
> ndev->watchdog_timeo = TX_TIMEOUT;
> --
> 1.7.1
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH] net: fec: fix the error to get the previous BD entry
@ 2013-09-02 11:35 Fugang Duan
2013-09-02 14:30 ` Li Frank-B20596
0 siblings, 1 reply; 21+ messages in thread
From: Fugang Duan @ 2013-09-02 11:35 UTC (permalink / raw)
To: b20596, davem; +Cc: netdev, bhutchings, stephen
Bug: error to get the previous BD entry. When the current BD
is the first BD, the previous BD entry must be the last BD,
not "bdp - 1" in current logic.
V2:
Add tx_ring_size and rx_ring_size to struct fec_enet_private.
Replace api fec_enet_get_nextdesc() with next_bd().
Replace api fec_enet_get_prevdesc() with pre_bd().
Move all ring size check logic to next_bd() and pre_bd, which
simplifies the code redundancy.
V1:
Add BD ring size check to get the previous BD entry in correctly.
Signed-off-by: Fugang Duan <B38611@freescale.com>
---
drivers/net/ethernet/freescale/fec.h | 3 +
drivers/net/ethernet/freescale/fec_main.c | 94 +++++++++++++++-------------
2 files changed, 53 insertions(+), 44 deletions(-)
diff --git a/drivers/net/ethernet/freescale/fec.h b/drivers/net/ethernet/freescale/fec.h
index ae23600..0120217 100644
--- a/drivers/net/ethernet/freescale/fec.h
+++ b/drivers/net/ethernet/freescale/fec.h
@@ -296,6 +296,9 @@ struct fec_enet_private {
/* The ring entries to be free()ed */
struct bufdesc *dirty_tx;
+ unsigned short tx_ring_size;
+ unsigned short rx_ring_size;
+
struct platform_device *pdev;
int opened;
diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index 4ea1555..9894dd3 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -239,22 +239,35 @@ MODULE_PARM_DESC(macaddr, "FEC Ethernet MAC address");
static int mii_cnt;
-static struct bufdesc *fec_enet_get_nextdesc(struct bufdesc *bdp, int is_ex)
+static inline
+struct bufdesc *next_bd(struct bufdesc *bdp, struct bufdesc *base,
+ int ring_size, int is_ex)
{
- struct bufdesc_ex *ex = (struct bufdesc_ex *)bdp;
+ struct bufdesc *new_bd = bdp + 1;
+ struct bufdesc_ex *ex_new_bd = (struct bufdesc_ex *)bdp + 1;
+ struct bufdesc_ex *ex_base = (struct bufdesc_ex *)base;
+
if (is_ex)
- return (struct bufdesc *)(ex + 1);
+ return (struct bufdesc *)((ex_new_bd >= (ex_base + ring_size)) ?
+ (ex_new_bd - ring_size) : ex_new_bd);
else
- return bdp + 1;
+ return (new_bd >= (base + ring_size)) ?
+ (new_bd - ring_size) : new_bd;
}
-static struct bufdesc *fec_enet_get_prevdesc(struct bufdesc *bdp, int is_ex)
+static inline
+struct bufdesc *pre_bd(struct bufdesc *bdp, struct bufdesc *base,
+ int ring_size, int is_ex)
{
- struct bufdesc_ex *ex = (struct bufdesc_ex *)bdp;
+ struct bufdesc *new_bd = bdp - 1;
+ struct bufdesc_ex *ex_new_bd = (struct bufdesc_ex *)bdp - 1;
+ struct bufdesc_ex *ex_base = (struct bufdesc_ex *)base;
+
if (is_ex)
- return (struct bufdesc *)(ex - 1);
+ return (struct bufdesc *)((ex_new_bd < ex_base) ?
+ (ex_new_bd + ring_size) : ex_new_bd);
else
- return bdp - 1;
+ return (new_bd < base) ? (new_bd + ring_size) : new_bd;
}
static void *swap_buffer(void *bufaddr, int len)
@@ -380,7 +393,7 @@ fec_enet_start_xmit(struct sk_buff *skb, struct net_device *ndev)
}
}
- bdp_pre = fec_enet_get_prevdesc(bdp, fep->bufdesc_ex);
+ bdp_pre = pre_bd(bdp, fep->tx_bd_base, fep->tx_ring_size, fep->bufdesc_ex);
if ((id_entry->driver_data & FEC_QUIRK_ERR006358) &&
!(bdp_pre->cbd_sc & BD_ENET_TX_READY)) {
fep->delay_work.trig_tx = true;
@@ -389,10 +402,7 @@ fec_enet_start_xmit(struct sk_buff *skb, struct net_device *ndev)
}
/* If this was the last BD in the ring, start at the beginning again. */
- if (status & BD_ENET_TX_WRAP)
- bdp = fep->tx_bd_base;
- else
- bdp = fec_enet_get_nextdesc(bdp, fep->bufdesc_ex);
+ bdp = next_bd(bdp, fep->tx_bd_base, fep->tx_ring_size, fep->bufdesc_ex);
fep->cur_tx = bdp;
@@ -417,18 +427,18 @@ static void fec_enet_bd_init(struct net_device *dev)
/* Initialize the receive buffer descriptors. */
bdp = fep->rx_bd_base;
- for (i = 0; i < RX_RING_SIZE; i++) {
+ for (i = 0; i < fep->rx_ring_size; i++) {
/* Initialize the BD for every fragment in the page. */
if (bdp->cbd_bufaddr)
bdp->cbd_sc = BD_ENET_RX_EMPTY;
else
bdp->cbd_sc = 0;
- bdp = fec_enet_get_nextdesc(bdp, fep->bufdesc_ex);
+ bdp = next_bd(bdp, fep->rx_bd_base, fep->rx_ring_size, fep->bufdesc_ex);
}
/* Set the last buffer to wrap */
- bdp = fec_enet_get_prevdesc(bdp, fep->bufdesc_ex);
+ bdp = pre_bd(bdp, fep->rx_bd_base, fep->rx_ring_size, fep->bufdesc_ex);
bdp->cbd_sc |= BD_SC_WRAP;
fep->cur_rx = fep->rx_bd_base;
@@ -436,7 +446,7 @@ static void fec_enet_bd_init(struct net_device *dev)
/* ...and the same for transmit */
bdp = fep->tx_bd_base;
fep->cur_tx = bdp;
- for (i = 0; i < TX_RING_SIZE; i++) {
+ for (i = 0; i < fep->tx_ring_size; i++) {
/* Initialize the BD for every fragment in the page. */
bdp->cbd_sc = 0;
@@ -445,11 +455,11 @@ static void fec_enet_bd_init(struct net_device *dev)
fep->tx_skbuff[i] = NULL;
}
bdp->cbd_bufaddr = 0;
- bdp = fec_enet_get_nextdesc(bdp, fep->bufdesc_ex);
+ bdp = next_bd(bdp, fep->tx_bd_base, fep->tx_ring_size, fep->bufdesc_ex);
}
/* Set the last buffer to wrap */
- bdp = fec_enet_get_prevdesc(bdp, fep->bufdesc_ex);
+ bdp = pre_bd(bdp, fep->tx_bd_base, fep->tx_ring_size, fep->bufdesc_ex);
bdp->cbd_sc |= BD_SC_WRAP;
fep->dirty_tx = bdp;
}
@@ -510,10 +520,10 @@ fec_restart(struct net_device *ndev, int duplex)
writel(fep->bd_dma, fep->hwp + FEC_R_DES_START);
if (fep->bufdesc_ex)
writel((unsigned long)fep->bd_dma + sizeof(struct bufdesc_ex)
- * RX_RING_SIZE, fep->hwp + FEC_X_DES_START);
+ * fep->rx_ring_size, fep->hwp + FEC_X_DES_START);
else
writel((unsigned long)fep->bd_dma + sizeof(struct bufdesc)
- * RX_RING_SIZE, fep->hwp + FEC_X_DES_START);
+ * fep->rx_ring_size, fep->hwp + FEC_X_DES_START);
for (i = 0; i <= TX_RING_MOD_MASK; i++) {
@@ -727,10 +737,7 @@ fec_enet_tx(struct net_device *ndev)
bdp = fep->dirty_tx;
/* get next bdp of dirty_tx */
- if (bdp->cbd_sc & BD_ENET_TX_WRAP)
- bdp = fep->tx_bd_base;
- else
- bdp = fec_enet_get_nextdesc(bdp, fep->bufdesc_ex);
+ bdp = next_bd(bdp, fep->tx_bd_base, fep->tx_ring_size, fep->bufdesc_ex);
while (((status = bdp->cbd_sc) & BD_ENET_TX_READY) == 0) {
@@ -800,10 +807,7 @@ fec_enet_tx(struct net_device *ndev)
fep->dirty_tx = bdp;
/* Update pointer to next buffer descriptor to be transmitted */
- if (status & BD_ENET_TX_WRAP)
- bdp = fep->tx_bd_base;
- else
- bdp = fec_enet_get_nextdesc(bdp, fep->bufdesc_ex);
+ bdp = next_bd(bdp, fep->tx_bd_base, fep->tx_ring_size, fep->bufdesc_ex);
/* Since we have freed up a buffer, the ring is no longer full
*/
@@ -994,10 +998,8 @@ rx_processing_done:
}
/* Update BD pointer to next entry */
- if (status & BD_ENET_RX_WRAP)
- bdp = fep->rx_bd_base;
- else
- bdp = fec_enet_get_nextdesc(bdp, fep->bufdesc_ex);
+ bdp = next_bd(bdp, fep->rx_bd_base, fep->rx_ring_size, fep->bufdesc_ex);
+
/* Doing this here will keep the FEC running while we process
* incoming frames. On a heavily loaded network, we should be
* able to keep up at the expense of system resources.
@@ -1663,7 +1665,7 @@ static void fec_enet_free_buffers(struct net_device *ndev)
struct bufdesc *bdp;
bdp = fep->rx_bd_base;
- for (i = 0; i < RX_RING_SIZE; i++) {
+ for (i = 0; i < fep->rx_ring_size; i++) {
skb = fep->rx_skbuff[i];
if (bdp->cbd_bufaddr)
@@ -1671,11 +1673,11 @@ static void fec_enet_free_buffers(struct net_device *ndev)
FEC_ENET_RX_FRSIZE, DMA_FROM_DEVICE);
if (skb)
dev_kfree_skb(skb);
- bdp = fec_enet_get_nextdesc(bdp, fep->bufdesc_ex);
+ bdp = next_bd(bdp, fep->rx_bd_base, fep->rx_ring_size, fep->bufdesc_ex);
}
bdp = fep->tx_bd_base;
- for (i = 0; i < TX_RING_SIZE; i++)
+ for (i = 0; i < fep->tx_ring_size; i++)
kfree(fep->tx_bounce[i]);
}
@@ -1687,7 +1689,7 @@ static int fec_enet_alloc_buffers(struct net_device *ndev)
struct bufdesc *bdp;
bdp = fep->rx_bd_base;
- for (i = 0; i < RX_RING_SIZE; i++) {
+ for (i = 0; i < fep->rx_ring_size; i++) {
skb = netdev_alloc_skb(ndev, FEC_ENET_RX_FRSIZE);
if (!skb) {
fec_enet_free_buffers(ndev);
@@ -1704,15 +1706,15 @@ static int fec_enet_alloc_buffers(struct net_device *ndev)
ebdp->cbd_esc = BD_ENET_RX_INT;
}
- bdp = fec_enet_get_nextdesc(bdp, fep->bufdesc_ex);
+ bdp = next_bd(bdp, fep->rx_bd_base, fep->rx_ring_size, fep->bufdesc_ex);
}
/* Set the last buffer to wrap. */
- bdp = fec_enet_get_prevdesc(bdp, fep->bufdesc_ex);
+ bdp = pre_bd(bdp, fep->rx_bd_base, fep->rx_ring_size, fep->bufdesc_ex);
bdp->cbd_sc |= BD_SC_WRAP;
bdp = fep->tx_bd_base;
- for (i = 0; i < TX_RING_SIZE; i++) {
+ for (i = 0; i < fep->tx_ring_size; i++) {
fep->tx_bounce[i] = kmalloc(FEC_ENET_TX_FRSIZE, GFP_KERNEL);
bdp->cbd_sc = 0;
@@ -1723,11 +1725,11 @@ static int fec_enet_alloc_buffers(struct net_device *ndev)
ebdp->cbd_esc = BD_ENET_TX_INT;
}
- bdp = fec_enet_get_nextdesc(bdp, fep->bufdesc_ex);
+ bdp = next_bd(bdp, fep->tx_bd_base, fep->tx_ring_size, fep->bufdesc_ex);
}
/* Set the last buffer to wrap. */
- bdp = fec_enet_get_prevdesc(bdp, fep->bufdesc_ex);
+ bdp = pre_bd(bdp, fep->tx_bd_base, fep->tx_ring_size, fep->bufdesc_ex);
bdp->cbd_sc |= BD_SC_WRAP;
return 0;
@@ -1967,13 +1969,17 @@ static int fec_enet_init(struct net_device *ndev)
/* Get the Ethernet address */
fec_get_mac(ndev);
+ /* init the tx & rx ring size */
+ fep->tx_ring_size = TX_RING_SIZE;
+ fep->rx_ring_size = RX_RING_SIZE;
+
/* Set receive and transmit descriptor base. */
fep->rx_bd_base = cbd_base;
if (fep->bufdesc_ex)
fep->tx_bd_base = (struct bufdesc *)
- (((struct bufdesc_ex *)cbd_base) + RX_RING_SIZE);
+ (((struct bufdesc_ex *)cbd_base) + fep->rx_ring_size);
else
- fep->tx_bd_base = cbd_base + RX_RING_SIZE;
+ fep->tx_bd_base = cbd_base + fep->rx_ring_size;
/* The FEC Ethernet specific entries in the device structure */
ndev->watchdog_timeo = TX_TIMEOUT;
--
1.7.1
^ permalink raw reply related [flat|nested] 21+ messages in thread
* RE: [PATCH] net: fec: fix the error to get the previous BD entry
2013-08-22 10:40 Fugang Duan
@ 2013-08-22 14:14 ` Li Frank-B20596
0 siblings, 0 replies; 21+ messages in thread
From: Li Frank-B20596 @ 2013-08-22 14:14 UTC (permalink / raw)
To: Duan Fugang-B38611, Zhou Luwei-B45643, davem
Cc: netdev, shawn.guo, bhutchings, Estevam Fabio-R49496, stephen
>
> - bdp_pre = fec_enet_get_prevdesc(bdp, fep->bufdesc_ex);
> + if (bdp == fep->tx_bd_base)
> + bdp_pre = bdp + TX_RING_SIZE - 1;
You need consider both extended BD format and legacy format.
The size of BD is difference, you can NOT directly you bdp+TX_RING_SIZE -1
Best regards
Frank Li
> + else
> + bdp_pre = fec_enet_get_prevdesc(bdp, fep->bufdesc_ex);
> if ((id_entry->driver_data & FEC_QUIRK_ERR006358) &&
> !(bdp_pre->cbd_sc & BD_ENET_TX_READY)) {
> fep->delay_work.trig_tx = true;
> --
> 1.7.1
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH] net: fec: fix the error to get the previous BD entry
@ 2013-08-22 10:40 Fugang Duan
2013-08-22 14:14 ` Li Frank-B20596
0 siblings, 1 reply; 21+ messages in thread
From: Fugang Duan @ 2013-08-22 10:40 UTC (permalink / raw)
To: b20596, b45643, davem; +Cc: netdev, shawn.guo, bhutchings, R49496, stephen
When the current BD is the first BD, the previous BD entry
must be the last BD, not "bdp - 1".
Add BD entry check to get the previous BD entry correctly.
Signed-off-by: Fugang Duan <B38611@freescale.com>
---
drivers/net/ethernet/freescale/fec_main.c | 5 ++++-
1 files changed, 4 insertions(+), 1 deletions(-)
diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index 4ea1555..4349a9e 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -380,7 +380,10 @@ fec_enet_start_xmit(struct sk_buff *skb, struct net_device *ndev)
}
}
- bdp_pre = fec_enet_get_prevdesc(bdp, fep->bufdesc_ex);
+ if (bdp == fep->tx_bd_base)
+ bdp_pre = bdp + TX_RING_SIZE - 1;
+ else
+ bdp_pre = fec_enet_get_prevdesc(bdp, fep->bufdesc_ex);
if ((id_entry->driver_data & FEC_QUIRK_ERR006358) &&
!(bdp_pre->cbd_sc & BD_ENET_TX_READY)) {
fep->delay_work.trig_tx = true;
--
1.7.1
^ permalink raw reply related [flat|nested] 21+ messages in thread
end of thread, other threads:[~2013-09-04 18:13 UTC | newest]
Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-08-30 4:35 [PATCH] net: fec: fix the error to get the previous BD entry Fugang Duan
2013-08-30 21:59 ` David Miller
2013-09-02 1:37 ` Duan Fugang-B38611
2013-09-02 2:09 ` Li Frank-B20596
2013-09-02 2:23 ` Duan Fugang-B38611
2013-09-02 2:25 ` Li Frank-B20596
-- strict thread matches above, loose matches on Subject: below --
2013-09-03 2:41 Fugang Duan
2013-09-03 3:00 ` Li Frank-B20596
2013-09-03 3:35 ` David Miller
2013-09-03 15:10 ` Li Frank-B20596
2013-09-03 15:14 ` Fabio Estevam
2013-09-03 15:20 ` Li Frank-B20596
2013-09-04 18:13 ` David Miller
2013-09-03 2:18 Fugang Duan
2013-09-03 2:33 ` Li Frank-B20596
2013-09-03 2:49 ` Duan Fugang-B38611
2013-09-02 11:35 Fugang Duan
2013-09-02 14:30 ` Li Frank-B20596
2013-09-03 1:39 ` Duan Fugang-B38611
2013-08-22 10:40 Fugang Duan
2013-08-22 14:14 ` Li Frank-B20596
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).