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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [DMA Engine]     [Linux Coverity]     [Linux USB]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Greybus]

  Powered by Linux