Re: [PATCH net-next 1/1] net: fec: clear receive interrupts before processing a packet
Hi Andy, can you resubmit it, adding also my Reported-by: Philippe De Muyter and explaining that it also prevents a complete rx blockage failure ? Philippe On Wed, Sep 02, 2015 at 11:40:15AM +0200, Philippe De Muyter wrote: > On Wed, Sep 02, 2015 at 05:24:14PM +0800, Fugang Duan wrote: > > From: Russell King > > > > The patch just to re-submit the patch "db3421c114cfa6326" because the > > patch "4d494cdc92b3b9a0" remove the change. > > I think you should mention also the titles of the commits. > > And maybe send it also to stable. > > > > Clear any pending receive interrupt before we process a pending packet. > > This helps to avoid any spurious interrupts being raised after we have > > fully cleaned the receive ring, while still allowing an interrupt to be > > raised if we receive another packet. > > > > The position of this is critical: we must do this prior to reading the > > next packet status to avoid potentially dropping an interrupt when a > > packet is still pending. > > > > Acked-by: Fugang Duan > > Signed-off-by: Russell King > > --- > > drivers/net/ethernet/freescale/fec_main.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/drivers/net/ethernet/freescale/fec_main.c > > b/drivers/net/ethernet/freescale/fec_main.c > > index 1f89c59..6bed0ff 100644 > > --- a/drivers/net/ethernet/freescale/fec_main.c > > +++ b/drivers/net/ethernet/freescale/fec_main.c > > @@ -1400,6 +1400,7 @@ fec_enet_rx_queue(struct net_device *ndev, int > > budget, u16 queue_id) > > if ((status & BD_ENET_RX_LAST) == 0) > > netdev_err(ndev, "rcv is not +last\n"); > > > Could a comment be added here to avoid another future removal ? > > > + writel(FEC_ENET_RXF, fep->hwp + FEC_IEVENT); > > > > /* Check for errors. */ > > if (status & (BD_ENET_RX_LG | BD_ENET_RX_SH | BD_ENET_RX_NO | > > -- > > 1.9.1 > > Philippe -- Philippe De Muyter +32 2 6101532 Macq SA rue de l'Aeronef 2 B-1140 Bruxelles -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH net-next 1/1] net: fec: clear receive interrupts before processing a packet
On Wed, Sep 02, 2015 at 05:24:14PM +0800, Fugang Duan wrote: > From: Russell King > > The patch just to re-submit the patch "db3421c114cfa6326" because the > patch "4d494cdc92b3b9a0" remove the change. I think you should mention also the titles of the commits. And maybe send it also to stable. > > Clear any pending receive interrupt before we process a pending packet. > This helps to avoid any spurious interrupts being raised after we have > fully cleaned the receive ring, while still allowing an interrupt to be > raised if we receive another packet. > > The position of this is critical: we must do this prior to reading the > next packet status to avoid potentially dropping an interrupt when a > packet is still pending. > > Acked-by: Fugang Duan > Signed-off-by: Russell King > --- > drivers/net/ethernet/freescale/fec_main.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/net/ethernet/freescale/fec_main.c > b/drivers/net/ethernet/freescale/fec_main.c > index 1f89c59..6bed0ff 100644 > --- a/drivers/net/ethernet/freescale/fec_main.c > +++ b/drivers/net/ethernet/freescale/fec_main.c > @@ -1400,6 +1400,7 @@ fec_enet_rx_queue(struct net_device *ndev, int budget, > u16 queue_id) > if ((status & BD_ENET_RX_LAST) == 0) > netdev_err(ndev, "rcv is not +last\n"); > Could a comment be added here to avoid another future removal ? > + writel(FEC_ENET_RXF, fep->hwp + FEC_IEVENT); > > /* Check for errors. */ > if (status & (BD_ENET_RX_LG | BD_ENET_RX_SH | BD_ENET_RX_NO | > -- > 1.9.1 Philippe -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] 7990 : Various fixes and cleanups
On Tue, Jul 10, 2007 at 12:38:45PM -0400, Jeff Garzik wrote: > Philippe De Muyter wrote: > >This patch > >- avoids 7990 blocking when no tx buffer is available, > [...] > >diff -r 6c0a10cc415a drivers/net/7990.c > >--- a/drivers/net/7990.c Thu Jul 5 16:10:16 2007 -0700 > >+++ b/drivers/net/7990.c Fri Jul 6 11:27:20 2007 +0200 > [...] > >@@ -541,9 +546,6 @@ int lance_start_xmit (struct sk_buff *sk > > static int outs; > > unsigned long flags; > > > >-if (!TX_BUFFS_AVAIL) > >-return -1; > >- > > netif_stop_queue (dev); > > > > skblen = skb->len; > > > NAK > > It "avoids" by removing an overrun check in hard_start_xmit that should > not be removed. Yup, sorry. The real fact is still that this prevents/fixes lance/driver blocking on my board, while the tx_timeout mechanism does not succeed at that, and that on my board the driver is blocked when we return -1 on !TX_BUFFS_AVAIL. I'll investigate why. Philippe PS : did you apply the rest of the patch ? -- Philippe De Muyter phdm at macqel dot be Tel +32 27029044 Macq Electronique SA rue de l'Aeronef 2 B-1140 Bruxelles Fax +32 27029077 - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] make all initialized struct seq_operations const
Hi all
Make all initialized struct seq_operations in net/ const
Signed-off-by: Philippe De Muyter <[EMAIL PROTECTED]>
diff -r 6c0a10cc415a net/802/tr.c
--- a/net/802/tr.c Thu Jul 5 16:10:16 2007 -0700
+++ b/net/802/tr.c Fri Jul 6 15:17:53 2007 +0200
@@ -567,7 +567,7 @@ static int rif_seq_show(struct seq_file
}
-static struct seq_operations rif_seq_ops = {
+static const struct seq_operations rif_seq_ops = {
.start = rif_seq_start,
.next = rif_seq_next,
.stop = rif_seq_stop,
diff -r 6c0a10cc415a net/8021q/vlanproc.c
--- a/net/8021q/vlanproc.c Thu Jul 5 16:10:16 2007 -0700
+++ b/net/8021q/vlanproc.c Fri Jul 6 15:17:53 2007 +0200
@@ -69,7 +69,7 @@ static const char name_conf[] = "config
* Generic /proc/net/vlan/ file and inode operations
*/
-static struct seq_operations vlan_seq_ops = {
+static const struct seq_operations vlan_seq_ops = {
.start = vlan_seq_start,
.next = vlan_seq_next,
.stop = vlan_seq_stop,
diff -r 6c0a10cc415a net/appletalk/aarp.c
--- a/net/appletalk/aarp.c Thu Jul 5 16:10:16 2007 -0700
+++ b/net/appletalk/aarp.c Fri Jul 6 15:17:53 2007 +0200
@@ -1024,7 +1024,7 @@ static int aarp_seq_show(struct seq_file
return 0;
}
-static struct seq_operations aarp_seq_ops = {
+static const struct seq_operations aarp_seq_ops = {
.start = aarp_seq_start,
.next = aarp_seq_next,
.stop = aarp_seq_stop,
diff -r 6c0a10cc415a net/appletalk/atalk_proc.c
--- a/net/appletalk/atalk_proc.cThu Jul 5 16:10:16 2007 -0700
+++ b/net/appletalk/atalk_proc.cFri Jul 6 15:17:53 2007 +0200
@@ -204,21 +204,21 @@ out:
return 0;
}
-static struct seq_operations atalk_seq_interface_ops = {
+static const struct seq_operations atalk_seq_interface_ops = {
.start = atalk_seq_interface_start,
.next = atalk_seq_interface_next,
.stop = atalk_seq_interface_stop,
.show = atalk_seq_interface_show,
};
-static struct seq_operations atalk_seq_route_ops = {
+static const struct seq_operations atalk_seq_route_ops = {
.start = atalk_seq_route_start,
.next = atalk_seq_route_next,
.stop = atalk_seq_route_stop,
.show = atalk_seq_route_show,
};
-static struct seq_operations atalk_seq_socket_ops = {
+static const struct seq_operations atalk_seq_socket_ops = {
.start = atalk_seq_socket_start,
.next = atalk_seq_socket_next,
.stop = atalk_seq_socket_stop,
diff -r 6c0a10cc415a net/atm/br2684.c
--- a/net/atm/br2684.c Thu Jul 5 16:10:16 2007 -0700
+++ b/net/atm/br2684.c Fri Jul 6 15:17:53 2007 +0200
@@ -772,7 +772,7 @@ static int br2684_seq_show(struct seq_fi
return 0;
}
-static struct seq_operations br2684_seq_ops = {
+static const struct seq_operations br2684_seq_ops = {
.start = br2684_seq_start,
.next = br2684_seq_next,
.stop = br2684_seq_stop,
diff -r 6c0a10cc415a net/atm/clip.c
--- a/net/atm/clip.cThu Jul 5 16:10:16 2007 -0700
+++ b/net/atm/clip.cFri Jul 6 15:17:53 2007 +0200
@@ -928,7 +928,7 @@ static int clip_seq_show(struct seq_file
return 0;
}
-static struct seq_operations arp_seq_ops = {
+static const struct seq_operations arp_seq_ops = {
.start = clip_seq_start,
.next = neigh_seq_next,
.stop = neigh_seq_stop,
diff -r 6c0a10cc415a net/atm/lec.c
--- a/net/atm/lec.c Thu Jul 5 16:10:16 2007 -0700
+++ b/net/atm/lec.c Fri Jul 6 15:17:53 2007 +0200
@@ -1174,7 +1174,7 @@ static int lec_seq_show(struct seq_file
return 0;
}
-static struct seq_operations lec_seq_ops = {
+static const struct seq_operations lec_seq_ops = {
.start = lec_seq_start,
.next = lec_seq_next,
.stop = lec_seq_stop,
diff -r 6c0a10cc415a net/atm/mpoa_proc.c
--- a/net/atm/mpoa_proc.c Thu Jul 5 16:10:16 2007 -0700
+++ b/net/atm/mpoa_proc.c Fri Jul 6 15:17:53 2007 +0200
@@ -177,7 +177,7 @@ static int mpc_show(struct seq_file *m,
return 0;
}
-static struct seq_operations mpc_op = {
+static const struct seq_operations mpc_op = {
.start =mpc_start,
.next = mpc_next,
.stop = mpc_stop,
diff -r 6c0a10cc415a net/atm/proc.c
--- a/net/atm/proc.cThu Jul 5 16:10:16 2007 -0700
+++ b/net/atm/proc.cFri Jul 6 15:17:53 2007 +0200
@@ -260,7 +260,7 @@ static int atm_dev_seq_show(struct seq_f
return 0;
}
-static struct seq_operations atm_dev_seq_ops = {
+static const struct seq_operations atm_dev_seq_ops = {
.start = atm_dev_seq_start,
.next = atm_dev_seq_next,
.stop = atm_dev_seq_stop,
@@ -295,7 +295,7 @@ static int pvc_seq_show(struct seq_file
return 0;
}
-static struct seq_operations pvc_seq_ops = {
+static const struct seq_operations pvc_seq_ops = {
.start = vcc_seq_start,
.next = vcc_seq_next,
.stop = vcc_seq_stop
Re: [PATCH] net : make netlink_seq_ops const
On Fri, Jul 06, 2007 at 02:52:11PM +0200, Patrick McHardy wrote: > [please send networking patches to netdev] > > Philippe De Muyter wrote: > > Hi all, > > > > Make netlink_seq_ops const > > > Might make more sense to do a big patch for all occurences of this > in net/: > > # grep "^static struct seq_operations" -r net/ | wc -l > 76 OK Philippe - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] 7990 : Various fixes and cleanups
Hi all,
This patch
- avoids 7990 blocking when no tx buffer is available,
- implements tx_bytes statistic that was missing
- sets skb->dev before calling netix_rx()
- improves readability and code efficiency in buffer rings initialisation
- avoids useless memset part for tx packets smaller than ETH_ZLEN
Signed-off-by: Philippe De Muyter <[EMAIL PROTECTED]>
diff -r 6c0a10cc415a drivers/net/7990.c
--- a/drivers/net/7990.cThu Jul 5 16:10:16 2007 -0700
+++ b/drivers/net/7990.cFri Jul 6 11:27:20 2007 +0200
@@ -179,12 +179,14 @@ static void lance_init_ring (struct net_
lp->tx_full = 0;
/* Setup the Tx ring entries */
for (i = 0; i < (1<lance_log_tx_bufs); i++) {
+ volatile struct lance_tx_desc *td;
+td = &ib->btx_ring [i];
leptr = LANCE_ADDR(&aib->tx_buf[i][0]);
-ib->btx_ring [i].tmd0 = leptr;
-ib->btx_ring [i].tmd1_hadr = leptr >> 16;
-ib->btx_ring [i].tmd1_bits = 0;
-ib->btx_ring [i].length= 0xf000; /* The ones required by
tmd2 */
-ib->btx_ring [i].misc = 0;
+td->tmd0 = leptr;
+td->tmd1_hadr = leptr >> 16;
+td->tmd1_bits = 0;
+td->length= 0xf000; /* The ones required by tmd2 */
+td->misc = 0;
if (DEBUG_IRING)
printk ("%d: 0x%8.8x\n", i, leptr);
}
@@ -193,14 +195,16 @@ static void lance_init_ring (struct net_
if (DEBUG_IRING)
printk ("RX rings:\n");
for (i = 0; i < (1<lance_log_rx_bufs); i++) {
+ volatile struct lance_rx_desc *rd;
+rd = &ib->brx_ring [i];
leptr = LANCE_ADDR(&aib->rx_buf[i][0]);
-ib->brx_ring [i].rmd0 = leptr;
-ib->brx_ring [i].rmd1_hadr = leptr >> 16;
-ib->brx_ring [i].rmd1_bits = LE_R1_OWN;
+rd->rmd0 = leptr;
+rd->rmd1_hadr = leptr >> 16;
+rd->rmd1_bits = LE_R1_OWN;
/* 0xf000 == bits that must be one (reserved, presumably) */
-ib->brx_ring [i].length= -RX_BUFF_SIZE | 0xf000;
-ib->brx_ring [i].mblength = 0;
+rd->length= -RX_BUFF_SIZE | 0xf000;
+rd->mblength = 0;
if (DEBUG_IRING)
printk ("%d: 0x%8.8x\n", i, leptr);
}
@@ -331,6 +335,7 @@ static int lance_rx (struct net_device *
return 0;
}
+skb->dev = dev;
skb_reserve (skb, 2); /* 16 byte align */
skb_put (skb, len); /* make room */
eth_copy_and_sum(skb,
@@ -541,9 +546,6 @@ int lance_start_xmit (struct sk_buff *sk
static int outs;
unsigned long flags;
-if (!TX_BUFFS_AVAIL)
-return -1;
-
netif_stop_queue (dev);
skblen = skb->len;
@@ -565,9 +567,11 @@ int lance_start_xmit (struct sk_buff *sk
ib->btx_ring [entry].length = (-len) | 0xf000;
ib->btx_ring [entry].misc = 0;
- if (skb->len < ETH_ZLEN)
- memset((void *)&ib->tx_buf[entry][0], 0, ETH_ZLEN);
skb_copy_from_linear_data(skb, (void *)&ib->tx_buf[entry][0], skblen);
+ if (skblen < ETH_ZLEN)
+ memset((char *)&ib->tx_buf[entry][0] + skblen, 0, ETH_ZLEN -
skblen);
+
+ lp->stats.tx_bytes += skblen;
/* Now, give the packet to the lance */
ib->btx_ring [entry].tmd1_bits = (LE_T1_POK|LE_T1_OWN);
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch 3/8] sundance: remove TxStartThresh and RxEarlyThresh
On Fri, Oct 20, 2006 at 02:42:05PM -0700, [EMAIL PROTECTED] wrote:
> From: Jesse Huang <[EMAIL PROTECTED]>
> For patent issue need to remove TxStartThresh and RxEarlyThresh. This patent
> is cut-through patent. If use this function, Tx will start to transmit after
> few data be move in to Tx FIFO. We are not allow to use those function in
> DFE530/DFE550/DFE580/DL10050/IP100/IP100A. It will decrease a little
> performance.
[...]
> @@ -,6 +1109,7 @@ static irqreturn_t intr_handler(int irq,
> int tx_cnt;
> int tx_status;
> int handled = 0;
> + int i;
What the use here of adding this variable ? Is that actually a part of another
patch ?
>
>
> do {
> @@ -1153,17 +1152,14 @@ static irqreturn_t intr_handler(int irq,
> np->stats.tx_fifo_errors++;
> if (tx_status & 0x02)
> np->stats.tx_window_errors++;
> +
> /*
> ** This reset has been verified on
> ** DFE-580TX boards ! [EMAIL PROTECTED]
> */
> if (tx_status & 0x10) { /* TxUnderrun */
> - unsigned short txthreshold;
> -
> - txthreshold = ioread16 (ioaddr
> + TxStartThresh);
> /* Restart Tx FIFO and
> transmitter */
> sundance_reset(dev,
> (NetworkReset|FIFOReset|TxReset) << 16);
> - iowrite16 (txthreshold, ioaddr
> + TxStartThresh);
I must be dense, but I do not understand how merely preserving the value of
a register across a reset can infringe on any patent
Philippe
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/4] IP100A: Fix TX Pause bug (reset_tx, intr_handler)
Hi Jesse,
On Mon, Sep 18, 2006 at 07:11:29PM +0800, Jesse Huang wrote:
> Dear Philippe:
> (1)Because this is a patent issue, we are not allow to use it again, even it
> is in Data Sheet.
I surmise this is only a concern for icplus as a hardware company.
The sundance driver in Linux is meant to work also with the previous versions
of the chip (Sundance, Kendin, D-Link). If you wish you can make it clear
that those registers have disappeared or have no effect in the icplus 100A
version.
>
> (2)Ok, sorry for this, I will add it back.
Thanks
>
> Should I resent those 4 patches? Or generate this as a new patch?
I do not know about the other patches, but for this one of course you should
Philippe
>
> Thanks very much!
>
> Best Regards,
> Jesse Huang.
>
> - Original Message -
> From: "Philippe De Muyter" <[EMAIL PROTECTED]>
> To: "Jesse Huang" <[EMAIL PROTECTED]>
> Cc:
> Sent: Monday, September 18, 2006 5:41 PM
> Subject: Re: [PATCH 1/4] IP100A: Fix TX Pause bug (reset_tx, intr_handler)
>
>
> On Mon, Sep 18, 2006 at 11:41:09AM +0800, Jesse Huang wrote:
> > Dear Philippe:
> >
> > (1) We are not allow to support register TxStartThresh and, RxEarlyThresh,
> > so
> > we remove it.
>
> Could you develop ?
> - What do you mean by `We are not allow'
> - Is it specific to the IP100A chip ?
>
> Those register are documented in the Sundance Technology ST201 Data Sheet
> and when modified with fine-tuned values, they can have a real positive
> effect on the overall throughput on a loaded system.
>
> >
> > (2) Your consideration is right. But reset_tx is workaround for customer's
> > embedded system, I don't have this
> > enviroment now. I can't sure it will work fine if I removed this.
>
> On DFE-580TX boards, the reset_tx way did not work. The ports remained
> blocked until a power-cycle. I do not know if the TxUnderrun problem ever
> happened with earlier (one port) boards, so I doubt that the reset_tx way
> ever worked. Is was even commented as not being tested. On DFE-580TX
> boards, the current way has been verified by me and others to work, so
> please do not break it.
>
> Best regards
>
> Philippe
>
> >
> > Thanks you very mutch.
> >
> > Best Regards,
> > Jesse Huang.
> >
> > - Original Message -
> > From: "Philippe De Muyter" <[EMAIL PROTECTED]>
> > To: "Jesse Huang" <[EMAIL PROTECTED]>
> > Cc:
> > Sent: Friday, September 15, 2006 7:44 PM
> > Subject: Re: [PATCH 1/4] IP100A: Fix TX Pause bug (reset_tx, intr_handler)
> >
> >
> > On Thu, Sep 14, 2006 at 12:58:30AM +, Jesse Huang wrote:
> > [...]
> > > @@ -262,8 +262,6 @@ enum alta_offsets {
> > > ASICCtrl = 0x30,
> > > EEData = 0x34,
> > > EECtrl = 0x36,
> > > - TxStartThresh = 0x3c,
> > > - RxEarlyThresh = 0x3e,
> >
> > Why ?
> >
> > > FlashAddr = 0x40,
> > > FlashData = 0x44,
> > > TxStatus = 0x46,
> > [...]
> > > @@ -1156,29 +1160,29 @@ static irqreturn_t intr_handler(int irq,
> > > np->stats.tx_fifo_errors++;
> > > if (tx_status & 0x02)
> > > np->stats.tx_window_errors++;
> > > - /*
> > > - ** This reset has been verified on
> > > - ** DFE-580TX boards ! [EMAIL PROTECTED]
> > > - */
> > > - if (tx_status & 0x10) { /* TxUnderrun */
> > > - unsigned short txthreshold;
> > > -
> > > - txthreshold = ioread16 (ioaddr + TxStartThresh);
> > > - /* Restart Tx FIFO and transmitter */
> > > - sundance_reset(dev, (NetworkReset|FIFOReset|TxReset) << 16);
> > > - iowrite16 (txthreshold, ioaddr + TxStartThresh);
> > > - /* No need to reset the Tx pointer here */
> > > +
> > > + /* FIFO ERROR need to be reset tx */
> > > + if (tx_status & 0x10) { /* Reset the Tx. */
> > > + spin_lock(&np->lock);
> > > + reset_tx(dev);
> > > + spin_unlock(&np->lock);
> > > + }
> >
> > Just as the comments say, on DFE-580TX 4 port boards, where it is easy to
> > reproduce TxUnderrun problems, just resetting on the chip the Tx FIFO and
> > transmitter is enough.
> > There is no need to call reset_tx, which discards all pending messages and
> > frees all the skb's. It is also not necessary to reload the Tx pointer.
> >
> > Is it different with newer versions of the chip ?
> >
> > Philippe
> >
>
> --
>
--
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/4] IP100A: Fix TX Pause bug (reset_tx, intr_handler)
On Mon, Sep 18, 2006 at 11:41:09AM +0800, Jesse Huang wrote:
> Dear Philippe:
>
> (1) We are not allow to support register TxStartThresh and, RxEarlyThresh,
> so
> we remove it.
Could you develop ?
- What do you mean by `We are not allow'
- Is it specific to the IP100A chip ?
Those register are documented in the Sundance Technology ST201 Data Sheet
and when modified with fine-tuned values, they can have a real positive
effect on the overall throughput on a loaded system.
>
> (2) Your consideration is right. But reset_tx is workaround for customer's
> embedded system, I don't have this
> enviroment now. I can't sure it will work fine if I removed this.
On DFE-580TX boards, the reset_tx way did not work. The ports remained
blocked until a power-cycle. I do not know if the TxUnderrun problem ever
happened with earlier (one port) boards, so I doubt that the reset_tx way
ever worked. Is was even commented as not being tested. On DFE-580TX
boards, the current way has been verified by me and others to work, so
please do not break it.
Best regards
Philippe
>
> Thanks you very mutch.
>
> Best Regards,
> Jesse Huang.
>
> - Original Message -
> From: "Philippe De Muyter" <[EMAIL PROTECTED]>
> To: "Jesse Huang" <[EMAIL PROTECTED]>
> Cc:
> Sent: Friday, September 15, 2006 7:44 PM
> Subject: Re: [PATCH 1/4] IP100A: Fix TX Pause bug (reset_tx, intr_handler)
>
>
> On Thu, Sep 14, 2006 at 12:58:30AM +, Jesse Huang wrote:
> [...]
> > @@ -262,8 +262,6 @@ enum alta_offsets {
> > ASICCtrl = 0x30,
> > EEData = 0x34,
> > EECtrl = 0x36,
> > - TxStartThresh = 0x3c,
> > - RxEarlyThresh = 0x3e,
>
> Why ?
>
> > FlashAddr = 0x40,
> > FlashData = 0x44,
> > TxStatus = 0x46,
> [...]
> > @@ -1156,29 +1160,29 @@ static irqreturn_t intr_handler(int irq,
> > np->stats.tx_fifo_errors++;
> > if (tx_status & 0x02)
> > np->stats.tx_window_errors++;
> > - /*
> > - ** This reset has been verified on
> > - ** DFE-580TX boards ! [EMAIL PROTECTED]
> > - */
> > - if (tx_status & 0x10) { /* TxUnderrun */
> > - unsigned short txthreshold;
> > -
> > - txthreshold = ioread16 (ioaddr + TxStartThresh);
> > - /* Restart Tx FIFO and transmitter */
> > - sundance_reset(dev, (NetworkReset|FIFOReset|TxReset) << 16);
> > - iowrite16 (txthreshold, ioaddr + TxStartThresh);
> > - /* No need to reset the Tx pointer here */
> > +
> > + /* FIFO ERROR need to be reset tx */
> > + if (tx_status & 0x10) { /* Reset the Tx. */
> > + spin_lock(&np->lock);
> > + reset_tx(dev);
> > + spin_unlock(&np->lock);
> > + }
>
> Just as the comments say, on DFE-580TX 4 port boards, where it is easy to
> reproduce TxUnderrun problems, just resetting on the chip the Tx FIFO and
> transmitter is enough.
> There is no need to call reset_tx, which discards all pending messages and
> frees all the skb's. It is also not necessary to reload the Tx pointer.
>
> Is it different with newer versions of the chip ?
>
> Philippe
>
--
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/4] IP100A: Fix TX Pause bug (reset_tx, intr_handler)
On Thu, Sep 14, 2006 at 12:58:30AM +, Jesse Huang wrote:
[...]
> @@ -262,8 +262,6 @@ enum alta_offsets {
> ASICCtrl = 0x30,
> EEData = 0x34,
> EECtrl = 0x36,
> - TxStartThresh = 0x3c,
> - RxEarlyThresh = 0x3e,
Why ?
> FlashAddr = 0x40,
> FlashData = 0x44,
> TxStatus = 0x46,
[...]
> @@ -1156,29 +1160,29 @@ static irqreturn_t intr_handler(int irq,
> np->stats.tx_fifo_errors++;
> if (tx_status & 0x02)
> np->stats.tx_window_errors++;
> - /*
> - ** This reset has been verified on
> - ** DFE-580TX boards ! [EMAIL PROTECTED]
> - */
> - if (tx_status & 0x10) { /* TxUnderrun */
> - unsigned short txthreshold;
> -
> - txthreshold = ioread16 (ioaddr
> + TxStartThresh);
> - /* Restart Tx FIFO and
> transmitter */
> - sundance_reset(dev,
> (NetworkReset|FIFOReset|TxReset) << 16);
> - iowrite16 (txthreshold, ioaddr
> + TxStartThresh);
> - /* No need to reset the Tx
> pointer here */
> +
> + /* FIFO ERROR need to be reset tx */
> + if (tx_status & 0x10) { /* Reset the
> Tx. */
> + spin_lock(&np->lock);
> + reset_tx(dev);
> + spin_unlock(&np->lock);
> + }
Just as the comments say, on DFE-580TX 4 port boards, where it is easy to
reproduce TxUnderrun problems, just resetting on the chip the Tx FIFO and
transmitter is enough.
There is no need to call reset_tx, which discards all pending messages and
frees all the skb's. It is also not necessary to reload the Tx pointer.
Is it different with newer versions of the chip ?
Philippe
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] sundance: small cleanup
This patch uses the sundance_reset function everywhere a reset is done,
and adds a link to the archives of the original mailing list.
Signed-off-by: Philippe De Muyter <[EMAIL PROTECTED]>
diff -r 9062402c439c drivers/net/sundance.c
--- a/drivers/net/sundance.cThu Aug 3 12:35:26 2006 +0700
+++ b/drivers/net/sundance.cThu Aug 3 18:17:31 2006 +0200
@@ -17,6 +17,8 @@
Support and updates available at
http://www.scyld.com/network/sundance.html
[link no longer provides useful info -jgarzik]
+ Archives of the mailing list are still available at
+ http://www.beowulf.org/pipermail/netdrivers/
*/
@@ -646,7 +648,7 @@ static int __devinit sundance_probe1 (st
/* Reset the chip to erase previous misconfiguration. */
if (netif_msg_hw(np))
printk("ASIC Control is %x.\n", ioread32(ioaddr + ASICCtrl));
- iowrite16(0x00ff, ioaddr + ASICCtrl + 2);
+ sundance_reset(dev, 0x00ff << 16);
if (netif_msg_hw(np))
printk("ASIC Control is now %x.\n", ioread32(ioaddr +
ASICCtrl));
@@ -1075,13 +1077,8 @@ reset_tx (struct net_device *dev)
/* Reset tx logic, TxListPtr will be cleaned */
iowrite16 (TxDisable, ioaddr + MACCtrl1);
- iowrite16 (TxReset | DMAReset | FIFOReset | NetworkReset,
- ioaddr + ASICCtrl + 2);
- for (i=50; i > 0; i--) {
- if ((ioread16(ioaddr + ASICCtrl + 2) & ResetBusy) == 0)
- break;
- mdelay(1);
- }
+ sundance_reset(dev, (NetworkReset|FIFOReset|DMAReset|TxReset) << 16);
+
/* free all tx skbuff */
for (i = 0; i < TX_RING_SIZE; i++) {
skb = np->tx_skbuff[i];
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Q: link up/down, netif_carrier_on/off & tx_timeout
Hi all, I work with the fec driver (used with embedded linuxes), and I am not very satisfied of the behaviour regarding link down / link up. So here are my questions : 1. When/where should netif_carrierr_on/off or another similar routine be called ? 2. If the above are called properly, will tx_timeout be called, and when ? Thanks for listening Philippe - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Fwd: [patch 11/15] ppp: handle misaligned accesses]
I wrote :
> I just noticed something at the end of process_input_packet :
> In the normal case, skb is given to the next stage and ap->rpkt is reset,
> but in the error case, skb is kept, ap->rpkt is not reset, so we keep
> the skb with skb->data aligned for one message and we put another one
> into it :)
>
> Could that not be the culprit ?
Based on my previous observation, here is a revised patch, that replaces
the previous one.
This patch avoids ppp-generated kernel crashes on machines where
unaligned accesses are forbidden, by fixing ppp alignment setting
for reused skb's.
Signed-off-by: Philippe De Muyter <[EMAIL PROTECTED]>
--- drivers/net/ppp_async.c 2004/05/07 08:38:32 1.1.1.1
+++ drivers/net/ppp_async.c 2005/08/11 11:21:33
@@ -30,6 +30,7 @@
#include
#include
#include
+#include
#define PPP_VERSION"2.4.2"
@@ -846,7 +847,11 @@ process_input_packet(struct asyncppp *ap
/* frame had an error, remember that, reset SC_TOSS & SC_ESCAPE */
ap->state = SC_PREV_ERROR;
if (skb)
+ {
+ /* make skb appear as freshly allocated */
skb_trim(skb, 0);
+ skb_reserve(skb, - skb_headroom(skb));
+ }
}
/* called when the tty driver has data for us. */
@@ -897,10 +902,18 @@ ppp_async_input(struct asyncppp *ap, con
skb = dev_alloc_skb(ap->mru + PPP_HDRLEN + 2);
if (skb == 0)
goto nomem;
+ ap->rpkt = skb;
+ }
+ if (skb->len == 0) {
/* Try to get the payload 4-byte aligned */
+ /* This should match the
+ ** PPP_ALLSTATIONS/PPP_UI/compressed tests
+ ** in process_input_packet,
+ ** but we do not have enough chars here to
+ ** test buf[1] and buf[2].
+ */
if (buf[0] != PPP_ALLSTATIONS)
skb_reserve(skb, 2 + (buf[0] & 1));
- ap->rpkt = skb;
}
if (n > skb_tailroom(skb)) {
/* packet overflowed MRU */
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Fwd: [patch 11/15] ppp: handle misaligned accesses]
Paul Mackerras wrote : > Philippe De Muyter writes: > > > Actually, that's probably the case I had, but my fix gets the ip adresses > > 4byte aligned in my case : I had verified the address of the saddr field, > > and I needed to shift the buffer by 3, not 1, to get it 4byte aligned. > > Please outline the code flow that leads to that situation. AFAICS we > would only need to shift the buffer by 3 if we had a compressed > (1-byte) protocol number at the original skb->data, implying that the > protocol byte was first. But if the protocol byte was first, then > this code: > > if (buf[0] != PPP_ALLSTATIONS) > skb_reserve(skb, 2 + (buf[0] & 1)); > > at line 893 should have moved skb->data up by 3 bytes already, since a > 1-byte protocol number is always odd. > > If that is not the case then there is something else going on beyond > the data getting misaligned, and I would like to know what it is. I just noticed something at the end of process_input_packet : /* queue the frame to be processed */ skb->cb[0] = ap->state; skb_queue_tail(&ap->rqueue, skb); ap->rpkt = 0; ap->state = 0; return; err: /* frame had an error, remember that, reset SC_TOSS & SC_ESCAPE */ ap->state = SC_PREV_ERROR; if (skb) skb_trim(skb, 0); } In the normal case, skb is given to the next stage and ap->rpkt is reset, but in the error case, skb is kept, ap->rpkt is not reset, so we keep the skb with skb->data aligned for one message and we put another one into it :) Could that not be the culprit ? Philippe > > Paul. > - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Fwd: [patch 11/15] ppp: handle misaligned accesses]
Paul Mackerras wrote :
> Philippe De Muyter writes:
>
> > > This patch seems a bit strange and/or incomplete. Are we trying to
> > > get 2-byte alignment or 4-byte alignment of the payload? It seems
> >
> > Actually, we try to get a 4n+2 alignment for skb->data, to get the
> > ip-addresses
> > field 4bytes aligned.
> > I think the only thing wrong is the old comment that said :
> > /* Try to get the payload 4-byte aligned */
> > and that I did not change.
>
> Yes, the payload is the part after the protocol field, so the comment
> is still correct.
>
> > > that if the protocol field is uncompressed, we don't do anything to
> > > the alignment, but if it is compressed, we do this:
> > >
> > > > /* protocol is compressed */
> > > > - skb_push(skb, 1)[0] = 0;
> > > > + if ((unsigned long)skb->data & 1)
> > > > + skb_push(skb, 1)[0] = 0;
> > > > + else { /* Ditto, but realign the payload to 4-byte
> > > > boundary */
> > > > + short len = skb->len;
> > > > +
> > > > + skb_put(skb, 3);
> > > > + memmove(skb->data + 3, skb->data, len);
> > > > + skb_pull(skb, 2)[0] = 0;
> > >
> > > I'm puzzled that we are not testing ((unsigned long)skb->data & 2) if
> > > we are really trying to achieve 4-byte alignment. In fact, if the
> > > skb->data that we get from dev_alloc_skb is 4-byte aligned to start
> > > with, this will end up with the payload starting at the original
> > > skb->data + 6, i.e. 2-byte aligned but not 4-byte aligned AFAICS.
> > >
> > > Can we assume that dev_alloc_skb will give us a 4-byte aligned
> > > skb->data? If we can then I suggest we change 3 to 1 in the skb_put
> >
> > Are you not forgetting that the alignment of skb->data is changed (by the
> > existing code ! ) :
> > if (buf[0] != PPP_ALLSTATIONS)
> > skb_reserve(skb, 2 + (buf[0] & 1));
>
> No, I'm not forgetting. If we assume that skb->data starts out 4-byte
> aligned, then the only case in which we will have
>
> ((unsigned long)skb->data & 1) == 0
>
> is if we have protocol field compression (and a compressible protocol
> number, i.e. less than 0x100) but not address/control compression
> (which would be a weird combination, but legal). In that case, with
> your patch we will move the protocol byte to the original skb->data+5
> and have the payload at +6.
Actually, that's probably the case I had, but my fix gets the ip adresses
4byte aligned in my case : I had verified the address of the saddr field,
and I needed to shift the buffer by 3, not 1, to get it 4byte aligned.
>
> If there is any possibility that skb->data is not 4-byte aligned when
No, that's not the problem I had. skb->data was always 4-byte aligned
at allocation time.
> the skb is first allocated, I think that we should do something like
>
> if ((unsigned long)skb->data & 3)
> skb_reserve(skb, 4 - ((unsigned long)skb->data & 3));
>
> immediately after allocating it, and then just memmove the stuff up
> one byte (rather than 3) if it isn't aligned as we want.
>
> Paul.
>
Philippe
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Fwd: [patch 11/15] ppp: handle misaligned accesses]
Paul Mackerras wrote :
> Jeff Garzik writes:
>
> > From: "Philippe De Muyter" <[EMAIL PROTECTED]>
> >
> > Avoid ppp-generated kernel crashes on machines where unaligned accesses are
> > forbidden (ie: 68000-based CPUs)
>
> This patch seems a bit strange and/or incomplete. Are we trying to
> get 2-byte alignment or 4-byte alignment of the payload? It seems
Actually, we try to get a 4n+2 alignment for skb->data, to get the ip-addresses
field 4bytes aligned.
I think the only thing wrong is the old comment that said :
/* Try to get the payload 4-byte aligned */
and that I did not change.
> that if the protocol field is uncompressed, we don't do anything to
> the alignment, but if it is compressed, we do this:
>
> > /* protocol is compressed */
> > - skb_push(skb, 1)[0] = 0;
> > + if ((unsigned long)skb->data & 1)
> > + skb_push(skb, 1)[0] = 0;
> > + else { /* Ditto, but realign the payload to 4-byte boundary */
> > + short len = skb->len;
> > +
> > + skb_put(skb, 3);
> > + memmove(skb->data + 3, skb->data, len);
> > + skb_pull(skb, 2)[0] = 0;
>
> I'm puzzled that we are not testing ((unsigned long)skb->data & 2) if
> we are really trying to achieve 4-byte alignment. In fact, if the
> skb->data that we get from dev_alloc_skb is 4-byte aligned to start
> with, this will end up with the payload starting at the original
> skb->data + 6, i.e. 2-byte aligned but not 4-byte aligned AFAICS.
>
> Can we assume that dev_alloc_skb will give us a 4-byte aligned
> skb->data? If we can then I suggest we change 3 to 1 in the skb_put
Are you not forgetting that the alignment of skb->data is changed (by the
existing code ! ) :
if (buf[0] != PPP_ALLSTATIONS)
skb_reserve(skb, 2 + (buf[0] & 1));
> and memmove above, and get rid of the if (since its condition will
> always be false).
>
> Paul.
>
Philippe
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch 11/15] ppp: handle misaligned accesses
Andrew Morton wrote : > Jeff Garzik <[EMAIL PROTECTED]> wrote: > > > > [EMAIL PROTECTED] wrote: > > > From: "Philippe De Muyter" <[EMAIL PROTECTED]> > > > > > > Avoid ppp-generated kernel crashes on machines where unaligned accesses > > > are > > > forbidden (ie: 68000-based CPUs) > > > > > > Signed-off-by: Philippe De Muyter <[EMAIL PROTECTED]> > > > Cc: "David S. Miller" <[EMAIL PROTECTED]> > > > Signed-off-by: Andrew Morton <[EMAIL PROTECTED]> > > > > Can you add Paul Mackerras to the CC list for all PPP patches? > > > > He's still actively maintaining it, and had not seen these patches. > > > > Actually Paul has seen the patches, and he commented on them several weeks > ago and there hasn't been a response iirc. > Sorry, I did not see any Paul's comment. I would be glad to see them. Philippe - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
