[PATCH] PCI: aer_inject: Log actual error causes

[PATCH] PCI: aer_inject: Log actual error causes

[Jean Delvare]

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;
 		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;
 		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");
 		ret = -EINVAL;
+	}
 out_put:
 	kfree(err_alloc);
 	kfree(rperr_alloc);

-- 
Jean Delvare
SUSE L3 Support

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

[Borislav Petkov]

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.

>  		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()?

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

Thanks.

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
-- 

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

[Jean Delvare]

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

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

[Borislav Petkov]

On Tue, Jan 26, 2016 at 01:27:18PM +0100, Jean Delvare wrote:
> 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")?

Makes sense.

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

I'd do a separate patch but this is only my opinion. I guess that's
Bjorn's call.

> I'd rather ask, why printk? ;-) Using raw printk is considered bad and
> should be avoided whenever possible.

Hmm, interesting. Why?

> So says checkpatch.pl.

Please don't tell me you believe what checkpatch says.

> > 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.

True, that's a good argument.

However, if you're doing aer injection, you already *know* the device
you're injecting too. Unless you want to inject in multiple devices and
then it is helpful.

So sure, dev_* sounds better as it gives more info about which device
fails, but then please convert the whole driver.

Thanks.

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
-- 

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

[Jean Delvare]

Le Tuesday 26 January 2016 à 13:49 +0100, Borislav Petkov a écrit :
> On Tue, Jan 26, 2016 at 01:27:18PM +0100, Jean Delvare wrote:
> > 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")?
> 
> Makes sense.
> 
> > I can change it if nobody objects. I think the change can be included in
> > this patch as it is quite related.
> 
> I'd do a separate patch but this is only my opinion. I guess that's
> Bjorn's call.

I am almost always advocating for separate patches, but here it seemed
like hairsplitting so I wasn't sure. I'm fine both ways really.

> > I'd rather ask, why printk? ;-) Using raw printk is considered bad and
> > should be avoided whenever possible.
> 
> Hmm, interesting. Why?

I guess the idea is that it makes message formats more consistent and
valuable.

> > So says checkpatch.pl.
> 
> Please don't tell me you believe what checkpatch says.

Of course I believe it, as long as it says what I want to hear. If not
then I just claim it's a piece of crap and ignore it ;-) As everybody
does, it seems.

> > > 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.
> 
> True, that's a good argument.
> 
> However, if you're doing aer injection, you already *know* the device
> you're injecting too. Unless you want to inject in multiple devices and
> then it is helpful.

You know the device, but you don't know its root device, which
apparently matters a lot for AER, and is used for 2 of the messages I
introduced.

Even for the device itself, a confirmation of the PCI device name is
always good to have, to avoid confusion if you made a typo in your
injection data for example.

> So sure, dev_* sounds better as it gives more info about which device
> fails, but then please convert the whole driver.

OK, I'll work on this once the first round of reviews if done. I don't
know if others have more comments, so let's wait a bit.

-- 
Jean Delvare
SUSE L3 Support

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

[Bjorn Helgaas]

On Tue, Jan 26, 2016 at 02:05:54PM +0100, Jean Delvare wrote:
> Le Tuesday 26 January 2016 à 13:49 +0100, Borislav Petkov a écrit :
> > On Tue, Jan 26, 2016 at 01:27:18PM +0100, Jean Delvare wrote:
> > > 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")?
> > 
> > Makes sense.
> > 
> > > I can change it if nobody objects. I think the change can be included in
> > > this patch as it is quite related.
> > 
> > I'd do a separate patch but this is only my opinion. I guess that's
> > Bjorn's call.
> 
> I am almost always advocating for separate patches, but here it seemed
> like hairsplitting so I wasn't sure. I'm fine both ways really.

I'd prefer one patch to change the errno (only) and another to add the
printk logging.

I definitely prefer dev_* whenever possible.  The aer_inject user
knows the relevant device at the time, but dev_* makes the dmesg log
more useful later.

In fact, your patch only adds logging to some error paths.  I'd like
to have some indication in dmesg that aer_inject was used at all.
Maybe even a synopsis of the injected error, though maybe that's too
much if aer_inject is used in an automated way.  It would just be nice
to have a dmesg indication that subsequent AER events *might* be
injected rather than real errors.

Bjorn