linux-kernel - Re: [PATCH] Cleanup pc87413 watchdog driver to use request_muxed

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20110520162054.GI28362@earth.li>
Date:	Fri, 20 May 2011 09:20:54 -0700
From:	Jonathan McDowell <noodles@...th.li>
To:	Wim Van Sebroeck <wim@...ana.be>
Cc:	linux-watchdog@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] Cleanup pc87413 watchdog driver to use
 request_muxed_region for SuperIO area

On Fri, May 20, 2011 at 02:21:29PM +0200, Wim Van Sebroeck wrote:
> Hi Jonathan,
> 
> > > @@ -528,7 +511,8 @@ static int __init pc87413_init(void)
> > >  	printk(KERN_INFO PFX "Version " VERSION " at io 0x%X\n",
> > >  							WDT_INDEX_IO_PORT);
> > >  
> > > -	/* request_region(io, 2, "pc87413"); */
> > > +	if (!request_muxed_region(io, 2, MODNAME))
> > > +		return -EBUSY;
> > >  
> > >  	ret = register_reboot_notifier(&pc87413_notifier);
> > >  	if (ret != 0) {
> > > @@ -541,12 +525,32 @@ static int __init pc87413_init(void)
> > >  		printk(KERN_ERR PFX
> > >  			"cannot register miscdev on minor=%d (err=%d)\n",
> > >  			WATCHDOG_MINOR, ret);
> > > -		unregister_reboot_notifier(&pc87413_notifier);
> > > -		return ret;
> > > +		goto reboot_unreg;
> > >  	}
> > >  	printk(KERN_INFO PFX "initialized. timeout=%d min \n", timeout);
> > > +
> > > +	pc87413_select_wdt_out();
> > > +	pc87413_enable_swc();
> > > +	pc87413_get_swc_base_addr();
> > > +
> > > +	if (!request_region(swc_base_addr, 0x20, MODNAME)) {
> > > +		printk(KERN_ERR PFX
> > > +			"cannot request SWC region at 0x%x\n", swc_base_addr);
> > > +		ret = -EBUSY;
> > > +		goto misc_unreg;
> > > +	}
> > > +
> > >  	pc87413_enable();
> > > +
> > > +	release_region(io, 2);
> > >  	return 0;
> > > +
> > > +misc_unreg:
> > > +	misc_deregister(&pc87413_miscdev);
> > > +reboot_unreg:
> > > +	unregister_reboot_notifier(&pc87413_notifier);
> > > +	release_region(io, 2);
> > > +	return ret;
> > >  }
> > >  
> > >  /**
> > > @@ -569,7 +573,7 @@ static void __exit pc87413_exit(void)
> > >  
> > >  	misc_deregister(&pc87413_miscdev);
> > >  	unregister_reboot_notifier(&pc87413_notifier);
> > > -	/* release_region(io, 2); */
> > > +	release_region(swc_base_addr, 0x20);
> > >  
> > >  	printk(KERN_INFO MODNAME " watchdog component driver removed.\n");
> > >  }

> I don't think this is the correct way to do this. You do the
> request_muxed_region only at init and exit of the module.

I do a single request_muxed_region in the init function, and release it
at the end of the init function. It's only necessary to hold the LPC
region while the WDT functionality is enabled and the base address for
the SWC block is retrieved.

After that point all access is via the SWC region, so there's no need to
call request_muxed_region. The call in the exit function releases the
SWC block. We're not holding the LPC region for the entire duration of
the driver being loaded.

J.

-- 
  Hail Eris. All hail Discordia.   |  .''`.  Debian GNU/Linux Developer
              Fnord?               | : :' :  Happy to accept PGP signed
                                   | `. `'   or encrypted mail - RSA
                                   |   `-    key on the keyservers.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/