On Fri, Feb 05, 2021 at 05:08:20PM -0800, James Bottomley wrote:
 

Effectively all of this shuffles the tpmrm device allocation from
chip_alloc to chip_add ... I'm not averse to this but it does mean we
can suffer allocation failures now in the add routine and it makes
error handling a bit more complex.  

We already have to handle failures here, so this doesn't seem any
worse (and the existing error handling looked wrong, I fixed it)

quoted

 		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-

quoted

devs.devt),

 				MINOR(chip->devs.devt), rc);
-			return rc;
+			goto out_put_devs;
 		}
 	}
 

@@ -460,6 +459,10 @@ static int tpm_add_char_device(struct tpm_chip

*chip)
 	idr_replace(&dev_nums_idr, chip, chip->dev_num);
 	mutex_unlock(&idr_lock);
 
+out_put_devs:
+	put_device(&chip->devs);

I think there should be a if (chip->flags & TPM_CHIP_FLAG_TPM2) here.

I realise you got everything semantically correct and you only ever go
to this label from somewhere that already has the check, but guess what
will happen when the bot rewriters get hold of this ...

Makes sense
 

quoted

+out_del_dev:
+	cdev_device_del(&chip->cdev);
 	return rc;
 }
 

@@ -640,8 +643,10 @@ void tpm_chip_unregister(struct tpm_chip *chip)
 	if (IS_ENABLED(CONFIG_HW_RANDOM_TPM))
 		hwrng_unregister(&chip->hwrng);
 	tpm_bios_log_teardown(chip);
-	if (chip->flags & TPM_CHIP_FLAG_TPM2)
+	if (chip->flags & TPM_CHIP_FLAG_TPM2) {
 		cdev_device_del(&chip->cdevs, &chip->devs);
+		put_device(&chip->devs);
+	}
 	tpm_del_char_device(chip);

Actually, I think you want to go further here.  If there's a 

put_device(&chips->dev)

as the last statement (or moved into tpm_del_char_device) we should
now

The proper TPM driver remove sequence is:

remove()
{
   /* Upon return the core guarentees no driver callback is running or
    * will ever run again */
   tpm_chip_unregister()

   // Safe to do this because nothing will now use the HW resources
   free_irq(chip->XXX)
   unmap_memory(chip->YYY)

   // Now we are done with the memory
   put_device(&chip-dev);
}

ie the general driver design should expect the chip memory to continue
to exist after unregister because it will need to refer to it to
destroy any driver resources.

have no active reference on the devices from the kernel and we can
eliminate the 

	rc = devm_add_action_or_reset(pdev,
				      (void (*)(void *)) put_device,
				      &chip->dev);

This devm exists because adding the put_device to the error unwinds of
every driver probe function was too daunting. It can be removed only
if someone goes and updates every driver to correctly error-unwind
tpm_chip_alloc() with put_device() in the driver probe function.

Jason