Re: PCI extended configuration support

[christos%zoulas.com@localhost (Christos Zoulas)]

On Sep 28,  7:51pm, msaitoh%execsw.org@localhost (Masanobu SAITOH) wrote:
-- Subject: Re: PCI extended configuration support

| On 2015/09/27 11:03, Christos Zoulas wrote:
| > In article <5604D1BE.3080502%execsw.org@localhost>,
| > Masanobu SAITOH  <msaitoh%execsw.org@localhost> wrote:
| >> On 2015/09/11 19:44, Masanobu SAITOH wrote:
| >>
| >> More machines support the extended configuration area than before.
| >>
| >>   Is it OK to commit?
| > 
| > LGTM, but since this is not performance critical can't we centralize the
| > range test and do it in a wrapper?
| 
|  We have no good idea... :-(

I meant instead of editing each of the drivers edit the pci_machdep.h file
of each arch from:

#define pci_conf_read(c, t, r)                                          \
    (*(c)->pc_conf_read)((c)->pc_conf_v, (t), (r))
#define pci_conf_write(c, t, r, v)                                      \
    (*(c)->pc_conf_write)((c)->pc_conf_v, (t), (r), (v))

to:

static __inline pcireg_t pci_conf_read(void *_cpv, pcitag_t _tag, int _offset) {
	if ((unsigned int)_offset >= PCI_CONF_SIZE)
		return (pcireg_t)-1;
	return *((c)->pc_conf_read)(
	    ((struct <arch>_pci_chipset *)_cpv)->pc_conf_v, _tag, _offset);
}

static __inline void pci_conf_write(void *_cpv, pcitag_t _tag, int _offset) {
	if ((unsigned int)_offset >= PCI_CONF_SIZE)
		return;
	return *((c)->pc_conf_write)(
	    ((struct <arch>_pci_chipset *)_cpv)->pc_conf_v, _tag, _offset);
}

Or you could even make a macro of it in pci_var.h like:

#define PCI_ARCH_CONF_READ(c, t, r) \
	do { \
		if ((unsigned int)(r) >= PCI_CONF_SIZE) \
			return (pcireg_t) -1; \
		return *((c)->pc_conf_read)((c), (t), (r)); \
	while (/*CONSTCOND*/0)

And then change the macro in arch/include/pci_machdep.h to:

#define pci_conf_read(c, t, r) PCI_ARCH_CONF_READ(c, t, r)

Or you could just centralize the machdep macro to pci_var.h, and put it
around ifdefs so that the machdep code can override it:

#ifndef pci_conf_read
#define pci_conf_read(c, t, r) \
	do { \
		if ((unsigned int)(r) >= PCI_CONF_SIZE) \
			return (pcireg_t) -1; \
		return *((c)->pc_conf_read)((c), (t), (r)); \
	while (/*CONSTCOND*/0)
#endif

| > Plus this can't be negative so perhaps
| > it should be unsigned or checked for < 0 too??
| > 
| > +	if (reg >= PCI_CONF_SIZE)
| > +		return (pcireg_t) -1;
| > +
| 
|  Done.

I think you missed some...

| > Finally a few more constants could be defined instead
| > of hard-coded values....
| > 
| > +	switch (pcie_devtype) {
| > +	case 0x4:	/* Root Port of PCI Express Root Complex */
| > +	case 0xa:	/* Root Complex Event Collector */
| 
|  Done.

Thanks!

christos