tpm_tis: Clean up force module parameter

tpm_tis: Clean up force module parameter

[Jason Gunthorpe]

Drive the force=1 flow through the driver core. There are two main reasons to do this:
 1) To enable tpm_tis for OF environments requires a platform_device anyhow, so
    the force_device needs to be re-used for them.
 2) Recent changes in the core code break the assumption that a driver will be
    'attached' to things created through platform_device_register_simple,
    which causes the tpm core to blow up.


Jason Gunthorpe (2):
  tpm_tis: Disable interrupt auto probing on a per-device basis
  tpm_tis: Clean up the force=1 module parameter

 drivers/char/tpm/tpm_tis.c | 177 +++++++++++++++++++++++++++------------------
 1 file changed, 108 insertions(+), 69 deletions(-)

-- 
2.1.4

[PATCH 1/2] tpm_tis: Disable interrupt auto probing on a per-device basis

[Jason Gunthorpe]

Instead of clearing the global interrupts flag when any device
does not have an interrupt just pass -1 through tpm_info.irq.

The only thing that asks for autoprobing is the force=1 path.

Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
---
 drivers/char/tpm/tpm_tis.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
index 8a3509cb10da..0a2d94f3d679 100644
--- a/drivers/char/tpm/tpm_tis.c
+++ b/drivers/char/tpm/tpm_tis.c
@@ -69,7 +69,7 @@ enum tis_defaults {
 struct tpm_info {
 	unsigned long start;
 	unsigned long len;
-	unsigned int irq;
+	int irq;
 };
 
 static struct tpm_info tis_default_info = {
@@ -807,7 +807,7 @@ static int tpm_tis_init(struct device *dev, struct tpm_info *tpm_info,
 	/* INTERRUPT Setup */
 	init_waitqueue_head(&chip->vendor.read_queue);
 	init_waitqueue_head(&chip->vendor.int_queue);
-	if (interrupts) {
+	if (interrupts && tpm_info->irq != -1) {
 		if (tpm_info->irq) {
 			tpm_tis_probe_irq_single(chip, intmask, IRQF_SHARED,
 						 tpm_info->irq);
@@ -895,9 +895,9 @@ static SIMPLE_DEV_PM_OPS(tpm_tis_pm, tpm_pm_suspend, tpm_tis_resume);
 
 #ifdef CONFIG_PNP
 static int tpm_tis_pnp_init(struct pnp_dev *pnp_dev,
-				      const struct pnp_device_id *pnp_id)
+			    const struct pnp_device_id *pnp_id)
 {
-	struct tpm_info tpm_info = tis_default_info;
+	struct tpm_info tpm_info = {};
 	acpi_handle acpi_dev_handle = NULL;
 
 	tpm_info.start = pnp_mem_start(pnp_dev, 0);
@@ -906,7 +906,7 @@ static int tpm_tis_pnp_init(struct pnp_dev *pnp_dev,
 	if (pnp_irq_valid(pnp_dev, 0))
 		tpm_info.irq = pnp_irq(pnp_dev, 0);
 	else
-		interrupts = false;
+		tpm_info.irq = -1;
 
 #ifdef CONFIG_ACPI
 	if (pnp_acpi_device(pnp_dev)) {
@@ -977,13 +977,14 @@ static int tpm_check_resource(struct acpi_resource *ares, void *data)
 static int tpm_tis_acpi_init(struct acpi_device *acpi_dev)
 {
 	struct list_head resources;
-	struct tpm_info tpm_info = tis_default_info;
+	struct tpm_info tpm_info = {};
 	int ret;
 
 	if (!is_fifo(acpi_dev))
 		return -ENODEV;
 
 	INIT_LIST_HEAD(&resources);
+	tpm_info.irq = -1;
 	ret = acpi_dev_get_resources(acpi_dev, &resources, tpm_check_resource,
 				     &tpm_info);
 	if (ret < 0)
@@ -991,9 +992,6 @@ static int tpm_tis_acpi_init(struct acpi_device *acpi_dev)
 
 	acpi_dev_free_resource_list(&resources);
 
-	if (!tpm_info.irq)
-		interrupts = false;
-
 	if (is_itpm(acpi_dev))
 		itpm = true;
 
-- 
2.1.4

[PATCH 2/2] tpm_tis: Clean up the force=1 module parameter

[Jason Gunthorpe]

The TPM core has long assumed that every device has a driver attached,
however b8b2c7d845d5 ("base/platform: assert that dev_pm_domain callbacks are
called unconditionally") breaks that assumption.

Rework the TPM setup to create a platform device with resources and
then allow the driver core to naturally bind and probe it through the
normal mechanisms. All this structure is needed anyhow to enable TPM
for OF environments.

Finally, since the entire flow is changing convert the init/exit to use
the modern ifdef-less coding style when possible

Reported-by: "Wilck, Martin" <martin.wilck@ts.fujitsu.com>
Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
---
 drivers/char/tpm/tpm_tis.c | 161 ++++++++++++++++++++++++++++-----------------
 1 file changed, 101 insertions(+), 60 deletions(-)

diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
index 0a2d94f3d679..0e5c282aa37e 100644
--- a/drivers/char/tpm/tpm_tis.c
+++ b/drivers/char/tpm/tpm_tis.c
@@ -60,8 +60,6 @@ enum tis_int_flags {
 };
 
 enum tis_defaults {
-	TIS_MEM_BASE = 0xFED40000,
-	TIS_MEM_LEN = 0x5000,
 	TIS_SHORT_TIMEOUT = 750,	/* ms */
 	TIS_LONG_TIMEOUT = 2000,	/* 2 sec */
 };
@@ -72,12 +70,6 @@ struct tpm_info {
 	int irq;
 };
 
-static struct tpm_info tis_default_info = {
-	.start = TIS_MEM_BASE,
-	.len = TIS_MEM_LEN,
-	.irq = 0,
-};
-
 /* Some timeout values are needed before it is known whether the chip is
  * TPM 1.0 or TPM 2.0.
  */
@@ -847,7 +839,6 @@ out_err:
 	return rc;
 }
 
-#ifdef CONFIG_PM_SLEEP
 static void tpm_tis_reenable_interrupts(struct tpm_chip *chip)
 {
 	u32 intmask;
@@ -889,11 +880,9 @@ static int tpm_tis_resume(struct device *dev)
 
 	return 0;
 }
-#endif
 
 static SIMPLE_DEV_PM_OPS(tpm_tis_pm, tpm_pm_suspend, tpm_tis_resume);
 
-#ifdef CONFIG_PNP
 static int tpm_tis_pnp_init(struct pnp_dev *pnp_dev,
 			    const struct pnp_device_id *pnp_id)
 {
@@ -908,14 +897,12 @@ static int tpm_tis_pnp_init(struct pnp_dev *pnp_dev,
 	else
 		tpm_info.irq = -1;
 
-#ifdef CONFIG_ACPI
 	if (pnp_acpi_device(pnp_dev)) {
 		if (is_itpm(pnp_acpi_device(pnp_dev)))
 			itpm = true;
 
-		acpi_dev_handle = pnp_acpi_device(pnp_dev)->handle;
+		acpi_dev_handle = ACPI_HANDLE(&pnp_dev->dev);
 	}
-#endif
 
 	return tpm_tis_init(&pnp_dev->dev, &tpm_info, acpi_dev_handle);
 }
@@ -956,7 +943,6 @@ static struct pnp_driver tis_pnp_driver = {
 module_param_string(hid, tpm_pnp_tbl[TIS_HID_USR_IDX].id,
 		    sizeof(tpm_pnp_tbl[TIS_HID_USR_IDX].id), 0444);
 MODULE_PARM_DESC(hid, "Set additional specific HID for this driver to probe");
-#endif
 
 #ifdef CONFIG_ACPI
 static int tpm_check_resource(struct acpi_resource *ares, void *data)
@@ -1029,80 +1015,135 @@ static struct acpi_driver tis_acpi_driver = {
 };
 #endif
 
+static struct platform_device *force_pdev;
+
+static int tpm_tis_plat_probe(struct platform_device *pdev)
+{
+	struct tpm_info tpm_info = {};
+        struct resource *res;
+
+        res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+        if (res == NULL) {
+		dev_err(&pdev->dev, "no memory resource defined\n");
+		return -ENODEV;
+        }
+	tpm_info.start = res->start;
+	tpm_info.len = resource_size(res);
+
+        res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
+	if (res)
+		tpm_info.irq = res->start;
+	else {
+		if (pdev == force_pdev)
+			tpm_info.irq = -1;
+		else
+			/* When forcing auto probe the IRQ */
+			tpm_info.irq = 0;
+	}
+
+	return tpm_tis_init(&pdev->dev, &tpm_info, NULL);
+}
+
+static int tpm_tis_plat_remove(struct platform_device *pdev)
+{
+	struct tpm_chip *chip = dev_get_drvdata(&pdev->dev);
+
+	tpm_chip_unregister(chip);
+	tpm_tis_remove(chip);
+
+	return 0;
+}
+
 static struct platform_driver tis_drv = {
+	.probe = tpm_tis_plat_probe,
+	.remove = tpm_tis_plat_remove,
 	.driver = {
 		.name		= "tpm_tis",
 		.pm		= &tpm_tis_pm,
 	},
 };
 
-static struct platform_device *pdev;
-
 static bool force;
+#ifdef CONFIG_X86
 module_param(force, bool, 0444);
 MODULE_PARM_DESC(force, "Force device probe rather than using ACPI entry");
+#endif
+
+static int force_device(void)
+{
+	struct platform_device *pdev;
+	static const struct resource x86_resources[] ={
+		{
+			.start = 0xFED40000,
+			.end = 0xFED44FFF,
+			.flags = IORESOURCE_MEM,
+		},
+	};
+
+	if (!force)
+		return 0;
+
+	/* The driver core will match the name tpm_tis of the device to
+	 * the tpm_tis platform driver and complete the setup via
+	 * tpm_tis_plat_probe
+	 */
+	pdev = platform_device_register_simple("tpm_tis", -1, x86_resources,
+					       ARRAY_SIZE(x86_resources));
+	if (IS_ERR(pdev))
+		return PTR_ERR(pdev);
+	force_pdev = pdev;
+
+	return 0;
+}
+
 static int __init init_tis(void)
 {
 	int rc;
-#ifdef CONFIG_PNP
-	if (!force) {
+
+	rc = force_device();
+	if (rc)
+		goto out1;
+
+	if (IS_ENABLED(CONFIG_PNP)) {
 		rc = pnp_register_driver(&tis_pnp_driver);
 		if (rc)
-			return rc;
+			goto out2;
 	}
-#endif
+
 #ifdef CONFIG_ACPI
-	if (!force) {
-		rc = acpi_bus_register_driver(&tis_acpi_driver);
-		if (rc) {
-#ifdef CONFIG_PNP
-			pnp_unregister_driver(&tis_pnp_driver);
-#endif
-			return rc;
-		}
-	}
+	rc = acpi_bus_register_driver(&tis_acpi_driver);
+	if (rc)
+		goto out3;
 #endif
-	if (!force)
-		return 0;
 
 	rc = platform_driver_register(&tis_drv);
-	if (rc < 0)
-		return rc;
-	pdev = platform_device_register_simple("tpm_tis", -1, NULL, 0);
-	if (IS_ERR(pdev)) {
-		rc = PTR_ERR(pdev);
-		goto err_dev;
-	}
-	rc = tpm_tis_init(&pdev->dev, &tis_default_info, NULL);
 	if (rc)
-		goto err_init;
+		goto out4;
+
 	return 0;
-err_init:
-	platform_device_unregister(pdev);
-err_dev:
-	platform_driver_unregister(&tis_drv);
+out4:
+#ifdef CONFIG_ACPI
+	acpi_bus_unregister_driver(&tis_acpi_driver);
+out3:
+#endif
+	pnp_unregister_driver(&tis_pnp_driver);
+out2:
+	platform_device_unregister(force_pdev);
+out1:
 	return rc;
 }
 
 static void __exit cleanup_tis(void)
 {
-	struct tpm_chip *chip;
-#if defined(CONFIG_PNP) || defined(CONFIG_ACPI)
-	if (!force) {
+	platform_driver_unregister(&tis_drv);
 #ifdef CONFIG_ACPI
-		acpi_bus_unregister_driver(&tis_acpi_driver);
-#endif
-#ifdef CONFIG_PNP
-		pnp_unregister_driver(&tis_pnp_driver);
+	acpi_bus_unregister_driver(&tis_acpi_driver);
 #endif
-		return;
-	}
-#endif
-	chip = dev_get_drvdata(&pdev->dev);
-	tpm_chip_unregister(chip);
-	tpm_tis_remove(chip);
-	platform_device_unregister(pdev);
-	platform_driver_unregister(&tis_drv);
+	pnp_unregister_driver(&tis_pnp_driver);
+
+	if (force_pdev)
+		platform_device_unregister(force_pdev);
+
 }
 
 module_init(init_tis);
-- 
2.1.4

Re: [PATCH 1/2] tpm_tis: Disable interrupt auto probing on a per-device basis

[Uwe Kleine-König]

Hello Jason,

On Mon, Nov 30, 2015 at 12:27:11PM -0700, Jason Gunthorpe wrote:
> Instead of clearing the global interrupts flag when any device
> does not have an interrupt just pass -1 through tpm_info.irq.

Is there a reason not to use 0 for the invalid irq?

> The only thing that asks for autoprobing is the force=1 path.
> 
> Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> ---
>  drivers/char/tpm/tpm_tis.c | 16 +++++++---------
>  1 file changed, 7 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
> index 8a3509cb10da..0a2d94f3d679 100644
> --- a/drivers/char/tpm/tpm_tis.c
> +++ b/drivers/char/tpm/tpm_tis.c
> @@ -69,7 +69,7 @@ enum tis_defaults {
>  struct tpm_info {
>  	unsigned long start;
>  	unsigned long len;
> -	unsigned int irq;
> +	int irq;
>  };
>  
>  static struct tpm_info tis_default_info = {
> @@ -807,7 +807,7 @@ static int tpm_tis_init(struct device *dev, struct tpm_info *tpm_info,
>  	/* INTERRUPT Setup */
>  	init_waitqueue_head(&chip->vendor.read_queue);
>  	init_waitqueue_head(&chip->vendor.int_queue);
> -	if (interrupts) {
> +	if (interrupts && tpm_info->irq != -1) {
>  		if (tpm_info->irq) {

There is even a check for irq == 0 that could be reused maybe?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH 2/2] tpm_tis: Clean up the force=1 module parameter

[Uwe Kleine-König]

On Mon, Nov 30, 2015 at 12:27:12PM -0700, Jason Gunthorpe wrote:
> The TPM core has long assumed that every device has a driver attached,
> however b8b2c7d845d5 ("base/platform: assert that dev_pm_domain callbacks are
> called unconditionally") breaks that assumption.

Maybe it's worth to point out that b8b2c7d845d5 didn't break it on
purpose and is fixed accordingly. Still the assumption isn't valid, but
works in practise.

> Rework the TPM setup to create a platform device with resources and
> then allow the driver core to naturally bind and probe it through the
> normal mechanisms. All this structure is needed anyhow to enable TPM
> for OF environments.
> 
> Finally, since the entire flow is changing convert the init/exit to use
> the modern ifdef-less coding style when possible
> 
> Reported-by: "Wilck, Martin" <martin.wilck@ts.fujitsu.com>
> Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> ---
>  drivers/char/tpm/tpm_tis.c | 161 ++++++++++++++++++++++++++++-----------------
>  1 file changed, 101 insertions(+), 60 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
> index 0a2d94f3d679..0e5c282aa37e 100644
> --- a/drivers/char/tpm/tpm_tis.c
> +++ b/drivers/char/tpm/tpm_tis.c
> @@ -60,8 +60,6 @@ enum tis_int_flags {
>  };
>  
>  enum tis_defaults {
> -	TIS_MEM_BASE = 0xFED40000,
> -	TIS_MEM_LEN = 0x5000,
>  	TIS_SHORT_TIMEOUT = 750,	/* ms */
>  	TIS_LONG_TIMEOUT = 2000,	/* 2 sec */
>  };
> @@ -72,12 +70,6 @@ struct tpm_info {
>  	int irq;
>  };
>  
> -static struct tpm_info tis_default_info = {
> -	.start = TIS_MEM_BASE,
> -	.len = TIS_MEM_LEN,
> -	.irq = 0,
> -};
> -
>  /* Some timeout values are needed before it is known whether the chip is
>   * TPM 1.0 or TPM 2.0.
>   */
> @@ -847,7 +839,6 @@ out_err:
>  	return rc;
>  }
>  
> -#ifdef CONFIG_PM_SLEEP
>  static void tpm_tis_reenable_interrupts(struct tpm_chip *chip)
>  {
>  	u32 intmask;
> @@ -889,11 +880,9 @@ static int tpm_tis_resume(struct device *dev)
>  
>  	return 0;
>  }
> -#endif
>  
>  static SIMPLE_DEV_PM_OPS(tpm_tis_pm, tpm_pm_suspend, tpm_tis_resume);
>  
> -#ifdef CONFIG_PNP
>  static int tpm_tis_pnp_init(struct pnp_dev *pnp_dev,
>  			    const struct pnp_device_id *pnp_id)
>  {
> @@ -908,14 +897,12 @@ static int tpm_tis_pnp_init(struct pnp_dev *pnp_dev,
>  	else
>  		tpm_info.irq = -1;
>  
> -#ifdef CONFIG_ACPI
>  	if (pnp_acpi_device(pnp_dev)) {
>  		if (is_itpm(pnp_acpi_device(pnp_dev)))
>  			itpm = true;
>  
> -		acpi_dev_handle = pnp_acpi_device(pnp_dev)->handle;
> +		acpi_dev_handle = ACPI_HANDLE(&pnp_dev->dev);
>  	}
> -#endif
>  
>  	return tpm_tis_init(&pnp_dev->dev, &tpm_info, acpi_dev_handle);
>  }
> @@ -956,7 +943,6 @@ static struct pnp_driver tis_pnp_driver = {
>  module_param_string(hid, tpm_pnp_tbl[TIS_HID_USR_IDX].id,
>  		    sizeof(tpm_pnp_tbl[TIS_HID_USR_IDX].id), 0444);
>  MODULE_PARM_DESC(hid, "Set additional specific HID for this driver to probe");
> -#endif
>  
>  #ifdef CONFIG_ACPI
>  static int tpm_check_resource(struct acpi_resource *ares, void *data)
> @@ -1029,80 +1015,135 @@ static struct acpi_driver tis_acpi_driver = {
>  };
>  #endif
>  
> +static struct platform_device *force_pdev;
> +
> +static int tpm_tis_plat_probe(struct platform_device *pdev)
> +{
> +	struct tpm_info tpm_info = {};
> +        struct resource *res;
> +
> +        res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +        if (res == NULL) {

indention problems here.

> +		dev_err(&pdev->dev, "no memory resource defined\n");
> +		return -ENODEV;
> +        }
> +	tpm_info.start = res->start;
> +	tpm_info.len = resource_size(res);
> +
> +        res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> +	if (res)
> +		tpm_info.irq = res->start;
> +	else {

If one branch of an if/else has braces, all of them should.

> +		if (pdev == force_pdev)
> +			tpm_info.irq = -1;
> +		else
> +			/* When forcing auto probe the IRQ */
> +			tpm_info.irq = 0;
> +	}

ah, so 0 means autoprobe and -1 means invalid. Hmm.

> +
> +	return tpm_tis_init(&pdev->dev, &tpm_info, NULL);
> +}
> +
> +static int tpm_tis_plat_remove(struct platform_device *pdev)
> +{
> +	struct tpm_chip *chip = dev_get_drvdata(&pdev->dev);
> +
> +	tpm_chip_unregister(chip);
> +	tpm_tis_remove(chip);
> +
> +	return 0;
> +}
> +
>  static struct platform_driver tis_drv = {
> +	.probe = tpm_tis_plat_probe,
> +	.remove = tpm_tis_plat_remove,
>  	.driver = {
>  		.name		= "tpm_tis",
>  		.pm		= &tpm_tis_pm,
>  	},
>  };
>  
> -static struct platform_device *pdev;
> -
>  static bool force;
> +#ifdef CONFIG_X86
>  module_param(force, bool, 0444);
>  MODULE_PARM_DESC(force, "Force device probe rather than using ACPI entry");
> +#endif

Is this added ifdef intended to be in this commit?

> +static int force_device(void)
> +{
> +	struct platform_device *pdev;
> +	static const struct resource x86_resources[] ={
> +		{
> +			.start = 0xFED40000,
> +			.end = 0xFED44FFF,
> +			.flags = IORESOURCE_MEM,
> +		},
> +	};
> +
> +	if (!force)
> +		return 0;
> +
> +	/* The driver core will match the name tpm_tis of the device to
> +	 * the tpm_tis platform driver and complete the setup via
> +	 * tpm_tis_plat_probe
> +	 */
> +	pdev = platform_device_register_simple("tpm_tis", -1, x86_resources,
> +					       ARRAY_SIZE(x86_resources));
> +	if (IS_ERR(pdev))
> +		return PTR_ERR(pdev);
> +	force_pdev = pdev;
> +
> +	return 0;
> +}
> +
>  static int __init init_tis(void)
>  {
>  	int rc;
> -#ifdef CONFIG_PNP
> -	if (!force) {
> +
> +	rc = force_device();
> +	if (rc)
> +		goto out1;
> +
> +	if (IS_ENABLED(CONFIG_PNP)) {
>  		rc = pnp_register_driver(&tis_pnp_driver);
>  		if (rc)
> -			return rc;
> +			goto out2;
>  	}
> -#endif
> +
>  #ifdef CONFIG_ACPI
> -	if (!force) {
> -		rc = acpi_bus_register_driver(&tis_acpi_driver);
> -		if (rc) {
> -#ifdef CONFIG_PNP
> -			pnp_unregister_driver(&tis_pnp_driver);
> -#endif
> -			return rc;
> -		}
> -	}
> +	rc = acpi_bus_register_driver(&tis_acpi_driver);
> +	if (rc)
> +		goto out3;
>  #endif
> -	if (!force)
> -		return 0;
>  
>  	rc = platform_driver_register(&tis_drv);
> -	if (rc < 0)
> -		return rc;
> -	pdev = platform_device_register_simple("tpm_tis", -1, NULL, 0);
> -	if (IS_ERR(pdev)) {
> -		rc = PTR_ERR(pdev);
> -		goto err_dev;
> -	}
> -	rc = tpm_tis_init(&pdev->dev, &tis_default_info, NULL);
>  	if (rc)
> -		goto err_init;
> +		goto out4;
> +
>  	return 0;
> -err_init:
> -	platform_device_unregister(pdev);
> -err_dev:
> -	platform_driver_unregister(&tis_drv);
> +out4:
> +#ifdef CONFIG_ACPI
> +	acpi_bus_unregister_driver(&tis_acpi_driver);
> +out3:
> +#endif
> +	pnp_unregister_driver(&tis_pnp_driver);
> +out2:
> +	platform_device_unregister(force_pdev);
> +out1:

Might be a matter of taste, but having nicer names for the error labels
makes review easier. For example I would have called "out3"
"err_register_acpi" instead. Then you can easily verify that it's placed
right in the error path being directly after acpi_bus_unregister_driver.

Also all kind of strange things happen if you later need to add a label
between out2 and out3. drivers/scsi/hpsa.c for example used "clean2_5"
in a similar situation. The alternative is to renumber the label makeing
patch review still harder.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH 2/2] tpm_tis: Clean up the force=1 module parameter

[Jarkko Sakkinen]

Hi

On Tue, Dec 01, 2015 at 08:28:35AM +0100, Uwe Kleine-König wrote:
> On Mon, Nov 30, 2015 at 12:27:12PM -0700, Jason Gunthorpe wrote:
> > The TPM core has long assumed that every device has a driver attached,
> > however b8b2c7d845d5 ("base/platform: assert that dev_pm_domain callbacks are
> > called unconditionally") breaks that assumption.
> 
> Maybe it's worth to point out that b8b2c7d845d5 didn't break it on
> purpose and is fixed accordingly. Still the assumption isn't valid, but
> works in practise.
> 
> > Rework the TPM setup to create a platform device with resources and
> > then allow the driver core to naturally bind and probe it through the
> > normal mechanisms. All this structure is needed anyhow to enable TPM
> > for OF environments.
> > 
> > Finally, since the entire flow is changing convert the init/exit to use
> > the modern ifdef-less coding style when possible
> > 
> > Reported-by: "Wilck, Martin" <martin.wilck@ts.fujitsu.com>
> > Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> > ---
> >  drivers/char/tpm/tpm_tis.c | 161 ++++++++++++++++++++++++++++-----------------
> >  1 file changed, 101 insertions(+), 60 deletions(-)
> > 
> > diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
> > index 0a2d94f3d679..0e5c282aa37e 100644
> > --- a/drivers/char/tpm/tpm_tis.c
> > +++ b/drivers/char/tpm/tpm_tis.c
> > @@ -60,8 +60,6 @@ enum tis_int_flags {
> >  };
> >  
> >  enum tis_defaults {
> > -	TIS_MEM_BASE = 0xFED40000,
> > -	TIS_MEM_LEN = 0x5000,
> >  	TIS_SHORT_TIMEOUT = 750,	/* ms */
> >  	TIS_LONG_TIMEOUT = 2000,	/* 2 sec */
> >  };
> > @@ -72,12 +70,6 @@ struct tpm_info {
> >  	int irq;
> >  };
> >  
> > -static struct tpm_info tis_default_info = {
> > -	.start = TIS_MEM_BASE,
> > -	.len = TIS_MEM_LEN,
> > -	.irq = 0,
> > -};
> > -
> >  /* Some timeout values are needed before it is known whether the chip is
> >   * TPM 1.0 or TPM 2.0.
> >   */
> > @@ -847,7 +839,6 @@ out_err:
> >  	return rc;
> >  }
> >  
> > -#ifdef CONFIG_PM_SLEEP
> >  static void tpm_tis_reenable_interrupts(struct tpm_chip *chip)
> >  {
> >  	u32 intmask;
> > @@ -889,11 +880,9 @@ static int tpm_tis_resume(struct device *dev)
> >  
> >  	return 0;
> >  }
> > -#endif
> >  
> >  static SIMPLE_DEV_PM_OPS(tpm_tis_pm, tpm_pm_suspend, tpm_tis_resume);
> >  
> > -#ifdef CONFIG_PNP
> >  static int tpm_tis_pnp_init(struct pnp_dev *pnp_dev,
> >  			    const struct pnp_device_id *pnp_id)
> >  {
> > @@ -908,14 +897,12 @@ static int tpm_tis_pnp_init(struct pnp_dev *pnp_dev,
> >  	else
> >  		tpm_info.irq = -1;
> >  
> > -#ifdef CONFIG_ACPI
> >  	if (pnp_acpi_device(pnp_dev)) {
> >  		if (is_itpm(pnp_acpi_device(pnp_dev)))
> >  			itpm = true;
> >  
> > -		acpi_dev_handle = pnp_acpi_device(pnp_dev)->handle;
> > +		acpi_dev_handle = ACPI_HANDLE(&pnp_dev->dev);
> >  	}
> > -#endif
> >  
> >  	return tpm_tis_init(&pnp_dev->dev, &tpm_info, acpi_dev_handle);
> >  }
> > @@ -956,7 +943,6 @@ static struct pnp_driver tis_pnp_driver = {
> >  module_param_string(hid, tpm_pnp_tbl[TIS_HID_USR_IDX].id,
> >  		    sizeof(tpm_pnp_tbl[TIS_HID_USR_IDX].id), 0444);
> >  MODULE_PARM_DESC(hid, "Set additional specific HID for this driver to probe");
> > -#endif
> >  
> >  #ifdef CONFIG_ACPI
> >  static int tpm_check_resource(struct acpi_resource *ares, void *data)
> > @@ -1029,80 +1015,135 @@ static struct acpi_driver tis_acpi_driver = {
> >  };
> >  #endif
> >  
> > +static struct platform_device *force_pdev;
> > +
> > +static int tpm_tis_plat_probe(struct platform_device *pdev)
> > +{
> > +	struct tpm_info tpm_info = {};
> > +        struct resource *res;
> > +
> > +        res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +        if (res == NULL) {
> 
> indention problems here.
> 
> > +		dev_err(&pdev->dev, "no memory resource defined\n");
> > +		return -ENODEV;
> > +        }
> > +	tpm_info.start = res->start;
> > +	tpm_info.len = resource_size(res);
> > +
> > +        res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> > +	if (res)
> > +		tpm_info.irq = res->start;
> > +	else {
> 
> If one branch of an if/else has braces, all of them should.
> 
> > +		if (pdev == force_pdev)
> > +			tpm_info.irq = -1;
> > +		else
> > +			/* When forcing auto probe the IRQ */
> > +			tpm_info.irq = 0;
> > +	}
> 
> ah, so 0 means autoprobe and -1 means invalid. Hmm.
> 
> > +
> > +	return tpm_tis_init(&pdev->dev, &tpm_info, NULL);
> > +}
> > +
> > +static int tpm_tis_plat_remove(struct platform_device *pdev)
> > +{
> > +	struct tpm_chip *chip = dev_get_drvdata(&pdev->dev);
> > +
> > +	tpm_chip_unregister(chip);
> > +	tpm_tis_remove(chip);
> > +
> > +	return 0;
> > +}
> > +
> >  static struct platform_driver tis_drv = {
> > +	.probe = tpm_tis_plat_probe,
> > +	.remove = tpm_tis_plat_remove,
> >  	.driver = {
> >  		.name		= "tpm_tis",
> >  		.pm		= &tpm_tis_pm,
> >  	},
> >  };
> >  
> > -static struct platform_device *pdev;
> > -
> >  static bool force;
> > +#ifdef CONFIG_X86
> >  module_param(force, bool, 0444);
> >  MODULE_PARM_DESC(force, "Force device probe rather than using ACPI entry");
> > +#endif
> 
> Is this added ifdef intended to be in this commit?
> 
> > +static int force_device(void)
> > +{
> > +	struct platform_device *pdev;
> > +	static const struct resource x86_resources[] ={
> > +		{
> > +			.start = 0xFED40000,
> > +			.end = 0xFED44FFF,
> > +			.flags = IORESOURCE_MEM,
> > +		},
> > +	};
> > +
> > +	if (!force)
> > +		return 0;
> > +
> > +	/* The driver core will match the name tpm_tis of the device to
> > +	 * the tpm_tis platform driver and complete the setup via
> > +	 * tpm_tis_plat_probe
> > +	 */
> > +	pdev = platform_device_register_simple("tpm_tis", -1, x86_resources,
> > +					       ARRAY_SIZE(x86_resources));
> > +	if (IS_ERR(pdev))
> > +		return PTR_ERR(pdev);
> > +	force_pdev = pdev;
> > +
> > +	return 0;
> > +}
> > +
> >  static int __init init_tis(void)
> >  {
> >  	int rc;
> > -#ifdef CONFIG_PNP
> > -	if (!force) {
> > +
> > +	rc = force_device();
> > +	if (rc)
> > +		goto out1;
> > +
> > +	if (IS_ENABLED(CONFIG_PNP)) {
> >  		rc = pnp_register_driver(&tis_pnp_driver);
> >  		if (rc)
> > -			return rc;
> > +			goto out2;
> >  	}
> > -#endif
> > +
> >  #ifdef CONFIG_ACPI
> > -	if (!force) {
> > -		rc = acpi_bus_register_driver(&tis_acpi_driver);
> > -		if (rc) {
> > -#ifdef CONFIG_PNP
> > -			pnp_unregister_driver(&tis_pnp_driver);
> > -#endif
> > -			return rc;
> > -		}
> > -	}
> > +	rc = acpi_bus_register_driver(&tis_acpi_driver);
> > +	if (rc)
> > +		goto out3;
> >  #endif
> > -	if (!force)
> > -		return 0;
> >  
> >  	rc = platform_driver_register(&tis_drv);
> > -	if (rc < 0)
> > -		return rc;
> > -	pdev = platform_device_register_simple("tpm_tis", -1, NULL, 0);
> > -	if (IS_ERR(pdev)) {
> > -		rc = PTR_ERR(pdev);
> > -		goto err_dev;
> > -	}
> > -	rc = tpm_tis_init(&pdev->dev, &tis_default_info, NULL);
> >  	if (rc)
> > -		goto err_init;
> > +		goto out4;
> > +
> >  	return 0;
> > -err_init:
> > -	platform_device_unregister(pdev);
> > -err_dev:
> > -	platform_driver_unregister(&tis_drv);
> > +out4:
> > +#ifdef CONFIG_ACPI
> > +	acpi_bus_unregister_driver(&tis_acpi_driver);
> > +out3:
> > +#endif
> > +	pnp_unregister_driver(&tis_pnp_driver);
> > +out2:
> > +	platform_device_unregister(force_pdev);
> > +out1:
> 
> Might be a matter of taste, but having nicer names for the error labels
> makes review easier. For example I would have called "out3"
> "err_register_acpi" instead. Then you can easily verify that it's placed
> right in the error path being directly after acpi_bus_unregister_driver.
> 
> Also all kind of strange things happen if you later need to add a label
> between out2 and out3. drivers/scsi/hpsa.c for example used "clean2_5"
> in a similar situation. The alternative is to renumber the label makeing
> patch review still harder.

Agreed that named labels would be the preferred choice. It's hard to
make sense of these labels (or at least slower than with named labels).

In addition I want this fix as a single patch, not as two-patch set.
The first patch might have made sense when the fix was being developed
but now it's just really akward change.

I had to squash the patches to make any sense of this.

> Best regards
> Uwe
> 
> -- 
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |

/Jarkko

Re: [tpmdd-devel] [PATCH 2/2] tpm_tis: Clean up the force=1 module parameter

[Wilck, Martin]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1528 bytes --]

On Mo, 2015-11-30 at 12:27 -0700, Jason Gunthorpe wrote:
> The TPM core has long assumed that every device has a driver attached,
> however b8b2c7d845d5 ("base/platform: assert that dev_pm_domain callbacks are
> called unconditionally") breaks that assumption.
> 
> Rework the TPM setup to create a platform device with resources and
> then allow the driver core to naturally bind and probe it through the
> normal mechanisms. All this structure is needed anyhow to enable TPM
> for OF environments.
> 
> Finally, since the entire flow is changing convert the init/exit to use
> the modern ifdef-less coding style when possible
> 
> Reported-by: "Wilck, Martin" <martin.wilck@ts.fujitsu.com>
> Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>

I tested this on my system, deliberately reverting my own fix for
platform_driver_probe() beforehand. It works, no panic any more.

The patch introduces one user-visible change, because now the ACPI and
PnP drivers are registered even with "force=1". This causes my TPM to be
show up twice in sysfs:

/sys/bus/acpi/drivers/tpm_tis/MSFT0101:00
-> ../../../../devices/LNXSYSTM:00/LNXSYBUS:00/MSFT0101:00
/sys/bus/platform/drivers/tpm_tis/tpm_tis
-> ../../../../devices/platform/tpm_tis

Only the platform device is actually bound to the physical device,
though. I'm not sure if I like this.

Regards
Martin



ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH 1/2] tpm_tis: Disable interrupt auto probing on a per-device basis

[Jason Gunthorpe]

On Tue, Dec 01, 2015 at 08:17:43AM +0100, Uwe Kleine-König wrote:
> Hello Jason,
> 
> On Mon, Nov 30, 2015 at 12:27:11PM -0700, Jason Gunthorpe wrote:
> > Instead of clearing the global interrupts flag when any device
> > does not have an interrupt just pass -1 through tpm_info.irq.
> 
> Is there a reason not to use 0 for the invalid irq?

0 already means 'no IRQ given, do auto-probe'. -1 is now 'no IRQ
given, disable IRQ operation'

Jason

Re: [PATCH 2/2] tpm_tis: Clean up the force=1 module parameter

[Jason Gunthorpe]

On Tue, Dec 01, 2015 at 08:28:35AM +0100, Uwe Kleine-König wrote:
> On Mon, Nov 30, 2015 at 12:27:12PM -0700, Jason Gunthorpe wrote:
> > The TPM core has long assumed that every device has a driver attached,
> > however b8b2c7d845d5 ("base/platform: assert that dev_pm_domain callbacks are
> > called unconditionally") breaks that assumption.
> 
> Maybe it's worth to point out that b8b2c7d845d5 didn't break it on
> purpose and is fixed accordingly. Still the assumption isn't valid, but
> works in practise.

Purposeful or not, it is the source of the bug this patch is
fixing.. I'm happy with any language, proposal?
> > +        res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +        if (res == NULL) {
> 
> indention problems here.

Woops, forgot to run check patch..

> > +	if (res)
> > +		tpm_info.irq = res->start;
> > +	else {
> 
> If one branch of an if/else has braces, all of them should.

Is that a thing now? Surprised checkpatch doesn't complain.

> >  static bool force;
> > +#ifdef CONFIG_X86
> >  module_param(force, bool, 0444);
> >  MODULE_PARM_DESC(force, "Force device probe rather than using ACPI entry");
> > +#endif
> 
> Is this added ifdef intended to be in this commit?

Yes, upon auditing all this it is clear the values we have are
hard-wired to x86, so this will never work on another platform.

I'm happy to put that in another patch.

> > -err_init:
> > -	platform_device_unregister(pdev);
> > -err_dev:
> > -	platform_driver_unregister(&tis_drv);
> > +out4:
> > +#ifdef CONFIG_ACPI
> > +	acpi_bus_unregister_driver(&tis_acpi_driver);
> > +out3:
> > +#endif
> > +	pnp_unregister_driver(&tis_pnp_driver);
> > +out2:
> > +	platform_device_unregister(force_pdev);
> > +out1:
> 
> Might be a matter of taste, but having nicer names for the error labels
> makes review easier. For example I would have called "out3"
> "err_register_acpi" instead. Then you can easily verify that it's placed
> right in the error path being directly after
> acpi_bus_unregister_driver.

The downside is it is harder to review the goto sites because there is
no longer much logic to their ordering, but sure, names seem a bit
more common in tpm.

> Also all kind of strange things happen if you later need to add a label
> between out2 and out3. drivers/scsi/hpsa.c for example used "clean2_5"
> in a similar situation.

Yes, it isn't so bad to do that.

Jason

Re: [tpmdd-devel] [PATCH 2/2] tpm_tis: Clean up the force=1 module parameter

[Jason Gunthorpe]

On Tue, Dec 01, 2015 at 12:50:28PM +0100, Wilck, Martin wrote:

> The patch introduces one user-visible change, because now the ACPI and
> PnP drivers are registered even with "force=1". This causes my TPM to be
> show up twice in sysfs:

Registering all the drivers is deliberate, IMHO, force should not be
used if the driver binds normally.

However, getting two is a bug, and it is because tpm_tis is not doing
resource management.

I'll add another patch to fix that..

Also, I'll change the order of the driver registers so that the
platform driver goes first, that way if force is used the platform
driver will bind and still auto probe irqs and then the other drivers
will bounce off the claimed resource.

Jason

Re: [PATCH 2/2] tpm_tis: Clean up the force=1 module parameter

[Jason Gunthorpe]

On Tue, Dec 01, 2015 at 10:35:08AM +0200, Jarkko Sakkinen wrote:

> In addition I want this fix as a single patch, not as two-patch set.
> The first patch might have made sense when the fix was being developed
> but now it's just really akward change.

No, you are not in tune with the kernel standard when you are
suggesting merging these patches. Each patch is self contained, encompasses a
single idea/change, and is justifiable on its own.

Ie SubmittingPatches explains:

 The point to remember is that each patch should make an easily understood
 change that can be verified by reviewers.  Each patch should be
 justifiable on its own merits.

If anything the larger patch should be split, because there is alot
going on there..

Jason

Re: [PATCH 2/2] tpm_tis: Clean up the force=1 module parameter

[Jarkko Sakkinen]

On Tue, Dec 01, 2015 at 10:43:42AM -0700, Jason Gunthorpe wrote:
> On Tue, Dec 01, 2015 at 10:35:08AM +0200, Jarkko Sakkinen wrote:
> 
> > In addition I want this fix as a single patch, not as two-patch set.
> > The first patch might have made sense when the fix was being developed
> > but now it's just really akward change.
> 
> No, you are not in tune with the kernel standard when you are
> suggesting merging these patches. Each patch is self contained, encompasses a
> single idea/change, and is justifiable on its own.
> 
> Ie SubmittingPatches explains:
> 
>  The point to remember is that each patch should make an easily understood
>  change that can be verified by reviewers.  Each patch should be
>  justifiable on its own merits.
> 
> If anything the larger patch should be split, because there is alot
> going on there..

Just saying that at least for me it was easier to understand what was
going on once I squashed the patch. Labels were the only really
confusing part, not the patch size...

> Jason

/Jarkko