Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org> wrote on 01/25/2016 10:46:32 PM:

>
> On Mon, Jan 25, 2016 at 08:05:14PM -0500, Stefan Berger wrote:
>
> > > And confused why there is a miscdev and a alloc_chrdev_region ?
> >
> > miscdev = /dev/vtpmx - that should be ok, no ?
>
> Yes
>
> > So, I just removed alloc_chrdev_region and with that the assignment of a
> > major+minor to the virtual device. This works fine on device creation but on
> > device destruction something odd happens in sysfs.
>
> > With two 'vtpmctrl' test programs running sysfs looks like this:
> >
> > # find /sys/devices/virtual/vtpm/
> > /sys/devices/virtual/vtpm/
> > /sys/devices/virtual/vtpm/vtpms0
> > /sys/devices/virtual/vtpm/vtpms0/dev
>
> Ugh.
>
> So, in an ideal world vtpms0 wouldn't even exist. The need for it is
> more tpm core breakage. You could avoid this by having the tpm core
> not create sysfs files on the parent for the vtpms. I think we are
> very close to being able to do that.

We're not explicitly registering the parent with sysfs, yet it appears there. How do you prevent it from appearing there? The only other solution I could think of is being able to pass a NULL for 'dev' into the existing tpmm_chip_alloc() and avoid the parent device altogether.

>
> The it would just be /sys/devices/virtual/tpm0/dev
>
> If vtpms0 is kept, then it shouldn't have a 'dev':

Now it doesn't have it anymore.

>
> > /sys/devices/virtual/vtpm/vtpms0/dev
>
> The plus side is that it should be able to act as the devm container
> for the tpmm_ function.
>
> This should be the only dev:
>
> > /sys/devices/virtual/vtpm/vtpms0/tpm0/dev
>
> That is a big clue something is very wrong if the above exists but
> the alloc_chrdev_region was removed.
>
> > That's all that is left now. .../vtpms1/tpm1/ has also already disappeared.
> > Needless to say, the kernel is very unhappy once the other
> vtpmctrl terminates.
> >
> > The virtual device needs a major/minor as well. Are there any other possible
> > solutions?
>
> This bad behavior is some secondary bug.. Just looking around I think
> all the lifetime stuff needs a good go over. I noticed at least...
>
> This looks wrong:
>
> +static int vtpm_fops_release(struct inode *inode, struct file *filp)
>
> +    put_device(&vtpm_dev->dev);
> +
> +   _vtpm_delete_device_pair(vtpm_dev);
>
> put_device should be last because it can kfree(vtpm_dev)

Yes. Fixed.

>
> This looks questionable too:
>
> +   device_destroy(vtpm_class, vtpm_dev->dev.devt);
>
> The driver never calls device_create, it shouldn't call
> device_destroy.

My bad! This now has to be device_del() + put_device() and then it works.

>
> The device_initialize transfers kref ownership to the caller, so
> vtpm_dev->dev needs a matching device_put on all error paths, not
> device_destroy
>
> IMHO, the put_device in the fops->release should be the match to
> device_initialize, and it should be the last thing. Audit eveything
> else to make that true
>
> This looks wrong too:
>
> +    /* device is needed for core TPM driver */
> +    device_initialize(&vtpm_dev->dev);
> +    vtpm_dev->dev.devt = devt;
> +    vtpm_dev->dev.class = vtpm_class;
> +    vtpm_dev->dev.release = vtpm_dev_release;
>
> I usualle like to put device_initialize last in that sequence, drop
> the devt stuff so you don't get a 'dev'

I did that in the same order as what 'device_create' ends up doing :

1697        device_initialize(dev);
1698        dev->devt = devt;
1699        dev->class = class;
1700        dev->parent= parent;
1701        dev->groups = groups;
1702        dev->release= device_create_release;
1703        dev_set_drvdata(dev, drvdata);

http://lxr.free-electrons.com/source/drivers/base/core.c#L1680

Usage of 'devt' is gone.

I put the fixed version of the driver here:

https://github.com/stefanberger/linux/commits/vtpm-driver.v2


Thanks for the review.

   Stefan

>
> The RCU stuff looks off too - where is the synchronize_rcu ?
>
> Jason
>