Hi Linus,
This patch forward ports some changes from latest 2.4 driver:
- Update maintainer email address
- Mark pci_device_id list with __devinitdata
- Set correct protocol type on packet receive (this caused the kernel to
drop all packets received)
- Add #ifdef DEBUG around debug printk()
Kudos to Ivan Passos / Daniela Squassoni
Please apply.
--- linux-2.6.1/drivers/net/wan/pc300_drv.c.orig 2004-01-28 12:48:48.000000000 -0200
+++ linux-2.6.1/drivers/net/wan/pc300_drv.c 2004-01-28 13:21:23.634860712 -0200
@@ -6,9 +6,9 @@
* pc300.c Cyclades-PC300(tm) Driver.
*
* Author: Ivan Passos <[email protected]>
- * Maintainer: Henrique Gobbi <[email protected]>
+ * Maintainer: PC300 Maintainer <[email protected]>
*
- * Copyright: (c) 1999-2002 Cyclades Corp.
+ * Copyright: (c) 1999-2003 Cyclades Corp.
*
* This program is free software; you can redistribute it and/or
* modify it under the terms of the GNU General Public License
@@ -252,7 +252,7 @@
#undef PC300_DEBUG_RX
#undef PC300_DEBUG_OTHER
-static struct pci_device_id cpc_pci_dev_id[] = {
+static struct pci_device_id cpc_pci_dev_id[] __devinitdata = {
/* PC300/RSV or PC300/X21, 2 chan */
{0x120e, 0x300, PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0x300},
/* PC300/RSV or PC300/X21, 1 chan */
@@ -1961,7 +1961,7 @@
}
stats->rx_packets++;
skb->mac.raw = skb->data;
- skb->protocol = htons(ETH_P_HDLC);
+ skb->protocol = hdlc_type_trans(skb, dev);
netif_rx(skb);
}
}
@@ -2088,9 +2088,10 @@
}
}
if (!(dsr_rx = cpc_readb(scabase + DSR_RX(ch)) & DSR_DE)) {
-
-printk("%s: RX intr chan[%d] (st=0x%08lx, dsr=0x%02x, dsr2=0x%02x)\n",
- dev->name, ch, status, drx_stat, dsr_rx);
+#ifdef PC300_DEBUG_INTR
+ printk("%s: RX intr chan[%d] (st=0x%08lx, dsr=0x%02x, dsr2=0x%02x)\n",
+ dev->name, ch, status, drx_stat, dsr_rx);
+#endif
cpc_writeb(scabase + DSR_RX(ch), (dsr_rx | DSR_DE) & 0xfe);
}
}
@@ -3690,6 +3691,6 @@
MODULE_DESCRIPTION("Cyclades-PC300 cards driver");
MODULE_AUTHOR( "Author: Ivan Passos <[email protected]>\r\n"
- "Maintainer: Henrique Gobbi <[email protected]");
+ "Maintainer: PC300 Maintainer <[email protected]");
MODULE_LICENSE("GPL");
> - Mark pci_device_id list with __devinitdata
This is bogus and can crash the kernel if you're unlucky.
> - Add #ifdef DEBUG around debug printk()
What about dprintk or friends instead?
On Wed, Jan 28, 2004 at 05:42:11PM -0200, Marcelo Tosatti wrote:
>
> - Mark pci_device_id list with __devinitdata
Noooo!!! I think we've finally audited all uses of this. Do not do
this please, it is wrong for 2.6.
> - Add #ifdef DEBUG around debug printk()
What's wrong with dev_dbg()? It gives you a much better idea of which
device is spitting out the messages.
thanks,
greg k-h
Hi Christoph!
On Wed, 28 Jan 2004, Christoph Hellwig wrote:
> > - Mark pci_device_id list with __devinitdata
>
> This is bogus and can crash the kernel if you're unlucky.
Other wan drivers are doing the same:
[marcelo@logos wan]$ grep __devinitdata *
farsync.c:static char *type_strings[] __devinitdata = {
wanxl.c:static struct pci_device_id wanxl_pci_tbl[] __devinitdata = {
I believe a handful of others are using "__devinitdata". How can the
kernel crash because of this? Who will try to touch the data?
> > - Add #ifdef DEBUG around debug printk()
>
> What about dprintk or friends instead?
Yes I will change to dev_dbg() as Greg suggested. Thanks!
Hi Greg!
On Wed, 28 Jan 2004, Greg KH wrote:
> On Wed, Jan 28, 2004 at 05:42:11PM -0200, Marcelo Tosatti wrote:
> >
> > - Mark pci_device_id list with __devinitdata
>
> Noooo!!! I think we've finally audited all uses of this. Do not do
> this please, it is wrong for 2.6.
[root@yage wan]# pwd
/root/linux-2.6.1/drivers/net/wan
[root@yage wan]# grep __devexit_p *
dscc4.c: .remove = __devexit_p(dscc4_remove_one),
farsync.c: .remove = __devexit_p(fst_remove_one),
So this ones are wrong too I assume?
Mind explaining me what is the deal with __devexit_p() in 2.6?
> > - Add #ifdef DEBUG around debug printk()
>
> What's wrong with dev_dbg()? It gives you a much better idea of which
> device is spitting out the messages.
Indeed, dev_dbg() is fine. I will replace all #ifdef DEBUG entries in the
driver with dev_dgb(...). Thanks for the hint.
Linus, hold the patch :)
On Wed, Jan 28, 2004 at 10:06:25PM -0200, Marcelo Tosatti wrote:
>
>
> Hi Christoph!
>
> On Wed, 28 Jan 2004, Christoph Hellwig wrote:
>
> > > - Mark pci_device_id list with __devinitdata
> >
> > This is bogus and can crash the kernel if you're unlucky.
>
> Other wan drivers are doing the same:
>
> [marcelo@logos wan]$ grep __devinitdata *
> farsync.c:static char *type_strings[] __devinitdata = {
> wanxl.c:static struct pci_device_id wanxl_pci_tbl[] __devinitdata = {
>
> I believe a handful of others are using "__devinitdata". How can the
> kernel crash because of this? Who will try to touch the data?
The id table is register witch the pci core who will look at it for
probing (e.g. when a new module is loaded), so it may not simply disappear
behind the scenes.
On Wed, Jan 28, 2004 at 10:06:25PM -0200, Marcelo Tosatti wrote:
> On Wed, 28 Jan 2004, Christoph Hellwig wrote:
> > > - Mark pci_device_id list with __devinitdata
> >
> > This is bogus and can crash the kernel if you're unlucky.
>
> Other wan drivers are doing the same:
>
> [marcelo@logos wan]$ grep __devinitdata *
> farsync.c:static char *type_strings[] __devinitdata = {
> wanxl.c:static struct pci_device_id wanxl_pci_tbl[] __devinitdata = {
>
> I believe a handful of others are using "__devinitdata". How can the
> kernel crash because of this? Who will try to touch the data?
It is particularly notable when someone inserts a Cardbus device. A
new PCI device is added, and we scan all drivers looking for a match
using the PCI ID tables.
If _any_ PCI ID table which is part registered as part of a driver is
marked using __devinitdata or __initdata, this will either cause the
kernel to read invalid data (possibly entering a long loop) or oops.
--
Russell King
Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/
maintainer of: 2.6 PCMCIA - http://pcmcia.arm.linux.org.uk/
2.6 Serial core
On Thu, Jan 29, 2004 at 09:02:22AM +0000, Russell King wrote:
> If _any_ PCI ID table which is part registered as part of a driver is
> marked using __devinitdata or __initdata, this will either cause the
> kernel to read invalid data (possibly entering a long loop) or oops.
After doing some more digging, I don't think __devinitdata is a problem
anymore.
There seem to be two scenarios where we look at the PCI device ID tables:
- when a new PCI device is added
- when the drivers newid file is written to
The first case should only ever occur if CONFIG_HOTPLUG is set (and
indeed we only compile PCMCIA/Cardbus if it is.)
The second case is disabled if CONFIG_HOTPLUG is not set.
Therefore, I think marking PCI device ID tables with __devinitdata
should theoretically be fine, but marking them with __initdata is
most definitely unsafe.
--
Russell King
Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/
maintainer of: 2.6 PCMCIA - http://pcmcia.arm.linux.org.uk/
2.6 Serial core