* Re: [PATCH net-next 1/3] net: phy: Create sysfs reciprocal links for attached_dev/phydev
From: Florian Fainelli @ 2017-05-27 1:26 UTC (permalink / raw)
To: Woojung.Huh, netdev; +Cc: davem, andrew
In-Reply-To: <9235D6609DB808459E95D78E17F2E43D40A9DB32@CHN-SV-EXMX02.mchp-main.com>
On 05/26/2017 06:00 PM, Woojung.Huh@microchip.com wrote:
> Hi Florian,
>>>> OK, so here is what is happening: macb_mii_init() calls macb_mii_probe()
>>>> and so by the time we call phy_connect_direct(), we have not called
>>>> register_netdevice() yet, netdev_register_kobject() has not been called
>>>> either, and so sysfs_create_link() fails....
>>> I just found same thing.
>>> Yes, register_netdev() was not called at phy_connect_direct() time.
>>>
>>>> Let me think about a way to solve that, even though I am leaning towards
>>>> ignoring the errors from sysfs_create_link() rather than fixing each and
>>>> every Ethernet driver to make it probe its MII bus *after* calling
>>>> register_netdevice()....
>>> Agree.
>>
>> Thanks, would the following work for you? I don't want to do something
>> more complex than that, although, if we really wanted to see this
>> information, we could imagine having netdev_register_kobject() check
>> whether phydev->dev.kobj is valid, and set the symbolic link at that
>> point... The problem with that approach is that we are no longer
>> symetrical within the core PHYLIB code (phy_attach_direct and phy_detach).
>>
>> Let me know if this works so I can make a formal submission:
>>
>> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
>> index f84414b8f2ee..daad816ee1d1 100644
>> --- a/drivers/net/phy/phy_device.c
>> +++ b/drivers/net/phy/phy_device.c
>> @@ -960,14 +960,20 @@ int phy_attach_direct(struct net_device *dev,
>> struct phy_device *phydev,
>>
>> phydev->attached_dev = dev;
>> dev->phydev = phydev;
>> +
>> + /* Some Ethernet drivers try to connect to a PHY device before
>> + * calling register_netdevice(). register_netdevice() does
>> ultimately
>> + * lead to netdev_register_kobject() which would do the
>> dev->dev.kobj
>> + * initialization. Here we explicitly ignore those particular errors
>> + */
>> err = sysfs_create_link(&phydev->mdio.dev.kobj, &dev->dev.kobj,
>> "attached_dev");
>> - if (err)
>> + if (err && err != -ENOENT)
>> goto error;
> This one fine. However, next one returns -14 (-EFAULT)
>
>> err = sysfs_create_link(&dev->dev.kobj, &phydev->mdio.dev.kobj,
>> "phydev");
>> - if (err)
>> + if (err && err != -ENOENT)
>> goto error;
> No need to call 2nd sysfs_create_link(), how about following?
>
> err = sysfs_create_link(&phydev->mdio.dev.kobj, &dev->dev.kobj,
> "attached_dev");
> - if (err)
> + if (err && err != -ENOENT)
> goto error;
>
> - err = sysfs_create_link(&dev->dev.kobj, &phydev->mdio.dev.kobj,
> - "phydev");
> - if (err)
> - goto error;
> + if (!err) {
> + err = sysfs_create_link(&dev->dev.kobj, &phydev->mdio.dev.kobj,
> + "phydev");
> + if (err)
> + goto error;
> + }
Yes, that's a very valid point, how about we even do this:
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index f84414b8f2ee..dc666ec13651 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -960,15 +960,21 @@ int phy_attach_direct(struct net_device *dev,
struct phy_device *phydev,
phydev->attached_dev = dev;
dev->phydev = phydev;
+
+ /* Some Ethernet drivers try to connect to a PHY device before
+ * calling register_netdevice() -> netdev_register_kobject() and
+ * does the dev->dev.kobj initialization. Here we only check for
+ * success which indicates that the network device kobject is
+ * ready.
+ */
err = sysfs_create_link(&phydev->mdio.dev.kobj, &dev->dev.kobj,
"attached_dev");
- if (err)
- goto error;
-
- err = sysfs_create_link(&dev->dev.kobj, &phydev->mdio.dev.kobj,
- "phydev");
- if (err)
- goto error;
+ if (!err) {
+ err = sysfs_create_link(&dev->dev.kobj,
&phydev->mdio.dev.kobj,
+ "phydev");
+ if (err)
+ goto error;
+ }
phydev->dev_flags = flags;
--
Florian
^ permalink raw reply related
* RE: [PATCH net-next 1/3] net: phy: Create sysfs reciprocal links for attached_dev/phydev
From: Woojung.Huh @ 2017-05-27 1:31 UTC (permalink / raw)
To: f.fainelli, netdev; +Cc: davem, andrew
In-Reply-To: <dc4a3f50-3ff3-e690-9893-0575ddea42f6@gmail.com>
>
> Yes, that's a very valid point, how about we even do this:
>
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index f84414b8f2ee..dc666ec13651 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -960,15 +960,21 @@ int phy_attach_direct(struct net_device *dev,
> struct phy_device *phydev,
>
> phydev->attached_dev = dev;
> dev->phydev = phydev;
> +
> + /* Some Ethernet drivers try to connect to a PHY device before
> + * calling register_netdevice() -> netdev_register_kobject() and
> + * does the dev->dev.kobj initialization. Here we only check for
> + * success which indicates that the network device kobject is
> + * ready.
> + */
> err = sysfs_create_link(&phydev->mdio.dev.kobj, &dev->dev.kobj,
> "attached_dev");
> - if (err)
> - goto error;
> -
> - err = sysfs_create_link(&dev->dev.kobj, &phydev->mdio.dev.kobj,
> - "phydev");
> - if (err)
> - goto error;
> + if (!err) {
> + err = sysfs_create_link(&dev->dev.kobj,
> &phydev->mdio.dev.kobj,
> + "phydev");
> + if (err)
> + goto error;
> + }
>
> phydev->dev_flags = flags;
>
Looks better and clean.
How about sysfs_remove_link() in phy_detach()?
- Woojung
^ permalink raw reply
* Re: [PATCH net-next 1/3] net: phy: Create sysfs reciprocal links for attached_dev/phydev
From: Florian Fainelli @ 2017-05-27 3:22 UTC (permalink / raw)
To: Woojung.Huh, netdev; +Cc: davem, andrew
In-Reply-To: <9235D6609DB808459E95D78E17F2E43D40A9DB6A@CHN-SV-EXMX02.mchp-main.com>
On 05/26/2017 06:31 PM, Woojung.Huh@microchip.com wrote:
>>
>> Yes, that's a very valid point, how about we even do this:
>>
>> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
>> index f84414b8f2ee..dc666ec13651 100644
>> --- a/drivers/net/phy/phy_device.c
>> +++ b/drivers/net/phy/phy_device.c
>> @@ -960,15 +960,21 @@ int phy_attach_direct(struct net_device *dev,
>> struct phy_device *phydev,
>>
>> phydev->attached_dev = dev;
>> dev->phydev = phydev;
>> +
>> + /* Some Ethernet drivers try to connect to a PHY device before
>> + * calling register_netdevice() -> netdev_register_kobject() and
>> + * does the dev->dev.kobj initialization. Here we only check for
>> + * success which indicates that the network device kobject is
>> + * ready.
>> + */
>> err = sysfs_create_link(&phydev->mdio.dev.kobj, &dev->dev.kobj,
>> "attached_dev");
>> - if (err)
>> - goto error;
>> -
>> - err = sysfs_create_link(&dev->dev.kobj, &phydev->mdio.dev.kobj,
>> - "phydev");
>> - if (err)
>> - goto error;
>> + if (!err) {
>> + err = sysfs_create_link(&dev->dev.kobj,
>> &phydev->mdio.dev.kobj,
>> + "phydev");
>> + if (err)
>> + goto error;
>> + }
>>
>> phydev->dev_flags = flags;
>>
> Looks better and clean.
>
> How about sysfs_remove_link() in phy_detach()?
Good catch, I was able to reproduce the situation in which macb calls
phy_connect_direct(), will submit a patch shortly. Thanks!
--
Florian
^ permalink raw reply
* [PATCH net-next] net: phy: Relax error checking on sysfs_create_link()
From: Florian Fainelli @ 2017-05-27 3:34 UTC (permalink / raw)
To: netdev; +Cc: davem, andrew, Woojung.Huh, Florian Fainelli
Some Ethernet drivers will attach/connect to a PHY device before calling
register_netdevice() which is responsible for calling netdev_register_kobject()
which would do the network device's kobject initialization. In such a case,
sysfs_create_link() would return -ENOENT because the network device's kobject
is not ready yet, and we would fail to connect to the PHY device.
In order to keep things simple and symetrical, we just take the success path as
indicative of the ability to access the network device's kobject, and create
the second link if that's the case.
Fixes: 5568363f0cb3 ("net: phy: Create sysfs reciprocal links for attached_dev/phydev")
Reported-by: Woojung Hung <Woojung.Huh@microchip.com>
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
drivers/net/phy/phy_device.c | 28 ++++++++++++++++++++--------
include/linux/phy.h | 2 ++
2 files changed, 22 insertions(+), 8 deletions(-)
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index f84414b8f2ee..523366bd705a 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -960,15 +960,25 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
phydev->attached_dev = dev;
dev->phydev = phydev;
+
+ /* Some Ethernet drivers try to connect to a PHY device before
+ * calling register_netdevice() -> netdev_register_kobject() and
+ * does the dev->dev.kobj initialization. Here we only check for
+ * success which indicates that the network device kobject is
+ * ready. Once we do that we still need to keep track of whether
+ * links were successfully set up or not for phy_detach() to
+ * remove them accordingly.
+ */
err = sysfs_create_link(&phydev->mdio.dev.kobj, &dev->dev.kobj,
"attached_dev");
- if (err)
- goto error;
+ if (!err) {
+ err = sysfs_create_link(&dev->dev.kobj, &phydev->mdio.dev.kobj,
+ "phydev");
+ if (err)
+ goto error;
- err = sysfs_create_link(&dev->dev.kobj, &phydev->mdio.dev.kobj,
- "phydev");
- if (err)
- goto error;
+ phydev->sysfs_links = true;
+ }
phydev->dev_flags = flags;
@@ -1059,8 +1069,10 @@ void phy_detach(struct phy_device *phydev)
struct mii_bus *bus;
int i;
- sysfs_remove_link(&dev->dev.kobj, "phydev");
- sysfs_remove_link(&phydev->mdio.dev.kobj, "attached_dev");
+ if (phydev->sysfs_links) {
+ sysfs_remove_link(&dev->dev.kobj, "phydev");
+ sysfs_remove_link(&phydev->mdio.dev.kobj, "attached_dev");
+ }
phydev->attached_dev->phydev = NULL;
phydev->attached_dev = NULL;
phy_suspend(phydev);
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 5a808a26e4cf..58f1b45a4c44 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -363,6 +363,7 @@ struct phy_c45_device_ids {
* is_pseudo_fixed_link: Set to true if this phy is an Ethernet switch, etc.
* has_fixups: Set to true if this phy has fixups/quirks.
* suspended: Set to true if this phy has been suspended successfully.
+ * sysfs_links: Internal boolean tracking sysfs symbolic links setup/removal.
* state: state of the PHY for management purposes
* dev_flags: Device-specific flags used by the PHY driver.
* link_timeout: The number of timer firings to wait before the
@@ -399,6 +400,7 @@ struct phy_device {
bool is_pseudo_fixed_link;
bool has_fixups;
bool suspended;
+ bool sysfs_links;
enum phy_state state;
--
2.11.0
^ permalink raw reply related
* [PATCH] dsa: mv88e6xxx: fix returnvar.cocci warnings
From: Julia Lawall @ 2017-05-27 4:38 UTC (permalink / raw)
To: Andrew Lunn
Cc: netdev, Vivien Didelot, Florian Fainelli, netdev, linux-kernel,
kbuild-all
Remove unneeded variable used to store return value.
Generated by: scripts/coccinelle/misc/returnvar.cocci
CC: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Julia Lawall <julia.lawall@lip6.fr>
Signed-off-by: Fengguang Wu <fengguang.wu@intel.com>
---
It's a minor issue, but since there is no error, the code is a bit
misleading.
tree: https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git
master
head: 47936d35edbac5e58064bd15e51136050b2f2717
commit: 04aca9938255fc7097b3fb5700f408524656f2e2 [330/362] dsa: mv88e6xxx:
Enable/Disable SERDES on port enable/disable
chip.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -1982,13 +1982,12 @@ static int mv88e6xxx_port_enable(struct
struct phy_device *phydev)
{
struct mv88e6xxx_chip *chip = ds->priv;
- int err = 0;
mutex_lock(&chip->reg_lock);
mv88e6xxx_serdes_power(chip, port, true);
mutex_unlock(&chip->reg_lock);
- return err;
+ return 0;
}
static void mv88e6xxx_port_disable(struct dsa_switch *ds, int port,
^ permalink raw reply
* Re: [PATCH net-next v2 0/8] net: extend RTM_GETROUTE to return fib result
From: Roopa Prabhu @ 2017-05-27 6:00 UTC (permalink / raw)
To: David Miller
Cc: David Ahern, Rami Rosen, netdev@vger.kernel.org,
Nikolay Aleksandrov
In-Reply-To: <20170526.141838.790479821297151569.davem@davemloft.net>
On Fri, May 26, 2017 at 11:18 AM, David Miller <davem@davemloft.net> wrote:
> From: Roopa Prabhu <roopa@cumulusnetworks.com>
> Date: Thu, 25 May 2017 10:42:32 -0700
>
>> This series adds a new RTM_F_FIB_MATCH flag to return matched fib result
>> with RTM_GETROUTE. This is useful for applications and protocols in
>> userspace wanting to query the selected route.
>
> Looks good, series applied, thanks.
>
thank you.
> Have you considered taking this further and allowing one to see which
> nexthop a route lookup picked?
since the default RTM_GETROUTE output gives most of the attributes
from the resolved dst,
have not considered adding more to it yet...but certainly can if
needed in the future.
^ permalink raw reply
* Re: [PATCH 2/2] ARM: VF610: ZII devel b: Add switch eeprom-length properties
From: Shawn Guo @ 2017-05-27 8:10 UTC (permalink / raw)
To: Andrew Lunn; +Cc: David Miller, netdev, linux ARM, Vivien Didelot
In-Reply-To: <1495755884-31341-3-git-send-email-andrew@lunn.ch>
On Fri, May 26, 2017 at 01:44:44AM +0200, Andrew Lunn wrote:
> Two of the Ethernet switches on this board have EEPROMs connected.
> Add the eeprom-length property to the device tree, making it possible
> to access the EEPROM using ethtool -e.
>
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
Applied with a bit update on subject prefix. Thanks.
Shawn
^ permalink raw reply
* Re: [PATCH 0/2] Document and use eeprom-length property
From: Shawn Guo @ 2017-05-27 8:13 UTC (permalink / raw)
To: David Miller; +Cc: andrew, netdev, linux-arm-kernel, vivien.didelot
In-Reply-To: <20170526.150242.515745885812232154.davem@davemloft.net>
On Fri, May 26, 2017 at 03:02:42PM -0400, David Miller wrote:
> From: Andrew Lunn <andrew@lunn.ch>
> Date: Fri, 26 May 2017 01:44:42 +0200
>
> > The mv88e6xxx switch driver allows the size of the attached EEPROM to
> > be described in DT. This property is missing from the binding
> > documentation. Add it. And make use of it on the ZII Devel B board.
> >
> > David, Shawn, please could you talk amongs yourself to decide who
> > takes what.
>
> I can take this if it works for Shawn, otherwise I'm also fine if Shawn
> takes it and if so feel free to add my:
>
> Acked-by: David S. Miller <davem@davemloft.net>
Hi David,
I see these two patches can be applied separately, so I picked up 2/2
and left 1/2 to you.
Shawn
^ permalink raw reply
* Re: [patch net-next] net/sched: let chain_get to figure out the return value
From: Jiri Pirko @ 2017-05-27 9:54 UTC (permalink / raw)
To: David Miller; +Cc: netdev, jhs, xiyou.wangcong, mlxsw
In-Reply-To: <20170526.105912.229870645071385383.davem@davemloft.net>
Fri, May 26, 2017 at 04:59:12PM CEST, davem@davemloft.net wrote:
>From: Jiri Pirko <jiri@resnulli.us>
>Date: Fri, 26 May 2017 09:21:29 +0200
>
>> From: Jiri Pirko <jiri@mellanox.com>
>>
>> Alhough I believe that this create/nocreate dance is completelly
>> pointless, at least make it a bit nicer and easier to read.
>> Push the decision on what error value is returned to chain_get function
>> and use ERR macros.
>>
>> Signed-off-by: Jiri Pirko <jiri@mellanox.com>
>
>No, this is quite worse.
>
>You're leaving pointer error values in structures. That's extremely
>error prone.
Yet used everywhere in kernel.
>
>And as stated in the other thread, I don't think Cong's logic is strange
>or hard to understand at all.
That is why tc code looks how it does :/
But perhaps I'm slow and everything is crystal-clear to everyone else.
^ permalink raw reply
* Re: [Patch net-next] net_sched: only create filter chains for new filters/actions
From: Jiri Pirko @ 2017-05-27 10:02 UTC (permalink / raw)
To: David Miller; +Cc: xiyou.wangcong, netdev, jhs, jiri
In-Reply-To: <20170526.105443.1489276661727770629.davem@davemloft.net>
Fri, May 26, 2017 at 04:54:43PM CEST, davem@davemloft.net wrote:
>From: Jiri Pirko <jiri@resnulli.us>
>Date: Fri, 26 May 2017 07:53:52 +0200
>
>> Thu, May 25, 2017 at 06:14:56PM CEST, davem@davemloft.net wrote:
>>>From: Cong Wang <xiyou.wangcong@gmail.com>
>>>Date: Tue, 23 May 2017 09:42:37 -0700
>>>
>>>> tcf_chain_get() always creates a new filter chain if not found
>>>> in existing ones. This is totally unnecessary when we get or
>>>> delete filters, new chain should be only created for new filters
>>>> (or new actions).
>>>>
>>>> Fixes: 5bc1701881e3 ("net: sched: introduce multichain support for filters")
>>>> Cc: Jamal Hadi Salim <jhs@mojatatu.com>
>>>> Cc: Jiri Pirko <jiri@mellanox.com>
>>>> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
>>>
>>>Indeed, get and delete requests should not create new objects, ever.
>>>
>>>I have pretty much no idea why Jiri is making such a big fuss about
>>>this change, to be quite honest. :-)
>>
>> Because it makes already hard to read code even worse, for *no* benefit.
>> That's why.
>
>Jiri, if you say the same thing 100 times, it doesn't help anyone
>understand your arguments any better.
>
>Creating new objects when a GET or a DEL is requested is flat out
>wrong.
Allright. I ack that.
>
>And Cong is fixing that.
>
>And I also didn't find the boolean logic hard to understand at all.
>
>It is in fact a very common pattern to pass a "create" boolean into
>lookup functions, to tell them whether to create a new object on
>lookup failure or not. And then also to control that boolean via
>what kind of netlink request we are processing.
>
>So you tell me what's so bad about his change given the above?
>
>Give me details and real facts, like I just did, rather than vague
>statements about "benefit" and "hard to read".
What I don't like is the double "n->nlmsg_type == RTM_NEWTFILTER" check
and return value decusion according to the latter check. The code logic
is split into tcf_chain_get function and its caller. That is
at least odd.
Since you don't like the PTR_ERR approach, I'll try to figure out how to
do this another way.
^ permalink raw reply
* Re: [Patch net-next] net_sched: only create filter chains for new filters/actions
From: Jiri Pirko @ 2017-05-27 10:05 UTC (permalink / raw)
To: Cong Wang
Cc: David Miller, Linux Kernel Network Developers, Jamal Hadi Salim,
Jiri Pirko
In-Reply-To: <CAM_iQpUJcK0ZSqK8=a_D9dBw1OED++A=bC9-UUov4ng0mMpJUg@mail.gmail.com>
Fri, May 26, 2017 at 06:55:25PM CEST, xiyou.wangcong@gmail.com wrote:
>On Fri, May 26, 2017 at 7:54 AM, David Miller <davem@davemloft.net> wrote:
>> And I also didn't find the boolean logic hard to understand at all.
>>
>> It is in fact a very common pattern to pass a "create" boolean into
>> lookup functions, to tell them whether to create a new object on
>> lookup failure or not. And then also to control that boolean via
>> what kind of netlink request we are processing.
>
>+10
>
>It is a widely used pattern among the kernel source code.
>I'd be surprised if an experienced kernel developer is not
>aware of this pattern. ;)
Cong, as you wisely put, I'm not aware of this pattern and I'm also
unaware of existence of ternary operator. Are this notes necessary?
Does that make you feel better?
^ permalink raw reply
* Re: [PATCH 1/2] PCI: Add new PCIe Fabric End Node flag, PCI_DEV_FLAGS_NO_RELAXED_ORDERING
From: Ding Tianhong @ 2017-05-27 10:34 UTC (permalink / raw)
To: Alexander Duyck
Cc: Mark Rutland, Gabriele Paoloni, Asit K Mallick, Catalin Marinas,
Will Deacon, LinuxArm, Raj, Ashok, Bjorn Helgaas, Ganesh GR,
Jeff Kirsher, Bob Shaw, Casey Leedom, Patrick J Cramer, Arjun V.,
Michael Werner, linux-arm-kernel@lists.infradead.org, Amir Ancel,
Netdev, David Laight, Suravee Suthikulpanit, Robin Murphy,
David Miller, h
In-Reply-To: <CAKgT0UfwnGEf2yok5E2KeZPP2JkqhTpCf+25+A8j5jfupLRJ5A@mail.gmail.com>
On 2017/5/26 3:49, Alexander Duyck wrote:
> On Thu, May 25, 2017 at 6:35 AM, Ding Tianhong <dingtianhong@huawei.com> wrote:
>>
>> On 2017/5/9 8:48, Casey Leedom wrote:
>>>
>>> | From: Alexander Duyck <alexander.duyck@gmail.com>
>>> | Date: Saturday, May 6, 2017 11:07 AM
>>> |
>>> | | From: Ding Tianhong <dingtianhong@huawei.com>
>>> | | Date: Fri, May 5, 2017 at 8:08 PM
>>> | |
>>> | | According the suggestion, I could only think of this code:
>>> | | ..
>>> |
>>> | This is a bit simplistic but it is a start.
>>>
>>> Yes, something tells me that this is going to be more complicated than any
>>> of us like ...
>>>
>>> | The other bit I was getting at is that we need to update the core PCIe
>>> | code so that when we configure devices and the root complex reports no
>>> | support for relaxed ordering it should be clearing the relaxed
>>> | ordering bits in the PCIe configuration registers on the upstream
>>> | facing devices.
>>>
>>> Of course, this can be written to by the Driver at any time ... and is in
>>> the case of the cxgb4 Driver ...
>>>
>>> After a lot of rummaging around, it does look like KVM prohibits writes to
>>> the PCIe Capability Device Control register in drivers/xen/xen-pciback/
>>> conf_space_capability.c and conf_space.c simply because writes aren't
>>> allowed unless "permissive" is set. So it ~looks~ like a driver running in
>>> a Virtual Machine can't turn Enable Relaxed Ordering back on ...
>>>
>>> | The last bit we need in all this is a way to allow for setups where
>>> | peer-to-peer wants to perform relaxed ordering but for writes to the
>>> | host we have to not use relaxed ordering. For that we need to enable a
>>> | special case and that isn't handled right now in any of the solutions
>>> | we have coded up so far.
>>>
>>> Yes, we do need this.
>>>
>>>
>>> | From: Alexander Duyck <alexander.duyck@gmail.com>
>>> | Date: Saturday, May 8, 2017 08:22 AM
>>> |
>>> | The problem is we need to have something that can be communicated
>>> | through a VM. Your change doesn't work in that regard. That was why I
>>> | suggested just updating the code so that we when we initialized PCIe
>>> | devices what we do is either set or clear the relaxed ordering bit in
>>> | the PCIe device control register. That way when we direct assign an
>>> | interface it could know just based on the bits int the PCIe
>>> | configuration if it could use relaxed ordering or not.
>>> |
>>> | At that point the driver code itself becomes very simple since you
>>> | could just enable the relaxed ordering by default in the igb/ixgbe
>>> | driver and if the bit is set or cleared in the PCIe configuration then
>>> | we are either sending with relaxed ordering requests or not and don't
>>> | have to try and locate the root complex.
>>> |
>>> | So from the sound of it Casey has a special use case where he doesn't
>>> | want to send relaxed ordering frames to the root complex, but instead
>>> | would like to send them to another PCIe device. To do that he needs to
>>> | have a way to enable the relaxed ordering bit in the PCIe
>>> | configuration but then not send any to the root complex. Odds are that
>>> | is something he might be able to just implement in the driver, but is
>>> | something that may become a more general case in the future. I don't
>>> | see our change here impacting it as long as we keep the solution
>>> | generic and mostly confined to when we instantiate the devices as the
>>> | driver could likely make the decision to change the behavior later.
>>>
>>> It's not just me. Intel has said that while RO directed at the Root
>>> Complex Host Coherent Memory has a performance bug (not Data Corruption),
>>> it's a performance win for Peer-to-Peer writes to MMIO Space. (I'll be very
>>> interested in hearing what the bug is if we get that much detail. The very
>>> same TLPs directed to the Root Complex Port without Relaxed Ordering set get
>>> good performance. So this is essentially a bug in the hardware that was
>>> ~trying~ to implement a performance win.)
>>>
>>> Meanwhile, I currently only know of a single PCIe End Point which causes
>>> catastrophic results: the AMD A1100 ARM SoC ("SEATTLE"). And it's not even
>>> clear that product is even alive anymore since I haven't been able to get
>>> any responses from them for several months.
>>>
>>> What I'm saying is: let's try to architect a solution which doesn't throw
>>> the baby out with the bath water ...
>>>
>>> I think that if a Device's Root Complex Port has problems with Relaxed
>>> Ordering, it ~probably~ makes sense to turn off the PCIe Capability Device
>>> Control[Enable Relaxed Ordering] when we assign a device to a Virtual
>>> Machine since the Device Driver can no longer query the Relaxed Ordering
>>> Support of the Root Complex Port. The only down side of this would be if we
>>> assigned two Peers to a VM in an application which wanted to do Peer-to-Peer
>>> transfers. But that seems like a hard application to support in any case
>>> since the PCI Bus:Slot.Function IDs for assigned Devices within a VM don't
>>> match the actual values.
>>>
>>> For Devices running in the base OS/Hypervisor, their Drivers can query the
>>> Relaxed Ordering Support for the Root Complex Port or a Peer Device. So a
>>> simple flag within the (struct pci_dev *)->dev_flags would serve for that
>>> along with a per-Architecture/Platform mechanism for setting it ...
>>>
>>> Casey
>>>
>>
>> I have take a time to talk to our kvm team about how to distinguish the relaxed
>> ordering in the VM for some vf just like 82599-vf, the probe routine looks like
>> could work like this:
>> 1) QEMU could emulate the platform by the Vender ID and device ID which could be
>> read from the host.
>> 2) The QEMU could create a virtual PCIe dev complex and recognize the PCIe bus address which
>> come and detach from the host to the guest.
>> 3) the PCI quirk could enable the Relaxed Ordering by the Vendor ID and Device ID.
>> 4) The VF drivers could read the flag and set to the hw.
>>
>> So I think we could set the PCI_DEV_FLAGS_RELAXED_ORDERING_ENABLED for some special platform
>> and don't enable by default, if I miss something, please not hesitate to enlighten me :)
>
> This isn't what I had in mind at all.
>
> So what Casey had originally submitted was a step in the direction of
> what I was thinking. Basically on platforms where it is not advisable
> to enable relaxed ordering we need to advertise that relaxed ordering
> is not safe. Then when we are initializing the devices underneath
> those we need to be clearing the relaxed ordering enable bits in the
> PCI configuration, that is the piece that was missing from Casey's
> original patch. In addition we then need to have a way for devices to
> optionally enable relaxed ordering for cases like Casey pointed out
> where they might want to use relaxed ordering for peer-to-peer
> transactions, but not for transactions to the root complex. Finally in
> the case of the Intel drivers we could then just drop the compile time
> checks entirely and just enable the device to configure the internal
> registers for relaxed ordering because the configuration space becomes
> the spot that controls if this gets enabled or not.
>
> So the initial set of patches Casey submitted only really consisted of
> 2 patches. What I am proposing is that we would be looking at
> expanding this out to about 4 patches. The first patch is the original
> 1 of 2, the second patch would be to modify the PCI initialization
> code to clear the relaxed ordering enable bit in the event that the
> root complex doesn't want relaxed ordering enabled, the third would be
> to make changes to the Chelsio driver as needed to allow for the
> peer-to-peer case to be enabled when the relaxed ordering bit in the
> configuration space is not enabled without triggering any relaxed
> ordering requests to the root complex, and the last one would be to
> drop the defines in ixgbe and whatever other Intel drivers are
> currently checking for either SPARC or the define that was added to
> support relaxed ordering and just act like we are going to do it
> always with the PCI configuration space controlling if we do or not.
>
> Ideally as a part of the second patch we should have a way of testing
> if a given path can support relaxed ordering. That way when we go to
> try to enable a peer-to-peer setup we can be certain that a given path
> will work and don't try enabling it in paths that would be unsupported
> for peer-to-peer.
>
> This ends up being somewhat of a risk for the Intel NICs, but if the
> Chelsio devices have been running with relaxed ordering enabled for
> some time and have identified the chipsets that should be issues, then
> odds are we should be fine as well.
>
According to your opinion, I try to build the second patch again,
1. in the pcie probe time, the pci_configure_relaxed_ordering will used to set the relaxed ordering
bit for configuration space.
2. export the pcie_get_relaxed_ordering for drivers and the drivers could decide whether should enable the relaxed ordering.
If I still go to the wrong path, please correct me, thanks.
---------------------------------------------------------------
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index b01bd5b..0076e4a 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -4852,6 +4852,28 @@ int pcie_get_mps(struct pci_dev *dev)
}
EXPORT_SYMBOL(pcie_get_mps);
+int pcie_set_relaxed_ordering(struct pci_dev *dev)
+{
+ return pcie_capability_set_word(dev, PCI_EXP_DEVCTL, PCI_EXP_DEVCTL_RELAX_EN);
+}
+EXPORT_SYMBOL(pcie_set_relaxed_ordering);
+
+int pcie_clear_relaxed_ordering(struct pci_dev *dev)
+{
+ return pcie_capability_clear_word(dev, PCI_EXP_DEVCTL, PCI_EXP_DEVCTL_RELAX_EN);
+}
+EXPORT_SYMBOL(pcie_clear_relaxed_ordering);
+
+int pcie_get_relaxed_ordering(struct pci_dev *dev)
+{
+ u16 v;
+
+ pcie_capability_read_word(dev, PCI_EXP_DEVCTL, &v);
+
+ return (v & PCI_EXP_DEVCTL_RELAX_EN) >> 4;
+}
+EXPORT_SYMBOL(pcie_get_relaxed_ordering);
+
/**
* pcie_set_mps - set PCI Express maximum payload size
* @dev: PCI device to query
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 19c8950..aeb22b5 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1701,6 +1701,16 @@ static void pci_configure_extended_tags(struct pci_dev *dev)
PCI_EXP_DEVCTL_EXT_TAG);
}
+static void pci_configure_relaxed_ordering(struct pci_dev *dev)
+{
+ int ret;
+
+ if (dev && (dev->dev_flags & PCI_DEV_FLAGS_NO_RELAXED_ORDERING))
+ pcie_set_relaxed_ordering(dev);
+ else
+ pcie_clear_relaxed_ordering(dev);
+}
+
static void pci_configure_device(struct pci_dev *dev)
{
struct hotplug_params hpp;
@@ -1708,6 +1718,7 @@ static void pci_configure_device(struct pci_dev *dev)
pci_configure_mps(dev);
pci_configure_extended_tags(dev);
+ pci_configure_relaxed_ordering(dev);
memset(&hpp, 0, sizeof(hpp));
ret = pci_get_hp_params(dev, &hpp);
Thanks
Ding
>>> .
>>>
>>
>
> .
>
^ permalink raw reply related
* TCP get SND_CWND change on loss event
From: Lars Erik Storbukås @ 2017-05-27 12:12 UTC (permalink / raw)
To: Netdev
I want to store the value of snd_cwnd when a congestion event occurs
(value before snd_cwnd is reduced), and the new value of snd_cwnd (the
value it has been reduced to). In other words: the congestion window
before and after a congestion event occurs.
I'm uncertain where (and how) it would be logical to implement this. I
have found two possible locations in the tcp_input.c where (I think)
it could be implemented:
static void tcp_cong_control(...) {
...
if (tcp_in_cwnd_reduction(sk)) {
struct tcp_sock *tp = tcp_sk(sk);
prior_congestion_window = tp->snd_cwnd;
/* Reduce cwnd if state mandates */
tcp_cwnd_reduction(sk, acked_sacked, flag);
reduced_congestion_window = tp->snd_cwnd;
}
...
}
or
static void tcp_fastretrans_alert(...) {
...
default:
...
struct tcp_sock *tp = tcp_sk(sk);
prior_congestion_window = tp->snd_cwnd;
/* Otherwise enter Recovery state */
tcp_enter_recovery(sk, (flag & FLAG_ECE));
fast_rexmit = 1;
reduced_congestion_window = tp->snd_cwnd;
...
}
Does anyone have advice on where (and how) to implement this? Does any
of the proposed solutions above seem logical?
/ Lars Erik Storbukås
^ permalink raw reply
* Re: [PATCH v2 2/6] stmmac: pci: Use stmmac_pci_info for all devices
From: Andy Shevchenko @ 2017-05-27 13:13 UTC (permalink / raw)
To: Jan Kiszka
Cc: Giuseppe Cavallaro, Alexandre Torgue, David Miller, netdev,
linux-kernel@vger.kernel.org
In-Reply-To: <dc0eaa024838f4ef34913aed63315771ac81dcc1.1495814872.git.jan.kiszka@siemens.com>
On Fri, May 26, 2017 at 7:07 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> Make stmmac_default_data compatible with stmmac_pci_info.setup and use
> an info structure for all devices. This allows to make the probing more
> regular.
> +#define STMMAC_DEVICE(vendor_id, dev_id, info) { \
> + PCI_DEVICE(vendor_id, dev_id), \
Perhaps
#define STMMAC_DEVICE(_vid, _did, info) { \
PCI_DEVICE(PCI_VENDOR_ID_##_vid, _did), \
Or converting defines first to PCI_DEVICE_ID_*
and
#define STMMAC_DEVICE(_vid, _did, info) { \
PCI_DEVICE(PCI_VENDOR_ID_##_vid, PCI_DEVICE_ID_##_did),
\
which I like even better.
> + .driver_data = (kernel_ulong_t)&info \
> + }
> +
> static const struct pci_device_id stmmac_id_table[] = {
> - {PCI_DEVICE(STMMAC_VENDOR_ID, STMMAC_DEVICE_ID)},
> - {PCI_DEVICE(PCI_VENDOR_ID_STMICRO, PCI_DEVICE_ID_STMICRO_MAC)},
> - {PCI_VDEVICE(INTEL, STMMAC_QUARK_ID), (kernel_ulong_t)&quark_pci_info},
> + STMMAC_DEVICE(STMMAC_VENDOR_ID, STMMAC_DEVICE_ID, stmmac_pci_info),
> + STMMAC_DEVICE(PCI_VENDOR_ID_STMICRO, PCI_DEVICE_ID_STMICRO_MAC,
> + stmmac_pci_info),
> + STMMAC_DEVICE(PCI_VENDOR_ID_INTEL, STMMAC_QUARK_ID, quark_pci_info),
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH v2 2/6] stmmac: pci: Use stmmac_pci_info for all devices
From: Andy Shevchenko @ 2017-05-27 13:16 UTC (permalink / raw)
To: Jan Kiszka
Cc: Giuseppe Cavallaro, Alexandre Torgue, David Miller, netdev,
linux-kernel@vger.kernel.org
In-Reply-To: <CAHp75Ve=CJ-ZRmaAFX+tezMJ4u11v=ptrDDORF485f3_iFEWeg@mail.gmail.com>
On Sat, May 27, 2017 at 4:13 PM, Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Fri, May 26, 2017 at 7:07 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>> Make stmmac_default_data compatible with stmmac_pci_info.setup and use
>> an info structure for all devices. This allows to make the probing more
>> regular.
> Or converting defines first to PCI_DEVICE_ID_*
It looks even for previously mentioned approach we need to rename
constants first.
> and
>
> #define STMMAC_DEVICE(_vid, _did, info) { \
> PCI_DEVICE(PCI_VENDOR_ID_##_vid, PCI_DEVICE_ID_##_did),
> \
>
> which I like even better.
Or even
#define STMMAC_DEVICE(_vid, _did, info) { \
PCI_VDEVICE(_vid, PCI_DEVICE_ID_##_did), \
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH v2 3/6] stmmac: pci: Make stmmac_pci_find_phy_addr truly generic
From: Andy Shevchenko @ 2017-05-27 13:25 UTC (permalink / raw)
To: Jan Kiszka
Cc: Giuseppe Cavallaro, Alexandre Torgue, David Miller, netdev,
linux-kernel@vger.kernel.org
In-Reply-To: <61af7f05bb92da3220ed5799dfa5bb84e102f067.1495814872.git.jan.kiszka@siemens.com>
On Fri, May 26, 2017 at 7:07 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> Move the special case for the early Galileo firmware into
> quark_default_setup. This allows to use stmmac_pci_find_phy_addr for
> non-quark cases.
> ret = stmmac_pci_find_phy_addr(pdev, info);
> - if (ret < 0)
> - return ret;
> + if (ret < 0) {
> + /*
> + * Galileo boards with old firmware don't support DMI. We always
> + * use 1 here as PHY address, so at least the first found MAC
> + * controller would be probed.
> + */
> + if (!dmi_get_system_info(DMI_BOARD_NAME))
> + ret = 1;
> + else
> + return ret;
Perhaps
/* Return error to the caller on DMI enabled boards */
if (dmi_...)
return ret;
/*
* Comment goes here, I suppose.
*/
ret = 1;
> + }
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH v2 5/6] stmmac: pci: Use dmi_system_id table for retrieving PHY addresses
From: Andy Shevchenko @ 2017-05-27 13:28 UTC (permalink / raw)
To: Jan Kiszka
Cc: Giuseppe Cavallaro, Alexandre Torgue, David Miller, netdev,
linux-kernel@vger.kernel.org
In-Reply-To: <6e12b2e63c7eaab6db937a99fedf79ec806d176f.1495814872.git.jan.kiszka@siemens.com>
On Fri, May 26, 2017 at 7:07 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> Avoids reimplementation of DMI matching in stmmac_pci_find_phy_addr.
> struct stmmac_pci_dmi_data {
> - const char *name;
> - const char *asset_tag;
> - unsigned int func;
> + int func;
> int phy_addr;
> };
Can we leave unsigned type here...
> -static struct stmmac_pci_dmi_data quark_pci_dmi_data[] = {
> +static const struct stmmac_pci_dmi_data galileo_stmmac_dmi_data[] = {
> + {-1, -1},
> +};
> +static const struct stmmac_pci_dmi_data iot2040_stmmac_dmi_data[] = {
> + {-1, -1},
> +};
...and avoid this not so standard terminators?
> + .matches = {
> + DMI_EXACT_MATCH(DMI_BOARD_NAME, "GalileoGen2"),
> + },
> + .driver_data = (void *)galileo_stmmac_dmi_data,
Can't be slightly better
.driver_data = &galileo_stmmac_dmi_data,
?
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH v2 6/6] stmmac: pci: Remove setup handler indirection via stmmac_pci_info
From: Andy Shevchenko @ 2017-05-27 13:38 UTC (permalink / raw)
To: Jan Kiszka
Cc: Giuseppe Cavallaro, Alexandre Torgue, David Miller, netdev,
linux-kernel@vger.kernel.org
In-Reply-To: <6183ce221f01384f4d50b0cfd62beabe337f5b39.1495814872.git.jan.kiszka@siemens.com>
On Fri, May 26, 2017 at 7:07 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> By now, stmmac_pci_info only contains a single entry.
_For now_.
> Register this
> directly with the PCI device table, removing one indirection.
I am not sure this patch is needed.
Next time something comes up we would need to extend this and
effectively revert this change.
So, my vote is to leave it as is for now.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH net-next v2 0/8] net: extend RTM_GETROUTE to return fib result
From: David Ahern @ 2017-05-27 14:02 UTC (permalink / raw)
To: Roopa Prabhu, David Miller
Cc: Rami Rosen, netdev@vger.kernel.org, Nikolay Aleksandrov
In-Reply-To: <CAJieiUiK=G1B-LnNH_=MYjFjqQVWu2n15a9Tg5fi_fyvSDpcFQ@mail.gmail.com>
On 5/27/17 12:00 AM, Roopa Prabhu wrote:
> On Fri, May 26, 2017 at 11:18 AM, David Miller <davem@davemloft.net> wrote:
>> From: Roopa Prabhu <roopa@cumulusnetworks.com>
>> Date: Thu, 25 May 2017 10:42:32 -0700
>>
>>> This series adds a new RTM_F_FIB_MATCH flag to return matched fib result
>>> with RTM_GETROUTE. This is useful for applications and protocols in
>>> userspace wanting to query the selected route.
>>
>> Looks good, series applied, thanks.
>>
>
> thank you.
>
>> Have you considered taking this further and allowing one to see which
>> nexthop a route lookup picked?
>
> since the default RTM_GETROUTE output gives most of the attributes
> from the resolved dst,
> have not considered adding more to it yet...but certainly can if
> needed in the future.
>
One extension is to pass in prefix and length (plus any options to such
as metric to uniquely discriminate a route) and get back the route
entry. It is needed to retrieve the BPF code for routes with a bpf
encap. This patch set makes it easier.
^ permalink raw reply
* [PATCH V6 net-next 0/2] rtnetlink: Updates to rtnetlink_event()
From: Vladislav Yasevich @ 2017-05-27 14:14 UTC (permalink / raw)
To: netdev; +Cc: dsahern, roopa, jiri, vfalico, andy, Vladislav Yasevich
First is the patch to add IFLA_EVENT attribute to the netlink message. It
supports only currently white-listed events.
Like before, this is just an attribute that gets added to the rtnetlink
message only when the messaged was generated as a result of a netdev event.
In my case, this is necessary since I want to trap NETDEV_NOTIFY_PEERS
event (also possibly NETDEV_RESEND_IGMP event) and perform certain actions
in user space. This is not possible since the messages generated as
a result of netdev events do not usually contain any changed data. They
are just notifications. This patch exposes this notification type to
userspace.
Second, I remove duplicate messages that a result of a change to bonding
options. If netlink is used to configure bonding options, 2 messages
are generated, one as a result NETDEV_CHANGEINFODATA event triggered by
bonding code and one a result of device state changes triggered by
netdev_state_change (called from do_setlink).
V6: Updated names and refactored to make it less tied to netdev events.
(From David Ahern)
V5: Rebased. Added iproute2 patch to the series.
V4:
* Removed the patch the removed NETDEV_CHANGENAME from event whitelist.
It doesn't trigger duplicate messages since name changes can only be
done while device is down and netdev_state_change() doesn't report
changes while device is down.
* Added a patch to clean-up duplicate messages on bonding option changes.
V3: Rebased. Cleaned-up duplicate event.
V2: Added missed events (from David Ahern)
Vladislav Yasevich (2):
rtnl: Add support for netdev event to link messages
bonding: Prevent duplicate userspace notification
drivers/net/bonding/bond_main.c | 3 +-
drivers/net/bonding/bond_options.c | 27 ++++++++++++++--
include/linux/rtnetlink.h | 3 +-
include/net/bond_options.h | 2 ++
include/uapi/linux/if_link.h | 11 +++++++
net/core/dev.c | 2 +-
net/core/rtnetlink.c | 65 ++++++++++++++++++++++++++++++++------
7 files changed, 99 insertions(+), 14 deletions(-)
--
2.7.4
^ permalink raw reply
* [PATCH V6 net-next 1/2] rtnl: Add support for netdev event to link messages
From: Vladislav Yasevich @ 2017-05-27 14:14 UTC (permalink / raw)
To: netdev; +Cc: dsahern, roopa, jiri, vfalico, andy, Vladislav Yasevich
In-Reply-To: <1495894476-9726-1-git-send-email-vyasevic@redhat.com>
When netdev events happen, a rtnetlink_event() handler will send
messages for every event in it's white list. These messages contain
current information about a particular device, but they do not include
the iformation about which event just happened. So, it is impossible
to tell what just happend for these events.
This patch adds a new extension to RTM_NEWLINK message called IFLA_EVENT
that would have an encoding of event that triggered this
message. This would allow the the message consumer to easily determine
if it needs to perform certain actions.
Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
---
include/linux/rtnetlink.h | 3 +-
include/uapi/linux/if_link.h | 11 ++++++++
net/core/dev.c | 2 +-
net/core/rtnetlink.c | 65 ++++++++++++++++++++++++++++++++++++++------
4 files changed, 70 insertions(+), 11 deletions(-)
diff --git a/include/linux/rtnetlink.h b/include/linux/rtnetlink.h
index 57e5484..dea59c8 100644
--- a/include/linux/rtnetlink.h
+++ b/include/linux/rtnetlink.h
@@ -18,7 +18,8 @@ extern int rtnl_put_cacheinfo(struct sk_buff *skb, struct dst_entry *dst,
void rtmsg_ifinfo(int type, struct net_device *dev, unsigned change, gfp_t flags);
struct sk_buff *rtmsg_ifinfo_build_skb(int type, struct net_device *dev,
- unsigned change, gfp_t flags);
+ unsigned change, u32 event,
+ gfp_t flags);
void rtmsg_ifinfo_send(struct sk_buff *skb, struct net_device *dev,
gfp_t flags);
diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
index 15ac203..8ed679f 100644
--- a/include/uapi/linux/if_link.h
+++ b/include/uapi/linux/if_link.h
@@ -157,6 +157,7 @@ enum {
IFLA_GSO_MAX_SIZE,
IFLA_PAD,
IFLA_XDP,
+ IFLA_EVENT,
__IFLA_MAX
};
@@ -911,4 +912,14 @@ enum {
#define IFLA_XDP_MAX (__IFLA_XDP_MAX - 1)
+enum {
+ IFLA_EVENT_NONE,
+ IFLA_EVENT_REBOOT, /* internal reset / reboot */
+ IFLA_EVENT_FEATURES, /* change in offload features */
+ IFLA_EVENT_BONDING_FAILOVER, /* change in active slave */
+ IFLA_EVENT_NOTIFY_PEERS, /* re-sent grat. arp/ndisc */
+ IFLA_EVENT_IGMP_RESEND, /* re-sent IGMP JOIN */
+ IFLA_EVENT_BONDING_OPTIONS, /* change in bonding options */
+};
+
#endif /* _UAPI_LINUX_IF_LINK_H */
diff --git a/net/core/dev.c b/net/core/dev.c
index 3d98fbf..06e0a74 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -7084,7 +7084,7 @@ static void rollback_registered_many(struct list_head *head)
if (!dev->rtnl_link_ops ||
dev->rtnl_link_state == RTNL_LINK_INITIALIZED)
- skb = rtmsg_ifinfo_build_skb(RTM_DELLINK, dev, ~0U,
+ skb = rtmsg_ifinfo_build_skb(RTM_DELLINK, dev, ~0U, 0,
GFP_KERNEL);
/*
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index dab2834..07218eb 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -941,6 +941,7 @@ static noinline size_t if_nlmsg_size(const struct net_device *dev,
+ nla_total_size(MAX_PHYS_ITEM_ID_LEN) /* IFLA_PHYS_SWITCH_ID */
+ nla_total_size(IFNAMSIZ) /* IFLA_PHYS_PORT_NAME */
+ rtnl_xdp_size() /* IFLA_XDP */
+ + nla_total_size(4) /* IFLA_EVENT */
+ nla_total_size(1); /* IFLA_PROTO_DOWN */
}
@@ -1282,9 +1283,40 @@ static int rtnl_xdp_fill(struct sk_buff *skb, struct net_device *dev)
return err;
}
+static u32 rtnl_get_event(unsigned long event)
+{
+ u32 rtnl_event_type = IFLA_EVENT_NONE;
+
+ switch (event) {
+ case NETDEV_REBOOT:
+ rtnl_event_type = IFLA_EVENT_REBOOT;
+ break;
+ case NETDEV_FEAT_CHANGE:
+ rtnl_event_type = IFLA_EVENT_FEATURES;
+ break;
+ case NETDEV_BONDING_FAILOVER:
+ rtnl_event_type = IFLA_EVENT_BONDING_FAILOVER;
+ break;
+ case NETDEV_NOTIFY_PEERS:
+ rtnl_event_type = IFLA_EVENT_NOTIFY_PEERS;
+ break;
+ case NETDEV_RESEND_IGMP:
+ rtnl_event_type = IFLA_EVENT_IGMP_RESEND;
+ break;
+ case NETDEV_CHANGEINFODATA:
+ rtnl_event_type = IFLA_EVENT_BONDING_OPTIONS;
+ break;
+ default:
+ break;
+ }
+
+ return rtnl_event_type;
+}
+
static int rtnl_fill_ifinfo(struct sk_buff *skb, struct net_device *dev,
int type, u32 pid, u32 seq, u32 change,
- unsigned int flags, u32 ext_filter_mask)
+ unsigned int flags, u32 ext_filter_mask,
+ u32 event)
{
struct ifinfomsg *ifm;
struct nlmsghdr *nlh;
@@ -1333,6 +1365,11 @@ static int rtnl_fill_ifinfo(struct sk_buff *skb, struct net_device *dev,
nla_put_u8(skb, IFLA_PROTO_DOWN, dev->proto_down))
goto nla_put_failure;
+ if (event != IFLA_EVENT_NONE) {
+ if (nla_put_u32(skb, IFLA_EVENT, event))
+ goto nla_put_failure;
+ }
+
if (rtnl_fill_link_ifmap(skb, dev))
goto nla_put_failure;
@@ -1467,6 +1504,7 @@ static const struct nla_policy ifla_policy[IFLA_MAX+1] = {
[IFLA_LINK_NETNSID] = { .type = NLA_S32 },
[IFLA_PROTO_DOWN] = { .type = NLA_U8 },
[IFLA_XDP] = { .type = NLA_NESTED },
+ [IFLA_EVENT] = { .type = NLA_U32 },
};
static const struct nla_policy ifla_info_policy[IFLA_INFO_MAX+1] = {
@@ -1626,7 +1664,7 @@ static int rtnl_dump_ifinfo(struct sk_buff *skb, struct netlink_callback *cb)
NETLINK_CB(cb->skb).portid,
cb->nlh->nlmsg_seq, 0,
flags,
- ext_filter_mask);
+ ext_filter_mask, 0);
if (err < 0) {
if (likely(skb->len))
@@ -2736,7 +2774,7 @@ static int rtnl_getlink(struct sk_buff *skb, struct nlmsghdr *nlh,
return -ENOBUFS;
err = rtnl_fill_ifinfo(nskb, dev, RTM_NEWLINK, NETLINK_CB(skb).portid,
- nlh->nlmsg_seq, 0, 0, ext_filter_mask);
+ nlh->nlmsg_seq, 0, 0, ext_filter_mask, 0);
if (err < 0) {
/* -EMSGSIZE implies BUG in if_nlmsg_size */
WARN_ON(err == -EMSGSIZE);
@@ -2808,7 +2846,8 @@ static int rtnl_dump_all(struct sk_buff *skb, struct netlink_callback *cb)
}
struct sk_buff *rtmsg_ifinfo_build_skb(int type, struct net_device *dev,
- unsigned int change, gfp_t flags)
+ unsigned int change,
+ u32 event, gfp_t flags)
{
struct net *net = dev_net(dev);
struct sk_buff *skb;
@@ -2819,7 +2858,7 @@ struct sk_buff *rtmsg_ifinfo_build_skb(int type, struct net_device *dev,
if (skb == NULL)
goto errout;
- err = rtnl_fill_ifinfo(skb, dev, type, 0, 0, change, 0, 0);
+ err = rtnl_fill_ifinfo(skb, dev, type, 0, 0, change, 0, 0, event);
if (err < 0) {
/* -EMSGSIZE implies BUG in if_nlmsg_size() */
WARN_ON(err == -EMSGSIZE);
@@ -2840,18 +2879,25 @@ void rtmsg_ifinfo_send(struct sk_buff *skb, struct net_device *dev, gfp_t flags)
rtnl_notify(skb, net, 0, RTNLGRP_LINK, NULL, flags);
}
-void rtmsg_ifinfo(int type, struct net_device *dev, unsigned int change,
- gfp_t flags)
+static void rtmsg_ifinfo_event(int type, struct net_device *dev,
+ unsigned int change, u32 event,
+ gfp_t flags)
{
struct sk_buff *skb;
if (dev->reg_state != NETREG_REGISTERED)
return;
- skb = rtmsg_ifinfo_build_skb(type, dev, change, flags);
+ skb = rtmsg_ifinfo_build_skb(type, dev, change, event, flags);
if (skb)
rtmsg_ifinfo_send(skb, dev, flags);
}
+
+void rtmsg_ifinfo(int type, struct net_device *dev, unsigned int change,
+ gfp_t flags)
+{
+ rtmsg_ifinfo_event(type, dev, change, IFLA_EVENT_NONE, flags);
+}
EXPORT_SYMBOL(rtmsg_ifinfo);
static int nlmsg_populate_fdb_fill(struct sk_buff *skb,
@@ -4165,7 +4211,8 @@ static int rtnetlink_event(struct notifier_block *this, unsigned long event, voi
case NETDEV_NOTIFY_PEERS:
case NETDEV_RESEND_IGMP:
case NETDEV_CHANGEINFODATA:
- rtmsg_ifinfo(RTM_NEWLINK, dev, 0, GFP_KERNEL);
+ rtmsg_ifinfo_event(RTM_NEWLINK, dev, 0, rtnl_get_event(event),
+ GFP_KERNEL);
break;
default:
break;
--
2.7.4
^ permalink raw reply related
* [PATCH V6 net-next 2/2] bonding: Prevent duplicate userspace notification
From: Vladislav Yasevich @ 2017-05-27 14:14 UTC (permalink / raw)
To: netdev; +Cc: dsahern, roopa, jiri, vfalico, andy, Vladislav Yasevich,
David Ahern
In-Reply-To: <1495894476-9726-1-git-send-email-vyasevic@redhat.com>
Whenever a user changes bonding options, a NETDEV_CHANGEINFODATA
notificatin is generated which results in a rtnelink message to
be sent. While runnig 'ip monitor', we can actually see 2 messages,
one a result of the event, and the other a result of state change
that is generated bo netdev_state_change(). However, this is not
always the case. If bonding changes were done via sysfs or ifenslave
(old ioctl interface), then only 1 message is seen.
This patch removes duplicate messages in the case of using netlink
to configure bonding. It introduceds a separte function that
triggers a netdev event and uses that function in the syfs and ioctl
cases.
This was discovered while auditing all the different envents and
continues the effort of cleaning up duplicated netlink messages.
CC: David Ahern <dsa@cumulusnetworks.com>
CC: Jiri Pirko <jiri@resnulli.us>
Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
---
drivers/net/bonding/bond_main.c | 3 ++-
drivers/net/bonding/bond_options.c | 27 +++++++++++++++++++++++++--
include/net/bond_options.h | 2 ++
3 files changed, 29 insertions(+), 3 deletions(-)
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 7331331..d7aa137 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -3481,7 +3481,8 @@ static int bond_do_ioctl(struct net_device *bond_dev, struct ifreq *ifr, int cmd
case BOND_CHANGE_ACTIVE_OLD:
case SIOCBONDCHANGEACTIVE:
bond_opt_initstr(&newval, slave_dev->name);
- res = __bond_opt_set(bond, BOND_OPT_ACTIVE_SLAVE, &newval);
+ res = __bond_opt_set_notify(bond, BOND_OPT_ACTIVE_SLAVE,
+ &newval);
break;
default:
res = -EOPNOTSUPP;
diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c
index 1bcbb89..8ca6833 100644
--- a/drivers/net/bonding/bond_options.c
+++ b/drivers/net/bonding/bond_options.c
@@ -673,7 +673,30 @@ int __bond_opt_set(struct bonding *bond,
out:
if (ret)
bond_opt_error_interpret(bond, opt, ret, val);
- else if (bond->dev->reg_state == NETREG_REGISTERED)
+
+ return ret;
+}
+/**
+ * __bond_opt_set_notify - set a bonding option
+ * @bond: target bond device
+ * @option: option to set
+ * @val: value to set it to
+ *
+ * This function is used to change the bond's option value and trigger
+ * a notification to user sapce. It can be used for both enabling/changing
+ * an option and for disabling it. RTNL lock must be obtained before calling
+ * this function.
+ */
+int __bond_opt_set_notify(struct bonding *bond,
+ unsigned int option, struct bond_opt_value *val)
+{
+ int ret = -ENOENT;
+
+ ASSERT_RTNL();
+
+ ret = __bond_opt_set(bond, option, val);
+
+ if (!ret && (bond->dev->reg_state == NETREG_REGISTERED))
call_netdevice_notifiers(NETDEV_CHANGEINFODATA, bond->dev);
return ret;
@@ -696,7 +719,7 @@ int bond_opt_tryset_rtnl(struct bonding *bond, unsigned int option, char *buf)
if (!rtnl_trylock())
return restart_syscall();
bond_opt_initstr(&optval, buf);
- ret = __bond_opt_set(bond, option, &optval);
+ ret = __bond_opt_set_notify(bond, option, &optval);
rtnl_unlock();
return ret;
diff --git a/include/net/bond_options.h b/include/net/bond_options.h
index 1797235..d79d28f 100644
--- a/include/net/bond_options.h
+++ b/include/net/bond_options.h
@@ -104,6 +104,8 @@ struct bond_option {
int __bond_opt_set(struct bonding *bond, unsigned int option,
struct bond_opt_value *val);
+int __bond_opt_set_notify(struct bonding *bond, unsigned int option,
+ struct bond_opt_value *val);
int bond_opt_tryset_rtnl(struct bonding *bond, unsigned int option, char *buf);
const struct bond_opt_value *bond_opt_parse(const struct bond_option *opt,
--
2.7.4
^ permalink raw reply related
* [PATCH V6 net-next iproute] ip: Add support for netdev events to monitor
From: Vladislav Yasevich @ 2017-05-27 14:14 UTC (permalink / raw)
To: netdev; +Cc: dsahern, roopa, jiri, vfalico, andy, Vladislav Yasevich
In-Reply-To: <1495894476-9726-1-git-send-email-vyasevic@redhat.com>
Add IFLA_EVENT handling so that event types can be viewed with
'monitor' command. This gives a little more information for why
a given message was receivied.
Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
---
include/linux/if_link.h | 11 +++++++++++
ip/ipaddress.c | 21 +++++++++++++++++++++
2 files changed, 32 insertions(+)
diff --git a/include/linux/if_link.h b/include/linux/if_link.h
index 5a3a048..c0a6769 100644
--- a/include/linux/if_link.h
+++ b/include/linux/if_link.h
@@ -157,6 +157,7 @@ enum {
IFLA_GSO_MAX_SIZE,
IFLA_PAD,
IFLA_XDP,
+ IFLA_EVENT,
__IFLA_MAX
};
@@ -909,4 +910,14 @@ enum {
#define IFLA_XDP_MAX (__IFLA_XDP_MAX - 1)
+enum {
+ IFLA_EVENT_NONE,
+ IFLA_EVENT_REBOOT, /* internal reset / reboot */
+ IFLA_EVENT_FEATURES, /* change in offload features */
+ IFLA_EVENT_BONDING_FAILOVER, /* hange in active slave */
+ IFLA_EVENT_NOTIFY_PEERS, /* re-sent grat. arp/ndisc */
+ IFLA_EVENT_IGMP_RESEND, /* re-sent IGMP JOIN */
+ IFLA_EVENT_BONDING_OPTIONS, /* change in bonding options */
+};
+
#endif /* _LINUX_IF_LINK_H */
diff --git a/ip/ipaddress.c b/ip/ipaddress.c
index b8d9c7d..c6e7413 100644
--- a/ip/ipaddress.c
+++ b/ip/ipaddress.c
@@ -753,6 +753,24 @@ int print_linkinfo_brief(const struct sockaddr_nl *who,
return 0;
}
+static const char *netdev_events[] = {"NONE",
+ "REBOOT",
+ "FEATURE CHANGE",
+ "BONDING FAILOVER",
+ "NOTIFY PEERS",
+ "RESEND IGMP",
+ "BONDING OPTION"};
+
+static void print_dev_event(FILE *f, __u32 event)
+{
+ if (event >= ARRAY_SIZE(netdev_events))
+ fprintf(f, "event %d ", event);
+ else {
+ if (event)
+ fprintf(f, "event %s ", netdev_events[event]);
+ }
+}
+
int print_linkinfo(const struct sockaddr_nl *who,
struct nlmsghdr *n, void *arg)
{
@@ -858,6 +876,9 @@ int print_linkinfo(const struct sockaddr_nl *who,
if (filter.showqueue)
print_queuelen(fp, tb);
+ if (tb[IFLA_EVENT])
+ print_dev_event(fp, rta_getattr_u32(tb[IFLA_EVENT]));
+
if (!filter.family || filter.family == AF_PACKET || show_details) {
SPRINT_BUF(b1);
fprintf(fp, "%s", _SL_);
--
2.7.4
^ permalink raw reply related
* Re: [PATCH] dsa: mv88e6xxx: fix returnvar.cocci warnings
From: Andrew Lunn @ 2017-05-27 14:51 UTC (permalink / raw)
To: Julia Lawall
Cc: netdev, Vivien Didelot, Florian Fainelli, linux-kernel,
kbuild-all
In-Reply-To: <alpine.DEB.2.20.1705270635210.2907@hadrien>
On Sat, May 27, 2017 at 06:38:14AM +0200, Julia Lawall wrote:
> Remove unneeded variable used to store return value.
>
> Generated by: scripts/coccinelle/misc/returnvar.cocci
Hi Julia
Thanks for the patch. However, Vivien already submitted a patch.
Andrew
^ permalink raw reply
* Re: [PATCH net-next] net: ndisc.c: reduce size of __ndisc_fill_addr_option()
From: Alexey Dobriyan @ 2017-05-27 15:15 UTC (permalink / raw)
To: Linyu.Yuan; +Cc: netdev, davem
> --- a/net/ipv6/ndisc.c
> +++ b/net/ipv6/ndisc.c
> @@ -148,17 +148,18 @@ void __ndisc_fill_addr_option(struct sk_buff *skb, int type, void *data,
> space -= data_len;
> - if (space > 0)
> - memset(opt, 0, space);
> +
> + memset(opt, 0, space);
This can't be right.
And what size are you reducing?
^ 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