Re: [patch] ns558 bug

[ambx1@neo.rr.com (Adam Belay)]

On Fri, Feb 04, 2005 at 07:21:15PM -0800, Andrew Morton wrote:
> ambx1@neo.rr.com (Adam Belay) wrote:
> >
> >  It looks ok.  My only concern is what would happen if the isa probe succeded
> >  but the pnp_register_driver failed?  "pnp_register_driver" return -ENODEV if
> >  "CONFIG_PNP" isn't enabled.  Do you think this would conflict with legacy
> >  probing?
> 
> Fair enough.  How about this?

The code no longer is leaking devices.  I may be overlooking something, but I
think it still doesn't retain legacy probed devices when PnP isn't available.

If "pnp_register_driver" fails then "ns588_unregister_ports" will unregister
the correctly detected ISA ports.  "pnp_register_driver" will always fail if
pnp support isn't compiled into the kernel.  We need a way of not 
unregistering "ns558_pnp_driver" in "ns558_exit" but retaining the ports
detected by the legacy probe.  In a way, this is sort of a driver model
problem.

As a more general solution for all drivers, I've been thinking about doing
something like this in the long term.

int ret;
if (!(ret = register_driver(&ns558_driver)))
	return ret;
add_driver_protocol(&ns558_driver, &ns558_pnp);
add_driver_protocol(&ns558_driver, &ns558_isa);

and then on exit:

unregister_driver(&ns558_driver); /* this tears down any successfully
				     registered bus protocol automatically */


For now a less invasive solution might be better.

Comments?

Thanks,
Adam


> 
> static void ns588_unregister_ports(void)
> {
> 	struct ns558 *port;
> 
> 	list_for_each_entry(port, &ns558_list, node) {
> 		gameport_unregister_port(&port->gameport);
> 		switch (port->type) {
> 
> #ifdef CONFIG_PNP
> 			case NS558_PNP:
> 				/* fall through */
> #endif
> 			case NS558_ISA:
> 				release_region(port->gameport.io &
> 					~(port->size - 1), port->size);
> 				kfree(port);
> 				break;
> 
> 			default:
> 				break;
> 		}
> 	}
> }
> 
> static int __init ns558_init(void)
> {
> 	int i = 0;
> 	int ret;
> 
> /*
>  * Probe for ISA ports.
>  */
> 
> 	while (ns558_isa_portlist[i])
> 		ns558_isa_probe(ns558_isa_portlist[i++]);
> 
> 	ret = pnp_register_driver(&ns558_pnp_driver);
> 	if (ret < 0) {
> 		ns588_unregister_ports();
> 		return ret;
> 	}
> 	return 0;
> }
> 
> static void __exit ns558_exit(void)
> {
> 	ns588_unregister_ports();
> 	pnp_unregister_driver(&ns558_pnp_driver);
> }