[PATCH] PC300 update

[PATCH] PC300 update

[Marcelo Tosatti]

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 <ivan@cyclades.com>
- * Maintainer:	Henrique Gobbi <henrique@cyclades.com>
+ * Maintainer:	PC300 Maintainer <pc300@cyclades.com>
  *
- * 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 <ivan@cyclades.com>\r\n"
-                "Maintainer: Henrique Gobbi <henrique.gobbi@cyclades.com");
+                "Maintainer: PC300 Maintainer <pc300@cyclades.com");
 MODULE_LICENSE("GPL");

Re: [PATCH] PC300 update

[Christoph Hellwig]

> - 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?

Re: [PATCH] PC300 update

[Greg KH]

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

Re: [PATCH] PC300 update

[Marcelo Tosatti]

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 :)

Re: [PATCH] PC300 update

[Marcelo Tosatti]

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!

Re: [PATCH] PC300 update

[Christoph Hellwig]

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.

Re: [PATCH] PC300 update

[Russell King]

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

Re: [PATCH] PC300 update

[Russell King]

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

[PATCH] PC300 update

[Marcelo Tosatti]

Hi Linus,

This patch forward ports a few important fixes from the 2.4 driver. This
changes have been well tested.

Please apply.

Changelog:

- Update maintainer email address
- Mark pci_device_id list with __devinitdata.

Greg: rmk and hch checked this and found no problem.
  A bunch of other drivers are doing the same.

- Set correct protocol type on packet receive (this caused the kernel to
  drop all packets received)
- Add #ifdef DEBUG around debug printk()

Greg, Christoph: dprintk()/etc would be nice, but we're not trying
a rewrite here. Yes its ugly but its consistent with the rest.
Maybe we can do it for the whole driver, later on.

- ioctl: Add missing size checks before
  copying data from userspace.


--- 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-30 10:36:00.756908936 -0200
@@ -6,9 +6,9 @@
  * pc300.c	Cyclades-PC300(tm) Driver.
  *
  * Author:	Ivan Passos <ivan@cyclades.com>
- * Maintainer:	Henrique Gobbi <henrique@cyclades.com>
+ * Maintainer:	PC300 Maintainer <pc300@cyclades.com>
  *
- * 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);
 				}
 			}
@@ -2770,6 +2771,10 @@
 					if (!capable(CAP_NET_ADMIN)) {
 						return -EPERM;
 					}
+					/* incorrect data len? */
+					if (ifr->ifr_settings.size != size) {
+						return -ENOBUFS;
+					}

 					if (copy_from_user(&conf->phys_settings,
 							   settings->ifs_ifsu.sync, size)) {
@@ -2788,12 +2793,18 @@
 				case IF_IFACE_T1:
 				case IF_IFACE_E1:
 				{
+					const size_t te_size = sizeof(te1_settings);
 					const size_t size = sizeof(sync_serial_settings);

 					if (!capable(CAP_NET_ADMIN)) {
 						return -EPERM;
 					}
-
+
+					/* incorrect data len? */
+					if (ifr->ifr_settings.size != te_size) {
+						return -ENOBUFS;
+					}
+
 					if (copy_from_user(&conf->phys_settings,
 							   settings->ifs_ifsu.te1, size)) {
 						return -EFAULT;
@@ -3667,12 +3678,10 @@
 }

 static struct pci_driver cpc_driver = {
-	.name = "pc300",
-	.id_table = cpc_pci_dev_id,
-	.probe = cpc_init_one,
-	.remove = cpc_remove_one,
-	.suspend = NULL,
-	.resume = NULL,
+	.name           = "pc300",
+	.id_table       = cpc_pci_dev_id,
+	.probe          = cpc_init_one,
+	.remove         = __devexit_p(cpc_remove_one),
 };

 static int __init cpc_init(void)
@@ -3690,6 +3699,6 @@

 MODULE_DESCRIPTION("Cyclades-PC300 cards driver");
 MODULE_AUTHOR(  "Author: Ivan Passos <ivan@cyclades.com>\r\n"
-                "Maintainer: Henrique Gobbi <henrique.gobbi@cyclades.com");
+                "Maintainer: PC300 Maintainer <pc300@cyclades.com");
 MODULE_LICENSE("GPL");