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