Re: [PATCH net-next v2 01/13] net: sched: Pass qdisc reference in struct flow_block_offload

[Petr Machata <petrm@mellanox.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);