Re: [tpmdd-devel] [PATCH v7 05/10] tpm: device class for tpm

[Stefan Berger <stefanb@linux.vnet.ibm.com>]

On 11/11/2014 08:45 AM, Jarkko Sakkinen wrote:
> Added own device class for TPM. Uses MISC_MAJOR:TPM_MINOR for the
> first character device in order to retain backwards compatability.
> Added tpm_dev_release() back attached to the character device.
> devm now only calls put_device when the platform device is removed.
>
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> ---
>   drivers/char/tpm/tpm-chip.c        | 81 ++++++++++++++++++++++++++++++++++----
>   drivers/char/tpm/tpm-dev.c         | 36 ++---------------
>   drivers/char/tpm/tpm-interface.c   | 29 ++++++++++++++
>   drivers/char/tpm/tpm.h             | 12 ++++--
>   drivers/char/tpm/tpm_i2c_nuvoton.c |  2 +-
>   drivers/char/tpm/tpm_tis.c         |  4 +-
>   6 files changed, 116 insertions(+), 48 deletions(-)
>
> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> index 96ea8eb..df40eee 100644
> --- a/drivers/char/tpm/tpm-chip.c
> +++ b/drivers/char/tpm/tpm-chip.c
> @@ -25,6 +25,7 @@
>   #include <linux/mutex.h>
>   #include <linux/spinlock.h>
>   #include <linux/freezer.h>
> +#include <linux/major.h>
>   #include "tpm.h"
>   #include "tpm_eventlog.h"
>
> @@ -32,6 +33,9 @@ static DECLARE_BITMAP(dev_mask, TPM_NUM_DEVICES);
>   static LIST_HEAD(tpm_chip_list);
>   static DEFINE_SPINLOCK(driver_lock);
>
> +struct class *tpm_class;
> +dev_t tpm_devt;
> +
>   /*
>    * tpm_chip_find_get - return tpm_chip for a given chip number
>    * @chip_num the device number for the chip
> @@ -55,16 +59,24 @@ struct tpm_chip *tpm_chip_find_get(int chip_num)
>   }
>
>   /**
> - * tpmm_chip_remove() - free chip memory and device number
> - * @data: points to struct tpm_chip instance
> + * tpmm_put_device() - wrap put_device() for devm
> + * @data: points to the device
> + */
> +static void tpmm_put_device(void *data)
> +{
> +	struct device *dev = (struct device *) data;
> +	put_device(dev);
> +}
> +
> +/**
> + * tpm_dev_release() - free chip memory and the device number
> + * @dev: the character device for the TPM chip
>    *
> - * This is used internally by tpmm_chip_alloc() and called by devres
> - * when the device is released. This function does the opposite of
> - * tpmm_chip_alloc() freeing memory and the device number.
> + * This is used as the release function for the character device.
>    */
> -static void tpmm_chip_remove(void *data)
> +static void tpm_dev_release(struct device *dev)
>   {
> -	struct tpm_chip *chip = (struct tpm_chip *) data;
> +	struct tpm_chip *chip = container_of(dev, struct tpm_chip, dev);
>
>   	spin_lock(&driver_lock);
>   	clear_bit(chip->dev_num, dev_mask);
> @@ -112,13 +124,66 @@ struct tpm_chip *tpmm_chip_alloc(struct device *dev,
>   		  chip->dev_num);
>
>   	chip->pdev = dev;
> -	devm_add_action(dev, tpmm_chip_remove, chip);
> +
>   	dev_set_drvdata(dev, chip);
>
> +	chip->dev.class = tpm_class;
> +	chip->dev.release = tpm_dev_release;
> +	chip->dev.parent = chip->pdev;
> +
> +	if (chip->dev_num == 0)
> +		chip->dev.devt = MKDEV(MISC_MAJOR, TPM_MINOR);
> +	else
> +		chip->dev.devt = MKDEV(MAJOR(tpm_devt), chip->dev_num);
> +
> +	dev_set_name(&chip->dev, chip->devname);
> +
> +	device_initialize(&chip->dev);
> +
> +	chip->cdev.owner = chip->pdev->driver->owner;
> +	cdev_init(&chip->cdev, &tpm_fops);
> +
> +	devm_add_action(dev, tpmm_put_device, chip);
>   	return chip;
>   }
>   EXPORT_SYMBOL_GPL(tpmm_chip_alloc);
>
> +static int tpm_dev_add_device(struct tpm_chip *chip)
> +{
> +	int rc;
> +
> +	rc = device_add(&chip->dev);
> +	if (rc) {
> +		dev_err(&chip->dev,
> +			"unable to device_register %s, major %d, minor %d " \
> +			"err=%d\n",
> +			chip->devname, MAJOR(chip->dev.devt),
> +			MINOR(chip->dev.devt), rc);
> +
> +		return rc;
> +	}
> +
> +	rc = cdev_add(&chip->cdev, chip->dev.devt, 1);
> +	if (rc) {
> +		dev_err(&chip->dev,
> +			"unable to cdev_add %s, major %d, minor %d " \
> +			"err=%d\n",
> +			chip->devname, MAJOR(chip->dev.devt),
> +			MINOR(chip->dev.devt), rc);
> +
> +		device_unregister(&chip->dev);
> +		return rc;
> +	}
> +
> +	return rc;
> +}
> +
> +static void tpm_dev_del_device(struct tpm_chip *chip)
> +{
> +	cdev_del(&chip->cdev);
> +	device_unregister(&chip->dev);
> +}
> +
>   /*
>    * tpm_chip_register() - create a misc driver for the TPM chip
>    * @chip: TPM chip to use.
> diff --git a/drivers/char/tpm/tpm-dev.c b/drivers/char/tpm/tpm-dev.c
> index 3568321..de0337e 100644
> --- a/drivers/char/tpm/tpm-dev.c
> +++ b/drivers/char/tpm/tpm-dev.c
> @@ -17,7 +17,6 @@
>    * License.
>    *
>    */
> -#include <linux/miscdevice.h>
>   #include <linux/slab.h>
>   #include <linux/uaccess.h>
>   #include "tpm.h"
> @@ -54,9 +53,8 @@ static void timeout_work(struct work_struct *work)
>
>   static int tpm_open(struct inode *inode, struct file *file)
>   {
> -	struct miscdevice *misc = file->private_data;
> -	struct tpm_chip *chip = container_of(misc, struct tpm_chip,
> -					     vendor.miscdev);
> +	struct tpm_chip *chip =
> +		container_of(inode->i_cdev, struct tpm_chip, cdev);
>   	struct file_priv *priv;
>
>   	/* It's assured that the chip will be opened just once,
> @@ -173,7 +171,7 @@ static int tpm_release(struct inode *inode, struct file *file)
>   	return 0;
>   }
>
> -static const struct file_operations tpm_fops = {
> +const struct file_operations tpm_fops = {
>   	.owner = THIS_MODULE,
>   	.llseek = no_llseek,
>   	.open = tpm_open,
> @@ -182,32 +180,4 @@ static const struct file_operations tpm_fops = {
>   	.release = tpm_release,
>   };
>
> -int tpm_dev_add_device(struct tpm_chip *chip)
> -{
> -	int rc;
>
> -	chip->vendor.miscdev.fops = &tpm_fops;
> -	if (chip->dev_num == 0)
> -		chip->vendor.miscdev.minor = TPM_MINOR;
> -	else
> -		chip->vendor.miscdev.minor = MISC_DYNAMIC_MINOR;
> -
> -	chip->vendor.miscdev.name = chip->devname;
> -	chip->vendor.miscdev.parent = chip->pdev;
> -
> -	rc = misc_register(&chip->vendor.miscdev);
> -	if (rc) {
> -		chip->vendor.miscdev.name = NULL;
> -		dev_err(chip->pdev,
> -			"unable to misc_register %s, minor %d err=%d\n",
> -			chip->vendor.miscdev.name,
> -			chip->vendor.miscdev.minor, rc);
> -	}
> -	return rc;
> -}
> -
> -void tpm_dev_del_device(struct tpm_chip *chip)
> -{
> -	if (chip->vendor.miscdev.name)
> -		misc_deregister(&chip->vendor.miscdev);
> -}
> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> index e6b08bd..9e4ce4d 100644
> --- a/drivers/char/tpm/tpm-interface.c
> +++ b/drivers/char/tpm/tpm-interface.c
> @@ -997,6 +997,35 @@ int tpm_get_random(u32 chip_num, u8 *out, size_t max)
>   }
>   EXPORT_SYMBOL_GPL(tpm_get_random);
>
> +static int __init tpm_init(void)
> +{
> +	int rc;
> +
> +	tpm_class = class_create(THIS_MODULE, "tpm");
> +	if (IS_ERR(tpm_class)) {
> +		pr_err("couldn't create tpm class\n");
> +		return PTR_ERR(tpm_class);
> +	}
> +
> +	rc = alloc_chrdev_region(&tpm_devt, 0, TPM_NUM_DEVICES, "tpm");
> +	if (rc < 0) {
> +		pr_err("tpm: failed to allocate char dev region\n");
> +		class_destroy(tpm_class);
> +		return rc;
> +	}
> +
> +	return 0;
> +}
> +
> +static void __exit tpm_exit(void)
> +{
> +	class_destroy(tpm_class);
> +	unregister_chrdev_region(tpm_devt, TPM_NUM_DEVICES);
> +}
> +
> +subsys_initcall(tpm_init);
> +module_exit(tpm_exit);
> +
>   MODULE_AUTHOR("Leendert van Doorn (leendert@watson.ibm.com)");
>   MODULE_DESCRIPTION("TPM Driver");
>   MODULE_VERSION("2.0");
> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> index b3a7c76..83103e0 100644
> --- a/drivers/char/tpm/tpm.h
> +++ b/drivers/char/tpm/tpm.h
> @@ -23,11 +23,11 @@
>   #include <linux/fs.h>
>   #include <linux/mutex.h>
>   #include <linux/sched.h>
> -#include <linux/miscdevice.h>
>   #include <linux/platform_device.h>
>   #include <linux/io.h>
>   #include <linux/tpm.h>
>   #include <linux/acpi.h>
> +#include <linux/cdev.h>
>
>   enum tpm_const {
>   	TPM_MINOR = 224,	/* officially assigned */
> @@ -74,7 +74,6 @@ struct tpm_vendor_specific {
>   	int region_size;
>   	int have_region;
>
> -	struct miscdevice miscdev;
>   	struct list_head list;
>   	int locality;
>   	unsigned long timeout_a, timeout_b, timeout_c, timeout_d; /* jiffies */
> @@ -99,6 +98,9 @@ struct tpm_vendor_specific {
>
>   struct tpm_chip {
>   	struct device *pdev;	/* Device stuff */
> +	struct device dev;
> +	struct cdev cdev;
> +
>   	const struct tpm_class_ops *ops;
>
>   	int dev_num;		/* /dev/tpm# */
> @@ -320,6 +322,10 @@ struct tpm_cmd_t {
>   	tpm_cmd_params	params;
>   } __packed;
>
> +extern struct class *tpm_class;
> +extern dev_t tpm_devt;
> +extern const struct file_operations tpm_fops;
> +
>   ssize_t	tpm_getcap(struct device *, __be32, cap_t *, const char *);
>   ssize_t tpm_transmit(struct tpm_chip *chip, const char *buf,
>   		     size_t bufsiz);
> @@ -340,8 +346,6 @@ extern struct tpm_chip *tpmm_chip_alloc(struct device *dev,
>   extern int tpm_chip_register(struct tpm_chip *chip);
>   extern void tpm_chip_unregister(struct tpm_chip *chip);
>
> -int tpm_dev_add_device(struct tpm_chip *chip);
> -void tpm_dev_del_device(struct tpm_chip *chip);
>   int tpm_sysfs_add_device(struct tpm_chip *chip);
>   void tpm_sysfs_del_device(struct tpm_chip *chip);
>
> diff --git a/drivers/char/tpm/tpm_i2c_nuvoton.c b/drivers/char/tpm/tpm_i2c_nuvoton.c
> index 92ee9fa..14246e2 100644
> --- a/drivers/char/tpm/tpm_i2c_nuvoton.c
> +++ b/drivers/char/tpm/tpm_i2c_nuvoton.c
> @@ -557,7 +557,7 @@ static int i2c_nuvoton_probe(struct i2c_client *client,
>   		rc = devm_request_irq(dev, chip->vendor.irq,
>   				      i2c_nuvoton_int_handler,
>   				      IRQF_TRIGGER_LOW,
> -				      chip->vendor.miscdev.name,
> +				      chip->devname,
>   				      chip);
>   		if (rc) {
>   			dev_err(dev, "%s() Unable to request irq: %d for use\n",
> diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
> index 660d9af..7a2c59b 100644
> --- a/drivers/char/tpm/tpm_tis.c
> +++ b/drivers/char/tpm/tpm_tis.c
> @@ -661,7 +661,7 @@ static int tpm_tis_init(struct device *dev, acpi_handle acpi_dev_handle,
>   				 TPM_INT_VECTOR(chip->vendor.locality));
>   			if (devm_request_irq
>   			    (dev, i, tis_int_probe, IRQF_SHARED,
> -			     chip->vendor.miscdev.name, chip) != 0) {
> +			     chip->devname, chip) != 0) {
>   				dev_info(chip->pdev,
>   					 "Unable to request irq: %d for probe\n",
>   					 i);
> @@ -708,7 +708,7 @@ static int tpm_tis_init(struct device *dev, acpi_handle acpi_dev_handle,
>   			 TPM_INT_VECTOR(chip->vendor.locality));
>   		if (devm_request_irq
>   		    (dev, chip->vendor.irq, tis_int_handler, IRQF_SHARED,
> -		     chip->vendor.miscdev.name, chip) != 0) {
> +		     chip->devname, chip) != 0) {
>   			dev_info(chip->pdev,
>   				 "Unable to request irq: %d for use\n",
>   				 chip->vendor.irq);

This looks ok to me.

Reviewed-by: Stefan Berger <stefanb@linux.vnet.ibm.com>