linux-diag-devel — discussion of diagnostics on linux

[Linux-diag-devel] Fwd: systool -c fc_host does not list all fc_hosts

[Jiatong S. <ysh...@gm...>]

---------- Forwarded message ---------
From: Jiatong Shen <ysh...@gm...>
Date: Sat, Aug 13, 2022 at 9:01 AM
Subject: systool -c fc_host does not list all fc_hosts
To: <an...@in...>, <dst...@us...>, <mo...@in...>, <
lin...@li...>


Hello,

  I am trouble shooting a weird situation where host1 is not displayed.
Annd the output is at last. After reading through code, I am suspecting
that https://github.com/Distrotech/sysfsutils/issues/4 the comparison might
be incorrect. The reason is that readidr may not return dir in an ascending
order which hereby causing host1 matching host18 which already exists.

So my question is is it possible to change strncmp to strcmp? Thank you
very much.

And the logs:

# systool -c fc_host
Class = "fc_host"

  Class Device = "host16"
    Device = "host16"

  Class Device = "host17"
    Device = "host17"

  Class Device = "host18"
    Device = "host18"


:~# ls /sys/class/fc_host
host1  host16  host17  host18

-- 

Best Regards,

Jiatong Shen


-- 

Best Regards,

Jiatong Shen

Re: [Linux-diag-devel] Anybody home?

[Lee D. <ld...@su...>]

On 2/26/20 1:36 AM, Vasant Hegde wrote:
> On 2/4/20 10:13 AM, Ananth N Mavinakayanahalli wrote:
>> On 1/31/20 8:02 PM, Thomas Guyot-Sionnest wrote:
>>>
>>> -----BEGIN PGP SIGNED MESSAGE-----
>>> Hash: SHA1
>>>
>>> Hi Ananth,
>>>
>>> On 2020-01-31 02:22, Ananth N Mavinakayanahalli wrote:
>>>> Is this list still active? Is anybody out there?
>>>>
>>>> I am one of the original developers of systool/libsysfs, way back then.
>>>>
>>>> Most active projects from here have transitioned to github.
>>>>
>>>
>>> I see SF git repos, RO CVS, but I couldn't find a Github organization
>>> related to this SF project. If there is one could you please point it
>>> out, or the repos? I'm fine using either one (SF or GH) but if GH is
>>> used it should have an official org too with the same members as the SF
>>> project.
>>
>> The other active tools that transitioned were all Power specific ones
>> and were moved to https://github.com/power-ras.
>>
>>>> Thomas,
>>>> Glad to hear you are interested in contributing and as you mention
>>> down the thread.
>>>>
>>>> If you folks would like to maintain it, please let me know. Once the
>>> transition is done, I will add a comment on this site redirecting users
>>> to the new repo.
>>>
>>> I'll be glad to help, however I wouldn't want to commit as being "the"
>>> maintainer right now. I can help doing conversions to Git if some repos
>>> are sill on CVS, possibly some code/PR reviews and minor hygiene too...
>>>
>>> Let me know what you think.
>>
>> Sourabh (on CC) will take over transitioning this project to github and
>> maintaining it. He will update here with details of the new repo, etc.
> 
> We have moved sysfsutils to github.
> 
> New home: https://github.com/linux-ras/sysfsutils
> 
> This needs some cleanup. Patches are welcome :-)
> 
> 
> -Vasant

Sweet! I opened an issue and a pull request. Thank you so much for your
efforts!

-- 
Lee Duncan
SUSE kernel hacker

Re: [Linux-diag-devel] Anybody home?

[Vasant H. <heg...@li...>]

On 2/4/20 10:13 AM, Ananth N Mavinakayanahalli wrote:
> On 1/31/20 8:02 PM, Thomas Guyot-Sionnest wrote:
>>
>> -----BEGIN PGP SIGNED MESSAGE-----
>> Hash: SHA1
>>
>> Hi Ananth,
>>
>> On 2020-01-31 02:22, Ananth N Mavinakayanahalli wrote:
>>> Is this list still active? Is anybody out there?
>>>
>>> I am one of the original developers of systool/libsysfs, way back then.
>>>
>>> Most active projects from here have transitioned to github.
>>>
>>
>> I see SF git repos, RO CVS, but I couldn't find a Github organization
>> related to this SF project. If there is one could you please point it
>> out, or the repos? I'm fine using either one (SF or GH) but if GH is
>> used it should have an official org too with the same members as the SF
>> project.
> 
> The other active tools that transitioned were all Power specific ones and were 
> moved to https://github.com/power-ras.
> 
>>> Thomas,
>>> Glad to hear you are interested in contributing and as you mention
>> down the thread.
>>>
>>> If you folks would like to maintain it, please let me know. Once the
>> transition is done, I will add a comment on this site redirecting users
>> to the new repo.
>>
>> I'll be glad to help, however I wouldn't want to commit as being "the"
>> maintainer right now. I can help doing conversions to Git if some repos
>> are sill on CVS, possibly some code/PR reviews and minor hygiene too...
>>
>> Let me know what you think.
> 
> Sourabh (on CC) will take over transitioning this project to github and
> maintaining it. He will update here with details of the new repo, etc.

We have moved sysfsutils to github.

New home: https://github.com/linux-ras/sysfsutils

This needs some cleanup. Patches are welcome :-)


-Vasant

Re: [Linux-diag-devel] Anybody home?

[Ananth N M. <an...@li...>]

On 1/31/20 8:02 PM, Thomas Guyot-Sionnest wrote:
> 
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> Hi Ananth,
> 
> On 2020-01-31 02:22, Ananth N Mavinakayanahalli wrote:
>> Is this list still active? Is anybody out there?
>>
>> I am one of the original developers of systool/libsysfs, way back then.
>>
>> Most active projects from here have transitioned to github.
>>
> 
> I see SF git repos, RO CVS, but I couldn't find a Github organization
> related to this SF project. If there is one could you please point it
> out, or the repos? I'm fine using either one (SF or GH) but if GH is
> used it should have an official org too with the same members as the SF
> project.

The other active tools that transitioned were all Power specific ones 
and were moved to https://github.com/power-ras.

>> Thomas,
>> Glad to hear you are interested in contributing and as you mention
> down the thread.
>>
>> If you folks would like to maintain it, please let me know. Once the
> transition is done, I will add a comment on this site redirecting users
> to the new repo.
> 
> I'll be glad to help, however I wouldn't want to commit as being "the"
> maintainer right now. I can help doing conversions to Git if some repos
> are sill on CVS, possibly some code/PR reviews and minor hygiene too...
> 
> Let me know what you think.

Sourabh (on CC) will take over transitioning this project to github and
maintaining it. He will update here with details of the new repo, etc.

Regards,
Ananth

Re: [Linux-diag-devel] Anybody home?

[Lee D. <LD...@su...>]

On 1/30/20 7:25 PM, Thomas Guyot-Sionnest wrote:
>
> Hi Lee,
> 
> On 2020-01-30 19:01, Lee Duncan wrote:
>> On 1/30/20 9:40 AM, Thomas Guyot-Sionnest wrote:
>>> I haven't seen a soul since I joined in November, but thanks for
>>> reminding me that I wanted to suggest an init script for sysfsutils.
>> What would it need to do at system startup time? Just curious.
> 
> Just to be sure, we're not talking of the same package; this SF project
> has many subprojects/code repos.

You lost me. Yes, I know sysfsutils seems to be just one of the packages
that got abandoned, but based on the number of downloads that
sourceforge lists, it's the most-used "module" by far.

If we wanted to use github for sources, it supports the concept of an
organization. For example, github.com/open-iscsi is a grouping, with
open-iscsi itself, open-isns, targetcli, open-isns, and target-isns
under it. Perhaps we could group the various CVS "modules", each as it's
own repo/project on github?

> 
> Debian added an init script in their sysfsutils package to set sysfs
> variables very similar to sysctl (in procps package). I have noticed it
> is missing in other distros, and I believe it would be a good thing to
> standardize on so application packagers could leverage /etc/sysfs.d/ to
> add important system tunings.

Yes, it looks like sysctl fills that need for SUSE/openSUSE (my distro).
But, if I understand correctly, many kernel folks say sysctl is evil and
broken. I'm no expert in that area.

> 
>> I tried to read from the CVS repo, which says it's read-only, but the
>> repo kept asking me for a password, so that got nowhere.
> 
>> But I found  https://github.com/Distrotech/sysfsutils
> 
>> Is that unofficial?
> It appears to be a private fork maintained by a South African company;
> see http://www.mzanziopensource.co.za/

Ah. That explains some. Like me (I'm guessing), they needed a place to
put the code so they could use it.

> 
>> Either way, I'd be glad to contribute. If the current git repo is close
>> enough, we can fork it and start from there.
> 
>> If it is not good enough (it has no version tags, and apparently no
>> support), we can use a tool (as you suggest) to import the CVS stuff and
>> start from there.
> 
>> This stuff seems to still have plenty of value.
> Yea... the tooling certainly improved... Subversion was notorious for
> failing to import branches, primarily because those were free-form and
> you could end up importing directories of branches. I haven't needed
> branches on my cvs imports but since it's implemented natively it should
> be easy.
> 
> Asides from that, Git normally has user name and email where CVS has
> only user id's, so a table is needed for conversion (for the email,
> <userid>@users.sourceforge.com generally works, and some users have made
> their name public, there could be other sources else the name will be
> only user id).

I take it you are talking about how to get the revision history
converted from CVS to git?

I was thinking more like this: leave the current CVS repository as it
is, or perhaps move it, if needed. Leave it as a read-only reference,
and start the github version from now, going forward. That would save a
ton of work. But, as a newcomer to these packages, I don't know how much
history would be mostly lost with this approach, or if such loss
matters. What do you think?

> 
> There's also the $Id$, $Author$ and similar tags. They need to be
> cleaned up in headers, and if they are also used in the code such as for
> generating version strings, they need to be replaced by something
> git-specific (ex git-describe) and run as a dependency to the build process.

I could easily fix that kind of stuff. And I could probably manage
trying to convert from CVS to git, as a side project (because it sounds
somewhat interesting, perhaps because I've never tried it).

I just don't want to make such changes in a vacuum, if others are still
using this software. After all, I believe open software has value
because multiple people can contribute. :)

Can you get me access to a CVS tree? As I said, I tried to get it from
sourceforge but ran into some sort of password issue.

> 
> Regards,
> 
> Thomas
>

Re: [Linux-diag-devel] Anybody home?

[Thomas Guyot-S. <de...@ae...>]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi Ananth,

On 2020-01-31 02:22, Ananth N Mavinakayanahalli wrote:
> Is this list still active? Is anybody out there?
>
> I am one of the original developers of systool/libsysfs, way back then.
>
> Most active projects from here have transitioned to github.
>

I see SF git repos, RO CVS, but I couldn't find a Github organization
related to this SF project. If there is one could you please point it
out, or the repos? I'm fine using either one (SF or GH) but if GH is
used it should have an official org too with the same members as the SF
project.

> Thomas,
> Glad to hear you are interested in contributing and as you mention
down the thread.
>
> If you folks would like to maintain it, please let me know. Once the
transition is done, I will add a comment on this site redirecting users
to the new repo.

I'll be glad to help, however I wouldn't want to commit as being "the"
maintainer right now. I can help doing conversions to Git if some repos
are sill on CVS, possibly some code/PR reviews and minor hygiene too...

Let me know what you think.

Regards,

- --
Thomas
-----BEGIN PGP SIGNATURE-----

iF0EARECAB0WIQQmNIgxdxnDh64S6frp1n4q3kFyFgUCXjQ6cAAKCRDp1n4q3kFy
FtZxAJ9tC6xSJ2uRfFVGjHtZTHvfPsQvyQCfeFshKtKzNKNuxeqx2Y6/6yc7lsU=
=Noxr
-----END PGP SIGNATURE-----

Re: [Linux-diag-devel] Fwd: Anybody home?

[Ananth N M. <an...@li...>]

Hi Lee,

 > Is this list still active? Is anybody out there?

I am one of the original developers of systool/libsysfs, way back then.

Most active projects from here have transitioned to github.

 > I have some changes I'd like to submit for systool to enable it to
 > compile using gcc-9.
 >
 > Does anyone care?
 >
 > This command (systool) seems to still be used but is no longer being
 > supported?
 >
 > Can anybody explain the status? I just inherited support for this
 > command for our Linux disto, so I'd like to figure it out.

I have moved on from this project a long time ago and don't really have
the bandwidth to maintain it. As it is, the tool has been in use for
over a decade without so much as an update.

Thomas,
Glad to hear you are interested in contributing and as you mention down 
the thread.

If you folks would like to maintain it, please let me know. Once the 
transition is done, I will add a comment on this site redirecting users 
to the new repo.

-- 
Ananth

Re: [Linux-diag-devel] Anybody home?

[Ananth N M. <an...@li...>]

Hi Lee,

> Is this list still active? Is anybody out there?

I am one of the original developers of systool/libsysfs, way back then.

Most active projects from here have transitioned to github.

> I have some changes I'd like to submit for systool to enable it to
> compile using gcc-9.
> 
> Does anyone care?
> 
> This command (systool) seems to still be used but is no longer being
> supported?
> 
> Can anybody explain the status? I just inherited support for this
> command for our Linux disto, so I'd like to figure it out.

I have moved on from this project a long time ago and don't really have
the bandwidth to maintain it. As it is, the tool has been in use for
over a decade without so much as an update.

Thomas,
Glad to hear you are interested in contributing and as you mention down 
the thread.

If you folks would like to maintain it, please let me know. Once the 
transition is done, I will add a comment on this site redirecting users 
to the new repo.

-- 
Ananth

Re: [Linux-diag-devel] Anybody home?

[Thomas Guyot-S. <de...@ae...>]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi Lee,

On 2020-01-30 19:01, Lee Duncan wrote:
> On 1/30/20 9:40 AM, Thomas Guyot-Sionnest wrote:
>> I haven't seen a soul since I joined in November, but thanks for
>> reminding me that I wanted to suggest an init script for sysfsutils.
> What would it need to do at system startup time? Just curious.

Just to be sure, we're not talking of the same package; this SF project
has many subprojects/code repos.

Debian added an init script in their sysfsutils package to set sysfs
variables very similar to sysctl (in procps package). I have noticed it
is missing in other distros, and I believe it would be a good thing to
standardize on so application packagers could leverage /etc/sysfs.d/ to
add important system tunings.

> I tried to read from the CVS repo, which says it's read-only, but the
> repo kept asking me for a password, so that got nowhere.
>
> But I found  https://github.com/Distrotech/sysfsutils
>
> Is that unofficial?
It appears to be a private fork maintained by a South African company;
see http://www.mzanziopensource.co.za/
>
> Either way, I'd be glad to contribute. If the current git repo is close
> enough, we can fork it and start from there.
>
> If it is not good enough (it has no version tags, and apparently no
> support), we can use a tool (as you suggest) to import the CVS stuff and
> start from there.
>
> This stuff seems to still have plenty of value.
Yea... the tooling certainly improved... Subversion was notorious for
failing to import branches, primarily because those were free-form and
you could end up importing directories of branches. I haven't needed
branches on my cvs imports but since it's implemented natively it should
be easy.

Asides from that, Git normally has user name and email where CVS has
only user id's, so a table is needed for conversion (for the email,
<userid>@users.sourceforge.com generally works, and some users have made
their name public, there could be other sources else the name will be
only user id).

There's also the $Id$, $Author$ and similar tags. They need to be
cleaned up in headers, and if they are also used in the code such as for
generating version strings, they need to be replaced by something
git-specific (ex git-describe) and run as a dependency to the build process.

Regards,

Thomas
-----BEGIN PGP SIGNATURE-----

iF0EARECAB0WIQQmNIgxdxnDh64S6frp1n4q3kFyFgUCXjOePAAKCRDp1n4q3kFy
FsIZAJ9mIE9iwQDQMUSIDtTDo8QIDYChYACfZLmsHwmM0lGRdp43DSXK1oL1nZs=
=tY/A
-----END PGP SIGNATURE-----

Re: [Linux-diag-devel] Anybody home?

[Lee D. <LD...@su...>]

On 1/30/20 9:40 AM, Thomas Guyot-Sionnest wrote:
>
> Hi Lee,
> 
> I haven't seen a soul since I joined in November, but thanks for
> reminding me that I wanted to suggest an init script for sysfsutils.

What would it need to do at system startup time? Just curious.

> 
> It seems a lot of repos are still on an RO CVS - I haven't even tried to
> check it out yet, but I've done many CVS and SVN conversions to git in
> the past (Nagios/Nagios-plugins + many proprietary repos), I'm wiling to
> give a stab at it if someone is there to add them to the SF project.

I tried to read from the CVS repo, which says it's read-only, but the
repo kept asking me for a password, so that got nowhere.

But I found  https://github.com/Distrotech/sysfsutils

Is that unofficial?

Either way, I'd be glad to contribute. If the current git repo is close
enough, we can fork it and start from there.

If it is not good enough (it has no version tags, and apparently no
support), we can use a tool (as you suggest) to import the CVS stuff and
start from there.

This stuff seems to still have plenty of value.

> 
> Regards,
> 
> Thomas
> 
> On 2020-01-30 10:57, Lee Duncan wrote:
>> Hi:
> 
>> Is this list still active? Is anybody out there?
> 
>> I have some changes I'd like to submit for systool to enable it to
>> compile using gcc-9.
> 
>> Does anyone care?
> 
>> This command (systool) seems to still be used but is no longer being
>> supported?
> 
>> Can anybody explain the status? I just inherited support for this
>> command for our Linux disto, so I'd like to figure it out.
> 
>> Thank you.
> 
>

Re: [Linux-diag-devel] Anybody home?

[Thomas Guyot-S. <de...@ae...>]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi Lee,

I haven't seen a soul since I joined in November, but thanks for
reminding me that I wanted to suggest an init script for sysfsutils.

It seems a lot of repos are still on an RO CVS - I haven't even tried to
check it out yet, but I've done many CVS and SVN conversions to git in
the past (Nagios/Nagios-plugins + many proprietary repos), I'm wiling to
give a stab at it if someone is there to add them to the SF project.

Regards,

Thomas

On 2020-01-30 10:57, Lee Duncan wrote:
> Hi:
>
> Is this list still active? Is anybody out there?
>
> I have some changes I'd like to submit for systool to enable it to
> compile using gcc-9.
>
> Does anyone care?
>
> This command (systool) seems to still be used but is no longer being
> supported?
>
> Can anybody explain the status? I just inherited support for this
> command for our Linux disto, so I'd like to figure it out.
>
> Thank you.

-----BEGIN PGP SIGNATURE-----

iF0EARECAB0WIQQmNIgxdxnDh64S6frp1n4q3kFyFgUCXjMVGgAKCRDp1n4q3kFy
FulaAKCoHL/baoRJCpg8MhLA0Kj+saXs/gCffXkEgCFBlX8tOlEE+zvTrzPc+dQ=
=3pr9
-----END PGP SIGNATURE-----

[Linux-diag-devel] Anybody home?

[Lee D. <LD...@su...>]

Hi:

Is this list still active? Is anybody out there?

I have some changes I'd like to submit for systool to enable it to
compile using gcc-9.

Does anyone care?

This command (systool) seems to still be used but is no longer being
supported?

Can anybody explain the status? I just inherited support for this
command for our Linux disto, so I'd like to figure it out.

Thank you.
-- 
Lee Duncan

Re: [Linux-diag-devel] [PATCH] lpd: marvell: mark the indirect-access registers as volatile

[Vasant H. <heg...@li...>]

On 12/13/2016 06:58 PM, Mauricio Faria de Oliveira wrote:
> In optimization level -O3, a sequence of VSR read/write operations
> can be incorrectly optimized away as the compiler doesn't know the
> underlying memory locations can change and react to read/writes.
>
> So, mark the pointers used to read/write to the indirect-access
> registers which access the VSRs with the 'volatile' keyword.


Merged as cf718b32.

-Vasant

[Linux-diag-devel] [PATCH] lpd: marvell: mark the indirect-access registers as volatile

[Mauricio F. de O. <mau...@li...>]

In optimization level -O3, a sequence of VSR read/write operations
can be incorrectly optimized away as the compiler doesn't know the
underlying memory locations can change and react to read/writes.

So, mark the pointers used to read/write to the indirect-access
registers which access the VSRs with the 'volatile' keyword.

Test-cases:
----------

CFLAGS=-O2 (default)

    # make
    # LPD_DEBUG=1 ./lpd/usysident
    ...
    DEBUG: get_mv_indicator(): Data Out: '0x00000018'
    DEBUG: get_mv_indicator(): Data Out Enable: '0xffffffe7'
    DEBUG: get_mv_indicator(): Port Active Select: '0x2db6da8d'
    ...

CFLAGS=-O3

    # rm lpd/indicator_marvell.o
    # CFLAGS='-O3' make
    # LPD_DEBUG=1 ./lpd/usysident
    ...
    DEBUG: get_mv_indicator(): Data Out: '0x2db6da8d'
    DEBUG: get_mv_indicator(): Data Out Enable: '0x2db6da8d'
    DEBUG: get_mv_indicator(): Port Active Select: '0x2db6da8d'
    ...

CFLAGS=-O3 (patched)

    # rm lpd/indicator_marvell.o
    # CFLAGS='-O3' make
    # LPD_DEBUG=1 ./lpd/usysident
    ...
    DEBUG: get_mv_indicator(): Data Out: '0x00000018'
    DEBUG: get_mv_indicator(): Data Out Enable: '0xffffffe7'
    DEBUG: get_mv_indicator(): Port Active Select: '0x2db6da8d'

Signed-off-by: Mauricio Faria de Oliveira <mau...@li...>
Fixes: efb9a4df3f88 ("lpd: Add support for Marvell HDD LEDs on S822LC for HPC")
---
 lpd/indicator_marvell.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/lpd/indicator_marvell.c b/lpd/indicator_marvell.c
index ec2580a73f03..ace808dde53f 100644
--- a/lpd/indicator_marvell.c
+++ b/lpd/indicator_marvell.c
@@ -183,8 +183,8 @@ mv_munmap_bar5(void *bar5, int fd)
 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);
+	volatile uint32_t *addr = (uint32_t *)(bar5 + mv_pci_bar5_vsr_addr);
+	volatile uint32_t *data = (uint32_t *)(bar5 + mv_pci_bar5_vsr_data);
 
 	/* set address and read data */
 	*addr = vsr_addr;
@@ -204,8 +204,8 @@ mv_vsr_read(void* bar5, uint32_t vsr_addr)
 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);
+	volatile uint32_t *addr = (uint32_t *)(bar5 + mv_pci_bar5_vsr_addr);
+	volatile uint32_t *data = (uint32_t *)(bar5 + mv_pci_bar5_vsr_data);
 
 	/* set address and write data */
 	*addr = vsr_addr;
-- 
1.8.3.1

Re: [Linux-diag-devel] [PATCH v3 0/3] Support for Marvell SATA HDD LEDs on IBM Power System S822LC for HPC

[Mauricio F. de O. <mau...@li...>]

On 11/21/2016 08:35 AM, Vasant Hegde wrote:
> On 11/18/2016 04:30 AM, Mauricio Faria de Oliveira wrote:
>> Sorry; even w/ your explanation I didn't get why/what exactly
>> needs to be fixed in this function. Can you say it for dummies?
>
> Don't worry.. I will fix it.

Thanks a bunch.

>>> On error scandir returns -1 right?
>>
>> Yes, but is doesn't error in this case. The /sys/class/leds dir is
>> present even without LED modules loaded (led-class.o is built-in.)
>
>
> I was talking about the scenario where kernel won't create sysfs node.
> Probably we don't need to worry about this case for now.

Ok.

>> Would you please check if the assumption to remove the fault mode check
>> is correct? I'm afraid I don't understand enough about this to be sure.
>
> I've tweaked your patch slightly and merged it.

Cool. Thanks again for all your help!


-- 
Mauricio Faria de Oliveira
IBM Linux Technology Center

[Linux-diag-devel] [PATCH] lpd/test: Fix test case

[Vasant H. <heg...@li...>]

Commit 4c7400ba added Marvell HDD LED code, but forgot to update test
dir Makefile. Though we don't support fault LED on Marvell HDD, we
need to update test/Makefile, else compilation will fail.

I think we have to rewrite part of test code. But that's for some other day.

Fixes: 4c7400ba (lpd: Add support for Marvell HDD LEDs on S822LC for HPC)
CC: Mauricio Faria de Oliveira <mau...@li...>
Signed-off-by: Vasant Hegde <heg...@li...>
---
 lpd/test/Makefile |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lpd/test/Makefile b/lpd/test/Makefile
index c855426..5c599fb 100644
--- a/lpd/test/Makefile
+++ b/lpd/test/Makefile
@@ -16,7 +16,7 @@ LPD_DIR = $(ROOT_DIR)/lpd
 COMMON_OBJS = $(COMMON_DIR)/utils.o $(COMMON_DIR)/platform.o
 LPD_OBJS = $(COMMON_OBJS) ../files.o ../lp_util.o ../indicator_ses.o \
 	   ../indicator_rtas.o ../indicator_opal.o ../indicator.o \
-	   ../servicelog.o
+	   ../servicelog.o ../indicator_marvell.o
 
 LED_TOOL_OBJS =  ledtool.o
 SES_TOOL_OBJS = sesdevices.o

Re: [Linux-diag-devel] [PATCH v3 0/3] Support for Marvell SATA HDD LEDs on IBM Power System S822LC for HPC

[Vasant H. <heg...@li...>]

On 11/18/2016 04:30 AM, Mauricio Faria de Oliveira wrote:
> Hi Vasant,
>
> On 11/17/2016 12:42 PM, Vasant Hegde wrote:
>>>  > - opal_get_indicator_mode()
>>>  >   We need to add check in this function.
>>>
>>> Can you clarify this request?
>
>> Now that we have special case, we should fix this function.
>
> Sorry; even w/ your explanation I didn't get why/what exactly
> needs to be fixed in this function. Can you say it for dummies?

Don't worry.. I will fix it.

>
>>>  > - opal_get_indicator_list()
> [snip]
>> opal_get_indicator_list ->     opal_get_indices -> sysfs led class dir
>> not exists -> return -1 ..
> [snip]
>>> - the other call is for opal_get_indices(),
>>>    which does not fail because scandir() should have returned 0
>>>    (no entries), not -1 in case of error.
>>
>> On error scandir returns -1 right?
>
> Yes, but is doesn't error in this case. The /sys/class/leds dir is
> present even without LED modules loaded (led-class.o is built-in.)


I was talking about the scenario where kernel won't create sysfs node. Probably 
we don't need to worry about this case for now.



>
>      # ls /sys/class/leds
>      #
>      # lsmod | grep -i led
>      #
>
>      # ./lpd/usysident
>      Some service indicators are not supported on this system.
>      Make sure 'leds_powernv' kernel module is loaded.

This may confuse the user. I will discard above message. (will send patch for that).

>      -B0-T0-L0     [on]
>      -B0-T0-L0     [off]
>
> And on empty dirs, its rc is 2 (for . and ..)
>
>      $ cat scandir.c
>      #include <dirent.h>
>      #include <stdlib.h>
>
>      int main() {
>          struct dirent **dirlist;
>          return scandir("emptydir", &dirlist, NULL, alphasort);
>      }
>
>      $ gcc -o scandir scandir.c
>
>      $ mkdir emptydir
>      $ ./scandir; echo $?
>      2
>      $ rmdir emptydir
>      $ ./scandir; echo $?
>      255
>
> Well, but with the patch for the section below, I guess this topic
> is no longer necessary :- ) as a patch makes opal_get_indicator_list()
> not to return early if opal_get_indices() fails.
>
>> So now we have number of sources to get LEDs (sysfs, ses_leds, marvel
>> leds).
>> If we fail to get LEDs from one source, lets continue with other sources
>> and build list instead of returning .
>
> Okay, submitting a patch for this. It checks whether any of the function
> calls initialized the list pointer to non-NULL (ie, allocated an entry
> for an indicator).
>
> Would you please check if the assumption to remove the fault mode check
> is correct? I'm afraid I don't understand enough about this to be sure.

I've tweaked your patch slightly and merged it.

-Vasant

Re: [Linux-diag-devel] [PATCH] lpd: opal: call all get_indices() in opal_get_indicator_list()

[Vasant H. <heg...@li...>]

On 11/18/2016 04:36 AM, Mauricio Faria de Oliveira wrote:
> Dont return early in opal_get_indicator_list() in case any of
> the get_indices() call happens to fail.  Let's go through all
> the calls to ensure any other pending/possible indicator that
> can work still goes into the list.
>
> Signed-off-by: Mauricio Faria de Oliveira <mau...@li...>
> ---
>   lpd/indicator_opal.c | 24 ++++++++----------------
>   1 file changed, 8 insertions(+), 16 deletions(-)
>
> diff --git a/lpd/indicator_opal.c b/lpd/indicator_opal.c
> index 712f57fea4e4..8a2003dc6696 100644
> --- a/lpd/indicator_opal.c
> +++ b/lpd/indicator_opal.c
> @@ -423,27 +423,15 @@ opal_get_indicator_mode(void)
>   static int
>   opal_get_indicator_list(int indicator, struct loc_code **list)
>   {
> -	int	rc;
> -
>   	/*
>   	 * We treat first indicator in fault indicator list as
>   	 * check log indicator. Hence parse attention indicator.
>   	 */
> -	if (indicator == LED_TYPE_FAULT) {
> -		rc = opal_get_indices(LED_TYPE_ATTN, list);
> -		if (rc)
> -			return rc;
> -	}
> +	if (indicator == LED_TYPE_FAULT)
> +		opal_get_indices(LED_TYPE_ATTN, list);
>
>   	/* Get OPAL indicator list */
> -	rc = opal_get_indices(indicator, list);
> -	if (rc)
> -		return rc;
> -
> -	/* FRU fault indicators are not supported in Guiding Light mode */
> -	if (indicator == LED_TYPE_FAULT &&
> -	    operating_mode == LED_MODE_GUIDING_LIGHT)
> -		return rc;


I've retained above check and merged as 53f12274.

-Vasant

[Linux-diag-devel] [PATCH 3/3] lpd: Check for device tree property

[Vasant H. <heg...@li...>]

Not all OPAL system supports OPAL leds. Check the existence of DT
property before using. Also if DT property doesn't exists, then
assume its in Light Path mode.

Signed-off-by: Vasant Hegde <heg...@li...>
---
 lpd/indicator_opal.c |    7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/lpd/indicator_opal.c b/lpd/indicator_opal.c
index f9780fc..5fb4d75 100644
--- a/lpd/indicator_opal.c
+++ b/lpd/indicator_opal.c
@@ -378,6 +378,13 @@ opal_get_indicator_mode(void)
 	int	rc = 0;
 	int	fd;
 	int	readsz;
+	struct stat sbuf;
+
+	rc = stat(OPAL_LED_MODE_PROPERTY, &sbuf);
+	if (rc) {
+		operating_mode = LED_MODE_LIGHT_PATH;
+		return 0;
+	}
 
 	fd = open(OPAL_LED_MODE_PROPERTY, O_RDONLY);
 	if (fd == -1) {

[Linux-diag-devel] [PATCH 2/3] lpd: Remove redundant warning message

[Vasant H. <heg...@li...>]

We will be able to support indicators from other sources (like HDD)
even though OPAL platform indicators are not present.

Also leds_powernv driver is loaded automatically if we have platform
support.

Hence remove this message.

Signed-off-by: Vasant Hegde <heg...@li...>
---
 lpd/indicator_opal.c |    2 --
 1 file changed, 2 deletions(-)

diff --git a/lpd/indicator_opal.c b/lpd/indicator_opal.c
index 712f57f..f9780fc 100644
--- a/lpd/indicator_opal.c
+++ b/lpd/indicator_opal.c
@@ -314,8 +314,6 @@ opal_indicator_probe_led_class(void)
 		return 0;
 	}
 
-	fprintf(stderr, "Some service indicators are not supported on this system."
-		"\nMake sure 'leds_powernv' kernel module is loaded.\n");
 	close_sysfs_led_dir(led_dir);
 	return rc;
 }

[Linux-diag-devel] [PATCH 1/3] lpd: Validate get_indicator_for_loc_code() return value

[Vasant H. <heg...@li...>]

Just to be on safer side, validate get_indicator_for_loc_code() return
value.

Signed-off-by: Vasant Hegde <heg...@li...>
---
 lpd/lp_diag.c |    8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/lpd/lp_diag.c b/lpd/lp_diag.c
index cce8e87..c80f302 100644
--- a/lpd/lp_diag.c
+++ b/lpd/lp_diag.c
@@ -758,6 +758,14 @@ UI_commit(WINDOW *my_menu_win, MENU *my_menu,
 		if (!strcmp(desc, "loc")) {
 			name = (char *)item_name(items[i]);
 			loc = get_indicator_for_loc_code(ident_list, name);
+			if (!loc) {
+				 if (cur_state[i] == '+') {
+					 cur_state[i] = prev_state[i];
+					 err = 1;
+				 }
+				continue;
+			}
+
 			rc = get_indicator_state(LED_TYPE_IDENT, loc, &ident);
 			if (rc) {
 				if (cur_state[i] == '+') {

[Linux-diag-devel] [PATCH] lpd: opal: call all get_indices() in opal_get_indicator_list()

[Mauricio F. de O. <mau...@li...>]

Dont return early in opal_get_indicator_list() in case any of
the get_indices() call happens to fail.  Let's go through all
the calls to ensure any other pending/possible indicator that
can work still goes into the list.

Signed-off-by: Mauricio Faria de Oliveira <mau...@li...>
---
 lpd/indicator_opal.c | 24 ++++++++----------------
 1 file changed, 8 insertions(+), 16 deletions(-)

diff --git a/lpd/indicator_opal.c b/lpd/indicator_opal.c
index 712f57fea4e4..8a2003dc6696 100644
--- a/lpd/indicator_opal.c
+++ b/lpd/indicator_opal.c
@@ -423,27 +423,15 @@ opal_get_indicator_mode(void)
 static int
 opal_get_indicator_list(int indicator, struct loc_code **list)
 {
-	int	rc;
-
 	/*
 	 * We treat first indicator in fault indicator list as
 	 * check log indicator. Hence parse attention indicator.
 	 */
-	if (indicator == LED_TYPE_FAULT) {
-		rc = opal_get_indices(LED_TYPE_ATTN, list);
-		if (rc)
-			return rc;
-	}
+	if (indicator == LED_TYPE_FAULT)
+		opal_get_indices(LED_TYPE_ATTN, list);
 
 	/* Get OPAL indicator list */
-	rc = opal_get_indices(indicator, list);
-	if (rc)
-		return rc;
-
-	/* FRU fault indicators are not supported in Guiding Light mode */
-	if (indicator == LED_TYPE_FAULT &&
-	    operating_mode == LED_MODE_GUIDING_LIGHT)
-		return rc;
+	opal_get_indices(indicator, list);
 
 	/* SES indicators */
 	get_ses_indices(indicator, list);
@@ -451,7 +439,11 @@ opal_get_indicator_list(int indicator, struct loc_code **list)
 	/* Marvell HDD LEDs (indicators) */
 	get_mv_indices(indicator, list);
 
-	return rc;
+	/*
+	 * The list pointer (*list) is initially NULL.
+	 * If it's not-NULL here, we found indicators.
+	 */
+	return !(*list);
 }
 
 /**
-- 
1.8.3.1

Re: [Linux-diag-devel] [PATCH v3 0/3] Support for Marvell SATA HDD LEDs on IBM Power System S822LC for HPC

[Mauricio F. de O. <mau...@li...>]

Hi Vasant,

On 11/17/2016 12:42 PM, Vasant Hegde wrote:
>>  > - opal_get_indicator_mode()
>>  >   We need to add check in this function.
>>
>> Can you clarify this request?

> Now that we have special case, we should fix this function.

Sorry; even w/ your explanation I didn't get why/what exactly
needs to be fixed in this function. Can you say it for dummies?

>>  > - opal_get_indicator_list()
[snip]
> opal_get_indicator_list ->     opal_get_indices -> sysfs led class dir
> not exists -> return -1 ..
[snip]
>> - the other call is for opal_get_indices(),
>>    which does not fail because scandir() should have returned 0
>>    (no entries), not -1 in case of error.
>
> On error scandir returns -1 right?

Yes, but is doesn't error in this case. The /sys/class/leds dir is
present even without LED modules loaded (led-class.o is built-in.)

	# ls /sys/class/leds
	#
	# lsmod | grep -i led
	#

	# ./lpd/usysident
	Some service indicators are not supported on this system.
	Make sure 'leds_powernv' kernel module is loaded.
	-B0-T0-L0	 [on]
	-B0-T0-L0	 [off]

And on empty dirs, its rc is 2 (for . and ..)

	$ cat scandir.c
	#include <dirent.h>
	#include <stdlib.h>

	int main() {
		struct dirent **dirlist;
		return scandir("emptydir", &dirlist, NULL, alphasort);
	}

	$ gcc -o scandir scandir.c

	$ mkdir emptydir
	$ ./scandir; echo $?
	2
	$ rmdir emptydir
	$ ./scandir; echo $?
	255

Well, but with the patch for the section below, I guess this topic
is no longer necessary :- ) as a patch makes opal_get_indicator_list()
not to return early if opal_get_indices() fails.

> So now we have number of sources to get LEDs (sysfs, ses_leds, marvel
> leds).
> If we fail to get LEDs from one source, lets continue with other sources
> and build list instead of returning .

Okay, submitting a patch for this. It checks whether any of the function
calls initialized the list pointer to non-NULL (ie, allocated an entry
for an indicator).

Would you please check if the assumption to remove the fault mode check
is correct? I'm afraid I don't understand enough about this to be sure.

Thanks!



-- 
Mauricio Faria de Oliveira
IBM Linux Technology Center

Re: [Linux-diag-devel] [PATCH v3 0/3] Support for Marvell SATA HDD LEDs on IBM Power System S822LC for HPC

[Vasant H. <heg...@li...>]

On 11/16/2016 09:47 PM, Mauricio Faria de Oliveira wrote:
> Hi Vasant,
>
>
> On 11/13/2016 01:24 PM, Vasant Hegde wrote:
>> Overall patchset looks good.. Had few minor issues which I've fixed
>> locally. [snip]
>
> Okay, I see the if-style changes in mv_unmap_bar5(), a couple places where I
> forgot to remove or add a return statement (sorry, I had the
> impression I fixed it..) and header guards in indicator_marvell.h...
> nice catch! Plus the vfprintf/printf change in debug messages patch.
>
> Thanks for handling those!

:-)

Generally if its trivial changes I do it myself rather than asking for another 
respin. That way we can save some time.


>
>  > [snip] We still have few issues to fix.. Since I forgot to mention
>  > this in previous iteration and also it can come as separate patch,
>  > I've merged this patchset.
>
> Cool, thanks.
>
>  > - opal_get_indicator_mode()
>  >   We need to add check in this function.
>
> Can you clarify this request?
>
> AFAICT opal_get_indicator_mode() is only called for LED_TYPE_FAULT,
> but the marvell LEDs are LED_TYPE_IDENT only.
>
>      main() @ usysident.c:

This is what happens if  I merge patches over weekend!

So we also have lp_diag (ncurses based UI)...which uses same functions.

Basically it calls get_indicator_mode() to LED operating mode for operating on 
fault indicator.

Also operating_mode variable is set by get_indicator_mode() and then used by 
opal_get_indicator_list().
Original code always assumed system level LEDs are supported always.

Now that we have special case, we should fix this function.

>
>          if (indicator == LED_TYPE_FAULT) {
>                  rc = get_indicator_mode();
>                  if (rc)
>                          return rc;
>          }
>
>
>  > - opal_get_indicator_list()
>  >   It worked today because we had sysfs leds on system. May be we should
>  > fix this function continue even though we fail  to get led list from
>  > sysfs led class.
>
> I believe it still works w/out any LED class modules; I recall testing
> in that scenario.


opal_get_indicator_list -> 	opal_get_indices -> sysfs led class dir not exists 
-> return -1 ..

>
> I'd guess it's because
> - 2 of the 3 early 'return rc' calls are for LED_TYPE_FAULT
>   (this uses LED_TYPE_IDENT),
> - the other call is for opal_get_indices(),
>    which does not fail because scandir() should have returned 0
>    (no entries), not -1 in case of error.

On error scandir returns -1 right?

>
> Anyway, do you mind explaining the expected behavior a bit, so that I
> can write the changes correctly? I have this question..

So now we have number of sources to get LEDs (sysfs, ses_leds, marvel leds).
If we fail to get LEDs from one source, lets continue with other sources and 
build list instead of returning .



>
> Should that function really return early (w/out reaching the 'ses' and
> 'marvell' get_indices calls)...

No.. we should just continue and list from other source.


Vasant

Re: [Linux-diag-devel] [PATCH v3 0/3] Support for Marvell SATA HDD LEDs on IBM Power System S822LC for HPC

[Mauricio F. de O. <mau...@li...>]

Hi Vasant,


On 11/13/2016 01:24 PM, Vasant Hegde wrote:
> Overall patchset looks good.. Had few minor issues which I've fixed
> locally. [snip]

Okay, I see the if-style changes in mv_unmap_bar5(), a couple places 
where I forgot to remove or add a return statement (sorry, I had the
impression I fixed it..) and header guards in indicator_marvell.h...
nice catch! Plus the vfprintf/printf change in debug messages patch.

Thanks for handling those!

 > [snip] We still have few issues to fix.. Since I forgot to mention
 > this in previous iteration and also it can come as separate patch,
 > I've merged this patchset.

Cool, thanks.

 > - opal_get_indicator_mode()
 >   We need to add check in this function.

Can you clarify this request?

AFAICT opal_get_indicator_mode() is only called for LED_TYPE_FAULT,
but the marvell LEDs are LED_TYPE_IDENT only.

	main() @ usysident.c:

         if (indicator == LED_TYPE_FAULT) {
                 rc = get_indicator_mode();
                 if (rc)
                         return rc;
         }


 > - opal_get_indicator_list()
 >   It worked today because we had sysfs leds on system. May be we should
 > fix this function continue even though we fail  to get led list from
 > sysfs led class.

I believe it still works w/out any LED class modules; I recall testing
in that scenario.

I'd guess it's because
- 2 of the 3 early 'return rc' calls are for LED_TYPE_FAULT
  (this uses LED_TYPE_IDENT),
- the other call is for opal_get_indices(),
   which does not fail because scandir() should have returned 0
   (no entries), not -1 in case of error.

Anyway, do you mind explaining the expected behavior a bit, so that I
can write the changes correctly? I have this question..

Should that function really return early (w/out reaching the 'ses' and
'marvell' get_indices calls)...
- only in the 2 checks for LED_TYPE_FAULT, but not in the check for
   opal_get_indices()...
- or not return early in case of failure in all of them?

Thank you!

-- 
Mauricio Faria de Oliveira
IBM Linux Technology Center