Re: [PATCH] Update in-kernel orinoco drivers to upstream current CVS

Re: [PATCH] Update in-kernel orinoco drivers to upstream current CVS

[Jean Tourrilhes]

Jeff Garzik wrote :
> Dan Williams wrote:
> > Hi,
> > 
> > This patch is simply the fixed-up diff between the kernel's current
> > 0.13e version and the upstream 0.15rc1+ version from savannah CVS.
> > 0.15rc1 has been out for a couple months now and seems stable.
> 
> I'm desperately hoping that someone will split this up into multiple 
> patches...
> 
> 	Jeff

	David Gibson is the official maintainer of the Orinoco
driver. Pavel Roskin is the person that did most of the work on
0.15rc1+. I think it would be a nice idea to involve those two person
in such a discussion (therefore, cc'ed).
	The difference between 0.13e and 0.15rc1+ is not small. I
believe Pavel did a good job in splitting the various patches in small
pieces when adding them to the CVS, and David has tracked the kernel,
but reconciliating the two branches is no trivial matter.
	Jeff, does BitKeeper allow to merge patches at an earlier
point than the last version and reconciliate both branches ? That
might come handy.
	Good luck...

	Jean

Re: [PATCH] Update in-kernel orinoco drivers to upstream current CVS

[Francois Romieu]

Jean Tourrilhes <jt@bougret.hpl.hp.com> :
[...]
> 	The difference between 0.13e and 0.15rc1+ is not small. I
> believe Pavel did a good job in splitting the various patches in small
> pieces when adding them to the CVS, and David has tracked the kernel,
> but reconciliating the two branches is no trivial matter.

I have extracted a few things from the bz2 ball that Dan sent (against
2.6.7-mm5 which already contains some orinoco bits):

-rw-r--r--    1 romieu   users        4564 jui  3 00:47 orinoco-10.patch
-rw-r--r--    1 romieu   users       15999 jui  3 00:47 orinoco-20.patch
-rw-r--r--    1 romieu   users       33135 jui  3 00:47 orinoco-30.patch
-rw-r--r--    1 romieu   users       11463 jui  3 00:47 orinoco-40.patch

Available at http://www.fr.zoreil.com/linux/kernel/2.6.x/2.6.7-mm5/

So far the patches lack comments but they are quite simple.

--
Ueimor

Re: [PATCH] Update in-kernel orinoco drivers to upstream current CVS

[Francois Romieu]

[drivers/net/wireless/orinoco_tmd.c:orinoco_tmd_init_one]
        err = pci_enable_device(pdev);
-       if (err)
-               return -EIO;
-
-       printk(KERN_DEBUG "TMD setup\n");
-       pccard_ioaddr = pci_resource_start(pdev, 2);
-       pccard_iolen = pci_resource_len(pdev, 2);
-       if (! request_region(pccard_ioaddr, pccard_iolen, dev_info)) {
-               printk(KERN_ERR PFX "I/O resource at 0x%lx len 0x%lx busy\n",
-                       pccard_ioaddr, pccard_iolen);
-               pccard_ioaddr = 0;
-               err = -EBUSY;
-               goto fail;
+       if (err) {
+               printk(KERN_ERR PFX "Cannot enable PCI device\n");
+               return -err;

-> translates into
   err = pci_enable_device(dev);
   if (err) {
           printk(KERN_ERR PFX "Cannot enable PCI device\n");
           return -err;

i.e. incorrectly returns > 0 

--
Ueimor

Re: [PATCH] Update in-kernel orinoco drivers to upstream current CVS

[Francois Romieu]

Francois Romieu <romieu@fr.zoreil.com> :
[...]
> -rw-r--r--    1 romieu   users        4564 jui  3 00:47 orinoco-10.patch
> -rw-r--r--    1 romieu   users       15999 jui  3 00:47 orinoco-20.patch
> -rw-r--r--    1 romieu   users       33135 jui  3 00:47 orinoco-30.patch
> -rw-r--r--    1 romieu   users       11463 jui  3 00:47 orinoco-40.patch

Updated to:
-rw-r--r--    1 romieu   users        5111 jui  4 01:02 orinoco-10.patch
-rw-r--r--    1 romieu   users       15999 jui  4 01:02 orinoco-20.patch
-rw-r--r--    1 romieu   users       33135 jui  4 01:02 orinoco-30.patch
-rw-r--r--    1 romieu   users       11463 jui  4 01:02 orinoco-40.patch
-rw-r--r--    1 romieu   users       10414 jui  4 01:02 orinoco-50.patch
-rw-r--r--    1 romieu   users        1515 jui  4 01:02 orinoco-60.patch
-rw-r--r--    1 romieu   users        5499 jui  4 01:02 orinoco-70.patch
-rw-r--r--    1 romieu   users        8907 jui  4 01:02 orinoco-80.patch
-rw-r--r--    1 romieu   users        4800 jui  4 01:02 orinoco-90.patch
-rw-r--r--    1 romieu   users        2185 jui  4 01:02 orinoco-100.patch
-rw-r--r--    1 romieu   users        1907 jui  4 01:02 orinoco-110.patch
-rw-r--r--    1 romieu   users        1639 jui  4 01:02 orinoco-120.patch
-rw-r--r--    1 romieu   users        6721 jui  4 01:02 orinoco-130.patch
-rw-r--r--    1 romieu   users        2647 jui  4 01:02 orinoco-140.patch

Still available at http://www.fr.zoreil.com/linux/kernel/2.6.x/2.6.7-mm5/

An extra netdev_priv() has been added to orinoco-10.patch.

--
Ueimor

Re: [PATCH] Update in-kernel orinoco drivers to upstream current CVS

[David Gibson]

On Sat, Jul 03, 2004 at 01:07:09AM +0200, Francois Romieu wrote:
> Jean Tourrilhes <jt@bougret.hpl.hp.com> :
> [...]
> > 	The difference between 0.13e and 0.15rc1+ is not small. I
> > believe Pavel did a good job in splitting the various patches in small
> > pieces when adding them to the CVS, and David has tracked the kernel,
> > but reconciliating the two branches is no trivial matter.
> 
> I have extracted a few things from the bz2 ball that Dan sent (against
> 2.6.7-mm5 which already contains some orinoco bits):
> 
> -rw-r--r--    1 romieu   users        4564 jui  3 00:47 orinoco-10.patch
> -rw-r--r--    1 romieu   users       15999 jui  3 00:47 orinoco-20.patch
> -rw-r--r--    1 romieu   users       33135 jui  3 00:47 orinoco-30.patch
> -rw-r--r--    1 romieu   users       11463 jui  3 00:47 orinoco-40.patch
> 
> Available at http://www.fr.zoreil.com/linux/kernel/2.6.x/2.6.7-mm5/
> 
> So far the patches lack comments but they are quite simple.

Aha, that's a good start.  During the week I'll try to look at these,
put my rubber stamp on them, and send them on to Jeff.

One thing I notice though (from your later ones, actually) is that you
seem to be moving from current 2.6 to the CVS HEAD branch.  That
includes the orinoco_usb stuff, which I still don't think is done
right and would rather not push.  I know this is asking a bir of a
favour, given how useless I've been at doing the update myself, but
your patches would be much more useful if they aimed at the CVS
"for_linus" branch.

-- 
David Gibson			| For every complex problem there is a
david AT gibson.dropbear.id.au	| solution which is simple, neat and
				| wrong.
http://www.ozlabs.org/people/dgibson

Re: [PATCH] Update in-kernel orinoco drivers to upstream current CVS

[Francois Romieu]

David Gibson <hermes@gibson.dropbear.id.au> :
[...]
> your patches would be much more useful if they aimed at the CVS
> "for_linus" branch.

The savannah thing refused the cvs+ssh connection. It would help if
someone could post the url of the relevant tarball. 

--
Ueimor

Re: [PATCH] Update in-kernel orinoco drivers to upstream current CVS

[Francois Romieu]

The news:
- I got the adequate patch from the cvs repository
- 35 patches are available at the usual location. The series-mm file
  describes the ordering of the patches. I'll redo the numbering as
  it starts to be scary
- the remaining diff weights ~210k so far

At least it makes reviewing easier.

--
Ueimor

Re: [PATCH] Update in-kernel orinoco drivers to upstream current CVS

[Jeff Garzik]

Francois Romieu wrote:
> The news:
> - I got the adequate patch from the cvs repository
> - 35 patches are available at the usual location. The series-mm file
>   describes the ordering of the patches. I'll redo the numbering as
>   it starts to be scary
> - the remaining diff weights ~210k so far
> 
> At least it makes reviewing easier.


If you are willing to do some re-diffing, feel free to send out the 
boring, and easy-to-review parts such as netdev_priv() or obvious 
cleanups.  That would help, at least, to cut things to more meat, and 
less noise.

	Jeff

Re: [PATCH] Update in-kernel orinoco drivers to upstream current CVS

[Francois Romieu]

Jeff Garzik <jgarzik@pobox.com> :
> Francois Romieu wrote:
> > The news:
> > - I got the adequate patch from the cvs repository
> > - 35 patches are available at the usual location. The series-mm file
> >   describes the ordering of the patches. I'll redo the numbering as
> >   it starts to be scary
> > - the remaining diff weights ~210k so far
> > 
> > At least it makes reviewing easier.
> 
> 
> If you are willing to do some re-diffing, feel free to send out the 
> boring, and easy-to-review parts such as netdev_priv() or obvious 
> cleanups.  That would help, at least, to cut things to more meat, and 
> less noise.

Actually it does not induce a noticeable noise. The remaining patch is
down to 162 ko. 50 ko have disappeared while partially moving code on
the target sources (I'll keep this part separated from the "normal"
patches).

The renumbered patches + one or two new ones are available at
http://www.fr.zoreil.com/linux/kernel/2.6.x/2.6.7-mm6

--
Ueimor

Re: [PATCH] Update in-kernel orinoco drivers to upstream current CVS

[Michael Clark]

[-- Attachment #1: Type: text/plain, Size: 1463 bytes --]

Just like to chime in as a tester. I've been running the orinoco CVS
for around 6 weeks now. Has been very stable although I needed the
attached patch to make suspend/resume work (thinkpad with APM).

I posted a bug on orinoco savannah but haven't seen any changes in CVS.

BTW - what is the *correct* ordering of pci_(save|restore)_state,
pci_set_power_state?

The new scanning functionality is great. Thanks guys for the good work.

~mc

On 07/07/04 06:54, Francois Romieu wrote:
> Jeff Garzik <jgarzik@pobox.com> :
> 
>>Francois Romieu wrote:
>>
>>>The news:
>>>- I got the adequate patch from the cvs repository
>>>- 35 patches are available at the usual location. The series-mm file
>>>  describes the ordering of the patches. I'll redo the numbering as
>>>  it starts to be scary
>>>- the remaining diff weights ~210k so far
>>>
>>>At least it makes reviewing easier.
>>
>>
>>If you are willing to do some re-diffing, feel free to send out the 
>>boring, and easy-to-review parts such as netdev_priv() or obvious 
>>cleanups.  That would help, at least, to cut things to more meat, and 
>>less noise.
> 
> 
> Actually it does not induce a noticeable noise. The remaining patch is
> down to 162 ko. 50 ko have disappeared while partially moving code on
> the target sources (I'll keep this part separated from the "normal"
> patches).
> 
> The renumbered patches + one or two new ones are available at
> http://www.fr.zoreil.com/linux/kernel/2.6.x/2.6.7-mm6

[-- Attachment #2: orinoco-fix-powerstate.patch --]
[-- Type: text/x-patch, Size: 552 bytes --]

--- orinoco_pci.c.orig	2004-06-21 20:35:22.000000000 +0800
+++ orinoco_pci.c	2004-06-22 12:32:24.000000000 +0800
@@ -327,8 +327,8 @@
 	
 	orinoco_unlock(priv, &flags);
 
-	pci_set_power_state(pdev, 3);
 	pci_save_state(pdev, card->pci_state);
+	pci_set_power_state(pdev, 3);
 
 	return 0;
 }
@@ -343,8 +343,8 @@
 
 	printk(KERN_DEBUG "%s: Orinoco-PCI waking up\n", dev->name);
 
-	pci_restore_state(pdev, card->pci_state);
 	pci_set_power_state(pdev, 0);
+	pci_restore_state(pdev, card->pci_state);
 
 	err = orinoco_reinit_firmware(dev);
 	if (err) {

Re: [PATCH] Update in-kernel orinoco drivers to upstream current CVS

[Pavel Roskin]

Hello!

[trimming the cc: a bit]

On Wed, 7 Jul 2004, Michael Clark wrote:

> Just like to chime in as a tester. I've been running the orinoco CVS
> for around 6 weeks now. Has been very stable although I needed the
> attached patch to make suspend/resume work (thinkpad with APM).
>
> I posted a bug on orinoco savannah but haven't seen any changes in CVS.

Sorry, but bug tracking systems rarely provide fast turnaround.  It's 
better that you post to orinoco-devel@lists.sourceforge.net to get a reply 
fast.

The patch you submitted on Savannah reverts the patch submitted by Simon 
Huggins: http://sourceforge.net/mailarchive/message.php?msg_id=7995284

The patch you attached seems to be much better.  I've applied it.

> BTW - what is the *correct* ordering of pci_(save|restore)_state,
> pci_set_power_state?

drivers/net/pci-skeleton.c doesn't have power management code, but the 
driver it was based on, 8139too.c, has such code and uses 
pci_set_power_state() after pci_(save|restore)_state().  Other well 
maintained drivers (e.g. e100.c) use pci_set_power_state() after 
pci_save_state() and before pci_restore_state().  I think it's reasonable 
to follow this example.  Jeff?

-- 
Regards,
Pavel Roskin

Re: [PATCH] Update in-kernel orinoco drivers to upstream current CVS

[Jeff Garzik]

Pavel Roskin wrote:
> drivers/net/pci-skeleton.c doesn't have power management code, but the 
> driver it was based on, 8139too.c, has such code and uses 
> pci_set_power_state() after pci_(save|restore)_state().  Other well 
> maintained drivers (e.g. e100.c) use pci_set_power_state() after 
> pci_save_state() and before pci_restore_state().  I think it's 
> reasonable to follow this example.  Jeff?


Yeah, that's working code.  Feel free to cut-n-paste, and/or even update 
pci-skeleton.c :)

	Jeff

[PATCH] Updated pci-skeleton.c

[Pavel Roskin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1003 bytes --]

On Thu, 8 Jul 2004, Jeff Garzik wrote:

> Pavel Roskin wrote:
>> drivers/net/pci-skeleton.c doesn't have power management code, but the 
>> driver it was based on, 8139too.c, has such code and uses 
>> pci_set_power_state() after pci_(save|restore)_state().  Other well 
>> maintained drivers (e.g. e100.c) use pci_set_power_state() after 
>> pci_save_state() and before pci_restore_state().  I think it's reasonable 
>> to follow this example.  Jeff?
>
>
> Yeah, that's working code.  Feel free to cut-n-paste, and/or even update 
> pci-skeleton.c :)

Here it is.  The patch fixes all compile errors and warnings in 
pci-skeleton.c.  The "debug" parameter lacks corresponding variable, so I 
removed it to avoid a warning on module load.  Obsolete pci_power_on() and 
pci_power_off() have been replaced with the new code using 
pci_set_power_state(), pci_save_state() and pci_restore_state().  The 
driver has been tested by compiling it as module and as part of the 
kernel.

-- 
Regards,
Pavel Roskin

[-- Attachment #2: Type: TEXT/PLAIN, Size: 3429 bytes --]

--- linux.orig/drivers/net/pci-skeleton.c
+++ linux/drivers/net/pci-skeleton.c
@@ -481,6 +481,7 @@ struct netdrv_private {
 	unsigned int mediasense:1;	/* Media sensing in progress. */
 	spinlock_t lock;
 	chip_t chipset;
+	u32 pci_state[16];	/* Data saved during suspend */
 };
 
 MODULE_AUTHOR ("Jeff Garzik <jgarzik@pobox.com>");
@@ -488,12 +489,10 @@ MODULE_DESCRIPTION ("Skeleton for a PCI 
 MODULE_LICENSE("GPL");
 MODULE_PARM (multicast_filter_limit, "i");
 MODULE_PARM (max_interrupt_work, "i");
-MODULE_PARM (debug, "i");
 MODULE_PARM (media, "1-" __MODULE_STRING(8) "i");
 MODULE_PARM_DESC (multicast_filter_limit, "pci-skeleton maximum number of filtered multicast addresses");
 MODULE_PARM_DESC (max_interrupt_work, "pci-skeleton maximum events handled per interrupt");
 MODULE_PARM_DESC (media, "pci-skeleton: Bits 0-3: media type, bit 17: full duplex");
-MODULE_PARM_DESC (debug, "(unused)");
 
 static int read_eeprom (void *ioaddr, int location, int addr_len);
 static int netdrv_open (struct net_device *dev);
@@ -588,7 +587,6 @@ static int __devinit netdrv_init_board (
 	void *ioaddr = NULL;
 	struct net_device *dev;
 	struct netdrv_private *tp;
-	u8 tmp8;
 	int rc, i;
 	u32 pio_start, pio_end, pio_flags, pio_len;
 	unsigned long mmio_start, mmio_end, mmio_flags, mmio_len;
@@ -744,7 +742,6 @@ static int __devinit netdrv_init_one (st
 	int i, addr_len, option;
 	void *ioaddr = NULL;
 	static int board_idx = -1;
-	u8 tmp;
 
 /* when built into the kernel, we only print version if device is found */
 #ifndef MODULE
@@ -868,7 +865,7 @@ static void __devexit netdrv_remove_one 
 
 	pci_set_drvdata (pdev, NULL);
 
-	pci_power_off (pdev, -1);
+	pci_disable_device (pdev);
 
 	DPRINTK ("EXIT\n");
 }
@@ -1136,7 +1133,6 @@ static void netdrv_hw_start (struct net_
 	struct netdrv_private *tp = dev->priv;
 	void *ioaddr = tp->mmio_addr;
 	u32 i;
-	u8 tmp;
 
 	DPRINTK ("ENTER\n");
 
@@ -1875,9 +1871,11 @@ static void netdrv_set_rx_mode (struct n
 		rx_mode = AcceptBroadcast | AcceptMulticast | AcceptMyPhys;
 		mc_filter[1] = mc_filter[0] = 0;
 		for (i = 0, mclist = dev->mc_list; mclist && i < dev->mc_count;
-		     i++, mclist = mclist->next)
-			set_bit (ether_crc (ETH_ALEN, mclist->dmi_addr) >> 26,
-				 mc_filter);
+		     i++, mclist = mclist->next) {
+			int bit_nr = ether_crc(ETH_ALEN, mclist->dmi_addr) >> 26;
+
+			mc_filter[bit_nr >> 5] |= 1 << (bit_nr & 31);
+		}
 	}
 
 	/* if called from irq handler, lock already acquired */
@@ -1908,7 +1906,7 @@ static int netdrv_suspend (struct pci_de
 	unsigned long flags;
 
 	if (!netif_running(dev))
-		return;
+		return 0;
 	netif_device_detach (dev);
 
 	spin_lock_irqsave (&tp->lock, flags);
@@ -1923,7 +1921,8 @@ static int netdrv_suspend (struct pci_de
 
 	spin_unlock_irqrestore (&tp->lock, flags);
 
-	pci_power_off (pdev, -1);
+	pci_save_state (pdev, tp->pci_state);
+	pci_set_power_state (pdev, 3);
 
 	return 0;
 }
@@ -1932,10 +1931,12 @@ static int netdrv_suspend (struct pci_de
 static int netdrv_resume (struct pci_dev *pdev)
 {
 	struct net_device *dev = pci_get_drvdata (pdev);
+	struct netdrv_private *tp = dev->priv;
 
 	if (!netif_running(dev))
-		return;
-	pci_power_on (pdev);
+		return 0;
+	pci_set_power_state (pdev, 0);
+	pci_restore_state (pdev, tp->pci_state);
 	netif_device_attach (dev);
 	netdrv_hw_start (dev);
 

Re: [PATCH] Updated pci-skeleton.c

[Jeff Garzik]

applied, but please make an effort in the future to send patches that 
are -not- base64 encoded.  this breaks my apply-patch scripts.

	Jeff

Re: [PATCH] Update in-kernel orinoco drivers to upstream current CVS

[Francois Romieu]

Francois Romieu <romieu@fr.zoreil.com> :
[...]

Down to 110 ko.

Updated patches are available at
http://www.fr.zoreil.com/linux/kernel/2.6.x/2.6.7-mm6

It compiles due to cast abuse but I'll have to modify the order to
get the dependencies right.

Dumb question: how should the patches be published once they start to be
ready for wider review ?

--
Ueimor

Re: [PATCH] Update in-kernel orinoco drivers to upstream current CVS

[Jeff Garzik]

Francois Romieu wrote:
> Dumb question: how should the patches be published once they start to be
> ready for wider review ?


Individual emails or foo.tar.bz2, I would think...

	Jeff

Re: [PATCH] Update in-kernel orinoco drivers to upstream current CVS

[Jeff Garzik]

Jean Tourrilhes wrote:
> 	Jeff, does BitKeeper allow to merge patches at an earlier
> point than the last version and reconciliate both branches ? That
> might come handy.


Yes, this is one of BitKeeper's strengths.

I can create a BitKeeper repository circa 2.6.0, merge a patch, and then 
merge that repository into the 2.6.7-BK-latest repository.

	Jeff