On Thu, 2017-05-04 at 19:28 -0700, Linus Torvalds wrote:
> On Thu, May 4, 2017 at 5:18 PM, Greg KH <gre...@linuxfoundation.org>
> wrote:
> >
> > Here is the big set of new char/misc driver drivers and features
> > for 4.12-rc1.
>
> Ugh. I'm not particularly happy with the conflicts I got and my
> resolutions there-of.
Yes, we really should have done this via a postmerge tree. We've had
so little cause to use them recently, I suspect everyone's forgotten
how.
> I did resolve them - in the case of drivers/scsi/osd/osd_uld.c as per
> James' suggestion from his SCSI pull. Thanks.
>
> But even that resolution I'm not entirely happy with: somebody should
> check that it cleans up oud properly
It looks fine to me, but I'll ask the osd people (well person, since
it's very little used).
> But the one I'm really unhappy with is my tpm-chip.c resolution.
>
> In particular, commit 8dbbf5825181 ("tpm-chip: utilize new
> cdev_device_add helper function") made the tpm-chip code use that
> cdev_device_add/del pattern for chip->[c]dev. Fine.
>
> But then commit fdc915f7f719 ("tpm: expose spaces via a device link
> /dev/tpmrm<n>") added the *exact* same old pattern to a new
> "chip->[c]devs" (note the extra 's') and did so in a particularly
> ugly
> way too.
>
> James, why did you do that nasty
>
> if (chip->flags & TPM_CHIP_FLAG_TPM2)
>
> *twice*, instead of just doing things properly inside *one* test?
>
> Anyway, my merge resolution tries to just apply the same
> cdev_device_add/del pattern to the new chip->[c]devs entries, because
> not doing so seemed criminally ugly.
>
> It compiles. It looks better than the mess it was. But it may not
> work.
>
> James, Jarkko, you need to look at that tpm merge of mine. And James,
> double-check my osd_uld thing too.
I'm not going to defend the earlier coding, but you've lost the real
device_add() calls in the merge, meaning the tpm devices don't actually
get made visible at all. I suspect assuming device_add() is done by
cdev_device_add() because of the name is going to be our next anti
-pattern, so you're at least ahead of the game ...
I think the below is the correct fix, but we should really take it
through the TPM list (or at least give me time for some runtime
testing) because I suddenly think the error leg unwinding isn't correct
in the original, so I've redone that too.
James
---
diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index 322b8a51ffc6..867cef5ca9e5 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -272,24 +272,30 @@ EXPORT_SYMBOL_GPL(tpmm_chip_alloc);
static int tpm_add_char_device(struct tpm_chip *chip)
{
int rc;
+ const char *errstr;
+ struct device *errdev = &chip->dev;
rc = cdev_device_add(&chip->cdev, &chip->dev);
if (rc) {
- dev_err(&chip->dev,
- "unable to cdev_device_add() %s, major %d, minor %d,
err=%d\n",
- dev_name(&chip->dev), MAJOR(chip->dev.devt),
- MINOR(chip->dev.devt), rc);
- return rc;
+ errstr = "cdev_device_add for main device";
+ goto error1;
+ }
+ rc = device_add(&chip->dev);
+ if (rc) {
+ errstr = "device_add for main device";
+ goto error2;
}
-
if (chip->flags & TPM_CHIP_FLAG_TPM2) {
+ errdev = &chip->devs;
rc = cdev_device_add(&chip->cdevs, &chip->devs);
if (rc) {
- dev_err(&chip->devs,
- "unable to cdev_device_add() %s, major %d,
minor %d, err=%d\n",
- dev_name(&chip->devs), MAJOR(chip->devs.devt),
- MINOR(chip->devs.devt), rc);
- return rc;
+ errstr = "cdev_device_add for rm device";
+ goto error3;
+ }
+ rc = device_add(&chip->devs);
+ if (rc) {
+ errstr = "device_add for rm device";
+ goto error4;
}
}
@@ -299,6 +305,19 @@ static int tpm_add_char_device(struct tpm_chip *chip)
mutex_unlock(&idr_lock);
return rc;
+
+ error4:
+ cdev_del(&chip->cdevs);
+ error3:
+ device_del(&chip->dev);
+ error2:
+ cdev_del(&chip->cdev);
+ error1:
+ dev_err(&chip->dev,
+ "Failed %s %s, major %d, minor %d, err=%d\n",
+ errstr, dev_name(errdev), MAJOR(errdev->devt),
+ MINOR(errdev->devt), rc);
+ return rc;
}
static void tpm_del_char_device(struct tpm_chip *chip)
