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

Groups > linux.kernel > #1281118 > unrolled thread

[PATCH v2 0/3] tpm_tis: Clean up force module parameter

Back to article view | Back to linux.kernel

Contents

  [PATCH v2 0/3] tpm_tis: Clean up force module parameter Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2015-12-01 20:00 +0100
    [PATCH v2 3/3] tpm_tis: Clean up the force=1 module parameter Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2015-12-01 20:00 +0100
      Re: [PATCH v2 3/3] tpm_tis: Clean up the force=1 module parameter Uwe Kleine-König   <u.kleine-koenig@pengutronix.de> - 2015-12-01 20:40 +0100
        Re: [PATCH v2 3/3] tpm_tis: Clean up the force=1 module parameter Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2015-12-01 21:00 +0100
    [PATCH v2 1/3] tpm_tis: Disable interrupt auto probing on a per-device basis Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2015-12-01 20:00 +0100
      Re: [PATCH v2 1/3] tpm_tis: Disable interrupt auto probing on a  per-device basis Uwe Kleine-König   <u.kleine-koenig@pengutronix.de> - 2015-12-01 20:20 +0100
        Re: [PATCH v2 1/3] tpm_tis: Disable interrupt auto probing on a  per-device basis Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2015-12-01 20:40 +0100
    [PATCH v2 2/3] tpm_tis: Use devm_ioremap_resource Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2015-12-01 20:00 +0100
      Re: [PATCH v2 2/3] tpm_tis: Use devm_ioremap_resource Uwe Kleine-König   <u.kleine-koenig@pengutronix.de> - 2015-12-01 20:30 +0100
        Re: [PATCH v2 2/3] tpm_tis: Use devm_ioremap_resource Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2015-12-01 20:50 +0100
          Re: [PATCH v2 2/3] tpm_tis: Use devm_ioremap_resource Uwe Kleine-König   <u.kleine-koenig@pengutronix.de> - 2015-12-01 21:00 +0100
            Re: [PATCH v2 2/3] tpm_tis: Use devm_ioremap_resource Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2015-12-01 22:00 +0100
    Re: [PATCH v2 0/3] tpm_tis: Clean up force module parameter Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2015-12-01 22:20 +0100
    Re: [PATCH v2 0/3] tpm_tis: Clean up force module parameter Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2015-12-01 22:40 +0100
      Re: [PATCH v2 0/3] tpm_tis: Clean up force module parameter Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2015-12-02 00:10 +0100
        Re: [PATCH v2 0/3] tpm_tis: Clean up force module parameter Peter Huewe <peterhuewe@gmx.de> - 2015-12-02 02:20 +0100
          Re: [PATCH v2 0/3] tpm_tis: Clean up force module parameter Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2015-12-02 09:20 +0100
            Re: [PATCH v2 0/3] tpm_tis: Clean up force module parameter Peter Huewe <peterhuewe@gmx.de> - 2015-12-02 10:20 +0100
        Re: [PATCH v2 0/3] tpm_tis: Clean up force module parameter Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2015-12-02 09:20 +0100
          Re: [PATCH v2 0/3] tpm_tis: Clean up force module parameter Uwe Kleine-König   <u.kleine-koenig@pengutronix.de> - 2015-12-02 09:30 +0100
            Re: [PATCH v2 0/3] tpm_tis: Clean up force module parameter Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2015-12-02 18:00 +0100
              Re: [PATCH v2 0/3] tpm_tis: Clean up force module parameter Uwe Kleine-König   <u.kleine-koenig@pengutronix.de> - 2015-12-02 18:10 +0100
              [PATCH v3] base/platform: fix binding for drivers without probe callback martin.wilck@ts.fujitsu.com - 2015-12-03 10:00 +0100
                Re: [PATCH v3] base/platform: fix binding for drivers without probe  callback Uwe Kleine-König   <u.kleine-koenig@pengutronix.de> - 2015-12-03 10:10 +0100
                Re: [tpmdd-devel] [PATCH v3] base/platform: fix binding for drivers  without probe callback Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2015-12-03 10:40 +0100
      Re: [PATCH v2 0/3] tpm_tis: Clean up force module parameter Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2015-12-02 19:30 +0100
        Re: [tpmdd-devel] [PATCH v2 0/3] tpm_tis: Clean up force module  parameter Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2015-12-02 20:20 +0100
          Re: [tpmdd-devel] [PATCH v2 0/3] tpm_tis: Clean up force module  parameter Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2015-12-03 07:10 +0100
            Re: [tpmdd-devel] [PATCH v2 0/3] tpm_tis: Clean up force module  parameter Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2015-12-03 19:20 +0100
              Re: [tpmdd-devel] [PATCH v2 0/3] tpm_tis: Clean up force module  parameter Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2015-12-06 05:10 +0100
                Re: [tpmdd-devel] [PATCH v2 0/3] tpm_tis: Clean up force module  parameter Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2015-12-06 05:20 +0100
                  Re: [tpmdd-devel] [PATCH v2 0/3] tpm_tis: Clean up force module  parameter Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2015-12-06 05:30 +0100
                  Re: [tpmdd-devel] [PATCH v2 0/3] tpm_tis: Clean up force module  parameter Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2015-12-07 07:20 +0100
                  Re: [tpmdd-devel] [PATCH v2 0/3] tpm_tis: Clean up force module  parameter "Wilck, Martin" <martin.wilck@ts.fujitsu.com> - 2015-12-07 09:10 +0100
                    Re: [tpmdd-devel] [PATCH v2 0/3] tpm_tis: Clean up force module  parameter Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2015-12-07 10:00 +0100
                      Re: [tpmdd-devel] [PATCH v2 0/3] tpm_tis: Clean up force module  parameter "Wilck, Martin" <martin.wilck@ts.fujitsu.com> - 2015-12-07 11:00 +0100
                        Re: [tpmdd-devel] [PATCH v2 0/3] tpm_tis: Clean up force module  parameter Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2015-12-07 11:20 +0100
          Re: [tpmdd-devel] [PATCH v2 0/3] tpm_tis: Clean up force module  parameter "Wilck, Martin" <martin.wilck@ts.fujitsu.com> - 2015-12-03 09:40 +0100
            Re: [tpmdd-devel] [PATCH v2 0/3] tpm_tis: Clean up force module  parameter Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2015-12-03 18:10 +0100
              Re: [tpmdd-devel] [PATCH v2 0/3] tpm_tis: Clean up force module  parameter "Wilck, Martin" <martin.wilck@ts.fujitsu.com> - 2015-12-04 09:40 +0100
              Re: [tpmdd-devel] [PATCH v2 0/3] tpm_tis: Clean up force module  parameter "Wilck, Martin" <martin.wilck@ts.fujitsu.com> - 2015-12-04 10:20 +0100
                Re: [tpmdd-devel] [PATCH v2 0/3] tpm_tis: Clean up force module  parameter Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2015-12-04 19:10 +0100
                  Re: [tpmdd-devel] [PATCH v2 0/3] tpm_tis: Clean up force module  parameter "Wilck, Martin" <martin.wilck@ts.fujitsu.com> - 2015-12-07 11:00 +0100
                    Re: [tpmdd-devel] [PATCH v2 0/3] tpm_tis: Clean up force module  parameter Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2015-12-07 18:40 +0100
        Re: [PATCH v2 0/3] tpm_tis: Clean up force module parameter Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2015-12-03 07:00 +0100
    Re: [PATCH v2 0/3] tpm_tis: Clean up force module parameter "Wilck, Martin" <martin.wilck@ts.fujitsu.com> - 2015-12-02 13:40 +0100
      Re: [PATCH v2 0/3] tpm_tis: Clean up force module parameter Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2015-12-02 19:20 +0100

Page 1 of 3  [1] 2 3  Next page →

#1281118 — [PATCH v2 0/3] tpm_tis: Clean up force module parameter

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 probe/release code needs to be re-used for that.
 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.

v2:
 - Make sure we request the mem resource in tpm_tis to avoid double-loading
   the driver
 - Re-order the init sequence so that a forced platform device gets first crack at
   loading, and excludes the other mechanisms via the above
 - Checkpatch clean
 - Gotos renamed

Martin, this should fix the double loading you noticed, please confirm.  There
is a possibility the force path needs a bit more code to be compatible with
devm_ioremap_resource, I'm not sure, hoping not.

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

 drivers/char/tpm/tpm_tis.c | 203 +++++++++++++++++++++++++++------------------
 1 file changed, 122 insertions(+), 81 deletions(-)

-- 
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]

#1281124 — [PATCH v2 3/3] tpm_tis: Clean up the force=1 module parameter

The TPM core has long assumed that every device has a driver attached,
however commit 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 | 170 +++++++++++++++++++++++++++------------------
 1 file changed, 104 insertions(+), 66 deletions(-)

diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
index 1032855c46b2..19beaf57f2a9 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 */
 };
@@ -71,15 +69,6 @@ struct tpm_info {
 	int irq;
 };
 
-static struct tpm_info tis_default_info = {
-	.res = {
-		.start = TIS_MEM_BASE,
-		.end = TIS_MEM_BASE + TIS_MEM_LEN - 1,
-		.flags = IORESOURCE_MEM,
-	},
-	.irq = 0,
-};
-
 /* Some timeout values are needed before it is known whether the chip is
  * TPM 1.0 or TPM 2.0.
  */
@@ -849,7 +838,6 @@ out_err:
 	return rc;
 }
 
-#ifdef CONFIG_PM_SLEEP
 static void tpm_tis_reenable_interrupts(struct tpm_chip *chip)
 {
 	u32 intmask;
@@ -891,11 +879,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)
 {
@@ -913,14 +899,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);
 }
@@ -961,7 +945,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)
@@ -1034,80 +1017,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;
+	}
+	memcpy(&tpm_info.res, res, sizeof(*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 = pnp_register_driver(&tis_pnp_driver);
-		if (rc)
-			return rc;
-	}
-#endif
+
+	rc = force_device();
+	if (rc)
+		goto err_force;
+
+	rc = platform_driver_register(&tis_drv);
+	if (rc)
+		goto err_platform;
+
 #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 err_acpi;
 #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;
+	if (IS_ENABLED(CONFIG_PNP)) {
+		rc = pnp_register_driver(&tis_pnp_driver);
+		if (rc)
+			goto err_pnp;
 	}
-	rc = tpm_tis_init(&pdev->dev, &tis_default_info, NULL);
-	if (rc)
-		goto err_init;
+
 	return 0;
-err_init:
-	platform_device_unregister(pdev);
-err_dev:
-	platform_driver_unregister(&tis_drv);
+
+err_pnp:
+#ifdef CONFIG_ACPI
+	acpi_bus_unregister_driver(&tis_acpi_driver);
+err_acpi:
+#endif
+	platform_device_unregister(force_pdev);
+err_platform:
+	if (force_pdev)
+		platform_device_unregister(force_pdev);
+err_force:
 	return rc;
 }
 
 static void __exit cleanup_tis(void)
 {
-	struct tpm_chip *chip;
-#if defined(CONFIG_PNP) || defined(CONFIG_ACPI)
-	if (!force) {
+	pnp_unregister_driver(&tis_pnp_driver);
 #ifdef CONFIG_ACPI
-		acpi_bus_unregister_driver(&tis_acpi_driver);
-#endif
-#ifdef CONFIG_PNP
-		pnp_unregister_driver(&tis_pnp_driver);
-#endif
-		return;
-	}
+	acpi_bus_unregister_driver(&tis_acpi_driver);
 #endif
-	chip = dev_get_drvdata(&pdev->dev);
-	tpm_chip_unregister(chip);
-	tpm_tis_remove(chip);
-	platform_device_unregister(pdev);
 	platform_driver_unregister(&tis_drv);
+
+	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] | [prev] | [next] | [standalone]

#1281150 — Re: [PATCH v2 3/3] tpm_tis: Clean up the force=1 module parameter

Hello,

On Tue, Dec 01, 2015 at 11:58:29AM -0700, Jason Gunthorpe wrote:
> The TPM core has long assumed that every device has a driver attached,
> however commit b8b2c7d845d5 ("base/platform: assert that dev_pm_domain
> callbacks are called unconditionally") breaks that assumption.

you asked for an alternative wording here. What about:

	The TPM core has long assumed that every device has a driver
	attached, which is not valid. This was noticed with commit
	b8b2c7d845d5 ("base/platform: assert that dev_pm_domain
	callbacks are called unconditionally") which made probing of the
	tpm_tis device fail by mistake and resulted in an oops later on.
	
?

> 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>

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]

#1281160 — Re: [PATCH v2 3/3] tpm_tis: Clean up the force=1 module parameter

On Tue, Dec 01, 2015 at 08:33:58PM +0100, Uwe Kleine-König wrote:
> Hello,
> 
> On Tue, Dec 01, 2015 at 11:58:29AM -0700, Jason Gunthorpe wrote:
> > The TPM core has long assumed that every device has a driver attached,
> > however commit b8b2c7d845d5 ("base/platform: assert that dev_pm_domain
> > callbacks are called unconditionally") breaks that assumption.
> 
> you asked for an alternative wording here. What about:
> 
> 	The TPM core has long assumed that every device has a driver
> 	attached, which is not valid.

But it is valid, it is an invariant of the tpm core that a driver be
attached, and prior to 'b8b that has been satisfied.

>       This was noticed with commit
> 	b8b2c7d845d5 ("base/platform: assert that dev_pm_domain
> 	callbacks are called unconditionally") which made probing of the
> 	tpm_tis device fail by mistake and resulted in an oops later on.

The probe didn't fail, the 'b8b causes a NULL probe function to result
in no driver being attached.

How about:

 The TPM has for a long time required that every device it uses has an
 attached driver. In the force case the tpm_tis driver met this via
 platform_register_simple and a NULL probe function for the driver.
 However, commit b8b2c7d845d5 ("base/platform: assert that dev_pm_domain
 callbacks are called unconditionally") causes NULL probe functions
 to no longer bind a driver.

Did we ever reach a conclusion if Martin's patch should go ahead?

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]

#1281125 — [PATCH v2 1/3] tpm_tis: Disable interrupt auto probing on a per-device basis

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

--
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]

#1281140 — Re: [PATCH v2 1/3] tpm_tis: Disable interrupt auto probing on a per-device basis

Hello,

On Tue, Dec 01, 2015 at 11:58:27AM -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.
> 
> 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;

I'd add a comment here about the possible values of irq and their
interpretation. Something like:

	/*
	 * irq > 0 means: use irq $irq;
	 * irq = 0 means: autoprobe for an irq;
	 * irq = -1 means: no irq support
	 */

>  };
>  
>  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;

It's definitly a nice improvement of this patch that the init functions
don't change the module parameter any more. (I didn't check if all
changes are gone now, but at least it's two modifications less.)

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]

#1281152 — Re: [PATCH v2 1/3] tpm_tis: Disable interrupt auto probing on a per-device basis

On Tue, Dec 01, 2015 at 08:19:18PM +0100, Uwe Kleine-König wrote:
> I'd add a comment here about the possible values of irq and their
> interpretation. Something like:

Done

> It's definitly a nice improvement of this patch that the init functions
> don't change the module parameter any more. (I didn't check if all
> changes are gone now, but at least it's two modifications less.)

I checked, this gets them all.

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]

#1281129 — [PATCH v2 2/3] tpm_tis: Use devm_ioremap_resource

This does a request_resource under the covers which means tis holds a
lock on the memory range it is using so other drivers cannot grab it.
When doing probing it is important to ensure that other drivers are
not using the same range before tis starts touching it.

To do this flow the actual struct resource from the device right
through to devm_ioremap_resource. This ensures all the proper resource
meta-data is carried down.

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

diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
index 0a2d94f3d679..1032855c46b2 100644
--- a/drivers/char/tpm/tpm_tis.c
+++ b/drivers/char/tpm/tpm_tis.c
@@ -67,14 +67,16 @@ enum tis_defaults {
 };
 
 struct tpm_info {
-	unsigned long start;
-	unsigned long len;
+	struct resource res;
 	int irq;
 };
 
 static struct tpm_info tis_default_info = {
-	.start = TIS_MEM_BASE,
-	.len = TIS_MEM_LEN,
+	.res = {
+		.start = TIS_MEM_BASE,
+		.end = TIS_MEM_BASE + TIS_MEM_LEN - 1,
+		.flags = IORESOURCE_MEM,
+	},
 	.irq = 0,
 };
 
@@ -716,7 +718,7 @@ static int tpm_tis_init(struct device *dev, struct tpm_info *tpm_info,
 	chip->acpi_dev_handle = acpi_dev_handle;
 #endif
 
-	chip->vendor.iobase = devm_ioremap(dev, tpm_info->start, tpm_info->len);
+	chip->vendor.iobase = devm_ioremap_resource(dev, &tpm_info->res);
 	if (!chip->vendor.iobase)
 		return -EIO;
 
@@ -899,9 +901,12 @@ static int tpm_tis_pnp_init(struct pnp_dev *pnp_dev,
 {
 	struct tpm_info tpm_info = {};
 	acpi_handle acpi_dev_handle = NULL;
+	struct resource *res;
 
-	tpm_info.start = pnp_mem_start(pnp_dev, 0);
-	tpm_info.len = pnp_mem_len(pnp_dev, 0);
+	res = pnp_get_resource(pnp_dev, IORESOURCE_MEM, 0);
+	if (!res)
+		return -ENODEV;
+	memcpy(&tpm_info.res, res, sizeof(*res));
 
 	if (pnp_irq_valid(pnp_dev, 0))
 		tpm_info.irq = pnp_irq(pnp_dev, 0);
@@ -964,12 +969,9 @@ static int tpm_check_resource(struct acpi_resource *ares, void *data)
 	struct tpm_info *tpm_info = (struct tpm_info *) data;
 	struct resource res;
 
-	if (acpi_dev_resource_interrupt(ares, 0, &res)) {
+	if (acpi_dev_resource_interrupt(ares, 0, &res))
 		tpm_info->irq = res.start;
-	} else if (acpi_dev_resource_memory(ares, &res)) {
-		tpm_info->start = res.start;
-		tpm_info->len = resource_size(&res);
-	}
+	acpi_dev_resource_memory(ares, &tpm_info->res);
 
 	return 1;
 }
@@ -992,6 +994,9 @@ static int tpm_tis_acpi_init(struct acpi_device *acpi_dev)
 
 	acpi_dev_free_resource_list(&resources);
 
+	if (resource_size(&tpm_info.res) == 0)
+		return -ENODEV;
+
 	if (is_itpm(acpi_dev))
 		itpm = true;
 
-- 
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] | [prev] | [next] | [standalone]

#1281146 — Re: [PATCH v2 2/3] tpm_tis: Use devm_ioremap_resource

Hello,

On Tue, Dec 01, 2015 at 11:58:28AM -0700, Jason Gunthorpe wrote:
> This does a request_resource under the covers which means tis holds a
> lock on the memory range it is using so other drivers cannot grab it.
> When doing probing it is important to ensure that other drivers are
> not using the same range before tis starts touching it.
> 
> To do this flow the actual struct resource from the device right
> through to devm_ioremap_resource. This ensures all the proper resource
> meta-data is carried down.
> 
> Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> ---
>  drivers/char/tpm/tpm_tis.c | 29 +++++++++++++++++------------
>  1 file changed, 17 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
> index 0a2d94f3d679..1032855c46b2 100644
> --- a/drivers/char/tpm/tpm_tis.c
> +++ b/drivers/char/tpm/tpm_tis.c
> @@ -67,14 +67,16 @@ enum tis_defaults {
>  };
>  
>  struct tpm_info {
> -	unsigned long start;
> -	unsigned long len;
> +	struct resource res;
>  	int irq;
>  };
>  
>  static struct tpm_info tis_default_info = {
> -	.start = TIS_MEM_BASE,
> -	.len = TIS_MEM_LEN,
> +	.res = {
> +		.start = TIS_MEM_BASE,
> +		.end = TIS_MEM_BASE + TIS_MEM_LEN - 1,
> +		.flags = IORESOURCE_MEM,
> +	},
>  	.irq = 0,
>  };
>  
> @@ -716,7 +718,7 @@ static int tpm_tis_init(struct device *dev, struct tpm_info *tpm_info,
>  	chip->acpi_dev_handle = acpi_dev_handle;
>  #endif
>  
> -	chip->vendor.iobase = devm_ioremap(dev, tpm_info->start, tpm_info->len);
> +	chip->vendor.iobase = devm_ioremap_resource(dev, &tpm_info->res);
>  	if (!chip->vendor.iobase)
>  		return -EIO;
>  
> @@ -899,9 +901,12 @@ static int tpm_tis_pnp_init(struct pnp_dev *pnp_dev,
>  {
>  	struct tpm_info tpm_info = {};
>  	acpi_handle acpi_dev_handle = NULL;
> +	struct resource *res;
>  
> -	tpm_info.start = pnp_mem_start(pnp_dev, 0);
> -	tpm_info.len = pnp_mem_len(pnp_dev, 0);
> +	res = pnp_get_resource(pnp_dev, IORESOURCE_MEM, 0);
> +	if (!res)
> +		return -ENODEV;
> +	memcpy(&tpm_info.res, res, sizeof(*res));

I think you can do

	tpm_info.res = res;

here, which IMHO reads nicer and maybe is even more efficient (I don't
know much about x86).

>  	if (pnp_irq_valid(pnp_dev, 0))
>  		tpm_info.irq = pnp_irq(pnp_dev, 0);
> @@ -964,12 +969,9 @@ static int tpm_check_resource(struct acpi_resource *ares, void *data)
>  	struct tpm_info *tpm_info = (struct tpm_info *) data;
>  	struct resource res;
>  
> -	if (acpi_dev_resource_interrupt(ares, 0, &res)) {
> +	if (acpi_dev_resource_interrupt(ares, 0, &res))
>  		tpm_info->irq = res.start;
> -	} else if (acpi_dev_resource_memory(ares, &res)) {
> -		tpm_info->start = res.start;
> -		tpm_info->len = resource_size(&res);
> -	}
> +	acpi_dev_resource_memory(ares, &tpm_info->res);
>  
>  	return 1;
>  }
> @@ -992,6 +994,9 @@ static int tpm_tis_acpi_init(struct acpi_device *acpi_dev)
>  
>  	acpi_dev_free_resource_list(&resources);
>  
> +	if (resource_size(&tpm_info.res) == 0)
> +		return -ENODEV;
> +

Does this result in an error message from the upper layers?

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]

#1281158 — Re: [PATCH v2 2/3] tpm_tis: Use devm_ioremap_resource

On Tue, Dec 01, 2015 at 08:22:40PM +0100, Uwe Kleine-König wrote:

> here, which IMHO reads nicer and maybe is even more efficient (I don't
> know much about x86).

Sure

> > +	if (resource_size(&tpm_info.res) == 0)
> > +		return -ENODEV;
> > +
> 
> Does this result in an error message from the upper layers?

I think so, yes. The probe will fail which causes the driver core to
report a message.

The scenario this triggers is if the acpi stuff doesn't have a mem
resource, which is a firmware bug, I think. It could get a dedicated
print if that is what you are thinking?

-	if (resource_size(&tpm_info.res) == 0)
+	if (tpm_info.res.flags == 0) {
+		dev_err(&pdev->dev, FW_BUG "no memory resource defined\n");
 		return -ENODEV;
+	}
 
 	if (is_itpm(acpi_dev))
 		itpm = true;

[resource_size is wrong as well since it will return 1 for
 0'd struct resource, sigh..]

Previously it would try to call devm_ioremap with start/len=0 as the
range which should also fails in broadly the same way. So this is just
moving the existing failure up.

Something was needed because the change to struct resource means the
new code would call devm_ioremap_resource with a 0'd resource struct,
which is not as safe as start/len=0 as before.

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]

#1281162 — Re: [PATCH v2 2/3] tpm_tis: Use devm_ioremap_resource

Hello Jason,

On Tue, Dec 01, 2015 at 12:44:19PM -0700, Jason Gunthorpe wrote:
> On Tue, Dec 01, 2015 at 08:22:40PM +0100, Uwe Kleine-König wrote:
> > > +	if (resource_size(&tpm_info.res) == 0)
> > > +		return -ENODEV;
> > > +
> > 
> > Does this result in an error message from the upper layers?
> 
> I think so, yes. The probe will fail which causes the driver core to
> report a message.
> 
> The scenario this triggers is if the acpi stuff doesn't have a mem
> resource, which is a firmware bug, I think. It could get a dedicated
> print if that is what you are thinking?

The issue I saw is: There are three(?) ways the tpm could be bound. If
one of the succeeds, the other two are expected to fail. But in this
case an error message, that the tpm failed to be bound is at least
misleading.

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]

#1281208 — Re: [PATCH v2 2/3] tpm_tis: Use devm_ioremap_resource

On Tue, Dec 01, 2015 at 08:52:17PM +0100, Uwe Kleine-König wrote:

> The issue I saw is: There are three(?) ways the tpm could be bound. If
> one of the succeeds, the other two are expected to fail. But in this
> case an error message, that the tpm failed to be bound is at least
> misleading.

My expectation is that the platform will never have a device that can
be bound to more than one and/or the driver core will prevent it (ie
if a PNP and ACPI driver claim the same ID the core should bind the
ACPI device only, not bind the ACPI device then downgrade to PNP and
try to bind the PNP device)

This issue pre-exists this patch. All this patch is doing is forcing
the tpm_tis to fail to bind instead of potentially running two drivers
on the same iorange at once.

The only case where this might not be true is if the user specifies
force. In this case, if forcing and there is acpi/pnp tpm at the same
address, then there will be a message failing the acpi/pnp bind. I
feel that is OK because it does indicate the user has done something
very questionable. (there is little reason to use force if acpi
already has the tpm at the same address range)

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]

#1281222

On Tue, Dec 01, 2015 at 11:58:26AM -0700, Jason Gunthorpe wrote:
> 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 probe/release code needs to be re-used for that.
>  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.
> 
> v2:
>  - Make sure we request the mem resource in tpm_tis to avoid double-loading
>    the driver
>  - Re-order the init sequence so that a forced platform device gets first crack at
>    loading, and excludes the other mechanisms via the above
>  - Checkpatch clean
>  - Gotos renamed
> 
> Martin, this should fix the double loading you noticed, please confirm.  There
> is a possibility the force path needs a bit more code to be compatible with
> devm_ioremap_resource, I'm not sure, hoping not.

Just wanted to quickly say that I'm good with your interrupt rework
patches. I did only one squash as you can see:

https://github.com/jsakkine/linux-tpmdd/commits/master

For the other changes your arguments how patches should be separated in
this rework made perfect sense after a few re-reads.

I can accept them once I've tested them but in order to test them we
have to get these patches reviewed first as soon as possible.

/Jarkko

> Jason Gunthorpe (3):
>   tpm_tis: Disable interrupt auto probing on a per-device basis
>   tpm_tis: Use devm_ioremap_resource
>   tpm_tis: Clean up the force=1 module parameter
> 
>  drivers/char/tpm/tpm_tis.c | 203 +++++++++++++++++++++++++++------------------
>  1 file changed, 122 insertions(+), 81 deletions(-)
> 
> -- 
> 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] | [prev] | [next] | [standalone]

#1281233

On Tue, Dec 01, 2015 at 11:58:26AM -0700, Jason Gunthorpe wrote:
> 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 probe/release code needs to be re-used for that.
>  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.
> 
> v2:
>  - Make sure we request the mem resource in tpm_tis to avoid double-loading
>    the driver
>  - Re-order the init sequence so that a forced platform device gets first crack at
>    loading, and excludes the other mechanisms via the above
>  - Checkpatch clean
>  - Gotos renamed
> 
> Martin, this should fix the double loading you noticed, please confirm.  There
> is a possibility the force path needs a bit more code to be compatible with
> devm_ioremap_resource, I'm not sure, hoping not.
> 
> Jason Gunthorpe (3):
>   tpm_tis: Disable interrupt auto probing on a per-device basis
>   tpm_tis: Use devm_ioremap_resource
>   tpm_tis: Clean up the force=1 module parameter

I went through the patches and didn't see anything that would shock me
enough not to apply the patches in the current if they also work when
tested *but* are these release critical for Linux v4.4?

I got a bit confused about the discussion that was going on about "where
to fix the probe" crash whether or not both it should be fixed in both
places.

Could you possibly make these apply on top of security/next and
re-submit if needed?

/Jarkko

>  drivers/char/tpm/tpm_tis.c | 203 +++++++++++++++++++++++++++------------------
>  1 file changed, 122 insertions(+), 81 deletions(-)
> 
> -- 
> 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] | [prev] | [next] | [standalone]

#1281295

On Tue, Dec 01, 2015 at 11:33:51PM +0200, Jarkko Sakkinen wrote:

> I went through the patches and didn't see anything that would shock me
> enough not to apply the patches in the current if they also work when
> tested *but* are these release critical for Linux v4.4?
> 
> I got a bit confused about the discussion that was going on about "where
> to fix the probe" crash whether or not both it should be fixed in both
> places.

I'm also confused by that..

It sounds like force=1 is broken in 4.4 right now - do we care? Should
we fix this by using Martin's patch?

These changes are complex enough they really shouldn't go into 4.4
unless absolutely necessary.

> Could you possibly make these apply on top of security/next and
> re-submit if needed?

It isn't trivial to reorder all 10 patches to do this, I'd like to
know we need to do this for sure first. Uwe?

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]

#1281389

Am 1. Dezember 2015 14:22:23 PST, schrieb Jason Gunthorpe <jgunthorpe@obsidianresearch.com>:
>On Tue, Dec 01, 2015 at 11:33:51PM +0200, Jarkko Sakkinen wrote:
>
>> I went through the patches and didn't see anything that would shock
>me
>> enough not to apply the patches in the current if they also work when
>> tested *but* are these release critical for Linux v4.4?
>> 
>> I got a bit confused about the discussion that was going on about
>"where
>> to fix the probe" crash whether or not both it should be fixed in
>both
>> places.
>
>I'm also confused by that..
>
>It sounds like force=1 is broken in 4.4 right now - do we care? Should
>we fix this by using Martin's patch?

I'm not 100% sure if force=1 is broken in 4.3 as well, as I oops when I have my tpm_crb loaded and then call modprobe tpm_tis force=1
Peter
>
>These changes are complex enough they really shouldn't go into 4.4
>unless absolutely necessary.
>
>> Could you possibly make these apply on top of security/next and
>> re-submit if needed?
>
>It isn't trivial to reorder all 10 patches to do this, I'd like to
>know we need to do this for sure first. Uwe?
>
>Jason

-- 
Sent from my mobile
--
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]

#1281513

On Tue, Dec 01, 2015 at 05:15:14PM -0800, Peter Huewe wrote:
> 
> 
> Am 1. Dezember 2015 14:22:23 PST, schrieb Jason Gunthorpe <jgunthorpe@obsidianresearch.com>:
> >On Tue, Dec 01, 2015 at 11:33:51PM +0200, Jarkko Sakkinen wrote:
> >
> >> I went through the patches and didn't see anything that would shock
> >me
> >> enough not to apply the patches in the current if they also work when
> >> tested *but* are these release critical for Linux v4.4?
> >> 
> >> I got a bit confused about the discussion that was going on about
> >"where
> >> to fix the probe" crash whether or not both it should be fixed in
> >both
> >> places.
> >
> >I'm also confused by that..
> >
> >It sounds like force=1 is broken in 4.4 right now - do we care? Should
> >we fix this by using Martin's patch?
> 
> I'm not 100% sure if force=1 is broken in 4.3 as well, as I oops when
> I have my tpm_crb loaded and then call modprobe tpm_tis force=1
> Peter

It'd have to be a different regression because v4.3 does not contain the
change that breaks this in v4.4. You had a NUC with discrete TPM module,
am I remembering right?

/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]

#1281536

Am 2. Dezember 2015 00:14:23 PST, schrieb Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>:
>On Tue, Dec 01, 2015 at 05:15:14PM -0800, Peter Huewe wrote:
>> 
>> 
>> Am 1. Dezember 2015 14:22:23 PST, schrieb Jason Gunthorpe
><jgunthorpe@obsidianresearch.com>:
>> >On Tue, Dec 01, 2015 at 11:33:51PM +0200, Jarkko Sakkinen wrote:
>> >
>> >> I went through the patches and didn't see anything that would
>shock
>> >me
>> >> enough not to apply the patches in the current if they also work
>when
>> >> tested *but* are these release critical for Linux v4.4?
>> >> 
>> >> I got a bit confused about the discussion that was going on about
>> >"where
>> >> to fix the probe" crash whether or not both it should be fixed in
>> >both
>> >> places.
>> >
>> >I'm also confused by that..
>> >
>> >It sounds like force=1 is broken in 4.4 right now - do we care?
>Should
>> >we fix this by using Martin's patch?
>> 
>> I'm not 100% sure if force=1 is broken in 4.3 as well, as I oops when
>> I have my tpm_crb loaded and then call modprobe tpm_tis force=1
>> Peter
>
>It'd have to be a different regression because v4.3 does not contain
>the
>change that breaks this in v4.4. You had a NUC with discrete TPM
>module,
>am I remembering right?
>
Nope, intel fw tpm2.0 in my acer laptop.
No nucs here
>/Jarkko

-- 
Sent from my mobile
--
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]

#1281511

On Tue, Dec 01, 2015 at 03:22:23PM -0700, Jason Gunthorpe wrote:
> On Tue, Dec 01, 2015 at 11:33:51PM +0200, Jarkko Sakkinen wrote:
> 
> > I went through the patches and didn't see anything that would shock me
> > enough not to apply the patches in the current if they also work when
> > tested *but* are these release critical for Linux v4.4?
> > 
> > I got a bit confused about the discussion that was going on about "where
> > to fix the probe" crash whether or not both it should be fixed in both
> > places.
> 
> I'm also confused by that..
> 
> It sounds like force=1 is broken in 4.4 right now - do we care? Should
> we fix this by using Martin's patch?
> 
> These changes are complex enough they really shouldn't go into 4.4
> unless absolutely necessary.

The reasons I'm asking this are:

* I'm planning to do v4.5 pull request soon.
* If this need to be get this into v4.4, we should act fast. Given the
  complexity of the changes I'd not recommend that unless it is a life
  and death question.

> > Could you possibly make these apply on top of security/next and
> > re-submit if needed?
> 
> It isn't trivial to reorder all 10 patches to do this, I'd like to
> know we need to do this for sure first. Uwe?

Agreed. First we have to know whether these changes have go to v4.4.

> 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]

#1281520

Hello,

Cc += gregkh

On Wed, Dec 02, 2015 at 10:11:14AM +0200, Jarkko Sakkinen wrote:
> On Tue, Dec 01, 2015 at 03:22:23PM -0700, Jason Gunthorpe wrote:
> > On Tue, Dec 01, 2015 at 11:33:51PM +0200, Jarkko Sakkinen wrote:
> > 
> > > I went through the patches and didn't see anything that would shock me
> > > enough not to apply the patches in the current if they also work when
> > > tested *but* are these release critical for Linux v4.4?
> > > 
> > > I got a bit confused about the discussion that was going on about "where
> > > to fix the probe" crash whether or not both it should be fixed in both
> > > places.
> > 
> > I'm also confused by that..
> > 
> > It sounds like force=1 is broken in 4.4 right now - do we care? Should
> > we fix this by using Martin's patch?
> > 
> > These changes are complex enough they really shouldn't go into 4.4
> > unless absolutely necessary.
> 
> The reasons I'm asking this are:
> 
> * I'm planning to do v4.5 pull request soon.
> * If this need to be get this into v4.4, we should act fast. Given the
>   complexity of the changes I'd not recommend that unless it is a life
>   and death question.

I'd say we should repair b8b2c7d845d5 ("base/platform: assert that
dev_pm_domain callbacks are called unconditionally") for 4.4-rc$next and
live with the problem that the tpm driver had since long another
release.

The fix is already available, just some minor nitpicking regarding the
commit log has still to be resolved.
 
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]

Page 1 of 3  [1] 2 3  Next page →

Back to top | Article view | linux.kernel

csiph-web