On 17/03/2023 22.21, Stanislav Fomichev wrote:
On 03/17, Jesper Dangaard Brouer wrote:
When driver doesn't implement a bpf_xdp_metadata kfunc the fallback
implementation returns EOPNOTSUPP, which indicate device driver doesn't
implement this kfunc.
Currently many drivers also return EOPNOTSUPP when the hint isn't
available, which is inconsistent from an API point of view. Instead
change drivers to return ENODATA in these cases.
There can be natural cases why a driver doesn't provide any hardware
info for a specific hint, even on a frame to frame basis (e.g. PTP).
Lets keep these cases as separate return codes.
When describing the return values, adjust the function kernel-doc layout
to get proper rendering for the return values.
Signed-off-by: Jesper Dangaard Brouer <brouer@xxxxxxxxxx>
I don't remember whether the previous discussion ended in something?
IIRC Martin was preferring to use xdp-features for this instead?
IIRC Martin asked for a second vote/opinion to settle the vote.
The xdp-features use is orthogonal and this patch does not prohibit the
later implementation of xdp-features, to detect if driver doesn't
implement kfuncs via using global vars. Not applying this patch leaves
the API in an strange inconsistent state, because of an argument that in
the *future* we can use xdp-features to solve *one* of the discussed
use-cases for another return code.
I argued for a practical PTP use-case where not all frames contain the
PTP timestamp. This patch solve this use-case *now*, so I don't see why
we should stall solving this, because of a "future" feature we might
never get around to implement, which require the user to use global vars.
Personally I'm fine with having this convention, but I'm not sure how well
we'll be able to enforce them. (In general, I'm not a fan of userspace
changing it's behavior based on errno. If it's mostly for
debugging/development - seems ok)
We enforce the API by documenting the return behavior, like below. If a
driver violate this, then we will fix the driver code with a fixes tag.
My ask is simply let not have ambiguous return codes.
---
Documentation/networking/xdp-rx-metadata.rst | 7 +++++--
drivers/net/ethernet/mellanox/mlx4/en_rx.c | 4 ++--
drivers/net/ethernet/mellanox/mlx5/core/en/xdp.c | 4 ++--
drivers/net/veth.c | 4 ++--
net/core/xdp.c | 10 ++++++++--
5 files changed, 19 insertions(+), 10 deletions(-)
[...]
diff --git a/net/core/xdp.c b/net/core/xdp.c
index 8d3ad315f18d..7133017bcd74 100644
--- a/net/core/xdp.c
+++ b/net/core/xdp.c
@@ -705,7 +705,10 @@ __diag_ignore_all("-Wmissing-prototypes",
* @ctx: XDP context pointer.
* @timestamp: Return value pointer.
*
- * Returns 0 on success or ``-errno`` on error.
+ * Return:
+ * * Returns 0 on success or ``-errno`` on error.
+ * * ``-EOPNOTSUPP`` : means device driver does not implement kfunc
+ * * ``-ENODATA`` : means no RX-timestamp available for this frame
*/
__bpf_kfunc int bpf_xdp_metadata_rx_timestamp(const struct xdp_md
*ctx, u64 *timestamp)
{
@@ -717,7 +720,10 @@ __bpf_kfunc int
bpf_xdp_metadata_rx_timestamp(const struct xdp_md *ctx, u64 *tim
* @ctx: XDP context pointer.
* @hash: Return value pointer.
*
- * Returns 0 on success or ``-errno`` on error.
+ * Return:
+ * * Returns 0 on success or ``-errno`` on error.
+ * * ``-EOPNOTSUPP`` : means device driver doesn't implement kfunc
+ * * ``-ENODATA`` : means no RX-hash available for this frame
*/
__bpf_kfunc int bpf_xdp_metadata_rx_hash(const struct xdp_md *ctx,
u32 *hash)
{
