Re: [PATCH 3/3 v4] virtio: vdpa: new SolidNET DPU driver.

[Guenter Roeck <linux@roeck-us.net>]

On 12/12/22 08:29, Alvaro Karsz wrote:
> Hi Guenter,
> Thanks for your comments.
> 
>> This is wrong. It should be possible to build the driver without it, and
>> without forcing everyone to enable hwmon just to get support for this device -
>> even more so since hwmon support is explicitly marked as optional below.
>> Why force people to compile it if it is not mandatory ?
>>
>>
>> Yes, I know, "select HWMON" is done elsewhere as well, but it is just as wrong
>> there. No one should be forced to enable HWMON support just to get, say, support
>> for the IDT PCIe-switch Non-Transparent Bridge.
> 
> 
> You have a good point.
> I will remove it from the Kconfig file, and I will add:
> #if IS_REACHABLE(CONFIG_HWMON)
> in relevant places
> 
> Something like:
> 
> solidrun/Makefile:
> obj-$(CONFIG_SNET_VDPA) += snet_vdpa.o
> snet_vdpa-$(CONFIG_SNET_VDPA) += snet_main.o
> #if IS_REACHABLE(CONFIG_HWMON)

I don't think that works in Makefiles. It would have to be ifdef.

> snet_vdpa-$(CONFIG_SNET_VDPA) += snet_hwmon.o
> #endif
> 

Even better would be a separate CONFIG_SNET_VDPA_HWMON Kconfig option.

> solidrun/snet_main.c, snet_vdpa_probe_pf function:
> 
> if (PSNET_FLAG_ON(psnet, SNET_CFG_FLAG_HWMON)) {
> #if IS_REACHABLE(CONFIG_HWMON)
>          psnet_create_hwmon(pdev);
> #else
>          SNET_ERR(pdev, "Can't start HWMON, CONFIG_HWMON is not reachable\n");
> #endif

Per your own statement, that is not an error, and thus should not be logged
as one.

> }
> 
> solidrun/snet_vdpa.h, snet_vdpa_probe_pf function:
> #if IS_REACHABLE(CONFIG_HWMON)
> void psnet_create_hwmon(struct pci_dev *pdev);
> #endif
> 
> What do you think?
> 

It would be much better to add a shim function in the include file.

There should be a dependency

	depends on HWMON || HWMON=n

in Kconfig, and the shim function would then be

#if IS_REACHABLE(CONFIG_HWMON)
void psnet_create_hwmon(struct pci_dev *pdev);
#else
void psnet_create_hwmon(struct pci_dev *pdev) {}
#endif

>> I do not see why the second include would be needed.
> 
> You're right, I'll remove it.
> 
>>
>> Tpecast seems unnecessary.
> 
> I'll remove it.
> 
>> Kind of obvious.
> 
> Ok, I'll remove the comment.
> 
>> Badly misleading indent. No idea why checkpatch doesn't report it.
>>
>>
>> That makes me wonder: Did you not run checkpatch --strict, or did you choose
>> to ignore what it reports ?
> 
> I did run checkpatch (without --strict).
> I tried now with --strict. and I'm not getting any indent
> errors/warnings, this is strange..
> I will fix it.
> 

I referred to the other problems it reports, such as using macro arguments
without ().

>> FWIW, a _hwmon ending in a hwmon driver device name is redundant.
>> What else would it be ? Why not just use pci_name() ?
> 
> I'll change it to "snet_%s", pci_name(pdev)
> 
>> devm_hwmon_device_register_with_info() returns an ERR_PTR on error,
>> not NULL.
> 
> Ok, I'll fix it.
> 
>> I hope you know what you are doing here. This may result in people wondering
>> why hwmon support doesn't work if they expect it to work. No one looks
>> into the kernel log. Besides, ignoring the error doesn't really help
>> much because that error return means that something serious is wrong.
> 
> You have a point, but the hwmon is not the "main" functionality of
> this device, so I don't want to fail the entire device because of a
> "side" functionality.
> Now that the SNET vdpa driver doesn't select CONFIG_HWMON, we may have
> a situation when the SNET_CFG_FLAG_HWMON flag is set, but the kernel
> is compiled without CONFIG_HWMON.
> I don't think that I should fail probe in this case.
> 
>> Wow, a 5-second hot loop. Not my responsibility to accept or reject this
>> part of the code, but personally I think this is completely unaccceptable.
> 
> The SNET DPU may require some time to become ready.
> If the driver is compiled as a module, this is not a problem, but if
> the driver is builtin in the kernel, we may need to wait a little for
> the DPU.
> But you're right, 5 secs is indeed a big number, I'll change it to 2 secs.
> 

That isn't the point. A 2-second hot loop is just as bad.
There should be a usleep_range() or similar between loop iterations.

Guenter

>> Memory allocation failures are not commonly logged since the low level code
>> already does that.
> 
> Right, I'll remove the error log.
> 
> Alvaro