[Intel-wired-lan] [PATCH net-next 08/12] ice: Add VLAN FDB support in switchdev mode

[Alexander Lobakin]

From: Wojciech Drewek <wojciech.drewek at intel.com>
Date: Thu, 27 Apr 2023 12:28:21 +0200

> 
> 
>> -----Original Message-----
>> From: Lobakin, Aleksander <aleksander.lobakin at intel.com>
>> Sent: piątek, 21 kwietnia 2023 17:25
>> To: Drewek, Wojciech <wojciech.drewek at intel.com>
>> Cc: intel-wired-lan at lists.osuosl.org; netdev at vger.kernel.org; Lobakin, Aleksander <aleksander.lobakin at intel.com>; Ertman, David M
>> <david.m.ertman at intel.com>; michal.swiatkowski at linux.intel.com; marcin.szycik at linux.intel.com; Chmielewski, Pawel
>> <pawel.chmielewski at intel.com>; Samudrala, Sridhar <sridhar.samudrala at intel.com>
>> Subject: Re: [PATCH net-next 08/12] ice: Add VLAN FDB support in switchdev mode

[...]

>>> +		break;
>>> +	default:
>>> +		return -EOPNOTSUPP;
>>> +	}
>>> +
>>> +	return 0;
>>> +}
>>
>> [...]
>>
>>> +	br_offloads->switchdev_blk.notifier_call =
>>> +		ice_eswitch_br_event_blocking;
>>
>> Oh, you have two usages of ->switchdev_blk here, so you can add an
>> intermediate variable to avoid line breaking, which would also shorten
>> the line below :D
>>
>> 	nb = &br_offloads->switchdev_blk;
>> 	nb->notifier_call = ice_eswitch_br_event_blocking;
>> 	...
> 
> Hmmm, I feel like it is more readable right now. It's clear that we're registering
> switchdev blocking notifier block (switchdev_blk). Introducing generic variable (nb)
> might a bit ambiguous IMO. So if you have nothing against it I'd leave it as it is.

Noprob, up to you :)

[...]

>>>  	int ifindex;
>>
>> (BTW, ifindex is also usually unsigned unless it's not an error)
> 
> It is defined as int in net_device, so I don’t know

I know, but back then people don't care much and were usually defining
everything that doesn't need explicit size as `int` :D No interfaces
have a negative index, indexes start from 1, which is reserved for
loopback, so even 0 is not used. Up to you anyway, I just usually don't
use sign when it's not needed. But it's more of a personal.

[...]

Thanks,
Olek