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

On 11/08/2016 06:35 PM, Mauricio Faria de Oliveira wrote:
> Hi Vasant,
>

.../...

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

.../...

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

cool!

.../...

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

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

cool!

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


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

Thats fine ..

> I imagine the location code is exported via device tree?

Yep. We use sysfs and dt in combination to get location code.

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


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