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

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

Hi

Apologies if I'm asking things that have popped out already in this
discussion (and also being passive so far!). I'm still in the process of
reading the discussion about [3/4] of the patch set that Stefan sent.

I should have done that and read the source code properly before sending
any responses. Sorry for being unforgivable sloppy with this.

> > Thanks for all feedback. I will eventually post v2. For now it's to be found
> > here:
> > 
> > https://github.com/stefanberger/linux
> > 
> > The branch is easy to find.
> 
> That looks much nicer, yes.
> 
> I'd also merge the tpm-vtpm.h into the .c file
> 
> However, I'm still not sure this is right:
> 
> +	vtpm_dev->chip = tpmm_chip_alloc(&vtpm_dev->dev, &vtpm_tpm_ops);
> 
> 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?

What do you mean by "tpm attach" here? Are you talking about the ioctl
for the vtpmx device that creates the device pair? Doesn't the fallback
path call destroy_device()?

I'm not sure which devm associated resources you are talking about.

> As I said before the tpmm stuff was a hack I did to make it easier to
> migrate the large number of drivers, core code should not be relying
> on devm like that... One good option here would be unwind that a bit
> and create a tpm_chip_alloc/tpm_chip_register flow that did not use
> devm at all. That could be fairly straight forward..

AFAIK I implemented two phase initialization or are you talking about
something else?

> Also, what is up with the _vtpm prefix on some functions? That doesn't
> seem aligned with the kernel style..
> 
> And confused why there is a miscdev and a alloc_chrdev_region ?
> 
> Jason

/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