Re: [PATCH v1 2/3] tpm: two-phase chip management functions

[Jason Gunthorpe]

On Wed, Oct 22, 2014 at 07:23:55PM +0300, Jarkko Sakkinen wrote:
> tpm_register_hardware() and tpm_remove_hardware() are called often
> before initializing the device. This is wrong order since it could
> be that main TPM driver needs a fully initialized chip to be able to
> do its job. For example, now it is impossible to move common startup
> functions such as tpm_do_selftest() to tpm_register_hardware().
 
> Added tpm_chip_alloc() and tpm_chip_register() where tpm_chip_alloc()

It is called tpmm_chip_alloc() in this version..

> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@xxxxxxxxxxxxxxx>

Reviewed-By: Jason Gunthorpe <jgunthorpe@xxxxxxxxxxxxxxxxxxxx>

Looks fine, if you have to spin this again you can incorporate the
nits.

> +
> +static DECLARE_BITMAP(dev_mask, TPM_NUM_DEVICES);

Not for this patch, but while this area of code is being looked at
this should probably be an IDR/IDA like other subsytems?

> +		if (try_module_get(pos->dev->driver->owner)) {
> +			chip = pos;
> +			break;
> +		}

Not for this patch, this module stuff should be wiped in favor of chip->ops
locking.

> +static void tpmm_chip_remove(void *data)
> +{
> +	struct tpm_chip *chip = (struct tpm_chip *) data;
> +	dev_dbg(chip->dev, "%s\n", __func__);

This print is silent in the default compile, right?

> +	chip->dev_num = find_first_zero_bit(dev_mask, TPM_NUM_DEVICES);
[..]
> +	set_bit(chip->dev_num, dev_mask);

Not for this patch but somehow there is no locking for dev_mask
here. I guess it should use the driver_lock spinlock?

> +	chip->bios_dir = tpm_bios_log_setup(chip->devname);

Not for this patch, but tpm_bios_log_setup can return NULL if
securityfs setup fails.

> diff --git a/drivers/char/tpm/tpm_atmel.c b/drivers/char/tpm/tpm_atmel.c
> index 6069d13..8e2576a 100644
> +++ b/drivers/char/tpm/tpm_atmel.c
> @@ -138,11 +138,11 @@ static void atml_plat_remove(void)
>  	struct tpm_chip *chip = dev_get_drvdata(&pdev->dev);
>  
>  	if (chip) {
> +		tpm_chip_unregister(chip);
>  		if (chip->vendor.have_region)
>  			atmel_release_region(chip->vendor.base,
>  					     chip->vendor.region_size);
>  		atmel_put_base_addr(chip->vendor.iobase);
> -		tpm_remove_hardware(chip->dev);
>  		platform_device_unregister(pdev);

Missed this before, the Atmel driver is the same as the TIS driver in
force mode, ie it isn't going through the driver APIs, but instead
force creating platform devices. Same comment as for TIS - I'm not
sure devm works properly like this.

I guess just add the same comment as for TIS?

> diff --git a/drivers/char/tpm/tpm_i2c_infineon.c b/drivers/char/tpm/tpm_i2c_infineon.c
> index 472af4b..6f00bc3 100644
> +++ b/drivers/char/tpm/tpm_i2c_infineon.c
> @@ -581,10 +581,9 @@ static int tpm_tis_i2c_init(struct device *dev)
>  	int rc = 0;
>  	struct tpm_chip *chip;
>  
> -	chip = tpm_register_hardware(dev, &tpm_tis_i2c);
> -	if (!chip) {
> -		dev_err(dev, "could not register hardware\n");
> -		rc = -ENODEV;
> +	chip = tpmm_chip_alloc(dev, &tpm_tis_i2c);
> +	if (IS_ERR(chip)) {
> +		rc = PTR_ERR(chip);
>  		goto out_err;

Nit: out_err is synonymous with 'return rc;', so all the goto out_err
in this driver can just return;

> +	rc = tpm_chip_register(chip);
> +	if (rc)
> +		return rc;
> +	return 0;

Nit: could just be return tpm_chip_register(chip);

> @@ -619,10 +619,9 @@ tpm_st33_i2c_probe(struct i2c_client *client, const struct i2c_device_id *id)
>  		goto end;
>  	}
>  
> -	chip = tpm_register_hardware(&client->dev, &st_i2c_tpm);
> -	if (!chip) {
> -		dev_info(&client->dev, "fail chip\n");
> -		err = -ENODEV;
> +	chip = tpmm_chip_alloc(&client->dev, &st_i2c_tpm);
> +	if (IS_ERR(chip)) {
> +		err = PTR_ERR(chip);
>  		goto end;
>  	}

Nit: Same comment, 'goto end' can just be 'return rc'

> -	dev_info(chip->dev, "TPM I2C Initialized\n");
> +	err = tpm_chip_register(chip);
> +	if (err)
> +		goto _irq_set;
> +

Nit: Same comment

>  end:
>  	pr_info("TPM I2C initialisation fail\n");

This pr_info should be deleted

> @@ -573,6 +580,8 @@ static void tpm_inf_pnp_remove(struct pnp_dev *dev)
>  	struct tpm_chip *chip = pnp_get_drvdata(dev);
>  
>  	if (chip) {

Nit: This if in the remove callback is an anti-pattern and should be
globally removed. If remove is being called then probe succeeded,
there is no way for probe to succed and drvdata to be unset.

>  static void tpm_nsc_remove(struct device *dev)
>  {
>  	struct tpm_chip *chip = dev_get_drvdata(dev);
> -	if ( chip ) {
> +	if (chip) {

Same comment

> @@ -836,7 +831,7 @@ MODULE_PARM_DESC(hid, "Set additional specific HID for this driver to probe");
>  
>  static struct platform_driver tis_drv = {
>  	.driver = {
> -		.name = "tpm_tis",
> +		.name		= "tpm_tis",
>  		.owner		= THIS_MODULE,
>  		.pm		= &tpm_tis_pm,
>  	},

There is no remove method here in this platform_driver, this ties into
the question if force works or not. The tpm_tis_remove call in the
cleanup_hardware should be done through the .remove method of this
driver structure..

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/