* Re: [PATCH] iwlwifi: mvm: Move static keyword to the front of declarations
From: Krzysztof Wilczynski @ 2019-09-02 21:03 UTC (permalink / raw)
To: Luca Coelho
Cc: Kalle Valo, Johannes Berg, Emmanuel Grumbach,
Intel Linux Wireless, David S. Miller, Sara Sharon,
Shaul Triebitz, Liad Kaufman, linux-wireless, netdev,
linux-kernel
In-Reply-To: <c22d4775fdad4e34fdc386e2cf728b63dfe13ffe.camel@coelho.fi>
Hi Luca,
[...]
> Thanks for your patch! Though we already have this change in our
> internal tree (submitted by YueHaibing) and it will reach the mainline
> soon.
Thank you for letting me know. I am glad it's fixed. :)
Krzysztof
^ permalink raw reply
* Re: [PATCH] Fix a double free bug in rsi_91x_deinit
From: Greg KH @ 2019-09-02 20:06 UTC (permalink / raw)
To: Guenter Roeck
Cc: Kalle Valo, Hui Peng, security, Mathias Payer, David S. Miller,
linux-wireless, netdev, linux-kernel
In-Reply-To: <804fb4dc-23e5-3442-c64e-9857d61d6b6c@roeck-us.net>
On Mon, Sep 02, 2019 at 12:32:37PM -0700, Guenter Roeck wrote:
> On 9/2/19 11:47 AM, Greg KH wrote:
> > On Sun, Sep 01, 2019 at 07:08:29AM -0700, Guenter Roeck wrote:
> > > On 9/1/19 1:03 AM, Kalle Valo wrote:
> > > > Guenter Roeck <linux@roeck-us.net> writes:
> > > >
> > > > > On Mon, Aug 19, 2019 at 06:02:29PM -0400, Hui Peng wrote:
> > > > > > `dev` (struct rsi_91x_usbdev *) field of adapter
> > > > > > (struct rsi_91x_usbdev *) is allocated and initialized in
> > > > > > `rsi_init_usb_interface`. If any error is detected in information
> > > > > > read from the device side, `rsi_init_usb_interface` will be
> > > > > > freed. However, in the higher level error handling code in
> > > > > > `rsi_probe`, if error is detected, `rsi_91x_deinit` is called
> > > > > > again, in which `dev` will be freed again, resulting double free.
> > > > > >
> > > > > > This patch fixes the double free by removing the free operation on
> > > > > > `dev` in `rsi_init_usb_interface`, because `rsi_91x_deinit` is also
> > > > > > used in `rsi_disconnect`, in that code path, the `dev` field is not
> > > > > > (and thus needs to be) freed.
> > > > > >
> > > > > > This bug was found in v4.19, but is also present in the latest version
> > > > > > of kernel.
> > > > > >
> > > > > > Reported-by: Hui Peng <benquike@gmail.com>
> > > > > > Reported-by: Mathias Payer <mathias.payer@nebelwelt.net>
> > > > > > Signed-off-by: Hui Peng <benquike@gmail.com>
> > > > >
> > > > > FWIW:
> > > > >
> > > > > Reviewed-by: Guenter Roeck <linux@roeck-us.net>
> > > > >
> > > > > This patch is listed as fix for CVE-2019-15504, which has a CVSS 2.0 score
> > > > > of 10.0 (high) and CVSS 3.0 score of 9.8 (critical).
> > > >
> > > > A double free in error path is considered as a critical CVE issue? I'm
> > > > very curious, why is that?
> > > >
> > >
> > > You'd have to ask the people assigning CVSS scores. However, if the memory
> > > was reallocated, that reallocated memory (which is still in use) is freed.
> > > Then all kinds of bad things can happen.
> >
> > Yes, but moving from "bad things _can_ happen" to "bad things happen" in
> > an instance like this will be a tough task. It also requires physical
> > access to the machine.
> >
>
> Is this correct even with usbip enabled ?
Who has usbip enabled anywhere? :)
I don't know if usbip can trigger this type of thing, maybe someone
needs to test that...
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH] Fix a double free bug in rsi_91x_deinit
From: Guenter Roeck @ 2019-09-02 19:32 UTC (permalink / raw)
To: Greg KH
Cc: Kalle Valo, Hui Peng, security, Mathias Payer, David S. Miller,
linux-wireless, netdev, linux-kernel
In-Reply-To: <20190902184722.GC5697@kroah.com>
On 9/2/19 11:47 AM, Greg KH wrote:
> On Sun, Sep 01, 2019 at 07:08:29AM -0700, Guenter Roeck wrote:
>> On 9/1/19 1:03 AM, Kalle Valo wrote:
>>> Guenter Roeck <linux@roeck-us.net> writes:
>>>
>>>> On Mon, Aug 19, 2019 at 06:02:29PM -0400, Hui Peng wrote:
>>>>> `dev` (struct rsi_91x_usbdev *) field of adapter
>>>>> (struct rsi_91x_usbdev *) is allocated and initialized in
>>>>> `rsi_init_usb_interface`. If any error is detected in information
>>>>> read from the device side, `rsi_init_usb_interface` will be
>>>>> freed. However, in the higher level error handling code in
>>>>> `rsi_probe`, if error is detected, `rsi_91x_deinit` is called
>>>>> again, in which `dev` will be freed again, resulting double free.
>>>>>
>>>>> This patch fixes the double free by removing the free operation on
>>>>> `dev` in `rsi_init_usb_interface`, because `rsi_91x_deinit` is also
>>>>> used in `rsi_disconnect`, in that code path, the `dev` field is not
>>>>> (and thus needs to be) freed.
>>>>>
>>>>> This bug was found in v4.19, but is also present in the latest version
>>>>> of kernel.
>>>>>
>>>>> Reported-by: Hui Peng <benquike@gmail.com>
>>>>> Reported-by: Mathias Payer <mathias.payer@nebelwelt.net>
>>>>> Signed-off-by: Hui Peng <benquike@gmail.com>
>>>>
>>>> FWIW:
>>>>
>>>> Reviewed-by: Guenter Roeck <linux@roeck-us.net>
>>>>
>>>> This patch is listed as fix for CVE-2019-15504, which has a CVSS 2.0 score
>>>> of 10.0 (high) and CVSS 3.0 score of 9.8 (critical).
>>>
>>> A double free in error path is considered as a critical CVE issue? I'm
>>> very curious, why is that?
>>>
>>
>> You'd have to ask the people assigning CVSS scores. However, if the memory
>> was reallocated, that reallocated memory (which is still in use) is freed.
>> Then all kinds of bad things can happen.
>
> Yes, but moving from "bad things _can_ happen" to "bad things happen" in
> an instance like this will be a tough task. It also requires physical
> access to the machine.
>
Is this correct even with usbip enabled ?
> Anyway, that doesn't mean we shouldn't fix it, it's just that CVSS can
> be crazy when it comes to kernel patches (i.e. almost all fixes should
> be "critical"...)
>
Not all of them, but probably too many. That is why I asked if the problem
is real. I _used_ to trust CVSS scores, but by now I am at least somewhat
suspicious - especially if a patch wasn't applied for a period of time,
like this series of usb patches.
Having said that, I am even more wary of double-free problems - those tend
to be notoriously difficult to debug. I'd rather have them out of my way,
even if they are unlikely to be seen in the real world (plus, Murphy
says that anything unlikely is going to happen almost immediately).
Guenter
^ permalink raw reply
* Re: [net-next 01/18] net/mlx5: Add flow steering actions to fs_cmd shim layer
From: David Miller @ 2019-09-02 19:10 UTC (permalink / raw)
To: saeedm; +Cc: netdev, valex, erezsh, maorg, markb
In-Reply-To: <20190902072213.7683-2-saeedm@mellanox.com>
From: Saeed Mahameed <saeedm@mellanox.com>
Date: Mon, 2 Sep 2019 07:22:52 +0000
> + maction->flow_action_raw.pkt_reformat =
> + mlx5_packet_reformat_alloc(dev->mdev, prm_prt, len,
> + in, namespace);
> + if (IS_ERR(maction->flow_action_raw.pkt_reformat))
> return ret;
Don't you have to initialize 'ret' to the pointer error here?
This transformation doesn't look correct.
^ permalink raw reply
* Re: [PATCH net-next 0/2] mvpp2: per-cpu buffers
From: David Miller @ 2019-09-02 19:08 UTC (permalink / raw)
To: mcroce
Cc: netdev, linux-kernel, maxime.chevallier, mw, antoine.tenart,
stefanc, nadavh, lorenzo
In-Reply-To: <20190902102137.841-1-mcroce@redhat.com>
From: Matteo Croce <mcroce@redhat.com>
Date: Mon, 2 Sep 2019 12:21:35 +0200
> This patchset workarounds an PP2 HW limitation which prevents to use
> per-cpu rx buffers.
> The first patch is just a refactor to prepare for the second one.
> The second one allocates percpu buffers if the following conditions are met:
> - CPU number is less or equal 4
> - no port is using jumbo frames
>
> If the following conditions are not met at load time, of jumbo frame is enabled
> later on, the shared allocation is reverted.
Series applied to net-next.
^ permalink raw reply
* Re: [PATCH net-next] net/ncsi: support unaligned payload size in NC-SI cmd handler
From: David Miller @ 2019-09-02 19:03 UTC (permalink / raw)
To: benwei; +Cc: sam, netdev, linux-kernel, openbmc
In-Reply-To: <CH2PR15MB368619179F403EAE47FD61F7A3BE0@CH2PR15MB3686.namprd15.prod.outlook.com>
From: Ben Wei <benwei@fb.com>
Date: Mon, 2 Sep 2019 02:46:52 +0000
> Update NC-SI command handler (both standard and OEM) to take into
> account of payload paddings in allocating skb (in case of payload
> size is not 32-bit aligned).
>
> The checksum field follows payload field, without taking payload
> padding into account can cause checksum being truncated, leading to
> dropped packets.
>
> Signed-off-by: Ben Wei <benwei@fb.com>
If you have to align and add padding, I do not see where you are
clearing out that padding memory to make sure it is initialized.
You do comparisons with 'payload' but make adjustments to 'len'.
The logic is very confusing.
^ permalink raw reply
* Re: [PATCH] net: dsa: Fix off-by-one number of calls to devlink_port_unregister
From: David Miller @ 2019-09-02 19:00 UTC (permalink / raw)
To: olteanv; +Cc: f.fainelli, vivien.didelot, andrew, netdev
In-Reply-To: <20190831124619.460-1-olteanv@gmail.com>
From: Vladimir Oltean <olteanv@gmail.com>
Date: Sat, 31 Aug 2019 15:46:19 +0300
> When a function such as dsa_slave_create fails, currently the following
> stack trace can be seen:
...
> devlink_free is complaining right here:
>
> WARN_ON(!list_empty(&devlink->port_list));
>
> This happens because devlink_port_unregister is no longer done right
> away in dsa_port_setup when a DSA_PORT_TYPE_USER has failed.
> Vivien said about this change that:
>
> Also no need to call devlink_port_unregister from within dsa_port_setup
> as this step is inconditionally handled by dsa_port_teardown on error.
>
> which is not really true. The devlink_port_unregister function _is_
> being called unconditionally from within dsa_port_setup, but not for
> this port that just failed, just for the previous ones which were set
> up.
>
> ports_teardown:
> for (i = 0; i < port; i++)
> dsa_port_teardown(&ds->ports[i]);
>
> Initially I was tempted to fix this by extending the "for" loop to also
> cover the port that failed during setup. But this could have potentially
> unforeseen consequences unrelated to devlink_port or even other types of
> ports than user ports, which I can't really test for. For example, if
> for some reason devlink_port_register itself would fail, then
> unconditionally unregistering it in dsa_port_teardown would not be a
> smart idea. The list might go on.
>
> So just make dsa_port_setup undo the setup it had done upon failure, and
> let the for loop undo the work of setting up the previous ports, which
> are guaranteed to be brought up to a consistent state.
>
> Fixes: 955222ca5281 ("net: dsa: use a single switch statement for port setup")
> Signed-off-by: Vladimir Oltean <olteanv@gmail.com>
Applied to net-next, thanks.
^ permalink raw reply
* Re: [PATCH] net: stmmac: dwmac-sun8i: Variable "val" in function sun8i_dwmac_set_syscon() could be uninitialized
From: David Miller @ 2019-09-02 18:48 UTC (permalink / raw)
To: yzhai003
Cc: csong, zhiyunq, peppe.cavallaro, alexandre.torgue, joabreu,
mripard, wens, mcoquelin.stm32, netdev, linux-arm-kernel,
linux-stm32, linux-kernel
In-Reply-To: <20190831020049.6516-1-yzhai003@ucr.edu>
From: Yizhuo <yzhai003@ucr.edu>
Date: Fri, 30 Aug 2019 19:00:48 -0700
> In function sun8i_dwmac_set_syscon(), local variable "val" could
> be uninitialized if function regmap_field_read() returns -EINVAL.
> However, it will be used directly in the if statement, which
> is potentially unsafe.
>
> Signed-off-by: Yizhuo <yzhai003@ucr.edu>
Applied, thank you.
^ permalink raw reply
* Re: [PATCH] Fix a double free bug in rsi_91x_deinit
From: Greg KH @ 2019-09-02 18:47 UTC (permalink / raw)
To: Guenter Roeck
Cc: Kalle Valo, Hui Peng, security, Mathias Payer, David S. Miller,
linux-wireless, netdev, linux-kernel
In-Reply-To: <385361d3-048e-9b3f-c749-aa5861e397e7@roeck-us.net>
On Sun, Sep 01, 2019 at 07:08:29AM -0700, Guenter Roeck wrote:
> On 9/1/19 1:03 AM, Kalle Valo wrote:
> > Guenter Roeck <linux@roeck-us.net> writes:
> >
> > > On Mon, Aug 19, 2019 at 06:02:29PM -0400, Hui Peng wrote:
> > > > `dev` (struct rsi_91x_usbdev *) field of adapter
> > > > (struct rsi_91x_usbdev *) is allocated and initialized in
> > > > `rsi_init_usb_interface`. If any error is detected in information
> > > > read from the device side, `rsi_init_usb_interface` will be
> > > > freed. However, in the higher level error handling code in
> > > > `rsi_probe`, if error is detected, `rsi_91x_deinit` is called
> > > > again, in which `dev` will be freed again, resulting double free.
> > > >
> > > > This patch fixes the double free by removing the free operation on
> > > > `dev` in `rsi_init_usb_interface`, because `rsi_91x_deinit` is also
> > > > used in `rsi_disconnect`, in that code path, the `dev` field is not
> > > > (and thus needs to be) freed.
> > > >
> > > > This bug was found in v4.19, but is also present in the latest version
> > > > of kernel.
> > > >
> > > > Reported-by: Hui Peng <benquike@gmail.com>
> > > > Reported-by: Mathias Payer <mathias.payer@nebelwelt.net>
> > > > Signed-off-by: Hui Peng <benquike@gmail.com>
> > >
> > > FWIW:
> > >
> > > Reviewed-by: Guenter Roeck <linux@roeck-us.net>
> > >
> > > This patch is listed as fix for CVE-2019-15504, which has a CVSS 2.0 score
> > > of 10.0 (high) and CVSS 3.0 score of 9.8 (critical).
> >
> > A double free in error path is considered as a critical CVE issue? I'm
> > very curious, why is that?
> >
>
> You'd have to ask the people assigning CVSS scores. However, if the memory
> was reallocated, that reallocated memory (which is still in use) is freed.
> Then all kinds of bad things can happen.
Yes, but moving from "bad things _can_ happen" to "bad things happen" in
an instance like this will be a tough task. It also requires physical
access to the machine.
Anyway, that doesn't mean we shouldn't fix it, it's just that CVSS can
be crazy when it comes to kernel patches (i.e. almost all fixes should
be "critical"...)
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH v3 1/2] net: core: Notify on changes to dev->promiscuity.
From: Jiri Pirko @ 2019-09-02 18:45 UTC (permalink / raw)
To: Allan W. Nielsen
Cc: David Miller, idosch, andrew, horatiu.vultur, alexandre.belloni,
UNGLinuxDriver, ivecera, f.fainelli, netdev, linux-kernel
In-Reply-To: <20190902180519.ytbs6x2dx5z23hys@lx-anielsen.microsemi.net>
Mon, Sep 02, 2019 at 08:05:20PM CEST, allan.nielsen@microchip.com wrote:
>The 09/02/2019 19:51, Jiri Pirko wrote:
>> External E-Mail
>>
>>
>> Mon, Sep 02, 2019 at 07:42:31PM CEST, allan.nielsen@microchip.com wrote:
>> >Hi Jiri,
>> >
>> >Sorry for joining the discussion this late, but I have been without mail access
>> >for the last few days.
>> >
>> >
>> >The 08/30/2019 08:36, Jiri Pirko wrote:
>> >> Fri, Aug 30, 2019 at 08:02:33AM CEST, davem@davemloft.net wrote:
>> >> >From: Jiri Pirko <jiri@resnulli.us>
>> >> >Date: Fri, 30 Aug 2019 07:39:40 +0200
>> >> >
>> >> >> Because the "promisc mode" would gain another meaning. Now how the
>> >> >> driver should guess which meaning the user ment when he setted it?
>> >> >> filter or trap?
>> >> >>
>> >> >> That is very confusing. If the flag is the way to do this, let's
>> >> >> introduce another flag, like IFF_TRAPPING indicating that user wants
>> >> >> exactly this.
>> >> >
>> >> >I don't understand how the meaning of promiscuous mode for a
>> >> >networking device has suddenly become ambiguous, when did this start
>> >> >happening?
>> >>
>> >> The promiscuity is a way to setup the rx filter. So promics == rx filter
>> >> off. For normal nics, where there is no hw fwd datapath,
>> >> this coincidentally means all received packets go to cpu.
>> >> But if there is hw fwd datapath, rx filter is still off, all rxed packets
>> >> are processed. But that does not mean they should be trapped to cpu.
>> >>
>> >> Simple example:
>> >> I need to see slowpath packets, for example arps/stp/bgp/... that
>> >> are going to cpu, I do:
>> >> tcpdump -i swp1
>> >
>> >How is this different from "tcpdump -p -i swp1"
>> >
>> >> I don't want to get all the traffic running over hw running this cmd.
>> >> This is a valid usecase.
>> >>
>> >> To cope with hw fwd datapath devices, I believe that tcpdump has to have
>> >> notion of that. Something like:
>> >>
>> >> tcpdump -i swp1 --hw-trapping-mode
>> >>
>> >> The logic can be inverse:
>> >> tcpdump -i swp1
>> >> tcpdump -i swp1 --no-hw-trapping-mode
>> >>
>> >> However, that would provide inconsistent behaviour between existing and
>> >> patched tcpdump/kernel.
>> >>
>> >> All I'm trying to say, there are 2 flags
>> >> needed (if we don't use tc trap).
>> >
>> >I have been reading through this thread several times and I still do not get it.
>> >
>> >As far as I understand you are arguing that we need 3 modes:
>> >
>> >- tcpdump -i swp1
>>
>> Depends on default. Promisc is on.
>>
>>
>> >- tcpdump -p -i swp1
>>
>> All traffic that is trapped to the cpu by default, not promisc means
>> only mac of the interface (if bridge for example haven't set promisc
>> already) and special macs. So host traffic (ip of host), bgp, arp, nsnd,
>> etc.
>
>In the case where the interface is enslaved to a bridge, it is put into promisc
>mode, which means that "tcpdump -i swp1" and "tcpdump -p -i swp1" give the same
>result, right?
>
>Is this desirable?
Yes, that is correct and expected. It it might not be bridged, depends
on a usecase.
>
>> >- tcpdump -i swp1 --hw-trapping-mode
>>
>> Promisc is on, all traffic received on the port and pushed to cpu. User
>> has to be careful because in case of mlxsw this can lead to couple
>> hundred gigabit traffic going over limited pci bandwidth (gigabits).
>>
>>
>> >
>> >Would you mind provide an example of the traffic you want to see in the 3 cases
>> >(or the traffic which you do not want to see).
>> >
>> >/Allan
>> >
>>
>
>--
>/Allan
^ permalink raw reply
* Re: [patch net-next v2] mlx5: Add missing init_net check in FIB notifier
From: David Miller @ 2019-09-02 18:45 UTC (permalink / raw)
To: jiri; +Cc: netdev, saeedm, leon, roid, mlxsw
In-Reply-To: <20190830082530.8958-1-jiri@resnulli.us>
From: Jiri Pirko <jiri@resnulli.us>
Date: Fri, 30 Aug 2019 10:25:30 +0200
> From: Jiri Pirko <jiri@mellanox.com>
>
> Take only FIB events that are happening in init_net into account. No other
> namespaces are supported.
>
> Signed-off-by: Jiri Pirko <jiri@mellanox.com>
> ---
> v1->v2:
> - no change, just cced maintainers (fat finger made me avoid them in v1)
Applied, thanks Jiri.
^ permalink raw reply
* Re: [PATCH net-next] net_failover: get rid of the limitaion receive packet from standy dev when primary exist
From: David Miller @ 2019-09-02 18:42 UTC (permalink / raw)
To: wenxu; +Cc: sridhar.samudrala, netdev
In-Reply-To: <1567141668-12196-1-git-send-email-wenxu@ucloud.cn>
From: wenxu@ucloud.cn
Date: Fri, 30 Aug 2019 13:07:48 +0800
> From: wenxu <wenxu@ucloud.cn>
>
> For receive side the standby, primary and failover is the same one,
> If the packet receive from standby or primary should can be deliver
> to failover dev.
>
> For example: there are VF and virtio device failover together.
> When live migration the VF detached and send/recv packet through
> virtio device. When VF attached again some ingress traffic may
> receive from virtio device for cache reason(TC flower offload in
> sw mode).
>
> Signed-off-by: wenxu <wenxu@ucloud.cn>
I don't understand, the device logic guarding the device rewriting in
this handler looks very much intentional.
After your changes we have no logic at all, we just unconditionally
always rewrite.
^ permalink raw reply
* Re: [PATCH net-next] r8152: modify rtl8152_set_speed function
From: Heiner Kallweit @ 2019-09-02 18:36 UTC (permalink / raw)
To: Hayes Wang, netdev; +Cc: nic_swsd, linux-kernel
In-Reply-To: <1394712342-15778-326-Taiwan-albertk@realtek.com>
On 02.09.2019 13:52, Hayes Wang wrote:
> First, for AUTONEG_DISABLE, we only need to modify MII_BMCR.
>
> Second, add advertising parameter for rtl8152_set_speed(). Add
> RTL_ADVERTISED_xxx for advertising parameter of rtl8152_set_speed().
> Then, the advertising settings from ethtool could be saved.
>
Seeing all this code it might be a good idea to switch this driver
to phylib, similar to what I did with r8169 some time ago.
> Signed-off-by: Hayes Wang <hayeswang@realtek.com>
> ---
> drivers/net/usb/r8152.c | 196 +++++++++++++++++++++++++++-------------
> 1 file changed, 132 insertions(+), 64 deletions(-)
>
[...]
^ permalink raw reply
* Re: [net-next 1/3] ravb: correct typo in FBP field of SFO register
From: David Miller @ 2019-09-02 18:33 UTC (permalink / raw)
To: horms+renesas
Cc: sergei.shtylyov, magnus.damm, netdev, linux-renesas-soc,
kazuya.mizuguchi.ks
In-Reply-To: <20190902080603.5636-2-horms+renesas@verge.net.au>
From: Simon Horman <horms+renesas@verge.net.au>
Date: Mon, 2 Sep 2019 10:06:01 +0200
> - SFO_FPB = 0x0000003F,
> + SFO_FBP = 0x0000003F,
> };
>
> /* RTC */
> ---
> drivers/net/ethernet/renesas/ravb.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
Simon please clean this up, I don't know what happened here :-)
^ permalink raw reply
* [PATCH][next] net/sched: cbs: remove redundant assignment to variable port_rate
From: Colin King @ 2019-09-02 18:26 UTC (permalink / raw)
To: Jamal Hadi Salim, Cong Wang, Jiri Pirko, David S . Miller, netdev
Cc: kernel-janitors, linux-kernel
From: Colin Ian King <colin.king@canonical.com>
Variable port_rate is being initialized with a value that is never read
and is being re-assigned a little later on. The assignment is redundant
and hence can be removed.
Addresses-Coverity: ("Unused value")
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
net/sched/sch_cbs.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/sched/sch_cbs.c b/net/sched/sch_cbs.c
index 810645b5c086..93b58fde99b7 100644
--- a/net/sched/sch_cbs.c
+++ b/net/sched/sch_cbs.c
@@ -299,7 +299,7 @@ static void cbs_set_port_rate(struct net_device *dev, struct cbs_sched_data *q)
{
struct ethtool_link_ksettings ecmd;
int speed = SPEED_10;
- int port_rate = -1;
+ int port_rate;
int err;
err = __ethtool_get_link_ksettings(dev, &ecmd);
--
2.20.1
^ permalink raw reply related
* [PATCH v3 2/2] selftests: forwrading: tc vlan bridge test
From: Zahari Doychev @ 2019-09-02 18:10 UTC (permalink / raw)
To: netdev
Cc: bridge, nikolay, roopa, jhs, dsahern, simon.horman,
makita.toshiaki, xiyou.wangcong, jiri, alexei.starovoitov,
johannes, Zahari Doychev
In-Reply-To: <20190902181000.25638-1-zahari.doychev@linux.com>
Add bridge vlan aware forwarding test for vlans added by tc-act_vlan. The
forwarding is tested in two cases when the bridge protocol and outer vlan
tag protocol match and mismatch. The tests checks the correct usage of
skb->mac_len in the bridge code.
Signed-off-by: Zahari Doychev <zahari.doychev@linux.com>
---
v2->v3:
- selftest added
---
.../forwarding/bridge_vlan_aware_tc_vlan.sh | 187 ++++++++++++++++++
1 file changed, 187 insertions(+)
create mode 100755 tools/testing/selftests/net/forwarding/bridge_vlan_aware_tc_vlan.sh
diff --git a/tools/testing/selftests/net/forwarding/bridge_vlan_aware_tc_vlan.sh b/tools/testing/selftests/net/forwarding/bridge_vlan_aware_tc_vlan.sh
new file mode 100755
index 000000000000..215d6293fa54
--- /dev/null
+++ b/tools/testing/selftests/net/forwarding/bridge_vlan_aware_tc_vlan.sh
@@ -0,0 +1,187 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0
+#
+# This test uses the standard topology for testing bridge forwarding. See
+# README for more details.
+#
+# tc vlan actions are applied on one of the bridge ports and on other one
+# the corresponding vlan network devices are created.
+#
+
+ALL_TESTS="
+ test_tc_vlan_bridge_ipv4_forwarding
+ test_tc_vlan_bridge_ipv4_forwarding_proto
+ test_tc_vlan_bridge_ipv6_forwarding
+ test_tc_vlan_bridge_ipv6_forwarding_proto
+"
+
+NUM_NETIFS=4
+CHECK_TC="yes"
+source lib.sh
+
+h_create()
+{
+ local dev=$1; shift
+ local ip=$1; shift
+ local ip6=$1
+
+ simple_if_init $dev $ip $ip6
+}
+
+h_destroy()
+{
+ local dev=$1; shift
+ local ip=$1; shift
+ local ip6=$1
+
+ simple_if_fini $dev $ip $ip6
+}
+
+switch_create()
+{
+ ip link add dev br0 type bridge vlan_filtering 1 vlan_protocol 802.1q \
+ mcast_snooping 0
+
+ ip link set dev $swp1 master br0
+ ip link set dev $swp2 master br0
+
+ ip link set dev br0 up
+ ip link set dev $swp1 up
+ ip link set dev $swp2 up
+
+ bridge vlan add dev $swp1 vid $svid master
+ bridge vlan add dev br0 vid $svid self
+ bridge vlan add dev $swp2 vid $svid master
+}
+
+switch_destroy()
+{
+ ip link set dev $swp2 down
+ ip link set dev $swp1 down
+
+ ip link del dev br0
+}
+
+tc_vlan_create()
+{
+ tc qdisc add dev $swp1 clsact
+
+ tc filter add dev $swp1 ingress pref 1 protocol all flower skip_hw \
+ action vlan push id $cvid protocol 802.1q pipe \
+ action vlan push id $svid protocol 802.1q
+
+ tc filter add dev $swp1 egress pref 1 protocol 802.1q \
+ flower skip_hw vlan_id $svid \
+ vlan_ethtype 802.1q cvlan_id $cvid \
+ action vlan pop pipe action vlan pop
+}
+
+tc_vlan_destroy()
+{
+ tc filter del dev $swp1 ingress pref 1
+ tc filter del dev $swp1 egress pref 1
+ tc qdisc del dev $swp1 clsact
+}
+
+vlan_create()
+{
+ local dev=$1; shift
+ local vid=$1; shift
+ local tpid=$1;
+
+ ip link add link $dev name $dev.$vid type vlan id $vid proto $tpid
+ ip link set dev $dev up
+ ip link set dev $dev.$vid
+}
+
+vlan_destroy()
+{
+ local dev=$1
+
+ ip link del dev $dev
+}
+
+setup_prepare()
+{
+ h1=${NETIFS[p1]}
+ swp1=${NETIFS[p2]}
+
+ swp2=${NETIFS[p3]}
+ h2=${NETIFS[p4]}
+
+ cvid=10
+ svid=100
+
+ vrf_prepare
+
+ switch_create
+
+ tc_vlan_create
+
+ h_create $h1 192.0.2.1/24 2001:db8:1::1/64
+
+ vlan_create $h2 $svid 802.1q
+ vlan_create $h2.$svid $cvid 802.1q
+
+ h_create $h2.$svid.$cvid 192.0.2.2/24 2001:db8:1::2/64
+}
+
+cleanup()
+{
+ pre_cleanup
+
+ tc_vlan_destroy
+
+ switch_destroy
+
+ h_destroy $h1 192.0.2.1/24 2001:db8:1::1/64
+ h_destroy $h2.$svid.$cvid 192.0.2.2/24 2001:db8:1::2/64
+
+ vlan_destroy $h2.$svid.$cvid
+ vlan_destroy $h2.$svid
+
+ ip link del dev $h1
+ ip link del dev $h2
+
+ vrf_cleanup
+}
+
+test_tc_vlan_bridge_ipv4_forwarding()
+{
+ ip link set dev br0 type bridge vlan_protocol 802.1q
+ ping_do $h1 192.0.2.2
+ check_err $? "Packets were not forwarded"
+ log_test "IPv4 tc-vlan bridge forwarding"
+}
+
+test_tc_vlan_bridge_ipv4_forwarding_proto()
+{
+ ip link set dev br0 type bridge vlan_protocol 802.1ad
+ ping_do $h1 192.0.2.2
+ check_err $? "Packets were not forwarded"
+ log_test "IPv4 tc-vlan bridge forwarding protocol mismatch"
+}
+
+test_tc_vlan_bridge_ipv6_forwarding()
+{
+ ip link set dev br0 type bridge vlan_protocol 802.1q
+ ping6_do $h1 2001:db8:1::2
+ check_err $? "Packets were not forwarded"
+ log_test "IPv6 tc-vlan bridge forwarding"
+}
+
+test_tc_vlan_bridge_ipv6_forwarding_proto()
+{
+ ip link set dev br0 type bridge vlan_protocol 802.1ad
+ ping6_do $h1 2001:db8:1::2
+ check_err $? "Packet were not forwarded"
+ log_test "IPv6 tc-vlan bridge forwarding protocol mismatch"
+}
+trap cleanup EXIT
+
+setup_prepare
+setup_wait
+
+tests_run
+
+exit $EXIT_STATUS
--
2.22.0
^ permalink raw reply related
* [PATCH v3 1/2] net: bridge: use mac_len in bridge forwarding
From: Zahari Doychev @ 2019-09-02 18:09 UTC (permalink / raw)
To: netdev
Cc: bridge, nikolay, roopa, jhs, dsahern, simon.horman,
makita.toshiaki, xiyou.wangcong, jiri, alexei.starovoitov,
johannes, Zahari Doychev
The bridge code cannot forward packets from various paths that set up the
SKBs in different ways. Some of these packets get corrupted during the
forwarding as not always is just ETH_HLEN pulled at the front.
This happens e.g. when VLAN tags are pushed by using tc act_vlan on
ingress. Example configuration is provided below. The test setup consists
of two netdevs connected to external hosts. There is act_vlan on one of
them adding two vlan tags on ingress and removing the tags on egress.
The configuration is done using the following commands:
ip link add name br0 type bridge vlan_filtering 1
ip link set dev br0 up
ip link set dev net0 up
ip link set dev net0 master br0
ip link set dev net1 up
ip link set dev net1 master br0
bridge vlan add dev net0 vid 100 master
bridge vlan add dev br0 vid 100 self
bridge vlan add dev net1 vid 100 master
tc qdisc add dev net0 handle ffff: clsact
tc qdisc add dev net1 handle ffff: clsact
tc filter add dev net0 ingress pref 1 protocol all flower \
action vlan push id 10 pipe action vlan push id 100
tc filter add dev net0 egress pref 1 protocol 802.1q flower \
vlan_id 100 vlan_ethtype 802.1q cvlan_id 10 \
action vlan pop pipe action vlan pop
When using the setup above the packets coming on net0 get double tagged but
the MAC headers gets corrupted when the packets go out of net1.
The skb->data is pushed only by the ETH_HLEN length instead of mac_len in
br_dev_queue_push_xmit. This later causes the function validate_xmit_vlan
to insert the outer vlan tag behind the inner vlan tag as the skb->data
does not point to the start of packet.
The problem is fixed by using skb->mac_len instead of ETH_HLEN, which makes
sure that the skb headers are correctly restored. This usually does not
change anything, execpt the local bridge transmits which now need to set
the skb->mac_len correctly in br_dev_xmit, as well as the broken case noted
above.
Signed-off-by: Zahari Doychev <zahari.doychev@linux.com>
---
v2->v3:
- move cover letter description to commit message
---
net/bridge/br_device.c | 3 ++-
net/bridge/br_forward.c | 4 ++--
net/bridge/br_vlan.c | 3 ++-
3 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c
index 681b72862c16..aeb77ff60311 100644
--- a/net/bridge/br_device.c
+++ b/net/bridge/br_device.c
@@ -55,8 +55,9 @@ netdev_tx_t br_dev_xmit(struct sk_buff *skb, struct net_device *dev)
BR_INPUT_SKB_CB(skb)->frag_max_size = 0;
skb_reset_mac_header(skb);
+ skb_reset_mac_len(skb);
eth = eth_hdr(skb);
- skb_pull(skb, ETH_HLEN);
+ skb_pull(skb, skb->mac_len);
if (!br_allowed_ingress(br, br_vlan_group_rcu(br), skb, &vid))
goto out;
diff --git a/net/bridge/br_forward.c b/net/bridge/br_forward.c
index 86637000f275..edb4f3533f05 100644
--- a/net/bridge/br_forward.c
+++ b/net/bridge/br_forward.c
@@ -32,7 +32,7 @@ static inline int should_deliver(const struct net_bridge_port *p,
int br_dev_queue_push_xmit(struct net *net, struct sock *sk, struct sk_buff *skb)
{
- skb_push(skb, ETH_HLEN);
+ skb_push(skb, skb->mac_len);
if (!is_skb_forwardable(skb->dev, skb))
goto drop;
@@ -94,7 +94,7 @@ static void __br_forward(const struct net_bridge_port *to,
net = dev_net(indev);
} else {
if (unlikely(netpoll_tx_running(to->br->dev))) {
- skb_push(skb, ETH_HLEN);
+ skb_push(skb, skb->mac_len);
if (!is_skb_forwardable(skb->dev, skb))
kfree_skb(skb);
else
diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
index bb98984cd27d..419067b314d7 100644
--- a/net/bridge/br_vlan.c
+++ b/net/bridge/br_vlan.c
@@ -466,13 +466,14 @@ static bool __allowed_ingress(const struct net_bridge *br,
/* Tagged frame */
if (skb->vlan_proto != br->vlan_proto) {
/* Protocol-mismatch, empty out vlan_tci for new tag */
- skb_push(skb, ETH_HLEN);
+ skb_push(skb, skb->mac_len);
skb = vlan_insert_tag_set_proto(skb, skb->vlan_proto,
skb_vlan_tag_get(skb));
if (unlikely(!skb))
return false;
skb_pull(skb, ETH_HLEN);
+ skb_reset_network_header(skb);
skb_reset_mac_len(skb);
*vid = 0;
tagged = false;
--
2.22.0
^ permalink raw reply related
* Re: [PATCH v3 1/2] net: core: Notify on changes to dev->promiscuity.
From: Allan W. Nielsen @ 2019-09-02 18:05 UTC (permalink / raw)
To: Jiri Pirko
Cc: David Miller, idosch, andrew, horatiu.vultur, alexandre.belloni,
UNGLinuxDriver, ivecera, f.fainelli, netdev, linux-kernel
In-Reply-To: <20190902175124.GA2312@nanopsycho>
The 09/02/2019 19:51, Jiri Pirko wrote:
> External E-Mail
>
>
> Mon, Sep 02, 2019 at 07:42:31PM CEST, allan.nielsen@microchip.com wrote:
> >Hi Jiri,
> >
> >Sorry for joining the discussion this late, but I have been without mail access
> >for the last few days.
> >
> >
> >The 08/30/2019 08:36, Jiri Pirko wrote:
> >> Fri, Aug 30, 2019 at 08:02:33AM CEST, davem@davemloft.net wrote:
> >> >From: Jiri Pirko <jiri@resnulli.us>
> >> >Date: Fri, 30 Aug 2019 07:39:40 +0200
> >> >
> >> >> Because the "promisc mode" would gain another meaning. Now how the
> >> >> driver should guess which meaning the user ment when he setted it?
> >> >> filter or trap?
> >> >>
> >> >> That is very confusing. If the flag is the way to do this, let's
> >> >> introduce another flag, like IFF_TRAPPING indicating that user wants
> >> >> exactly this.
> >> >
> >> >I don't understand how the meaning of promiscuous mode for a
> >> >networking device has suddenly become ambiguous, when did this start
> >> >happening?
> >>
> >> The promiscuity is a way to setup the rx filter. So promics == rx filter
> >> off. For normal nics, where there is no hw fwd datapath,
> >> this coincidentally means all received packets go to cpu.
> >> But if there is hw fwd datapath, rx filter is still off, all rxed packets
> >> are processed. But that does not mean they should be trapped to cpu.
> >>
> >> Simple example:
> >> I need to see slowpath packets, for example arps/stp/bgp/... that
> >> are going to cpu, I do:
> >> tcpdump -i swp1
> >
> >How is this different from "tcpdump -p -i swp1"
> >
> >> I don't want to get all the traffic running over hw running this cmd.
> >> This is a valid usecase.
> >>
> >> To cope with hw fwd datapath devices, I believe that tcpdump has to have
> >> notion of that. Something like:
> >>
> >> tcpdump -i swp1 --hw-trapping-mode
> >>
> >> The logic can be inverse:
> >> tcpdump -i swp1
> >> tcpdump -i swp1 --no-hw-trapping-mode
> >>
> >> However, that would provide inconsistent behaviour between existing and
> >> patched tcpdump/kernel.
> >>
> >> All I'm trying to say, there are 2 flags
> >> needed (if we don't use tc trap).
> >
> >I have been reading through this thread several times and I still do not get it.
> >
> >As far as I understand you are arguing that we need 3 modes:
> >
> >- tcpdump -i swp1
>
> Depends on default. Promisc is on.
>
>
> >- tcpdump -p -i swp1
>
> All traffic that is trapped to the cpu by default, not promisc means
> only mac of the interface (if bridge for example haven't set promisc
> already) and special macs. So host traffic (ip of host), bgp, arp, nsnd,
> etc.
In the case where the interface is enslaved to a bridge, it is put into promisc
mode, which means that "tcpdump -i swp1" and "tcpdump -p -i swp1" give the same
result, right?
Is this desirable?
> >- tcpdump -i swp1 --hw-trapping-mode
>
> Promisc is on, all traffic received on the port and pushed to cpu. User
> has to be careful because in case of mlxsw this can lead to couple
> hundred gigabit traffic going over limited pci bandwidth (gigabits).
>
>
> >
> >Would you mind provide an example of the traffic you want to see in the 3 cases
> >(or the traffic which you do not want to see).
> >
> >/Allan
> >
>
--
/Allan
^ permalink raw reply
* Re: [PATCH v3 1/2] net: core: Notify on changes to dev->promiscuity.
From: Allan W. Nielsen @ 2019-09-02 17:55 UTC (permalink / raw)
To: Andrew Lunn
Cc: Ido Schimmel, David Miller, jiri, horatiu.vultur,
alexandre.belloni, UNGLinuxDriver, ivecera, f.fainelli, netdev,
linux-kernel
In-Reply-To: <20190901184819.GA24673@lunn.ch>
The 09/01/2019 20:48, Andrew Lunn wrote:
> > Look, this again boils down to what promisc mode means with regards to
> > hardware offload. You want it to mean punt all traffic to the CPU? Fine.
> > Does not seem like anyone will be switching sides anyway, so lets move
> > forward. But the current approach is not good. Each driver needs to have
> > this special case logic and the semantics of promisc mode change not
> > only with regards to the value of the promisc counter, but also with
> > regards to the interface's uppers. This is highly fragile and confusing.
> Yes, i agree. We want one function, in the core, which handles all the
> different uppers. Maybe 2, if we need to consider L2 and L3 switches
> differently.
Are you suggesting that we continue the path in v3, but add a utility function
to determinate if the interface needs to go into promisc mode (taking the
bridge stats, the promisc counter, and the upper devices into account).
Or are you suggest that we like Ido go back to v2?
/Allan
^ permalink raw reply
* Re: [net-next 3/3] ravb: TROCR register is only present on R-Car Gen3
From: Sergei Shtylyov @ 2019-09-02 17:54 UTC (permalink / raw)
To: Simon Horman, David Miller; +Cc: Magnus Damm, netdev, linux-renesas-soc
In-Reply-To: <20190902080603.5636-4-horms+renesas@verge.net.au>
On 09/02/2019 11:06 AM, Simon Horman wrote:
> Only use the TROCR register on R-Car Gen3.
> It is not present on other SoCs.
>
> Offsets used for the undocumented registers are also considered reserved
> and should not be written to.
>
> After some internal investigation with Renesas it remains unclear why this
> driver accesses these fields on R-Car Gen2 but regardless of what the
> historical reasons are the current code is considered incorrect.
Most probably copy&paste from sh_eth.c...
> Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
[...]
Acked-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
MBR, Sergei
^ permalink raw reply
* Re: [PATCH v3 1/2] net: core: Notify on changes to dev->promiscuity.
From: Jiri Pirko @ 2019-09-02 17:51 UTC (permalink / raw)
To: Allan W. Nielsen
Cc: David Miller, idosch, andrew, horatiu.vultur, alexandre.belloni,
UNGLinuxDriver, ivecera, f.fainelli, netdev, linux-kernel
In-Reply-To: <20190902174229.uur7r7duq4dvbnqq@lx-anielsen.microsemi.net>
Mon, Sep 02, 2019 at 07:42:31PM CEST, allan.nielsen@microchip.com wrote:
>Hi Jiri,
>
>Sorry for joining the discussion this late, but I have been without mail access
>for the last few days.
>
>
>The 08/30/2019 08:36, Jiri Pirko wrote:
>> Fri, Aug 30, 2019 at 08:02:33AM CEST, davem@davemloft.net wrote:
>> >From: Jiri Pirko <jiri@resnulli.us>
>> >Date: Fri, 30 Aug 2019 07:39:40 +0200
>> >
>> >> Because the "promisc mode" would gain another meaning. Now how the
>> >> driver should guess which meaning the user ment when he setted it?
>> >> filter or trap?
>> >>
>> >> That is very confusing. If the flag is the way to do this, let's
>> >> introduce another flag, like IFF_TRAPPING indicating that user wants
>> >> exactly this.
>> >
>> >I don't understand how the meaning of promiscuous mode for a
>> >networking device has suddenly become ambiguous, when did this start
>> >happening?
>>
>> The promiscuity is a way to setup the rx filter. So promics == rx filter
>> off. For normal nics, where there is no hw fwd datapath,
>> this coincidentally means all received packets go to cpu.
>> But if there is hw fwd datapath, rx filter is still off, all rxed packets
>> are processed. But that does not mean they should be trapped to cpu.
>>
>> Simple example:
>> I need to see slowpath packets, for example arps/stp/bgp/... that
>> are going to cpu, I do:
>> tcpdump -i swp1
>
>How is this different from "tcpdump -p -i swp1"
>
>> I don't want to get all the traffic running over hw running this cmd.
>> This is a valid usecase.
>>
>> To cope with hw fwd datapath devices, I believe that tcpdump has to have
>> notion of that. Something like:
>>
>> tcpdump -i swp1 --hw-trapping-mode
>>
>> The logic can be inverse:
>> tcpdump -i swp1
>> tcpdump -i swp1 --no-hw-trapping-mode
>>
>> However, that would provide inconsistent behaviour between existing and
>> patched tcpdump/kernel.
>>
>> All I'm trying to say, there are 2 flags
>> needed (if we don't use tc trap).
>
>I have been reading through this thread several times and I still do not get it.
>
>As far as I understand you are arguing that we need 3 modes:
>
>- tcpdump -i swp1
Depends on default. Promisc is on.
>- tcpdump -p -i swp1
All traffic that is trapped to the cpu by default, not promisc means
only mac of the interface (if bridge for example haven't set promisc
already) and special macs. So host traffic (ip of host), bgp, arp, nsnd,
etc.
>- tcpdump -i swp1 --hw-trapping-mode
Promisc is on, all traffic received on the port and pushed to cpu. User
has to be careful because in case of mlxsw this can lead to couple
hundred gigabit traffic going over limited pci bandwidth (gigabits).
>
>Would you mind provide an example of the traffic you want to see in the 3 cases
>(or the traffic which you do not want to see).
>
>/Allan
>
^ permalink raw reply
* Re: [PATCH v3 1/2] net: core: Notify on changes to dev->promiscuity.
From: Allan W. Nielsen @ 2019-09-02 17:42 UTC (permalink / raw)
To: Jiri Pirko
Cc: David Miller, idosch, andrew, horatiu.vultur, alexandre.belloni,
UNGLinuxDriver, ivecera, f.fainelli, netdev, linux-kernel
In-Reply-To: <20190830063624.GN2312@nanopsycho>
Hi Jiri,
Sorry for joining the discussion this late, but I have been without mail access
for the last few days.
The 08/30/2019 08:36, Jiri Pirko wrote:
> Fri, Aug 30, 2019 at 08:02:33AM CEST, davem@davemloft.net wrote:
> >From: Jiri Pirko <jiri@resnulli.us>
> >Date: Fri, 30 Aug 2019 07:39:40 +0200
> >
> >> Because the "promisc mode" would gain another meaning. Now how the
> >> driver should guess which meaning the user ment when he setted it?
> >> filter or trap?
> >>
> >> That is very confusing. If the flag is the way to do this, let's
> >> introduce another flag, like IFF_TRAPPING indicating that user wants
> >> exactly this.
> >
> >I don't understand how the meaning of promiscuous mode for a
> >networking device has suddenly become ambiguous, when did this start
> >happening?
>
> The promiscuity is a way to setup the rx filter. So promics == rx filter
> off. For normal nics, where there is no hw fwd datapath,
> this coincidentally means all received packets go to cpu.
> But if there is hw fwd datapath, rx filter is still off, all rxed packets
> are processed. But that does not mean they should be trapped to cpu.
>
> Simple example:
> I need to see slowpath packets, for example arps/stp/bgp/... that
> are going to cpu, I do:
> tcpdump -i swp1
How is this different from "tcpdump -p -i swp1"
> I don't want to get all the traffic running over hw running this cmd.
> This is a valid usecase.
>
> To cope with hw fwd datapath devices, I believe that tcpdump has to have
> notion of that. Something like:
>
> tcpdump -i swp1 --hw-trapping-mode
>
> The logic can be inverse:
> tcpdump -i swp1
> tcpdump -i swp1 --no-hw-trapping-mode
>
> However, that would provide inconsistent behaviour between existing and
> patched tcpdump/kernel.
>
> All I'm trying to say, there are 2 flags
> needed (if we don't use tc trap).
I have been reading through this thread several times and I still do not get it.
As far as I understand you are arguing that we need 3 modes:
- tcpdump -i swp1
- tcpdump -p -i swp1
- tcpdump -i swp1 --hw-trapping-mode
Would you mind provide an example of the traffic you want to see in the 3 cases
(or the traffic which you do not want to see).
/Allan
^ permalink raw reply
* Re: [net-next 2/3] ravb: Remove undocumented processing
From: Sergei Shtylyov @ 2019-09-02 17:41 UTC (permalink / raw)
To: Simon Horman, David Miller
Cc: Magnus Damm, netdev, linux-renesas-soc, Kazuya Mizuguchi
In-Reply-To: <20190902080603.5636-3-horms+renesas@verge.net.au>
On 09/02/2019 11:06 AM, Simon Horman wrote:
> From: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com>
>
> This patch removes the use of the undocumented registers
> CDCR, LCCR, CERCR, CEECR and the undocumented BOC bit of the CCC register.
The driver has many more #define's marked as undocumented. It's not clear
why you crammed the counters and the endianness bit in one patch. It clearly
needs to be split -- one patch for the MAC counters and one patch for the
AVB-DMAC bit.
> Current documentation for EtherAVB (ravb) describes the offset of
> what the driver uses as the BOC bit as reserved and that only a value of
> 0 should be written. Furthermore, the offsets used for the undocumented
> registers are also considered reserved nd should not be written to.
>
> After some internal investigation with Renesas it remains unclear
> why this driver accesses these fields but regardless of what the historical
> reasons are the current code is considered incorrect.
>
> Signed-off-by: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com>
> Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
[...]
MBR, Sergei
^ permalink raw reply
* Re: [net-next 11/15] i40e: Implement debug macro hw_dbg using pr_debug
From: Mauro Rodrigues @ 2019-09-02 17:24 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Jeff Kirsher, davem, netdev, nhorman, sassmann, Andrew Bowers
In-Reply-To: <20190828153936.57ababbc@cakuba.netronome.com>
On Wed, Aug 28, 2019 at 03:39:53PM -0700, Jakub Kicinski wrote:
> On Tue, 27 Aug 2019 23:44:03 -0700, Jeff Kirsher wrote:
> > diff --git a/drivers/net/ethernet/intel/i40e/i40e_osdep.h b/drivers/net/ethernet/intel/i40e/i40e_osdep.h
> > index a07574bff550..c0c9ce3eab23 100644
> > --- a/drivers/net/ethernet/intel/i40e/i40e_osdep.h
> > +++ b/drivers/net/ethernet/intel/i40e/i40e_osdep.h
> > @@ -18,7 +18,12 @@
> > * actual OS primitives
> > */
> >
> > -#define hw_dbg(hw, S, A...) do {} while (0)
> > +#define hw_dbg(hw, S, A...) \
> > +do { \
> > + int domain = pci_domain_nr(((struct i40e_pf *)(hw)->back)->pdev->bus); \
> > + pr_debug("i40e %04x:%02x:%02x.%x " S, domain, (hw)->bus.bus_id, \
> > + (hw)->bus.device, (hw)->bus.func, ## A); \
>
> This looks like open coded dev_dbg() / dev_namie(), why?
Indeed, thanks for pointing out. I'll fix this and the other patch you
reviewed and resubmit.
I'm not sure what should be the preferred approach here though, just use
dev_dbg to implement this macro or replace all of its occurrence in the
code by dev_dbg?
Jeff, do you have any preference?
>
> > +} while (0)
^ permalink raw reply
* Re: [net-next 1/3] ravb: correct typo in FBP field of SFO register
From: Sergei Shtylyov @ 2019-09-02 17:12 UTC (permalink / raw)
To: Simon Horman, David Miller
Cc: Magnus Damm, netdev, linux-renesas-soc, Kazuya Mizuguchi
In-Reply-To: <20190902080603.5636-2-horms+renesas@verge.net.au>
Hello!
On 09/02/2019 11:06 AM, Simon Horman wrote:
> From: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com>
>
> The field name is FBP rather than FPB.
>
> This field is unused and could equally be removed from the driver entirely.
> But there seems no harm in leaving as documentation of the presence of the
> field.
>
> Signed-off-by: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com>
> Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
Acked-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
> ---
> v0 - Kazuya Mizuguchi
>
> v1 - Simon Horman
> * Extracted from larger patch
I'd just claim the authorship in this case (and mentioned that it's based
on Mizuguchi-san's large patch right in the change log).
> * Wrote changelog
>
> diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
> index ac9195add811..bdb051f04b0c 100644
> --- a/drivers/net/ethernet/renesas/ravb.h
> +++ b/drivers/net/ethernet/renesas/ravb.h
> @@ -317,7 +312,7 @@ enum UFCD_BIT {
>
> /* SFO */
> enum SFO_BIT {
> - SFO_FPB = 0x0000003F,
> + SFO_FBP = 0x0000003F,
> };
>
> /* RTC */
> ---
This is where the actual patch starts, right?
> drivers/net/ethernet/renesas/ravb.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
> index ac9195add811..2596a95a4300 100644
> --- a/drivers/net/ethernet/renesas/ravb.h
> +++ b/drivers/net/ethernet/renesas/ravb.h
> @@ -317,7 +317,7 @@ enum UFCD_BIT {
>
> /* SFO */
> enum SFO_BIT {
> - SFO_FPB = 0x0000003F,
> + SFO_FBP = 0x0000003F,
> };
>
> /* RTC */
MBR, Sergei
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox