From: Petr Machata <petrm@mellanox•com>
To: Jakub Kicinski <kuba@kernel•org>
Cc: netdev@vger•kernel.org, Pablo Neira Ayuso <pablo@netfilter•org>,
davem@davemloft•net, jiri@mellanox•com, mlxsw@mellanox•com,
michael.chan@broadcom•com, saeedm@mellanox•com, leon@kernel•org,
kadlec@netfilter•org, fw@strlen•de, jhs@mojatatu•com,
xiyou.wangcong@gmail•com, simon.horman@netronome•com,
Ido Schimmel <idosch@mellanox•com>
Subject: Re: [PATCH net-next v2 01/13] net: sched: Pass qdisc reference in struct flow_block_offload
Date: Mon, 13 Jul 2020 13:46:08 +0200 [thread overview]
Message-ID: <87k0z7fxj3.fsf@mellanox.com> (raw)
In-Reply-To: <20200711112256.4153f2d9@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Jakub Kicinski <kuba@kernel•org> writes:
> On Sat, 11 Jul 2020 00:55:03 +0300 Petr Machata wrote:
>> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c
>> index 0a9a4467d7c7..e82e5cf64d61 100644
>> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c
>> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c
>> @@ -1911,7 +1911,7 @@ static int bnxt_tc_setup_indr_block(struct net_device *netdev, struct bnxt *bp,
>> block_cb = flow_indr_block_cb_alloc(bnxt_tc_setup_indr_block_cb,
>> cb_priv, cb_priv,
>> bnxt_tc_setup_indr_rel, f,
>> - netdev, data, bp, cleanup);
>> + netdev, sch, data, bp, cleanup);
>
> nit: the number of arguments is getting out of hand here, perhaps it's
> time to pass a structure in.
I would say that adding a distinctly-typed and distinctly-named argument
does not make the other arguments less clear, or increase the risk of
mixing them up in a way that the compiler would not catch.
Taken from another angle, the argument list is not passed through layers
of other functions, so the structure would be assembled in
bnxt_tc_setup_indr_block() and then unpacked again in
flow_indr_block_cb_alloc(). Nothing is saved in that regard either.
Also, flow_indr_block_cb_alloc() is a simple function. When a structure
needs to be introduced to carry the arguments, it might be more
straightforward to just open-code it.
>> if (IS_ERR(block_cb)) {
>> list_del(&cb_priv->list);
>> kfree(cb_priv);
next prev parent reply other threads:[~2020-07-13 11:46 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-07-10 21:55 [PATCH net-next v2 00/13] mlxsw: Add support for buffer drops mirroring Petr Machata
2020-07-10 21:55 ` [PATCH net-next v2 01/13] net: sched: Pass qdisc reference in struct flow_block_offload Petr Machata
2020-07-11 18:22 ` Jakub Kicinski
2020-07-13 11:46 ` Petr Machata [this message]
2020-07-10 21:55 ` [PATCH net-next v2 02/13] mlxsw: reg: Add Monitoring Mirror Trigger Enable Register Petr Machata
2020-07-10 21:55 ` [PATCH net-next v2 03/13] mlxsw: reg: Add Monitoring Port Analyzer Global Register Petr Machata
2020-07-10 21:55 ` [PATCH net-next v2 04/13] mlxsw: spectrum_span: Move SPAN operations out of global file Petr Machata
2020-07-10 21:55 ` [PATCH net-next v2 05/13] mlxsw: spectrum_span: Prepare for global mirroring triggers Petr Machata
2020-07-10 21:55 ` [PATCH net-next v2 06/13] mlxsw: spectrum_span: Add support " Petr Machata
2020-07-10 21:55 ` [PATCH net-next v2 07/13] mlxsw: spectrum_span: Add APIs to enable / disable " Petr Machata
2020-07-10 21:55 ` [PATCH net-next v2 08/13] mlxsw: spectrum_flow: Convert a goto to a return Petr Machata
2020-07-10 21:55 ` [PATCH net-next v2 09/13] mlxsw: spectrum_flow: Drop an unused field Petr Machata
2020-07-10 21:55 ` [PATCH net-next v2 10/13] mlxsw: spectrum_matchall: Publish matchall data structures Petr Machata
2020-07-10 21:55 ` [PATCH net-next v2 11/13] mlxsw: spectrum_flow: Promote binder-type dispatch to spectrum.c Petr Machata
2020-07-10 21:55 ` [PATCH net-next v2 12/13] mlxsw: spectrum_qdisc: Offload mirroring on RED qevent early_drop Petr Machata
2020-07-10 21:55 ` [PATCH net-next v2 13/13] selftests: mlxsw: RED: Test offload of mirror on RED early_drop qevent Petr Machata
2020-07-14 0:22 ` [PATCH net-next v2 00/13] mlxsw: Add support for buffer drops mirroring David Miller
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=87k0z7fxj3.fsf@mellanox.com \
--to=petrm@mellanox$(echo .)com \
--cc=davem@davemloft$(echo .)net \
--cc=fw@strlen$(echo .)de \
--cc=idosch@mellanox$(echo .)com \
--cc=jhs@mojatatu$(echo .)com \
--cc=jiri@mellanox$(echo .)com \
--cc=kadlec@netfilter$(echo .)org \
--cc=kuba@kernel$(echo .)org \
--cc=leon@kernel$(echo .)org \
--cc=michael.chan@broadcom$(echo .)com \
--cc=mlxsw@mellanox$(echo .)com \
--cc=netdev@vger$(echo .)kernel.org \
--cc=pablo@netfilter$(echo .)org \
--cc=saeedm@mellanox$(echo .)com \
--cc=simon.horman@netronome$(echo .)com \
--cc=xiyou.wangcong@gmail$(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