Message-ID: <20151201083508.GA14084@intel.com>
Date:	Tue, 1 Dec 2015 10:35:08 +0200
From:	Jarkko Sakkinen <jarkko.sakkinen@...ux.intel.com>
To:	Uwe Kleine-König 
	<u.kleine-koenig@...gutronix.de>
Cc:	Jason Gunthorpe <jgunthorpe@...idianresearch.com>,
	tpmdd-devel@...ts.sourceforge.net, linux-kernel@...r.kernel.org,
	Martin Wilck <Martin.Wilck@...fujitsu.com>,
	Peter Huewe <peterhuewe@....de>
Subject: Re: [PATCH 2/2] tpm_tis: Clean up the force=1 module parameter

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@...fujitsu.com>
> > Signed-off-by: Jason Gunthorpe <jgunthorpe@...idianresearch.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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists