lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <af492889-6b0d-024d-91e9-a953f99419d8@intel.com>
Date: Tue, 9 May 2023 17:06:40 +0200
From: Alexander Lobakin <aleksander.lobakin@...el.com>
To: "Drewek, Wojciech" <wojciech.drewek@...el.com>
CC: "intel-wired-lan@...ts.osuosl.org" <intel-wired-lan@...ts.osuosl.org>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>, "Ertman, David M"
	<david.m.ertman@...el.com>, "michal.swiatkowski@...ux.intel.com"
	<michal.swiatkowski@...ux.intel.com>, "marcin.szycik@...ux.intel.com"
	<marcin.szycik@...ux.intel.com>, "Chmielewski, Pawel"
	<pawel.chmielewski@...el.com>, "Samudrala, Sridhar"
	<sridhar.samudrala@...el.com>
Subject: Re: [PATCH net-next 09/12] ice: implement bridge port vlan

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

> 
> 
>> -----Original Message-----
>> From: Lobakin, Aleksander <aleksander.lobakin@...el.com>
>> Sent: piÄ…tek, 21 kwietnia 2023 18:35
>> To: Drewek, Wojciech <wojciech.drewek@...el.com>
>> Cc: intel-wired-lan@...ts.osuosl.org; netdev@...r.kernel.org; Ertman, David M <david.m.ertman@...el.com>;
>> michal.swiatkowski@...ux.intel.com; marcin.szycik@...ux.intel.com; Chmielewski, Pawel <pawel.chmielewski@...el.com>;
>> Samudrala, Sridhar <sridhar.samudrala@...el.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

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ