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

Hi Vasant,

Thanks for the review.

On 11/08/2016 08:52 AM, Vasant Hegde wrote:
> 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.

> 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.

>> +/*
>> + * 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.


>> +/*
>> + * 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.

> 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.


>> +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.

>
>
>> +        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.

>> +static int
>> +mv_indicator_list(struct loc_code **list, struct dev_vpd *vpd)
>> +{
<...>
>> +    /*
>> +     * 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.

>> +    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/...').

>> +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).

>> +    /* 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.



>> +    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.

>
> -Vasant
>



-- 
Mauricio Faria de Oliveira
IBM Linux Technology Center


------------------------------------------------------------------------------
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