Re: [PATCH v4 4/6] tpm: modify tpm_pcr_read() definition to pass a TPM hash algorithm

[Peter Huewe]

Am 8. November 2018 16:15:04 MEZ schrieb Jarkko Sakkinen 
:
>On Thu, Nov 08, 2018 at 03:16:03PM +0100, Roberto Sassu wrote:
>> On 11/8/2018 3:04 PM, Jarkko Sakkinen wrote:
>> > On Tue, Nov 06, 2018 at 04:01:57PM +0100, Roberto Sassu wrote:
>> > > Currently the TPM driver allows other kernel subsystems to read
>only the
>> > > SHA1 PCR bank. This patch modifies the parameters of
>tpm_pcr_read() and
>> > > tpm2_pcr_read() to pass a tpm_digest structure, which contains
>the desired
>> > > hash algorithm. Also, since commit 125a22105410 ("tpm: React
>correctly to
>> > > RC_TESTING from TPM 2.0 self tests") removed the call to
>tpm2_pcr_read(),
>> > > the new parameter is expected to be always not NULL.
>> > > 
>> > > Due to the API change, IMA functions have been modified.
>> > > 
>> > > Signed-off-by: Roberto Sassu 
>> > > Acked-by: Mimi Zohar 
>> > 
>> > Does not apply to the current upstream (with tpm1-cmd.c).
>> 
>> Unfortunately, I cannot fetch the repository as infradead.org only
>> supports the git protocol (I'm behind a proxy).
>> 
>> Roberto
>
>I use a proxy script similar to this:
>
>https://gist.github.com/sit/49288
>
>(random googling but gives the idea)
>
>/Jarkko
Moving to a kernel.org repo would be really a benefit or convincing them to 
have a https interface as well.
We have the same proxy issue with infradead.
Peter
-- 
Sent from my mobile

Re: [PATCH v4 4/6] tpm: modify tpm_pcr_read() definition to pass a TPM hash algorithm

[Peter Huewe]

Am 8. November 2018 16:15:04 MEZ schrieb Jarkko Sakkinen 
:
>On Thu, Nov 08, 2018 at 03:16:03PM +0100, Roberto Sassu wrote:
>> On 11/8/2018 3:04 PM, Jarkko Sakkinen wrote:
>> > On Tue, Nov 06, 2018 at 04:01:57PM +0100, Roberto Sassu wrote:
>> > > Currently the TPM driver allows other kernel subsystems to read
>only the
>> > > SHA1 PCR bank. This patch modifies the parameters of
>tpm_pcr_read() and
>> > > tpm2_pcr_read() to pass a tpm_digest structure, which contains
>the desired
>> > > hash algorithm. Also, since commit 125a22105410 ("tpm: React
>correctly to
>> > > RC_TESTING from TPM 2.0 self tests") removed the call to
>tpm2_pcr_read(),
>> > > the new parameter is expected to be always not NULL.
>> > > 
>> > > Due to the API change, IMA functions have been modified.
>> > > 
>> > > Signed-off-by: Roberto Sassu 
>> > > Acked-by: Mimi Zohar 
>> > 
>> > Does not apply to the current upstream (with tpm1-cmd.c).
>> 
>> Unfortunately, I cannot fetch the repository as infradead.org only
>> supports the git protocol (I'm behind a proxy).
>> 
>> Roberto
>
>I use a proxy script similar to this:
>
>https://gist.github.com/sit/49288
>
>(random googling but gives the idea)
>
>/Jarkko
Moving to a kernel.org repo would be really a benefit or convincing them to 
have a https interface as well.
We have the same proxy issue with infradead.
Peter
-- 
Sent from my mobile

[PATCH] tpm: Make SECURITYFS a weak dependency

[Peter Huewe]

While having SECURITYFS enabled for the tpm subsystem is beneficial in
most cases, it is not strictly necessary to have it enabled at all.
Especially on platforms without any boot firmware integration of the TPM
(e.g. raspberry pi) it does not add any value for the tpm subsystem,
as there is no eventlog present.

By turning it from 'select' to 'imply' it still gets selected per
default, but enables users who want to save some kb of ram by turning
SECURITYFS off.

Signed-off-by: Peter Huewe 
---
 drivers/char/tpm/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig
index 18c81cbe4704..536e55d3919f 100644
--- a/drivers/char/tpm/Kconfig
+++ b/drivers/char/tpm/Kconfig
@@ -5,7 +5,7 @@
 menuconfig TCG_TPM
tristate "TPM Hardware Support"
depends on HAS_IOMEM
-   select SECURITYFS
+   imply SECURITYFS
select CRYPTO
select CRYPTO_HASH_INFO
---help---
-- 
2.16.4

[PATCH] tpm: Make SECURITYFS a weak dependency

[Peter Huewe]

While having SECURITYFS enabled for the tpm subsystem is beneficial in
most cases, it is not strictly necessary to have it enabled at all.
Especially on platforms without any boot firmware integration of the TPM
(e.g. raspberry pi) it does not add any value for the tpm subsystem,
as there is no eventlog present.

By turning it from 'select' to 'imply' it still gets selected per
default, but enables users who want to save some kb of ram by turning
SECURITYFS off.

Signed-off-by: Peter Huewe 
---
 drivers/char/tpm/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig
index 18c81cbe4704..536e55d3919f 100644
--- a/drivers/char/tpm/Kconfig
+++ b/drivers/char/tpm/Kconfig
@@ -5,7 +5,7 @@
 menuconfig TCG_TPM
tristate "TPM Hardware Support"
depends on HAS_IOMEM
-   select SECURITYFS
+   imply SECURITYFS
select CRYPTO
select CRYPTO_HASH_INFO
---help---
-- 
2.16.4

Re: [PATCH] MAINTAINERS: update TPM driver infrastructure changes

[Peter Huewe]

Hi,

Am Freitag, 29. September 2017 22:23:13 CEST schrieb Jarkko Sakkinen:

On Fri, Sep 29, 2017 at 08:29:09AM +, peter.hu...@infineon.com wrote:

...

Spinics is archiving us at
https://www.spinics.net/lists/linux-integrity/

It would be good to add this to the vger page + documented on 
the wiki, which should be added to MAINTAINERS.

http://kernsec.org/wiki/index.php/Linux_Kernel_Integrity as wiki page


Peter ...


Can you submit a follow-up patch that adds this information?


I added the information to the kernsec wiki at
http://kernsec.org/wiki/index.php/Linux_Kernel_Integrity

However, since your MAINTAINERS patch is neither upstream, nor in any of 
your

branches (next / master) I cannot send an update.

About the wiki - I added quite a few other links, especially about tpm 2.0.
If anyone wants to have any information added, please let me know.

I definitely would like to extend the wiki usage.
So if you have any documents, e.g. 
- about how the patch flow is/should be,

- how stuff gets tested
- subsystem specific rules
- subsystem architecuture

but also links to interesting pages/projects PLEASE let me know.

Thanks,
Peter

Re: [PATCH] MAINTAINERS: update TPM driver infrastructure changes

[Peter Huewe]

Hi,

Am Freitag, 29. September 2017 22:23:13 CEST schrieb Jarkko Sakkinen:

On Fri, Sep 29, 2017 at 08:29:09AM +, peter.hu...@infineon.com wrote:

...

Spinics is archiving us at
https://www.spinics.net/lists/linux-integrity/

It would be good to add this to the vger page + documented on 
the wiki, which should be added to MAINTAINERS.

http://kernsec.org/wiki/index.php/Linux_Kernel_Integrity as wiki page


Peter ...


Can you submit a follow-up patch that adds this information?


I added the information to the kernsec wiki at
http://kernsec.org/wiki/index.php/Linux_Kernel_Integrity

However, since your MAINTAINERS patch is neither upstream, nor in any of 
your

branches (next / master) I cannot send an update.

About the wiki - I added quite a few other links, especially about tpm 2.0.
If anyone wants to have any information added, please let me know.

I definitely would like to extend the wiki usage.
So if you have any documents, e.g. 
- about how the patch flow is/should be,

- how stuff gets tested
- subsystem specific rules
- subsystem architecuture

but also links to interesting pages/projects PLEASE let me know.

Thanks,
Peter

Re: Fixing CVE-2017-15361

[Peter Huewe]

Am 25. Oktober 2017 20:53:49 MESZ schrieb Jarkko Sakkinen 
:
>On Wed, Oct 25, 2017 at 07:17:17AM -0700, Matthew Garrett wrote:
>> On Wed, Oct 25, 2017 at 6:44 AM, Jarkko Sakkinen
>>  wrote:
>> > I'm implementing a fix for CVE-2017-15361 that simply blacklists
>> > vulnerable FW versions. I think this is the only responsible action
>from
>> > my side that I can do.
>> 
>> I'm not sure this is ideal - do Infineon have any Linux tooling for
>> performing firmware updates, and if so will that continue working if
>> the device is blacklisted? It's also a poor user experience to have
>> systems using TPM-backed disk encryption keys suddenly rendered
>> unbootable, and making it as easy as possible for people to do an
>> upgrade and then re-seal secrets with new keys feels like the correct
>> approach.
>
>I talked today with Alexander Steffen in the KS unconference and we
>concluded that this would be a terrible idea.
>
>Alexander stated the following things about FW updates (Alexander,
>please correct me if I state something incorrectly or if you have
>something to add):
>
>* FW update can be constructed either in a way that the keys in the
>  NVRAM are not cleared or in a way that they are cleared.
>* FW update cannot be directly applied to the TPM but must come as
>  part of the firmware update from the vendor.
>
>I proposed the following as an alternative:
>
>* Print a message to the klog (which log level would be appropriate?).
Info?
Maybe warn, definitely not err

>* Possibly sleep for few seconds. Is this a good idea?
Helps how?

>
>While writing this email yet another alternative popped into my mind:
>what if we allow only in-kernel use but disallow the use of /dev/tpm0?
>You could still use trusted keys.
>
No, same terrible idea since you block the upgrade path.
Upgrade tools work from userspace via the kernel driver. 
So /dev/tpm0 is necessary.

>Here are all the ideas that I have and I am open for better
>alternatives.
>
>/Jarkko

-- 
Sent from my mobile

Re: Fixing CVE-2017-15361

[Peter Huewe]

Am 25. Oktober 2017 20:53:49 MESZ schrieb Jarkko Sakkinen 
:
>On Wed, Oct 25, 2017 at 07:17:17AM -0700, Matthew Garrett wrote:
>> On Wed, Oct 25, 2017 at 6:44 AM, Jarkko Sakkinen
>>  wrote:
>> > I'm implementing a fix for CVE-2017-15361 that simply blacklists
>> > vulnerable FW versions. I think this is the only responsible action
>from
>> > my side that I can do.
>> 
>> I'm not sure this is ideal - do Infineon have any Linux tooling for
>> performing firmware updates, and if so will that continue working if
>> the device is blacklisted? It's also a poor user experience to have
>> systems using TPM-backed disk encryption keys suddenly rendered
>> unbootable, and making it as easy as possible for people to do an
>> upgrade and then re-seal secrets with new keys feels like the correct
>> approach.
>
>I talked today with Alexander Steffen in the KS unconference and we
>concluded that this would be a terrible idea.
>
>Alexander stated the following things about FW updates (Alexander,
>please correct me if I state something incorrectly or if you have
>something to add):
>
>* FW update can be constructed either in a way that the keys in the
>  NVRAM are not cleared or in a way that they are cleared.
>* FW update cannot be directly applied to the TPM but must come as
>  part of the firmware update from the vendor.
>
>I proposed the following as an alternative:
>
>* Print a message to the klog (which log level would be appropriate?).
Info?
Maybe warn, definitely not err

>* Possibly sleep for few seconds. Is this a good idea?
Helps how?

>
>While writing this email yet another alternative popped into my mind:
>what if we allow only in-kernel use but disallow the use of /dev/tpm0?
>You could still use trusted keys.
>
No, same terrible idea since you block the upgrade path.
Upgrade tools work from userspace via the kernel driver. 
So /dev/tpm0 is necessary.

>Here are all the ideas that I have and I am open for better
>alternatives.
>
>/Jarkko

-- 
Sent from my mobile

Re: [tpmdd-devel] [PATCH] tpm: remove chip_num parameter from in-kernel API

[Peter Huewe]

Am 24. Oktober 2017 20:15:12 MESZ schrieb Jarkko Sakkinen 
:
>On Tue, Oct 24, 2017 at 10:02:00AM -0700, Dmitry Torokhov wrote:
>> On Tue, Oct 24, 2017 at 9:11 AM, Jason Gunthorpe
>>  wrote:
>> > On Tue, Oct 24, 2017 at 09:37:33PM +0530, PrasannaKumar
>Muralidharan wrote:
>> >> Hi Jason,
>> >>
>> >> On 24 October 2017 at 21:25, Jason Gunthorpe
>> >>  wrote:
>> >> > On Tue, Oct 24, 2017 at 09:21:15PM +0530, PrasannaKumar
>Muralidharan wrote:
>> >> >
>> >> >> Please check the RFC [1]. It does use chip id. The rfc has
>issues and
>> >> >> has to be fixed but still there could be users of the API.
>> >> >>
>> >> >> 1. https://www.spinics.net/lists/linux-crypto/msg28282.html
>> >> >
>> >> > That patch isn't safe at all. You need to store a kref to th
>chip in
>> >> > the hwrng, not parse a string.
>> >>
>> >> The drivers/char/hw_random/tpm-rng.c module does not store the
>chip
>> >> reference so I guess the usage is safe.
>> >
>> > It is using the default TPM, it is always safe to use the default
>tpm.
>> 
>> tpm-rng is abomination that should be kicked out as soon as possible.
>> It wrecks havoc with the power management (TPM chip drivers may go
>> into suspend state, but tpm_rng does not do any power management and
>> happily forwards requests to suspended hardware) and may be available
>> when there is no TPM at all yet (the drivers have not been probed
>yet,
>> or have gotten a deferral, etc).
>> 
>> TPM core should register HWRNGs when chips are ready.
>> 
>> Thanks.
>> 
>> -- 
>> Dmitry
>
>I'm fine to review a two patch set where:
>
>1. Patch 1 removes the existing TPM rng driver
>2. Patch 2 makes the TPM driver as rng producer

Yes, but tpm must be kept a hwrng source.
This is imho an important use case.

>
>Unrelate to patch that I'm proposing now but this sounds sensible.
>
>/Jarkko

-- 
Sent from my mobile

Re: [tpmdd-devel] [PATCH] tpm: remove chip_num parameter from in-kernel API

[Peter Huewe]

Am 24. Oktober 2017 20:15:12 MESZ schrieb Jarkko Sakkinen 
:
>On Tue, Oct 24, 2017 at 10:02:00AM -0700, Dmitry Torokhov wrote:
>> On Tue, Oct 24, 2017 at 9:11 AM, Jason Gunthorpe
>>  wrote:
>> > On Tue, Oct 24, 2017 at 09:37:33PM +0530, PrasannaKumar
>Muralidharan wrote:
>> >> Hi Jason,
>> >>
>> >> On 24 October 2017 at 21:25, Jason Gunthorpe
>> >>  wrote:
>> >> > On Tue, Oct 24, 2017 at 09:21:15PM +0530, PrasannaKumar
>Muralidharan wrote:
>> >> >
>> >> >> Please check the RFC [1]. It does use chip id. The rfc has
>issues and
>> >> >> has to be fixed but still there could be users of the API.
>> >> >>
>> >> >> 1. https://www.spinics.net/lists/linux-crypto/msg28282.html
>> >> >
>> >> > That patch isn't safe at all. You need to store a kref to th
>chip in
>> >> > the hwrng, not parse a string.
>> >>
>> >> The drivers/char/hw_random/tpm-rng.c module does not store the
>chip
>> >> reference so I guess the usage is safe.
>> >
>> > It is using the default TPM, it is always safe to use the default
>tpm.
>> 
>> tpm-rng is abomination that should be kicked out as soon as possible.
>> It wrecks havoc with the power management (TPM chip drivers may go
>> into suspend state, but tpm_rng does not do any power management and
>> happily forwards requests to suspended hardware) and may be available
>> when there is no TPM at all yet (the drivers have not been probed
>yet,
>> or have gotten a deferral, etc).
>> 
>> TPM core should register HWRNGs when chips are ready.
>> 
>> Thanks.
>> 
>> -- 
>> Dmitry
>
>I'm fine to review a two patch set where:
>
>1. Patch 1 removes the existing TPM rng driver
>2. Patch 2 makes the TPM driver as rng producer

Yes, but tpm must be kept a hwrng source.
This is imho an important use case.

>
>Unrelate to patch that I'm proposing now but this sounds sensible.
>
>/Jarkko

-- 
Sent from my mobile

Re: Re: [PATCH] MAINTAINERS: update TPM driver infrastructure changes

[Peter Huewe]

Will do.
Peter

Am 4. Oktober 2017 09:27:08 MESZ schrieb Jarkko Sakkinen 
<jarkko.sakki...@linux.intel.com>:
>On Fri, Sep 29, 2017 at 08:31:30PM +0200, Peter Huewe wrote:
>> 
>>  
>>  
>> 
>> Gesendet: Freitag, 29. September 2017 um 18:59 Uhr
>> Von: "Jarkko Sakkinen" <jarkko.sakki...@linux.intel.com>
>> An: "Mimi Zohar" <zo...@linux.vnet.ibm.com>, peterhu...@gmx.de
>> Cc: linux-kernel@vger.kernel.org, linux-integr...@vger.kernel.org
>> Betreff: Re: [PATCH] MAINTAINERS: update TPM driver infrastructure
>changes
>> On Tue, Sep 26, 2017 at 12:16:20PM -0400, Mimi Zohar wrote:
>> > Hi Jarkko,
>> >
>> > > +Q: https://patchwork.kernel.org/project/linux-integrity/list/
>> >
>> > Is there a way of viewing not just the posted patches, but the
>> > discussion as well?  Do we need to set up an archive as well?
>> >
>> > Mimi
>> 
>> > Peter, did you setup archiving?
>> Jarkko, did you read my other email?
>> 
>> /Peter
>> 
>> 
>> p.s.: 
>> Spinics is archiving us at
>>
>https://www.spinics.net/lists/linux-integrity/[https://www.spinics.net/lists/linux-integrity/]
>> 
>> It would be good to add this to the vger page + documented on the
>wiki, which should be added to MAINTAINERS.
>>
>http://kernsec.org/wiki/index.php/Linux_Kernel_Integrity[http://kernsec.org/wiki/index.php/Linux_Kernel_Integrity]
>as wiki page
>
>Can you send a follow-up patch to update MAINTAINERS?
>
>The first one is already applied so it does not make sense to
>revise it and it doesn't contain any errors.
>
>/Jarkko

-- 
Sent from my mobile

Re: Re: [PATCH] MAINTAINERS: update TPM driver infrastructure changes

[Peter Huewe]

Will do.
Peter

Am 4. Oktober 2017 09:27:08 MESZ schrieb Jarkko Sakkinen 
:
>On Fri, Sep 29, 2017 at 08:31:30PM +0200, Peter Huewe wrote:
>> 
>>  
>>  
>> 
>> Gesendet: Freitag, 29. September 2017 um 18:59 Uhr
>> Von: "Jarkko Sakkinen" 
>> An: "Mimi Zohar" , peterhu...@gmx.de
>> Cc: linux-kernel@vger.kernel.org, linux-integr...@vger.kernel.org
>> Betreff: Re: [PATCH] MAINTAINERS: update TPM driver infrastructure
>changes
>> On Tue, Sep 26, 2017 at 12:16:20PM -0400, Mimi Zohar wrote:
>> > Hi Jarkko,
>> >
>> > > +Q: https://patchwork.kernel.org/project/linux-integrity/list/
>> >
>> > Is there a way of viewing not just the posted patches, but the
>> > discussion as well?  Do we need to set up an archive as well?
>> >
>> > Mimi
>> 
>> > Peter, did you setup archiving?
>> Jarkko, did you read my other email?
>> 
>> /Peter
>> 
>> 
>> p.s.: 
>> Spinics is archiving us at
>>
>https://www.spinics.net/lists/linux-integrity/[https://www.spinics.net/lists/linux-integrity/]
>> 
>> It would be good to add this to the vger page + documented on the
>wiki, which should be added to MAINTAINERS.
>>
>http://kernsec.org/wiki/index.php/Linux_Kernel_Integrity[http://kernsec.org/wiki/index.php/Linux_Kernel_Integrity]
>as wiki page
>
>Can you send a follow-up patch to update MAINTAINERS?
>
>The first one is already applied so it does not make sense to
>revise it and it doesn't contain any errors.
>
>/Jarkko

-- 
Sent from my mobile

Aw: Re: [PATCH] MAINTAINERS: update TPM driver infrastructure changes

[Peter Huewe]

 
 

Gesendet: Freitag, 29. September 2017 um 18:59 Uhr
Von: "Jarkko Sakkinen" 
An: "Mimi Zohar" , peterhu...@gmx.de
Cc: linux-kernel@vger.kernel.org, linux-integr...@vger.kernel.org
Betreff: Re: [PATCH] MAINTAINERS: update TPM driver infrastructure changes
On Tue, Sep 26, 2017 at 12:16:20PM -0400, Mimi Zohar wrote:
> Hi Jarkko,
>
> > +Q: https://patchwork.kernel.org/project/linux-integrity/list/
>
> Is there a way of viewing not just the posted patches, but the
> discussion as well?  Do we need to set up an archive as well?
>
> Mimi

> Peter, did you setup archiving?
Jarkko, did you read my other email?

/Peter


p.s.: 
Spinics is archiving us at
https://www.spinics.net/lists/linux-integrity/[https://www.spinics.net/lists/linux-integrity/]

It would be good to add this to the vger page + documented on the wiki, which 
should be added to MAINTAINERS.
http://kernsec.org/wiki/index.php/Linux_Kernel_Integrity[http://kernsec.org/wiki/index.php/Linux_Kernel_Integrity]
 as wiki page

Aw: Re: [PATCH] MAINTAINERS: update TPM driver infrastructure changes

[Peter Huewe]

 
 

Gesendet: Freitag, 29. September 2017 um 18:59 Uhr
Von: "Jarkko Sakkinen" 
An: "Mimi Zohar" , peterhu...@gmx.de
Cc: linux-kernel@vger.kernel.org, linux-integr...@vger.kernel.org
Betreff: Re: [PATCH] MAINTAINERS: update TPM driver infrastructure changes
On Tue, Sep 26, 2017 at 12:16:20PM -0400, Mimi Zohar wrote:
> Hi Jarkko,
>
> > +Q: https://patchwork.kernel.org/project/linux-integrity/list/
>
> Is there a way of viewing not just the posted patches, but the
> discussion as well?  Do we need to set up an archive as well?
>
> Mimi

> Peter, did you setup archiving?
Jarkko, did you read my other email?

/Peter


p.s.: 
Spinics is archiving us at
https://www.spinics.net/lists/linux-integrity/[https://www.spinics.net/lists/linux-integrity/]

It would be good to add this to the vger page + documented on the wiki, which 
should be added to MAINTAINERS.
http://kernsec.org/wiki/index.php/Linux_Kernel_Integrity[http://kernsec.org/wiki/index.php/Linux_Kernel_Integrity]
 as wiki page

Re: [tpmdd-devel] New ML for TPM and IMA

[Peter Huewe]

Am 15. September 2017 13:07:58 GMT-07:00 schrieb Ken Goldman 
:
>Newbie question:  Do I have to subscribe, or are email addresses being 
>migrated

You have to subscribe yourself.
Due to the new privacy regulations I do not have access to any member 
information anymore on the old list.


Just send a plaintext  email with the body/text
subscribe linux-integrity
to majord...@vger.kernel.org

Peter
>
>On 9/15/2017 1:18 PM, Jarkko Sakkinen wrote:
>> Hi
>> 
>> Many people were kicked out from the SourceForge mailing list in the
>> July because SF required a resubscription, including non-human users,
>> such as patchwork.
>> 
>> We decided to create a new mailing list
>linux-integ...@vger.kernel.org
>> to cover both TPM and IMA since they tend to have cross dependencies.
>> Otherwise, the maintainer hierarchy etc. will stay the same.
>> 
>> It is all documented here:
>> 
>> http://kernsec.org/wiki/index.php/Linux_Kernel_Integrity
>> 
>>  From now on use this list for patches and discussion instead of the
>> legacy list.
>
>
>--
>Check out the vibrant tech community on one of the world's most
>engaging tech sites, Slashdot.org! http://sdm.link/slashdot
>___
>tpmdd-devel mailing list
>tpmdd-de...@lists.sourceforge.net
>https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

-- 
Sent from my mobile

Re: [tpmdd-devel] New ML for TPM and IMA

[Peter Huewe]

Am 15. September 2017 13:07:58 GMT-07:00 schrieb Ken Goldman 
:
>Newbie question:  Do I have to subscribe, or are email addresses being 
>migrated

You have to subscribe yourself.
Due to the new privacy regulations I do not have access to any member 
information anymore on the old list.


Just send a plaintext  email with the body/text
subscribe linux-integrity
to majord...@vger.kernel.org

Peter
>
>On 9/15/2017 1:18 PM, Jarkko Sakkinen wrote:
>> Hi
>> 
>> Many people were kicked out from the SourceForge mailing list in the
>> July because SF required a resubscription, including non-human users,
>> such as patchwork.
>> 
>> We decided to create a new mailing list
>linux-integ...@vger.kernel.org
>> to cover both TPM and IMA since they tend to have cross dependencies.
>> Otherwise, the maintainer hierarchy etc. will stay the same.
>> 
>> It is all documented here:
>> 
>> http://kernsec.org/wiki/index.php/Linux_Kernel_Integrity
>> 
>>  From now on use this list for patches and discussion instead of the
>> legacy list.
>
>
>--
>Check out the vibrant tech community on one of the world's most
>engaging tech sites, Slashdot.org! http://sdm.link/slashdot
>___
>tpmdd-devel mailing list
>tpmdd-de...@lists.sourceforge.net
>https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

-- 
Sent from my mobile

Re: [tpmdd-devel] New ML for TPM and IMA

[Peter Huewe]

Am 15. September 2017 10:40:14 GMT-07:00 schrieb Joe Perches :
>On Fri, 2017-09-15 at 10:36 -0700, Jarkko Sakkinen wrote:
>> On Fri, Sep 15, 2017 at 10:25:36AM -0700, Joe Perches wrote:
>> > On Fri, 2017-09-15 at 10:18 -0700, Jarkko Sakkinen wrote:
>[]
>> > > We decided to create a new mailing list
>linux-integ...@vger.kernel.org
>> > > to cover both TPM and IMA since they tend to have cross
>dependencies.
>[]
>> > And is there to be an update to the MAINTAINERS file entries?
>[]
>> Yes, I'll get onto it. BTW, do we need two entries for TPM in the
>> MAINTAINERS file or can I merge those?
>
>As the individual maintainers are different for the two sections,
>I think you need both entries.

Try to get a hold of ashley and ask whether she is actively maintaining.

While updating Maintainers I vote for removing Marcel Selhorst for tpm.

You can keep me for now :)
Peter
>
>
>--
>Check out the vibrant tech community on one of the world's most
>engaging tech sites, Slashdot.org! http://sdm.link/slashdot
>___
>tpmdd-devel mailing list
>tpmdd-de...@lists.sourceforge.net
>https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

-- 
Sent from my mobile

Re: [tpmdd-devel] New ML for TPM and IMA

[Peter Huewe]

Am 15. September 2017 10:40:14 GMT-07:00 schrieb Joe Perches :
>On Fri, 2017-09-15 at 10:36 -0700, Jarkko Sakkinen wrote:
>> On Fri, Sep 15, 2017 at 10:25:36AM -0700, Joe Perches wrote:
>> > On Fri, 2017-09-15 at 10:18 -0700, Jarkko Sakkinen wrote:
>[]
>> > > We decided to create a new mailing list
>linux-integ...@vger.kernel.org
>> > > to cover both TPM and IMA since they tend to have cross
>dependencies.
>[]
>> > And is there to be an update to the MAINTAINERS file entries?
>[]
>> Yes, I'll get onto it. BTW, do we need two entries for TPM in the
>> MAINTAINERS file or can I merge those?
>
>As the individual maintainers are different for the two sections,
>I think you need both entries.

Try to get a hold of ashley and ask whether she is actively maintaining.

While updating Maintainers I vote for removing Marcel Selhorst for tpm.

You can keep me for now :)
Peter
>
>
>--
>Check out the vibrant tech community on one of the world's most
>engaging tech sites, Slashdot.org! http://sdm.link/slashdot
>___
>tpmdd-devel mailing list
>tpmdd-de...@lists.sourceforge.net
>https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

-- 
Sent from my mobile

Re: [tpmdd-devel] New ML for TPM and IMA

[Peter Huewe]

Am 15. September 2017 10:18:25 GMT-07:00 schrieb Jarkko Sakkinen 
:
>Hi
>
>Many people were kicked out from the SourceForge mailing list in the
>July because SF required a resubscription, including non-human users,
>such as patchwork.
>
>We decided to create a new mailing list linux-integ...@vger.kernel.org

Of course it's
linux-integr...@vger.kernel.org 
(Cc'ed)




>to cover both TPM and IMA since they tend to have cross dependencies.
>Otherwise, the maintainer hierarchy etc. will stay the same.
>
>It is all documented here:
>
>http://kernsec.org/wiki/index.php/Linux_Kernel_Integrity
>
>From now on use this list for patches and discussion instead of the
>legacy list.
>
>Thank you.
>
>/Jarkko
>
>--
>Check out the vibrant tech community on one of the world's most
>engaging tech sites, Slashdot.org! http://sdm.link/slashdot
>___
>tpmdd-devel mailing list
>tpmdd-de...@lists.sourceforge.net
>https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

-- 
Sent from my mobile

Re: [tpmdd-devel] New ML for TPM and IMA

[Peter Huewe]

Am 15. September 2017 10:18:25 GMT-07:00 schrieb Jarkko Sakkinen 
:
>Hi
>
>Many people were kicked out from the SourceForge mailing list in the
>July because SF required a resubscription, including non-human users,
>such as patchwork.
>
>We decided to create a new mailing list linux-integ...@vger.kernel.org

Of course it's
linux-integr...@vger.kernel.org 
(Cc'ed)




>to cover both TPM and IMA since they tend to have cross dependencies.
>Otherwise, the maintainer hierarchy etc. will stay the same.
>
>It is all documented here:
>
>http://kernsec.org/wiki/index.php/Linux_Kernel_Integrity
>
>From now on use this list for patches and discussion instead of the
>legacy list.
>
>Thank you.
>
>/Jarkko
>
>--
>Check out the vibrant tech community on one of the world's most
>engaging tech sites, Slashdot.org! http://sdm.link/slashdot
>___
>tpmdd-devel mailing list
>tpmdd-de...@lists.sourceforge.net
>https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

-- 
Sent from my mobile

Re: [tpmdd-devel] [PATCH v2 1/4] tpm: ignore burstcount to improve tpm_tis send() performance.

[Peter Huewe]

Am 13. September 2017 11:52:12 GMT-07:00 schrieb Ken Goldman 
:
>On 9/6/2017 12:12 PM, Jason Gunthorpe wrote:
>> 
>> The problem with this approach is that the TPM could totally block
>> the CPU for very long periods of time.
>> 
>> It seems very risky to enable..
>> 
>
>How would you characterize "very long"?
>
>The TPM vendors confirm that they empty the FIFO at internal speeds
>that
>are comparable to the bus speed.  Thus, any stall will be sub-usec.  Is
>that an issue?

If the tpm does behave correctly, this is fine. 
If the tpm hangs for whatever reason, your machine is frozen and you will never 
figure out why.

That's my concern there.
However ddwg seems fine.

>
>In addition, new TPMs have ever larger FIFO's, making stalls less
>likely
>going forward.
But also reduced the polling loops that introduce the performance penalty ;)

>
>
>--
>Check out the vibrant tech community on one of the world's most
>engaging tech sites, Slashdot.org! http://sdm.link/slashdot
>___
>tpmdd-devel mailing list
>tpmdd-de...@lists.sourceforge.net
>https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

-- 
Sent from my mobile

Re: [tpmdd-devel] [PATCH v2 1/4] tpm: ignore burstcount to improve tpm_tis send() performance.

[Peter Huewe]

Am 13. September 2017 11:52:12 GMT-07:00 schrieb Ken Goldman 
:
>On 9/6/2017 12:12 PM, Jason Gunthorpe wrote:
>> 
>> The problem with this approach is that the TPM could totally block
>> the CPU for very long periods of time.
>> 
>> It seems very risky to enable..
>> 
>
>How would you characterize "very long"?
>
>The TPM vendors confirm that they empty the FIFO at internal speeds
>that
>are comparable to the bus speed.  Thus, any stall will be sub-usec.  Is
>that an issue?

If the tpm does behave correctly, this is fine. 
If the tpm hangs for whatever reason, your machine is frozen and you will never 
figure out why.

That's my concern there.
However ddwg seems fine.

>
>In addition, new TPMs have ever larger FIFO's, making stalls less
>likely
>going forward.
But also reduced the polling loops that introduce the performance penalty ;)

>
>
>--
>Check out the vibrant tech community on one of the world's most
>engaging tech sites, Slashdot.org! http://sdm.link/slashdot
>___
>tpmdd-devel mailing list
>tpmdd-de...@lists.sourceforge.net
>https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

-- 
Sent from my mobile

Re: [PATCH v2 1/4] tpm: ignore burstcount to improve tpm_tis send() performance.

[Peter Huewe]

Am 12. September 2017 17:45:08 GMT-07:00 schrieb Jarkko Sakkinen 
:
>On Wed, Sep 06, 2017 at 08:56:36AM -0400, Nayna Jain wrote:
>> The TPM burstcount status indicates the number of bytes that can
>> be sent to the TPM without causing bus wait states.  Effectively,
>> it is the number of empty bytes in the command FIFO. Further,
>> some TPMs have a static burstcount, when the value remains zero
>> until the entire FIFO is empty.
>> 
>> This patch adds an optimization to check for burstcount only once.
>> And if it is valid, it writes all the bytes at once, permitting
>> wait states. The performance of a 34 byte extend on a TPM 1.2 with
>> an 8 byte burstcount improved from 41 msec to 14 msec.
>> 
>> This functionality is enabled only by passing module
>> parameter ignore_burst_count=1. By default, this parameter
>> is disabled.
>> 
>> After this change, performance on a TPM 1.2 with an 8 byte
>> burstcount for 1000 extends improved from ~41sec to ~14sec.
>> 
>> Suggested-by: Ken Goldman  in
>> conjunction with the TPM Device Driver work group.
>> Signed-off-by: Nayna Jain 
>> Acked-by: Mimi Zohar 
>> ---
>>  Documentation/admin-guide/kernel-parameters.txt |  8 
>>  drivers/char/tpm/tpm_tis_core.c | 24
>+---
>>  2 files changed, 29 insertions(+), 3 deletions(-)
>> 
>> diff --git a/Documentation/admin-guide/kernel-parameters.txt
>b/Documentation/admin-guide/kernel-parameters.txt
>> index 4e303be83df6..3c59bb91e1ee 100644
>> --- a/Documentation/admin-guide/kernel-parameters.txt
>> +++ b/Documentation/admin-guide/kernel-parameters.txt
>> @@ -1465,6 +1465,14 @@
>>  mode generally follows that for the NaN encoding,
>>  except where unsupported by hardware.
>>  
>> +ignore_burst_count [TPM_TIS_CORE]
>> +tpm_tis_core driver queries for the burstcount before
>> +every send call in a loop. However, it causes delay to
>> +the send command for TPMs with low burstcount value.
>> +Setting this value to 1, will make driver to query for
>> +burstcount only once in the loop to improve the
>> +performance. By default, its value is set to 0.
>> +
>>  ignore_loglevel [KNL]
>>  Ignore loglevel setting - this will print /all/
>>  kernel messages to the console. Useful for debugging.
>> diff --git a/drivers/char/tpm/tpm_tis_core.c
>b/drivers/char/tpm/tpm_tis_core.c
>> index 63bc6c3b949e..6b9bf4c4d434 100644
>> --- a/drivers/char/tpm/tpm_tis_core.c
>> +++ b/drivers/char/tpm/tpm_tis_core.c
>> @@ -31,6 +31,11 @@
>>  #include "tpm.h"
>>  #include "tpm_tis_core.h"
>>  
>> +static bool ignore_burst_count = false;
>> +module_param(ignore_burst_count, bool, 0444);
>> +MODULE_PARM_DESC(ignore_burst_count,
>> +"Ignore burstcount value while writing data");
>> +
>>  /* Before we attempt to access the TPM we must see that the valid
>bit is set.
>>   * The specification says that this bit is 0 at reset and remains 0
>until the
>>   * 'TPM has gone through its self test and initialization and has
>established
>> @@ -256,6 +261,7 @@ static int tpm_tis_send_data(struct tpm_chip
>*chip, u8 *buf, size_t len)
>>  {
>>  struct tpm_tis_data *priv = dev_get_drvdata(>dev);
>>  int rc, status, burstcnt;
>> +int sendcnt;
>>  size_t count = 0;
>>  bool itpm = priv->flags & TPM_TIS_ITPM_WORKAROUND;
>>  
>> @@ -271,19 +277,31 @@ static int tpm_tis_send_data(struct tpm_chip
>*chip, u8 *buf, size_t len)
>>  }
>>  
>>  while (count < len - 1) {
>> +
>> +/*
>> + * Get the initial burstcount to ensure TPM is ready to
>> + * accept data, even when waiting for burstcount is disabled.
>> + */
>>  burstcnt = get_burstcount(chip);
>>  if (burstcnt < 0) {
>>  dev_err(>dev, "Unable to read burstcount\n");
>>  rc = burstcnt;
>>  goto out_err;
>>  }
>> -burstcnt = min_t(int, burstcnt, len - count - 1);
>> +
>> +if (ignore_burst_count)
>> +sendcnt = len - 1;
>> +else
>> +sendcnt = min_t(int, burstcnt, len - count - 1);
>> +
>>  rc = tpm_tis_write_bytes(priv, TPM_DATA_FIFO(priv->locality),
>> - burstcnt, buf + count);
>> + sendcnt, buf + count);
>>  if (rc < 0)
>>  goto out_err;
>>  
>> -count += burstcnt;
>> +count += sendcnt;
>> +if (ignore_burst_count)
>> +continue;
>>  
>>  if (wait_for_tpm_stat(chip, TPM_STS_VALID, chip->timeout_c,
>>  

Re: [PATCH v2 1/4] tpm: ignore burstcount to improve tpm_tis send() performance.

[Peter Huewe]

Am 12. September 2017 17:45:08 GMT-07:00 schrieb Jarkko Sakkinen 
:
>On Wed, Sep 06, 2017 at 08:56:36AM -0400, Nayna Jain wrote:
>> The TPM burstcount status indicates the number of bytes that can
>> be sent to the TPM without causing bus wait states.  Effectively,
>> it is the number of empty bytes in the command FIFO. Further,
>> some TPMs have a static burstcount, when the value remains zero
>> until the entire FIFO is empty.
>> 
>> This patch adds an optimization to check for burstcount only once.
>> And if it is valid, it writes all the bytes at once, permitting
>> wait states. The performance of a 34 byte extend on a TPM 1.2 with
>> an 8 byte burstcount improved from 41 msec to 14 msec.
>> 
>> This functionality is enabled only by passing module
>> parameter ignore_burst_count=1. By default, this parameter
>> is disabled.
>> 
>> After this change, performance on a TPM 1.2 with an 8 byte
>> burstcount for 1000 extends improved from ~41sec to ~14sec.
>> 
>> Suggested-by: Ken Goldman  in
>> conjunction with the TPM Device Driver work group.
>> Signed-off-by: Nayna Jain 
>> Acked-by: Mimi Zohar 
>> ---
>>  Documentation/admin-guide/kernel-parameters.txt |  8 
>>  drivers/char/tpm/tpm_tis_core.c | 24
>+---
>>  2 files changed, 29 insertions(+), 3 deletions(-)
>> 
>> diff --git a/Documentation/admin-guide/kernel-parameters.txt
>b/Documentation/admin-guide/kernel-parameters.txt
>> index 4e303be83df6..3c59bb91e1ee 100644
>> --- a/Documentation/admin-guide/kernel-parameters.txt
>> +++ b/Documentation/admin-guide/kernel-parameters.txt
>> @@ -1465,6 +1465,14 @@
>>  mode generally follows that for the NaN encoding,
>>  except where unsupported by hardware.
>>  
>> +ignore_burst_count [TPM_TIS_CORE]
>> +tpm_tis_core driver queries for the burstcount before
>> +every send call in a loop. However, it causes delay to
>> +the send command for TPMs with low burstcount value.
>> +Setting this value to 1, will make driver to query for
>> +burstcount only once in the loop to improve the
>> +performance. By default, its value is set to 0.
>> +
>>  ignore_loglevel [KNL]
>>  Ignore loglevel setting - this will print /all/
>>  kernel messages to the console. Useful for debugging.
>> diff --git a/drivers/char/tpm/tpm_tis_core.c
>b/drivers/char/tpm/tpm_tis_core.c
>> index 63bc6c3b949e..6b9bf4c4d434 100644
>> --- a/drivers/char/tpm/tpm_tis_core.c
>> +++ b/drivers/char/tpm/tpm_tis_core.c
>> @@ -31,6 +31,11 @@
>>  #include "tpm.h"
>>  #include "tpm_tis_core.h"
>>  
>> +static bool ignore_burst_count = false;
>> +module_param(ignore_burst_count, bool, 0444);
>> +MODULE_PARM_DESC(ignore_burst_count,
>> +"Ignore burstcount value while writing data");
>> +
>>  /* Before we attempt to access the TPM we must see that the valid
>bit is set.
>>   * The specification says that this bit is 0 at reset and remains 0
>until the
>>   * 'TPM has gone through its self test and initialization and has
>established
>> @@ -256,6 +261,7 @@ static int tpm_tis_send_data(struct tpm_chip
>*chip, u8 *buf, size_t len)
>>  {
>>  struct tpm_tis_data *priv = dev_get_drvdata(>dev);
>>  int rc, status, burstcnt;
>> +int sendcnt;
>>  size_t count = 0;
>>  bool itpm = priv->flags & TPM_TIS_ITPM_WORKAROUND;
>>  
>> @@ -271,19 +277,31 @@ static int tpm_tis_send_data(struct tpm_chip
>*chip, u8 *buf, size_t len)
>>  }
>>  
>>  while (count < len - 1) {
>> +
>> +/*
>> + * Get the initial burstcount to ensure TPM is ready to
>> + * accept data, even when waiting for burstcount is disabled.
>> + */
>>  burstcnt = get_burstcount(chip);
>>  if (burstcnt < 0) {
>>  dev_err(>dev, "Unable to read burstcount\n");
>>  rc = burstcnt;
>>  goto out_err;
>>  }
>> -burstcnt = min_t(int, burstcnt, len - count - 1);
>> +
>> +if (ignore_burst_count)
>> +sendcnt = len - 1;
>> +else
>> +sendcnt = min_t(int, burstcnt, len - count - 1);
>> +
>>  rc = tpm_tis_write_bytes(priv, TPM_DATA_FIFO(priv->locality),
>> - burstcnt, buf + count);
>> + sendcnt, buf + count);
>>  if (rc < 0)
>>  goto out_err;
>>  
>> -count += burstcnt;
>> +count += sendcnt;
>> +if (ignore_burst_count)
>> +continue;
>>  
>>  if (wait_for_tpm_stat(chip, TPM_STS_VALID, chip->timeout_c,
>>  >int_queue, false) < 0) {
>> -- 
>> 2.13.3
>> 
>
>Makes sense to discuss whether to have the kernel command-line

Aw: Re: [tpmdd-devel] [PATCH] tpm: improve tpm_tis send() performance by ignoring burstcount

[Peter Huewe]

Hi Ken,
(again speaking only on my behalf, not my employer)

> Does anyone know of platforms where this occurs?
> I suspect (but not sure) that the days of SuperIO connecting floppy
> drives, printer ports, and PS/2 mouse ports on the LPC bus are over, and
> such legacy systems will not have a TPM. Would SuperIO even support the
> special TPM LPC bus cycles?

Since we are the linux kernel, we do have to care for legacy devices.
And a system with LPC, PS2Mouse on SuperIO and a TPM are not that uncommon.

And heck, we even have support for 1.1b TPM devices


>> One more viewpoint: TCG must added the burst count for a reason (might
>> be very well related what Peter said). Is ignoring it something that TCG
>> recommends? Not following standard exactly in the driver code sometimes
>> makes sense on *small details* but I would not say that this a small
>> detail...

> I checked with the TCG's device driver work group (DDWG). Both the spec
> editor and 3 TPM vendors - Infineon, Nuvoton, and ST Micro - agreed that
> ignoring burst count may incur wait states but nothing more. Operations
> will still be successful.

Interesting - let me check with Georg tomorrow.
Unfortunately I do not have access to my tcg mails from home (since I'm not 
working :), 
but did you _explicitly_ talk about LPC and the system?
I'm sure the TPM does not care about the waitstates...

If my memory does not betray me, 
it is actually possible to "freeze up" a system completly by flooding the lpc 
bus.
Let me double check tomorrow...


In anycase - I really would like to see a much more performant tpm subsystem - 
however it will be quite an effort with a lot of legacy testing.
(which I unfortunately cannot spend on my private time ... and also of course 
lacking test systems).

Thanks,
Peter

Aw: Re: [tpmdd-devel] [PATCH] tpm: improve tpm_tis send() performance by ignoring burstcount

[Peter Huewe]

Hi Ken,
(again speaking only on my behalf, not my employer)

> Does anyone know of platforms where this occurs?
> I suspect (but not sure) that the days of SuperIO connecting floppy
> drives, printer ports, and PS/2 mouse ports on the LPC bus are over, and
> such legacy systems will not have a TPM. Would SuperIO even support the
> special TPM LPC bus cycles?

Since we are the linux kernel, we do have to care for legacy devices.
And a system with LPC, PS2Mouse on SuperIO and a TPM are not that uncommon.

And heck, we even have support for 1.1b TPM devices


>> One more viewpoint: TCG must added the burst count for a reason (might
>> be very well related what Peter said). Is ignoring it something that TCG
>> recommends? Not following standard exactly in the driver code sometimes
>> makes sense on *small details* but I would not say that this a small
>> detail...

> I checked with the TCG's device driver work group (DDWG). Both the spec
> editor and 3 TPM vendors - Infineon, Nuvoton, and ST Micro - agreed that
> ignoring burst count may incur wait states but nothing more. Operations
> will still be successful.

Interesting - let me check with Georg tomorrow.
Unfortunately I do not have access to my tcg mails from home (since I'm not 
working :), 
but did you _explicitly_ talk about LPC and the system?
I'm sure the TPM does not care about the waitstates...

If my memory does not betray me, 
it is actually possible to "freeze up" a system completly by flooding the lpc 
bus.
Let me double check tomorrow...


In anycase - I really would like to see a much more performant tpm subsystem - 
however it will be quite an effort with a lot of legacy testing.
(which I unfortunately cannot spend on my private time ... and also of course 
lacking test systems).

Thanks,
Peter

Aw: Re: [tpmdd-devel] [PATCH] tpm: improve tpm_tis send() performance by ignoring burstcount

[Peter Huewe]

Hi Ken,

(speaking on behalf of myself here, not my employer :) )
> On Mon, Aug 07, 2017 at 01:52:34PM +0200, Peter Huewe wrote:
>> Imho: NACK from my side.
> After these viewpoints definitive NACK from my side too...


> I responded to the thread comments separately. However, assuming NACK
> is the final response, I have a question.

Nothing is ever final :)

> The problem is the 5 msec sleep between polls of burst count. In the
> case of one TPM with an 8 byte FIFO, a 32 byte transfer incurs 4 of
> these sleeps.

Yes that's bad, especially with current msleep(5) is actually msleep(20).
However, please also keep in mind SPI tpms show a much higher burst count 
value,  (255)
our I2C TPM SLB9645 even shows something in the range of 1k. :)


> Would another solution be to reduce the burst count poll and sleep to,
> e.g., 100 usec or even 10 usec? This would probably help greatly, but
> till not incur the wait states that triggered the NACK.

Imho the 5ms were an arbitrary chosen value for old old tpms.
Back then also only msleep was available which was msleep(20) anyway.


> My worry is that the scheduler would not be able to context switch that
> fast, and so we wouldn't actually see usec speed polling.
> Can a kernel expert offer an opinion?
If you use sleep it's not guaranteed that you wakeup after exactly xx specified 
amount of time.
Just that you sleep at least xx amount of time. Otherwise you would have to do 
delay/busywaiting.

https://www.kernel.org/doc/Documentation/timers/timers-howto.txt


Imho the best option is to
figure out whether any vendor can determine the "FIFO flush time" i.e. how much 
time does it take to empty the fifo and fillup the burstcount and use this on 
the lower bound of an usleep range.
I don't think tpms are in the range of < 10 us...

@Ken: Maybe can you check in DDWG?

What we often see/saw during some performance optimization tests is something 
like 
burstcount 7, burstcount 1, burstcount 7, burstcount 1...
which is the oposite of what you want to achieve.

You'd like to have something like
burstcount 8, burstcount 8, burstcount 8

Unfortunately TPMS don't report their max burstcount (afaik),
otherwise it would be easy to write a calibration routine for the poll times.


Thanks,
Peter

Aw: Re: [tpmdd-devel] [PATCH] tpm: improve tpm_tis send() performance by ignoring burstcount

[Peter Huewe]

Hi Ken,

(speaking on behalf of myself here, not my employer :) )
> On Mon, Aug 07, 2017 at 01:52:34PM +0200, Peter Huewe wrote:
>> Imho: NACK from my side.
> After these viewpoints definitive NACK from my side too...


> I responded to the thread comments separately. However, assuming NACK
> is the final response, I have a question.

Nothing is ever final :)

> The problem is the 5 msec sleep between polls of burst count. In the
> case of one TPM with an 8 byte FIFO, a 32 byte transfer incurs 4 of
> these sleeps.

Yes that's bad, especially with current msleep(5) is actually msleep(20).
However, please also keep in mind SPI tpms show a much higher burst count 
value,  (255)
our I2C TPM SLB9645 even shows something in the range of 1k. :)


> Would another solution be to reduce the burst count poll and sleep to,
> e.g., 100 usec or even 10 usec? This would probably help greatly, but
> till not incur the wait states that triggered the NACK.

Imho the 5ms were an arbitrary chosen value for old old tpms.
Back then also only msleep was available which was msleep(20) anyway.


> My worry is that the scheduler would not be able to context switch that
> fast, and so we wouldn't actually see usec speed polling.
> Can a kernel expert offer an opinion?
If you use sleep it's not guaranteed that you wakeup after exactly xx specified 
amount of time.
Just that you sleep at least xx amount of time. Otherwise you would have to do 
delay/busywaiting.

https://www.kernel.org/doc/Documentation/timers/timers-howto.txt


Imho the best option is to
figure out whether any vendor can determine the "FIFO flush time" i.e. how much 
time does it take to empty the fifo and fillup the burstcount and use this on 
the lower bound of an usleep range.
I don't think tpms are in the range of < 10 us...

@Ken: Maybe can you check in DDWG?

What we often see/saw during some performance optimization tests is something 
like 
burstcount 7, burstcount 1, burstcount 7, burstcount 1...
which is the oposite of what you want to achieve.

You'd like to have something like
burstcount 8, burstcount 8, burstcount 8

Unfortunately TPMS don't report their max burstcount (afaik),
otherwise it would be easy to write a calibration routine for the poll times.


Thanks,
Peter

Re: [PATCH] tpm: improve tpm_tis send() performance by ignoring burstcount

[Peter Huewe]

Am 7. August 2017 13:46:32 MESZ schrieb Nayna Jain :
>The TPM burstcount status indicates the number of bytes that can
>be sent to the TPM without causing bus wait states.  Effectively,
>it is the number of empty bytes in the command FIFO. Further,
>some TPMs have a static burstcount, when the value remains zero
>until the entire FIFO is empty.
>
>This patch ignores burstcount, permitting wait states, and thus
>writes the command as fast as the TPM can accept the bytes.
>The performance of a 34 byte extend on a TPM 1.2 improved from
>52 msec to 11 msec.
>
>Suggested-by: Ken Goldman  in
>conjunction with the TPM Device Driver work group.
>Signed-off-by: Nayna Jain 
>Acked-by: Mimi Zohar 

Are you sure this is a good idea?
On lpc systems this more or less stalls the bus, including keyboard/mouse (if 
connected via superio lpc).

On which systems have you tested this?
Spi/Lpc? Architecture?

This might not be noticable for small transfers, but think about much larger 
transfers

Imho: NACK from my side.

Thanks,
Peter

>---
>drivers/char/tpm/tpm_tis_core.c | 45
>++---
> 1 file changed, 2 insertions(+), 43 deletions(-)
>
>diff --git a/drivers/char/tpm/tpm_tis_core.c
>b/drivers/char/tpm/tpm_tis_core.c
>index b617b2eeb080..478cbc0f61c3 100644
>--- a/drivers/char/tpm/tpm_tis_core.c
>+++ b/drivers/char/tpm/tpm_tis_core.c
>@@ -255,9 +255,7 @@ static int tpm_tis_recv(struct tpm_chip *chip, u8
>*buf, size_t count)
>static int tpm_tis_send_data(struct tpm_chip *chip, u8 *buf, size_t
>len)
> {
>   struct tpm_tis_data *priv = dev_get_drvdata(>dev);
>-  int rc, status, burstcnt;
>-  size_t count = 0;
>-  bool itpm = priv->flags & TPM_TIS_ITPM_WORKAROUND;
>+  int rc, status;
> 
>   status = tpm_tis_status(chip);
>   if ((status & TPM_STS_COMMAND_READY) == 0) {
>@@ -270,49 +268,10 @@ static int tpm_tis_send_data(struct tpm_chip
>*chip, u8 *buf, size_t len)
>   }
>   }
> 
>-  while (count < len - 1) {
>-  burstcnt = get_burstcount(chip);
>-  if (burstcnt < 0) {
>-  dev_err(>dev, "Unable to read burstcount\n");
>-  rc = burstcnt;
>-  goto out_err;
>-  }
>-  burstcnt = min_t(int, burstcnt, len - count - 1);
>-  rc = tpm_tis_write_bytes(priv, TPM_DATA_FIFO(priv->locality),
>-   burstcnt, buf + count);
>-  if (rc < 0)
>-  goto out_err;
>-
>-  count += burstcnt;
>-
>-  if (wait_for_tpm_stat(chip, TPM_STS_VALID, chip->timeout_c,
>-  >int_queue, false) < 0) {
>-  rc = -ETIME;
>-  goto out_err;
>-  }
>-  status = tpm_tis_status(chip);
>-  if (!itpm && (status & TPM_STS_DATA_EXPECT) == 0) {
>-  rc = -EIO;
>-  goto out_err;
>-  }
>-  }
>-
>-  /* write last byte */
>-  rc = tpm_tis_write8(priv, TPM_DATA_FIFO(priv->locality), buf[count]);
>+  rc = tpm_tis_write_bytes(priv, TPM_DATA_FIFO(priv->locality), len,
>buf);
>   if (rc < 0)
>   goto out_err;
> 
>-  if (wait_for_tpm_stat(chip, TPM_STS_VALID, chip->timeout_c,
>-  >int_queue, false) < 0) {
>-  rc = -ETIME;
>-  goto out_err;
>-  }
>-  status = tpm_tis_status(chip);
>-  if (!itpm && (status & TPM_STS_DATA_EXPECT) != 0) {
>-  rc = -EIO;
>-  goto out_err;
>-  }
>-
>   return 0;
> 
> out_err:

-- 
Sent from my mobile

Re: [PATCH] tpm: improve tpm_tis send() performance by ignoring burstcount

[Peter Huewe]

Am 7. August 2017 13:46:32 MESZ schrieb Nayna Jain :
>The TPM burstcount status indicates the number of bytes that can
>be sent to the TPM without causing bus wait states.  Effectively,
>it is the number of empty bytes in the command FIFO. Further,
>some TPMs have a static burstcount, when the value remains zero
>until the entire FIFO is empty.
>
>This patch ignores burstcount, permitting wait states, and thus
>writes the command as fast as the TPM can accept the bytes.
>The performance of a 34 byte extend on a TPM 1.2 improved from
>52 msec to 11 msec.
>
>Suggested-by: Ken Goldman  in
>conjunction with the TPM Device Driver work group.
>Signed-off-by: Nayna Jain 
>Acked-by: Mimi Zohar 

Are you sure this is a good idea?
On lpc systems this more or less stalls the bus, including keyboard/mouse (if 
connected via superio lpc).

On which systems have you tested this?
Spi/Lpc? Architecture?

This might not be noticable for small transfers, but think about much larger 
transfers

Imho: NACK from my side.

Thanks,
Peter

>---
>drivers/char/tpm/tpm_tis_core.c | 45
>++---
> 1 file changed, 2 insertions(+), 43 deletions(-)
>
>diff --git a/drivers/char/tpm/tpm_tis_core.c
>b/drivers/char/tpm/tpm_tis_core.c
>index b617b2eeb080..478cbc0f61c3 100644
>--- a/drivers/char/tpm/tpm_tis_core.c
>+++ b/drivers/char/tpm/tpm_tis_core.c
>@@ -255,9 +255,7 @@ static int tpm_tis_recv(struct tpm_chip *chip, u8
>*buf, size_t count)
>static int tpm_tis_send_data(struct tpm_chip *chip, u8 *buf, size_t
>len)
> {
>   struct tpm_tis_data *priv = dev_get_drvdata(>dev);
>-  int rc, status, burstcnt;
>-  size_t count = 0;
>-  bool itpm = priv->flags & TPM_TIS_ITPM_WORKAROUND;
>+  int rc, status;
> 
>   status = tpm_tis_status(chip);
>   if ((status & TPM_STS_COMMAND_READY) == 0) {
>@@ -270,49 +268,10 @@ static int tpm_tis_send_data(struct tpm_chip
>*chip, u8 *buf, size_t len)
>   }
>   }
> 
>-  while (count < len - 1) {
>-  burstcnt = get_burstcount(chip);
>-  if (burstcnt < 0) {
>-  dev_err(>dev, "Unable to read burstcount\n");
>-  rc = burstcnt;
>-  goto out_err;
>-  }
>-  burstcnt = min_t(int, burstcnt, len - count - 1);
>-  rc = tpm_tis_write_bytes(priv, TPM_DATA_FIFO(priv->locality),
>-   burstcnt, buf + count);
>-  if (rc < 0)
>-  goto out_err;
>-
>-  count += burstcnt;
>-
>-  if (wait_for_tpm_stat(chip, TPM_STS_VALID, chip->timeout_c,
>-  >int_queue, false) < 0) {
>-  rc = -ETIME;
>-  goto out_err;
>-  }
>-  status = tpm_tis_status(chip);
>-  if (!itpm && (status & TPM_STS_DATA_EXPECT) == 0) {
>-  rc = -EIO;
>-  goto out_err;
>-  }
>-  }
>-
>-  /* write last byte */
>-  rc = tpm_tis_write8(priv, TPM_DATA_FIFO(priv->locality), buf[count]);
>+  rc = tpm_tis_write_bytes(priv, TPM_DATA_FIFO(priv->locality), len,
>buf);
>   if (rc < 0)
>   goto out_err;
> 
>-  if (wait_for_tpm_stat(chip, TPM_STS_VALID, chip->timeout_c,
>-  >int_queue, false) < 0) {
>-  rc = -ETIME;
>-  goto out_err;
>-  }
>-  status = tpm_tis_status(chip);
>-  if (!itpm && (status & TPM_STS_DATA_EXPECT) != 0) {
>-  rc = -EIO;
>-  goto out_err;
>-  }
>-
>   return 0;
> 
> out_err:

-- 
Sent from my mobile

[PATCH] tpm, tpmrm: Mark tpmrm_write as static

[Peter Huewe]

sparse complains that tpmrm_write can be made static, and since it is
right we make it static.

Signed-off-by: Peter Huewe <peterhu...@gmx.de>
---
 drivers/char/tpm/tpmrm-dev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/char/tpm/tpmrm-dev.c b/drivers/char/tpm/tpmrm-dev.c
index c636e7f..1a0e97a 100644
--- a/drivers/char/tpm/tpmrm-dev.c
+++ b/drivers/char/tpm/tpmrm-dev.c
@@ -45,7 +45,7 @@ static int tpmrm_release(struct inode *inode, struct file 
*file)
return 0;
 }
 
-ssize_t tpmrm_write(struct file *file, const char __user *buf,
+static ssize_t tpmrm_write(struct file *file, const char __user *buf,
   size_t size, loff_t *off)
 {
struct file_priv *fpriv = file->private_data;
-- 
2.10.2

[PATCH] tpm, tpmrm: Mark tpmrm_write as static

[Peter Huewe]

sparse complains that tpmrm_write can be made static, and since it is
right we make it static.

Signed-off-by: Peter Huewe 
---
 drivers/char/tpm/tpmrm-dev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/char/tpm/tpmrm-dev.c b/drivers/char/tpm/tpmrm-dev.c
index c636e7f..1a0e97a 100644
--- a/drivers/char/tpm/tpmrm-dev.c
+++ b/drivers/char/tpm/tpmrm-dev.c
@@ -45,7 +45,7 @@ static int tpmrm_release(struct inode *inode, struct file 
*file)
return 0;
 }
 
-ssize_t tpmrm_write(struct file *file, const char __user *buf,
+static ssize_t tpmrm_write(struct file *file, const char __user *buf,
   size_t size, loff_t *off)
 {
struct file_priv *fpriv = file->private_data;
-- 
2.10.2

Aw: Re: [PATCH] tpm: remove struct tpm_pcrextend_in

[Peter Huewe]

> Removed struct tpm_pcrextend_in as it is not used for anything anymore.
>
> Signed-off-by: Jarkko Sakkinen <jarkko.sakki...@linux.intel.com>
LGTM.
Reviewed-by: Peter Huewe <peterhu...@gmx.de>

> ---
> drivers/char/tpm/tpm.h | 6 --
> 1 file changed, 6 deletions(-)
>
> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> index dd1173427fb2..af05c1403c6e 100644
> --- a/drivers/char/tpm/tpm.h
> +++ b/drivers/char/tpm/tpm.h
> @@ -363,11 +363,6 @@ struct tpm_pcrread_in {
> __be32 pcr_idx;
> } __packed;
>
> -struct tpm_pcrextend_in {
> - __be32 pcr_idx;
> - u8 hash[TPM_DIGEST_SIZE];
> -} __packed;
> -
> /* 128 bytes is an arbitrary cap. This could be as large as TPM_BUFSIZE - 18
> * bytes, but 128 is still a relatively large number of random bytes and
> * anything much bigger causes users of struct tpm_cmd_t to start getting
> @@ -392,7 +387,6 @@ typedef union {
> u8 readpubek_out_buffer[sizeof(struct tpm_readpubek_params_out)];
> struct tpm_pcrread_in pcrread_in;
> struct tpm_pcrread_out pcrread_out;
> - struct tpm_pcrextend_in pcrextend_in;
> struct tpm_getrandom_in getrandom_in;
> struct tpm_getrandom_out getrandom_out;
> struct tpm_startup_in startup_in;
> --
> 2.11.0
>

Aw: Re: [PATCH] tpm: remove struct tpm_pcrextend_in

[Peter Huewe]

> Removed struct tpm_pcrextend_in as it is not used for anything anymore.
>
> Signed-off-by: Jarkko Sakkinen 
LGTM.
Reviewed-by: Peter Huewe 

> ---
> drivers/char/tpm/tpm.h | 6 --
> 1 file changed, 6 deletions(-)
>
> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> index dd1173427fb2..af05c1403c6e 100644
> --- a/drivers/char/tpm/tpm.h
> +++ b/drivers/char/tpm/tpm.h
> @@ -363,11 +363,6 @@ struct tpm_pcrread_in {
> __be32 pcr_idx;
> } __packed;
>
> -struct tpm_pcrextend_in {
> - __be32 pcr_idx;
> - u8 hash[TPM_DIGEST_SIZE];
> -} __packed;
> -
> /* 128 bytes is an arbitrary cap. This could be as large as TPM_BUFSIZE - 18
> * bytes, but 128 is still a relatively large number of random bytes and
> * anything much bigger causes users of struct tpm_cmd_t to start getting
> @@ -392,7 +387,6 @@ typedef union {
> u8 readpubek_out_buffer[sizeof(struct tpm_readpubek_params_out)];
> struct tpm_pcrread_in pcrread_in;
> struct tpm_pcrread_out pcrread_out;
> - struct tpm_pcrextend_in pcrextend_in;
> struct tpm_getrandom_in getrandom_in;
> struct tpm_getrandom_out getrandom_out;
> struct tpm_startup_in startup_in;
> --
> 2.11.0
>

Aw: Re: [PATCH] tpm, tpm_infineon: remove useless snprintf() calls

[Peter Huewe]

> The memory copy from rodata to stack is useless.
>
> Signed-off-by: Jarkko Sakkinen <jarkko.sakki...@linux.intel.com>

After review, yes that should work.
Reviewed-by: Peter Huewe <peterhu...@gmx.de>

> ---
> drivers/char/tpm/tpm_infineon.c | 8 
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/char/tpm/tpm_infineon.c b/drivers/char/tpm/tpm_infineon.c
> index e3cf9f3545c5..3b1b9f9322d5 100644
> --- a/drivers/char/tpm/tpm_infineon.c
> +++ b/drivers/char/tpm/tpm_infineon.c
> @@ -397,7 +397,7 @@ static int tpm_inf_pnp_probe(struct pnp_dev *dev,
> int vendorid[2];
> int version[2];
> int productid[2];
> - char chipname[20];
> + const char *chipname;
> struct tpm_chip *chip;
>
> /* read IO-ports through PnP */
> @@ -488,13 +488,13 @@ static int tpm_inf_pnp_probe(struct pnp_dev *dev,
>
> switch ((productid[0] << 8) | productid[1]) {
> case 6:
> - snprintf(chipname, sizeof(chipname), " (SLD 9630 TT 1.1)");
> + chipname = " (SLD 9630 TT 1.1)";
> break;
> case 11:
> - snprintf(chipname, sizeof(chipname), " (SLB 9635 TT 1.2)");
> + chipname = " (SLB 9635 TT 1.2)";
> break;
> default:
> - snprintf(chipname, sizeof(chipname), " (unknown chip)");
> + chipname = " (unknown chip)";
> break;
> }
>
> --
> 2.11.0
>

Aw: Re: [PATCH] tpm, tpm_infineon: remove useless snprintf() calls

[Peter Huewe]

> The memory copy from rodata to stack is useless.
>
> Signed-off-by: Jarkko Sakkinen 

After review, yes that should work.
Reviewed-by: Peter Huewe 

> ---
> drivers/char/tpm/tpm_infineon.c | 8 
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/char/tpm/tpm_infineon.c b/drivers/char/tpm/tpm_infineon.c
> index e3cf9f3545c5..3b1b9f9322d5 100644
> --- a/drivers/char/tpm/tpm_infineon.c
> +++ b/drivers/char/tpm/tpm_infineon.c
> @@ -397,7 +397,7 @@ static int tpm_inf_pnp_probe(struct pnp_dev *dev,
> int vendorid[2];
> int version[2];
> int productid[2];
> - char chipname[20];
> + const char *chipname;
> struct tpm_chip *chip;
>
> /* read IO-ports through PnP */
> @@ -488,13 +488,13 @@ static int tpm_inf_pnp_probe(struct pnp_dev *dev,
>
> switch ((productid[0] << 8) | productid[1]) {
> case 6:
> - snprintf(chipname, sizeof(chipname), " (SLD 9630 TT 1.1)");
> + chipname = " (SLD 9630 TT 1.1)";
> break;
> case 11:
> - snprintf(chipname, sizeof(chipname), " (SLB 9635 TT 1.2)");
> + chipname = " (SLB 9635 TT 1.2)";
> break;
> default:
> - snprintf(chipname, sizeof(chipname), " (unknown chip)");
> + chipname = " (unknown chip)";
> break;
> }
>
> --
> 2.11.0
>

Re: [1/5] w83627ehf: Use hwmon_device_register_with_info and sensor groups

[Peter Huewe]

Am 17. Mai 2017 21:35:52 MESZ schrieb Alexander Sverdlin 
<alexander.sverd...@gmail.com>:
>Hello Peter!
>
>On 23/03/17 14:03, Peter Huewe wrote:
>> This patch replaces the old, deprecated call to hwmon_device_register
>> with the new hwmon_device_register_with_info and converts the whole
>> driver to the new hwmon interface using the hwmon_chip_info methods
>> and the attribute_group method.
>> 
>> All standard attributes were converted to the corresponding
>> hwmon_chip_info methods.
>> For some functions a hwmon channel to device channel conversion had
>to
>> be performed, e.g. hwmon_in_alarm has the info for alert_5 in channel
>8.
>> 
>> All non-standard attributes are converted to the attribute_group
>method,
>> by
>> - adding them statically to the attribute_group if they are available
>> for all variants of devices supported by this driver
>> - adding them at probe time to the attribute_group if the
>availability
>> is depending on the actual chip type.
>> The appropriate count of entries was reserved.
>> 
>> As a pre-condition a reference to the sio_data structure was moved
>into
>> w83627ehf_data for easier retrieval of the information, since this is
>> much easier than trying to access the platform_data.
>> 
>> The driver is now much more "checkpatch clean" than it used to be,
>but
>> still not completely.
>> The conversion saves about 20k in the resulting .ko
>> 
>> Tested with a NCT6776F chip.
>> 
>> v2:
>> - converted to proper is_visible methods
>> - applied minor feedback of v1
>> 
>> Signed-off-by: Peter Huewe <peterhu...@gmx.de>
>
>I've applied the whole series (5 of 5 patches) on top of hwmon-next
>and tested on Intel D525MW, where the module finds "W83627DHG-P chip at
>0x290".
>
>The values of all the inputs are exactly the same as back in Linux
>v4.7.x
>(when the used HWMON API wasn't deprecated).
>
>Therefore, feel free to add
>
>Tested-by: Alexander Sverdlin <alexander.sverd...@gmail.com>
>
>to the whole series when you re-spin.
>Thanks for fixing this!


Thanks for testing!
I will respin as soon as I find time.

Thanks
Peter
>
>> ---
>>  drivers/hwmon/w83627ehf.c | 1536
>+++--
>>  1 file changed, 778 insertions(+), 758 deletions(-)
>
>--
>Alexander.

-- 
Sent from my mobile

Re: [1/5] w83627ehf: Use hwmon_device_register_with_info and sensor groups

[Peter Huewe]

Am 17. Mai 2017 21:35:52 MESZ schrieb Alexander Sverdlin 
:
>Hello Peter!
>
>On 23/03/17 14:03, Peter Huewe wrote:
>> This patch replaces the old, deprecated call to hwmon_device_register
>> with the new hwmon_device_register_with_info and converts the whole
>> driver to the new hwmon interface using the hwmon_chip_info methods
>> and the attribute_group method.
>> 
>> All standard attributes were converted to the corresponding
>> hwmon_chip_info methods.
>> For some functions a hwmon channel to device channel conversion had
>to
>> be performed, e.g. hwmon_in_alarm has the info for alert_5 in channel
>8.
>> 
>> All non-standard attributes are converted to the attribute_group
>method,
>> by
>> - adding them statically to the attribute_group if they are available
>> for all variants of devices supported by this driver
>> - adding them at probe time to the attribute_group if the
>availability
>> is depending on the actual chip type.
>> The appropriate count of entries was reserved.
>> 
>> As a pre-condition a reference to the sio_data structure was moved
>into
>> w83627ehf_data for easier retrieval of the information, since this is
>> much easier than trying to access the platform_data.
>> 
>> The driver is now much more "checkpatch clean" than it used to be,
>but
>> still not completely.
>> The conversion saves about 20k in the resulting .ko
>> 
>> Tested with a NCT6776F chip.
>> 
>> v2:
>> - converted to proper is_visible methods
>> - applied minor feedback of v1
>> 
>> Signed-off-by: Peter Huewe 
>
>I've applied the whole series (5 of 5 patches) on top of hwmon-next
>and tested on Intel D525MW, where the module finds "W83627DHG-P chip at
>0x290".
>
>The values of all the inputs are exactly the same as back in Linux
>v4.7.x
>(when the used HWMON API wasn't deprecated).
>
>Therefore, feel free to add
>
>Tested-by: Alexander Sverdlin 
>
>to the whole series when you re-spin.
>Thanks for fixing this!


Thanks for testing!
I will respin as soon as I find time.

Thanks
Peter
>
>> ---
>>  drivers/hwmon/w83627ehf.c | 1536
>+++--
>>  1 file changed, 778 insertions(+), 758 deletions(-)
>
>--
>Alexander.

-- 
Sent from my mobile

Re: [PATCH v3] tpm: Apply a sane minimum adapterlimit value for retransmission.

[Peter Huewe]

Am 5. April 2017 11:03:27 MESZ schrieb Jarkko Sakkinen 
<jarkko.sakki...@linux.intel.com>:
>On Tue, Mar 28, 2017 at 05:29:38PM +0200, Enric Balletbo i Serra wrote:
>> From: Bryan Freed <bfr...@chromium.org>
>> 
>> When the I2C Infineon part is attached to an I2C adapter that imposes
>> a size limitation, large requests will fail with -EOPNOTSUPP. Retry
>> them with a sane minimum size without re-issuing the 0x05 command
>> as this appears to occasionally put the TPM in a bad state.
>> 
>> Signed-off-by: Bryan Freed <bfr...@chromium.org>
>> [rework the patch to adapt to the feedback received]
>> Signed-off-by: Enric Balletbo i Serra <enric.balle...@collabora.com>
>> ---
>> This is a reworked version of the original patch based on the
>> suggestion made by Wolfram Sang to simply fall back to a sane minimum
>> when the maximum fails.
>> 
>> Changes since v2:
>>  - Do not remove faster transfers when chip is SLB9645 (Peter Huewe)
>>  - Remember the adapterlimit length once it fails to not generate
>extra
>>i2c core messages (suggested by Andrew Lunn)
>> Changes since v1:
>>  - Check the correct return value (-EOPNOTSUPP instead of -EINVAL)
>>  - Fall back len to I2C_SMBUS_BLOCK_MAX if fails.
>> 
>>  drivers/char/tpm/tpm_i2c_infineon.c | 76
>+++--
>>  1 file changed, 56 insertions(+), 20 deletions(-)
>> 
>> diff --git a/drivers/char/tpm/tpm_i2c_infineon.c
>b/drivers/char/tpm/tpm_i2c_infineon.c
>> index 62ee44e..fdefcdb 100644
>> --- a/drivers/char/tpm/tpm_i2c_infineon.c
>> +++ b/drivers/char/tpm/tpm_i2c_infineon.c
>> @@ -70,6 +70,7 @@ struct tpm_inf_dev {
>>  u8 buf[TPM_BUFSIZE + sizeof(u8)]; /* max. buffer size + addr */
>>  struct tpm_chip *chip;
>>  enum i2c_chip_type chip_type;
>> +unsigned int adapterlimit;
>>  };
>>  
>>  static struct tpm_inf_dev tpm_dev;
>> @@ -111,6 +112,7 @@ static int iic_tpm_read(u8 addr, u8 *buffer,
>size_t len)
>>  
>>  int rc = 0;
>>  int count;
>> +unsigned int msglen = len;
>>  
>>  /* Lock the adapter for the duration of the whole sequence. */
>>  if (!tpm_dev.client->adapter->algo->master_xfer)
>> @@ -131,27 +133,61 @@ static int iic_tpm_read(u8 addr, u8 *buffer,
>size_t len)
>>  usleep_range(SLEEP_DURATION_LOW, SLEEP_DURATION_HI);
>>  }
>>  } else {
>> -/* slb9635 protocol should work in all cases */
>> -for (count = 0; count < MAX_COUNT; count++) {
>> -rc = __i2c_transfer(tpm_dev.client->adapter, , 1);
>> -if (rc > 0)
>> -break;  /* break here to skip sleep */
>> -
>> -usleep_range(SLEEP_DURATION_LOW, SLEEP_DURATION_HI);
>> -}
>> -
>> -if (rc <= 0)
>> -goto out;
>> -
>> -/* After the TPM has successfully received the register address
>> - * it needs some time, thus we're sleeping here again, before
>> - * retrieving the data
>> +/* Expect to send one command message and one data message, but
>> + * support looping over each or both if necessary.
>>   */
>> -for (count = 0; count < MAX_COUNT; count++) {
>> -usleep_range(SLEEP_DURATION_LOW, SLEEP_DURATION_HI);
>> -rc = __i2c_transfer(tpm_dev.client->adapter, , 1);
>> -if (rc > 0)
>> -break;
>> +while (len > 0) {
>> +/* slb9635 protocol should work in all cases */
>> +for (count = 0; count < MAX_COUNT; count++) {
>> +rc = __i2c_transfer(tpm_dev.client->adapter,
>> +, 1);
>> +if (rc > 0)
>> +break;  /* break here to skip sleep */
>> +
>> +usleep_range(SLEEP_DURATION_LOW,
>> + SLEEP_DURATION_HI);
>> +}
>> +
>> +if (rc <= 0)
>> +goto out;
>> +
>> +/* After the TPM has successfully received the register
>> + * address it needs some time, thus we're sleeping here
>> + * again, before retrievin

Re: [PATCH v3] tpm: Apply a sane minimum adapterlimit value for retransmission.

[Peter Huewe]

Am 5. April 2017 11:03:27 MESZ schrieb Jarkko Sakkinen 
:
>On Tue, Mar 28, 2017 at 05:29:38PM +0200, Enric Balletbo i Serra wrote:
>> From: Bryan Freed 
>> 
>> When the I2C Infineon part is attached to an I2C adapter that imposes
>> a size limitation, large requests will fail with -EOPNOTSUPP. Retry
>> them with a sane minimum size without re-issuing the 0x05 command
>> as this appears to occasionally put the TPM in a bad state.
>> 
>> Signed-off-by: Bryan Freed 
>> [rework the patch to adapt to the feedback received]
>> Signed-off-by: Enric Balletbo i Serra 
>> ---
>> This is a reworked version of the original patch based on the
>> suggestion made by Wolfram Sang to simply fall back to a sane minimum
>> when the maximum fails.
>> 
>> Changes since v2:
>>  - Do not remove faster transfers when chip is SLB9645 (Peter Huewe)
>>  - Remember the adapterlimit length once it fails to not generate
>extra
>>i2c core messages (suggested by Andrew Lunn)
>> Changes since v1:
>>  - Check the correct return value (-EOPNOTSUPP instead of -EINVAL)
>>  - Fall back len to I2C_SMBUS_BLOCK_MAX if fails.
>> 
>>  drivers/char/tpm/tpm_i2c_infineon.c | 76
>+++--
>>  1 file changed, 56 insertions(+), 20 deletions(-)
>> 
>> diff --git a/drivers/char/tpm/tpm_i2c_infineon.c
>b/drivers/char/tpm/tpm_i2c_infineon.c
>> index 62ee44e..fdefcdb 100644
>> --- a/drivers/char/tpm/tpm_i2c_infineon.c
>> +++ b/drivers/char/tpm/tpm_i2c_infineon.c
>> @@ -70,6 +70,7 @@ struct tpm_inf_dev {
>>  u8 buf[TPM_BUFSIZE + sizeof(u8)]; /* max. buffer size + addr */
>>  struct tpm_chip *chip;
>>  enum i2c_chip_type chip_type;
>> +unsigned int adapterlimit;
>>  };
>>  
>>  static struct tpm_inf_dev tpm_dev;
>> @@ -111,6 +112,7 @@ static int iic_tpm_read(u8 addr, u8 *buffer,
>size_t len)
>>  
>>  int rc = 0;
>>  int count;
>> +unsigned int msglen = len;
>>  
>>  /* Lock the adapter for the duration of the whole sequence. */
>>  if (!tpm_dev.client->adapter->algo->master_xfer)
>> @@ -131,27 +133,61 @@ static int iic_tpm_read(u8 addr, u8 *buffer,
>size_t len)
>>  usleep_range(SLEEP_DURATION_LOW, SLEEP_DURATION_HI);
>>  }
>>  } else {
>> -/* slb9635 protocol should work in all cases */
>> -for (count = 0; count < MAX_COUNT; count++) {
>> -rc = __i2c_transfer(tpm_dev.client->adapter, , 1);
>> -if (rc > 0)
>> -break;  /* break here to skip sleep */
>> -
>> -usleep_range(SLEEP_DURATION_LOW, SLEEP_DURATION_HI);
>> -}
>> -
>> -if (rc <= 0)
>> -goto out;
>> -
>> -/* After the TPM has successfully received the register address
>> - * it needs some time, thus we're sleeping here again, before
>> - * retrieving the data
>> +/* Expect to send one command message and one data message, but
>> + * support looping over each or both if necessary.
>>   */
>> -for (count = 0; count < MAX_COUNT; count++) {
>> -usleep_range(SLEEP_DURATION_LOW, SLEEP_DURATION_HI);
>> -rc = __i2c_transfer(tpm_dev.client->adapter, , 1);
>> -if (rc > 0)
>> -break;
>> +while (len > 0) {
>> +/* slb9635 protocol should work in all cases */
>> +for (count = 0; count < MAX_COUNT; count++) {
>> +rc = __i2c_transfer(tpm_dev.client->adapter,
>> +, 1);
>> +if (rc > 0)
>> +break;  /* break here to skip sleep */
>> +
>> +usleep_range(SLEEP_DURATION_LOW,
>> + SLEEP_DURATION_HI);
>> +}
>> +
>> +if (rc <= 0)
>> +goto out;
>> +
>> +/* After the TPM has successfully received the register
>> + * address it needs some time, thus we're sleeping here
>> + * again, before retrieving the data
>> + */
>> +for (count = 0; count < MAX_COUNT; count++) 

Re: [PATCH 1/5] w83627ehf: Use hwmon_device_register_with_info and sensor groups

[Peter Huewe]

Am 23. März 2017 19:43:39 MEZ schrieb Guenter Roeck <li...@roeck-us.net>:
>On Thu, Mar 23, 2017 at 02:03:04PM +0100, Peter Huewe wrote:
>> This patch replaces the old, deprecated call to hwmon_device_register
>> with the new hwmon_device_register_with_info and converts the whole
>> driver to the new hwmon interface using the hwmon_chip_info methods
>> and the attribute_group method.
>> 
>> All standard attributes were converted to the corresponding
>> hwmon_chip_info methods.
>> For some functions a hwmon channel to device channel conversion had
>to
>> be performed, e.g. hwmon_in_alarm has the info for alert_5 in channel
>8.
>> 
>> All non-standard attributes are converted to the attribute_group
>method,
>> by
>> - adding them statically to the attribute_group if they are available
>> for all variants of devices supported by this driver
>> - adding them at probe time to the attribute_group if the
>availability
>> is depending on the actual chip type.
>> The appropriate count of entries was reserved.
>> 
>> As a pre-condition a reference to the sio_data structure was moved
>into
>> w83627ehf_data for easier retrieval of the information, since this is
>> much easier than trying to access the platform_data.
>> 
>> The driver is now much more "checkpatch clean" than it used to be,
>but
>> still not completely.
>> The conversion saves about 20k in the resulting .ko
>> 
>> Tested with a NCT6776F chip.
>> 
>> v2:
>> - converted to proper is_visible methods
>> - applied minor feedback of v1
>> 
>Nicely done.
>
>Couple of nitpicks:
>- Please no double empty lines (removed some, added some ;-)
Okay, interestingly checkpatch did not complain.

>- still a couple of c99 comments
I thought I had them removed, or atleast not introduced new ones

>- (later or intro patch): dem_request_region() in probe could help
> with cleanup, you could use the devm_ function for hwmon registration,
>  and drop the remove function entirely.
Yea makes sense, already thought about it, but then decided against it for the 
first comversion.

>- if () return; else return; causes static analyzer hiccup.
Okay, which analyzer has trouble with that?
I ran Wolfram's ninja-check against it (sparse, smatch, coccinelle etc...) and 
didn't see any issues.
>  Please use if () return; return; (no else)
>  You do that already most of the time but not always.

Shall I respin one more time?
Or fixup in seperate patches?


>
>Thanks,
>Guenter
>
>> Signed-off-by: Peter Huewe <peterhu...@gmx.de>
>> ---
>>  drivers/hwmon/w83627ehf.c | 1536
>+++--
>>  1 file changed, 778 insertions(+), 758 deletions(-)
>> 
>> diff --git a/drivers/hwmon/w83627ehf.c b/drivers/hwmon/w83627ehf.c
>> index ad68b6d9ff17..f2f33dd35fd4 100644
>> --- a/drivers/hwmon/w83627ehf.c
>> +++ b/drivers/hwmon/w83627ehf.c
>> @@ -1,6 +1,7 @@
>>  /*
>>   *  w83627ehf - Driver for the hardware monitoring functionality of
>>   *  the Winbond W83627EHF Super-I/O chip
>> + *  Copyright (C) 2017  Peter Huewe <peterhu...@gmx.de>
>>   *  Copyright (C) 2005-2012  Jean Delvare <jdelv...@suse.de>
>>   *  Copyright (C) 2006  Yuan Mu (Winbond),
>>   *  Rudolf Marek <r.ma...@assembler.cz>
>> @@ -426,6 +427,11 @@ static inline u8 in_to_reg(u32 val, u8 nr, const
>u16 *scale_in)
>>  /*
>>   * Data structures and manipulation thereof
>>   */
>> +struct w83627ehf_sio_data {
>> +int sioreg;
>> +enum kinds kind;
>> +};
>> +
>>  
>>  struct w83627ehf_data {
>>  int addr;   /* IO base of hw monitor block */
>> @@ -514,11 +520,7 @@ struct w83627ehf_data {
>>  u8 fandiv1;
>>  u8 fandiv2;
>>  #endif
>> -};
>> -
>> -struct w83627ehf_sio_data {
>> -int sioreg;
>> -enum kinds kind;
>> +struct w83627ehf_sio_data *sio_data;
>>  };
>>  
>>  /*
>> @@ -679,7 +681,7 @@ static void w83627ehf_write_fan_div(struct
>w83627ehf_data *data, int nr)
>>  static void w83627ehf_write_fan_div_common(struct device *dev,
>> struct w83627ehf_data *data, int nr)
>>  {
>> -struct w83627ehf_sio_data *sio_data = dev_get_platdata(dev);
>> +struct w83627ehf_sio_data *sio_data = data->sio_data;
>>  
>>  if (sio_data->kind == nct6776)
>>  ; /* no dividers, do nothing */
>> @@ -730,11 +732,9 @@ static void w83627ehf_update_fan_div(struct
>w83627ehf_data *data)
>

Re: [PATCH 1/5] w83627ehf: Use hwmon_device_register_with_info and sensor groups

[Peter Huewe]

Am 23. März 2017 19:43:39 MEZ schrieb Guenter Roeck :
>On Thu, Mar 23, 2017 at 02:03:04PM +0100, Peter Huewe wrote:
>> This patch replaces the old, deprecated call to hwmon_device_register
>> with the new hwmon_device_register_with_info and converts the whole
>> driver to the new hwmon interface using the hwmon_chip_info methods
>> and the attribute_group method.
>> 
>> All standard attributes were converted to the corresponding
>> hwmon_chip_info methods.
>> For some functions a hwmon channel to device channel conversion had
>to
>> be performed, e.g. hwmon_in_alarm has the info for alert_5 in channel
>8.
>> 
>> All non-standard attributes are converted to the attribute_group
>method,
>> by
>> - adding them statically to the attribute_group if they are available
>> for all variants of devices supported by this driver
>> - adding them at probe time to the attribute_group if the
>availability
>> is depending on the actual chip type.
>> The appropriate count of entries was reserved.
>> 
>> As a pre-condition a reference to the sio_data structure was moved
>into
>> w83627ehf_data for easier retrieval of the information, since this is
>> much easier than trying to access the platform_data.
>> 
>> The driver is now much more "checkpatch clean" than it used to be,
>but
>> still not completely.
>> The conversion saves about 20k in the resulting .ko
>> 
>> Tested with a NCT6776F chip.
>> 
>> v2:
>> - converted to proper is_visible methods
>> - applied minor feedback of v1
>> 
>Nicely done.
>
>Couple of nitpicks:
>- Please no double empty lines (removed some, added some ;-)
Okay, interestingly checkpatch did not complain.

>- still a couple of c99 comments
I thought I had them removed, or atleast not introduced new ones

>- (later or intro patch): dem_request_region() in probe could help
> with cleanup, you could use the devm_ function for hwmon registration,
>  and drop the remove function entirely.
Yea makes sense, already thought about it, but then decided against it for the 
first comversion.

>- if () return; else return; causes static analyzer hiccup.
Okay, which analyzer has trouble with that?
I ran Wolfram's ninja-check against it (sparse, smatch, coccinelle etc...) and 
didn't see any issues.
>  Please use if () return; return; (no else)
>  You do that already most of the time but not always.

Shall I respin one more time?
Or fixup in seperate patches?


>
>Thanks,
>Guenter
>
>> Signed-off-by: Peter Huewe 
>> ---
>>  drivers/hwmon/w83627ehf.c | 1536
>+++--
>>  1 file changed, 778 insertions(+), 758 deletions(-)
>> 
>> diff --git a/drivers/hwmon/w83627ehf.c b/drivers/hwmon/w83627ehf.c
>> index ad68b6d9ff17..f2f33dd35fd4 100644
>> --- a/drivers/hwmon/w83627ehf.c
>> +++ b/drivers/hwmon/w83627ehf.c
>> @@ -1,6 +1,7 @@
>>  /*
>>   *  w83627ehf - Driver for the hardware monitoring functionality of
>>   *  the Winbond W83627EHF Super-I/O chip
>> + *  Copyright (C) 2017  Peter Huewe 
>>   *  Copyright (C) 2005-2012  Jean Delvare 
>>   *  Copyright (C) 2006  Yuan Mu (Winbond),
>>   *  Rudolf Marek 
>> @@ -426,6 +427,11 @@ static inline u8 in_to_reg(u32 val, u8 nr, const
>u16 *scale_in)
>>  /*
>>   * Data structures and manipulation thereof
>>   */
>> +struct w83627ehf_sio_data {
>> +int sioreg;
>> +enum kinds kind;
>> +};
>> +
>>  
>>  struct w83627ehf_data {
>>  int addr;   /* IO base of hw monitor block */
>> @@ -514,11 +520,7 @@ struct w83627ehf_data {
>>  u8 fandiv1;
>>  u8 fandiv2;
>>  #endif
>> -};
>> -
>> -struct w83627ehf_sio_data {
>> -int sioreg;
>> -enum kinds kind;
>> +struct w83627ehf_sio_data *sio_data;
>>  };
>>  
>>  /*
>> @@ -679,7 +681,7 @@ static void w83627ehf_write_fan_div(struct
>w83627ehf_data *data, int nr)
>>  static void w83627ehf_write_fan_div_common(struct device *dev,
>> struct w83627ehf_data *data, int nr)
>>  {
>> -struct w83627ehf_sio_data *sio_data = dev_get_platdata(dev);
>> +struct w83627ehf_sio_data *sio_data = data->sio_data;
>>  
>>  if (sio_data->kind == nct6776)
>>  ; /* no dividers, do nothing */
>> @@ -730,11 +732,9 @@ static void w83627ehf_update_fan_div(struct
>w83627ehf_data *data)
>>  static void w83627ehf_update_fan_div_common(struct device *dev,
>>  struct

[PATCH 4/5] w83627ehf: Drop FSFE template and replace with SPDX License information

[Peter Huewe]

As indicated by checkpatch it makes sense to not use the FSFE Template
about GPLv2+

Signed-off-by: Peter Huewe <peterhu...@gmx.de>
---
 drivers/hwmon/w83627ehf.c | 15 ++-
 1 file changed, 2 insertions(+), 13 deletions(-)

diff --git a/drivers/hwmon/w83627ehf.c b/drivers/hwmon/w83627ehf.c
index a6f56b12727b..c4b755cd506c 100644
--- a/drivers/hwmon/w83627ehf.c
+++ b/drivers/hwmon/w83627ehf.c
@@ -18,19 +18,8 @@
  *  This driver also supports the W83627EHG, which is the lead-free
  *  version of the W83627EHF.
  *
- *  This program is free software; you can redistribute it and/or modify
- *  it under the terms of the GNU General Public License as published by
- *  the Free Software Foundation; either version 2 of the License, or
- *  (at your option) any later version.
- *
- *  This program is distributed in the hope that it will be useful,
- *  but WITHOUT ANY WARRANTY; without even the implied warranty of
- *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- *  GNU General Public License for more details.
- *
- *  You should have received a copy of the GNU General Public License
- *  along with this program; if not, write to the Free Software
- *  Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+ *  Released under the GPLv2 or later.
+ *  SPDX-License-Identifier: GPL-2.0+
  *
  *  Supports the following chips:
  *
-- 
2.10.2

[PATCH 4/5] w83627ehf: Drop FSFE template and replace with SPDX License information

[Peter Huewe]

As indicated by checkpatch it makes sense to not use the FSFE Template
about GPLv2+

Signed-off-by: Peter Huewe 
---
 drivers/hwmon/w83627ehf.c | 15 ++-
 1 file changed, 2 insertions(+), 13 deletions(-)

diff --git a/drivers/hwmon/w83627ehf.c b/drivers/hwmon/w83627ehf.c
index a6f56b12727b..c4b755cd506c 100644
--- a/drivers/hwmon/w83627ehf.c
+++ b/drivers/hwmon/w83627ehf.c
@@ -18,19 +18,8 @@
  *  This driver also supports the W83627EHG, which is the lead-free
  *  version of the W83627EHF.
  *
- *  This program is free software; you can redistribute it and/or modify
- *  it under the terms of the GNU General Public License as published by
- *  the Free Software Foundation; either version 2 of the License, or
- *  (at your option) any later version.
- *
- *  This program is distributed in the hope that it will be useful,
- *  but WITHOUT ANY WARRANTY; without even the implied warranty of
- *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- *  GNU General Public License for more details.
- *
- *  You should have received a copy of the GNU General Public License
- *  along with this program; if not, write to the Free Software
- *  Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+ *  Released under the GPLv2 or later.
+ *  SPDX-License-Identifier: GPL-2.0+
  *
  *  Supports the following chips:
  *
-- 
2.10.2

[PATCH 2/5] w83627ehf: Use octal values for access rights of sysfs files

[Peter Huewe]

As indicated by checkpatch, use the octal representation for the access
rights.

S_IWUSR | S_IRUGO => 0644
S_IRUGO => 0444

Signed-off-by: Peter Huewe <peterhu...@gmx.de>
---
 drivers/hwmon/w83627ehf.c | 52 ---
 1 file changed, 22 insertions(+), 30 deletions(-)

diff --git a/drivers/hwmon/w83627ehf.c b/drivers/hwmon/w83627ehf.c
index f2f33dd35fd4..0acadeece509 100644
--- a/drivers/hwmon/w83627ehf.c
+++ b/drivers/hwmon/w83627ehf.c
@@ -1098,25 +1098,17 @@ store_tolerance(struct device *dev, struct 
device_attribute *attr,
 }
 
 static struct sensor_device_attribute sda_target_temp[] = {
-   SENSOR_ATTR(pwm1_target, S_IWUSR | S_IRUGO, show_target_temp,
-   store_target_temp, 0),
-   SENSOR_ATTR(pwm2_target, S_IWUSR | S_IRUGO, show_target_temp,
-   store_target_temp, 1),
-   SENSOR_ATTR(pwm3_target, S_IWUSR | S_IRUGO, show_target_temp,
-   store_target_temp, 2),
-   SENSOR_ATTR(pwm4_target, S_IWUSR | S_IRUGO, show_target_temp,
-   store_target_temp, 3),
+   SENSOR_ATTR(pwm1_target, 0644, show_target_temp, store_target_temp, 0),
+   SENSOR_ATTR(pwm2_target, 0644, show_target_temp, store_target_temp, 1),
+   SENSOR_ATTR(pwm3_target, 0644, show_target_temp, store_target_temp, 2),
+   SENSOR_ATTR(pwm4_target, 0644, show_target_temp, store_target_temp, 3),
 };
 
 static struct sensor_device_attribute sda_tolerance[] = {
-   SENSOR_ATTR(pwm1_tolerance, S_IWUSR | S_IRUGO, show_tolerance,
-   store_tolerance, 0),
-   SENSOR_ATTR(pwm2_tolerance, S_IWUSR | S_IRUGO, show_tolerance,
-   store_tolerance, 1),
-   SENSOR_ATTR(pwm3_tolerance, S_IWUSR | S_IRUGO, show_tolerance,
-   store_tolerance, 2),
-   SENSOR_ATTR(pwm4_tolerance, S_IWUSR | S_IRUGO, show_tolerance,
-   store_tolerance, 3),
+   SENSOR_ATTR(pwm1_tolerance, 0644, show_tolerance, store_tolerance, 0),
+   SENSOR_ATTR(pwm2_tolerance, 0644, show_tolerance, store_tolerance, 1),
+   SENSOR_ATTR(pwm3_tolerance, 0644, show_tolerance, store_tolerance, 2),
+   SENSOR_ATTR(pwm4_tolerance, 0644, show_tolerance, store_tolerance, 3),
 };
 
 /* Smart Fan registers */
@@ -1194,24 +1186,24 @@ store_##reg(struct device *dev, struct device_attribute 
*attr, \
 fan_time_functions(fan_stop_time, FAN_STOP_TIME)
 
 static struct sensor_device_attribute sda_sf3_arrays_fan4[] = {
-   SENSOR_ATTR(pwm4_stop_time, S_IWUSR | S_IRUGO, show_fan_stop_time,
+   SENSOR_ATTR(pwm4_stop_time, 0644, show_fan_stop_time,
store_fan_stop_time, 3),
-   SENSOR_ATTR(pwm4_start_output, S_IWUSR | S_IRUGO, show_fan_start_output,
+   SENSOR_ATTR(pwm4_start_output, 0644, show_fan_start_output,
store_fan_start_output, 3),
-   SENSOR_ATTR(pwm4_stop_output, S_IWUSR | S_IRUGO, show_fan_stop_output,
+   SENSOR_ATTR(pwm4_stop_output, 0644, show_fan_stop_output,
store_fan_stop_output, 3),
-   SENSOR_ATTR(pwm4_max_output, S_IWUSR | S_IRUGO, show_fan_max_output,
+   SENSOR_ATTR(pwm4_max_output, 0644, show_fan_max_output,
store_fan_max_output, 3),
-   SENSOR_ATTR(pwm4_step_output, S_IWUSR | S_IRUGO, show_fan_step_output,
+   SENSOR_ATTR(pwm4_step_output, 0644, show_fan_step_output,
store_fan_step_output, 3),
 };
 
 static struct sensor_device_attribute sda_sf3_arrays_fan3[] = {
-   SENSOR_ATTR(pwm3_stop_time, S_IWUSR | S_IRUGO, show_fan_stop_time,
+   SENSOR_ATTR(pwm3_stop_time, 0644, show_fan_stop_time,
store_fan_stop_time, 2),
-   SENSOR_ATTR(pwm3_start_output, S_IWUSR | S_IRUGO, show_fan_start_output,
+   SENSOR_ATTR(pwm3_start_output, 0644, show_fan_start_output,
store_fan_start_output, 2),
-   SENSOR_ATTR(pwm3_stop_output, S_IWUSR | S_IRUGO, show_fan_stop_output,
+   SENSOR_ATTR(pwm3_stop_output, 0644, show_fan_stop_output,
store_fan_stop_output, 2),
 };
 
@@ -1233,17 +1225,17 @@ static SENSOR_DEVICE_ATTR(pwm2_stop_output, 0644, 
show_fan_stop_output,
  * Need to check support while generating/removing attribute files.
  */
 static struct sensor_device_attribute sda_sf3_max_step_arrays[] = {
-   SENSOR_ATTR(pwm1_max_output, S_IWUSR | S_IRUGO, show_fan_max_output,
+   SENSOR_ATTR(pwm1_max_output, 0644, show_fan_max_output,
store_fan_max_output, 0),
-   SENSOR_ATTR(pwm1_step_output, S_IWUSR | S_IRUGO, show_fan_step_output,
+   SENSOR_ATTR(pwm1_step_output, 0644, show_fan_step_output,
store_fan_step_output, 0),
-   SENSOR_ATTR(pwm2_max_output, S_IWUSR | S_IRUGO, show_fan_max_output,
+   SENSOR_ATTR(pwm2_max_output, 0644, show_fan_max_output,
store_fan_max_output, 1),
-   SENSOR_ATTR(pwm2_step_output, S_IWUSR | S_IRUGO, show_fan_step_output,
+   SENSOR_ATTR(pwm2

[PATCH 2/5] w83627ehf: Use octal values for access rights of sysfs files

[Peter Huewe]

As indicated by checkpatch, use the octal representation for the access
rights.

S_IWUSR | S_IRUGO => 0644
S_IRUGO => 0444

Signed-off-by: Peter Huewe 
---
 drivers/hwmon/w83627ehf.c | 52 ---
 1 file changed, 22 insertions(+), 30 deletions(-)

diff --git a/drivers/hwmon/w83627ehf.c b/drivers/hwmon/w83627ehf.c
index f2f33dd35fd4..0acadeece509 100644
--- a/drivers/hwmon/w83627ehf.c
+++ b/drivers/hwmon/w83627ehf.c
@@ -1098,25 +1098,17 @@ store_tolerance(struct device *dev, struct 
device_attribute *attr,
 }
 
 static struct sensor_device_attribute sda_target_temp[] = {
-   SENSOR_ATTR(pwm1_target, S_IWUSR | S_IRUGO, show_target_temp,
-   store_target_temp, 0),
-   SENSOR_ATTR(pwm2_target, S_IWUSR | S_IRUGO, show_target_temp,
-   store_target_temp, 1),
-   SENSOR_ATTR(pwm3_target, S_IWUSR | S_IRUGO, show_target_temp,
-   store_target_temp, 2),
-   SENSOR_ATTR(pwm4_target, S_IWUSR | S_IRUGO, show_target_temp,
-   store_target_temp, 3),
+   SENSOR_ATTR(pwm1_target, 0644, show_target_temp, store_target_temp, 0),
+   SENSOR_ATTR(pwm2_target, 0644, show_target_temp, store_target_temp, 1),
+   SENSOR_ATTR(pwm3_target, 0644, show_target_temp, store_target_temp, 2),
+   SENSOR_ATTR(pwm4_target, 0644, show_target_temp, store_target_temp, 3),
 };
 
 static struct sensor_device_attribute sda_tolerance[] = {
-   SENSOR_ATTR(pwm1_tolerance, S_IWUSR | S_IRUGO, show_tolerance,
-   store_tolerance, 0),
-   SENSOR_ATTR(pwm2_tolerance, S_IWUSR | S_IRUGO, show_tolerance,
-   store_tolerance, 1),
-   SENSOR_ATTR(pwm3_tolerance, S_IWUSR | S_IRUGO, show_tolerance,
-   store_tolerance, 2),
-   SENSOR_ATTR(pwm4_tolerance, S_IWUSR | S_IRUGO, show_tolerance,
-   store_tolerance, 3),
+   SENSOR_ATTR(pwm1_tolerance, 0644, show_tolerance, store_tolerance, 0),
+   SENSOR_ATTR(pwm2_tolerance, 0644, show_tolerance, store_tolerance, 1),
+   SENSOR_ATTR(pwm3_tolerance, 0644, show_tolerance, store_tolerance, 2),
+   SENSOR_ATTR(pwm4_tolerance, 0644, show_tolerance, store_tolerance, 3),
 };
 
 /* Smart Fan registers */
@@ -1194,24 +1186,24 @@ store_##reg(struct device *dev, struct device_attribute 
*attr, \
 fan_time_functions(fan_stop_time, FAN_STOP_TIME)
 
 static struct sensor_device_attribute sda_sf3_arrays_fan4[] = {
-   SENSOR_ATTR(pwm4_stop_time, S_IWUSR | S_IRUGO, show_fan_stop_time,
+   SENSOR_ATTR(pwm4_stop_time, 0644, show_fan_stop_time,
store_fan_stop_time, 3),
-   SENSOR_ATTR(pwm4_start_output, S_IWUSR | S_IRUGO, show_fan_start_output,
+   SENSOR_ATTR(pwm4_start_output, 0644, show_fan_start_output,
store_fan_start_output, 3),
-   SENSOR_ATTR(pwm4_stop_output, S_IWUSR | S_IRUGO, show_fan_stop_output,
+   SENSOR_ATTR(pwm4_stop_output, 0644, show_fan_stop_output,
store_fan_stop_output, 3),
-   SENSOR_ATTR(pwm4_max_output, S_IWUSR | S_IRUGO, show_fan_max_output,
+   SENSOR_ATTR(pwm4_max_output, 0644, show_fan_max_output,
store_fan_max_output, 3),
-   SENSOR_ATTR(pwm4_step_output, S_IWUSR | S_IRUGO, show_fan_step_output,
+   SENSOR_ATTR(pwm4_step_output, 0644, show_fan_step_output,
store_fan_step_output, 3),
 };
 
 static struct sensor_device_attribute sda_sf3_arrays_fan3[] = {
-   SENSOR_ATTR(pwm3_stop_time, S_IWUSR | S_IRUGO, show_fan_stop_time,
+   SENSOR_ATTR(pwm3_stop_time, 0644, show_fan_stop_time,
store_fan_stop_time, 2),
-   SENSOR_ATTR(pwm3_start_output, S_IWUSR | S_IRUGO, show_fan_start_output,
+   SENSOR_ATTR(pwm3_start_output, 0644, show_fan_start_output,
store_fan_start_output, 2),
-   SENSOR_ATTR(pwm3_stop_output, S_IWUSR | S_IRUGO, show_fan_stop_output,
+   SENSOR_ATTR(pwm3_stop_output, 0644, show_fan_stop_output,
store_fan_stop_output, 2),
 };
 
@@ -1233,17 +1225,17 @@ static SENSOR_DEVICE_ATTR(pwm2_stop_output, 0644, 
show_fan_stop_output,
  * Need to check support while generating/removing attribute files.
  */
 static struct sensor_device_attribute sda_sf3_max_step_arrays[] = {
-   SENSOR_ATTR(pwm1_max_output, S_IWUSR | S_IRUGO, show_fan_max_output,
+   SENSOR_ATTR(pwm1_max_output, 0644, show_fan_max_output,
store_fan_max_output, 0),
-   SENSOR_ATTR(pwm1_step_output, S_IWUSR | S_IRUGO, show_fan_step_output,
+   SENSOR_ATTR(pwm1_step_output, 0644, show_fan_step_output,
store_fan_step_output, 0),
-   SENSOR_ATTR(pwm2_max_output, S_IWUSR | S_IRUGO, show_fan_max_output,
+   SENSOR_ATTR(pwm2_max_output, 0644, show_fan_max_output,
store_fan_max_output, 1),
-   SENSOR_ATTR(pwm2_step_output, S_IWUSR | S_IRUGO, show_fan_step_output,
+   SENSOR_ATTR(pwm2_step_output

[PATCH 5/5] w83627ehf: Drop support for nct6775/nct6776

[Peter Huewe]

Since there exists a dedicated driver for nct6775/nct6776 it makes sense
to remove support for these chips from this driver, in order to have
only one code base for these types of chips.

This also improves maintainability and readability (and size) of this
driver.

Some not so-obvious changes are:
- removal of fan_debounce module parameter (now unused)
- removal of has_fan_div flag (nct6776 specific)
- w83627ehf_update_fan_div_common -> w83627ehf_update_fan_div
  (no distinction needed anymore)
- w83627ehf_update_pwm_common -> w83627ehf_update_pwm
  (no distinction needed anymore)
- NUM_REG_TEMP changed to ARRAY_SIZE(W83627EHF_REG_TEMP)
  (different number of max temp sensors)
- removal of intrusion1_alarm (nct6776 specific)

v2: Updated Kconfig, Documentation and removed unused temp_sensors (5-9)

Tested on NCT6776F (not probed anymore)

Signed-off-by: Peter Huewe <peterhu...@gmx.de>
---
 Documentation/hwmon/w83627ehf |  36 +--
 drivers/hwmon/Kconfig |   5 +-
 drivers/hwmon/w83627ehf.c | 546 --
 3 files changed, 57 insertions(+), 530 deletions(-)

diff --git a/Documentation/hwmon/w83627ehf b/Documentation/hwmon/w83627ehf
index 735c42a85ead..d6b05abc1dcb 100644
--- a/Documentation/hwmon/w83627ehf
+++ b/Documentation/hwmon/w83627ehf
@@ -26,14 +26,6 @@ Supported chips:
 Prefix: 'w83667hg'
 Addresses scanned: ISA address retrieved from Super I/O registers
 Datasheet: Available from Nuvoton upon request
-  * Nuvoton NCT6775F/W83667HG-I
-Prefix: 'nct6775'
-Addresses scanned: ISA address retrieved from Super I/O registers
-Datasheet: Available from Nuvoton upon request
-  * Nuvoton NCT6776F
-Prefix: 'nct6776'
-Addresses scanned: ISA address retrieved from Super I/O registers
-Datasheet: Available from Nuvoton upon request
 
 Authors:
 Jean Delvare <jdelv...@suse.de>
@@ -46,24 +38,22 @@ Description
 ---
 
 This driver implements support for the Winbond W83627EHF, W83627EHG,
-W83627DHG, W83627DHG-P, W83627UHG, W83667HG, W83667HG-B, W83667HG-I
-(NCT6775F), and NCT6776F super I/O chips. We will refer to them collectively
-as Winbond chips.
+W83627DHG, W83627DHG-P, W83627UHG, W83667HG and W83667HG-B super I/O chips.
+We will refer to them collectively as Winbond chips.
 
-The chips implement 3 to 4 temperature sensors (9 for NCT6775F and NCT6776F),
-2 to 5 fan rotation speed sensors, 8 to 10 analog voltage sensors, one VID
-(except for 627UHG), alarms with beep warnings (control unimplemented),
-and some automatic fan regulation strategies (plus manual fan control mode).
+The chips implement 3 to 4 temperature sensors, 2 to 5 fan rotation speed
+sensors, 8 to 10 analog voltage sensors, one VID (except for 627UHG), alarms
+with beep warnings (control unimplemented), and some automatic fan regulation
+strategies (plus manual fan control mode).
 
-The temperature sensor sources on W82677HG-B, NCT6775F, and NCT6776F are
-configurable. temp4 and higher attributes are only reported if its temperature
-source differs from the temperature sources of the already reported temperature
-sensors. The configured source for each of the temperature sensors is provided
-in tempX_label.
+The temperature sensor sources on W82677HG-B are configurable. temp4 and higher
+attributes are only reported if its temperature source differs from the
+temperature sources of the already reported temperature sensors. The configured
+source for each of the temperature sensors is provided in tempX_label.
 
 Temperatures are measured in degrees Celsius and measurement resolution is 1
 degC for temp1 and and 0.5 degC for temp2 and temp3. For temp4 and higher,
-resolution is 1 degC for W83667HG-B and 0.0 degC for NCT6775F and NCT6776F.
+resolution is 1 degC for W83667HG-B.
 An alarm is triggered when the temperature gets higher than high limit;
 it stays on until the temperature falls below the hysteresis value.
 Alarms are only supported for temp1, temp2, and temp3.
@@ -110,10 +100,8 @@ pwm[1-4]_enable - this file controls mode of 
fan/temperature control:
* 4 "Smart Fan III" mode
* 5 "Smart Fan IV" mode
 
-   SmartFan III mode is not supported on NCT6776F.
-
SmartFan IV mode is configurable only if it was configured at system
-   startup, and is only supported for W83677HG-B, NCT6775F, and NCT6776F.
+   startup, and is only supported for W83677HG-B.
SmartFan IV operational parameters can not be configured at this time,
and the various pwm attributes are not used in SmartFan IV mode.
The attributes can be written to, which is useful if you plan to
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 0649d53f3d16..38edc81fb87e 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -1807,7 +1807,7 @@ config SENSORS_W83627HF
  will be called w83627hf.
 
 config SENSORS_W83627EHF
-   tristate "Winbond W83627EH

[PATCH 5/5] w83627ehf: Drop support for nct6775/nct6776

[Peter Huewe]

Since there exists a dedicated driver for nct6775/nct6776 it makes sense
to remove support for these chips from this driver, in order to have
only one code base for these types of chips.

This also improves maintainability and readability (and size) of this
driver.

Some not so-obvious changes are:
- removal of fan_debounce module parameter (now unused)
- removal of has_fan_div flag (nct6776 specific)
- w83627ehf_update_fan_div_common -> w83627ehf_update_fan_div
  (no distinction needed anymore)
- w83627ehf_update_pwm_common -> w83627ehf_update_pwm
  (no distinction needed anymore)
- NUM_REG_TEMP changed to ARRAY_SIZE(W83627EHF_REG_TEMP)
  (different number of max temp sensors)
- removal of intrusion1_alarm (nct6776 specific)

v2: Updated Kconfig, Documentation and removed unused temp_sensors (5-9)

Tested on NCT6776F (not probed anymore)

Signed-off-by: Peter Huewe 
---
 Documentation/hwmon/w83627ehf |  36 +--
 drivers/hwmon/Kconfig |   5 +-
 drivers/hwmon/w83627ehf.c | 546 --
 3 files changed, 57 insertions(+), 530 deletions(-)

diff --git a/Documentation/hwmon/w83627ehf b/Documentation/hwmon/w83627ehf
index 735c42a85ead..d6b05abc1dcb 100644
--- a/Documentation/hwmon/w83627ehf
+++ b/Documentation/hwmon/w83627ehf
@@ -26,14 +26,6 @@ Supported chips:
 Prefix: 'w83667hg'
 Addresses scanned: ISA address retrieved from Super I/O registers
 Datasheet: Available from Nuvoton upon request
-  * Nuvoton NCT6775F/W83667HG-I
-Prefix: 'nct6775'
-Addresses scanned: ISA address retrieved from Super I/O registers
-Datasheet: Available from Nuvoton upon request
-  * Nuvoton NCT6776F
-Prefix: 'nct6776'
-Addresses scanned: ISA address retrieved from Super I/O registers
-Datasheet: Available from Nuvoton upon request
 
 Authors:
 Jean Delvare 
@@ -46,24 +38,22 @@ Description
 ---
 
 This driver implements support for the Winbond W83627EHF, W83627EHG,
-W83627DHG, W83627DHG-P, W83627UHG, W83667HG, W83667HG-B, W83667HG-I
-(NCT6775F), and NCT6776F super I/O chips. We will refer to them collectively
-as Winbond chips.
+W83627DHG, W83627DHG-P, W83627UHG, W83667HG and W83667HG-B super I/O chips.
+We will refer to them collectively as Winbond chips.
 
-The chips implement 3 to 4 temperature sensors (9 for NCT6775F and NCT6776F),
-2 to 5 fan rotation speed sensors, 8 to 10 analog voltage sensors, one VID
-(except for 627UHG), alarms with beep warnings (control unimplemented),
-and some automatic fan regulation strategies (plus manual fan control mode).
+The chips implement 3 to 4 temperature sensors, 2 to 5 fan rotation speed
+sensors, 8 to 10 analog voltage sensors, one VID (except for 627UHG), alarms
+with beep warnings (control unimplemented), and some automatic fan regulation
+strategies (plus manual fan control mode).
 
-The temperature sensor sources on W82677HG-B, NCT6775F, and NCT6776F are
-configurable. temp4 and higher attributes are only reported if its temperature
-source differs from the temperature sources of the already reported temperature
-sensors. The configured source for each of the temperature sensors is provided
-in tempX_label.
+The temperature sensor sources on W82677HG-B are configurable. temp4 and higher
+attributes are only reported if its temperature source differs from the
+temperature sources of the already reported temperature sensors. The configured
+source for each of the temperature sensors is provided in tempX_label.
 
 Temperatures are measured in degrees Celsius and measurement resolution is 1
 degC for temp1 and and 0.5 degC for temp2 and temp3. For temp4 and higher,
-resolution is 1 degC for W83667HG-B and 0.0 degC for NCT6775F and NCT6776F.
+resolution is 1 degC for W83667HG-B.
 An alarm is triggered when the temperature gets higher than high limit;
 it stays on until the temperature falls below the hysteresis value.
 Alarms are only supported for temp1, temp2, and temp3.
@@ -110,10 +100,8 @@ pwm[1-4]_enable - this file controls mode of 
fan/temperature control:
* 4 "Smart Fan III" mode
* 5 "Smart Fan IV" mode
 
-   SmartFan III mode is not supported on NCT6776F.
-
SmartFan IV mode is configurable only if it was configured at system
-   startup, and is only supported for W83677HG-B, NCT6775F, and NCT6776F.
+   startup, and is only supported for W83677HG-B.
SmartFan IV operational parameters can not be configured at this time,
and the various pwm attributes are not used in SmartFan IV mode.
The attributes can be written to, which is useful if you plan to
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 0649d53f3d16..38edc81fb87e 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -1807,7 +1807,7 @@ config SENSORS_W83627HF
  will be called w83627hf.
 
 config SENSORS_W83627EHF
-   tristate "Winbond W83627EHF/EHG/DHG/UHG, W83667HG, NCT6775F, NCT6776F"
+  

[PATCH 3/5] w83627ehf: Minor readability fixes

[Peter Huewe]

just some cosmetics for better readability, proposed by
Lindent/checkpatch

Signed-off-by: Peter Huewe <peterhu...@gmx.de>
---
 drivers/hwmon/w83627ehf.c | 39 +++
 1 file changed, 15 insertions(+), 24 deletions(-)

diff --git a/drivers/hwmon/w83627ehf.c b/drivers/hwmon/w83627ehf.c
index 0acadeece509..a6f56b12727b 100644
--- a/drivers/hwmon/w83627ehf.c
+++ b/drivers/hwmon/w83627ehf.c
@@ -410,6 +410,7 @@ div_from_reg(u8 reg)
 static const u16 scale_in_common[10] = {
800, 800, 1600, 1600, 800, 800, 800, 1600, 1600, 800
 };
+
 static const u16 scale_in_w83627uhg[9] = {
800, 800, 3328, 3424, 800, 800, 0, 3328, 3400
 };
@@ -432,7 +433,6 @@ struct w83627ehf_sio_data {
enum kinds kind;
 };
 
-
 struct w83627ehf_data {
int addr;   /* IO base of hw monitor block */
const char *name;
@@ -532,6 +532,7 @@ struct w83627ehf_data {
 static inline void w83627ehf_set_bank(struct w83627ehf_data *data, u16 reg)
 {
u8 bank = reg >> 8;
+
if (data->bank != bank) {
outb_p(W83627EHF_REG_BANK, data->addr + ADDR_REG_OFFSET);
outb_p(bank, data->addr + DATA_REG_OFFSET);
@@ -549,8 +550,7 @@ static u16 w83627ehf_read_value(struct w83627ehf_data 
*data, u16 reg)
outb_p(reg & 0xff, data->addr + ADDR_REG_OFFSET);
res = inb_p(data->addr + DATA_REG_OFFSET);
if (word_sized) {
-   outb_p((reg & 0xff) + 1,
-  data->addr + ADDR_REG_OFFSET);
+   outb_p((reg & 0xff) + 1, data->addr + ADDR_REG_OFFSET);
res = (res << 8) + inb_p(data->addr + DATA_REG_OFFSET);
}
 
@@ -569,8 +569,7 @@ static int w83627ehf_write_value(struct w83627ehf_data 
*data, u16 reg,
outb_p(reg & 0xff, data->addr + ADDR_REG_OFFSET);
if (word_sized) {
outb_p(value >> 8, data->addr + DATA_REG_OFFSET);
-   outb_p((reg & 0xff) + 1,
-  data->addr + ADDR_REG_OFFSET);
+   outb_p((reg & 0xff) + 1, data->addr + ADDR_REG_OFFSET);
}
outb_p(value & 0xff, data->addr + DATA_REG_OFFSET);
 
@@ -590,8 +589,7 @@ static u16 w83627ehf_read_temp(struct w83627ehf_data *data, 
u16 reg)
return res;
 }
 
-static int w83627ehf_write_temp(struct w83627ehf_data *data, u16 reg,
-  u16 value)
+static int w83627ehf_write_temp(struct w83627ehf_data *data, u16 reg, u16 
value)
 {
if (!is_word_sized(reg))
value >>= 8;
@@ -700,7 +698,7 @@ static void nct6775_update_fan_div(struct w83627ehf_data 
*data)
data->fan_div[1] = (i & 0x70) >> 4;
i = w83627ehf_read_value(data, NCT6775_REG_FANDIV2);
data->fan_div[2] = i & 0x7;
-   if (data->has_fan & (1<<3))
+   if (data->has_fan & (1 << 3))
data->fan_div[3] = (i & 0x70) >> 4;
 }
 
@@ -935,9 +933,7 @@ static struct w83627ehf_data 
*w83627ehf_update_device(struct device *dev)
return data;
 }
 
-
-static void
-store_fan_min(struct device *dev, u32 channel, unsigned long val)
+static void store_fan_min(struct device *dev, u32 channel, unsigned long val)
 {
struct w83627ehf_data *data = dev_get_drvdata(dev);
int nr = channel;
@@ -1014,10 +1010,8 @@ store_fan_min(struct device *dev, u32 channel, unsigned 
long val)
data->last_updated = jiffies;
}
 done:
-   w83627ehf_write_value(data, data->REG_FAN_MIN[nr],
- data->fan_min[nr]);
+   w83627ehf_write_value(data, data->REG_FAN_MIN[nr], data->fan_min[nr]);
mutex_unlock(>update_lock);
-
 }
 
 #define show_tol_temp(reg) \
@@ -1036,7 +1030,7 @@ show_tol_temp(target_temp)
 
 static ssize_t
 store_target_temp(struct device *dev, struct device_attribute *attr,
-   const char *buf, size_t count)
+ const char *buf, size_t count)
 {
struct w83627ehf_data *data = dev_get_drvdata(dev);
struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
@@ -1059,7 +1053,7 @@ store_target_temp(struct device *dev, struct 
device_attribute *attr,
 
 static ssize_t
 store_tolerance(struct device *dev, struct device_attribute *attr,
-   const char *buf, size_t count)
+   const char *buf, size_t count)
 {
struct w83627ehf_data *data = dev_get_drvdata(dev);
struct w83627ehf_sio_data *sio_data = data->sio_data;
@@ -1243,6 +1237,7 @@ static ssize_t
 cpu0_vid_show(struct device *dev, struct device_attribute *attr, char *buf)
 {
struct w83627ehf_data *data = dev_get_drvdata(dev);
+
return sprintf(buf, "%d\n", vid_from_reg(data->vid, data->vrm));
 }
 static DEVICE_ATTR_RO(cpu0_vid);
@@ -1486,8 +1481,7 @@ static 

[PATCH 3/5] w83627ehf: Minor readability fixes

[Peter Huewe]

just some cosmetics for better readability, proposed by
Lindent/checkpatch

Signed-off-by: Peter Huewe 
---
 drivers/hwmon/w83627ehf.c | 39 +++
 1 file changed, 15 insertions(+), 24 deletions(-)

diff --git a/drivers/hwmon/w83627ehf.c b/drivers/hwmon/w83627ehf.c
index 0acadeece509..a6f56b12727b 100644
--- a/drivers/hwmon/w83627ehf.c
+++ b/drivers/hwmon/w83627ehf.c
@@ -410,6 +410,7 @@ div_from_reg(u8 reg)
 static const u16 scale_in_common[10] = {
800, 800, 1600, 1600, 800, 800, 800, 1600, 1600, 800
 };
+
 static const u16 scale_in_w83627uhg[9] = {
800, 800, 3328, 3424, 800, 800, 0, 3328, 3400
 };
@@ -432,7 +433,6 @@ struct w83627ehf_sio_data {
enum kinds kind;
 };
 
-
 struct w83627ehf_data {
int addr;   /* IO base of hw monitor block */
const char *name;
@@ -532,6 +532,7 @@ struct w83627ehf_data {
 static inline void w83627ehf_set_bank(struct w83627ehf_data *data, u16 reg)
 {
u8 bank = reg >> 8;
+
if (data->bank != bank) {
outb_p(W83627EHF_REG_BANK, data->addr + ADDR_REG_OFFSET);
outb_p(bank, data->addr + DATA_REG_OFFSET);
@@ -549,8 +550,7 @@ static u16 w83627ehf_read_value(struct w83627ehf_data 
*data, u16 reg)
outb_p(reg & 0xff, data->addr + ADDR_REG_OFFSET);
res = inb_p(data->addr + DATA_REG_OFFSET);
if (word_sized) {
-   outb_p((reg & 0xff) + 1,
-  data->addr + ADDR_REG_OFFSET);
+   outb_p((reg & 0xff) + 1, data->addr + ADDR_REG_OFFSET);
res = (res << 8) + inb_p(data->addr + DATA_REG_OFFSET);
}
 
@@ -569,8 +569,7 @@ static int w83627ehf_write_value(struct w83627ehf_data 
*data, u16 reg,
outb_p(reg & 0xff, data->addr + ADDR_REG_OFFSET);
if (word_sized) {
outb_p(value >> 8, data->addr + DATA_REG_OFFSET);
-   outb_p((reg & 0xff) + 1,
-  data->addr + ADDR_REG_OFFSET);
+   outb_p((reg & 0xff) + 1, data->addr + ADDR_REG_OFFSET);
}
outb_p(value & 0xff, data->addr + DATA_REG_OFFSET);
 
@@ -590,8 +589,7 @@ static u16 w83627ehf_read_temp(struct w83627ehf_data *data, 
u16 reg)
return res;
 }
 
-static int w83627ehf_write_temp(struct w83627ehf_data *data, u16 reg,
-  u16 value)
+static int w83627ehf_write_temp(struct w83627ehf_data *data, u16 reg, u16 
value)
 {
if (!is_word_sized(reg))
value >>= 8;
@@ -700,7 +698,7 @@ static void nct6775_update_fan_div(struct w83627ehf_data 
*data)
data->fan_div[1] = (i & 0x70) >> 4;
i = w83627ehf_read_value(data, NCT6775_REG_FANDIV2);
data->fan_div[2] = i & 0x7;
-   if (data->has_fan & (1<<3))
+   if (data->has_fan & (1 << 3))
data->fan_div[3] = (i & 0x70) >> 4;
 }
 
@@ -935,9 +933,7 @@ static struct w83627ehf_data 
*w83627ehf_update_device(struct device *dev)
return data;
 }
 
-
-static void
-store_fan_min(struct device *dev, u32 channel, unsigned long val)
+static void store_fan_min(struct device *dev, u32 channel, unsigned long val)
 {
struct w83627ehf_data *data = dev_get_drvdata(dev);
int nr = channel;
@@ -1014,10 +1010,8 @@ store_fan_min(struct device *dev, u32 channel, unsigned 
long val)
data->last_updated = jiffies;
}
 done:
-   w83627ehf_write_value(data, data->REG_FAN_MIN[nr],
- data->fan_min[nr]);
+   w83627ehf_write_value(data, data->REG_FAN_MIN[nr], data->fan_min[nr]);
mutex_unlock(>update_lock);
-
 }
 
 #define show_tol_temp(reg) \
@@ -1036,7 +1030,7 @@ show_tol_temp(target_temp)
 
 static ssize_t
 store_target_temp(struct device *dev, struct device_attribute *attr,
-   const char *buf, size_t count)
+ const char *buf, size_t count)
 {
struct w83627ehf_data *data = dev_get_drvdata(dev);
struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
@@ -1059,7 +1053,7 @@ store_target_temp(struct device *dev, struct 
device_attribute *attr,
 
 static ssize_t
 store_tolerance(struct device *dev, struct device_attribute *attr,
-   const char *buf, size_t count)
+   const char *buf, size_t count)
 {
struct w83627ehf_data *data = dev_get_drvdata(dev);
struct w83627ehf_sio_data *sio_data = data->sio_data;
@@ -1243,6 +1237,7 @@ static ssize_t
 cpu0_vid_show(struct device *dev, struct device_attribute *attr, char *buf)
 {
struct w83627ehf_data *data = dev_get_drvdata(dev);
+
return sprintf(buf, "%d\n", vid_from_reg(data->vid, data->vrm));
 }
 static DEVICE_ATTR_RO(cpu0_vid);
@@ -1486,8 +1481,7 @@ static inline void w83627ehf_init_device(struct 
w83627ehf

[PATCH 1/5] w83627ehf: Use hwmon_device_register_with_info and sensor groups

[Peter Huewe]

This patch replaces the old, deprecated call to hwmon_device_register
with the new hwmon_device_register_with_info and converts the whole
driver to the new hwmon interface using the hwmon_chip_info methods
and the attribute_group method.

All standard attributes were converted to the corresponding
hwmon_chip_info methods.
For some functions a hwmon channel to device channel conversion had to
be performed, e.g. hwmon_in_alarm has the info for alert_5 in channel 8.

All non-standard attributes are converted to the attribute_group method,
by
- adding them statically to the attribute_group if they are available
for all variants of devices supported by this driver
- adding them at probe time to the attribute_group if the availability
is depending on the actual chip type.
The appropriate count of entries was reserved.

As a pre-condition a reference to the sio_data structure was moved into
w83627ehf_data for easier retrieval of the information, since this is
much easier than trying to access the platform_data.

The driver is now much more "checkpatch clean" than it used to be, but
still not completely.
The conversion saves about 20k in the resulting .ko

Tested with a NCT6776F chip.

v2:
- converted to proper is_visible methods
- applied minor feedback of v1

Signed-off-by: Peter Huewe <peterhu...@gmx.de>
---
 drivers/hwmon/w83627ehf.c | 1536 +++--
 1 file changed, 778 insertions(+), 758 deletions(-)

diff --git a/drivers/hwmon/w83627ehf.c b/drivers/hwmon/w83627ehf.c
index ad68b6d9ff17..f2f33dd35fd4 100644
--- a/drivers/hwmon/w83627ehf.c
+++ b/drivers/hwmon/w83627ehf.c
@@ -1,6 +1,7 @@
 /*
  *  w83627ehf - Driver for the hardware monitoring functionality of
  * the Winbond W83627EHF Super-I/O chip
+ *  Copyright (C) 2017  Peter Huewe <peterhu...@gmx.de>
  *  Copyright (C) 2005-2012  Jean Delvare <jdelv...@suse.de>
  *  Copyright (C) 2006  Yuan Mu (Winbond),
  * Rudolf Marek <r.ma...@assembler.cz>
@@ -426,6 +427,11 @@ static inline u8 in_to_reg(u32 val, u8 nr, const u16 
*scale_in)
 /*
  * Data structures and manipulation thereof
  */
+struct w83627ehf_sio_data {
+   int sioreg;
+   enum kinds kind;
+};
+
 
 struct w83627ehf_data {
int addr;   /* IO base of hw monitor block */
@@ -514,11 +520,7 @@ struct w83627ehf_data {
u8 fandiv1;
u8 fandiv2;
 #endif
-};
-
-struct w83627ehf_sio_data {
-   int sioreg;
-   enum kinds kind;
+   struct w83627ehf_sio_data *sio_data;
 };
 
 /*
@@ -679,7 +681,7 @@ static void w83627ehf_write_fan_div(struct w83627ehf_data 
*data, int nr)
 static void w83627ehf_write_fan_div_common(struct device *dev,
   struct w83627ehf_data *data, int nr)
 {
-   struct w83627ehf_sio_data *sio_data = dev_get_platdata(dev);
+   struct w83627ehf_sio_data *sio_data = data->sio_data;
 
if (sio_data->kind == nct6776)
; /* no dividers, do nothing */
@@ -730,11 +732,9 @@ static void w83627ehf_update_fan_div(struct w83627ehf_data 
*data)
 static void w83627ehf_update_fan_div_common(struct device *dev,
struct w83627ehf_data *data)
 {
-   struct w83627ehf_sio_data *sio_data = dev_get_platdata(dev);
-
-   if (sio_data->kind == nct6776)
+   if (data->sio_data->kind == nct6776)
; /* no dividers, do nothing */
-   else if (sio_data->kind == nct6775)
+   else if (data->sio_data->kind == nct6775)
nct6775_update_fan_div(data);
else
w83627ehf_update_fan_div(data);
@@ -787,7 +787,7 @@ static void w83627ehf_update_pwm(struct w83627ehf_data 
*data)
 static void w83627ehf_update_pwm_common(struct device *dev,
struct w83627ehf_data *data)
 {
-   struct w83627ehf_sio_data *sio_data = dev_get_platdata(dev);
+   struct w83627ehf_sio_data *sio_data = data->sio_data;
 
if (sio_data->kind == nct6775 || sio_data->kind == nct6776)
nct6775_update_pwm(data);
@@ -798,8 +798,7 @@ static void w83627ehf_update_pwm_common(struct device *dev,
 static struct w83627ehf_data *w83627ehf_update_device(struct device *dev)
 {
struct w83627ehf_data *data = dev_get_drvdata(dev);
-   struct w83627ehf_sio_data *sio_data = dev_get_platdata(dev);
-
+   struct w83627ehf_sio_data *sio_data = data->sio_data;
int i;
 
mutex_lock(>update_lock);
@@ -936,157 +935,15 @@ static struct w83627ehf_data 
*w83627ehf_update_device(struct device *dev)
return data;
 }
 
-/*
- * Sysfs callback functions
- */
-#define show_in_reg(reg) \
-static ssize_t \
-show_##reg(struct device *dev, struct device_attribute *attr, \
-  char *buf) \
-{ \
-   struct w83627ehf_data *data = w83627ehf_update_device(dev); \
-   struct sensor_device_attribute *sensor_attr = \
- 

[PATCH 1/5] w83627ehf: Use hwmon_device_register_with_info and sensor groups

[Peter Huewe]

This patch replaces the old, deprecated call to hwmon_device_register
with the new hwmon_device_register_with_info and converts the whole
driver to the new hwmon interface using the hwmon_chip_info methods
and the attribute_group method.

All standard attributes were converted to the corresponding
hwmon_chip_info methods.
For some functions a hwmon channel to device channel conversion had to
be performed, e.g. hwmon_in_alarm has the info for alert_5 in channel 8.

All non-standard attributes are converted to the attribute_group method,
by
- adding them statically to the attribute_group if they are available
for all variants of devices supported by this driver
- adding them at probe time to the attribute_group if the availability
is depending on the actual chip type.
The appropriate count of entries was reserved.

As a pre-condition a reference to the sio_data structure was moved into
w83627ehf_data for easier retrieval of the information, since this is
much easier than trying to access the platform_data.

The driver is now much more "checkpatch clean" than it used to be, but
still not completely.
The conversion saves about 20k in the resulting .ko

Tested with a NCT6776F chip.

v2:
- converted to proper is_visible methods
- applied minor feedback of v1

Signed-off-by: Peter Huewe 
---
 drivers/hwmon/w83627ehf.c | 1536 +++--
 1 file changed, 778 insertions(+), 758 deletions(-)

diff --git a/drivers/hwmon/w83627ehf.c b/drivers/hwmon/w83627ehf.c
index ad68b6d9ff17..f2f33dd35fd4 100644
--- a/drivers/hwmon/w83627ehf.c
+++ b/drivers/hwmon/w83627ehf.c
@@ -1,6 +1,7 @@
 /*
  *  w83627ehf - Driver for the hardware monitoring functionality of
  * the Winbond W83627EHF Super-I/O chip
+ *  Copyright (C) 2017  Peter Huewe 
  *  Copyright (C) 2005-2012  Jean Delvare 
  *  Copyright (C) 2006  Yuan Mu (Winbond),
  * Rudolf Marek 
@@ -426,6 +427,11 @@ static inline u8 in_to_reg(u32 val, u8 nr, const u16 
*scale_in)
 /*
  * Data structures and manipulation thereof
  */
+struct w83627ehf_sio_data {
+   int sioreg;
+   enum kinds kind;
+};
+
 
 struct w83627ehf_data {
int addr;   /* IO base of hw monitor block */
@@ -514,11 +520,7 @@ struct w83627ehf_data {
u8 fandiv1;
u8 fandiv2;
 #endif
-};
-
-struct w83627ehf_sio_data {
-   int sioreg;
-   enum kinds kind;
+   struct w83627ehf_sio_data *sio_data;
 };
 
 /*
@@ -679,7 +681,7 @@ static void w83627ehf_write_fan_div(struct w83627ehf_data 
*data, int nr)
 static void w83627ehf_write_fan_div_common(struct device *dev,
   struct w83627ehf_data *data, int nr)
 {
-   struct w83627ehf_sio_data *sio_data = dev_get_platdata(dev);
+   struct w83627ehf_sio_data *sio_data = data->sio_data;
 
if (sio_data->kind == nct6776)
; /* no dividers, do nothing */
@@ -730,11 +732,9 @@ static void w83627ehf_update_fan_div(struct w83627ehf_data 
*data)
 static void w83627ehf_update_fan_div_common(struct device *dev,
struct w83627ehf_data *data)
 {
-   struct w83627ehf_sio_data *sio_data = dev_get_platdata(dev);
-
-   if (sio_data->kind == nct6776)
+   if (data->sio_data->kind == nct6776)
; /* no dividers, do nothing */
-   else if (sio_data->kind == nct6775)
+   else if (data->sio_data->kind == nct6775)
nct6775_update_fan_div(data);
else
w83627ehf_update_fan_div(data);
@@ -787,7 +787,7 @@ static void w83627ehf_update_pwm(struct w83627ehf_data 
*data)
 static void w83627ehf_update_pwm_common(struct device *dev,
struct w83627ehf_data *data)
 {
-   struct w83627ehf_sio_data *sio_data = dev_get_platdata(dev);
+   struct w83627ehf_sio_data *sio_data = data->sio_data;
 
if (sio_data->kind == nct6775 || sio_data->kind == nct6776)
nct6775_update_pwm(data);
@@ -798,8 +798,7 @@ static void w83627ehf_update_pwm_common(struct device *dev,
 static struct w83627ehf_data *w83627ehf_update_device(struct device *dev)
 {
struct w83627ehf_data *data = dev_get_drvdata(dev);
-   struct w83627ehf_sio_data *sio_data = dev_get_platdata(dev);
-
+   struct w83627ehf_sio_data *sio_data = data->sio_data;
int i;
 
mutex_lock(>update_lock);
@@ -936,157 +935,15 @@ static struct w83627ehf_data 
*w83627ehf_update_device(struct device *dev)
return data;
 }
 
-/*
- * Sysfs callback functions
- */
-#define show_in_reg(reg) \
-static ssize_t \
-show_##reg(struct device *dev, struct device_attribute *attr, \
-  char *buf) \
-{ \
-   struct w83627ehf_data *data = w83627ehf_update_device(dev); \
-   struct sensor_device_attribute *sensor_attr = \
-   to_sensor_dev_attr(attr); \
-   int nr = sensor_attr->index; \
-   return

[PATCH] w83627ehf: Drop support for nct6775/nct6776

[Peter Huewe]

Since there exists a dedicated driver for nct6775/nct6776 it makes sense
to remove support for these chips from this driver, in order to have
only one code base for these types of chips.

This also improves maintainability and readability (and size) of this
driver.

Some not so-obvious changes are:
- removal of fan_debounce module parameter (now unused)
- removal of has_fan_div flag (nct6776 specific)
- w83627ehf_update_fan_div_common -> w83627ehf_update_fan_div
  (no distinction needed anymore)
- w83627ehf_update_pwm_common -> w83627ehf_update_pwm
  (no distinction needed anymore)
- NUM_REG_TEMP changed to ARRAY_SIZE(W83627EHF_REG_TEMP)
  (different number of max temp sensors)
- removal of intrusion1_alarm (nct6776 specific)

Tested with NCT6776F that it does not get probed anymore.

Signed-off-by: Peter Huewe <peterhu...@gmx.de>
---
Please apply after my conversion patch series.

 drivers/hwmon/w83627ehf.c | 542 --
 1 file changed, 43 insertions(+), 499 deletions(-)

diff --git a/drivers/hwmon/w83627ehf.c b/drivers/hwmon/w83627ehf.c
index bba26623af36..8e7ad86422ed 100644
--- a/drivers/hwmon/w83627ehf.c
+++ b/drivers/hwmon/w83627ehf.c
@@ -31,8 +31,6 @@
  *  w83627uhg8  2   2   3  0xa230 0xc10x5ca3
  *  w83667hg 9  5   3   3  0xa510 0xc10x5ca3
  *  w83667hg-b   9  5   3   4  0xb350 0xc10x5ca3
- *  nct6775f 9  4   3   9  0xb470 0xc10x5ca3
- *  nct6776f 9  5   3   9  0xC330 0xc10x5ca3
  */
 
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
@@ -53,7 +51,7 @@
 
 enum kinds {
w83627ehf, w83627dhg, w83627dhg_p, w83627uhg,
-   w83667hg, w83667hg_b, nct6775, nct6776,
+   w83667hg, w83667hg_b,
 };
 
 /* used to set data->name = w83627ehf_device_names[data->sio_kind] */
@@ -64,18 +62,12 @@ static const char * const w83627ehf_device_names[] = {
"w83627uhg",
"w83667hg",
"w83667hg",
-   "nct6775",
-   "nct6776",
 };
 
 static unsigned short force_id;
 module_param(force_id, ushort, 0);
 MODULE_PARM_DESC(force_id, "Override the detected device ID");
 
-static unsigned short fan_debounce;
-module_param(fan_debounce, ushort, 0);
-MODULE_PARM_DESC(fan_debounce, "Enable debouncing for fan RPM signal");
-
 #define DRVNAME "w83627ehf"
 
 /*
@@ -100,8 +92,6 @@ MODULE_PARM_DESC(fan_debounce, "Enable debouncing for fan 
RPM signal");
 #define SIO_W83627UHG_ID   0xa230
 #define SIO_W83667HG_ID0xa510
 #define SIO_W83667HG_B_ID  0xb350
-#define SIO_NCT6775_ID 0xb470
-#define SIO_NCT6776_ID 0xc330
 #define SIO_ID_MASK0xFFF0
 
 static inline void
@@ -184,11 +174,6 @@ static const u16 W83627EHF_REG_TEMP_CONFIG[] = { 0, 0x152, 
0x252, 0 };
 #define W83627EHF_REG_DIODE0x59
 #define W83627EHF_REG_SMI_OVT  0x4C
 
-/* NCT6775F has its own fan divider registers */
-#define NCT6775_REG_FANDIV10x506
-#define NCT6775_REG_FANDIV20x507
-#define NCT6775_REG_FAN_DEBOUNCE   0xf0
-
 #define W83627EHF_REG_ALARM1   0x459
 #define W83627EHF_REG_ALARM2   0x45A
 #define W83627EHF_REG_ALARM3   0x45B
@@ -232,28 +217,6 @@ static const u16 W83627EHF_REG_FAN_STEP_OUTPUT_W83667_B[]
 
 static const u16 W83627EHF_REG_TEMP_OFFSET[] = { 0x454, 0x455, 0x456 };
 
-static const u16 NCT6775_REG_TARGET[] = { 0x101, 0x201, 0x301 };
-static const u16 NCT6775_REG_FAN_MODE[] = { 0x102, 0x202, 0x302 };
-static const u16 NCT6775_REG_FAN_STOP_OUTPUT[] = { 0x105, 0x205, 0x305 };
-static const u16 NCT6775_REG_FAN_START_OUTPUT[] = { 0x106, 0x206, 0x306 };
-static const u16 NCT6775_REG_FAN_STOP_TIME[] = { 0x107, 0x207, 0x307 };
-static const u16 NCT6775_REG_PWM[] = { 0x109, 0x209, 0x309 };
-static const u16 NCT6775_REG_FAN_MAX_OUTPUT[] = { 0x10a, 0x20a, 0x30a };
-static const u16 NCT6775_REG_FAN_STEP_OUTPUT[] = { 0x10b, 0x20b, 0x30b };
-static const u16 NCT6775_REG_FAN[] = { 0x630, 0x632, 0x634, 0x636, 0x638 };
-static const u16 NCT6776_REG_FAN_MIN[] = { 0x63a, 0x63c, 0x63e, 0x640, 0x642};
-
-static const u16 NCT6775_REG_TEMP[]
-   = { 0x27, 0x150, 0x250, 0x73, 0x75, 0x77, 0x62b, 0x62c, 0x62d };
-static const u16 NCT6775_REG_TEMP_CONFIG[]
-   = { 0, 0x152, 0x252, 0, 0, 0, 0x628, 0x629, 0x62A };
-static const u16 NCT6775_REG_TEMP_HYST[]
-   = { 0x3a, 0x153, 0x253, 0, 0, 0, 0x673, 0x678, 0x67D };
-static const u16 NCT6775_REG_TEMP_OVER[]
-   = { 0x39, 0x155, 0x255, 0, 0, 0, 0x672, 0x677, 0x67C };
-static const u16 NCT6775_REG_TEMP_SOURCE[]
-   = { 0x621, 0x622, 0x623, 0x100, 0x200, 0x300, 0x624, 0x625, 0x626 };
-
 static const char *const w83667hg_b_temp_label[] = {
"SYSTIN",
"CPUTIN",
@@ -265,57 +228,7 @@ static const char *const w83667hg_b_temp_label[] = {
"PECI Agent 4&

[PATCH] w83627ehf: Drop support for nct6775/nct6776

[Peter Huewe]

Since there exists a dedicated driver for nct6775/nct6776 it makes sense
to remove support for these chips from this driver, in order to have
only one code base for these types of chips.

This also improves maintainability and readability (and size) of this
driver.

Some not so-obvious changes are:
- removal of fan_debounce module parameter (now unused)
- removal of has_fan_div flag (nct6776 specific)
- w83627ehf_update_fan_div_common -> w83627ehf_update_fan_div
  (no distinction needed anymore)
- w83627ehf_update_pwm_common -> w83627ehf_update_pwm
  (no distinction needed anymore)
- NUM_REG_TEMP changed to ARRAY_SIZE(W83627EHF_REG_TEMP)
  (different number of max temp sensors)
- removal of intrusion1_alarm (nct6776 specific)

Tested with NCT6776F that it does not get probed anymore.

Signed-off-by: Peter Huewe 
---
Please apply after my conversion patch series.

 drivers/hwmon/w83627ehf.c | 542 --
 1 file changed, 43 insertions(+), 499 deletions(-)

diff --git a/drivers/hwmon/w83627ehf.c b/drivers/hwmon/w83627ehf.c
index bba26623af36..8e7ad86422ed 100644
--- a/drivers/hwmon/w83627ehf.c
+++ b/drivers/hwmon/w83627ehf.c
@@ -31,8 +31,6 @@
  *  w83627uhg8  2   2   3  0xa230 0xc10x5ca3
  *  w83667hg 9  5   3   3  0xa510 0xc10x5ca3
  *  w83667hg-b   9  5   3   4  0xb350 0xc10x5ca3
- *  nct6775f 9  4   3   9  0xb470 0xc10x5ca3
- *  nct6776f 9  5   3   9  0xC330 0xc10x5ca3
  */
 
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
@@ -53,7 +51,7 @@
 
 enum kinds {
w83627ehf, w83627dhg, w83627dhg_p, w83627uhg,
-   w83667hg, w83667hg_b, nct6775, nct6776,
+   w83667hg, w83667hg_b,
 };
 
 /* used to set data->name = w83627ehf_device_names[data->sio_kind] */
@@ -64,18 +62,12 @@ static const char * const w83627ehf_device_names[] = {
"w83627uhg",
"w83667hg",
"w83667hg",
-   "nct6775",
-   "nct6776",
 };
 
 static unsigned short force_id;
 module_param(force_id, ushort, 0);
 MODULE_PARM_DESC(force_id, "Override the detected device ID");
 
-static unsigned short fan_debounce;
-module_param(fan_debounce, ushort, 0);
-MODULE_PARM_DESC(fan_debounce, "Enable debouncing for fan RPM signal");
-
 #define DRVNAME "w83627ehf"
 
 /*
@@ -100,8 +92,6 @@ MODULE_PARM_DESC(fan_debounce, "Enable debouncing for fan 
RPM signal");
 #define SIO_W83627UHG_ID   0xa230
 #define SIO_W83667HG_ID0xa510
 #define SIO_W83667HG_B_ID  0xb350
-#define SIO_NCT6775_ID 0xb470
-#define SIO_NCT6776_ID 0xc330
 #define SIO_ID_MASK0xFFF0
 
 static inline void
@@ -184,11 +174,6 @@ static const u16 W83627EHF_REG_TEMP_CONFIG[] = { 0, 0x152, 
0x252, 0 };
 #define W83627EHF_REG_DIODE0x59
 #define W83627EHF_REG_SMI_OVT  0x4C
 
-/* NCT6775F has its own fan divider registers */
-#define NCT6775_REG_FANDIV10x506
-#define NCT6775_REG_FANDIV20x507
-#define NCT6775_REG_FAN_DEBOUNCE   0xf0
-
 #define W83627EHF_REG_ALARM1   0x459
 #define W83627EHF_REG_ALARM2   0x45A
 #define W83627EHF_REG_ALARM3   0x45B
@@ -232,28 +217,6 @@ static const u16 W83627EHF_REG_FAN_STEP_OUTPUT_W83667_B[]
 
 static const u16 W83627EHF_REG_TEMP_OFFSET[] = { 0x454, 0x455, 0x456 };
 
-static const u16 NCT6775_REG_TARGET[] = { 0x101, 0x201, 0x301 };
-static const u16 NCT6775_REG_FAN_MODE[] = { 0x102, 0x202, 0x302 };
-static const u16 NCT6775_REG_FAN_STOP_OUTPUT[] = { 0x105, 0x205, 0x305 };
-static const u16 NCT6775_REG_FAN_START_OUTPUT[] = { 0x106, 0x206, 0x306 };
-static const u16 NCT6775_REG_FAN_STOP_TIME[] = { 0x107, 0x207, 0x307 };
-static const u16 NCT6775_REG_PWM[] = { 0x109, 0x209, 0x309 };
-static const u16 NCT6775_REG_FAN_MAX_OUTPUT[] = { 0x10a, 0x20a, 0x30a };
-static const u16 NCT6775_REG_FAN_STEP_OUTPUT[] = { 0x10b, 0x20b, 0x30b };
-static const u16 NCT6775_REG_FAN[] = { 0x630, 0x632, 0x634, 0x636, 0x638 };
-static const u16 NCT6776_REG_FAN_MIN[] = { 0x63a, 0x63c, 0x63e, 0x640, 0x642};
-
-static const u16 NCT6775_REG_TEMP[]
-   = { 0x27, 0x150, 0x250, 0x73, 0x75, 0x77, 0x62b, 0x62c, 0x62d };
-static const u16 NCT6775_REG_TEMP_CONFIG[]
-   = { 0, 0x152, 0x252, 0, 0, 0, 0x628, 0x629, 0x62A };
-static const u16 NCT6775_REG_TEMP_HYST[]
-   = { 0x3a, 0x153, 0x253, 0, 0, 0, 0x673, 0x678, 0x67D };
-static const u16 NCT6775_REG_TEMP_OVER[]
-   = { 0x39, 0x155, 0x255, 0, 0, 0, 0x672, 0x677, 0x67C };
-static const u16 NCT6775_REG_TEMP_SOURCE[]
-   = { 0x621, 0x622, 0x623, 0x100, 0x200, 0x300, 0x624, 0x625, 0x626 };
-
 static const char *const w83667hg_b_temp_label[] = {
"SYSTIN",
"CPUTIN",
@@ -265,57 +228,7 @@ static const char *const w83667hg_b_temp_label[] = {
"PECI Agent 4"
 };
 
-static co

[PATCH 1/4] w83627ehf: Use hwmon_device_register_with_info and sensor groups

[Peter Huewe]

This patch replaces the old, deprecated call to hwmon_device_register
with the new hwmon_device_register_with_info and converts the whole
driver to the new hwmon interface using the hwmon_chip_info methods
and the attribute_group method.

Unfortunately this makes the patch quite large, but it is the most
sensible way to do so, without doing everything twice.

All standard attributes were converted to the corresponding
hwmon_chip_info methods.
For some functions a hwmon channel to device channel conversion had to
be performed, e.g. hwmon_in_alarm has the info for alert_5 in channel 8.

All non-standard attributes are converted to the attribute_group method,
by
- adding them statically to the attribute_group if they are available
for all variants of devices supported by this driver
- adding them at probe time to the attribute_group if the availability
is depending on the actual chip type.
The appropriate count of entries was reserved.

As a pre-condition a reference to the sio_data structure was moved into
w83627ehf_data for easier retrieval of the information, since this is
much easier than trying to access the platform_data.

The driver is now much more "checkpatch clean" than it used to be, but
still not completely.
The conversion saves about 20k in the resulting .ko

Tested with a NCT6776F chip.

Signed-off-by: Peter Huewe <peterhu...@gmx.de>
---
Target-Branch: groeck/hwmon

Please cherry-pick
 46dc4a97 hwmon: Constify str parameter of hwmon_ops->read_string
before this patch series


 drivers/hwmon/w83627ehf.c | 1387 +
 1 file changed, 648 insertions(+), 739 deletions(-)

diff --git a/drivers/hwmon/w83627ehf.c b/drivers/hwmon/w83627ehf.c
index ab346ed142de..785ddd47c588 100644
--- a/drivers/hwmon/w83627ehf.c
+++ b/drivers/hwmon/w83627ehf.c
@@ -1,6 +1,7 @@
 /*
  *  w83627ehf - Driver for the hardware monitoring functionality of
  * the Winbond W83627EHF Super-I/O chip
+ *  Copyright (C) 2017  Peter Huewe <peterhu...@gmx.de>
  *  Copyright (C) 2005-2012  Jean Delvare <jdelv...@suse.de>
  *  Copyright (C) 2006  Yuan Mu (Winbond),
  * Rudolf Marek <r.ma...@assembler.cz>
@@ -420,6 +421,11 @@ static inline u8 in_to_reg(u32 val, u8 nr, const u16 
*scale_in)
 /*
  * Data structures and manipulation thereof
  */
+struct w83627ehf_sio_data {
+   int sioreg;
+   enum kinds kind;
+};
+
 
 struct w83627ehf_data {
int addr;   /* IO base of hw monitor block */
@@ -508,11 +514,7 @@ struct w83627ehf_data {
u8 fandiv1;
u8 fandiv2;
 #endif
-};
-
-struct w83627ehf_sio_data {
-   int sioreg;
-   enum kinds kind;
+   struct w83627ehf_sio_data *sio_data;
 };
 
 /*
@@ -673,7 +675,7 @@ static void w83627ehf_write_fan_div(struct w83627ehf_data 
*data, int nr)
 static void w83627ehf_write_fan_div_common(struct device *dev,
   struct w83627ehf_data *data, int nr)
 {
-   struct w83627ehf_sio_data *sio_data = dev_get_platdata(dev);
+   struct w83627ehf_sio_data *sio_data = data->sio_data;
 
if (sio_data->kind == nct6776)
; /* no dividers, do nothing */
@@ -724,14 +726,14 @@ static void w83627ehf_update_fan_div(struct 
w83627ehf_data *data)
 static void w83627ehf_update_fan_div_common(struct device *dev,
struct w83627ehf_data *data)
 {
-   struct w83627ehf_sio_data *sio_data = dev_get_platdata(dev);
-
-   if (sio_data->kind == nct6776)
-   ; /* no dividers, do nothing */
-   else if (sio_data->kind == nct6775)
-   nct6775_update_fan_div(data);
-   else
-   w83627ehf_update_fan_div(data);
+   if (data->sio_data) {
+   if (data->sio_data->kind == nct6776)
+   ; /* no dividers, do nothing */
+   else if (data->sio_data->kind == nct6775)
+   nct6775_update_fan_div(data);
+   else
+   w83627ehf_update_fan_div(data);
+   }
 }
 
 static void nct6775_update_pwm(struct w83627ehf_data *data)
@@ -781,7 +783,7 @@ static void w83627ehf_update_pwm(struct w83627ehf_data 
*data)
 static void w83627ehf_update_pwm_common(struct device *dev,
struct w83627ehf_data *data)
 {
-   struct w83627ehf_sio_data *sio_data = dev_get_platdata(dev);
+   struct w83627ehf_sio_data *sio_data = data->sio_data;
 
if (sio_data->kind == nct6775 || sio_data->kind == nct6776)
nct6775_update_pwm(data);
@@ -792,8 +794,7 @@ static void w83627ehf_update_pwm_common(struct device *dev,
 static struct w83627ehf_data *w83627ehf_update_device(struct device *dev)
 {
struct w83627ehf_data *data = dev_get_drvdata(dev);
-   struct w83627ehf_sio_data *sio_data = dev_get_platdata(dev);
-
+   struct w83627ehf_sio_data *sio_data = data-

[PATCH 1/4] w83627ehf: Use hwmon_device_register_with_info and sensor groups

[Peter Huewe]

This patch replaces the old, deprecated call to hwmon_device_register
with the new hwmon_device_register_with_info and converts the whole
driver to the new hwmon interface using the hwmon_chip_info methods
and the attribute_group method.

Unfortunately this makes the patch quite large, but it is the most
sensible way to do so, without doing everything twice.

All standard attributes were converted to the corresponding
hwmon_chip_info methods.
For some functions a hwmon channel to device channel conversion had to
be performed, e.g. hwmon_in_alarm has the info for alert_5 in channel 8.

All non-standard attributes are converted to the attribute_group method,
by
- adding them statically to the attribute_group if they are available
for all variants of devices supported by this driver
- adding them at probe time to the attribute_group if the availability
is depending on the actual chip type.
The appropriate count of entries was reserved.

As a pre-condition a reference to the sio_data structure was moved into
w83627ehf_data for easier retrieval of the information, since this is
much easier than trying to access the platform_data.

The driver is now much more "checkpatch clean" than it used to be, but
still not completely.
The conversion saves about 20k in the resulting .ko

Tested with a NCT6776F chip.

Signed-off-by: Peter Huewe 
---
Target-Branch: groeck/hwmon

Please cherry-pick
 46dc4a97 hwmon: Constify str parameter of hwmon_ops->read_string
before this patch series


 drivers/hwmon/w83627ehf.c | 1387 +
 1 file changed, 648 insertions(+), 739 deletions(-)

diff --git a/drivers/hwmon/w83627ehf.c b/drivers/hwmon/w83627ehf.c
index ab346ed142de..785ddd47c588 100644
--- a/drivers/hwmon/w83627ehf.c
+++ b/drivers/hwmon/w83627ehf.c
@@ -1,6 +1,7 @@
 /*
  *  w83627ehf - Driver for the hardware monitoring functionality of
  * the Winbond W83627EHF Super-I/O chip
+ *  Copyright (C) 2017  Peter Huewe 
  *  Copyright (C) 2005-2012  Jean Delvare 
  *  Copyright (C) 2006  Yuan Mu (Winbond),
  * Rudolf Marek 
@@ -420,6 +421,11 @@ static inline u8 in_to_reg(u32 val, u8 nr, const u16 
*scale_in)
 /*
  * Data structures and manipulation thereof
  */
+struct w83627ehf_sio_data {
+   int sioreg;
+   enum kinds kind;
+};
+
 
 struct w83627ehf_data {
int addr;   /* IO base of hw monitor block */
@@ -508,11 +514,7 @@ struct w83627ehf_data {
u8 fandiv1;
u8 fandiv2;
 #endif
-};
-
-struct w83627ehf_sio_data {
-   int sioreg;
-   enum kinds kind;
+   struct w83627ehf_sio_data *sio_data;
 };
 
 /*
@@ -673,7 +675,7 @@ static void w83627ehf_write_fan_div(struct w83627ehf_data 
*data, int nr)
 static void w83627ehf_write_fan_div_common(struct device *dev,
   struct w83627ehf_data *data, int nr)
 {
-   struct w83627ehf_sio_data *sio_data = dev_get_platdata(dev);
+   struct w83627ehf_sio_data *sio_data = data->sio_data;
 
if (sio_data->kind == nct6776)
; /* no dividers, do nothing */
@@ -724,14 +726,14 @@ static void w83627ehf_update_fan_div(struct 
w83627ehf_data *data)
 static void w83627ehf_update_fan_div_common(struct device *dev,
struct w83627ehf_data *data)
 {
-   struct w83627ehf_sio_data *sio_data = dev_get_platdata(dev);
-
-   if (sio_data->kind == nct6776)
-   ; /* no dividers, do nothing */
-   else if (sio_data->kind == nct6775)
-   nct6775_update_fan_div(data);
-   else
-   w83627ehf_update_fan_div(data);
+   if (data->sio_data) {
+   if (data->sio_data->kind == nct6776)
+   ; /* no dividers, do nothing */
+   else if (data->sio_data->kind == nct6775)
+   nct6775_update_fan_div(data);
+   else
+   w83627ehf_update_fan_div(data);
+   }
 }
 
 static void nct6775_update_pwm(struct w83627ehf_data *data)
@@ -781,7 +783,7 @@ static void w83627ehf_update_pwm(struct w83627ehf_data 
*data)
 static void w83627ehf_update_pwm_common(struct device *dev,
struct w83627ehf_data *data)
 {
-   struct w83627ehf_sio_data *sio_data = dev_get_platdata(dev);
+   struct w83627ehf_sio_data *sio_data = data->sio_data;
 
if (sio_data->kind == nct6775 || sio_data->kind == nct6776)
nct6775_update_pwm(data);
@@ -792,8 +794,7 @@ static void w83627ehf_update_pwm_common(struct device *dev,
 static struct w83627ehf_data *w83627ehf_update_device(struct device *dev)
 {
struct w83627ehf_data *data = dev_get_drvdata(dev);
-   struct w83627ehf_sio_data *sio_data = dev_get_platdata(dev);
-
+   struct w83627ehf_sio_data *sio_data = data->sio_data;
int i;
 
mutex_lock(>update_lock);
@@ -930,157 +931,15 @@ static struct w836

[PATCH 2/4] w83627ehf: Use octal values for access rights of sysfs files

[Peter Huewe]

As indicated by checkpatch, use the octal representation for the access
rights.

S_IWUSR | S_IRUGO => 0644
S_IRUGO => 0444

Signed-off-by: Peter Huewe <peterhu...@gmx.de>
---
 drivers/hwmon/w83627ehf.c | 52 ---
 1 file changed, 22 insertions(+), 30 deletions(-)

diff --git a/drivers/hwmon/w83627ehf.c b/drivers/hwmon/w83627ehf.c
index 785ddd47c588..b0eac8e35cc5 100644
--- a/drivers/hwmon/w83627ehf.c
+++ b/drivers/hwmon/w83627ehf.c
@@ -1094,25 +1094,17 @@ store_tolerance(struct device *dev, struct 
device_attribute *attr,
 }
 
 static struct sensor_device_attribute sda_target_temp[] = {
-   SENSOR_ATTR(pwm1_target, S_IWUSR | S_IRUGO, show_target_temp,
-   store_target_temp, 0),
-   SENSOR_ATTR(pwm2_target, S_IWUSR | S_IRUGO, show_target_temp,
-   store_target_temp, 1),
-   SENSOR_ATTR(pwm3_target, S_IWUSR | S_IRUGO, show_target_temp,
-   store_target_temp, 2),
-   SENSOR_ATTR(pwm4_target, S_IWUSR | S_IRUGO, show_target_temp,
-   store_target_temp, 3),
+   SENSOR_ATTR(pwm1_target, 0644, show_target_temp, store_target_temp, 0),
+   SENSOR_ATTR(pwm2_target, 0644, show_target_temp, store_target_temp, 1),
+   SENSOR_ATTR(pwm3_target, 0644, show_target_temp, store_target_temp, 2),
+   SENSOR_ATTR(pwm4_target, 0644, show_target_temp, store_target_temp, 3),
 };
 
 static struct sensor_device_attribute sda_tolerance[] = {
-   SENSOR_ATTR(pwm1_tolerance, S_IWUSR | S_IRUGO, show_tolerance,
-   store_tolerance, 0),
-   SENSOR_ATTR(pwm2_tolerance, S_IWUSR | S_IRUGO, show_tolerance,
-   store_tolerance, 1),
-   SENSOR_ATTR(pwm3_tolerance, S_IWUSR | S_IRUGO, show_tolerance,
-   store_tolerance, 2),
-   SENSOR_ATTR(pwm4_tolerance, S_IWUSR | S_IRUGO, show_tolerance,
-   store_tolerance, 3),
+   SENSOR_ATTR(pwm1_tolerance, 0644, show_tolerance, store_tolerance, 0),
+   SENSOR_ATTR(pwm2_tolerance, 0644, show_tolerance, store_tolerance, 1),
+   SENSOR_ATTR(pwm3_tolerance, 0644, show_tolerance, store_tolerance, 2),
+   SENSOR_ATTR(pwm4_tolerance, 0644, show_tolerance, store_tolerance, 3),
 };
 
 /* Smart Fan registers */
@@ -1190,24 +1182,24 @@ store_##reg(struct device *dev, struct device_attribute 
*attr, \
 fan_time_functions(fan_stop_time, FAN_STOP_TIME)
 
 static struct sensor_device_attribute sda_sf3_arrays_fan4[] = {
-   SENSOR_ATTR(pwm4_stop_time, S_IWUSR | S_IRUGO, show_fan_stop_time,
+   SENSOR_ATTR(pwm4_stop_time, 0644, show_fan_stop_time,
store_fan_stop_time, 3),
-   SENSOR_ATTR(pwm4_start_output, S_IWUSR | S_IRUGO, show_fan_start_output,
+   SENSOR_ATTR(pwm4_start_output, 0644, show_fan_start_output,
store_fan_start_output, 3),
-   SENSOR_ATTR(pwm4_stop_output, S_IWUSR | S_IRUGO, show_fan_stop_output,
+   SENSOR_ATTR(pwm4_stop_output, 0644, show_fan_stop_output,
store_fan_stop_output, 3),
-   SENSOR_ATTR(pwm4_max_output, S_IWUSR | S_IRUGO, show_fan_max_output,
+   SENSOR_ATTR(pwm4_max_output, 0644, show_fan_max_output,
store_fan_max_output, 3),
-   SENSOR_ATTR(pwm4_step_output, S_IWUSR | S_IRUGO, show_fan_step_output,
+   SENSOR_ATTR(pwm4_step_output, 0644, show_fan_step_output,
store_fan_step_output, 3),
 };
 
 static struct sensor_device_attribute sda_sf3_arrays_fan3[] = {
-   SENSOR_ATTR(pwm3_stop_time, S_IWUSR | S_IRUGO, show_fan_stop_time,
+   SENSOR_ATTR(pwm3_stop_time, 0644, show_fan_stop_time,
store_fan_stop_time, 2),
-   SENSOR_ATTR(pwm3_start_output, S_IWUSR | S_IRUGO, show_fan_start_output,
+   SENSOR_ATTR(pwm3_start_output, 0644, show_fan_start_output,
store_fan_start_output, 2),
-   SENSOR_ATTR(pwm3_stop_output, S_IWUSR | S_IRUGO, show_fan_stop_output,
+   SENSOR_ATTR(pwm3_stop_output, 0644, show_fan_stop_output,
store_fan_stop_output, 2),
 };
 
@@ -1229,17 +1221,17 @@ static SENSOR_DEVICE_ATTR(pwm2_stop_output, 0644, 
show_fan_stop_output,
  * Need to check support while generating/removing attribute files.
  */
 static struct sensor_device_attribute sda_sf3_max_step_arrays[] = {
-   SENSOR_ATTR(pwm1_max_output, S_IWUSR | S_IRUGO, show_fan_max_output,
+   SENSOR_ATTR(pwm1_max_output, 0644, show_fan_max_output,
store_fan_max_output, 0),
-   SENSOR_ATTR(pwm1_step_output, S_IWUSR | S_IRUGO, show_fan_step_output,
+   SENSOR_ATTR(pwm1_step_output, 0644, show_fan_step_output,
store_fan_step_output, 0),
-   SENSOR_ATTR(pwm2_max_output, S_IWUSR | S_IRUGO, show_fan_max_output,
+   SENSOR_ATTR(pwm2_max_output, 0644, show_fan_max_output,
store_fan_max_output, 1),
-   SENSOR_ATTR(pwm2_step_output, S_IWUSR | S_IRUGO, show_fan_step_output,
+   SENSOR_ATTR(pwm2

[PATCH 2/4] w83627ehf: Use octal values for access rights of sysfs files

[Peter Huewe]

As indicated by checkpatch, use the octal representation for the access
rights.

S_IWUSR | S_IRUGO => 0644
S_IRUGO => 0444

Signed-off-by: Peter Huewe 
---
 drivers/hwmon/w83627ehf.c | 52 ---
 1 file changed, 22 insertions(+), 30 deletions(-)

diff --git a/drivers/hwmon/w83627ehf.c b/drivers/hwmon/w83627ehf.c
index 785ddd47c588..b0eac8e35cc5 100644
--- a/drivers/hwmon/w83627ehf.c
+++ b/drivers/hwmon/w83627ehf.c
@@ -1094,25 +1094,17 @@ store_tolerance(struct device *dev, struct 
device_attribute *attr,
 }
 
 static struct sensor_device_attribute sda_target_temp[] = {
-   SENSOR_ATTR(pwm1_target, S_IWUSR | S_IRUGO, show_target_temp,
-   store_target_temp, 0),
-   SENSOR_ATTR(pwm2_target, S_IWUSR | S_IRUGO, show_target_temp,
-   store_target_temp, 1),
-   SENSOR_ATTR(pwm3_target, S_IWUSR | S_IRUGO, show_target_temp,
-   store_target_temp, 2),
-   SENSOR_ATTR(pwm4_target, S_IWUSR | S_IRUGO, show_target_temp,
-   store_target_temp, 3),
+   SENSOR_ATTR(pwm1_target, 0644, show_target_temp, store_target_temp, 0),
+   SENSOR_ATTR(pwm2_target, 0644, show_target_temp, store_target_temp, 1),
+   SENSOR_ATTR(pwm3_target, 0644, show_target_temp, store_target_temp, 2),
+   SENSOR_ATTR(pwm4_target, 0644, show_target_temp, store_target_temp, 3),
 };
 
 static struct sensor_device_attribute sda_tolerance[] = {
-   SENSOR_ATTR(pwm1_tolerance, S_IWUSR | S_IRUGO, show_tolerance,
-   store_tolerance, 0),
-   SENSOR_ATTR(pwm2_tolerance, S_IWUSR | S_IRUGO, show_tolerance,
-   store_tolerance, 1),
-   SENSOR_ATTR(pwm3_tolerance, S_IWUSR | S_IRUGO, show_tolerance,
-   store_tolerance, 2),
-   SENSOR_ATTR(pwm4_tolerance, S_IWUSR | S_IRUGO, show_tolerance,
-   store_tolerance, 3),
+   SENSOR_ATTR(pwm1_tolerance, 0644, show_tolerance, store_tolerance, 0),
+   SENSOR_ATTR(pwm2_tolerance, 0644, show_tolerance, store_tolerance, 1),
+   SENSOR_ATTR(pwm3_tolerance, 0644, show_tolerance, store_tolerance, 2),
+   SENSOR_ATTR(pwm4_tolerance, 0644, show_tolerance, store_tolerance, 3),
 };
 
 /* Smart Fan registers */
@@ -1190,24 +1182,24 @@ store_##reg(struct device *dev, struct device_attribute 
*attr, \
 fan_time_functions(fan_stop_time, FAN_STOP_TIME)
 
 static struct sensor_device_attribute sda_sf3_arrays_fan4[] = {
-   SENSOR_ATTR(pwm4_stop_time, S_IWUSR | S_IRUGO, show_fan_stop_time,
+   SENSOR_ATTR(pwm4_stop_time, 0644, show_fan_stop_time,
store_fan_stop_time, 3),
-   SENSOR_ATTR(pwm4_start_output, S_IWUSR | S_IRUGO, show_fan_start_output,
+   SENSOR_ATTR(pwm4_start_output, 0644, show_fan_start_output,
store_fan_start_output, 3),
-   SENSOR_ATTR(pwm4_stop_output, S_IWUSR | S_IRUGO, show_fan_stop_output,
+   SENSOR_ATTR(pwm4_stop_output, 0644, show_fan_stop_output,
store_fan_stop_output, 3),
-   SENSOR_ATTR(pwm4_max_output, S_IWUSR | S_IRUGO, show_fan_max_output,
+   SENSOR_ATTR(pwm4_max_output, 0644, show_fan_max_output,
store_fan_max_output, 3),
-   SENSOR_ATTR(pwm4_step_output, S_IWUSR | S_IRUGO, show_fan_step_output,
+   SENSOR_ATTR(pwm4_step_output, 0644, show_fan_step_output,
store_fan_step_output, 3),
 };
 
 static struct sensor_device_attribute sda_sf3_arrays_fan3[] = {
-   SENSOR_ATTR(pwm3_stop_time, S_IWUSR | S_IRUGO, show_fan_stop_time,
+   SENSOR_ATTR(pwm3_stop_time, 0644, show_fan_stop_time,
store_fan_stop_time, 2),
-   SENSOR_ATTR(pwm3_start_output, S_IWUSR | S_IRUGO, show_fan_start_output,
+   SENSOR_ATTR(pwm3_start_output, 0644, show_fan_start_output,
store_fan_start_output, 2),
-   SENSOR_ATTR(pwm3_stop_output, S_IWUSR | S_IRUGO, show_fan_stop_output,
+   SENSOR_ATTR(pwm3_stop_output, 0644, show_fan_stop_output,
store_fan_stop_output, 2),
 };
 
@@ -1229,17 +1221,17 @@ static SENSOR_DEVICE_ATTR(pwm2_stop_output, 0644, 
show_fan_stop_output,
  * Need to check support while generating/removing attribute files.
  */
 static struct sensor_device_attribute sda_sf3_max_step_arrays[] = {
-   SENSOR_ATTR(pwm1_max_output, S_IWUSR | S_IRUGO, show_fan_max_output,
+   SENSOR_ATTR(pwm1_max_output, 0644, show_fan_max_output,
store_fan_max_output, 0),
-   SENSOR_ATTR(pwm1_step_output, S_IWUSR | S_IRUGO, show_fan_step_output,
+   SENSOR_ATTR(pwm1_step_output, 0644, show_fan_step_output,
store_fan_step_output, 0),
-   SENSOR_ATTR(pwm2_max_output, S_IWUSR | S_IRUGO, show_fan_max_output,
+   SENSOR_ATTR(pwm2_max_output, 0644, show_fan_max_output,
store_fan_max_output, 1),
-   SENSOR_ATTR(pwm2_step_output, S_IWUSR | S_IRUGO, show_fan_step_output,
+   SENSOR_ATTR(pwm2_step_output

[PATCH 3/4] w83627ehf: Minor readability fixes

[Peter Huewe]

just some cosmetics for better readability, proposed by Lindent/checkpatch

Signed-off-by: Peter Huewe <peterhu...@gmx.de>
---
 drivers/hwmon/w83627ehf.c | 41 +++--
 1 file changed, 15 insertions(+), 26 deletions(-)

diff --git a/drivers/hwmon/w83627ehf.c b/drivers/hwmon/w83627ehf.c
index b0eac8e35cc5..dbe5735963ac 100644
--- a/drivers/hwmon/w83627ehf.c
+++ b/drivers/hwmon/w83627ehf.c
@@ -404,6 +404,7 @@ div_from_reg(u8 reg)
 static const u16 scale_in_common[10] = {
800, 800, 1600, 1600, 800, 800, 800, 1600, 1600, 800
 };
+
 static const u16 scale_in_w83627uhg[9] = {
800, 800, 3328, 3424, 800, 800, 0, 3328, 3400
 };
@@ -426,7 +427,6 @@ struct w83627ehf_sio_data {
enum kinds kind;
 };
 
-
 struct w83627ehf_data {
int addr;   /* IO base of hw monitor block */
const char *name;
@@ -526,6 +526,7 @@ struct w83627ehf_data {
 static inline void w83627ehf_set_bank(struct w83627ehf_data *data, u16 reg)
 {
u8 bank = reg >> 8;
+
if (data->bank != bank) {
outb_p(W83627EHF_REG_BANK, data->addr + ADDR_REG_OFFSET);
outb_p(bank, data->addr + DATA_REG_OFFSET);
@@ -543,8 +544,7 @@ static u16 w83627ehf_read_value(struct w83627ehf_data 
*data, u16 reg)
outb_p(reg & 0xff, data->addr + ADDR_REG_OFFSET);
res = inb_p(data->addr + DATA_REG_OFFSET);
if (word_sized) {
-   outb_p((reg & 0xff) + 1,
-  data->addr + ADDR_REG_OFFSET);
+   outb_p((reg & 0xff) + 1, data->addr + ADDR_REG_OFFSET);
res = (res << 8) + inb_p(data->addr + DATA_REG_OFFSET);
}
 
@@ -563,8 +563,7 @@ static int w83627ehf_write_value(struct w83627ehf_data 
*data, u16 reg,
outb_p(reg & 0xff, data->addr + ADDR_REG_OFFSET);
if (word_sized) {
outb_p(value >> 8, data->addr + DATA_REG_OFFSET);
-   outb_p((reg & 0xff) + 1,
-  data->addr + ADDR_REG_OFFSET);
+   outb_p((reg & 0xff) + 1, data->addr + ADDR_REG_OFFSET);
}
outb_p(value & 0xff, data->addr + DATA_REG_OFFSET);
 
@@ -584,8 +583,7 @@ static u16 w83627ehf_read_temp(struct w83627ehf_data *data, 
u16 reg)
return res;
 }
 
-static int w83627ehf_write_temp(struct w83627ehf_data *data, u16 reg,
-  u16 value)
+static int w83627ehf_write_temp(struct w83627ehf_data *data, u16 reg, u16 
value)
 {
if (!is_word_sized(reg))
value >>= 8;
@@ -694,7 +692,7 @@ static void nct6775_update_fan_div(struct w83627ehf_data 
*data)
data->fan_div[1] = (i & 0x70) >> 4;
i = w83627ehf_read_value(data, NCT6775_REG_FANDIV2);
data->fan_div[2] = i & 0x7;
-   if (data->has_fan & (1<<3))
+   if (data->has_fan & (1 << 3))
data->fan_div[3] = (i & 0x70) >> 4;
 }
 
@@ -931,9 +929,7 @@ static struct w83627ehf_data 
*w83627ehf_update_device(struct device *dev)
return data;
 }
 
-
-static void
-store_fan_min(struct device *dev, u32 channel, unsigned long val)
+static void store_fan_min(struct device *dev, u32 channel, unsigned long val)
 {
struct w83627ehf_data *data = dev_get_drvdata(dev);
int nr = channel;
@@ -1010,10 +1006,8 @@ store_fan_min(struct device *dev, u32 channel, unsigned 
long val)
data->last_updated = jiffies;
}
 done:
-   w83627ehf_write_value(data, data->REG_FAN_MIN[nr],
- data->fan_min[nr]);
+   w83627ehf_write_value(data, data->REG_FAN_MIN[nr], data->fan_min[nr]);
mutex_unlock(>update_lock);
-
 }
 
 #define show_tol_temp(reg) \
@@ -1032,7 +1026,7 @@ show_tol_temp(target_temp)
 
 static ssize_t
 store_target_temp(struct device *dev, struct device_attribute *attr,
-   const char *buf, size_t count)
+ const char *buf, size_t count)
 {
struct w83627ehf_data *data = dev_get_drvdata(dev);
struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
@@ -1055,7 +1049,7 @@ store_target_temp(struct device *dev, struct 
device_attribute *attr,
 
 static ssize_t
 store_tolerance(struct device *dev, struct device_attribute *attr,
-   const char *buf, size_t count)
+   const char *buf, size_t count)
 {
struct w83627ehf_data *data = dev_get_drvdata(dev);
struct w83627ehf_sio_data *sio_data = data->sio_data;
@@ -1239,11 +1233,11 @@ static ssize_t
 cpu0_vid_show(struct device *dev, struct device_attribute *attr, char *buf)
 {
struct w83627ehf_data *data = dev_get_drvdata(dev);
+
return sprintf(buf, "%d\n", vid_from_reg(data->vid, data->vrm));
 }
 static DEVICE_ATTR_RO(cpu0_vid);
 
-
 /* Case open detection */
 
 static ssize_t
@@

[PATCH 3/4] w83627ehf: Minor readability fixes

[Peter Huewe]

just some cosmetics for better readability, proposed by Lindent/checkpatch

Signed-off-by: Peter Huewe 
---
 drivers/hwmon/w83627ehf.c | 41 +++--
 1 file changed, 15 insertions(+), 26 deletions(-)

diff --git a/drivers/hwmon/w83627ehf.c b/drivers/hwmon/w83627ehf.c
index b0eac8e35cc5..dbe5735963ac 100644
--- a/drivers/hwmon/w83627ehf.c
+++ b/drivers/hwmon/w83627ehf.c
@@ -404,6 +404,7 @@ div_from_reg(u8 reg)
 static const u16 scale_in_common[10] = {
800, 800, 1600, 1600, 800, 800, 800, 1600, 1600, 800
 };
+
 static const u16 scale_in_w83627uhg[9] = {
800, 800, 3328, 3424, 800, 800, 0, 3328, 3400
 };
@@ -426,7 +427,6 @@ struct w83627ehf_sio_data {
enum kinds kind;
 };
 
-
 struct w83627ehf_data {
int addr;   /* IO base of hw monitor block */
const char *name;
@@ -526,6 +526,7 @@ struct w83627ehf_data {
 static inline void w83627ehf_set_bank(struct w83627ehf_data *data, u16 reg)
 {
u8 bank = reg >> 8;
+
if (data->bank != bank) {
outb_p(W83627EHF_REG_BANK, data->addr + ADDR_REG_OFFSET);
outb_p(bank, data->addr + DATA_REG_OFFSET);
@@ -543,8 +544,7 @@ static u16 w83627ehf_read_value(struct w83627ehf_data 
*data, u16 reg)
outb_p(reg & 0xff, data->addr + ADDR_REG_OFFSET);
res = inb_p(data->addr + DATA_REG_OFFSET);
if (word_sized) {
-   outb_p((reg & 0xff) + 1,
-  data->addr + ADDR_REG_OFFSET);
+   outb_p((reg & 0xff) + 1, data->addr + ADDR_REG_OFFSET);
res = (res << 8) + inb_p(data->addr + DATA_REG_OFFSET);
}
 
@@ -563,8 +563,7 @@ static int w83627ehf_write_value(struct w83627ehf_data 
*data, u16 reg,
outb_p(reg & 0xff, data->addr + ADDR_REG_OFFSET);
if (word_sized) {
outb_p(value >> 8, data->addr + DATA_REG_OFFSET);
-   outb_p((reg & 0xff) + 1,
-  data->addr + ADDR_REG_OFFSET);
+   outb_p((reg & 0xff) + 1, data->addr + ADDR_REG_OFFSET);
}
outb_p(value & 0xff, data->addr + DATA_REG_OFFSET);
 
@@ -584,8 +583,7 @@ static u16 w83627ehf_read_temp(struct w83627ehf_data *data, 
u16 reg)
return res;
 }
 
-static int w83627ehf_write_temp(struct w83627ehf_data *data, u16 reg,
-  u16 value)
+static int w83627ehf_write_temp(struct w83627ehf_data *data, u16 reg, u16 
value)
 {
if (!is_word_sized(reg))
value >>= 8;
@@ -694,7 +692,7 @@ static void nct6775_update_fan_div(struct w83627ehf_data 
*data)
data->fan_div[1] = (i & 0x70) >> 4;
i = w83627ehf_read_value(data, NCT6775_REG_FANDIV2);
data->fan_div[2] = i & 0x7;
-   if (data->has_fan & (1<<3))
+   if (data->has_fan & (1 << 3))
data->fan_div[3] = (i & 0x70) >> 4;
 }
 
@@ -931,9 +929,7 @@ static struct w83627ehf_data 
*w83627ehf_update_device(struct device *dev)
return data;
 }
 
-
-static void
-store_fan_min(struct device *dev, u32 channel, unsigned long val)
+static void store_fan_min(struct device *dev, u32 channel, unsigned long val)
 {
struct w83627ehf_data *data = dev_get_drvdata(dev);
int nr = channel;
@@ -1010,10 +1006,8 @@ store_fan_min(struct device *dev, u32 channel, unsigned 
long val)
data->last_updated = jiffies;
}
 done:
-   w83627ehf_write_value(data, data->REG_FAN_MIN[nr],
- data->fan_min[nr]);
+   w83627ehf_write_value(data, data->REG_FAN_MIN[nr], data->fan_min[nr]);
mutex_unlock(>update_lock);
-
 }
 
 #define show_tol_temp(reg) \
@@ -1032,7 +1026,7 @@ show_tol_temp(target_temp)
 
 static ssize_t
 store_target_temp(struct device *dev, struct device_attribute *attr,
-   const char *buf, size_t count)
+ const char *buf, size_t count)
 {
struct w83627ehf_data *data = dev_get_drvdata(dev);
struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
@@ -1055,7 +1049,7 @@ store_target_temp(struct device *dev, struct 
device_attribute *attr,
 
 static ssize_t
 store_tolerance(struct device *dev, struct device_attribute *attr,
-   const char *buf, size_t count)
+   const char *buf, size_t count)
 {
struct w83627ehf_data *data = dev_get_drvdata(dev);
struct w83627ehf_sio_data *sio_data = data->sio_data;
@@ -1239,11 +1233,11 @@ static ssize_t
 cpu0_vid_show(struct device *dev, struct device_attribute *attr, char *buf)
 {
struct w83627ehf_data *data = dev_get_drvdata(dev);
+
return sprintf(buf, "%d\n", vid_from_reg(data->vid, data->vrm));
 }
 static DEVICE_ATTR_RO(cpu0_vid);
 
-
 /* Case open detection */
 
 static ssize_t
@@ -1320,8 +1314,7 @@ static

[PATCH 4/4] w83627ehf: Drop FSFE template and replace with SPDX License information

[Peter Huewe]

As indicated by checkpatch it makes sense to not use the FSFE Template
about GPLv2+

Signed-off-by: Peter Huewe <peterhu...@gmx.de>
---
 drivers/hwmon/w83627ehf.c | 15 ++-
 1 file changed, 2 insertions(+), 13 deletions(-)

diff --git a/drivers/hwmon/w83627ehf.c b/drivers/hwmon/w83627ehf.c
index dbe5735963ac..bba26623af36 100644
--- a/drivers/hwmon/w83627ehf.c
+++ b/drivers/hwmon/w83627ehf.c
@@ -18,19 +18,8 @@
  *  This driver also supports the W83627EHG, which is the lead-free
  *  version of the W83627EHF.
  *
- *  This program is free software; you can redistribute it and/or modify
- *  it under the terms of the GNU General Public License as published by
- *  the Free Software Foundation; either version 2 of the License, or
- *  (at your option) any later version.
- *
- *  This program is distributed in the hope that it will be useful,
- *  but WITHOUT ANY WARRANTY; without even the implied warranty of
- *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- *  GNU General Public License for more details.
- *
- *  You should have received a copy of the GNU General Public License
- *  along with this program; if not, write to the Free Software
- *  Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+ *  Released under the GPLv2 or later.
+ *  SPDX-License-Identifier: GPL-2.0+
  *
  *  Supports the following chips:
  *
-- 
2.10.2

[PATCH 4/4] w83627ehf: Drop FSFE template and replace with SPDX License information

[Peter Huewe]

As indicated by checkpatch it makes sense to not use the FSFE Template
about GPLv2+

Signed-off-by: Peter Huewe 
---
 drivers/hwmon/w83627ehf.c | 15 ++-
 1 file changed, 2 insertions(+), 13 deletions(-)

diff --git a/drivers/hwmon/w83627ehf.c b/drivers/hwmon/w83627ehf.c
index dbe5735963ac..bba26623af36 100644
--- a/drivers/hwmon/w83627ehf.c
+++ b/drivers/hwmon/w83627ehf.c
@@ -18,19 +18,8 @@
  *  This driver also supports the W83627EHG, which is the lead-free
  *  version of the W83627EHF.
  *
- *  This program is free software; you can redistribute it and/or modify
- *  it under the terms of the GNU General Public License as published by
- *  the Free Software Foundation; either version 2 of the License, or
- *  (at your option) any later version.
- *
- *  This program is distributed in the hope that it will be useful,
- *  but WITHOUT ANY WARRANTY; without even the implied warranty of
- *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- *  GNU General Public License for more details.
- *
- *  You should have received a copy of the GNU General Public License
- *  along with this program; if not, write to the Free Software
- *  Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+ *  Released under the GPLv2 or later.
+ *  SPDX-License-Identifier: GPL-2.0+
  *
  *  Supports the following chips:
  *
-- 
2.10.2

[PATCH] hwmon: Add missing HWMON_T_ALARM

[Peter Huewe]

Unfortunately the HWMON_T_ALARM define was missing,
although the associated entry was present in hwmon_temp_attributes.
This is needed to convert drivers to the new interface which use channel
based alarms.

Signed-off-by: Peter Huewe <peterhu...@gmx.de>
---
 include/linux/hwmon.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/linux/hwmon.h b/include/linux/hwmon.h
index 78d59dba563e..88b673749121 100644
--- a/include/linux/hwmon.h
+++ b/include/linux/hwmon.h
@@ -88,6 +88,7 @@ enum hwmon_temp_attributes {
 #define HWMON_T_CRIT_HYST  BIT(hwmon_temp_crit_hyst)
 #define HWMON_T_EMERGENCY  BIT(hwmon_temp_emergency)
 #define HWMON_T_EMERGENCY_HYST BIT(hwmon_temp_emergency_hyst)
+#define HWMON_T_ALARM  BIT(hwmon_temp_alarm)
 #define HWMON_T_MIN_ALARM  BIT(hwmon_temp_min_alarm)
 #define HWMON_T_MAX_ALARM  BIT(hwmon_temp_max_alarm)
 #define HWMON_T_CRIT_ALARM BIT(hwmon_temp_crit_alarm)
-- 
2.7.3

[PATCH] hwmon: Add missing HWMON_T_ALARM

[Peter Huewe]

Unfortunately the HWMON_T_ALARM define was missing,
although the associated entry was present in hwmon_temp_attributes.
This is needed to convert drivers to the new interface which use channel
based alarms.

Signed-off-by: Peter Huewe 
---
 include/linux/hwmon.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/linux/hwmon.h b/include/linux/hwmon.h
index 78d59dba563e..88b673749121 100644
--- a/include/linux/hwmon.h
+++ b/include/linux/hwmon.h
@@ -88,6 +88,7 @@ enum hwmon_temp_attributes {
 #define HWMON_T_CRIT_HYST  BIT(hwmon_temp_crit_hyst)
 #define HWMON_T_EMERGENCY  BIT(hwmon_temp_emergency)
 #define HWMON_T_EMERGENCY_HYST BIT(hwmon_temp_emergency_hyst)
+#define HWMON_T_ALARM  BIT(hwmon_temp_alarm)
 #define HWMON_T_MIN_ALARM  BIT(hwmon_temp_min_alarm)
 #define HWMON_T_MAX_ALARM  BIT(hwmon_temp_max_alarm)
 #define HWMON_T_CRIT_ALARM BIT(hwmon_temp_crit_alarm)
-- 
2.7.3

Re: [tpmdd-devel] [PATCH] tpm: Add sysfs interface to show TPM hardware version

[Peter Huewe]

Am 14. März 2017 19:18:15 MEZ schrieb Ken Goldman <kg...@linux.vnet.ibm.com>:
>On 3/13/2017 3:10 AM, Peter Huewe wrote:
>
>> And yes you are right there is currently no way, except for trial and
>> error, for the userspace to determine this. So an interface to get
>> this information makes sense to me.
>
>In practice, I suspect that a single user space application won't
>support both TPMs. 
Think of init scripts.
Which daemon should it start?


> It will send the first command, get an error
>response code that says it's the wrong TPM, and exit.
>
>Note that, although there is no overlap in the command API, the packet
>format is compatible enough that a meaningful response can be returned.
>
>
>
>
>--
>Check out the vibrant tech community on one of the world's most
>engaging tech sites, Slashdot.org! http://sdm.link/slashdot
>___
>tpmdd-devel mailing list
>tpmdd-de...@lists.sourceforge.net
>https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

-- 
Sent from my mobile

Re: [tpmdd-devel] [PATCH] tpm: Add sysfs interface to show TPM hardware version

[Peter Huewe]

Am 14. März 2017 19:18:15 MEZ schrieb Ken Goldman :
>On 3/13/2017 3:10 AM, Peter Huewe wrote:
>
>> And yes you are right there is currently no way, except for trial and
>> error, for the userspace to determine this. So an interface to get
>> this information makes sense to me.
>
>In practice, I suspect that a single user space application won't
>support both TPMs. 
Think of init scripts.
Which daemon should it start?


> It will send the first command, get an error
>response code that says it's the wrong TPM, and exit.
>
>Note that, although there is no overlap in the command API, the packet
>format is compatible enough that a meaningful response can be returned.
>
>
>
>
>--
>Check out the vibrant tech community on one of the world's most
>engaging tech sites, Slashdot.org! http://sdm.link/slashdot
>___
>tpmdd-devel mailing list
>tpmdd-de...@lists.sourceforge.net
>https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

-- 
Sent from my mobile

Re: [PATCH] tpm: Add sysfs interface to show TPM hardware version

[Peter Huewe]

Hi,
Thanks for your patch.

Am 13. März 2017 06:21:57 MEZ schrieb meng...@windriver.com:
>From: Limeng 
>
>So far, there is not a sysfs interface for user space code to
>check the TPM hardware version(TPM1.x or TPM2). So, add a
>file named description in /sys/class/tpm/tpmX/ to show it.
It's not really the hardware version but the "TPM Family" according to tcg.

And yes you are right there is currently no way, except for trial and error, 
for the userspace to determine this.
So an interface to get this information makes sense to me.
>
>Signed-off-by: Meng Li 
>---
>drivers/char/tpm/tpm-chip.c |   85
>+++
> 1 file changed, 85 insertions(+)
>
>diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
>index c406343..da2cd69 100644
>--- a/drivers/char/tpm/tpm-chip.c
>+++ b/drivers/char/tpm/tpm-chip.c
>@@ -36,6 +36,83 @@
> dev_t tpm_devt;
> 
> /**
>+ * show_description - sysfs interface for checking current TPM
>hardware version.
>+ * @dev:  pointer to tpm chip device
>+ * @attr: unused
>+ * @buf:  char buffer to be filled with TPM hardware version info
>+ *
>+ * Provides sysfs interface for showing current TPM hardware version.
>+ */
>+static ssize_t show_description(struct device *dev,
>+  struct device_attribute *attr, char *buf)
>+{
>+  struct tpm_chip *chip = (struct tpm_chip *)container_of(dev,struct
>tpm_chip,dev);
>+  int ret;
>+
>+  if (chip->flags & TPM_CHIP_FLAG_TPM2)
>+  ret = sprintf(buf, "TPM 2.0");
>+  else
>+  ret = sprintf(buf, "TPM 1.x");
>+
>+  return ret;
>+}
>+
>+/**
>+ * store_description - interface for manually setting data.
>+ * @dev:  unused
>+ * @attr: unused
>+ * @buf:  unused
>+ * @count:unused
>+ *
>+ * There is not any process in this function, reserve for feature.
>+ */
>+static ssize_t store_description(struct device *dev, struct
>device_attribute *attr,
>+  const char *buf, size_t count)
>+{
>+  return count;
>+}
Since it does not do anything
I would not create this function and leave the sysfs node as S_IRUGO.

>+
>+static struct device_attribute tpm_attrs[] = {
>+  __ATTR(description, S_IRUGO | S_IWUSR, show_description,
>store_description),
>+};
>+
>+/**
>+ * tpm_create_sysfs - Create tpm sysfs interface.
>+ * @dev:  pointer to tpm chip device
>+ *
>+ * Create sysfs interface for checking current TPM hardware version.
>+ */
>+static int tpm_create_sysfs(struct device *dev)
>+{
>+  int r, t;
>+
>+  for (t = 0; t < ARRAY_SIZE(tpm_attrs); t++) {
>+  r = device_create_file(dev, _attrs[t]);
>+  if (r) {
>+  dev_err(dev, "failed to create sysfs file\n");
>+  return r;
>+  }
>+  }
>+
>+  return 0;
>+}
>+
>+/**
>+ * tpm_remove_sysfs - Remove tpm sysfs interface.
>+ * @dev:  pointer to tpm chip device
>+ *
>+ * Remove sysfs interface for checking current TPM hardware version.
>+ */
>+static void tpm_remove_sysfs(struct device *dev)
>+{
>+  int  t;
>+
>+  for (t = 0; t < ARRAY_SIZE(tpm_attrs); t++) {
>+  device_remove_file(dev, _attrs[t]);
>+  }
>+}
>+
>+/**
>  * tpm_try_get_ops() - Get a ref to the tpm_chip
>  * @chip: Chip to ref
>  *
>@@ -363,6 +440,13 @@ int tpm_chip_register(struct tpm_chip *chip)
>   return rc;
>   }
> 
>+  rc = tpm_create_sysfs(>dev);
>+  if (rc) {
>+  tpm_del_legacy_sysfs(chip);
>+  tpm_chip_unregister(chip);
>+  return rc;
>+  }
>+
>   return 0;
> }
> EXPORT_SYMBOL_GPL(tpm_chip_register);
>@@ -382,6 +466,7 @@ int tpm_chip_register(struct tpm_chip *chip)
>  */
> void tpm_chip_unregister(struct tpm_chip *chip)
> {
>+  tpm_remove_sysfs(>dev);
>   tpm_del_legacy_sysfs(chip);
>   tpm_bios_log_teardown(chip);
>   tpm_del_char_device(chip);

-- 
Sent from my mobile

Re: [PATCH] tpm: Add sysfs interface to show TPM hardware version

[Peter Huewe]

Hi,
Thanks for your patch.

Am 13. März 2017 06:21:57 MEZ schrieb meng...@windriver.com:
>From: Limeng 
>
>So far, there is not a sysfs interface for user space code to
>check the TPM hardware version(TPM1.x or TPM2). So, add a
>file named description in /sys/class/tpm/tpmX/ to show it.
It's not really the hardware version but the "TPM Family" according to tcg.

And yes you are right there is currently no way, except for trial and error, 
for the userspace to determine this.
So an interface to get this information makes sense to me.
>
>Signed-off-by: Meng Li 
>---
>drivers/char/tpm/tpm-chip.c |   85
>+++
> 1 file changed, 85 insertions(+)
>
>diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
>index c406343..da2cd69 100644
>--- a/drivers/char/tpm/tpm-chip.c
>+++ b/drivers/char/tpm/tpm-chip.c
>@@ -36,6 +36,83 @@
> dev_t tpm_devt;
> 
> /**
>+ * show_description - sysfs interface for checking current TPM
>hardware version.
>+ * @dev:  pointer to tpm chip device
>+ * @attr: unused
>+ * @buf:  char buffer to be filled with TPM hardware version info
>+ *
>+ * Provides sysfs interface for showing current TPM hardware version.
>+ */
>+static ssize_t show_description(struct device *dev,
>+  struct device_attribute *attr, char *buf)
>+{
>+  struct tpm_chip *chip = (struct tpm_chip *)container_of(dev,struct
>tpm_chip,dev);
>+  int ret;
>+
>+  if (chip->flags & TPM_CHIP_FLAG_TPM2)
>+  ret = sprintf(buf, "TPM 2.0");
>+  else
>+  ret = sprintf(buf, "TPM 1.x");
>+
>+  return ret;
>+}
>+
>+/**
>+ * store_description - interface for manually setting data.
>+ * @dev:  unused
>+ * @attr: unused
>+ * @buf:  unused
>+ * @count:unused
>+ *
>+ * There is not any process in this function, reserve for feature.
>+ */
>+static ssize_t store_description(struct device *dev, struct
>device_attribute *attr,
>+  const char *buf, size_t count)
>+{
>+  return count;
>+}
Since it does not do anything
I would not create this function and leave the sysfs node as S_IRUGO.

>+
>+static struct device_attribute tpm_attrs[] = {
>+  __ATTR(description, S_IRUGO | S_IWUSR, show_description,
>store_description),
>+};
>+
>+/**
>+ * tpm_create_sysfs - Create tpm sysfs interface.
>+ * @dev:  pointer to tpm chip device
>+ *
>+ * Create sysfs interface for checking current TPM hardware version.
>+ */
>+static int tpm_create_sysfs(struct device *dev)
>+{
>+  int r, t;
>+
>+  for (t = 0; t < ARRAY_SIZE(tpm_attrs); t++) {
>+  r = device_create_file(dev, _attrs[t]);
>+  if (r) {
>+  dev_err(dev, "failed to create sysfs file\n");
>+  return r;
>+  }
>+  }
>+
>+  return 0;
>+}
>+
>+/**
>+ * tpm_remove_sysfs - Remove tpm sysfs interface.
>+ * @dev:  pointer to tpm chip device
>+ *
>+ * Remove sysfs interface for checking current TPM hardware version.
>+ */
>+static void tpm_remove_sysfs(struct device *dev)
>+{
>+  int  t;
>+
>+  for (t = 0; t < ARRAY_SIZE(tpm_attrs); t++) {
>+  device_remove_file(dev, _attrs[t]);
>+  }
>+}
>+
>+/**
>  * tpm_try_get_ops() - Get a ref to the tpm_chip
>  * @chip: Chip to ref
>  *
>@@ -363,6 +440,13 @@ int tpm_chip_register(struct tpm_chip *chip)
>   return rc;
>   }
> 
>+  rc = tpm_create_sysfs(>dev);
>+  if (rc) {
>+  tpm_del_legacy_sysfs(chip);
>+  tpm_chip_unregister(chip);
>+  return rc;
>+  }
>+
>   return 0;
> }
> EXPORT_SYMBOL_GPL(tpm_chip_register);
>@@ -382,6 +466,7 @@ int tpm_chip_register(struct tpm_chip *chip)
>  */
> void tpm_chip_unregister(struct tpm_chip *chip)
> {
>+  tpm_remove_sysfs(>dev);
>   tpm_del_legacy_sysfs(chip);
>   tpm_bios_log_teardown(chip);
>   tpm_del_char_device(chip);

-- 
Sent from my mobile

Re: Question about hwmon_attr_show_string

[Peter Huewe]

Am 7. März 2017 10:08:46 MEZ schrieb Jean Delvare <jdelv...@suse.de>:
>Hi Guenter,
>
>On Mon, 6 Mar 2017 15:47:55 -0800, Guenter Roeck wrote:
>> On Mon, Mar 06, 2017 at 09:48:35PM +0100, Peter Hüwe wrote:
>> > Hi Guenter,
>> > 
>> > I was wondering whether there was a particular reason why 
>> > hwmon_attr_show_string passes only an "empty" pointer(pointer) to
>the ops-
>> > >read_string function rather than the buffer itself?
>> > 
>> > Wouldn't this mean that in ops->read_string I'd have to reserve
>some space for 
>> > the value on the heap (and taking care to free it somewhere, since
>returning 
>> > an address on the stack is bad idea), instead of calling
>sprintf(buf, "%s\n", 
>> > s) directly?
>> > 
>> > With the current implementation I have to sprintf it into my local
>buffer and 
>> > you sprintf it again into the final buffer.
>>
>> The idea was that the called code would return a pointer to a
>constant string,
>> ie one that isn't changing from call to call.
>
>In that case, what about the following change?
>
>Subject: hwmon: Constify str parameter of hwmon_ops->read_string
>
>The read_string callback is supposed to retrieve a pointer to a
>constant string.

I would think that clarifies the situation and also gets rid warnings about 
dropping const qualifier at the point of assignment.


Reviewed-by: Peter Huewe <peterhu...@gmx.de>


>
>Signed-off-by: Jean Delvare <jdelv...@suse.de>
>---
> drivers/hwmon/hwmon.c |2 +-
> include/linux/hwmon.h |2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
>--- linux-4.10.orig/drivers/hwmon/hwmon.c  2017-02-19 23:34:00.0
>+0100
>+++ linux-4.10/drivers/hwmon/hwmon.c   2017-03-07 08:22:27.784527968
>+0100
>@@ -186,7 +186,7 @@ static ssize_t hwmon_attr_show_string(st
> char *buf)
> {
>   struct hwmon_device_attribute *hattr = to_hwmon_attr(devattr);
>-  char *s;
>+  const char *s;
>   int ret;
> 
>   ret = hattr->ops->read_string(dev, hattr->type, hattr->attr,
>--- linux-4.10.orig/include/linux/hwmon.h  2017-02-19 23:34:00.0
>+0100
>+++ linux-4.10/include/linux/hwmon.h   2017-03-07 08:21:28.247998585
>+0100
>@@ -336,7 +336,7 @@ struct hwmon_ops {
>   int (*read)(struct device *dev, enum hwmon_sensor_types type,
>   u32 attr, int channel, long *val);
>   int (*read_string)(struct device *dev, enum hwmon_sensor_types type,
>-  u32 attr, int channel, char **str);
>+  u32 attr, int channel, const char **str);
>   int (*write)(struct device *dev, enum hwmon_sensor_types type,
>u32 attr, int channel, long val);
> };

-- 
Sent from my mobile

Re: Question about hwmon_attr_show_string

[Peter Huewe]

Am 7. März 2017 10:08:46 MEZ schrieb Jean Delvare :
>Hi Guenter,
>
>On Mon, 6 Mar 2017 15:47:55 -0800, Guenter Roeck wrote:
>> On Mon, Mar 06, 2017 at 09:48:35PM +0100, Peter Hüwe wrote:
>> > Hi Guenter,
>> > 
>> > I was wondering whether there was a particular reason why 
>> > hwmon_attr_show_string passes only an "empty" pointer(pointer) to
>the ops-
>> > >read_string function rather than the buffer itself?
>> > 
>> > Wouldn't this mean that in ops->read_string I'd have to reserve
>some space for 
>> > the value on the heap (and taking care to free it somewhere, since
>returning 
>> > an address on the stack is bad idea), instead of calling
>sprintf(buf, "%s\n", 
>> > s) directly?
>> > 
>> > With the current implementation I have to sprintf it into my local
>buffer and 
>> > you sprintf it again into the final buffer.
>>
>> The idea was that the called code would return a pointer to a
>constant string,
>> ie one that isn't changing from call to call.
>
>In that case, what about the following change?
>
>Subject: hwmon: Constify str parameter of hwmon_ops->read_string
>
>The read_string callback is supposed to retrieve a pointer to a
>constant string.

I would think that clarifies the situation and also gets rid warnings about 
dropping const qualifier at the point of assignment.


Reviewed-by: Peter Huewe 


>
>Signed-off-by: Jean Delvare 
>---
> drivers/hwmon/hwmon.c |2 +-
> include/linux/hwmon.h |2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
>--- linux-4.10.orig/drivers/hwmon/hwmon.c  2017-02-19 23:34:00.0
>+0100
>+++ linux-4.10/drivers/hwmon/hwmon.c   2017-03-07 08:22:27.784527968
>+0100
>@@ -186,7 +186,7 @@ static ssize_t hwmon_attr_show_string(st
> char *buf)
> {
>   struct hwmon_device_attribute *hattr = to_hwmon_attr(devattr);
>-  char *s;
>+  const char *s;
>   int ret;
> 
>   ret = hattr->ops->read_string(dev, hattr->type, hattr->attr,
>--- linux-4.10.orig/include/linux/hwmon.h  2017-02-19 23:34:00.0
>+0100
>+++ linux-4.10/include/linux/hwmon.h   2017-03-07 08:21:28.247998585
>+0100
>@@ -336,7 +336,7 @@ struct hwmon_ops {
>   int (*read)(struct device *dev, enum hwmon_sensor_types type,
>   u32 attr, int channel, long *val);
>   int (*read_string)(struct device *dev, enum hwmon_sensor_types type,
>-  u32 attr, int channel, char **str);
>+  u32 attr, int channel, const char **str);
>   int (*write)(struct device *dev, enum hwmon_sensor_types type,
>u32 attr, int channel, long val);
> };

-- 
Sent from my mobile

[PATCH v2 3/5] tpm_tis_spi: Check correct byte for wait state indicator

[Peter Huewe]

Wait states are signaled in the last byte received from the TPM in
response to the header, not the first byte. Check rx_buf[3] instead of
rx_buf[0].

Cc: <sta...@vger.kernel.org>
Fixes: 0edbfea537d1 ("tpm/tpm_tis_spi: Add support for spi phy")
Signed-off-by: Alexander Steffen <alexander.stef...@infineon.com>
Signed-off-by: Peter Huewe <peter.hu...@infineon.com>
Reviewed-by: Jarkko Sakkinen <jarkko.sakki...@linux.intel.com>
Tested-by: Benoit Houyere <benoit.houy...@st.com>
---
 drivers/char/tpm/tpm_tis_spi.c | 36 ++--
 1 file changed, 18 insertions(+), 18 deletions(-)

diff --git a/drivers/char/tpm/tpm_tis_spi.c b/drivers/char/tpm/tpm_tis_spi.c
index 639614f2d415..62f50b6c9ef6 100644
--- a/drivers/char/tpm/tpm_tis_spi.c
+++ b/drivers/char/tpm/tpm_tis_spi.c
@@ -85,25 +85,25 @@ static int tpm_tis_spi_transfer(struct tpm_tis_data *data, 
u32 addr, u16 len,
if (ret < 0)
goto exit;
 
-   phy->tx_buf[0] = 0;
-
-   /* According to TCG PTP specification, if there is no TPM present at
-* all, then the design has a weak pull-up on MISO. If a TPM is not
-* present, a pull-up on MISO means that the SB controller sees a 1,
-* and will latch in 0xFF on the read.
-*/
-   for (i = 0; (phy->rx_buf[0] & 0x01) == 0 && i < TPM_RETRY; i++) {
-   spi_xfer.len = 1;
-   spi_message_init();
-   spi_message_add_tail(_xfer, );
-   ret = spi_sync_locked(phy->spi_device, );
-   if (ret < 0)
+   if ((phy->rx_buf[3] & 0x01) == 0) {
+   // handle SPI wait states
+   phy->tx_buf[0] = 0;
+
+   for (i = 0; i < TPM_RETRY; i++) {
+   spi_xfer.len = 1;
+   spi_message_init();
+   spi_message_add_tail(_xfer, );
+   ret = spi_sync_locked(phy->spi_device, );
+   if (ret < 0)
+   goto exit;
+   if (phy->rx_buf[0] & 0x01)
+   break;
+   }
+
+   if (i == TPM_RETRY) {
+   ret = -ETIMEDOUT;
goto exit;
-   }
-
-   if (i == TPM_RETRY) {
-   ret = -ETIMEDOUT;
-   goto exit;
+   }
}
 
spi_xfer.cs_change = 0;
-- 
2.7.4

[PATCH v2 3/5] tpm_tis_spi: Check correct byte for wait state indicator

[Peter Huewe]

Wait states are signaled in the last byte received from the TPM in
response to the header, not the first byte. Check rx_buf[3] instead of
rx_buf[0].

Cc: 
Fixes: 0edbfea537d1 ("tpm/tpm_tis_spi: Add support for spi phy")
Signed-off-by: Alexander Steffen 
Signed-off-by: Peter Huewe 
Reviewed-by: Jarkko Sakkinen 
Tested-by: Benoit Houyere 
---
 drivers/char/tpm/tpm_tis_spi.c | 36 ++--
 1 file changed, 18 insertions(+), 18 deletions(-)

diff --git a/drivers/char/tpm/tpm_tis_spi.c b/drivers/char/tpm/tpm_tis_spi.c
index 639614f2d415..62f50b6c9ef6 100644
--- a/drivers/char/tpm/tpm_tis_spi.c
+++ b/drivers/char/tpm/tpm_tis_spi.c
@@ -85,25 +85,25 @@ static int tpm_tis_spi_transfer(struct tpm_tis_data *data, 
u32 addr, u16 len,
if (ret < 0)
goto exit;
 
-   phy->tx_buf[0] = 0;
-
-   /* According to TCG PTP specification, if there is no TPM present at
-* all, then the design has a weak pull-up on MISO. If a TPM is not
-* present, a pull-up on MISO means that the SB controller sees a 1,
-* and will latch in 0xFF on the read.
-*/
-   for (i = 0; (phy->rx_buf[0] & 0x01) == 0 && i < TPM_RETRY; i++) {
-   spi_xfer.len = 1;
-   spi_message_init();
-   spi_message_add_tail(_xfer, );
-   ret = spi_sync_locked(phy->spi_device, );
-   if (ret < 0)
+   if ((phy->rx_buf[3] & 0x01) == 0) {
+   // handle SPI wait states
+   phy->tx_buf[0] = 0;
+
+   for (i = 0; i < TPM_RETRY; i++) {
+   spi_xfer.len = 1;
+   spi_message_init();
+   spi_message_add_tail(_xfer, );
+   ret = spi_sync_locked(phy->spi_device, );
+   if (ret < 0)
+   goto exit;
+   if (phy->rx_buf[0] & 0x01)
+   break;
+   }
+
+   if (i == TPM_RETRY) {
+   ret = -ETIMEDOUT;
goto exit;
-   }
-
-   if (i == TPM_RETRY) {
-   ret = -ETIMEDOUT;
-   goto exit;
+   }
}
 
spi_xfer.cs_change = 0;
-- 
2.7.4

[PATCH v2 1/5] tpm_tis_spi: Use single function to transfer data

[Peter Huewe]

The algorithm for sending data to the TPM is mostly identical to the
algorithm for receiving data from the TPM, so a single function is
sufficient to handle both cases.

This is a prequisite for all the other fixes, so we don't have to fix
everything twice (send/receive)

v2: u16 instead of u8 for the length.
Cc: <sta...@vger.kernel.org>
Fixes: 0edbfea537d1 ("tpm/tpm_tis_spi: Add support for spi phy")
Signed-off-by: Alexander Steffen <alexander.stef...@infineon.com>
Signed-off-by: Peter Huewe <peter.hu...@infineon.com>
Reviewed-by: Jarkko Sakkinen <jarkko.sakki...@linux.intel.com>
Tested-by: Benoit Houyere <benoit.houy...@st.com>
---
 drivers/char/tpm/tpm_tis_spi.c | 87 --
 1 file changed, 24 insertions(+), 63 deletions(-)

diff --git a/drivers/char/tpm/tpm_tis_spi.c b/drivers/char/tpm/tpm_tis_spi.c
index 5292e5768a7e..062799e04f04 100644
--- a/drivers/char/tpm/tpm_tis_spi.c
+++ b/drivers/char/tpm/tpm_tis_spi.c
@@ -47,8 +47,8 @@ struct tpm_tis_spi_phy {
struct tpm_tis_data priv;
struct spi_device *spi_device;
 
-   u8 tx_buf[MAX_SPI_FRAMESIZE + 4];
-   u8 rx_buf[MAX_SPI_FRAMESIZE + 4];
+   u8 tx_buf[4];
+   u8 rx_buf[4];
 };
 
 static inline struct tpm_tis_spi_phy *to_tpm_tis_spi_phy(struct tpm_tis_data 
*data)
@@ -56,8 +56,8 @@ static inline struct tpm_tis_spi_phy 
*to_tpm_tis_spi_phy(struct tpm_tis_data *da
return container_of(data, struct tpm_tis_spi_phy, priv);
 }
 
-static int tpm_tis_spi_read_bytes(struct tpm_tis_data *data, u32 addr,
- u16 len, u8 *result)
+static int tpm_tis_spi_transfer(struct tpm_tis_data *data, u32 addr, u16 len,
+   u8 *buffer, u8 direction)
 {
struct tpm_tis_spi_phy *phy = to_tpm_tis_spi_phy(data);
int ret, i;
@@ -66,17 +66,17 @@ static int tpm_tis_spi_read_bytes(struct tpm_tis_data 
*data, u32 addr,
.tx_buf = phy->tx_buf,
.rx_buf = phy->rx_buf,
.len = 4,
+   .cs_change = 1,
};
 
if (len > MAX_SPI_FRAMESIZE)
return -ENOMEM;
 
-   phy->tx_buf[0] = 0x80 | (len - 1);
+   phy->tx_buf[0] = direction | (len - 1);
phy->tx_buf[1] = 0xd4;
-   phy->tx_buf[2] = (addr >> 8)  & 0xFF;
-   phy->tx_buf[3] = addr & 0xFF;
+   phy->tx_buf[2] = addr >> 8;
+   phy->tx_buf[3] = addr;
 
-   spi_xfer.cs_change = 1;
spi_message_init();
spi_message_add_tail(_xfer, );
 
@@ -85,7 +85,7 @@ static int tpm_tis_spi_read_bytes(struct tpm_tis_data *data, 
u32 addr,
if (ret < 0)
goto exit;
 
-   memset(phy->tx_buf, 0, len);
+   phy->tx_buf[0] = 0;
 
/* According to TCG PTP specification, if there is no TPM present at
 * all, then the design has a weak pull-up on MISO. If a TPM is not
@@ -103,7 +103,14 @@ static int tpm_tis_spi_read_bytes(struct tpm_tis_data 
*data, u32 addr,
 
spi_xfer.cs_change = 0;
spi_xfer.len = len;
-   spi_xfer.rx_buf = result;
+
+   if (direction) {
+   spi_xfer.tx_buf = NULL;
+   spi_xfer.rx_buf = buffer;
+   } else {
+   spi_xfer.tx_buf = buffer;
+   spi_xfer.rx_buf = NULL;
+   }
 
spi_message_init();
spi_message_add_tail(_xfer, );
@@ -114,62 +121,16 @@ static int tpm_tis_spi_read_bytes(struct tpm_tis_data 
*data, u32 addr,
return ret;
 }
 
+static int tpm_tis_spi_read_bytes(struct tpm_tis_data *data, u32 addr,
+ u16 len, u8 *result)
+{
+   return tpm_tis_spi_transfer(data, addr, len, result, 0x80);
+}
+
 static int tpm_tis_spi_write_bytes(struct tpm_tis_data *data, u32 addr,
   u16 len, u8 *value)
 {
-   struct tpm_tis_spi_phy *phy = to_tpm_tis_spi_phy(data);
-   int ret, i;
-   struct spi_message m;
-   struct spi_transfer spi_xfer = {
-   .tx_buf = phy->tx_buf,
-   .rx_buf = phy->rx_buf,
-   .len = 4,
-   };
-
-   if (len > MAX_SPI_FRAMESIZE)
-   return -ENOMEM;
-
-   phy->tx_buf[0] = len - 1;
-   phy->tx_buf[1] = 0xd4;
-   phy->tx_buf[2] = (addr >> 8)  & 0xFF;
-   phy->tx_buf[3] = addr & 0xFF;
-
-   spi_xfer.cs_change = 1;
-   spi_message_init();
-   spi_message_add_tail(_xfer, );
-
-   spi_bus_lock(phy->spi_device->master);
-   ret = spi_sync_locked(phy->spi_device, );
-   if (ret < 0)
-   goto exit;
-
-   memset(phy->tx_buf, 0, len);
-
-   /* According to TCG PTP specification, if there is no TPM present at
-* all, then the design has a weak pull-up on MISO. If a TPM is not
-* present, a pull-up on MISO means that the SB controller sees a 1,
-* and w

[PATCH v2 1/5] tpm_tis_spi: Use single function to transfer data

[Peter Huewe]

The algorithm for sending data to the TPM is mostly identical to the
algorithm for receiving data from the TPM, so a single function is
sufficient to handle both cases.

This is a prequisite for all the other fixes, so we don't have to fix
everything twice (send/receive)

v2: u16 instead of u8 for the length.
Cc: 
Fixes: 0edbfea537d1 ("tpm/tpm_tis_spi: Add support for spi phy")
Signed-off-by: Alexander Steffen 
Signed-off-by: Peter Huewe 
Reviewed-by: Jarkko Sakkinen 
Tested-by: Benoit Houyere 
---
 drivers/char/tpm/tpm_tis_spi.c | 87 --
 1 file changed, 24 insertions(+), 63 deletions(-)

diff --git a/drivers/char/tpm/tpm_tis_spi.c b/drivers/char/tpm/tpm_tis_spi.c
index 5292e5768a7e..062799e04f04 100644
--- a/drivers/char/tpm/tpm_tis_spi.c
+++ b/drivers/char/tpm/tpm_tis_spi.c
@@ -47,8 +47,8 @@ struct tpm_tis_spi_phy {
struct tpm_tis_data priv;
struct spi_device *spi_device;
 
-   u8 tx_buf[MAX_SPI_FRAMESIZE + 4];
-   u8 rx_buf[MAX_SPI_FRAMESIZE + 4];
+   u8 tx_buf[4];
+   u8 rx_buf[4];
 };
 
 static inline struct tpm_tis_spi_phy *to_tpm_tis_spi_phy(struct tpm_tis_data 
*data)
@@ -56,8 +56,8 @@ static inline struct tpm_tis_spi_phy 
*to_tpm_tis_spi_phy(struct tpm_tis_data *da
return container_of(data, struct tpm_tis_spi_phy, priv);
 }
 
-static int tpm_tis_spi_read_bytes(struct tpm_tis_data *data, u32 addr,
- u16 len, u8 *result)
+static int tpm_tis_spi_transfer(struct tpm_tis_data *data, u32 addr, u16 len,
+   u8 *buffer, u8 direction)
 {
struct tpm_tis_spi_phy *phy = to_tpm_tis_spi_phy(data);
int ret, i;
@@ -66,17 +66,17 @@ static int tpm_tis_spi_read_bytes(struct tpm_tis_data 
*data, u32 addr,
.tx_buf = phy->tx_buf,
.rx_buf = phy->rx_buf,
.len = 4,
+   .cs_change = 1,
};
 
if (len > MAX_SPI_FRAMESIZE)
return -ENOMEM;
 
-   phy->tx_buf[0] = 0x80 | (len - 1);
+   phy->tx_buf[0] = direction | (len - 1);
phy->tx_buf[1] = 0xd4;
-   phy->tx_buf[2] = (addr >> 8)  & 0xFF;
-   phy->tx_buf[3] = addr & 0xFF;
+   phy->tx_buf[2] = addr >> 8;
+   phy->tx_buf[3] = addr;
 
-   spi_xfer.cs_change = 1;
spi_message_init();
spi_message_add_tail(_xfer, );
 
@@ -85,7 +85,7 @@ static int tpm_tis_spi_read_bytes(struct tpm_tis_data *data, 
u32 addr,
if (ret < 0)
goto exit;
 
-   memset(phy->tx_buf, 0, len);
+   phy->tx_buf[0] = 0;
 
/* According to TCG PTP specification, if there is no TPM present at
 * all, then the design has a weak pull-up on MISO. If a TPM is not
@@ -103,7 +103,14 @@ static int tpm_tis_spi_read_bytes(struct tpm_tis_data 
*data, u32 addr,
 
spi_xfer.cs_change = 0;
spi_xfer.len = len;
-   spi_xfer.rx_buf = result;
+
+   if (direction) {
+   spi_xfer.tx_buf = NULL;
+   spi_xfer.rx_buf = buffer;
+   } else {
+   spi_xfer.tx_buf = buffer;
+   spi_xfer.rx_buf = NULL;
+   }
 
spi_message_init();
spi_message_add_tail(_xfer, );
@@ -114,62 +121,16 @@ static int tpm_tis_spi_read_bytes(struct tpm_tis_data 
*data, u32 addr,
return ret;
 }
 
+static int tpm_tis_spi_read_bytes(struct tpm_tis_data *data, u32 addr,
+ u16 len, u8 *result)
+{
+   return tpm_tis_spi_transfer(data, addr, len, result, 0x80);
+}
+
 static int tpm_tis_spi_write_bytes(struct tpm_tis_data *data, u32 addr,
   u16 len, u8 *value)
 {
-   struct tpm_tis_spi_phy *phy = to_tpm_tis_spi_phy(data);
-   int ret, i;
-   struct spi_message m;
-   struct spi_transfer spi_xfer = {
-   .tx_buf = phy->tx_buf,
-   .rx_buf = phy->rx_buf,
-   .len = 4,
-   };
-
-   if (len > MAX_SPI_FRAMESIZE)
-   return -ENOMEM;
-
-   phy->tx_buf[0] = len - 1;
-   phy->tx_buf[1] = 0xd4;
-   phy->tx_buf[2] = (addr >> 8)  & 0xFF;
-   phy->tx_buf[3] = addr & 0xFF;
-
-   spi_xfer.cs_change = 1;
-   spi_message_init();
-   spi_message_add_tail(_xfer, );
-
-   spi_bus_lock(phy->spi_device->master);
-   ret = spi_sync_locked(phy->spi_device, );
-   if (ret < 0)
-   goto exit;
-
-   memset(phy->tx_buf, 0, len);
-
-   /* According to TCG PTP specification, if there is no TPM present at
-* all, then the design has a weak pull-up on MISO. If a TPM is not
-* present, a pull-up on MISO means that the SB controller sees a 1,
-* and will latch in 0xFF on the read.
-*/
-   for (i = 0; (phy->rx_buf[0] & 0x01) == 0 && i < TPM_RETRY; i++) {
-   spi_xfer.len = 1;

Re: [tpmdd-devel] [PATCH v2] tpm: Apply a sane minimum adapterlimit value for retransmission.

[Peter Huewe]

Am 2. März 2017 13:55:43 MEZ schrieb Jarkko Sakkinen 
:
>On Wed, Mar 01, 2017 at 04:36:17PM +0100, Enric Balletbo i Serra wrote:
>> From: Bryan Freed 
>> 
>> When the I2C Infineon part is attached to an I2C adapter that imposes
>> a size limitation, large requests will fail with -EOPNOTSUPP. Retry
>> them with a sane minimum size without re-issuing the 0x05 command
>> as this appears to occasionally put the TPM in a bad state.
>
>What is 0x05 command?
0x05 is the address of the fifo.
I honestly think that it needs toll​ be repeated after a stop condition.
I'll look that up.

>
>/Jarkko
>
>> Signed-off-by: Bryan Freed 
>> [rework the patch to adapt to the feedback received]
>> Signed-off-by: Enric Balletbo i Serra 
>> ---
>> This is a reworked version of the original patch based on the
>> suggestion made by Wolfram Sang to simply fall back to a sane minimum
>> when the maximum fails.
>> 
>> Changes since v1:
>>  - Check the correct return value (-EOPNOTSUPP instead of -EINVAL)
>>  - Fall back len to I2C_SMBUS_BLOCK_MAX if fails.
>> 
>>  drivers/char/tpm/tpm_i2c_infineon.c | 45
>+
>>  1 file changed, 26 insertions(+), 19 deletions(-)
>> 
>> diff --git a/drivers/char/tpm/tpm_i2c_infineon.c
>b/drivers/char/tpm/tpm_i2c_infineon.c
>> index 62ee44e..88bf947 100644
>> --- a/drivers/char/tpm/tpm_i2c_infineon.c
>> +++ b/drivers/char/tpm/tpm_i2c_infineon.c
>> @@ -107,39 +107,27 @@ static int iic_tpm_read(u8 addr, u8 *buffer,
>size_t len)
>>  .len = len,
>>  .buf = buffer
>>  };
>> -struct i2c_msg msgs[] = {msg1, msg2};
>>  
>>  int rc = 0;
>>  int count;
>> +unsigned int adapterlimit = len;
>>  
>>  /* Lock the adapter for the duration of the whole sequence. */
>>  if (!tpm_dev.client->adapter->algo->master_xfer)
>>  return -EOPNOTSUPP;
>>  i2c_lock_adapter(tpm_dev.client->adapter);
>>  
>> -if (tpm_dev.chip_type == SLB9645) {
Why are you / bryan removing this code path here?
I put it there for a good reason (i.e. faster transfers)

Thanks,
Peter
>> -/* use a combined read for newer chips
>> - * unfortunately the smbus functions are not suitable due to
>> - * the 32 byte limit of the smbus.
>> - * retries should usually not be needed, but are kept just to
>> - * be on the safe side.
>> - */
>> -for (count = 0; count < MAX_COUNT; count++) {
>> -rc = __i2c_transfer(tpm_dev.client->adapter, msgs, 2);
>> -if (rc > 0)
>> -break;  /* break here to skip sleep */
>> -usleep_range(SLEEP_DURATION_LOW, SLEEP_DURATION_HI);
>> -}
>> -} else {
>> +/* Expect to send one command message and one data message, but
>> + * support looping over each or both if necessary.
>> + */
>> +while (len > 0) {
>>  /* slb9635 protocol should work in all cases */
>>  for (count = 0; count < MAX_COUNT; count++) {
>>  rc = __i2c_transfer(tpm_dev.client->adapter, , 1);
>>  if (rc > 0)
>> -break;  /* break here to skip sleep */
>> -
>> +break;
>>  usleep_range(SLEEP_DURATION_LOW, SLEEP_DURATION_HI);
>>  }
>> -
>>  if (rc <= 0)
>>  goto out;
>>  
>> @@ -148,11 +136,30 @@ static int iic_tpm_read(u8 addr, u8 *buffer,
>size_t len)
>>   * retrieving the data
>>   */
>>  for (count = 0; count < MAX_COUNT; count++) {
>> +unsigned int msglen = msg2.len =
>> +min_t(unsigned int, adapterlimit, len);
>>  usleep_range(SLEEP_DURATION_LOW, SLEEP_DURATION_HI);
>>  rc = __i2c_transfer(tpm_dev.client->adapter, , 1);
>> -if (rc > 0)
>> +if (rc > 0) {
>> +/* Since len is unsigned, make doubly sure we
>> + * do not underflow it.
>> + */
>> +if (msglen > len)
>> +len = 0;
>> +else
>> +len -= msglen;
>> +msg2.buf += msglen;
>>  break;
>> +}
>> +/* If the I2C adapter rejected the request (e.g when
>> + * the quirk read_max_len < len) fall back to a sane
>> + * minimum value and try again.
>> + */
>> +if (rc == -EOPNOTSUPP)
>> +adapterlimit = I2C_SMBUS_BLOCK_MAX;
>>  }
>> +if (rc <= 

Re: [tpmdd-devel] [PATCH v2] tpm: Apply a sane minimum adapterlimit value for retransmission.

[Peter Huewe]

Am 2. März 2017 13:55:43 MEZ schrieb Jarkko Sakkinen 
:
>On Wed, Mar 01, 2017 at 04:36:17PM +0100, Enric Balletbo i Serra wrote:
>> From: Bryan Freed 
>> 
>> When the I2C Infineon part is attached to an I2C adapter that imposes
>> a size limitation, large requests will fail with -EOPNOTSUPP. Retry
>> them with a sane minimum size without re-issuing the 0x05 command
>> as this appears to occasionally put the TPM in a bad state.
>
>What is 0x05 command?
0x05 is the address of the fifo.
I honestly think that it needs toll​ be repeated after a stop condition.
I'll look that up.

>
>/Jarkko
>
>> Signed-off-by: Bryan Freed 
>> [rework the patch to adapt to the feedback received]
>> Signed-off-by: Enric Balletbo i Serra 
>> ---
>> This is a reworked version of the original patch based on the
>> suggestion made by Wolfram Sang to simply fall back to a sane minimum
>> when the maximum fails.
>> 
>> Changes since v1:
>>  - Check the correct return value (-EOPNOTSUPP instead of -EINVAL)
>>  - Fall back len to I2C_SMBUS_BLOCK_MAX if fails.
>> 
>>  drivers/char/tpm/tpm_i2c_infineon.c | 45
>+
>>  1 file changed, 26 insertions(+), 19 deletions(-)
>> 
>> diff --git a/drivers/char/tpm/tpm_i2c_infineon.c
>b/drivers/char/tpm/tpm_i2c_infineon.c
>> index 62ee44e..88bf947 100644
>> --- a/drivers/char/tpm/tpm_i2c_infineon.c
>> +++ b/drivers/char/tpm/tpm_i2c_infineon.c
>> @@ -107,39 +107,27 @@ static int iic_tpm_read(u8 addr, u8 *buffer,
>size_t len)
>>  .len = len,
>>  .buf = buffer
>>  };
>> -struct i2c_msg msgs[] = {msg1, msg2};
>>  
>>  int rc = 0;
>>  int count;
>> +unsigned int adapterlimit = len;
>>  
>>  /* Lock the adapter for the duration of the whole sequence. */
>>  if (!tpm_dev.client->adapter->algo->master_xfer)
>>  return -EOPNOTSUPP;
>>  i2c_lock_adapter(tpm_dev.client->adapter);
>>  
>> -if (tpm_dev.chip_type == SLB9645) {
Why are you / bryan removing this code path here?
I put it there for a good reason (i.e. faster transfers)

Thanks,
Peter
>> -/* use a combined read for newer chips
>> - * unfortunately the smbus functions are not suitable due to
>> - * the 32 byte limit of the smbus.
>> - * retries should usually not be needed, but are kept just to
>> - * be on the safe side.
>> - */
>> -for (count = 0; count < MAX_COUNT; count++) {
>> -rc = __i2c_transfer(tpm_dev.client->adapter, msgs, 2);
>> -if (rc > 0)
>> -break;  /* break here to skip sleep */
>> -usleep_range(SLEEP_DURATION_LOW, SLEEP_DURATION_HI);
>> -}
>> -} else {
>> +/* Expect to send one command message and one data message, but
>> + * support looping over each or both if necessary.
>> + */
>> +while (len > 0) {
>>  /* slb9635 protocol should work in all cases */
>>  for (count = 0; count < MAX_COUNT; count++) {
>>  rc = __i2c_transfer(tpm_dev.client->adapter, , 1);
>>  if (rc > 0)
>> -break;  /* break here to skip sleep */
>> -
>> +break;
>>  usleep_range(SLEEP_DURATION_LOW, SLEEP_DURATION_HI);
>>  }
>> -
>>  if (rc <= 0)
>>  goto out;
>>  
>> @@ -148,11 +136,30 @@ static int iic_tpm_read(u8 addr, u8 *buffer,
>size_t len)
>>   * retrieving the data
>>   */
>>  for (count = 0; count < MAX_COUNT; count++) {
>> +unsigned int msglen = msg2.len =
>> +min_t(unsigned int, adapterlimit, len);
>>  usleep_range(SLEEP_DURATION_LOW, SLEEP_DURATION_HI);
>>  rc = __i2c_transfer(tpm_dev.client->adapter, , 1);
>> -if (rc > 0)
>> +if (rc > 0) {
>> +/* Since len is unsigned, make doubly sure we
>> + * do not underflow it.
>> + */
>> +if (msglen > len)
>> +len = 0;
>> +else
>> +len -= msglen;
>> +msg2.buf += msglen;
>>  break;
>> +}
>> +/* If the I2C adapter rejected the request (e.g when
>> + * the quirk read_max_len < len) fall back to a sane
>> + * minimum value and try again.
>> + */
>> +if (rc == -EOPNOTSUPP)
>> +adapterlimit = I2C_SMBUS_BLOCK_MAX;
>>  }
>> +if (rc <= 0)
>> +goto out;
>>  }
>>  
>>  out:
>> -- 
>> 2.9.3
>> 
>> 
>>

[PATCH v2 4/5] tpm_tis_spi: Remove limitation of transfers to MAX_SPI_FRAMESIZE bytes

[Peter Huewe]

Limiting transfers to MAX_SPI_FRAMESIZE was not expected by the upper
layers, as tpm_tis has no such limitation. Add a loop to hide that
limitation.

v2: Moved scope of spi_message to the top as requested by Jarkko
Cc: <sta...@vger.kernel.org>
Fixes: 0edbfea537d1 ("tpm/tpm_tis_spi: Add support for spi phy")
Signed-off-by: Alexander Steffen <alexander.stef...@infineon.com>
Signed-off-by: Peter Huewe <peter.hu...@infineon.com>
Reviewed-by: Jarkko Sakkinen <jarkko.sakki...@linux.intel.com>
Tested-by: Benoit Houyere <benoit.houy...@st.com>
---
 drivers/char/tpm/tpm_tis_spi.c | 107 ++---
 1 file changed, 58 insertions(+), 49 deletions(-)

diff --git a/drivers/char/tpm/tpm_tis_spi.c b/drivers/char/tpm/tpm_tis_spi.c
index 62f50b6c9ef6..3015c8b65f18 100644
--- a/drivers/char/tpm/tpm_tis_spi.c
+++ b/drivers/char/tpm/tpm_tis_spi.c
@@ -60,67 +60,76 @@ static int tpm_tis_spi_transfer(struct tpm_tis_data *data, 
u32 addr, u16 len,
u8 *buffer, u8 direction)
 {
struct tpm_tis_spi_phy *phy = to_tpm_tis_spi_phy(data);
-   int ret, i;
+   int ret = 0;
+   int i;
struct spi_message m;
-   struct spi_transfer spi_xfer = {
-   .tx_buf = phy->tx_buf,
-   .rx_buf = phy->rx_buf,
-   .len = 4,
-   .cs_change = 1,
-   };
-
-   if (len > MAX_SPI_FRAMESIZE)
-   return -ENOMEM;
+   struct spi_transfer spi_xfer;
+   u8 transfer_len;
 
-   phy->tx_buf[0] = direction | (len - 1);
-   phy->tx_buf[1] = 0xd4;
-   phy->tx_buf[2] = addr >> 8;
-   phy->tx_buf[3] = addr;
+   spi_bus_lock(phy->spi_device->master);
 
-   spi_message_init();
-   spi_message_add_tail(_xfer, );
+   while (len) {
+   transfer_len = min_t(u16, len, MAX_SPI_FRAMESIZE);
 
-   spi_bus_lock(phy->spi_device->master);
-   ret = spi_sync_locked(phy->spi_device, );
-   if (ret < 0)
-   goto exit;
-
-   if ((phy->rx_buf[3] & 0x01) == 0) {
-   // handle SPI wait states
-   phy->tx_buf[0] = 0;
-
-   for (i = 0; i < TPM_RETRY; i++) {
-   spi_xfer.len = 1;
-   spi_message_init();
-   spi_message_add_tail(_xfer, );
-   ret = spi_sync_locked(phy->spi_device, );
-   if (ret < 0)
+   phy->tx_buf[0] = direction | (transfer_len - 1);
+   phy->tx_buf[1] = 0xd4;
+   phy->tx_buf[2] = addr >> 8;
+   phy->tx_buf[3] = addr;
+
+   memset(_xfer, 0, sizeof(spi_xfer));
+   spi_xfer.tx_buf = phy->tx_buf;
+   spi_xfer.rx_buf = phy->rx_buf;
+   spi_xfer.len = 4;
+   spi_xfer.cs_change = 1;
+
+   spi_message_init();
+   spi_message_add_tail(_xfer, );
+   ret = spi_sync_locked(phy->spi_device, );
+   if (ret < 0)
+   goto exit;
+
+   if ((phy->rx_buf[3] & 0x01) == 0) {
+   // handle SPI wait states
+   phy->tx_buf[0] = 0;
+
+   for (i = 0; i < TPM_RETRY; i++) {
+   spi_xfer.len = 1;
+   spi_message_init();
+   spi_message_add_tail(_xfer, );
+   ret = spi_sync_locked(phy->spi_device, );
+   if (ret < 0)
+   goto exit;
+   if (phy->rx_buf[0] & 0x01)
+   break;
+   }
+
+   if (i == TPM_RETRY) {
+   ret = -ETIMEDOUT;
goto exit;
-   if (phy->rx_buf[0] & 0x01)
-   break;
+   }
}
 
-   if (i == TPM_RETRY) {
-   ret = -ETIMEDOUT;
-   goto exit;
+   spi_xfer.cs_change = 0;
+   spi_xfer.len = transfer_len;
+
+   if (direction) {
+   spi_xfer.tx_buf = NULL;
+   spi_xfer.rx_buf = buffer;
+   } else {
+   spi_xfer.tx_buf = buffer;
+   spi_xfer.rx_buf = NULL;
}
-   }
 
-   spi_xfer.cs_change = 0;
-   spi_xfer.len = len;
+   spi_message_init();
+   spi_message_add_tail(_xfer, );
+   ret = spi_sync_locked(phy->spi_device, );
+   if (ret < 0)
+   goto exit;
 
-   if (direction) {
-   spi_xfer.tx_buf = NULL;
-  

[PATCH v2 4/5] tpm_tis_spi: Remove limitation of transfers to MAX_SPI_FRAMESIZE bytes

[Peter Huewe]

Limiting transfers to MAX_SPI_FRAMESIZE was not expected by the upper
layers, as tpm_tis has no such limitation. Add a loop to hide that
limitation.

v2: Moved scope of spi_message to the top as requested by Jarkko
Cc: 
Fixes: 0edbfea537d1 ("tpm/tpm_tis_spi: Add support for spi phy")
Signed-off-by: Alexander Steffen 
Signed-off-by: Peter Huewe 
Reviewed-by: Jarkko Sakkinen 
Tested-by: Benoit Houyere 
---
 drivers/char/tpm/tpm_tis_spi.c | 107 ++---
 1 file changed, 58 insertions(+), 49 deletions(-)

diff --git a/drivers/char/tpm/tpm_tis_spi.c b/drivers/char/tpm/tpm_tis_spi.c
index 62f50b6c9ef6..3015c8b65f18 100644
--- a/drivers/char/tpm/tpm_tis_spi.c
+++ b/drivers/char/tpm/tpm_tis_spi.c
@@ -60,67 +60,76 @@ static int tpm_tis_spi_transfer(struct tpm_tis_data *data, 
u32 addr, u16 len,
u8 *buffer, u8 direction)
 {
struct tpm_tis_spi_phy *phy = to_tpm_tis_spi_phy(data);
-   int ret, i;
+   int ret = 0;
+   int i;
struct spi_message m;
-   struct spi_transfer spi_xfer = {
-   .tx_buf = phy->tx_buf,
-   .rx_buf = phy->rx_buf,
-   .len = 4,
-   .cs_change = 1,
-   };
-
-   if (len > MAX_SPI_FRAMESIZE)
-   return -ENOMEM;
+   struct spi_transfer spi_xfer;
+   u8 transfer_len;
 
-   phy->tx_buf[0] = direction | (len - 1);
-   phy->tx_buf[1] = 0xd4;
-   phy->tx_buf[2] = addr >> 8;
-   phy->tx_buf[3] = addr;
+   spi_bus_lock(phy->spi_device->master);
 
-   spi_message_init();
-   spi_message_add_tail(_xfer, );
+   while (len) {
+   transfer_len = min_t(u16, len, MAX_SPI_FRAMESIZE);
 
-   spi_bus_lock(phy->spi_device->master);
-   ret = spi_sync_locked(phy->spi_device, );
-   if (ret < 0)
-   goto exit;
-
-   if ((phy->rx_buf[3] & 0x01) == 0) {
-   // handle SPI wait states
-   phy->tx_buf[0] = 0;
-
-   for (i = 0; i < TPM_RETRY; i++) {
-   spi_xfer.len = 1;
-   spi_message_init();
-   spi_message_add_tail(_xfer, );
-   ret = spi_sync_locked(phy->spi_device, );
-   if (ret < 0)
+   phy->tx_buf[0] = direction | (transfer_len - 1);
+   phy->tx_buf[1] = 0xd4;
+   phy->tx_buf[2] = addr >> 8;
+   phy->tx_buf[3] = addr;
+
+   memset(_xfer, 0, sizeof(spi_xfer));
+   spi_xfer.tx_buf = phy->tx_buf;
+   spi_xfer.rx_buf = phy->rx_buf;
+   spi_xfer.len = 4;
+   spi_xfer.cs_change = 1;
+
+   spi_message_init();
+   spi_message_add_tail(_xfer, );
+   ret = spi_sync_locked(phy->spi_device, );
+   if (ret < 0)
+   goto exit;
+
+   if ((phy->rx_buf[3] & 0x01) == 0) {
+   // handle SPI wait states
+   phy->tx_buf[0] = 0;
+
+   for (i = 0; i < TPM_RETRY; i++) {
+   spi_xfer.len = 1;
+   spi_message_init();
+   spi_message_add_tail(_xfer, );
+   ret = spi_sync_locked(phy->spi_device, );
+   if (ret < 0)
+   goto exit;
+   if (phy->rx_buf[0] & 0x01)
+   break;
+   }
+
+   if (i == TPM_RETRY) {
+   ret = -ETIMEDOUT;
goto exit;
-   if (phy->rx_buf[0] & 0x01)
-   break;
+   }
}
 
-   if (i == TPM_RETRY) {
-   ret = -ETIMEDOUT;
-   goto exit;
+   spi_xfer.cs_change = 0;
+   spi_xfer.len = transfer_len;
+
+   if (direction) {
+   spi_xfer.tx_buf = NULL;
+   spi_xfer.rx_buf = buffer;
+   } else {
+   spi_xfer.tx_buf = buffer;
+   spi_xfer.rx_buf = NULL;
}
-   }
 
-   spi_xfer.cs_change = 0;
-   spi_xfer.len = len;
+   spi_message_init();
+   spi_message_add_tail(_xfer, );
+   ret = spi_sync_locked(phy->spi_device, );
+   if (ret < 0)
+   goto exit;
 
-   if (direction) {
-   spi_xfer.tx_buf = NULL;
-   spi_xfer.rx_buf = buffer;
-   } else {
-   spi_xfer.tx_buf = buffer;
-   spi_xfer.rx_buf = NULL;
+   len -= transfer_len;
+

[PATCH v2 2/5] tpm_tis_spi: Abort transfer when too many wait states are signaled

[Peter Huewe]

Abort the transfer with ETIMEDOUT when the TPM signals more than
TPM_RETRY wait states. Continuing with the transfer in this state
will only lead to arbitrary failures in other parts of the code.

Cc: <sta...@vger.kernel.org>
Fixes: 0edbfea537d1 ("tpm/tpm_tis_spi: Add support for spi phy")
Signed-off-by: Alexander Steffen <alexander.stef...@infineon.com>
Signed-off-by: Peter Huewe <peter.hu...@infineon.com>
Reviewed-by: Jarkko Sakkinen <jarkko.sakki...@linux.intel.com>
Tested-by: Benoit Houyere <benoit.houy...@st.com>
---
 drivers/char/tpm/tpm_tis_spi.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/drivers/char/tpm/tpm_tis_spi.c b/drivers/char/tpm/tpm_tis_spi.c
index 062799e04f04..639614f2d415 100644
--- a/drivers/char/tpm/tpm_tis_spi.c
+++ b/drivers/char/tpm/tpm_tis_spi.c
@@ -101,6 +101,11 @@ static int tpm_tis_spi_transfer(struct tpm_tis_data *data, 
u32 addr, u16 len,
goto exit;
}
 
+   if (i == TPM_RETRY) {
+   ret = -ETIMEDOUT;
+   goto exit;
+   }
+
spi_xfer.cs_change = 0;
spi_xfer.len = len;
 
-- 
2.7.4

[PATCH v2 2/5] tpm_tis_spi: Abort transfer when too many wait states are signaled

[Peter Huewe]

Abort the transfer with ETIMEDOUT when the TPM signals more than
TPM_RETRY wait states. Continuing with the transfer in this state
will only lead to arbitrary failures in other parts of the code.

Cc: 
Fixes: 0edbfea537d1 ("tpm/tpm_tis_spi: Add support for spi phy")
Signed-off-by: Alexander Steffen 
Signed-off-by: Peter Huewe 
Reviewed-by: Jarkko Sakkinen 
Tested-by: Benoit Houyere 
---
 drivers/char/tpm/tpm_tis_spi.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/drivers/char/tpm/tpm_tis_spi.c b/drivers/char/tpm/tpm_tis_spi.c
index 062799e04f04..639614f2d415 100644
--- a/drivers/char/tpm/tpm_tis_spi.c
+++ b/drivers/char/tpm/tpm_tis_spi.c
@@ -101,6 +101,11 @@ static int tpm_tis_spi_transfer(struct tpm_tis_data *data, 
u32 addr, u16 len,
goto exit;
}
 
+   if (i == TPM_RETRY) {
+   ret = -ETIMEDOUT;
+   goto exit;
+   }
+
spi_xfer.cs_change = 0;
spi_xfer.len = len;
 
-- 
2.7.4

Re: [tpmdd-devel] [PATCH 1/2] tpm: Apply an adapterlimit for retransmission.

[Peter Huewe]

Am 27. Februar 2017 20:12:45 MEZ schrieb Wolfram Sang :
>Hi,
>
>> >> > Rather than trying small and smaller transfers, would it not be
>better
>> >> > to get the i2c core to expose the quirk info about transfer
>limits?
>> >> >
>> >>
>> >> Sounds a good idea to me, I guess the quirk info can be accessed
>with
>> >>
>> >>   tpm_dev.client->adapter->quirks->max_read_len
>> >>
>> >> so I think we don't need to touch the i2c core. I'll propose a
>second version of the patch.
>> >
>> > Hi Enric
>> >
>> > You should probably ask Wolfram Sang , the i2c
>> > subsystem maintainer. He may prefer adding an API call.
>
>Thanks for pointing me to this thread.
>
>I understand it looks tempting to use the quirks struct directly, but I
>don't think this is the proper solution. Quirks are complex and and to
>determine which one finally applies, you need all the logic encoded in
>i2c_check_for_quirks(). Which already gets called on every transfer.
>
>So, my suggestion would be to simply fall back to a sane minimum when
>the maximum failed. 32 (I2C_SMBUS_BLOCK_MAX) should be a good choice.

Hi,
One problem is however that e.g. in the case of the atmel tpms, there is no 
sane minimum, since you mustn't split the tpm apdu.

If the command is larger than the supported adapter limit the command the only 
viable option is to return an error.
For this the tpm layer would need the adapter limit.

If somehow possible I would seriously vote for a adapter limit field (maybe not 
in the quirks).

TPMs are a not the best devices when it comes to i2c :/

Peter


>
>BTW I noted that the original patch checks for -EINVAL. The core
>returns
>-EOPNOTSUPP, though. So, a) the patch needs to be adapted and b) it
>looks the i2c host driver returning -EINVAL could be converted to use
>the quirk infrastructure? Which driver is it?
>
>Regards,
>
>   Wolfram

-- 
Sent from my mobile

Re: [tpmdd-devel] [PATCH 1/2] tpm: Apply an adapterlimit for retransmission.

[Peter Huewe]

Am 27. Februar 2017 20:12:45 MEZ schrieb Wolfram Sang :
>Hi,
>
>> >> > Rather than trying small and smaller transfers, would it not be
>better
>> >> > to get the i2c core to expose the quirk info about transfer
>limits?
>> >> >
>> >>
>> >> Sounds a good idea to me, I guess the quirk info can be accessed
>with
>> >>
>> >>   tpm_dev.client->adapter->quirks->max_read_len
>> >>
>> >> so I think we don't need to touch the i2c core. I'll propose a
>second version of the patch.
>> >
>> > Hi Enric
>> >
>> > You should probably ask Wolfram Sang , the i2c
>> > subsystem maintainer. He may prefer adding an API call.
>
>Thanks for pointing me to this thread.
>
>I understand it looks tempting to use the quirks struct directly, but I
>don't think this is the proper solution. Quirks are complex and and to
>determine which one finally applies, you need all the logic encoded in
>i2c_check_for_quirks(). Which already gets called on every transfer.
>
>So, my suggestion would be to simply fall back to a sane minimum when
>the maximum failed. 32 (I2C_SMBUS_BLOCK_MAX) should be a good choice.

Hi,
One problem is however that e.g. in the case of the atmel tpms, there is no 
sane minimum, since you mustn't split the tpm apdu.

If the command is larger than the supported adapter limit the command the only 
viable option is to return an error.
For this the tpm layer would need the adapter limit.

If somehow possible I would seriously vote for a adapter limit field (maybe not 
in the quirks).

TPMs are a not the best devices when it comes to i2c :/

Peter


>
>BTW I noted that the original patch checks for -EINVAL. The core
>returns
>-EOPNOTSUPP, though. So, a) the patch needs to be adapted and b) it
>looks the i2c host driver returning -EINVAL could be converted to use
>the quirk infrastructure? Which driver is it?
>
>Regards,
>
>   Wolfram

-- 
Sent from my mobile

[PATCH v2 5/5] tpm_tis_spi: Add small delay after last transfer

[Peter Huewe]

Testing the implementation with a Raspberry Pi 2 showed that under some
circumstances its SPI master erroneously releases the CS line before the
transfer is complete, i.e. before the end of the last clock. In this case
the TPM ignores the transfer and misses for example the GO command. The
driver is unable to detect this communication problem and will wait for a
command response that is never going to arrive, timing out eventually.

As a workaround, the small delay ensures that the CS line is held long
enough, even with a faulty SPI master. Other SPI masters are not affected,
except for a negligible performance penalty.

Cc: <sta...@vger.kernel.org>
Fixes: 0edbfea537d1 ("tpm/tpm_tis_spi: Add support for spi phy")
Signed-off-by: Alexander Steffen <alexander.stef...@infineon.com>
Signed-off-by: Peter Huewe <peter.hu...@infineon.com>
Reviewed-by: Jarkko Sakkinen <jarkko.sakki...@linux.intel.com>
Tested-by: Benoit Houyere <benoit.houy...@st.com>
---
 drivers/char/tpm/tpm_tis_spi.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/char/tpm/tpm_tis_spi.c b/drivers/char/tpm/tpm_tis_spi.c
index 3015c8b65f18..88fe72ae967f 100644
--- a/drivers/char/tpm/tpm_tis_spi.c
+++ b/drivers/char/tpm/tpm_tis_spi.c
@@ -111,6 +111,7 @@ static int tpm_tis_spi_transfer(struct tpm_tis_data *data, 
u32 addr, u16 len,
 
spi_xfer.cs_change = 0;
spi_xfer.len = transfer_len;
+   spi_xfer.delay_usecs = 5;
 
if (direction) {
spi_xfer.tx_buf = NULL;
-- 
2.7.4

[PATCH v2 5/5] tpm_tis_spi: Add small delay after last transfer

[Peter Huewe]

Testing the implementation with a Raspberry Pi 2 showed that under some
circumstances its SPI master erroneously releases the CS line before the
transfer is complete, i.e. before the end of the last clock. In this case
the TPM ignores the transfer and misses for example the GO command. The
driver is unable to detect this communication problem and will wait for a
command response that is never going to arrive, timing out eventually.

As a workaround, the small delay ensures that the CS line is held long
enough, even with a faulty SPI master. Other SPI masters are not affected,
except for a negligible performance penalty.

Cc: 
Fixes: 0edbfea537d1 ("tpm/tpm_tis_spi: Add support for spi phy")
Signed-off-by: Alexander Steffen 
Signed-off-by: Peter Huewe 
Reviewed-by: Jarkko Sakkinen 
Tested-by: Benoit Houyere 
---
 drivers/char/tpm/tpm_tis_spi.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/char/tpm/tpm_tis_spi.c b/drivers/char/tpm/tpm_tis_spi.c
index 3015c8b65f18..88fe72ae967f 100644
--- a/drivers/char/tpm/tpm_tis_spi.c
+++ b/drivers/char/tpm/tpm_tis_spi.c
@@ -111,6 +111,7 @@ static int tpm_tis_spi_transfer(struct tpm_tis_data *data, 
u32 addr, u16 len,
 
spi_xfer.cs_change = 0;
spi_xfer.len = transfer_len;
+   spi_xfer.delay_usecs = 5;
 
if (direction) {
spi_xfer.tx_buf = NULL;
-- 
2.7.4

Re: [PATCH] tpm: do not suspend/resume if power stays on

[Peter Huewe]

Am 1. März 2017 12:51:16 MEZ schrieb Enric Balletbo i Serra 
:
>From: Sonny Rao 
>
>The suspend/resume behavior of the TPM can be controlled
>by setting "powered-while-suspended" in the DTS.
>
>Signed-off-by: Sonny Rao 
>Signed-off-by: Enric Balletbo i Serra 
>---
>Documentation/devicetree/bindings/tpm/tpm.txt | 25
>+
>drivers/char/tpm/tpm_i2c_infineon.c   | 25
>-
> 2 files changed, 49 insertions(+), 1 deletion(-)
> create mode 100644 Documentation/devicetree/bindings/tpm/tpm.txt
>
>diff --git a/Documentation/devicetree/bindings/tpm/tpm.txt
>b/Documentation/devicetree/bindings/tpm/tpm.txt
>new file mode 100644
>index 000..af4de0d
>--- /dev/null
>+++ b/Documentation/devicetree/bindings/tpm/tpm.txt

Hi, for this device there exists a binding in the i2c subfolder

https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/i2c/trivial-devices.txt?id=refs/tags/v4.10

Peter
>@@ -0,0 +1,25 @@
>+TPM (Trusted Platform Module)
>+
>+A TPM on the I2C bus is a child of the node for the bus.
>+
>+Required properties:
>+- compatible: should be "infineon,"
>+- reg: the I2C address
>+
>+Optional properties:
>+- powered-while-suspended: present when the TPM is left powered on
>between
>+  suspend and resume (makes the suspend/resume callbacks do nothing).
>+
>+Example:
>+  i2c@12C9 {
>+  samsung,i2c-sda-delay = <100>;
>+  samsung,i2c-max-bus-freq = <66000>;
>+  gpios = < 2 3 3 0>,
>+  < 3 3 3 0>;
>+
>+  tpm {
>+  compatible = "infineon,slb9635tt";
>+  reg = <0x20>;
>+  powered-while-suspended;
>+  };
>+  };
>diff --git a/drivers/char/tpm/tpm_i2c_infineon.c
>b/drivers/char/tpm/tpm_i2c_infineon.c
>index 62ee44e..19d9522 100644
>--- a/drivers/char/tpm/tpm_i2c_infineon.c
>+++ b/drivers/char/tpm/tpm_i2c_infineon.c
>@@ -70,6 +70,7 @@ struct tpm_inf_dev {
>   u8 buf[TPM_BUFSIZE + sizeof(u8)]; /* max. buffer size + addr */
>   struct tpm_chip *chip;
>   enum i2c_chip_type chip_type;
>+  bool powered_while_suspended;
> };
> 
> static struct tpm_inf_dev tpm_dev;
>@@ -599,6 +600,11 @@ static int tpm_tis_i2c_init(struct device *dev)
>   goto out_err;
>   }
> 
>+  if (dev->of_node &&
>+  of_get_property(dev->of_node, "powered-while-suspended", NULL)) {
>+  tpm_dev.powered_while_suspended = true;
>+  }
>+
>   /* read four bytes from DID_VID register */
>   if (iic_tpm_read(TPM_DID_VID(0), (u8 *), 4) < 0) {
>   dev_err(dev, "could not read vendor id\n");
>@@ -662,7 +668,24 @@ static const struct of_device_id
>tpm_tis_i2c_of_match[] = {
> MODULE_DEVICE_TABLE(of, tpm_tis_i2c_of_match);
> #endif
> 
>-static SIMPLE_DEV_PM_OPS(tpm_tis_i2c_ops, tpm_pm_suspend,
>tpm_pm_resume);
>+static int __maybe_unused tpm_tis_i2c_suspend(struct device *dev)
>+{
>+  if (tpm_dev.powered_while_suspended)
>+  return 0;
>+
>+  return tpm_pm_suspend(dev);
>+}
>+
>+static int __maybe_unused tpm_tis_i2c_resume(struct device *dev)
>+{
>+  if (tpm_dev.powered_while_suspended)
>+  return 0;
>+
>+  return tpm_pm_resume(dev);
>+}
>+
>+static SIMPLE_DEV_PM_OPS(tpm_tis_i2c_ops, tpm_tis_i2c_suspend,
>+   tpm_tis_i2c_resume);
> 
> static int tpm_tis_i2c_probe(struct i2c_client *client,
>const struct i2c_device_id *id)

-- 
Sent from my mobile

Re: [PATCH] tpm: do not suspend/resume if power stays on

[Peter Huewe]

Am 1. März 2017 12:51:16 MEZ schrieb Enric Balletbo i Serra 
:
>From: Sonny Rao 
>
>The suspend/resume behavior of the TPM can be controlled
>by setting "powered-while-suspended" in the DTS.
>
>Signed-off-by: Sonny Rao 
>Signed-off-by: Enric Balletbo i Serra 
>---
>Documentation/devicetree/bindings/tpm/tpm.txt | 25
>+
>drivers/char/tpm/tpm_i2c_infineon.c   | 25
>-
> 2 files changed, 49 insertions(+), 1 deletion(-)
> create mode 100644 Documentation/devicetree/bindings/tpm/tpm.txt
>
>diff --git a/Documentation/devicetree/bindings/tpm/tpm.txt
>b/Documentation/devicetree/bindings/tpm/tpm.txt
>new file mode 100644
>index 000..af4de0d
>--- /dev/null
>+++ b/Documentation/devicetree/bindings/tpm/tpm.txt

Hi, for this device there exists a binding in the i2c subfolder

https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/i2c/trivial-devices.txt?id=refs/tags/v4.10

Peter
>@@ -0,0 +1,25 @@
>+TPM (Trusted Platform Module)
>+
>+A TPM on the I2C bus is a child of the node for the bus.
>+
>+Required properties:
>+- compatible: should be "infineon,"
>+- reg: the I2C address
>+
>+Optional properties:
>+- powered-while-suspended: present when the TPM is left powered on
>between
>+  suspend and resume (makes the suspend/resume callbacks do nothing).
>+
>+Example:
>+  i2c@12C9 {
>+  samsung,i2c-sda-delay = <100>;
>+  samsung,i2c-max-bus-freq = <66000>;
>+  gpios = < 2 3 3 0>,
>+  < 3 3 3 0>;
>+
>+  tpm {
>+  compatible = "infineon,slb9635tt";
>+  reg = <0x20>;
>+  powered-while-suspended;
>+  };
>+  };
>diff --git a/drivers/char/tpm/tpm_i2c_infineon.c
>b/drivers/char/tpm/tpm_i2c_infineon.c
>index 62ee44e..19d9522 100644
>--- a/drivers/char/tpm/tpm_i2c_infineon.c
>+++ b/drivers/char/tpm/tpm_i2c_infineon.c
>@@ -70,6 +70,7 @@ struct tpm_inf_dev {
>   u8 buf[TPM_BUFSIZE + sizeof(u8)]; /* max. buffer size + addr */
>   struct tpm_chip *chip;
>   enum i2c_chip_type chip_type;
>+  bool powered_while_suspended;
> };
> 
> static struct tpm_inf_dev tpm_dev;
>@@ -599,6 +600,11 @@ static int tpm_tis_i2c_init(struct device *dev)
>   goto out_err;
>   }
> 
>+  if (dev->of_node &&
>+  of_get_property(dev->of_node, "powered-while-suspended", NULL)) {
>+  tpm_dev.powered_while_suspended = true;
>+  }
>+
>   /* read four bytes from DID_VID register */
>   if (iic_tpm_read(TPM_DID_VID(0), (u8 *), 4) < 0) {
>   dev_err(dev, "could not read vendor id\n");
>@@ -662,7 +668,24 @@ static const struct of_device_id
>tpm_tis_i2c_of_match[] = {
> MODULE_DEVICE_TABLE(of, tpm_tis_i2c_of_match);
> #endif
> 
>-static SIMPLE_DEV_PM_OPS(tpm_tis_i2c_ops, tpm_pm_suspend,
>tpm_pm_resume);
>+static int __maybe_unused tpm_tis_i2c_suspend(struct device *dev)
>+{
>+  if (tpm_dev.powered_while_suspended)
>+  return 0;
>+
>+  return tpm_pm_suspend(dev);
>+}
>+
>+static int __maybe_unused tpm_tis_i2c_resume(struct device *dev)
>+{
>+  if (tpm_dev.powered_while_suspended)
>+  return 0;
>+
>+  return tpm_pm_resume(dev);
>+}
>+
>+static SIMPLE_DEV_PM_OPS(tpm_tis_i2c_ops, tpm_tis_i2c_suspend,
>+   tpm_tis_i2c_resume);
> 
> static int tpm_tis_i2c_probe(struct i2c_client *client,
>const struct i2c_device_id *id)

-- 
Sent from my mobile

Re: [PATCH] tpm: select CONFIG_CRYPTO

[Peter Huewe]

Am 28. Februar 2017 22:12:55 MEZ schrieb Arnd Bergmann <a...@arndb.de>:
>We get a newly introduced harmless warning when CONFIG_CRYPTO is
>disabled:
>
>warning: (TCG_TPM && TRUSTED_KEYS && IMA) selects CRYPTO_HASH_INFO
>which has unmet direct dependencies (CRYPTO)
>
>This adds another select to avoid the warning, consistent with other
>users
>of the crypto code.
>
>Fixes: c1f92b4b04ad ("tpm: enhance TPM 2.0 PCR extend to support
>multiple banks")
>Signed-off-by: Arnd Bergmann <a...@arndb.de>
Lgtm

Reviewed-by: Peter Huewe <peterhu...@gmx.de>
>---
> drivers/char/tpm/Kconfig | 1 +
> 1 file changed, 1 insertion(+)
>
>diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig
>index af985cca413c..d520ac51c11c 100644
>--- a/drivers/char/tpm/Kconfig
>+++ b/drivers/char/tpm/Kconfig
>@@ -6,6 +6,7 @@ menuconfig TCG_TPM
>   tristate "TPM Hardware Support"
>   depends on HAS_IOMEM
>   select SECURITYFS
>+  select CRYPTO
>   select CRYPTO_HASH_INFO
>   ---help---
> If you have a TPM security chip in your system, which

-- 
Sent from my mobile

Re: [PATCH] tpm: select CONFIG_CRYPTO

[Peter Huewe]

Am 28. Februar 2017 22:12:55 MEZ schrieb Arnd Bergmann :
>We get a newly introduced harmless warning when CONFIG_CRYPTO is
>disabled:
>
>warning: (TCG_TPM && TRUSTED_KEYS && IMA) selects CRYPTO_HASH_INFO
>which has unmet direct dependencies (CRYPTO)
>
>This adds another select to avoid the warning, consistent with other
>users
>of the crypto code.
>
>Fixes: c1f92b4b04ad ("tpm: enhance TPM 2.0 PCR extend to support
>multiple banks")
>Signed-off-by: Arnd Bergmann 
Lgtm

Reviewed-by: Peter Huewe 
>---
> drivers/char/tpm/Kconfig | 1 +
> 1 file changed, 1 insertion(+)
>
>diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig
>index af985cca413c..d520ac51c11c 100644
>--- a/drivers/char/tpm/Kconfig
>+++ b/drivers/char/tpm/Kconfig
>@@ -6,6 +6,7 @@ menuconfig TCG_TPM
>   tristate "TPM Hardware Support"
>   depends on HAS_IOMEM
>   select SECURITYFS
>+  select CRYPTO
>   select CRYPTO_HASH_INFO
>   ---help---
> If you have a TPM security chip in your system, which

-- 
Sent from my mobile

Re: [PATCH 0/5] Fix whole native SPI TPM driver

[Peter Huewe]

Am 22. Februar 2017 22:19:24 MEZ schrieb Jarkko Sakkinen 
<jarkko.sakki...@linux.intel.com>:
>On Thu, Feb 16, 2017 at 04:08:21PM +0000, Peter Huewe wrote:
>> During our testing it showed that unfortunately the whole native spi
>tpm driver
>> was more or less non-functional since it was merged, e.g. the wrong
>byte for
>> waitstate handling was used and transfers larger than 64 bytes did
>not work at all.
>> 
>> This was probably caused by the merging of the different approaches
>back then,
>> as the initial RFC patch did not have these problems, and also my
>sudden lack
>> of time/commitment back then.
>> I'm sorry that the final driver code went untested for that long
>time.
>> 
>> This patch set fixes these issues one by one.
>> In order to avoid duplication the read/write function was
>consolidated to one
>> transfer function, so we do not have to apply the same fix at two
>locations.
>> Maybe consider squashing it - we splitted it for easier review.
>>  
>> Affected Kernels: 4.8, 4.9, 4.10
>> Patchset was tested on Raspberry Pi2 with SLB9670 (TPM1.2 and TPM2.0)
>> 
>> Peter Huewe (5):
>>   tpm_tis_spi: Use single function to transfer data
>>   tpm_tis_spi: Abort transfer when too many wait states are signaled
>>   tpm_tis_spi: Check correct byte for wait state indicator
>>   tpm_tis_spi: Remove limitation of transfers to MAX_SPI_FRAMESIZE
>bytes
>>   tpm_tis_spi: Add small delay after last transfer
>> 
>>  drivers/char/tpm/tpm_tis_spi.c | 163
>+
>>  1 file changed, 69 insertions(+), 94 deletions(-)
>> 
>> -- 
>> 2.7.4
>> 
>
>I also found type errors a whilea ago:
>
>https://patchwork.kernel.org/patch/9547993/
>
>Do this take care of these issues? 
Not that I'm aware of.
We did not test it on any big endian system yet so the problem never became 
visible.

In any case it would be really nice to see this patchset to be applied so 
people have a working,but maybe not perfect, spi tpm on the common little 
endian platforms.
When do you think you can merge and pull-request them?

Peter
>
>/JArkko

-- 
Sent from my mobile

Re: [PATCH 0/5] Fix whole native SPI TPM driver

[Peter Huewe]

Am 22. Februar 2017 22:19:24 MEZ schrieb Jarkko Sakkinen 
:
>On Thu, Feb 16, 2017 at 04:08:21PM +0000, Peter Huewe wrote:
>> During our testing it showed that unfortunately the whole native spi
>tpm driver
>> was more or less non-functional since it was merged, e.g. the wrong
>byte for
>> waitstate handling was used and transfers larger than 64 bytes did
>not work at all.
>> 
>> This was probably caused by the merging of the different approaches
>back then,
>> as the initial RFC patch did not have these problems, and also my
>sudden lack
>> of time/commitment back then.
>> I'm sorry that the final driver code went untested for that long
>time.
>> 
>> This patch set fixes these issues one by one.
>> In order to avoid duplication the read/write function was
>consolidated to one
>> transfer function, so we do not have to apply the same fix at two
>locations.
>> Maybe consider squashing it - we splitted it for easier review.
>>  
>> Affected Kernels: 4.8, 4.9, 4.10
>> Patchset was tested on Raspberry Pi2 with SLB9670 (TPM1.2 and TPM2.0)
>> 
>> Peter Huewe (5):
>>   tpm_tis_spi: Use single function to transfer data
>>   tpm_tis_spi: Abort transfer when too many wait states are signaled
>>   tpm_tis_spi: Check correct byte for wait state indicator
>>   tpm_tis_spi: Remove limitation of transfers to MAX_SPI_FRAMESIZE
>bytes
>>   tpm_tis_spi: Add small delay after last transfer
>> 
>>  drivers/char/tpm/tpm_tis_spi.c | 163
>+
>>  1 file changed, 69 insertions(+), 94 deletions(-)
>> 
>> -- 
>> 2.7.4
>> 
>
>I also found type errors a whilea ago:
>
>https://patchwork.kernel.org/patch/9547993/
>
>Do this take care of these issues? 
Not that I'm aware of.
We did not test it on any big endian system yet so the problem never became 
visible.

In any case it would be really nice to see this patchset to be applied so 
people have a working,but maybe not perfect, spi tpm on the common little 
endian platforms.
When do you think you can merge and pull-request them?

Peter
>
>/JArkko

-- 
Sent from my mobile

Re: [tpmdd-devel] [PATCH 1/2] tpm: Apply an adapterlimit for retransmission.

[Peter Huewe]

Am 21. Februar 2017 17:29:48 MEZ schrieb Andrew Lunn :
>On Tue, Feb 21, 2017 at 03:44:59PM +0100, Enric Balletbo i Serra wrote:
>> From: Bryan Freed 
>> 
>> When the I2C Infineon part is attached to an I2C adapter that imposes
>> a size limitation, large requests will fail -EINVAL.
>> Retry them with size backoff without re-issuing the 0x05 command
>> as this appears to occasionally put the TPM in a bad state.
>
>Hi Enric
>
>Rather than trying small and smaller transfers, would it not be better
>to get the i2c core to expose the quirk info about transfer limits?
>
+1
I think that would be the better idea.
Peter
>   Andrew

-- 
Sent from my mobile

Re: [tpmdd-devel] [PATCH 1/2] tpm: Apply an adapterlimit for retransmission.

[Peter Huewe]

Am 21. Februar 2017 17:29:48 MEZ schrieb Andrew Lunn :
>On Tue, Feb 21, 2017 at 03:44:59PM +0100, Enric Balletbo i Serra wrote:
>> From: Bryan Freed 
>> 
>> When the I2C Infineon part is attached to an I2C adapter that imposes
>> a size limitation, large requests will fail -EINVAL.
>> Retry them with size backoff without re-issuing the 0x05 command
>> as this appears to occasionally put the TPM in a bad state.
>
>Hi Enric
>
>Rather than trying small and smaller transfers, would it not be better
>to get the i2c core to expose the quirk info about transfer limits?
>
+1
I think that would be the better idea.
Peter
>   Andrew

-- 
Sent from my mobile

Re: [PATCH 5/5] tpm_tis_spi: Add small delay after last transfer

[Peter Huewe]

Am 17. Februar 2017 06:09:42 MEZ schrieb Christophe Ricard 
<christophe.ric...@gmail.com>:
>Are you sure it is not better to introduce this delay directly in the 
>rpi spi driver ?
>
>Other than that i don't see any issue with it.

Yes - it would be perfect to fix the issue in the upstream rpi spi master 
driver.
However this only happens sporadically in some strange cases (i.e. 300-500khz, 
single byte transfer after having more bytes in the same #cs frame) (unrelated 
to the tpm)

We are still looking into it, why /where it happens and how to reproduce it 
reliably and then file a bug/fix it.

For now however the priority is to make the tpm_tis_spi driver work reliably 
also on the rpi, and this is what this workaround does.

We can simply remove it once the rpi spi master is fixed.

Peter
>
>
>On 16/02/2017 08:09, Peter Huewe wrote:
>> Testing the implementation with a Raspberry Pi 2 showed that under
>some
>> circumstances its SPI master erroneously releases the CS line before
>the
>> transfer is complete, i.e. before the end of the last clock. In this
>case
>> the TPM ignores the transfer and misses for example the GO command.
>The
>> driver is unable to detect this communication problem and will wait
>for a
>> command response that is never going to arrive, timing out
>eventually.
>>
>> As a workaround, the small delay ensures that the CS line is held
>long
>> enough, even with a faulty SPI master. Other SPI masters are not
>affected,
>> except for a negligible performance penalty.
>>
>> Cc: <sta...@vger.kernel.org>
>> Fixes: 0edbfea537d1 ("tpm/tpm_tis_spi: Add support for spi phy")
>> Signed-off-by: Alexander Steffen <alexander.stef...@infineon.com>
>> Signed-off-by: Peter Huewe <peter.hu...@infineon.com>
>> ---
>>   drivers/char/tpm/tpm_tis_spi.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/char/tpm/tpm_tis_spi.c
>b/drivers/char/tpm/tpm_tis_spi.c
>> index b50c5b072df3..685c51bf5d7e 100644
>> --- a/drivers/char/tpm/tpm_tis_spi.c
>> +++ b/drivers/char/tpm/tpm_tis_spi.c
>> @@ -110,6 +110,7 @@ static int tpm_tis_spi_transfer(struct
>tpm_tis_data *data, u32 addr, u8 len,
>>   
>>  spi_xfer.cs_change = 0;
>>  spi_xfer.len = transfer_len;
>> +spi_xfer.delay_usecs = 5;
>>   
>>  if (direction) {
>>  spi_xfer.tx_buf = NULL;

-- 
Sent from my mobile

Re: [PATCH 5/5] tpm_tis_spi: Add small delay after last transfer

[Peter Huewe]

Am 17. Februar 2017 06:09:42 MEZ schrieb Christophe Ricard 
:
>Are you sure it is not better to introduce this delay directly in the 
>rpi spi driver ?
>
>Other than that i don't see any issue with it.

Yes - it would be perfect to fix the issue in the upstream rpi spi master 
driver.
However this only happens sporadically in some strange cases (i.e. 300-500khz, 
single byte transfer after having more bytes in the same #cs frame) (unrelated 
to the tpm)

We are still looking into it, why /where it happens and how to reproduce it 
reliably and then file a bug/fix it.

For now however the priority is to make the tpm_tis_spi driver work reliably 
also on the rpi, and this is what this workaround does.

We can simply remove it once the rpi spi master is fixed.

Peter
>
>
>On 16/02/2017 08:09, Peter Huewe wrote:
>> Testing the implementation with a Raspberry Pi 2 showed that under
>some
>> circumstances its SPI master erroneously releases the CS line before
>the
>> transfer is complete, i.e. before the end of the last clock. In this
>case
>> the TPM ignores the transfer and misses for example the GO command.
>The
>> driver is unable to detect this communication problem and will wait
>for a
>> command response that is never going to arrive, timing out
>eventually.
>>
>> As a workaround, the small delay ensures that the CS line is held
>long
>> enough, even with a faulty SPI master. Other SPI masters are not
>affected,
>> except for a negligible performance penalty.
>>
>> Cc: 
>> Fixes: 0edbfea537d1 ("tpm/tpm_tis_spi: Add support for spi phy")
>> Signed-off-by: Alexander Steffen 
>> Signed-off-by: Peter Huewe 
>> ---
>>   drivers/char/tpm/tpm_tis_spi.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/char/tpm/tpm_tis_spi.c
>b/drivers/char/tpm/tpm_tis_spi.c
>> index b50c5b072df3..685c51bf5d7e 100644
>> --- a/drivers/char/tpm/tpm_tis_spi.c
>> +++ b/drivers/char/tpm/tpm_tis_spi.c
>> @@ -110,6 +110,7 @@ static int tpm_tis_spi_transfer(struct
>tpm_tis_data *data, u32 addr, u8 len,
>>   
>>  spi_xfer.cs_change = 0;
>>  spi_xfer.len = transfer_len;
>> +spi_xfer.delay_usecs = 5;
>>   
>>  if (direction) {
>>  spi_xfer.tx_buf = NULL;

-- 
Sent from my mobile

Re: [PATCH 4/5] tpm_tis_spi: Remove limitation of transfers to MAX_SPI_FRAMESIZE bytes

[Peter Huewe]

Am 17. Februar 2017 06:11:53 MEZ schrieb Christophe Ricard 
<christophe.ric...@gmail.com>:
>I am not sure i understand here, are you considering there could be 
>burstcount > 64 with "TCG" TPM ?
>
With the current upstream version, any command larger with a response of more 
than 64 byte is broken, since the splitting loop went missing during the merge.
At least with our tpms that's the case.
(I think we are signalling a higher burstcount value than 64, which is allowed)

>Or is this because of TIS vs PTP differences ?
>
>To be honest, this is a little behind me now :-)
>
>
>On 16/02/2017 08:08, Peter Huewe wrote:
>> Limiting transfers to MAX_SPI_FRAMESIZE was not expected by the upper
>> layers, as tpm_tis has no such limitation. Add a loop to hide that
>> limitation.
>>
>> Cc: <sta...@vger.kernel.org>
>> Fixes: 0edbfea537d1 ("tpm/tpm_tis_spi: Add support for spi phy")
>> Signed-off-by: Alexander Steffen <alexander.stef...@infineon.com>
>> Signed-off-by: Peter Huewe <peter.hu...@infineon.com>
>> ---
>>   drivers/char/tpm/tpm_tis_spi.c | 108
>++---
>>   1 file changed, 57 insertions(+), 51 deletions(-)
>>
>> diff --git a/drivers/char/tpm/tpm_tis_spi.c
>b/drivers/char/tpm/tpm_tis_spi.c
>> index 16938e2253d2..b50c5b072df3 100644
>> --- a/drivers/char/tpm/tpm_tis_spi.c
>> +++ b/drivers/char/tpm/tpm_tis_spi.c
>> @@ -61,68 +61,74 @@ static int tpm_tis_spi_transfer(struct
>tpm_tis_data *data, u32 addr, u8 len,
>>   {
>>  struct tpm_tis_spi_phy *phy = to_tpm_tis_spi_phy(data);
>>  int ret;
>> -struct spi_message m;
>> -struct spi_transfer spi_xfer = {
>> -.tx_buf = phy->tx_buf,
>> -.rx_buf = phy->rx_buf,
>> -.len = 4,
>> -.cs_change = 1,
>> -};
>> -
>> -if (len > MAX_SPI_FRAMESIZE)
>> -return -ENOMEM;
>>   
>> -phy->tx_buf[0] = direction | (len - 1);
>> -phy->tx_buf[1] = 0xd4;
>> -phy->tx_buf[2] = addr >> 8;
>> -phy->tx_buf[3] = addr;
>> +spi_bus_lock(phy->spi_device->master);
>>   
>> -spi_message_init();
>> -spi_message_add_tail(_xfer, );
>> +while (len) {
>> +struct spi_message m;
>> +struct spi_transfer spi_xfer = {
>> +.tx_buf = phy->tx_buf,
>> +.rx_buf = phy->rx_buf,
>> +.len = 4,
>> +.cs_change = 1,
>> +};
>> +u8 transfer_len = min_t(u16, len, MAX_SPI_FRAMESIZE);
>> +
>> +phy->tx_buf[0] = direction | (transfer_len - 1);
>> +phy->tx_buf[1] = 0xd4;
>> +phy->tx_buf[2] = addr >> 8;
>> +phy->tx_buf[3] = addr;
>> +
>> +spi_message_init();
>> +spi_message_add_tail(_xfer, );
>> +ret = spi_sync_locked(phy->spi_device, );
>> +if (ret < 0)
>> +goto exit;
>>   
>> -spi_bus_lock(phy->spi_device->master);
>> -ret = spi_sync_locked(phy->spi_device, );
>> -if (ret < 0)
>> -goto exit;
>> -
>> -if ((phy->rx_buf[3] & 0x01) == 0) {
>> -// handle SPI wait states
>> -int i;
>> -
>> -phy->tx_buf[0] = 0;
>> -
>> -for (i = 0; i < TPM_RETRY; i++) {
>> -spi_xfer.len = 1;
>> -spi_message_init();
>> -spi_message_add_tail(_xfer, );
>> -ret = spi_sync_locked(phy->spi_device, );
>> -if (ret < 0)
>> +if ((phy->rx_buf[3] & 0x01) == 0) {
>> +// handle SPI wait states
>> +int i;
>> +
>> +phy->tx_buf[0] = 0;
>> +
>> +for (i = 0; i < TPM_RETRY; i++) {
>> +spi_xfer.len = 1;
>> +spi_message_init();
>> +spi_message_add_tail(_xfer, );
>> +ret = spi_sync_locked(phy->spi_device, );
>> +if (ret < 0)
>> +goto exit;
>> +if (phy->rx_buf[0] & 0x01)
>> +break;
>> +  

Re: [PATCH 4/5] tpm_tis_spi: Remove limitation of transfers to MAX_SPI_FRAMESIZE bytes

[Peter Huewe]

Am 17. Februar 2017 06:11:53 MEZ schrieb Christophe Ricard 
:
>I am not sure i understand here, are you considering there could be 
>burstcount > 64 with "TCG" TPM ?
>
With the current upstream version, any command larger with a response of more 
than 64 byte is broken, since the splitting loop went missing during the merge.
At least with our tpms that's the case.
(I think we are signalling a higher burstcount value than 64, which is allowed)

>Or is this because of TIS vs PTP differences ?
>
>To be honest, this is a little behind me now :-)
>
>
>On 16/02/2017 08:08, Peter Huewe wrote:
>> Limiting transfers to MAX_SPI_FRAMESIZE was not expected by the upper
>> layers, as tpm_tis has no such limitation. Add a loop to hide that
>> limitation.
>>
>> Cc: 
>> Fixes: 0edbfea537d1 ("tpm/tpm_tis_spi: Add support for spi phy")
>> Signed-off-by: Alexander Steffen 
>> Signed-off-by: Peter Huewe 
>> ---
>>   drivers/char/tpm/tpm_tis_spi.c | 108
>++---
>>   1 file changed, 57 insertions(+), 51 deletions(-)
>>
>> diff --git a/drivers/char/tpm/tpm_tis_spi.c
>b/drivers/char/tpm/tpm_tis_spi.c
>> index 16938e2253d2..b50c5b072df3 100644
>> --- a/drivers/char/tpm/tpm_tis_spi.c
>> +++ b/drivers/char/tpm/tpm_tis_spi.c
>> @@ -61,68 +61,74 @@ static int tpm_tis_spi_transfer(struct
>tpm_tis_data *data, u32 addr, u8 len,
>>   {
>>  struct tpm_tis_spi_phy *phy = to_tpm_tis_spi_phy(data);
>>  int ret;
>> -struct spi_message m;
>> -struct spi_transfer spi_xfer = {
>> -.tx_buf = phy->tx_buf,
>> -.rx_buf = phy->rx_buf,
>> -.len = 4,
>> -.cs_change = 1,
>> -};
>> -
>> -if (len > MAX_SPI_FRAMESIZE)
>> -return -ENOMEM;
>>   
>> -phy->tx_buf[0] = direction | (len - 1);
>> -phy->tx_buf[1] = 0xd4;
>> -phy->tx_buf[2] = addr >> 8;
>> -phy->tx_buf[3] = addr;
>> +spi_bus_lock(phy->spi_device->master);
>>   
>> -spi_message_init();
>> -spi_message_add_tail(_xfer, );
>> +while (len) {
>> +struct spi_message m;
>> +struct spi_transfer spi_xfer = {
>> +.tx_buf = phy->tx_buf,
>> +.rx_buf = phy->rx_buf,
>> +.len = 4,
>> +.cs_change = 1,
>> +};
>> +u8 transfer_len = min_t(u16, len, MAX_SPI_FRAMESIZE);
>> +
>> +phy->tx_buf[0] = direction | (transfer_len - 1);
>> +phy->tx_buf[1] = 0xd4;
>> +phy->tx_buf[2] = addr >> 8;
>> +phy->tx_buf[3] = addr;
>> +
>> +spi_message_init();
>> +spi_message_add_tail(_xfer, );
>> +ret = spi_sync_locked(phy->spi_device, );
>> +if (ret < 0)
>> +goto exit;
>>   
>> -spi_bus_lock(phy->spi_device->master);
>> -ret = spi_sync_locked(phy->spi_device, );
>> -if (ret < 0)
>> -goto exit;
>> -
>> -if ((phy->rx_buf[3] & 0x01) == 0) {
>> -// handle SPI wait states
>> -int i;
>> -
>> -phy->tx_buf[0] = 0;
>> -
>> -for (i = 0; i < TPM_RETRY; i++) {
>> -spi_xfer.len = 1;
>> -spi_message_init();
>> -spi_message_add_tail(_xfer, );
>> -ret = spi_sync_locked(phy->spi_device, );
>> -if (ret < 0)
>> +if ((phy->rx_buf[3] & 0x01) == 0) {
>> +// handle SPI wait states
>> +int i;
>> +
>> +phy->tx_buf[0] = 0;
>> +
>> +for (i = 0; i < TPM_RETRY; i++) {
>> +spi_xfer.len = 1;
>> +spi_message_init();
>> +spi_message_add_tail(_xfer, );
>> +ret = spi_sync_locked(phy->spi_device, );
>> +if (ret < 0)
>> +goto exit;
>> +if (phy->rx_buf[0] & 0x01)
>> +break;
>> +}
>> +
>> +if (i == TPM_RETRY) {
>> +ret = -ETIMEDOUT;
>> 

Re: [PATCH 3/5] tpm_tis_spi: Check correct byte for wait state indicator

[Peter Huewe]

Am 17. Februar 2017 06:09:30 MEZ schrieb Christophe Ricard 
<christophe.ric...@gmail.com>:
>That's is correct, this is a mistake on my side and never saw it :-(.
>
>I guess it was possibly leading to "waste" at least 1 wait state on
>some 
>TPMs.

Unfortunately the 1 for indicating end of waitstates does only appear once so 
it actually rendered the driver non-functional - atleast with our tpms.


>
>Wouldn't it be better to merge that with #1 and update the comment 
>consequently?

Yes, that's what I wanted to express in the cover letter, logically it makes 
sense to squash #1 and #3 - but reviewing it merged with #1 is quite hard since 
it "obfuscates" the problem - since too much stuff moves around.
That's why I decided to split it - for easier review.

Peter


>
>
>On 16/02/2017 08:08, Peter Huewe wrote:
>> Wait states are signaled in the last byte received from the TPM in
>> response to the header, not the first byte. Check rx_buf[3] instead
>of
>> rx_buf[0].
>>
>> Cc: <sta...@vger.kernel.org>
>> Fixes: 0edbfea537d1 ("tpm/tpm_tis_spi: Add support for spi phy")
>> Signed-off-by: Alexander Steffen <alexander.stef...@infineon.com>
>> Signed-off-by: Peter Huewe <peter.hu...@infineon.com>
>> ---
>>   drivers/char/tpm/tpm_tis_spi.c | 40
>+---
>>   1 file changed, 21 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/char/tpm/tpm_tis_spi.c
>b/drivers/char/tpm/tpm_tis_spi.c
>> index d782b9974c14..16938e2253d2 100644
>> --- a/drivers/char/tpm/tpm_tis_spi.c
>> +++ b/drivers/char/tpm/tpm_tis_spi.c
>> @@ -60,7 +60,7 @@ static int tpm_tis_spi_transfer(struct tpm_tis_data
>*data, u32 addr, u8 len,
>>  u8 *buffer, u8 direction)
>>   {
>>  struct tpm_tis_spi_phy *phy = to_tpm_tis_spi_phy(data);
>> -int ret, i;
>> +int ret;
>>  struct spi_message m;
>>  struct spi_transfer spi_xfer = {
>>  .tx_buf = phy->tx_buf,
>> @@ -85,25 +85,27 @@ static int tpm_tis_spi_transfer(struct
>tpm_tis_data *data, u32 addr, u8 len,
>>  if (ret < 0)
>>  goto exit;
>>   
>> -phy->tx_buf[0] = 0;
>> -
>> -/* According to TCG PTP specification, if there is no TPM present
>at
>> - * all, then the design has a weak pull-up on MISO. If a TPM is not
>> - * present, a pull-up on MISO means that the SB controller sees a
>1,
>> - * and will latch in 0xFF on the read.
>> - */
>> -for (i = 0; (phy->rx_buf[0] & 0x01) == 0 && i < TPM_RETRY; i++) {
>> -spi_xfer.len = 1;
>> -spi_message_init();
>> -spi_message_add_tail(_xfer, );
>> -ret = spi_sync_locked(phy->spi_device, );
>> -if (ret < 0)
>> +if ((phy->rx_buf[3] & 0x01) == 0) {
>> +// handle SPI wait states
>> +int i;
>> +
>> +phy->tx_buf[0] = 0;
>> +
>> +for (i = 0; i < TPM_RETRY; i++) {
>> +spi_xfer.len = 1;
>> +spi_message_init();
>> +spi_message_add_tail(_xfer, );
>> +ret = spi_sync_locked(phy->spi_device, );
>> +if (ret < 0)
>> +goto exit;
>> +if (phy->rx_buf[0] & 0x01)
>> +break;
>> +}
>> +
>> +if (i == TPM_RETRY) {
>> +ret = -ETIMEDOUT;
>>  goto exit;
>> -}
>> -
>> -if (i == TPM_RETRY) {
>> -ret = -ETIMEDOUT;
>> -goto exit;
>> +}
>>  }
>>   
>>  spi_xfer.cs_change = 0;

-- 
Sent from my mobile

Re: [PATCH 3/5] tpm_tis_spi: Check correct byte for wait state indicator

[Peter Huewe]

Am 17. Februar 2017 06:09:30 MEZ schrieb Christophe Ricard 
:
>That's is correct, this is a mistake on my side and never saw it :-(.
>
>I guess it was possibly leading to "waste" at least 1 wait state on
>some 
>TPMs.

Unfortunately the 1 for indicating end of waitstates does only appear once so 
it actually rendered the driver non-functional - atleast with our tpms.


>
>Wouldn't it be better to merge that with #1 and update the comment 
>consequently?

Yes, that's what I wanted to express in the cover letter, logically it makes 
sense to squash #1 and #3 - but reviewing it merged with #1 is quite hard since 
it "obfuscates" the problem - since too much stuff moves around.
That's why I decided to split it - for easier review.

Peter


>
>
>On 16/02/2017 08:08, Peter Huewe wrote:
>> Wait states are signaled in the last byte received from the TPM in
>> response to the header, not the first byte. Check rx_buf[3] instead
>of
>> rx_buf[0].
>>
>> Cc: 
>> Fixes: 0edbfea537d1 ("tpm/tpm_tis_spi: Add support for spi phy")
>> Signed-off-by: Alexander Steffen 
>> Signed-off-by: Peter Huewe 
>> ---
>>   drivers/char/tpm/tpm_tis_spi.c | 40
>+---
>>   1 file changed, 21 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/char/tpm/tpm_tis_spi.c
>b/drivers/char/tpm/tpm_tis_spi.c
>> index d782b9974c14..16938e2253d2 100644
>> --- a/drivers/char/tpm/tpm_tis_spi.c
>> +++ b/drivers/char/tpm/tpm_tis_spi.c
>> @@ -60,7 +60,7 @@ static int tpm_tis_spi_transfer(struct tpm_tis_data
>*data, u32 addr, u8 len,
>>  u8 *buffer, u8 direction)
>>   {
>>  struct tpm_tis_spi_phy *phy = to_tpm_tis_spi_phy(data);
>> -int ret, i;
>> +int ret;
>>  struct spi_message m;
>>  struct spi_transfer spi_xfer = {
>>  .tx_buf = phy->tx_buf,
>> @@ -85,25 +85,27 @@ static int tpm_tis_spi_transfer(struct
>tpm_tis_data *data, u32 addr, u8 len,
>>  if (ret < 0)
>>  goto exit;
>>   
>> -phy->tx_buf[0] = 0;
>> -
>> -/* According to TCG PTP specification, if there is no TPM present
>at
>> - * all, then the design has a weak pull-up on MISO. If a TPM is not
>> - * present, a pull-up on MISO means that the SB controller sees a
>1,
>> - * and will latch in 0xFF on the read.
>> - */
>> -for (i = 0; (phy->rx_buf[0] & 0x01) == 0 && i < TPM_RETRY; i++) {
>> -spi_xfer.len = 1;
>> -spi_message_init();
>> -spi_message_add_tail(_xfer, );
>> -ret = spi_sync_locked(phy->spi_device, );
>> -if (ret < 0)
>> +if ((phy->rx_buf[3] & 0x01) == 0) {
>> +// handle SPI wait states
>> +int i;
>> +
>> +phy->tx_buf[0] = 0;
>> +
>> +for (i = 0; i < TPM_RETRY; i++) {
>> +spi_xfer.len = 1;
>> +spi_message_init();
>> +spi_message_add_tail(_xfer, );
>> +ret = spi_sync_locked(phy->spi_device, );
>> +if (ret < 0)
>> +goto exit;
>> +if (phy->rx_buf[0] & 0x01)
>> +break;
>> +}
>> +
>> +if (i == TPM_RETRY) {
>> +ret = -ETIMEDOUT;
>>  goto exit;
>> -}
>> -
>> -if (i == TPM_RETRY) {
>> -ret = -ETIMEDOUT;
>> -goto exit;
>> +}
>>  }
>>   
>>  spi_xfer.cs_change = 0;

-- 
Sent from my mobile

[PATCH 5/5] tpm_tis_spi: Add small delay after last transfer

[Peter Huewe]

Testing the implementation with a Raspberry Pi 2 showed that under some
circumstances its SPI master erroneously releases the CS line before the
transfer is complete, i.e. before the end of the last clock. In this case
the TPM ignores the transfer and misses for example the GO command. The
driver is unable to detect this communication problem and will wait for a
command response that is never going to arrive, timing out eventually.

As a workaround, the small delay ensures that the CS line is held long
enough, even with a faulty SPI master. Other SPI masters are not affected,
except for a negligible performance penalty.

Cc: <sta...@vger.kernel.org>
Fixes: 0edbfea537d1 ("tpm/tpm_tis_spi: Add support for spi phy")
Signed-off-by: Alexander Steffen <alexander.stef...@infineon.com>
Signed-off-by: Peter Huewe <peter.hu...@infineon.com>
---
 drivers/char/tpm/tpm_tis_spi.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/char/tpm/tpm_tis_spi.c b/drivers/char/tpm/tpm_tis_spi.c
index b50c5b072df3..685c51bf5d7e 100644
--- a/drivers/char/tpm/tpm_tis_spi.c
+++ b/drivers/char/tpm/tpm_tis_spi.c
@@ -110,6 +110,7 @@ static int tpm_tis_spi_transfer(struct tpm_tis_data *data, 
u32 addr, u8 len,
 
spi_xfer.cs_change = 0;
spi_xfer.len = transfer_len;
+   spi_xfer.delay_usecs = 5;
 
if (direction) {
spi_xfer.tx_buf = NULL;
-- 
2.7.4

[PATCH 5/5] tpm_tis_spi: Add small delay after last transfer

[Peter Huewe]

Testing the implementation with a Raspberry Pi 2 showed that under some
circumstances its SPI master erroneously releases the CS line before the
transfer is complete, i.e. before the end of the last clock. In this case
the TPM ignores the transfer and misses for example the GO command. The
driver is unable to detect this communication problem and will wait for a
command response that is never going to arrive, timing out eventually.

As a workaround, the small delay ensures that the CS line is held long
enough, even with a faulty SPI master. Other SPI masters are not affected,
except for a negligible performance penalty.

Cc: 
Fixes: 0edbfea537d1 ("tpm/tpm_tis_spi: Add support for spi phy")
Signed-off-by: Alexander Steffen 
Signed-off-by: Peter Huewe 
---
 drivers/char/tpm/tpm_tis_spi.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/char/tpm/tpm_tis_spi.c b/drivers/char/tpm/tpm_tis_spi.c
index b50c5b072df3..685c51bf5d7e 100644
--- a/drivers/char/tpm/tpm_tis_spi.c
+++ b/drivers/char/tpm/tpm_tis_spi.c
@@ -110,6 +110,7 @@ static int tpm_tis_spi_transfer(struct tpm_tis_data *data, 
u32 addr, u8 len,
 
spi_xfer.cs_change = 0;
spi_xfer.len = transfer_len;
+   spi_xfer.delay_usecs = 5;
 
if (direction) {
spi_xfer.tx_buf = NULL;
-- 
2.7.4