![]() | |||||||||||||
Messages in this thread |
On Mon, 31 Jul 2006 12:41:36 +0200
Jan Beulich <jbeulich@novell.com> wrote:
> Previously, since determination whether there was an Intel random
> number generator was based on a single bit, on systems with a matching
> bridge device but without a firmware hub, there was a 50% chance that
> the code would incorrectly decide that the system had an RNG. This
> patch adds detection of the firmware hub to better qualify the
> existence of an RNG.> > There is one issue with the patch: I was unable to determine the LPC
> equivalent for the PCI bridge 8086:2430 (since the old code didn't
> care about which of the many devices provided by the ICH/ESB it was
> chose to use the PCI bridge device, but the FWH settings live in the
> LPC device, so the device list needed to be changed).
> > > ...> > +#ifdef CONFIG_SMP> +static char __initdata waitflag;> +> +static void __init intel_init_wait (void *unused)
No space before the (, please.
> +{> + while (waitflag) {> + cpu_relax();> + smp_rmb();> + }
I believe we decided that cpu_relax() implies a barrier.
> +#endif> +> static int __init mod_init(void)
> {
> int err = -ENODEV;
> + unsigned i;
> + struct pci_dev *dev = NULL;
> void __iomem *mem;
> - u8 hw_status;
> -
> - if (!pci_dev_present(pci_tbl))
> + unsigned long flags;
> + u8 bios_cntl_off, fwh_dec_en1_off;
> + u8 bios_cntl_val = 0xff, fwh_dec_en1_val = 0xff;
> + u8 hw_status, mfc, dvc;
> +
> + for (i = 0; !dev && pci_tbl[i].vendor; ++i)
> + dev = pci_get_device(pci_tbl[i].vendor, pci_tbl[i].device, NULL);
> +
> + if (!dev)
> goto out; /* Device not found. */
>
> + /* Check for Intel 82802 */
> + if (dev->device < 0x2640) {
> + fwh_dec_en1_off = FWH_DEC_EN1_REG_OLD;
> + bios_cntl_off = BIOS_CNTL_REG_OLD;
> + }
> + else {
Please do
} else {
> + fwh_dec_en1_off = FWH_DEC_EN1_REG_NEW;> + bios_cntl_off = BIOS_CNTL_REG_NEW;> + }> +> + pci_read_config_byte(dev, fwh_dec_en1_off, &fwh_dec_en1_val);
> + pci_read_config_byte(dev, bios_cntl_off, &bios_cntl_val);
> +> + mem = ioremap_nocache(INTEL_FWH_ADDR, INTEL_FWH_ADDR_LEN);
> + if (mem == NULL) {> + pci_dev_put(dev);> + err = -EBUSY;> + goto out;> + }> +> +#ifdef CONFIG_SMP> + waitflag = 1;> + smp_wmb();> + if (smp_call_function(intel_init_wait, NULL, 1, 0) != 0) {
> + waitflag = 0;> + smp_wmb();> + pci_dev_put(dev);> + printk(KERN_ERR PFX "cannot run on all processors\n");
> + err = -EAGAIN;> + goto err_unmap;> + }> +#else> +#define waitflag err> +#endif
awww man, this is nasty. We're #defining the name of a file-global
variable to that it puns a local? On uniproc only?
Please, not in Linux. Let's find a better way.
> + local_irq_save(flags);
I think the code needs a comment explaining the local_irq_save(). Its
reasoning is not apparent from reading the implementation.
> + if (!(fwh_dec_en1_val & FWH_F8_EN_MASK))
> + pci_write_config_byte(dev,
> + fwh_dec_en1_off,
> + fwh_dec_en1_val | FWH_F8_EN_MASK);
> + if (!(bios_cntl_val
> + & (BIOS_CNTL_LOCK_ENABLE_MASK|BIOS_CNTL_WRITE_ENABLE_MASK)))
> + pci_write_config_byte(dev,
> + bios_cntl_off,
> + bios_cntl_val | BIOS_CNTL_WRITE_ENABLE_MASK);
> +
> + writeb(INTEL_FWH_RESET_CMD, mem);
> + writeb(INTEL_FWH_READ_ID_CMD, mem);
> + mfc = readb(mem + INTEL_FWH_MANUFACTURER_CODE_ADDRESS);
> + dvc = readb(mem + INTEL_FWH_DEVICE_CODE_ADDRESS);
> + writeb(INTEL_FWH_RESET_CMD, mem);
> +
> + if (!(bios_cntl_val
> + & (BIOS_CNTL_LOCK_ENABLE_MASK|BIOS_CNTL_WRITE_ENABLE_MASK)))
It would be (a little) more conventional to do
if (!(bios_cntl_val &
> + pci_write_config_byte(dev, bios_cntl_off, bios_cntl_val);
> + if (!(fwh_dec_en1_val & FWH_F8_EN_MASK))
> + pci_write_config_byte(dev, fwh_dec_en1_off, fwh_dec_en1_val);
> +
> + local_irq_restore(flags);
> + waitflag = 0;
> + smp_wmb();
Again, the barrier is hard to understand. Pretty much all open-coded
barriers should have an explanatory comment.
> + iounmap(mem);> + pci_dev_put(dev);> +> + if (mfc != INTEL_FWH_MANUFACTURER_CODE> + || (dvc != INTEL_FWH_DEVICE_CODE_8M> + && dvc != INTEL_FWH_DEVICE_CODE_4M)) {
Again,
if (mfc != INTEL_FWH_MANUFACTURER_CODE ||
(dvc != INTEL_FWH_DEVICE_CODE_8M &&
dvc != INTEL_FWH_DEVICE_CODE_4M)) {
would be more typical.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
| ||||||||||||
| Last update: 2006-08-01 07:39 [from the cache] ©2003-2008 | |||||||||||||