lkml.org 
[lkml]   [2008]   [Aug]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH] ne.c fix for hibernate and rmmod oops fix
From
On Wed, 27 Aug 2008 23:11:15 -0500, David Fries <david@fries.net> wrote:
> do_ne_probe uses dev->mem_end special value 0xbad as a flag to deal
> specially when some bad, but usable devices. The io port platform
> resource has a start and end, but if start is 0x300 and end is 0xbad,
> it will just fail with io ports in use and not probe the device. Is
> using the flag IORESOURCE_DISABLED acceptable? There doesn't seem to
> be any good way to pass some extra driver dependent data through to
> the platform probe function.
>
> + /* Bad, crippled, how about disabled?
> + * Any other suggestions for passing this through?
> + */
> + if (dev->mem_end == BAD)
> + r[0].flags |= IORESOURCE_DISABLED;

I'm not sure IORESOURCE_DISABLED is OK, but (ab)using r[0].name might
be safer. Something like this:

if (dev->mem_end == BAD)
r[0].name = "bad";

> platform_driver support was added earlier, but without any
> platform_device_register* calls I don't think it was being used.

Well, I added platform_driver support for some MIPS board. I did not
touch ISA part at that time while it looks too complex for me. Thank
you for better integration.

> Now all devices are registered using platform_device_register_simple and
> pointers are kept to unregister the ones that the probe failed for or
> unregister all devices on module shutdown. ne_init and ne_exit are no
> longer compiled into the module to reduce confusion (and multiple
> unregister paths that caused the rmmod oops). With the devices
> now registered they are added to the platform driver and get suspend
> and resume events. A call to pnp_stop_dev and pnp_start_dev now
> shutsdown and initializes plug and play devices.

With your patch, cleanup_module() and ne_exit() is exactly same. How
about unifying them?

> mdelay(10) replaced by msleep(10) to give up the CPU.

I think this part is worth to do by separate patch.

> + struct resource r[2] = {
> + {
> + .name = "io_port",
> + .flags = IORESOURCE_IO},
> + {
> + .name = "irq",
> + .flags = IORESOURCE_IRQ} };

This .name initialization seems a bit redundant for me. They can be
left NULL.

Also I'd suggest CC-ing your patch to netdev@vger.kerne.org for more
reviews from genius people.

---
Atsushi Nemoto


\
 
 \ /
  Last update: 2008-08-29 17:27    [W:0.075 / U:0.172 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site