Re: [PATCH] PCI: aer_inject: Log actual error causes

[Jean Delvare <jdelvare@suse.de>]

Hi Borislav,

Thanks for the quick review.

Le Tuesday 26 January 2016 à 11:12 +0100, Borislav Petkov a écrit :
> On Tue, Jan 26, 2016 at 09:52:05AM +0100, Jean Delvare (by way of Jean Delvare <jdelvare@suse.de>) wrote:
> > The aer_inject driver is very quiet. In most cases, it merely returns
> > an error code to user-space, leaving the user with little clue about
> > the actual reason for the failure.
> > 
> > So, log error messages for 4 of the most frequent causes of failure:
> > * Can't find the root port of the specified device.
> > * Device doesn't support AER.
> > * Root port doesn't support AER.
> > * AER device not found.
> > This gives the user a chance to understand why aer-inject failed.
> > 
> > Based on a preliminary patch by Thomas Renninger.
> > 
> > Signed-off-by: Jean Delvare <jdelvare@suse.de>
> > Cc: Thomas Renninger <trenn@suse.de>
> > Cc: Bjorn Helgaas <bhelgaas@google.com>
> > ---
> >  drivers/pci/pcie/aer/aer_inject.c |    8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> > 
> > --- linux-4.5-rc0.orig/drivers/pci/pcie/aer/aer_inject.c	2016-01-20 09:25:54.815852332 +0100
> > +++ linux-4.5-rc0/drivers/pci/pcie/aer/aer_inject.c	2016-01-26 09:41:17.361994839 +0100
> > @@ -334,12 +334,14 @@ static int aer_inject(struct aer_error_i
> >  		return -ENODEV;
> >  	rpdev = pcie_find_root_port(dev);
> >  	if (!rpdev) {
> > +		dev_err(&dev->dev, "aer_inject: Root port not found\n");
> >  		ret = -ENODEV;
> >  		goto out_put;
> >  	}
> >  
> >  	pos_cap_err = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ERR);
> >  	if (!pos_cap_err) {
> > +		dev_err(&dev->dev, "aer_inject: Device doesn't support AER\n");
> >  		ret = -EPERM;
> 
> Btw, this -EPERM looks wrong - if we're checking for capabilities, we
> shouldn't be returning -EPERM but maybe something like -ENODEV or so.

I agree. It was originally -ENOTTY, changed to -EPERM by:

commit e82b14bdd390c534750a191f9936f842bab255d4
Author: Prarit Bhargava <prarit@redhat.com>
Date:   Wed Mar 20 12:04:43 2013 +0000

But I'd say -EPERM is hardly better. The problem with -ENODEV is that it
is already returned by this function for several other error causes.
Also the aer-inject user-space tool will print the error message from
the error code, and I don't think "No such device" is helpful in that
case. What about -ENOTSUPP ("Operation not supported") or
-EEPROTONOSUPPORT ("Protocol not supported")?

I can change it if nobody objects. I think the change can be included in
this patch as it is quite related.

> >  		goto out_put;
> >  	}
> > @@ -350,6 +352,8 @@ static int aer_inject(struct aer_error_i
> >  
> >  	rp_pos_cap_err = pci_find_ext_capability(rpdev, PCI_EXT_CAP_ID_ERR);
> >  	if (!rp_pos_cap_err) {
> > +		dev_err(&rpdev->dev,
> > +			"aer_inject: Root port doesn't support AER\n");
> >  		ret = -EPERM;
> 
> Ditto.
> 
> >  		goto out_put;
> >  	}
> > @@ -462,8 +466,10 @@ static int aer_inject(struct aer_error_i
> >  			goto out_put;
> >  		}
> >  		aer_irq(-1, edev);
> > -	} else
> > +	} else {
> > +		dev_err(&rpdev->dev, "aer_inject: AER device not found\n");
> 
> So other error prints in that function do printk(KERN_WARNING. Why
> dev_err()?

I'd rather ask, why printk? ;-) Using raw printk is considered bad and
should be avoided whenever possible. So says checkpatch.pl. If anything,
all these printks should be converted to at least pr_* and ideally
dev_*. But that would be a separate patch.

> Why not pr_err() and define pr_fmt to "aer_inject: " and then drop
> that prefix from the messages?

Because I believe that including the device name in the error messages
makes them more helpful to understand and diagnose the problem. If the
device where we try to inject the error has a problem, it's PCI name
will be included in the error message. If the error is with the root
port, then we include the root port's PCI name. If I used pr_err()
instead then the device information would be missing.

-- 
Jean Delvare
SUSE L3 Support