Re: [PATCH 1/1] driver: Tpm hardware enablement --updated version

[Greg KH <greg@kroah.com>]

On Thu, Dec 16, 2004 at 04:37:34PM -0600, Kylene Hall wrote:
> +config TCG_TPM
> +	tristate "TPM Hardware Support"
> +	depends on EXPERIMENTAL
> +	---help---
> +	  If you have a TPM security chip in your system, which
> +	  implements the Trusted Computing Group's specification,
> +	  say Yes and it will be accessible from within Linux. To 
> +	  compile this driver as a module, choose M here; the module 
> +	  will be called tpm. For more information see 
> +	  www.trustedcomputinggroup.org. A implementation of the 
> +	  Trusted Software Stack (TSS), the userspace enablement piece 
> +	  of the specification, can be obtained at 
> +	  http://sourceforge.net/projects/trousers
> +	  If unsure, say N.

What happened to the "if built as a module..." text?

> +
> +config TCG_NSC
> +	tristate "National Semiconductor TPM Interface"
> +	depends on TCG_TPM
> +
> +config TCG_ATMEL
> +	tristate "Atmel TPM Interface"
> +	depends on TCG_TPM

Please provide help text for these options.

> +/*
> + * Vendor specific TPMs will have a unique name and probe function.
> + * Those fields should be populated prior to calling this function in
> + * tpm_<specific>.c's module init function.
> + */
> +int register_tpm_driver(struct pci_driver *drv)
> +{
> +	drv->id_table = tpm_pci_tbl;
> +	drv->remove = __devexit_p(tpm_remove);
> +	drv->suspend = tpm_pm_suspend;
> +	drv->resume = tpm_pm_resume;
> +
> +	return pci_register_driver(drv);
> +}
> +
> +EXPORT_SYMBOL(register_tpm_driver);

Why not EXPORT_SYMBOL_GPL()?  Based on the content of these drivers, I'd
feel better if they all were that way, but that's just me :)

Actually, why even have this function at all?  It's not needed, just
export the suspend, resume, and remove functions, and you are set.

Also, don't say that other drivers really support the other pci devices,
when they do not.  The MODULE_DEVICE_TABLE() stuff needs to be in the
driver that actually supports that hardware.  Otherwise all of the
hotplug functionality will not work properly.

> +
> +void unregister_tpm_driver(struct pci_driver *drv)
> +{
> +	pci_unregister_driver(drv);
> +}
> +
> +EXPORT_SYMBOL(unregister_tpm_driver);

Um, why even have such a function?


> +EXPORT_SYMBOL(register_tpm_hardware);

EXPORT_SYMBOL_GPL() (same goes for all of these exported symbols...)

> diff -uprN linux-2.6.9/drivers/char/tpm.h linux-2.6.9-tpm/drivers/char/tpm.h
> --- linux-2.6.9/drivers/char/tpm.h	1969-12-31 18:00:00.000000000 -0600
> +++ linux-2.6.9-tpm/drivers/char/tpm.h	2004-12-16 17:16:50.000000000 -0600
> +extern void tpm_time_expired(unsigned long);
> +extern int rdx(int);
> +extern void wrx(int, int);

Please use better names for these functions.  That's very cryptic for a
global symbol.

> +extern int lpc_bus_init(struct pci_dev *, u16);

No "tpm"?

> +extern int register_tpm_driver(struct pci_driver *);
> +extern void unregister_tpm_driver(struct pci_driver *);
> +extern int register_tpm_hardware(struct pci_dev *, struct tpm_chip_ops *,
> +				 u16);

Try putting "tpm" first here, for these functions, so the namespace is sane.

thanks,

greg k-h