diff mbox

[tpmdd-devel,v5,7/7] tpm: replace or remove printk error messages

Message ID 1476838185-24007-8-git-send-email-nayna@linux.vnet.ibm.com
State New
Headers show

Commit Message

Nayna Oct. 19, 2016, 12:49 a.m. UTC
This patch removes the unnecessary error messages on failing to
allocate memory and replaces pr_err/printk with dev_dbg/dev_info
as applicable.

Suggested-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
Signed-off-by: Nayna Jain <nayna@linux.vnet.ibm.com>
---
 drivers/char/tpm/tpm_acpi.c | 17 +++++------------
 drivers/char/tpm/tpm_of.c   | 30 ++++++++++--------------------
 2 files changed, 15 insertions(+), 32 deletions(-)

Comments

Jarkko Sakkinen Oct. 19, 2016, 4:18 p.m. UTC | #1
On Tue, Oct 18, 2016 at 08:49:45PM -0400, Nayna Jain wrote:
> This patch removes the unnecessary error messages on failing to
> allocate memory and replaces pr_err/printk with dev_dbg/dev_info
> as applicable.
> 
> Suggested-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> Signed-off-by: Nayna Jain <nayna@linux.vnet.ibm.com>

Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>

/Jarkko

> ---
>  drivers/char/tpm/tpm_acpi.c | 17 +++++------------
>  drivers/char/tpm/tpm_of.c   | 30 ++++++++++--------------------
>  2 files changed, 15 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm_acpi.c b/drivers/char/tpm/tpm_acpi.c
> index 859bdba..22e42da 100644
> --- a/drivers/char/tpm/tpm_acpi.c
> +++ b/drivers/char/tpm/tpm_acpi.c
> @@ -60,11 +60,8 @@ int read_log_acpi(struct tpm_chip *chip)
>  	status = acpi_get_table(ACPI_SIG_TCPA, 1,
>  				(struct acpi_table_header **)&buff);
>  
> -	if (ACPI_FAILURE(status)) {
> -		printk(KERN_ERR "%s: ERROR - Could not get TCPA table\n",
> -		       __func__);
> +	if (ACPI_FAILURE(status))
>  		return -EIO;
> -	}
>  
>  	switch(buff->platform_class) {
>  	case BIOS_SERVER:
> @@ -78,25 +75,21 @@ int read_log_acpi(struct tpm_chip *chip)
>  		break;
>  	}
>  	if (!len) {
> -		printk(KERN_ERR "%s: ERROR - TCPA log area empty\n", __func__);
> +		dev_dbg(&chip->dev, "%s: ERROR - TCPA log area empty\n",
> +			__func__);
>  		return -EIO;
>  	}
>  
>  	/* malloc EventLog space */
>  	log->bios_event_log = kmalloc(len, GFP_KERNEL);
> -	if (!log->bios_event_log) {
> -		printk("%s: ERROR - Not enough  Memory for BIOS measurements\n",
> -			__func__);
> +	if (!log->bios_event_log)
>  		return -ENOMEM;
> -	}
>  
>  	log->bios_event_log_end = log->bios_event_log + len;
>  
>  	virt = acpi_os_map_iomem(start, len);
> -	if (!virt) {
> -		printk("%s: ERROR - Unable to map memory\n", __func__);
> +	if (!virt)
>  		goto err;
> -	}
>  
>  	memcpy_fromio(log->bios_event_log, virt, len);
>  
> diff --git a/drivers/char/tpm/tpm_of.c b/drivers/char/tpm/tpm_of.c
> index 22b8f81..8b7e677 100644
> --- a/drivers/char/tpm/tpm_of.c
> +++ b/drivers/char/tpm/tpm_of.c
> @@ -31,40 +31,30 @@ int read_log_of(struct tpm_chip *chip)
>  	log = &chip->log;
>  	if (chip->dev.parent->of_node)
>  		np = chip->dev.parent->of_node;
> -	if (!np) {
> -		pr_err("%s: ERROR - IBMVTPM not supported\n", __func__);
> +	if (!np)
>  		return -ENODEV;
> -	}
>  
>  	sizep = of_get_property(np, "linux,sml-size", NULL);
> -	if (sizep == NULL) {
> -		pr_err("%s: ERROR - SML size not found\n", __func__);
> -		goto cleanup_eio;
> -	}
> +	if (sizep == NULL)
> +		return -EIO;
> +
>  	if (*sizep == 0) {
> -		pr_err("%s: ERROR - event log area empty\n", __func__);
> -		goto cleanup_eio;
> +		dev_info(&chip->dev, "%s: ERROR - event log area empty\n",
> +			__func__);
> +		return -EIO;
>  	}
>  
>  	basep = of_get_property(np, "linux,sml-base", NULL);
> -	if (basep == NULL) {
> -		pr_err("%s: ERROR - SML not found\n", __func__);
> -		goto cleanup_eio;
> -	}
> +	if (basep == NULL)
> +		return -EIO;
>  
>  	log->bios_event_log = kmalloc(*sizep, GFP_KERNEL);
> -	if (!log->bios_event_log) {
> -		pr_err("%s: ERROR - Not enough memory for BIOS measurements\n",
> -		       __func__);
> +	if (!log->bios_event_log)
>  		return -ENOMEM;
> -	}
>  
>  	log->bios_event_log_end = log->bios_event_log + *sizep;
>  
>  	memcpy(log->bios_event_log, __va(*basep), *sizep);
>  
>  	return 0;
> -
> -cleanup_eio:
> -	return -EIO;
>  }
> -- 
> 2.5.0
> 

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most 
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
Tomas Winkler Oct. 20, 2016, 7:34 a.m. UTC | #2
> On Tue, Oct 18, 2016 at 08:49:45PM -0400, Nayna Jain wrote:
> > This patch removes the unnecessary error messages on failing to
> > allocate memory and replaces pr_err/printk with dev_dbg/dev_info as
> > applicable.
> >
> > Suggested-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> > Signed-off-by: Nayna Jain <nayna@linux.vnet.ibm.com>
> 
> Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> 
> /Jarkko
> 
> > ---
> >  drivers/char/tpm/tpm_acpi.c | 17 +++++------------
> >  drivers/char/tpm/tpm_of.c   | 30 ++++++++++--------------------
> >  2 files changed, 15 insertions(+), 32 deletions(-)
> >
> > diff --git a/drivers/char/tpm/tpm_acpi.c b/drivers/char/tpm/tpm_acpi.c
> > index 859bdba..22e42da 100644
> > --- a/drivers/char/tpm/tpm_acpi.c
> > +++ b/drivers/char/tpm/tpm_acpi.c
> > @@ -60,11 +60,8 @@ int read_log_acpi(struct tpm_chip *chip)
> >  	status = acpi_get_table(ACPI_SIG_TCPA, 1,
> >  				(struct acpi_table_header **)&buff);
> >
> > -	if (ACPI_FAILURE(status)) {
> > -		printk(KERN_ERR "%s: ERROR - Could not get TCPA table\n",
> > -		       __func__);
> > +	if (ACPI_FAILURE(status))
> >  		return -EIO;
> > -	}
> >
> >  	switch(buff->platform_class) {
> >  	case BIOS_SERVER:
> > @@ -78,25 +75,21 @@ int read_log_acpi(struct tpm_chip *chip)
> >  		break;
> >  	}
> >  	if (!len) {
> > -		printk(KERN_ERR "%s: ERROR - TCPA log area empty\n",
> __func__);
> > +		dev_dbg(&chip->dev, "%s: ERROR - TCPA log area empty\n",
> > +			__func__);


Why to keep __func__ here, dev_dbg already does it for you. 

> >  		return -EIO;
> >  	}
> >
> >  	/* malloc EventLog space */
> >  	log->bios_event_log = kmalloc(len, GFP_KERNEL);
> > -	if (!log->bios_event_log) {
> > -		printk("%s: ERROR - Not enough  Memory for BIOS
> measurements\n",
> > -			__func__);
> > +	if (!log->bios_event_log)
> >  		return -ENOMEM;
> > -	}
> >
> >  	log->bios_event_log_end = log->bios_event_log + len;
> >
> >  	virt = acpi_os_map_iomem(start, len);
> > -	if (!virt) {
> > -		printk("%s: ERROR - Unable to map memory\n", __func__);
> > +	if (!virt)
> >  		goto err;
> > -	}
> >
> >  	memcpy_fromio(log->bios_event_log, virt, len);
> >
> > diff --git a/drivers/char/tpm/tpm_of.c b/drivers/char/tpm/tpm_of.c
> > index 22b8f81..8b7e677 100644
> > --- a/drivers/char/tpm/tpm_of.c
> > +++ b/drivers/char/tpm/tpm_of.c
> > @@ -31,40 +31,30 @@ int read_log_of(struct tpm_chip *chip)
> >  	log = &chip->log;
> >  	if (chip->dev.parent->of_node)
> >  		np = chip->dev.parent->of_node;
> > -	if (!np) {
> > -		pr_err("%s: ERROR - IBMVTPM not supported\n", __func__);
> > +	if (!np)
> >  		return -ENODEV;
> > -	}
> >
> >  	sizep = of_get_property(np, "linux,sml-size", NULL);
> > -	if (sizep == NULL) {
> > -		pr_err("%s: ERROR - SML size not found\n", __func__);
> > -		goto cleanup_eio;
> > -	}
> > +	if (sizep == NULL)
> > +		return -EIO;
> > +
> >  	if (*sizep == 0) {
> > -		pr_err("%s: ERROR - event log area empty\n", __func__);
> > -		goto cleanup_eio;
> > +		dev_info(&chip->dev, "%s: ERROR - event log area empty\n",
> > +			__func__);
> > +		return -EIO;
> >  	}
> >
> >  	basep = of_get_property(np, "linux,sml-base", NULL);
> > -	if (basep == NULL) {
> > -		pr_err("%s: ERROR - SML not found\n", __func__);
> > -		goto cleanup_eio;
> > -	}
> > +	if (basep == NULL)
> > +		return -EIO;
> >
> >  	log->bios_event_log = kmalloc(*sizep, GFP_KERNEL);
> > -	if (!log->bios_event_log) {
> > -		pr_err("%s: ERROR - Not enough memory for BIOS
> measurements\n",
> > -		       __func__);
> > +	if (!log->bios_event_log)
> >  		return -ENOMEM;
> > -	}
> >
> >  	log->bios_event_log_end = log->bios_event_log + *sizep;
> >
> >  	memcpy(log->bios_event_log, __va(*basep), *sizep);
> >
> >  	return 0;
> > -
> > -cleanup_eio:
> > -	return -EIO;
> >  }
> > --
> > 2.5.0
> >
> 


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most 
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
Jarkko Sakkinen Oct. 20, 2016, 11:24 a.m. UTC | #3
On Thu, Oct 20, 2016 at 07:34:37AM +0000, Winkler, Tomas wrote:
> > On Tue, Oct 18, 2016 at 08:49:45PM -0400, Nayna Jain wrote:
> > > This patch removes the unnecessary error messages on failing to
> > > allocate memory and replaces pr_err/printk with dev_dbg/dev_info as
> > > applicable.
> > >
> > > Suggested-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> > > Signed-off-by: Nayna Jain <nayna@linux.vnet.ibm.com>
> > 
> > Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > 
> > /Jarkko
> > 
> > > ---
> > >  drivers/char/tpm/tpm_acpi.c | 17 +++++------------
> > >  drivers/char/tpm/tpm_of.c   | 30 ++++++++++--------------------
> > >  2 files changed, 15 insertions(+), 32 deletions(-)
> > >
> > > diff --git a/drivers/char/tpm/tpm_acpi.c b/drivers/char/tpm/tpm_acpi.c
> > > index 859bdba..22e42da 100644
> > > --- a/drivers/char/tpm/tpm_acpi.c
> > > +++ b/drivers/char/tpm/tpm_acpi.c
> > > @@ -60,11 +60,8 @@ int read_log_acpi(struct tpm_chip *chip)
> > >  	status = acpi_get_table(ACPI_SIG_TCPA, 1,
> > >  				(struct acpi_table_header **)&buff);
> > >
> > > -	if (ACPI_FAILURE(status)) {
> > > -		printk(KERN_ERR "%s: ERROR - Could not get TCPA table\n",
> > > -		       __func__);
> > > +	if (ACPI_FAILURE(status))
> > >  		return -EIO;
> > > -	}
> > >
> > >  	switch(buff->platform_class) {
> > >  	case BIOS_SERVER:
> > > @@ -78,25 +75,21 @@ int read_log_acpi(struct tpm_chip *chip)
> > >  		break;
> > >  	}
> > >  	if (!len) {
> > > -		printk(KERN_ERR "%s: ERROR - TCPA log area empty\n",
> > __func__);
> > > +		dev_dbg(&chip->dev, "%s: ERROR - TCPA log area empty\n",
> > > +			__func__);
> 
> 
> Why to keep __func__ here, dev_dbg already does it for you. 

Good catch. I would actually consider also changing this to

dev_err(dev, FW_BUG "TCPA log area empty\n");

If TCPA exists but it's empty, that's most likely a FW bug.

/Jarkko

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most 
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
Nayna Oct. 20, 2016, 1:31 p.m. UTC | #4
On 10/20/2016 01:04 PM, Winkler, Tomas wrote:
>> On Tue, Oct 18, 2016 at 08:49:45PM -0400, Nayna Jain wrote:
>>> This patch removes the unnecessary error messages on failing to
>>> allocate memory and replaces pr_err/printk with dev_dbg/dev_info as
>>> applicable.
>>>
>>> Suggested-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
>>> Signed-off-by: Nayna Jain <nayna@linux.vnet.ibm.com>
>>
>> Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
>>
>> /Jarkko
>>
>>> ---
>>>   drivers/char/tpm/tpm_acpi.c | 17 +++++------------
>>>   drivers/char/tpm/tpm_of.c   | 30 ++++++++++--------------------
>>>   2 files changed, 15 insertions(+), 32 deletions(-)
>>>
>>> diff --git a/drivers/char/tpm/tpm_acpi.c b/drivers/char/tpm/tpm_acpi.c
>>> index 859bdba..22e42da 100644
>>> --- a/drivers/char/tpm/tpm_acpi.c
>>> +++ b/drivers/char/tpm/tpm_acpi.c
>>> @@ -60,11 +60,8 @@ int read_log_acpi(struct tpm_chip *chip)
>>>   	status = acpi_get_table(ACPI_SIG_TCPA, 1,
>>>   				(struct acpi_table_header **)&buff);
>>>
>>> -	if (ACPI_FAILURE(status)) {
>>> -		printk(KERN_ERR "%s: ERROR - Could not get TCPA table\n",
>>> -		       __func__);
>>> +	if (ACPI_FAILURE(status))
>>>   		return -EIO;
>>> -	}
>>>
>>>   	switch(buff->platform_class) {
>>>   	case BIOS_SERVER:
>>> @@ -78,25 +75,21 @@ int read_log_acpi(struct tpm_chip *chip)
>>>   		break;
>>>   	}
>>>   	if (!len) {
>>> -		printk(KERN_ERR "%s: ERROR - TCPA log area empty\n",
>> __func__);
>>> +		dev_dbg(&chip->dev, "%s: ERROR - TCPA log area empty\n",
>>> +			__func__);
>
>
> Why to keep __func__ here, dev_dbg already does it for you.

I tried to trace through dev_dbg
- http://lxr.free-electrons.com/source/include/linux/device.h#L1195
which further calls dev_printk from 
http://lxr.free-electrons.com/source/drivers/base/core.c#L2199

I couldn't find it printing __func__ anywhere.. I just see it printing 
"driver" and "dev" as tpm : tpm0.

Please let me know if I am missing something

Thanks & Regards,
- Nayna
>
>>>   		return -EIO;
>>>   	}
>>>
>>>   	/* malloc EventLog space */
>>>   	log->bios_event_log = kmalloc(len, GFP_KERNEL);
>>> -	if (!log->bios_event_log) {
>>> -		printk("%s: ERROR - Not enough  Memory for BIOS
>> measurements\n",
>>> -			__func__);
>>> +	if (!log->bios_event_log)
>>>   		return -ENOMEM;
>>> -	}
>>>
>>>   	log->bios_event_log_end = log->bios_event_log + len;
>>>
>>>   	virt = acpi_os_map_iomem(start, len);
>>> -	if (!virt) {
>>> -		printk("%s: ERROR - Unable to map memory\n", __func__);
>>> +	if (!virt)
>>>   		goto err;
>>> -	}
>>>
>>>   	memcpy_fromio(log->bios_event_log, virt, len);
>>>
>>> diff --git a/drivers/char/tpm/tpm_of.c b/drivers/char/tpm/tpm_of.c
>>> index 22b8f81..8b7e677 100644
>>> --- a/drivers/char/tpm/tpm_of.c
>>> +++ b/drivers/char/tpm/tpm_of.c
>>> @@ -31,40 +31,30 @@ int read_log_of(struct tpm_chip *chip)
>>>   	log = &chip->log;
>>>   	if (chip->dev.parent->of_node)
>>>   		np = chip->dev.parent->of_node;
>>> -	if (!np) {
>>> -		pr_err("%s: ERROR - IBMVTPM not supported\n", __func__);
>>> +	if (!np)
>>>   		return -ENODEV;
>>> -	}
>>>
>>>   	sizep = of_get_property(np, "linux,sml-size", NULL);
>>> -	if (sizep == NULL) {
>>> -		pr_err("%s: ERROR - SML size not found\n", __func__);
>>> -		goto cleanup_eio;
>>> -	}
>>> +	if (sizep == NULL)
>>> +		return -EIO;
>>> +
>>>   	if (*sizep == 0) {
>>> -		pr_err("%s: ERROR - event log area empty\n", __func__);
>>> -		goto cleanup_eio;
>>> +		dev_info(&chip->dev, "%s: ERROR - event log area empty\n",
>>> +			__func__);
>>> +		return -EIO;
>>>   	}
>>>
>>>   	basep = of_get_property(np, "linux,sml-base", NULL);
>>> -	if (basep == NULL) {
>>> -		pr_err("%s: ERROR - SML not found\n", __func__);
>>> -		goto cleanup_eio;
>>> -	}
>>> +	if (basep == NULL)
>>> +		return -EIO;
>>>
>>>   	log->bios_event_log = kmalloc(*sizep, GFP_KERNEL);
>>> -	if (!log->bios_event_log) {
>>> -		pr_err("%s: ERROR - Not enough memory for BIOS
>> measurements\n",
>>> -		       __func__);
>>> +	if (!log->bios_event_log)
>>>   		return -ENOMEM;
>>> -	}
>>>
>>>   	log->bios_event_log_end = log->bios_event_log + *sizep;
>>>
>>>   	memcpy(log->bios_event_log, __va(*basep), *sizep);
>>>
>>>   	return 0;
>>> -
>>> -cleanup_eio:
>>> -	return -EIO;
>>>   }
>>> --
>>> 2.5.0
>>>
>>
>


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most 
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
Jarkko Sakkinen Oct. 20, 2016, 1:43 p.m. UTC | #5
On Thu, Oct 20, 2016 at 07:01:51PM +0530, Nayna wrote:
> 
> 
> On 10/20/2016 01:04 PM, Winkler, Tomas wrote:
> > > On Tue, Oct 18, 2016 at 08:49:45PM -0400, Nayna Jain wrote:
> > > > This patch removes the unnecessary error messages on failing to
> > > > allocate memory and replaces pr_err/printk with dev_dbg/dev_info as
> > > > applicable.
> > > > 
> > > > Suggested-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> > > > Signed-off-by: Nayna Jain <nayna@linux.vnet.ibm.com>
> > > 
> > > Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > > 
> > > /Jarkko
> > > 
> > > > ---
> > > >   drivers/char/tpm/tpm_acpi.c | 17 +++++------------
> > > >   drivers/char/tpm/tpm_of.c   | 30 ++++++++++--------------------
> > > >   2 files changed, 15 insertions(+), 32 deletions(-)
> > > > 
> > > > diff --git a/drivers/char/tpm/tpm_acpi.c b/drivers/char/tpm/tpm_acpi.c
> > > > index 859bdba..22e42da 100644
> > > > --- a/drivers/char/tpm/tpm_acpi.c
> > > > +++ b/drivers/char/tpm/tpm_acpi.c
> > > > @@ -60,11 +60,8 @@ int read_log_acpi(struct tpm_chip *chip)
> > > >   	status = acpi_get_table(ACPI_SIG_TCPA, 1,
> > > >   				(struct acpi_table_header **)&buff);
> > > > 
> > > > -	if (ACPI_FAILURE(status)) {
> > > > -		printk(KERN_ERR "%s: ERROR - Could not get TCPA table\n",
> > > > -		       __func__);
> > > > +	if (ACPI_FAILURE(status))
> > > >   		return -EIO;
> > > > -	}
> > > > 
> > > >   	switch(buff->platform_class) {
> > > >   	case BIOS_SERVER:
> > > > @@ -78,25 +75,21 @@ int read_log_acpi(struct tpm_chip *chip)
> > > >   		break;
> > > >   	}
> > > >   	if (!len) {
> > > > -		printk(KERN_ERR "%s: ERROR - TCPA log area empty\n",
> > > __func__);
> > > > +		dev_dbg(&chip->dev, "%s: ERROR - TCPA log area empty\n",
> > > > +			__func__);
> > 
> > 
> > Why to keep __func__ here, dev_dbg already does it for you.
> 
> I tried to trace through dev_dbg
> - http://lxr.free-electrons.com/source/include/linux/device.h#L1195
> which further calls dev_printk from
> http://lxr.free-electrons.com/source/drivers/base/core.c#L2199
> 
> I couldn't find it printing __func__ anywhere.. I just see it printing
> "driver" and "dev" as tpm : tpm0.
> 
> Please let me know if I am missing something
> 
> Thanks & Regards,
> - Nayna

Sorry I mixed up in my last response. You can filter in dynamic
debug with a function name but doesn't print function name.

Anyway I would recommend to change this to dev_err in a way that
I explained in my response.

/Jarkko

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most 
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
Nayna Oct. 21, 2016, 3:22 a.m. UTC | #6
On 10/20/2016 04:54 PM, Jarkko Sakkinen wrote:
> On Thu, Oct 20, 2016 at 07:34:37AM +0000, Winkler, Tomas wrote:
>>> On Tue, Oct 18, 2016 at 08:49:45PM -0400, Nayna Jain wrote:
>>>> This patch removes the unnecessary error messages on failing to
>>>> allocate memory and replaces pr_err/printk with dev_dbg/dev_info as
>>>> applicable.
>>>>
>>>> Suggested-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
>>>> Signed-off-by: Nayna Jain <nayna@linux.vnet.ibm.com>
>>>
>>> Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
>>>
>>> /Jarkko
>>>
>>>> ---
>>>>   drivers/char/tpm/tpm_acpi.c | 17 +++++------------
>>>>   drivers/char/tpm/tpm_of.c   | 30 ++++++++++--------------------
>>>>   2 files changed, 15 insertions(+), 32 deletions(-)
>>>>
>>>> diff --git a/drivers/char/tpm/tpm_acpi.c b/drivers/char/tpm/tpm_acpi.c
>>>> index 859bdba..22e42da 100644
>>>> --- a/drivers/char/tpm/tpm_acpi.c
>>>> +++ b/drivers/char/tpm/tpm_acpi.c
>>>> @@ -60,11 +60,8 @@ int read_log_acpi(struct tpm_chip *chip)
>>>>   	status = acpi_get_table(ACPI_SIG_TCPA, 1,
>>>>   				(struct acpi_table_header **)&buff);
>>>>
>>>> -	if (ACPI_FAILURE(status)) {
>>>> -		printk(KERN_ERR "%s: ERROR - Could not get TCPA table\n",
>>>> -		       __func__);
>>>> +	if (ACPI_FAILURE(status))
>>>>   		return -EIO;
>>>> -	}
>>>>
>>>>   	switch(buff->platform_class) {
>>>>   	case BIOS_SERVER:
>>>> @@ -78,25 +75,21 @@ int read_log_acpi(struct tpm_chip *chip)
>>>>   		break;
>>>>   	}
>>>>   	if (!len) {
>>>> -		printk(KERN_ERR "%s: ERROR - TCPA log area empty\n",
>>> __func__);
>>>> +		dev_dbg(&chip->dev, "%s: ERROR - TCPA log area empty\n",
>>>> +			__func__);
>>
>>
>> Why to keep __func__ here, dev_dbg already does it for you.
>
> Good catch. I would actually consider also changing this to
>
> dev_err(dev, FW_BUG "TCPA log area empty\n");
>
> If TCPA exists but it's empty, that's most likely a FW bug.

If it can be possibly a FW bug, should it fail the probe also just like 
-ENOMEM error ?

Thanks & Regards,
     - Nayna

>
> /Jarkko
>


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most 
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
Jarkko Sakkinen Oct. 21, 2016, 3:02 p.m. UTC | #7
On Fri, Oct 21, 2016 at 08:52:14AM +0530, Nayna wrote:
> 
> 
> On 10/20/2016 04:54 PM, Jarkko Sakkinen wrote:
> > On Thu, Oct 20, 2016 at 07:34:37AM +0000, Winkler, Tomas wrote:
> > > > On Tue, Oct 18, 2016 at 08:49:45PM -0400, Nayna Jain wrote:
> > > > > This patch removes the unnecessary error messages on failing to
> > > > > allocate memory and replaces pr_err/printk with dev_dbg/dev_info as
> > > > > applicable.
> > > > > 
> > > > > Suggested-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> > > > > Signed-off-by: Nayna Jain <nayna@linux.vnet.ibm.com>
> > > > 
> > > > Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > > > 
> > > > /Jarkko
> > > > 
> > > > > ---
> > > > >   drivers/char/tpm/tpm_acpi.c | 17 +++++------------
> > > > >   drivers/char/tpm/tpm_of.c   | 30 ++++++++++--------------------
> > > > >   2 files changed, 15 insertions(+), 32 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/char/tpm/tpm_acpi.c b/drivers/char/tpm/tpm_acpi.c
> > > > > index 859bdba..22e42da 100644
> > > > > --- a/drivers/char/tpm/tpm_acpi.c
> > > > > +++ b/drivers/char/tpm/tpm_acpi.c
> > > > > @@ -60,11 +60,8 @@ int read_log_acpi(struct tpm_chip *chip)
> > > > >   	status = acpi_get_table(ACPI_SIG_TCPA, 1,
> > > > >   				(struct acpi_table_header **)&buff);
> > > > > 
> > > > > -	if (ACPI_FAILURE(status)) {
> > > > > -		printk(KERN_ERR "%s: ERROR - Could not get TCPA table\n",
> > > > > -		       __func__);
> > > > > +	if (ACPI_FAILURE(status))
> > > > >   		return -EIO;
> > > > > -	}
> > > > > 
> > > > >   	switch(buff->platform_class) {
> > > > >   	case BIOS_SERVER:
> > > > > @@ -78,25 +75,21 @@ int read_log_acpi(struct tpm_chip *chip)
> > > > >   		break;
> > > > >   	}
> > > > >   	if (!len) {
> > > > > -		printk(KERN_ERR "%s: ERROR - TCPA log area empty\n",
> > > > __func__);
> > > > > +		dev_dbg(&chip->dev, "%s: ERROR - TCPA log area empty\n",
> > > > > +			__func__);
> > > 
> > > 
> > > Why to keep __func__ here, dev_dbg already does it for you.
> > 
> > Good catch. I would actually consider also changing this to
> > 
> > dev_err(dev, FW_BUG "TCPA log area empty\n");
> > 
> > If TCPA exists but it's empty, that's most likely a FW bug.
> 
> If it can be possibly a FW bug, should it fail the probe also just like
> -ENOMEM error ?

I think so but I hold for second opinion on this. I mean wouldn't
it seem like a bit broken situation where TCPA tabe would exist but
would also be empty?

/Jarkko

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most 
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
Nayna Oct. 26, 2016, 2:22 a.m. UTC | #8
On 10/21/2016 08:32 PM, Jarkko Sakkinen wrote:
> On Fri, Oct 21, 2016 at 08:52:14AM +0530, Nayna wrote:
>>
>>
>> On 10/20/2016 04:54 PM, Jarkko Sakkinen wrote:
>>> On Thu, Oct 20, 2016 at 07:34:37AM +0000, Winkler, Tomas wrote:
>>>>> On Tue, Oct 18, 2016 at 08:49:45PM -0400, Nayna Jain wrote:
>>>>>> This patch removes the unnecessary error messages on failing to
>>>>>> allocate memory and replaces pr_err/printk with dev_dbg/dev_info as
>>>>>> applicable.
>>>>>>
>>>>>> Suggested-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
>>>>>> Signed-off-by: Nayna Jain <nayna@linux.vnet.ibm.com>
>>>>>
>>>>> Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
>>>>>
>>>>> /Jarkko
>>>>>
>>>>>> ---
>>>>>>    drivers/char/tpm/tpm_acpi.c | 17 +++++------------
>>>>>>    drivers/char/tpm/tpm_of.c   | 30 ++++++++++--------------------
>>>>>>    2 files changed, 15 insertions(+), 32 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/char/tpm/tpm_acpi.c b/drivers/char/tpm/tpm_acpi.c
>>>>>> index 859bdba..22e42da 100644
>>>>>> --- a/drivers/char/tpm/tpm_acpi.c
>>>>>> +++ b/drivers/char/tpm/tpm_acpi.c
>>>>>> @@ -60,11 +60,8 @@ int read_log_acpi(struct tpm_chip *chip)
>>>>>>    	status = acpi_get_table(ACPI_SIG_TCPA, 1,
>>>>>>    				(struct acpi_table_header **)&buff);
>>>>>>
>>>>>> -	if (ACPI_FAILURE(status)) {
>>>>>> -		printk(KERN_ERR "%s: ERROR - Could not get TCPA table\n",
>>>>>> -		       __func__);
>>>>>> +	if (ACPI_FAILURE(status))
>>>>>>    		return -EIO;
>>>>>> -	}
>>>>>>
>>>>>>    	switch(buff->platform_class) {
>>>>>>    	case BIOS_SERVER:
>>>>>> @@ -78,25 +75,21 @@ int read_log_acpi(struct tpm_chip *chip)
>>>>>>    		break;
>>>>>>    	}
>>>>>>    	if (!len) {
>>>>>> -		printk(KERN_ERR "%s: ERROR - TCPA log area empty\n",
>>>>> __func__);
>>>>>> +		dev_dbg(&chip->dev, "%s: ERROR - TCPA log area empty\n",
>>>>>> +			__func__);
>>>>
>>>>
>>>> Why to keep __func__ here, dev_dbg already does it for you.
>>>
>>> Good catch. I would actually consider also changing this to
>>>
>>> dev_err(dev, FW_BUG "TCPA log area empty\n");
>>>
>>> If TCPA exists but it's empty, that's most likely a FW bug.
>>
>> If it can be possibly a FW bug, should it fail the probe also just like
>> -ENOMEM error ?
>
> I think so but I hold for second opinion on this. I mean wouldn't
> it seem like a bit broken situation where TCPA tabe would exist but
> would also be empty?

Hmm, is it possible for this to be firmware implementation dependent ?

Thanks & Regards,
    - Nayna
>
> /Jarkko
>


------------------------------------------------------------------------------
The Command Line: Reinvented for Modern Developers
Did the resurgence of CLI tooling catch you by surprise?
Reconnect with the command line and become more productive. 
Learn the new .NET and ASP.NET CLI. Get your free copy!
http://sdm.link/telerik
Jarkko Sakkinen Oct. 26, 2016, 10:56 a.m. UTC | #9
On Wed, Oct 26, 2016 at 07:52:53AM +0530, Nayna wrote:
> 
> 
> On 10/21/2016 08:32 PM, Jarkko Sakkinen wrote:
> > On Fri, Oct 21, 2016 at 08:52:14AM +0530, Nayna wrote:
> > > 
> > > 
> > > On 10/20/2016 04:54 PM, Jarkko Sakkinen wrote:
> > > > On Thu, Oct 20, 2016 at 07:34:37AM +0000, Winkler, Tomas wrote:
> > > > > > On Tue, Oct 18, 2016 at 08:49:45PM -0400, Nayna Jain wrote:
> > > > > > > This patch removes the unnecessary error messages on failing to
> > > > > > > allocate memory and replaces pr_err/printk with dev_dbg/dev_info as
> > > > > > > applicable.
> > > > > > > 
> > > > > > > Suggested-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> > > > > > > Signed-off-by: Nayna Jain <nayna@linux.vnet.ibm.com>
> > > > > > 
> > > > > > Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > > > > > 
> > > > > > /Jarkko
> > > > > > 
> > > > > > > ---
> > > > > > >    drivers/char/tpm/tpm_acpi.c | 17 +++++------------
> > > > > > >    drivers/char/tpm/tpm_of.c   | 30 ++++++++++--------------------
> > > > > > >    2 files changed, 15 insertions(+), 32 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/drivers/char/tpm/tpm_acpi.c b/drivers/char/tpm/tpm_acpi.c
> > > > > > > index 859bdba..22e42da 100644
> > > > > > > --- a/drivers/char/tpm/tpm_acpi.c
> > > > > > > +++ b/drivers/char/tpm/tpm_acpi.c
> > > > > > > @@ -60,11 +60,8 @@ int read_log_acpi(struct tpm_chip *chip)
> > > > > > >    	status = acpi_get_table(ACPI_SIG_TCPA, 1,
> > > > > > >    				(struct acpi_table_header **)&buff);
> > > > > > > 
> > > > > > > -	if (ACPI_FAILURE(status)) {
> > > > > > > -		printk(KERN_ERR "%s: ERROR - Could not get TCPA table\n",
> > > > > > > -		       __func__);
> > > > > > > +	if (ACPI_FAILURE(status))
> > > > > > >    		return -EIO;
> > > > > > > -	}
> > > > > > > 
> > > > > > >    	switch(buff->platform_class) {
> > > > > > >    	case BIOS_SERVER:
> > > > > > > @@ -78,25 +75,21 @@ int read_log_acpi(struct tpm_chip *chip)
> > > > > > >    		break;
> > > > > > >    	}
> > > > > > >    	if (!len) {
> > > > > > > -		printk(KERN_ERR "%s: ERROR - TCPA log area empty\n",
> > > > > > __func__);
> > > > > > > +		dev_dbg(&chip->dev, "%s: ERROR - TCPA log area empty\n",
> > > > > > > +			__func__);
> > > > > 
> > > > > 
> > > > > Why to keep __func__ here, dev_dbg already does it for you.
> > > > 
> > > > Good catch. I would actually consider also changing this to
> > > > 
> > > > dev_err(dev, FW_BUG "TCPA log area empty\n");
> > > > 
> > > > If TCPA exists but it's empty, that's most likely a FW bug.
> > > 
> > > If it can be possibly a FW bug, should it fail the probe also just like
> > > -ENOMEM error ?
> > 
> > I think so but I hold for second opinion on this. I mean wouldn't
> > it seem like a bit broken situation where TCPA tabe would exist but
> > would also be empty?
> 
> Hmm, is it possible for this to be firmware implementation dependent ?

Let me put it this way. Why would anyone expose TCPA to the user space
that is empty? What would be the point?

/Jarkko

------------------------------------------------------------------------------
The Command Line: Reinvented for Modern Developers
Did the resurgence of CLI tooling catch you by surprise?
Reconnect with the command line and become more productive. 
Learn the new .NET and ASP.NET CLI. Get your free copy!
http://sdm.link/telerik
Nayna Oct. 26, 2016, 5:31 p.m. UTC | #10
On 10/26/2016 04:26 PM, Jarkko Sakkinen wrote:
> On Wed, Oct 26, 2016 at 07:52:53AM +0530, Nayna wrote:
>>
>>
>> On 10/21/2016 08:32 PM, Jarkko Sakkinen wrote:
>>> On Fri, Oct 21, 2016 at 08:52:14AM +0530, Nayna wrote:
>>>>
>>>>
>>>> On 10/20/2016 04:54 PM, Jarkko Sakkinen wrote:
>>>>> On Thu, Oct 20, 2016 at 07:34:37AM +0000, Winkler, Tomas wrote:
>>>>>>> On Tue, Oct 18, 2016 at 08:49:45PM -0400, Nayna Jain wrote:
>>>>>>>> This patch removes the unnecessary error messages on failing to
>>>>>>>> allocate memory and replaces pr_err/printk with dev_dbg/dev_info as
>>>>>>>> applicable.
>>>>>>>>
>>>>>>>> Suggested-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
>>>>>>>> Signed-off-by: Nayna Jain <nayna@linux.vnet.ibm.com>
>>>>>>>
>>>>>>> Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
>>>>>>>
>>>>>>> /Jarkko
>>>>>>>
>>>>>>>> ---
>>>>>>>>     drivers/char/tpm/tpm_acpi.c | 17 +++++------------
>>>>>>>>     drivers/char/tpm/tpm_of.c   | 30 ++++++++++--------------------
>>>>>>>>     2 files changed, 15 insertions(+), 32 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/char/tpm/tpm_acpi.c b/drivers/char/tpm/tpm_acpi.c
>>>>>>>> index 859bdba..22e42da 100644
>>>>>>>> --- a/drivers/char/tpm/tpm_acpi.c
>>>>>>>> +++ b/drivers/char/tpm/tpm_acpi.c
>>>>>>>> @@ -60,11 +60,8 @@ int read_log_acpi(struct tpm_chip *chip)
>>>>>>>>     	status = acpi_get_table(ACPI_SIG_TCPA, 1,
>>>>>>>>     				(struct acpi_table_header **)&buff);
>>>>>>>>
>>>>>>>> -	if (ACPI_FAILURE(status)) {
>>>>>>>> -		printk(KERN_ERR "%s: ERROR - Could not get TCPA table\n",
>>>>>>>> -		       __func__);
>>>>>>>> +	if (ACPI_FAILURE(status))
>>>>>>>>     		return -EIO;
>>>>>>>> -	}
>>>>>>>>
>>>>>>>>     	switch(buff->platform_class) {
>>>>>>>>     	case BIOS_SERVER:
>>>>>>>> @@ -78,25 +75,21 @@ int read_log_acpi(struct tpm_chip *chip)
>>>>>>>>     		break;
>>>>>>>>     	}
>>>>>>>>     	if (!len) {
>>>>>>>> -		printk(KERN_ERR "%s: ERROR - TCPA log area empty\n",
>>>>>>> __func__);
>>>>>>>> +		dev_dbg(&chip->dev, "%s: ERROR - TCPA log area empty\n",
>>>>>>>> +			__func__);
>>>>>>
>>>>>>
>>>>>> Why to keep __func__ here, dev_dbg already does it for you.
>>>>>
>>>>> Good catch. I would actually consider also changing this to
>>>>>
>>>>> dev_err(dev, FW_BUG "TCPA log area empty\n");
>>>>>
>>>>> If TCPA exists but it's empty, that's most likely a FW bug.
>>>>
>>>> If it can be possibly a FW bug, should it fail the probe also just like
>>>> -ENOMEM error ?
>>>
>>> I think so but I hold for second opinion on this. I mean wouldn't
>>> it seem like a bit broken situation where TCPA tabe would exist but
>>> would also be empty?
>>
>> Hmm, is it possible for this to be firmware implementation dependent ?
>
> Let me put it this way. Why would anyone expose TCPA to the user space
> that is empty? What would be the point?

If I understand correctly,  the reserved memory for event log would be 
allocated much earlier and firmware would directly write to this 
allocated memory. So, if there is a firmware bug, and events are not 
recorded, it will get exposed to userspace as empty event log and this 
might also help applications to know that it is broken.  Is there any 
wrong assumption here ?

Thanks & Regards,
- Nayna

>
> /Jarkko
>


------------------------------------------------------------------------------
The Command Line: Reinvented for Modern Developers
Did the resurgence of CLI tooling catch you by surprise?
Reconnect with the command line and become more productive. 
Learn the new .NET and ASP.NET CLI. Get your free copy!
http://sdm.link/telerik
Jarkko Sakkinen Oct. 27, 2016, 1:53 p.m. UTC | #11
On Wed, Oct 26, 2016 at 11:01:00PM +0530, Nayna wrote:
> 
> 
> On 10/26/2016 04:26 PM, Jarkko Sakkinen wrote:
> > On Wed, Oct 26, 2016 at 07:52:53AM +0530, Nayna wrote:
> > > 
> > > 
> > > On 10/21/2016 08:32 PM, Jarkko Sakkinen wrote:
> > > > On Fri, Oct 21, 2016 at 08:52:14AM +0530, Nayna wrote:
> > > > > 
> > > > > 
> > > > > On 10/20/2016 04:54 PM, Jarkko Sakkinen wrote:
> > > > > > On Thu, Oct 20, 2016 at 07:34:37AM +0000, Winkler, Tomas wrote:
> > > > > > > > On Tue, Oct 18, 2016 at 08:49:45PM -0400, Nayna Jain wrote:
> > > > > > > > > This patch removes the unnecessary error messages on failing to
> > > > > > > > > allocate memory and replaces pr_err/printk with dev_dbg/dev_info as
> > > > > > > > > applicable.
> > > > > > > > > 
> > > > > > > > > Suggested-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> > > > > > > > > Signed-off-by: Nayna Jain <nayna@linux.vnet.ibm.com>
> > > > > > > > 
> > > > > > > > Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > > > > > > > 
> > > > > > > > /Jarkko
> > > > > > > > 
> > > > > > > > > ---
> > > > > > > > >     drivers/char/tpm/tpm_acpi.c | 17 +++++------------
> > > > > > > > >     drivers/char/tpm/tpm_of.c   | 30 ++++++++++--------------------
> > > > > > > > >     2 files changed, 15 insertions(+), 32 deletions(-)
> > > > > > > > > 
> > > > > > > > > diff --git a/drivers/char/tpm/tpm_acpi.c b/drivers/char/tpm/tpm_acpi.c
> > > > > > > > > index 859bdba..22e42da 100644
> > > > > > > > > --- a/drivers/char/tpm/tpm_acpi.c
> > > > > > > > > +++ b/drivers/char/tpm/tpm_acpi.c
> > > > > > > > > @@ -60,11 +60,8 @@ int read_log_acpi(struct tpm_chip *chip)
> > > > > > > > >     	status = acpi_get_table(ACPI_SIG_TCPA, 1,
> > > > > > > > >     				(struct acpi_table_header **)&buff);
> > > > > > > > > 
> > > > > > > > > -	if (ACPI_FAILURE(status)) {
> > > > > > > > > -		printk(KERN_ERR "%s: ERROR - Could not get TCPA table\n",
> > > > > > > > > -		       __func__);
> > > > > > > > > +	if (ACPI_FAILURE(status))
> > > > > > > > >     		return -EIO;
> > > > > > > > > -	}
> > > > > > > > > 
> > > > > > > > >     	switch(buff->platform_class) {
> > > > > > > > >     	case BIOS_SERVER:
> > > > > > > > > @@ -78,25 +75,21 @@ int read_log_acpi(struct tpm_chip *chip)
> > > > > > > > >     		break;
> > > > > > > > >     	}
> > > > > > > > >     	if (!len) {
> > > > > > > > > -		printk(KERN_ERR "%s: ERROR - TCPA log area empty\n",
> > > > > > > > __func__);
> > > > > > > > > +		dev_dbg(&chip->dev, "%s: ERROR - TCPA log area empty\n",
> > > > > > > > > +			__func__);
> > > > > > > 
> > > > > > > 
> > > > > > > Why to keep __func__ here, dev_dbg already does it for you.
> > > > > > 
> > > > > > Good catch. I would actually consider also changing this to
> > > > > > 
> > > > > > dev_err(dev, FW_BUG "TCPA log area empty\n");
> > > > > > 
> > > > > > If TCPA exists but it's empty, that's most likely a FW bug.
> > > > > 
> > > > > If it can be possibly a FW bug, should it fail the probe also just like
> > > > > -ENOMEM error ?
> > > > 
> > > > I think so but I hold for second opinion on this. I mean wouldn't
> > > > it seem like a bit broken situation where TCPA tabe would exist but
> > > > would also be empty?
> > > 
> > > Hmm, is it possible for this to be firmware implementation dependent ?
> > 
> > Let me put it this way. Why would anyone expose TCPA to the user space
> > that is empty? What would be the point?
> 
> If I understand correctly,  the reserved memory for event log would be
> allocated much earlier and firmware would directly write to this allocated
> memory. So, if there is a firmware bug, and events are not recorded, it will
> get exposed to userspace as empty event log and this might also help
> applications to know that it is broken.  Is there any wrong assumption here
> ?

I think the right question to ask is can event log be legally empty?
If not, then FW_BUG should be used here.

/Jarkko

------------------------------------------------------------------------------
The Command Line: Reinvented for Modern Developers
Did the resurgence of CLI tooling catch you by surprise?
Reconnect with the command line and become more productive. 
Learn the new .NET and ASP.NET CLI. Get your free copy!
http://sdm.link/telerik
Nayna Oct. 27, 2016, 3:05 p.m. UTC | #12
On 10/27/2016 07:23 PM, Jarkko Sakkinen wrote:
> On Wed, Oct 26, 2016 at 11:01:00PM +0530, Nayna wrote:
>>
>>
>> On 10/26/2016 04:26 PM, Jarkko Sakkinen wrote:
>>> On Wed, Oct 26, 2016 at 07:52:53AM +0530, Nayna wrote:
>>>>
>>>>
>>>> On 10/21/2016 08:32 PM, Jarkko Sakkinen wrote:
>>>>> On Fri, Oct 21, 2016 at 08:52:14AM +0530, Nayna wrote:
>>>>>>
>>>>>>
>>>>>> On 10/20/2016 04:54 PM, Jarkko Sakkinen wrote:
>>>>>>> On Thu, Oct 20, 2016 at 07:34:37AM +0000, Winkler, Tomas wrote:
>>>>>>>>> On Tue, Oct 18, 2016 at 08:49:45PM -0400, Nayna Jain wrote:
>>>>>>>>>> This patch removes the unnecessary error messages on failing to
>>>>>>>>>> allocate memory and replaces pr_err/printk with dev_dbg/dev_info as
>>>>>>>>>> applicable.
>>>>>>>>>>
>>>>>>>>>> Suggested-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
>>>>>>>>>> Signed-off-by: Nayna Jain <nayna@linux.vnet.ibm.com>
>>>>>>>>>
>>>>>>>>> Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
>>>>>>>>>
>>>>>>>>> /Jarkko
>>>>>>>>>
>>>>>>>>>> ---
>>>>>>>>>>      drivers/char/tpm/tpm_acpi.c | 17 +++++------------
>>>>>>>>>>      drivers/char/tpm/tpm_of.c   | 30 ++++++++++--------------------
>>>>>>>>>>      2 files changed, 15 insertions(+), 32 deletions(-)
>>>>>>>>>>
>>>>>>>>>> diff --git a/drivers/char/tpm/tpm_acpi.c b/drivers/char/tpm/tpm_acpi.c
>>>>>>>>>> index 859bdba..22e42da 100644
>>>>>>>>>> --- a/drivers/char/tpm/tpm_acpi.c
>>>>>>>>>> +++ b/drivers/char/tpm/tpm_acpi.c
>>>>>>>>>> @@ -60,11 +60,8 @@ int read_log_acpi(struct tpm_chip *chip)
>>>>>>>>>>      	status = acpi_get_table(ACPI_SIG_TCPA, 1,
>>>>>>>>>>      				(struct acpi_table_header **)&buff);
>>>>>>>>>>
>>>>>>>>>> -	if (ACPI_FAILURE(status)) {
>>>>>>>>>> -		printk(KERN_ERR "%s: ERROR - Could not get TCPA table\n",
>>>>>>>>>> -		       __func__);
>>>>>>>>>> +	if (ACPI_FAILURE(status))
>>>>>>>>>>      		return -EIO;
>>>>>>>>>> -	}
>>>>>>>>>>
>>>>>>>>>>      	switch(buff->platform_class) {
>>>>>>>>>>      	case BIOS_SERVER:
>>>>>>>>>> @@ -78,25 +75,21 @@ int read_log_acpi(struct tpm_chip *chip)
>>>>>>>>>>      		break;
>>>>>>>>>>      	}
>>>>>>>>>>      	if (!len) {
>>>>>>>>>> -		printk(KERN_ERR "%s: ERROR - TCPA log area empty\n",
>>>>>>>>> __func__);
>>>>>>>>>> +		dev_dbg(&chip->dev, "%s: ERROR - TCPA log area empty\n",
>>>>>>>>>> +			__func__);
>>>>>>>>
>>>>>>>>
>>>>>>>> Why to keep __func__ here, dev_dbg already does it for you.
>>>>>>>
>>>>>>> Good catch. I would actually consider also changing this to
>>>>>>>
>>>>>>> dev_err(dev, FW_BUG "TCPA log area empty\n");
>>>>>>>
>>>>>>> If TCPA exists but it's empty, that's most likely a FW bug.
>>>>>>
>>>>>> If it can be possibly a FW bug, should it fail the probe also just like
>>>>>> -ENOMEM error ?
>>>>>
>>>>> I think so but I hold for second opinion on this. I mean wouldn't
>>>>> it seem like a bit broken situation where TCPA tabe would exist but
>>>>> would also be empty?
>>>>
>>>> Hmm, is it possible for this to be firmware implementation dependent ?
>>>
>>> Let me put it this way. Why would anyone expose TCPA to the user space
>>> that is empty? What would be the point?
>>
>> If I understand correctly,  the reserved memory for event log would be
>> allocated much earlier and firmware would directly write to this allocated
>> memory. So, if there is a firmware bug, and events are not recorded, it will
>> get exposed to userspace as empty event log and this might also help
>> applications to know that it is broken.  Is there any wrong assumption here
>> ?
>
> I think the right question to ask is can event log be legally empty?

No

> If not, then FW_BUG should be used here.
Ok. yeah. and then I think we would want it to fail the probe ?

Thanks & Regards,
    - Nayna

>
> /Jarkko
>


------------------------------------------------------------------------------
The Command Line: Reinvented for Modern Developers
Did the resurgence of CLI tooling catch you by surprise?
Reconnect with the command line and become more productive. 
Learn the new .NET and ASP.NET CLI. Get your free copy!
http://sdm.link/telerik
Jarkko Sakkinen Nov. 4, 2016, 5:18 a.m. UTC | #13
On Tue, Oct 18, 2016 at 08:49:45PM -0400, Nayna Jain wrote:
> This patch removes the unnecessary error messages on failing to
> allocate memory and replaces pr_err/printk with dev_dbg/dev_info
> as applicable.
> 
> Suggested-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> Signed-off-by: Nayna Jain <nayna@linux.vnet.ibm.com>

Why subject line is "replace or remove"? What does that mean?
If you have both, maybe using the phrase "clean up" would be
better.

/Jarkko

> ---
>  drivers/char/tpm/tpm_acpi.c | 17 +++++------------
>  drivers/char/tpm/tpm_of.c   | 30 ++++++++++--------------------
>  2 files changed, 15 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm_acpi.c b/drivers/char/tpm/tpm_acpi.c
> index 859bdba..22e42da 100644
> --- a/drivers/char/tpm/tpm_acpi.c
> +++ b/drivers/char/tpm/tpm_acpi.c
> @@ -60,11 +60,8 @@ int read_log_acpi(struct tpm_chip *chip)
>  	status = acpi_get_table(ACPI_SIG_TCPA, 1,
>  				(struct acpi_table_header **)&buff);
>  
> -	if (ACPI_FAILURE(status)) {
> -		printk(KERN_ERR "%s: ERROR - Could not get TCPA table\n",
> -		       __func__);
> +	if (ACPI_FAILURE(status))
>  		return -EIO;
> -	}
>  
>  	switch(buff->platform_class) {
>  	case BIOS_SERVER:
> @@ -78,25 +75,21 @@ int read_log_acpi(struct tpm_chip *chip)
>  		break;
>  	}
>  	if (!len) {
> -		printk(KERN_ERR "%s: ERROR - TCPA log area empty\n", __func__);
> +		dev_dbg(&chip->dev, "%s: ERROR - TCPA log area empty\n",
> +			__func__);
>  		return -EIO;
>  	}
>  
>  	/* malloc EventLog space */
>  	log->bios_event_log = kmalloc(len, GFP_KERNEL);
> -	if (!log->bios_event_log) {
> -		printk("%s: ERROR - Not enough  Memory for BIOS measurements\n",
> -			__func__);
> +	if (!log->bios_event_log)
>  		return -ENOMEM;
> -	}
>  
>  	log->bios_event_log_end = log->bios_event_log + len;
>  
>  	virt = acpi_os_map_iomem(start, len);
> -	if (!virt) {
> -		printk("%s: ERROR - Unable to map memory\n", __func__);
> +	if (!virt)
>  		goto err;
> -	}
>  
>  	memcpy_fromio(log->bios_event_log, virt, len);
>  
> diff --git a/drivers/char/tpm/tpm_of.c b/drivers/char/tpm/tpm_of.c
> index 22b8f81..8b7e677 100644
> --- a/drivers/char/tpm/tpm_of.c
> +++ b/drivers/char/tpm/tpm_of.c
> @@ -31,40 +31,30 @@ int read_log_of(struct tpm_chip *chip)
>  	log = &chip->log;
>  	if (chip->dev.parent->of_node)
>  		np = chip->dev.parent->of_node;
> -	if (!np) {
> -		pr_err("%s: ERROR - IBMVTPM not supported\n", __func__);
> +	if (!np)
>  		return -ENODEV;
> -	}
>  
>  	sizep = of_get_property(np, "linux,sml-size", NULL);
> -	if (sizep == NULL) {
> -		pr_err("%s: ERROR - SML size not found\n", __func__);
> -		goto cleanup_eio;
> -	}
> +	if (sizep == NULL)
> +		return -EIO;
> +
>  	if (*sizep == 0) {
> -		pr_err("%s: ERROR - event log area empty\n", __func__);
> -		goto cleanup_eio;
> +		dev_info(&chip->dev, "%s: ERROR - event log area empty\n",
> +			__func__);
> +		return -EIO;
>  	}
>  
>  	basep = of_get_property(np, "linux,sml-base", NULL);
> -	if (basep == NULL) {
> -		pr_err("%s: ERROR - SML not found\n", __func__);
> -		goto cleanup_eio;
> -	}
> +	if (basep == NULL)
> +		return -EIO;
>  
>  	log->bios_event_log = kmalloc(*sizep, GFP_KERNEL);
> -	if (!log->bios_event_log) {
> -		pr_err("%s: ERROR - Not enough memory for BIOS measurements\n",
> -		       __func__);
> +	if (!log->bios_event_log)
>  		return -ENOMEM;
> -	}
>  
>  	log->bios_event_log_end = log->bios_event_log + *sizep;
>  
>  	memcpy(log->bios_event_log, __va(*basep), *sizep);
>  
>  	return 0;
> -
> -cleanup_eio:
> -	return -EIO;
>  }
> -- 
> 2.5.0
> 

------------------------------------------------------------------------------
Developer Access Program for Intel Xeon Phi Processors
Access to Intel Xeon Phi processor-based developer platforms.
With one year of Intel Parallel Studio XE.
Training and support from Colfax.
Order your platform today. http://sdm.link/xeonphi
diff mbox

Patch

diff --git a/drivers/char/tpm/tpm_acpi.c b/drivers/char/tpm/tpm_acpi.c
index 859bdba..22e42da 100644
--- a/drivers/char/tpm/tpm_acpi.c
+++ b/drivers/char/tpm/tpm_acpi.c
@@ -60,11 +60,8 @@  int read_log_acpi(struct tpm_chip *chip)
 	status = acpi_get_table(ACPI_SIG_TCPA, 1,
 				(struct acpi_table_header **)&buff);
 
-	if (ACPI_FAILURE(status)) {
-		printk(KERN_ERR "%s: ERROR - Could not get TCPA table\n",
-		       __func__);
+	if (ACPI_FAILURE(status))
 		return -EIO;
-	}
 
 	switch(buff->platform_class) {
 	case BIOS_SERVER:
@@ -78,25 +75,21 @@  int read_log_acpi(struct tpm_chip *chip)
 		break;
 	}
 	if (!len) {
-		printk(KERN_ERR "%s: ERROR - TCPA log area empty\n", __func__);
+		dev_dbg(&chip->dev, "%s: ERROR - TCPA log area empty\n",
+			__func__);
 		return -EIO;
 	}
 
 	/* malloc EventLog space */
 	log->bios_event_log = kmalloc(len, GFP_KERNEL);
-	if (!log->bios_event_log) {
-		printk("%s: ERROR - Not enough  Memory for BIOS measurements\n",
-			__func__);
+	if (!log->bios_event_log)
 		return -ENOMEM;
-	}
 
 	log->bios_event_log_end = log->bios_event_log + len;
 
 	virt = acpi_os_map_iomem(start, len);
-	if (!virt) {
-		printk("%s: ERROR - Unable to map memory\n", __func__);
+	if (!virt)
 		goto err;
-	}
 
 	memcpy_fromio(log->bios_event_log, virt, len);
 
diff --git a/drivers/char/tpm/tpm_of.c b/drivers/char/tpm/tpm_of.c
index 22b8f81..8b7e677 100644
--- a/drivers/char/tpm/tpm_of.c
+++ b/drivers/char/tpm/tpm_of.c
@@ -31,40 +31,30 @@  int read_log_of(struct tpm_chip *chip)
 	log = &chip->log;
 	if (chip->dev.parent->of_node)
 		np = chip->dev.parent->of_node;
-	if (!np) {
-		pr_err("%s: ERROR - IBMVTPM not supported\n", __func__);
+	if (!np)
 		return -ENODEV;
-	}
 
 	sizep = of_get_property(np, "linux,sml-size", NULL);
-	if (sizep == NULL) {
-		pr_err("%s: ERROR - SML size not found\n", __func__);
-		goto cleanup_eio;
-	}
+	if (sizep == NULL)
+		return -EIO;
+
 	if (*sizep == 0) {
-		pr_err("%s: ERROR - event log area empty\n", __func__);
-		goto cleanup_eio;
+		dev_info(&chip->dev, "%s: ERROR - event log area empty\n",
+			__func__);
+		return -EIO;
 	}
 
 	basep = of_get_property(np, "linux,sml-base", NULL);
-	if (basep == NULL) {
-		pr_err("%s: ERROR - SML not found\n", __func__);
-		goto cleanup_eio;
-	}
+	if (basep == NULL)
+		return -EIO;
 
 	log->bios_event_log = kmalloc(*sizep, GFP_KERNEL);
-	if (!log->bios_event_log) {
-		pr_err("%s: ERROR - Not enough memory for BIOS measurements\n",
-		       __func__);
+	if (!log->bios_event_log)
 		return -ENOMEM;
-	}
 
 	log->bios_event_log_end = log->bios_event_log + *sizep;
 
 	memcpy(log->bios_event_log, __va(*basep), *sizep);
 
 	return 0;
-
-cleanup_eio:
-	return -EIO;
 }