Re: [Intel-wired-lan] [PATCH net-next 04/12] ice: Implement basic eswitch bridge setup

[Alexander Lobakin <aleksander.lobakin@intel.com>]

From: Wojciech Drewek <wojciech.drewek@intel.com>
Date: Thu, 20 Apr 2023 11:54:15 +0200

> Thanks for review Olek!
> 
> Most of the comments sound reasonable to me (and I will include them) with some exceptions.

Anytime, it's always a pleasure to review your team's code :p

>>> +static struct ice_esw_br_port *
>>> +ice_eswitch_br_port_init(struct ice_esw_br *bridge)
>>> +{
>>> +	struct ice_esw_br_port *br_port;
>>> +
>>> +	br_port = kzalloc(sizeof(*br_port), GFP_KERNEL);
>>> +	if (!br_port)
>>> +		return ERR_PTR(-ENOMEM);
>>> +
>>> +	br_port->bridge = bridge;
>>
>> Since you always pass @bridge from the call site either way, does it
>> make sense to do that or you could just assign -> bridge on the call
>> sites after a successful allocation?
> 
> I could do that but I prefer to keep it this way.
> We have two types of ports and this function is generic, It setups
> things common for both types, including bridge ref.
> Are you ok with it? 

Yes, sure. I noticed after sending that keeping this function as it is
will be more consistent with another one, which is pretty similar. So
I'm taking my words back :D

[...]

>>> +struct ice_esw_br {
>>> +	struct ice_esw_br_offloads *br_offloads;
>>> +	int ifindex;
>>> +
>>> +	struct xarray ports;
>>
>> (not sure about this one, but potentially there can be a hole between
>>  those two)
> 
> Move ifindex at the end?

I think the compilers will align this struct to 8 bytes. I'd try moving
it to the end, but I think it will just convert hole into padding at the
end. Then there's no difference and it can stay where it is now.
Holes can be filled any time when we're adding new fields, so not a big
problem.

> 
>>
>>> +};
>>> +
>>> +struct ice_esw_br_offloads {
>>> +	struct ice_pf *pf;
>>> +	struct ice_esw_br *bridge;
>>> +	struct notifier_block netdev_nb;
>>> +};
>>> +
>>> +#define ice_nb_to_br_offloads(nb, nb_name) \
>>> +	container_of(nb, \
>>> +		     struct ice_esw_br_offloads, \
>>> +		     nb_name)
>>
>> Hmm, you use it only once and only with `netdev_nb` field. Do you plan
>> to add more call sites of this macro? Otherwise you could embed the
>> second argument into the macro itself (mentioned `netdev_nb`) or even
>> just open-code the whole macro in the sole call site.
> 
> I the next patch it is used with different nb_name (switchdev_nb)

I noticed that, but only after reviewing the next patch, so sorry, this
one is closed :D

> 
>>
>>> +
>>> +void
>>> +ice_eswitch_br_offloads_deinit(struct ice_pf *pf);
>>> +int
>>> +ice_eswitch_br_offloads_init(struct ice_pf *pf);
>>> +
>>> +#endif /* _ICE_ESWITCH_BR_H_ */
>> [...]
>>
>> Thanks,
>> Olek

Thanks,
Olek
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan