Discussion:
[Linux-diag-devel] [PATCH 0/3] Support for Marvell SATA HDD LEDs on IBM Power System S822LC for HPC
Mauricio Faria de Oliveira
2016-10-21 14:01:16 UTC
Permalink
This patch-set 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).

Patch 1/3 implements the 'indicator' interface required by lpd/usysident.
Patch 2/3 introduces one quirk for non-unique location code on the disks.
Patch 3/3 guarantees the usysident program runs without kernel LEDs when
there is Marvell HDD LEDs available.

It is written and verified on commit "92ea7c8 ppc64-diag v2.7.2 release".

Mauricio Faria de Oliveira (3):
lpd: Add support for Marvell HDD LEDs on S822LC for HPC
lpd: marvell: handle non-unique/duplicate location codes
lpd: marvell: do not exit early on empty /sys/class/leds dir (OPAL)

lpd/Makefile | 2 +-
lpd/indicator.h | 3 +-
lpd/indicator_mv.c | 594 +++++++++++++++++++++++++++++++++++++++++++++++++++
lpd/indicator_mv.h | 27 +++
lpd/indicator_opal.c | 21 ++
lpd/usysident.c | 48 ++++-
6 files changed, 692 insertions(+), 3 deletions(-)
create mode 100644 lpd/indicator_mv.c
create mode 100644 lpd/indicator_mv.h
--
1.8.3.1
Mauricio Faria de Oliveira
2016-10-21 14:01:17 UTC
Permalink
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).

indicator.h: add the indicator type 'TYPE_MARVELL'.
indicator_mv.{h,c}: implement the 'indicator' interface (get indices,
get/set indicator), and required hardware logic.
indicator_opal.c: call such interface on 'TYPE_MARVELL' devices.
Makefile.c: build and reference the indicator_mv.o file.

The code for the hardware logic is based on a sample code written by
Douglas Miller <***@us.ibm.com>, acknowledged in indicator_mv.c.

Signed-off-by: Mauricio Faria de Oliveira <***@linux.vnet.ibm.com>
---
lpd/Makefile | 2 +-
lpd/indicator.h | 3 +-
lpd/indicator_mv.c | 594 +++++++++++++++++++++++++++++++++++++++++++++++++++
lpd/indicator_mv.h | 27 +++
lpd/indicator_opal.c | 8 +
5 files changed, 632 insertions(+), 2 deletions(-)
create mode 100644 lpd/indicator_mv.c
create mode 100644 lpd/indicator_mv.h

diff --git a/lpd/Makefile b/lpd/Makefile
index 9390fd3..a85f752 100644
--- a/lpd/Makefile
+++ b/lpd/Makefile
@@ -9,7 +9,7 @@ CMDS = lp_diag usysident usysattn
COMMON_UTILS_OBJ = $(COMMON_DIR)/utils.o $(COMMON_DIR)/platform.o

LPD_COMMON_OBJS = files.o lp_util.o indicator_ses.o indicator_rtas.o \
- indicator_opal.o indicator.o
+ indicator_opal.o indicator_mv.o indicator.o
COMMON_OBJS = $(COMMON_UTILS_OBJ) $(LPD_COMMON_OBJS)

LP_DIAG_OBJS = servicelog.o lp_diag.o
diff --git a/lpd/indicator.h b/lpd/indicator.h
index 81ae98c..b8b9208 100644
--- a/lpd/indicator.h
+++ b/lpd/indicator.h
@@ -43,7 +43,8 @@
#define TYPE_SES 2
#define TYPE_OS 3
#define TYPE_OPAL 4
-#define TYPE_EOL 5
+#define TYPE_MARVELL 5
+#define TYPE_EOL 6

/* Indicator state */
#define LED_STATE_SAME -1
diff --git a/lpd/indicator_mv.c b/lpd/indicator_mv.c
new file mode 100644
index 0000000..eee653b
--- /dev/null
+++ b/lpd/indicator_mv.c
@@ -0,0 +1,594 @@
+/**
+ * @file indicator_mv.c
+ * @brief Marvell HDD LEDs (indicators) manipulation routines
+ *
+ * Copyright (C) 2016 IBM Corporation
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
+ *
+ * @author Mauricio Faria de Oliveira <***@linux.vnet.ibm.com>
+ * @author Douglas Miller <***@us.ibm.com>
+ */
+
+#include <ctype.h>
+#include <dirent.h>
+#include <errno.h>
+#include <fcntl.h>
+#include <limits.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <sys/mman.h>
+#include <sys/types.h>
+#include <sys/wait.h>
+#include <unistd.h>
+
+#include "indicator.h"
+#include "lp_util.h"
+#include "utils.h"
+
+/*
+ * For debugging messages, export this environment variable.
+ */
+#define mv_dbg(fmt, args...) if (getenv("USYSIDENT_MARVELL_DEBUG")) printf(fmt, ##args)
+
+/*
+ * Path to block devices in sysfs
+ */
+#define SYS_BLOCK_PATH "/sys/block"
+
+/*
+ * PCI IDs of the Marvell 9235 SATA controller in
+ * IBM Power System S822LC for HPC (Garrison/Minsky).
+ */
+#define MV_PCI_VID "0x1b4b"
+#define MV_PCI_DID "0x9235"
+#define MV_PCI_SVID "0x1014"
+#define MV_PCI_SSID "0x0612"
+#define MV_PCI_ID_BUF_LEN 7 /* "0x3456" and '\0' */
+
+/*
+ * 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;
+static const uint32_t MV_PCI_BAR5_VSR_DATA = 0xac;
+static const uint32_t MV_PCI_BAR5_LENGTH = 2048;
+
+/*
+ * The LEDs are controlled via GPIO pins or SATA link/activity status (default).
+ *
+ * This involves 3 VSRs:
+ * - The LED mode of a SATA port is set in the PORT_ACTIVE_SEL register.
+ * (either GPIO pin state or SATA link/activity status for all/one SATA port)
+ * - The GPIO pin state is set in the DATA_OUT register.
+ * - The GPIO pin state output is enabled in the DATA_OUT_ENABLE register.
+ *
+ * On Garrison/Minsky the identification LED of the SATA port 'N' is wired
+ * to the GPIO pin of the SATA port 'N + 2' (the max. number of disks is 2).
+ * The PORT_ACTIVE_SEL and DATA_OUT_ENABLE registers are set by default so
+ * that SATA ports 3 and 4 (that is, the identification LEDs of SATA ports
+ * 1 and 2) are GPIO-pin based, and only the DATA_OUT register needs to be
+ * modified to (un)light the identification LEDs (a.k.a. fault LEDs).
+ *
+ * Register information:
+ *
+ * GPIO Data Out:
+ * - 1 bit per GPIO pin (31-0)
+ * - default: 0x00000018 (all low, except for pins 3 and 4)
+ * - The LEDs are ON on low and OFF on high.
+ *
+ * GPIO Data Out Enable:
+ * - 1 bit per GPIO bin (31-0)
+ * - default: 0xffffffe7 (all high, except for pins 3 and 4)
+ * - active low (data output enabled on bit value 0)
+ *
+ * Port Active Enable (or Port GPIOx Active Select)
+ * - bits 31-30: reserved (zero)
+ * - bits 29-00: 10x 3-bit mode (SATA ports 9-0)
+ * - default: 0x2db6da8d
+ */
+static const uint32_t MV_GPIO_DATA_OUT = 0x07071020;
+static const uint32_t MV_GPIO_DATA_OUT_ENABLE = 0x07071024;
+static const uint32_t MV_GPIO_PORT_ACTIVE_SEL = 0x07071058;
+
+static const uint32_t MV_PORT_NUMBER_OFFSET = 2;
+static const uint32_t MV_PORT_NUMBER_MAX = 9;
+
+static const uint32_t MV_PORT_MODE_GPIO = 0x5;
+static const uint32_t MV_PORT_MODE_MAX = 0x7;
+
+static char *MV_PORT_MODE[] = {
+ "All SATAx LINK and ACT", // 0x0
+ "SATA0 LINK and ACT", // 0x1
+ "SATA1 LINK and ACT", // 0x2
+ "SATA2 LINK and ACT", // 0x3
+ "SATA3 LINK and ACT", // 0x4
+ "GPIO_DATA_OUT[n]", // 0x5
+ "Invalid (6)", // 0x6
+ "Invalid (7)" // 0x7
+};
+
+/**
+ * mv_mmap_bar5 - open() and mmap() a PCI BAR5 (resource5) file.
+ * the caller should unmap/close (see mv_munmap_bar5()).
+ *
+ * @path path to the 'resource5' file
+ * @fd pointer to a file descriptor
+ *
+ * Return:
+ * - success: pointer to the mmap()'ed PCI BAR5
+ * - failure: NULL
+ */
+static void *
+mv_mmap_bar5(const char* path, int *fd)
+{
+ void *bar5;
+
+ /* open and mmap PCI BAR5 */
+ *fd = open(path, O_RDWR);
+ if (*fd < 0) {
+ log_msg("Unable to open file '%s'", path);
+ return NULL;
+ }
+
+ bar5 = mmap(NULL, MV_PCI_BAR5_LENGTH, PROT_READ | PROT_WRITE,
+ MAP_SHARED, *fd, 0);
+ if (bar5 == MAP_FAILED) {
+ log_msg("Unable to mmap file '%s'", path);
+ close(*fd);
+ return NULL;
+ }
+
+ return bar5;
+}
+
+/**
+ * mv_munmap_bar5 - munmap() and close() a PCI BAR5 (resource5) file.
+ * (release resources from mv_mmap_bar5()).
+ *
+ * @bar5 pointer to the mmap()'ed PCI BAR5
+ * @fd file descriptor of the open()'ed/mmap()'ed PCI BAR5
+ *
+ * Return:
+ * - nothing
+ */
+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");
+ return;
+ }
+
+ if (close(fd)) {
+ log_msg("Unable to close file");
+ return;
+ }
+}
+
+/**
+ * mv_vsr_read - read a VSR
+ *
+ * @bar5 pointer to mmap()'ed PCI BAR5 (see mv_mmap_bar5())
+ * @vsr_addr VSR address to read from
+ *
+ * Return:
+ * - VSR data read
+ */
+static uint32_t
+mv_vsr_read(void* bar5, uint32_t vsr_addr)
+{
+ uint32_t *addr = (uint32_t *)(bar5 + MV_PCI_BAR5_VSR_ADDR);
+ uint32_t *data = (uint32_t *)(bar5 + MV_PCI_BAR5_VSR_DATA);
+
+ /* set address and read data */
+ *addr = vsr_addr;
+ return *data;
+}
+
+/**
+ * mv_vsr_write - write a VSR
+ *
+ * @bar5 pointer to mmap()'ed PCI BAR5 (see mv_mmap_bar5())
+ * @vsr_addr VSR address to write to
+ * @vsr_data VSR data to write
+ *
+ * Return:
+ * - nothing
+ */
+static void
+mv_vsr_write(void* bar5, uint32_t vsr_addr, uint32_t vsr_data)
+{
+ uint32_t *addr = (uint32_t *)(bar5 + MV_PCI_BAR5_VSR_ADDR);
+ uint32_t *data = (uint32_t *)(bar5 + MV_PCI_BAR5_VSR_DATA);
+
+ /* set address and write data */
+ *addr = vsr_addr;
+ *data = vsr_data;
+ return;
+}
+
+/**
+ * mv_get_port_mode - get the LED mode of a SATA port (from register copy)
+ *
+ * @port_number number of the SATA port (0-9)
+ * @port_mode pointer to the LED mode (result) (see MV_PORT_MODE[])
+ * @port_active_sel copy of the PORT_ACTIVE_SEL register
+ *
+ * Return:
+ * - -1 on failure
+ * - 0 on success
+ */
+static int
+mv_get_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;
+ }
+
+ /* get the 3-bit mode of SATA port N */
+ shift = 3 * port_number;
+ mask = 0x7 << shift;
+
+ *port_mode = (port_active_sel & mask) >> shift;
+
+ return 0;
+}
+
+/*
+ * 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
+/**
+ * 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
+
+/**
+ * check_pci_id() - checks the value of a PCI ID file.
+ *
+ * Return:
+ * - 0 on success (open/read file, and match value)
+ * - -1 on failure
+ * - +1 on different values
+ */
+static int
+check_pci_id(char *path, char *pci_id)
+{
+ FILE *f;
+ char *pci_id_rc, pci_id_buf[MV_PCI_ID_BUF_LEN];
+
+ f = fopen(path, "r");
+ if (!f) {
+ log_msg("Unable to open file '%s'", path);
+ return -1;
+ }
+
+ pci_id_rc = fgets(pci_id_buf, MV_PCI_ID_BUF_LEN, f);
+ if (!pci_id_rc) {
+ log_msg("Unable to read file '%s'", path);
+ fclose(f);
+ return -1;
+ }
+
+ fclose(f);
+ return !!strncmp(pci_id, pci_id_buf, MV_PCI_ID_BUF_LEN);
+}
+
+/**
+ * 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
+ * '../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/' */
+ 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)
+ return;
+
+ /* get block devices' vpd information */
+ vpd_list = read_device_vpd(SYS_BLOCK_PATH);
+ 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;
+}
+
+/**
+ * get_mv_indicator - get Marvell HDD LED/indicator state
+ *
+ * @loc loc_code structure
+ * @state return indicator state of given loc
+ *
+ * Return:
+ * - 0 on success
+ * - -1 on failure
+ */
+int
+get_mv_indicator(int indicator, struct loc_code *loc, int *state)
+{
+ int fd;
+ void *bar5;
+ uint32_t data_out, data_out_enable, active_sel;
+ uint32_t port_number, port_mode;
+
+ /* support for identification LEDs only */
+ if (indicator != LED_TYPE_IDENT)
+ return -1;
+
+ /* read registers from PCI BAR5 */
+ bar5 = mv_mmap_bar5(loc->dev, &fd);
+ if (!bar5)
+ return -1; /* log_msg() done */
+
+ /* for debugging, read the 3 VSRs; only DATA_OUT is required */
+ data_out = mv_vsr_read(bar5, MV_GPIO_DATA_OUT);
+ data_out_enable = mv_vsr_read(bar5, MV_GPIO_DATA_OUT_ENABLE);
+ active_sel = mv_vsr_read(bar5, MV_GPIO_PORT_ACTIVE_SEL);
+
+ /* for debugging, get the LED mode of the SATA port 'N + offset' */
+ port_number = loc->index + MV_PORT_NUMBER_OFFSET;
+
+ if (mv_get_port_mode(port_number, &port_mode, active_sel))
+ return -1; /* log_msg() done */
+
+ mv_dbg("SATA disk '%s'\n", (loc->devname) ? loc->devname : "(null)");
+ mv_dbg("Data Out: '0x%08x'\n", data_out);
+ mv_dbg("Data Out Enable: '0x%08x'\n", data_out_enable);
+ mv_dbg("Port Active Select: '0x%08x'\n", active_sel);
+ mv_dbg("ATA Index: '%u', Port Number: '%u', Port Mode: '%u' ('%s')\n",
+ loc->index, port_number, port_mode, MV_PORT_MODE[port_mode]);
+
+ /* LED state: ON on low (bit 0) and OFF on high (bit 1) */
+ *state = (data_out & (1 << port_number)) ? LED_STATE_OFF : LED_STATE_ON;
+
+ mv_munmap_bar5(bar5, fd);
+ return 0;
+}
+
+/**
+ * set_mv_indicator - set Marvell HDD LED/indicator state
+ *
+ * @loc loc_code structure
+ * @state value to update indicator to
+ *
+ * Return:
+ * - 0 on success
+ * - -1 on failure
+ */
+int
+set_mv_indicator(int indicator, struct loc_code *loc, int new_value)
+{
+ int fd;
+ void *bar5;
+ uint32_t data_out, port_number;
+
+ /* support for identification LEDs only */
+ if (indicator != LED_TYPE_IDENT)
+ return -1;
+
+ /* sanity check */
+ if (new_value != LED_STATE_ON && new_value != LED_STATE_OFF)
+ return -1;
+
+ /* read/write registers from/to PCI BAR5 */
+ bar5 = mv_mmap_bar5(loc->dev, &fd);
+ if (!bar5)
+ return -1; /* log_msg() done */
+
+ /*
+ * only changes to the DATA_OUT register are required with the default
+ * values for the DATA_OUT_ENABLE and PORT_ACTIVE_SEL registers on the
+ * Garrison/Minsky system (see header comments).
+ */
+ data_out = mv_vsr_read(bar5, MV_GPIO_DATA_OUT);
+
+ /* set the LED state of the SATA port 'N + offset' */
+ port_number = loc->index + MV_PORT_NUMBER_OFFSET;
+
+ /* set/clear the bit in the DATA_OUT register;
+ * LED state: ON on low (bit 0) and OFF on high (bit 1) */
+ if (new_value == LED_STATE_ON)
+ data_out &= ~(1 << port_number);
+ else
+ data_out |= 1 << port_number;
+
+ /* update the register */
+ mv_vsr_write(bar5, MV_GPIO_DATA_OUT, data_out);
+
+ /* for debugging, read the register again after writing */
+ mv_dbg("Data Out (written to): '0x%08x'\n", data_out);
+ data_out = mv_vsr_read(bar5, MV_GPIO_DATA_OUT);
+ mv_dbg("Data Out (read again): '0x%08x'\n", data_out);
+
+ mv_munmap_bar5(bar5, fd);
+ return 0;
+}
diff --git a/lpd/indicator_mv.h b/lpd/indicator_mv.h
new file mode 100644
index 0000000..1877905
--- /dev/null
+++ b/lpd/indicator_mv.h
@@ -0,0 +1,27 @@
+/**
+ * Copyright (C) 2016 IBM Corporation
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
+ */
+
+#ifndef INDICATOR_MV_H
+#define INDICATOR_MV_H
+
+/* Marvell HDD LEDs (indicators) support */
+extern void get_mv_indices(int, struct loc_code **);
+extern int get_mv_indicator(int, struct loc_code *, int *);
+extern int set_mv_indicator(int, struct loc_code *, int);
+
+#endif /* INDICATOR_MV_H */
diff --git a/lpd/indicator_opal.c b/lpd/indicator_opal.c
index 707bed0..31e3fd2 100644
--- a/lpd/indicator_opal.c
+++ b/lpd/indicator_opal.c
@@ -33,6 +33,7 @@

#include "indicator.h"
#include "indicator_ses.h"
+#include "indicator_mv.h"

/* LED directory */
#define OPAL_LED_SYSFS_PATH "/sys/class/leds"
@@ -400,6 +401,9 @@ opal_get_indicator_list(int indicator, struct loc_code **list)
/* SES indicators */
get_ses_indices(indicator, list);

+ /* Marvell HDD LEDs (indicators) */
+ get_mv_indices(indicator, list);
+
return rc;
}

@@ -421,6 +425,8 @@ opal_get_indicator_state(int indicator, struct loc_code *loc, int *state)
return opal_get_indicator(loc, state);
case TYPE_SES:
return get_ses_indicator(indicator, loc, state);
+ case TYPE_MARVELL:
+ return get_mv_indicator(indicator, loc, state);
default:
break;
}
@@ -449,6 +455,8 @@ opal_set_indicator_state(int indicator, struct loc_code *loc, int new_value)
return opal_set_indicator(loc, new_value);
case TYPE_SES:
return set_ses_indicator(indicator, loc, new_value);
+ case TYPE_MARVELL:
+ return set_mv_indicator(indicator, loc, new_value);
default:
break;
}
--
1.8.3.1
Vasant Hegde
2016-11-08 10:52:39 UTC
Permalink
Post by Mauricio Faria de Oliveira
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.

.../...
Post by Mauricio Faria de Oliveira
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) ?



.../...
Post by Mauricio Faria de Oliveira
+/*
+ * 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?
Post by Mauricio Faria de Oliveira
+/*
+ * 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 ;-)


.../...
Post by Mauricio Faria de Oliveira
+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.
Post by Mauricio Faria de Oliveira
+ return;
+ }
+
+ if (close(fd)) {
+ log_msg("Unable to close file");
+ return;
+ }
+}
+
.../...
Post by Mauricio Faria de Oliveira
+
+/*
+ * 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?
Post by Mauricio Faria de Oliveira
+/**
+ * mv_set_port_mode - set the LED mode of a SATA port (on register copy)
+ *
+ *
+ * - -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
+
.../...
Post by Mauricio Faria de Oliveira
+/**
+ * mv_indicator_list - Build Marvell HDD LED (indicator) list for the given disk vpd
+ *
+ *
+ * - 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.
Post by Mauricio Faria de Oliveira
+ * '../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?
Post by Mauricio Faria de Oliveira
+ 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
+ *
+ *
+ * - 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.
Post by Mauricio Faria de Oliveira
+ 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 .
Post by Mauricio Faria de Oliveira
+ 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
Mauricio Faria de Oliveira
2016-11-08 13:05:52 UTC
Permalink
Hi Vasant,

Thanks for the review.
Post by Vasant Hegde
Overall patch looks good. You have very good inline comments which helps
to understand the code.
Glad to have a confirmation it was worth writing them; thanks.
Post by Vasant Hegde
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) ?
Sure; renamed to indicator_marvell for clarity.
Post by Vasant Hegde
Post by Mauricio Faria de Oliveira
+/*
+ * 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?
Yes, I see. I had build problems w/ the original code & LPD_DEBUG iirc
(not sure if I had set it correctly though; could you demonstrate it?)

I didn't try to debug/fix for too long, and eventually decided not to
change other things to keep this submission more contained and easier
to review.

If that's a blocker for this submission, I'd like to ask for your help
to clarify how to enable it and debug that build failure.
Post by Vasant Hegde
Post by Mauricio Faria de Oliveira
+/*
+ * 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?
I'm usually in favor of typed variables instead of purely numeric macros
so to benefit more from compiler type checking; I see recommendations of
this practice.

I know we can use type suffixes on numeric literals (e.g, ULL) but tend
to find the types simpler.

If that's a problem/diverges from the project/your preferences, surely
I can change it to macros.
Post by Vasant Hegde
Also personally capital letter in variable name makes me to feel that
I'm reading non C code ;-)
Indeed; that was an habit for global variables. :)

Changed all of them to lowercase.
Post by Vasant Hegde
Post by Mauricio Faria de Oliveira
+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.
Absolutely; not sure where my head was at.

Removed this return statement.
Post by Vasant Hegde
Post by Mauricio Faria de Oliveira
+ 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?
That was just in case of a future hardware expansion; I expected push :)

Removed it.
Post by Vasant Hegde
Post by Mauricio Faria de Oliveira
+static int
+mv_indicator_list(struct loc_code **list, struct dev_vpd *vpd)
+{
<...>
Post by Vasant Hegde
Post by Mauricio Faria de Oliveira
+ /*
+ * 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.
Yes, there are 4 SATA ports in this controller; and only 2 disks at most
(using the first 2 SATA ports). That's an static config for the PCI ID
that is handled with this code.
Post by Vasant Hegde
Post by Mauricio Faria de Oliveira
+ 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?
Indeed. This was just simple (ie, lazy) since the size of ataX is known.

Changed for strstr() with '/host' (from '.../ataX/hostY/...').
Post by Vasant Hegde
Post by Mauricio Faria de Oliveira
+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.
That's correct.

Although the purpose of these patches is to turn on/off what we call
'fault LEDs' for the disks in marvell sata controller, those are not
really fault LEDs -- like those that light on automatically once a
fault occurs, and can be turned off with usysattn/fault.

For this controller, what we have is the user manually turns the LEDs on
(and off) with usysident, in order to unplug a disk, for example.

So, the code only runs w/ usysident (LED_TYPE_IDENT / CMD_IDENTIFY).
Post by Vasant Hegde
Post by Mauricio Faria de Oliveira
+ /* 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 .
I'd think so, but I don't know it right now.

I imagine the location code is exported via device tree?

Then, we can go up in the device sysfs path until the PCI device dir,
and follow the contents of the devspec file into /proc/device-tree,
then examine the device-tree properties.

For example, (but I see ibm,loc-code just gives 'Sata Backplane' now)

# lspci | grep -i marvell
0005:04:00.0 SATA controller: Marvell Technology Group Ltd.
88SE9235 PCIe 2.0 x2 4-port SATA 6 Gb/s Controller (rev 11)

# ls -ld /sys/block/sd* | grep 0005:04:00
<...> ->
../devices/<...>/0005:04:00.0/ata1/host3/target3:0:0/3:0:0:0/block/sdi
<...> ->
../devices/<...>/0005:04:00.0/ata2/host4/target4:0:0/4:0:0:0/block/sdj

# ls -1 /proc/device-tree/$(cat
/sys/devices/<...>/0005:04:00.0/devspec)/
class-code
device-id
ibm,loc-code
<...>

Not sure how lsvpd does that, but yes, theoretically we could mimic it.
Post by Vasant Hegde
Post by Mauricio Faria de Oliveira
+ 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.
Oops. Removed.
Post by Vasant Hegde
-Vasant
--
Mauricio Faria de Oliveira
IBM Linux Technology Center
Mauricio Faria de Oliveira
2016-11-08 16:41:14 UTC
Permalink
Post by Mauricio Faria de Oliveira
Post by Vasant Hegde
We have "LPD_DEBUG" for debug lpd code. Why not use same macro here?
Yes, I see. I had build problems w/ the original code & LPD_DEBUG iirc
(not sure if I had set it correctly though; could you demonstrate it?)
Hey, nevermind. I can't explain. Now it works.

$ make CFLAGS='-DLPD_DEBUG'

The point I don't like is that this is a compile time change. If one
wants to change the behavior at runtime (get debug messages), it's not
possible.

I can change _dbg() to consider both the compile time flag and runtime
env var. If it's set at build time, always print the debug messages,
and if it's not set an build time, but the env var is set at runtime,
then print debug messages too.

I'll submit a v3 of this patch only with this change, if you like
that approach.

Thanks for poking at that. :)
--
Mauricio Faria de Oliveira
IBM Linux Technology Center
Vasant Hegde
2016-11-09 10:05:49 UTC
Permalink
Post by Mauricio Faria de Oliveira
Post by Mauricio Faria de Oliveira
Post by Vasant Hegde
We have "LPD_DEBUG" for debug lpd code. Why not use same macro here?
Yes, I see. I had build problems w/ the original code & LPD_DEBUG iirc
(not sure if I had set it correctly though; could you demonstrate it?)
Hey, nevermind. I can't explain. Now it works.
Cool :-)
Post by Mauricio Faria de Oliveira
$ make CFLAGS='-DLPD_DEBUG'
The point I don't like is that this is a compile time change. If one
wants to change the behavior at runtime (get debug messages), it's not
possible.
I can change _dbg() to consider both the compile time flag and runtime
env var. If it's set at build time, always print the debug messages,
and if it's not set an build time, but the env var is set at runtime,
then print debug messages too.
I'll submit a v3 of this patch only with this change, if you like
that approach.
That's better. Lets not have multiple DEBUG macro for same codebase..

Well, ideally I'd prefer single DEBUG option for entire code base.. But
unfortunately we endup having multiple DEBUG option.. I blame myself for adding
new one for LPD code.. :-(

-Vasant
Vasant Hegde
2016-11-09 10:43:10 UTC
Permalink
Post by Mauricio Faria de Oliveira
Hi Vasant,
.../...
Post by Mauricio Faria de Oliveira
Post by Vasant Hegde
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) ?
Sure; renamed to indicator_marvell for clarity.
Cool!

.../...
Post by Mauricio Faria de Oliveira
Post by Vasant Hegde
Post by Mauricio Faria de Oliveira
+static const uint32_t MV_PCI_BAR5_VSR_ADDR = 0xa8;
Why not macros for all the variables?
I'm usually in favor of typed variables instead of purely numeric macros
so to benefit more from compiler type checking; I see recommendations of
this practice.
I know we can use type suffixes on numeric literals (e.g, ULL) but tend
to find the types simpler.
If that's a problem/diverges from the project/your preferences, surely
I can change it to macros.
Post by Vasant Hegde
Also personally capital letter in variable name makes me to feel that
I'm reading non C code ;-)
Indeed; that was an habit for global variables. :)
Changed all of them to lowercase.
cool!

.../...
Post by Mauricio Faria de Oliveira
Post by Vasant Hegde
Post by Mauricio Faria de Oliveira
+
+/*
+ * 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?
That was just in case of a future hardware expansion; I expected push :)
Yeah. I don't like unused codes.
Post by Mauricio Faria de Oliveira
Removed it.
Post by Vasant Hegde
Post by Mauricio Faria de Oliveira
+static int
+mv_indicator_list(struct loc_code **list, struct dev_vpd *vpd)
+{
<...>
Post by Vasant Hegde
Post by Mauricio Faria de Oliveira
+ /*
+ * 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.
Yes, there are 4 SATA ports in this controller; and only 2 disks at most
(using the first 2 SATA ports). That's an static config for the PCI ID
that is handled with this code.
Post by Vasant Hegde
Post by Mauricio Faria de Oliveira
+ 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?
Indeed. This was just simple (ie, lazy) since the size of ataX is known.
Changed for strstr() with '/host' (from '.../ataX/hostY/...').
cool!
Post by Mauricio Faria de Oliveira
Post by Vasant Hegde
Post by Mauricio Faria de Oliveira
+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.
That's correct.
Although the purpose of these patches is to turn on/off what we call
'fault LEDs' for the disks in marvell sata controller, those are not
really fault LEDs -- like those that light on automatically once a
fault occurs, and can be turned off with usysattn/fault.
For this controller, what we have is the user manually turns the LEDs on
(and off) with usysident, in order to unplug a disk, for example.
Sorry. I'm not sure I understood it correctly.

If I understood correctly we have one physical LED .. which is used for
identifying the device. and it doesn't have fault LED concept. Is that right?
Post by Mauricio Faria de Oliveira
So, the code only runs w/ usysident (LED_TYPE_IDENT / CMD_IDENTIFY).
Post by Vasant Hegde
Post by Mauricio Faria de Oliveira
+ /* 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 .
I'd think so, but I don't know it right now.
Thats fine ..
Post by Mauricio Faria de Oliveira
I imagine the location code is exported via device tree?
Yep. We use sysfs and dt in combination to get location code.
Post by Mauricio Faria de Oliveira
Then, we can go up in the device sysfs path until the PCI device dir,
and follow the contents of the devspec file into /proc/device-tree,
then examine the device-tree properties.
For example, (but I see ibm,loc-code just gives 'Sata Backplane' now)
Yeah. That's because we hardcode location code in OPAL firmware and its
different from IBM style of location code...

-Vasant
Mauricio Faria de Oliveira
2016-11-09 11:43:35 UTC
Permalink
Post by Vasant Hegde
Post by Mauricio Faria de Oliveira
Post by Vasant Hegde
Post by Mauricio Faria de Oliveira
+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.
That's correct.
Although the purpose of these patches is to turn on/off what we call
'fault LEDs' for the disks in marvell sata controller, those are not
really fault LEDs -- like those that light on automatically once a
fault occurs, and can be turned off with usysattn/fault.
For this controller, what we have is the user manually turns the LEDs on
(and off) with usysident, in order to unplug a disk, for example.
Sorry. I'm not sure I understood it correctly.
If I understood correctly we have one physical LED .. which is used for
identifying the device. and it doesn't have fault LED concept. Is that right?
iirc, 2 LEDs per disk; 1 for link/activity and 1 for identification;
this patch allows the user to control the latter manually.

yes, no attention/fault LED in the sense of a LED that is automatically
turned on by the adapter/controller firmware once it detects a problem
(and which the user could turn off with usysattn/usysfault).
--
Mauricio Faria de Oliveira
IBM Linux Technology Center
Mauricio Faria de Oliveira
2016-10-21 14:01:18 UTC
Permalink
On some systems, it's been observed that different disks of the Marvell
SATA HDD controller might be listed with the very same location code.

This could cause an user to specify a device by location code, and have
the wrong device identified; if the service operation is a hotplug/pull
the disk, that error would cause serious problems.

Even though this might be a problem that requires a fix elsewhere, it's
still possible to support such scenario, with the usysident option that
specify the element by device name (e.g., '-d sda') instead of location
code (e.g., '-l -B0-T0-L0').

This patch adds a simple check and workaround for that -- restricted to
the Marvell devices (TYPE_MARVELL); once a device match is found:

- if the device is specified by device name,
then proceed normally.

- if the device is specified by location code,
then check for other devices with this location code.

- if not found,
then proceed normally.

- else,
stop, and ask the user to specify the device name with '-d'.

Signed-off-by: Mauricio Faria de Oliveira <***@linux.vnet.ibm.com>
---
lpd/usysident.c | 48 +++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 47 insertions(+), 1 deletion(-)

diff --git a/lpd/usysident.c b/lpd/usysident.c
index 9eed946..e3dcad0 100644
--- a/lpd/usysident.c
+++ b/lpd/usysident.c
@@ -113,6 +113,7 @@ main(int argc, char **argv)
char *othervalue = NULL;
struct loc_code *current;
struct loc_code *list = NULL;
+ struct loc_code *list_start = NULL;

program_name = argv[0];
if (probe_indicator() != 0)
@@ -345,12 +346,14 @@ main(int argc, char **argv)
strncpy(temp, lvalue, LOCATION_LENGTH);
temp[LOCATION_LENGTH - 1] = '\0';

+ list_start = list;
retry:
- current = get_indicator_for_loc_code(list, lvalue);
+ current = get_indicator_for_loc_code(list_start, lvalue);
if (!current) {
if (trunc) {
if (truncate_loc_code(lvalue)) {
truncated = 1;
+ list_start = list; /* start again */
goto retry;
}
}
@@ -363,6 +366,48 @@ retry:
fprintf(stdout, "Truncated location code : "
"%s\n", lvalue);

+ if (current->type == TYPE_MARVELL) {
+
+ /*
+ * On some systems...
+ * The Marvell SATA HDD controller may have one location code
+ * for all disks. So, in order to uniquely identify a disk in
+ * the list, the -d <dev_name> (dvalue) option must be used.
+ *
+ * eg, if the -l <loc_code> (lvalue) option is used then the
+ * first disk in that location code will match, and it might
+ * be the wrong disk!
+ *
+ * So, check if there is another list entry in this system
+ * with the same location code as the current entry; if so,
+ * only handle Marvell devices if '-d <dev_name>' is used.
+ */
+ if (!dvalue &&
+ get_indicator_for_loc_code(current->next, lvalue)) {
+ fprintf(stdout, "The Marvell HDD LEDs must be "
+ "specified by device name (-d option);"
+ " non-unique location codes found.\n");
+ rc = 1;
+ goto no_retry;
+ }
+
+ /*
+ * At this point, after the check for "if (dvalue && lvalue)",
+ * if dvalue is non-NULL then lvalue is from get_loc_code_dev()
+ * (based on device name: dvalue), and dvalue is from cmdline.
+ *
+ * So, if dvalue is non-NULL and doesn't match the device name
+ * specified in 'current' (found location code), let's try the
+ * next elements in the list (which may match the device name);
+ * for this we need to retry, re-starting on the next element.
+ */
+ if (dvalue && current->devname &&
+ strncmp(dvalue, current->devname, DEV_LENGTH)) {
+ list_start = current->next;
+ goto retry;
+ }
+ }
+
if (svalue) {
rc = get_indicator_state(indicator, current,
&state);
@@ -388,6 +433,7 @@ retry:
}
} /* if-else end */
} /* lvalue end */
+no_retry:

/* Turn on/off all indicators */
if (othervalue) {
--
1.8.3.1
Vasant Hegde
2016-11-08 11:02:33 UTC
Permalink
Post by Mauricio Faria de Oliveira
On some systems, it's been observed that different disks of the Marvell
SATA HDD controller might be listed with the very same location code.
IIUC each disk is an independent FRU. Just wondering why we may endup have same
loc code for multiple disks.
Post by Mauricio Faria de Oliveira
This could cause an user to specify a device by location code, and have
the wrong device identified; if the service operation is a hotplug/pull
the disk, that error would cause serious problems.
Even though this might be a problem that requires a fix elsewhere, it's
I'd prefer to fix at the source (avoid duplicate loc code) instead of having
workaround. But given the implications, I'm ok to accept this patch.

But who is creating these dup location codes? lsvpd tools or sysfs (kernel/DT) ?

-Vasant
Mauricio Faria de Oliveira
2016-11-08 13:28:03 UTC
Permalink
Post by Vasant Hegde
Post by Mauricio Faria de Oliveira
This could cause an user to specify a device by location code, and have
the wrong device identified; if the service operation is a hotplug/pull
the disk, that error would cause serious problems.
Even though this might be a problem that requires a fix elsewhere, it's
I'd prefer to fix at the source (avoid duplicate loc code) instead of
having workaround. But given the implications, I'm ok to accept this patch.
Thanks; that helps for now.
Post by Vasant Hegde
But who is creating these dup location codes? lsvpd tools or sysfs (kernel/DT) ?
Taking a look now, it seems to be lsvpd, which handles it as SCSI disks
(with Bus, Target, and LUN numbers).

DeviceTreeCollector::buildSCSILocCode()
...
val << "-B" << bus->dataValue << "-T" << target->dataValue
<< "-L" << lun->dataValue;
...

And from lsvpd:

# lsvpd --list=sdj
<...>
*DS -SCSI Disk Drive
<...>
*MF ATA
<...>
*YL -B0-T0-L0

I can try to come up w/ a patch for it later on. If you want to tackle
it yourself, just let me know.
--
Mauricio Faria de Oliveira
IBM Linux Technology Center
Vasant Hegde
2016-11-09 10:47:21 UTC
Permalink
Post by Mauricio Faria de Oliveira
Post by Vasant Hegde
Post by Mauricio Faria de Oliveira
This could cause an user to specify a device by location code, and have
the wrong device identified; if the service operation is a hotplug/pull
the disk, that error would cause serious problems.
Even though this might be a problem that requires a fix elsewhere, it's
I'd prefer to fix at the source (avoid duplicate loc code) instead of
having workaround. But given the implications, I'm ok to accept this patch.
Thanks; that helps for now.
Post by Vasant Hegde
But who is creating these dup location codes? lsvpd tools or sysfs (kernel/DT) ?
Taking a look now, it seems to be lsvpd, which handles it as SCSI disks
(with Bus, Target, and LUN numbers).
DeviceTreeCollector::buildSCSILocCode()
...
val << "-B" << bus->dataValue << "-T" << target->dataValue
<< "-L" << lun->dataValue;
...
# lsvpd --list=sdj
<...>
*DS -SCSI Disk Drive
<...>
*MF ATA
<...>
*YL -B0-T0-L0
I can try to come up w/ a patch for it later on. If you want to tackle
it yourself, just let me know.
Patch is always welcome :-) If you have time please fix it. Else I will chase
this issue some other day.

-Vasant
Mauricio Faria de Oliveira
2016-10-21 14:01:19 UTC
Permalink
Some systems/conditions have no kernel LEDs present (thus nothing in
the /sys/class/leds directory), which would cause usysident to early
exit in the OPAL platform, and not give a chance for other LED types
(e.g., Marvell HDD LEDs) to be used.

This patch adds a check for that in the OPAL platform probe function,
and in case Marvell HDD devices are found, prevent usysident to exit
early, so they can be used.

Signed-off-by: Mauricio Faria de Oliveira <***@linux.vnet.ibm.com>
---
lpd/indicator_opal.c | 13 +++++++++++++
1 file changed, 13 insertions(+)

diff --git a/lpd/indicator_opal.c b/lpd/indicator_opal.c
index 31e3fd2..0d28bab 100644
--- a/lpd/indicator_opal.c
+++ b/lpd/indicator_opal.c
@@ -293,6 +293,7 @@ opal_indicator_probe(void)
int rc = -1;
DIR *led_dir;
struct dirent *dirent;
+ struct loc_code *list = NULL;

led_dir = open_sysfs_led_dir();
if (!led_dir)
@@ -314,6 +315,18 @@ opal_indicator_probe(void)
return 0;
}

+ /*
+ * Marvell HDD LEDs are not presented/controlled via kernel LEDs
+ * (i.e., /sys/class/leds), and some OPAL systems might not have
+ * any kernel LEDs (e.g., modules not loaded) but still have the
+ * Marvell SATA controller with LEDs available, and able to work.
+ */
+ get_mv_indices(LED_TYPE_IDENT, &list);
+ if (list) {
+ free_indicator_list(list);
+ return 0;
+ }
+
fprintf(stderr, "Service indicators are not supported on this system."
"\nMake sure 'leds_powernv' kernel module is loaded.\n");
close_sysfs_led_dir(led_dir);
--
1.8.3.1
Vasant Hegde
2016-11-08 11:09:56 UTC
Permalink
Post by Mauricio Faria de Oliveira
Some systems/conditions have no kernel LEDs present (thus nothing in
the /sys/class/leds directory), which would cause usysident to early
exit in the OPAL platform, and not give a chance for other LED types
(e.g., Marvell HDD LEDs) to be used.
This patch adds a check for that in the OPAL platform probe function,
and in case Marvell HDD devices are found, prevent usysident to exit
early, so they can be used.
---
lpd/indicator_opal.c | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/lpd/indicator_opal.c b/lpd/indicator_opal.c
index 31e3fd2..0d28bab 100644
--- a/lpd/indicator_opal.c
+++ b/lpd/indicator_opal.c
@@ -293,6 +293,7 @@ opal_indicator_probe(void)
int rc = -1;
DIR *led_dir;
struct dirent *dirent;
+ struct loc_code *list = NULL;
led_dir = open_sysfs_led_dir();
if (!led_dir)
@@ -314,6 +315,18 @@ opal_indicator_probe(void)
return 0;
}
In theory, open_sysfs_led_dir() may fail. You are returning if
open_sysfs_led_dir() fails.. I think we have to fix that.

Also I'm thinking of splitting this function. How about splitting this to two
function
- One detects platform LEDS and second one for Marvell LED ?
Post by Mauricio Faria de Oliveira
+ /*
+ * Marvell HDD LEDs are not presented/controlled via kernel LEDs
+ * (i.e., /sys/class/leds), and some OPAL systems might not have
+ * any kernel LEDs (e.g., modules not loaded) but still have the
+ * Marvell SATA controller with LEDs available, and able to work.
+ */
+ get_mv_indices(LED_TYPE_IDENT, &list);
+ if (list) {
+ free_indicator_list(list);
+close_sysfs_led_dir(led_dir);

-Vasant
Mauricio Faria de Oliveira
2016-11-08 13:46:49 UTC
Permalink
Post by Vasant Hegde
Post by Mauricio Faria de Oliveira
@@ -293,6 +293,7 @@ opal_indicator_probe(void)
<...>
Post by Vasant Hegde
Post by Mauricio Faria de Oliveira
led_dir = open_sysfs_led_dir();
if (!led_dir)
@@ -314,6 +315,18 @@ opal_indicator_probe(void)
return 0;
}
In theory, open_sysfs_led_dir() may fail. You are returning if
open_sysfs_led_dir() fails.. I think we have to fix that.
Nice catch.
Post by Vasant Hegde
Also I'm thinking of splitting this function. How about splitting this
to two function
- One detects platform LEDS and second one for Marvell LED ?
Okay, sure. Implemented in v2.
Post by Vasant Hegde
Post by Mauricio Faria de Oliveira
+ /*
+ * Marvell HDD LEDs are not presented/controlled via kernel LEDs
+ * (i.e., /sys/class/leds), and some OPAL systems might not have
+ * any kernel LEDs (e.g., modules not loaded) but still have the
+ * Marvell SATA controller with LEDs available, and able to work.
+ */
+ get_mv_indices(LED_TYPE_IDENT, &list);
+ if (list) {
+ free_indicator_list(list);
+close_sysfs_led_dir(led_dir);
Nice catch.
Post by Vasant Hegde
-Vasant
Again, thanks for the review. I'll test the v2 and submit once OK.

Kind regards,
--
Mauricio Faria de Oliveira
IBM Linux Technology Center
Loading...