Page MenuHomeFreeBSD
Differential D7697 Diff 19798 sys/dev/pci/pci_pci.c

Changeset View

Standalone View

View Options

sys/dev/pci/pci_pci.c

Context not available.
#include <sys/rman.h> #include <sys/rman.h>
#include <sys/sysctl.h> #include <sys/sysctl.h>
#include <sys/systm.h> #include <sys/systm.h>
#include <sys/queue.h>
#include <sys/mutex.h>
#include <sys/lock.h>
#include <machine/pci_cfgreg.h>
#include <dev/pci/pcivar.h> #include <dev/pci/pcivar.h>
#include <dev/pci/pcireg.h> #include <dev/pci/pcireg.h>
Context not available.
DEVMETHOD_END DEVMETHOD_END
}; };
static MALLOC_DEFINE(M_AER, "AER", "Advanced Error Report");
static STAILQ_HEAD(, AER_node) AER_root;
static int AER_counter = 0;
static struct mtx AER_lock;
static devclass_t pcib_devclass; static devclass_t pcib_devclass;
DEFINE_CLASS_0(pcib, pcib_driver, pcib_methods, sizeof(struct pcib_softc)); DEFINE_CLASS_0(pcib, pcib_driver, pcib_methods, sizeof(struct pcib_softc));
Context not available.
return(ENXIO); return(ENXIO);
} }
/*
* sysctl handler: reads error records.
*/
static int
sysctl_pcib_error_records(SYSCTL_HANDLER_ARGS)
{
int *name;
int report_idx;
u_int namelen;
struct AER_error_data *data;
struct AER_node *node;
name = (int*)arg1;
report_idx = name[0];
namelen = arg2;
data = NULL;
/* Input index cannot exceed #error actually recv */
if (namelen != 1)
return (EINVAL);
mtx_lock_spin(&AER_lock);
if (report_idx < 0 || report_idx >= AER_counter ||
report_idx < (AER_counter - AER_REC_MAXLEN)) {
mtx_unlock_spin(&AER_lock);
return (EINVAL);
}
/* Loop to index and sysctl */
STAILQ_FOREACH(node, &AER_root, next) {
if (node->index == report_idx) {
data = node->AER_error_data_container;
break;
}
}
mtx_unlock_spin(&AER_lock);
return (SYSCTL_OUT(req, data, sizeof(*data)));
}
/*
* sysctl handler: An optional feature to clear the error records.
* using sysctl call to hw.pci.pcib_clear_error_records
* with parameter name[]
*
*/
/*
static int
sysctl_pcib_clear_error_records(SYSCTL_HANDLER_ARGS)
{
int *name;
int clear;
name = (int *)arg1;
clear = name[0];
if (clear == 1) {
impUnsubmitted
Not Done
Inline Actions

How do I atomically read and clear the records?

imp: How do I atomically read and clear the records?
mtx_lock_spin(&AER_lock);
STAILQ_INIT(&AER_root);
AER_counter = 0;
mtx_unlock_spin(&AER_lock);
}
else
return (EINVAL);
return 0;
}
SYSCTL_NODE(_hw_pci, OID_AUTO, pcib_clear_error_records, CTLFLAG_RD, sysctl_pcib_clear_error_records,
"PCI Bridge Clear Error records");
*/
/*
* sysctl handler: read pci config registers For error injection purpose.
*/
static int
sysctl_pcib_probe(SYSCTL_HANDLER_ARGS)
{
int *name, bus, slot, func, reg, bytes, result;
u_int namelen;
namelen = arg2;
if (namelen != 5)
return (EINVAL);
name = (int *)arg1;
bus = name[0];
slot = name[1];
func = name[2];
reg = name[3];
bytes = name[4];
result = pci_cfgregread(bus, slot, func, reg, bytes);
return (SYSCTL_OUT(req, &result, sizeof(int)));
}
/*
* Setup sysctl on hw.pci
*/
SYSCTL_INT(_hw_pci, OID_AUTO, pcib_error_count, CTLFLAG_RD, &AER_counter, 0,
"PCI Bridge Intr count");
impUnsubmitted
Not Done
Inline Actions

I don't see where this is set.
Also, it should be per-bridge maybe?
We'd like to get counts on a per-card basis, and I don't see that happening here...

imp: I don't see where this is set. Also, it should be per-bridge maybe? We'd like to get counts on…
SYSCTL_NODE(_hw_pci, OID_AUTO, pcib_error_records, CTLFLAG_RD, sysctl_pcib_error_records,
"PCI Bridge Error records");
SYSCTL_NODE(_hw_pci, OID_AUTO, pcib_probe, CTLFLAG_RD, sysctl_pcib_probe,
"PCI Bridge Probing for Error Injection");
/*
* Interrupt handler
*/
static void
pcib_intr_handler(void *arg)
{
struct pcib_softc *sc;
struct AER_node *current;
struct AER_error_data *error_data;
device_t dev;
int offsAER;
int offsPCIE;
int temp;
int error;
int current_AER_counter;
sc = arg;
dev = sc->dev;
device_printf(dev, ": New Interrupt Received\n");
mtx_lock_spin(&AER_lock);
AER_counter += 1;
current_AER_counter = AER_counter;
mtx_unlock_spin(&AER_lock);
if ((current_AER_counter > AER_REC_MAXLEN) && !STAILQ_EMPTY(&AER_root)) {
STAILQ_REMOVE_HEAD(&AER_root, next);
printf("PCI-E Bridge AER Record list full, remove oldest record.\n");
impUnsubmitted
Not Done
Inline Actions

I'd be tempted to rate limit this message.
Also, it dropped the oldest record, so maybe that's a better message?
And aren't we leaking the report by not freeing 'next' here?

imp: I'd be tempted to rate limit this message. Also, it dropped the oldest record, so maybe that's…
}
/* Record Data */
error_data = malloc(sizeof(*error_data), M_AER, M_WAITOK | M_ZERO);
impUnsubmitted
Not Done
Inline Actions

WAITOK seems unwise here.

imp: WAITOK seems unwise here.
pci_find_cap(dev, PCIY_EXPRESS, &offsPCIE);
error = pci_find_extcap(dev, PCIZ_AER, &offsAER);
/* PCI-E error status */
temp = pci_read_config(dev, offsPCIE + PCIER_DEVICE_STA, 2);
error_data->pcie_cap_status = temp;
if (temp >= 0)
pci_write_config(dev, offsPCIE + PCIER_DEVICE_STA, BCLR_PCIE_STATUS, 1);
impUnsubmitted
Not Done
Inline Actions

We should only clear the bits that we're interested in. temp, or masked temp. Not all of them. There's a race here if we use a fixed mask.

imp: We should only clear the bits that we're interested in. temp, or masked temp. Not all of them.
if (error) {
device_printf(dev, ": AER not supported\n");
impUnsubmitted
Not Done
Inline Actions

do not print this in the interrupt handler.
Just exit. That would also simplify the code.

imp: do not print this in the interrupt handler. Just exit. That would also simplify the code.
} else {
/* Record source tag */
error_data->error_number = current_AER_counter;
error_data->dev_name = device_get_name(dev);
error_data->unit = device_get_unit(dev);
/* Uncorrectable error reg */
temp = pci_read_config(dev, offsAER+PCIR_AER_UC_STATUS, 4);
impUnsubmitted
Not Done
Inline Actions

This code only reads one record. There may be more than one.

imp: This code only reads one record. There may be more than one.
error_data->unc_err_status = temp;
if (temp >= 0)
pci_write_config(dev, offsAER + PCIR_AER_UC_STATUS, BCLR_AER_UNC_STATUS, 4);
impUnsubmitted
Not Done
Inline Actions

And this is why. We should clear things one bit at a time to support devices that keep a stack of errors rather than just the last one. We also ignore the pointer to the last error which is present even in devices that don't support multiple error reports.

imp: And this is why. We should clear things one bit at a time to support devices that keep a stack…
/* Uncorrectable error severity */
error_data->unc_err_seve = pci_read_config(dev, offsAER + PCIR_AER_UC_SEVERITY, 4);
/* Correctable error reg */
temp = pci_read_config(dev, offsAER+PCIR_AER_COR_STATUS, 4);
error_data->c_err_status = temp;
if (temp >= 0)
pci_write_config(dev, offsAER + PCIR_AER_COR_STATUS, BCLR_AER_C_STATUS, 4);
impUnsubmitted
Not Done
Inline Actions

Same criticism.

imp: Same criticism.
/* ECRC */
error_data->AER_ECRC = pci_read_config(dev, offsAER + PCIR_AER_CAP_CONTROL, 4);
/* Root Port error status */
temp = pci_read_config(dev, offsAER + PCIR_AER_ROOTERR_STATUS, 4);
error_data->root_err_status = temp;
if (temp >= 0)
pci_write_config(dev, offsAER + PCIR_AER_ROOTERR_STATUS, BCLR_AER_RTSTATUS, 4);
/* ERROR Source */
error_data->err_src_ID = pci_read_config(dev, offsAER+PCIR_AER_ERR_SOURCE_ID, 2);
error_data->c_err_src_ID = pci_read_config(dev, offsAER+PCIR_AER_COR_SOURCE_ID, 2);
impUnsubmitted
Not Done
Inline Actions

All of these are only valid for Root Complex or Root Event Collector devices. They will be garbage values when not present (on my system they are all ff's).

imp: All of these are only valid for Root Complex or Root Event Collector devices. They will be…
}
/* Save Node */
current = malloc(sizeof(*current), M_AER, M_WAITOK | M_ZERO);
impUnsubmitted
Not Done
Inline Actions

WAITOK in an interrupt handler? That strikes me as unwise. Plus we're limiting it to a certain number: seems like it would be better to preallocate.

imp: WAITOK in an interrupt handler? That strikes me as unwise. Plus we're limiting it to a certain…
current->index = current_AER_counter-1;
current->AER_error_data_container = error_data;
mtx_lock_spin(&AER_lock);
STAILQ_INSERT_TAIL(&AER_root, current, next);
mtx_unlock_spin(&AER_lock);
}
/*
* Setup Interrupt & Reg interrupt handler
*/
static void
pcib_setup_intr_common(struct pcib_softc *sc)
{
int error;
int offsAER;
int offsPCIE;
int dev_ctl;
int intr_count;
device_t dev = sc->dev;
pci_find_cap(dev, PCIY_EXPRESS, &offsPCIE);
error = pci_find_extcap(dev, PCIZ_AER, &offsAER);
/* Enable device error report */
if (error != 0) {
device_printf(dev, "Interrupt not registered: AER not supported\n");
return;
}
pci_write_config(dev, offsAER + PCIR_AER_ROOTERR_CMD, 7, 4);
impUnsubmitted
Not Done
Inline Actions

This only works for Root Complex or Event Collecting device types. Yet we do this for all devices. No interrupts will issue on the other bridges.

imp: This only works for Root Complex or Event Collecting device types. Yet we do this for all…
dev_ctl = pci_read_config(dev, offsPCIE + PCIER_DEVICE_CTL, 1);
pci_write_config(dev, offsPCIE + PCIER_DEVICE_CTL, (dev_ctl | PCIEM_CTL_COR_ENABLE |
PCIEM_CTL_NFER_ENABLE | PCIEM_CTL_FER_ENABLE | PCIEM_CTL_URR_ENABLE), 1);
device_printf(dev,"Error Report Enabled\n");
/* Setup Interrupt & resource allocation */
sc->sc_irq_rid = -1;
intr_count = 1;
if (pci_msix_count(dev) == 1) {
error = pci_alloc_msix(dev, &intr_count);
if (!error)
sc->sc_irq_rid = 1;
} else if (pci_msi_count(dev) > 0) {
error = pci_alloc_msi(dev, &intr_count);
if (!error)
sc->sc_irq_rid = 1;
}
if (sc->sc_irq_rid < 0)
sc->sc_irq_rid = 0;
sc-> sc_irq_res = bus_alloc_resource_any(dev, SYS_RES_IRQ,
&sc->sc_irq_rid, RF_ACTIVE);
if (!sc->sc_irq_res) {
device_printf(dev, "Unable to allocate interrupt resource\n");
if (sc->sc_irq_rid == 1)
pci_release_msi(dev);
return;
}
/* Reg intr handler */
error = bus_setup_intr(dev, sc->sc_irq_res, INTR_TYPE_MISC,
NULL, pcib_intr_handler, sc, &sc->sc_irq_cookie);
if (error) {
device_printf(dev, "Unable to register interrupt handler\n");
bus_release_resource(dev, SYS_RES_IRQ, sc->sc_irq_rid, sc-> sc_irq_res);
if (sc->sc_irq_rid == 1)
pci_release_msi(dev);
return;
}
impUnsubmitted
Not Done
Inline Actions

All this code now conflicts with hot plug code. The hot plug code establishes the same interrupt. There's a register to find out which interrupt MSI to use for Root Complex or Root Event Collector devices that we're ignoring here. To the extent that this code works, it may be happy accident.

imp: All this code now conflicts with hot plug code. The hot plug code establishes the same…
device_printf(dev, "Interrupt registered\n");
STAILQ_INIT(&AER_root);
mtx_init(&AER_lock, "AER Record", NULL, MTX_SPIN);
}
/*
* Remove Interrupt & release interrupt handler &
* Add interrupt release to bus_generic_detach
*
* Leave it commented since have not find a way to test it.
*/
/*
static int
pcib_release_intr_common(struct pcib_softc *sc)
{
device_t dev;
int error;
dev = sc->dev;
error = bus_teardown_intr(dev, sc->sc_irq_res, sc->sc_irq_cookie);
if (error) {
device_printf(dev, "Unable to teardown interrupt\n");
return error;
}
error = bus_free_resource(dev, SYS_RES_IRQ, sc->sc_irq_res);
if (error) {
device_printf(dev, "Unable to free interrupt resouce\n");
return error;
}
error = pci_release_msi(dev);
if (error)
device_printf(dev, "Unable to release interrupt\n");
return error;
}
int
pcib_detach(device_t dev)
{
int error;
struct pcib_softc *sc;
sc = device_get_softc(dev);
error = bus_generic_detach(dev);
if (error)
return (error);
if (sc->sc_irq_res)
error = pcib_release_intr_common(sc);
return error;
}
*/
void void
pcib_attach_common(device_t dev) pcib_attach_common(device_t dev)
{ {
Context not available.
sc = device_get_softc(dev); sc = device_get_softc(dev);
sc->dev = dev; sc->dev = dev;
pcib_setup_intr_common(sc);
impUnsubmitted
Not Done
Inline Actions

Where do we setup to get the individual device counts? this looks like it does the bridges, but it appears that devices also report this information. In experiments that I've done on a machine suffering from these issues, it is rare to see the pcibX device match the down-stream nvmeY device, for example.

imp: Where do we setup to get the individual device counts? this looks like it does the bridges, but…
/* /*
* Get current bridge configuration. * Get current bridge configuration.
*/ */
Context not available.
SYSCTL_ADD_UINT(sctx, SYSCTL_CHILDREN(soid), OID_AUTO, "subbus", SYSCTL_ADD_UINT(sctx, SYSCTL_CHILDREN(soid), OID_AUTO, "subbus",
CTLFLAG_RD, &sc->bus.sub, 0, "Subordinate bus number"); CTLFLAG_RD, &sc->bus.sub, 0, "Subordinate bus number");
/* /*
* Quirk handling. * Quirk handling.
*/ */
Context not available.