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

[Jiri Pirko <jiri@resnulli.us>]

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