Re: tpm: Introduce TPM_CHIP_FLAG_VIRTUAL

Re: tpm: Introduce TPM_CHIP_FLAG_VIRTUAL

[Christophe Ricard]

[-- Attachment #1.1: Type: text/plain, Size: 1885 bytes --]

Hi Stefan,

[Adding tpmdd mailing list].
I agree that it would work when calling tpm_chip_alloc with NULL, my only
worry is that it is not chain the same way in other drivers.
Currently we are not using chip->dev.drvdata to store chip but platform
data.
chip is stored into the device platform data from the physical layer (e.g:
i2c, spi, pdev...).

At the end, I think this would not cause any issue as TPM_CHIP_FLAG_VIRTUAL
will covert it. However in the current form i believe
It looks like priv field will be required only for the purpose of
vtpm_proxy.

After previous discussion with Jarkko and Jason, priv is currently on its
way to get removed... https://patchwork.kernel.org/patch/8705291/

Would it be reasonable to use chip->dev.platform_data to store the newly
allocated proxy_dev instead of using a dedicated field (priv)?

Best Regards
Christophe


2016-03-31 13:43 GMT+02:00 Stefan Berger <stefanb-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>:

> On 03/30/2016 05:56 AM, Christophe Ricard wrote:
>
>> Hi Stefan,
>>
>> One question here:
>>   int tpm_sysfs_add_device(struct tpm_chip *chip)
>>   {
>> - int err;
>> - err = sysfs_create_group(&chip->dev.parent->kobj,
>> - &tpm_dev_group);
>> + int err = 0;
>> +
>> + if (!(chip->flags & TPM_CHIP_FLAG_VIRTUAL))
>> + err = sysfs_create_group(&chip->dev.parent->kobj,
>> + &tpm_dev_group);
>> + else {
>> + dev_set_drvdata(&chip->dev, chip);
>> What is the purpose of setting chip->dev.drvdata with chip ?
>> I don't see any place with a dev_get_drvdata. Can you explain ?
>>
>
> The subsequent patches implement a device driver that calls
> tpm_chip_alloc() with NULL for the driver parameter and dev_set_drvdata(),
> that typically gets called in tpmm_chip_alloc(), is not called. There are
> dev_get_drvdata() calls in tpm-sysfs.c that would not work without us
> calling dev_set_drvdata() here.
>
>    Stefan
>
>

[-- Attachment #1.2: Type: text/html, Size: 3346 bytes --]

[-- Attachment #2: Type: text/plain, Size: 291 bytes --]

------------------------------------------------------------------------------
Transform Data into Opportunity.
Accelerate data analysis in your applications with
Intel Data Analytics Acceleration Library.
Click to learn more.
http://pubads.g.doubleclick.net/gampad/clk?id=278785471&iu=/4140

[-- Attachment #3: Type: text/plain, Size: 192 bytes --]

_______________________________________________
tpmdd-devel mailing list
tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

Re: tpm: Introduce TPM_CHIP_FLAG_VIRTUAL

[Stefan Berger]

[-- Attachment #1.1: Type: text/plain, Size: 2404 bytes --]

On 03/31/2016 09:06 AM, Christophe Ricard wrote:
> Hi Stefan,
>
> [Adding tpmdd mailing list].
> I agree that it would work when calling tpm_chip_alloc with NULL, my 
> only worry is that it is not chain the same way in other drivers.
> Currently we are not using chip->dev.drvdata to store chip but 
> platform data.
> chip is stored into the device platform data from the physical layer 
> (e.g: i2c, spi, pdev...).

Yes, aware of that.

>
> At the end, I think this would not cause any issue as 
> TPM_CHIP_FLAG_VIRTUAL will covert it. However in the current form i 
> believe
> It looks like priv field will be required only for the purpose of 
> vtpm_proxy.

Yes. And a priv field could be introduced in the chip structure to be 
used by the tpm_vtpm_proxy driver.

>
> After previous discussion with Jarkko and Jason, priv is currently on 
> its way to get removed... https://patchwork.kernel.org/patch/8705291/

Seen it.
>
> Would it be reasonable to use chip->dev.platform_data to store the 
> newly allocated proxy_dev instead of using a dedicated field (priv)?

That's a possibility we could look into.

       Stefan


>
> Best Regards
> Christophe
>
>
> 2016-03-31 13:43 GMT+02:00 Stefan Berger <stefanb-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org 
> <mailto:stefanb-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>>:
>
>     On 03/30/2016 05:56 AM, Christophe Ricard wrote:
>
>         Hi Stefan,
>
>         One question here:
>           int tpm_sysfs_add_device(struct tpm_chip *chip)
>           {
>         - int err;
>         - err = sysfs_create_group(&chip->dev.parent->kobj,
>         - &tpm_dev_group);
>         + int err = 0;
>         +
>         + if (!(chip->flags & TPM_CHIP_FLAG_VIRTUAL))
>         + err = sysfs_create_group(&chip->dev.parent->kobj,
>         + &tpm_dev_group);
>         + else {
>         + dev_set_drvdata(&chip->dev, chip);
>         What is the purpose of setting chip->dev.drvdata with chip ?
>         I don't see any place with a dev_get_drvdata. Can you explain ?
>
>
>     The subsequent patches implement a device driver that calls
>     tpm_chip_alloc() with NULL for the driver parameter and
>     dev_set_drvdata(), that typically gets called in
>     tpmm_chip_alloc(), is not called. There are dev_get_drvdata()
>     calls in tpm-sysfs.c that would not work without us calling
>     dev_set_drvdata() here.
>
>        Stefan
>
>


[-- Attachment #1.2: Type: text/html, Size: 5847 bytes --]

[-- Attachment #2: Type: text/plain, Size: 291 bytes --]

------------------------------------------------------------------------------
Transform Data into Opportunity.
Accelerate data analysis in your applications with
Intel Data Analytics Acceleration Library.
Click to learn more.
http://pubads.g.doubleclick.net/gampad/clk?id=278785471&iu=/4140

[-- Attachment #3: Type: text/plain, Size: 192 bytes --]

_______________________________________________
tpmdd-devel mailing list
tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel