netdev - Re: [PATCH RFC v4 net-next 02/13] pds_core: add devcmd device interfaces

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: <ZAmm/bUs8FbWn+wp@nanopsycho>
Date:   Thu, 9 Mar 2023 10:29:33 +0100
From:   Jiri Pirko <jiri@...nulli.us>
To:     Shannon Nelson <shannon.nelson@....com>
Cc:     brett.creeley@....com, davem@...emloft.net, netdev@...r.kernel.org,
        kuba@...nel.org, drivers@...sando.io, leon@...nel.org
Subject: Re: [PATCH RFC v4 net-next 02/13] pds_core: add devcmd device
 interfaces

Thu, Mar 09, 2023 at 03:05:13AM CET, shannon.nelson@....com wrote:
>On 3/8/23 1:37 AM, Jiri Pirko wrote:
>> Wed, Mar 08, 2023 at 06:12:59AM CET, shannon.nelson@....com wrote:

[..]


>> > +static int identity_show(struct seq_file *seq, void *v)
>> > +{
>> > +      struct pdsc *pdsc = seq->private;
>> > +      struct pds_core_dev_identity *ident;
>> > +      int vt;
>> > +
>> > +      ident = &pdsc->dev_ident;
>> > +
>> > +      seq_printf(seq, "asic_type:        0x%x\n", pdsc->dev_info.asic_type);
>> > +      seq_printf(seq, "asic_rev:         0x%x\n", pdsc->dev_info.asic_rev);
>> > +      seq_printf(seq, "serial_num:       %s\n", pdsc->dev_info.serial_num);
>> > +      seq_printf(seq, "fw_version:       %s\n", pdsc->dev_info.fw_version);
>> 
>> What is the exact reason of exposing this here and not trought well
>> defined devlink info interface?
>
>These do show up in devlink dev info eventually, but that isn't for another
>couple of patches.  This gives us info here for debugging the earlier patches
>if needed.

Implement it properly from the start and avoid these, please.


>
>> 
>> 
>> > +      seq_printf(seq, "fw_status:        0x%x\n",
>> > +                 ioread8(&pdsc->info_regs->fw_status));
>> > +      seq_printf(seq, "fw_heartbeat:     0x%x\n",
>> > +                 ioread32(&pdsc->info_regs->fw_heartbeat));
>> > +


[..]

>> > +static int pdsc_identify(struct pdsc *pdsc)
>> > +{
>> > +      struct pds_core_drv_identity drv = { 0 };
>> > +      size_t sz;
>> > +      int err;
>> > +
>> > +      drv.drv_type = cpu_to_le32(PDS_DRIVER_LINUX);
>> > +      drv.kernel_ver = cpu_to_le32(LINUX_VERSION_CODE);
>> > +      snprintf(drv.kernel_ver_str, sizeof(drv.kernel_ver_str),
>> > +               "%s %s", utsname()->release, utsname()->version);
>> > +      snprintf(drv.driver_ver_str, sizeof(drv.driver_ver_str),
>> > +               "%s %s", PDS_CORE_DRV_NAME, utsname()->release);
>> 
>> Why exactly are you doing this? Looks very wrong.
>
>This helps us when debugging on the DSC side - we can see which host driver
>is using the interface (PXE, Linux, etc).  This is modeled from what we have
>in our out-of-tree ionic driver interface, but I need to trim it down to be
>less snoopy.

Device should not care who is on the other side in my opinion. Jakub,
any thoughts?

>
>sln