* Re: [PATCH net-next 1/3] net: phy: Create sysfs reciprocal links for attached_dev/phydev
From: Florian Fainelli @ 2017-05-26 23:44 UTC (permalink / raw)
To: Woojung.Huh, netdev; +Cc: davem, andrew
In-Reply-To: <9235D6609DB808459E95D78E17F2E43D40A9DAA2@CHN-SV-EXMX02.mchp-main.com>
Hi Woojung,
On 05/26/2017 04:34 PM, Woojung.Huh@microchip.com wrote:
>> @@ -960,6 +960,15 @@ int phy_attach_direct(struct net_device *dev, struct
>> phy_device *phydev,
>>
>> phydev->attached_dev = dev;
>> dev->phydev = phydev;
>> + 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;
>>
> Florian,
>
> I knew that it is applied to net-next.
> However, sysfs_create_link() fails when fixed phy (drivers/net/phy/fixed_phy.c)
> Do you have a chance to test with it?
I did, my main test system (BCM7445 w/ bcm_sf2) has one normal PHY
driver and 3 fixed PHYs (including one for the CPU port/master netdev),
see below.
What kind of error do you get here?
# ls -l /sys/class/net/gphy/phydev
lrwxrwxrwx 1 root root 0 Jan 1 00:00
/sys/class/net/gphy/phydev ->
../../../f0b403c0.mdio/mdio_bus/f0b403c0.mdio/f0b403c0.mdio:05
# ls -l /sys/class/net/*/phydev
lrwxrwxrwx 1 root root 0 Jan 1 00:01
/sys/class/net/eth0/phydev -> ../../../../Fixed MDIO
bus.0/mdio_bus/fixed-0/fixed-0:00
lrwxrwxrwx 1 root root 0 Jan 1 00:00
/sys/class/net/gphy/phydev ->
../../../f0b403c0.mdio/mdio_bus/f0b403c0.mdio/f0b403c0.mdio:05
lrwxrwxrwx 1 root root 0 Jan 1 00:01
/sys/class/net/moca/phydev -> ../../../../../Fixed MDIO
bus.0/mdio_bus/fixed-0/fixed-0:02
lrwxrwxrwx 1 root root 0 Jan 1 00:01
/sys/class/net/rgmii_1/phydev -> ../../../mdio_bus/sf2-1/sf2-1:00
lrwxrwxrwx 1 root root 0 Jan 1 00:01
/sys/class/net/rgmii_2/phydev -> ../../../../../Fixed MDIO
bus.0/mdio_bus/fixed-0/fixed-0:01
# ls -l /sys/class/mdio_bus/fixed-0/*/attached_dev
lrwxrwxrwx 1 root root 0 Jan 1 00:01
/sys/class/mdio_bus/fixed-0/fixed-0:00/attached_dev ->
../../../../rdb/f04a0000.ethernet/net/eth0
lrwxrwxrwx 1 root root 0 Jan 1 00:02
/sys/class/mdio_bus/fixed-0/fixed-0:01/attached_dev ->
../../../../rdb/rdb:switch_top@f0b00000/f0b00000.ethernet_switch/net/rgmii_2
lrwxrwxrwx 1 root root 0 Jan 1 00:02
/sys/class/mdio_bus/fixed-0/fixed-0:02/attached_dev ->
../../../../rdb/rdb:switch_top@f0b00000/f0b00000.ethernet_switch/net/moca
# ls -l /sys/class/mdio_bus/f0b403c0.mdio/f0b403c0.mdio\:05/attached_dev
lrwxrwxrwx 1 root root 0 Jan 1 00:02
/sys/class/mdio_bus/f0b403c0.mdio/f0b403c0.mdio:05/attached_dev ->
../../../../f0b00000.ethernet_switch/net/gphy
>
> - Woojung
>
--
Florian
^ permalink raw reply
* RE: [PATCH net-next 1/3] net: phy: Create sysfs reciprocal links for attached_dev/phydev
From: Woojung.Huh @ 2017-05-26 23:52 UTC (permalink / raw)
To: f.fainelli, netdev; +Cc: davem, andrew
In-Reply-To: <d2d97683-3fcc-6603-09e1-13818464de18@gmail.com>
Hi Florian,
> > I knew that it is applied to net-next.
> > However, sysfs_create_link() fails when fixed phy
> (drivers/net/phy/fixed_phy.c)
> > Do you have a chance to test with it?
>
> I did, my main test system (BCM7445 w/ bcm_sf2) has one normal PHY
> driver and 3 fixed PHYs (including one for the CPU port/master netdev),
> see below.
>
> What kind of error do you get here?
sysfs_create_link() returns -2 (-ENOENT).
>
> # ls -l /sys/class/net/gphy/phydev
> lrwxrwxrwx 1 root root 0 Jan 1 00:00
> /sys/class/net/gphy/phydev ->
> ../../../f0b403c0.mdio/mdio_bus/f0b403c0.mdio/f0b403c0.mdio:05
> # ls -l /sys/class/net/*/phydev
> lrwxrwxrwx 1 root root 0 Jan 1 00:01
> /sys/class/net/eth0/phydev -> ../../../../Fixed MDIO
> bus.0/mdio_bus/fixed-0/fixed-0:00
> lrwxrwxrwx 1 root root 0 Jan 1 00:00
> /sys/class/net/gphy/phydev ->
> ../../../f0b403c0.mdio/mdio_bus/f0b403c0.mdio/f0b403c0.mdio:05
> lrwxrwxrwx 1 root root 0 Jan 1 00:01
> /sys/class/net/moca/phydev -> ../../../../../Fixed MDIO
> bus.0/mdio_bus/fixed-0/fixed-0:02
> lrwxrwxrwx 1 root root 0 Jan 1 00:01
> /sys/class/net/rgmii_1/phydev -> ../../../mdio_bus/sf2-1/sf2-1:00
> lrwxrwxrwx 1 root root 0 Jan 1 00:01
> /sys/class/net/rgmii_2/phydev -> ../../../../../Fixed MDIO
> bus.0/mdio_bus/fixed-0/fixed-0:01
>
> # ls -l /sys/class/mdio_bus/fixed-0/*/attached_dev
> lrwxrwxrwx 1 root root 0 Jan 1 00:01
> /sys/class/mdio_bus/fixed-0/fixed-0:00/attached_dev ->
> ../../../../rdb/f04a0000.ethernet/net/eth0
> lrwxrwxrwx 1 root root 0 Jan 1 00:02
> /sys/class/mdio_bus/fixed-0/fixed-0:01/attached_dev ->
> ../../../../rdb/rdb:switch_top@f0b00000/f0b00000.ethernet_switch/net/rgm
> ii_2
> lrwxrwxrwx 1 root root 0 Jan 1 00:02
> /sys/class/mdio_bus/fixed-0/fixed-0:02/attached_dev ->
> ../../../../rdb/rdb:switch_top@f0b00000/f0b00000.ethernet_switch/net/moc
> a
>
> # ls -l /sys/class/mdio_bus/f0b403c0.mdio/f0b403c0.mdio\:05/attached_dev
> lrwxrwxrwx 1 root root 0 Jan 1 00:02
> /sys/class/mdio_bus/f0b403c0.mdio/f0b403c0.mdio:05/attached_dev ->
> ../../../../f0b00000.ethernet_switch/net/gphy
/sys/class/mdio_bus/devices/fixed-0:00/* exists.
But not /sys/class/net/eth0.. because Ethernet driver initialization failed.
- 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 0:02 UTC (permalink / raw)
To: Woojung.Huh, netdev; +Cc: davem, andrew
In-Reply-To: <9235D6609DB808459E95D78E17F2E43D40A9DACE@CHN-SV-EXMX02.mchp-main.com>
On 05/26/2017 04:52 PM, Woojung.Huh@microchip.com wrote:
> Hi Florian,
>
>>> I knew that it is applied to net-next.
>>> However, sysfs_create_link() fails when fixed phy
>> (drivers/net/phy/fixed_phy.c)
>>> Do you have a chance to test with it?
>>
>> I did, my main test system (BCM7445 w/ bcm_sf2) has one normal PHY
>> driver and 3 fixed PHYs (including one for the CPU port/master netdev),
>> see below.
>>
>> What kind of error do you get here?
> sysfs_create_link() returns -2 (-ENOENT).
>
>>
>> # ls -l /sys/class/net/gphy/phydev
>> lrwxrwxrwx 1 root root 0 Jan 1 00:00
>> /sys/class/net/gphy/phydev ->
>> ../../../f0b403c0.mdio/mdio_bus/f0b403c0.mdio/f0b403c0.mdio:05
>> # ls -l /sys/class/net/*/phydev
>> lrwxrwxrwx 1 root root 0 Jan 1 00:01
>> /sys/class/net/eth0/phydev -> ../../../../Fixed MDIO
>> bus.0/mdio_bus/fixed-0/fixed-0:00
>> lrwxrwxrwx 1 root root 0 Jan 1 00:00
>> /sys/class/net/gphy/phydev ->
>> ../../../f0b403c0.mdio/mdio_bus/f0b403c0.mdio/f0b403c0.mdio:05
>> lrwxrwxrwx 1 root root 0 Jan 1 00:01
>> /sys/class/net/moca/phydev -> ../../../../../Fixed MDIO
>> bus.0/mdio_bus/fixed-0/fixed-0:02
>> lrwxrwxrwx 1 root root 0 Jan 1 00:01
>> /sys/class/net/rgmii_1/phydev -> ../../../mdio_bus/sf2-1/sf2-1:00
>> lrwxrwxrwx 1 root root 0 Jan 1 00:01
>> /sys/class/net/rgmii_2/phydev -> ../../../../../Fixed MDIO
>> bus.0/mdio_bus/fixed-0/fixed-0:01
>>
>> # ls -l /sys/class/mdio_bus/fixed-0/*/attached_dev
>> lrwxrwxrwx 1 root root 0 Jan 1 00:01
>> /sys/class/mdio_bus/fixed-0/fixed-0:00/attached_dev ->
>> ../../../../rdb/f04a0000.ethernet/net/eth0
>> lrwxrwxrwx 1 root root 0 Jan 1 00:02
>> /sys/class/mdio_bus/fixed-0/fixed-0:01/attached_dev ->
>> ../../../../rdb/rdb:switch_top@f0b00000/f0b00000.ethernet_switch/net/rgm
>> ii_2
>> lrwxrwxrwx 1 root root 0 Jan 1 00:02
>> /sys/class/mdio_bus/fixed-0/fixed-0:02/attached_dev ->
>> ../../../../rdb/rdb:switch_top@f0b00000/f0b00000.ethernet_switch/net/moc
>> a
>>
>> # ls -l /sys/class/mdio_bus/f0b403c0.mdio/f0b403c0.mdio\:05/attached_dev
>> lrwxrwxrwx 1 root root 0 Jan 1 00:02
>> /sys/class/mdio_bus/f0b403c0.mdio/f0b403c0.mdio:05/attached_dev ->
>> ../../../../f0b00000.ethernet_switch/net/gphy
>
> /sys/class/mdio_bus/devices/fixed-0:00/* exists.
> But not /sys/class/net/eth0.. because Ethernet driver initialization failed.
OK, I am confused now. You are describing what is going on with your
platform right? Can you describe a bit further here what is happening
and with which type of interface? Is this with the CPU interface or
something?
--
Florian
^ permalink raw reply
* Re: [patch iproute2 v2 repost 1/3] tc_filter: add support for chain index
From: Stephen Hemminger @ 2017-05-27 0:11 UTC (permalink / raw)
To: Daniel Borkmann
Cc: Jiri Pirko, netdev, davem, jhs, xiyou.wangcong, dsa, edumazet,
alexander.h.duyck, simon.horman, mlxsw
In-Reply-To: <59288675.4040009@iogearbox.net>
On Fri, 26 May 2017 21:48:05 +0200
Daniel Borkmann <daniel@iogearbox.net> wrote:
> On 05/23/2017 03:40 PM, Jiri Pirko wrote:
> > Mon, May 22, 2017 at 10:33:15PM CEST, stephen@networkplumber.org wrote:
> [...]
> >> Applied to net-next branch of iproute2
> >
> > Looks like you missed the uapi part:
> >
> > CC tc_filter.o
> > tc_filter.c: In function ‘tc_filter_modify’:
> > tc_filter.c:159:34: error: ‘TCA_CHAIN’ undeclared (first use in this function)
> > addattr32(&req.n, sizeof(req), TCA_CHAIN, chain_index);
> > ^~~~~~~~~
>
> Looks like still unresolved in the iproute2 -net-next branch ...
On the net-next branch include/linux/rtnetlink.h now has TCA_CHAIN, and everything
builds fine. You may have local changes or be using different includes
$ git grep TCA_CHAIN
include/linux/rtnetlink.h: TCA_CHAIN,
tc/tc_filter.c: addattr32(&req.n, sizeof(req), TCA_CHAIN, chain_index);
tc/tc_filter.c: if (tb[TCA_CHAIN]) {
tc/tc_filter.c: __u32 chain_index = rta_getattr_u32(tb[TCA_CHAIN]);
tc/tc_filter.c: addattr32(&req.n, sizeof(req), TCA_CHAIN, chain_index);
tc/tc_filter.c: addattr32(&req.n, sizeof(req), TCA_CHAIN, chain_index);
^ permalink raw reply
* Re: [PATCH net-next] net: dsa: remove dsa_port_is_bridged
From: Florian Fainelli @ 2017-05-27 0:15 UTC (permalink / raw)
To: Vivien Didelot, netdev; +Cc: linux-kernel, kernel, David S. Miller, Andrew Lunn
In-Reply-To: <20170526221242.14875-1-vivien.didelot@savoirfairelinux.com>
On 05/26/2017 03:12 PM, Vivien Didelot wrote:
> The helper is only used once and makes the code more complicated that it
> should. Remove it and reorganize the variables so that it fits on 80
> columns.
>
> Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
--
Florian
^ permalink raw reply
* RE: [PATCH net-next 1/3] net: phy: Create sysfs reciprocal links for attached_dev/phydev
From: Woojung.Huh @ 2017-05-27 0:20 UTC (permalink / raw)
To: f.fainelli, netdev; +Cc: davem, andrew
In-Reply-To: <44f24fbd-5093-faa2-02cd-f8d4d835b59d@gmail.com>
> OK, I am confused now. You are describing what is going on with your
> platform right? Can you describe a bit further here what is happening
> and with which type of interface? Is this with the CPU interface or
> something?
Yes. It's on our platform.
Like your platform, fixed phy is used to connect switch CPU port/master netdev.
GMAC of SoC is cadence/macb.c with fixed phy modification.
static int macb_mii_probe(struct net_device *dev)
{
...
phydev = phy_find_first(bp->mii_bus);
if (!phydev) {
phydev = fixed_phy_register(PHY_POLL, &fphy_status, -1, NULL);
if (IS_ERR(phydev)) {
netdev_err(dev, "no PHY found\n");
return -ENXIO;
}
}
...
When failed to find phydev from phy_find_first(), it forces to fixed phy.
...
/* attach the mac to the phy */
ret = phy_connect_direct(dev, phydev, &macb_handle_link_change,
bp->phy_interface);
sysfs_create_lin() inside of phy_connect_direct() fails.
What is driver you are testing? I can check the file.
Thanks.
- Woojung
^ permalink raw reply
* Re: [patch iproute2 v2 repost 1/3] tc_filter: add support for chain index
From: Daniel Borkmann @ 2017-05-27 0:24 UTC (permalink / raw)
To: Stephen Hemminger
Cc: Jiri Pirko, netdev, davem, jhs, xiyou.wangcong, dsa, edumazet,
alexander.h.duyck, simon.horman, mlxsw
In-Reply-To: <20170526171137.0acccbc5@xeon-e3>
On 05/27/2017 02:11 AM, Stephen Hemminger wrote:
> On Fri, 26 May 2017 21:48:05 +0200
> Daniel Borkmann <daniel@iogearbox.net> wrote:
>
>> On 05/23/2017 03:40 PM, Jiri Pirko wrote:
>>> Mon, May 22, 2017 at 10:33:15PM CEST, stephen@networkplumber.org wrote:
>> [...]
>>>> Applied to net-next branch of iproute2
>>>
>>> Looks like you missed the uapi part:
>>>
>>> CC tc_filter.o
>>> tc_filter.c: In function ‘tc_filter_modify’:
>>> tc_filter.c:159:34: error: ‘TCA_CHAIN’ undeclared (first use in this function)
>>> addattr32(&req.n, sizeof(req), TCA_CHAIN, chain_index);
>>> ^~~~~~~~~
>>
>> Looks like still unresolved in the iproute2 -net-next branch ...
>
> On the net-next branch include/linux/rtnetlink.h now has TCA_CHAIN, and everything
> builds fine. You may have local changes or be using different includes
Ok, now it works, thanks for pushing the header rebase out.
^ 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 0:33 UTC (permalink / raw)
To: Woojung.Huh, netdev; +Cc: davem, andrew
In-Reply-To: <9235D6609DB808459E95D78E17F2E43D40A9DAF6@CHN-SV-EXMX02.mchp-main.com>
On 05/26/2017 05:20 PM, Woojung.Huh@microchip.com wrote:
>> OK, I am confused now. You are describing what is going on with your
>> platform right? Can you describe a bit further here what is happening
>> and with which type of interface? Is this with the CPU interface or
>> something?
>
> Yes. It's on our platform.
> Like your platform, fixed phy is used to connect switch CPU port/master netdev.
> GMAC of SoC is cadence/macb.c with fixed phy modification.
>
> static int macb_mii_probe(struct net_device *dev)
> {
> ...
> phydev = phy_find_first(bp->mii_bus);
> if (!phydev) {
> phydev = fixed_phy_register(PHY_POLL, &fphy_status, -1, NULL);
> if (IS_ERR(phydev)) {
> netdev_err(dev, "no PHY found\n");
> return -ENXIO;
> }> }
> ...
> When failed to find phydev from phy_find_first(), it forces to fixed phy.
> ...
> /* attach the mac to the phy */
> ret = phy_connect_direct(dev, phydev, &macb_handle_link_change,
> bp->phy_interface);
>
> sysfs_create_lin() inside of phy_connect_direct() fails.
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....
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()....
>
> What is driver you are testing? I can check the file.
Drivers involved are the following:
drivers/net/ethernet/broadcom/bcmsysport.c,
drivers/net/dsa/bcm_sf2.c
drivers/net/ethernet/broadcom/genet/
drivers/net/phy/bcm7xxx.c
--
Florian
^ permalink raw reply
* RE: [PATCH net-next 1/3] net: phy: Create sysfs reciprocal links for attached_dev/phydev
From: Woojung.Huh @ 2017-05-27 0:43 UTC (permalink / raw)
To: f.fainelli, netdev; +Cc: davem, andrew
In-Reply-To: <05a159af-6689-70db-9506-09059a09f1a5@gmail.com>
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.
> >
> > What is driver you are testing? I can check the file.
>
> Drivers involved are the following:
>
> drivers/net/ethernet/broadcom/bcmsysport.c,
> drivers/net/dsa/bcm_sf2.c
> drivers/net/ethernet/broadcom/genet/
> drivers/net/phy/bcm7xxx.c
Thanks for list of files.
- 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 0:46 UTC (permalink / raw)
To: Woojung.Huh, netdev; +Cc: davem, andrew
In-Reply-To: <9235D6609DB808459E95D78E17F2E43D40A9DB14@CHN-SV-EXMX02.mchp-main.com>
Hi Woojung,
On 05/26/2017 05:43 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;
err = sysfs_create_link(&dev->dev.kobj, &phydev->mdio.dev.kobj,
"phydev");
- if (err)
+ if (err && err != -ENOENT)
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:00 UTC (permalink / raw)
To: f.fainelli, netdev; +Cc: davem, andrew
In-Reply-To: <0413eb98-61cc-2fca-a4d6-7f4ef91843ba@gmail.com>
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;
+ }
phydev->dev_flags = flags;
- Woojung
^ permalink raw reply
* RE: [PATCH] e1000e: use disable_hardirq() also for MSIX vectors in e1000_netpoll()
From: Brown, Aaron F @ 2017-05-27 1:21 UTC (permalink / raw)
To: Konstantin Khlebnikov, netdev@vger.kernel.org,
intel-wired-lan@lists.osuosl.org, Kirsher, Jeffrey T
Cc: Dave Jones, WANG Cong, David S. Miller, Sabrina Dubroca
In-Reply-To: <149517832870.37403.16008624383220981105.stgit@buzz>
> From: netdev-owner@vger.kernel.org [mailto:netdev-
> owner@vger.kernel.org] On Behalf Of Konstantin Khlebnikov
> Sent: Friday, May 19, 2017 12:19 AM
> To: netdev@vger.kernel.org; intel-wired-lan@lists.osuosl.org; Kirsher, Jeffrey
> T <jeffrey.t.kirsher@intel.com>
> Cc: Dave Jones <davej@codemonkey.org.uk>; WANG Cong
> <xiyou.wangcong@gmail.com>; David S. Miller <davem@davemloft.net>;
> Sabrina Dubroca <sd@queasysnail.net>
> Subject: [PATCH] e1000e: use disable_hardirq() also for MSIX vectors in
> e1000_netpoll()
>
> Replace disable_irq() which waits for threaded irq handlers with
> disable_hardirq() which waits only for hardirq part.
>
> Signed-off-by: Konstantin Khlebnikov <khlebnikov@yandex-team.ru>
> Fixes: 311191297125 ("e1000: use disable_hardirq() for e1000_netpoll()")
> ---
> drivers/net/ethernet/intel/e1000e/netdev.c | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
Tested-by: Aaron Brown <aaron.f.brown@intel.com>
^ 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 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
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