Re: [PATCH 1/3] lpd: Add support for Marvell HDD LEDs on S822LC for HPC

On 10/21/2016 07:31 PM, Mauricio Faria de Oliveira wrote:
> This patch introduces support for the identification/fault LEDs of the
> Marvell SATA HDD controller present in the IBM Power System S822LC for
> HPC (a.k.a. Garrison/Minsky).
>

Overall patch looks good. You have very good inline comments which helps to 
understand the code.

Few minor comments. see below.

.../...

 >   lpd/Makefile         |   2 +-
 >   lpd/indicator.h      |   3 +-
 >   lpd/indicator_mv.c   | 594   +++++++++++++++++++++++++++++++++++++++
 >   lpd/indicator_mv.h   |  27 +++

Somehow I didn't like filename. Moment I see mv I feel its move :-)
Can we change it to something like mrv (or some other name) ?



.../...

> +/*
> + * For debugging messages, export this environment variable.
> + */
> +#define mv_dbg(fmt, args...) if (getenv("USYSIDENT_MARVELL_DEBUG")) printf(fmt, ##args)

We have "LPD_DEBUG" for debug lpd code. Why not use same macro here?

> +/*
> + * Vendor-Specific Registers (VSR) are accessed indirectly
> + * through a pair of registers in PCI BAR5.
> + *
> + * First, set the VSR address in VSR_ADDR;
> + * Then, read/write data from/to VSR_DATA.
> + */
> +static const uint32_t MV_PCI_BAR5_VSR_ADDR = 0xa8;

Why not macros for all the variables?
Also personally capital letter in variable name makes me to feel that I'm 
reading non C code ;-)


.../...

> +static void
> +mv_munmap_bar5(void *bar5, int fd)
> +{
> +	/* munmap() and close PCI BAR5 */
> +	if (munmap(bar5, MV_PCI_BAR5_LENGTH)) {
> +		log_msg("Unable to munmap file");

I think its better to continue and try to close fd.


> +		return;
> +	}
> +
> +	if (close(fd)) {
> +		log_msg("Unable to close file");
> +		return;
> +	}
> +}
> +

.../...

> +
> +/*
> + * This function is currently not required with the default value of
> + * the PORT_ACTIVE_SEL register (see header comments); however, keep
> + * it in case it's needed on hardware support expansion.
> + */
> +#if 0

Do you really want to keep unused code?

> +/**
> + * mv_set_port_mode - set the LED mode of a SATA port (on register copy)
> + *
> + * @port_number		number of the SATA port (0-9)
> + * @port_mode		value of the LED mode (0-7) (see MV_PORT_MODE[])
> + * @port_active_sel	pointer to copy of the PORT_ACTIVE_SEL register (result)
> + *
> + * Return:
> + * - -1 on failure
> + * -  0 on success
> + */
> +static int
> +mv_set_port_mode(uint32_t port_number, uint32_t port_mode,
> +		 uint32_t *port_active_sel)
> +{
> +	uint32_t shift, mask;
> +
> +	if (port_number > MV_PORT_NUMBER_MAX) {
> +		log_msg("Invalid port number: '%u'", port_number);
> +		return -1;
> +	}
> +
> +	if (port_mode > MV_PORT_MODE_MAX) {
> +		log_msg("Invalid port mode: '%u'", port_mode);
> +		return -1;
> +	}
> +
> +	/* set the 3-bit mode of SATA port N */
> +	shift = 3 * port_number;
> +	mask = 0x7 << shift;
> +
> +	/* clear then set the 3 bits */
> +	*port_active_sel &= ~mask;
> +	*port_active_sel |= port_mode << shift;
> +
> +	return 0;
> +}
> +#endif
> +


.../...

> +/**
> + * mv_indicator_list - Build Marvell HDD LED (indicator) list for the given disk vpd
> + *
> + * @list	loc_code structure
> + * @vpd		dev_vpd structure
> + *
> + * Return:
> + * -  0 on success or skip vpd (not a Marvell SATA disk)
> + * - -1 on failure
> + */
> +static int
> +mv_indicator_list(struct loc_code **list, struct dev_vpd *vpd)
> +{
> +	struct	loc_code *list_curr, *list_new;
> +	char	path[PATH_MAX], symlink[PATH_MAX];
> +	char	*ata_device;
> +	ssize_t	len;
> +
> +	/* check for an 'sdX' device name */
> +	if (strncmp(vpd->dev, "sd", 2))
> +		return 0;
> +
> +	/* read the '/sys/block/sdX' symlink to '../device/pci.../sdX' */
> +	snprintf(path, PATH_MAX, "%s/%s", SYS_BLOCK_PATH, vpd->dev);
> +
> +	len = readlink(path, symlink, PATH_MAX);
> +	if (len < 0) {
> +		log_msg("Unable to read the contents of symbolic link '%s'", path);
> +		return -1;
> +	}
> +	symlink[len] = '\0';
> +
> +	/*
> +	 * check for an 'ataX' subdir (w/ trailing 'X' digit); for example 'ata1' in

Are you sure  'X' >= 0 && X <= 9 is true always? Else you have to fix below logic.


> +	 * '../devices/pci<...>/0001:08:00.0/ata1/host3/target3:0:0/3:0:0:0/block/sdj'
> +	 */
> +
> +	ata_device = strstr(symlink, "/ata");
> +	if (!ata_device || !isdigit(ata_device[4]))
> +		return 0;
> +
> +	/* split symlink into relative path for PCI device dir and ataX device */
> +	ata_device[0] = '\0';	/* end symlink on leading '/' of '/ataX/' */
> +	ata_device[5] = '\0';	/* end ata_device on trailing '/' of '/ataX/' */

Or may be better to use another strstr here rather than assuming 'ataX' length?

> +	ata_device++;		/* skip the leading '/' of '/ataX/' */
> +
> +	/*
> +	 * get the absolute path for the PCI device dir from symlink,
> +	 * which is relative to /sys/block (e.g., '../devices/pci...'),
> +	 * so skip the leading '../' (3 chars)
> +	 */
> +	len = snprintf(path, PATH_MAX, "%s/%s", "/sys", &symlink[3]);
> +	if (len < 0) {
> +		log_msg("Unable to format absolute pathname of '%s'", symlink);
> +		return -1;
> +	}
> +
> +	/*
> +	 * check for the PCI IDs of the Marvell 9235 SATA controller.
> +	 *
> +	 * concatenate the PCI ID files' basename after the dirname
> +	 * with strncpy() (string length + 1 for '\0').
> +	 */
> +	strncpy(&path[len], "/vendor", 8);
> +	if (check_pci_id(path, MV_PCI_VID))
> +		return 0;
> +
> +	strncpy(&path[len], "/device", 8);
> +	if (check_pci_id(path, MV_PCI_DID))
> +		return 0;
> +
> +	strncpy(&path[len], "/subsystem_vendor", 18);
> +	if (check_pci_id(path, MV_PCI_SVID))
> +		return 0;
> +
> +	strncpy(&path[len], "/subsystem_device", 18);
> +	if (check_pci_id(path, MV_PCI_SSID))
> +		return 0;
> +
> +	path[len] = '\0'; /* restore path as dirname of PCI device dir */
> +
> +	/* Allocate one loc_code element, and insert/append it to the list */
> +	list_new = calloc(1, sizeof(struct loc_code));
> +	if (!list_new) {
> +		log_msg("Out of memory");
> +		return 1;
> +	}
> +
> +	if (*list) {
> +		/* position list_curr in the last element of the list */
> +		list_curr = *list;
> +		while (list_curr->next)
> +			list_curr = list_curr->next;
> +
> +		/* append the new element to the list */
> +		list_curr->next = list_new;
> +	} else {
> +		/* null list; insert the new element at the list head */
> +		*list = list_new;
> +	}
> +
> +	/* set the new element's properties */
> +	list_new->type = TYPE_MARVELL;
> +	strncpy(list_new->code, vpd->location, LOCATION_LENGTH);  /* loc. code */
> +	strncpy(list_new->devname, vpd->dev, DEV_LENGTH);   /* sdX device name */
> +	snprintf(list_new->dev, DEV_LENGTH, "%s/resource5", path); /* PCI BAR5 */
> +	list_new->index = (uint32_t)atoi(&ata_device[3]); /* use for ATA index */
> +
> +	mv_dbg("Marvell HDD LED:\n");
> +	mv_dbg("- location code: '%s'\n", list_new->code);
> +	mv_dbg("- device name (disk): '%s'\n", list_new->devname);
> +	mv_dbg("- device name (pci bar5): '%s'\n", list_new->dev);
> +	mv_dbg("- ata index: '%u'\n", list_new->index);
> +
> +	return 0;
> +}
> +
> +/**
> + * get_mv_indices - Get Marvell HDD LEDs (indicators) list
> + *
> + * @indicator	led type
> + * @loc_list	loc_code structure
> + *
> + * Return:
> + * - nothing
> + */
> +void
> +get_mv_indices(int indicator, struct loc_code **loc_list)
> +{
> +	struct	dev_vpd *vpd_list, *vpd_curr;
> +
> +	/* support for identification LEDs only */
> +	if (indicator != LED_TYPE_IDENT)

I assume it doesn't have fault indicator.

> +		return;
> +
> +	/* get block devices' vpd information */
> +        vpd_list = read_device_vpd(SYS_BLOCK_PATH);


This is fine for now as current code uses lsvpd family tools to get VPD data for 
given device.
But on long run (if possible) I'd like to get rid of lsvpd dependency.

By any chance do we have other way to get required data for HDD? You don't need 
to implement now .


> +	if (!vpd_list)
> +		return;
> +
> +	/* search for Marvell HDDs, and append to the list */
> +	for (vpd_curr = vpd_list; vpd_curr; vpd_curr = vpd_curr->next)
> +		if (vpd_curr->location[0] != '\0')
> +			if (mv_indicator_list(loc_list, vpd_curr))
> +				break;
> +
> +	free_device_vpd(vpd_list);
> +	return;

You don't need explicit return for void function.

-Vasant


------------------------------------------------------------------------------
Developer Access Program for Intel Xeon Phi Processors
Access to Intel Xeon Phi processor-based developer platforms.
With one year of Intel Parallel Studio XE.
Training and support from Colfax.
Order your platform today. http://sdm.link/xeonphi