Re: [tpmdd-devel] [PATCH v2 3/7] tpm: clean up tpm_tis driver life-cycle

[Jason Gunthorpe]

On Tue, Oct 07, 2014 at 08:01:13PM +0300, Jarkko Sakkinen wrote:

> +	chip = tpm_chip_alloc(dev, &tpm_tis);
> +	if (!chip)
>  		return -ENODEV;

Needs to use ERR_PTR
  
> +	rc = tpm_chip_register(chip);
> +	if (rc)
> +		return -ENODEV;

Wrong ordering, this needs to be last in the probe function

Return rc not -ENODEV
  
> +static void tpm_tis_chip_remove(struct tpm_chip *chip)
> +{
> +	iowrite32(~TPM_GLOBAL_INT_ENABLE &
> +		  ioread32(chip->vendor.iobase +
> +			   TPM_INT_ENABLE(chip->vendor.
> +					  locality)),
> +		  chip->vendor.iobase +
> +		  TPM_INT_ENABLE(chip->vendor.locality));
> +	release_locality(chip, chip->vendor.locality, 1);
> +	if (chip->vendor.irq)
> +		free_irq(chip->vendor.irq, chip);
> +
> +	tpm_chip_unregister(chip);
> +}

Wrong ordering, tpm_chip_unregister needs to be first

> +	chip = dev_get_drvdata(&pdev->dev);
> +	tpm_tis_chip_remove(chip);
>  	platform_device_unregister(pdev);

I'm under the impression devm does not work outside a device driver
context, so adding devm breaks force mode in this driver. Do you see
differently?

AFAIK the two options are to fix force mode so that it attaches the
dummy platform driver (that is what it is for after all) or remove
force mode.

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/