diff mbox

[v7,3/3] aerdrv: Cleanup log output for CPER based AER

Message ID 20121204210318.4515.10180.stgit@grignak.americas.hpqcorp.net (mailing list archive)
State New, archived
Delegated to: Bjorn Helgaas
Headers show

Commit Message

Lance Ortiz Dec. 4, 2012, 9:03 p.m. UTC 
These changes make cper_print_aer more consistent with aer_print_error
which is called in the AER interrupt case. The string in the variable
'prefix' is printed at the beginning of each print statement in
cper_print_aer(). The prefix is a string containing the driver name
and the device's slot location.  From the callers the value of prefix
is never assigned and is NULL, so when cper_print_aer prints data the
initial string does not get printed.  This string is important because
it identifies the device that the error is on.

v1-v2 fix some compile errors withinn the #ifdef
v3-v4 remove agent id stuff and kept print the same to avoid
compatibility issues

Signed-off-by: Lance Ortiz <lance.ortiz@hp.com>
---

 drivers/pci/pcie/aer/aerdrv_errprint.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)


--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Mauro Carvalho Chehab Dec. 5, 2012, 2:41 p.m. UTC | #1 
Em Tue, 04 Dec 2012 14:03:18 -0700
Lance Ortiz <lance.ortiz@hp.com> escreveu:

Hmm... I did a reply to v6 of this patch, but i pressed the wrong button
and it went only to Joe. Excuse-me for that. Let me resend the
comments to everybody:

On Tue, 2012-12-04 at 16:33 -0200, Mauro Carvalho Chehab wrote:
> Em Tue, 04 Dec 2012 09:39:23 -0800
> Joe Perches <joe@perches.com> escreveu:
>   
> > On Tue, 2012-12-04 at 10:04 -0700, Lance Ortiz wrote:  
> > > These changes make cper_print_aer more consistent with aer_print_error
> > > which is called in the AER interrupt case. The string in the variable
> > > 'prefix' is printed at the beginning of each print statement in
> > > cper_print_aer(). The prefix is a string containing the driver name
> > > and the device's slot location.  From the callers the value of prefix
> > > is never assigned and is NULL, so when cper_print_aer prints data the
> > > initial string does not get printed.  This string is important because
> > > it identifies the device that the error is on.  
> > 
> > Hi again Lance.
> >   
> > > diff --git a/drivers/pci/pcie/aer/aerdrv_errprint.c b/drivers/pci/pcie/aer/aerdrv_errprint.c  
> > []  
> > > @@ -228,9 +228,14 @@ void cper_print_aer(struct pci_dev *dev, int cper_severity,
> > >  	int aer_severity, layer, agent, status_strs_size, tlp_header_valid = 0;
> > >  	u32 status, mask;
> > >  	const char **status_strs;
> > > -	char *prefix = NULL;
> > > +	char prefix[44];
> > >  
> > >  	aer_severity = cper_severity_to_aer(cper_severity);
> > > +	snprintf(prefix, sizeof(prefix), "%s%s %s: ",
> > > +		(aer_severity == AER_CORRECTABLE) ?
> > > +		KERN_WARNING : KERN_ERR,
> > > +		dev_driver_string(&dev->dev), dev_name(&dev->dev));
> > > +
> > >  	if (aer_severity == AER_CORRECTABLE) {
> > >  		status = aer->cor_status;
> > >  		mask = aer->cor_mask;  
> > 
> > Perhaps this would be better just using dev_printk
> > instead of snprintf into a prefix with printk to
> > emulate dev_printk.
> > 
> > Also, perhaps KERN_NOTICE is preferable to KERN_WARNING
> > in the CORRECTABLE case.  
> 
> Hmm... IMHO, it should be KERN_ERR, even for a correctable one ;) 
> 
> This is an error and probably means some hardware problem or a bad 
> contact. In any case, a preventive action is likely required.
>   
> > 
> > Maybe something like:
> > 
> > 	const char *level = KERN_ERR;
> > 	if (aer_severity == AER_CORRECTABLE)
> > 		level = KERN_NOTICE;
> > 
> > 	...
> > 
> > 	dev_printk(level, &dev->dev, etc...);
> > 
> > Maybe do this after this series of patches is accepted.
> > Enough with the revisions for awhile...
> >   
> 
>   

Regards,
Mauro
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Borislav Petkov Dec. 5, 2012, 2:50 p.m. UTC | #2 
On Tue, Dec 04, 2012 at 02:03:18PM -0700, Lance Ortiz wrote:
> These changes make cper_print_aer more consistent with aer_print_error
> which is called in the AER interrupt case. The string in the variable
> 'prefix' is printed at the beginning of each print statement in
> cper_print_aer(). The prefix is a string containing the driver name
> and the device's slot location.  From the callers the value of prefix
> is never assigned and is NULL, so when cper_print_aer prints data the
> initial string does not get printed.  This string is important because
> it identifies the device that the error is on.
> 
> v1-v2 fix some compile errors withinn the #ifdef
> v3-v4 remove agent id stuff and kept print the same to avoid
> compatibility issues
> 
> Signed-off-by: Lance Ortiz <lance.ortiz@hp.com>
> ---
> 
>  drivers/pci/pcie/aer/aerdrv_errprint.c |    7 ++++++-
>  1 files changed, 6 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/pci/pcie/aer/aerdrv_errprint.c b/drivers/pci/pcie/aer/aerdrv_errprint.c
> index 34d96e4..58ff4c0 100644
> --- a/drivers/pci/pcie/aer/aerdrv_errprint.c
> +++ b/drivers/pci/pcie/aer/aerdrv_errprint.c
> @@ -228,9 +228,14 @@ void cper_print_aer(struct pci_dev *dev, int cper_severity,
>  	int aer_severity, layer, agent, status_strs_size, tlp_header_valid = 0;
>  	u32 status, mask;
>  	const char **status_strs;
> -	char *prefix = NULL;
> +	char prefix[44];
>  
>  	aer_severity = cper_severity_to_aer(cper_severity);
> +	snprintf(prefix, sizeof(prefix), "%s%s %s: ",
> +		(aer_severity == AER_CORRECTABLE) ?
> +		KERN_WARNING : KERN_ERR,
> +		dev_driver_string(&dev->dev), dev_name(&dev->dev));

Here it is, you're initializing 'prefix' here although it is being
used in the previous patch. You should concentrate the whole prefix
initialization and passing in one patch so that there are no breakages.

Also, why is it exactly 44 chars long, is that some magic number
that always works? At least this is what aer_print_error does
too. I'm guessing someone chose it because it is large enough for
dev_driver_string() and dev_name() and a couple more formatting
characters. Oh well.

Thanks.
Ortiz, Lance E Dec. 5, 2012, 4:16 p.m. UTC | #3 
PiANCj4gSGVyZSBpdCBpcywgeW91J3JlIGluaXRpYWxpemluZyAncHJlZml4JyBoZXJlIGFsdGhv
dWdoIGl0IGlzIGJlaW5nDQo+IHVzZWQgaW4gdGhlIHByZXZpb3VzIHBhdGNoLiBZb3Ugc2hvdWxk
IGNvbmNlbnRyYXRlIHRoZSB3aG9sZSBwcmVmaXgNCj4gaW5pdGlhbGl6YXRpb24gYW5kIHBhc3Np
bmcgaW4gb25lIHBhdGNoIHNvIHRoYXQgdGhlcmUgYXJlIG5vIGJyZWFrYWdlcy4NCg0KWWVzIEkg
cHJvYmFibHkgY291bGQgaGF2ZSBicm9rZW4gdXAgdGhlIHBhdGNoZXMgaW4gYSBjbGVhbmVyIHdh
eS4gIFRoYW5rcy4NCg0KPiANCj4gQWxzbywgd2h5IGlzIGl0IGV4YWN0bHkgNDQgY2hhcnMgbG9u
ZywgaXMgdGhhdCBzb21lIG1hZ2ljIG51bWJlcg0KPiB0aGF0IGFsd2F5cyB3b3Jrcz8gQXQgbGVh
c3QgdGhpcyBpcyB3aGF0IGFlcl9wcmludF9lcnJvciBkb2VzDQo+IHRvby4gSSdtIGd1ZXNzaW5n
IHNvbWVvbmUgY2hvc2UgaXQgYmVjYXVzZSBpdCBpcyBsYXJnZSBlbm91Z2ggZm9yDQo+IGRldl9k
cml2ZXJfc3RyaW5nKCkgYW5kIGRldl9uYW1lKCkgYW5kIGEgY291cGxlIG1vcmUgZm9ybWF0dGlu
Zw0KPiBjaGFyYWN0ZXJzLiBPaCB3ZWxsLg0KPiANCg0KWWVzLCBJIHdhcyB1c2luZyB0aGUgc2Ft
ZSBjb2RlIGFzIGluIGFlcl9wcmludF9lcnJvci4gIFRoaXMgd2FzIGJyb3VnaHQgdXAgaW4gYSBw
cmV2aW91cyBtYWlsIHdpdGggSm9lIGFuZCB3ZSBkaXNjdXNzZWQgY2xlYW5pbmcgYWxsIG9mIHRo
aXMgdXAgaW4gYSBsYXRlciBwYXRjaC4NCg0KTGFuY2UNCg0K
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/pci/pcie/aer/aerdrv_errprint.c b/drivers/pci/pcie/aer/aerdrv_errprint.c
index 34d96e4..58ff4c0 100644
--- a/drivers/pci/pcie/aer/aerdrv_errprint.c
+++ b/drivers/pci/pcie/aer/aerdrv_errprint.c
@@ -228,9 +228,14 @@  void cper_print_aer(struct pci_dev *dev, int cper_severity,
 	int aer_severity, layer, agent, status_strs_size, tlp_header_valid = 0;
 	u32 status, mask;
 	const char **status_strs;
-	char *prefix = NULL;
+	char prefix[44];
 
 	aer_severity = cper_severity_to_aer(cper_severity);
+	snprintf(prefix, sizeof(prefix), "%s%s %s: ",
+		(aer_severity == AER_CORRECTABLE) ?
+		KERN_WARNING : KERN_ERR,
+		dev_driver_string(&dev->dev), dev_name(&dev->dev));
+
 	if (aer_severity == AER_CORRECTABLE) {
 		status = aer->cor_status;
 		mask = aer->cor_mask;