Dear all,
I am currently writing virtual TPM device driver. This driver exposes
itself and behaves like regular TPM device (i.e. uses TPM layer which is
already present in kernel), but instead of talking to hardware it talks
to user space.
In my setup, I can create several virtual devices and delete them in
runtime. During this I noticed that current implementation is prone to
segfault when tpm_remove_hardware is called while the device is in its
receiving routine.
The problem is in tpm_remove_hardware which does all necessary steps to
remove the device, but it does not wait for the device being actually
removed and kfrees tpm_chip struct right away. If the device is at this
time in its receiving routine (through read call, for example) it blocks
the device removal until the call is completed. The call cannot be
completed though because tpm_chip struct was already kfreed.
Since there is significant timeout and it is possible to meet that
timeout in normal operation this situation is quite easily reproducible.
What I present below is rather quickfix with least impact on other TPM
parts (drivers). The patch uses device->remove callback (of
platform_device device) and reroutes this to itself. In this
callback it eventually calls vendor callback and finally kfrees all
memory resources allocated on its own.
The correct solution for this I believe would require removing TPM
hardware (as it is done by tpm_remove_hardware right now) in two steps
both called by vendor driver. In the first one vendor driver will just
"unregister" the hardware. In the second step vendor driver will call
tpm_release_resources routine which kfrees all resources allocated by
tpm.c. In order to be able to call tpm_release_resources, vendor driver
will have to register as platform driver and handle
platform_driver.probe and platform_driver.remove.
This will however require some changes in all drivers, which I cannot
test (but I could eventually write the patch). Also I see some "issues"
in tpm_tis which will have to be clarified.
--
Richard
>From bd80b63ca2e1edb761a3ffcf87bd86c30a44ca5f Mon Sep 17 00:00:00 2001
From: Richard Musil <[email protected]>
Date: Tue, 21 Aug 2007 16:46:06 +0200
Subject: [PATCH] Change in TPM module:
The clean up procedure now uses platform device "release" callback to
handle memory clean up. For this purpose "release" function callback was
added to struct tpm_vendor_specific, so hw device driver provider can get
called when it is safe to remove all allocated resources.
This is supposed to fix a bug in device removal, where device while in
receive function (waiting on timeout) was prone to segfault, if the
tpm_chip struct was unallocated before the timeout expired (in
tpm_remove_hardware).
---
drivers/char/tpm/tpm.c | 46 +++++++++++++++++++++++++++++-----------------
drivers/char/tpm/tpm.h | 2 ++
2 files changed, 31 insertions(+), 17 deletions(-)
diff --git a/drivers/char/tpm/tpm.c b/drivers/char/tpm/tpm.c
index 9bb5429..41eba7e 100644
--- a/drivers/char/tpm/tpm.c
+++ b/drivers/char/tpm/tpm.c
@@ -1031,18 +1031,13 @@ void tpm_remove_hardware(struct device *dev)
spin_unlock(&driver_lock);
- dev_set_drvdata(dev, NULL);
misc_deregister(&chip->vendor.miscdev);
- kfree(chip->vendor.miscdev.name);
sysfs_remove_group(&dev->kobj, chip->vendor.attr_group);
tpm_bios_log_teardown(chip->bios_dir);
- clear_bit(chip->dev_num, dev_mask);
-
- kfree(chip);
-
- put_device(dev);
+ /* write it this way to be explicit (chip->dev == dev) */
+ put_device(chip->dev);
}
EXPORT_SYMBOL_GPL(tpm_remove_hardware);
@@ -1083,6 +1078,28 @@ int tpm_pm_resume(struct device *dev)
EXPORT_SYMBOL_GPL(tpm_pm_resume);
/*
+ * Once all references to platform device are down to 0,
+ * release all allocated structures.
+ * In case vendor provided release function,
+ * call it too.
+ */
+static void tpm_dev_release(struct device *dev)
+{
+ struct tpm_chip *chip = dev_get_drvdata(dev);
+ /* call vendor release, if defined */
+ if (chip->vendor.release)
+ chip->vendor.release(dev);
+
+ /* it *should* be: chip->release != NULL */
+ if (likely(chip->release))
+ chip->release(dev);
+
+ clear_bit(chip->dev_num, dev_mask);
+ kfree(chip->vendor.miscdev.name);
+ kfree(chip);
+}
+
+/*
* Called from tpm_<specific>.c probe function only for devices
* the driver has determined it should claim. Prior to calling
* this function the specific probe function has called pci_enable_device
@@ -1136,23 +1153,21 @@ struct tpm_chip *tpm_register_hardware(struct device *dev, const struct tpm_vend
chip->vendor.miscdev.parent = dev;
chip->dev = get_device(dev);
+ chip->release = dev->release;
+ dev->release = tpm_dev_release;
+ dev_set_drvdata(dev, chip);
if (misc_register(&chip->vendor.miscdev)) {
dev_err(chip->dev,
"unable to misc_register %s, minor %d\n",
chip->vendor.miscdev.name,
chip->vendor.miscdev.minor);
- put_device(dev);
- clear_bit(chip->dev_num, dev_mask);
- kfree(chip);
- kfree(devname);
+ put_device(chip->dev);
return NULL;
}
spin_lock(&driver_lock);
- dev_set_drvdata(dev, chip);
-
list_add(&chip->list, &tpm_chip_list);
spin_unlock(&driver_lock);
@@ -1160,10 +1175,7 @@ struct tpm_chip *tpm_register_hardware(struct device *dev, const struct tpm_vend
if (sysfs_create_group(&dev->kobj, chip->vendor.attr_group)) {
list_del(&chip->list);
misc_deregister(&chip->vendor.miscdev);
- put_device(dev);
- clear_bit(chip->dev_num, dev_mask);
- kfree(chip);
- kfree(devname);
+ put_device(chip->dev);
return NULL;
}
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index b2e2b00..f1c265e 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -74,6 +74,7 @@ struct tpm_vendor_specific {
int (*send) (struct tpm_chip *, u8 *, size_t);
void (*cancel) (struct tpm_chip *);
u8 (*status) (struct tpm_chip *);
+ void (*release) (struct device *);
struct miscdevice miscdev;
struct attribute_group *attr_group;
struct list_head list;
@@ -106,6 +107,7 @@ struct tpm_chip {
struct dentry **bios_dir;
struct list_head list;
+ void (*release) (struct device *);
};
#define to_tpm_chip(n) container_of(n, struct tpm_chip, vendor)
--
1.5.3.rc5
On Thu, Aug 23, 2007 at 10:46:55AM +0200, Richard MUSIL wrote:
> Dear all,
>
> I am currently writing virtual TPM device driver. This driver exposes
> itself and behaves like regular TPM device (i.e. uses TPM layer which is
> already present in kernel), but instead of talking to hardware it talks
> to user space.
Heh, I like the idea, I can imagine what it could be used for :)
> What I present below is rather quickfix with least impact on other TPM
> parts (drivers). The patch uses device->remove callback (of
> platform_device device) and reroutes this to itself. In this
> callback it eventually calls vendor callback and finally kfrees all
> memory resources allocated on its own.
It looks sane to me, nice fixup.
thanks,
greg k-h
Hello all,
sometime ago I submitted patch to TPM layer, originally I thought this
patch could be accepted into kernel (see below). However,
since this did not happen, I wonder, if there are some problems with the
patch or whether I am expected to do/provide something else, in order to
have it accepted.
The patch follows even more below.
Thanks,
Richard
Greg KH wrote:
> On Thu, Aug 23, 2007 at 10:46:55AM +0200, Richard MUSIL wrote:
>> Dear all,
>>
>> I am currently writing virtual TPM device driver. This driver exposes
>> itself and behaves like regular TPM device (i.e. uses TPM layer which is
>> already present in kernel), but instead of talking to hardware it talks
>> to user space.
>
> Heh, I like the idea, I can imagine what it could be used for :)
>
>> What I present below is rather quickfix with least impact on other TPM
>> parts (drivers). The patch uses device->remove callback (of
>> platform_device device) and reroutes this to itself. In this
>> callback it eventually calls vendor callback and finally kfrees all
>> memory resources allocated on its own.
>
> It looks sane to me, nice fixup.
>
> thanks,
>
> greg k-h
>
>From bd80b63ca2e1edb761a3ffcf87bd86c30a44ca5f Mon Sep 17 00:00:00 2001
From: Richard Musil <[email protected]>
Date: Tue, 21 Aug 2007 16:46:06 +0200
Subject: [PATCH] Change in TPM module:
The clean up procedure now uses platform device "release" callback to
handle memory clean up. For this purpose "release" function callback was
added to struct tpm_vendor_specific, so hw device driver provider can get
called when it is safe to remove all allocated resources.
This is supposed to fix a bug in device removal, where device while in
receive function (waiting on timeout) was prone to segfault, if the
tpm_chip struct was unallocated before the timeout expired (in
tpm_remove_hardware).
---
drivers/char/tpm/tpm.c | 46 +++++++++++++++++++++++++++++-----------------
drivers/char/tpm/tpm.h | 2 ++
2 files changed, 31 insertions(+), 17 deletions(-)
diff --git a/drivers/char/tpm/tpm.c b/drivers/char/tpm/tpm.c
index 9bb5429..41eba7e 100644
--- a/drivers/char/tpm/tpm.c
+++ b/drivers/char/tpm/tpm.c
@@ -1031,18 +1031,13 @@ void tpm_remove_hardware(struct device *dev)
spin_unlock(&driver_lock);
- dev_set_drvdata(dev, NULL);
misc_deregister(&chip->vendor.miscdev);
- kfree(chip->vendor.miscdev.name);
sysfs_remove_group(&dev->kobj, chip->vendor.attr_group);
tpm_bios_log_teardown(chip->bios_dir);
- clear_bit(chip->dev_num, dev_mask);
-
- kfree(chip);
-
- put_device(dev);
+ /* write it this way to be explicit (chip->dev == dev) */
+ put_device(chip->dev);
}
EXPORT_SYMBOL_GPL(tpm_remove_hardware);
@@ -1083,6 +1078,28 @@ int tpm_pm_resume(struct device *dev)
EXPORT_SYMBOL_GPL(tpm_pm_resume);
/*
+ * Once all references to platform device are down to 0,
+ * release all allocated structures.
+ * In case vendor provided release function,
+ * call it too.
+ */
+static void tpm_dev_release(struct device *dev)
+{
+ struct tpm_chip *chip = dev_get_drvdata(dev);
+ /* call vendor release, if defined */
+ if (chip->vendor.release)
+ chip->vendor.release(dev);
+
+ /* it *should* be: chip->release != NULL */
+ if (likely(chip->release))
+ chip->release(dev);
+
+ clear_bit(chip->dev_num, dev_mask);
+ kfree(chip->vendor.miscdev.name);
+ kfree(chip);
+}
+
+/*
* Called from tpm_<specific>.c probe function only for devices
* the driver has determined it should claim. Prior to calling
* this function the specific probe function has called pci_enable_device
@@ -1136,23 +1153,21 @@ struct tpm_chip *tpm_register_hardware(struct device *dev, const struct tpm_vend
chip->vendor.miscdev.parent = dev;
chip->dev = get_device(dev);
+ chip->release = dev->release;
+ dev->release = tpm_dev_release;
+ dev_set_drvdata(dev, chip);
if (misc_register(&chip->vendor.miscdev)) {
dev_err(chip->dev,
"unable to misc_register %s, minor %d\n",
chip->vendor.miscdev.name,
chip->vendor.miscdev.minor);
- put_device(dev);
- clear_bit(chip->dev_num, dev_mask);
- kfree(chip);
- kfree(devname);
+ put_device(chip->dev);
return NULL;
}
spin_lock(&driver_lock);
- dev_set_drvdata(dev, chip);
-
list_add(&chip->list, &tpm_chip_list);
spin_unlock(&driver_lock);
@@ -1160,10 +1175,7 @@ struct tpm_chip *tpm_register_hardware(struct device *dev, const struct tpm_vend
if (sysfs_create_group(&dev->kobj, chip->vendor.attr_group)) {
list_del(&chip->list);
misc_deregister(&chip->vendor.miscdev);
- put_device(dev);
- clear_bit(chip->dev_num, dev_mask);
- kfree(chip);
- kfree(devname);
+ put_device(chip->dev);
return NULL;
}
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index b2e2b00..f1c265e 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -74,6 +74,7 @@ struct tpm_vendor_specific {
int (*send) (struct tpm_chip *, u8 *, size_t);
void (*cancel) (struct tpm_chip *);
u8 (*status) (struct tpm_chip *);
+ void (*release) (struct device *);
struct miscdevice miscdev;
struct attribute_group *attr_group;
struct list_head list;
@@ -106,6 +107,7 @@ struct tpm_chip {
struct dentry **bios_dir;
struct list_head list;
+ void (*release) (struct device *);
};
#define to_tpm_chip(n) container_of(n, struct tpm_chip, vendor)
-- 1.5.3.rc5
On Tue, Sep 25, 2007 at 03:14:50PM +0200, Richard MUSIL wrote:
> Hello all,
>
> sometime ago I submitted patch to TPM layer, originally I thought this
> patch could be accepted into kernel (see below). However,
> since this did not happen, I wonder, if there are some problems with the
> patch or whether I am expected to do/provide something else, in order to
> have it accepted.
That would be up to the maintainer of the TPM code, which last I looked
was Kylene and Marcel (both of whom should be on the tpmdd-devel list
and should have picked this up already...)
thanks,
greg k-h
Dear Richard, dear Greg,
thanks for the patch, I will have a look at it next week and get back
to you as soon as I can acknowledge it.
Thanks,
Marcel
Greg KH schrieb:
> On Tue, Sep 25, 2007 at 03:14:50PM +0200, Richard MUSIL wrote:
>> Hello all,
>>
>> sometime ago I submitted patch to TPM layer, originally I thought this
>> patch could be accepted into kernel (see below). However,
>> since this did not happen, I wonder, if there are some problems with the
>> patch or whether I am expected to do/provide something else, in order to
>> have it accepted.
>
> That would be up to the maintainer of the TPM code, which last I looked
> was Kylene and Marcel (both of whom should be on the tpmdd-devel list
> and should have picked this up already...)
>
> thanks,
>
> greg k-h
>
> -------------------------------------------------------------------------
> This SF.net email is sponsored by: Microsoft
> Defy all challenges. Microsoft(R) Visual Studio 2005.
> http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
> _______________________________________________
> tpmdd-devel mailing list
> [email protected]
> https://lists.sourceforge.net/lists/listinfo/tpmdd-devel
>
Dear all,
sorry for the delay. The patch below was posted 3 months ago and fixes
possible problems during unloading of TPM device drivers.
Acked by: Marcel Selhorst <[email protected]>
---
Richard MUSIL wrote:
> Dear all,
>
> I am currently writing virtual TPM device driver. This driver exposes
> itself and behaves like regular TPM device (i.e. uses TPM layer which is
> already present in kernel), but instead of talking to hardware it talks
> to user space.
>
> In my setup, I can create several virtual devices and delete them in
> runtime. During this I noticed that current implementation is prone to
> segfault when tpm_remove_hardware is called while the device is in its
> receiving routine.
>
> The problem is in tpm_remove_hardware which does all necessary steps to
> remove the device, but it does not wait for the device being actually
> removed and kfrees tpm_chip struct right away. If the device is at this
> time in its receiving routine (through read call, for example) it blocks
> the device removal until the call is completed. The call cannot be
> completed though because tpm_chip struct was already kfreed.
>
> Since there is significant timeout and it is possible to meet that
> timeout in normal operation this situation is quite easily reproducible.
>
> What I present below is rather quickfix with least impact on other TPM
> parts (drivers). The patch uses device->remove callback (of
> platform_device device) and reroutes this to itself. In this
> callback it eventually calls vendor callback and finally kfrees all
> memory resources allocated on its own.
>
> The correct solution for this I believe would require removing TPM
> hardware (as it is done by tpm_remove_hardware right now) in two steps
> both called by vendor driver. In the first one vendor driver will just
> "unregister" the hardware. In the second step vendor driver will call
> tpm_release_resources routine which kfrees all resources allocated by
> tpm.c. In order to be able to call tpm_release_resources, vendor driver
> will have to register as platform driver and handle
> platform_driver.probe and platform_driver.remove.
>
> This will however require some changes in all drivers, which I cannot
> test (but I could eventually write the patch). Also I see some "issues"
> in tpm_tis which will have to be clarified.
>
> --
> Richard
>
>
>> >From bd80b63ca2e1edb761a3ffcf87bd86c30a44ca5f Mon Sep 17 00:00:00 2001
> From: Richard Musil <[email protected]>
> Date: Tue, 21 Aug 2007 16:46:06 +0200
> Subject: [PATCH] Change in TPM module:
>
> The clean up procedure now uses platform device "release" callback to
> handle memory clean up. For this purpose "release" function callback was
> added to struct tpm_vendor_specific, so hw device driver provider can get
> called when it is safe to remove all allocated resources.
>
> This is supposed to fix a bug in device removal, where device while in
> receive function (waiting on timeout) was prone to segfault, if the
> tpm_chip struct was unallocated before the timeout expired (in
> tpm_remove_hardware).
> ---
> drivers/char/tpm/tpm.c | 46 +++++++++++++++++++++++++++++-----------------
> drivers/char/tpm/tpm.h | 2 ++
> 2 files changed, 31 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/char/tpm/tpm.c b/drivers/char/tpm/tpm.c
> index 9bb5429..41eba7e 100644
> --- a/drivers/char/tpm/tpm.c
> +++ b/drivers/char/tpm/tpm.c
> @@ -1031,18 +1031,13 @@ void tpm_remove_hardware(struct device *dev)
>
> spin_unlock(&driver_lock);
>
> - dev_set_drvdata(dev, NULL);
> misc_deregister(&chip->vendor.miscdev);
> - kfree(chip->vendor.miscdev.name);
>
> sysfs_remove_group(&dev->kobj, chip->vendor.attr_group);
> tpm_bios_log_teardown(chip->bios_dir);
>
> - clear_bit(chip->dev_num, dev_mask);
> -
> - kfree(chip);
> -
> - put_device(dev);
> + /* write it this way to be explicit (chip->dev == dev) */
> + put_device(chip->dev);
> }
> EXPORT_SYMBOL_GPL(tpm_remove_hardware);
>
> @@ -1083,6 +1078,28 @@ int tpm_pm_resume(struct device *dev)
> EXPORT_SYMBOL_GPL(tpm_pm_resume);
>
> /*
> + * Once all references to platform device are down to 0,
> + * release all allocated structures.
> + * In case vendor provided release function,
> + * call it too.
> + */
> +static void tpm_dev_release(struct device *dev)
> +{
> + struct tpm_chip *chip = dev_get_drvdata(dev);
> + /* call vendor release, if defined */
> + if (chip->vendor.release)
> + chip->vendor.release(dev);
> +
> + /* it *should* be: chip->release != NULL */
> + if (likely(chip->release))
> + chip->release(dev);
> +
> + clear_bit(chip->dev_num, dev_mask);
> + kfree(chip->vendor.miscdev.name);
> + kfree(chip);
> +}
> +
> +/*
> * Called from tpm_<specific>.c probe function only for devices
> * the driver has determined it should claim. Prior to calling
> * this function the specific probe function has called pci_enable_device
> @@ -1136,23 +1153,21 @@ struct tpm_chip *tpm_register_hardware(struct device *dev, const struct tpm_vend
>
> chip->vendor.miscdev.parent = dev;
> chip->dev = get_device(dev);
> + chip->release = dev->release;
> + dev->release = tpm_dev_release;
> + dev_set_drvdata(dev, chip);
>
> if (misc_register(&chip->vendor.miscdev)) {
> dev_err(chip->dev,
> "unable to misc_register %s, minor %d\n",
> chip->vendor.miscdev.name,
> chip->vendor.miscdev.minor);
> - put_device(dev);
> - clear_bit(chip->dev_num, dev_mask);
> - kfree(chip);
> - kfree(devname);
> + put_device(chip->dev);
> return NULL;
> }
>
> spin_lock(&driver_lock);
>
> - dev_set_drvdata(dev, chip);
> -
> list_add(&chip->list, &tpm_chip_list);
>
> spin_unlock(&driver_lock);
> @@ -1160,10 +1175,7 @@ struct tpm_chip *tpm_register_hardware(struct device *dev, const struct tpm_vend
> if (sysfs_create_group(&dev->kobj, chip->vendor.attr_group)) {
> list_del(&chip->list);
> misc_deregister(&chip->vendor.miscdev);
> - put_device(dev);
> - clear_bit(chip->dev_num, dev_mask);
> - kfree(chip);
> - kfree(devname);
> + put_device(chip->dev);
> return NULL;
> }
>
> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> index b2e2b00..f1c265e 100644
> --- a/drivers/char/tpm/tpm.h
> +++ b/drivers/char/tpm/tpm.h
> @@ -74,6 +74,7 @@ struct tpm_vendor_specific {
> int (*send) (struct tpm_chip *, u8 *, size_t);
> void (*cancel) (struct tpm_chip *);
> u8 (*status) (struct tpm_chip *);
> + void (*release) (struct device *);
> struct miscdevice miscdev;
> struct attribute_group *attr_group;
> struct list_head list;
> @@ -106,6 +107,7 @@ struct tpm_chip {
> struct dentry **bios_dir;
>
> struct list_head list;
> + void (*release) (struct device *);
> };
>
> #define to_tpm_chip(n) container_of(n, struct tpm_chip, vendor)
On Mon, Nov 19, 2007 at 12:13:19AM +0100, Marcel Selhorst wrote:
> Dear all,
>
> sorry for the delay. The patch below was posted 3 months ago and fixes
> possible problems during unloading of TPM device drivers.
>
> Acked by: Marcel Selhorst <[email protected]>
Ok, but shouldn't you be sending this patch onward to Andrew and/or
others?
Who is the TPM driver maintainer these days?
thanks,
greg k-h
Hi Greg,
>> Acked by: Marcel Selhorst <[email protected]>
> Ok, but shouldn't you be sending this patch onward to Andrew and/or
> others?
You are right, I replied to all but he wasn't in CC... Sorry, I
forwarded the patch to him.
> Who is the TPM driver maintainer these days?
That would currently be me ;)
Best regards,
Marcel Selhorst
On Tue, 25 Sep 2007 15:14:50 +0200 Richard MUSIL <[email protected]> wrote:
> Hello all,
>
> sometime ago I submitted patch to TPM layer, originally I thought this
> patch could be accepted into kernel (see below). However,
> since this did not happen, I wonder, if there are some problems with the
> patch or whether I am expected to do/provide something else, in order to
> have it accepted.
>
> The patch follows even more below.
>
Thanks. We prefer that contributors sign off their work as per
Documentation/SubmittingPatches. Please review that and if agrereable,
send a Signed-off-by: for this work.
> /*
> + * Once all references to platform device are down to 0,
> + * release all allocated structures.
> + * In case vendor provided release function,
> + * call it too.
> + */
> +static void tpm_dev_release(struct device *dev)
> +{
> + struct tpm_chip *chip = dev_get_drvdata(dev);
> + /* call vendor release, if defined */
That's not the most useful of comments ;)
> + if (chip->vendor.release)
> + chip->vendor.release(dev);
> +
> + /* it *should* be: chip->release != NULL */
And that one's actually wrong in the context of kernel coding practices.
But whatever.
> + if (likely(chip->release))
> + chip->release(dev);
>From my reading, neither of these fields can ever be NULL, so the tests
simply aren't needed?
> + clear_bit(chip->dev_num, dev_mask);
> + kfree(chip->vendor.miscdev.name);
> + kfree(chip);
> +}
> +
> +/*
> * Called from tpm_<specific>.c probe function only for devices
> * the driver has determined it should claim. Prior to calling
> * this function the specific probe function has called pci_enable_device
> @@ -1136,23 +1153,21 @@ struct tpm_chip *tpm_register_hardware(struct device *dev, const struct tpm_vend
>
> chip->vendor.miscdev.parent = dev;
> chip->dev = get_device(dev);
> + chip->release = dev->release;
> + dev->release = tpm_dev_release;
> + dev_set_drvdata(dev, chip);
>
> if (misc_register(&chip->vendor.miscdev)) {
> dev_err(chip->dev,
> "unable to misc_register %s, minor %d\n",
> chip->vendor.miscdev.name,
> chip->vendor.miscdev.minor);
> - put_device(dev);
> - clear_bit(chip->dev_num, dev_mask);
> - kfree(chip);
> - kfree(devname);
> + put_device(chip->dev);
> return NULL;
> }
>
> spin_lock(&driver_lock);
>
> - dev_set_drvdata(dev, chip);
> -
> list_add(&chip->list, &tpm_chip_list);
>
> spin_unlock(&driver_lock);
> @@ -1160,10 +1175,7 @@ struct tpm_chip *tpm_register_hardware(struct device *dev, const struct tpm_vend
> if (sysfs_create_group(&dev->kobj, chip->vendor.attr_group)) {
> list_del(&chip->list);
> misc_deregister(&chip->vendor.miscdev);
> - put_device(dev);
> - clear_bit(chip->dev_num, dev_mask);
> - kfree(chip);
> - kfree(devname);
> + put_device(chip->dev);
> return NULL;
> }
>
> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> index b2e2b00..f1c265e 100644
> --- a/drivers/char/tpm/tpm.h
> +++ b/drivers/char/tpm/tpm.h
> @@ -74,6 +74,7 @@ struct tpm_vendor_specific {
> int (*send) (struct tpm_chip *, u8 *, size_t);
> void (*cancel) (struct tpm_chip *);
> u8 (*status) (struct tpm_chip *);
> + void (*release) (struct device *);
> struct miscdevice miscdev;
> struct attribute_group *attr_group;
> struct list_head list;
> @@ -106,6 +107,7 @@ struct tpm_chip {
> struct dentry **bios_dir;
>
> struct list_head list;
> + void (*release) (struct device *);
> };
>
> #define to_tpm_chip(n) container_of(n, struct tpm_chip, vendor)
Hello Andrew,
I am including 2nd version of the patch, slightly modified according
to your comments. See inline my response:
On 20.11.2007 7:37, Andrew Morton wrote:
> On Tue, 25 Sep 2007 15:14:50 +0200 Richard MUSIL <[email protected]> wrote:
>
>> The patch follows even more below.
>
> Thanks. We prefer that contributors sign off their work as per
> Documentation/SubmittingPatches. Please review that and if agrereable,
> send a Signed-off-by: for this work.
Ok, done in repost.
>> /*
>> + * Once all references to platform device are down to 0,
>> + * release all allocated structures.
>> + * In case vendor provided release function,
>> + * call it too.
>> + */
>> +static void tpm_dev_release(struct device *dev)
>> +{
>> + struct tpm_chip *chip = dev_get_drvdata(dev);
>> + /* call vendor release, if defined */
>
> That's not the most useful of comments ;)
Agreed and removed :).
>> + if (chip->vendor.release)
>> + chip->vendor.release(dev);
>> +
>> + /* it *should* be: chip->release != NULL */
>
> And that one's actually wrong in the context of kernel coding practices.
> But whatever.
Well I am not sure, what is exactly against coding practices (this is
my first patch, so bear with me). Was it the comment? Or the "likely".
But, anyway, I guess I was a bit paranoic. chip->release is set to
original device::release and this should be set to platform_device_release
at least (and if someone messed with it, it should not be NULL anyway).
So I removed complete condition.
>
>> + if (likely(chip->release))
>> + chip->release(dev);
>
>>From my reading, neither of these fields can ever be NULL, so the tests
> simply aren't needed?
The first condition is needed, since the vendor does not have to
define release function for its own purpose. In fact, for example
current TIS driver does not even know about it.
--
Richard
>From 070bd6e6753ae213f4ebe8367135cc3f4dfcb5ca Mon Sep 17 00:00:00 2001
From: Richard Musil <[email protected]>
Date: Tue, 20 Nov 2007 14:10:48 +0100
Subject: [PATCH] TPM: fix for segfault in device removal
The clean up procedure now uses platform device "release" callback to
handle memory clean up. For this purpose "release" function callback was
added to struct tpm_vendor_specific, so hw device driver provider can get
called when it is safe to remove all allocated resources.
This is supposed to fix a bug in device removal, where device while in
receive function (waiting on timeout) was prone to segfault, if the
tpm_chip struct was unallocated before the timeout expired (in
tpm_remove_hardware).
Signed-off-by: Richard Musil <[email protected]>
---
drivers/char/tpm/tpm.c | 42 +++++++++++++++++++++++++-----------------
drivers/char/tpm/tpm.h | 2 ++
2 files changed, 27 insertions(+), 17 deletions(-)
diff --git a/drivers/char/tpm/tpm.c b/drivers/char/tpm/tpm.c
index 9bb5429..59919b5 100644
--- a/drivers/char/tpm/tpm.c
+++ b/drivers/char/tpm/tpm.c
@@ -1031,18 +1031,13 @@ void tpm_remove_hardware(struct device *dev)
spin_unlock(&driver_lock);
- dev_set_drvdata(dev, NULL);
misc_deregister(&chip->vendor.miscdev);
- kfree(chip->vendor.miscdev.name);
sysfs_remove_group(&dev->kobj, chip->vendor.attr_group);
tpm_bios_log_teardown(chip->bios_dir);
- clear_bit(chip->dev_num, dev_mask);
-
- kfree(chip);
-
- put_device(dev);
+ /* write it this way to be explicit (chip->dev == dev) */
+ put_device(chip->dev);
}
EXPORT_SYMBOL_GPL(tpm_remove_hardware);
@@ -1083,6 +1078,24 @@ int tpm_pm_resume(struct device *dev)
EXPORT_SYMBOL_GPL(tpm_pm_resume);
/*
+ * Once all references to platform device are down to 0,
+ * release all allocated structures.
+ * In case vendor provided release function,
+ * call it too.
+ */
+static void tpm_dev_release(struct device *dev)
+{
+ struct tpm_chip *chip = dev_get_drvdata(dev);
+
+ if (chip->vendor.release)
+ chip->vendor.release(dev);
+
+ clear_bit(chip->dev_num, dev_mask);
+ kfree(chip->vendor.miscdev.name);
+ kfree(chip);
+}
+
+/*
* Called from tpm_<specific>.c probe function only for devices
* the driver has determined it should claim. Prior to calling
* this function the specific probe function has called pci_enable_device
@@ -1136,23 +1149,21 @@ struct tpm_chip *tpm_register_hardware(struct device *dev, const struct tpm_vend
chip->vendor.miscdev.parent = dev;
chip->dev = get_device(dev);
+ chip->release = dev->release;
+ dev->release = tpm_dev_release;
+ dev_set_drvdata(dev, chip);
if (misc_register(&chip->vendor.miscdev)) {
dev_err(chip->dev,
"unable to misc_register %s, minor %d\n",
chip->vendor.miscdev.name,
chip->vendor.miscdev.minor);
- put_device(dev);
- clear_bit(chip->dev_num, dev_mask);
- kfree(chip);
- kfree(devname);
+ put_device(chip->dev);
return NULL;
}
spin_lock(&driver_lock);
- dev_set_drvdata(dev, chip);
-
list_add(&chip->list, &tpm_chip_list);
spin_unlock(&driver_lock);
@@ -1160,10 +1171,7 @@ struct tpm_chip *tpm_register_hardware(struct device *dev, const struct tpm_vend
if (sysfs_create_group(&dev->kobj, chip->vendor.attr_group)) {
list_del(&chip->list);
misc_deregister(&chip->vendor.miscdev);
- put_device(dev);
- clear_bit(chip->dev_num, dev_mask);
- kfree(chip);
- kfree(devname);
+ put_device(chip->dev);
return NULL;
}
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index b2e2b00..f1c265e 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -74,6 +74,7 @@ struct tpm_vendor_specific {
int (*send) (struct tpm_chip *, u8 *, size_t);
void (*cancel) (struct tpm_chip *);
u8 (*status) (struct tpm_chip *);
+ void (*release) (struct device *);
struct miscdevice miscdev;
struct attribute_group *attr_group;
struct list_head list;
@@ -106,6 +107,7 @@ struct tpm_chip {
struct dentry **bios_dir;
struct list_head list;
+ void (*release) (struct device *);
};
#define to_tpm_chip(n) container_of(n, struct tpm_chip, vendor)
--
1.5.3.4
Gentlemen,
I am sorry for confusion, really do not have my day today :(.
In the last patch I mistakenly removed call to original release.
Now it should be OK.
Richard
>From 208991bcea7034202b9504c2e26c9b2edbf6e31d Mon Sep 17 00:00:00 2001
From: Richard Musil <[email protected]>
Date: Tue, 20 Nov 2007 14:47:27 +0100
Subject: [PATCH] Subject: [PATCH] TPM: fix for segfault in device removal
The clean up procedure now uses platform device "release" callback to
handle memory clean up. For this purpose "release" function callback was
added to struct tpm_vendor_specific, so hw device driver provider can get
called when it is safe to remove all allocated resources.
This is supposed to fix a bug in device removal, where device while in
receive function (waiting on timeout) was prone to segfault, if the
tpm_chip struct was unallocated before the timeout expired (in
tpm_remove_hardware).
Signed-off-by: Richard Musil <[email protected]>
---
drivers/char/tpm/tpm.c | 44 +++++++++++++++++++++++++++-----------------
drivers/char/tpm/tpm.h | 2 ++
2 files changed, 29 insertions(+), 17 deletions(-)
diff --git a/drivers/char/tpm/tpm.c b/drivers/char/tpm/tpm.c
index 9bb5429..ba48a11 100644
--- a/drivers/char/tpm/tpm.c
+++ b/drivers/char/tpm/tpm.c
@@ -1031,18 +1031,13 @@ void tpm_remove_hardware(struct device *dev)
spin_unlock(&driver_lock);
- dev_set_drvdata(dev, NULL);
misc_deregister(&chip->vendor.miscdev);
- kfree(chip->vendor.miscdev.name);
sysfs_remove_group(&dev->kobj, chip->vendor.attr_group);
tpm_bios_log_teardown(chip->bios_dir);
- clear_bit(chip->dev_num, dev_mask);
-
- kfree(chip);
-
- put_device(dev);
+ /* write it this way to be explicit (chip->dev == dev) */
+ put_device(chip->dev);
}
EXPORT_SYMBOL_GPL(tpm_remove_hardware);
@@ -1083,6 +1078,26 @@ int tpm_pm_resume(struct device *dev)
EXPORT_SYMBOL_GPL(tpm_pm_resume);
/*
+ * Once all references to platform device are down to 0,
+ * release all allocated structures.
+ * In case vendor provided release function,
+ * call it too.
+ */
+static void tpm_dev_release(struct device *dev)
+{
+ struct tpm_chip *chip = dev_get_drvdata(dev);
+
+ if (chip->vendor.release)
+ chip->vendor.release(dev);
+
+ chip->release(dev);
+
+ clear_bit(chip->dev_num, dev_mask);
+ kfree(chip->vendor.miscdev.name);
+ kfree(chip);
+}
+
+/*
* Called from tpm_<specific>.c probe function only for devices
* the driver has determined it should claim. Prior to calling
* this function the specific probe function has called pci_enable_device
@@ -1136,23 +1151,21 @@ struct tpm_chip *tpm_register_hardware(struct device *dev, const struct tpm_vend
chip->vendor.miscdev.parent = dev;
chip->dev = get_device(dev);
+ chip->release = dev->release;
+ dev->release = tpm_dev_release;
+ dev_set_drvdata(dev, chip);
if (misc_register(&chip->vendor.miscdev)) {
dev_err(chip->dev,
"unable to misc_register %s, minor %d\n",
chip->vendor.miscdev.name,
chip->vendor.miscdev.minor);
- put_device(dev);
- clear_bit(chip->dev_num, dev_mask);
- kfree(chip);
- kfree(devname);
+ put_device(chip->dev);
return NULL;
}
spin_lock(&driver_lock);
- dev_set_drvdata(dev, chip);
-
list_add(&chip->list, &tpm_chip_list);
spin_unlock(&driver_lock);
@@ -1160,10 +1173,7 @@ struct tpm_chip *tpm_register_hardware(struct device *dev, const struct tpm_vend
if (sysfs_create_group(&dev->kobj, chip->vendor.attr_group)) {
list_del(&chip->list);
misc_deregister(&chip->vendor.miscdev);
- put_device(dev);
- clear_bit(chip->dev_num, dev_mask);
- kfree(chip);
- kfree(devname);
+ put_device(chip->dev);
return NULL;
}
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index b2e2b00..f1c265e 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -74,6 +74,7 @@ struct tpm_vendor_specific {
int (*send) (struct tpm_chip *, u8 *, size_t);
void (*cancel) (struct tpm_chip *);
u8 (*status) (struct tpm_chip *);
+ void (*release) (struct device *);
struct miscdevice miscdev;
struct attribute_group *attr_group;
struct list_head list;
@@ -106,6 +107,7 @@ struct tpm_chip {
struct dentry **bios_dir;
struct list_head list;
+ void (*release) (struct device *);
};
#define to_tpm_chip(n) container_of(n, struct tpm_chip, vendor)
--
1.5.3.4
On Tue, 20 Nov 2007 13:32:06 +0100
Richard MUSIL <[email protected]> wrote:
> >> + if (chip->vendor.release)
> >> + chip->vendor.release(dev);
> >> +
> >> + /* it *should* be: chip->release != NULL */
> >
> > And that one's actually wrong in the context of kernel coding practices.
> > But whatever.
>
> Well I am not sure, what is exactly against coding practices (this is
> my first patch, so bear with me). Was it the comment? Or the "likely".
The code was
/* it *should* be: chip->release != NULL */
if (chip->release)
and the I took the comment to mean that it should be
if (chip->release != NULL)
I was just pointing out that the test-pointer-as-truth-value trick is
smiled upon in kernel coding.
> But, anyway, I guess I was a bit paranoic. chip->release is set to
> original device::release and this should be set to platform_device_release
> at least (and if someone messed with it, it should not be NULL anyway).
> So I removed complete condition.
>From the above it appears that the code comment misled me.