| Message ID | 20171214160614.11808-2-Alexander.Steffen@infineon.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On Thu, Dec 14, 2017 at 05:06:12PM +0100, Alexander Steffen wrote: > According to the comments, adding/removing the chip from the list should be > the first/last action in (un)register. But currently it is done in a > subfunction in the middle of the process. Moving the code from the > subfunctions to the appropriate places within (un)register ensures that the > code matches the comments. The code is in the proper place. The visibility of the IDR and the cdev are linked together, they should be in the same place, side by side. It doesn't make sense to dis-associate them in the code... If you have a problem with comments they should be revised instead. Jason
> On Thu, Dec 14, 2017 at 05:06:12PM +0100, Alexander Steffen wrote: > > According to the comments, adding/removing the chip from the list should > be > > the first/last action in (un)register. But currently it is done in a > > subfunction in the middle of the process. Moving the code from the > > subfunctions to the appropriate places within (un)register ensures that the > > code matches the comments. > > The code is in the proper place. > > The visibility of the IDR and the cdev are linked together, they > should be in the same place, side by side. Could you explain why that is? My understanding is that the cdev is for the user space to talk to the TPM, the IDR is for the kernel to talk to the TPM. They share the underlying device, but should otherwise be completely independent. You could (in theory) have the user space interface without the kernel interface or vice versa. What am I missing? > It doesn't make sense to dis-associate them in the code... If you have > a problem with comments they should be revised instead. > > Jason
On Thu, Dec 14, 2017 at 05:06:12PM +0100, Alexander Steffen wrote: > According to the comments, adding/removing the chip from the list should be > the first/last action in (un)register. But currently it is done in a > subfunction in the middle of the process. Moving the code from the > subfunctions to the appropriate places within (un)register ensures that the > code matches the comments. > > Signed-off-by: Alexander Steffen <Alexander.Steffen@infineon.com> What is the problem you are having that this commit tries to address. Rather would not make changes for cosmetic reasons to init code. /Jarkko
On Thu, Dec 14, 2017 at 05:06:12PM +0100, Alexander Steffen wrote: > According to the comments, adding/removing the chip from the list should be > the first/last action in (un)register. But currently it is done in a > subfunction in the middle of the process. Moving the code from the > subfunctions to the appropriate places within (un)register ensures that the > code matches the comments. > > Signed-off-by: Alexander Steffen <Alexander.Steffen@infineon.com> NAK Not compatible with init_module(). http://man7.org/linux/man-pages/man2/init_module.2.html /Jarkko
diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c index a114e8f..25ebe49 100644 --- a/drivers/char/tpm/tpm-chip.c +++ b/drivers/char/tpm/tpm-chip.c @@ -321,11 +321,6 @@ static int tpm_add_char_device(struct tpm_chip *chip) } } - /* Make the chip available. */ - mutex_lock(&idr_lock); - idr_replace(&dev_nums_idr, chip, chip->dev_num); - mutex_unlock(&idr_lock); - return rc; } @@ -333,11 +328,6 @@ static void tpm_del_char_device(struct tpm_chip *chip) { cdev_device_del(&chip->cdev, &chip->dev); - /* Make the chip unavailable. */ - mutex_lock(&idr_lock); - idr_replace(&dev_nums_idr, NULL, chip->dev_num); - mutex_unlock(&idr_lock); - /* Make the driver uncallable. */ down_write(&chip->ops_sem); if (chip->flags & TPM_CHIP_FLAG_TPM2) @@ -432,6 +422,11 @@ int tpm_chip_register(struct tpm_chip *chip) return rc; } + /* Make the chip available. */ + mutex_lock(&idr_lock); + idr_replace(&dev_nums_idr, chip, chip->dev_num); + mutex_unlock(&idr_lock); + return 0; } EXPORT_SYMBOL_GPL(tpm_chip_register); @@ -451,6 +446,11 @@ EXPORT_SYMBOL_GPL(tpm_chip_register); */ void tpm_chip_unregister(struct tpm_chip *chip) { + /* Make the chip unavailable. */ + mutex_lock(&idr_lock); + idr_replace(&dev_nums_idr, NULL, chip->dev_num); + mutex_unlock(&idr_lock); + tpm_del_legacy_sysfs(chip); tpm_bios_log_teardown(chip); if (chip->flags & TPM_CHIP_FLAG_TPM2)
According to the comments, adding/removing the chip from the list should be the first/last action in (un)register. But currently it is done in a subfunction in the middle of the process. Moving the code from the subfunctions to the appropriate places within (un)register ensures that the code matches the comments. Signed-off-by: Alexander Steffen <Alexander.Steffen@infineon.com> --- drivers/char/tpm/tpm-chip.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-)