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

[Jean Delvare <jdelvare@suse.de>]

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