* Re: [patch net-next v4 21/21] rocker: Use logical operators on booleans
From: Scott Feldman @ 2014-11-27 21:04 UTC (permalink / raw)
To: Jiri Pirko
Cc: Netdev, David S. Miller, nhorman@tuxdriver.com, Andy Gospodarek,
Thomas Graf, dborkman@redhat.com, ogerlitz@mellanox.com,
jesse@nicira.com, pshelar@nicira.com, azhou@nicira.com,
ben@decadent.org.uk, stephen@networkplumber.org,
Kirsher, Jeffrey T, vyasevic@redhat.com, Cong Wang,
Fastabend, John R, Eric Dumazet, Jamal Hadi Salim,
Florian Fainelli, Roopa Prabhu, John Linville
In-Reply-To: <1417084826-9875-22-git-send-email-jiri@resnulli.us>
Signed-off-by: Scott Feldman <sfeldma@gmail.com>
On Thu, Nov 27, 2014 at 12:40 AM, Jiri Pirko <jiri@resnulli.us> wrote:
> From: Thomas Graf <tgraf@suug.ch>
>
> Silences various sparse warnings
>
> Signed-off-by: Thomas Graf <tgraf@suug.ch>
> Signed-off-by: Jiri Pirko <jiri@resnulli.us>
> ---
> new in v4
> ---
> drivers/net/ethernet/rocker/rocker.c | 16 ++++++++--------
> 1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/net/ethernet/rocker/rocker.c b/drivers/net/ethernet/rocker/rocker.c
> index 30687bf..fded127 100644
> --- a/drivers/net/ethernet/rocker/rocker.c
> +++ b/drivers/net/ethernet/rocker/rocker.c
> @@ -2404,17 +2404,17 @@ static int rocker_flow_tbl_bridge(struct rocker_port *rocker_port,
> }
>
> priority = ROCKER_PRIORITY_UNKNOWN;
> - if (vlan_bridging & dflt & wild)
> + if (vlan_bridging && dflt && wild)
> priority = ROCKER_PRIORITY_BRIDGING_VLAN_DFLT_WILD;
> - else if (vlan_bridging & dflt & !wild)
> + else if (vlan_bridging && dflt && !wild)
> priority = ROCKER_PRIORITY_BRIDGING_VLAN_DFLT_EXACT;
> - else if (vlan_bridging & !dflt)
> + else if (vlan_bridging && !dflt)
> priority = ROCKER_PRIORITY_BRIDGING_VLAN;
> - else if (!vlan_bridging & dflt & wild)
> + else if (!vlan_bridging && dflt && wild)
> priority = ROCKER_PRIORITY_BRIDGING_TENANT_DFLT_WILD;
> - else if (!vlan_bridging & dflt & !wild)
> + else if (!vlan_bridging && dflt && !wild)
> priority = ROCKER_PRIORITY_BRIDGING_TENANT_DFLT_EXACT;
> - else if (!vlan_bridging & !dflt)
> + else if (!vlan_bridging && !dflt)
> priority = ROCKER_PRIORITY_BRIDGING_TENANT;
>
> entry->key.priority = priority;
> @@ -3010,9 +3010,9 @@ static void rocker_port_fdb_learn_work(struct work_struct *work)
> bool removing = (lw->flags & ROCKER_OP_FLAG_REMOVE);
> bool learned = (lw->flags & ROCKER_OP_FLAG_LEARNED);
>
> - if (learned & removing)
> + if (learned && removing)
> br_fdb_external_learn_del(lw->dev, lw->addr, lw->vid);
> - else if (learned & !removing)
> + else if (learned && !removing)
> br_fdb_external_learn_add(lw->dev, lw->addr, lw->vid);
>
> kfree(work);
> --
> 1.9.3
>
^ permalink raw reply
* Re: [PATCH v3] can: Convert to runtime_pm
From: Marc Kleine-Budde @ 2014-11-27 21:17 UTC (permalink / raw)
To: Kedareswara rao Appana, wg, michal.simek, soren.brinkmann,
grant.likely, robh+dt
Cc: linux-can, netdev, linux-arm-kernel, linux-kernel, devicetree,
Kedareswara rao Appana
In-Reply-To: <6b6db31d46074512a322eae6e4ef64d4@BN1BFFO11FD038.protection.gbl>
[-- Attachment #1: Type: text/plain, Size: 9724 bytes --]
On 11/27/2014 02:08 PM, Kedareswara rao Appana wrote:
> Instead of enabling/disabling clocks at several locations in the driver,
> use the runtime_pm framework. This consolidates the actions for
> runtime PM in the appropriate callbacks and makes the driver more
> readable and mantainable.
>
> Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
> Signed-off-by: Kedareswara rao Appana <appanad@xilinx.com>
> ---
> Changes for v3:
> - Converted the driver to use runtime_pm.
> Changes for v2:
> - Removed the struct platform_device* from suspend/resume
> as suggest by Lothar.
>
> drivers/net/can/xilinx_can.c | 119 +++++++++++++++++++++++++----------------
> 1 files changed, 72 insertions(+), 47 deletions(-)
>
> diff --git a/drivers/net/can/xilinx_can.c b/drivers/net/can/xilinx_can.c
> index 8a998e3..1be28ed 100644
> --- a/drivers/net/can/xilinx_can.c
> +++ b/drivers/net/can/xilinx_can.c
> @@ -32,6 +32,7 @@
> #include <linux/can/dev.h>
> #include <linux/can/error.h>
> #include <linux/can/led.h>
> +#include <linux/pm_runtime.h>
>
> #define DRIVER_NAME "xilinx_can"
>
> @@ -138,7 +139,7 @@ struct xcan_priv {
> u32 (*read_reg)(const struct xcan_priv *priv, enum xcan_reg reg);
> void (*write_reg)(const struct xcan_priv *priv, enum xcan_reg reg,
> u32 val);
> - struct net_device *dev;
> + struct device *dev;
> void __iomem *reg_base;
> unsigned long irq_flags;
> struct clk *bus_clk;
> @@ -842,6 +843,13 @@ static int xcan_open(struct net_device *ndev)
> struct xcan_priv *priv = netdev_priv(ndev);
> int ret;
>
> + ret = pm_runtime_get_sync(priv->dev);
> + if (ret < 0) {
> + netdev_err(ndev, "%s: runtime CAN resume failed(%d)\n\r",
> + __func__, ret);
> + return ret;
> + }
> +
> ret = request_irq(ndev->irq, xcan_interrupt, priv->irq_flags,
> ndev->name, ndev);
> if (ret < 0) {
> @@ -849,29 +857,17 @@ static int xcan_open(struct net_device *ndev)
> goto err;
> }
>
> - ret = clk_prepare_enable(priv->can_clk);
> - if (ret) {
> - netdev_err(ndev, "unable to enable device clock\n");
> - goto err_irq;
> - }
> -
> - ret = clk_prepare_enable(priv->bus_clk);
> - if (ret) {
> - netdev_err(ndev, "unable to enable bus clock\n");
> - goto err_can_clk;
> - }
> -
> /* Set chip into reset mode */
> ret = set_reset_mode(ndev);
> if (ret < 0) {
> netdev_err(ndev, "mode resetting failed!\n");
> - goto err_bus_clk;
> + goto err_irq;
> }
>
> /* Common open */
> ret = open_candev(ndev);
> if (ret)
> - goto err_bus_clk;
> + goto err_irq;
>
> ret = xcan_chip_start(ndev);
> if (ret < 0) {
> @@ -887,13 +883,11 @@ static int xcan_open(struct net_device *ndev)
>
> err_candev:
> close_candev(ndev);
> -err_bus_clk:
> - clk_disable_unprepare(priv->bus_clk);
> -err_can_clk:
> - clk_disable_unprepare(priv->can_clk);
> err_irq:
> free_irq(ndev->irq, ndev);
> err:
> + pm_runtime_put(priv->dev);
> +
> return ret;
> }
>
> @@ -910,12 +904,11 @@ static int xcan_close(struct net_device *ndev)
> netif_stop_queue(ndev);
> napi_disable(&priv->napi);
> xcan_chip_stop(ndev);
> - clk_disable_unprepare(priv->bus_clk);
> - clk_disable_unprepare(priv->can_clk);
> free_irq(ndev->irq, ndev);
> close_candev(ndev);
>
> can_led_event(ndev, CAN_LED_EVENT_STOP);
> + pm_runtime_put(priv->dev);
>
> return 0;
> }
> @@ -934,27 +927,21 @@ static int xcan_get_berr_counter(const struct net_device *ndev,
> struct xcan_priv *priv = netdev_priv(ndev);
> int ret;
>
> - ret = clk_prepare_enable(priv->can_clk);
> - if (ret)
> - goto err;
> + ret = pm_runtime_get_sync(priv->dev);
> + if (ret < 0) {
> + netdev_err(ndev, "%s: runtime resume failed(%d)\n\r",
> + __func__, ret);
> + return ret;
> + }
>
> - ret = clk_prepare_enable(priv->bus_clk);
> - if (ret)
> - goto err_clk;
>
> bec->txerr = priv->read_reg(priv, XCAN_ECR_OFFSET) & XCAN_ECR_TEC_MASK;
> bec->rxerr = ((priv->read_reg(priv, XCAN_ECR_OFFSET) &
> XCAN_ECR_REC_MASK) >> XCAN_ESR_REC_SHIFT);
>
> - clk_disable_unprepare(priv->bus_clk);
> - clk_disable_unprepare(priv->can_clk);
> + pm_runtime_put(priv->dev);
>
> return 0;
> -
> -err_clk:
> - clk_disable_unprepare(priv->can_clk);
> -err:
> - return ret;
> }
>
>
> @@ -967,15 +954,45 @@ static const struct net_device_ops xcan_netdev_ops = {
>
> /**
> * xcan_suspend - Suspend method for the driver
> - * @dev: Address of the platform_device structure
> + * @dev: Address of the net_device structure
> *
> * Put the driver into low power mode.
> - * Return: 0 always
> + * Return: 0 on success and failure value on error
> */
> static int __maybe_unused xcan_suspend(struct device *dev)
> {
> - struct platform_device *pdev = dev_get_drvdata(dev);
> - struct net_device *ndev = platform_get_drvdata(pdev);
> + if (!device_may_wakeup(dev))
> + return pm_runtime_force_suspend(dev);
> +
> + return 0;
> +}
> +
> +/**
> + * xcan_resume - Resume from suspend
> + * @dev: Address of the net_device structure
> + *
> + * Resume operation after suspend.
> + * Return: 0 on success and failure value on error
> + */
> +static int __maybe_unused xcan_resume(struct device *dev)
> +{
> + if (!device_may_wakeup(dev))
> + return pm_runtime_force_resume(dev);
> +
> + return 0;
> +
> +}
> +
> +/**
> + * xcan_runtime_suspend - Runtime suspend method for the driver
> + * @dev: Address of the net_device structure
> + *
> + * Put the driver into low power mode.
> + * Return: 0 always
> + */
> +static int __maybe_unused xcan_runtime_suspend(struct device *dev)
> +{
> + struct net_device *ndev = dev_get_drvdata(dev);
> struct xcan_priv *priv = netdev_priv(ndev);
>
> if (netif_running(ndev)) {
> @@ -993,16 +1010,15 @@ static int __maybe_unused xcan_suspend(struct device *dev)
> }
>
> /**
> - * xcan_resume - Resume from suspend
> - * @dev: Address of the platformdevice structure
> + * xcan_runtime_resume - Runtime resume from suspend
> + * @dev: Address of the net_device structure
> *
> * Resume operation after suspend.
> * Return: 0 on success and failure value on error
> */
> -static int __maybe_unused xcan_resume(struct device *dev)
> +static int __maybe_unused xcan_runtime_resume(struct device *dev)
> {
> - struct platform_device *pdev = dev_get_drvdata(dev);
> - struct net_device *ndev = platform_get_drvdata(pdev);
> + struct net_device *ndev = dev_get_drvdata(dev);
> struct xcan_priv *priv = netdev_priv(ndev);
> int ret;
...adding the rest of the function to comment it:
> ret = clk_enable(priv->bus_clk);
> if (ret) {
> dev_err(dev, "Cannot enable clock.\n");
> return ret;
> }
> ret = clk_enable(priv->can_clk);
> if (ret) {
> dev_err(dev, "Cannot enable clock.\n");
> clk_disable_unprepare(priv->bus_clk);
> return ret;
> }
>
> priv->write_reg(priv, XCAN_MSR_OFFSET, 0);
> priv->write_reg(priv, XCAN_SRR_OFFSET, XCAN_SRR_CEN_MASK);
> priv->can.state = CAN_STATE_ERROR_ACTIVE;
Does it have any side effects, when writing these values when the CAN
device is not UP? (e.g. when running the berr callback?) Setting its
state to ERROR active is probably wrong. Do you have to move it into the
if(netif_running())?
> if (netif_running(ndev)) {
> netif_device_attach(ndev);
> netif_start_queue(ndev);
> }
>
> return 0;
> }
>
> @@ -1030,7 +1046,10 @@ static int __maybe_unused xcan_resume(struct device *dev)
> return 0;
> }
>
> -static SIMPLE_DEV_PM_OPS(xcan_dev_pm_ops, xcan_suspend, xcan_resume);
> +static const struct dev_pm_ops xcan_dev_pm_ops = {
> + SET_SYSTEM_SLEEP_PM_OPS(xcan_suspend, xcan_resume)
> + SET_PM_RUNTIME_PM_OPS(xcan_runtime_suspend, xcan_runtime_resume, NULL)
> +};
>
> /**
> * xcan_probe - Platform registration call
> @@ -1071,7 +1090,7 @@ static int xcan_probe(struct platform_device *pdev)
> return -ENOMEM;
>
> priv = netdev_priv(ndev);
> - priv->dev = ndev;
> + priv->dev = &pdev->dev;
> priv->can.bittiming_const = &xcan_bittiming_const;
> priv->can.do_set_mode = xcan_do_set_mode;
> priv->can.do_get_berr_counter = xcan_get_berr_counter;
> @@ -1137,6 +1156,11 @@ static int xcan_probe(struct platform_device *pdev)
>
> netif_napi_add(ndev, &priv->napi, xcan_rx_poll, rx_max);
>
> + pm_runtime_set_active(&pdev->dev);
> + pm_runtime_irq_safe(&pdev->dev);
You use just clock_enable()/disable() in the runtime functions, thus you
can say they are irq_safe. On the other the the zync grpio driver uses
"full" prepare_enable/disable_unprepare calls. What's best practice here?
> + pm_runtime_enable(&pdev->dev);
> + pm_runtime_get_sync(&pdev->dev);
> +
> ret = register_candev(ndev);
> if (ret) {
> dev_err(&pdev->dev, "fail to register failed (err=%d)\n", ret);
I think in case of an error, you have to call pm_runtime_put() here
> @@ -1144,8 +1168,9 @@ static int xcan_probe(struct platform_device *pdev)
> }
>
> devm_can_led_init(ndev);
> - clk_disable_unprepare(priv->bus_clk);
> - clk_disable_unprepare(priv->can_clk);
> +
> + pm_runtime_put(&pdev->dev);
> +
> netdev_dbg(ndev, "reg_base=0x%p irq=%d clock=%d, tx fifo depth:%d\n",
> priv->reg_base, ndev->irq, priv->can.clock.freq,
> priv->tx_max);
>
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions | Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH v7 6/8] net: can: c_can: Disable pins when CAN interface is down
From: Marc Kleine-Budde @ 2014-11-27 21:19 UTC (permalink / raw)
To: Linus Walleij, Roger Quadros
Cc: wg, Wolfram Sang, Tony Lindgren, Thomas Gleixner, Mugunthan V N,
George Cherian, Felipe Balbi, Sekhar Nori, Nishanth Menon,
Sergei Shtylyov, Linux-OMAP, linux-can, netdev@vger.kernel.org
In-Reply-To: <CACRpkdbVXPi7fNjGEzChayQ1KQg8h2R6sm0TNRsVOFJmAT=YMQ@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1852 bytes --]
On 11/27/2014 02:26 PM, Linus Walleij wrote:
> On Fri, Nov 14, 2014 at 4:40 PM, Roger Quadros <rogerq@ti.com> wrote:
>
>> DRA7 CAN IP suffers from a problem which causes it to be prevented
>> from fully turning OFF (i.e. stuck in transition) if the module was
>> disabled while there was traffic on the CAN_RX line.
>>
>> To work around this issue we select the SLEEP pin state by default
>> on probe and use the DEFAULT pin state on CAN up and back to the
>> SLEEP pin state on CAN down.
>>
>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Thanks, however the patch is already upstream.
>
> I see you figured it out all by yourselves :D
>
> (Sorry for being absent.)
>
>> +#include <linux/pinctrl/consumer.h>
>> + pinctrl_pm_select_default_state(dev->dev.parent);
>> + pinctrl_pm_select_sleep_state(dev->dev.parent);
>> + pinctrl_pm_select_sleep_state(dev->dev.parent);
>
> NB: in drivers/base/pinctrl.c:
>
> #ifdef CONFIG_PM
> /*
> * If power management is enabled, we also look for the optional
> * sleep and idle pin states, with semantics as defined in
> * <linux/pinctrl/pinctrl-state.h>
> */
> dev->pins->sleep_state = pinctrl_lookup_state(dev->pins->p,
> PINCTRL_STATE_SLEEP);
>
> So if these states are necessary for the driver to work, put
> depends on PM or select PM in the Kconfig.
Roger, you can prepare a patch, if needed.
Thanks,
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions | Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [patch net-next v3 02/17] net: make vid as a parameter for ndo_fdb_add/ndo_fdb_del
From: Jiri Pirko @ 2014-11-27 21:55 UTC (permalink / raw)
To: Scott Feldman
Cc: Jamal Hadi Salim, John Fastabend, Netdev, David S. Miller,
nhorman@tuxdriver.com, Andy Gospodarek, Thomas Graf,
dborkman@redhat.com, ogerlitz@mellanox.com, jesse@nicira.com,
pshelar@nicira.com, azhou@nicira.com, ben@decadent.org.uk,
stephen@networkplumber.org, Kirsher, Jeffrey T,
vyasevic@redhat.com, Cong Wang, Eric Dumazet, Florian Fainelli,
Roopa Prabhu, John Linville
In-Reply-To: <CAE4R7bDzxo1xY-MHzqtQ70s+nf+3dkWhr7neV-atk1YkMP3KJw@mail.gmail.com>
Thu, Nov 27, 2014 at 09:59:37PM CET, sfeldma@gmail.com wrote:
>Ya right now the driver just doesn't call br_fdb_external_learn_add()
>if LEARNING_SYNC is not set. It's a port driver setting so it seems
>fine to handle it in the port driver. You could move the check up to
>br_fdb_external_learn_add(), but then you have an extra call every 1s
>for each fdb entry being refreshed. (1s or whatever the refresh
>frequency is). Easier to avoid this overhead and make the decision at
>the source.
I have been thinking about moving the check into bridge code, it to make
it there as well as in drivers. This is easily changeable on demenad
later though, so I left this for now.
>
>-scott
>
>On Thu, Nov 27, 2014 at 2:14 AM, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>> On 11/27/14 01:50, Scott Feldman wrote:
>>
>> [..]
>>
>>>
>>> It's there: IFLA_BRPORT_LEARNING_SYNC. From iproute2:
>>>
>>> $ bridge -d link show dev swp1
>>> 2: swp1 state UNKNOWN : <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500
>>> master br0 state forwarding priority 32 cost 2
>>> hairpin off guard off root_block off fastleave off learning off flood
>>> off
>>> 2: swp1 state UNKNOWN : <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 master
>>> br0
>>> learning on learning_sync on hwmode swdev
>>>
>>> Turn it off:
>>>
>>> $ bridge link set dev swp1 hwmode swdev learning_sync off
>>>
>>> And now:
>>>
>>> $ bridge -d link show dev swp1
>>> 2: swp1 state UNKNOWN : <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500
>>> master br0 state forwarding priority 32 cost 2
>>> hairpin off guard off root_block off fastleave off learning off flood
>>> off
>>> 2: swp1 state UNKNOWN : <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 master
>>> br0
>>> learning on learning_sync off hwmode swdev
>>>
>>>
>>
>> Yes, this is the nice control portion.
>> From reviewing the patches, I didnt see how the core to the driver was
>> using the learning_sync. IOW, how do i turn off the drivers sync
>> from being activated? Maybe you are doing this in the rocker patches
>> which i didnt review? i think this needs to be core infrastructure i.e
>> if you are doing this in a timer (as opposed to interrupt driven), then
>> the core sync timer would kick in and call some driver ops.
>> In any case, details that can be ironed out later..
>>
>> cheers,
>> jamal
>>
^ permalink raw reply
* Re: [PATCH v3] can: Convert to runtime_pm
From: Sören Brinkmann @ 2014-11-27 22:47 UTC (permalink / raw)
To: Marc Kleine-Budde
Cc: Kedareswara rao Appana, wg, michal.simek, grant.likely, robh+dt,
linux-can, netdev, linux-arm-kernel, linux-kernel, devicetree,
Kedareswara rao Appana
In-Reply-To: <54779501.8060009@pengutronix.de>
On Thu, 2014-11-27 at 10:17PM +0100, Marc Kleine-Budde wrote:
> On 11/27/2014 02:08 PM, Kedareswara rao Appana wrote:
> > Instead of enabling/disabling clocks at several locations in the driver,
> > use the runtime_pm framework. This consolidates the actions for
> > runtime PM in the appropriate callbacks and makes the driver more
> > readable and mantainable.
> >
> > Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
> > Signed-off-by: Kedareswara rao Appana <appanad@xilinx.com>
> > ---
> > Changes for v3:
> > - Converted the driver to use runtime_pm.
> > Changes for v2:
> > - Removed the struct platform_device* from suspend/resume
> > as suggest by Lothar.
> >
> > drivers/net/can/xilinx_can.c | 119 +++++++++++++++++++++++++----------------
> > 1 files changed, 72 insertions(+), 47 deletions(-)
> >
> > diff --git a/drivers/net/can/xilinx_can.c b/drivers/net/can/xilinx_can.c
> > index 8a998e3..1be28ed 100644
> > --- a/drivers/net/can/xilinx_can.c
> > +++ b/drivers/net/can/xilinx_can.c
[...]
> > @@ -1030,7 +1046,10 @@ static int __maybe_unused xcan_resume(struct device *dev)
> > return 0;
> > }
> >
> > -static SIMPLE_DEV_PM_OPS(xcan_dev_pm_ops, xcan_suspend, xcan_resume);
> > +static const struct dev_pm_ops xcan_dev_pm_ops = {
> > + SET_SYSTEM_SLEEP_PM_OPS(xcan_suspend, xcan_resume)
> > + SET_PM_RUNTIME_PM_OPS(xcan_runtime_suspend, xcan_runtime_resume, NULL)
> > +};
> >
> > /**
> > * xcan_probe - Platform registration call
> > @@ -1071,7 +1090,7 @@ static int xcan_probe(struct platform_device *pdev)
> > return -ENOMEM;
> >
> > priv = netdev_priv(ndev);
> > - priv->dev = ndev;
> > + priv->dev = &pdev->dev;
> > priv->can.bittiming_const = &xcan_bittiming_const;
> > priv->can.do_set_mode = xcan_do_set_mode;
> > priv->can.do_get_berr_counter = xcan_get_berr_counter;
> > @@ -1137,6 +1156,11 @@ static int xcan_probe(struct platform_device *pdev)
> >
> > netif_napi_add(ndev, &priv->napi, xcan_rx_poll, rx_max);
> >
> > + pm_runtime_set_active(&pdev->dev);
> > + pm_runtime_irq_safe(&pdev->dev);
>
> You use just clock_enable()/disable() in the runtime functions, thus you
> can say they are irq_safe. On the other the the zync grpio driver uses
> "full" prepare_enable/disable_unprepare calls. What's best practice here?
IIRC, the prepare/unprepare functions can sleep. xcan_get_berr_counter
is called from atomic context. So, I think we have to use the
disable/enable functions without the prepare/unprepare.
In the GPIO driver the that problem does not exist.
Sören
^ permalink raw reply
* Re: [PATCH v3] can: Convert to runtime_pm
From: Marc Kleine-Budde @ 2014-11-27 22:55 UTC (permalink / raw)
To: Sören Brinkmann
Cc: Kedareswara rao Appana, wg, michal.simek, grant.likely, robh+dt,
linux-can, netdev, linux-arm-kernel, linux-kernel, devicetree,
Kedareswara rao Appana
In-Reply-To: <820358d5369f465188dfa9306eae5658@BL2FFO11FD009.protection.gbl>
[-- Attachment #1: Type: text/plain, Size: 3275 bytes --]
On 11/27/2014 11:47 PM, Sören Brinkmann wrote:
> On Thu, 2014-11-27 at 10:17PM +0100, Marc Kleine-Budde wrote:
>> On 11/27/2014 02:08 PM, Kedareswara rao Appana wrote:
>>> Instead of enabling/disabling clocks at several locations in the driver,
>>> use the runtime_pm framework. This consolidates the actions for
>>> runtime PM in the appropriate callbacks and makes the driver more
>>> readable and mantainable.
>>>
>>> Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
>>> Signed-off-by: Kedareswara rao Appana <appanad@xilinx.com>
>>> ---
>>> Changes for v3:
>>> - Converted the driver to use runtime_pm.
>>> Changes for v2:
>>> - Removed the struct platform_device* from suspend/resume
>>> as suggest by Lothar.
>>>
>>> drivers/net/can/xilinx_can.c | 119 +++++++++++++++++++++++++----------------
>>> 1 files changed, 72 insertions(+), 47 deletions(-)
>>>
>>> diff --git a/drivers/net/can/xilinx_can.c b/drivers/net/can/xilinx_can.c
>>> index 8a998e3..1be28ed 100644
>>> --- a/drivers/net/can/xilinx_can.c
>>> +++ b/drivers/net/can/xilinx_can.c
> [...]
>>> @@ -1030,7 +1046,10 @@ static int __maybe_unused xcan_resume(struct device *dev)
>>> return 0;
>>> }
>>>
>>> -static SIMPLE_DEV_PM_OPS(xcan_dev_pm_ops, xcan_suspend, xcan_resume);
>>> +static const struct dev_pm_ops xcan_dev_pm_ops = {
>>> + SET_SYSTEM_SLEEP_PM_OPS(xcan_suspend, xcan_resume)
>>> + SET_PM_RUNTIME_PM_OPS(xcan_runtime_suspend, xcan_runtime_resume, NULL)
>>> +};
>>>
>>> /**
>>> * xcan_probe - Platform registration call
>>> @@ -1071,7 +1090,7 @@ static int xcan_probe(struct platform_device *pdev)
>>> return -ENOMEM;
>>>
>>> priv = netdev_priv(ndev);
>>> - priv->dev = ndev;
>>> + priv->dev = &pdev->dev;
>>> priv->can.bittiming_const = &xcan_bittiming_const;
>>> priv->can.do_set_mode = xcan_do_set_mode;
>>> priv->can.do_get_berr_counter = xcan_get_berr_counter;
>>> @@ -1137,6 +1156,11 @@ static int xcan_probe(struct platform_device *pdev)
>>>
>>> netif_napi_add(ndev, &priv->napi, xcan_rx_poll, rx_max);
>>>
>>> + pm_runtime_set_active(&pdev->dev);
>>> + pm_runtime_irq_safe(&pdev->dev);
>>
>> You use just clock_enable()/disable() in the runtime functions, thus you
>> can say they are irq_safe. On the other the the zync grpio driver uses
>> "full" prepare_enable/disable_unprepare calls. What's best practice here?
>
> IIRC, the prepare/unprepare functions can sleep. xcan_get_berr_counter
> is called from atomic context. So, I think we have to use the
> disable/enable functions without the prepare/unprepare.
> In the GPIO driver the that problem does not exist.
IC, yes, correct.
This is why we introducted in other drivers a __get_berr_counter()
function, that doesn't touch the clocks, which is used from within the
driver (from the atomic contects), while get_berr_counter() will fiddle
with the clocks. This function is used for the
priv->can.do_get_berr_counter callback.
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions | Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH v3] can: Convert to runtime_pm
From: Sören Brinkmann @ 2014-11-27 23:46 UTC (permalink / raw)
To: Marc Kleine-Budde
Cc: Kedareswara rao Appana, wg, michal.simek, grant.likely, robh+dt,
linux-can, netdev, linux-arm-kernel, linux-kernel, devicetree,
Kedareswara rao Appana
In-Reply-To: <5477ABF3.1020605@pengutronix.de>
On Thu, 2014-11-27 at 11:55PM +0100, Marc Kleine-Budde wrote:
> On 11/27/2014 11:47 PM, Sören Brinkmann wrote:
> > On Thu, 2014-11-27 at 10:17PM +0100, Marc Kleine-Budde wrote:
> >> On 11/27/2014 02:08 PM, Kedareswara rao Appana wrote:
> >>> Instead of enabling/disabling clocks at several locations in the driver,
> >>> use the runtime_pm framework. This consolidates the actions for
> >>> runtime PM in the appropriate callbacks and makes the driver more
> >>> readable and mantainable.
> >>>
> >>> Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
> >>> Signed-off-by: Kedareswara rao Appana <appanad@xilinx.com>
> >>> ---
> >>> Changes for v3:
> >>> - Converted the driver to use runtime_pm.
> >>> Changes for v2:
> >>> - Removed the struct platform_device* from suspend/resume
> >>> as suggest by Lothar.
> >>>
> >>> drivers/net/can/xilinx_can.c | 119 +++++++++++++++++++++++++----------------
> >>> 1 files changed, 72 insertions(+), 47 deletions(-)
> >>>
> >>> diff --git a/drivers/net/can/xilinx_can.c b/drivers/net/can/xilinx_can.c
> >>> index 8a998e3..1be28ed 100644
> >>> --- a/drivers/net/can/xilinx_can.c
> >>> +++ b/drivers/net/can/xilinx_can.c
> > [...]
> >>> @@ -1030,7 +1046,10 @@ static int __maybe_unused xcan_resume(struct device *dev)
> >>> return 0;
> >>> }
> >>>
> >>> -static SIMPLE_DEV_PM_OPS(xcan_dev_pm_ops, xcan_suspend, xcan_resume);
> >>> +static const struct dev_pm_ops xcan_dev_pm_ops = {
> >>> + SET_SYSTEM_SLEEP_PM_OPS(xcan_suspend, xcan_resume)
> >>> + SET_PM_RUNTIME_PM_OPS(xcan_runtime_suspend, xcan_runtime_resume, NULL)
> >>> +};
> >>>
> >>> /**
> >>> * xcan_probe - Platform registration call
> >>> @@ -1071,7 +1090,7 @@ static int xcan_probe(struct platform_device *pdev)
> >>> return -ENOMEM;
> >>>
> >>> priv = netdev_priv(ndev);
> >>> - priv->dev = ndev;
> >>> + priv->dev = &pdev->dev;
> >>> priv->can.bittiming_const = &xcan_bittiming_const;
> >>> priv->can.do_set_mode = xcan_do_set_mode;
> >>> priv->can.do_get_berr_counter = xcan_get_berr_counter;
> >>> @@ -1137,6 +1156,11 @@ static int xcan_probe(struct platform_device *pdev)
> >>>
> >>> netif_napi_add(ndev, &priv->napi, xcan_rx_poll, rx_max);
> >>>
> >>> + pm_runtime_set_active(&pdev->dev);
> >>> + pm_runtime_irq_safe(&pdev->dev);
> >>
> >> You use just clock_enable()/disable() in the runtime functions, thus you
> >> can say they are irq_safe. On the other the the zync grpio driver uses
> >> "full" prepare_enable/disable_unprepare calls. What's best practice here?
> >
> > IIRC, the prepare/unprepare functions can sleep. xcan_get_berr_counter
> > is called from atomic context. So, I think we have to use the
> > disable/enable functions without the prepare/unprepare.
> > In the GPIO driver the that problem does not exist.
>
> IC, yes, correct.
>
> This is why we introducted in other drivers a __get_berr_counter()
> function, that doesn't touch the clocks, which is used from within the
> driver (from the atomic contects), while get_berr_counter() will fiddle
> with the clocks. This function is used for the
> priv->can.do_get_berr_counter callback.
I have the feeling I'm missing something. If I remove the 'must not
sleep' requirement from the runtime suspend/resume functions, I get
this:
BUG: sleeping function called from invalid context at drivers/base/power/runtime.c:954
in_atomic(): 0, irqs_disabled(): 0, pid: 161, name: ip
INFO: lockdep is turned off.
CPU: 0 PID: 161 Comm: ip Not tainted 3.18.0-rc1-xilinx-00059-g21da26693b61-dirty #104
[<c00186a8>] (unwind_backtrace) from [<c00139f4>] (show_stack+0x20/0x24)
[<c00139f4>] (show_stack) from [<c055a41c>] (dump_stack+0x8c/0xd0)
[<c055a41c>] (dump_stack) from [<c0054808>] (__might_sleep+0x1ac/0x1e4)
[<c0054808>] (__might_sleep) from [<c034f8f0>] (__pm_runtime_resume+0x40/0x9c)
[<c034f8f0>] (__pm_runtime_resume) from [<c03b48d8>] (xcan_get_berr_counter+0x2c/0x9c)
[<c03b48d8>] (xcan_get_berr_counter) from [<c03b2ecc>] (can_fill_info+0x160/0x1f4)
[<c03b2ecc>] (can_fill_info) from [<c049f3b0>] (rtnl_fill_ifinfo+0x794/0x970)
[<c049f3b0>] (rtnl_fill_ifinfo) from [<c04a0048>] (rtnl_dump_ifinfo+0x1b4/0x2fc)
[<c04a0048>] (rtnl_dump_ifinfo) from [<c04af9c8>] (netlink_dump+0xe4/0x270)
[<c04af9c8>] (netlink_dump) from [<c04b0764>] (__netlink_dump_start+0xdc/0x170)
[<c04b0764>] (__netlink_dump_start) from [<c04a1fc4>] (rtnetlink_rcv_msg+0x154/0x1e0)
[<c04a1fc4>] (rtnetlink_rcv_msg) from [<c04b1e88>] (netlink_rcv_skb+0x68/0xc4)
[<c04b1e88>] (netlink_rcv_skb) from [<c04a045c>] (rtnetlink_rcv+0x28/0x34)
[<c04a045c>] (rtnetlink_rcv) from [<c04b1770>] (netlink_unicast+0x144/0x210)
[<c04b1770>] (netlink_unicast) from [<c04b1c9c>] (netlink_sendmsg+0x394/0x414)
[<c04b1c9c>] (netlink_sendmsg) from [<c046ffcc>] (sock_sendmsg+0x8c/0xc0)
[<c046ffcc>] (sock_sendmsg) from [<c04726bc>] (SyS_sendto+0xd8/0x114)
[<c04726bc>] (SyS_sendto) from [<c000f3e0>] (ret_fast_syscall+0x0/0x48)
I.e. the core calls this function from atomic context. And in an earlier
thread you said the core can also call this before/after calling the
open/close callbacks (which applies here too, I think).
I think the callback is required to
- not sleep
- get the device in a power state that allows querying its registers
So, I don't see how splitting the xcan_get_berr_counter callback helps
here, especially since it is not even used within the driver.
Thanks,
Sören
^ permalink raw reply
* Possible memory leak in function (wil_write_file_wmi) not freeing pointer (wmi) on error path
From: Ahmed Tamrawi @ 2014-11-27 23:46 UTC (permalink / raw)
To: netdev
Bug Report: https://bugzilla.kernel.org/show_bug.cgi?id=88971
Linux Version [3.17-rc1]
Configuration: Default configuration for x86
Function (wil_write_file_wmi) in file
(drivers/net/wireless/ath/wil6210/debugfs.c) allocates pointer (wmi)
on line (536) and passes it as a parameter to function
(simple_write_to_buffer) at line (540). Function
(simple_write_to_buffer) returns on line (539) with a negative value.
This causes function (wil_write_file_wmi) to return on line (542)
without freeing pointer (wmi). Thus, causing a possible memory leak
not freeing pointer (wmi).
(wil_write_file_wmi) source code reference:
http://lxr.free-electrons.com/source/drivers/net/wireless/ath/wil6210/debugfs.c#L523
(simple_write_to_buffer) source code reference:
http://lxr.free-electrons.com/source/fs/libfs.c#L625
~Ahmed
^ permalink raw reply
* Re: [PATCH rfc] packet: zerocopy packet_snd
From: Willem de Bruijn @ 2014-11-28 0:32 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Network Development, David Miller, Eric Dumazet, Daniel Borkmann
In-Reply-To: <20141127072717.GA8644@redhat.com>
On Thu, Nov 27, 2014 at 2:27 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Wed, Nov 26, 2014 at 06:05:16PM -0500, Willem de Bruijn wrote:
>> On Wed, Nov 26, 2014 at 4:20 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>> > On Wed, Nov 26, 2014 at 02:59:34PM -0500, Willem de Bruijn wrote:
>> >> > The main problem with zero copy ATM is with queueing disciplines
>> >> > which might keep the socket around essentially forever.
>> >> > The case was described here:
>> >> > https://lkml.org/lkml/2014/1/17/105
>> >> > and of course this will make it more serious now that
>> >> > more applications will be able to do this, so
>> >> > chances that an administrator enables this
>> >> > are higher.
>> >>
>> >> The denial of service issue raised there, that a single queue can
>> >> block an entire virtio-net device, is less problematic in the case of
>> >> packet sockets. A socket can run out of sk_wmem_alloc, but a prudent
>> >> application can increase the limit or use separate sockets for
>> >> separate flows.
>> >
>> > Sounds like this interface is very hard to use correctly.
>>
>> Actually, this socket alloc issue is the same for zerocopy and
>> non-zerocopy. Packets can be held in deep queues at which point
>> the packet socket is blocked. This is accepted behavior.
>>
>> >From the above thread:
>>
>> "It's ok for non-zerocopy packet to be blocked since VM1 thought the
>> packets has been sent instead of pending in the virtqueue. So VM1 can
>> still send packet to other destination."
>>
>> This is very specific to virtio and vhost-net. I don't think that that
>> concern applies to a packet interface.
>
> Well, you are obviously building the interface with some use-case in mind.
> Let's try to make it work for multiple use-cases.
>
> So at some level, you are right. The issue is not running out of wmem.
> But I think I'm right too - this is hard to use correctly.
>
> I think the difference is that with your patch, application
> can't reuse the memory until packet is transmitted, otherwise junk goes
> out on the network.
The packet headers are in linear skb memory, so the integrity
of the kernel is not affected. But payload (and derived data,
like checksums) may be junk.
> Even closing the socket won't help.
> Is this true?
Good point. Indeed. Your approach in the follow up email,
to let the process release its mappings, sounds like a good
answer.
In general, I think that leaving this under process control is
preferable over kernel guarantees using a timer, because it
is it is more predictable from a process point of view. Both
the state of the system at any point in time, and the per-packet
cycle cost are easier to reason about (were packet sent out
with zero-copy, or were deep copies triggered by a timer? this
question would be difficult to answer otherwise, and it is
important, as deep copies are likely more expensive).
>
> I see this as a problem.
>
> I'm trying to figure out how would one use this interface, one obvious
> use would be to tunnel out raw packets directly from VM memory.
> For this application, a zero copy packet never completing is a problem:
> at minimum, you want to be able to remove the device, which
> translates to a requirement that closing the socket effectively stops
> using userspace memory.
The application can ensure this by releasing the relevant
pages. For mmap()ed pages, munmap() is sufficient. The approach
you mentioned with madvise may be more widely applicable. I had
so far only targeted mmapped (incl. MAP_HUGETLB) pages.
Alternatively, the kernel could enforce this, by keeping a reference
on all in-flight ubuf_info structs associated with the sk. Though this
adds non trivial accounting overhead and potentially massive
copying at socket close.
If outstanding ubuf_info are tracked, then the forced copy can
be triggered by other actions, as well, such as in response to a
sendmsg cmsg or setsockopt.
Because there is per-call cost associated with maintaining
kernel state, but not necessarily with userspace strategies,
I would choose to leave this responsibility to the user. That
is already the behavior with vmsplice gifting, I think.
> In case you want to be able to run e.g. a watchdog,
> ability to specify a deadline also seems benefitial.
>> Another issue, though, is that the method currently really only helps
>> TSO because ll other paths cause a deep copy. There are more use
>> cases once it can send up to 64KB MTU over loopback or send out
>> GSO datagrams without triggering skb_copy_ubufs. I have not looked
>> into how (or if) that can be achieved yet.
>
> I think this was done intentionally at some point,
> try to look at git history to find out the reasons.
>
>> >
>> >> > One possible solution is some kind of timer orphaning frags
>> >> > for skbs that have been around for too long.
>> >>
>> >> Perhaps this can be approximated without an explicit timer by calling
>> >> skb_copy_ubufs on enqueue whenever qlen exceeds a threshold value?
>> >
>> > Not sure. I'll have to see that patch to judge.
^ permalink raw reply
* Re: [PATCH rfc] packet: zerocopy packet_snd
From: Willem de Bruijn @ 2014-11-28 0:39 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Jason Wang, Network Development, David Miller, Eric Dumazet,
Daniel Borkmann
In-Reply-To: <20141127104445.GA8961@redhat.com>
On Thu, Nov 27, 2014 at 5:44 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Thu, Nov 27, 2014 at 09:18:12AM +0008, Jason Wang wrote:
>>
>>
>> On Thu, Nov 27, 2014 at 5:17 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
>> >On Wed, Nov 26, 2014 at 02:59:34PM -0500, Willem de Bruijn wrote:
>> >> > The main problem with zero copy ATM is with queueing disciplines
>> >> > which might keep the socket around essentially forever.
>> >> > The case was described here:
>> >> > https://lkml.org/lkml/2014/1/17/105
>> >> > and of course this will make it more serious now that
>> >> > more applications will be able to do this, so
>> >> > chances that an administrator enables this
>> >> > are higher.
>> >> The denial of service issue raised there, that a single queue can
>> >> block an entire virtio-net device, is less problematic in the case of
>> >> packet sockets. A socket can run out of sk_wmem_alloc, but a prudent
>> >> application can increase the limit or use separate sockets for
>> >> separate flows.
>> >
>> >Socket per flow? Maybe just use TCP then? increasing the limit
>> >sounds like a wrong solution, it hurts security.
>> >
>> >> > One possible solution is some kind of timer orphaning frags
>> >> > for skbs that have been around for too long.
>> >> Perhaps this can be approximated without an explicit timer by calling
>> >> skb_copy_ubufs on enqueue whenever qlen exceeds a threshold value?
>> >
>> >Hard to say. Will have to see that patch to judge how robust this is.
>>
>> This could not work, consider if the threshold is greater than vring size
>> or vhost_net pending limit, transmission may still be blocked.
>
>
> Well, application can e.g. just switch to non zero copy after
> reaching a specific number of requests.
> I think the real problem isn't reaching the queue full
> condition, it's the fact a specific buffer might never
> get freed. This API isn't half as useful as it could be
> if applications had a way to force the memory
> to be reclaimed.
>
>
> And actually, I see a way for applications to reclaim the memory:
> application could invoke something like MADV_SOFT_OFFLINE on the memory
> submitted for zero copy transmit, to invalidate PTEs, and make next
> access fault new pages in.
> If dedicated memory is used for packets, you could even use
> MADV_DONTNEED - but this doesn't work in many cases, certainly
> not for virtualization type workloads.
Why not?
>
>
> Playting with PTEs needs to invalidate the TLB so it is not fast,
> but it does not need to be: we are talking about ability to close the
> socket, which should be rare.
>
> For example, an application/hypervisor can detect a timeout when a
> packet is not transmitted within a predefined time period, and trigger
> such reclaim.
> Making this period shorter than network watchdog timer of the VM
> will ensure that watchdog does not trigger within VM.
> Alternatively, VM network watchdog could trigger this reclaim
> in order to recover packet memory.
>
> With this idea, if application merely reads memory, we incur a lot of
> overhead with pagefaults. So maybe a new call to enable COW for a range
> of pages would be a good idea.
>
>
> We'd have to make sure whatever's used for reclaim works for
> a wide range of memory types: mmap-ed file, hugetlbfs, anonymous memory.
>
>
> Thoughts?
Very nice. When exactly are these madvise hints preferable over munmap?
> --
> MST
^ permalink raw reply
* [PATCH v2 net] sh_eth: Fix sleeping function called from invalid context
From: Simon Horman @ 2014-11-28 1:04 UTC (permalink / raw)
To: David S. Miller, netdev, linux-sh
Cc: linux-arm-kernel, Magnus Damm, Sergei Shtylyov, Yoshihiro Kaneko
From: Mitsuhiro Kimura <mitsuhiro.kimura.kc@renesas.com>
This resolves the following bug which can be reproduced by building the
kernel with CONFIG_DEBUG_ATOMIC_SLEEP=y and reading network statistics
while the network interface is down.
e.g.:
ifconfig eth0 down
cat /sys/class/net/eth0/statistics/tx_errors
----
[ 1238.161349] BUG: sleeping function called from invalid context at drivers/base/power/runtime.c:952
[ 1238.188279] in_atomic(): 1, irqs_disabled(): 0, pid: 1388, name: cat
[ 1238.207425] CPU: 0 PID: 1388 Comm: cat Not tainted 3.10.31-ltsi-00046-gefa0b46 #1087
[ 1238.230737] Backtrace:
[ 1238.238123] [<c0012e64>] (dump_backtrace+0x0/0x10c) from [<c0013000>] (show_stack+0x18/0x1c)
[ 1238.263499] r6:000003b8 r5:c06160c0 r4:c0669e00 r3:00404000
[ 1238.280583] [<c0012fe8>] (show_stack+0x0/0x1c) from [<c04515a4>] (dump_stack+0x20/0x28)
[ 1238.304631] [<c0451584>] (dump_stack+0x0/0x28) from [<c004970c>] (__might_sleep+0xf8/0x118)
[ 1238.329734] [<c0049614>] (__might_sleep+0x0/0x118) from [<c02465ac>] (__pm_runtime_resume+0x38/0x90)
[ 1238.357170] r7:d616f000 r6:c049c458 r5:00000004 r4:d6a17210
[ 1238.374251] [<c0246574>] (__pm_runtime_resume+0x0/0x90) from [<c029b1c4>] (sh_eth_get_stats+0x44/0x280)
[ 1238.402468] r7:d616f000 r6:c049c458 r5:d5c21000 r4:d5c21000
[ 1238.419552] [<c029b180>] (sh_eth_get_stats+0x0/0x280) from [<c03ae39c>] (dev_get_stats+0x54/0x88)
[ 1238.446204] r5:d5c21000 r4:d5ed7e08
[ 1238.456980] [<c03ae348>] (dev_get_stats+0x0/0x88) from [<c03c677c>] (netstat_show.isra.15+0x54/0x9c)
[ 1238.484413] r6:d5c21000 r5:d5c21238 r4:00000028 r3:00000001
[ 1238.501495] [<c03c6728>] (netstat_show.isra.15+0x0/0x9c) from [<c03c69b8>] (show_tx_errors+0x18/0x1c)
[ 1238.529196] r7:d5f945d8 r6:d5f945c0 r5:c049716c r4:c0650e7c
[ 1238.546279] [<c03c69a0>] (show_tx_errors+0x0/0x1c) from [<c023963c>] (dev_attr_show+0x24/0x50)
[ 1238.572157] [<c0239618>] (dev_attr_show+0x0/0x50) from [<c010c148>] (sysfs_read_file+0xb0/0x140)
[ 1238.598554] r5:c049716c r4:d5c21240
[ 1238.609326] [<c010c098>] (sysfs_read_file+0x0/0x140) from [<c00b9ee4>] (vfs_read+0xb0/0x13c)
[ 1238.634679] [<c00b9e34>] (vfs_read+0x0/0x13c) from [<c00ba0ac>] (SyS_read+0x44/0x74)
[ 1238.657944] r8:bef45bf0 r7:00000000 r6:d6ac0600 r5:00000000 r4:00000000
[ 1238.678172] [<c00ba068>] (SyS_read+0x0/0x74) from [<c000eec0>] (ret_fast_syscall+0x0/0x30)
----
Signed-off-by: Mitsuhiro Kimura <mitsuhiro.kimura.kc@renesas.com>
Signed-off-by: Yoshihiro Kaneko <ykaneko0929@gmail.com>
Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
---
v2 [Simon Horman]
* Enhance changelog to describe how the problem may be reproduced.
* Use bitfield rather than boolean for is_opened field of
struct sh_eth_private. As suggested by Sergei Shtylyov.
---
drivers/net/ethernet/renesas/sh_eth.c | 64 +++++++++++++++++++----------------
drivers/net/ethernet/renesas/sh_eth.h | 1 +
2 files changed, 36 insertions(+), 29 deletions(-)
diff --git a/drivers/net/ethernet/renesas/sh_eth.c b/drivers/net/ethernet/renesas/sh_eth.c
index 60e9c2c..862a691 100644
--- a/drivers/net/ethernet/renesas/sh_eth.c
+++ b/drivers/net/ethernet/renesas/sh_eth.c
@@ -2042,6 +2042,8 @@ static int sh_eth_open(struct net_device *ndev)
if (ret)
goto out_free_irq;
+ mdp->is_opened = 1;
+
return ret;
out_free_irq:
@@ -2131,6 +2133,36 @@ static int sh_eth_start_xmit(struct sk_buff *skb, struct net_device *ndev)
return NETDEV_TX_OK;
}
+static struct net_device_stats *sh_eth_get_stats(struct net_device *ndev)
+{
+ struct sh_eth_private *mdp = netdev_priv(ndev);
+
+ if (sh_eth_is_rz_fast_ether(mdp))
+ return &ndev->stats;
+
+ if (!mdp->is_opened)
+ return &ndev->stats;
+
+ ndev->stats.tx_dropped += sh_eth_read(ndev, TROCR);
+ sh_eth_write(ndev, 0, TROCR); /* (write clear) */
+ ndev->stats.collisions += sh_eth_read(ndev, CDCR);
+ sh_eth_write(ndev, 0, CDCR); /* (write clear) */
+ ndev->stats.tx_carrier_errors += sh_eth_read(ndev, LCCR);
+ sh_eth_write(ndev, 0, LCCR); /* (write clear) */
+
+ if (sh_eth_is_gether(mdp)) {
+ ndev->stats.tx_carrier_errors += sh_eth_read(ndev, CERCR);
+ sh_eth_write(ndev, 0, CERCR); /* (write clear) */
+ ndev->stats.tx_carrier_errors += sh_eth_read(ndev, CEECR);
+ sh_eth_write(ndev, 0, CEECR); /* (write clear) */
+ } else {
+ ndev->stats.tx_carrier_errors += sh_eth_read(ndev, CNDCR);
+ sh_eth_write(ndev, 0, CNDCR); /* (write clear) */
+ }
+
+ return &ndev->stats;
+}
+
/* device close function */
static int sh_eth_close(struct net_device *ndev)
{
@@ -2145,6 +2177,7 @@ static int sh_eth_close(struct net_device *ndev)
sh_eth_write(ndev, 0, EDTRR);
sh_eth_write(ndev, 0, EDRRR);
+ sh_eth_get_stats(ndev);
/* PHY Disconnect */
if (mdp->phydev) {
phy_stop(mdp->phydev);
@@ -2163,36 +2196,9 @@ static int sh_eth_close(struct net_device *ndev)
pm_runtime_put_sync(&mdp->pdev->dev);
- return 0;
-}
-
-static struct net_device_stats *sh_eth_get_stats(struct net_device *ndev)
-{
- struct sh_eth_private *mdp = netdev_priv(ndev);
-
- if (sh_eth_is_rz_fast_ether(mdp))
- return &ndev->stats;
-
- pm_runtime_get_sync(&mdp->pdev->dev);
-
- ndev->stats.tx_dropped += sh_eth_read(ndev, TROCR);
- sh_eth_write(ndev, 0, TROCR); /* (write clear) */
- ndev->stats.collisions += sh_eth_read(ndev, CDCR);
- sh_eth_write(ndev, 0, CDCR); /* (write clear) */
- ndev->stats.tx_carrier_errors += sh_eth_read(ndev, LCCR);
- sh_eth_write(ndev, 0, LCCR); /* (write clear) */
- if (sh_eth_is_gether(mdp)) {
- ndev->stats.tx_carrier_errors += sh_eth_read(ndev, CERCR);
- sh_eth_write(ndev, 0, CERCR); /* (write clear) */
- ndev->stats.tx_carrier_errors += sh_eth_read(ndev, CEECR);
- sh_eth_write(ndev, 0, CEECR); /* (write clear) */
- } else {
- ndev->stats.tx_carrier_errors += sh_eth_read(ndev, CNDCR);
- sh_eth_write(ndev, 0, CNDCR); /* (write clear) */
- }
- pm_runtime_put_sync(&mdp->pdev->dev);
+ mdp->is_opened = 0;
- return &ndev->stats;
+ return 0;
}
/* ioctl to device function */
diff --git a/drivers/net/ethernet/renesas/sh_eth.h b/drivers/net/ethernet/renesas/sh_eth.h
index b37c427..81fcab3 100644
--- a/drivers/net/ethernet/renesas/sh_eth.h
+++ b/drivers/net/ethernet/renesas/sh_eth.h
@@ -522,6 +522,7 @@ struct sh_eth_private {
unsigned no_ether_link:1;
unsigned ether_link_active_low:1;
+ unsigned is_opened:1;
};
static inline void sh_eth_soft_swap(char *src, int len)
--
2.1.3
^ permalink raw reply related
* RE: [PATCH v3] can: Convert to runtime_pm
From: Appana Durga Kedareswara Rao @ 2014-11-28 1:47 UTC (permalink / raw)
To: Michal Simek, wg@grandegger.com, mkl@pengutronix.de, Michal Simek,
Soren Brinkmann, grant.likely@linaro.org, robh+dt@kernel.org
Cc: linux-can@vger.kernel.org, netdev@vger.kernel.org,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, devicetree@vger.kernel.org
In-Reply-To: <54773948.2070605@xilinx.com>
Hi Michal,
-----Original Message-----
From: Michal Simek [mailto:michal.simek@xilinx.com]
Sent: Thursday, November 27, 2014 8:17 PM
To: Appana Durga Kedareswara Rao; wg@grandegger.com; mkl@pengutronix.de; Michal Simek; Soren Brinkmann; grant.likely@linaro.org; robh+dt@kernel.org
Cc: linux-can@vger.kernel.org; netdev@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org; devicetree@vger.kernel.org; Appana Durga Kedareswara Rao
Subject: Re: [PATCH v3] can: Convert to runtime_pm
On 11/27/2014 02:08 PM, Kedareswara rao Appana wrote:
> Instead of enabling/disabling clocks at several locations in the
> driver, use the runtime_pm framework. This consolidates the actions
> for runtime PM in the appropriate callbacks and makes the driver more
> readable and mantainable.
>
> Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
> Signed-off-by: Kedareswara rao Appana <appanad@xilinx.com>
> ---
> Changes for v3:
> - Converted the driver to use runtime_pm.
> Changes for v2:
> - Removed the struct platform_device* from suspend/resume
> as suggest by Lothar.
>
> drivers/net/can/xilinx_can.c | 119
> +++++++++++++++++++++++++----------------
> 1 files changed, 72 insertions(+), 47 deletions(-)
>
> diff --git a/drivers/net/can/xilinx_can.c
> b/drivers/net/can/xilinx_can.c index 8a998e3..1be28ed 100644
> --- a/drivers/net/can/xilinx_can.c
> +++ b/drivers/net/can/xilinx_can.c
> @@ -32,6 +32,7 @@
> #include <linux/can/dev.h>
> #include <linux/can/error.h>
> #include <linux/can/led.h>
> +#include <linux/pm_runtime.h>
>
> #define DRIVER_NAME "xilinx_can"
>
> @@ -138,7 +139,7 @@ struct xcan_priv {
> u32 (*read_reg)(const struct xcan_priv *priv, enum xcan_reg reg);
> void (*write_reg)(const struct xcan_priv *priv, enum xcan_reg reg,
> u32 val);
> - struct net_device *dev;
> + struct device *dev;
> void __iomem *reg_base;
> unsigned long irq_flags;
> struct clk *bus_clk;
> @@ -842,6 +843,13 @@ static int xcan_open(struct net_device *ndev)
> struct xcan_priv *priv = netdev_priv(ndev);
> int ret;
>
> + ret = pm_runtime_get_sync(priv->dev);
> + if (ret < 0) {
> + netdev_err(ndev, "%s: runtime CAN resume failed(%d)\n\r",
> + __func__, ret);
> + return ret;
> + }
> +
> ret = request_irq(ndev->irq, xcan_interrupt, priv->irq_flags,
> ndev->name, ndev);
> if (ret < 0) {
> @@ -849,29 +857,17 @@ static int xcan_open(struct net_device *ndev)
> goto err;
> }
>
> - ret = clk_prepare_enable(priv->can_clk);
> - if (ret) {
> - netdev_err(ndev, "unable to enable device clock\n");
> - goto err_irq;
> - }
> -
> - ret = clk_prepare_enable(priv->bus_clk);
> - if (ret) {
> - netdev_err(ndev, "unable to enable bus clock\n");
> - goto err_can_clk;
> - }
> -
> /* Set chip into reset mode */
> ret = set_reset_mode(ndev);
> if (ret < 0) {
> netdev_err(ndev, "mode resetting failed!\n");
> - goto err_bus_clk;
> + goto err_irq;
> }
>
> /* Common open */
> ret = open_candev(ndev);
> if (ret)
> - goto err_bus_clk;
> + goto err_irq;
>
> ret = xcan_chip_start(ndev);
> if (ret < 0) {
> @@ -887,13 +883,11 @@ static int xcan_open(struct net_device *ndev)
>
> err_candev:
> close_candev(ndev);
> -err_bus_clk:
> - clk_disable_unprepare(priv->bus_clk);
> -err_can_clk:
> - clk_disable_unprepare(priv->can_clk);
> err_irq:
> free_irq(ndev->irq, ndev);
> err:
> + pm_runtime_put(priv->dev);
> +
> return ret;
> }
>
> @@ -910,12 +904,11 @@ static int xcan_close(struct net_device *ndev)
> netif_stop_queue(ndev);
> napi_disable(&priv->napi);
> xcan_chip_stop(ndev);
> - clk_disable_unprepare(priv->bus_clk);
> - clk_disable_unprepare(priv->can_clk);
> free_irq(ndev->irq, ndev);
> close_candev(ndev);
>
> can_led_event(ndev, CAN_LED_EVENT_STOP);
> + pm_runtime_put(priv->dev);
>
> return 0;
> }
> @@ -934,27 +927,21 @@ static int xcan_get_berr_counter(const struct net_device *ndev,
> struct xcan_priv *priv = netdev_priv(ndev);
> int ret;
>
> - ret = clk_prepare_enable(priv->can_clk);
> - if (ret)
> - goto err;
> + ret = pm_runtime_get_sync(priv->dev);
> + if (ret < 0) {
> + netdev_err(ndev, "%s: runtime resume failed(%d)\n\r",
> + __func__, ret);
> + return ret;
> + }
>
> - ret = clk_prepare_enable(priv->bus_clk);
> - if (ret)
> - goto err_clk;
>
> bec->txerr = priv->read_reg(priv, XCAN_ECR_OFFSET) & XCAN_ECR_TEC_MASK;
> bec->rxerr = ((priv->read_reg(priv, XCAN_ECR_OFFSET) &
> XCAN_ECR_REC_MASK) >> XCAN_ESR_REC_SHIFT);
>
> - clk_disable_unprepare(priv->bus_clk);
> - clk_disable_unprepare(priv->can_clk);
> + pm_runtime_put(priv->dev);
>
> return 0;
> -
> -err_clk:
> - clk_disable_unprepare(priv->can_clk);
> -err:
> - return ret;
> }
>
>
> @@ -967,15 +954,45 @@ static const struct net_device_ops
> xcan_netdev_ops = {
>
> /**
> * xcan_suspend - Suspend method for the driver
> - * @dev: Address of the platform_device structure
> + * @dev: Address of the net_device structure
This is just the device structure.
K will modify in the next version of the patch .
> *
> * Put the driver into low power mode.
> - * Return: 0 always
> + * Return: 0 on success and failure value on error
> */
> static int __maybe_unused xcan_suspend(struct device *dev) {
> - struct platform_device *pdev = dev_get_drvdata(dev);
> - struct net_device *ndev = platform_get_drvdata(pdev);
> + if (!device_may_wakeup(dev))
> + return pm_runtime_force_suspend(dev);
> +
> + return 0;
> +}
> +
> +/**
> + * xcan_resume - Resume from suspend
> + * @dev: Address of the net_device structure
ditto
K will modify in the next version of the patch .
> + *
> + * Resume operation after suspend.
> + * Return: 0 on success and failure value on error */ static int
> +__maybe_unused xcan_resume(struct device *dev) {
> + if (!device_may_wakeup(dev))
> + return pm_runtime_force_resume(dev);
> +
> + return 0;
> +
> +}
> +
> +/**
> + * xcan_runtime_suspend - Runtime suspend method for the driver
> + * @dev: Address of the net_device structure
ditto.
K will modify in the next version of the patch .
> + *
> + * Put the driver into low power mode.
> + * Return: 0 always
> + */
> +static int __maybe_unused xcan_runtime_suspend(struct device *dev) {
> + struct net_device *ndev = dev_get_drvdata(dev);
> struct xcan_priv *priv = netdev_priv(ndev);
>
> if (netif_running(ndev)) {
> @@ -993,16 +1010,15 @@ static int __maybe_unused xcan_suspend(struct
> device *dev) }
>
> /**
> - * xcan_resume - Resume from suspend
> - * @dev: Address of the platformdevice structure
> + * xcan_runtime_resume - Runtime resume from suspend
> + * @dev: Address of the net_device structure
ditto.
K will modify in the next version of the patch .
Regards,
Kedar.
Thanks,
Michal
This email and any attachments are intended for the sole use of the named recipient(s) and contain(s) confidential information that may be proprietary, privileged or copyrighted under applicable law. If you are not the intended recipient, do not read, copy, or forward this email message or any attachments. Delete this email message and any attachments immediately.
^ permalink raw reply
* [PATCH] wil6210: Fix potential memory leaks on error paths
From: Lino Sanfilippo @ 2014-11-28 1:47 UTC (permalink / raw)
To: qca_vkondrat; +Cc: netdev, ahmedtamrawi, davem, Lino Sanfilippo
Fix missing memory deallocation on error paths in wil_write_file_wmi()
and wil_write_file_txmgmt().
Reported-by: Ahmed Tamrawi <ahmedtamrawi@gmail.com>
Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de>
---
drivers/net/wireless/ath/wil6210/debugfs.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/net/wireless/ath/wil6210/debugfs.c b/drivers/net/wireless/ath/wil6210/debugfs.c
index 54a6ddc..4e6e145 100644
--- a/drivers/net/wireless/ath/wil6210/debugfs.c
+++ b/drivers/net/wireless/ath/wil6210/debugfs.c
@@ -573,8 +573,10 @@ static ssize_t wil_write_file_txmgmt(struct file *file, const char __user *buf,
if (!frame)
return -ENOMEM;
- if (copy_from_user(frame, buf, len))
+ if (copy_from_user(frame, buf, len)) {
+ kfree(frame);
return -EIO;
+ }
params.buf = frame;
params.len = len;
@@ -614,8 +616,10 @@ static ssize_t wil_write_file_wmi(struct file *file, const char __user *buf,
return -ENOMEM;
rc = simple_write_to_buffer(wmi, len, ppos, buf, len);
- if (rc < 0)
+ if (rc < 0) {
+ kfree(wmi);
return rc;
+ }
cmd = &wmi[1];
cmdid = le16_to_cpu(wmi->id);
--
1.9.1
^ permalink raw reply related
* RE: [PATCH v3] can: Convert to runtime_pm
From: Appana Durga Kedareswara Rao @ 2014-11-28 1:49 UTC (permalink / raw)
To: Soren Brinkmann
Cc: wg@grandegger.com, mkl@pengutronix.de, Michal Simek,
grant.likely@linaro.org, robh+dt@kernel.org,
devicetree@vger.kernel.org, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org, linux-can@vger.kernel.org,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <20141127182337.GF17884@xsjandreislx>
Hi Soren,
-----Original Message-----
From: Sören Brinkmann [mailto:soren.brinkmann@xilinx.com]
Sent: Thursday, November 27, 2014 11:54 PM
To: Appana Durga Kedareswara Rao
Cc: wg@grandegger.com; mkl@pengutronix.de; Michal Simek; grant.likely@linaro.org; robh+dt@kernel.org; devicetree@vger.kernel.org; netdev@vger.kernel.org; linux-kernel@vger.kernel.org; linux-can@vger.kernel.org; Appana Durga Kedareswara Rao; linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v3] can: Convert to runtime_pm
Hi Kedar,
On Thu, 2014-11-27 at 06:38PM +0530, Kedareswara rao Appana wrote:
> Instead of enabling/disabling clocks at several locations in the
> driver, use the runtime_pm framework. This consolidates the actions
> for runtime PM in the appropriate callbacks and makes the driver more
> readable and mantainable.
>
> Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
> Signed-off-by: Kedareswara rao Appana <appanad@xilinx.com>
> ---
> Changes for v3:
> - Converted the driver to use runtime_pm.
> Changes for v2:
> - Removed the struct platform_device* from suspend/resume
> as suggest by Lothar.
>
> drivers/net/can/xilinx_can.c | 119
> +++++++++++++++++++++++++----------------
> 1 files changed, 72 insertions(+), 47 deletions(-)
>
> diff --git a/drivers/net/can/xilinx_can.c
> b/drivers/net/can/xilinx_can.c index 8a998e3..1be28ed 100644
> --- a/drivers/net/can/xilinx_can.c
> +++ b/drivers/net/can/xilinx_can.c
> @@ -32,6 +32,7 @@
> #include <linux/can/dev.h>
> #include <linux/can/error.h>
> #include <linux/can/led.h>
> +#include <linux/pm_runtime.h>
>
> #define DRIVER_NAME "xilinx_can"
>
> @@ -138,7 +139,7 @@ struct xcan_priv {
> u32 (*read_reg)(const struct xcan_priv *priv, enum xcan_reg reg);
> void (*write_reg)(const struct xcan_priv *priv, enum xcan_reg reg,
> u32 val);
> - struct net_device *dev;
> + struct device *dev;
> void __iomem *reg_base;
> unsigned long irq_flags;
> struct clk *bus_clk;
> @@ -842,6 +843,13 @@ static int xcan_open(struct net_device *ndev)
> struct xcan_priv *priv = netdev_priv(ndev);
> int ret;
>
> + ret = pm_runtime_get_sync(priv->dev);
> + if (ret < 0) {
> + netdev_err(ndev, "%s: runtime CAN resume failed(%d)\n\r",
There might be other issues than the resume that make this fail. It should probably just say 'pm_runtime_get failed'.
The CAN in the string should not be needed, the netdev_err macro makes sure the device name is printed.
Can we have a space between 'failed' and the error code?
There should not be a '\r'
K sure will modify the error message as you explained above.
> + __func__, ret);
> + return ret;
> + }
> +
> ret = request_irq(ndev->irq, xcan_interrupt, priv->irq_flags,
> ndev->name, ndev);
> if (ret < 0) {
> @@ -849,29 +857,17 @@ static int xcan_open(struct net_device *ndev)
> goto err;
> }
>
> - ret = clk_prepare_enable(priv->can_clk);
> - if (ret) {
> - netdev_err(ndev, "unable to enable device clock\n");
> - goto err_irq;
> - }
> -
> - ret = clk_prepare_enable(priv->bus_clk);
> - if (ret) {
> - netdev_err(ndev, "unable to enable bus clock\n");
> - goto err_can_clk;
> - }
> -
> /* Set chip into reset mode */
> ret = set_reset_mode(ndev);
> if (ret < 0) {
> netdev_err(ndev, "mode resetting failed!\n");
> - goto err_bus_clk;
> + goto err_irq;
> }
>
> /* Common open */
> ret = open_candev(ndev);
> if (ret)
> - goto err_bus_clk;
> + goto err_irq;
>
> ret = xcan_chip_start(ndev);
> if (ret < 0) {
> @@ -887,13 +883,11 @@ static int xcan_open(struct net_device *ndev)
>
> err_candev:
> close_candev(ndev);
> -err_bus_clk:
> - clk_disable_unprepare(priv->bus_clk);
> -err_can_clk:
> - clk_disable_unprepare(priv->can_clk);
> err_irq:
> free_irq(ndev->irq, ndev);
> err:
> + pm_runtime_put(priv->dev);
> +
> return ret;
> }
>
> @@ -910,12 +904,11 @@ static int xcan_close(struct net_device *ndev)
> netif_stop_queue(ndev);
> napi_disable(&priv->napi);
> xcan_chip_stop(ndev);
> - clk_disable_unprepare(priv->bus_clk);
> - clk_disable_unprepare(priv->can_clk);
> free_irq(ndev->irq, ndev);
> close_candev(ndev);
>
> can_led_event(ndev, CAN_LED_EVENT_STOP);
> + pm_runtime_put(priv->dev);
>
> return 0;
> }
> @@ -934,27 +927,21 @@ static int xcan_get_berr_counter(const struct net_device *ndev,
> struct xcan_priv *priv = netdev_priv(ndev);
> int ret;
>
> - ret = clk_prepare_enable(priv->can_clk);
> - if (ret)
> - goto err;
> + ret = pm_runtime_get_sync(priv->dev);
> + if (ret < 0) {
> + netdev_err(ndev, "%s: runtime resume failed(%d)\n\r",
> + __func__, ret);
As above.
K will modify this too.
Regards,
Kedar.
Sören
This email and any attachments are intended for the sole use of the named recipient(s) and contain(s) confidential information that may be proprietary, privileged or copyrighted under applicable law. If you are not the intended recipient, do not read, copy, or forward this email message or any attachments. Delete this email message and any attachments immediately.
^ permalink raw reply
* Re: 3.12.33 Bug with ipvs
From: Smart Weblications GmbH - Florian Wiessner @ 2014-11-28 2:02 UTC (permalink / raw)
To: Julian Anastasov; +Cc: netdev
In-Reply-To: <alpine.LFD.2.11.1411270945240.7068@ja.home.ssi.bg>
Hi,
Am 27.11.2014 09:08, schrieb Julian Anastasov:
>
> Hello,
>
> On Wed, 26 Nov 2014, Smart Weblications GmbH - Florian Wiessner wrote:
>
>> Hi netdev,
>>
>> On 3.12.33 i see this every 3 hours or so on a box with ip_vs running with a
>> setup which made no problems on 3.10.40. Could someone give me hints how to
>> debug this? It seems to happen instantly, when i add ip_vs_ftp and have some nat
>> rules. Setup is like this:
>>
>
>> [13230.431740] RIP [<ffffffff814ff2fc>] xfrm_selector_match+0x25/0x2f6
>> [13230.431772] RSP <ffff88083fd83a68>
>> [13230.431795] CR2: 00000000000600d0
>> [13230.432240] ---[ end trace 103912aa204977dc ]---
>>
>> node01:/ocfs2/usr/src/linux-3.12.33/scripts# ./decodecode </tmp/oops.log
>> [13230.431464] Code: 5d 41 5e 41 5f c3 41 55 66 83 fa 02 41 54 55 48 89 fd 53 48
>> 89 f3 41 50 74 11 31 c0 66 83 fa 0a 0f 85 ce 02 00 00 e9 fd 00 00 00 <0f> b6 47
>> 2a 8b 17 8b 76 18 84 c0 74 1a b9 20 00 00 00 31 f2 29
>> All code
>> ========
>> 0: 5d pop %rbp
>> 1: 41 5e pop %r14
>> 3: 41 5f pop %r15
>> 5: c3 retq
>> 6: 41 55 push %r13
>> 8: 66 83 fa 02 cmp $0x2,%dx
>> c: 41 54 push %r12
>> e: 55 push %rbp
>> f: 48 89 fd mov %rdi,%rbp
>> 12: 53 push %rbx
>> 13: 48 89 f3 mov %rsi,%rbx
>> 16: 41 50 push %r8
>> 18: 74 11 je 0x2b
>> 1a: 31 c0 xor %eax,%eax
>> 1c: 66 83 fa 0a cmp $0xa,%dx
>> 20: 0f 85 ce 02 00 00 jne 0x2f4
>> 26: e9 fd 00 00 00 jmpq 0x128
>> 2b:* 0f b6 47 2a movzbl 0x2a(%rdi),%eax <-- trapping
>> instruction
>
> Above instruction is 'sel->prefixlen_d' from
> the addr4_match call in __xfrm4_selector_match. Looks like
> we dereference sel (%rdi) with bad value of 00000000000600a6.
> xfrm_sk_policy_lookup() provides &pol->selector to
> xfrm_selector_match, so pol has a bad value. I don't remember
> for such problem, not sure if the 3-hour period is some timer
> in xfrm.
>
In fact it could be timer related:
1st. try
[13061.933733] IP: [<ffffffff8154f5a3>] xfrm_selector_match+0x25/0x2f6
[13061.934440] RIP: 0010:[<ffffffff8154f5a3>] [<ffffffff8154f5a3>]
xfrm_selector_match+0x25/0x2f6
[13061.936477] RIP [<ffffffff8154f5a3>] xfrm_selector_match+0x25/0x2f6
2nd. try
[13230.422541] IP: [<ffffffff814ff2fc>] xfrm_selector_match+0x25/0x2f6
[13230.423440] RIP: 0010:[<ffffffff814ff2fc>] [<ffffffff814ff2fc>]
xfrm_selector_match+0x25/0x2f6
[13230.431740] RIP [<ffffffff814ff2fc>] xfrm_selector_match+0x25/0x2f6
>> Could someone shed some light on the decoded output and point me somewhere so i
>> can debug this further?
>
> If noone else has idea what can be wrong, can you try
> some kernels between 3.10.40 and 3.12.33 or even some lastest
> kernel?
>
I tried 3.17.4 which seems not have this issue any more, but has another
regression in ocfs2 which is why we cannot use it.
3.10.61 looks fine so far, but i cannot tell for sure, uptime is 1:23 right now,
i'll keep you updated.
--
Mit freundlichen Grüßen,
Florian Wiessner
Smart Weblications GmbH
Martinsberger Str. 1
D-95119 Naila
fon.: +49 9282 9638 200
fax.: +49 9282 9638 205
24/7: +49 900 144 000 00 - 0,99 EUR/Min*
http://www.smart-weblications.de
--
Sitz der Gesellschaft: Naila
Geschäftsführer: Florian Wiessner
HRB-Nr.: HRB 3840 Amtsgericht Hof
*aus dem dt. Festnetz, ggf. abweichende Preise aus dem Mobilfunknetz
^ permalink raw reply
* Charity Work
From: luv2charitys @ 2014-11-28 2:20 UTC (permalink / raw)
To: netdev
Hello,this is Mr Paul N,i sent you an email on charity work but i am yet to hear fom you,do reply with this code CHA-2015 to my email address paulcharity@qq.com i Look forward to hearing from you this time,God bless Brother Paul
^ permalink raw reply
* Re: Multiple DSA switch on shared MII
From: Florian Fainelli @ 2014-11-28 12:21 UTC (permalink / raw)
To: Rajib Karmakar; +Cc: netdev
In-Reply-To: <CAOi_9k9h4N-uPf7shapytV+rCtHFWiVco26hCVjTMN=00o8KAg@mail.gmail.com>
Le 26/11/2014 23:18, Rajib Karmakar a écrit :
> This could be a naive question; but can I add all LAN and WAN
> interfaces in a single DSA switch (one cpu port, one netdev)?
One DSA switch is backed by a single netdev, I would rather register two
dsa switches, even if they actually map to the same physical switch,
rather than hacking around a single instance to work with multiple
network devices.
Thanks
--
Florian
^ permalink raw reply
* Re: [PATCH][v3] mdio-mux-gpio: Use GPIO descriptor interface and new gpiod_set_array function
From: Alexandre Courbot @ 2014-11-28 4:30 UTC (permalink / raw)
To: Rojhalat Ibrahim
Cc: linux-gpio@vger.kernel.org, netdev, Linus Walleij,
Alexandre Courbot, David Miller, Florian Fainelli, David Daney
In-Reply-To: <1581158.UhESICdgeM@pcimr>
On Tue, Nov 25, 2014 at 10:57 PM, Rojhalat Ibrahim <imr@rtschenk.de> wrote:
> Convert mdio-mux-gpio to the GPIO descriptor interface and use the new
> gpiod_set_array function to set all output signals simultaneously.
>
> Signed-off-by: Rojhalat Ibrahim <imr@rtschenk.de>
> Acked-by: David S. Miller <davem@davemloft.net>
> --
> This patch depends on the gpiod_set_array function, which is available in
> the linux-gpio devel tree.
>
> Change log:
> v3: remove unnecessary braces
> v2: fix gpiod_get_index usage
>
> drivers/net/phy/mdio-mux-gpio.c | 37 +++++++++++++------------------------
> 1 file changed, 13 insertions(+), 24 deletions(-)
Looks all good to me! Nice use of this new interface, leading to
simpler client code.
Reviewed-by: Alexandre Courbot <acourbot@nvidia.com>
^ permalink raw reply
* Re: ipv4: Use standard iovec primitive in raw_probe_proto_opt
From: Al Viro @ 2014-11-28 5:14 UTC (permalink / raw)
To: Jon Maloy
Cc: Herbert Xu, David Miller, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org, bcrl@kvack.org, Masahide Nakamura,
Hideaki YOSHIFUJI
In-Reply-To: <20141106221608.GA7996@ZenIV.linux.org.uk>
On Thu, Nov 06, 2014 at 10:16:08PM +0000, Al Viro wrote:
> On Thu, Nov 06, 2014 at 09:55:31AM +0000, Jon Maloy wrote:
> > > Point, but that might very well be a pattern to watch for - there's at least one
> > > more instance in TIPC (also not exploitable, according to TIPC folks) and such
> >
> > I don't recall this, and I can't see where it would be either. Can you please
> > point to where it is?
>
> The same dest_name_check() thing. This
> if (copy_from_user(&hdr, m->msg_iov[0].iov_base, sizeof(hdr)))
> return -EFAULT;
> if ((ntohs(hdr.tcm_type) & 0xC000) && (!capable(CAP_NET_ADMIN)))
> return -EACCES;
> is easily bypassed. Suppose you want to send a packet with these two
> bits in ->tcm_type not being 00, and you don't have CAP_NET_ADMIN.
> Not a problem - spawn two threads sharing memory, have one trying to
> call sendmsg() while another keeps flipping these two bits. Sooner
> of later you'll get the timing right and have these bits observed as 00
> in dest_name_check() and 11 when it comes to memcpy_fromiovecend() actually
> copying the whole thing. And considering that the interval between those
> two is much longer than the loop in the second thread would take on
> each iteration, I'd expect the odds around 25% per attempted sendmsg().
>
> IOW, this test is either pointless and can be removed completely, or there's
> an exploitable race. As far as I understand from your replies both back then
> and in another branch of this thread, it's the former and the proper fix is
> to remove at least that part of dest_name_check(). So this case is also
> not something exploitable, but it certainly matches the same pattern.
>
> My point was simply that this pattern is worth watching for - recurrent bug
> classes like that have a good chance to spawn an instance that will be
> exploitable.
Ping? Can we simply remove dest_name_check() completely? That's one of the
few remaining obstacles to making ->sendmsg() iov_iter-clean. For now I'm
simply commenting its call out in tipc_sendmsg(); if it _is_ needed for
anything, we'll need to get rid of that double copying from userland. I can
do that, but my impression from your comments back in April is that you
planned to removed the damn check anyway.
Another question: in tipc_send_stream() we have
mtu = tsk->max_pkt;
send = min_t(uint, dsz - sent, TIPC_MAX_USER_MSG_SIZE);
__skb_queue_head_init(&head);
rc = tipc_msg_build(mhdr, m, sent, send, mtu, &head);
if (unlikely(rc < 0))
goto exit;
do {
if (likely(!tsk_conn_cong(tsk))) {
rc = tipc_link_xmit(&head, dnode, ref);
if (likely(!rc)) {
tsk->sent_unacked++;
sent += send;
if (sent == dsz)
break;
goto next;
}
if (rc == -EMSGSIZE) {
tsk->max_pkt = tipc_node_get_mtu(dnode, ref);
goto next;
}
How can it hit that EMSGSIZE? AFAICS, it can come only from
int __tipc_link_xmit(struct tipc_link *link, struct sk_buff_head *list)
{
struct tipc_msg *msg = buf_msg(skb_peek(list));
uint psz = msg_size(msg);
...
uint mtu = link->max_pkt;
...
/* Has valid packet limit been used ? */
if (unlikely(psz > mtu)) {
__skb_queue_purge(list);
return -EMSGSIZE;
}
and msg_size() is basically the bits copied into skb by tipc_msg_build() and
set by msg_set_size() in there. And unless I'm seriously misreading that
function, it can't be more than pktmax argument, i.e. mtu. So unless something
manages to crap into our skb or change mtu right under us, it shouldn't be
possible. And mtu (i.e. ->max_pkt) ought to be protected by lock_sock() there.
What's going on there?
^ permalink raw reply
* Re: [PATCH net] bpf: x86: fix epilogue generation for eBPF programs
From: Alexei Starovoitov @ 2014-11-28 5:55 UTC (permalink / raw)
To: Daniel Borkmann
Cc: David S. Miller, Zi Shen Lim, Eric Dumazet, H. Peter Anvin,
Thomas Gleixner, Ingo Molnar, Network Development, LKML
In-Reply-To: <5476F471.80500@redhat.com>
On Thu, Nov 27, 2014 at 1:52 AM, Daniel Borkmann <dborkman@redhat.com> wrote:
> On 11/27/2014 06:02 AM, Alexei Starovoitov wrote:
>>
>> classic BPF has a restriction that last insn is always BPF_RET.
>> eBPF doesn't have BPF_RET instruction and this restriction.
>> It has BPF_EXIT insn which can appear anywhere in the program
>> one or more times and it doesn't have to be last insn.
>> Fix eBPF JIT to emit epilogue when first BPF_EXIT is seen
>> and all other BPF_EXIT instructions will be emitted as jump.
>>
>> Signed-off-by: Alexei Starovoitov <ast@plumgrid.com>
>> ---
>> Note, this bug is applicable only to native eBPF programs
>> which first were introduced in 3.18, so no need to send it
>> to stable and therefore no 'Fixes' tag.
>
>
> Btw, even if it's not sent to -stable, a 'Fixes:' tag is useful
> information for backporting and regression tracking, preferably
> always mentioned where it can clearly be identified.
Well I didn't mention it, as I said, because I don't think it
needs backporting. Otherwise with the tag the tools might
pick it up automatically? Just a guess.
Anyway:
Fixes: 622582786c9e ("net: filter: x86: internal BPF JIT")
>> arm64 JIT has the same problem, but the fix is not as trivial,
>> so will be done as separate patch.
>>
>> Since 3.18 can only load eBPF programs and cannot execute them,
>> this patch can even be done in net-next only, but I think it's worth
>> to apply it to 3.18(net), so that JITed output for native eBPF
>> programs is correct when bpf syscall loads it with
>> net.core.bpf_jit_enable=2
>
>
> Yes, sounds good to me, the condition insn_cnt - 1 is still held
> with BPF to eBPF transformations.
Correct. That's what I meant that prior to 3.18 it's not needed.
and 'insn_cnt - 1' condition will still hold for classic in the future.
>> arch/x86/net/bpf_jit_comp.c | 6 ++++--
>> 1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
>> index 3f62734..7e90244 100644
>> --- a/arch/x86/net/bpf_jit_comp.c
>> +++ b/arch/x86/net/bpf_jit_comp.c
>> @@ -178,7 +178,7 @@ static void jit_fill_hole(void *area, unsigned int
>> size)
>> }
>>
>> struct jit_context {
>> - unsigned int cleanup_addr; /* epilogue code offset */
>> + int cleanup_addr; /* epilogue code offset */
>
>
> Why this type change here? This seems a bit out of context (I would
> have expected a mention of this in the commit message, otherwise).
Ok. Will respin with updated commit msg.
The reason for signed is the following:
jmp offset to epilogue is computed as:
jmp_offset = ctx->cleanup_addr - addrs[i]
when cleanup_addr was always last insn it wasn't a problem,
since result of subtraction was positive.
Now, since epilogue will be in the middle of JITed
code the jmps to epilogue may be negative, so
signed int is need to do the math correctly.
In other words, it should be:
(long long) ((int)20 - (int)30)
instead of:
(long long) ((unsigned int)20 - (int)30)
^ permalink raw reply
* Re: Is this 32-bit NCM?
From: Enrico Mioso @ 2014-11-28 5:59 UTC (permalink / raw)
To: Alex Strizhevsky
Cc: Bjørn Mork, ShaojunMidge.Tan-6C2+4RG2qWF0ubjbjo6WXg,
Mingying.Zhu-6C2+4RG2qWF0ubjbjo6WXg,
youtux-Re5JQEeQqe8AvxtiuMwx3w, linux-usb-u79uwXL29TY76Z2rM5mHXA,
netdev-u79uwXL29TY76Z2rM5mHXA,
Eli.Britstein-6C2+4RG2qWF0ubjbjo6WXg
In-Reply-To: <CAPChA0cLuvRpYTYuFoi3bewqASgf_oZHSz5yQQ8JYm96dZ4-TQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
Hello to everyone.
Can you guys try to compare what happens withe standard, and 32-bit version of
the driver, with the E3276, so that we see what happens and what are the
differences?
And it would be very nice if you look what happens with the standard 16-bit
driver to confirm what Bjorn suspected regarding the alignment problems: this
morning I will take a lookt at the align_tail function (it's called something
like that) in cdc_ncm.c (standard version).
Bjorn and all: do you think it's ok if I keep you all + mailing lists in CC or
would you like me to do something different? Please comment on this if any
change is required.
Taking a shower right now,
Enrico
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH rfc] packet: zerocopy packet_snd
From: Jason Wang @ 2014-11-28 6:21 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Willem de Bruijn, Network Development, David Miller, Eric Dumazet,
Daniel Borkmann
In-Reply-To: <20141127104445.GA8961@redhat.com>
On Thu, Nov 27, 2014 at 6:44 PM, Michael S. Tsirkin <mst@redhat.com>
wrote:
> On Thu, Nov 27, 2014 at 09:18:12AM +0008, Jason Wang wrote:
>>
>>
>> On Thu, Nov 27, 2014 at 5:17 AM, Michael S. Tsirkin
>> <mst@redhat.com> wrote:
>> >On Wed, Nov 26, 2014 at 02:59:34PM -0500, Willem de Bruijn wrote:
>> >> > The main problem with zero copy ATM is with queueing
>> disciplines
>> >> > which might keep the socket around essentially forever.
>> >> > The case was described here:
>> >> > https://lkml.org/lkml/2014/1/17/105
>> >> > and of course this will make it more serious now that
>> >> > more applications will be able to do this, so
>> >> > chances that an administrator enables this
>> >> > are higher.
>> >> The denial of service issue raised there, that a single queue can
>> >> block an entire virtio-net device, is less problematic in the
>> case of
>> >> packet sockets. A socket can run out of sk_wmem_alloc, but a
>> prudent
>> >> application can increase the limit or use separate sockets for
>> >> separate flows.
>> >
>> >Socket per flow? Maybe just use TCP then? increasing the limit
>> >sounds like a wrong solution, it hurts security.
>> >
>> >> > One possible solution is some kind of timer orphaning frags
>> >> > for skbs that have been around for too long.
>> >> Perhaps this can be approximated without an explicit timer by
>> calling
>> >> skb_copy_ubufs on enqueue whenever qlen exceeds a threshold
>> value?
>> >
>> >Hard to say. Will have to see that patch to judge how robust this
>> is.
>>
>> This could not work, consider if the threshold is greater than
>> vring size
>> or vhost_net pending limit, transmission may still be blocked.
>
> Well, application can e.g. just switch to non zero copy after
> reaching a specific number of requests.
Yes but only works if user are ok for out of order completion.
>
> I think the real problem isn't reaching the queue full
> condition, it's the fact a specific buffer might never
> get freed. This API isn't half as useful as it could be
> if applications had a way to force the memory
> to be reclaimed.
>
Agree.
>
> And actually, I see a way for applications to reclaim the memory:
> application could invoke something like MADV_SOFT_OFFLINE on the
> memory
> submitted for zero copy transmit, to invalidate PTEs, and make next
> access fault new pages in.
> If dedicated memory is used for packets, you could even use
> MADV_DONTNEED - but this doesn't work in many cases, certainly
> not for virtualization type workloads.
>
> Playting with PTEs needs to invalidate the TLB so it is not fast,
> but it does not need to be: we are talking about ability to close the
> socket, which should be rare.
>
> For example, an application/hypervisor can detect a timeout when a
> packet is not transmitted within a predefined time period, and trigger
> such reclaim.
> Making this period shorter than network watchdog timer of the VM
> will ensure that watchdog does not trigger within VM.
> Alternatively, VM network watchdog could trigger this reclaim
> in order to recover packet memory.
Doing such in hypervisor seems better. It could reduce the possible
guest triggered behavior.
But this just can fix the transmission stuck in guest, host socket
still need to wait for the packet to be sent by host?
>
>
> With this idea, if application merely reads memory, we incur a lot of
> overhead with pagefaults. So maybe a new call to enable COW for a
> range
> of pages would be a good idea.
>
Not very clear, doesn't COW still depends on pagefault to work?
>
> We'd have to make sure whatever's used for reclaim works for
> a wide range of memory types: mmap-ed file, hugetlbfs, anonymous
> memory.
>
>
> Thoughts?
>
> --
> MST
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" 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: Is this 32-bit NCM?
From: Enrico Mioso @ 2014-11-28 6:23 UTC (permalink / raw)
To: Alex Strizhevsky
Cc: Bjørn Mork, ShaojunMidge.Tan, Mingying.Zhu, youtux,
linux-usb, netdev, Eli.Britstein
In-Reply-To: <CAPChA0cLuvRpYTYuFoi3bewqASgf_oZHSz5yQQ8JYm96dZ4-TQ@mail.gmail.com>
And... a precisation: when I talk about the 32-bit mode driver, I am not
referring to the windows one, but to the Linux one I and Alessio "implemented",
modifying the standard cdc_ncm.c driver.
This version of the driver should be available here:
On Thu, 27 Nov 2014, Alex Strizhevsky wrote:
http://www.gstorm.eu/cdc_ncm.c
And you might compile it with a kernel >= 3.16.
Overmore, if this driver is able to work with E3272, you might compare it with
the Windows driver in E3272 and then compare differences when working with
E3276.
Please Kevin, Midge, and anyone who want to help: do your best. I can't do much
because I don't have non-working device and a Windows installation ready at
hand; but the real problem is the devices.
But for me this is a personal challenge - a matter of heart; and solving this
problem means solving lots of them in future maybe.
So - waiting for any data and consideration.
If you need anything by my side - you know where you can find me:
- here
- or use this mail address as google talk messaging + google plus; and this is
true even for who is reading linux-usb and netdev mailing lists.
Enrico
^ permalink raw reply
* [WTF?] random test in netlink_sendmsg()
From: Al Viro @ 2014-11-28 6:23 UTC (permalink / raw)
To: David Miller; +Cc: Patrick McHardy, netdev
In netlink_sendmsg() we have the following:
if (netlink_tx_is_mmaped(sk) &&
msg->msg_iov->iov_base == NULL) {
err = netlink_mmap_sendmsg(sk, msg, dst_portid, dst_group,
siocb);
goto out;
}
Now, suppose sendmsg(2) is called with msg.msg_iovlen == 0. We'll have
->msg_iov in kernel-side copy pointing at the uninitialized array in
stack frame of ___sys_sendmsg() - neither new nor old code touches elements
past the first msg_iovlen ones. So in that case it checks if an
uninitialized word on stack is zero.
What is that check trying to do? Is that simply missing
"(msg->msg_iovlen > 0) &&"? And why on the Earth didn't it simply use
zero msg_iovlen as the indicator, instead of messing with iovec contents?
Obviously too late to change, but... ouch.
Patrick, it had been that way since your commit last year ("netlink: implement
memory mapped sendmsg()"); could you explain what's the intended ABI?
Incidentally, WTF is "atomic_read(&nlk->mapped) > 1" part of check in
netlink_mmap_sendmsg() trying to achieve? AFAICS, ->mapped tries to
keep track of the number of VMAs, right? If so, it's bloody pointless -
one can have memory accessible in more than one process without any
extra VMAs. Just clone(2) with CLONE_VM. Voila - child shares the
entire address space. No extra VMAs or calls of ->open() in sight...
^ permalink raw reply
* [PATCH net] gso: do GSO for local skb with size bigger than MTU
From: Fan Du @ 2014-11-28 6:33 UTC (permalink / raw)
To: netdev; +Cc: davem, fw, Fan Du
Test scenario: two KVM guests sitting in different
hosts communicate to each other with a vxlan tunnel.
All interface MTU is default 1500 Bytes, from guest point
of view, its skb gso_size could be as bigger as 1448Bytes,
however after guest skb goes through vxlan encapuslation,
individual segments length of a gso packet could exceed
physical NIC MTU 1500, which will be lost at recevier side.
So it's possible in virtualized environment, locally created
skb len after encapslation could be bigger than underlayer
MTU. In such case, it's reasonable to do GSO first,
then fragment any packet bigger than MTU as possible.
+---------------+ TX RX +---------------+
| KVM Guest | -> ... -> | KVM Guest |
+-+-----------+-+ +-+-----------+-+
|Qemu/VirtIO| |Qemu/VirtIO|
+-----------+ +-----------+
| |
v tap0 tap0 v
+-----------+ +-----------+
| ovs bridge| | ovs bridge|
+-----------+ +-----------+
| vxlan vxlan |
v v
+-----------+ +-----------+
| NIC | <------> | NIC |
+-----------+ +-----------+
Steps to reproduce:
1. Using kernel builtin openvswitch module to setup ovs bridge.
2. Runing iperf without -M, communication will stuck.
Signed-off-by: Fan Du <fan.du@intel.com>
---
net/ipv4/ip_output.c | 7 ++++---
1 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c
index bc6471d..558b5f8 100644
--- a/net/ipv4/ip_output.c
+++ b/net/ipv4/ip_output.c
@@ -217,9 +217,10 @@ static int ip_finish_output_gso(struct sk_buff *skb)
struct sk_buff *segs;
int ret = 0;
- /* common case: locally created skb or seglen is <= mtu */
- if (((IPCB(skb)->flags & IPSKB_FORWARDED) == 0) ||
- skb_gso_network_seglen(skb) <= ip_skb_dst_mtu(skb))
+ /* Both locally created skb and forwarded skb could exceed
+ * MTU size, so make a unified rule for them all.
+ */
+ if (skb_gso_network_seglen(skb) <= ip_skb_dst_mtu(skb))
return ip_finish_output2(skb);
/* Slowpath - GSO segment length is exceeding the dst MTU.
--
1.7.1
^ 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