Re: [PATCH]mpt fusion: add sysfs attributes to display IOC parameters

[Brian King <brking@linux.vnet.ibm.com>]

Prakash, Sathya wrote: 
> +static void
> +mpt_get_manufacturing_pg_0(MPT_ADAPTER *ioc)
> +{

> +
> +	memcpy(ioc->board_name, pbuf->BoardName, 16);
> +	memcpy(ioc->board_assembly, pbuf->BoardAssembly, 16);
> +	memcpy(ioc->board_tracer, pbuf->BoardTracerNumber, 16);

Are these guaranteed to be NULL terminated? The code below appears to
assume they are. Also you might want to use sizeof instead of hardcoding
16 here.

> +static ssize_t
> +mptscsih_version_driver_show(struct class_device *cdev, char *buf)
> +{
> +	return snprintf(buf, PAGE_SIZE, "%02d.%02d.%02d.%02d\n",
> +	    MPT_LINUX_MAJOR_VERSION, MPT_LINUX_MINOR_VERSION,
> +	    MPT_LINUX_BUILD_VERSION, MPT_LINUX_RELEASE_VERSION);
> +}
> +static CLASS_DEVICE_ATTR(version_driver, S_IRUGO, mptscsih_version_driver_show, NULL);

You should be using the MODULE_VERSION macro instead of this. This results
in the version showing up in /sys/module/mpt_fusion/version

> +
> +static ssize_t
> +mptscsih_io_delay_show(struct class_device *cdev, char *buf)
> +{
> +	struct Scsi_Host *host = class_to_shost(cdev);
> +	MPT_SCSI_HOST	*hd = (MPT_SCSI_HOST *)host->hostdata;
> +	MPT_ADAPTER *ioc = hd->ioc;
> +	char delay_str[30];
> +
> +	sprintf(delay_str, "%02d", ioc->io_missing_delay);
> +	return snprintf(buf, PAGE_SIZE, "%s\n", delay_str);
> +}

Why not just do:

	return snprintf(buf, PAGE_SIZE, "%02d", ioc->io_missing_delay);

and get rid of the delay_str on the stack? In fact you have a lot
of double sprintf's which seem like they could be just a single snprintf.


-Brian

-- 
Brian King
Linux on Power Virtualization
IBM Linux Technology Center