[PATCH 1/3] cxgb3: Replace PCI related literals with defines & correct variable

[PATCH 1/3] cxgb3: Replace PCI related literals with defines & correct variable

[Ilpo Järvinen]

Replace literals 0, 2, 0x1425 with PCI_VENDOR_ID, PCI_DEVICE_ID,
PCI_VENDOR_ID_CHELSIO, respectively. Rename devid variable to vendor_id
to remove confusion.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/net/ethernet/chelsio/cxgb3/t3_hw.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/chelsio/cxgb3/t3_hw.c b/drivers/net/ethernet/chelsio/cxgb3/t3_hw.c
index a06003bfa04b..4e917a578c77 100644
--- a/drivers/net/ethernet/chelsio/cxgb3/t3_hw.c
+++ b/drivers/net/ethernet/chelsio/cxgb3/t3_hw.c
@@ -30,6 +30,8 @@
  * SOFTWARE.
  */
 #include <linux/etherdevice.h>
+#include <linux/pci.h>
+
 #include "common.h"
 #include "regs.h"
 #include "sge_defs.h"
@@ -3262,7 +3264,7 @@ static void config_pcie(struct adapter *adap)
 	pcie_capability_read_word(adap->pdev, PCI_EXP_DEVCTL, &val);
 	pldsize = (val & PCI_EXP_DEVCTL_PAYLOAD) >> 5;
 
-	pci_read_config_word(adap->pdev, 0x2, &devid);
+	pci_read_config_word(adap->pdev, PCI_DEVICE_ID, &devid);
 	if (devid == 0x37) {
 		pcie_capability_write_word(adap->pdev, PCI_EXP_DEVCTL,
 					   val & ~PCI_EXP_DEVCTL_READRQ &
@@ -3477,7 +3479,7 @@ static void mac_prep(struct cmac *mac, struct adapter *adapter, int index)
 	u16 devid;
 
 	mac->adapter = adapter;
-	pci_read_config_word(adapter->pdev, 0x2, &devid);
+	pci_read_config_word(adapter->pdev, PCI_DEVICE_ID, &devid);
 
 	if (devid == 0x37 && !adapter->params.vpd.xauicfg[1])
 		index = 0;
@@ -3528,7 +3530,7 @@ int t3_reset_adapter(struct adapter *adapter)
 {
 	int i, save_and_restore_pcie =
 	    adapter->params.rev < T3_REV_B2 && is_pcie(adapter);
-	uint16_t devid = 0;
+	u16 vendor_id = 0;
 
 	if (save_and_restore_pcie)
 		pci_save_state(adapter->pdev);
@@ -3540,12 +3542,12 @@ int t3_reset_adapter(struct adapter *adapter)
 	 */
 	for (i = 0; i < 10; i++) {
 		msleep(50);
-		pci_read_config_word(adapter->pdev, 0x00, &devid);
-		if (devid == 0x1425)
+		pci_read_config_word(adapter->pdev, PCI_VENDOR_ID, &vendor_id);
+		if (vendor_id == PCI_VENDOR_ID_CHELSIO)
 			break;
 	}
 
-	if (devid != 0x1425)
+	if (vendor_id != PCI_VENDOR_ID_CHELSIO)
 		return -1;
 
 	if (save_and_restore_pcie)

base-commit: 19272b37aa4f83ca52bdf9c16d5d81bdd1354494
-- 
2.39.5

[PATCH 2/3] cxgb3: Use FIELD_GET() for PCI register fields

[Ilpo Järvinen]

Instead of literals and open-coding shifts and masks, use FIELD_GET()
and the correct field define from pci_regs.h.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/net/ethernet/chelsio/cxgb3/t3_hw.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/chelsio/cxgb3/t3_hw.c b/drivers/net/ethernet/chelsio/cxgb3/t3_hw.c
index 4e917a578c77..171bf6cf1abf 100644
--- a/drivers/net/ethernet/chelsio/cxgb3/t3_hw.c
+++ b/drivers/net/ethernet/chelsio/cxgb3/t3_hw.c
@@ -29,6 +29,7 @@
  * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
  * SOFTWARE.
  */
+#include <linux/bitfield.h>
 #include <linux/etherdevice.h>
 #include <linux/pci.h>
 
@@ -3262,7 +3263,7 @@ static void config_pcie(struct adapter *adap)
 	unsigned int fst_trn_rx, fst_trn_tx, acklat, rpllmt;
 
 	pcie_capability_read_word(adap->pdev, PCI_EXP_DEVCTL, &val);
-	pldsize = (val & PCI_EXP_DEVCTL_PAYLOAD) >> 5;
+	pldsize = FIELD_GET(PCI_EXP_DEVCTL_PAYLOAD, val);
 
 	pci_read_config_word(adap->pdev, PCI_DEVICE_ID, &devid);
 	if (devid == 0x37) {
@@ -3400,7 +3401,7 @@ static void get_pci_mode(struct adapter *adapter, struct pci_params *p)
 
 		p->variant = PCI_VARIANT_PCIE;
 		pcie_capability_read_word(adapter->pdev, PCI_EXP_LNKSTA, &val);
-		p->width = (val >> 4) & 0x3f;
+		p->width = FIELD_GET(PCI_EXP_LNKSTA_NLW, val);
 		return;
 	}
 
-- 
2.39.5

[PATCH 3/3] cxgb3: Split complex PCI write statement into logic + write

[Ilpo Järvinen]

Instead of trying to complex logic within the PCI capability write
statement, split the logic onto separate lines for better readability.
Also, don't pretend just clearing the fields, but set the fields to
what 0 means (128 bytes).

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/net/ethernet/chelsio/cxgb3/t3_hw.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/chelsio/cxgb3/t3_hw.c b/drivers/net/ethernet/chelsio/cxgb3/t3_hw.c
index 171bf6cf1abf..b9327d8c6893 100644
--- a/drivers/net/ethernet/chelsio/cxgb3/t3_hw.c
+++ b/drivers/net/ethernet/chelsio/cxgb3/t3_hw.c
@@ -3267,9 +3267,9 @@ static void config_pcie(struct adapter *adap)
 
 	pci_read_config_word(adap->pdev, PCI_DEVICE_ID, &devid);
 	if (devid == 0x37) {
-		pcie_capability_write_word(adap->pdev, PCI_EXP_DEVCTL,
-					   val & ~PCI_EXP_DEVCTL_READRQ &
-					   ~PCI_EXP_DEVCTL_PAYLOAD);
+		val &= ~(PCI_EXP_DEVCTL_PAYLOAD|PCI_EXP_DEVCTL_READRQ);
+		val |= PCI_EXP_DEVCTL_PAYLOAD_128B|PCI_EXP_DEVCTL_READRQ_128B;
+		pcie_capability_write_word(adap->pdev, PCI_EXP_DEVCTL, val);
 		pldsize = 0;
 	}
 
-- 
2.39.5

Re: [PATCH 1/3] cxgb3: Replace PCI related literals with defines & correct variable

[Jakub Kicinski]

On Tue, 10 Jun 2025 13:32:03 +0300 Ilpo Järvinen wrote:
> Replace literals 0, 2, 0x1425 with PCI_VENDOR_ID, PCI_DEVICE_ID,
> PCI_VENDOR_ID_CHELSIO, respectively. Rename devid variable to vendor_id
> to remove confusion.

This series is missing a cover letter. An explanation of why you're
touching this very very old driver is in order, and please comment
on whether you can test this on real HW, because we don't like
refactoring of very old code:

Quoting documentation:

  Clean-up patches
  ~~~~~~~~~~~~~~~~
  
  Netdev discourages patches which perform simple clean-ups, which are not in
  the context of other work. For example:
  
  * Addressing ``checkpatch.pl`` warnings
  * Addressing :ref:`Local variable ordering<rcs>` issues
  * Conversions to device-managed APIs (``devm_`` helpers)
  
  This is because it is felt that the churn that such changes produce comes
  at a greater cost than the value of such clean-ups.
  
  Conversely, spelling and grammar fixes are not discouraged.
  
See: https://www.kernel.org/doc/html/next/process/maintainer-netdev.html#clean-up-patches
-- 
pw-bot: cr

Re: [PATCH 1/3] cxgb3: Replace PCI related literals with defines & correct variable

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 1918 bytes --]

On Tue, 10 Jun 2025, Jakub Kicinski wrote:
> On Tue, 10 Jun 2025 13:32:03 +0300 Ilpo Järvinen wrote:
> > Replace literals 0, 2, 0x1425 with PCI_VENDOR_ID, PCI_DEVICE_ID,
> > PCI_VENDOR_ID_CHELSIO, respectively. Rename devid variable to vendor_id
> > to remove confusion.
> 
> This series is missing a cover letter. An explanation of why you're
> touching this very very old driver is in order, and please comment
> on whether you can test this on real HW, because we don't like

No, I don't have the HW available.

> refactoring of very old code:
> 
> Quoting documentation:
> 
>   Clean-up patches
>   ~~~~~~~~~~~~~~~~
>   
>   Netdev discourages patches which perform simple clean-ups, which are not in
>   the context of other work. For example:
>   
>   * Addressing ``checkpatch.pl`` warnings
>   * Addressing :ref:`Local variable ordering<rcs>` issues
>   * Conversions to device-managed APIs (``devm_`` helpers)
>   
>   This is because it is felt that the churn that such changes produce comes
>   at a greater cost than the value of such clean-ups.
>   
>   Conversely, spelling and grammar fixes are not discouraged.
>   
> See: https://www.kernel.org/doc/html/next/process/maintainer-netdev.html#clean-up-patches

Fine, I don't _need_ to get these accepted. I'll keep this in mind in
future.

It probably came up during some other work when I had to look through use 
of all PCI accessor functions and understand what the callers are trying 
to do. When I took a look at the C file, I ended up noticing other things 
too.

As these are/were not direct requirement for the actual accessor work, I 
tend to send such series separately to avoid complicating review of one or 
the other, feature series tend to be complicated enough even without the 
cleanup patches.

It seems that in the unsent state, these patches predated adding that 
guidance.

-- 
 i.