Hello Jakub,
> On 8. Apr 2022, at 05:54, Jakub Kicinski <k...@kernel.org> wrote:
>
> On Thu, 7 Apr 2022 12:28:47 +0200 Jakob Koschel wrote:
>> diff --git a/drivers/net/dsa/sja1105/sja1105_vl.c
>> b/drivers/net/dsa/sja1105/sja1105_vl.c
>> index b7e95d60a6e4..cfcae4d19eef 100644
>> --- a/drivers/net/dsa/sja1105/sja1105_vl.c
>> +++ b/drivers/net/dsa/sja1105/sja1105_vl.c
>> @@ -27,20 +27,24 @@ static int sja1105_insert_gate_entry(struct
>> sja1105_gating_config *gating_cfg,
>> if (list_empty(&gating_cfg->entries)) {
>> list_add(&e->list, &gating_cfg->entries);
>> } else {
>> - struct sja1105_gate_entry *p;
>> + struct sja1105_gate_entry *p = NULL, *iter;
>>
>> - list_for_each_entry(p, &gating_cfg->entries, list) {
>> - if (p->interval == e->interval) {
>> + list_for_each_entry(iter, &gating_cfg->entries, list) {
>> + if (iter->interval == e->interval) {
>> NL_SET_ERR_MSG_MOD(extack,
>> "Gate conflict");
>> rc = -EBUSY;
>> goto err;
>> }
>>
>> - if (e->interval < p->interval)
>> + if (e->interval < iter->interval) {
>> + p = iter;
>> + list_add(&e->list, iter->list.prev);
>> break;
>> + }
>> }
>> - list_add(&e->list, p->list.prev);
>> + if (!p)
>> + list_add(&e->list, gating_cfg->entries.prev);
>
> This turns a pretty slick piece of code into something ugly :(
> I'd rather you open coded the iteration here than make it more
> complex to satisfy "safe coding guidelines".
I'm not entirely sure I understand what you mean with
"open coded the iteration". But maybe the solution proposed by Vladimir [1]
works for you? I'm planning to rewrite the cases in that way for the relevant
ones.
>
> Also the list_add() could be converted to list_add_tail().
Good point, I wasn't sure if that's considered as something that should be
done as a separate change. I'm happy to include it in v2.
Thanks for the input.
Jakob
[1] https://lore.kernel.org/linux-kernel/20220408114120.tvf2lxvhfqbnrlml@skbuf/
