lkml.org 
[lkml]   [2006]   [Aug]   [1]   [last100]   RSS Feed
Views: [more markup]  [less markup]  [headers]  [forward] 
 
Messages in this thread
/
DateMon, 31 Jul 2006 22:35:59 -0700
FromAndrew Morton <>
SubjectRe: [PATCH] fix Intel RNG detection (take 2)
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