Re: [Intel-wired-lan] [PATCH net-next 09/12] ice: implement bridge port vlan

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

From: Wojciech Drewek <wojciech.drewek@intel.com>
Date: Tue, 9 May 2023 13:25:40 +0200

> 
> 
>> -----Original Message-----
>> From: Lobakin, Aleksander <aleksander.lobakin@intel.com>
>> Sent: piątek, 21 kwietnia 2023 18:35
>> To: Drewek, Wojciech <wojciech.drewek@intel.com>
>> Cc: intel-wired-lan@lists.osuosl.org; netdev@vger.kernel.org; Ertman, David M <david.m.ertman@intel.com>;
>> michal.swiatkowski@linux.intel.com; marcin.szycik@linux.intel.com; Chmielewski, Pawel <pawel.chmielewski@intel.com>;
>> Samudrala, Sridhar <sridhar.samudrala@intel.com>
>> Subject: Re: [PATCH net-next 09/12] ice: implement bridge port vlan

[...]

>>> +	/* Setting port vlan on uplink isn't supported by hw */
>>> +	if (port->type == ICE_ESWITCH_BR_UPLINK_PORT)
>>> +		return -EOPNOTSUPP;
>>> +
>>> +	if (port->pvid) {
>>> +		dev_info(dev,
>>
>> dev_err()?
> 
> To me it's not an error, port vlan is already configured

Usually, every user action leading to an errno instead of 0 (success) is
an error, it's the user who is responsible for not doing such things.
A bit more details below, I reply bottom-up this time :z

> 
>>
>>> +			 "Port VLAN (vsi=%u, vid=%u) already exists on the port, remove it before adding new one\n",
>>> +			 port->vsi_idx, port->pvid);
>>> +		return -EEXIST;
>>
>> Hmm, isn't -EBUSY more common for such cases?
>>
>> (below as well)
> 
> I don't think so, user is trying to configure something that is already done.

+

>>> @@ -639,14 +697,29 @@ ice_eswitch_br_vlan_create(u16 vid, u16 flags, struct ice_esw_br_port *port)
>>>
>>>  	vlan->vid = vid;
>>>  	vlan->flags = flags;
>>> +	if ((flags & BRIDGE_VLAN_INFO_PVID) &&
>>> +	    (flags & BRIDGE_VLAN_INFO_UNTAGGED)) {
>>> +		err = ice_eswitch_br_set_pvid(port, vlan);
>>> +		if (err)
>>> +			goto err_set_pvid;
>>> +	} else if ((flags & BRIDGE_VLAN_INFO_PVID) ||
>>> +		   (flags & BRIDGE_VLAN_INFO_UNTAGGED)) {
>>> +		dev_info(dev, "VLAN push and pop are supported only simultaneously\n");
>>
>> (same for dev_err(), as well as below)
> 
> 
> Again, is this an error really? We just don't support such case.

Well, "not supported" is an error in the kernel usually. It's like,
"user is responsible for checking the capabilities before trying to
configure/use something, if he didn't care, then we don't as well" :D
The main problem here is as follows:

1. Most distros have "quiet" in the default command line, which limits
   the default output to errors+.
2. User tries to configure something, which is not supported.
3. Essentially has a bail out with -EOPNOTSUPP.
4. The default kernel output says nothing.

It's not an issue for tools like dmesg, since they usually display the
whole log with every loglevel, but still not really consistent as for
me. Plus, even in such tools, dev_info() will just lost amidst tons of
other nonsensical output, while dev_err() would be marked bold red.

>>
>>> +		return ERR_PTR(-EOPNOTSUPP);
>>> +	}
[...]

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