RE: [PATCH V3 08/25] smartpqi: add support for long firmware version

[<Don.Brace@microchip.com>]

-----Original Message-----
From: Martin Wilck [mailto:mwilck@suse.com] 
Subject: Re: [PATCH V3 08/25] smartpqi: add support for long firmware version

> @@ -1405,7 +1405,7 @@ enum pqi_ctrl_mode {  struct 
> bmic_identify_controller {
>         u8      configured_logical_drive_count;
>         __le32  configuration_signature;
> -       u8      firmware_version[4];
> +       u8      firmware_version_short[4];
>         u8      reserved[145];
>         __le16  extended_logical_unit_count;
>         u8      reserved1[34];
> @@ -1413,11 +1413,17 @@ struct bmic_identify_controller {
>         u8      reserved2[8];
>         u8      vendor_id[8];
>         u8      product_id[16];
> -       u8      reserved3[68];
> +       u8      reserved3[62];
> +       __le32  extra_controller_flags;
> +       u8      reserved4[2];
>         u8      controller_mode;
> -       u8      reserved4[32];
> +       u8      spare_part_number[32];
> +       u8      firmware_version_long[32];
>  };
>
> --- a/drivers/scsi/smartpqi/smartpqi_init.c
> +++ b/drivers/scsi/smartpqi/smartpqi_init.c
> pqi_get_ctrl_product_details(struct pqi_ctrl_info *ctrl_info)
>         if (rc)
>                 goto out;
>
> +       if (get_unaligned_le32(&identify->extra_controller_flags) &
> +               BMIC_IDENTIFY_EXTRA_FLAGS_LONG_FW_VERSION_SUPPORTED)
> {
> +               memcpy(ctrl_info->firmware_version,
> +                       identify->firmware_version_long,
> +                       sizeof(identify->firmware_version_long));
> +       } else {
> +               memcpy(ctrl_info->firmware_version,
> +                       identify->firmware_version_short,
> +                       sizeof(identify->firmware_version_short));
> +               ctrl_info->firmware_version
> +                       [sizeof(identify->firmware_version_short)] =
> '\0';
> +               snprintf(ctrl_info->firmware_version +
> +                       strlen(ctrl_info->firmware_version),
> +                       sizeof(ctrl_info->firmware_version),

This looks wrong. I suppose a real overflow can't happen, but shouldn't it rather be written like this?

snprintf(ctrl_info->firmware_version +
 sizeof(identify->firmware_version_short),
 sizeof(ctrl_info->firmware_version)
 - sizeof(identify->firmware_version_short),
 "-u", ...)

> +                       "-%u",
> +                       get_unaligned_le16(&identify-
> > firmware_build_number));


Don: Agreed. Updated. 
Thanks for your revew.