[PATCH 0/3] pci: fix/cleanup pcix get and set mmrbc functions

[PATCH 0/3] pci: fix/cleanup pcix get and set mmrbc functions

[Dean Nelson]

A customer running RHEL4.8 encountered

 "e1000: eth0: e1000_clean_tx_irq: Detected Tx Unit Hang"

type errors, which were determined to be the result of a bad return value
from e1000_pcix_get_mmrbc() causing the call to e1000_pcix_set_mmrbc() to
be skipped in the following snippet of code from e1000_init_hw().

	switch (hw->mac_type) {
	case e1000_82545_rev_3:
	case e1000_82546_rev_3:
		break;
	default:
		/* Workaround for PCI-X problem when BIOS sets MMRBC incorrectly. */
		if (hw->bus_type == e1000_bus_type_pcix
		    && e1000_pcix_get_mmrbc(hw) > 2048)
			e1000_pcix_set_mmrbc(hw, 2048);
		break;
	}

e1000_pcix_get_mmrbc() is basically a wrapper for a call to pcix_get_mmrbc().
e1000_pcix_set_mmrbc() is the same for pcix_set_mmrbc().

The following three patches are a response to the problems that were found to
exist with pcix_get_max_mmrbc(), pcix_get_mmrbc() and pcix_set_mmrbc().

Versions of these patches applicable to RHEL4 were verified by the customer to
solve their problem.

[PATCH 1/3] pci: fix return value from pcix_get_max_mmrbc()

[Dean Nelson]

For the PCI_X_STATUS register, pcix_get_max_mmrbc() is returning an incorrect
value, which is based on:

	(stat & PCI_X_STATUS_MAX_READ) >> 12

Valid return values are 512, 1024, 2048, 4096, which correspond to a 'stat'
(masked and right shifted by 21) of 0, 1, 2, 3, respectively.

A right shift by 11 would generate the correct return value when 'stat' (masked
and right shifted by 21) has a value of 1 or 2. But for a value of 0 or 3 it's
not possible to generate the correct return value by only right shifting.

Fix is based on pcix_get_mmrbc()'s similar dealings with the PCI_X_CMD register.

Signed-off-by: Dean Nelson <dnelson@redhat.com>

---

 drivers/pci/pci.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 5b548ae..1decd4f 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -2571,7 +2571,7 @@ int pcix_get_max_mmrbc(struct pci_dev *dev)
 	if (err)
 		return -EINVAL;
 
-	return (stat & PCI_X_STATUS_MAX_READ) >> 12;
+	return 512 << ((stat & PCI_X_STATUS_MAX_READ) >> 21);
 }
 EXPORT_SYMBOL(pcix_get_max_mmrbc);
 

Re: [PATCH 1/3] pci: fix return value from pcix_get_max_mmrbc()

[Jesse Barnes]

On Tue, 9 Mar 2010 22:26:40 -0500
Dean Nelson <dnelson@redhat.com> wrote:

> For the PCI_X_STATUS register, pcix_get_max_mmrbc() is returning an incorrect
> value, which is based on:
> 
> 	(stat & PCI_X_STATUS_MAX_READ) >> 12
> 
> Valid return values are 512, 1024, 2048, 4096, which correspond to a 'stat'
> (masked and right shifted by 21) of 0, 1, 2, 3, respectively.
> 
> A right shift by 11 would generate the correct return value when 'stat' (masked
> and right shifted by 21) has a value of 1 or 2. But for a value of 0 or 3 it's
> not possible to generate the correct return value by only right shifting.
> 
> Fix is based on pcix_get_mmrbc()'s similar dealings with the PCI_X_CMD register.
> 
> Signed-off-by: Dean Nelson <dnelson@redhat.com>
> 
> ---
> 
>  drivers/pci/pci.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 5b548ae..1decd4f 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -2571,7 +2571,7 @@ int pcix_get_max_mmrbc(struct pci_dev *dev)
>  	if (err)
>  		return -EINVAL;
>  
> -	return (stat & PCI_X_STATUS_MAX_READ) >> 12;
> +	return 512 << ((stat & PCI_X_STATUS_MAX_READ) >> 21);
>  }
>  EXPORT_SYMBOL(pcix_get_max_mmrbc);

Applied this series, thanks Dean.

-- 
Jesse Barnes, Intel Open Source Technology Center

[PATCH 2/3] pci: fix access of PCI_X_CMD by pcix get and set mmrbc functions

[Dean Nelson]

An e1000 driver on a system with a PCI-X bus was always being returned
a value of 135 from both pcix_get_mmrbc() and pcix_set_mmrbc(). This
value reflects an error return of PCIBIOS_BAD_REGISTER_NUMBER from
pci_bus_read_config_dword(,, cap + PCI_X_CMD,).

This is because for a dword, the following portion of the PCI_OP_READ()
macro:

	if (PCI_##size##_BAD) return PCIBIOS_BAD_REGISTER_NUMBER;

expands to:

	if (pos & 3) return PCIBIOS_BAD_REGISTER_NUMBER;

And is always true for 'cap + PCI_X_CMD', which is 0xe4 + 2 = 0xe6. ('cap' is
the result of calling pci_find_capability(, PCI_CAP_ID_PCIX).)

The same problem exists for pci_bus_write_config_dword(,, cap + PCI_X_CMD,).
In both cases, instead of calling _dword(), _word() should be called.

Signed-off-by: Dean Nelson <dnelson@redhat.com>

---

 drivers/pci/pci.c |   11 ++++++-----
 1 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 1decd4f..cdf201e 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -2585,13 +2585,13 @@ EXPORT_SYMBOL(pcix_get_max_mmrbc);
 int pcix_get_mmrbc(struct pci_dev *dev)
 {
 	int ret, cap;
-	u32 cmd;
+	u16 cmd;
 
 	cap = pci_find_capability(dev, PCI_CAP_ID_PCIX);
 	if (!cap)
 		return -EINVAL;
 
-	ret = pci_read_config_dword(dev, cap + PCI_X_CMD, &cmd);
+	ret = pci_read_config_word(dev, cap + PCI_X_CMD, &cmd);
 	if (!ret)
 		ret = 512 << ((cmd & PCI_X_CMD_MAX_READ) >> 2);
 
@@ -2611,7 +2611,8 @@ EXPORT_SYMBOL(pcix_get_mmrbc);
 int pcix_set_mmrbc(struct pci_dev *dev, int mmrbc)
 {
 	int cap, err = -EINVAL;
-	u32 stat, cmd, v, o;
+	u32 stat, v, o;
+	u16 cmd;
 
 	if (mmrbc < 512 || mmrbc > 4096 || !is_power_of_2(mmrbc))
 		goto out;
@@ -2629,7 +2630,7 @@ int pcix_set_mmrbc(struct pci_dev *dev, int mmrbc)
 	if (v > (stat & PCI_X_STATUS_MAX_READ) >> 21)
 		return -E2BIG;
 
-	err = pci_read_config_dword(dev, cap + PCI_X_CMD, &cmd);
+	err = pci_read_config_word(dev, cap + PCI_X_CMD, &cmd);
 	if (err)
 		goto out;
 
@@ -2641,7 +2642,7 @@ int pcix_set_mmrbc(struct pci_dev *dev, int mmrbc)
 
 		cmd &= ~PCI_X_CMD_MAX_READ;
 		cmd |= v << 2;
-		err = pci_write_config_dword(dev, cap + PCI_X_CMD, cmd);
+		err = pci_write_config_word(dev, cap + PCI_X_CMD, cmd);
 	}
 out:
 	return err;

[PATCH 3/3] pci: cleanup error return for pcix get and set mmrbc functions

[Dean Nelson]

pcix_get_mmrbc() returns the maximum memory read byte count (mmrbc), if
successful, or an appropriate error value, if not.

Distinguishing errors from correct values and understanding the meaning of an
error can be somewhat confusing in that:

	correct values: 512, 1024, 2048, 4096
	errors: -EINVAL  			-22
 		PCIBIOS_FUNC_NOT_SUPPORTED	0x81
		PCIBIOS_BAD_VENDOR_ID		0x83
		PCIBIOS_DEVICE_NOT_FOUND	0x86
		PCIBIOS_BAD_REGISTER_NUMBER	0x87
		PCIBIOS_SET_FAILED		0x88
		PCIBIOS_BUFFER_TOO_SMALL	0x89

The PCIBIOS_ errors are returned from the PCI functions generated by the
PCI_OP_READ() and PCI_OP_WRITE() macros.

In a similar manner, pcix_set_mmrbc() also returns the PCIBIOS_ error values
returned from pci_read_config_[word|dword]() and pci_write_config_word().

Following pcix_get_max_mmrbc()'s example, the following patch simply returns
-EINVAL for all PCIBIOS_ errors encountered by pcix_get_mmrbc(), and -EINVAL
or -EIO for those encountered by pcix_set_mmrbc().

This simplification was chosen in light of the fact that none of the current
callers of these functions are interested in the specific type of error
encountered. In the future, should this change, one could simply create a
function that maps each PCIBIOS_ error to a corresponding unique errno value,
which could be called by pcix_get_max_mmrbc(), pcix_get_mmrbc(), and
pcix_set_mmrbc().

Additionally, this patch eliminates some unnecessary variables.

Signed-off-by: Dean Nelson <dnelson@redhat.com>

---

 drivers/pci/pci.c |   36 ++++++++++++++++--------------------
 1 files changed, 16 insertions(+), 20 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index cdf201e..5b6ed6f 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -2560,15 +2560,14 @@ EXPORT_SYMBOL_GPL(pci_reset_function);
  */
 int pcix_get_max_mmrbc(struct pci_dev *dev)
 {
-	int err, cap;
+	int cap;
 	u32 stat;
 
 	cap = pci_find_capability(dev, PCI_CAP_ID_PCIX);
 	if (!cap)
 		return -EINVAL;
 
-	err = pci_read_config_dword(dev, cap + PCI_X_STATUS, &stat);
-	if (err)
+	if (pci_read_config_dword(dev, cap + PCI_X_STATUS, &stat))
 		return -EINVAL;
 
 	return 512 << ((stat & PCI_X_STATUS_MAX_READ) >> 21);
@@ -2584,18 +2583,17 @@ EXPORT_SYMBOL(pcix_get_max_mmrbc);
  */
 int pcix_get_mmrbc(struct pci_dev *dev)
 {
-	int ret, cap;
+	int cap;
 	u16 cmd;
 
 	cap = pci_find_capability(dev, PCI_CAP_ID_PCIX);
 	if (!cap)
 		return -EINVAL;
 
-	ret = pci_read_config_word(dev, cap + PCI_X_CMD, &cmd);
-	if (!ret)
-		ret = 512 << ((cmd & PCI_X_CMD_MAX_READ) >> 2);
+	if (pci_read_config_word(dev, cap + PCI_X_CMD, &cmd))
+		return -EINVAL;
 
-	return ret;
+	return 512 << ((cmd & PCI_X_CMD_MAX_READ) >> 2);
 }
 EXPORT_SYMBOL(pcix_get_mmrbc);
 
@@ -2610,29 +2608,27 @@ EXPORT_SYMBOL(pcix_get_mmrbc);
  */
 int pcix_set_mmrbc(struct pci_dev *dev, int mmrbc)
 {
-	int cap, err = -EINVAL;
+	int cap;
 	u32 stat, v, o;
 	u16 cmd;
 
 	if (mmrbc < 512 || mmrbc > 4096 || !is_power_of_2(mmrbc))
-		goto out;
+		return -EINVAL;
 
 	v = ffs(mmrbc) - 10;
 
 	cap = pci_find_capability(dev, PCI_CAP_ID_PCIX);
 	if (!cap)
-		goto out;
+		return -EINVAL;
 
-	err = pci_read_config_dword(dev, cap + PCI_X_STATUS, &stat);
-	if (err)
-		goto out;
+	if (pci_read_config_dword(dev, cap + PCI_X_STATUS, &stat))
+		return -EINVAL;
 
 	if (v > (stat & PCI_X_STATUS_MAX_READ) >> 21)
 		return -E2BIG;
 
-	err = pci_read_config_word(dev, cap + PCI_X_CMD, &cmd);
-	if (err)
-		goto out;
+	if (pci_read_config_word(dev, cap + PCI_X_CMD, &cmd))
+		return -EINVAL;
 
 	o = (cmd & PCI_X_CMD_MAX_READ) >> 2;
 	if (o != v) {
@@ -2642,10 +2638,10 @@ int pcix_set_mmrbc(struct pci_dev *dev, int mmrbc)
 
 		cmd &= ~PCI_X_CMD_MAX_READ;
 		cmd |= v << 2;
-		err = pci_write_config_word(dev, cap + PCI_X_CMD, cmd);
+		if (pci_write_config_word(dev, cap + PCI_X_CMD, cmd))
+			return -EIO;
 	}
-out:
-	return err;
+	return 0;
 }
 EXPORT_SYMBOL(pcix_set_mmrbc);
 

Re: [PATCH 0/3] pci: fix/cleanup pcix get and set mmrbc functions

[Jesse Barnes]

On Tue, 9 Mar 2010 22:26:33 -0500
Dean Nelson <dnelson@redhat.com> wrote:

> A customer running RHEL4.8 encountered
> 
>  "e1000: eth0: e1000_clean_tx_irq: Detected Tx Unit Hang"
> 
> type errors, which were determined to be the result of a bad return value
> from e1000_pcix_get_mmrbc() causing the call to e1000_pcix_set_mmrbc() to
> be skipped in the following snippet of code from e1000_init_hw().
> 
> 	switch (hw->mac_type) {
> 	case e1000_82545_rev_3:
> 	case e1000_82546_rev_3:
> 		break;
> 	default:
> 		/* Workaround for PCI-X problem when BIOS sets MMRBC incorrectly. */
> 		if (hw->bus_type == e1000_bus_type_pcix
> 		    && e1000_pcix_get_mmrbc(hw) > 2048)
> 			e1000_pcix_set_mmrbc(hw, 2048);
> 		break;
> 	}
> 
> e1000_pcix_get_mmrbc() is basically a wrapper for a call to pcix_get_mmrbc().
> e1000_pcix_set_mmrbc() is the same for pcix_set_mmrbc().
> 
> The following three patches are a response to the problems that were found to
> exist with pcix_get_max_mmrbc(), pcix_get_mmrbc() and pcix_set_mmrbc().
> 
> Versions of these patches applicable to RHEL4 were verified by the customer to
> solve their problem.

Thanks Dean, I'll pull these in and send them to Linus for 2.6.34.  I
assume they should also be included in the stable kernel series?  If
so, I'll add a cc: stable@kernel.org when I commit them.

Thanks,
-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 0/3] pci: fix/cleanup pcix get and set mmrbc functions

[Dean Nelson]

On 03/12/2010 07:00 PM, Jesse Barnes wrote:
> On Tue, 9 Mar 2010 22:26:33 -0500
> Dean Nelson<dnelson@redhat.com>  wrote:
>
>> A customer running RHEL4.8 encountered
>>
>>   "e1000: eth0: e1000_clean_tx_irq: Detected Tx Unit Hang"
>>
>> type errors, which were determined to be the result of a bad return value
>> from e1000_pcix_get_mmrbc() causing the call to e1000_pcix_set_mmrbc() to
>> be skipped in the following snippet of code from e1000_init_hw().
>>
>> 	switch (hw->mac_type) {
>> 	case e1000_82545_rev_3:
>> 	case e1000_82546_rev_3:
>> 		break;
>> 	default:
>> 		/* Workaround for PCI-X problem when BIOS sets MMRBC incorrectly. */
>> 		if (hw->bus_type == e1000_bus_type_pcix
>> 		&&  e1000_pcix_get_mmrbc(hw)>  2048)
>> 			e1000_pcix_set_mmrbc(hw, 2048);
>> 		break;
>> 	}
>>
>> e1000_pcix_get_mmrbc() is basically a wrapper for a call to pcix_get_mmrbc().
>> e1000_pcix_set_mmrbc() is the same for pcix_set_mmrbc().
>>
>> The following three patches are a response to the problems that were found to
>> exist with pcix_get_max_mmrbc(), pcix_get_mmrbc() and pcix_set_mmrbc().
>>
>> Versions of these patches applicable to RHEL4 were verified by the customer to
>> solve their problem.
>
> Thanks Dean, I'll pull these in and send them to Linus for 2.6.34.  I
> assume they should also be included in the stable kernel series?  If
> so, I'll add a cc: stable@kernel.org when I commit them.

Yes, they should also be included in the stable kernel series. Thanks so 
much Jesse.

Dean