Thread: [Ndiswrapper-general] [PATCH/RFC] ndiswrapper: add ndiswrapper_fix_broken_driver call
Status: Beta
Brought to you by:
pgiri
From: Prarit B. <pr...@sg...> - 2005-05-19 12:07:12
Attachments:
diff
|
Ndiswrapper coders, This patch is an effort to address strange hardware that only supports 30-bit addressing and thus requires that the DMA masks for a particular wireless device be set appropriately. Since there are likely many such "broken" devices out there, I felt it best to implement a broken_driver function call which could fix/modify settings that a driver might assume was set up prior to loading. In the specific case I was dealing with, the Broadcom netbc5 64-bit driver erroneously assumes that the DMA mask on the card has been set prior to loading. In fact, this is not the case and this leads to hangs and other strange behaviour. The fix was in response to a modification made in CVS where GFP_DMA was tagged onto all DMA requests. The GFP_DMA flag requests memory for the DMA transaction below 16M. IMO, while this "works" it is not truly a fix and can lead to performance issues. 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 /* apply other driver's modifications here */ is an indication of how to extend the call. The diff is generated against the ndiswrapper-1.2rc1 source tarball available at sourceforge. Thanks, P. |
From: Alex H. <ale...@wa...> - 2005-05-19 13:40:10
|
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 B. <pr...@sg...> - 2005-05-22 00:09:14
|
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 > Lin...@li... > http://lists.pcxperience.com/cgi-bin/mailman/listinfo/linuxr3000 > Wiki at http://prinsig.se/weekee/ |
From: D. H. R. <hu...@mi...> - 2005-05-19 19:55:06
|
| From: Prarit Bhargava <pr...@sg...> [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. |
From: Prarit B. <pr...@sg...> - 2005-05-20 11:51:58
|
D. Hugh Redelmeier wrote: > | From: Prarit Bhargava <pr...@sg...> > | + */ > | +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 > Ndi...@li... > https://lists.sourceforge.net/lists/listinfo/ndiswrapper-general > |
From: Zwane M. <zw...@ar...> - 2005-05-20 15:31:37
|
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: D. H. R. <hu...@mi...> - 2005-05-21 15:26:09
|
| From: Zwane Mwaikambo <zw...@ar...> | 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. |
From: Zwane M. <zw...@ar...> - 2005-05-21 19:20:55
|
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). |
From: Prarit B. <pr...@sg...> - 2005-05-22 00:20:33
Attachments:
diff
|
New patch with suggestions from Hugh. |
From: Giridhar P. <gi...@lm...> - 2005-05-22 01:34:48
Attachments:
dma-fix
|
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: D. H. R. <hu...@mi...> - 2005-05-22 04:25:22
|
| From: Giridhar Pemmasani <gi...@lm...> | | 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. |
From: Giridhar P. <gi...@lm...> - 2005-05-22 04:38:22
|
On Sun, 22 May 2005 00:25:07 -0400 (EDT), "D. Hugh Redelmeier" <hu...@mi...> 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 |
From: Prarit B. <pr...@sg...> - 2005-05-22 14:42:25
|
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. |
From: Prarit B. <pr...@sg...> - 2005-05-22 14:41:10
|
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. |
From: Giridhar P. <gi...@lm...> - 2005-05-22 15:14:03
|
Thanks, patch committed. Will soon release 1.2rc2, which hopefully becomes 1.2. -- Giri |
From: Prarit B. <pr...@sg...> - 2005-05-22 15:21:05
|
Giridhar Pemmasani wrote: > Thanks, patch committed. Will soon release 1.2rc2, which hopefully becomes > 1.2. > Thanks Giri. P. |