Re: [PATCH v2 net-next 2/7] net: dsa: merge all bools of struct dsa_port into a single u8

[Florian Fainelli <f.fainelli@gmail.com>]

On 1/5/22 10:56 AM, Vladimir Oltean wrote:
> On Wed, Jan 05, 2022 at 10:46:44AM -0800, Florian Fainelli wrote:
>> On 1/5/22 10:39 AM, Vladimir Oltean wrote:
>>> Hi Florian,
>>>
>>> On Wed, Jan 05, 2022 at 10:30:54AM -0800, Florian Fainelli wrote:
>>>> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
>>>
>>> Thanks a lot for the review.
>>>
>>> I'm a bit on the fence on this patch and the other one for dsa_switch.
>>> The thing is that bit fields are not atomic in C89, so if we update any
>>> of the flags inside dp or ds concurrently (like dp->vlan_filtering),
>>> we're in trouble. Right now this isn't a problem, because most of the
>>> flags are set either during probe, or during ds->ops->setup, or are
>>> serialized by the rtnl_mutex in ways that are there to stay (switchdev
>>> notifiers). That's why I didn't say anything about it. But it may be a
>>> caveat to watch out for in the future. Do you think we need to do
>>> something about it? A lock would not be necessary, strictly speaking.
>>
>> I would probably start with a comment that describes these pitfalls, I
>> wish we had a programmatic way to ensure that these flags would not be
>> set dynamically and outside of the probe/setup path but that won't
>> happen easily.
> 
> A comment is probably good.
> 
>> Should we be switching to a bitmask and bitmap helpers to be future proof?
> 
> We could have done that, and it would certainly raise the awareness a
> bit more, but the reason I went with the bit fields at least in the
> first place was to reduce the churn in drivers. Otherwise, sure, if
> driver changes are on the table for this, we can even discuss about
> adding more arguments to dsa_register_switch(). The amount of poking
> that drivers do inside struct dsa_switch has grown, and sometimes it's
> not even immediately clear which members of that structure are supposed
> to be populated by whom and when. We could definitely just tell them to
> provide their desired behavior as arguments (vlan_filtering_is_global,
> needs_standalone_vlan_filtering, configure_vlan_while_not_filtering,
> untag_bridge_pvid, assisted_learning_on_cpu_port, ageing_time_min,
> ageing_time_max, phys_mii_mask, num_tx_queues, num_lag_ids, max_num_bridges)
> and only DSA will update struct dsa_switch, and we could then control
> races better that way. But the downside is that we'd have 11 extra
> arguments to dsa_register_switch()....

I would not mind if we introduced a dsa_switch::features or similar
bitmap and require driver to set bits in there before
dsa_register_switch() but that seems outside the scope of your patch
set, and I am not sure what the benefits would be unless we do happen to
do dynamic bit/feature toggling.

So for now, I believe a comment is enough, and if we spot a driver
changing that paradigm we consider a more "dynamic" solution.
-- 
Florian