diff mbox series

[06/12] bnx2x: Search VPD with pci_vpd_find_ro_info_keyword()

Message ID a9f730cf-e31e-902b-7b39-0ff2e99636e0@gmail.com
State New
Headers show
Series [01/12] sfc: falcon: Read VPD with pci_vpd_alloc() | expand

Commit Message

Heiner Kallweit Aug. 22, 2021, 1:54 p.m. UTC 
Use pci_vpd_find_ro_info_keyword() to search for keywords in VPD to
simplify the code.

str_id_reg and str_id_cap hold the same string and are used in the same
comparison. This doesn't make sense, use one string str_id instead.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 .../net/ethernet/broadcom/bnx2x/bnx2x_main.c  | 57 +++++--------------
 1 file changed, 15 insertions(+), 42 deletions(-)

Comments

Bjorn Helgaas Aug. 24, 2021, 5:02 p.m. UTC | #1 
On Sun, Aug 22, 2021 at 03:54:23PM +0200, Heiner Kallweit wrote:
> Use pci_vpd_find_ro_info_keyword() to search for keywords in VPD to

> simplify the code.

> 

> str_id_reg and str_id_cap hold the same string and are used in the same

> comparison. This doesn't make sense, use one string str_id instead.


str_id_reg is printed with "%04x" (lower-case hex letters) and
str_id_cap with "%04X" (upper-case hex letters), so the previous code
would match either 0xabcd or 0xABCD.  After this patch, we'd match
only the latter.

PCI_VENDOR_ID_DELL is 0x1028, so it shouldn't make any difference,
which makes me wonder why somebody bothered with both.

But it does seem like a potential landmine to change the case
sensitivity.  Maybe strncasecmp() instead?

> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>

> ---

>  .../net/ethernet/broadcom/bnx2x/bnx2x_main.c  | 57 +++++--------------

>  1 file changed, 15 insertions(+), 42 deletions(-)

> 

> diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c

> index 0466adf8d..2c7bfc416 100644

> --- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c

> +++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c

> @@ -12189,11 +12189,10 @@ static int bnx2x_get_hwinfo(struct bnx2x *bp)

>  

>  static void bnx2x_read_fwinfo(struct bnx2x *bp)

>  {

> -	int i, block_end, rodi;

> -	char str_id_reg[VENDOR_ID_LEN+1];

> -	char str_id_cap[VENDOR_ID_LEN+1];

> -	unsigned int vpd_len;

> -	u8 *vpd_data, len;

> +	char str_id[VENDOR_ID_LEN + 1];

> +	unsigned int vpd_len, kw_len;

> +	u8 *vpd_data;

> +	int rodi;

>  

>  	memset(bp->fw_ver, 0, sizeof(bp->fw_ver));

>  

> @@ -12201,46 +12200,20 @@ static void bnx2x_read_fwinfo(struct bnx2x *bp)

>  	if (IS_ERR(vpd_data))

>  		return;

>  

> -	/* VPD RO tag should be first tag after identifier string, hence

> -	 * we should be able to find it in first BNX2X_VPD_LEN chars

> -	 */

> -	i = pci_vpd_find_tag(vpd_data, vpd_len, PCI_VPD_LRDT_RO_DATA);

> -	if (i < 0)

> -		goto out_not_found;

> -

> -	block_end = i + PCI_VPD_LRDT_TAG_SIZE +

> -		    pci_vpd_lrdt_size(&vpd_data[i]);

> -	i += PCI_VPD_LRDT_TAG_SIZE;

> -

> -	rodi = pci_vpd_find_info_keyword(vpd_data, i, block_end,

> -				   PCI_VPD_RO_KEYWORD_MFR_ID);

> -	if (rodi < 0)

> -		goto out_not_found;

> -

> -	len = pci_vpd_info_field_size(&vpd_data[rodi]);

> -

> -	if (len != VENDOR_ID_LEN)

> +	rodi = pci_vpd_find_ro_info_keyword(vpd_data, vpd_len,

> +					    PCI_VPD_RO_KEYWORD_MFR_ID, &kw_len);

> +	if (rodi < 0 || kw_len != VENDOR_ID_LEN)

>  		goto out_not_found;

>  

> -	rodi += PCI_VPD_INFO_FLD_HDR_SIZE;

> -

>  	/* vendor specific info */

> -	snprintf(str_id_reg, VENDOR_ID_LEN + 1, "%04x", PCI_VENDOR_ID_DELL);

> -	snprintf(str_id_cap, VENDOR_ID_LEN + 1, "%04X", PCI_VENDOR_ID_DELL);

> -	if (!strncmp(str_id_reg, &vpd_data[rodi], VENDOR_ID_LEN) ||

> -	    !strncmp(str_id_cap, &vpd_data[rodi], VENDOR_ID_LEN)) {

> -

> -		rodi = pci_vpd_find_info_keyword(vpd_data, i, block_end,

> -						PCI_VPD_RO_KEYWORD_VENDOR0);

> -		if (rodi >= 0) {

> -			len = pci_vpd_info_field_size(&vpd_data[rodi]);

> -

> -			rodi += PCI_VPD_INFO_FLD_HDR_SIZE;

> -

> -			if (len < 32 && (len + rodi) <= vpd_len) {

> -				memcpy(bp->fw_ver, &vpd_data[rodi], len);

> -				bp->fw_ver[len] = ' ';

> -			}

> +	snprintf(str_id, VENDOR_ID_LEN + 1, "%04X", PCI_VENDOR_ID_DELL);

> +	if (!strncmp(str_id, &vpd_data[rodi], VENDOR_ID_LEN)) {

> +		rodi = pci_vpd_find_ro_info_keyword(vpd_data, vpd_len,

> +						    PCI_VPD_RO_KEYWORD_VENDOR0,

> +						    &kw_len);

> +		if (rodi >= 0 && kw_len < sizeof(bp->fw_ver)) {

> +			memcpy(bp->fw_ver, &vpd_data[rodi], kw_len);

> +			bp->fw_ver[kw_len] = ' ';

>  		}

>  	}

>  out_not_found:

> -- 

> 2.33.0

> 

>
Heiner Kallweit Aug. 24, 2021, 6:01 p.m. UTC | #2 
On 24.08.2021 19:02, Bjorn Helgaas wrote:
> On Sun, Aug 22, 2021 at 03:54:23PM +0200, Heiner Kallweit wrote:

>> Use pci_vpd_find_ro_info_keyword() to search for keywords in VPD to

>> simplify the code.

>>

>> str_id_reg and str_id_cap hold the same string and are used in the same

>> comparison. This doesn't make sense, use one string str_id instead.

> 

> str_id_reg is printed with "%04x" (lower-case hex letters) and

> str_id_cap with "%04X" (upper-case hex letters), so the previous code

> would match either 0xabcd or 0xABCD.  After this patch, we'd match

> only the latter.

> 

Right, I missed this difference. strncasecmp() would be an easy solution.
Alternatively we could avoid this stringification and string comparison
by using kstrtouint_from_user():

kstrtouint_from_user(&vpd_data[rodi], kw_len, 16, &val)
if (val == PCI_VENDOR_ID_DELL)

But if there's no strong preference then I'd say we go the easy way.
Would you like me to re-send or are you going to adjust the patch?

> PCI_VENDOR_ID_DELL is 0x1028, so it shouldn't make any difference,

> which makes me wonder why somebody bothered with both.

> 

> But it does seem like a potential landmine to change the case

> sensitivity.  Maybe strncasecmp() instead?

> 

>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>

>> ---

>>  .../net/ethernet/broadcom/bnx2x/bnx2x_main.c  | 57 +++++--------------

>>  1 file changed, 15 insertions(+), 42 deletions(-)

>>

>> diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c

>> index 0466adf8d..2c7bfc416 100644

>> --- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c

>> +++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c

>> @@ -12189,11 +12189,10 @@ static int bnx2x_get_hwinfo(struct bnx2x *bp)

>>  

>>  static void bnx2x_read_fwinfo(struct bnx2x *bp)

>>  {

>> -	int i, block_end, rodi;

>> -	char str_id_reg[VENDOR_ID_LEN+1];

>> -	char str_id_cap[VENDOR_ID_LEN+1];

>> -	unsigned int vpd_len;

>> -	u8 *vpd_data, len;

>> +	char str_id[VENDOR_ID_LEN + 1];

>> +	unsigned int vpd_len, kw_len;

>> +	u8 *vpd_data;

>> +	int rodi;

>>  

>>  	memset(bp->fw_ver, 0, sizeof(bp->fw_ver));

>>  

>> @@ -12201,46 +12200,20 @@ static void bnx2x_read_fwinfo(struct bnx2x *bp)

>>  	if (IS_ERR(vpd_data))

>>  		return;

>>  

>> -	/* VPD RO tag should be first tag after identifier string, hence

>> -	 * we should be able to find it in first BNX2X_VPD_LEN chars

>> -	 */

>> -	i = pci_vpd_find_tag(vpd_data, vpd_len, PCI_VPD_LRDT_RO_DATA);

>> -	if (i < 0)

>> -		goto out_not_found;

>> -

>> -	block_end = i + PCI_VPD_LRDT_TAG_SIZE +

>> -		    pci_vpd_lrdt_size(&vpd_data[i]);

>> -	i += PCI_VPD_LRDT_TAG_SIZE;

>> -

>> -	rodi = pci_vpd_find_info_keyword(vpd_data, i, block_end,

>> -				   PCI_VPD_RO_KEYWORD_MFR_ID);

>> -	if (rodi < 0)

>> -		goto out_not_found;

>> -

>> -	len = pci_vpd_info_field_size(&vpd_data[rodi]);

>> -

>> -	if (len != VENDOR_ID_LEN)

>> +	rodi = pci_vpd_find_ro_info_keyword(vpd_data, vpd_len,

>> +					    PCI_VPD_RO_KEYWORD_MFR_ID, &kw_len);

>> +	if (rodi < 0 || kw_len != VENDOR_ID_LEN)

>>  		goto out_not_found;

>>  

>> -	rodi += PCI_VPD_INFO_FLD_HDR_SIZE;

>> -

>>  	/* vendor specific info */

>> -	snprintf(str_id_reg, VENDOR_ID_LEN + 1, "%04x", PCI_VENDOR_ID_DELL);

>> -	snprintf(str_id_cap, VENDOR_ID_LEN + 1, "%04X", PCI_VENDOR_ID_DELL);

>> -	if (!strncmp(str_id_reg, &vpd_data[rodi], VENDOR_ID_LEN) ||

>> -	    !strncmp(str_id_cap, &vpd_data[rodi], VENDOR_ID_LEN)) {

>> -

>> -		rodi = pci_vpd_find_info_keyword(vpd_data, i, block_end,

>> -						PCI_VPD_RO_KEYWORD_VENDOR0);

>> -		if (rodi >= 0) {

>> -			len = pci_vpd_info_field_size(&vpd_data[rodi]);

>> -

>> -			rodi += PCI_VPD_INFO_FLD_HDR_SIZE;

>> -

>> -			if (len < 32 && (len + rodi) <= vpd_len) {

>> -				memcpy(bp->fw_ver, &vpd_data[rodi], len);

>> -				bp->fw_ver[len] = ' ';

>> -			}

>> +	snprintf(str_id, VENDOR_ID_LEN + 1, "%04X", PCI_VENDOR_ID_DELL);

>> +	if (!strncmp(str_id, &vpd_data[rodi], VENDOR_ID_LEN)) {

>> +		rodi = pci_vpd_find_ro_info_keyword(vpd_data, vpd_len,

>> +						    PCI_VPD_RO_KEYWORD_VENDOR0,

>> +						    &kw_len);

>> +		if (rodi >= 0 && kw_len < sizeof(bp->fw_ver)) {

>> +			memcpy(bp->fw_ver, &vpd_data[rodi], kw_len);

>> +			bp->fw_ver[kw_len] = ' ';

>>  		}

>>  	}

>>  out_not_found:

>> -- 

>> 2.33.0

>>

>>
Bjorn Helgaas Aug. 24, 2021, 6:47 p.m. UTC | #3 
On Tue, Aug 24, 2021 at 08:01:34PM +0200, Heiner Kallweit wrote:
> On 24.08.2021 19:02, Bjorn Helgaas wrote:

> > On Sun, Aug 22, 2021 at 03:54:23PM +0200, Heiner Kallweit wrote:

> >> Use pci_vpd_find_ro_info_keyword() to search for keywords in VPD to

> >> simplify the code.

> >>

> >> str_id_reg and str_id_cap hold the same string and are used in the same

> >> comparison. This doesn't make sense, use one string str_id instead.

> > 

> > str_id_reg is printed with "%04x" (lower-case hex letters) and

> > str_id_cap with "%04X" (upper-case hex letters), so the previous code

> > would match either 0xabcd or 0xABCD.  After this patch, we'd match

> > only the latter.

> > 

> Right, I missed this difference. strncasecmp() would be an easy solution.

> Alternatively we could avoid this stringification and string comparison

> by using kstrtouint_from_user():

> 

> kstrtouint_from_user(&vpd_data[rodi], kw_len, 16, &val)

> if (val == PCI_VENDOR_ID_DELL)

> 

> But if there's no strong preference then I'd say we go the easy way.

> Would you like me to re-send or are you going to adjust the patch?


I adjusted it, thanks!

> > PCI_VENDOR_ID_DELL is 0x1028, so it shouldn't make any difference,

> > which makes me wonder why somebody bothered with both.

> > 

> > But it does seem like a potential landmine to change the case

> > sensitivity.  Maybe strncasecmp() instead?

> > 

> >> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>

> >> ---

> >>  .../net/ethernet/broadcom/bnx2x/bnx2x_main.c  | 57 +++++--------------

> >>  1 file changed, 15 insertions(+), 42 deletions(-)

> >>

> >> diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c

> >> index 0466adf8d..2c7bfc416 100644

> >> --- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c

> >> +++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c

> >> @@ -12189,11 +12189,10 @@ static int bnx2x_get_hwinfo(struct bnx2x *bp)

> >>  

> >>  static void bnx2x_read_fwinfo(struct bnx2x *bp)

> >>  {

> >> -	int i, block_end, rodi;

> >> -	char str_id_reg[VENDOR_ID_LEN+1];

> >> -	char str_id_cap[VENDOR_ID_LEN+1];

> >> -	unsigned int vpd_len;

> >> -	u8 *vpd_data, len;

> >> +	char str_id[VENDOR_ID_LEN + 1];

> >> +	unsigned int vpd_len, kw_len;

> >> +	u8 *vpd_data;

> >> +	int rodi;

> >>  

> >>  	memset(bp->fw_ver, 0, sizeof(bp->fw_ver));

> >>  

> >> @@ -12201,46 +12200,20 @@ static void bnx2x_read_fwinfo(struct bnx2x *bp)

> >>  	if (IS_ERR(vpd_data))

> >>  		return;

> >>  

> >> -	/* VPD RO tag should be first tag after identifier string, hence

> >> -	 * we should be able to find it in first BNX2X_VPD_LEN chars

> >> -	 */

> >> -	i = pci_vpd_find_tag(vpd_data, vpd_len, PCI_VPD_LRDT_RO_DATA);

> >> -	if (i < 0)

> >> -		goto out_not_found;

> >> -

> >> -	block_end = i + PCI_VPD_LRDT_TAG_SIZE +

> >> -		    pci_vpd_lrdt_size(&vpd_data[i]);

> >> -	i += PCI_VPD_LRDT_TAG_SIZE;

> >> -

> >> -	rodi = pci_vpd_find_info_keyword(vpd_data, i, block_end,

> >> -				   PCI_VPD_RO_KEYWORD_MFR_ID);

> >> -	if (rodi < 0)

> >> -		goto out_not_found;

> >> -

> >> -	len = pci_vpd_info_field_size(&vpd_data[rodi]);

> >> -

> >> -	if (len != VENDOR_ID_LEN)

> >> +	rodi = pci_vpd_find_ro_info_keyword(vpd_data, vpd_len,

> >> +					    PCI_VPD_RO_KEYWORD_MFR_ID, &kw_len);

> >> +	if (rodi < 0 || kw_len != VENDOR_ID_LEN)

> >>  		goto out_not_found;

> >>  

> >> -	rodi += PCI_VPD_INFO_FLD_HDR_SIZE;

> >> -

> >>  	/* vendor specific info */

> >> -	snprintf(str_id_reg, VENDOR_ID_LEN + 1, "%04x", PCI_VENDOR_ID_DELL);

> >> -	snprintf(str_id_cap, VENDOR_ID_LEN + 1, "%04X", PCI_VENDOR_ID_DELL);

> >> -	if (!strncmp(str_id_reg, &vpd_data[rodi], VENDOR_ID_LEN) ||

> >> -	    !strncmp(str_id_cap, &vpd_data[rodi], VENDOR_ID_LEN)) {

> >> -

> >> -		rodi = pci_vpd_find_info_keyword(vpd_data, i, block_end,

> >> -						PCI_VPD_RO_KEYWORD_VENDOR0);

> >> -		if (rodi >= 0) {

> >> -			len = pci_vpd_info_field_size(&vpd_data[rodi]);

> >> -

> >> -			rodi += PCI_VPD_INFO_FLD_HDR_SIZE;

> >> -

> >> -			if (len < 32 && (len + rodi) <= vpd_len) {

> >> -				memcpy(bp->fw_ver, &vpd_data[rodi], len);

> >> -				bp->fw_ver[len] = ' ';

> >> -			}

> >> +	snprintf(str_id, VENDOR_ID_LEN + 1, "%04X", PCI_VENDOR_ID_DELL);

> >> +	if (!strncmp(str_id, &vpd_data[rodi], VENDOR_ID_LEN)) {

> >> +		rodi = pci_vpd_find_ro_info_keyword(vpd_data, vpd_len,

> >> +						    PCI_VPD_RO_KEYWORD_VENDOR0,

> >> +						    &kw_len);

> >> +		if (rodi >= 0 && kw_len < sizeof(bp->fw_ver)) {

> >> +			memcpy(bp->fw_ver, &vpd_data[rodi], kw_len);

> >> +			bp->fw_ver[kw_len] = ' ';

> >>  		}

> >>  	}

> >>  out_not_found:

> >> -- 

> >> 2.33.0

> >>

> >>

>
diff mbox series

Patch

diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
index 0466adf8d..2c7bfc416 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
@@ -12189,11 +12189,10 @@  static int bnx2x_get_hwinfo(struct bnx2x *bp)
 
 static void bnx2x_read_fwinfo(struct bnx2x *bp)
 {
-	int i, block_end, rodi;
-	char str_id_reg[VENDOR_ID_LEN+1];
-	char str_id_cap[VENDOR_ID_LEN+1];
-	unsigned int vpd_len;
-	u8 *vpd_data, len;
+	char str_id[VENDOR_ID_LEN + 1];
+	unsigned int vpd_len, kw_len;
+	u8 *vpd_data;
+	int rodi;
 
 	memset(bp->fw_ver, 0, sizeof(bp->fw_ver));
 
@@ -12201,46 +12200,20 @@  static void bnx2x_read_fwinfo(struct bnx2x *bp)
 	if (IS_ERR(vpd_data))
 		return;
 
-	/* VPD RO tag should be first tag after identifier string, hence
-	 * we should be able to find it in first BNX2X_VPD_LEN chars
-	 */
-	i = pci_vpd_find_tag(vpd_data, vpd_len, PCI_VPD_LRDT_RO_DATA);
-	if (i < 0)
-		goto out_not_found;
-
-	block_end = i + PCI_VPD_LRDT_TAG_SIZE +
-		    pci_vpd_lrdt_size(&vpd_data[i]);
-	i += PCI_VPD_LRDT_TAG_SIZE;
-
-	rodi = pci_vpd_find_info_keyword(vpd_data, i, block_end,
-				   PCI_VPD_RO_KEYWORD_MFR_ID);
-	if (rodi < 0)
-		goto out_not_found;
-
-	len = pci_vpd_info_field_size(&vpd_data[rodi]);
-
-	if (len != VENDOR_ID_LEN)
+	rodi = pci_vpd_find_ro_info_keyword(vpd_data, vpd_len,
+					    PCI_VPD_RO_KEYWORD_MFR_ID, &kw_len);
+	if (rodi < 0 || kw_len != VENDOR_ID_LEN)
 		goto out_not_found;
 
-	rodi += PCI_VPD_INFO_FLD_HDR_SIZE;
-
 	/* vendor specific info */
-	snprintf(str_id_reg, VENDOR_ID_LEN + 1, "%04x", PCI_VENDOR_ID_DELL);
-	snprintf(str_id_cap, VENDOR_ID_LEN + 1, "%04X", PCI_VENDOR_ID_DELL);
-	if (!strncmp(str_id_reg, &vpd_data[rodi], VENDOR_ID_LEN) ||
-	    !strncmp(str_id_cap, &vpd_data[rodi], VENDOR_ID_LEN)) {
-
-		rodi = pci_vpd_find_info_keyword(vpd_data, i, block_end,
-						PCI_VPD_RO_KEYWORD_VENDOR0);
-		if (rodi >= 0) {
-			len = pci_vpd_info_field_size(&vpd_data[rodi]);
-
-			rodi += PCI_VPD_INFO_FLD_HDR_SIZE;
-
-			if (len < 32 && (len + rodi) <= vpd_len) {
-				memcpy(bp->fw_ver, &vpd_data[rodi], len);
-				bp->fw_ver[len] = ' ';
-			}
+	snprintf(str_id, VENDOR_ID_LEN + 1, "%04X", PCI_VENDOR_ID_DELL);
+	if (!strncmp(str_id, &vpd_data[rodi], VENDOR_ID_LEN)) {
+		rodi = pci_vpd_find_ro_info_keyword(vpd_data, vpd_len,
+						    PCI_VPD_RO_KEYWORD_VENDOR0,
+						    &kw_len);
+		if (rodi >= 0 && kw_len < sizeof(bp->fw_ver)) {
+			memcpy(bp->fw_ver, &vpd_data[rodi], kw_len);
+			bp->fw_ver[kw_len] = ' ';
 		}
 	}
 out_not_found: