Re: [RFC PATCH 3/4] Implement driver for supporting multiple emulated TPMs

[Jarkko Sakkinen <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>]

On Mon, Jan 25, 2016 at 08:19:19PM -0700, Jason Gunthorpe wrote:
> On Mon, Jan 25, 2016 at 05:46:52PM -0800, Jarkko Sakkinen wrote:
> > > The only issue is error unwind, the tpmm version assumes devm works
> > > for that, but the vtpm_dev will not be destroyed if the tpm attach
> > > fails, do devm is not appropriate.
> > 
> > At the moment tpmm_chip_alloc() does not use devm for anything. Are you
> > speaking about stuff that happens between alloc and register as some of
> > the drivers use devm to associate resources to pdev at that point?
> 
> Ugh, well I missed that when you made those patches.
> 
> You need to put the devm back ASAP, as it is all the drivers now have
> broken error handling. eg
> 
> static int tpm_tis_init(struct device *dev, struct tpm_info *tpm_info,
>                         acpi_handle acpi_dev_handle)
> [..]
>         chip = tpmm_chip_alloc(dev, &tpm_tis);
> [..]
>         if (IS_ERR(chip))
> 	        return PTR_ERR(chip);
> 			
> Obviously leaks chip.

Could you describe how it leaks a chip and what do you mean by verb
"leak"?

> The function is called tpmm_ specifically because it is supposed to
> use devm resource unwind, it was a big mistake to remove the devm and
> not rename the function or update the comments.

The function does not reserve any resources using devm. Should it?

> With the change to use a struct device the devm action should have
> been a device_put to pair with the device_initialize.

Well, I found a regression that is luckily a rare one.

It does not happen when tpm_chip_register() is called because then it
will be paired with tpm_chip_unregister(), which calls
device_unregister().

I injected error to tpm_crb initialization in-between alloc and
register and release-callback was never called. When I did insmod
with a working driver it got device name /dev/tpm1.

So now I do have regression that does show up. If you don't have it,
it does not make sense to "fix" the code even if it looks obviously
wrong.

I have to ask one question. In earlier response to you stated that one
should get rid of devm? How'd you fix this problem without devm and why
you want to get rid of it in this scenario?

Thanks.

/Jarkko

------------------------------------------------------------------------------
Site24x7 APM Insight: Get Deep Visibility into Application Performance
APM + Mobile APM + RUM: Monitor 3 App instances at just $35/Month
Monitor end-to-end web transactions and take corrective actions now
Troubleshoot faster and improve end-user experience. Signup Now!
http://pubads.g.doubleclick.net/gampad/clk?id=267308311&iu=/4140