netdev - Re: [PATCH v9 net-next 04/14] pds_core: add devlink health facilities

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <c41dc420-e434-9944-584e-7ac9f915cd4a@amd.com>
Date:   Mon, 10 Apr 2023 12:18:14 -0700
From:   Shannon Nelson <shannon.nelson@....com>
To:     Leon Romanovsky <leon@...nel.org>
Cc:     brett.creeley@....com, davem@...emloft.net, netdev@...r.kernel.org,
        kuba@...nel.org, drivers@...sando.io, jiri@...nulli.us
Subject: Re: [PATCH v9 net-next 04/14] pds_core: add devlink health facilities

On 4/9/23 4:54 AM, Leon Romanovsky wrote:
> 
> On Thu, Apr 06, 2023 at 04:41:33PM -0700, Shannon Nelson wrote:
>> Add devlink health reporting on top of our fw watchdog.
>>
>> Example:
>>    # devlink health show pci/0000:2b:00.0 reporter fw
>>    pci/0000:2b:00.0:
>>      reporter fw
>>        state healthy error 0 recover 0
>>    # devlink health diagnose pci/0000:2b:00.0 reporter fw
>>     Status: healthy State: 1 Generation: 0 Recoveries: 0
>>
>> Signed-off-by: Shannon Nelson <shannon.nelson@....com>
>> ---
>>   .../device_drivers/ethernet/amd/pds_core.rst  | 12 ++++++
>>   drivers/net/ethernet/amd/pds_core/Makefile    |  1 +
>>   drivers/net/ethernet/amd/pds_core/core.c      |  6 +++
>>   drivers/net/ethernet/amd/pds_core/core.h      |  6 +++
>>   drivers/net/ethernet/amd/pds_core/devlink.c   | 37 +++++++++++++++++++
>>   drivers/net/ethernet/amd/pds_core/main.c      | 22 +++++++++++
>>   6 files changed, 84 insertions(+)
>>   create mode 100644 drivers/net/ethernet/amd/pds_core/devlink.c
> 
> <...>
> 
>> +int pdsc_fw_reporter_diagnose(struct devlink_health_reporter *reporter,
>> +                           struct devlink_fmsg *fmsg,
>> +                           struct netlink_ext_ack *extack)
>> +{
>> +     struct pdsc *pdsc = devlink_health_reporter_priv(reporter);
>> +     int err = 0;
>> +
>> +     if (test_bit(PDSC_S_FW_DEAD, &pdsc->state))
> 
> How is this check protected from race with your health workqueue added
> in previous patch?
> 
>> +             err = devlink_fmsg_string_pair_put(fmsg, "Status", "dead");
>> +     else if (!pdsc_is_fw_good(pdsc))
> 
> Same question.

Yes, it would be good to wrap these in the config_lock.


> 
>> +             err = devlink_fmsg_string_pair_put(fmsg, "Status", "unhealthy");
>> +     else
>> +             err = devlink_fmsg_string_pair_put(fmsg, "Status", "healthy");
>> +     if (err)
>> +             return err;
> 
> Thanks