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

Alexander Lobakin aleksander.lobakin at intel.com
Tue May 9 15:06:40 UTC 2023


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

> 
> 
>> -----Original Message-----
>> From: Lobakin, Aleksander <aleksander.lobakin at intel.com>
>> Sent: piątek, 21 kwietnia 2023 18:35
>> To: Drewek, Wojciech <wojciech.drewek at intel.com>
>> Cc: intel-wired-lan at lists.osuosl.org; netdev at vger.kernel.org; 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 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


More information about the Intel-wired-lan mailing list