| Message ID | d3da32136ba31c553fa267381eb6a01903525814.1666603600.git.ecree.xilinx@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | sfc: add basic flower matches to offload | expand |
On Mon, 24 Oct 2022 10:29:21 +0100 edward.cree@amd.com wrote: > From: Edward Cree <ecree.xilinx@gmail.com> > > Offloaded TC rules always match on recirc_id in the MAE, so we should > check that the MAE reported support for this match before attempting > to insert the rule. > > Fixes: d902e1a737d4 ("sfc: bare bones TC offload on EF100") This commit made it to net, needs to go separately there. > +/* Validate field mask against hardware capabilities. May return from caller */ > +#define CHECK(_mcdi, _field) do { \ > + enum mask_type typ = classify_mask((const u8 *)&mask->_field, \ > + sizeof(mask->_field)); \ > + \ > + rc = efx_mae_match_check_cap_typ(supported_fields[MAE_FIELD_ ## _mcdi],\ > + typ); \ > + if (rc) { \ > + NL_SET_ERR_MSG_FMT_MOD(extack, \ > + "No support for %s mask in field %s", \ > + mask_type_name(typ), #_field); \ > + return rc; \ We still don't allow flow control to hide inside macros. You add the checks next to each other (looking at the next patch) so you can return rc from the macro and easily combine the checks into one large if statement. Result - close to ~1 line per check. > + } \ > +} while (0) > + > int efx_mae_match_check_caps(struct efx_nic *efx, > const struct efx_tc_match_fields *mask, > struct netlink_ext_ack *extack) > @@ -269,6 +284,7 @@ int efx_mae_match_check_caps(struct efx_nic *efx, > mask_type_name(ingress_port_mask_type)); > return rc; > } > + CHECK(RECIRC_ID, recirc_id); > return 0; I think the #undef leaked into the next patch.
On 26/10/2022 03:40, Jakub Kicinski wrote: > On Mon, 24 Oct 2022 10:29:21 +0100 edward.cree@amd.com wrote: >> From: Edward Cree <ecree.xilinx@gmail.com> >> >> Offloaded TC rules always match on recirc_id in the MAE, so we should >> check that the MAE reported support for this match before attempting >> to insert the rule. >> >> Fixes: d902e1a737d4 ("sfc: bare bones TC offload on EF100") > > This commit made it to net, needs to go separately there. Hmm I might just drop the fixes tag; the cited commit isn't really broken, just suboptimal. > We still don't allow flow control to hide inside macros. > > You add the checks next to each other (looking at the next patch) > so you can return rc from the macro and easily combine the checks > into one large if statement. Result - close to ~1 line per check. Ah yeah, I forgot statement-like macros can still return values. Will fix, thanks. -ed
On Tue, 1 Nov 2022 13:30:10 +0000 Edward Cree wrote: > > This commit made it to net, needs to go separately there. > > Hmm I might just drop the fixes tag; the cited commit isn't really > broken, just suboptimal. Can you describe the current behavior is? Isn't the driver accepting rules it can't correctly offload?
On 01/11/2022 15:21, Jakub Kicinski wrote: > On Tue, 1 Nov 2022 13:30:10 +0000 Edward Cree wrote: >>> This commit made it to net, needs to go separately there. >> >> Hmm I might just drop the fixes tag; the cited commit isn't really >> broken, just suboptimal. > > Can you describe the current behavior is? Isn't the driver accepting > rules it can't correctly offload? The rule will pass the checks here, but then when we make the MCDI call to install it in hardware, MC_CMD_MAE_ACTION_RULE_INSERT will evoke an error response from the firmware, so the TC_SETUP_CLSFLOWER callback will ultimately return an error to the kernel as it should. The advantage of having these checks in the driver is that we get a useful error message rather than just "Failed to insert rule in hw", and also save the round trip across the PCIe bus to firmware.
On Tue, 1 Nov 2022 15:41:13 +0000 Edward Cree wrote: > > Can you describe the current behavior is? Isn't the driver accepting > > rules it can't correctly offload? > > The rule will pass the checks here, but then when we make the MCDI call > to install it in hardware, MC_CMD_MAE_ACTION_RULE_INSERT will evoke an > error response from the firmware, so the TC_SETUP_CLSFLOWER callback > will ultimately return an error to the kernel as it should. > The advantage of having these checks in the driver is that we get a > useful error message rather than just "Failed to insert rule in hw", > and also save the round trip across the PCIe bus to firmware. I see, net-next sounds good then. Do put this info into the commit message, please.
diff --git a/drivers/net/ethernet/sfc/mae.c b/drivers/net/ethernet/sfc/mae.c index 6f472ea0638a..4ceb8c8f5548 100644 --- a/drivers/net/ethernet/sfc/mae.c +++ b/drivers/net/ethernet/sfc/mae.c @@ -250,6 +250,21 @@ static int efx_mae_match_check_cap_typ(u8 support, enum mask_type typ) } } +/* Validate field mask against hardware capabilities. May return from caller */ +#define CHECK(_mcdi, _field) do { \ + enum mask_type typ = classify_mask((const u8 *)&mask->_field, \ + sizeof(mask->_field)); \ + \ + rc = efx_mae_match_check_cap_typ(supported_fields[MAE_FIELD_ ## _mcdi],\ + typ); \ + if (rc) { \ + NL_SET_ERR_MSG_FMT_MOD(extack, \ + "No support for %s mask in field %s", \ + mask_type_name(typ), #_field); \ + return rc; \ + } \ +} while (0) + int efx_mae_match_check_caps(struct efx_nic *efx, const struct efx_tc_match_fields *mask, struct netlink_ext_ack *extack) @@ -269,6 +284,7 @@ int efx_mae_match_check_caps(struct efx_nic *efx, mask_type_name(ingress_port_mask_type)); return rc; } + CHECK(RECIRC_ID, recirc_id); return 0; }