| Message ID | 1456766996-9300-5-git-send-email-stefanb@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
On Mon, Feb 29, 2016 at 12:29:50PM -0500, Stefan Berger wrote: > Now that the tpm core has strong locking around 'ops' it is possible > to remove a TPM driver, module and all, even while user space still > has things like /dev/tpmX open. For consistency and simplicity, drop > the module locking entirely. > > Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com> Reviewed-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com> Jason ------------------------------------------------------------------------------ Site24x7 APM Insight: Get Deep Visibility into Application Performance APM + Mobile APM + RUM: Monitor 3 App instances at just $35/Month Monitor end-to-end web transactions and take corrective actions now Troubleshoot faster and improve end-user experience. Signup Now! http://pubads.g.doubleclick.net/gampad/clk?id=272487151&iu=/4140
On Mon, Feb 29, 2016 at 12:29:50PM -0500, Stefan Berger wrote: > Now that the tpm core has strong locking around 'ops' it is possible > to remove a TPM driver, module and all, even while user space still > has things like /dev/tpmX open. For consistency and simplicity, drop > the module locking entirely. I don't understand why the user visible behavior of /dev/tpmX should be changed if there are no life and death reasons to do it. Do you think that this patch is absolutely crucial for the feature implemented? /Jarkko > Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com> > --- > drivers/char/tpm/tpm-chip.c | 6 +----- > 1 file changed, 1 insertion(+), 5 deletions(-) > > diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c > index 1ae30f2..8f4b5f2 100644 > --- a/drivers/char/tpm/tpm-chip.c > +++ b/drivers/char/tpm/tpm-chip.c > @@ -57,9 +57,6 @@ int tpm_try_get_ops(struct tpm_chip *chip) > if (!chip->ops) > goto out_lock; > > - if (!try_module_get(chip->dev.parent->driver->owner)) > - goto out_lock; > - > return 0; > out_lock: > up_read(&chip->ops_sem); > @@ -77,7 +74,6 @@ EXPORT_SYMBOL_GPL(tpm_try_get_ops); > */ > void tpm_put_ops(struct tpm_chip *chip) > { > - module_put(chip->dev.parent->driver->owner); > up_read(&chip->ops_sem); > put_device(&chip->dev); > } > @@ -183,7 +179,7 @@ struct tpm_chip *tpmm_chip_alloc(struct device *dev, > goto out; > > cdev_init(&chip->cdev, &tpm_fops); > - chip->cdev.owner = dev->driver->owner; > + chip->cdev.owner = THIS_MODULE; > chip->cdev.kobj.parent = &chip->dev.kobj; > > rc = devm_add_action(dev, (void (*)(void *)) put_device, &chip->dev); > -- > 2.4.3 > ------------------------------------------------------------------------------ Site24x7 APM Insight: Get Deep Visibility into Application Performance APM + Mobile APM + RUM: Monitor 3 App instances at just $35/Month Monitor end-to-end web transactions and take corrective actions now Troubleshoot faster and improve end-user experience. Signup Now! http://pubads.g.doubleclick.net/gampad/clk?id=272487151&iu=/4140
Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote on 02/29/2016 03:35:50 PM: > On Mon, Feb 29, 2016 at 12:29:50PM -0500, Stefan Berger wrote: > > Now that the tpm core has strong locking around 'ops' it is possible > > to remove a TPM driver, module and all, even while user space still > > has things like /dev/tpmX open. For consistency and simplicity, drop > > the module locking entirely. > > I don't understand why the user visible behavior of /dev/tpmX should > be changed if there are no life and death reasons to do it. Do you > think that this patch is absolutely crucial for the feature > implemented? This changes the module counter on the hardware / backend driver and not other user visible behavior from what I can tell. It's certainly/hopefully not a life-and-death thing, though I am wondering whether it's be applied sooner or later anyway for cleanup reasons following the preceding patch providing 'strong locking'. Stefan > > /Jarkko > > > Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com> > > --- > > drivers/char/tpm/tpm-chip.c | 6 +----- > > 1 file changed, 1 insertion(+), 5 deletions(-) > > > > diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c > > index 1ae30f2..8f4b5f2 100644 > > --- a/drivers/char/tpm/tpm-chip.c > > +++ b/drivers/char/tpm/tpm-chip.c > > @@ -57,9 +57,6 @@ int tpm_try_get_ops(struct tpm_chip *chip) > > if (!chip->ops) > > goto out_lock; > > > > - if (!try_module_get(chip->dev.parent->driver->owner)) > > - goto out_lock; > > - > > return 0; > > out_lock: > > up_read(&chip->ops_sem); > > @@ -77,7 +74,6 @@ EXPORT_SYMBOL_GPL(tpm_try_get_ops); > > */ > > void tpm_put_ops(struct tpm_chip *chip) > > { > > - module_put(chip->dev.parent->driver->owner); > > up_read(&chip->ops_sem); > > put_device(&chip->dev); > > } > > @@ -183,7 +179,7 @@ struct tpm_chip *tpmm_chip_alloc(struct device *dev, > > goto out; > > > > cdev_init(&chip->cdev, &tpm_fops); > > - chip->cdev.owner = dev->driver->owner; > > + chip->cdev.owner = THIS_MODULE; > > chip->cdev.kobj.parent = &chip->dev.kobj; > > > > rc = devm_add_action(dev, (void (*)(void *)) put_device, &chip->dev); > > -- > > 2.4.3 > > > ------------------------------------------------------------------------------ Site24x7 APM Insight: Get Deep Visibility into Application Performance APM + Mobile APM + RUM: Monitor 3 App instances at just $35/Month Monitor end-to-end web transactions and take corrective actions now Troubleshoot faster and improve end-user experience. Signup Now! http://pubads.g.doubleclick.net/gampad/clk?id=272487151&iu=/4140
On Mon, Feb 29, 2016 at 10:35:50PM +0200, Jarkko Sakkinen wrote: > On Mon, Feb 29, 2016 at 12:29:50PM -0500, Stefan Berger wrote: > > Now that the tpm core has strong locking around 'ops' it is possible > > to remove a TPM driver, module and all, even while user space still > > has things like /dev/tpmX open. For consistency and simplicity, drop > > the module locking entirely. > > I don't understand why the user visible behavior of /dev/tpmX should > be changed if there are no life and death reasons to do it. Do you > think that this patch is absolutely crucial for the feature > implemented? Stefan is addressing it here because once NULL is a legal parent for registration the chip->dev.parent->driver->owner expression becomes useless for module locking. If we want to keep module locking then Stefen has to add an 'owner' field to tpm_class_ops and update all drivers. The ops->owner would be used for the module locking instead of driver->owner Jason ------------------------------------------------------------------------------ Site24x7 APM Insight: Get Deep Visibility into Application Performance APM + Mobile APM + RUM: Monitor 3 App instances at just $35/Month Monitor end-to-end web transactions and take corrective actions now Troubleshoot faster and improve end-user experience. Signup Now! http://pubads.g.doubleclick.net/gampad/clk?id=272487151&iu=/4140
On Mon, Feb 29, 2016 at 03:49:55PM -0500, Stefan Berger wrote: > Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote on 02/29/2016 > 03:35:50 PM: > > > On Mon, Feb 29, 2016 at 12:29:50PM -0500, Stefan Berger wrote: > > > Now that the tpm core has strong locking around 'ops' it is possible > > > to remove a TPM driver, module and all, even while user space still > > > has things like /dev/tpmX open. For consistency and simplicity, drop > > > the module locking entirely. > > > > I don't understand why the user visible behavior of /dev/tpmX should > > be changed if there are no life and death reasons to do it. Do you > > think that this patch is absolutely crucial for the feature > > implemented? > > This changes the module counter on the hardware / backend driver and not > other user visible behavior from what I can tell. It's certainly/hopefully > not a life-and-death thing, though I am wondering whether it's be applied > sooner or later anyway for cleanup reasons following the preceding patch > providing 'strong locking'. OK, I just your and Jasons reply and will head soon to apply the series to my master branch (being there still give space to move with locking if needed). /Jarkko ------------------------------------------------------------------------------ Site24x7 APM Insight: Get Deep Visibility into Application Performance APM + Mobile APM + RUM: Monitor 3 App instances at just $35/Month Monitor end-to-end web transactions and take corrective actions now Troubleshoot faster and improve end-user experience. Signup Now! http://pubads.g.doubleclick.net/gampad/clk?id=272487151&iu=/4140
diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c index 1ae30f2..8f4b5f2 100644 --- a/drivers/char/tpm/tpm-chip.c +++ b/drivers/char/tpm/tpm-chip.c @@ -57,9 +57,6 @@ int tpm_try_get_ops(struct tpm_chip *chip) if (!chip->ops) goto out_lock; - if (!try_module_get(chip->dev.parent->driver->owner)) - goto out_lock; - return 0; out_lock: up_read(&chip->ops_sem); @@ -77,7 +74,6 @@ EXPORT_SYMBOL_GPL(tpm_try_get_ops); */ void tpm_put_ops(struct tpm_chip *chip) { - module_put(chip->dev.parent->driver->owner); up_read(&chip->ops_sem); put_device(&chip->dev); } @@ -183,7 +179,7 @@ struct tpm_chip *tpmm_chip_alloc(struct device *dev, goto out; cdev_init(&chip->cdev, &tpm_fops); - chip->cdev.owner = dev->driver->owner; + chip->cdev.owner = THIS_MODULE; chip->cdev.kobj.parent = &chip->dev.kobj; rc = devm_add_action(dev, (void (*)(void *)) put_device, &chip->dev);
Now that the tpm core has strong locking around 'ops' it is possible to remove a TPM driver, module and all, even while user space still has things like /dev/tpmX open. For consistency and simplicity, drop the module locking entirely. Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com> --- drivers/char/tpm/tpm-chip.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-)