Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1280219 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 2/2] tpm_tis: Clean up the force=1 module parameter Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2015-11-30 20:30 +0100
    Re: [PATCH 2/2] tpm_tis: Clean up the force=1 module parameter Uwe Kleine-König   <u.kleine-koenig@pengutronix.de> - 2015-12-01 08:30 +0100
      Re: [PATCH 2/2] tpm_tis: Clean up the force=1 module parameter Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2015-12-01 09:40 +0100
        Re: [PATCH 2/2] tpm_tis: Clean up the force=1 module parameter Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2015-12-01 18:50 +0100
          Re: [PATCH 2/2] tpm_tis: Clean up the force=1 module parameter Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2015-12-01 22:00 +0100
      Re: [PATCH 2/2] tpm_tis: Clean up the force=1 module parameter Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2015-12-01 18:40 +0100
    Re: [tpmdd-devel] [PATCH 2/2] tpm_tis: Clean up the force=1 module  parameter "Wilck, Martin" <martin.wilck@ts.fujitsu.com> - 2015-12-01 13:00 +0100
      Re: [tpmdd-devel] [PATCH 2/2] tpm_tis: Clean up the force=1 module  parameter Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2015-12-01 18:40 +0100

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

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

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

[toc] | [next] | [standalone]

#1280608

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

[toc] | [prev] | [next] | [standalone]

#1280638

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

[toc] | [prev] | [next] | [standalone]

#1281085

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

[toc] | [prev] | [next] | [standalone]

#1281209

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

[toc] | [prev] | [next] | [standalone]

#1281082

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

[toc] | [prev] | [next] | [standalone]

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

T24gTW8sIDIwMTUtMTEtMzAgYXQgMTI6MjcgLTA3MDAsIEphc29uIEd1bnRob3JwZSB3cm90ZToK
PiBUaGUgVFBNIGNvcmUgaGFzIGxvbmcgYXNzdW1lZCB0aGF0IGV2ZXJ5IGRldmljZSBoYXMgYSBk
cml2ZXIgYXR0YWNoZWQsCj4gaG93ZXZlciBiOGIyYzdkODQ1ZDUgKCJiYXNlL3BsYXRmb3JtOiBh
c3NlcnQgdGhhdCBkZXZfcG1fZG9tYWluIGNhbGxiYWNrcyBhcmUKPiBjYWxsZWQgdW5jb25kaXRp
b25hbGx5IikgYnJlYWtzIHRoYXQgYXNzdW1wdGlvbi4KPiAKPiBSZXdvcmsgdGhlIFRQTSBzZXR1
cCB0byBjcmVhdGUgYSBwbGF0Zm9ybSBkZXZpY2Ugd2l0aCByZXNvdXJjZXMgYW5kCj4gdGhlbiBh
bGxvdyB0aGUgZHJpdmVyIGNvcmUgdG8gbmF0dXJhbGx5IGJpbmQgYW5kIHByb2JlIGl0IHRocm91
Z2ggdGhlCj4gbm9ybWFsIG1lY2hhbmlzbXMuIEFsbCB0aGlzIHN0cnVjdHVyZSBpcyBuZWVkZWQg
YW55aG93IHRvIGVuYWJsZSBUUE0KPiBmb3IgT0YgZW52aXJvbm1lbnRzLgo+IAo+IEZpbmFsbHks
IHNpbmNlIHRoZSBlbnRpcmUgZmxvdyBpcyBjaGFuZ2luZyBjb252ZXJ0IHRoZSBpbml0L2V4aXQg
dG8gdXNlCj4gdGhlIG1vZGVybiBpZmRlZi1sZXNzIGNvZGluZyBzdHlsZSB3aGVuIHBvc3NpYmxl
Cj4gCj4gUmVwb3J0ZWQtYnk6ICJXaWxjaywgTWFydGluIiA8bWFydGluLndpbGNrQHRzLmZ1aml0
c3UuY29tPgo+IFNpZ25lZC1vZmYtYnk6IEphc29uIEd1bnRob3JwZSA8amd1bnRob3JwZUBvYnNp
ZGlhbnJlc2VhcmNoLmNvbT4KCkkgdGVzdGVkIHRoaXMgb24gbXkgc3lzdGVtLCBkZWxpYmVyYXRl
bHkgcmV2ZXJ0aW5nIG15IG93biBmaXggZm9yCnBsYXRmb3JtX2RyaXZlcl9wcm9iZSgpIGJlZm9y
ZWhhbmQuIEl0IHdvcmtzLCBubyBwYW5pYyBhbnkgbW9yZS4KClRoZSBwYXRjaCBpbnRyb2R1Y2Vz
IG9uZSB1c2VyLXZpc2libGUgY2hhbmdlLCBiZWNhdXNlIG5vdyB0aGUgQUNQSSBhbmQKUG5QIGRy
aXZlcnMgYXJlIHJlZ2lzdGVyZWQgZXZlbiB3aXRoICJmb3JjZT0xIi4gVGhpcyBjYXVzZXMgbXkg
VFBNIHRvIGJlCnNob3cgdXAgdHdpY2UgaW4gc3lzZnM6Cgovc3lzL2J1cy9hY3BpL2RyaXZlcnMv
dHBtX3Rpcy9NU0ZUMDEwMTowMAotPiAuLi8uLi8uLi8uLi9kZXZpY2VzL0xOWFNZU1RNOjAwL0xO
WFNZQlVTOjAwL01TRlQwMTAxOjAwCi9zeXMvYnVzL3BsYXRmb3JtL2RyaXZlcnMvdHBtX3Rpcy90
cG1fdGlzCi0+IC4uLy4uLy4uLy4uL2RldmljZXMvcGxhdGZvcm0vdHBtX3RpcwoKT25seSB0aGUg
cGxhdGZvcm0gZGV2aWNlIGlzIGFjdHVhbGx5IGJvdW5kIHRvIHRoZSBwaHlzaWNhbCBkZXZpY2Us
CnRob3VnaC4gSSdtIG5vdCBzdXJlIGlmIEkgbGlrZSB0aGlzLgoKUmVnYXJkcwpNYXJ0aW4KCgoK
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web