From: Andi Kleen <ak@suse.de>
To: "Zhang, Yanmin" <yanmin_zhang@linux.intel.com>
Cc: linux-pci maillist <linux-pci@atrey.karlin.mff.cuni.cz>,
Greg KH <greg@kroah.com>,
Tom Long Nguyen <tom.l.nguyen@intel.com>,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH 4/5] PCI-Express AER implemetation: AER core and aerdriver
Date: 12 Jul 2006 18:26:56 +0200 [thread overview]
Message-ID: <p73k66in60f.fsf@verdi.suse.de> (raw)
In-Reply-To: <1152691570.28493.250.camel@ymzhang-perf.sh.intel.com>
"Zhang, Yanmin" <yanmin_zhang@linux.intel.com> writes:
> With Arjan's comments, I changed EXPORT_SYMBOL to EXPORT_SYMBOL_GPL.
> Sorry for flooding your emailbox again. :)
This means that non GPL drivers will reimplement these functions
on their own (which is possible, just ugly) The fallout of them getting that wrong
might be significant.
I would change it back. _GPL should be only for core services, not
for generic driver interfaces.
> --- linux-2.6.17/drivers/pci/pcie/aer/aerdrv_core.c 1970-01-01 08:00:00.000000000 +0800
> +++ linux-2.6.17_aer/drivers/pci/pcie/aer/aerdrv_core.c 2006-07-12 15:47:38.000000000 +0800
> @@ -0,0 +1,737 @@
> +/*
> + * Copyright (C) 2006 Intel
> + * Tom Long Nguyen (tom.l.nguyen@intel.com)
> + * Zhang Yanmin (yanmin.zhang@intel.com)
Comment describing what the file does missing. At least one paragraph
of design rationale would be good
> +
> +config PCIEAER
> + tristate "Root Port Advanced Error Reporting support"
> + depends on PCIEPORTBUS
> + default y
> + help
> + This enables Root Port Advanced Error Reporting (AER) driver
> + support. Error reporting messages sent to Root Port will be
> + handled by PCI Express AER driver.
I hope it's clear from the context this is PCI-E specific?
> --- linux-2.6.17/drivers/pci/pcie/aer/Makefile 1970-01-01 08:00:00.000000000 +0800
> +++ linux-2.6.17_aer/drivers/pci/pcie/aer/Makefile 2006-06-22 16:46:29.000000000 +0800
> @@ -0,0 +1,10 @@
> +#
> +# Makefile for PCI-Express Root Port Advanced Error Reporting Driver
> +#
> +
> +obj-$(CONFIG_PCIEAER) += aerdriver.o
> +aerdrv_acpi-$(CONFIG_ACPI) += aerdrv_acpi.o
> +
> +aerdriver-objs := aerdrv_errprint.o aerdrv_core.o aerdrv.o
> +aerdriver-objs += $(aerdrv_acpi-y)
> +
> --- linux-2.6.17/drivers/pci/pcie/Kconfig 2006-06-22 16:26:43.000000000 +0800
> +++ linux-2.6.17_aer/drivers/pci/pcie/Kconfig 2006-06-22 16:46:29.000000000 +0800
> @@ -34,3 +34,4 @@ config HOTPLUG_PCI_PCIE_POLL_EVENT_MODE
>
> When in doubt, say N.
>
> +source "drivers/pci/pcie/aer/Kconfig"
> --- linux-2.6.17/drivers/pci/pcie/Makefile 2006-06-22 16:26:43.000000000 +0800
> +++ linux-2.6.17_aer/drivers/pci/pcie/Makefile 2006-06-22 16:46:29.000000000 +0800
> @@ -5,3 +5,6 @@
> pcieportdrv-y := portdrv_core.o portdrv_pci.o portdrv_bus.o
>
> obj-$(CONFIG_PCIEPORTBUS) += pcieportdrv.o
> +
> +# Build PCI Express AER if needed
> +obj-$(CONFIG_PCIEAER) += aer/
> --- linux-2.6.17/drivers/pci/pcie/aer/aerdrv_errprint.c 1970-01-01 08:00:00.000000000 +0800
> +++ linux-2.6.17_aer/drivers/pci/pcie/aer/aerdrv_errprint.c 2006-06-22 16:46:29.000000000 +0800
> @@ -0,0 +1,216 @@
> +/*
> + * Copyright (C) 2006 Intel
> + * Tom Long Nguyen (tom.l.nguyen@intel.com)
> + * Zhang Yanmin (yanmin.zhang@intel.com)
> + *
Comment what the code does missing.
At least one paragraph of design rationale would be good.
> + "Unknown Error Bit 22 ", /* Bit Position 22 */
> + "Unknown Error Bit 23 ", /* Bit Position 23 */
> + "Unknown Error Bit 24 ", /* Bit Position 24 */
> + "Unknown Error Bit 25 ", /* Bit Position 25 */
> + "Unknown Error Bit 26 ", /* Bit Position 26 */
> + "Unknown Error Bit 27 ", /* Bit Position 27 */
> + "Unknown Error Bit 28 ", /* Bit Position 28 */
> + "Unknown Error Bit 29 ", /* Bit Position 29 */
> + "Unknown Error Bit 30 ", /* Bit Position 30 */
> + "Unknown Error Bit 31 " /* Bit Position 31 */
Make all the unknown error bits a NULL and use a sprintf in the
decoder instead.
Similar for the following arrays.
> +void aer_print_error(struct pci_dev *dev, struct aer_err_info *info)
> +{
> + char * errmsg;
> + int err_layer, agent;
> +
> + printk(KERN_ERR "+------ PCI-Express Device Error ------+\n");
> + printk(KERN_ERR "Error Severity\t\t: %s\n",
> + aer_error_severity_string[info->severity]);
> +
> + if ( info->status == 0) {
> + printk(KERN_ERR "PCIE Bus Error type\t: (Unaccessible)\n");
KERN_ERR? THis means it will appear on consoles, won't it?
And surely not all these errors are fatal enough to need user attention
immediately and I bet there will be some devices who report these
errors unnecessarily. I would use a lower log level.
Also I would suggest you add something in the documentation
on what the messages mean exactly and how to decode them. I'm sure that will be a FAQ.
-Andi
next prev parent reply other threads:[~2006-07-12 16:27 UTC|newest]
Thread overview: 29+ messages / expand[flat|nested] mbox.gz Atom feed top
2006-07-12 7:10 [PATCH 1/5] PCI-Express AER implemetation: aer howto document Zhang, Yanmin
2006-07-12 7:16 ` [PATCH 2/5] PCI-Express AER implemetation: Add new defines to pci_regs.h Zhang, Yanmin
2006-07-12 7:22 ` [PATCH 3/5] PCI-Express AER implemetation: export pcie_port_bus_type Zhang, Yanmin
2006-07-12 7:32 ` [PATCH 4/5] PCI-Express AER implemetation: AER core and aerdriver Zhang, Yanmin
2006-07-12 7:38 ` [PATCH 5/5] PCI-Express AER implemetation: pcie_portdrv error handler Zhang, Yanmin
2006-07-12 8:06 ` [PATCH 4/5] PCI-Express AER implemetation: AER core and aerdriver Zhang, Yanmin
2006-07-12 13:16 ` Arjan van de Ven
2006-07-13 2:08 ` Zhang, Yanmin
2006-07-12 16:26 ` Andi Kleen [this message]
2006-07-13 2:16 ` Zhang, Yanmin
2006-07-12 8:00 ` [PATCH 3/5] PCI-Express AER implemetation: export pcie_port_bus_type Zhang, Yanmin
2006-07-14 5:25 ` [PATCH 1/5] PCI-Express AER implemetation: aer howto document Zhang, Yanmin
2006-07-14 5:27 ` [PATCH 2/5] PCI-Express AER implemetation: Add new defines to pci_regs.h Zhang, Yanmin
2006-07-14 5:28 ` [PATCH 3/5] PCI-Express AER implemetation: export pcie_port_bus_type Zhang, Yanmin
2006-07-14 5:30 ` [PATCH 4/5] PCI-Express AER implemetation: AER core and aerdriver Zhang, Yanmin
2006-07-14 5:32 ` [PATCH 4/5] PCI-Express AER implemetation: pcie_portdrv error handler Zhang, Yanmin
2006-07-14 5:35 ` [PATCH 5/5] " Zhang, Yanmin
2006-07-24 19:37 ` Linas Vepstas
2006-07-26 4:56 ` Zhang, Yanmin
2006-07-14 12:40 ` [PATCH 1/5] PCI-Express AER implemetation: aer howto document Andi Kleen
2006-07-17 1:24 ` Zhang, Yanmin
2006-07-24 20:48 ` Linas Vepstas
2006-07-26 5:48 ` Zhang, Yanmin
2006-07-12 17:21 [PATCH 4/5] PCI-Express AER implemetation: AER core and aerdriver Johnny Lever
2006-07-12 17:32 ` Arjan van de Ven
2006-07-12 18:07 ` Johnny Lever
2006-07-13 8:45 ` Christoph Hellwig
2006-07-13 11:26 ` Johnny Lever
2006-07-31 3:00 [PATCH 1/5] PCI-Express AER implemetation: aer howto document Zhang, Yanmin
2006-07-31 3:10 ` [PATCH 2/5] PCI-Express AER implemetation: Add new defines to pci_regs.h Zhang, Yanmin
2006-07-31 3:14 ` [PATCH 3/5] PCI-Express AER implemetation: export pcie_port_bus_type Zhang, Yanmin
2006-07-31 3:22 ` [PATCH 4/5] PCI-Express AER implemetation: AER core and aerdriver Zhang, Yanmin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=p73k66in60f.fsf@verdi.suse.de \
--to=ak@suse.de \
--cc=greg@kroah.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pci@atrey.karlin.mff.cuni.cz \
--cc=tom.l.nguyen@intel.com \
--cc=yanmin_zhang@linux.intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).