Hi Gabriel. Thanks for the patch! A couple of comments below.

On 06/10/2015 02:34 PM, Gabriel Krisman Bertazi wrote:
> show-ucode-levels prints all IPR adapters and devices, along with their
> current firmware levels as well as the latest available version that could
> be found on the filesystem.
> 
> Signed-off-by: Gabriel Krisman Bertazi <kris...@linux.vnet.ibm.com>
> ---
>  iprconfig.8 |   5 +++
>  iprconfig.c | 108 
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 113 insertions(+)
> 
> diff --git a/iprconfig.8 b/iprconfig.8
> index 2282fac..65cf1a1 100644
> --- a/iprconfig.8
> +++ b/iprconfig.8
> @@ -337,6 +337,11 @@ Show the microcode level that is currently loaded on the 
> specified device.
>  Note: The device specified may be the sg device associated with an IOA,
>  in which case the IOA's microcode level will be shown.
>  .TP
> +.B show-ucode-levels
> +.br
> +Show the microcode level that is currently loaded for every device and
> +adapter in the system
> +.TP
>  .B query-format-timeout [device]
>  .br
>  Show the current format timeout to be used when formatting the specified 
> disk.
> diff --git a/iprconfig.c b/iprconfig.c
> index 9e034ee..c8e0326 100644
> --- a/iprconfig.c
> +++ b/iprconfig.c
> @@ -15019,6 +15019,113 @@ static int update_ucode_cmd(char **args, int 
> num_args)
>  }
> 
>  /**
> + * ucode_level_str - format version string of microcode of a device
> + *
> + * @dev:             Device
> + * @buff:            Output buffer
> + * @buffsiz:         Buffer size
> + *
> + * Returns:
> + *   0 if success / non-zero on failure
> + **/
> +int ucode_level_str(struct ipr_dev *dev, char *buff, size_t bufsiz)
> +{
> +     u32 level, level_sw;
> +     char *asc;
> +
> +     if (!dev)
> +             return -EINVAL;
> +
> +     if (&dev->ioa->ioa == dev)
> +             snprintf(buff, bufsiz, "%08X",
> +                      get_ioa_fw_version(dev->ioa));
> +     else {
> +             level = get_dev_fw_version(dev);
> +             level_sw = htonl(level);
> +             asc = (char *)&level_sw;
> +             if (isprint(asc[0]) && isprint(asc[1]) &&
> +                 isprint(asc[2]) && isprint(asc[3]))
> +                     snprintf(buff, bufsiz, "%.8X (%c%c%c%c)",
> +                             level, asc[0], asc[1], asc[2], asc[3]);
> +             else
> +                     snprintf(buff, bufsiz, "%.8X", level);
> +     }
> +     return 0;
> +}
> +
> +/**
> + * search_latest_ucode_str - format version string of the latest
> + * microcode imagem found in the filesystem.
> + *
> + * @dev:             Device
> + * @buff:            Output buffer
> + * @buffsiz:         Buffer size
> + *
> + * Returns:
> + *   0 if success / non-zero on failure
> + **/
> +static int search_latest_ucode_str(struct ipr_dev *dev, char *buff,
> +                                size_t bufsiz)
> +{
> +     struct ipr_fw_images *fw = NULL;
> +     u32 version = 0;
> +     buff[0] = '\0';
> +
> +     if (!dev)
> +             return -ENODEV;
> +
> +     if (dev->scsi_dev_data->type == IPR_TYPE_ADAPTER)
> +             get_ioa_firmware_image_list(dev->ioa, &fw);
> +     else if (ipr_is_ses(dev))
> +             get_ses_firmware_image_list(dev, &fw);
> +     else if (ipr_is_gscsi(dev) || ipr_is_af_dasd_device(dev))
> +             get_dasd_firmware_image_list(dev, &fw);
> +
> +     if (!fw)
> +             return -EINVAL;
> +
> +     snprintf(buff, bufsiz, "%X (%s)", fw->version, fw->file);
> +
> +     free(fw);
> +     return 0;
> +}
> +
> +/**
> + * show_ucode_levels - List microcode level of every device and adapter
> + * @args:            argument vector
> + * @num_args:                number of arguments
> + *
> + * Returns:
> + *   0 if success / non-zero on failure
> + **/
> +static int show_ucode_levels(char **args, int num_args)
> +{
> +     struct ipr_ioa *ioa;
> +     struct ipr_dev *dev;
> +     size_t buflen = 32;
> +     char fwb[buflen];
> +     char lfwb[PATH_MAX];
> +
> +     printf("Name   Microcode Level  Latest Microcode Available\n"
> +            "------ ---------------- 
> ------------------------------------\n");

I'd like to see a little more device identification data for each device 
listed. We
probably don't need to list the full filename of the microcode image. It would 
also
be nice if we had a flag to indicate whether the firmware was up to date or not.
How about something like this:

Name   PCI/SCSI Location          Vendor   Product ID       Current  Available 
------ -------------------------  -------- ---------------- -------- ---------
sg0    0007:01:00.0/2:            IBM      57B4001SISIOA    12511700 12511701*
sg1    0007:01:00.0/2:0:0:0       IBM      HUC106030CSS600  41333236 41333236
sg2    0007:01:00.0/2:0:1:0       IBM      HUSMM1640ASS201  42303730 42303730
sdb    0007:01:00.0/2:0:2:0       IBM      HUC101860CS420   4E373230


> +
> +     for_each_ioa(ioa) {
> +             for_each_dev(ioa, dev) {
> +                     if (ipr_is_volume_set(dev))
> +                             continue;
> +
> +                     ucode_level_str(dev, fwb, buflen);
> +                     search_latest_ucode_str(dev, lfwb, PATH_MAX);
> +
> +                     printf("%-6s %-16s %s\n", basename(dev->gen_name),
> +                            fwb, lfwb);

I'd prefer to use dev->dev_name if its a non zero string, as that is more 
useful for JBOD devices.

> +             }
> +     }
> +
> +     return 0;
> +}
> +
> +/**
>   * disrupt_device_cmd -
>   * @args:            argument vector
>   * @num_args:                number of arguments
> @@ -18395,6 +18502,7 @@ static const struct {
>       { "raid-rebuild",                       1, 0, 1, raid_rebuild_cmd, 
> "sg6" },
>       { "disrupt-device",                     1, 0, 1, disrupt_device_cmd, 
> "sg6" },
>       { "update-ucode",                       2, 0, 2, update_ucode_cmd, "sg5 
> /root/ucode.bin" },
> +     { "show-ucode-levels",                  0, 0, 0, show_ucode_levels, "" 
> },
>       { "set-format-timeout",                 2, 0, 2, set_format_timeout, 
> "sg6 4" },
>       { "set-qdepth",                         2, 0, 2, set_qdepth, "sda 16" },
>       { "set-tcq-enable",                     2, 0, 2, set_tcq_enable, "sda 
> 0" },
> 


-- 
Brian King
Power Linux I/O
IBM Linux Technology Center



------------------------------------------------------------------------------
_______________________________________________
Iprdd-devel mailing list
Iprdd-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/iprdd-devel

Reply via email to