Re: [PATCH] (5/6) ibmtr -- probe2

Re: [PATCH] (5/6) ibmtr -- probe2

[Mike_Phillips]

>The problem with the structures was a BUG in the existing code!
>The PCMCIA code was setting dev->priv to a structure:

<snip>

AAARRRGGH, it got screwed up somewhere in the past, during 2.5 or 2.6
somewhere. Take a look at 2.4.22, dev-priv gets set to &info->ti. *sigh*,
apologies, it doesn't help when the code you're looking at in the main tree
isn't the same as my dev tree and what I remember. (Wonder if it works at
all in 2.6, if it doesn't that shows just how many people are actually
testing it ;)

I *can* see what you're trying to do with the rest of the patch, although I
need to grab net-2.5-exp to fully see how this all works. There's parts of
the patch where I can see where the code was grabbed from but doesn't make
total sense yet (like testing for #ifndef PCMCIA in ibmtr_probe when
another part of the patch removed the call to ibmtr_probe from the pcmcia
driver.)

Bear with me, I think there's probably a cleaner solution.

pcmcia allocating its own dev structures is a right pain, would be nice if
this could go away too and it could all be done from the probe2 api.

Mike

Re: [PATCH] (5/6) ibmtr -- probe2

[Mike_Phillips]

>The problem with the structures was a BUG in the existing code!
>The PCMCIA code was setting dev->priv to a structure:

>struct ibmtr_dev_t {
>    dev_link_t                      link;
>    struct net_device         *dev;
>    dev_node_t          node;
>    window_handle_t     sram_win_handle;
>    struct tok_info           ti;
>}

>and then later when registered, the dev->init callback goes to ibmtr_probe
and
>then ibmtr_probe1 which expects dev->priv to point to 'struct tok_info'

>The code is broken in 2.6.0-test7.

Interesting that it's worked for 4 yrs then really ;) The fix is wrong too
then, if the code is really set the pointer to the wrong location, then
just moving the tok_info to the beginning of the struct is an ugly kludge.
It should be fixed the right way.

No worries, let me take a look at it all and remember exactly why it was
written this way and test with hardware.

Mike

Re: [PATCH] (5/6) ibmtr -- probe2

[Stephen Hemminger]

On Fri, 17 Oct 2003 10:37:41 -0300
Mike_Phillips@URSCorp.com wrote:

> 
> In ibmtr_cs.c:
> 
> >+static int ibmtr_init(struct net_device *dev)
> >+{
> >+    extern int ibmtr_probe1(struct net_device *, int);
> >+
> >+    return ibmtr_probe1(dev, dev->base_addr);
> >+}
> >+
> > /*======================================================================
> 
> >     ibmtr_attach() creates an "instance" of the driver, allocating
> >@@ -194,7 +200,8 @@
> 
> >     link->irq.Instance = info->dev = dev;
> >
> >-    dev->init = &ibmtr_probe;
> >+    dev->init = ibmtr_init;
> >+
> 
> This changes dev->init from calling ibmtr_probe to ibmtr_probe1. The probe
> routines are specifically split into two because of ibmtr_cs. It doesn't
> need to do second probe routing because all the memory spaces get allocated
> by the pcmcia driver. Trying to allocate the same IO mem area will fail
> because its already been allocated by ibmtr_cs.

The probe routine interface needs to change to return the device structure,
and the interface that ibmtr_cs needs is one where the structure is allocated.
If you look at the details,  the new probe1 does pretty much the same thing
the old probe did.  Doesn't the PCMCIA infrastructure give the correct memory
address and set it during the config routine. 

The problem with the structures was a BUG in the existing code!
The PCMCIA code was setting dev->priv to a structure:
	
struct ibmtr_dev_t {
    dev_link_t		link;
    struct net_device	*dev;
    dev_node_t          node;
    window_handle_t     sram_win_handle;
    struct tok_info	ti;
}

and then later when registered, the dev->init callback goes to ibmtr_probe and
then ibmtr_probe1 which expects dev->priv to point to 'struct tok_info'

The code is broken in 2.6.0-test7.
	 

Re: [PATCH] (5/6) ibmtr -- probe2

[Mike_Phillips]

On a more general note: ibmtr and ibmtr_cs are just plain evil and have
lots of little quirks in them because of the multitude of different
adapters available that actually use the tropic chipset.

Jeff: Please don't apply until I get a chance to try this out with real
hardware.

Thanks
Mike

Re: [PATCH] (5/6) ibmtr -- probe2

[Mike_Phillips]

In ibmtr_cs.c:

>+static int ibmtr_init(struct net_device *dev)
>+{
>+    extern int ibmtr_probe1(struct net_device *, int);
>+
>+    return ibmtr_probe1(dev, dev->base_addr);
>+}
>+
> /*======================================================================

>     ibmtr_attach() creates an "instance" of the driver, allocating
>@@ -194,7 +200,8 @@

>     link->irq.Instance = info->dev = dev;
>
>-    dev->init = &ibmtr_probe;
>+    dev->init = ibmtr_init;
>+

This changes dev->init from calling ibmtr_probe to ibmtr_probe1. The probe
routines are specifically split into two because of ibmtr_cs. It doesn't
need to do second probe routing because all the memory spaces get allocated
by the pcmcia driver. Trying to allocate the same IO mem area will fail
because its already been allocated by ibmtr_cs.

Mike

Re: [PATCH] (5/6) ibmtr -- probe2

[Mike_Phillips]

>>
>> > typedef struct ibmtr_dev_t {
>> > +    struct tok_info         ti;         /* must be first! */
>>
>> Why must this be first ?? Sounds like an alignment issue, we shouldn't
>> assume some is aligned correctly just because it's the first entry.

>Because ibmtr_cs calls ibmtr_probe1 function which does:
>            struct tok_info *ti = dev->priv;
>and dereferences it in a couple of places.

Huh ?? All the patch did was move the definition of ti to the start of the
struct ibmtr_dev_t

Mike

Re: [PATCH] (5/6) ibmtr -- probe2

[Mike_Phillips]

> typedef struct ibmtr_dev_t {
> +    struct tok_info         ti;         /* must be first! */

Why must this be first ?? Sounds like an alignment issue, we shouldn't
assume some is aligned correctly just because it's the first entry.

>+           if (err)
>+                       goto out1;
>+
>+           return 0;
>+ out1:

Replace with:
            if (!err) return 0;
instead of the extra lines of code.

There also a patch of code at the end that changes stuff from dev_ibmtr[i]
to dev and then sets dev_ibmtr[i] = dev. Is there a really good reason for
this. I can't see any real reason for the change.

A small note for some of the reasoning behind the strangeness of the
probing. T/R ISA devices typically live on port A20, some old legacy system
only have 23 bit, not 24 bit addressing here and an automatic probe on A20
will conflict with anything on 220 (ie soundcard) and lock the machine
solid. Hence the ability to absolutely specify the port to be probed. (The
probing years ago used to be really, really simple, you just probed A20 and
A24 and saw if the card was there)

Mike

Re: [PATCH] (5/6) ibmtr -- probe2

[Stephen Hemminger]

On Thu, 16 Oct 2003 15:52:50 -0700
Mike_Phillips@URSCorp.com wrote:

> 
> > typedef struct ibmtr_dev_t {
> > +    struct tok_info         ti;         /* must be first! */
> 
> Why must this be first ?? Sounds like an alignment issue, we shouldn't
> assume some is aligned correctly just because it's the first entry.

Because ibmtr_cs calls ibmtr_probe1 function which does:
	struct tok_info *ti = dev->priv;
and dereferences it in a couple of places.

[PATCH] (5/6) ibmtr -- probe2

[Stephen Hemminger]

Convert IBM ISA tokenring driver to new probing.
Also, need to deal with the fact that the PCMCIA ibmtr_cs
driver was calling the same probe routine.

diff -Nru a/drivers/net/Space.c b/drivers/net/Space.c
--- a/drivers/net/Space.c	Wed Oct 15 13:34:56 2003
+++ b/drivers/net/Space.c	Wed Oct 15 13:34:56 2003
@@ -409,12 +409,15 @@
 
 #ifdef CONFIG_TR
 /* Token-ring device probe */
-extern int ibmtr_probe(struct net_device *);
+extern struct net_device *ibmtr_probe(int unit);
 extern struct net_device *sk_isa_probe(int unit);
 extern struct net_device *proteon_probe(int unit);
 extern struct net_device *smctr_probe(int unit);
 
 static struct devprobe2 tr_probes2[] __initdata = {
+#ifdef CONFIG_IBMTR
+	{ibmtr_probe, 0},
+#endif
 #ifdef CONFIG_SKISA
 	{sk_isa_probe, 0},
 #endif
@@ -439,9 +442,6 @@
 	sprintf(dev->name, "tr%d", unit);
 	netdev_boot_setup_check(dev);
 	if (
-#ifdef CONFIG_IBMTR
-	    ibmtr_probe(dev) == 0  ||
-#endif
 	    0 ) 
 		err = register_netdev(dev);
 		
diff -Nru a/drivers/net/pcmcia/ibmtr_cs.c b/drivers/net/pcmcia/ibmtr_cs.c
--- a/drivers/net/pcmcia/ibmtr_cs.c	Wed Oct 15 13:34:56 2003
+++ b/drivers/net/pcmcia/ibmtr_cs.c	Wed Oct 15 13:34:56 2003
@@ -125,18 +125,17 @@
 
 static dev_link_t *dev_list;
 
-extern int ibmtr_probe(struct net_device *dev);
 extern int trdev_init(struct net_device *dev);
 extern irqreturn_t tok_interrupt (int irq, void *dev_id, struct pt_regs *regs);
 
 /*====================================================================*/
 
 typedef struct ibmtr_dev_t {
+    struct tok_info	ti;	/* must be first! */
     dev_link_t		link;
     struct net_device	*dev;
     dev_node_t          node;
     window_handle_t     sram_win_handle;
-    struct tok_info	ti;
 } ibmtr_dev_t;
 
 static void netdev_get_drvinfo(struct net_device *dev,
@@ -149,6 +148,13 @@
 	.get_drvinfo		= netdev_get_drvinfo,
 };
 
+static int ibmtr_init(struct net_device *dev)
+{
+    extern int ibmtr_probe1(struct net_device *, int);
+
+    return ibmtr_probe1(dev, dev->base_addr);
+}
+
 /*======================================================================
 
     ibmtr_attach() creates an "instance" of the driver, allocating
@@ -194,7 +200,8 @@
 
     link->irq.Instance = info->dev = dev;
     
-    dev->init = &ibmtr_probe;
+    dev->init = ibmtr_init;
+
     SET_ETHTOOL_OPS(dev, &netdev_ethtool_ops);
 
     /* Register with Card Services */
diff -Nru a/drivers/net/tokenring/ibmtr.c b/drivers/net/tokenring/ibmtr.c
--- a/drivers/net/tokenring/ibmtr.c	Wed Oct 15 13:34:56 2003
+++ b/drivers/net/tokenring/ibmtr.c	Wed Oct 15 13:34:56 2003
@@ -187,8 +187,7 @@
 #define TRC_INITV 0x02		/*  verbose init trace points     */
 unsigned char ibmtr_debug_trace = 0;
 
-int 		ibmtr_probe(struct net_device *dev);
-static int	ibmtr_probe1(struct net_device *dev, int ioaddr);
+int	ibmtr_probe1(struct net_device *dev, int ioaddr);
 static unsigned char get_sram_size(struct tok_info *adapt_info);
 static int 	trdev_init(struct net_device *dev);
 static int 	tok_open(struct net_device *dev);
@@ -325,29 +324,57 @@
  *	which references it.
  ****************************************************************************/
 
-int __devinit ibmtr_probe(struct net_device *dev)
+struct net_device * __devinit ibmtr_probe(int unit)
 {
-	int i;
-	int base_addr = dev->base_addr;
+	struct net_device *dev = alloc_trdev(sizeof(struct tok_info));
+	const int *port;
+	int err = 0;
 
-	if (base_addr && base_addr <= 0x1ff) /* Don't probe at all. */
-		return -ENXIO;
-	if (base_addr > 0x1ff) { /* Check a single specified location.  */
-		if (!ibmtr_probe1(dev, base_addr)) return 0;
-		return -ENODEV;
+	if (!dev)
+		return ERR_PTR(-ENOMEM);
+
+	if (unit >= 0) {
+		sprintf(dev->name, "tr%d", unit);
+		netdev_boot_setup_check(dev);
 	}
-	find_turbo_adapters(ibmtr_portlist);
-	for (i = 0; ibmtr_portlist[i]; i++) {
-		int ioaddr = ibmtr_portlist[i];
 
-		if (!ibmtr_probe1(dev, ioaddr)) return 0;
+	if (dev->base_addr > 0x1ff) /* Check a single specified location.  */
+		err = ibmtr_probe1(dev, dev->base_addr);
+	else if (dev->base_addr != 0) /* Don't probe at all. */
+		err = -ENXIO;
+	else {
+		find_turbo_adapters(ibmtr_portlist);
+		for (port = ibmtr_portlist; *port; port++) {
+			err = ibmtr_probe1(dev, *port);
+			if (!err)
+				break;
+		}
+	}
+	if (err)
+		goto out;
+	
+	err = register_netdev(dev);
+	if (err)
+		goto out1;
+	
+	return 0;
+ out1:
+	release_region(dev->base_addr, IBMTR_IO_EXTENT);
+#ifndef PCMCIA
+	{ 
+		struct tok_info *ti = dev->priv;
+		iounmap((u32 *)ti->mmio);
+		iounmap((u32 *)ti->sram_virt);
 	}
-	return -ENODEV;
+#endif		
+ out:
+	free_netdev(dev);
+	return ERR_PTR(err);
 }
 
 /*****************************************************************************/
 
-static int __devinit ibmtr_probe1(struct net_device *dev, int PIOaddr)
+int ibmtr_probe1(struct net_device *dev, int PIOaddr)
 {
 
 	unsigned char segment, intr=0, irq=0, i, j, cardpresent=NOTOK, temp=0;
@@ -1918,7 +1945,8 @@
 
 static int __init ibmtr_init(void)
 {
-	int i;
+	struct net_device *dev;
+	int i, err;
 	int count=0;
 
 	find_turbo_adapters(io);
@@ -1926,26 +1954,29 @@
 	for (i = 0; io[i] && (i < IBMTR_MAX_ADAPTERS); i++) {
 		irq[i] = 0;
 		mem[i] = 0;
-		dev_ibmtr[i] = alloc_trdev(sizeof(struct tok_info));
-		if (dev_ibmtr[i] == NULL) { 
-			if (i == 0)
-				return -ENOMEM;
-			break;
-		}
-		dev_ibmtr[i]->base_addr = io[i];
-		dev_ibmtr[i]->irq = irq[i];
-		dev_ibmtr[i]->mem_start = mem[i];
-		dev_ibmtr[i]->init = &ibmtr_probe;
-		if (register_netdev(dev_ibmtr[i]) != 0) {
-			kfree(dev_ibmtr[i]);
-			dev_ibmtr[i] = NULL;
+		
+		dev = alloc_trdev(sizeof(struct tok_info));
+		if (!dev)
 			continue;
+		SET_MODULE_OWNER(dev);
+
+		dev->irq = irq[i];
+		dev->mem_start = mem[i];
+		err = ibmtr_probe1(dev, io[i]);
+		if (err) {
+			free_netdev(dev);
+			continue;
+		}			
+		err = register_netdev(dev);
+		if (err) {
+			printk("ibmtr: register_netdev() returned %d.\n", err);
+			break;
 		}
 		count++;
+		dev_ibmtr[i] = dev;
 	}
-	if (count) return 0;
-	printk("ibmtr: register_netdev() returned non-zero.\n");
-	return -EIO;
+
+	return count ? 0 : -ENODEV;
 }
 module_init(ibmtr_init);
 
@@ -1954,29 +1985,30 @@
 	int i;
 
 	for (i = 0; i < IBMTR_MAX_ADAPTERS; i++){
-		if (!dev_ibmtr[i])
+		struct net_device *dev = dev_ibmtr[i];
+
+		if (!dev)
 			continue;
-		if (dev_ibmtr[i]->base_addr) {
-			outb(0,dev_ibmtr[i]->base_addr+ADAPTRESET);
+		if (dev->base_addr) {
+			outb(0,dev->base_addr+ADAPTRESET);
 			
 			schedule_timeout(TR_RST_TIME); /* wait 50ms */
 
-                        outb(0,dev_ibmtr[i]->base_addr+ADAPTRESETREL);
+                        outb(0,dev->base_addr+ADAPTRESETREL);
                 }
 
-		unregister_netdev(dev_ibmtr[i]);
-		free_irq(dev_ibmtr[i]->irq, dev_ibmtr[i]);
-		release_region(dev_ibmtr[i]->base_addr, IBMTR_IO_EXTENT);
+		unregister_netdev(dev);
+		free_irq(dev->irq, dev);
+		release_region(dev->base_addr, IBMTR_IO_EXTENT);
 #ifndef PCMCIA
 		{ 
-			struct tok_info *ti = (struct tok_info *)
-				dev_ibmtr[i]->priv;
+			struct tok_info *ti = dev->priv;
 			iounmap((u32 *)ti->mmio);
 			iounmap((u32 *)ti->sram_virt);
 		}
 #endif		
-		free_netdev(dev_ibmtr[i]);
-		dev_ibmtr[i] = NULL;
+		free_netdev(dev);
+		dev_ibmtr[i] = 0;
 	}
 }
 module_exit(ibmtr_cleanup);