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@xxxxxxxxx>
>> ---
>> .../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
>>
>>
