Re: [PATCH 2/2] xirc2ps_cs update

Re: [PATCH 2/2] xirc2ps_cs update

[Geller Sandor]

On Tue, 10 Jun 2003, Daniel Ritz wrote:

> -    busy_loop(HZ/25);		     /* wait 40 msec */
> +    Wait(HZ/25);		     /* wait 40 msec */

Why not Wait(40); instead Wait(HZ/25) ? Currently HZ is 1000. However, the
value can change - as it changed from 100 to 1000.

  Geller Sandor <wildy@petra.hos.u-szeged.hu>

Re: [PATCH 2/2] xirc2ps_cs update

[Jeff Garzik]

On Tue, Jun 10, 2003 at 05:35:18PM +0200, Geller Sandor wrote:
> On Tue, 10 Jun 2003, Daniel Ritz wrote:
> 
> > -    busy_loop(HZ/25);		     /* wait 40 msec */
> > +    Wait(HZ/25);		     /* wait 40 msec */
> 
> Why not Wait(40); instead Wait(HZ/25) ? Currently HZ is 1000. However, the
> value can change - as it changed from 100 to 1000.

True enough...  the best solution is to grep the tree for a
msecs_to_jiffies macro, and use that.  Then it will look like

	Wait(msecs_to_jiffies(40))

and the macro does the proper scaling versus constant HZ.

	Jeff

[PATCH 1/2] xirc2ps_cs update

[Daniel Ritz]

hi

this patch does:
- net_device is no longer allocated as part of the driver's private structure,
  instead it's allocated via alloc_netdev
- xirc2ps_detach calls xirc2ps_release if necessary (like the other drivers)

against 2.5.70-bk.

rgds
-daniel


===== drivers/net/pcmcia/xirc2ps_cs.c 1.19 vs edited =====
--- 1.19/drivers/net/pcmcia/xirc2ps_cs.c	Sun May 25 17:07:51 2003
+++ edited/drivers/net/pcmcia/xirc2ps_cs.c	Mon Jun  9 15:27:53 2003
@@ -358,7 +358,6 @@
 
 typedef struct local_info_t {
     dev_link_t link;
-    struct net_device dev;
     dev_node_t node;
     struct net_device_stats stats;
     int card_type;
@@ -619,11 +618,12 @@
     flush_stale_links();
 
     /* Allocate the device structure */
-    local = kmalloc(sizeof(*local), GFP_KERNEL);
-    if (!local) return NULL;
-    memset(local, 0, sizeof(*local));
-    link = &local->link; dev = &local->dev;
-    link->priv = dev->priv = local;
+    dev = alloc_etherdev(sizeof(local_info_t));
+    if (!dev)
+	    return NULL;
+    local = dev->priv;
+    link = &local->link;
+    link->priv = dev;
 
     init_timer(&link->release);
     link->release.function = &xirc2ps_release;
@@ -645,7 +645,6 @@
     dev->get_stats = &do_get_stats;
     dev->do_ioctl = &do_ioctl;
     dev->set_multicast_list = &set_multicast_list;
-    ether_setup(dev);
     dev->open = &do_open;
     dev->stop = &do_stop;
 #ifdef HAVE_TX_TIMEOUT
@@ -684,7 +683,7 @@
 static void
 xirc2ps_detach(dev_link_t * link)
 {
-    local_info_t *local = link->priv;
+    struct net_device *dev = link->priv;
     dev_link_t **linkp;
 
     DEBUG(0, "detach(0x%p)\n", link);
@@ -706,10 +705,11 @@
      */
     del_timer(&link->release);
     if (link->state & DEV_CONFIG) {
-	DEBUG(0, "detach postponed, '%s' still locked\n",
-	      link->dev->dev_name);
-	link->state |= DEV_STALE_LINK;
-	return;
+	xirc2ps_release((unsigned long)link);
+	if (link->state & DEV_STALE_CONFIG) {
+		link->state |= DEV_STALE_LINK;
+		return;
+	}
     }
 
     /* Break the link with Card Services */
@@ -719,8 +719,8 @@
     /* Unlink device structure, free it */
     *linkp = link->next;
     if (link->dev)
-	unregister_netdev(&local->dev);
-    kfree(local);
+	unregister_netdev(dev);
+    kfree(dev);
 
 } /* xirc2ps_detach */
 
@@ -745,7 +745,8 @@
 static int
 set_card_type(dev_link_t *link, const void *s)
 {
-    local_info_t *local = link->priv;
+    struct net_device *dev = link->priv;
+    local_info_t *local = dev->priv;
   #ifdef PCMCIA_DEBUG
     unsigned cisrev = ((const unsigned char *)s)[2];
   #endif
@@ -839,8 +840,8 @@
 xirc2ps_config(dev_link_t * link)
 {
     client_handle_t handle = link->handle;
-    local_info_t *local = link->priv;
-    struct net_device *dev = &local->dev;
+    struct net_device *dev = link->priv;
+    local_info_t *local = dev->priv;
     tuple_t tuple;
     cisparse_t parse;
     ioaddr_t ioaddr;
@@ -1195,11 +1196,10 @@
 xirc2ps_release(u_long arg)
 {
     dev_link_t *link = (dev_link_t *) arg;
-    local_info_t *local = link->priv;
-    struct net_device *dev = &local->dev;
 
     DEBUG(0, "release(0x%p)\n", link);
 
+#if 0
     /*
      * If the device is currently in use, we won't release until it
      * is actually closed.
@@ -1210,8 +1210,10 @@
 	link->state |= DEV_STALE_CONFIG;
 	return;
     }
+#endif
 
     if (link->win) {
+	struct net_device *dev = link->priv;
 	local_info_t *local = dev->priv;
 	if (local->dingo)
 	    iounmap(local->dingo_ccr - 0x0800);
@@ -1243,8 +1245,7 @@
 	      event_callback_args_t * args)
 {
     dev_link_t *link = args->client_data;
-    local_info_t *lp = link->priv;
-    struct net_device *dev = &lp->dev;
+    struct net_device *dev = link->priv;
 
     DEBUG(0, "event(%d)\n", (int)event);
 
@@ -2083,12 +2084,8 @@
 {
 	pcmcia_unregister_driver(&xirc2ps_cs_driver);
 
-	while (dev_list) {
-		if (dev_list->state & DEV_CONFIG)
-			xirc2ps_release((u_long)dev_list);
-		if (dev_list)	/* xirc2ps_release() might already have detached... */
-			xirc2ps_detach(dev_list);
-	}
+	while (dev_list)
+		xirc2ps_detach(dev_list);
 }
 
 module_init(init_xirc2ps_cs);

[PATCH 2/2] xirc2ps_cs update

[Daniel Ritz]

the second patch:
replaces busy_loop with a simple macro doing a schedule_timeout. busy_loop was never
called from interrupt conext anyway, so no need for that. and the sti() is gone.

rgds
-daniel


--- linux-2.5/drivers/net/pcmcia/xirc2ps_cs.c~1	2003-06-09 15:28:22.000000000 -0400
+++ linux-2.5/drivers/net/pcmcia/xirc2ps_cs.c	2003-06-10 14:17:04.000000000 -0400
@@ -431,22 +431,10 @@
 #define PutByte(reg,value) outb((value), ioaddr+(reg))
 #define PutWord(reg,value) outw((value), ioaddr+(reg))
 
-static void
-busy_loop(u_long len)
-{
-    if (in_interrupt()) {
-	u_long timeout = jiffies + len;
-	u_long flags;
-	save_flags(flags);
-	sti();
-	while (time_before_eq(jiffies, timeout))
-	    ;
-	restore_flags(flags);
-    } else {
-	__set_current_state(TASK_UNINTERRUPTIBLE);
-	schedule_timeout(len);
-    }
-}
+#define Wait(n) do { \
+	set_current_state(TASK_UNINTERRUPTIBLE); \
+	schedule_timeout(n); \
+} while (0)
 
 /*====== Functions used for debugging =================================*/
 #if defined(PCMCIA_DEBUG) && 0 /* reading regs may change system status */
@@ -1780,12 +1768,12 @@
     SelectPage(4);
     udelay(1);
     PutByte(XIRCREG4_GPR1, 0);	     /* clear bit 0: power down */
-    busy_loop(HZ/25);		     /* wait 40 msec */
+    Wait(HZ/25);		     /* wait 40 msec */
     if (local->mohawk)
 	PutByte(XIRCREG4_GPR1, 1);	 /* set bit 0: power up */
     else
 	PutByte(XIRCREG4_GPR1, 1 | 4);	 /* set bit 0: power up, bit 2: AIC */
-    busy_loop(HZ/50);		     /* wait 20 msec */
+    Wait(HZ/50);		     /* wait 20 msec */
 }
 
 static void
@@ -1799,9 +1787,9 @@
 
     hardreset(dev);
     PutByte(XIRCREG_CR, SoftReset); /* set */
-    busy_loop(HZ/50);		     /* wait 20 msec */
+    Wait(HZ/50);		     /* wait 20 msec */
     PutByte(XIRCREG_CR, 0);	     /* clear */
-    busy_loop(HZ/25);		     /* wait 40 msec */
+    Wait(HZ/25);		     /* wait 40 msec */
     if (local->mohawk) {
 	SelectPage(4);
 	/* set pin GP1 and GP2 to output  (0x0c)
@@ -1812,7 +1800,7 @@
     }
 
     /* give the circuits some time to power up */
-    busy_loop(HZ/2);		/* about 500ms */
+    Wait(HZ/2);		/* about 500ms */
 
     local->last_ptr_value = 0;
     local->silicon = local->mohawk ? (GetByte(XIRCREG4_BOV) & 0x70) >> 4
@@ -1831,7 +1819,7 @@
 	SelectPage(0x42);
 	PutByte(XIRCREG42_SWC1, 0x80);
     }
-    busy_loop(HZ/25);		     /* wait 40 msec to let it complete */
+    Wait(HZ/25);		     /* wait 40 msec to let it complete */
 
   #ifdef PCMCIA_DEBUG
     if (pc_debug) {
@@ -1890,7 +1878,7 @@
 	    printk(KERN_INFO "%s: MII selected\n", dev->name);
 	    SelectPage(2);
 	    PutByte(XIRCREG2_MSR, GetByte(XIRCREG2_MSR) | 0x08);
-	    busy_loop(HZ/50);
+	    Wait(HZ/50);
 	} else {
 	    printk(KERN_INFO "%s: MII detected; using 10mbs\n",
 		   dev->name);
@@ -1899,7 +1887,7 @@
 		PutByte(XIRCREG42_SWC1, 0xC0);
 	    else  /* enable 10BaseT */
 		PutByte(XIRCREG42_SWC1, 0x80);
-	    busy_loop(HZ/25);	/* wait 40 msec to let it complete */
+	    Wait(HZ/25);	/* wait 40 msec to let it complete */
 	}
 	if (full_duplex)
 	    PutByte(XIRCREG1_ECR, GetByte(XIRCREG1_ECR | FullDuplex));
@@ -1992,7 +1980,7 @@
 	 * Fixme: Better to use a timer here!
 	 */
 	for (i=0; i < 35; i++) {
-	    busy_loop(HZ/10);	 /* wait 100 msec */
+	    Wait(HZ/10);	 /* wait 100 msec */
 	    status = mii_rd(ioaddr,  0, 1);
 	    if ((status & 0x0020) && (status & 0x0004))
 		break;

Re: [PATCH 2/2] xirc2ps_cs update

[Daniel Ritz]

On Tuesday 10 June 2003 11:46, Jeff Garzik wrote:
> On Tue, Jun 10, 2003 at 05:35:18PM +0200, Geller Sandor wrote:
> > On Tue, 10 Jun 2003, Daniel Ritz wrote:
> > > -    busy_loop(HZ/25);		     /* wait 40 msec */
> > > +    Wait(HZ/25);		     /* wait 40 msec */
> >
> > Why not Wait(40); instead Wait(HZ/25) ? Currently HZ is 1000. However,
> > the value can change - as it changed from 100 to 1000.
>
> True enough...  the best solution is to grep the tree for a
> msecs_to_jiffies macro, and use that.  Then it will look like
>

hmm...yes, but the macro is defined in include/net/irda/irda.h
move it to a place where everyone can use it? like time.h or kernel.h?

> 	Wait(msecs_to_jiffies(40))

i would do it in the Wait macro. looks nicer..
and also define the Wait macro (with a better name, suggestions?)
somewhere else, 'cos other drivers use set_current_state(); schedule_timeout()
too..

>
> and the macro does the proper scaling versus constant HZ.
>
> 	Jeff

-daniel