* Re: [PATCH] ethernet: stmmac: dwmac-rk: Add GMAC support for px30
From: Shawn Lin @ 2018-05-16 6:34 UTC (permalink / raw)
To: David Wu, robh+dt
Cc: davem, heiko, shawn.lin, mark.rutland, huangtao, netdev,
linux-kernel, linux-rockchip, linux-arm-kernel
In-Reply-To: <1526441925-12654-1-git-send-email-david.wu@rock-chips.com>
Hi David,
On 2018/5/16 11:38, David Wu wrote:
> Add constants and callback functions for the dwmac on px30 soc.
s/soc/SoC
> The base structure is the same, but registers and the bits in
> them moved slightly, and add the clk_mac_speed for the select
s/moved/are moved
> of mac speed.
for selecting mas speed.
>
> Signed-off-by: David Wu <david.wu@rock-chips.com>
git log --oneline drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
shows very inconsistent format wrt. commit title, so please
follow the exsiting exsamples as possible.
> ---
> .../devicetree/bindings/net/rockchip-dwmac.txt | 1 +
> drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c | 64 ++++++++++++++++++++++
> 2 files changed, 65 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/net/rockchip-dwmac.txt b/Documentation/devicetree/bindings/net/rockchip-dwmac.txt
> index 9c16ee2..3b71da7 100644
> --- a/Documentation/devicetree/bindings/net/rockchip-dwmac.txt
> +++ b/Documentation/devicetree/bindings/net/rockchip-dwmac.txt
It'd be better to split doc changes into a separate patch.
> @@ -4,6 +4,7 @@ The device node has following properties.
>
> Required properties:
> - compatible: should be "rockchip,<name>-gamc"
> + "rockchip,px30-gmac": found on PX30 SoCs
> "rockchip,rk3128-gmac": found on RK312x SoCs
> "rockchip,rk3228-gmac": found on RK322x SoCs
> "rockchip,rk3288-gmac": found on RK3288 SoCs
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
> index 13133b3..4b2ab71 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
> @@ -61,6 +61,7 @@ struct rk_priv_data {
> struct clk *mac_clk_tx;
> struct clk *clk_mac_ref;
> struct clk *clk_mac_refout;
> + struct clk *clk_mac_speed;
No need to do anything now but it seems you could consider doing some
cleanup by using clk bulk APIs in the future.
> struct clk *aclk_mac;
> struct clk *pclk_mac;
> struct clk *clk_phy;
> @@ -83,6 +84,64 @@ struct rk_priv_data {
> (((tx) ? soc##_GMAC_TXCLK_DLY_ENABLE : soc##_GMAC_TXCLK_DLY_DISABLE) | \
> ((rx) ? soc##_GMAC_RXCLK_DLY_ENABLE : soc##_GMAC_RXCLK_DLY_DISABLE))
>
> +#define PX30_GRF_GMAC_CON1 0X0904
s/0X0904/0x0904 , since the other constants in this file follow the
same format.
> +
> +/* PX30_GRF_GMAC_CON1 */
> +#define PX30_GMAC_PHY_INTF_SEL_RMII (GRF_CLR_BIT(4) | GRF_CLR_BIT(5) | \
> + GRF_BIT(6))
> +#define PX30_GMAC_SPEED_10M GRF_CLR_BIT(2)
> +#define PX30_GMAC_SPEED_100M GRF_BIT(2)
> +
> +static void px30_set_to_rmii(struct rk_priv_data *bsp_priv)
> +{
> + struct device *dev = &bsp_priv->pdev->dev;
> +
> + if (IS_ERR(bsp_priv->grf)) {
> + dev_err(dev, "%s: Missing rockchip,grf property\n", __func__);
> + return;
> + }
> +
> + regmap_write(bsp_priv->grf, PX30_GRF_GMAC_CON1,
> + PX30_GMAC_PHY_INTF_SEL_RMII);
> +}
> +
> +static void px30_set_rmii_speed(struct rk_priv_data *bsp_priv, int speed)
> +{
> + struct device *dev = &bsp_priv->pdev->dev;
> + int ret;
> +
> + if (IS_ERR(bsp_priv->clk_mac_speed)) {
> + dev_err(dev, "%s: Missing clk_mac_speed clock\n", __func__);
> + return;
> + }
> +
> + if (speed == 10) {
> + regmap_write(bsp_priv->grf, PX30_GRF_GMAC_CON1,
> + PX30_GMAC_SPEED_10M);
> +
> + ret = clk_set_rate(bsp_priv->clk_mac_speed, 2500000);
> + if (ret)
> + dev_err(dev, "%s: set clk_mac_speed rate 2500000 failed: %d\n",
> + __func__, ret);
> + } else if (speed == 100) {
> + regmap_write(bsp_priv->grf, PX30_GRF_GMAC_CON1,
> + PX30_GMAC_SPEED_100M);
> +
> + ret = clk_set_rate(bsp_priv->clk_mac_speed, 25000000);
> + if (ret)
> + dev_err(dev, "%s: set clk_mac_speed rate 25000000 failed: %d\n",
> + __func__, ret);
I know it follows the existing examples, but IMHO it duplicates
unnecessary code as all the difference is PX30_GMAC_SPEED_*
> +
> + } else {
> + dev_err(dev, "unknown speed value for RMII! speed=%d", speed);
> + }
> +}
> +
> +static const struct rk_gmac_ops px30_ops = {
> + .set_to_rmii = px30_set_to_rmii,
> + .set_rmii_speed = px30_set_rmii_speed,
> +};
> +
> #define RK3128_GRF_MAC_CON0 0x0168
> #define RK3128_GRF_MAC_CON1 0x016c
>
> @@ -1042,6 +1101,10 @@ static int rk_gmac_clk_init(struct plat_stmmacenet_data *plat)
> }
> }
>
> + bsp_priv->clk_mac_speed = devm_clk_get(dev, "clk_mac_speed");
Mightbe it'd be better to use "mac-speed" in DT bindings.
> + if (IS_ERR(bsp_priv->clk_mac_speed))
> + dev_err(dev, "cannot get clock %s\n", "clk_mac_speed");
> +
Would you like to handle deferred probe?
> if (bsp_priv->clock_input) {
> dev_info(dev, "clock input from PHY\n");
> } else {
> @@ -1424,6 +1487,7 @@ static int rk_gmac_resume(struct device *dev)
> static SIMPLE_DEV_PM_OPS(rk_gmac_pm_ops, rk_gmac_suspend, rk_gmac_resume);
>
> static const struct of_device_id rk_gmac_dwmac_match[] = {
> + { .compatible = "rockchip,px30-gmac", .data = &px30_ops },
> { .compatible = "rockchip,rk3128-gmac", .data = &rk3128_ops },
> { .compatible = "rockchip,rk3228-gmac", .data = &rk3228_ops },
> { .compatible = "rockchip,rk3288-gmac", .data = &rk3288_ops },
>
^ permalink raw reply
* Re: [PATCH 00/14] Modify action API for implementing lockless actions
From: Vlad Buslov @ 2018-05-16 6:43 UTC (permalink / raw)
To: Jamal Hadi Salim
Cc: netdev, davem, xiyou.wangcong, jiri, pablo, kadlec, fw, ast,
daniel, edumazet, kliteyn
In-Reply-To: <2ee4066e-643a-f901-8926-7001f8699163@mojatatu.com>
On Tue 15 May 2018 at 21:49, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> On 15/05/18 05:21 PM, Vlad Buslov wrote:
>>
>> On Tue 15 May 2018 at 18:25, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>>> On 14/05/18 04:46 PM, Vlad Buslov wrote:
>>>>
>>>> On Mon 14 May 2018 at 18:03, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>>>>> On 14/05/18 10:27 AM, Vlad Buslov wrote:
>>>
>>>
>>>> Hello Jamal,
>>>>
>>>> I'm trying to run tdc, but keep getting following error even on clean
>>>> branch without my patches:
>>>
>>> Vlad, not sure if you saw my email:
>>> Apply Roman's patch and try again
>>>
>>> https://marc.info/?l=linux-netdev&m=152639369112020&w=2
>>>
>>> cheers,
>>> jamal
>>
>> With patch applied I get following error:
>>
>> Test 7d50: Add skbmod action to set destination mac
>> exit: 255 0
>> dst MAC address <11:22:33:44:55:66>
>> RTNETLINK answers: No such file or directory
>> We have an error talking to the kernel
>>
>
> You may actually have broken something with your patches in this case.
Results is for net-next without my patches.
>
> Lucas - does this test pass on latest net-next?
>
> cheers,
> jamal
>
> PS:- any reason for the big Cc? I have trimmed it down.
This Cc was generated by gen_maintainer.
^ permalink raw reply
* Re: [PATCH net-next v2 15/15] arm64: dts: allwinner: a64: add SRAM controller device tree node
From: Chen-Yu Tsai @ 2018-05-16 6:47 UTC (permalink / raw)
To: Maxime Ripard
Cc: Icenowy Zheng, linux-arm-kernel, Mark Rutland, devicetree,
Stephen Boyd, netdev, Michael Turquette, Rob Herring,
Corentin Labbe, Mark Brown, Giuseppe Cavallaro, linux-clk
In-Reply-To: <20180514080310.ngev5h6cqe4taedl@flea>
On Mon, May 14, 2018 at 1:03 AM, Maxime Ripard
<maxime.ripard@bootlin.com> wrote:
> 1;5201;0c
> On Sun, May 13, 2018 at 12:37:49PM -0700, Chen-Yu Tsai wrote:
>> On Wed, May 2, 2018 at 4:54 AM, Maxime Ripard <maxime.ripard@bootlin.com> wrote:
>> > On Wed, May 02, 2018 at 06:19:51PM +0800, Icenowy Zheng wrote:
>> >>
>> >>
>> >> 于 2018年5月2日 GMT+08:00 下午5:53:21, Chen-Yu Tsai <wens@csie.org> 写到:
>> >> >On Wed, May 2, 2018 at 5:51 PM, Maxime Ripard
>> >> ><maxime.ripard@bootlin.com> wrote:
>> >> >> Hi,
>> >> >>
>> >> >> On Wed, May 02, 2018 at 12:12:27AM +0800, Chen-Yu Tsai wrote:
>> >> >>> From: Icenowy Zheng <icenowy@aosc.io>
>> >> >>>
>> >> >>> Allwinner A64 has a SRAM controller, and in the device tree
>> >> >currently
>> >> >>> we have a syscon node to enable EMAC driver to access the EMAC clock
>> >> >>> register. As SRAM controller driver can now export regmap for this
>> >> >>> register, replace the syscon node to the SRAM controller device
>> >> >node,
>> >> >>> and let EMAC driver to acquire its EMAC clock regmap.
>> >> >>>
>> >> >>> Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
>> >> >>> Signed-off-by: Chen-Yu Tsai <wens@csie.org>
>> >> >>> ---
>> >> >>> arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 23
>> >> >+++++++++++++++----
>> >> >>> 1 file changed, 19 insertions(+), 4 deletions(-)
>> >> >>>
>> >> >>> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
>> >> >b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
>> >> >>> index 1b2ef28c42bd..1c37659d9d41 100644
>> >> >>> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
>> >> >>> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
>> >> >>> @@ -168,10 +168,25 @@
>> >> >>> #size-cells = <1>;
>> >> >>> ranges;
>> >> >>>
>> >> >>> - syscon: syscon@1c00000 {
>> >> >>> - compatible =
>> >> >"allwinner,sun50i-a64-system-controller",
>> >> >>> - "syscon";
>> >> >>> + sram_controller: sram-controller@1c00000 {
>> >> >>> + compatible =
>> >> >"allwinner,sun50i-a64-sram-controller";
>> >> >>
>> >> >> I don't think there's anything preventing us from keeping the
>> >> >> -system-controller compatible. It's what was in the DT before, and
>> >> >> it's how it's called in the datasheet.
>> >> >
>> >> >I actually meant to ask you about this. The -system-controller
>> >> >compatible matches the datasheet better. Maybe we should just
>> >> >switch to that one?
>> >>
>> >> No, if we do the switch the system-controller compatible,
>> >> the device will be probed on the same memory region with
>> >> a syscon on old DTs.
>> >
>> > The device hasn't magically changed either. Maybe we just need to add
>> > a check to make sure we don't have the syscon compatible in the SRAM
>> > driver probe so that the double driver issue doesn't happen?
>>
>> The syscon interface (which is not even a full blown device driver)
>> only looks at the "syscon" compatible. Either way we're removing that
>> part from the device tree so things should be ok for new device trees.
>> As Maxime mentioned we can do a check for the syscon compatible and
>> either give a warning to the user asking them to update their device
>> tree, or not register our custom regmap, or not probe the SRAM driver.
>> Personally I prefer the first option. The system controller block is
>> probed before any syscon users, so we should be fine, given the dwmac
>> driver goes the custom regmap path first.
>>
>> BTW, I still might end up changing the compatible. The manual uses
>> "system control", not "system controller", which I think makes sense,
>> since it is just a bunch of register files, kind of like the GRF
>> (General Register Files) block found in Rockchip SoCs [1], and not an
>> actual "controller".
>
> I'm not really fond of that, but we should at least make it consistent
> on the other patches Paul sent then.
For the A10s / A13 right?
I think my naming is slightly better, but it's just a minor detail.
While we're still debating this, can I merge the R40 stuff first?
The driver bits are already in.
Thanks
ChenYu
^ permalink raw reply
* Re: [PATCH net-next v2 2/2] drivers: net: Remove device_node checks with of_mdiobus_register()
From: Antoine Tenart @ 2018-05-16 6:49 UTC (permalink / raw)
To: Florian Fainelli
Cc: netdev, Andrew Lunn, Vivien Didelot, David S. Miller,
Nicolas Ferre, Fugang Duan, Sergei Shtylyov, Giuseppe Cavallaro,
Alexandre Torgue, Jose Abreu, Grygorii Strashko, Woojung Huh,
Microchip Linux Driver Support, Rob Herring, Frank Rowand,
Antoine Tenart, Tobias Jordan, Russell King
In-Reply-To: <20180515235619.27773-3-f.fainelli@gmail.com>
Hi Florian,
On Tue, May 15, 2018 at 04:56:19PM -0700, Florian Fainelli wrote:
> A number of drivers have the following pattern:
>
> if (np)
> of_mdiobus_register()
> else
> mdiobus_register()
>
> which the implementation of of_mdiobus_register() now takes care of.
> Remove that pattern in drivers that strictly adhere to it.
>
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
> drivers/net/dsa/bcm_sf2.c | 8 ++------
> drivers/net/dsa/mv88e6xxx/chip.c | 5 +----
> drivers/net/ethernet/cadence/macb_main.c | 12 +++---------
> drivers/net/ethernet/freescale/fec_main.c | 8 ++------
> drivers/net/ethernet/marvell/mvmdio.c | 5 +----
For mvmdio,
Reviewed-by: Antoine Tenart <antoine.tenart@bootlin.com>
Thanks!
Antoine
> drivers/net/ethernet/renesas/sh_eth.c | 11 +++--------
> drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c | 5 +----
> drivers/net/ethernet/ti/davinci_mdio.c | 8 +++-----
> drivers/net/phy/mdio-gpio.c | 6 +-----
> drivers/net/phy/mdio-mscc-miim.c | 6 +-----
> drivers/net/usb/lan78xx.c | 7 ++-----
> 11 files changed, 20 insertions(+), 61 deletions(-)
>
> diff --git a/drivers/net/dsa/bcm_sf2.c b/drivers/net/dsa/bcm_sf2.c
> index ac621f44237a..02e8982519ce 100644
> --- a/drivers/net/dsa/bcm_sf2.c
> +++ b/drivers/net/dsa/bcm_sf2.c
> @@ -450,12 +450,8 @@ static int bcm_sf2_mdio_register(struct dsa_switch *ds)
> priv->slave_mii_bus->parent = ds->dev->parent;
> priv->slave_mii_bus->phy_mask = ~priv->indir_phy_mask;
>
> - if (dn)
> - err = of_mdiobus_register(priv->slave_mii_bus, dn);
> - else
> - err = mdiobus_register(priv->slave_mii_bus);
> -
> - if (err)
> + err = of_mdiobus_register(priv->slave_mii_bus, dn);
> + if (err && dn)
> of_node_put(dn);
>
> return err;
> diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
> index b23c11d9f4b2..2bb3f03ee1cb 100644
> --- a/drivers/net/dsa/mv88e6xxx/chip.c
> +++ b/drivers/net/dsa/mv88e6xxx/chip.c
> @@ -2454,10 +2454,7 @@ static int mv88e6xxx_mdio_register(struct mv88e6xxx_chip *chip,
> return err;
> }
>
> - if (np)
> - err = of_mdiobus_register(bus, np);
> - else
> - err = mdiobus_register(bus);
> + err = of_mdiobus_register(bus, np);
> if (err) {
> dev_err(chip->dev, "Cannot register MDIO bus (%d)\n", err);
> mv88e6xxx_g2_irq_mdio_free(chip, bus);
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index b4c9268100bb..3e93df5d4e3b 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -591,16 +591,10 @@ static int macb_mii_init(struct macb *bp)
> dev_set_drvdata(&bp->dev->dev, bp->mii_bus);
>
> np = bp->pdev->dev.of_node;
> + if (pdata)
> + bp->mii_bus->phy_mask = pdata->phy_mask;
>
> - if (np) {
> - err = of_mdiobus_register(bp->mii_bus, np);
> - } else {
> - if (pdata)
> - bp->mii_bus->phy_mask = pdata->phy_mask;
> -
> - err = mdiobus_register(bp->mii_bus);
> - }
> -
> + err = of_mdiobus_register(bp->mii_bus, np);
> if (err)
> goto err_out_free_mdiobus;
>
> diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
> index d4604bc8eb5b..f3e43db0d6cb 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -2052,13 +2052,9 @@ static int fec_enet_mii_init(struct platform_device *pdev)
> fep->mii_bus->parent = &pdev->dev;
>
> node = of_get_child_by_name(pdev->dev.of_node, "mdio");
> - if (node) {
> - err = of_mdiobus_register(fep->mii_bus, node);
> + err = of_mdiobus_register(fep->mii_bus, node);
> + if (node)
> of_node_put(node);
> - } else {
> - err = mdiobus_register(fep->mii_bus);
> - }
> -
> if (err)
> goto err_out_free_mdiobus;
>
> diff --git a/drivers/net/ethernet/marvell/mvmdio.c b/drivers/net/ethernet/marvell/mvmdio.c
> index 0495487f7b42..c5dac6bd2be4 100644
> --- a/drivers/net/ethernet/marvell/mvmdio.c
> +++ b/drivers/net/ethernet/marvell/mvmdio.c
> @@ -348,10 +348,7 @@ static int orion_mdio_probe(struct platform_device *pdev)
> goto out_mdio;
> }
>
> - if (pdev->dev.of_node)
> - ret = of_mdiobus_register(bus, pdev->dev.of_node);
> - else
> - ret = mdiobus_register(bus);
> + ret = of_mdiobus_register(bus, pdev->dev.of_node);
> if (ret < 0) {
> dev_err(&pdev->dev, "Cannot register MDIO bus (%d)\n", ret);
> goto out_mdio;
> diff --git a/drivers/net/ethernet/renesas/sh_eth.c b/drivers/net/ethernet/renesas/sh_eth.c
> index 5970d9e5ddf1..8dd41e08a6c6 100644
> --- a/drivers/net/ethernet/renesas/sh_eth.c
> +++ b/drivers/net/ethernet/renesas/sh_eth.c
> @@ -3025,15 +3025,10 @@ static int sh_mdio_init(struct sh_eth_private *mdp,
> pdev->name, pdev->id);
>
> /* register MDIO bus */
> - if (dev->of_node) {
> - ret = of_mdiobus_register(mdp->mii_bus, dev->of_node);
> - } else {
> - if (pd->phy_irq > 0)
> - mdp->mii_bus->irq[pd->phy] = pd->phy_irq;
> -
> - ret = mdiobus_register(mdp->mii_bus);
> - }
> + if (pd->phy_irq > 0)
> + mdp->mii_bus->irq[pd->phy] = pd->phy_irq;
>
> + ret = of_mdiobus_register(mdp->mii_bus, dev->of_node);
> if (ret)
> goto out_free_bus;
>
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
> index f5f37bfa1d58..5df1a608e566 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
> @@ -233,10 +233,7 @@ int stmmac_mdio_register(struct net_device *ndev)
> new_bus->phy_mask = mdio_bus_data->phy_mask;
> new_bus->parent = priv->device;
>
> - if (mdio_node)
> - err = of_mdiobus_register(new_bus, mdio_node);
> - else
> - err = mdiobus_register(new_bus);
> + err = of_mdiobus_register(new_bus, mdio_node);
> if (err != 0) {
> dev_err(dev, "Cannot register the MDIO bus\n");
> goto bus_register_fail;
> diff --git a/drivers/net/ethernet/ti/davinci_mdio.c b/drivers/net/ethernet/ti/davinci_mdio.c
> index 98a1c97fb95e..8ac72831af05 100644
> --- a/drivers/net/ethernet/ti/davinci_mdio.c
> +++ b/drivers/net/ethernet/ti/davinci_mdio.c
> @@ -429,12 +429,10 @@ static int davinci_mdio_probe(struct platform_device *pdev)
> * defined to support backward compatibility with DTs which assume that
> * Davinci MDIO will always scan the bus for PHYs detection.
> */
> - if (dev->of_node && of_get_child_count(dev->of_node)) {
> + if (dev->of_node && of_get_child_count(dev->of_node))
> data->skip_scan = true;
> - ret = of_mdiobus_register(data->bus, dev->of_node);
> - } else {
> - ret = mdiobus_register(data->bus);
> - }
> +
> + ret = of_mdiobus_register(data->bus, dev->of_node);
> if (ret)
> goto bail_out;
>
> diff --git a/drivers/net/phy/mdio-gpio.c b/drivers/net/phy/mdio-gpio.c
> index b501221819e1..4e4c8daf44c3 100644
> --- a/drivers/net/phy/mdio-gpio.c
> +++ b/drivers/net/phy/mdio-gpio.c
> @@ -179,11 +179,7 @@ static int mdio_gpio_probe(struct platform_device *pdev)
> if (!new_bus)
> return -ENODEV;
>
> - if (pdev->dev.of_node)
> - ret = of_mdiobus_register(new_bus, pdev->dev.of_node);
> - else
> - ret = mdiobus_register(new_bus);
> -
> + ret = of_mdiobus_register(new_bus, pdev->dev.of_node);
> if (ret)
> mdio_gpio_bus_deinit(&pdev->dev);
>
> diff --git a/drivers/net/phy/mdio-mscc-miim.c b/drivers/net/phy/mdio-mscc-miim.c
> index 8c689ccfdbca..badbc99bedd3 100644
> --- a/drivers/net/phy/mdio-mscc-miim.c
> +++ b/drivers/net/phy/mdio-mscc-miim.c
> @@ -151,11 +151,7 @@ static int mscc_miim_probe(struct platform_device *pdev)
> }
> }
>
> - if (pdev->dev.of_node)
> - ret = of_mdiobus_register(bus, pdev->dev.of_node);
> - else
> - ret = mdiobus_register(bus);
> -
> + ret = of_mdiobus_register(bus, pdev->dev.of_node);
> if (ret < 0) {
> dev_err(&pdev->dev, "Cannot register MDIO bus (%d)\n", ret);
> return ret;
> diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
> index 91761436709a..8dff87ec6d99 100644
> --- a/drivers/net/usb/lan78xx.c
> +++ b/drivers/net/usb/lan78xx.c
> @@ -1843,12 +1843,9 @@ static int lan78xx_mdio_init(struct lan78xx_net *dev)
> }
>
> node = of_get_child_by_name(dev->udev->dev.of_node, "mdio");
> - if (node) {
> - ret = of_mdiobus_register(dev->mdiobus, node);
> + ret = of_mdiobus_register(dev->mdiobus, node);
> + if (node)
> of_node_put(node);
> - } else {
> - ret = mdiobus_register(dev->mdiobus);
> - }
> if (ret) {
> netdev_err(dev->net, "can't register MDIO bus\n");
> goto exit1;
> --
> 2.14.1
>
--
Antoine Ténart, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply
* Re: [PATCH V2] mlx4_core: allocate ICM memory in page size chunks
From: Tariq Toukan @ 2018-05-16 7:04 UTC (permalink / raw)
To: Qing Huang, Tariq Toukan, davem, haakon.bugge, yanjun.zhu
Cc: netdev, linux-rdma, linux-kernel
In-Reply-To: <f4c5d8d3-1eea-8566-9921-a9dc48435f66@oracle.com>
On 15/05/2018 9:53 PM, Qing Huang wrote:
>
>
> On 5/15/2018 2:19 AM, Tariq Toukan wrote:
>>
>>
>> On 14/05/2018 7:41 PM, Qing Huang wrote:
>>>
>>>
>>> On 5/13/2018 2:00 AM, Tariq Toukan wrote:
>>>>
>>>>
>>>> On 11/05/2018 10:23 PM, Qing Huang wrote:
>>>>> When a system is under memory presure (high usage with fragments),
>>>>> the original 256KB ICM chunk allocations will likely trigger kernel
>>>>> memory management to enter slow path doing memory compact/migration
>>>>> ops in order to complete high order memory allocations.
>>>>>
>>>>> When that happens, user processes calling uverb APIs may get stuck
>>>>> for more than 120s easily even though there are a lot of free pages
>>>>> in smaller chunks available in the system.
>>>>>
>>>>> Syslog:
>>>>> ...
>>>>> Dec 10 09:04:51 slcc03db02 kernel: [397078.572732] INFO: task
>>>>> oracle_205573_e:205573 blocked for more than 120 seconds.
>>>>> ...
>>>>>
>>>>> With 4KB ICM chunk size on x86_64 arch, the above issue is fixed.
>>>>>
>>>>> However in order to support smaller ICM chunk size, we need to fix
>>>>> another issue in large size kcalloc allocations.
>>>>>
>>>>> E.g.
>>>>> Setting log_num_mtt=30 requires 1G mtt entries. With the 4KB ICM chunk
>>>>> size, each ICM chunk can only hold 512 mtt entries (8 bytes for
>>>>> each mtt
>>>>> entry). So we need a 16MB allocation for a table->icm pointer array to
>>>>> hold 2M pointers which can easily cause kcalloc to fail.
>>>>>
>>>>> The solution is to use vzalloc to replace kcalloc. There is no need
>>>>> for contiguous memory pages for a driver meta data structure (no need
>>>>> of DMA ops).
>>>>>
>>>>> Signed-off-by: Qing Huang <qing.huang@oracle.com>
>>>>> Acked-by: Daniel Jurgens <danielj@mellanox.com>
>>>>> Reviewed-by: Zhu Yanjun <yanjun.zhu@oracle.com>
>>>>> ---
>>>>> v2 -> v1: adjusted chunk size to reflect different architectures.
>>>>>
>>>>> drivers/net/ethernet/mellanox/mlx4/icm.c | 14 +++++++-------
>>>>> 1 file changed, 7 insertions(+), 7 deletions(-)
>>>>>
>>>>> diff --git a/drivers/net/ethernet/mellanox/mlx4/icm.c
>>>>> b/drivers/net/ethernet/mellanox/mlx4/icm.c
>>>>> index a822f7a..ccb62b8 100644
>>>>> --- a/drivers/net/ethernet/mellanox/mlx4/icm.c
>>>>> +++ b/drivers/net/ethernet/mellanox/mlx4/icm.c
>>>>> @@ -43,12 +43,12 @@
>>>>> #include "fw.h"
>>>>> /*
>>>>> - * We allocate in as big chunks as we can, up to a maximum of 256 KB
>>>>> - * per chunk.
>>>>> + * We allocate in page size (default 4KB on many archs) chunks to
>>>>> avoid high
>>>>> + * order memory allocations in fragmented/high usage memory
>>>>> situation.
>>>>> */
>>>>> enum {
>>>>> - MLX4_ICM_ALLOC_SIZE = 1 << 18,
>>>>> - MLX4_TABLE_CHUNK_SIZE = 1 << 18
>>>>> + MLX4_ICM_ALLOC_SIZE = 1 << PAGE_SHIFT,
>>>>> + MLX4_TABLE_CHUNK_SIZE = 1 << PAGE_SHIFT
>>>>
>>>> Which is actually PAGE_SIZE.
>>>
>>> Yes, we wanted to avoid high order memory allocations.
>>>
>>
>> Then please use PAGE_SIZE instead.
>
> PAGE_SIZE is usually defined as 1 << PAGE_SHIFT. So I think PAGE_SHIFT
> is actually more appropriate here.
>
Definition of PAGE_SIZE varies among different archs.
It is not always as simple as 1 << PAGE_SHIFT.
It might be:
PAGE_SIZE (1UL << PAGE_SHIFT)
PAGE_SIZE (_AC(1, UL) << PAGE_SHIFT)
etc...
Please replace 1 << PAGE_SHIFT with PAGE_SIZE.
>
>>
>>>> Also, please add a comma at the end of the last entry.
>>>
>>> Hmm..., followed the existing code style and checkpatch.pl didn't
>>> complain about the comma.
>>>
>>
>> I am in favor of having a comma also after the last element, so that
>> when another enum element is added we do not modify this line again,
>> which would falsely affect git blame.
>>
>> I know it didn't exist before your patch, but once we're here, let's
>> do it.
>
> I'm okay either way. If adding an extra comma is preferred by many
> people, someone should update checkpatch.pl to enforce it. :)
>
I agree.
Until then, please use an extra comma in this patch.
>>
>>>>
>>>>> };
>>>>> static void mlx4_free_icm_pages(struct mlx4_dev *dev, struct
>>>>> mlx4_icm_chunk *chunk)
>>>>> @@ -400,7 +400,7 @@ int mlx4_init_icm_table(struct mlx4_dev *dev,
>>>>> struct mlx4_icm_table *table,
>>>>> obj_per_chunk = MLX4_TABLE_CHUNK_SIZE / obj_size;
>>>>> num_icm = (nobj + obj_per_chunk - 1) / obj_per_chunk;
>>>>> - table->icm = kcalloc(num_icm, sizeof(*table->icm),
>>>>> GFP_KERNEL);
>>>>> + table->icm = vzalloc(num_icm * sizeof(*table->icm));
>>>>
>>>> Why not kvzalloc ?
>>>
>>> I think table->icm pointer array doesn't really need physically
>>> contiguous memory. Sometimes high order
>>> memory allocation by kmalloc variants may trigger slow path and cause
>>> tasks to be blocked.
>>>
>>
>> This is control path so it is less latency-sensitive.
>> Let's not produce unnecessary degradation here, please call kvzalloc
>> so we maintain a similar behavior when contiguous memory is available,
>> and a fallback for resiliency.
>
> No sure what exactly degradation is caused by vzalloc here. I think it's
> better to keep physically contiguous pages
> to other requests which really need them. Besides slow path/mem
> compacting can be really expensive.
>
Degradation is expected when you replace a contig memory with non-contig
memory, without any perf test.
We agree that when contig memory is not available, we should use
non-contig instead of simply failing, and for this you can call kvzalloc.
>>
>>> Thanks,
>>> Qing
>>>
>>>>
>>>>> if (!table->icm)
>>>>> return -ENOMEM;
>>>>> table->virt = virt;
>>>>> @@ -446,7 +446,7 @@ int mlx4_init_icm_table(struct mlx4_dev *dev,
>>>>> struct mlx4_icm_table *table,
>>>>> mlx4_free_icm(dev, table->icm[i], use_coherent);
>>>>> }
>>>>> - kfree(table->icm);
>>>>> + vfree(table->icm);
>>>>> return -ENOMEM;
>>>>> }
>>>>> @@ -462,5 +462,5 @@ void mlx4_cleanup_icm_table(struct mlx4_dev
>>>>> *dev, struct mlx4_icm_table *table)
>>>>> mlx4_free_icm(dev, table->icm[i], table->coherent);
>>>>> }
>>>>> - kfree(table->icm);
>>>>> + vfree(table->icm);
>>>>> }
>>>>>
>>>>
>>>> Thanks for your patch.
>>>>
>>>> I need to verify there is no dramatic performance degradation here.
>>>> You can prepare and send a v3 in the meanwhile.
>>>>
>>>> Thanks,
>>>> Tariq
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe
>>>> linux-rdma" in
>>>> the body of a message to majordomo@vger.kernel.org
>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply
* Re: [PATCH 08/14] net: sched: account for temporary action reference
From: Jiri Pirko @ 2018-05-16 7:12 UTC (permalink / raw)
To: Vlad Buslov
Cc: netdev, davem, jhs, xiyou.wangcong, pablo, kadlec, fw, ast,
daniel, edumazet, keescook, linux-kernel, netfilter-devel,
coreteam, kliteyn
In-Reply-To: <1526308035-12484-9-git-send-email-vladbu@mellanox.com>
Mon, May 14, 2018 at 04:27:09PM CEST, vladbu@mellanox.com wrote:
>tca_get_fill function has 'bind' and 'ref' arguments that get passed
>down to action dump function. These arguments values are subtracted from
>actual reference and bind counter values before writing them to skb.
>
>In order to prevent concurrent action delete, RTM_GETACTION handler
>acquires a reference to action before 'dumping' it and releases it
>afterwards. This reference is temporal and should not be accounted by
>userspace clients. (both logically and to preserver current API
>behavior)
>
>Use existing infrastructure of tca_get_fill arguments to subtract that
>temporary reference and not expose it to userspace.
>
>Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
>---
> net/sched/act_api.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
>diff --git a/net/sched/act_api.c b/net/sched/act_api.c
>index 3f02cd1..2772276e 100644
>--- a/net/sched/act_api.c
>+++ b/net/sched/act_api.c
>@@ -935,7 +935,7 @@ tcf_get_notify(struct net *net, u32 portid, struct nlmsghdr *n,
> if (!skb)
> return -ENOBUFS;
> if (tca_get_fill(skb, actions, portid, n->nlmsg_seq, 0, event,
>- 0, 0) <= 0) {
>+ 0, 1) <= 0) {
> NL_SET_ERR_MSG(extack, "Failed to fill netlink attributes while adding TC action");
> kfree_skb(skb);
> return -EINVAL;
>@@ -1125,7 +1125,7 @@ tcf_del_notify(struct net *net, struct nlmsghdr *n, struct list_head *actions,
> return -ENOBUFS;
>
> if (tca_get_fill(skb, actions, portid, n->nlmsg_seq, 0, RTM_DELACTION,
>- 0, 1) <= 0) {
>+ 0, 2) <= 0) {
So now you are adjusting dump because of a change in a different patch
right? This also breaks bisect.
^ permalink raw reply
* Re: linux-next: BUG: KASAN: use-after-free in tun_chr_close
From: Andrei Vagin @ 2018-05-16 7:12 UTC (permalink / raw)
To: netdev, Jason Wang
In-Reply-To: <20180516062825.GA11416@outlook.office365.com>
[-- Attachment #1: Type: text/plain, Size: 5091 bytes --]
Hi Jason,
I think the problem is in "tun: hold a tun socket during ptr_ring_cleanup".
Pls take a look at the attached patch.
On Tue, May 15, 2018 at 11:28:25PM -0700, Andrei Vagin wrote:
> We run CRIU tests on linux-next regularly and today we caught this bug:
>
> https://travis-ci.org/avagin/linux/jobs/379450631
>
> [ 50.264837] ==================================================================
> [ 50.264986] BUG: KASAN: use-after-free in __lock_acquire.isra.30+0x1ad4/0x1bb0
> [ 50.265088] Read of size 8 at addr ffff88018e1728f8 by task criu/1819
> [ 50.265167]
> [ 50.265249] CPU: 0 PID: 1819 Comm: criu Not tainted 4.17.0-rc5-next-20180515+ #1
> [ 50.265251] Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
> [ 50.265252] Call Trace:
> [ 50.265262] dump_stack+0x71/0xab
> [ 50.265265] ? __lock_acquire.isra.30+0x1ad4/0x1bb0
> [ 50.265271] print_address_description+0x6a/0x270
> [ 50.265273] ? __lock_acquire.isra.30+0x1ad4/0x1bb0
> [ 50.265275] kasan_report+0x237/0x360
> [ 50.265278] __lock_acquire.isra.30+0x1ad4/0x1bb0
> [ 50.265285] ? register_netdev+0x30/0x30
> [ 50.265288] lock_acquire+0x10b/0x2a0
> [ 50.265294] ? tun_chr_close+0x1d7/0x4c0
> [ 50.265298] ? kfree+0xd6/0x1f0
> [ 50.265303] _raw_spin_lock+0x25/0x30
> [ 50.265306] ? tun_chr_close+0x1d7/0x4c0
> [ 50.265308] tun_chr_close+0x1d7/0x4c0
> [ 50.265313] ? fcntl_setlk+0xaf0/0xaf0
> [ 50.265320] __fput+0x251/0x770
> [ 50.265324] task_work_run+0x10e/0x180
> [ 50.265330] exit_to_usermode_loop+0xcb/0xf0
> [ 50.265332] do_syscall_64+0x21d/0x280
> [ 50.265335] ? prepare_exit_to_usermode+0x88/0x130
> [ 50.265338] entry_SYSCALL_64_after_hwframe+0x44/0xa9
> [ 50.265342] RIP: 0033:0x1494fa6f93f0
> [ 50.265342] Code: 73 01 c3 48 8b 0d b8 9b 20 00 f7 d8 64 89 01 48 83 c8 ff c3 66 0f 1f 44 00 00 83 3d 59 e0 20 00 00 75 10 b8 03 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 31 c3 48 83 ec 08 e8 0e fa ff ff 48 89 04 24
> [ 50.265388] RSP: 002b:00007ffd229fe7f8 EFLAGS: 00000246 ORIG_RAX: 0000000000000003
> [ 50.265391] RAX: 0000000000000000 RBX: 0000000000000004 RCX: 00001494fa6f93f0
> [ 50.265393] RDX: 00007ffd229fe80c RSI: 00000000400454da RDI: 0000000000000004
> [ 50.265395] RBP: 0000000000000000 R08: 000000000000420b R09: 0000000000000000
> [ 50.265396] R10: 0000000000000000 R11: 0000000000000246 R12: 00001494fab116a0
> [ 50.265398] R13: 0000000000000d06 R14: 0000000000000000 R15: 0000000000000000
> [ 50.265400]
> [ 50.265476] Allocated by task 1819:
> [ 50.265554] kasan_kmalloc+0xa0/0xd0
> [ 50.265556] __kmalloc+0x13a/0x250
> [ 50.265561] sk_prot_alloc+0xd3/0x250
> [ 50.265564] sk_alloc+0x35/0x9d0
> [ 50.265566] tun_chr_open+0x7b/0x5a0
> [ 50.265570] misc_open+0x313/0x480
> [ 50.265573] chrdev_open+0x1d6/0x4b0
> [ 50.265575] do_dentry_open+0x6ae/0xee0
> [ 50.265578] path_openat+0xce6/0x2890
> [ 50.265580] do_filp_open+0x17a/0x270
> [ 50.265582] do_sys_open+0x203/0x340
> [ 50.265584] do_syscall_64+0xa0/0x280
> [ 50.265586] entry_SYSCALL_64_after_hwframe+0x44/0xa9
> [ 50.265587]
> [ 50.265667] Freed by task 1819:
> [ 50.265745] __kasan_slab_free+0x130/0x180
> [ 50.265747] kfree+0xd6/0x1f0
> [ 50.265750] __sk_destruct+0x46f/0x580
> [ 50.265752] tun_chr_close+0x330/0x4c0
> [ 50.265754] __fput+0x251/0x770
> [ 50.265756] task_work_run+0x10e/0x180
> [ 50.265758] exit_to_usermode_loop+0xcb/0xf0
> [ 50.265760] do_syscall_64+0x21d/0x280
> [ 50.265762] entry_SYSCALL_64_after_hwframe+0x44/0xa9
> [ 50.265762]
> [ 50.265840] The buggy address belongs to the object at ffff88018e172200
> [ 50.265840] which belongs to the cache kmalloc-2048 of size 2048
> [ 50.265927] The buggy address is located 1784 bytes inside of
> [ 50.265927] 2048-byte region [ffff88018e172200, ffff88018e172a00)
> [ 50.266011] The buggy address belongs to the page:
> [ 50.266089] page:ffffea0006385c00 count:1 mapcount:0 mapping:0000000000000000 index:0x0 compound_mapcount: 0
> [ 50.266178] flags: 0x17fff8000008100(slab|head)
> [ 50.266257] raw: 017fff8000008100 0000000000000000 0000000000000000 00000001800f000f
> [ 50.266342] raw: dead000000000100 dead000000000200 ffff8801d9016800 0000000000000000
> [ 50.266425] page dumped because: kasan: bad access detected
> [ 50.266501]
> [ 50.266590] Memory state around the buggy address:
> [ 50.266693] ffff88018e172780: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
> [ 50.266776] ffff88018e172800: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
> [ 50.266860] >ffff88018e172880: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
> [ 50.266943] ^
> [ 50.267020] ffff88018e172900: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
> [ 50.267103] ffff88018e172980: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
> [ 50.267192] ==================================================================
> [ 50.267275] Disabling lock debugging due to kernel taint
[-- Attachment #2: 0001-tun-hold-a-tun-socket-during-ptr_ring_cleanup.patch --]
[-- Type: text/plain, Size: 1867 bytes --]
>From 7efa9d087be20a67c5c3953f7bf26ae5bafaa061 Mon Sep 17 00:00:00 2001
From: Andrei Vagin <avagin@openvz.org>
Date: Wed, 16 May 2018 00:03:39 -0700
Subject: [PATCH] tun: hold a tun socket during ptr_ring_cleanup
Otherwise a socket will be destroyed together with a tun_file structure,
which is used in ptr_ring_cleanup.
This issue was reported by kasan:
BUG: KASAN: use-after-free in __lock_acquire.isra.30+0x1ad4/0x1bb0
Read of size 8 at addr ffff88018e1728f8 by task criu/1819
CPU: 0 PID: 1819 Comm: criu Not tainted 4.17.0-rc5-next-20180515+ #1
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
Call Trace:
dump_stack+0x71/0xab
print_address_description+0x6a/0x270
kasan_report+0x237/0x360
__lock_acquire.isra.30+0x1ad4/0x1bb0
lock_acquire+0x10b/0x2a0
_raw_spin_lock+0x25/0x30
tun_chr_close+0x1d7/0x4c0
__fput+0x251/0x770
task_work_run+0x10e/0x180
exit_to_usermode_loop+0xcb/0xf0
do_syscall_64+0x21d/0x280
? prepare_exit_to_usermode+0x88/0x130
entry_SYSCALL_64_after_hwframe+0x44/0xa9
Freed by task 1819:
__kasan_slab_free+0x130/0x180
kfree+0xd6/0x1f0
__sk_destruct+0x46f/0x580
tun_chr_close+0x330/0x4c0
__fput+0x251/0x770
task_work_run+0x10e/0x180
exit_to_usermode_loop+0xcb/0xf0
do_syscall_64+0x21d/0x280
entry_SYSCALL_64_after_hwframe+0x44/0xa9
Signed-off-by: Andrei Vagin <avagin@openvz.org>
---
drivers/net/tun.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index 8b0f0a0baab4..f3eae203cc58 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -3246,8 +3246,10 @@ static int tun_chr_close(struct inode *inode, struct file *file)
{
struct tun_file *tfile = file->private_data;
+ sock_hold(&tfile->sk);
tun_detach(tfile, true);
ptr_ring_cleanup(&tfile->tx_ring, tun_ptr_free);
+ sock_put(&tfile->sk);
return 0;
}
--
2.17.0
^ permalink raw reply related
* Re: [PATCH 00/14] Modify action API for implementing lockless actions
From: Vlad Buslov @ 2018-05-16 7:13 UTC (permalink / raw)
To: Lucas Bates
Cc: Jamal Hadi Salim, Linux Kernel Network Developers, David Miller,
Cong Wang, Jiri Pirko, pablo, kadlec, fw, ast, Daniel Borkmann,
edumazet, kliteyn
In-Reply-To: <CAMDBHYLg0sU4AWhr0YCE5R4RA=jhJ3cPciujLYOyKHGx8Rpcjg@mail.gmail.com>
On Tue 15 May 2018 at 22:07, Lucas Bates <lucasb@mojatatu.com> wrote:
> On Tue, May 15, 2018 at 6:03 PM, Lucas Bates <lucasb@mojatatu.com> wrote:
>> On Tue, May 15, 2018 at 5:49 PM, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>>>> Test 7d50: Add skbmod action to set destination mac
>>>> exit: 255 0
>>>> dst MAC address <11:22:33:44:55:66>
>>>> RTNETLINK answers: No such file or directory
>>>> We have an error talking to the kernel
>>>>
>>>
>>> You may actually have broken something with your patches in this case.
>>>
>>> Lucas - does this test pass on latest net-next?
>>
>> Yes, 7d50 has been passing on our builds for at least the last month.
>
> Also, Vlad, you can look at the JSON to see the test case data, or run
> tdc.py -s | less and search for the ID to see the commands being run.
> I'm here if you need help using tdc.
Hello Lucas,
I'll look into JSON test definition and try to understand whats wrong.
^ permalink raw reply
* Re: KMSAN: uninit-value in __sctp_v6_cmp_addr
From: Xin Long @ 2018-05-16 7:17 UTC (permalink / raw)
To: syzbot
Cc: davem, LKML, linux-sctp, network dev, Neil Horman, syzkaller-bugs,
Vlad Yasevich
In-Reply-To: <0000000000004f4075056c410b96@google.com>
On Wed, May 16, 2018 at 12:25 AM, syzbot
<syzbot+85490c30c260afff22f2@syzkaller.appspotmail.com> wrote:
> Hello,
>
> syzbot found the following crash on:
>
> HEAD commit: 74ee2200b89f kmsan: bump .config.example to v4.17-rc3
> git tree: https://github.com/google/kmsan.git/master
> console output: https://syzkaller.appspot.com/x/log.txt?x=169efb5b800000
> kernel config: https://syzkaller.appspot.com/x/.config?x=4ca1e57bafa8ab1f
> dashboard link: https://syzkaller.appspot.com/bug?extid=85490c30c260afff22f2
> compiler: clang version 7.0.0 (trunk 329391)
> syzkaller repro:https://syzkaller.appspot.com/x/repro.syz?x=157e9237800000
> C reproducer: https://syzkaller.appspot.com/x/repro.c?x=10fe5de7800000
>
> IMPORTANT: if you fix the bug, please add the following tag to the commit:
> Reported-by: syzbot+85490c30c260afff22f2@syzkaller.appspotmail.com
>
> random: sshd: uninitialized urandom read (32 bytes read)
> random: sshd: uninitialized urandom read (32 bytes read)
> random: sshd: uninitialized urandom read (32 bytes read)
> random: sshd: uninitialized urandom read (32 bytes read)
> ==================================================================
> BUG: KMSAN: uninit-value in __sctp_v6_cmp_addr+0x49a/0x850
> net/sctp/ipv6.c:580
> CPU: 0 PID: 4453 Comm: syz-executor325 Not tainted 4.17.0-rc3+ #88
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
> Google 01/01/2011
> Call Trace:
> <IRQ>
> __dump_stack lib/dump_stack.c:77 [inline]
> dump_stack+0x185/0x1d0 lib/dump_stack.c:113
> kmsan_report+0x142/0x240 mm/kmsan/kmsan.c:1067
> __msan_warning_32+0x6c/0xb0 mm/kmsan/kmsan_instr.c:683
> __sctp_v6_cmp_addr+0x49a/0x850 net/sctp/ipv6.c:580
Pls check if the testing kernel has this commit:
commit d625329b06e46bd20baf9ee40847d11982569204
Author: Xin Long <lucien.xin@gmail.com>
Date: Thu Apr 26 14:13:57 2018 +0800
sctp: handle two v4 addrs comparison in sctp_inet6_cmp_addr
Thanks.
> sctp_inet6_cmp_addr+0x3dc/0x400 net/sctp/ipv6.c:898
> sctp_bind_addr_match+0x18b/0x2f0 net/sctp/bind_addr.c:330
> sctp_addrs_lookup_transport+0x904/0xa20 net/sctp/input.c:942
> __sctp_lookup_association net/sctp/input.c:985 [inline]
> __sctp_rcv_lookup net/sctp/input.c:1249 [inline]
> sctp_rcv+0x15e6/0x4d30 net/sctp/input.c:170
> ip_local_deliver_finish+0x874/0xec0 net/ipv4/ip_input.c:215
> NF_HOOK include/linux/netfilter.h:288 [inline]
> ip_local_deliver+0x43c/0x4e0 net/ipv4/ip_input.c:256
> dst_input include/net/dst.h:450 [inline]
> ip_rcv_finish+0xa36/0x1d00 net/ipv4/ip_input.c:396
> NF_HOOK include/linux/netfilter.h:288 [inline]
> ip_rcv+0x118f/0x16d0 net/ipv4/ip_input.c:492
> __netif_receive_skb_core+0x47df/0x4a90 net/core/dev.c:4592
> __netif_receive_skb net/core/dev.c:4657 [inline]
> process_backlog+0x62d/0xe20 net/core/dev.c:5337
> napi_poll net/core/dev.c:5735 [inline]
> net_rx_action+0x7c1/0x1a70 net/core/dev.c:5801
> __do_softirq+0x56d/0x93d kernel/softirq.c:285
> do_softirq_own_stack+0x2a/0x40 arch/x86/entry/entry_64.S:1046
> </IRQ>
> do_softirq kernel/softirq.c:329 [inline]
> __local_bh_enable_ip+0x114/0x140 kernel/softirq.c:182
> local_bh_enable+0x36/0x40 include/linux/bottom_half.h:32
> rcu_read_unlock_bh include/linux/rcupdate.h:728 [inline]
> ip_finish_output2+0x135a/0x1470 net/ipv4/ip_output.c:231
> ip_finish_output+0xcb2/0xff0 net/ipv4/ip_output.c:317
> NF_HOOK_COND include/linux/netfilter.h:277 [inline]
> ip_output+0x505/0x5d0 net/ipv4/ip_output.c:405
> dst_output include/net/dst.h:444 [inline]
> ip_local_out net/ipv4/ip_output.c:124 [inline]
> ip_queue_xmit+0x1a1e/0x1d10 net/ipv4/ip_output.c:504
> sctp_v4_xmit+0x188/0x210 net/sctp/protocol.c:983
> sctp_packet_transmit+0x3eaa/0x4350 net/sctp/output.c:650
> sctp_outq_flush+0x1a7a/0x6320 net/sctp/outqueue.c:1197
> sctp_outq_uncork+0xd2/0xf0 net/sctp/outqueue.c:776
> sctp_cmd_interpreter net/sctp/sm_sideeffect.c:1820 [inline]
> sctp_side_effects net/sctp/sm_sideeffect.c:1220 [inline]
> sctp_do_sm+0x8707/0x8d20 net/sctp/sm_sideeffect.c:1191
> sctp_primitive_REQUESTHEARTBEAT+0x175/0x1a0 net/sctp/primitive.c:200
> sctp_apply_peer_addr_params+0x207/0x1670 net/sctp/socket.c:2487
> sctp_setsockopt_peer_addr_params net/sctp/socket.c:2683 [inline]
> sctp_setsockopt+0x10e5f/0x11600 net/sctp/socket.c:4258
> sock_common_setsockopt+0x136/0x170 net/core/sock.c:3039
> __sys_setsockopt+0x4af/0x560 net/socket.c:1903
> __do_sys_setsockopt net/socket.c:1914 [inline]
> __se_sys_setsockopt net/socket.c:1911 [inline]
> __x64_sys_setsockopt+0x15c/0x1c0 net/socket.c:1911
> do_syscall_64+0x154/0x220 arch/x86/entry/common.c:287
> entry_SYSCALL_64_after_hwframe+0x44/0xa9
> RIP: 0033:0x43fef9
> RSP: 002b:00007ffc00d9bfd8 EFLAGS: 00000207 ORIG_RAX: 0000000000000036
> RAX: ffffffffffffffda RBX: 00000000004002c8 RCX: 000000000043fef9
> RDX: 0000000000000009 RSI: 0000000000000084 RDI: 0000000000000003
> RBP: 00000000006ca018 R08: 0000000000000098 R09: 000000000000001c
> R10: 0000000020000180 R11: 0000000000000207 R12: 0000000000401820
> R13: 00000000004018b0 R14: 0000000000000000 R15: 0000000000000000
>
> Local variable description: ----dest@sctp_rcv
> Variable was created at:
> sctp_rcv+0x13d/0x4d30 net/sctp/input.c:97
> ip_local_deliver_finish+0x874/0xec0 net/ipv4/ip_input.c:215
> ==================================================================
>
>
> ---
> This bug is generated by a bot. It may contain errors.
> See https://goo.gl/tpsmEJ for more information about syzbot.
> syzbot engineers can be reached at syzkaller@googlegroups.com.
>
> syzbot will keep track of this bug report. See:
> https://goo.gl/tpsmEJ#bug-status-tracking for how to communicate with
> syzbot.
> syzbot can test patches for this bug, for details see:
> https://goo.gl/tpsmEJ#testing-patches
^ permalink raw reply
* Re: xdp and fragments with virtio
From: Jason Wang @ 2018-05-16 7:24 UTC (permalink / raw)
To: David Ahern, netdev@vger.kernel.org
In-Reply-To: <e35c149e-cb15-bb8c-2e03-d1641d21d694@gmail.com>
On 2018年05月16日 11:51, David Ahern wrote:
> Hi Jason:
>
> I am trying to test MTU changes to the BPF fib_lookup helper and seeing
> something odd. Hoping you can help.
>
> I have a VM with multiple virtio based NICs and tap backends. I install
> the xdp program on eth1 and eth2 to do forwarding. In the host I send a
> large packet to eth1:
>
> $ ping -s 1500 9.9.9.9
>
>
> The tap device in the host sees 2 packets:
>
> $ sudo tcpdump -nv -i vm02-eth1
> 20:44:33.943160 IP (tos 0x0, ttl 64, id 58746, offset 0, flags [+],
> proto ICMP (1), length 1500)
> 10.100.1.254 > 9.9.9.9: ICMP echo request, id 17917, seq 1, length 1480
> 20:44:33.943172 IP (tos 0x0, ttl 64, id 58746, offset 1480, flags
> [none], proto ICMP (1), length 48)
> 10.100.1.254 > 9.9.9.9: ip-proto-1
>
>
> In the VM, the XDP program only sees the first packet, not the fragment.
> I added a printk to the program (see diff below):
>
> $ cat trace_pipe
> <idle>-0 [003] ..s2 254.436467: 0: packet length 1514
>
>
> Anything come to mind in the virtio xdp implementation that affects
> fragment packets? I see this with both IPv4 and v6.
Not yet. But we do turn of tap gso when virtio has XDP set, but it
shouldn't matter this case.
Will try to see what's wrong.
Thanks
>
> Thanks,
> David
>
> [1] xdp program diff showing printk that dumps packet length:
>
> diff --git a/samples/bpf/xdp_fwd_kern.c b/samples/bpf/xdp_fwd_kern.c
> index 4a6be0f87505..f119b506e782 100644
> --- a/samples/bpf/xdp_fwd_kern.c
> +++ b/samples/bpf/xdp_fwd_kern.c
> @@ -52,6 +52,11 @@ static __always_inline int xdp_fwd_flags(struct
> xdp_md *ctx, u32 flags)
> u16 h_proto;
> u64 nh_off;
>
> + {
> + char fmt[] = "packet length %u\n";
> +
> + bpf_trace_printk(fmt, sizeof(fmt), ctx->data_end-ctx->data);
> + }
> nh_off = sizeof(*eth);
> if (data + nh_off > data_end)
> return XDP_DROP;
>
^ permalink raw reply
* Re: linux-next: BUG: KASAN: use-after-free in tun_chr_close
From: Jason Wang @ 2018-05-16 7:32 UTC (permalink / raw)
To: Andrei Vagin, netdev
In-Reply-To: <20180516071224.GB11416@outlook.office365.com>
[-- Attachment #1: Type: text/plain, Size: 363 bytes --]
On 2018年05月16日 15:12, Andrei Vagin wrote:
> Hi Jason,
>
> I think the problem is in "tun: hold a tun socket during ptr_ring_cleanup".
>
> Pls take a look at the attached patch.
Yes.
It looks to me it's not necessary to take extra refcnt during release,
we can just do the cleanup at __tun_detach().
Could you help to test the attached patch?
Thanks
[-- Attachment #2: 0001-tuntap-fix-use-after-free-during-release.patch --]
[-- Type: text/x-patch, Size: 1362 bytes --]
>From 4b5ad75208e379dcb32abb9ac4790a0446f8558b Mon Sep 17 00:00:00 2001
From: Jason Wang <jasowang@redhat.com>
Date: Wed, 16 May 2018 15:26:52 +0800
Subject: [PATCH] tuntap: fix use after free during release
After commit b196d88aba8a ("tun: fix use after free for ptr_ring") we
need clean up tx ring during release(). But unfortunately, it tries to
do the cleanup after socket were destroyed which will lead another
use-after-free. Fix this by doing the cleanup before dropping the last
reference of the socket in __tun_detach().
Reported-by: Andrei Vagin <avagin@virtuozzo.com>
Fixes: b196d88aba8a ("tun: fix use after free for ptr_ring")
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
drivers/net/tun.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index 9fbbb32..d45ac37 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -729,6 +729,7 @@ static void __tun_detach(struct tun_file *tfile, bool clean)
}
if (tun)
xdp_rxq_info_unreg(&tfile->xdp_rxq);
+ ptr_ring_cleanup(&tfile->tx_ring, tun_ptr_free);
sock_put(&tfile->sk);
}
}
@@ -3245,7 +3246,6 @@ static int tun_chr_close(struct inode *inode, struct file *file)
struct tun_file *tfile = file->private_data;
tun_detach(tfile, true);
- ptr_ring_cleanup(&tfile->tx_ring, tun_ptr_free);
return 0;
}
--
2.7.4
^ permalink raw reply related
* Re: linux-next: BUG: KASAN: use-after-free in tun_chr_close
From: Andrei Vagin @ 2018-05-16 7:40 UTC (permalink / raw)
To: Jason Wang; +Cc: netdev
In-Reply-To: <9a7440ca-05dd-7ff6-0fa0-a96afc9ed780@redhat.com>
On Wed, May 16, 2018 at 03:32:59PM +0800, Jason Wang wrote:
>
>
> On 2018年05月16日 15:12, Andrei Vagin wrote:
> > Hi Jason,
> >
> > I think the problem is in "tun: hold a tun socket during ptr_ring_cleanup".
> >
> > Pls take a look at the attached patch.
>
> Yes.
>
> It looks to me it's not necessary to take extra refcnt during release, we
> can just do the cleanup at __tun_detach().
>
> Could you help to test the attached patch?
I've run my test on the kernel with this patch. It fixes the problem.
The patch looks correct for me.
Acked-by: Andrei Vagin <avagin@virtuozzo.com>
>
> Thanks
>
> From 4b5ad75208e379dcb32abb9ac4790a0446f8558b Mon Sep 17 00:00:00 2001
> From: Jason Wang <jasowang@redhat.com>
> Date: Wed, 16 May 2018 15:26:52 +0800
> Subject: [PATCH] tuntap: fix use after free during release
>
> After commit b196d88aba8a ("tun: fix use after free for ptr_ring") we
> need clean up tx ring during release(). But unfortunately, it tries to
> do the cleanup after socket were destroyed which will lead another
> use-after-free. Fix this by doing the cleanup before dropping the last
> reference of the socket in __tun_detach().
>
> Reported-by: Andrei Vagin <avagin@virtuozzo.com>
> Fixes: b196d88aba8a ("tun: fix use after free for ptr_ring")
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
> drivers/net/tun.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index 9fbbb32..d45ac37 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c
> @@ -729,6 +729,7 @@ static void __tun_detach(struct tun_file *tfile, bool clean)
> }
> if (tun)
> xdp_rxq_info_unreg(&tfile->xdp_rxq);
> + ptr_ring_cleanup(&tfile->tx_ring, tun_ptr_free);
> sock_put(&tfile->sk);
> }
> }
> @@ -3245,7 +3246,6 @@ static int tun_chr_close(struct inode *inode, struct file *file)
> struct tun_file *tfile = file->private_data;
>
> tun_detach(tfile, true);
> - ptr_ring_cleanup(&tfile->tx_ring, tun_ptr_free);
>
> return 0;
> }
> --
> 2.7.4
>
^ permalink raw reply
* Re: [PATCH 09/14] net: sched: don't release reference on action overwrite
From: Jiri Pirko @ 2018-05-16 7:43 UTC (permalink / raw)
To: Vlad Buslov
Cc: netdev, davem, jhs, xiyou.wangcong, pablo, kadlec, fw, ast,
daniel, edumazet, keescook, linux-kernel, netfilter-devel,
coreteam, kliteyn
In-Reply-To: <1526308035-12484-10-git-send-email-vladbu@mellanox.com>
Mon, May 14, 2018 at 04:27:10PM CEST, vladbu@mellanox.com wrote:
>Return from action init function with reference to action taken,
>even when overwriting existing action.
>
>Action init API initializes its fourth argument (pointer to pointer to
>tc action) to either existing action with same index or newly created
>action. In case of existing index(and bind argument is zero), init
>function returns without incrementing action reference counter. Caller
>of action init then proceeds working with action without actually
>holding reference to it. This means that action could be deleted
>concurrently. To prevent such scenario this patch changes action init
Be imperative to the codebase in the patch description.
>behavior to always take reference to action before returning
>successfully.
Where's the balance? Who does the release instead? I'm probably missing
something.
>
>Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
>---
> net/sched/act_bpf.c | 8 ++++----
> net/sched/act_connmark.c | 5 +++--
> net/sched/act_csum.c | 8 ++++----
> net/sched/act_gact.c | 5 +++--
> net/sched/act_ife.c | 12 +++++-------
> net/sched/act_ipt.c | 5 +++--
> net/sched/act_mirred.c | 5 ++---
> net/sched/act_nat.c | 5 +++--
> net/sched/act_pedit.c | 5 +++--
> net/sched/act_police.c | 8 +++-----
> net/sched/act_sample.c | 8 +++-----
> net/sched/act_simple.c | 5 +++--
> net/sched/act_skbedit.c | 5 +++--
> net/sched/act_skbmod.c | 8 +++-----
> net/sched/act_tunnel_key.c | 8 +++-----
> net/sched/act_vlan.c | 8 +++-----
> 16 files changed, 51 insertions(+), 57 deletions(-)
>
>diff --git a/net/sched/act_bpf.c b/net/sched/act_bpf.c
>index 5d95c43..5554bf7 100644
>--- a/net/sched/act_bpf.c
>+++ b/net/sched/act_bpf.c
>@@ -311,9 +311,10 @@ static int tcf_bpf_init(struct net *net, struct nlattr *nla,
> if (bind)
> return 0;
>
>- tcf_idr_release(*act, bind);
>- if (!replace)
>+ if (!replace) {
>+ tcf_idr_release(*act, bind);
> return -EEXIST;
>+ }
> }
>
> is_bpf = tb[TCA_ACT_BPF_OPS_LEN] && tb[TCA_ACT_BPF_OPS];
[...]
^ permalink raw reply
* Re: [PATCH 09/14] net: sched: don't release reference on action overwrite
From: Vlad Buslov @ 2018-05-16 7:47 UTC (permalink / raw)
To: Jiri Pirko
Cc: netdev, davem, jhs, xiyou.wangcong, pablo, kadlec, fw, ast,
daniel, edumazet, keescook, linux-kernel, netfilter-devel,
coreteam, kliteyn
In-Reply-To: <20180516074332.GB1972@nanopsycho>
On Wed 16 May 2018 at 07:43, Jiri Pirko <jiri@resnulli.us> wrote:
> Mon, May 14, 2018 at 04:27:10PM CEST, vladbu@mellanox.com wrote:
>>Return from action init function with reference to action taken,
>>even when overwriting existing action.
>>
>>Action init API initializes its fourth argument (pointer to pointer to
>>tc action) to either existing action with same index or newly created
>>action. In case of existing index(and bind argument is zero), init
>>function returns without incrementing action reference counter. Caller
>>of action init then proceeds working with action without actually
>>holding reference to it. This means that action could be deleted
>>concurrently. To prevent such scenario this patch changes action init
>
> Be imperative to the codebase in the patch description.
>
>
>>behavior to always take reference to action before returning
>>successfully.
>
> Where's the balance? Who does the release instead? I'm probably missing
> something.
I've resplit these patches for V2 to always do take/release in same
patch.
>
>>
>>Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
>>---
>> net/sched/act_bpf.c | 8 ++++----
>> net/sched/act_connmark.c | 5 +++--
>> net/sched/act_csum.c | 8 ++++----
>> net/sched/act_gact.c | 5 +++--
>> net/sched/act_ife.c | 12 +++++-------
>> net/sched/act_ipt.c | 5 +++--
>> net/sched/act_mirred.c | 5 ++---
>> net/sched/act_nat.c | 5 +++--
>> net/sched/act_pedit.c | 5 +++--
>> net/sched/act_police.c | 8 +++-----
>> net/sched/act_sample.c | 8 +++-----
>> net/sched/act_simple.c | 5 +++--
>> net/sched/act_skbedit.c | 5 +++--
>> net/sched/act_skbmod.c | 8 +++-----
>> net/sched/act_tunnel_key.c | 8 +++-----
>> net/sched/act_vlan.c | 8 +++-----
>> 16 files changed, 51 insertions(+), 57 deletions(-)
>>
>>diff --git a/net/sched/act_bpf.c b/net/sched/act_bpf.c
>>index 5d95c43..5554bf7 100644
>>--- a/net/sched/act_bpf.c
>>+++ b/net/sched/act_bpf.c
>>@@ -311,9 +311,10 @@ static int tcf_bpf_init(struct net *net, struct nlattr *nla,
>> if (bind)
>> return 0;
>>
>>- tcf_idr_release(*act, bind);
>>- if (!replace)
>>+ if (!replace) {
>>+ tcf_idr_release(*act, bind);
>> return -EEXIST;
>>+ }
>> }
>>
>> is_bpf = tb[TCA_ACT_BPF_OPS_LEN] && tb[TCA_ACT_BPF_OPS];
>
> [...]
^ permalink raw reply
* RE: i40e - Is i40e_force_link_state doing the right thing ?
From: Stachura, Mariusz @ 2018-05-16 7:49 UTC (permalink / raw)
To: Chaitanya Lala
Cc: netdev@vger.kernel.org, Williams, Mitch A, Bowers, AndrewX,
Kirsher, Jeffrey T
In-Reply-To: <CABOGejK=HjghOat91xYkXbGAaKMtb_ECKcZRhD-=8Bet1AA=ew@mail.gmail.com>
> Hi Mariusz, ...
>
> On Tue, May 15, 2018 at 2:24 PM, Stachura, Mariusz <mariusz.stachura@intel.com> wrote:
>> On Tue, May 15, 2018 at 1:15 PM, Chaitanya Lala <chaitanya.lala@gmail.com> wrote:
>>> Hi,
>>>
>>> I am trying to bring up a Intel XL710 4x10G Intel card using the
>>> latest mainline top-of-tree.
>>> The problem is that "ifconfig up" and "ifconfig down" do not take
>>> effect at the link state level.
>>> I tracked the problem down to i40e_force_link_state() when it is
>>> called from i40e_down().
>>> It calls i40e_force_link_state with "is_up" == false. In-turn it
>>> calls, i40e_aq_set_link_restart_an(hw, true, NULL).
>>>
>>> Should the second argument of i40e_aq_set_link_restart_an be "is_up"
>>> vs the current "true"
>>> i.e. i40e_aq_set_link_restart_an(hw, is_up, NULL). ? When I make this
>>> change, the link state syncs-up with the interface administrative
>>> state.
>>>
>>> Is this a bug ?
>>>
>>> Thanks,
>>> Chaitanya
>>
>> Hello Chaitanya,
>>
>> i40e_down() calls i40e_force_link_state with "is_up" == false only if interface's private flag "link-down-on-close" is set. By default the link is left up for manageability and VF traffic, user can use this flag to power down the interface on the link level. Does that work for you?
>> The command is:
>> "ethtool --set-priv-flags IFNAME link-down-on-close on" and then
>
> This flag is _on_ in my setup and hencet i40e_force_link_state is being called with is_up == false in my setup. The problem is that irrespective of value of "is_up" flag, i40e_force_link_state invokes i40e_aq_set_link_restart_an with second argument (enable_link) as "true". So i40e_aq_set_link_restart_an is always trying to enable link even if is_up was false. Is that correct behavior ?
>
> I have pasted code with my annotations below marked with "//XXX".
> (...)
> Thanks,
> Chaitanya
Hey,
i40e_aq_set_link_restart_an has second argument set to "true" intentionally, as I understand the "link-down-on-close" does not work for you, right? I will double check if this feature works for me and get back to you, thank you again.
--------------------------------------------------------------------
Intel Technology Poland sp. z o.o.
ul. Slowackiego 173 | 80-298 Gdansk | Sad Rejonowy Gdansk Polnoc | VII Wydzial Gospodarczy Krajowego Rejestru Sadowego - KRS 101882 | NIP 957-07-52-316 | Kapital zakladowy 200.000 PLN.
Ta wiadomosc wraz z zalacznikami jest przeznaczona dla okreslonego adresata i moze zawierac informacje poufne. W razie przypadkowego otrzymania tej wiadomosci, prosimy o powiadomienie nadawcy oraz trwale jej usuniecie; jakiekolwiek
przegladanie lub rozpowszechnianie jest zabronione.
This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). If you are not the intended recipient, please contact the sender and delete all copies; any review or distribution by
others is strictly prohibited.
^ permalink raw reply
* Re: [PATCH 10/14] net: sched: extend act API for lockless actions
From: Jiri Pirko @ 2018-05-16 7:50 UTC (permalink / raw)
To: Vlad Buslov
Cc: netdev, davem, jhs, xiyou.wangcong, pablo, kadlec, fw, ast,
daniel, edumazet, keescook, linux-kernel, netfilter-devel,
coreteam, kliteyn
In-Reply-To: <1526308035-12484-11-git-send-email-vladbu@mellanox.com>
Mon, May 14, 2018 at 04:27:11PM CEST, vladbu@mellanox.com wrote:
>Implement new action API function to atomically delete action with
>specified index and to atomically insert unique action. These functions are
>required to implement init and delete functions for specific actions that
>do not rely on rtnl lock.
>
>Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
>---
> include/net/act_api.h | 2 ++
> net/sched/act_api.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 47 insertions(+)
>
>diff --git a/include/net/act_api.h b/include/net/act_api.h
>index a8c8570..bce0cf1 100644
>--- a/include/net/act_api.h
>+++ b/include/net/act_api.h
>@@ -153,7 +153,9 @@ int tcf_idr_create(struct tc_action_net *tn, u32 index, struct nlattr *est,
> struct tc_action **a, const struct tc_action_ops *ops,
> int bind, bool cpustats);
> void tcf_idr_insert(struct tc_action_net *tn, struct tc_action *a);
>+void tcf_idr_insert_unique(struct tc_action_net *tn, struct tc_action *a);
>
>+int tcf_idr_find_delete(struct tc_action_net *tn, u32 index);
> int __tcf_idr_release(struct tc_action *a, bool bind, bool strict);
>
> static inline int tcf_idr_release(struct tc_action *a, bool bind)
>diff --git a/net/sched/act_api.c b/net/sched/act_api.c
>index 2772276e..a5193dc 100644
>--- a/net/sched/act_api.c
>+++ b/net/sched/act_api.c
>@@ -330,6 +330,41 @@ bool tcf_idr_check(struct tc_action_net *tn, u32 index, struct tc_action **a,
> }
> EXPORT_SYMBOL(tcf_idr_check);
>
>+int tcf_idr_find_delete(struct tc_action_net *tn, u32 index)
>+{
>+ struct tcf_idrinfo *idrinfo = tn->idrinfo;
>+ struct tc_action *p;
>+ int ret = 0;
>+
>+ spin_lock_bh(&idrinfo->lock);
Why "_bh" is needed here?
>+ p = idr_find(&idrinfo->action_idr, index);
>+ if (!p) {
>+ spin_unlock(&idrinfo->lock);
>+ return -ENOENT;
>+ }
>+
>+ if (!atomic_read(&p->tcfa_bindcnt)) {
>+ if (refcount_dec_and_test(&p->tcfa_refcnt)) {
>+ struct module *owner = p->ops->owner;
>+
>+ WARN_ON(p != idr_remove(&idrinfo->action_idr,
>+ p->tcfa_index));
>+ spin_unlock_bh(&idrinfo->lock);
>+
>+ tcf_action_cleanup(p);
>+ module_put(owner);
>+ return 0;
>+ }
>+ ret = 0;
>+ } else {
>+ ret = -EPERM;
I wonder if "-EPERM" is the best error code for this...
>+ }
>+
>+ spin_unlock_bh(&idrinfo->lock);
>+ return ret;
>+}
>+EXPORT_SYMBOL(tcf_idr_find_delete);
>+
> int tcf_idr_create(struct tc_action_net *tn, u32 index, struct nlattr *est,
> struct tc_action **a, const struct tc_action_ops *ops,
> int bind, bool cpustats)
>@@ -407,6 +442,16 @@ void tcf_idr_insert(struct tc_action_net *tn, struct tc_action *a)
> }
> EXPORT_SYMBOL(tcf_idr_insert);
>
>+void tcf_idr_insert_unique(struct tc_action_net *tn, struct tc_action *a)
>+{
>+ struct tcf_idrinfo *idrinfo = tn->idrinfo;
>+
>+ spin_lock_bh(&idrinfo->lock);
>+ WARN_ON(idr_replace(&idrinfo->action_idr, a, a->tcfa_index));
Under which condition this WARN_ON is hit?
>+ spin_unlock_bh(&idrinfo->lock);
>+}
>+EXPORT_SYMBOL(tcf_idr_insert_unique);
>+
> void tcf_idrinfo_destroy(const struct tc_action_ops *ops,
> struct tcf_idrinfo *idrinfo)
> {
>--
>2.7.5
>
^ permalink raw reply
* Re: [PATCH 09/14] net: sched: don't release reference on action overwrite
From: Jiri Pirko @ 2018-05-16 7:50 UTC (permalink / raw)
To: Vlad Buslov
Cc: netdev, davem, jhs, xiyou.wangcong, pablo, kadlec, fw, ast,
daniel, edumazet, keescook, linux-kernel, netfilter-devel,
coreteam, kliteyn
In-Reply-To: <vbfd0xw11pn.fsf@reg-r-vrt-018-180.mtr.labs.mlnx>
Wed, May 16, 2018 at 09:47:32AM CEST, vladbu@mellanox.com wrote:
>
>On Wed 16 May 2018 at 07:43, Jiri Pirko <jiri@resnulli.us> wrote:
>> Mon, May 14, 2018 at 04:27:10PM CEST, vladbu@mellanox.com wrote:
>>>Return from action init function with reference to action taken,
>>>even when overwriting existing action.
>>>
>>>Action init API initializes its fourth argument (pointer to pointer to
>>>tc action) to either existing action with same index or newly created
>>>action. In case of existing index(and bind argument is zero), init
>>>function returns without incrementing action reference counter. Caller
>>>of action init then proceeds working with action without actually
>>>holding reference to it. This means that action could be deleted
>>>concurrently. To prevent such scenario this patch changes action init
>>
>> Be imperative to the codebase in the patch description.
>>
>>
>>>behavior to always take reference to action before returning
>>>successfully.
>>
>> Where's the balance? Who does the release instead? I'm probably missing
>> something.
>
>I've resplit these patches for V2 to always do take/release in same
>patch.
Good. Thanks.
>
>>
>>>
>>>Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
>>>---
>>> net/sched/act_bpf.c | 8 ++++----
>>> net/sched/act_connmark.c | 5 +++--
>>> net/sched/act_csum.c | 8 ++++----
>>> net/sched/act_gact.c | 5 +++--
>>> net/sched/act_ife.c | 12 +++++-------
>>> net/sched/act_ipt.c | 5 +++--
>>> net/sched/act_mirred.c | 5 ++---
>>> net/sched/act_nat.c | 5 +++--
>>> net/sched/act_pedit.c | 5 +++--
>>> net/sched/act_police.c | 8 +++-----
>>> net/sched/act_sample.c | 8 +++-----
>>> net/sched/act_simple.c | 5 +++--
>>> net/sched/act_skbedit.c | 5 +++--
>>> net/sched/act_skbmod.c | 8 +++-----
>>> net/sched/act_tunnel_key.c | 8 +++-----
>>> net/sched/act_vlan.c | 8 +++-----
>>> 16 files changed, 51 insertions(+), 57 deletions(-)
>>>
>>>diff --git a/net/sched/act_bpf.c b/net/sched/act_bpf.c
>>>index 5d95c43..5554bf7 100644
>>>--- a/net/sched/act_bpf.c
>>>+++ b/net/sched/act_bpf.c
>>>@@ -311,9 +311,10 @@ static int tcf_bpf_init(struct net *net, struct nlattr *nla,
>>> if (bind)
>>> return 0;
>>>
>>>- tcf_idr_release(*act, bind);
>>>- if (!replace)
>>>+ if (!replace) {
>>>+ tcf_idr_release(*act, bind);
>>> return -EEXIST;
>>>+ }
>>> }
>>>
>>> is_bpf = tb[TCA_ACT_BPF_OPS_LEN] && tb[TCA_ACT_BPF_OPS];
>>
>> [...]
>
^ permalink raw reply
* Re: linux-next: BUG: KASAN: use-after-free in tun_chr_close
From: Jason Wang @ 2018-05-16 7:52 UTC (permalink / raw)
To: Andrei Vagin; +Cc: netdev
In-Reply-To: <20180516074019.GA5601@outlook.office365.com>
On 2018年05月16日 15:40, Andrei Vagin wrote:
> On Wed, May 16, 2018 at 03:32:59PM +0800, Jason Wang wrote:
>> On 2018年05月16日 15:12, Andrei Vagin wrote:
>>> Hi Jason,
>>>
>>> I think the problem is in "tun: hold a tun socket during ptr_ring_cleanup".
>>>
>>> Pls take a look at the attached patch.
>> Yes.
>>
>> It looks to me it's not necessary to take extra refcnt during release, we
>> can just do the cleanup at __tun_detach().
>>
>> Could you help to test the attached patch?
> I've run my test on the kernel with this patch. It fixes the problem.
> The patch looks correct for me.
>
> Acked-by: Andrei Vagin<avagin@virtuozzo.com>
>
Cool, thanks a lot!
Let me post a formal patch.
^ permalink raw reply
* Re: [PATCH net-next 2/2] pfifo_fast: drop unneeded additional lock on dequeue
From: Paolo Abeni @ 2018-05-16 7:56 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: netdev, David S. Miller, Jamal Hadi Salim, Cong Wang, Jiri Pirko,
John Fastabend
In-Reply-To: <20180515221013-mutt-send-email-mst@kernel.org>
On Tue, 2018-05-15 at 23:17 +0300, Michael S. Tsirkin wrote:
> On Tue, May 15, 2018 at 04:24:37PM +0200, Paolo Abeni wrote:
> > After the previous patch, for NOLOCK qdiscs, q->seqlock is
> > always held when the dequeue() is invoked, we can drop
> > any additional locking to protect such operation.
> >
> > Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> > ---
> > include/linux/skb_array.h | 5 +++++
> > net/sched/sch_generic.c | 4 ++--
> > 2 files changed, 7 insertions(+), 2 deletions(-)
>
> Is the seqlock taken during qdisc_change_tx_queue_len?
> We need to prevent that racing with dequeue.
Thanks for the head-up! I missed that code-path.
I'll add the lock in qdisc_change_tx_queue_len() in v2.
Thanks you,
Paolo
^ permalink raw reply
* INFO: rcu detected stall in sctp_packet_transmit
From: syzbot @ 2018-05-16 8:11 UTC (permalink / raw)
To: davem, linux-kernel, linux-sctp, marcelo.leitner, netdev, nhorman,
syzkaller-bugs, vyasevich
Hello,
syzbot found the following crash on:
HEAD commit: 961423f9fcbc Merge branch 'sctp-Introduce-sctp_flush_ctx'
git tree: net-next
console output: https://syzkaller.appspot.com/x/log.txt?x=1366aea7800000
kernel config: https://syzkaller.appspot.com/x/.config?x=51fb0a6913f757db
dashboard link: https://syzkaller.appspot.com/bug?extid=ff0b569fb5111dcd1a36
compiler: gcc (GCC) 8.0.1 20180413 (experimental)
Unfortunately, I don't have any reproducer for this crash yet.
IMPORTANT: if you fix the bug, please add the following tag to the commit:
Reported-by: syzbot+ff0b569fb5111dcd1a36@syzkaller.appspotmail.com
INFO: rcu_sched self-detected stall on CPU
0-....: (1 GPs behind) idle=dae/1/4611686018427387908 softirq=93090/93091
fqs=30902
(t=125000 jiffies g=51107 c=51106 q=972)
NMI backtrace for cpu 0
CPU: 0 PID: 24668 Comm: syz-executor6 Not tainted 4.17.0-rc4+ #44
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
Google 01/01/2011
Call Trace:
<IRQ>
__dump_stack lib/dump_stack.c:77 [inline]
dump_stack+0x1b9/0x294 lib/dump_stack.c:113
nmi_cpu_backtrace.cold.4+0x19/0xce lib/nmi_backtrace.c:103
nmi_trigger_cpumask_backtrace+0x151/0x192 lib/nmi_backtrace.c:62
arch_trigger_cpumask_backtrace+0x14/0x20 arch/x86/kernel/apic/hw_nmi.c:38
trigger_single_cpu_backtrace include/linux/nmi.h:156 [inline]
rcu_dump_cpu_stacks+0x175/0x1c2 kernel/rcu/tree.c:1376
print_cpu_stall kernel/rcu/tree.c:1525 [inline]
check_cpu_stall.isra.61.cold.80+0x36c/0x59a kernel/rcu/tree.c:1593
__rcu_pending kernel/rcu/tree.c:3356 [inline]
rcu_pending kernel/rcu/tree.c:3401 [inline]
rcu_check_callbacks+0x21b/0xad0 kernel/rcu/tree.c:2763
update_process_times+0x2d/0x70 kernel/time/timer.c:1636
tick_sched_handle+0x9f/0x180 kernel/time/tick-sched.c:164
tick_sched_timer+0x45/0x130 kernel/time/tick-sched.c:1274
__run_hrtimer kernel/time/hrtimer.c:1398 [inline]
__hrtimer_run_queues+0x3e3/0x10a0 kernel/time/hrtimer.c:1460
hrtimer_interrupt+0x2f3/0x750 kernel/time/hrtimer.c:1518
local_apic_timer_interrupt arch/x86/kernel/apic/apic.c:1025 [inline]
smp_apic_timer_interrupt+0x15d/0x710 arch/x86/kernel/apic/apic.c:1050
apic_timer_interrupt+0xf/0x20 arch/x86/entry/entry_64.S:863
RIP: 0010:sctp_v6_xmit+0x259/0x6b0 net/sctp/ipv6.c:219
RSP: 0018:ffff8801dae068e8 EFLAGS: 00000246 ORIG_RAX: ffffffffffffff13
RAX: 0000000000000007 RBX: ffff8801bb7ec800 RCX: ffffffff86f1b345
RDX: 0000000000000000 RSI: ffffffff86f1b381 RDI: ffff8801b73d97c4
RBP: ffff8801dae06988 R08: ffff88019505c300 R09: ffffed003b5c46c2
R10: ffffed003b5c46c2 R11: ffff8801dae23613 R12: ffff88011fd57300
R13: ffff8801bb7ecec8 R14: 0000000000000029 R15: 0000000000000002
sctp_packet_transmit+0x26f6/0x3ba0 net/sctp/output.c:642
sctp_outq_flush_transports net/sctp/outqueue.c:1164 [inline]
sctp_outq_flush+0x5f5/0x3430 net/sctp/outqueue.c:1212
sctp_outq_uncork+0x6a/0x80 net/sctp/outqueue.c:776
sctp_cmd_interpreter net/sctp/sm_sideeffect.c:1820 [inline]
sctp_side_effects net/sctp/sm_sideeffect.c:1220 [inline]
sctp_do_sm+0x596/0x7160 net/sctp/sm_sideeffect.c:1191
sctp_generate_heartbeat_event+0x218/0x450 net/sctp/sm_sideeffect.c:406
call_timer_fn+0x230/0x940 kernel/time/timer.c:1326
expire_timers kernel/time/timer.c:1363 [inline]
__run_timers+0x79e/0xc50 kernel/time/timer.c:1666
run_timer_softirq+0x4c/0x70 kernel/time/timer.c:1692
__do_softirq+0x2e0/0xaf5 kernel/softirq.c:285
invoke_softirq kernel/softirq.c:365 [inline]
irq_exit+0x1d1/0x200 kernel/softirq.c:405
exiting_irq arch/x86/include/asm/apic.h:525 [inline]
smp_apic_timer_interrupt+0x17e/0x710 arch/x86/kernel/apic/apic.c:1052
apic_timer_interrupt+0xf/0x20 arch/x86/entry/entry_64.S:863
</IRQ>
RIP: 0010:arch_local_irq_restore arch/x86/include/asm/paravirt.h:783
[inline]
RIP: 0010:__raw_spin_unlock_irqrestore include/linux/spinlock_api_smp.h:160
[inline]
RIP: 0010:_raw_spin_unlock_irqrestore+0xa1/0xc0
kernel/locking/spinlock.c:184
RSP: 0018:ffff880196227328 EFLAGS: 00000286 ORIG_RAX: ffffffffffffff13
RAX: dffffc0000000000 RBX: 0000000000000286 RCX: 0000000000000000
RDX: 1ffffffff11a316d RSI: 0000000000000001 RDI: 0000000000000286
RBP: ffff880196227338 R08: ffffed003b5c4b81 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000000 R12: ffff8801dae25c00
R13: ffff8801dae25c80 R14: ffff880196227758 R15: ffff8801dae25c00
unlock_hrtimer_base kernel/time/hrtimer.c:887 [inline]
hrtimer_start_range_ns+0x692/0xd10 kernel/time/hrtimer.c:1118
hrtimer_start_expires include/linux/hrtimer.h:412 [inline]
futex_wait_queue_me+0x304/0x820 kernel/futex.c:2517
futex_wait+0x450/0x9f0 kernel/futex.c:2645
do_futex+0x336/0x27d0 kernel/futex.c:3527
__do_sys_futex kernel/futex.c:3587 [inline]
__se_sys_futex kernel/futex.c:3555 [inline]
__x64_sys_futex+0x46a/0x680 kernel/futex.c:3555
do_syscall_64+0x1b1/0x800 arch/x86/entry/common.c:287
entry_SYSCALL_64_after_hwframe+0x49/0xbe
RIP: 0033:0x455a09
RSP: 002b:0000000000a3e938 EFLAGS: 00000246 ORIG_RAX: 00000000000000ca
RAX: ffffffffffffffda RBX: 0000000000045a9b RCX: 0000000000455a09
RDX: 0000000000000000 RSI: 0000000000000000 RDI: 000000000072becc
RBP: 000000000072becc R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000a3e940 R11: 0000000000000246 R12: 0000000000000019
R13: 0000000000000002 R14: 000000000072bea0 R15: 0000000000045a8f
---
This bug is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzkaller@googlegroups.com.
syzbot will keep track of this bug report. See:
https://goo.gl/tpsmEJ#bug-status-tracking for how to communicate with
syzbot.
^ permalink raw reply
* Re: [PATCH 10/14] net: sched: extend act API for lockless actions
From: Vlad Buslov @ 2018-05-16 8:16 UTC (permalink / raw)
To: Jiri Pirko
Cc: netdev, davem, jhs, xiyou.wangcong, pablo, kadlec, fw, ast,
daniel, edumazet, keescook, linux-kernel, netfilter-devel,
coreteam, kliteyn
In-Reply-To: <20180516075000.GC1972@nanopsycho>
On Wed 16 May 2018 at 07:50, Jiri Pirko <jiri@resnulli.us> wrote:
> Mon, May 14, 2018 at 04:27:11PM CEST, vladbu@mellanox.com wrote:
>>Implement new action API function to atomically delete action with
>>specified index and to atomically insert unique action. These functions are
>>required to implement init and delete functions for specific actions that
>>do not rely on rtnl lock.
>>
>>Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
>>---
>> include/net/act_api.h | 2 ++
>> net/sched/act_api.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
>> 2 files changed, 47 insertions(+)
>>
>>diff --git a/include/net/act_api.h b/include/net/act_api.h
>>index a8c8570..bce0cf1 100644
>>--- a/include/net/act_api.h
>>+++ b/include/net/act_api.h
>>@@ -153,7 +153,9 @@ int tcf_idr_create(struct tc_action_net *tn, u32 index, struct nlattr *est,
>> struct tc_action **a, const struct tc_action_ops *ops,
>> int bind, bool cpustats);
>> void tcf_idr_insert(struct tc_action_net *tn, struct tc_action *a);
>>+void tcf_idr_insert_unique(struct tc_action_net *tn, struct tc_action *a);
>>
>>+int tcf_idr_find_delete(struct tc_action_net *tn, u32 index);
>> int __tcf_idr_release(struct tc_action *a, bool bind, bool strict);
>>
>> static inline int tcf_idr_release(struct tc_action *a, bool bind)
>>diff --git a/net/sched/act_api.c b/net/sched/act_api.c
>>index 2772276e..a5193dc 100644
>>--- a/net/sched/act_api.c
>>+++ b/net/sched/act_api.c
>>@@ -330,6 +330,41 @@ bool tcf_idr_check(struct tc_action_net *tn, u32 index, struct tc_action **a,
>> }
>> EXPORT_SYMBOL(tcf_idr_check);
>>
>>+int tcf_idr_find_delete(struct tc_action_net *tn, u32 index)
>>+{
>>+ struct tcf_idrinfo *idrinfo = tn->idrinfo;
>>+ struct tc_action *p;
>>+ int ret = 0;
>>+
>>+ spin_lock_bh(&idrinfo->lock);
>
> Why "_bh" is needed here?
Original idr remove function used _bh version so I used it here as well.
As I already replied to your previous question about idrinfo lock usage,
I don't see any particular reason for locking with _bh at this point.
I've contacted the author(Chris Mi) and he said that he just preserved
locking the same way as it was before he changed hash table to idr for
action lookup.
You want me to do standalone patch that cleans up idrinfo locking?
>
>
>>+ p = idr_find(&idrinfo->action_idr, index);
>>+ if (!p) {
>>+ spin_unlock(&idrinfo->lock);
>>+ return -ENOENT;
>>+ }
>>+
>>+ if (!atomic_read(&p->tcfa_bindcnt)) {
>>+ if (refcount_dec_and_test(&p->tcfa_refcnt)) {
>>+ struct module *owner = p->ops->owner;
>>+
>>+ WARN_ON(p != idr_remove(&idrinfo->action_idr,
>>+ p->tcfa_index));
>>+ spin_unlock_bh(&idrinfo->lock);
>>+
>>+ tcf_action_cleanup(p);
>>+ module_put(owner);
>>+ return 0;
>>+ }
>>+ ret = 0;
>>+ } else {
>>+ ret = -EPERM;
>
> I wonder if "-EPERM" is the best error code for this...
This is what original code returned so I decided to preserve
compatibility.
>
>
>>+ }
>>+
>>+ spin_unlock_bh(&idrinfo->lock);
>>+ return ret;
>>+}
>>+EXPORT_SYMBOL(tcf_idr_find_delete);
>>+
>> int tcf_idr_create(struct tc_action_net *tn, u32 index, struct nlattr *est,
>> struct tc_action **a, const struct tc_action_ops *ops,
>> int bind, bool cpustats)
>>@@ -407,6 +442,16 @@ void tcf_idr_insert(struct tc_action_net *tn, struct tc_action *a)
>> }
>> EXPORT_SYMBOL(tcf_idr_insert);
>>
>>+void tcf_idr_insert_unique(struct tc_action_net *tn, struct tc_action *a)
>>+{
>>+ struct tcf_idrinfo *idrinfo = tn->idrinfo;
>>+
>>+ spin_lock_bh(&idrinfo->lock);
>>+ WARN_ON(idr_replace(&idrinfo->action_idr, a, a->tcfa_index));
>
> Under which condition this WARN_ON is hit?
When idr replace returns non-NULL pointer, which means that somehow
concurrent insertion of action with same index has happened and we are
leaking memory.
By the way I'm still not sure if having this insert unique function is
warranted or I should just add WARN to regular idr insert. What is your
opinion on this?
>
>
>>+ spin_unlock_bh(&idrinfo->lock);
>>+}
>>+EXPORT_SYMBOL(tcf_idr_insert_unique);
>>+
>> void tcf_idrinfo_destroy(const struct tc_action_ops *ops,
>> struct tcf_idrinfo *idrinfo)
>> {
>>--
>>2.7.5
>>
^ permalink raw reply
* Re: Hangs in r8152 connected to power management in kernels at least up v4.17-rc4
From: Oliver Neukum @ 2018-05-16 8:26 UTC (permalink / raw)
To: Hayes Wang; +Cc: netdev@vger.kernel.org
In-Reply-To: <0835B3720019904CB8F7AA43166CEEB2D2E47ABE@RTITMBSV06.realtek.com.tw>
Am Mittwoch, den 16.05.2018, 03:37 +0000 schrieb Hayes Wang:
> Oliver Neukum [mailto:oneukum@suse.com]
> >
> > Hi,
> >
> > I got reports about hangs with this trace:
> >
> > May 13 01:36:55 neroon kernel: INFO: task kworker/0:0:4 blocked for more
> > than 60 seconds.
> > May 13 01:36:55 neroon kernel: Tainted: G U
> > 4.17.0-rc4-1.g8257a00-vanilla #1
> > May 13 01:36:55 neroon kernel: "echo 0 >
> > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
> > May 13 01:36:55 neroon kernel: kworker/0:0 D 0 4 2
> > 0x80000000
> > May 13 01:36:55 neroon kernel: Workqueue: events rtl_work_func_t [r8152]
> > May 13 01:36:55 neroon kernel: Call Trace:
> > May 13 01:36:55 neroon kernel: ? __schedule+0x289/0x880
> > May 13 01:36:55 neroon kernel: schedule+0x2f/0x90
> > May 13 01:36:55 neroon kernel: rpm_resume+0xf9/0x7a0
> > May 13 01:36:55 neroon kernel: ? wait_woken+0x80/0x80
> > May 13 01:36:55 neroon kernel: rpm_resume+0x547/0x7a0
> > May 13 01:36:55 neroon kernel: ? __switch_to_asm+0x40/0x70
> > May 13 01:36:55 neroon kernel: ? __switch_to_asm+0x34/0x70
> > May 13 01:36:55 neroon kernel: ? __switch_to_asm+0x40/0x70
> > May 13 01:36:55 neroon kernel: ? __switch_to_asm+0x34/0x70
> > May 13 01:36:55 neroon kernel: ? __switch_to_asm+0x40/0x70
> > May 13 01:36:55 neroon kernel: __pm_runtime_resume+0x3a/0x50
> > May 13 01:36:55 neroon kernel: usb_autopm_get_interface+0x1d/0x50 [usbcore]
>
> Would usb_autopm_get_interface() take a long time?
> The driver would wake the device if it has suspended.
> I have no idea about how usb_autopm_get_interface() works, so I don't know how to help.
Hi,
it basically calls r8152_resume() and makes a control request to the
hub. I think we are spinning in rtl8152_runtime_resume(), but where?
It has a lot of NAPI stuff. Any suggestions on how to instrument or
trace this?
Regards
Oliver
^ permalink raw reply
* [RFC v4 0/5] virtio: support packed ring
From: Tiwei Bie @ 2018-05-16 8:37 UTC (permalink / raw)
To: mst, jasowang, virtualization, linux-kernel, netdev
Cc: wexu, jfreimann, tiwei.bie
Hello everyone,
This RFC implements packed ring support in virtio driver.
Some simple functional tests have been done with Jason's
packed ring implementation in vhost:
https://lkml.org/lkml/2018/4/23/12
Both of ping and netperf worked as expected (with EVENT_IDX
disabled).
TODO:
- Refinements (for code and commit log);
- More tests;
- Bug fixes;
RFC v3 -> RFC v4:
- Make ID allocation support out-of-order (Jason);
- Various fixes for EVENT_IDX support;
RFC v2 -> RFC v3:
- Split into small patches (Jason);
- Add helper virtqueue_use_indirect() (Jason);
- Just set id for the last descriptor of a list (Jason);
- Calculate the prev in virtqueue_add_packed() (Jason);
- Fix/improve desc suppression code (Jason/MST);
- Refine the code layout for XXX_split/packed and wrappers (MST);
- Fix the comments and API in uapi (MST);
- Remove the BUG_ON() for indirect (Jason);
- Some other refinements and bug fixes;
RFC v1 -> RFC v2:
- Add indirect descriptor support - compile test only;
- Add event suppression supprt - compile test only;
- Move vring_packed_init() out of uapi (Jason, MST);
- Merge two loops into one in virtqueue_add_packed() (Jason);
- Split vring_unmap_one() for packed ring and split ring (Jason);
- Avoid using '%' operator (Jason);
- Rename free_head -> next_avail_idx (Jason);
- Add comments for virtio_wmb() in virtqueue_add_packed() (Jason);
- Some other refinements and bug fixes;
Thanks!
Tiwei Bie (5):
virtio: add packed ring definitions
virtio_ring: support creating packed ring
virtio_ring: add packed ring support
virtio_ring: add event idx support in packed ring
virtio_ring: enable packed ring
drivers/virtio/virtio_ring.c | 1338 ++++++++++++++++++++++------
include/linux/virtio_ring.h | 8 +-
include/uapi/linux/virtio_config.h | 12 +-
include/uapi/linux/virtio_ring.h | 36 +
4 files changed, 1116 insertions(+), 278 deletions(-)
--
2.17.0
^ permalink raw reply
* [RFC v4 1/5] virtio: add packed ring definitions
From: Tiwei Bie @ 2018-05-16 8:37 UTC (permalink / raw)
To: mst, jasowang, virtualization, linux-kernel, netdev; +Cc: wexu
In-Reply-To: <20180516083737.26504-1-tiwei.bie@intel.com>
Signed-off-by: Tiwei Bie <tiwei.bie@intel.com>
---
include/uapi/linux/virtio_config.h | 12 +++++++++-
include/uapi/linux/virtio_ring.h | 36 ++++++++++++++++++++++++++++++
2 files changed, 47 insertions(+), 1 deletion(-)
diff --git a/include/uapi/linux/virtio_config.h b/include/uapi/linux/virtio_config.h
index 308e2096291f..a6e392325e3a 100644
--- a/include/uapi/linux/virtio_config.h
+++ b/include/uapi/linux/virtio_config.h
@@ -49,7 +49,7 @@
* transport being used (eg. virtio_ring), the rest are per-device feature
* bits. */
#define VIRTIO_TRANSPORT_F_START 28
-#define VIRTIO_TRANSPORT_F_END 34
+#define VIRTIO_TRANSPORT_F_END 36
#ifndef VIRTIO_CONFIG_NO_LEGACY
/* Do we get callbacks when the ring is completely used, even if we've
@@ -71,4 +71,14 @@
* this is for compatibility with legacy systems.
*/
#define VIRTIO_F_IOMMU_PLATFORM 33
+
+/* This feature indicates support for the packed virtqueue layout. */
+#define VIRTIO_F_RING_PACKED 34
+
+/*
+ * This feature indicates that all buffers are used by the device
+ * in the same order in which they have been made available.
+ */
+#define VIRTIO_F_IN_ORDER 35
+
#endif /* _UAPI_LINUX_VIRTIO_CONFIG_H */
diff --git a/include/uapi/linux/virtio_ring.h b/include/uapi/linux/virtio_ring.h
index 6d5d5faa989b..3932cb80c347 100644
--- a/include/uapi/linux/virtio_ring.h
+++ b/include/uapi/linux/virtio_ring.h
@@ -44,6 +44,9 @@
/* This means the buffer contains a list of buffer descriptors. */
#define VRING_DESC_F_INDIRECT 4
+#define VRING_DESC_F_AVAIL(b) ((b) << 7)
+#define VRING_DESC_F_USED(b) ((b) << 15)
+
/* The Host uses this in used->flags to advise the Guest: don't kick me when
* you add a buffer. It's unreliable, so it's simply an optimization. Guest
* will still kick if it's out of buffers. */
@@ -53,6 +56,10 @@
* optimization. */
#define VRING_AVAIL_F_NO_INTERRUPT 1
+#define VRING_EVENT_F_ENABLE 0x0
+#define VRING_EVENT_F_DISABLE 0x1
+#define VRING_EVENT_F_DESC 0x2
+
/* We support indirect buffer descriptors */
#define VIRTIO_RING_F_INDIRECT_DESC 28
@@ -171,4 +178,33 @@ static inline int vring_need_event(__u16 event_idx, __u16 new_idx, __u16 old)
return (__u16)(new_idx - event_idx - 1) < (__u16)(new_idx - old);
}
+struct vring_packed_desc_event {
+ /* __virtio16 off : 15; // Descriptor Event Offset
+ * __virtio16 wrap : 1; // Descriptor Event Wrap Counter */
+ __virtio16 off_wrap;
+ /* __virtio16 flags : 2; // Descriptor Event Flags */
+ __virtio16 flags;
+};
+
+struct vring_packed_desc {
+ /* Buffer Address. */
+ __virtio64 addr;
+ /* Buffer Length. */
+ __virtio32 len;
+ /* Buffer ID. */
+ __virtio16 id;
+ /* The flags depending on descriptor type. */
+ __virtio16 flags;
+};
+
+struct vring_packed {
+ unsigned int num;
+
+ struct vring_packed_desc *desc;
+
+ struct vring_packed_desc_event *driver;
+
+ struct vring_packed_desc_event *device;
+};
+
#endif /* _UAPI_LINUX_VIRTIO_RING_H */
--
2.17.0
^ permalink raw reply related
* [RFC v4 2/5] virtio_ring: support creating packed ring
From: Tiwei Bie @ 2018-05-16 8:37 UTC (permalink / raw)
To: mst, jasowang, virtualization, linux-kernel, netdev
Cc: wexu, jfreimann, tiwei.bie
In-Reply-To: <20180516083737.26504-1-tiwei.bie@intel.com>
This commit introduces the support for creating packed ring.
All split ring specific functions are added _split suffix.
Some necessary stubs for packed ring are also added.
Signed-off-by: Tiwei Bie <tiwei.bie@intel.com>
---
drivers/virtio/virtio_ring.c | 764 +++++++++++++++++++++++------------
include/linux/virtio_ring.h | 8 +-
2 files changed, 513 insertions(+), 259 deletions(-)
diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index 71458f493cf8..62d7c407841a 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -64,8 +64,8 @@ struct vring_desc_state {
struct vring_virtqueue {
struct virtqueue vq;
- /* Actual memory layout for this queue */
- struct vring vring;
+ /* Is this a packed ring? */
+ bool packed;
/* Can we use weak barriers? */
bool weak_barriers;
@@ -79,19 +79,45 @@ struct vring_virtqueue {
/* Host publishes avail event idx */
bool event;
- /* Head of free buffer list. */
- unsigned int free_head;
/* Number we've added since last sync. */
unsigned int num_added;
/* Last used index we've seen. */
u16 last_used_idx;
- /* Last written value to avail->flags */
- u16 avail_flags_shadow;
+ union {
+ /* Available for split ring */
+ struct {
+ /* Actual memory layout for this queue. */
+ struct vring vring;
- /* Last written value to avail->idx in guest byte order */
- u16 avail_idx_shadow;
+ /* Head of free buffer list. */
+ unsigned int free_head;
+
+ /* Last written value to avail->flags */
+ u16 avail_flags_shadow;
+
+ /* Last written value to avail->idx in
+ * guest byte order. */
+ u16 avail_idx_shadow;
+ };
+
+ /* Available for packed ring */
+ struct {
+ /* Actual memory layout for this queue. */
+ struct vring_packed vring_packed;
+
+ /* Driver ring wrap counter. */
+ u8 wrap_counter;
+
+ /* Index of the next avail descriptor. */
+ u16 next_avail_idx;
+
+ /* Last written value to driver->flags in
+ * guest byte order. */
+ u16 event_flags_shadow;
+ };
+ };
/* How to notify other side. FIXME: commonalize hcalls! */
bool (*notify)(struct virtqueue *vq);
@@ -201,8 +227,17 @@ static dma_addr_t vring_map_single(const struct vring_virtqueue *vq,
cpu_addr, size, direction);
}
-static void vring_unmap_one(const struct vring_virtqueue *vq,
- struct vring_desc *desc)
+static int vring_mapping_error(const struct vring_virtqueue *vq,
+ dma_addr_t addr)
+{
+ if (!vring_use_dma_api(vq->vq.vdev))
+ return 0;
+
+ return dma_mapping_error(vring_dma_dev(vq), addr);
+}
+
+static void vring_unmap_one_split(const struct vring_virtqueue *vq,
+ struct vring_desc *desc)
{
u16 flags;
@@ -226,17 +261,9 @@ static void vring_unmap_one(const struct vring_virtqueue *vq,
}
}
-static int vring_mapping_error(const struct vring_virtqueue *vq,
- dma_addr_t addr)
-{
- if (!vring_use_dma_api(vq->vq.vdev))
- return 0;
-
- return dma_mapping_error(vring_dma_dev(vq), addr);
-}
-
-static struct vring_desc *alloc_indirect(struct virtqueue *_vq,
- unsigned int total_sg, gfp_t gfp)
+static struct vring_desc *alloc_indirect_split(struct virtqueue *_vq,
+ unsigned int total_sg,
+ gfp_t gfp)
{
struct vring_desc *desc;
unsigned int i;
@@ -257,14 +284,14 @@ static struct vring_desc *alloc_indirect(struct virtqueue *_vq,
return desc;
}
-static inline int virtqueue_add(struct virtqueue *_vq,
- struct scatterlist *sgs[],
- unsigned int total_sg,
- unsigned int out_sgs,
- unsigned int in_sgs,
- void *data,
- void *ctx,
- gfp_t gfp)
+static inline int virtqueue_add_split(struct virtqueue *_vq,
+ struct scatterlist *sgs[],
+ unsigned int total_sg,
+ unsigned int out_sgs,
+ unsigned int in_sgs,
+ void *data,
+ void *ctx,
+ gfp_t gfp)
{
struct vring_virtqueue *vq = to_vvq(_vq);
struct scatterlist *sg;
@@ -303,7 +330,7 @@ static inline int virtqueue_add(struct virtqueue *_vq,
/* If the host supports indirect descriptor tables, and we have multiple
* buffers, then go indirect. FIXME: tune this threshold */
if (vq->indirect && total_sg > 1 && vq->vq.num_free)
- desc = alloc_indirect(_vq, total_sg, gfp);
+ desc = alloc_indirect_split(_vq, total_sg, gfp);
else {
desc = NULL;
WARN_ON_ONCE(total_sg > vq->vring.num && !vq->indirect);
@@ -424,7 +451,7 @@ static inline int virtqueue_add(struct virtqueue *_vq,
for (n = 0; n < total_sg; n++) {
if (i == err_idx)
break;
- vring_unmap_one(vq, &desc[i]);
+ vring_unmap_one_split(vq, &desc[i]);
i = virtio16_to_cpu(_vq->vdev, vq->vring.desc[i].next);
}
@@ -435,6 +462,355 @@ static inline int virtqueue_add(struct virtqueue *_vq,
return -EIO;
}
+static bool virtqueue_kick_prepare_split(struct virtqueue *_vq)
+{
+ struct vring_virtqueue *vq = to_vvq(_vq);
+ u16 new, old;
+ bool needs_kick;
+
+ START_USE(vq);
+ /* We need to expose available array entries before checking avail
+ * event. */
+ virtio_mb(vq->weak_barriers);
+
+ old = vq->avail_idx_shadow - vq->num_added;
+ new = vq->avail_idx_shadow;
+ vq->num_added = 0;
+
+#ifdef DEBUG
+ if (vq->last_add_time_valid) {
+ WARN_ON(ktime_to_ms(ktime_sub(ktime_get(),
+ vq->last_add_time)) > 100);
+ }
+ vq->last_add_time_valid = false;
+#endif
+
+ if (vq->event) {
+ needs_kick = vring_need_event(virtio16_to_cpu(_vq->vdev, vring_avail_event(&vq->vring)),
+ new, old);
+ } else {
+ needs_kick = !(vq->vring.used->flags & cpu_to_virtio16(_vq->vdev, VRING_USED_F_NO_NOTIFY));
+ }
+ END_USE(vq);
+ return needs_kick;
+}
+
+static void detach_buf_split(struct vring_virtqueue *vq, unsigned int head,
+ void **ctx)
+{
+ unsigned int i, j;
+ __virtio16 nextflag = cpu_to_virtio16(vq->vq.vdev, VRING_DESC_F_NEXT);
+
+ /* Clear data ptr. */
+ vq->desc_state[head].data = NULL;
+
+ /* Put back on free list: unmap first-level descriptors and find end */
+ i = head;
+
+ while (vq->vring.desc[i].flags & nextflag) {
+ vring_unmap_one_split(vq, &vq->vring.desc[i]);
+ i = virtio16_to_cpu(vq->vq.vdev, vq->vring.desc[i].next);
+ vq->vq.num_free++;
+ }
+
+ vring_unmap_one_split(vq, &vq->vring.desc[i]);
+ vq->vring.desc[i].next = cpu_to_virtio16(vq->vq.vdev, vq->free_head);
+ vq->free_head = head;
+
+ /* Plus final descriptor */
+ vq->vq.num_free++;
+
+ if (vq->indirect) {
+ struct vring_desc *indir_desc = vq->desc_state[head].indir_desc;
+ u32 len;
+
+ /* Free the indirect table, if any, now that it's unmapped. */
+ if (!indir_desc)
+ return;
+
+ len = virtio32_to_cpu(vq->vq.vdev, vq->vring.desc[head].len);
+
+ BUG_ON(!(vq->vring.desc[head].flags &
+ cpu_to_virtio16(vq->vq.vdev, VRING_DESC_F_INDIRECT)));
+ BUG_ON(len == 0 || len % sizeof(struct vring_desc));
+
+ for (j = 0; j < len / sizeof(struct vring_desc); j++)
+ vring_unmap_one_split(vq, &indir_desc[j]);
+
+ kfree(indir_desc);
+ vq->desc_state[head].indir_desc = NULL;
+ } else if (ctx) {
+ *ctx = vq->desc_state[head].indir_desc;
+ }
+}
+
+static inline bool more_used_split(const struct vring_virtqueue *vq)
+{
+ return vq->last_used_idx != virtio16_to_cpu(vq->vq.vdev, vq->vring.used->idx);
+}
+
+static void *virtqueue_get_buf_ctx_split(struct virtqueue *_vq,
+ unsigned int *len,
+ void **ctx)
+{
+ struct vring_virtqueue *vq = to_vvq(_vq);
+ void *ret;
+ unsigned int i;
+ u16 last_used;
+
+ START_USE(vq);
+
+ if (unlikely(vq->broken)) {
+ END_USE(vq);
+ return NULL;
+ }
+
+ if (!more_used_split(vq)) {
+ pr_debug("No more buffers in queue\n");
+ END_USE(vq);
+ return NULL;
+ }
+
+ /* Only get used array entries after they have been exposed by host. */
+ virtio_rmb(vq->weak_barriers);
+
+ last_used = (vq->last_used_idx & (vq->vring.num - 1));
+ i = virtio32_to_cpu(_vq->vdev, vq->vring.used->ring[last_used].id);
+ *len = virtio32_to_cpu(_vq->vdev, vq->vring.used->ring[last_used].len);
+
+ if (unlikely(i >= vq->vring.num)) {
+ BAD_RING(vq, "id %u out of range\n", i);
+ return NULL;
+ }
+ if (unlikely(!vq->desc_state[i].data)) {
+ BAD_RING(vq, "id %u is not a head!\n", i);
+ return NULL;
+ }
+
+ /* detach_buf_split clears data, so grab it now. */
+ ret = vq->desc_state[i].data;
+ detach_buf_split(vq, i, ctx);
+ vq->last_used_idx++;
+ /* If we expect an interrupt for the next entry, tell host
+ * by writing event index and flush out the write before
+ * the read in the next get_buf call. */
+ if (!(vq->avail_flags_shadow & VRING_AVAIL_F_NO_INTERRUPT))
+ virtio_store_mb(vq->weak_barriers,
+ &vring_used_event(&vq->vring),
+ cpu_to_virtio16(_vq->vdev, vq->last_used_idx));
+
+#ifdef DEBUG
+ vq->last_add_time_valid = false;
+#endif
+
+ END_USE(vq);
+ return ret;
+}
+
+static void virtqueue_disable_cb_split(struct virtqueue *_vq)
+{
+ struct vring_virtqueue *vq = to_vvq(_vq);
+
+ if (!(vq->avail_flags_shadow & VRING_AVAIL_F_NO_INTERRUPT)) {
+ vq->avail_flags_shadow |= VRING_AVAIL_F_NO_INTERRUPT;
+ if (!vq->event)
+ vq->vring.avail->flags = cpu_to_virtio16(_vq->vdev, vq->avail_flags_shadow);
+ }
+}
+
+static unsigned virtqueue_enable_cb_prepare_split(struct virtqueue *_vq)
+{
+ struct vring_virtqueue *vq = to_vvq(_vq);
+ u16 last_used_idx;
+
+ START_USE(vq);
+
+ /* We optimistically turn back on interrupts, then check if there was
+ * more to do. */
+ /* Depending on the VIRTIO_RING_F_EVENT_IDX feature, we need to
+ * either clear the flags bit or point the event index at the next
+ * entry. Always do both to keep code simple. */
+ if (vq->avail_flags_shadow & VRING_AVAIL_F_NO_INTERRUPT) {
+ vq->avail_flags_shadow &= ~VRING_AVAIL_F_NO_INTERRUPT;
+ if (!vq->event)
+ vq->vring.avail->flags = cpu_to_virtio16(_vq->vdev, vq->avail_flags_shadow);
+ }
+ vring_used_event(&vq->vring) = cpu_to_virtio16(_vq->vdev, last_used_idx = vq->last_used_idx);
+ END_USE(vq);
+ return last_used_idx;
+}
+
+static bool virtqueue_poll_split(struct virtqueue *_vq, unsigned last_used_idx)
+{
+ struct vring_virtqueue *vq = to_vvq(_vq);
+
+ virtio_mb(vq->weak_barriers);
+ return (u16)last_used_idx != virtio16_to_cpu(_vq->vdev, vq->vring.used->idx);
+}
+
+static bool virtqueue_enable_cb_delayed_split(struct virtqueue *_vq)
+{
+ struct vring_virtqueue *vq = to_vvq(_vq);
+ u16 bufs;
+
+ START_USE(vq);
+
+ /* We optimistically turn back on interrupts, then check if there was
+ * more to do. */
+ /* Depending on the VIRTIO_RING_F_USED_EVENT_IDX feature, we need to
+ * either clear the flags bit or point the event index at the next
+ * entry. Always update the event index to keep code simple. */
+ if (vq->avail_flags_shadow & VRING_AVAIL_F_NO_INTERRUPT) {
+ vq->avail_flags_shadow &= ~VRING_AVAIL_F_NO_INTERRUPT;
+ if (!vq->event)
+ vq->vring.avail->flags = cpu_to_virtio16(_vq->vdev, vq->avail_flags_shadow);
+ }
+ /* TODO: tune this threshold */
+ bufs = (u16)(vq->avail_idx_shadow - vq->last_used_idx) * 3 / 4;
+
+ virtio_store_mb(vq->weak_barriers,
+ &vring_used_event(&vq->vring),
+ cpu_to_virtio16(_vq->vdev, vq->last_used_idx + bufs));
+
+ if (unlikely((u16)(virtio16_to_cpu(_vq->vdev, vq->vring.used->idx) - vq->last_used_idx) > bufs)) {
+ END_USE(vq);
+ return false;
+ }
+
+ END_USE(vq);
+ return true;
+}
+
+static void *virtqueue_detach_unused_buf_split(struct virtqueue *_vq)
+{
+ struct vring_virtqueue *vq = to_vvq(_vq);
+ unsigned int i;
+ void *buf;
+
+ START_USE(vq);
+
+ for (i = 0; i < vq->vring.num; i++) {
+ if (!vq->desc_state[i].data)
+ continue;
+ /* detach_buf clears data, so grab it now. */
+ buf = vq->desc_state[i].data;
+ detach_buf_split(vq, i, NULL);
+ vq->avail_idx_shadow--;
+ vq->vring.avail->idx = cpu_to_virtio16(_vq->vdev, vq->avail_idx_shadow);
+ END_USE(vq);
+ return buf;
+ }
+ /* That should have freed everything. */
+ BUG_ON(vq->vq.num_free != vq->vring.num);
+
+ END_USE(vq);
+ return NULL;
+}
+
+/*
+ * The layout for the packed ring is a continuous chunk of memory
+ * which looks like this.
+ *
+ * struct vring_packed {
+ * // The actual descriptors (16 bytes each)
+ * struct vring_packed_desc desc[num];
+ *
+ * // Padding to the next align boundary.
+ * char pad[];
+ *
+ * // Driver Event Suppression
+ * struct vring_packed_desc_event driver;
+ *
+ * // Device Event Suppression
+ * struct vring_packed_desc_event device;
+ * };
+ */
+static inline void vring_init_packed(struct vring_packed *vr, unsigned int num,
+ void *p, unsigned long align)
+{
+ vr->num = num;
+ vr->desc = p;
+ vr->driver = (void *)(((uintptr_t)p + sizeof(struct vring_packed_desc)
+ * num + align - 1) & ~(align - 1));
+ vr->device = vr->driver + 1;
+}
+
+static inline unsigned vring_size_packed(unsigned int num, unsigned long align)
+{
+ return ((sizeof(struct vring_packed_desc) * num + align - 1)
+ & ~(align - 1)) + sizeof(struct vring_packed_desc_event) * 2;
+}
+
+static inline int virtqueue_add_packed(struct virtqueue *_vq,
+ struct scatterlist *sgs[],
+ unsigned int total_sg,
+ unsigned int out_sgs,
+ unsigned int in_sgs,
+ void *data,
+ void *ctx,
+ gfp_t gfp)
+{
+ return -EIO;
+}
+
+static bool virtqueue_kick_prepare_packed(struct virtqueue *_vq)
+{
+ return false;
+}
+
+static inline bool more_used_packed(const struct vring_virtqueue *vq)
+{
+ return false;
+}
+
+static void *virtqueue_get_buf_ctx_packed(struct virtqueue *_vq,
+ unsigned int *len,
+ void **ctx)
+{
+ return NULL;
+}
+
+static void virtqueue_disable_cb_packed(struct virtqueue *_vq)
+{
+}
+
+static unsigned virtqueue_enable_cb_prepare_packed(struct virtqueue *_vq)
+{
+ return 0;
+}
+
+static bool virtqueue_poll_packed(struct virtqueue *_vq, unsigned last_used_idx)
+{
+ return false;
+}
+
+static bool virtqueue_enable_cb_delayed_packed(struct virtqueue *_vq)
+{
+ return false;
+}
+
+static void *virtqueue_detach_unused_buf_packed(struct virtqueue *_vq)
+{
+ return NULL;
+}
+
+static inline int virtqueue_add(struct virtqueue *_vq,
+ struct scatterlist *sgs[],
+ unsigned int total_sg,
+ unsigned int out_sgs,
+ unsigned int in_sgs,
+ void *data,
+ void *ctx,
+ gfp_t gfp)
+{
+ struct vring_virtqueue *vq = to_vvq(_vq);
+
+ return vq->packed ? virtqueue_add_packed(_vq, sgs, total_sg, out_sgs,
+ in_sgs, data, ctx, gfp) :
+ virtqueue_add_split(_vq, sgs, total_sg, out_sgs,
+ in_sgs, data, ctx, gfp);
+}
+
/**
* virtqueue_add_sgs - expose buffers to other end
* @vq: the struct virtqueue we're talking about.
@@ -551,34 +927,9 @@ EXPORT_SYMBOL_GPL(virtqueue_add_inbuf_ctx);
bool virtqueue_kick_prepare(struct virtqueue *_vq)
{
struct vring_virtqueue *vq = to_vvq(_vq);
- u16 new, old;
- bool needs_kick;
- START_USE(vq);
- /* We need to expose available array entries before checking avail
- * event. */
- virtio_mb(vq->weak_barriers);
-
- old = vq->avail_idx_shadow - vq->num_added;
- new = vq->avail_idx_shadow;
- vq->num_added = 0;
-
-#ifdef DEBUG
- if (vq->last_add_time_valid) {
- WARN_ON(ktime_to_ms(ktime_sub(ktime_get(),
- vq->last_add_time)) > 100);
- }
- vq->last_add_time_valid = false;
-#endif
-
- if (vq->event) {
- needs_kick = vring_need_event(virtio16_to_cpu(_vq->vdev, vring_avail_event(&vq->vring)),
- new, old);
- } else {
- needs_kick = !(vq->vring.used->flags & cpu_to_virtio16(_vq->vdev, VRING_USED_F_NO_NOTIFY));
- }
- END_USE(vq);
- return needs_kick;
+ return vq->packed ? virtqueue_kick_prepare_packed(_vq) :
+ virtqueue_kick_prepare_split(_vq);
}
EXPORT_SYMBOL_GPL(virtqueue_kick_prepare);
@@ -626,58 +977,9 @@ bool virtqueue_kick(struct virtqueue *vq)
}
EXPORT_SYMBOL_GPL(virtqueue_kick);
-static void detach_buf(struct vring_virtqueue *vq, unsigned int head,
- void **ctx)
-{
- unsigned int i, j;
- __virtio16 nextflag = cpu_to_virtio16(vq->vq.vdev, VRING_DESC_F_NEXT);
-
- /* Clear data ptr. */
- vq->desc_state[head].data = NULL;
-
- /* Put back on free list: unmap first-level descriptors and find end */
- i = head;
-
- while (vq->vring.desc[i].flags & nextflag) {
- vring_unmap_one(vq, &vq->vring.desc[i]);
- i = virtio16_to_cpu(vq->vq.vdev, vq->vring.desc[i].next);
- vq->vq.num_free++;
- }
-
- vring_unmap_one(vq, &vq->vring.desc[i]);
- vq->vring.desc[i].next = cpu_to_virtio16(vq->vq.vdev, vq->free_head);
- vq->free_head = head;
-
- /* Plus final descriptor */
- vq->vq.num_free++;
-
- if (vq->indirect) {
- struct vring_desc *indir_desc = vq->desc_state[head].indir_desc;
- u32 len;
-
- /* Free the indirect table, if any, now that it's unmapped. */
- if (!indir_desc)
- return;
-
- len = virtio32_to_cpu(vq->vq.vdev, vq->vring.desc[head].len);
-
- BUG_ON(!(vq->vring.desc[head].flags &
- cpu_to_virtio16(vq->vq.vdev, VRING_DESC_F_INDIRECT)));
- BUG_ON(len == 0 || len % sizeof(struct vring_desc));
-
- for (j = 0; j < len / sizeof(struct vring_desc); j++)
- vring_unmap_one(vq, &indir_desc[j]);
-
- kfree(indir_desc);
- vq->desc_state[head].indir_desc = NULL;
- } else if (ctx) {
- *ctx = vq->desc_state[head].indir_desc;
- }
-}
-
static inline bool more_used(const struct vring_virtqueue *vq)
{
- return vq->last_used_idx != virtio16_to_cpu(vq->vq.vdev, vq->vring.used->idx);
+ return vq->packed ? more_used_packed(vq) : more_used_split(vq);
}
/**
@@ -700,57 +1002,9 @@ void *virtqueue_get_buf_ctx(struct virtqueue *_vq, unsigned int *len,
void **ctx)
{
struct vring_virtqueue *vq = to_vvq(_vq);
- void *ret;
- unsigned int i;
- u16 last_used;
- START_USE(vq);
-
- if (unlikely(vq->broken)) {
- END_USE(vq);
- return NULL;
- }
-
- if (!more_used(vq)) {
- pr_debug("No more buffers in queue\n");
- END_USE(vq);
- return NULL;
- }
-
- /* Only get used array entries after they have been exposed by host. */
- virtio_rmb(vq->weak_barriers);
-
- last_used = (vq->last_used_idx & (vq->vring.num - 1));
- i = virtio32_to_cpu(_vq->vdev, vq->vring.used->ring[last_used].id);
- *len = virtio32_to_cpu(_vq->vdev, vq->vring.used->ring[last_used].len);
-
- if (unlikely(i >= vq->vring.num)) {
- BAD_RING(vq, "id %u out of range\n", i);
- return NULL;
- }
- if (unlikely(!vq->desc_state[i].data)) {
- BAD_RING(vq, "id %u is not a head!\n", i);
- return NULL;
- }
-
- /* detach_buf clears data, so grab it now. */
- ret = vq->desc_state[i].data;
- detach_buf(vq, i, ctx);
- vq->last_used_idx++;
- /* If we expect an interrupt for the next entry, tell host
- * by writing event index and flush out the write before
- * the read in the next get_buf call. */
- if (!(vq->avail_flags_shadow & VRING_AVAIL_F_NO_INTERRUPT))
- virtio_store_mb(vq->weak_barriers,
- &vring_used_event(&vq->vring),
- cpu_to_virtio16(_vq->vdev, vq->last_used_idx));
-
-#ifdef DEBUG
- vq->last_add_time_valid = false;
-#endif
-
- END_USE(vq);
- return ret;
+ return vq->packed ? virtqueue_get_buf_ctx_packed(_vq, len, ctx) :
+ virtqueue_get_buf_ctx_split(_vq, len, ctx);
}
EXPORT_SYMBOL_GPL(virtqueue_get_buf_ctx);
@@ -772,12 +1026,10 @@ void virtqueue_disable_cb(struct virtqueue *_vq)
{
struct vring_virtqueue *vq = to_vvq(_vq);
- if (!(vq->avail_flags_shadow & VRING_AVAIL_F_NO_INTERRUPT)) {
- vq->avail_flags_shadow |= VRING_AVAIL_F_NO_INTERRUPT;
- if (!vq->event)
- vq->vring.avail->flags = cpu_to_virtio16(_vq->vdev, vq->avail_flags_shadow);
- }
-
+ if (vq->packed)
+ virtqueue_disable_cb_packed(_vq);
+ else
+ virtqueue_disable_cb_split(_vq);
}
EXPORT_SYMBOL_GPL(virtqueue_disable_cb);
@@ -796,23 +1048,9 @@ EXPORT_SYMBOL_GPL(virtqueue_disable_cb);
unsigned virtqueue_enable_cb_prepare(struct virtqueue *_vq)
{
struct vring_virtqueue *vq = to_vvq(_vq);
- u16 last_used_idx;
- START_USE(vq);
-
- /* We optimistically turn back on interrupts, then check if there was
- * more to do. */
- /* Depending on the VIRTIO_RING_F_EVENT_IDX feature, we need to
- * either clear the flags bit or point the event index at the next
- * entry. Always do both to keep code simple. */
- if (vq->avail_flags_shadow & VRING_AVAIL_F_NO_INTERRUPT) {
- vq->avail_flags_shadow &= ~VRING_AVAIL_F_NO_INTERRUPT;
- if (!vq->event)
- vq->vring.avail->flags = cpu_to_virtio16(_vq->vdev, vq->avail_flags_shadow);
- }
- vring_used_event(&vq->vring) = cpu_to_virtio16(_vq->vdev, last_used_idx = vq->last_used_idx);
- END_USE(vq);
- return last_used_idx;
+ return vq->packed ? virtqueue_enable_cb_prepare_packed(_vq) :
+ virtqueue_enable_cb_prepare_split(_vq);
}
EXPORT_SYMBOL_GPL(virtqueue_enable_cb_prepare);
@@ -829,8 +1067,8 @@ bool virtqueue_poll(struct virtqueue *_vq, unsigned last_used_idx)
{
struct vring_virtqueue *vq = to_vvq(_vq);
- virtio_mb(vq->weak_barriers);
- return (u16)last_used_idx != virtio16_to_cpu(_vq->vdev, vq->vring.used->idx);
+ return vq->packed ? virtqueue_poll_packed(_vq, last_used_idx) :
+ virtqueue_poll_split(_vq, last_used_idx);
}
EXPORT_SYMBOL_GPL(virtqueue_poll);
@@ -868,34 +1106,9 @@ EXPORT_SYMBOL_GPL(virtqueue_enable_cb);
bool virtqueue_enable_cb_delayed(struct virtqueue *_vq)
{
struct vring_virtqueue *vq = to_vvq(_vq);
- u16 bufs;
- START_USE(vq);
-
- /* We optimistically turn back on interrupts, then check if there was
- * more to do. */
- /* Depending on the VIRTIO_RING_F_USED_EVENT_IDX feature, we need to
- * either clear the flags bit or point the event index at the next
- * entry. Always update the event index to keep code simple. */
- if (vq->avail_flags_shadow & VRING_AVAIL_F_NO_INTERRUPT) {
- vq->avail_flags_shadow &= ~VRING_AVAIL_F_NO_INTERRUPT;
- if (!vq->event)
- vq->vring.avail->flags = cpu_to_virtio16(_vq->vdev, vq->avail_flags_shadow);
- }
- /* TODO: tune this threshold */
- bufs = (u16)(vq->avail_idx_shadow - vq->last_used_idx) * 3 / 4;
-
- virtio_store_mb(vq->weak_barriers,
- &vring_used_event(&vq->vring),
- cpu_to_virtio16(_vq->vdev, vq->last_used_idx + bufs));
-
- if (unlikely((u16)(virtio16_to_cpu(_vq->vdev, vq->vring.used->idx) - vq->last_used_idx) > bufs)) {
- END_USE(vq);
- return false;
- }
-
- END_USE(vq);
- return true;
+ return vq->packed ? virtqueue_enable_cb_delayed_packed(_vq) :
+ virtqueue_enable_cb_delayed_split(_vq);
}
EXPORT_SYMBOL_GPL(virtqueue_enable_cb_delayed);
@@ -910,27 +1123,9 @@ EXPORT_SYMBOL_GPL(virtqueue_enable_cb_delayed);
void *virtqueue_detach_unused_buf(struct virtqueue *_vq)
{
struct vring_virtqueue *vq = to_vvq(_vq);
- unsigned int i;
- void *buf;
- START_USE(vq);
-
- for (i = 0; i < vq->vring.num; i++) {
- if (!vq->desc_state[i].data)
- continue;
- /* detach_buf clears data, so grab it now. */
- buf = vq->desc_state[i].data;
- detach_buf(vq, i, NULL);
- vq->avail_idx_shadow--;
- vq->vring.avail->idx = cpu_to_virtio16(_vq->vdev, vq->avail_idx_shadow);
- END_USE(vq);
- return buf;
- }
- /* That should have freed everything. */
- BUG_ON(vq->vq.num_free != vq->vring.num);
-
- END_USE(vq);
- return NULL;
+ return vq->packed ? virtqueue_detach_unused_buf_packed(_vq) :
+ virtqueue_detach_unused_buf_split(_vq);
}
EXPORT_SYMBOL_GPL(virtqueue_detach_unused_buf);
@@ -955,7 +1150,8 @@ irqreturn_t vring_interrupt(int irq, void *_vq)
EXPORT_SYMBOL_GPL(vring_interrupt);
struct virtqueue *__vring_new_virtqueue(unsigned int index,
- struct vring vring,
+ union vring_union vring,
+ bool packed,
struct virtio_device *vdev,
bool weak_barriers,
bool context,
@@ -963,19 +1159,20 @@ struct virtqueue *__vring_new_virtqueue(unsigned int index,
void (*callback)(struct virtqueue *),
const char *name)
{
- unsigned int i;
+ unsigned int num, i;
struct vring_virtqueue *vq;
- vq = kmalloc(sizeof(*vq) + vring.num * sizeof(struct vring_desc_state),
+ num = packed ? vring.vring_packed.num : vring.vring_split.num;
+
+ vq = kmalloc(sizeof(*vq) + num * sizeof(struct vring_desc_state),
GFP_KERNEL);
if (!vq)
return NULL;
- vq->vring = vring;
vq->vq.callback = callback;
vq->vq.vdev = vdev;
vq->vq.name = name;
- vq->vq.num_free = vring.num;
+ vq->vq.num_free = num;
vq->vq.index = index;
vq->we_own_ring = false;
vq->queue_dma_addr = 0;
@@ -984,9 +1181,8 @@ struct virtqueue *__vring_new_virtqueue(unsigned int index,
vq->weak_barriers = weak_barriers;
vq->broken = false;
vq->last_used_idx = 0;
- vq->avail_flags_shadow = 0;
- vq->avail_idx_shadow = 0;
vq->num_added = 0;
+ vq->packed = packed;
list_add_tail(&vq->vq.list, &vdev->vqs);
#ifdef DEBUG
vq->in_use = false;
@@ -997,18 +1193,37 @@ struct virtqueue *__vring_new_virtqueue(unsigned int index,
!context;
vq->event = virtio_has_feature(vdev, VIRTIO_RING_F_EVENT_IDX);
+ if (vq->packed) {
+ vq->vring_packed = vring.vring_packed;
+ vq->next_avail_idx = 0;
+ vq->wrap_counter = 1;
+ vq->event_flags_shadow = 0;
+ } else {
+ vq->vring = vring.vring_split;
+ vq->avail_flags_shadow = 0;
+ vq->avail_idx_shadow = 0;
+
+ /* Put everything in free lists. */
+ vq->free_head = 0;
+ for (i = 0; i < num-1; i++)
+ vq->vring.desc[i].next = cpu_to_virtio16(vdev, i + 1);
+ }
+
/* No callback? Tell other side not to bother us. */
if (!callback) {
- vq->avail_flags_shadow |= VRING_AVAIL_F_NO_INTERRUPT;
- if (!vq->event)
- vq->vring.avail->flags = cpu_to_virtio16(vdev, vq->avail_flags_shadow);
+ if (packed) {
+ vq->event_flags_shadow = VRING_EVENT_F_DISABLE;
+ vq->vring_packed.driver->flags = cpu_to_virtio16(vdev,
+ vq->event_flags_shadow);
+ } else {
+ vq->avail_flags_shadow |= VRING_AVAIL_F_NO_INTERRUPT;
+ if (!vq->event)
+ vq->vring.avail->flags = cpu_to_virtio16(vdev,
+ vq->avail_flags_shadow);
+ }
}
- /* Put everything in free lists. */
- vq->free_head = 0;
- for (i = 0; i < vring.num-1; i++)
- vq->vring.desc[i].next = cpu_to_virtio16(vdev, i + 1);
- memset(vq->desc_state, 0, vring.num * sizeof(struct vring_desc_state));
+ memset(vq->desc_state, 0, num * sizeof(struct vring_desc_state));
return &vq->vq;
}
@@ -1056,6 +1271,12 @@ static void vring_free_queue(struct virtio_device *vdev, size_t size,
}
}
+static inline int
+__vring_size(unsigned int num, unsigned long align, bool packed)
+{
+ return packed ? vring_size_packed(num, align) : vring_size(num, align);
+}
+
struct virtqueue *vring_create_virtqueue(
unsigned int index,
unsigned int num,
@@ -1072,7 +1293,8 @@ struct virtqueue *vring_create_virtqueue(
void *queue = NULL;
dma_addr_t dma_addr;
size_t queue_size_in_bytes;
- struct vring vring;
+ union vring_union vring;
+ bool packed;
/* We assume num is a power of 2. */
if (num & (num - 1)) {
@@ -1080,9 +1302,13 @@ struct virtqueue *vring_create_virtqueue(
return NULL;
}
+ packed = virtio_has_feature(vdev, VIRTIO_F_RING_PACKED);
+
/* TODO: allocate each queue chunk individually */
- for (; num && vring_size(num, vring_align) > PAGE_SIZE; num /= 2) {
- queue = vring_alloc_queue(vdev, vring_size(num, vring_align),
+ for (; num && __vring_size(num, vring_align, packed) > PAGE_SIZE;
+ num /= 2) {
+ queue = vring_alloc_queue(vdev, __vring_size(num, vring_align,
+ packed),
&dma_addr,
GFP_KERNEL|__GFP_NOWARN|__GFP_ZERO);
if (queue)
@@ -1094,17 +1320,21 @@ struct virtqueue *vring_create_virtqueue(
if (!queue) {
/* Try to get a single page. You are my only hope! */
- queue = vring_alloc_queue(vdev, vring_size(num, vring_align),
+ queue = vring_alloc_queue(vdev, __vring_size(num, vring_align,
+ packed),
&dma_addr, GFP_KERNEL|__GFP_ZERO);
}
if (!queue)
return NULL;
- queue_size_in_bytes = vring_size(num, vring_align);
- vring_init(&vring, num, queue, vring_align);
+ queue_size_in_bytes = __vring_size(num, vring_align, packed);
+ if (packed)
+ vring_init_packed(&vring.vring_packed, num, queue, vring_align);
+ else
+ vring_init(&vring.vring_split, num, queue, vring_align);
- vq = __vring_new_virtqueue(index, vring, vdev, weak_barriers, context,
- notify, callback, name);
+ vq = __vring_new_virtqueue(index, vring, packed, vdev, weak_barriers,
+ context, notify, callback, name);
if (!vq) {
vring_free_queue(vdev, queue_size_in_bytes, queue,
dma_addr);
@@ -1130,10 +1360,17 @@ struct virtqueue *vring_new_virtqueue(unsigned int index,
void (*callback)(struct virtqueue *vq),
const char *name)
{
- struct vring vring;
- vring_init(&vring, num, pages, vring_align);
- return __vring_new_virtqueue(index, vring, vdev, weak_barriers, context,
- notify, callback, name);
+ union vring_union vring;
+ bool packed;
+
+ packed = virtio_has_feature(vdev, VIRTIO_F_RING_PACKED);
+ if (packed)
+ vring_init_packed(&vring.vring_packed, num, pages, vring_align);
+ else
+ vring_init(&vring.vring_split, num, pages, vring_align);
+
+ return __vring_new_virtqueue(index, vring, packed, vdev, weak_barriers,
+ context, notify, callback, name);
}
EXPORT_SYMBOL_GPL(vring_new_virtqueue);
@@ -1143,7 +1380,9 @@ void vring_del_virtqueue(struct virtqueue *_vq)
if (vq->we_own_ring) {
vring_free_queue(vq->vq.vdev, vq->queue_size_in_bytes,
- vq->vring.desc, vq->queue_dma_addr);
+ vq->packed ? (void *)vq->vring_packed.desc :
+ (void *)vq->vring.desc,
+ vq->queue_dma_addr);
}
list_del(&_vq->list);
kfree(vq);
@@ -1185,7 +1424,7 @@ unsigned int virtqueue_get_vring_size(struct virtqueue *_vq)
struct vring_virtqueue *vq = to_vvq(_vq);
- return vq->vring.num;
+ return vq->packed ? vq->vring_packed.num : vq->vring.num;
}
EXPORT_SYMBOL_GPL(virtqueue_get_vring_size);
@@ -1228,6 +1467,10 @@ dma_addr_t virtqueue_get_avail_addr(struct virtqueue *_vq)
BUG_ON(!vq->we_own_ring);
+ if (vq->packed)
+ return vq->queue_dma_addr + ((char *)vq->vring_packed.driver -
+ (char *)vq->vring_packed.desc);
+
return vq->queue_dma_addr +
((char *)vq->vring.avail - (char *)vq->vring.desc);
}
@@ -1239,11 +1482,16 @@ dma_addr_t virtqueue_get_used_addr(struct virtqueue *_vq)
BUG_ON(!vq->we_own_ring);
+ if (vq->packed)
+ return vq->queue_dma_addr + ((char *)vq->vring_packed.device -
+ (char *)vq->vring_packed.desc);
+
return vq->queue_dma_addr +
((char *)vq->vring.used - (char *)vq->vring.desc);
}
EXPORT_SYMBOL_GPL(virtqueue_get_used_addr);
+/* Only available for split ring */
const struct vring *virtqueue_get_vring(struct virtqueue *vq)
{
return &to_vvq(vq)->vring;
diff --git a/include/linux/virtio_ring.h b/include/linux/virtio_ring.h
index bbf32524ab27..a0075894ad16 100644
--- a/include/linux/virtio_ring.h
+++ b/include/linux/virtio_ring.h
@@ -60,6 +60,11 @@ static inline void virtio_store_mb(bool weak_barriers,
struct virtio_device;
struct virtqueue;
+union vring_union {
+ struct vring vring_split;
+ struct vring_packed vring_packed;
+};
+
/*
* Creates a virtqueue and allocates the descriptor ring. If
* may_reduce_num is set, then this may allocate a smaller ring than
@@ -79,7 +84,8 @@ struct virtqueue *vring_create_virtqueue(unsigned int index,
/* Creates a virtqueue with a custom layout. */
struct virtqueue *__vring_new_virtqueue(unsigned int index,
- struct vring vring,
+ union vring_union vring,
+ bool packed,
struct virtio_device *vdev,
bool weak_barriers,
bool ctx,
--
2.17.0
^ permalink raw reply related
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