| Message ID | 25339251-513a-75c6-e96e-c284d23eed0f@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [net-next] cxgb4: remove bogus CHELSIO_VPD_UNIQUE_ID constant | expand |
On Sat, 16 Jan 2021 14:45:25 +0100 Heiner Kallweit wrote: > The comment is quite weird, there is no such thing as a vendor-specific > VPD id. 0x82 is the value of PCI_VPD_LRDT_ID_STRING. So what we are > doing here is simply checking whether the byte at VPD address VPD_BASE > is a valid string LRDT, same as what is done a few lines later in > the code. > LRDT = Large Resource Data Tag, see PCI 2.2 spec, VPD chapter > > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> Did you find this by code inspection? > diff --git a/drivers/net/ethernet/chelsio/cxgb4/t4_hw.c b/drivers/net/ethernet/chelsio/cxgb4/t4_hw.c > index 2c80371f9..48f20a6a0 100644 > --- a/drivers/net/ethernet/chelsio/cxgb4/t4_hw.c > +++ b/drivers/net/ethernet/chelsio/cxgb4/t4_hw.c > @@ -2689,7 +2689,6 @@ void t4_get_regs(struct adapter *adap, void *buf, size_t buf_size) > #define VPD_BASE 0x400 > #define VPD_BASE_OLD 0 > #define VPD_LEN 1024 > -#define CHELSIO_VPD_UNIQUE_ID 0x82 > > /** > * t4_eeprom_ptov - translate a physical EEPROM address to virtual > @@ -2743,9 +2742,9 @@ int t4_seeprom_wp(struct adapter *adapter, bool enable) > */ > int t4_get_raw_vpd_params(struct adapter *adapter, struct vpd_params *p) > { > - int i, ret = 0, addr; > + int i, ret = 0, addr = VPD_BASE; IMHO it's more readable if the addr is set to BASE or BASE_OLD in one place rather than having a default value at variable init which may be overwritten. > int ec, sn, pn, na; > - u8 *vpd, csum; > + u8 *vpd, csum, base_val = 0; > unsigned int vpdr_len, kw_offset, id_len; > > vpd = vmalloc(VPD_LEN); > @@ -2755,17 +2754,12 @@ int t4_get_raw_vpd_params(struct adapter *adapter, struct vpd_params *p) > /* Card information normally starts at VPD_BASE but early cards had > * it at 0. > */ > - ret = pci_read_vpd(adapter->pdev, VPD_BASE, sizeof(u32), vpd); > + ret = pci_read_vpd(adapter->pdev, VPD_BASE, 1, &base_val); Are we sure this works? I've seen silicon out there which has problems with small PCI accesses (granted those were not VPD accesses). > if (ret < 0) > goto out; > > - /* The VPD shall have a unique identifier specified by the PCI SIG. > - * For chelsio adapters, the identifier is 0x82. The first byte of a VPD > - * shall be CHELSIO_VPD_UNIQUE_ID (0x82). The VPD programming software > - * is expected to automatically put this entry at the > - * beginning of the VPD. > - */ > - addr = *vpd == CHELSIO_VPD_UNIQUE_ID ? VPD_BASE : VPD_BASE_OLD; > + if (base_val != PCI_VPD_LRDT_ID_STRING) > + addr = VPD_BASE_OLD; > > ret = pci_read_vpd(adapter->pdev, addr, VPD_LEN, vpd); > if (ret < 0)
On 19.01.2021 23:02, Jakub Kicinski wrote: > On Sat, 16 Jan 2021 14:45:25 +0100 Heiner Kallweit wrote: >> The comment is quite weird, there is no such thing as a vendor-specific >> VPD id. 0x82 is the value of PCI_VPD_LRDT_ID_STRING. So what we are >> doing here is simply checking whether the byte at VPD address VPD_BASE >> is a valid string LRDT, same as what is done a few lines later in >> the code. >> LRDT = Large Resource Data Tag, see PCI 2.2 spec, VPD chapter >> >> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> > > Did you find this by code inspection? > Well, more or less .. RTL8168 indicates it has VPD but in all cases I've seen there is no VPD EEPROM. Result is that the VPD code throws an "invalid VPD tag" error. When checking the VPD code (+ VPD spec) and its (few) users I came across the chelsio driver. >> diff --git a/drivers/net/ethernet/chelsio/cxgb4/t4_hw.c b/drivers/net/ethernet/chelsio/cxgb4/t4_hw.c >> index 2c80371f9..48f20a6a0 100644 >> --- a/drivers/net/ethernet/chelsio/cxgb4/t4_hw.c >> +++ b/drivers/net/ethernet/chelsio/cxgb4/t4_hw.c >> @@ -2689,7 +2689,6 @@ void t4_get_regs(struct adapter *adap, void *buf, size_t buf_size) >> #define VPD_BASE 0x400 >> #define VPD_BASE_OLD 0 >> #define VPD_LEN 1024 >> -#define CHELSIO_VPD_UNIQUE_ID 0x82 >> >> /** >> * t4_eeprom_ptov - translate a physical EEPROM address to virtual >> @@ -2743,9 +2742,9 @@ int t4_seeprom_wp(struct adapter *adapter, bool enable) >> */ >> int t4_get_raw_vpd_params(struct adapter *adapter, struct vpd_params *p) >> { >> - int i, ret = 0, addr; >> + int i, ret = 0, addr = VPD_BASE; > > IMHO it's more readable if the addr is set to BASE or BASE_OLD in one > place rather than having a default value at variable init which may be > overwritten. > OK. Thought was just that VPD_BASE is the more common case. I'll change this in a v2. >> int ec, sn, pn, na; >> - u8 *vpd, csum; >> + u8 *vpd, csum, base_val = 0; >> unsigned int vpdr_len, kw_offset, id_len; >> >> vpd = vmalloc(VPD_LEN); >> @@ -2755,17 +2754,12 @@ int t4_get_raw_vpd_params(struct adapter *adapter, struct vpd_params *p) >> /* Card information normally starts at VPD_BASE but early cards had >> * it at 0. >> */ >> - ret = pci_read_vpd(adapter->pdev, VPD_BASE, sizeof(u32), vpd); >> + ret = pci_read_vpd(adapter->pdev, VPD_BASE, 1, &base_val); > > Are we sure this works? I've seen silicon out there which has problems > with small PCI accesses (granted those were not VPD accesses). > The underlying PCI register access reads 4 bytes, the VPD code will ignore the higher three bytes here. >> if (ret < 0) >> goto out; >> >> - /* The VPD shall have a unique identifier specified by the PCI SIG. >> - * For chelsio adapters, the identifier is 0x82. The first byte of a VPD >> - * shall be CHELSIO_VPD_UNIQUE_ID (0x82). The VPD programming software >> - * is expected to automatically put this entry at the >> - * beginning of the VPD. >> - */ >> - addr = *vpd == CHELSIO_VPD_UNIQUE_ID ? VPD_BASE : VPD_BASE_OLD; >> + if (base_val != PCI_VPD_LRDT_ID_STRING) >> + addr = VPD_BASE_OLD; >> >> ret = pci_read_vpd(adapter->pdev, addr, VPD_LEN, vpd); >> if (ret < 0) >
diff --git a/drivers/net/ethernet/chelsio/cxgb4/t4_hw.c b/drivers/net/ethernet/chelsio/cxgb4/t4_hw.c index 2c80371f9..48f20a6a0 100644 --- a/drivers/net/ethernet/chelsio/cxgb4/t4_hw.c +++ b/drivers/net/ethernet/chelsio/cxgb4/t4_hw.c @@ -2689,7 +2689,6 @@ void t4_get_regs(struct adapter *adap, void *buf, size_t buf_size) #define VPD_BASE 0x400 #define VPD_BASE_OLD 0 #define VPD_LEN 1024 -#define CHELSIO_VPD_UNIQUE_ID 0x82 /** * t4_eeprom_ptov - translate a physical EEPROM address to virtual @@ -2743,9 +2742,9 @@ int t4_seeprom_wp(struct adapter *adapter, bool enable) */ int t4_get_raw_vpd_params(struct adapter *adapter, struct vpd_params *p) { - int i, ret = 0, addr; + int i, ret = 0, addr = VPD_BASE; int ec, sn, pn, na; - u8 *vpd, csum; + u8 *vpd, csum, base_val = 0; unsigned int vpdr_len, kw_offset, id_len; vpd = vmalloc(VPD_LEN); @@ -2755,17 +2754,12 @@ int t4_get_raw_vpd_params(struct adapter *adapter, struct vpd_params *p) /* Card information normally starts at VPD_BASE but early cards had * it at 0. */ - ret = pci_read_vpd(adapter->pdev, VPD_BASE, sizeof(u32), vpd); + ret = pci_read_vpd(adapter->pdev, VPD_BASE, 1, &base_val); if (ret < 0) goto out; - /* The VPD shall have a unique identifier specified by the PCI SIG. - * For chelsio adapters, the identifier is 0x82. The first byte of a VPD - * shall be CHELSIO_VPD_UNIQUE_ID (0x82). The VPD programming software - * is expected to automatically put this entry at the - * beginning of the VPD. - */ - addr = *vpd == CHELSIO_VPD_UNIQUE_ID ? VPD_BASE : VPD_BASE_OLD; + if (base_val != PCI_VPD_LRDT_ID_STRING) + addr = VPD_BASE_OLD; ret = pci_read_vpd(adapter->pdev, addr, VPD_LEN, vpd); if (ret < 0)
The comment is quite weird, there is no such thing as a vendor-specific VPD id. 0x82 is the value of PCI_VPD_LRDT_ID_STRING. So what we are doing here is simply checking whether the byte at VPD address VPD_BASE is a valid string LRDT, same as what is done a few lines later in the code. LRDT = Large Resource Data Tag, see PCI 2.2 spec, VPD chapter Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> --- drivers/net/ethernet/chelsio/cxgb4/t4_hw.c | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-)