On 6/1/24 9:06 PM, Simon Horman wrote:
On Wed, May 29, 2024 at 11:59:07AM +0800, D. Wythe wrote:
From: "D. Wythe" <alibuda@xxxxxxxxxxxxxxxxx>
This patch allows to create smc socket via AF_INET,
similar to the following code,
/* create v4 smc sock */
v4 = socket(AF_INET, SOCK_STREAM, IPPROTO_SMC);
/* create v6 smc sock */
v6 = socket(AF_INET6, SOCK_STREAM, IPPROTO_SMC);
There are several reasons why we believe it is appropriate here:
1. For smc sockets, it actually use IPv4 (AF-INET) or IPv6 (AF-INET6)
address. There is no AF_SMC address at all.
2. Create smc socket in the AF_INET(6) path, which allows us to reuse
the infrastructure of AF_INET(6) path, such as common ebpf hooks.
Otherwise, smc have to implement it again in AF_SMC path.
Signed-off-by: D. Wythe <alibuda@xxxxxxxxxxxxxxxxx>
...
diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
...
@@ -3594,9 +3595,31 @@ static int __init smc_init(void)
goto out_lo;
}
+ rc = proto_register(&smc_inet_prot, 1);
+ if (rc) {
+ pr_err("%s: proto_register smc_inet_prot fails with %d\n", __func__, rc);
Hi,
FWIIW, my feeling is that if a log message includes __func__ then it should
be a debug level message, and even then I'm dubious about the value of
__func__: we do have many tools including dynamic tracing or pinpointing
problems.
So I would suggest rephrasing this message and dropping __func__.
Or maybe removing it entirely.
Or if not, lowering the priority of this message to debug.
If for some reason __func__ remains, please do consider wrapping
the line to 80c columns or less, which can be trivially done here
(please don't split the format string in any case).
Flagged by checkpatch.pl --max-line-length=80
Hi Simon,
Thank you very much for your feedback.
Allow me to briefly explain the reasons for using pr_err and __func__ here.
Regarding pr_err, the failure here leads to the failure of the module
loading, which is definitely an error-level message rather than a
debug-level one.
As for __func__, I must admit that the purpose here is simply to align
with the format of other error messages in smc_init(). In fact, I also
feel that the presence of
__func__ doesn't hold significant value because this error will only
occur within this function. It's meaningless information for both users
and kernel developers.
Perhaps a more suitable format would be “smc: xxx: %d”.
However, if changes are needed, I think they should be made across the
board in order to maintain a consistent style. Maybe this can be
addressed by
submitting a new patch after this patch. @Wenjia, what do you think?
Therefore, for now, I would like to wrap this line to not exceed 80
characters, to ensure it can pass the checkpatch.pl.
What do you think?
Best wishes,
D. Wythe
+ goto out_ulp;
+ }
+ inet_register_protosw(&smc_inet_protosw);
+#if IS_ENABLED(CONFIG_IPV6)
+ rc = proto_register(&smc_inet6_prot, 1);
+ if (rc) {
+ pr_err("%s: proto_register smc_inet6_prot fails with %d\n", __func__, rc);
Here too.
+ goto out_inet_prot;
+ }
+ inet6_register_protosw(&smc_inet6_protosw);
+#endif
...
