Path: csiph.com!aioe.org!bofh.it!news.nic.it!robomod From: James Bottomley Newsgroups: linux.kernel Subject: Re: [GIT PULL] Char/Misc driver patches for 4.12-rc1 Date: Fri, 05 May 2017 18:10:01 +0200 Message-ID: References: X-Original-To: Linus Torvalds , Greg KH , Jarkko Sakkinen Dkim-Signature: v=1; a=rsa-sha256; c=simple/simple; d=hansenpartnership.com; s=20151216; t=1494000011; bh=iqkyEG8aDB56Lqh4niJDPpVAmEQ6ai6Pv8q8IhG4Kx0=; h=Subject:From:To:Cc:Date:In-Reply-To:References:From; b=XCnB8ZGU6a6ay1en7xNWTgVbKUi1G706uQl5npeXWzEz7zAz2LBxl9eb10XIgF0gD Hv8Fj1URpOfprwZdBMo2rTbBry/iYtx31hERx0bIc7fWTy76sD0WE3xacmOrtlp+Zy PQ8hhJ9H7+5grekvXoPD13GIyTDRk0raXERrtAio= Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.16.5 MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Sender: robomod@news.nic.it List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Approved: robomod@news.nic.it Lines: 132 Organization: linux.* mail to news gateway X-Original-Cc: Andrew Morton , Arnd Bergmann , Linux Kernel Mailing List X-Original-Date: Fri, 05 May 2017 09:00:06 -0700 X-Original-Message-ID: <1494000006.2399.7.camel@HansenPartnership.com> X-Original-References: <20170505001808.GA16769@kroah.com> X-Original-Sender: linux-kernel-owner@vger.kernel.org Xref: csiph.com linux.kernel:1636471 On Thu, 2017-05-04 at 19:28 -0700, Linus Torvalds wrote: > On Thu, May 4, 2017 at 5:18 PM, Greg KH > 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") 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)