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
