ndiswrapper

Prarit Bhargava wrote:
> I am by no means an ndiswrapper expert -- the below is simply an RFC.  
> I've implmented ndiswrapper_fix_broken_driver which is intended to be a 
> list of strcmp's -- one for each known "broken" driver.  The comment

Shouldn't it check for the specific broken version of the driver too, 
instead of just the name? Although unlikely, broken windows drivers 
sometimes do get fixed.


Alex.

| From: Prarit Bhargava <prarit@...>

[Note: these are based purely on reading the patch.  I am not a kernel
hacker nor am I an ndiswrapper hacker.]

| +/*
| + * ndiswrapper_fix_broken_driver - apply additional HW modifications that are
| + * required to get a driver to work properly under Linux.
| + * @usb_pci_device: pointer to the USB or PCI device that may be modified
| + * @driver_name: pointer to the driver's name
| + *
| + * Applies appropriate modifications to a USB or PCI device if the driver_name
| + * specified is going to be loaded.
| + *
| + */
| +static void ndiswrapper_fix_broken_driver(void *usb_pci_device,
| +					  char *driver_name)

I think that this function should return an indication of success or
failure.  In particular, I suspect that the BC564 is *dangerous* if
the fix does not install.  Why?  Because it probably zaps memory at
the address of the buffer mod 2^30, and without the fix, this memory
is going to belong to something else.  If the fix doesn't install, the
driver load should fail.


The first argument is of type void *.  As a C purist, I think that it
should be typed precisely.  When is it a struct pci_dev *?  What types
might it have?  It looks to me:

- struct pci_dev *
- struct usb_interface *	/* pre 2.6 kernel */
- struct usb_device *		/* 2.6 or later kernel */

I would say that passing these in one parameter is questionable.
Clearly the two USB pointers can be passed in one parameter -- they
perform the same function in different kernel versions.  I'd pass the
pci_dev pointer as a separate parameter.

I actually find it surprising that struct ndis_device does not contain
these pointers.  I would have thought that code would want to get from
a struct ndis_device * to a corresponding struct pci_dev *.

An even cleaner alternative would be to have separate fixup functions
for USB and PCI.  Unfortunately, that distinction has been lost inside
ndiswrapper_load_driver.

| +		struct pci_dev *pdev = usb_pci_device;

This just casts the void *.  If you separate the parameters as I just
suggested, the declaration is not needed.

| +		if (pci_set_dma_mask(pdev, 0x3fffffff))
| +			goto fix_error;
| +		if (pci_set_consistent_dma_mask(pdev, 0x3fffffff))
| +			goto fix_error;
| +
| +		printk(KERN_INFO "ndiswrapper: 30-bit Broadcom Wireless "
| +		       "Work Around enabled\n");
| +		return;
| +fix_error:
| +		printk(KERN_WARNING "ndiswrapper: 30-bit Broadcom Wireless "
| +		       "Work Around could not be implemented\n");

I dislike gotos.  I would have written this section as:

		if (pci_set_dma_mask(pdev, 0x3fffffff)
		|| pci_set_consistent_dma_mask(pdev, 0x3fffffff)) {
			printk(KERN_WARNING "ndiswrapper: 30-bit Broadcom Wireless "
			       "Work Around could not be implemented\n");
		} else {
			printk(KERN_INFO "ndiswrapper: 30-bit Broadcom Wireless "
			       "Work Around enabled\n");
		}

Not only is this shorter, but it has another advantage: it allows for
later code in the function to apply.

D. Hugh Redelmeier wrote:
> | From: Prarit Bhargava <prarit@...>

> | + */
> | +static void ndiswrapper_fix_broken_driver(void *usb_pci_device,
> | +                      char *driver_name)
> 
> I think that this function should return an indication of success or
> failure.  In particular, I suspect that the BC564 is *dangerous* if
> the fix does not install.  Why?  Because it probably zaps memory at
> the address of the buffer mod 2^30, and without the fix, this memory
> is going to belong to something else.  If the fix doesn't install, the
> driver load should fail.
> 

Sounds like a good suggestion.

> 
> The first argument is of type void *.  As a C purist, I think that it
> should be typed precisely.  When is it a struct pci_dev *?  What types
> might it have?  It looks to me:
> 
> - struct pci_dev *
> - struct usb_interface *    /* pre 2.6 kernel */
> - struct usb_device *        /* 2.6 or later kernel */
> 
> I would say that passing these in one parameter is questionable.
> Clearly the two USB pointers can be passed in one parameter -- they
> perform the same function in different kernel versions.  I'd pass the
> pci_dev pointer as a separate parameter.

Hmmm ... I'm casting the the void pointer if/when I need it, so IMO the code 
should be okay.  I'm too busy trying to save stack space I guess.  I'll change 
it to supply both a pci arg and a usb arg.

> I dislike gotos.  I would have written this section as:
> 
>         if (pci_set_dma_mask(pdev, 0x3fffffff)
>         || pci_set_consistent_dma_mask(pdev, 0x3fffffff)) {
>             printk(KERN_WARNING "ndiswrapper: 30-bit Broadcom Wireless "
>                    "Work Around could not be implemented\n");
>         } else {
>             printk(KERN_INFO "ndiswrapper: 30-bit Broadcom Wireless "
>                    "Work Around enabled\n");
>         }
> 
> Not only is this shorter, but it has another advantage: it allows for
> later code in the function to apply.

Sounds better.  Like I said :) I just hacked something together for an RFC. :)

P.
> 
> 
> -------------------------------------------------------
> This SF.Net email is sponsored by Oracle Space Sweepstakes
> Want to be the first software developer in space?
> Enter now for the Oracle Space Sweepstakes!
> http://ads.osdn.com/?ad_id=7412&alloc_id=16344&op=click
> _______________________________________________
> Ndiswrapper-general mailing list
> Ndiswrapper-general@...
> https://lists.sourceforge.net/lists/listinfo/ndiswrapper-general
>

On Fri, 20 May 2005, Prarit Bhargava wrote:

> > The first argument is of type void *.  As a C purist, I think that it
> > should be typed precisely.  When is it a struct pci_dev *?  What types
> > might it have?  It looks to me:
> > 
> > - struct pci_dev *
> > - struct usb_interface *    /* pre 2.6 kernel */
> > - struct usb_device *        /* 2.6 or later kernel */
> > 
> > I would say that passing these in one parameter is questionable.
> > Clearly the two USB pointers can be passed in one parameter -- they
> > perform the same function in different kernel versions.  I'd pass the
> > pci_dev pointer as a separate parameter.
> 
> Hmmm ... I'm casting the the void pointer if/when I need it, so IMO the code
> should be okay.  I'm too busy trying to save stack space I guess.  I'll change
> it to supply both a pci arg and a usb arg.

What happens when you get more busses?

> > I dislike gotos.  I would have written this section as:
> > 
> >         if (pci_set_dma_mask(pdev, 0x3fffffff)
> >         || pci_set_consistent_dma_mask(pdev, 0x3fffffff)) {
> >             printk(KERN_WARNING "ndiswrapper: 30-bit Broadcom Wireless "
> >                    "Work Around could not be implemented\n");
> >         } else {
> >             printk(KERN_INFO "ndiswrapper: 30-bit Broadcom Wireless "
> >                    "Work Around enabled\n");
> >         }
> > 
> > Not only is this shorter, but it has another advantage: it allows for
> > later code in the function to apply.
> 
> Sounds better.  Like I said :) I just hacked something together for an RFC. :)

gotos are perfect for things like error handling, you'll notice that a lot 
of the kernel code uses gotos extensively for that. It's better than a 
mish mash of if clauses with cleanup code in each. You just have the goto 
label and have cleanups in one location.

	func1()
	func2()
	if (error) {
		func1_cleanup();
		return;
	}

	func3()
	if (error) {
		func1_cleanup();
		func2_cleanup();
		return;
	}
	

You can see this gets messy very quickly and actually generates horrible 
code.

	func1();
	func2();
	if (error)
		goto error_cleanup_func1;

	func3();
	if (error)
		goto error_cleanup_func2;

	goto done;

error_cleanup_func1:
	func1_cleanup();
error_cleanup_func2:
	func2_cleanup();
done:
	return error;

| From: Zwane Mwaikambo <zwane@...>

| On Fri, 20 May 2005, Prarit Bhargava wrote:
| 
| > > The first argument is of type void *.  As a C purist, I think that it
| > > should be typed precisely.  When is it a struct pci_dev *?  What types
| > > might it have?  It looks to me:
| > > 
| > > - struct pci_dev *
| > > - struct usb_interface *    /* pre 2.6 kernel */
| > > - struct usb_device *        /* 2.6 or later kernel */
| > > 
| > > I would say that passing these in one parameter is questionable.
| > > Clearly the two USB pointers can be passed in one parameter -- they
| > > perform the same function in different kernel versions.  I'd pass the
| > > pci_dev pointer as a separate parameter.
| > 
| > Hmmm ... I'm casting the the void pointer if/when I need it, so IMO the code
| > should be okay.  I'm too busy trying to save stack space I guess.  I'll change
| > it to supply both a pci arg and a usb arg.
| 
| What happens when you get more busses?

The fundamental problem is that the code assumes it knows what the
pointer points to based on the name of the device.  That isn't safe.
A secondary problem is that with the original code the compiler is
unable to statically check that the code gets the types right.

If you don't like a pointer parameter per bus, a more scalable
approach would be to pass an enum parameter to specify the type of
pointer (bus) to supplement the void *.  This eliminates the
fundamental problem but not the secondary one.

At this point, the enum form seems like overengineering considering
that there are only two pointer types currently.

| gotos are perfect for things like error handling, you'll notice that a lot 
| of the kernel code uses gotos extensively for that. It's better than a 
| mish mash of if clauses with cleanup code in each. You just have the goto 
| label and have cleanups in one location.

gotos are not *perfect* for things like error handling.  They are a
useful pragmatic tool for doing things that are not well supported by
C control structures.  Another thing they are good for is the direct
representation of state machines.

The code in question is clearer without gotos.

| You can see this gets messy very quickly and actually generates horrible 
| code.

It is easy to come up with arbitrary examples where the goto version
is better.  Often rethinking the code leads to cleaner code with fewer
gotos.  Hard to do without context.  Just mechanically adding or
removing gotos is unlikely to lead to cleaner code -- it is the idea
that should be clearer, not just the representation.

On Sat, 21 May 2005, D. Hugh Redelmeier wrote:

> gotos are not *perfect* for things like error handling.  They are a
> useful pragmatic tool for doing things that are not well supported by
> C control structures.  Another thing they are good for is the direct
> representation of state machines.
> 
> The code in question is clearer without gotos.
> 
> | You can see this gets messy very quickly and actually generates horrible 
> | code.
> 
> It is easy to come up with arbitrary examples where the goto version
> is better.  Often rethinking the code leads to cleaner code with fewer
> gotos.  Hard to do without context.  Just mechanically adding or
> removing gotos is unlikely to lead to cleaner code -- it is the idea
> that should be clearer, not just the representation.

I won't argue as it's a bit offtopic here and i apologise for even 
bringing it up, but with respect to error handling, in my experience, 
there isn't much to "rethink" when it comes to unwinding and tearing 
things down (in this case C kernel code).

Alex Hermann wrote:
> Prarit Bhargava wrote:
> 
>> I am by no means an ndiswrapper expert -- the below is simply an RFC.  
>> I've implmented ndiswrapper_fix_broken_driver which is intended to be 
>> a list of strcmp's -- one for each known "broken" driver.  The comment
> 
> 
> Shouldn't it check for the specific broken version of the driver too, 
> instead of just the name? Although unlikely, broken windows drivers 
> sometimes do get fixed.
> 

I looked into this and I suspect that the reason I can't do it is 
because I do not have enough insight into ndiswrapper.

The version of the driver is only avaiable AFTER the driver is loaded in 
ndiswrapper_load_driver.  I'm not sure if I'm the person to add a 
driver->driver_version field (due to my inexperience with the 
ndiswrapper sources and arch).

The driver version appears as ",10/01/2002,3.70.17.5" when dumped after 
  the driver is loaded.

P.

> 
> Alex.
> _______________________________________________
> LinuxR3000 mailing list
> LinuxR3000@...
> http://lists.pcxperience.com/cgi-bin/mailman/listinfo/linuxr3000
> Wiki at http://prinsig.se/weekee/

New patch with suggestions from Hugh.

IIUC, the following simpler patch should work. Correct me if I am wrong. At
this point we don't know if any other drivers have the same problem; when we
get to another one, we can make a generic fix function.

| From: Giridhar Pemmasani <giri@...>
| 
| IIUC, the following simpler patch should work. Correct me if I am wrong. At
| this point we don't know if any other drivers have the same problem; when we
| get to another one, we can make a generic fix function.

I think that if the mask installation fails, then the loading of the
driver should fail.  The reason is that if a buffer is allocated above
1G, the device will probably zap memory not owned by it (explained in
earlier email).

Why were you able to eliminate this memcpy?
-		memcpy(&ndis_device->driver_name, device->driver_name,
-		       sizeof(ndis_device->driver_name));
This seems unrelated to the bc564 problem.

I assume that you have not tested this -- I don't think you have a
machine with this device.

On Sun, 22 May 2005 00:25:07 -0400 (EDT), "D. Hugh Redelmeier" <hugh@...> said:

  D> driver should fail.  The reason is that if a buffer is allocated above
  D> 1G, the device will probably zap memory not owned by it (explained in
  D> earlier email).

But if the user doesn't have more than 1GB, it is not an issue, so only the
warning is given. If the device doesn't work with more than 1GB, user should
know the issue from the warning.

  D> Why were you able to eliminate this memcpy?  -

It is unrelated to this issue; it is a duplicate memcpy.

-- 
Giri

Giridhar Pemmasani wrote:
> IIUC, the following simpler patch should work. Correct me if I am wrong. At
> this point we don't know if any other drivers have the same problem; when we
> get to another one, we can make a generic fix function.
> 
> 

Yep, that works.  But, I'm trying to be a bit more general about it 
though.  I'm wondering if some of the other "broken" drivers we've heard 
about might have similar setup issues.

P.

D. Hugh Redelmeier wrote:

> Why were you able to eliminate this memcpy?
> -		memcpy(&ndis_device->driver_name, device->driver_name,
> -		       sizeof(ndis_device->driver_name));
> This seems unrelated to the bc564 problem.

It's done TWICE (look about five-ten lines above). I just noticed it 
while examining the code.

P.

Thanks, patch committed. Will soon release 1.2rc2, which hopefully becomes
1.2. 

-- 
Giri

Giridhar Pemmasani wrote:
> Thanks, patch committed. Will soon release 1.2rc2, which hopefully becomes
> 1.2. 
> 

Thanks Giri.

P.