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)
snet_vdpa-$(CONFIG_SNET_VDPA) += snet_hwmon.o
#endif
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
}
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?
> 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.
> 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.
> Memory allocation failures are not commonly logged since the low level code
> already does that.
Right, I'll remove the error log.
Alvaro
