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

[Borislav Petkov <bp@suse.de>]

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