* [PATCH net-next] cxgb4: remove bogus CHELSIO_VPD_UNIQUE_ID constant
@ 2021-01-16 13:45 Heiner Kallweit
2021-01-19 22:02 ` Jakub Kicinski
0 siblings, 1 reply; 3+ messages in thread
From: Heiner Kallweit @ 2021-01-16 13:45 UTC (permalink / raw)
To: Raju Rangoju, David Miller, Jakub Kicinski; +Cc: netdev
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(-)
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)
--
2.30.0
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH net-next] cxgb4: remove bogus CHELSIO_VPD_UNIQUE_ID constant
2021-01-16 13:45 [PATCH net-next] cxgb4: remove bogus CHELSIO_VPD_UNIQUE_ID constant Heiner Kallweit
@ 2021-01-19 22:02 ` Jakub Kicinski
2021-01-20 7:07 ` Heiner Kallweit
0 siblings, 1 reply; 3+ messages in thread
From: Jakub Kicinski @ 2021-01-19 22:02 UTC (permalink / raw)
To: Heiner Kallweit; +Cc: Raju Rangoju, David Miller, netdev
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)
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH net-next] cxgb4: remove bogus CHELSIO_VPD_UNIQUE_ID constant
2021-01-19 22:02 ` Jakub Kicinski
@ 2021-01-20 7:07 ` Heiner Kallweit
0 siblings, 0 replies; 3+ messages in thread
From: Heiner Kallweit @ 2021-01-20 7:07 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: Raju Rangoju, David Miller, netdev
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)
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-01-20 7:09 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-16 13:45 [PATCH net-next] cxgb4: remove bogus CHELSIO_VPD_UNIQUE_ID constant Heiner Kallweit
2021-01-19 22:02 ` Jakub Kicinski
2021-01-20 7:07 ` Heiner Kallweit
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).