From: Florian Fainelli <f.fainelli@gmail•com>
To: Pablo Neira Ayuso <pablo@netfilter•org>, netdev@vger•kernel.org
Cc: davem@davemloft•net, thomas.lendacky@amd•com,
ariel.elior@cavium•com, michael.chan@broadcom•com,
santosh@chelsio•com, madalin.bucur@nxp•com,
yisen.zhuang@huawei•com, salil.mehta@huawei•com,
jeffrey.t.kirsher@intel•com, tariqt@mellanox•com,
saeedm@mellanox•com, jiri@mellanox•com, idosch@mellanox•com,
ganeshgr@chelsio•com, jakub.kicinski@netronome•com,
linux-net-drivers@solarflare•com, peppe.cavallaro@st•com,
alexandre.torgue@st•com, joabreu@synopsys•com,
grygorii.strashko@ti•com, andrew@lunn•ch,
vivien.didelot@savoirfairelinux•com
Subject: Re: [PATCH RFC,net-next 10/10] dsa: bcm_sf2: use flow_rule infrastructure
Date: Wed, 26 Sep 2018 11:41:30 -0700 [thread overview]
Message-ID: <9db26fa8-e1b0-acc5-80e8-6443d089fe2e@gmail.com> (raw)
In-Reply-To: <20180925192001.2482-11-pablo@netfilter.org>
Hi Pablo,
On 09/25/2018 12:20 PM, Pablo Neira Ayuso wrote:
> Update this driver to use the flow_rule infrastructure, hence the same
> code to populate hardware IR can be used from ethtool_rx_flow and the
> cls_flower interfaces.
Thanks for doing the conversion, I believe we could change things a
little bit such that there are fewer things to audit for correctness,
see below:
>
> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter•org>
> ---
> drivers/net/dsa/bcm_sf2_cfp.c | 311 ++++++++++++++++++++++--------------------
> 1 file changed, 166 insertions(+), 145 deletions(-)
>
> diff --git a/drivers/net/dsa/bcm_sf2_cfp.c b/drivers/net/dsa/bcm_sf2_cfp.c
> index 47c5f272a084..9dace0e25a3a 100644
> --- a/drivers/net/dsa/bcm_sf2_cfp.c
> +++ b/drivers/net/dsa/bcm_sf2_cfp.c
> @@ -251,10 +251,12 @@ static int bcm_sf2_cfp_act_pol_set(struct bcm_sf2_priv *priv,
> }
>
> static void bcm_sf2_cfp_slice_ipv4(struct bcm_sf2_priv *priv,
> - struct ethtool_tcpip4_spec *v4_spec,
> + struct flow_rule *flow_rule,
> unsigned int slice_num,
> bool mask)
> {
> + struct flow_match_ipv4_addrs ipv4;
> + struct flow_match_ports ports;
> u32 reg, offset;
>
> /* C-Tag [31:24]
> @@ -268,41 +270,54 @@ static void bcm_sf2_cfp_slice_ipv4(struct bcm_sf2_priv *priv,
> offset = CORE_CFP_DATA_PORT(4);
> core_writel(priv, reg, offset);
>
> + flow_rule_match_ipv4_addrs(flow_rule, &ipv4);
> + flow_rule_match_ports(flow_rule, &ports);
> +
> /* UDF_n_A7 [31:24]
> * UDF_n_A6 [23:8]
> * UDF_n_A5 [7:0]
> */
> - reg = be16_to_cpu(v4_spec->pdst) >> 8;
> - if (mask)
> + if (mask) {
> + reg = be16_to_cpu(ports.mask->dst) >> 8;
> offset = CORE_CFP_MASK_PORT(3);
> - else
> + } else {
> + reg = be16_to_cpu(ports.key->dst) >> 8;
> offset = CORE_CFP_DATA_PORT(3);
> + }
For instance here, instead of having to assign "reg" differently, just
have an intermediate struct flow_match_ports variable that either points
to &ports.mask or &ports.key.
I quickly glanced through the changes, and I suspect that they are
correct, but there is unfortunately way too much code movement within
the functions which greatly hinders the ability to have complete
confidence in the changes :)
Can you find a way to only perform the ethtool_rx_flow_spec to flow_rule
conversion as close as possible from the point of use within the callee?
Thanks!
--
Florian
next prev parent reply other threads:[~2018-09-27 0:56 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-09-25 19:19 [PATCH RFC,net-next 00/10] add flow_rule infrastructure Pablo Neira Ayuso
2018-09-25 19:19 ` [PATCH RFC,net-next 01/10] flow_dissector: add flow_rule and flow_match structures and use them Pablo Neira Ayuso
2018-09-25 19:19 ` [PATCH RFC,net-next 02/10] net/mlx5e: allow two independent packet edit actions Pablo Neira Ayuso
2018-09-25 19:19 ` [PATCH RFC,net-next 03/10] flow_dissector: add flow action infrastructure Pablo Neira Ayuso
2018-09-25 19:19 ` [PATCH RFC,net-next 04/10] cls_flower: add translator to flow_action representation Pablo Neira Ayuso
2018-09-26 15:47 ` Jakub Kicinski
2018-09-25 19:19 ` [PATCH RFC,net-next 05/10] cls_flower: add statistics retrieval infrastructure and use it Pablo Neira Ayuso
2018-09-25 19:19 ` [PATCH RFC,net-next 06/10] drivers: net: use flow action infrastructure Pablo Neira Ayuso
2018-09-25 19:19 ` [PATCH RFC,net-next 07/10] cls_flower: don't expose TC actions to drivers anymore Pablo Neira Ayuso
2018-09-25 19:19 ` [PATCH RFC,net-next 08/10] flow_dissector: add wake-up-on-lan and queue to flow_action Pablo Neira Ayuso
2018-09-26 18:51 ` Florian Fainelli
2018-09-25 19:20 ` [PATCH RFC,net-next 09/10] flow_dissector: add basic ethtool_rx_flow_spec to flow_rule structure translator Pablo Neira Ayuso
2018-09-25 19:20 ` [PATCH RFC,net-next 10/10] dsa: bcm_sf2: use flow_rule infrastructure Pablo Neira Ayuso
2018-09-26 18:41 ` Florian Fainelli [this message]
2018-09-26 15:51 ` [PATCH RFC,net-next 00/10] add " Jakub Kicinski
2018-09-26 18:41 ` Florian Fainelli
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=9db26fa8-e1b0-acc5-80e8-6443d089fe2e@gmail.com \
--to=f.fainelli@gmail$(echo .)com \
--cc=alexandre.torgue@st$(echo .)com \
--cc=andrew@lunn$(echo .)ch \
--cc=ariel.elior@cavium$(echo .)com \
--cc=davem@davemloft$(echo .)net \
--cc=ganeshgr@chelsio$(echo .)com \
--cc=grygorii.strashko@ti$(echo .)com \
--cc=idosch@mellanox$(echo .)com \
--cc=jakub.kicinski@netronome$(echo .)com \
--cc=jeffrey.t.kirsher@intel$(echo .)com \
--cc=jiri@mellanox$(echo .)com \
--cc=joabreu@synopsys$(echo .)com \
--cc=linux-net-drivers@solarflare$(echo .)com \
--cc=madalin.bucur@nxp$(echo .)com \
--cc=michael.chan@broadcom$(echo .)com \
--cc=netdev@vger$(echo .)kernel.org \
--cc=pablo@netfilter$(echo .)org \
--cc=peppe.cavallaro@st$(echo .)com \
--cc=saeedm@mellanox$(echo .)com \
--cc=salil.mehta@huawei$(echo .)com \
--cc=santosh@chelsio$(echo .)com \
--cc=tariqt@mellanox$(echo .)com \
--cc=thomas.lendacky@amd$(echo .)com \
--cc=vivien.didelot@savoirfairelinux$(echo .)com \
--cc=yisen.zhuang@huawei$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox