Jakub Kicinski <k...@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);
