* [PATCH 1/1 net-next] openvswitch: Remove unnecessary version.h inclusion
From: Syam Sidhardhan @ 2015-01-12 15:19 UTC (permalink / raw)
To: netdev, davem, dev, pshelar, syamsidhardh; +Cc: Syam Sidhardhan
version.h inclusion is not necessary as detected by versioncheck.
Signed-off-by: Syam Sidhardhan <s.syam@samsung.com>
Acked-by: Pravin B Shelar <pshelar@nicira.com>
---
No code changes. Add net-next prefix flag for net-next tree.
net/openvswitch/vport-geneve.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/net/openvswitch/vport-geneve.c b/net/openvswitch/vport-geneve.c
index 347fa23..70e2aae 100644
--- a/net/openvswitch/vport-geneve.c
+++ b/net/openvswitch/vport-geneve.c
@@ -9,8 +9,6 @@
#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
-#include <linux/version.h>
-
#include <linux/in.h>
#include <linux/ip.h>
#include <linux/net.h>
--
1.7.9.5
^ permalink raw reply related
* Re: [PATCH net-next v3] tcp: avoid reducing cwnd when ACK+DSACK is received
From: Eric Dumazet @ 2015-01-12 15:15 UTC (permalink / raw)
To: Sébastien Barré
Cc: David Miller, Neal Cardwell, Yuchung Cheng, netdev, Gregory Detal,
Nandita Dukkipati
In-Reply-To: <1421055040-8732-1-git-send-email-sebastien.barre@uclouvain.be>
On Mon, 2015-01-12 at 10:30 +0100, Sébastien Barré wrote:
> With TLP, the peer may reply to a probe with an
> ACK+D-SACK, with ack value set to tlp_high_seq. In the current code,
> such ACK+DSACK will be missed and only at next, higher ack will the TLP
> episode be considered done. Since the DSACK is not present anymore,
> this will cost a cwnd reduction.
Acked-by: Eric Dumazet <edumazet@google.com>
Thanks guys.
^ permalink raw reply
* [PATCH v5] can: Convert to runtime_pm
From: Kedareswara rao Appana @ 2015-01-12 15:04 UTC (permalink / raw)
To: wg, mkl, michal.simek, soren.brinkmann, grant.likely, robh+dt
Cc: linux-can, netdev, linux-arm-kernel, linux-kernel, devicetree,
Kedareswara rao Appana
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 v5:
- Updated with the review comments.
Updated the remove fuction to use runtime_pm.
Chnages for v4:
- Updated with the review comments.
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 | 157 ++++++++++++++++++++++++++++-------------
1 files changed, 107 insertions(+), 50 deletions(-)
diff --git a/drivers/net/can/xilinx_can.c b/drivers/net/can/xilinx_can.c
index 6c67643..67aef00 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: pm_runtime_get failed(%d)\n",
+ __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,20 @@ 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 = clk_prepare_enable(priv->bus_clk);
- if (ret)
- goto err_clk;
+ ret = pm_runtime_get_sync(priv->dev);
+ if (ret < 0) {
+ netdev_err(ndev, "%s: pm_runtime_get failed(%d)\n",
+ __func__, ret);
+ return ret;
+ }
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 +953,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 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 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 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,18 +1009,18 @@ 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 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;
+ u32 isr, status;
ret = clk_enable(priv->bus_clk);
if (ret) {
@@ -1014,15 +1030,28 @@ static int __maybe_unused xcan_resume(struct device *dev)
ret = clk_enable(priv->can_clk);
if (ret) {
dev_err(dev, "Cannot enable clock.\n");
- clk_disable_unprepare(priv->bus_clk);
+ clk_disable(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;
+ isr = priv->read_reg(priv, XCAN_ISR_OFFSET);
+ status = priv->read_reg(priv, XCAN_SR_OFFSET);
if (netif_running(ndev)) {
+ if (isr & XCAN_IXR_BSOFF_MASK) {
+ priv->can.state = CAN_STATE_BUS_OFF;
+ priv->write_reg(priv, XCAN_SRR_OFFSET,
+ XCAN_SRR_RESET_MASK);
+ } else if ((status & XCAN_SR_ESTAT_MASK) ==
+ XCAN_SR_ESTAT_MASK) {
+ priv->can.state = CAN_STATE_ERROR_PASSIVE;
+ } else if (status & XCAN_SR_ERRWRN_MASK) {
+ priv->can.state = CAN_STATE_ERROR_WARNING;
+ } else {
+ priv->can.state = CAN_STATE_ERROR_ACTIVE;
+ }
netif_device_attach(ndev);
netif_start_queue(ndev);
}
@@ -1030,7 +1059,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 +1103,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 +1169,16 @@ 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);
+ pm_runtime_enable(&pdev->dev);
+ ret = pm_runtime_get_sync(&pdev->dev);
+ if (ret < 0) {
+ netdev_err(ndev, "%s: pm_runtime_get failed(%d)\n",
+ __func__, ret);
+ goto err_pmdisable;
+ }
+
ret = register_candev(ndev);
if (ret) {
dev_err(&pdev->dev, "fail to register failed (err=%d)\n", ret);
@@ -1144,15 +1186,19 @@ 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);
return 0;
+err_pmdisable:
+ pm_runtime_disable(&pdev->dev);
err_unprepare_disable_busclk:
+ pm_runtime_put(priv->dev);
clk_disable_unprepare(priv->bus_clk);
err_unprepare_disable_dev:
clk_disable_unprepare(priv->can_clk);
@@ -1173,12 +1219,23 @@ static int xcan_remove(struct platform_device *pdev)
{
struct net_device *ndev = platform_get_drvdata(pdev);
struct xcan_priv *priv = netdev_priv(ndev);
+ int ret;
+
+ ret = pm_runtime_get_sync(&pdev->dev);
+ if (ret < 0) {
+ netdev_err(ndev, "%s: pm_runtime_get failed(%d)\n",
+ __func__, ret);
+ return ret;
+ }
if (set_reset_mode(ndev) < 0)
netdev_err(ndev, "mode resetting failed!\n");
unregister_candev(ndev);
+ pm_runtime_disable(&pdev->dev);
netif_napi_del(&priv->napi);
+ clk_disable_unprepare(priv->bus_clk);
+ clk_disable_unprepare(priv->can_clk);
free_candev(ndev);
return 0;
--
1.7.4
^ permalink raw reply related
* RE: [PATCH v4] can: Convert to runtime_pm
From: Appana Durga Kedareswara Rao @ 2015-01-12 15:04 UTC (permalink / raw)
To: Marc Kleine-Budde
Cc: linux-can@vger.kernel.org, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org, Soren Brinkmann,
grant.likely@linaro.org, wg@grandegger.com, Michal Simek
In-Reply-To: <54B3D1C0.200@pengutronix.de>
Hi Marc,
> -----Original Message-----
> From: Marc Kleine-Budde [mailto:mkl@pengutronix.de]
> Sent: Monday, January 12, 2015 7:23 PM
> To: Appana Durga Kedareswara Rao
> Cc: linux-can@vger.kernel.org; netdev@vger.kernel.org; linux-
> kernel@vger.kernel.org; Soren Brinkmann; grant.likely@linaro.org;
> wg@grandegger.com; Michal Simek
> Subject: Re: [PATCH v4] can: Convert to runtime_pm
>
> On 01/12/2015 02:49 PM, Appana Durga Kedareswara Rao wrote:
> > Hi Marc,
> >
> >> -----Original Message-----
> >> From: Marc Kleine-Budde [mailto:mkl@pengutronix.de]
> >> Sent: Monday, January 12, 2015 6:56 PM
> >> To: Appana Durga Kedareswara Rao
> >> Cc: linux-can@vger.kernel.org; netdev@vger.kernel.org; linux-
> >> kernel@vger.kernel.org; Soren Brinkmann; grant.likely@linaro.org;
> >> wg@grandegger.com; Michal Simek
> >> Subject: Re: [PATCH v4] can: Convert to runtime_pm
> >>
> >> On 01/12/2015 07:59 AM, Appana Durga Kedareswara Rao wrote:
> >>> Hi Marc,
> >>>
> >>>> -----Original Message-----
> >>>> From: Marc Kleine-Budde [mailto:mkl@pengutronix.de]
> >>>> Sent: Sunday, January 11, 2015 9:11 PM
> >>>> To: Appana Durga Kedareswara Rao
> >>>> Cc: linux-can@vger.kernel.org; netdev@vger.kernel.org; linux-
> >>>> kernel@vger.kernel.org; Soren Brinkmann; grant.likely@linaro.org;
> >>>> wg@grandegger.com
> >>>> Subject: Re: [PATCH v4] can: Convert to runtime_pm
> >>>>
> >>>> On 01/11/2015 06:34 AM, Appana Durga Kedareswara Rao wrote:
> >>>> [...]
> >>>>>>> return ret;
> >>>>>>> }
> >>>>>>>
> >>>>>>> priv->write_reg(priv, XCAN_MSR_OFFSET, 0);
> >>>>>>> priv->write_reg(priv, XCAN_SRR_OFFSET, XCAN_SRR_CEN_MASK);
> >>>>>>>
> >>>>>>> if (netif_running(ndev)) {
> >>>>>>> priv->can.state = CAN_STATE_ERROR_ACTIVE;
> >>>>>>
> >>>>>> What happens if the device was not in ACTIVE state prior to the
> >>>>>> runtime_suspend?
> >>>>>>
> >>>>>
> >>>>> I am not sure about the state of CAN at this point of time.
> >>>>> I just followed what other drivers are following for run time suspend
> :).
> >>>>
> >>>> Please check the state of the hardware if you go with bus off into
> >>>> suspend and then resume.
> >>>>
> >>>
> >>> if (netif_running(ndev)) {
> >>> if (isr & XCAN_IXR_BSOFF_MASK) {
> >>> priv->can.state = CAN_STATE_BUS_OFF;
> >>> priv->write_reg(priv, XCAN_SRR_OFFSET,
> >>> XCAN_SRR_RESET_MASK);
> >>> } else if ((status & XCAN_SR_ESTAT_MASK) ==
> >>> XCAN_SR_ESTAT_MASK) {
> >>> priv->can.state = CAN_STATE_ERROR_PASSIVE;
> >>> } else if (status & XCAN_SR_ERRWRN_MASK) {
> >>> priv->can.state = CAN_STATE_ERROR_WARNING;
> >>> } else {
> >>> priv->can.state = CAN_STATE_ERROR_ACTIVE;
> >>> }
> >>> }
> >>>
> >>> Is the above code snippet ok for you?
> >>
> >> Yes, but what's the state of the hardware when it wakes up again?
> >
> > It depends on the previous state of the CAN.
> > I mean In Suspend we are putting the device in sleep mode and in
> > resume we are waking up by putting the device into the Configuration
> > mode. We are not doing any reset of the core in the suspend/resume so it
> depends on the previous state of the CAN when it wakes up that's why
> checking for the status of the CAN in the status register here to put the
> device in appropriate mode.
>
> I understand the software side, but I don't know how your hardware
> behaves. This is why I'm asking.
As far as I know the above is the our can controller behavior. Anyway I will check with the h/w team
And will get back to you regarding this. Meanwhile I am sending the next version of the patch
Please review it.
Regards,
Kedar.
>
> >
> >>
> >>>
> >>>>>>> netif_device_attach(ndev);
> >>>>>>> netif_start_queue(ndev);
> >>>>>>> }
> >>>>>>>
> >>>>>>> return 0;
> >>>>>>> }
> >>>>>>
> >>>>>>
> >>>>>>>
> >>>>>>> @@ -1020,9 +1035,9 @@ static int __maybe_unused
> >> xcan_resume(struct
> >>>>>>> device *dev)
> >>>>>>>
> >>>>>>> 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;
> >>>>>>>
> >>>>>>> if (netif_running(ndev)) {
> >>>>>>> + priv->can.state = CAN_STATE_ERROR_ACTIVE;
> >>>>>>> netif_device_attach(ndev);
> >>>>>>> netif_start_queue(ndev);
> >>>>>>> }
> >>>>>>> @@ -1030,7 +1045,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 +1089,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,15
> >>>>>>> +1155,22 @@ 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);
> >>>>>>> + pm_runtime_enable(&pdev->dev);
> >>>>>>> + pm_runtime_get_sync(&pdev->dev);
> >>>>>> Check error values?
> >>>>>
> >>>>> Ok
> >>>>>>> +
> >>>>>>> ret = register_candev(ndev);
> >>>>>>> if (ret) {
> >>>>>>> dev_err(&pdev->dev, "fail to register failed
> >>>>>>> (err=%d)\n",
> >>>>>> ret);
> >>>>>>> + pm_runtime_put(priv->dev);
> >>>>>>
> >>>>>> Please move the pm_runtime_put into the common error exit path.
> >>>>>>
> >>>>>
> >>>>> Ok
> >>>>>
> >>>>>>> goto err_unprepare_disable_busclk;
> >>>>>>> }
> >>>>>>>
> >>>>>>> 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);
> >>>>>>>
> >>>>>>
> >>>>>> I think you have to convert the _remove() function, too. Have a
> >>>>>> look at the gpio-zynq.c driver:
> >>>>>>
> >>>>>>> static int zynq_gpio_remove(struct platform_device *pdev) {
> >>>>>>> struct zynq_gpio *gpio = platform_get_drvdata(pdev);
> >>>>>>>
> >>>>>>> pm_runtime_get_sync(&pdev->dev);
> >>>>>>
> >>>>>> However I don't understand why the get_sync() is here. Maybe
> >>>>>> Sören can help?
> >>>>>
> >>>>> I converted the remove function to use the run-time PM and .
> >>>>> Below is the remove code snippet.
> >>>>>
> >>>>> ret = pm_runtime_get_sync(&pdev->dev);
> >>>>> if (ret < 0) {
> >>>>> netdev_err(ndev, "%s: pm_runtime_get failed(%d)\n",
> >>>>> __func__, ret);
> >>>>> return ret;
> >>>>> }
> >>>>>
> >>>>> if (set_reset_mode(ndev) < 0)
> >>>>> netdev_err(ndev, "mode resetting failed!\n");
> >>>>>
> >>>>> unregister_candev(ndev);
> >>>>> netif_napi_del(&priv->napi);
> >>>>> free_candev(ndev);
> >>>>
> >>>>> pm_runtime_disable(&pdev->dev);
> >>>>
> >>>> Can this make a call to xcan_runtime_*()? I'm asking since the ndev
> >>>> has been unregistered and already free()ed. Better move this
> >>>> directly after the set_reset_mode(). This way you are symmetric to
> >>>> the probe()
> >> function.
> >>>
> >>> If I move the pm_runtime_disable after the set_reset_mode I am
> >>> getting the below error.
> >>> ERROR:
> >>> xilinx_can e0008000.can can0 (unregistering): xcan_get_berr_counter:
> >>> pm_runtime_get fail
> >>>
> >>> If I move the pm_runtime_disable after unregister_candev everything
> >>> is
> >> working fine.
> >>
> >> Fine - but who calls xcan_get_berr_counter here? Can you add a
> >> dump_stack() here?
> >>
> >
> > I think it is getting called from the atomic context.
> > When I am trying to do a rmmod I am getting the above error.
> > ERROR:
> > xilinx_can e0008000.can can0 (unregistering): xcan_get_berr_counter:
> > pm_runtime_get fail.
> >
> > I am getting only the above error in the console when I do rmmod.
>
> Put a dump_stack into xcan_get_berr_counter(), then you'll see where it's
> called from. However calling from atomic context should be fine.
>
> 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 |
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: [PATCH net-next v2] tcp: avoid reducing cwnd when ACK+DSACK is received
From: Neal Cardwell @ 2015-01-12 15:02 UTC (permalink / raw)
To: David Laight
Cc: Yuchung Cheng, Eric Dumazet, Sébastien Barré,
David Miller, Netdev, Gregory Detal, Nandita Dukkipati
In-Reply-To: <063D6719AE5E284EB5DD2968C1650D6D1CAC5844@AcuExch.aculab.com>
On Mon, Jan 12, 2015 at 6:52 AM, David Laight <David.Laight@aculab.com> wrote:
>> if (flag & FLAG_DSACKING_ACK) {
>> /* This DSACK means original and TLP probe arrived; no loss */
>> tp->tlp_high_seq = 0;
>
> I think I'd add a 'return' here.
What's the benefit of adding 'return' in those two spots? That adds
extra code to read, with no change in behavior, and no increase in
maintainability.
neal
^ permalink raw reply
* Re: [PATCH] gianfar: correct the bad expression while writing bit-pattern
From: Claudiu Manoil @ 2015-01-12 14:22 UTC (permalink / raw)
To: Sanjeev Sharma; +Cc: davem, peter.senna, shemminger, netdev, linux-kernel
In-Reply-To: <1421048596-22639-1-git-send-email-sanjeev_sharma@mentor.com>
On 1/12/2015 9:43 AM, Sanjeev Sharma wrote:
> This patch correct the bad expression while writing the
> bit-pattern from software's buffer to hardware registers.
>
> Signed-off-by: Sanjeev Sharma <Sanjeev_Sharma@mentor.com>
> ---
> drivers/net/ethernet/freescale/gianfar_ethtool.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/freescale/gianfar_ethtool.c b/drivers/net/ethernet/freescale/gianfar_ethtool.c
> index 3e1a9c1..1ccca72 100644
> --- a/drivers/net/ethernet/freescale/gianfar_ethtool.c
> +++ b/drivers/net/ethernet/freescale/gianfar_ethtool.c
> @@ -1586,7 +1586,7 @@ static int gfar_write_filer_table(struct gfar_private *priv,
> return -EBUSY;
>
> /* Fill regular entries */
> - for (; i < MAX_FILER_IDX - 1 && (tab->fe[i].ctrl | tab->fe[i].ctrl);
> + for (; i < MAX_FILER_IDX - 1 && ( i < tab->fe[i].ctrl);
> i++)
Why do you think 'i' can be compared with the 'ctrl' field?
Is the control field an index (provide proof if yes)? I doubt it...
Thanks,
Claudiu
^ permalink raw reply
* Re: [PATCH] gianfar: correct the bad expression while writing bit-pattern
From: Sergei Shtylyov @ 2015-01-12 14:12 UTC (permalink / raw)
To: Sanjeev Sharma, davem
Cc: claudiu.manoil, peter.senna, shemminger, netdev, linux-kernel
In-Reply-To: <1421048596-22639-1-git-send-email-sanjeev_sharma@mentor.com>
Hello.
On 1/12/2015 10:43 AM, Sanjeev Sharma wrote:
> This patch correct the bad expression while writing the
> bit-pattern from software's buffer to hardware registers.
> Signed-off-by: Sanjeev Sharma <Sanjeev_Sharma@mentor.com>
> ---
> drivers/net/ethernet/freescale/gianfar_ethtool.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> diff --git a/drivers/net/ethernet/freescale/gianfar_ethtool.c b/drivers/net/ethernet/freescale/gianfar_ethtool.c
> index 3e1a9c1..1ccca72 100644
> --- a/drivers/net/ethernet/freescale/gianfar_ethtool.c
> +++ b/drivers/net/ethernet/freescale/gianfar_ethtool.c
> @@ -1586,7 +1586,7 @@ static int gfar_write_filer_table(struct gfar_private *priv,
> return -EBUSY;
>
> /* Fill regular entries */
> - for (; i < MAX_FILER_IDX - 1 && (tab->fe[i].ctrl | tab->fe[i].ctrl);
> + for (; i < MAX_FILER_IDX - 1 && ( i < tab->fe[i].ctrl);
Inner parens not needed. And there shouldn't be space after (.
WBR, Sergei
^ permalink raw reply
* Re: [PATCH] brcmfmac: avoid duplicated suspend/resume operation
From: Sergei Shtylyov @ 2015-01-12 14:06 UTC (permalink / raw)
To: Fu, Zhonghui, brudley,
arend@broadcom.com >> Arend van Spriel, Franky Lin,
meuleman, kvalo, linville, pieterpg, hdegoede, wens,
linux-wireless, brcm80211-dev-list, netdev,
linux-kernel@vger.kernel.org
In-Reply-To: <54B36C88.3030609@linux.intel.com>
Hello.
On 1/12/2015 9:41 AM, Fu, Zhonghui wrote:
> From 8685c3c2746b4275fc808d9db23c364b2f54b52a Mon Sep 17 00:00:00 2001
> From: Zhonghui Fu <zhonghui.fu@linux.intel.com>
> Date: Mon, 12 Jan 2015 14:25:46 +0800
> Subject: [PATCH] brcmfmac: avoid duplicated suspend/resume operation
The lines above are not needed.
> WiFi chip has 2 SDIO functions, and PM core will trigger
> twice suspend/resume operations for one WiFi chip to do
> the same things. This patch avoid this case.
> Acked-by: Arend van Spriel<arend@broadcom.com>
> Acked-by: Sergei Shtylyov<sergei.shtylyov@cogentembedded.com>
> Signed-off-by: Zhonghui Fu <zhonghui.fu@linux.intel.com>
> ---
> drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c | 21 +++++++++++++++++----
> 1 files changed, 17 insertions(+), 4 deletions(-)
> diff --git a/drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c b/drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c
> index 9880dae..8f71485 100644
> --- a/drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c
> +++ b/drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c
> @@ -1070,7 +1070,7 @@ static int brcmf_ops_sdio_probe(struct sdio_func *func,
> */
> if ((sdio_get_host_pm_caps(sdiodev->func[1]) & MMC_PM_KEEP_POWER) &&
> ((sdio_get_host_pm_caps(sdiodev->func[1]) & MMC_PM_WAKE_SDIO_IRQ) ||
> - (sdiodev->pdata && sdiodev->pdata->oob_irq_supported)))
> + (sdiodev->pdata->oob_irq_supported)))
Inner parens not needed on this line.
[...]
WBR, Sergei
^ permalink raw reply
* Re: [PATCH v4] can: Convert to runtime_pm
From: Marc Kleine-Budde @ 2015-01-12 13:53 UTC (permalink / raw)
To: Appana Durga Kedareswara Rao
Cc: linux-can@vger.kernel.org, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org, Soren Brinkmann,
grant.likely@linaro.org, wg@grandegger.com, Michal Simek
In-Reply-To: <4a61c12b2e1d4103a41a3faf1d58837a@BN1BFFO11FD030.protection.gbl>
[-- Attachment #1: Type: text/plain, Size: 8610 bytes --]
On 01/12/2015 02:49 PM, Appana Durga Kedareswara Rao wrote:
> Hi Marc,
>
>> -----Original Message-----
>> From: Marc Kleine-Budde [mailto:mkl@pengutronix.de]
>> Sent: Monday, January 12, 2015 6:56 PM
>> To: Appana Durga Kedareswara Rao
>> Cc: linux-can@vger.kernel.org; netdev@vger.kernel.org; linux-
>> kernel@vger.kernel.org; Soren Brinkmann; grant.likely@linaro.org;
>> wg@grandegger.com; Michal Simek
>> Subject: Re: [PATCH v4] can: Convert to runtime_pm
>>
>> On 01/12/2015 07:59 AM, Appana Durga Kedareswara Rao wrote:
>>> Hi Marc,
>>>
>>>> -----Original Message-----
>>>> From: Marc Kleine-Budde [mailto:mkl@pengutronix.de]
>>>> Sent: Sunday, January 11, 2015 9:11 PM
>>>> To: Appana Durga Kedareswara Rao
>>>> Cc: linux-can@vger.kernel.org; netdev@vger.kernel.org; linux-
>>>> kernel@vger.kernel.org; Soren Brinkmann; grant.likely@linaro.org;
>>>> wg@grandegger.com
>>>> Subject: Re: [PATCH v4] can: Convert to runtime_pm
>>>>
>>>> On 01/11/2015 06:34 AM, Appana Durga Kedareswara Rao wrote:
>>>> [...]
>>>>>>> return ret;
>>>>>>> }
>>>>>>>
>>>>>>> priv->write_reg(priv, XCAN_MSR_OFFSET, 0);
>>>>>>> priv->write_reg(priv, XCAN_SRR_OFFSET, XCAN_SRR_CEN_MASK);
>>>>>>>
>>>>>>> if (netif_running(ndev)) {
>>>>>>> priv->can.state = CAN_STATE_ERROR_ACTIVE;
>>>>>>
>>>>>> What happens if the device was not in ACTIVE state prior to the
>>>>>> runtime_suspend?
>>>>>>
>>>>>
>>>>> I am not sure about the state of CAN at this point of time.
>>>>> I just followed what other drivers are following for run time suspend :).
>>>>
>>>> Please check the state of the hardware if you go with bus off into
>>>> suspend and then resume.
>>>>
>>>
>>> if (netif_running(ndev)) {
>>> if (isr & XCAN_IXR_BSOFF_MASK) {
>>> priv->can.state = CAN_STATE_BUS_OFF;
>>> priv->write_reg(priv, XCAN_SRR_OFFSET,
>>> XCAN_SRR_RESET_MASK);
>>> } else if ((status & XCAN_SR_ESTAT_MASK) ==
>>> XCAN_SR_ESTAT_MASK) {
>>> priv->can.state = CAN_STATE_ERROR_PASSIVE;
>>> } else if (status & XCAN_SR_ERRWRN_MASK) {
>>> priv->can.state = CAN_STATE_ERROR_WARNING;
>>> } else {
>>> priv->can.state = CAN_STATE_ERROR_ACTIVE;
>>> }
>>> }
>>>
>>> Is the above code snippet ok for you?
>>
>> Yes, but what's the state of the hardware when it wakes up again?
>
> It depends on the previous state of the CAN.
> I mean In Suspend we are putting the device in sleep mode and in resume we are waking up by putting the device into the
> Configuration mode. We are not doing any reset of the core in the suspend/resume so it depends on the previous state of the CAN
> when it wakes up that's why checking for the status of the CAN in the status register here to put the device in appropriate mode.
I understand the software side, but I don't know how your hardware
behaves. This is why I'm asking.
>
>>
>>>
>>>>>>> netif_device_attach(ndev);
>>>>>>> netif_start_queue(ndev);
>>>>>>> }
>>>>>>>
>>>>>>> return 0;
>>>>>>> }
>>>>>>
>>>>>>
>>>>>>>
>>>>>>> @@ -1020,9 +1035,9 @@ static int __maybe_unused
>> xcan_resume(struct
>>>>>>> device *dev)
>>>>>>>
>>>>>>> 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;
>>>>>>>
>>>>>>> if (netif_running(ndev)) {
>>>>>>> + priv->can.state = CAN_STATE_ERROR_ACTIVE;
>>>>>>> netif_device_attach(ndev);
>>>>>>> netif_start_queue(ndev);
>>>>>>> }
>>>>>>> @@ -1030,7 +1045,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 +1089,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,15
>>>>>>> +1155,22 @@ 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);
>>>>>>> + pm_runtime_enable(&pdev->dev);
>>>>>>> + pm_runtime_get_sync(&pdev->dev);
>>>>>> Check error values?
>>>>>
>>>>> Ok
>>>>>>> +
>>>>>>> ret = register_candev(ndev);
>>>>>>> if (ret) {
>>>>>>> dev_err(&pdev->dev, "fail to register failed
>>>>>>> (err=%d)\n",
>>>>>> ret);
>>>>>>> + pm_runtime_put(priv->dev);
>>>>>>
>>>>>> Please move the pm_runtime_put into the common error exit path.
>>>>>>
>>>>>
>>>>> Ok
>>>>>
>>>>>>> goto err_unprepare_disable_busclk;
>>>>>>> }
>>>>>>>
>>>>>>> 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);
>>>>>>>
>>>>>>
>>>>>> I think you have to convert the _remove() function, too. Have a
>>>>>> look at the gpio-zynq.c driver:
>>>>>>
>>>>>>> static int zynq_gpio_remove(struct platform_device *pdev) {
>>>>>>> struct zynq_gpio *gpio = platform_get_drvdata(pdev);
>>>>>>>
>>>>>>> pm_runtime_get_sync(&pdev->dev);
>>>>>>
>>>>>> However I don't understand why the get_sync() is here. Maybe Sören
>>>>>> can help?
>>>>>
>>>>> I converted the remove function to use the run-time PM and .
>>>>> Below is the remove code snippet.
>>>>>
>>>>> ret = pm_runtime_get_sync(&pdev->dev);
>>>>> if (ret < 0) {
>>>>> netdev_err(ndev, "%s: pm_runtime_get failed(%d)\n",
>>>>> __func__, ret);
>>>>> return ret;
>>>>> }
>>>>>
>>>>> if (set_reset_mode(ndev) < 0)
>>>>> netdev_err(ndev, "mode resetting failed!\n");
>>>>>
>>>>> unregister_candev(ndev);
>>>>> netif_napi_del(&priv->napi);
>>>>> free_candev(ndev);
>>>>
>>>>> pm_runtime_disable(&pdev->dev);
>>>>
>>>> Can this make a call to xcan_runtime_*()? I'm asking since the ndev
>>>> has been unregistered and already free()ed. Better move this directly
>>>> after the set_reset_mode(). This way you are symmetric to the probe()
>> function.
>>>
>>> If I move the pm_runtime_disable after the set_reset_mode I am
>>> getting the below error.
>>> ERROR:
>>> xilinx_can e0008000.can can0 (unregistering): xcan_get_berr_counter:
>>> pm_runtime_get fail
>>>
>>> If I move the pm_runtime_disable after unregister_candev everything is
>> working fine.
>>
>> Fine - but who calls xcan_get_berr_counter here? Can you add a
>> dump_stack() here?
>>
>
> I think it is getting called from the atomic context.
> When I am trying to do a rmmod I am getting the above error.
> ERROR:
> xilinx_can e0008000.can can0 (unregistering): xcan_get_berr_counter:
> pm_runtime_get fail.
>
> I am getting only the above error in the console when I do rmmod.
Put a dump_stack into xcan_get_berr_counter(), then you'll see where
it's called from. However calling from atomic context should be fine.
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 v4 3/4] can: kvaser_usb: Add support for the Usbcan-II family
From: Olivier Sobrie @ 2015-01-12 13:53 UTC (permalink / raw)
To: Ahmed S. Darwish
Cc: Oliver Hartkopp, Wolfgang Grandegger, Marc Kleine-Budde,
David S. Miller, Paul Gortmaker, Linux-CAN, netdev, LKML
In-Reply-To: <20150111203612.GA8999@linux>
Hello,
On Sun, Jan 11, 2015 at 03:36:12PM -0500, Ahmed S. Darwish wrote:
> From: Ahmed S. Darwish <ahmed.darwish@valeo.com>
>
> CAN to USB interfaces sold by the Swedish manufacturer Kvaser are
> divided into two major families: 'Leaf', and 'UsbcanII'. From an
> Operating System perspective, the firmware of both families behave
> in a not too drastically different fashion.
>
> This patch adds support for the UsbcanII family of devices to the
> current Kvaser Leaf-only driver.
>
> CAN frames sending, receiving, and error handling paths has been
> tested using the dual-channel "Kvaser USBcan II HS/LS" dongle. It
> should also work nicely with other products in the same category.
>
> List of new devices supported by this driver update:
>
> - Kvaser USBcan II HS/HS
> - Kvaser USBcan II HS/LS
> - Kvaser USBcan Rugged ("USBcan Rev B")
> - Kvaser Memorator HS/HS
> - Kvaser Memorator HS/LS
> - Scania VCI2 (if you have the Kvaser logo on top)
>
> Signed-off-by: Ahmed S. Darwish <ahmed.darwish@valeo.com>
Just two small remarks below.
Thanks,
Olivier
> ---
> drivers/net/can/usb/Kconfig | 8 +-
> drivers/net/can/usb/kvaser_usb.c | 612 ++++++++++++++++++++++++++++++---------
> 2 files changed, 487 insertions(+), 133 deletions(-)
>
> ** V4 Changelog:
> - Use type-safe C methods instead of cpp macros
> - Further clarify the code and comments on error events channel arbitration
> - Remove defensive checks against non-existing families
> - Re-order methods to remove forward declarations
> - Smaller stuff spotted by earlier review (function prefexes, etc.)
>
> ** V3 Changelog:
> - Fix padding for the usbcan_msg_tx_acknowledge command
> - Remove kvaser_usb->max_channels and the MAX_NET_DEVICES macro
> - Rename commands to CMD_LEAF_xxx and CMD_USBCAN_xxx
> - Apply checkpatch.pl suggestions ('net/' comments, multi-line strings, etc.)
>
> ** V2 Changelog:
> - Update Kconfig entries
> - Use actual number of CAN channels (instead of max) where appropriate
> - Rebase over a new set of UsbcanII-independent driver fixes
>
> diff --git a/drivers/net/can/usb/Kconfig b/drivers/net/can/usb/Kconfig
> index a77db919..f6f5500 100644
> --- a/drivers/net/can/usb/Kconfig
> +++ b/drivers/net/can/usb/Kconfig
> @@ -25,7 +25,7 @@ config CAN_KVASER_USB
> tristate "Kvaser CAN/USB interface"
> ---help---
> This driver adds support for Kvaser CAN/USB devices like Kvaser
> - Leaf Light.
> + Leaf Light and Kvaser USBcan II.
>
> The driver provides support for the following devices:
> - Kvaser Leaf Light
> @@ -46,6 +46,12 @@ config CAN_KVASER_USB
> - Kvaser USBcan R
> - Kvaser Leaf Light v2
> - Kvaser Mini PCI Express HS
> + - Kvaser USBcan II HS/HS
> + - Kvaser USBcan II HS/LS
> + - Kvaser USBcan Rugged ("USBcan Rev B")
> + - Kvaser Memorator HS/HS
> + - Kvaser Memorator HS/LS
> + - Scania VCI2 (if you have the Kvaser logo on top)
>
> If unsure, say N.
>
> diff --git a/drivers/net/can/usb/kvaser_usb.c b/drivers/net/can/usb/kvaser_usb.c
> index 0eb870b..da47d17 100644
> --- a/drivers/net/can/usb/kvaser_usb.c
> +++ b/drivers/net/can/usb/kvaser_usb.c
> @@ -6,10 +6,12 @@
> * Parts of this driver are based on the following:
> * - Kvaser linux leaf driver (version 4.78)
> * - CAN driver for esd CAN-USB/2
> + * - Kvaser linux usbcanII driver (version 5.3)
> *
> * Copyright (C) 2002-2006 KVASER AB, Sweden. All rights reserved.
> * Copyright (C) 2010 Matthias Fuchs <matthias.fuchs@esd.eu>, esd gmbh
> * Copyright (C) 2012 Olivier Sobrie <olivier@sobrie.be>
> + * Copyright (C) 2015 Valeo Corporation
> */
>
> #include <linux/completion.h>
> @@ -21,6 +23,15 @@
> #include <linux/can/dev.h>
> #include <linux/can/error.h>
>
> +/* Kvaser USB CAN dongles are divided into two major families:
> + * - Leaf: Based on Renesas M32C, running firmware labeled as 'filo'
> + * - UsbcanII: Based on Renesas M16C, running firmware labeled as 'helios'
> + */
> +enum kvaser_usb_family {
> + KVASER_LEAF,
> + KVASER_USBCAN,
> +};
> +
> #define MAX_TX_URBS 16
> #define MAX_RX_URBS 4
> #define START_TIMEOUT 1000 /* msecs */
> @@ -30,8 +41,9 @@
> #define RX_BUFFER_SIZE 3072
> #define CAN_USB_CLOCK 8000000
> #define MAX_NET_DEVICES 3
> +#define MAX_USBCAN_NET_DEVICES 2
>
> -/* Kvaser USB devices */
> +/* Leaf USB devices */
> #define KVASER_VENDOR_ID 0x0bfd
> #define USB_LEAF_DEVEL_PRODUCT_ID 10
> #define USB_LEAF_LITE_PRODUCT_ID 11
> @@ -56,6 +68,24 @@
> #define USB_LEAF_LITE_V2_PRODUCT_ID 288
> #define USB_MINI_PCIE_HS_PRODUCT_ID 289
>
> +static inline bool kvaser_is_leaf(const struct usb_device_id *id)
> +{
> + return id->idProduct >= USB_LEAF_DEVEL_PRODUCT_ID &&
> + id->idProduct <= USB_MINI_PCIE_HS_PRODUCT_ID;
> +}
> +
> +/* USBCANII devices */
> +#define USB_USBCAN_REVB_PRODUCT_ID 2
> +#define USB_VCI2_PRODUCT_ID 3
> +#define USB_USBCAN2_PRODUCT_ID 4
> +#define USB_MEMORATOR_PRODUCT_ID 5
> +
> +static inline bool kvaser_is_usbcan(const struct usb_device_id *id)
> +{
> + return id->idProduct >= USB_USBCAN_REVB_PRODUCT_ID &&
> + id->idProduct <= USB_MEMORATOR_PRODUCT_ID;
> +}
> +
> /* USB devices features */
> #define KVASER_HAS_SILENT_MODE BIT(0)
> #define KVASER_HAS_TXRX_ERRORS BIT(1)
> @@ -73,7 +103,7 @@
> #define MSG_FLAG_TX_ACK BIT(6)
> #define MSG_FLAG_TX_REQUEST BIT(7)
>
> -/* Can states */
> +/* Can states (M16C CxSTRH register) */
> #define M16C_STATE_BUS_RESET BIT(0)
> #define M16C_STATE_BUS_ERROR BIT(4)
> #define M16C_STATE_BUS_PASSIVE BIT(5)
> @@ -98,7 +128,13 @@
> #define CMD_START_CHIP_REPLY 27
> #define CMD_STOP_CHIP 28
> #define CMD_STOP_CHIP_REPLY 29
> -#define CMD_GET_CARD_INFO2 32
> +#define CMD_READ_CLOCK 30
> +#define CMD_READ_CLOCK_REPLY 31
These two defines are not used.
> +
> +#define CMD_LEAF_GET_CARD_INFO2 32
> +#define CMD_USBCAN_RESET_CLOCK 32
> +#define CMD_USBCAN_CLOCK_OVERFLOW_EVENT 33
> +
> #define CMD_GET_CARD_INFO 34
> #define CMD_GET_CARD_INFO_REPLY 35
> #define CMD_GET_SOFTWARE_INFO 38
> @@ -108,8 +144,9 @@
> #define CMD_RESET_ERROR_COUNTER 49
> #define CMD_TX_ACKNOWLEDGE 50
> #define CMD_CAN_ERROR_EVENT 51
> -#define CMD_USB_THROTTLE 77
> -#define CMD_LOG_MESSAGE 106
> +
> +#define CMD_LEAF_USB_THROTTLE 77
> +#define CMD_LEAF_LOG_MESSAGE 106
>
> /* error factors */
> #define M16C_EF_ACKE BIT(0)
> @@ -121,6 +158,14 @@
> #define M16C_EF_RCVE BIT(6)
> #define M16C_EF_TRE BIT(7)
>
> +/* Only Leaf-based devices can report M16C error factors,
> + * thus define our own error status flags for USBCANII
> + */
> +#define USBCAN_ERROR_STATE_NONE 0
> +#define USBCAN_ERROR_STATE_TX_ERROR BIT(0)
> +#define USBCAN_ERROR_STATE_RX_ERROR BIT(1)
> +#define USBCAN_ERROR_STATE_BUSERROR BIT(2)
> +
> /* bittiming parameters */
> #define KVASER_USB_TSEG1_MIN 1
> #define KVASER_USB_TSEG1_MAX 16
> @@ -137,7 +182,7 @@
> #define KVASER_CTRL_MODE_SELFRECEPTION 3
> #define KVASER_CTRL_MODE_OFF 4
>
> -/* log message */
> +/* Extended CAN identifier flag */
> #define KVASER_EXTENDED_FRAME BIT(31)
>
> struct kvaser_msg_simple {
> @@ -148,30 +193,55 @@ struct kvaser_msg_simple {
> struct kvaser_msg_cardinfo {
> u8 tid;
> u8 nchannels;
> - __le32 serial_number;
> - __le32 padding;
> + union {
> + struct {
> + __le32 serial_number;
> + __le32 padding;
> + } __packed leaf0;
> + struct {
> + __le32 serial_number_low;
> + __le32 serial_number_high;
> + } __packed usbcan0;
> + } __packed;
> __le32 clock_resolution;
> __le32 mfgdate;
> u8 ean[8];
> u8 hw_revision;
> - u8 usb_hs_mode;
> - __le16 padding2;
> + union {
> + struct {
> + u8 usb_hs_mode;
> + } __packed leaf1;
> + struct {
> + u8 padding;
> + } __packed usbcan1;
> + } __packed;
> + __le16 padding;
> } __packed;
>
> struct kvaser_msg_cardinfo2 {
> u8 tid;
> - u8 channel;
> + u8 reserved;
> u8 pcb_id[24];
> __le32 oem_unlock_code;
> } __packed;
>
> -struct kvaser_msg_softinfo {
> +struct leaf_msg_softinfo {
> u8 tid;
> - u8 channel;
> + u8 padding0;
> __le32 sw_options;
> __le32 fw_version;
> __le16 max_outstanding_tx;
> - __le16 padding[9];
> + __le16 padding1[9];
> +} __packed;
> +
> +struct usbcan_msg_softinfo {
> + u8 tid;
> + u8 fw_name[5];
> + __le16 max_outstanding_tx;
> + u8 padding[6];
> + __le32 fw_version;
> + __le16 checksum;
> + __le16 sw_options;
> } __packed;
>
> struct kvaser_msg_busparams {
> @@ -188,36 +258,86 @@ struct kvaser_msg_tx_can {
> u8 channel;
> u8 tid;
> u8 msg[14];
> - u8 padding;
> - u8 flags;
> + union {
> + struct {
> + u8 padding;
> + u8 flags;
> + } __packed leaf;
> + struct {
> + u8 flags;
> + u8 padding;
> + } __packed usbcan;
> + } __packed;
> +} __packed;
> +
> +struct kvaser_msg_rx_can_header {
> + u8 channel;
> + u8 flag;
> } __packed;
>
> -struct kvaser_msg_rx_can {
> +struct leaf_msg_rx_can {
> u8 channel;
> u8 flag;
> +
> __le16 time[3];
> u8 msg[14];
> } __packed;
>
> -struct kvaser_msg_chip_state_event {
> +struct usbcan_msg_rx_can {
> + u8 channel;
> + u8 flag;
> +
> + u8 msg[14];
> + __le16 time;
> +} __packed;
> +
> +struct leaf_msg_chip_state_event {
> u8 tid;
> u8 channel;
> +
> __le16 time[3];
> u8 tx_errors_count;
> u8 rx_errors_count;
> +
> u8 status;
> u8 padding[3];
> } __packed;
>
> -struct kvaser_msg_tx_acknowledge {
> +struct usbcan_msg_chip_state_event {
> + u8 tid;
> + u8 channel;
> +
> + u8 tx_errors_count;
> + u8 rx_errors_count;
> + __le16 time;
> +
> + u8 status;
> + u8 padding[3];
> +} __packed;
> +
> +struct kvaser_msg_tx_acknowledge_header {
> + u8 channel;
> + u8 tid;
> +};
Is this struct really needed? Can't you simply use
leaf_msg_tx_acknowledge or usbcan_msg_tx_acknowledge
structures to read the header.
Same for kvaser_msg_rx_can_header.
> +
> +struct leaf_msg_tx_acknowledge {
> u8 channel;
> u8 tid;
> +
> __le16 time[3];
> u8 flags;
> u8 time_offset;
> } __packed;
>
> -struct kvaser_msg_error_event {
> +struct usbcan_msg_tx_acknowledge {
> + u8 channel;
> + u8 tid;
> +
> + __le16 time;
> + __le16 padding;
> +} __packed;
> +
> +struct leaf_msg_error_event {
> u8 tid;
> u8 flags;
> __le16 time[3];
> @@ -229,6 +349,18 @@ struct kvaser_msg_error_event {
> u8 error_factor;
> } __packed;
>
> +struct usbcan_msg_error_event {
> + u8 tid;
> + u8 padding;
> + u8 tx_errors_count_ch0;
> + u8 rx_errors_count_ch0;
> + u8 tx_errors_count_ch1;
> + u8 rx_errors_count_ch1;
> + u8 status_ch0;
> + u8 status_ch1;
> + __le16 time;
> +} __packed;
> +
> struct kvaser_msg_ctrl_mode {
> u8 tid;
> u8 channel;
> @@ -243,7 +375,7 @@ struct kvaser_msg_flush_queue {
> u8 padding[3];
> } __packed;
>
> -struct kvaser_msg_log_message {
> +struct leaf_msg_log_message {
> u8 channel;
> u8 flags;
> __le16 time[3];
> @@ -260,19 +392,57 @@ struct kvaser_msg {
> struct kvaser_msg_simple simple;
> struct kvaser_msg_cardinfo cardinfo;
> struct kvaser_msg_cardinfo2 cardinfo2;
> - struct kvaser_msg_softinfo softinfo;
> struct kvaser_msg_busparams busparams;
> +
> + struct kvaser_msg_rx_can_header rx_can_header;
> + struct kvaser_msg_tx_acknowledge_header tx_acknowledge_header;
> +
> + union {
> + struct leaf_msg_softinfo softinfo;
> + struct leaf_msg_rx_can rx_can;
> + struct leaf_msg_chip_state_event chip_state_event;
> + struct leaf_msg_tx_acknowledge tx_acknowledge;
> + struct leaf_msg_error_event error_event;
> + struct leaf_msg_log_message log_message;
> + } __packed leaf;
> +
> + union {
> + struct usbcan_msg_softinfo softinfo;
> + struct usbcan_msg_rx_can rx_can;
> + struct usbcan_msg_chip_state_event chip_state_event;
> + struct usbcan_msg_tx_acknowledge tx_acknowledge;
> + struct usbcan_msg_error_event error_event;
> + } __packed usbcan;
> +
> struct kvaser_msg_tx_can tx_can;
> - struct kvaser_msg_rx_can rx_can;
> - struct kvaser_msg_chip_state_event chip_state_event;
> - struct kvaser_msg_tx_acknowledge tx_acknowledge;
> - struct kvaser_msg_error_event error_event;
> struct kvaser_msg_ctrl_mode ctrl_mode;
> struct kvaser_msg_flush_queue flush_queue;
> - struct kvaser_msg_log_message log_message;
> } u;
> } __packed;
>
> +/* Summary of a kvaser error event, for a unified Leaf/Usbcan error
> + * handling. Some discrepancies between the two families exist:
> + *
> + * - USBCAN firmware does not report M16C "error factors"
> + * - USBCAN controllers has difficulties reporting if the raised error
> + * event is for ch0 or ch1. They leave such arbitration to the OS
> + * driver by letting it compare error counters with previous values
> + * and decide the error event's channel. Thus for USBCAN, the channel
> + * field is only advisory.
> + */
> +struct kvaser_error_summary {
> + u8 channel, status, txerr, rxerr;
> + union {
> + struct {
> + u8 error_factor;
> + } leaf;
> + struct {
> + u8 other_ch_status;
> + u8 error_state;
> + } usbcan;
> + };
> +};
> +
> struct kvaser_usb_tx_urb_context {
> struct kvaser_usb_net_priv *priv;
> u32 echo_index;
> @@ -288,6 +458,7 @@ struct kvaser_usb {
>
> u32 fw_version;
> unsigned int nchannels;
> + enum kvaser_usb_family family;
>
> bool rxinitdone;
> void *rxbuf[MAX_RX_URBS];
> @@ -311,6 +482,7 @@ struct kvaser_usb_net_priv {
> };
>
> static const struct usb_device_id kvaser_usb_table[] = {
> + /* Leaf family IDs */
> { USB_DEVICE(KVASER_VENDOR_ID, USB_LEAF_DEVEL_PRODUCT_ID) },
> { USB_DEVICE(KVASER_VENDOR_ID, USB_LEAF_LITE_PRODUCT_ID) },
> { USB_DEVICE(KVASER_VENDOR_ID, USB_LEAF_PRO_PRODUCT_ID),
> @@ -360,6 +532,17 @@ static const struct usb_device_id kvaser_usb_table[] = {
> .driver_info = KVASER_HAS_TXRX_ERRORS },
> { USB_DEVICE(KVASER_VENDOR_ID, USB_LEAF_LITE_V2_PRODUCT_ID) },
> { USB_DEVICE(KVASER_VENDOR_ID, USB_MINI_PCIE_HS_PRODUCT_ID) },
> +
> + /* USBCANII family IDs */
> + { USB_DEVICE(KVASER_VENDOR_ID, USB_USBCAN2_PRODUCT_ID),
> + .driver_info = KVASER_HAS_TXRX_ERRORS },
> + { USB_DEVICE(KVASER_VENDOR_ID, USB_USBCAN_REVB_PRODUCT_ID),
> + .driver_info = KVASER_HAS_TXRX_ERRORS },
> + { USB_DEVICE(KVASER_VENDOR_ID, USB_MEMORATOR_PRODUCT_ID),
> + .driver_info = KVASER_HAS_TXRX_ERRORS },
> + { USB_DEVICE(KVASER_VENDOR_ID, USB_VCI2_PRODUCT_ID),
> + .driver_info = KVASER_HAS_TXRX_ERRORS },
> +
> { }
> };
> MODULE_DEVICE_TABLE(usb, kvaser_usb_table);
> @@ -463,7 +646,14 @@ static int kvaser_usb_get_software_info(struct kvaser_usb *dev)
> if (err)
> return err;
>
> - dev->fw_version = le32_to_cpu(msg.u.softinfo.fw_version);
> + switch (dev->family) {
> + case KVASER_LEAF:
> + dev->fw_version = le32_to_cpu(msg.u.leaf.softinfo.fw_version);
> + break;
> + case KVASER_USBCAN:
> + dev->fw_version = le32_to_cpu(msg.u.usbcan.softinfo.fw_version);
> + break;
> + }
>
> return 0;
> }
> @@ -482,7 +672,9 @@ static int kvaser_usb_get_card_info(struct kvaser_usb *dev)
> return err;
>
> dev->nchannels = msg.u.cardinfo.nchannels;
> - if (dev->nchannels > MAX_NET_DEVICES)
> + if ((dev->nchannels > MAX_NET_DEVICES) ||
> + (dev->family == KVASER_USBCAN &&
> + dev->nchannels > MAX_USBCAN_NET_DEVICES))
> return -EINVAL;
>
> return 0;
> @@ -496,8 +688,10 @@ static void kvaser_usb_tx_acknowledge(const struct kvaser_usb *dev,
> struct kvaser_usb_net_priv *priv;
> struct sk_buff *skb;
> struct can_frame *cf;
> - u8 channel = msg->u.tx_acknowledge.channel;
> - u8 tid = msg->u.tx_acknowledge.tid;
> + u8 channel, tid;
> +
> + channel = msg->u.tx_acknowledge_header.channel;
> + tid = msg->u.tx_acknowledge_header.tid;
>
> if (channel >= dev->nchannels) {
> dev_err(dev->udev->dev.parent,
> @@ -616,53 +810,24 @@ static void kvaser_usb_unlink_tx_urbs(struct kvaser_usb_net_priv *priv)
> }
>
> static void kvaser_usb_rx_error(const struct kvaser_usb *dev,
> - const struct kvaser_msg *msg)
> + struct kvaser_error_summary *es)
> {
> struct can_frame *cf;
> struct sk_buff *skb;
> struct net_device_stats *stats;
> struct kvaser_usb_net_priv *priv;
> unsigned int new_state;
> - u8 channel, status, txerr, rxerr, error_factor;
> -
> - switch (msg->id) {
> - case CMD_CAN_ERROR_EVENT:
> - channel = msg->u.error_event.channel;
> - status = msg->u.error_event.status;
> - txerr = msg->u.error_event.tx_errors_count;
> - rxerr = msg->u.error_event.rx_errors_count;
> - error_factor = msg->u.error_event.error_factor;
> - break;
> - case CMD_LOG_MESSAGE:
> - channel = msg->u.log_message.channel;
> - status = msg->u.log_message.data[0];
> - txerr = msg->u.log_message.data[2];
> - rxerr = msg->u.log_message.data[3];
> - error_factor = msg->u.log_message.data[1];
> - break;
> - case CMD_CHIP_STATE_EVENT:
> - channel = msg->u.chip_state_event.channel;
> - status = msg->u.chip_state_event.status;
> - txerr = msg->u.chip_state_event.tx_errors_count;
> - rxerr = msg->u.chip_state_event.rx_errors_count;
> - error_factor = 0;
> - break;
> - default:
> - dev_err(dev->udev->dev.parent, "Invalid msg id (%d)\n",
> - msg->id);
> - return;
> - }
>
> - if (channel >= dev->nchannels) {
> + if (es->channel >= dev->nchannels) {
> dev_err(dev->udev->dev.parent,
> - "Invalid channel number (%d)\n", channel);
> + "Invalid channel number (%d)\n", es->channel);
> return;
> }
>
> - priv = dev->nets[channel];
> + priv = dev->nets[es->channel];
> stats = &priv->netdev->stats;
>
> - if (status & M16C_STATE_BUS_RESET) {
> + if (es->status & M16C_STATE_BUS_RESET) {
> kvaser_usb_unlink_tx_urbs(priv);
> return;
> }
> @@ -675,9 +840,9 @@ static void kvaser_usb_rx_error(const struct kvaser_usb *dev,
>
> new_state = priv->can.state;
>
> - netdev_dbg(priv->netdev, "Error status: 0x%02x\n", status);
> + netdev_dbg(priv->netdev, "Error status: 0x%02x\n", es->status);
>
> - if (status & M16C_STATE_BUS_OFF) {
> + if (es->status & M16C_STATE_BUS_OFF) {
> cf->can_id |= CAN_ERR_BUSOFF;
>
> priv->can.can_stats.bus_off++;
> @@ -687,12 +852,12 @@ static void kvaser_usb_rx_error(const struct kvaser_usb *dev,
> netif_carrier_off(priv->netdev);
>
> new_state = CAN_STATE_BUS_OFF;
> - } else if (status & M16C_STATE_BUS_PASSIVE) {
> + } else if (es->status & M16C_STATE_BUS_PASSIVE) {
> if (priv->can.state != CAN_STATE_ERROR_PASSIVE) {
> cf->can_id |= CAN_ERR_CRTL;
>
> - if (txerr || rxerr)
> - cf->data[1] = (txerr > rxerr)
> + if (es->txerr || es->rxerr)
> + cf->data[1] = (es->txerr > es->rxerr)
> ? CAN_ERR_CRTL_TX_PASSIVE
> : CAN_ERR_CRTL_RX_PASSIVE;
> else
> @@ -703,13 +868,11 @@ static void kvaser_usb_rx_error(const struct kvaser_usb *dev,
> }
>
> new_state = CAN_STATE_ERROR_PASSIVE;
> - }
> -
> - if (status == M16C_STATE_BUS_ERROR) {
> + } else if (es->status & M16C_STATE_BUS_ERROR) {
> if ((priv->can.state < CAN_STATE_ERROR_WARNING) &&
> - ((txerr >= 96) || (rxerr >= 96))) {
> + ((es->txerr >= 96) || (es->rxerr >= 96))) {
> cf->can_id |= CAN_ERR_CRTL;
> - cf->data[1] = (txerr > rxerr)
> + cf->data[1] = (es->txerr > es->rxerr)
> ? CAN_ERR_CRTL_TX_WARNING
> : CAN_ERR_CRTL_RX_WARNING;
>
> @@ -723,7 +886,7 @@ static void kvaser_usb_rx_error(const struct kvaser_usb *dev,
> }
> }
>
> - if (!status) {
> + if (!es->status) {
> cf->can_id |= CAN_ERR_PROT;
> cf->data[2] = CAN_ERR_PROT_ACTIVE;
>
> @@ -739,34 +902,48 @@ static void kvaser_usb_rx_error(const struct kvaser_usb *dev,
> priv->can.can_stats.restarts++;
> }
>
> - if (error_factor) {
> - priv->can.can_stats.bus_error++;
> - stats->rx_errors++;
> -
> - cf->can_id |= CAN_ERR_BUSERROR | CAN_ERR_PROT;
> -
> - if (error_factor & M16C_EF_ACKE)
> - cf->data[3] |= (CAN_ERR_PROT_LOC_ACK);
> - if (error_factor & M16C_EF_CRCE)
> - cf->data[3] |= (CAN_ERR_PROT_LOC_CRC_SEQ |
> - CAN_ERR_PROT_LOC_CRC_DEL);
> - if (error_factor & M16C_EF_FORME)
> - cf->data[2] |= CAN_ERR_PROT_FORM;
> - if (error_factor & M16C_EF_STFE)
> - cf->data[2] |= CAN_ERR_PROT_STUFF;
> - if (error_factor & M16C_EF_BITE0)
> - cf->data[2] |= CAN_ERR_PROT_BIT0;
> - if (error_factor & M16C_EF_BITE1)
> - cf->data[2] |= CAN_ERR_PROT_BIT1;
> - if (error_factor & M16C_EF_TRE)
> - cf->data[2] |= CAN_ERR_PROT_TX;
> + switch (dev->family) {
> + case KVASER_LEAF:
> + if (es->leaf.error_factor) {
> + priv->can.can_stats.bus_error++;
> + stats->rx_errors++;
> +
> + cf->can_id |= CAN_ERR_BUSERROR | CAN_ERR_PROT;
> +
> + if (es->leaf.error_factor & M16C_EF_ACKE)
> + cf->data[3] |= (CAN_ERR_PROT_LOC_ACK);
> + if (es->leaf.error_factor & M16C_EF_CRCE)
> + cf->data[3] |= (CAN_ERR_PROT_LOC_CRC_SEQ |
> + CAN_ERR_PROT_LOC_CRC_DEL);
> + if (es->leaf.error_factor & M16C_EF_FORME)
> + cf->data[2] |= CAN_ERR_PROT_FORM;
> + if (es->leaf.error_factor & M16C_EF_STFE)
> + cf->data[2] |= CAN_ERR_PROT_STUFF;
> + if (es->leaf.error_factor & M16C_EF_BITE0)
> + cf->data[2] |= CAN_ERR_PROT_BIT0;
> + if (es->leaf.error_factor & M16C_EF_BITE1)
> + cf->data[2] |= CAN_ERR_PROT_BIT1;
> + if (es->leaf.error_factor & M16C_EF_TRE)
> + cf->data[2] |= CAN_ERR_PROT_TX;
> + }
> + break;
> + case KVASER_USBCAN:
> + if (es->usbcan.error_state & USBCAN_ERROR_STATE_TX_ERROR)
> + stats->tx_errors++;
> + if (es->usbcan.error_state & USBCAN_ERROR_STATE_RX_ERROR)
> + stats->rx_errors++;
> + if (es->usbcan.error_state & USBCAN_ERROR_STATE_BUSERROR) {
> + priv->can.can_stats.bus_error++;
> + cf->can_id |= CAN_ERR_BUSERROR;
> + }
> + break;
> }
>
> - cf->data[6] = txerr;
> - cf->data[7] = rxerr;
> + cf->data[6] = es->txerr;
> + cf->data[7] = es->rxerr;
>
> - priv->bec.txerr = txerr;
> - priv->bec.rxerr = rxerr;
> + priv->bec.txerr = es->txerr;
> + priv->bec.rxerr = es->rxerr;
>
> priv->can.state = new_state;
>
> @@ -775,6 +952,124 @@ static void kvaser_usb_rx_error(const struct kvaser_usb *dev,
> netif_rx(skb);
> }
>
> +/* For USBCAN, report error to userspace iff the channels's errors counter
> + * has increased, or we're the only channel seeing a bus error state.
> + */
> +static void kvaser_usbcan_conditionally_rx_error(const struct kvaser_usb *dev,
> + struct kvaser_error_summary *es)
> +{
> + struct kvaser_usb_net_priv *priv;
> + int channel;
> + bool report_error;
> +
> + channel = es->channel;
> + if (channel >= dev->nchannels) {
> + dev_err(dev->udev->dev.parent,
> + "Invalid channel number (%d)\n", channel);
> + return;
> + }
> +
> + priv = dev->nets[channel];
> + report_error = false;
> +
> + if (es->txerr > priv->bec.txerr) {
> + es->usbcan.error_state |= USBCAN_ERROR_STATE_TX_ERROR;
> + report_error = true;
> + }
> + if (es->rxerr > priv->bec.rxerr) {
> + es->usbcan.error_state |= USBCAN_ERROR_STATE_RX_ERROR;
> + report_error = true;
> + }
> + if ((es->status & M16C_STATE_BUS_ERROR) &&
> + !(es->usbcan.other_ch_status & M16C_STATE_BUS_ERROR)) {
> + es->usbcan.error_state |= USBCAN_ERROR_STATE_BUSERROR;
> + report_error = true;
> + }
> +
> + if (report_error)
> + kvaser_usb_rx_error(dev, es);
> +}
> +
> +static void kvaser_usbcan_rx_error(const struct kvaser_usb *dev,
> + const struct kvaser_msg *msg)
> +{
> + struct kvaser_error_summary es = { };
> +
> + switch (msg->id) {
> + /* Sometimes errors are sent as unsolicited chip state events */
> + case CMD_CHIP_STATE_EVENT:
> + es.channel = msg->u.usbcan.chip_state_event.channel;
> + es.status = msg->u.usbcan.chip_state_event.status;
> + es.txerr = msg->u.usbcan.chip_state_event.tx_errors_count;
> + es.rxerr = msg->u.usbcan.chip_state_event.rx_errors_count;
> + kvaser_usbcan_conditionally_rx_error(dev, &es);
> + break;
> +
> + case CMD_CAN_ERROR_EVENT:
> + es.channel = 0;
> + es.status = msg->u.usbcan.error_event.status_ch0;
> + es.txerr = msg->u.usbcan.error_event.tx_errors_count_ch0;
> + es.rxerr = msg->u.usbcan.error_event.rx_errors_count_ch0;
> + es.usbcan.other_ch_status =
> + msg->u.usbcan.error_event.status_ch1;
> + kvaser_usbcan_conditionally_rx_error(dev, &es);
> +
> + /* The USBCAN firmware does not support more than 2 channels.
> + * Now that ch0 was checked, check if ch1 has any errors.
> + */
> + if (dev->nchannels == MAX_USBCAN_NET_DEVICES) {
> + es.channel = 1;
> + es.status = msg->u.usbcan.error_event.status_ch1;
> + es.txerr = msg->u.usbcan.error_event.tx_errors_count_ch1;
> + es.rxerr = msg->u.usbcan.error_event.rx_errors_count_ch1;
> + es.usbcan.other_ch_status =
> + msg->u.usbcan.error_event.status_ch0;
> + kvaser_usbcan_conditionally_rx_error(dev, &es);
> + }
> + break;
> +
> + default:
> + dev_err(dev->udev->dev.parent, "Invalid msg id (%d)\n",
> + msg->id);
> + }
> +}
> +
> +static void kvaser_leaf_rx_error(const struct kvaser_usb *dev,
> + const struct kvaser_msg *msg)
> +{
> + struct kvaser_error_summary es = { };
> +
> + switch (msg->id) {
> + case CMD_CAN_ERROR_EVENT:
> + es.channel = msg->u.leaf.error_event.channel;
> + es.status = msg->u.leaf.error_event.status;
> + es.txerr = msg->u.leaf.error_event.tx_errors_count;
> + es.rxerr = msg->u.leaf.error_event.rx_errors_count;
> + es.leaf.error_factor = msg->u.leaf.error_event.error_factor;
> + break;
> + case CMD_LEAF_LOG_MESSAGE:
> + es.channel = msg->u.leaf.log_message.channel;
> + es.status = msg->u.leaf.log_message.data[0];
> + es.txerr = msg->u.leaf.log_message.data[2];
> + es.rxerr = msg->u.leaf.log_message.data[3];
> + es.leaf.error_factor = msg->u.leaf.log_message.data[1];
> + break;
> + case CMD_CHIP_STATE_EVENT:
> + es.channel = msg->u.leaf.chip_state_event.channel;
> + es.status = msg->u.leaf.chip_state_event.status;
> + es.txerr = msg->u.leaf.chip_state_event.tx_errors_count;
> + es.rxerr = msg->u.leaf.chip_state_event.rx_errors_count;
> + es.leaf.error_factor = 0;
> + break;
> + default:
> + dev_err(dev->udev->dev.parent, "Invalid msg id (%d)\n",
> + msg->id);
> + return;
> + }
> +
> + kvaser_usb_rx_error(dev, &es);
> +}
> +
> static void kvaser_usb_rx_can_err(const struct kvaser_usb_net_priv *priv,
> const struct kvaser_msg *msg)
> {
> @@ -782,16 +1077,16 @@ static void kvaser_usb_rx_can_err(const struct kvaser_usb_net_priv *priv,
> struct sk_buff *skb;
> struct net_device_stats *stats = &priv->netdev->stats;
>
> - if (msg->u.rx_can.flag & (MSG_FLAG_ERROR_FRAME |
> + if (msg->u.rx_can_header.flag & (MSG_FLAG_ERROR_FRAME |
> MSG_FLAG_NERR)) {
> netdev_err(priv->netdev, "Unknow error (flags: 0x%02x)\n",
> - msg->u.rx_can.flag);
> + msg->u.rx_can_header.flag);
>
> stats->rx_errors++;
> return;
> }
>
> - if (msg->u.rx_can.flag & MSG_FLAG_OVERRUN) {
> + if (msg->u.rx_can_header.flag & MSG_FLAG_OVERRUN) {
> stats->rx_over_errors++;
> stats->rx_errors++;
>
> @@ -817,7 +1112,8 @@ static void kvaser_usb_rx_can_msg(const struct kvaser_usb *dev,
> struct can_frame *cf;
> struct sk_buff *skb;
> struct net_device_stats *stats;
> - u8 channel = msg->u.rx_can.channel;
> + u8 channel = msg->u.rx_can_header.channel;
> + const u8 *rx_msg;
>
> if (channel >= dev->nchannels) {
> dev_err(dev->udev->dev.parent,
> @@ -828,19 +1124,32 @@ static void kvaser_usb_rx_can_msg(const struct kvaser_usb *dev,
> priv = dev->nets[channel];
> stats = &priv->netdev->stats;
>
> - if ((msg->u.rx_can.flag & MSG_FLAG_ERROR_FRAME) &&
> - (msg->id == CMD_LOG_MESSAGE)) {
> - kvaser_usb_rx_error(dev, msg);
> + if ((msg->u.rx_can_header.flag & MSG_FLAG_ERROR_FRAME) &&
> + (dev->family == KVASER_LEAF && msg->id == CMD_LEAF_LOG_MESSAGE)) {
> + kvaser_leaf_rx_error(dev, msg);
> return;
> - } else if (msg->u.rx_can.flag & (MSG_FLAG_ERROR_FRAME |
> - MSG_FLAG_NERR |
> - MSG_FLAG_OVERRUN)) {
> + } else if (msg->u.rx_can_header.flag & (MSG_FLAG_ERROR_FRAME |
> + MSG_FLAG_NERR |
> + MSG_FLAG_OVERRUN)) {
> kvaser_usb_rx_can_err(priv, msg);
> return;
> - } else if (msg->u.rx_can.flag & ~MSG_FLAG_REMOTE_FRAME) {
> + } else if (msg->u.rx_can_header.flag & ~MSG_FLAG_REMOTE_FRAME) {
> netdev_warn(priv->netdev,
> "Unhandled frame (flags: 0x%02x)",
> - msg->u.rx_can.flag);
> + msg->u.rx_can_header.flag);
> + return;
> + }
> +
> + switch (dev->family) {
> + case KVASER_LEAF:
> + rx_msg = msg->u.leaf.rx_can.msg;
> + break;
> + case KVASER_USBCAN:
> + rx_msg = msg->u.usbcan.rx_can.msg;
> + break;
> + default:
> + dev_err(dev->udev->dev.parent,
> + "Invalid device family (%d)\n", dev->family);
> return;
> }
>
> @@ -850,38 +1159,37 @@ static void kvaser_usb_rx_can_msg(const struct kvaser_usb *dev,
> return;
> }
>
> - if (msg->id == CMD_LOG_MESSAGE) {
> - cf->can_id = le32_to_cpu(msg->u.log_message.id);
> + if (dev->family == KVASER_LEAF && msg->id == CMD_LEAF_LOG_MESSAGE) {
> + cf->can_id = le32_to_cpu(msg->u.leaf.log_message.id);
> if (cf->can_id & KVASER_EXTENDED_FRAME)
> cf->can_id &= CAN_EFF_MASK | CAN_EFF_FLAG;
> else
> cf->can_id &= CAN_SFF_MASK;
>
> - cf->can_dlc = get_can_dlc(msg->u.log_message.dlc);
> + cf->can_dlc = get_can_dlc(msg->u.leaf.log_message.dlc);
>
> - if (msg->u.log_message.flags & MSG_FLAG_REMOTE_FRAME)
> + if (msg->u.leaf.log_message.flags & MSG_FLAG_REMOTE_FRAME)
> cf->can_id |= CAN_RTR_FLAG;
> else
> - memcpy(cf->data, &msg->u.log_message.data,
> + memcpy(cf->data, &msg->u.leaf.log_message.data,
> cf->can_dlc);
> } else {
> - cf->can_id = ((msg->u.rx_can.msg[0] & 0x1f) << 6) |
> - (msg->u.rx_can.msg[1] & 0x3f);
> + cf->can_id = ((rx_msg[0] & 0x1f) << 6) | (rx_msg[1] & 0x3f);
>
> if (msg->id == CMD_RX_EXT_MESSAGE) {
> cf->can_id <<= 18;
> - cf->can_id |= ((msg->u.rx_can.msg[2] & 0x0f) << 14) |
> - ((msg->u.rx_can.msg[3] & 0xff) << 6) |
> - (msg->u.rx_can.msg[4] & 0x3f);
> + cf->can_id |= ((rx_msg[2] & 0x0f) << 14) |
> + ((rx_msg[3] & 0xff) << 6) |
> + (rx_msg[4] & 0x3f);
> cf->can_id |= CAN_EFF_FLAG;
> }
>
> - cf->can_dlc = get_can_dlc(msg->u.rx_can.msg[5]);
> + cf->can_dlc = get_can_dlc(rx_msg[5]);
>
> - if (msg->u.rx_can.flag & MSG_FLAG_REMOTE_FRAME)
> + if (msg->u.rx_can_header.flag & MSG_FLAG_REMOTE_FRAME)
> cf->can_id |= CAN_RTR_FLAG;
> else
> - memcpy(cf->data, &msg->u.rx_can.msg[6],
> + memcpy(cf->data, &rx_msg[6],
> cf->can_dlc);
> }
>
> @@ -944,21 +1252,35 @@ static void kvaser_usb_handle_message(const struct kvaser_usb *dev,
>
> case CMD_RX_STD_MESSAGE:
> case CMD_RX_EXT_MESSAGE:
> - case CMD_LOG_MESSAGE:
> + kvaser_usb_rx_can_msg(dev, msg);
> + break;
> +
> + case CMD_LEAF_LOG_MESSAGE:
> + if (dev->family != KVASER_LEAF)
> + goto warn;
> kvaser_usb_rx_can_msg(dev, msg);
> break;
>
> case CMD_CHIP_STATE_EVENT:
> case CMD_CAN_ERROR_EVENT:
> - kvaser_usb_rx_error(dev, msg);
> + if (dev->family == KVASER_LEAF)
> + kvaser_leaf_rx_error(dev, msg);
> + else
> + kvaser_usbcan_rx_error(dev, msg);
> break;
>
> case CMD_TX_ACKNOWLEDGE:
> kvaser_usb_tx_acknowledge(dev, msg);
> break;
>
> + /* Ignored messages */
> + case CMD_USBCAN_CLOCK_OVERFLOW_EVENT:
> + if (dev->family != KVASER_USBCAN)
> + goto warn;
> + break;
> +
> default:
> - dev_warn(dev->udev->dev.parent,
> +warn: dev_warn(dev->udev->dev.parent,
> "Unhandled message (%d)\n", msg->id);
> break;
> }
> @@ -1178,7 +1500,7 @@ static void kvaser_usb_unlink_all_urbs(struct kvaser_usb *dev)
> dev->rxbuf[i],
> dev->rxbuf_dma[i]);
>
> - for (i = 0; i < MAX_NET_DEVICES; i++) {
> + for (i = 0; i < dev->nchannels; i++) {
> struct kvaser_usb_net_priv *priv = dev->nets[i];
>
> if (priv)
> @@ -1286,6 +1608,7 @@ static netdev_tx_t kvaser_usb_start_xmit(struct sk_buff *skb,
> struct kvaser_msg *msg;
> int i, err;
> int ret = NETDEV_TX_OK;
> + uint8_t *msg_tx_can_flags;
>
> if (can_dropped_invalid_skb(netdev, skb))
> return NETDEV_TX_OK;
> @@ -1307,9 +1630,23 @@ static netdev_tx_t kvaser_usb_start_xmit(struct sk_buff *skb,
>
> msg = buf;
> msg->len = MSG_HEADER_LEN + sizeof(struct kvaser_msg_tx_can);
> - msg->u.tx_can.flags = 0;
> msg->u.tx_can.channel = priv->channel;
>
> + switch (dev->family) {
> + case KVASER_LEAF:
> + msg_tx_can_flags = &msg->u.tx_can.leaf.flags;
> + break;
> + case KVASER_USBCAN:
> + msg_tx_can_flags = &msg->u.tx_can.usbcan.flags;
> + break;
> + default:
> + dev_err(dev->udev->dev.parent,
> + "Invalid device family (%d)\n", dev->family);
> + goto releasebuf;
> + }
> +
> + *msg_tx_can_flags = 0;
> +
> if (cf->can_id & CAN_EFF_FLAG) {
> msg->id = CMD_TX_EXT_MESSAGE;
> msg->u.tx_can.msg[0] = (cf->can_id >> 24) & 0x1f;
> @@ -1327,7 +1664,7 @@ static netdev_tx_t kvaser_usb_start_xmit(struct sk_buff *skb,
> memcpy(&msg->u.tx_can.msg[6], cf->data, cf->can_dlc);
>
> if (cf->can_id & CAN_RTR_FLAG)
> - msg->u.tx_can.flags |= MSG_FLAG_REMOTE_FRAME;
> + *msg_tx_can_flags |= MSG_FLAG_REMOTE_FRAME;
>
> for (i = 0; i < ARRAY_SIZE(priv->tx_contexts); i++) {
> if (priv->tx_contexts[i].echo_index == MAX_TX_URBS) {
> @@ -1596,6 +1933,17 @@ static int kvaser_usb_probe(struct usb_interface *intf,
> if (!dev)
> return -ENOMEM;
>
> + if (kvaser_is_leaf(id)) {
> + dev->family = KVASER_LEAF;
> + } else if (kvaser_is_usbcan(id)) {
> + dev->family = KVASER_USBCAN;
> + } else {
> + dev_err(&intf->dev,
> + "Product ID (%d) does not belong to any known Kvaser USB family",
> + id->idProduct);
> + return -ENODEV;
> + }
> +
> err = kvaser_usb_get_endpoints(intf, &dev->bulk_in, &dev->bulk_out);
> if (err) {
> dev_err(&intf->dev, "Cannot get usb endpoint(s)");
> --
> 1.9.1
>
^ permalink raw reply
* Re: [PATCH v4 4/4] can: kvaser_usb: Retry first bulk transfer on -ETIMEDOUT
From: Ahmed S. Darwish @ 2015-01-12 13:50 UTC (permalink / raw)
To: Olivier Sobrie
Cc: Marc Kleine-Budde, Oliver Hartkopp, Wolfgang Grandegger,
Greg Kroah-Hartman, Linux-USB, Linux-CAN, netdev, LKML
In-Reply-To: <20150112133330.GA18351@hposo>
On Mon, Jan 12, 2015 at 02:33:30PM +0100, Olivier Sobrie wrote:
> Hello,
>
> On Mon, Jan 12, 2015 at 05:14:07AM -0500, Ahmed S. Darwish wrote:
> > On Sun, Jan 11, 2015 at 09:51:10PM +0100, Marc Kleine-Budde wrote:
> > > On 01/11/2015 09:45 PM, Ahmed S. Darwish wrote:
> > > > From: Ahmed S. Darwish <ahmed.darwish@valeo.com>
> > > >
> > > > (This is a draft patch, I'm not sure if this fixes the USB
> > > > bug or only its psymptom. Feedback from the linux-usb folks
> > > > is really appreciated.)
> > > >
> > > > When plugging the Kvaser USB/CAN dongle the first time, everything
> > > > works as expected and all of the transfers from and to the USB
> > > > device succeeds.
> > > >
> > > > Meanwhile, after unplugging the device and plugging it again, the
> > > > first bulk transfer _always_ returns an -ETIMEDOUT. The following
> > > > behaviour was observied:
> > > >
> > > > - Setting higher timeout values for the first bulk transfer never
> > > > solved the issue.
> > > >
> > > > - Unloading, then loading, our kvaser_usb module in question
> > > > __always__ solved the issue.
> > > >
> > > > - Checking first bulk transfer status, and retry the transfer
> > > > again in case of an -ETIMEDOUT also __always__ solved the issue.
> > > > This is what the patch below does.
> > > >
> > > > - In the testing done so far, this issue appears only on laptops
> > > > but never on PCs (possibly power related?)
> > > >
> > > > Signed-off-by: Ahmed S. Darwish <ahmed.darwish@valeo.com>
> > >
> > > Does this patch apply apply between 3 and 4? If not, please re-arrange
> > > the series. As this is a bug fix, patches 1, 2 and 4 will go via
> > > net/master, 3 will go via net-next/master.
> > >
> >
> > Since no one complained earlier, I guess this issue only affects
> > USBCAN devices. That's why I've based it above patch #3: adding
> > USBCAN hardware support.
> >
> > Nonetheless, it won't do any harm for the current Leaf-only
> > driver. So _if_ this is the correct fix, I will update the commit
> > log, refactor the check into a 'do { } while()' loop, and then
> > base it above the Leaf-only net/master fixes on patch #1, and #2.
> >
> > Any feedback on the USB side of things?
>
> Can you take a wireshark capture showing the problem?
> It can maybe help people to figure out what happens.
>
Yeah, I'm planning on doing something similar.
> What kind of usbcan device do you use?
"Kvaser USBcan II HS/LS"
> Which firmware revision is loaded on the device?
>
The device reports firmware version 2.9.410.
Interesting. The changelog of their latest firmware states
that it "fixed USB configuration issue during USB attach."
That might be the problem.
I have two devices here. I'll update the firmware only for
one of them and see what happens.
Thanks,
Darwish
^ permalink raw reply
* RE: [PATCH v4] can: Convert to runtime_pm
From: Appana Durga Kedareswara Rao @ 2015-01-12 13:49 UTC (permalink / raw)
To: Marc Kleine-Budde
Cc: linux-can@vger.kernel.org, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org, Soren Brinkmann,
grant.likely@linaro.org, wg@grandegger.com, Michal Simek
In-Reply-To: <54B3CB51.3060207@pengutronix.de>
Hi Marc,
> -----Original Message-----
> From: Marc Kleine-Budde [mailto:mkl@pengutronix.de]
> Sent: Monday, January 12, 2015 6:56 PM
> To: Appana Durga Kedareswara Rao
> Cc: linux-can@vger.kernel.org; netdev@vger.kernel.org; linux-
> kernel@vger.kernel.org; Soren Brinkmann; grant.likely@linaro.org;
> wg@grandegger.com; Michal Simek
> Subject: Re: [PATCH v4] can: Convert to runtime_pm
>
> On 01/12/2015 07:59 AM, Appana Durga Kedareswara Rao wrote:
> > Hi Marc,
> >
> >> -----Original Message-----
> >> From: Marc Kleine-Budde [mailto:mkl@pengutronix.de]
> >> Sent: Sunday, January 11, 2015 9:11 PM
> >> To: Appana Durga Kedareswara Rao
> >> Cc: linux-can@vger.kernel.org; netdev@vger.kernel.org; linux-
> >> kernel@vger.kernel.org; Soren Brinkmann; grant.likely@linaro.org;
> >> wg@grandegger.com
> >> Subject: Re: [PATCH v4] can: Convert to runtime_pm
> >>
> >> On 01/11/2015 06:34 AM, Appana Durga Kedareswara Rao wrote:
> >> [...]
> >>>>> return ret;
> >>>>> }
> >>>>>
> >>>>> priv->write_reg(priv, XCAN_MSR_OFFSET, 0);
> >>>>> priv->write_reg(priv, XCAN_SRR_OFFSET, XCAN_SRR_CEN_MASK);
> >>>>>
> >>>>> if (netif_running(ndev)) {
> >>>>> priv->can.state = CAN_STATE_ERROR_ACTIVE;
> >>>>
> >>>> What happens if the device was not in ACTIVE state prior to the
> >>>> runtime_suspend?
> >>>>
> >>>
> >>> I am not sure about the state of CAN at this point of time.
> >>> I just followed what other drivers are following for run time suspend :).
> >>
> >> Please check the state of the hardware if you go with bus off into
> >> suspend and then resume.
> >>
> >
> > if (netif_running(ndev)) {
> > if (isr & XCAN_IXR_BSOFF_MASK) {
> > priv->can.state = CAN_STATE_BUS_OFF;
> > priv->write_reg(priv, XCAN_SRR_OFFSET,
> > XCAN_SRR_RESET_MASK);
> > } else if ((status & XCAN_SR_ESTAT_MASK) ==
> > XCAN_SR_ESTAT_MASK) {
> > priv->can.state = CAN_STATE_ERROR_PASSIVE;
> > } else if (status & XCAN_SR_ERRWRN_MASK) {
> > priv->can.state = CAN_STATE_ERROR_WARNING;
> > } else {
> > priv->can.state = CAN_STATE_ERROR_ACTIVE;
> > }
> > }
> >
> > Is the above code snippet ok for you?
>
> Yes, but what's the state of the hardware when it wakes up again?
It depends on the previous state of the CAN.
I mean In Suspend we are putting the device in sleep mode and in resume we are waking up by putting the device into the
Configuration mode. We are not doing any reset of the core in the suspend/resume so it depends on the previous state of the CAN
when it wakes up that's why checking for the status of the CAN in the status register here to put the device in appropriate mode.
>
> >
> >>>>> netif_device_attach(ndev);
> >>>>> netif_start_queue(ndev);
> >>>>> }
> >>>>>
> >>>>> return 0;
> >>>>> }
> >>>>
> >>>>
> >>>>>
> >>>>> @@ -1020,9 +1035,9 @@ static int __maybe_unused
> xcan_resume(struct
> >>>>> device *dev)
> >>>>>
> >>>>> 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;
> >>>>>
> >>>>> if (netif_running(ndev)) {
> >>>>> + priv->can.state = CAN_STATE_ERROR_ACTIVE;
> >>>>> netif_device_attach(ndev);
> >>>>> netif_start_queue(ndev);
> >>>>> }
> >>>>> @@ -1030,7 +1045,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 +1089,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,15
> >>>>> +1155,22 @@ 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);
> >>>>> + pm_runtime_enable(&pdev->dev);
> >>>>> + pm_runtime_get_sync(&pdev->dev);
> >>>> Check error values?
> >>>
> >>> Ok
> >>>>> +
> >>>>> ret = register_candev(ndev);
> >>>>> if (ret) {
> >>>>> dev_err(&pdev->dev, "fail to register failed
> >>>>> (err=%d)\n",
> >>>> ret);
> >>>>> + pm_runtime_put(priv->dev);
> >>>>
> >>>> Please move the pm_runtime_put into the common error exit path.
> >>>>
> >>>
> >>> Ok
> >>>
> >>>>> goto err_unprepare_disable_busclk;
> >>>>> }
> >>>>>
> >>>>> 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);
> >>>>>
> >>>>
> >>>> I think you have to convert the _remove() function, too. Have a
> >>>> look at the gpio-zynq.c driver:
> >>>>
> >>>>> static int zynq_gpio_remove(struct platform_device *pdev) {
> >>>>> struct zynq_gpio *gpio = platform_get_drvdata(pdev);
> >>>>>
> >>>>> pm_runtime_get_sync(&pdev->dev);
> >>>>
> >>>> However I don't understand why the get_sync() is here. Maybe Sören
> >>>> can help?
> >>>
> >>> I converted the remove function to use the run-time PM and .
> >>> Below is the remove code snippet.
> >>>
> >>> ret = pm_runtime_get_sync(&pdev->dev);
> >>> if (ret < 0) {
> >>> netdev_err(ndev, "%s: pm_runtime_get failed(%d)\n",
> >>> __func__, ret);
> >>> return ret;
> >>> }
> >>>
> >>> if (set_reset_mode(ndev) < 0)
> >>> netdev_err(ndev, "mode resetting failed!\n");
> >>>
> >>> unregister_candev(ndev);
> >>> netif_napi_del(&priv->napi);
> >>> free_candev(ndev);
> >>
> >>> pm_runtime_disable(&pdev->dev);
> >>
> >> Can this make a call to xcan_runtime_*()? I'm asking since the ndev
> >> has been unregistered and already free()ed. Better move this directly
> >> after the set_reset_mode(). This way you are symmetric to the probe()
> function.
> >
> > If I move the pm_runtime_disable after the set_reset_mode I am
> > getting the below error.
> > ERROR:
> > xilinx_can e0008000.can can0 (unregistering): xcan_get_berr_counter:
> > pm_runtime_get fail
> >
> > If I move the pm_runtime_disable after unregister_candev everything is
> working fine.
>
> Fine - but who calls xcan_get_berr_counter here? Can you add a
> dump_stack() here?
>
I think it is getting called from the atomic context.
When I am trying to do a rmmod I am getting the above error.
ERROR:
xilinx_can e0008000.can can0 (unregistering): xcan_get_berr_counter:
pm_runtime_get fail.
I am getting only the above error in the console when I do rmmod.
Regards,
Kedar.
> 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 |
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: [PATCH v4 4/4] can: kvaser_usb: Retry first bulk transfer on -ETIMEDOUT
From: Olivier Sobrie @ 2015-01-12 13:33 UTC (permalink / raw)
To: Ahmed S. Darwish
Cc: Marc Kleine-Budde, Oliver Hartkopp, Wolfgang Grandegger,
Greg Kroah-Hartman, Linux-USB, Linux-CAN, netdev, LKML
In-Reply-To: <20150112101407.GA9213@linux>
Hello,
On Mon, Jan 12, 2015 at 05:14:07AM -0500, Ahmed S. Darwish wrote:
> On Sun, Jan 11, 2015 at 09:51:10PM +0100, Marc Kleine-Budde wrote:
> > On 01/11/2015 09:45 PM, Ahmed S. Darwish wrote:
> > > From: Ahmed S. Darwish <ahmed.darwish@valeo.com>
> > >
> > > (This is a draft patch, I'm not sure if this fixes the USB
> > > bug or only its psymptom. Feedback from the linux-usb folks
> > > is really appreciated.)
> > >
> > > When plugging the Kvaser USB/CAN dongle the first time, everything
> > > works as expected and all of the transfers from and to the USB
> > > device succeeds.
> > >
> > > Meanwhile, after unplugging the device and plugging it again, the
> > > first bulk transfer _always_ returns an -ETIMEDOUT. The following
> > > behaviour was observied:
> > >
> > > - Setting higher timeout values for the first bulk transfer never
> > > solved the issue.
> > >
> > > - Unloading, then loading, our kvaser_usb module in question
> > > __always__ solved the issue.
> > >
> > > - Checking first bulk transfer status, and retry the transfer
> > > again in case of an -ETIMEDOUT also __always__ solved the issue.
> > > This is what the patch below does.
> > >
> > > - In the testing done so far, this issue appears only on laptops
> > > but never on PCs (possibly power related?)
> > >
> > > Signed-off-by: Ahmed S. Darwish <ahmed.darwish@valeo.com>
> >
> > Does this patch apply apply between 3 and 4? If not, please re-arrange
> > the series. As this is a bug fix, patches 1, 2 and 4 will go via
> > net/master, 3 will go via net-next/master.
> >
>
> Since no one complained earlier, I guess this issue only affects
> USBCAN devices. That's why I've based it above patch #3: adding
> USBCAN hardware support.
>
> Nonetheless, it won't do any harm for the current Leaf-only
> driver. So _if_ this is the correct fix, I will update the commit
> log, refactor the check into a 'do { } while()' loop, and then
> base it above the Leaf-only net/master fixes on patch #1, and #2.
>
> Any feedback on the USB side of things?
Can you take a wireshark capture showing the problem?
It can maybe help people to figure out what happens.
What kind of usbcan device do you use?
Which firmware revision is loaded on the device?
Kr,
--
Olivier
^ permalink raw reply
* Re: [PATCH v4] can: Convert to runtime_pm
From: Marc Kleine-Budde @ 2015-01-12 13:25 UTC (permalink / raw)
To: Appana Durga Kedareswara Rao
Cc: linux-can@vger.kernel.org, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org, Soren Brinkmann,
grant.likely@linaro.org, wg@grandegger.com, Michal Simek
In-Reply-To: <bb20847c9546459592241f801be2b536@BY2FFO11FD027.protection.gbl>
[-- Attachment #1: Type: text/plain, Size: 6749 bytes --]
On 01/12/2015 07:59 AM, Appana Durga Kedareswara Rao wrote:
> Hi Marc,
>
>> -----Original Message-----
>> From: Marc Kleine-Budde [mailto:mkl@pengutronix.de]
>> Sent: Sunday, January 11, 2015 9:11 PM
>> To: Appana Durga Kedareswara Rao
>> Cc: linux-can@vger.kernel.org; netdev@vger.kernel.org; linux-
>> kernel@vger.kernel.org; Soren Brinkmann; grant.likely@linaro.org;
>> wg@grandegger.com
>> Subject: Re: [PATCH v4] can: Convert to runtime_pm
>>
>> On 01/11/2015 06:34 AM, Appana Durga Kedareswara Rao wrote:
>> [...]
>>>>> return ret;
>>>>> }
>>>>>
>>>>> priv->write_reg(priv, XCAN_MSR_OFFSET, 0);
>>>>> priv->write_reg(priv, XCAN_SRR_OFFSET, XCAN_SRR_CEN_MASK);
>>>>>
>>>>> if (netif_running(ndev)) {
>>>>> priv->can.state = CAN_STATE_ERROR_ACTIVE;
>>>>
>>>> What happens if the device was not in ACTIVE state prior to the
>>>> runtime_suspend?
>>>>
>>>
>>> I am not sure about the state of CAN at this point of time.
>>> I just followed what other drivers are following for run time suspend :).
>>
>> Please check the state of the hardware if you go with bus off into suspend
>> and then resume.
>>
>
> if (netif_running(ndev)) {
> if (isr & XCAN_IXR_BSOFF_MASK) {
> priv->can.state = CAN_STATE_BUS_OFF;
> priv->write_reg(priv, XCAN_SRR_OFFSET,
> XCAN_SRR_RESET_MASK);
> } else if ((status & XCAN_SR_ESTAT_MASK) ==
> XCAN_SR_ESTAT_MASK) {
> priv->can.state = CAN_STATE_ERROR_PASSIVE;
> } else if (status & XCAN_SR_ERRWRN_MASK) {
> priv->can.state = CAN_STATE_ERROR_WARNING;
> } else {
> priv->can.state = CAN_STATE_ERROR_ACTIVE;
> }
> }
>
> Is the above code snippet ok for you?
Yes, but what's the state of the hardware when it wakes up again?
>
>>>>> netif_device_attach(ndev);
>>>>> netif_start_queue(ndev);
>>>>> }
>>>>>
>>>>> return 0;
>>>>> }
>>>>
>>>>
>>>>>
>>>>> @@ -1020,9 +1035,9 @@ static int __maybe_unused xcan_resume(struct
>>>>> device *dev)
>>>>>
>>>>> 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;
>>>>>
>>>>> if (netif_running(ndev)) {
>>>>> + priv->can.state = CAN_STATE_ERROR_ACTIVE;
>>>>> netif_device_attach(ndev);
>>>>> netif_start_queue(ndev);
>>>>> }
>>>>> @@ -1030,7 +1045,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 +1089,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,15
>>>>> +1155,22 @@ 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);
>>>>> + pm_runtime_enable(&pdev->dev);
>>>>> + pm_runtime_get_sync(&pdev->dev);
>>>> Check error values?
>>>
>>> Ok
>>>>> +
>>>>> ret = register_candev(ndev);
>>>>> if (ret) {
>>>>> dev_err(&pdev->dev, "fail to register failed
>>>>> (err=%d)\n",
>>>> ret);
>>>>> + pm_runtime_put(priv->dev);
>>>>
>>>> Please move the pm_runtime_put into the common error exit path.
>>>>
>>>
>>> Ok
>>>
>>>>> goto err_unprepare_disable_busclk;
>>>>> }
>>>>>
>>>>> 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);
>>>>>
>>>>
>>>> I think you have to convert the _remove() function, too. Have a look
>>>> at the gpio-zynq.c driver:
>>>>
>>>>> static int zynq_gpio_remove(struct platform_device *pdev) {
>>>>> struct zynq_gpio *gpio = platform_get_drvdata(pdev);
>>>>>
>>>>> pm_runtime_get_sync(&pdev->dev);
>>>>
>>>> However I don't understand why the get_sync() is here. Maybe Sören
>>>> can help?
>>>
>>> I converted the remove function to use the run-time PM and .
>>> Below is the remove code snippet.
>>>
>>> ret = pm_runtime_get_sync(&pdev->dev);
>>> if (ret < 0) {
>>> netdev_err(ndev, "%s: pm_runtime_get failed(%d)\n",
>>> __func__, ret);
>>> return ret;
>>> }
>>>
>>> if (set_reset_mode(ndev) < 0)
>>> netdev_err(ndev, "mode resetting failed!\n");
>>>
>>> unregister_candev(ndev);
>>> netif_napi_del(&priv->napi);
>>> free_candev(ndev);
>>
>>> pm_runtime_disable(&pdev->dev);
>>
>> Can this make a call to xcan_runtime_*()? I'm asking since the ndev has
>> been unregistered and already free()ed. Better move this directly after the
>> set_reset_mode(). This way you are symmetric to the probe() function.
>
> If I move the pm_runtime_disable after the set_reset_mode
> I am getting the below error.
> ERROR:
> xilinx_can e0008000.can can0 (unregistering): xcan_get_berr_counter: pm_runtime_get fail
>
> If I move the pm_runtime_disable after unregister_candev everything is working fine.
Fine - but who calls xcan_get_berr_counter here? Can you add a
dump_stack() here?
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: Fwd: [rhashtable] WARNING: CPU: 0 PID: 10 at kernel/locking/mutex.c:570 mutex_lock_nested()
From: Thomas Graf @ 2015-01-12 12:42 UTC (permalink / raw)
To: Ying Xue; +Cc: linux-kernel, lkp, Netdev
In-Reply-To: <54B3259B.7080601@windriver.com>
On 01/12/15 at 09:38am, Ying Xue wrote:
> Hi Thomas,
>
> I am really unable to see where is wrong leading to below warning
> complaints. Can you please help me check it?
Not sure yet. It's not your patch that introduced the issue though.
It merely exposed the affected code path.
Just wondering, did you test with CONFIG_DEBUG_MUTEXES enabled?
^ permalink raw reply
* Re: [PATCH v3] ath10k: fixup wait_for_completion_timeout return handling
From: Kalle Valo @ 2015-01-12 12:39 UTC (permalink / raw)
To: Nicholas Mc Guire
Cc: Chun-Yeow Yeoh, Sergei Shtylyov, netdev, linux-wireless,
linux-kernel, ath10k, Michal Kazior, Yanbo Li, Ben Greear
In-Reply-To: <1420720054-27870-1-git-send-email-der.herr@hofr.at>
Nicholas Mc Guire <der.herr@hofr.at> writes:
> wait_for_completion_timeout does not return negative values so the tests
> for <= 0 are not needed and the case differentiation in the error handling
> path unnecessary.
>
> v2: all wait_for_completion_timeout changes in a single patch
> v3: patch formatting cleanups - no change of actual patch
>
> Signed-off-by: Nicholas Mc Guire <der.herr@hofr.at>
> ---
> patch was only compile tested x86_64_defconfig + CONFIG_ATH_CARDS=m
> CONFIG_ATH10K=m
>
> patch is against linux-next 3.19.0-rc1 -next-20141226
>
> None of the proposed cleanups are critical.
> All changes should only be removing unreachable cases.
Sorry, I wasn't clear enough in my last email but everything related to
v2, v3 etc should be after "---" line. I fixed this in ath-next-test
branch and will apply your patch once kbuild has run it's tests.
--
Kalle Valo
^ permalink raw reply
* Re: [PATCH] usb/kaweth: use GFP_ATOMIC under spin_lock in usb_start_wait_urb()
From: Oliver Neukum @ 2015-01-12 12:37 UTC (permalink / raw)
To: Alexey Khoroshilov
Cc: David S. Miller, linux-usb, netdev, linux-kernel, ldv-project
In-Reply-To: <1420845382-25815-1-git-send-email-khoroshilov@ispras.ru>
On Sat, 2015-01-10 at 02:16 +0300, Alexey Khoroshilov wrote:
> Commit e4c7f259c5be ("USB: kaweth.c: use GFP_ATOMIC under spin_lock")
> makes sure that kaweth_internal_control_msg() allocates memory with
> GFP_ATOMIC,
> but kaweth_internal_control_msg() also calls usb_start_wait_urb()
> that still allocates memory with GFP_NOIO.
>
> The patch fixes usb_start_wait_urb() as well.
>
> Found by Linux Driver Verification project (linuxtesting.org).
>
> Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru>
Acked-by: Oliver Neukum <oliver@neukum.org>
^ permalink raw reply
* Re: [PATCH v4 3/4] can: kvaser_usb: Add support for the Usbcan-II family
From: Ahmed S. Darwish @ 2015-01-12 12:36 UTC (permalink / raw)
To: Marc Kleine-Budde
Cc: Olivier Sobrie, Oliver Hartkopp, Wolfgang Grandegger,
David S. Miller, Paul Gortmaker, Linux-CAN, netdev, LKML
In-Reply-To: <20150112120741.GC9213@linux>
On Mon, Jan 12, 2015 at 07:07:41AM -0500, Ahmed S. Darwish wrote:
> Hi Marc,
>
> On Mon, Jan 12, 2015 at 12:43:56PM +0100, Marc Kleine-Budde wrote:
> > On 01/11/2015 09:36 PM, Ahmed S. Darwish wrote:
> > > +
> > > + switch (dev->family) {
> > > + case KVASER_LEAF:
> > > + rx_msg = msg->u.leaf.rx_can.msg;
> > > + break;
> > > + case KVASER_USBCAN:
> > > + rx_msg = msg->u.usbcan.rx_can.msg;
> > > + break;
> > > + default:
> > > + dev_err(dev->udev->dev.parent,
> > > + "Invalid device family (%d)\n", dev->family);
> > > return;
> >
> > should not happen.
> >
>
> Yes, but otherwise I get GCC warnings of 'rx_msg' possibly
> being unused. I can add __maybe_unused to rx_msg of course,
> but such annotation may hide possible errors in the future.
>
Ah, what I meant is using uninitialized_var() to suppress the
GCC warning. But, really, using that macro has a bad history
of hiding errors in the future.
Kindly check http://lwn.net/Articles/529954/ for context.
Another solution might be initializing rx_msg to NULL.
> > > + switch (dev->family) {
> > > + case KVASER_LEAF:
> > > + msg_tx_can_flags = &msg->u.tx_can.leaf.flags;
> > > + break;
> > > + case KVASER_USBCAN:
> > > + msg_tx_can_flags = &msg->u.tx_can.usbcan.flags;
> > > + break;
> > > + default:
> > > + dev_err(dev->udev->dev.parent,
> > > + "Invalid device family (%d)\n", dev->family);
> > > + goto releasebuf;
> > should not happen, please remove
>
> Like the 'rx_msg' case above.
>
> Thanks,
> Darwish
^ permalink raw reply
* Re: [PATCH v3 4/4] can: kvaser_usb: Add support for the Usbcan-II family
From: Marc Kleine-Budde @ 2015-01-12 12:34 UTC (permalink / raw)
To: Ahmed S. Darwish
Cc: Olivier Sobrie, Oliver Hartkopp, Wolfgang Grandegger,
David S. Miller, Paul Gortmaker, Linux-CAN, netdev, LKML
In-Reply-To: <20150112122619.GD9213@linux>
[-- Attachment #1: Type: text/plain, Size: 2031 bytes --]
On 01/12/2015 01:26 PM, Ahmed S. Darwish wrote:
> On Mon, Jan 12, 2015 at 12:51:49PM +0100, Marc Kleine-Budde wrote:
>> On 01/08/2015 04:19 PM, Ahmed S. Darwish wrote:
>>
>> [...]
>>
>>>>> MODULE_DEVICE_TABLE(usb, kvaser_usb_table);
>>>>> @@ -463,7 +631,18 @@ static int kvaser_usb_get_software_info(struct kvaser_usb *dev)
>>>>> if (err)
>>>>> return err;
>>>>>
>>>>> - dev->fw_version = le32_to_cpu(msg.u.softinfo.fw_version);
>>>>> + switch (dev->family) {
>>>>> + case KVASER_LEAF:
>>>>> + dev->fw_version = le32_to_cpu(msg.u.leaf.softinfo.fw_version);
>>>>> + break;
>>>>> + case KVASER_USBCAN:
>>>>> + dev->fw_version = le32_to_cpu(msg.u.usbcan.softinfo.fw_version);
>>>>> + break;
>>>>> + default:
>>>>> + dev_err(dev->udev->dev.parent,
>>>>> + "Invalid device family (%d)\n", dev->family);
>>>>> + return -EINVAL;
>>>>
>>>> The default case should not happen. I think you can remove it.
>>
>>> It's true, it _should_ never happen. But I only add such checks if
>>> the follow-up code critically depends on a certain `dev->family`
>>> behavior. So it's kind of a defensive check against any possible
>>> bug in driver or memory.
>>>
>>> What do you think?
>>
>> The kernel is full of callback functions, if you have a bit flip there
>> you're in trouble anyways. A bug in the driver (or other parts of the
>> kernel) might overwrite the memory of dev->family, but if this happens,
>> more things will break.
>>
>
> I see. Thanks for the explanation.
>
> Most of them are now removed in latest submission, except two to
> avoid GCC warnings of variables that "may be used uninitialized".
Thanks, I'll look at the code later and try to figure out what gcc's
problem might be.
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
* [PATCH 6/6] openvswitch: Support VXLAN Group Policy extension
From: Thomas Graf @ 2015-01-12 12:26 UTC (permalink / raw)
To: davem, jesse, stephen, pshelar, therbert, alexei.starovoitov; +Cc: dev, netdev
In-Reply-To: <cover.1421064100.git.tgraf@suug.ch>
Introduces support for the group policy extension to the VXLAN virtual
port. The extension is disabled by default and only enabled if the user
has provided the respective configuration.
ovs-vsctl add-port br0 vxlan0 -- \
set Interface vxlan0 type=vxlan options:exts=gbp
The configuration interface to enable the extension is based on a new
attribute OVS_VXLAN_EXT_GBP nested inside OVS_TUNNEL_ATTR_EXTENSION
which can carry additional extensions as needed in the future.
The group policy metadata is stored as binary blob (struct ovs_vxlan_opts)
internally just like Geneve options but transported as nested Netlink
attributes to user space.
Renames the existing TUNNEL_OPTIONS_PRESENT to TUNNEL_GENEVE_OPT with the
binary value kept intact, a new flag TUNNEL_VXLAN_OPT is introduced.
The attributes OVS_TUNNEL_KEY_ATTR_VXLAN_OPTS and existing
OVS_TUNNEL_KEY_ATTR_GENEVE_OPTS are implemented mutually exclusive.
Signed-off-by: Thomas Graf <tgraf@suug.ch>
---
v2->v3:
- No change
v1->v2:
- Addressed Jesse's request to transport VXLAN options as Netlink
attributes instead of a binary blob. Allows a partial transport of
VXLAN extensions. Internally, the datapath continues to use a binary
blob (defined in vport-vxlan.h) for performance reasons.
- Added new TUNNEL_GENEVE_OPT and TUNNEL_VXLAN_OPT flags to mark
tunnel option flavour
- Correctly report VXLAN options to user space
include/net/ip_tunnels.h | 5 +-
include/uapi/linux/openvswitch.h | 11 ++++
net/openvswitch/flow_netlink.c | 114 ++++++++++++++++++++++++++++++++++-----
net/openvswitch/vport-geneve.c | 2 +-
net/openvswitch/vport-vxlan.c | 81 +++++++++++++++++++++++++++-
net/openvswitch/vport-vxlan.h | 11 ++++
6 files changed, 207 insertions(+), 17 deletions(-)
create mode 100644 net/openvswitch/vport-vxlan.h
diff --git a/include/net/ip_tunnels.h b/include/net/ip_tunnels.h
index 25a59eb..ce4db3c 100644
--- a/include/net/ip_tunnels.h
+++ b/include/net/ip_tunnels.h
@@ -97,7 +97,10 @@ struct ip_tunnel {
#define TUNNEL_DONT_FRAGMENT __cpu_to_be16(0x0100)
#define TUNNEL_OAM __cpu_to_be16(0x0200)
#define TUNNEL_CRIT_OPT __cpu_to_be16(0x0400)
-#define TUNNEL_OPTIONS_PRESENT __cpu_to_be16(0x0800)
+#define TUNNEL_GENEVE_OPT __cpu_to_be16(0x0800)
+#define TUNNEL_VXLAN_OPT __cpu_to_be16(0x1000)
+
+#define TUNNEL_OPTIONS_PRESENT (TUNNEL_GENEVE_OPT | TUNNEL_VXLAN_OPT)
struct tnl_ptk_info {
__be16 flags;
diff --git a/include/uapi/linux/openvswitch.h b/include/uapi/linux/openvswitch.h
index 3a6dcaa..e474c95 100644
--- a/include/uapi/linux/openvswitch.h
+++ b/include/uapi/linux/openvswitch.h
@@ -248,11 +248,21 @@ enum ovs_vport_attr {
#define OVS_VPORT_ATTR_MAX (__OVS_VPORT_ATTR_MAX - 1)
+enum {
+ OVS_VXLAN_EXT_UNSPEC,
+ OVS_VXLAN_EXT_GBP, /* Flag or __u32 */
+ __OVS_VXLAN_EXT_MAX,
+};
+
+#define OVS_VXLAN_EXT_MAX (__OVS_VXLAN_EXT_MAX - 1)
+
+
/* OVS_VPORT_ATTR_OPTIONS attributes for tunnels.
*/
enum {
OVS_TUNNEL_ATTR_UNSPEC,
OVS_TUNNEL_ATTR_DST_PORT, /* 16-bit UDP port, used by L4 tunnels. */
+ OVS_TUNNEL_ATTR_EXTENSION,
__OVS_TUNNEL_ATTR_MAX
};
@@ -324,6 +334,7 @@ enum ovs_tunnel_key_attr {
OVS_TUNNEL_KEY_ATTR_GENEVE_OPTS, /* Array of Geneve options. */
OVS_TUNNEL_KEY_ATTR_TP_SRC, /* be16 src Transport Port. */
OVS_TUNNEL_KEY_ATTR_TP_DST, /* be16 dst Transport Port. */
+ OVS_TUNNEL_KEY_ATTR_VXLAN_OPTS, /* Nested OVS_VXLAN_EXT_* */
__OVS_TUNNEL_KEY_ATTR_MAX
};
diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c
index 457ccf3..cea492b 100644
--- a/net/openvswitch/flow_netlink.c
+++ b/net/openvswitch/flow_netlink.c
@@ -49,6 +49,7 @@
#include <net/mpls.h>
#include "flow_netlink.h"
+#include "vport-vxlan.h"
struct ovs_len_tbl {
int len;
@@ -268,6 +269,9 @@ size_t ovs_tun_key_attr_size(void)
+ nla_total_size(0) /* OVS_TUNNEL_KEY_ATTR_CSUM */
+ nla_total_size(0) /* OVS_TUNNEL_KEY_ATTR_OAM */
+ nla_total_size(256) /* OVS_TUNNEL_KEY_ATTR_GENEVE_OPTS */
+ /* OVS_TUNNEL_KEY_ATTR_VXLAN_OPTS is mutually exclusive with
+ * OVS_TUNNEL_KEY_ATTR_GENEVE_OPTS and covered by it.
+ */
+ nla_total_size(2) /* OVS_TUNNEL_KEY_ATTR_TP_SRC */
+ nla_total_size(2); /* OVS_TUNNEL_KEY_ATTR_TP_DST */
}
@@ -308,6 +312,7 @@ static const struct ovs_len_tbl ovs_tunnel_key_lens[OVS_TUNNEL_KEY_ATTR_MAX + 1]
[OVS_TUNNEL_KEY_ATTR_TP_DST] = { .len = sizeof(u16) },
[OVS_TUNNEL_KEY_ATTR_OAM] = { .len = 0 },
[OVS_TUNNEL_KEY_ATTR_GENEVE_OPTS] = { .len = OVS_ATTR_NESTED },
+ [OVS_TUNNEL_KEY_ATTR_VXLAN_OPTS] = { .len = OVS_ATTR_NESTED },
};
/* The size of the argument for each %OVS_KEY_ATTR_* Netlink attribute. */
@@ -460,6 +465,41 @@ static int genev_tun_opt_from_nlattr(const struct nlattr *a,
return 0;
}
+static const struct nla_policy vxlan_opt_policy[OVS_VXLAN_EXT_MAX + 1] = {
+ [OVS_VXLAN_EXT_GBP] = { .type = NLA_U32 },
+};
+
+static int vxlan_tun_opt_from_nlattr(const struct nlattr *a,
+ struct sw_flow_match *match, bool is_mask,
+ bool log)
+{
+ struct nlattr *tb[OVS_VXLAN_EXT_MAX+1];
+ unsigned long opt_key_offset;
+ struct ovs_vxlan_opts opts;
+ int err;
+
+ BUILD_BUG_ON(sizeof(opts) > sizeof(match->key->tun_opts));
+
+ err = nla_parse_nested(tb, OVS_VXLAN_EXT_MAX, a, vxlan_opt_policy);
+ if (err < 0)
+ return err;
+
+ memset(&opts, 0, sizeof(opts));
+
+ if (tb[OVS_VXLAN_EXT_MAX])
+ opts.gbp = nla_get_u32(tb[OVS_VXLAN_EXT_MAX]);
+
+ if (!is_mask)
+ SW_FLOW_KEY_PUT(match, tun_opts_len, sizeof(opts), false);
+ else
+ SW_FLOW_KEY_PUT(match, tun_opts_len, 0xff, true);
+
+ opt_key_offset = TUN_METADATA_OFFSET(sizeof(opts));
+ SW_FLOW_KEY_MEMCPY_OFFSET(match, opt_key_offset, &opts, sizeof(opts),
+ is_mask);
+ return 0;
+}
+
static int ipv4_tun_from_nlattr(const struct nlattr *attr,
struct sw_flow_match *match, bool is_mask,
bool log)
@@ -468,6 +508,7 @@ static int ipv4_tun_from_nlattr(const struct nlattr *attr,
int rem;
bool ttl = false;
__be16 tun_flags = 0;
+ int opts_type = 0;
nla_for_each_nested(a, attr, rem) {
int type = nla_type(a);
@@ -527,11 +568,30 @@ static int ipv4_tun_from_nlattr(const struct nlattr *attr,
tun_flags |= TUNNEL_OAM;
break;
case OVS_TUNNEL_KEY_ATTR_GENEVE_OPTS:
+ if (opts_type) {
+ OVS_NLERR(log, "Multiple metadata blocks provided");
+ return -EINVAL;
+ }
+
err = genev_tun_opt_from_nlattr(a, match, is_mask, log);
if (err)
return err;
- tun_flags |= TUNNEL_OPTIONS_PRESENT;
+ tun_flags |= TUNNEL_GENEVE_OPT;
+ opts_type = type;
+ break;
+ case OVS_TUNNEL_KEY_ATTR_VXLAN_OPTS:
+ if (opts_type) {
+ OVS_NLERR(log, "Multiple metadata blocks provided");
+ return -EINVAL;
+ }
+
+ err = vxlan_tun_opt_from_nlattr(a, match, is_mask, log);
+ if (err)
+ return err;
+
+ tun_flags |= TUNNEL_VXLAN_OPT;
+ opts_type = type;
break;
default:
OVS_NLERR(log, "Unknown IPv4 tunnel attribute %d",
@@ -560,6 +620,23 @@ static int ipv4_tun_from_nlattr(const struct nlattr *attr,
}
}
+ return opts_type;
+}
+
+static int vxlan_opt_to_nlattr(struct sk_buff *skb,
+ const void *tun_opts, int swkey_tun_opts_len)
+{
+ const struct ovs_vxlan_opts *opts = tun_opts;
+ struct nlattr *nla;
+
+ nla = nla_nest_start(skb, OVS_TUNNEL_KEY_ATTR_VXLAN_OPTS);
+ if (!nla)
+ return -EMSGSIZE;
+
+ if (nla_put_u32(skb, OVS_VXLAN_EXT_GBP, opts->gbp) < 0)
+ return -EMSGSIZE;
+
+ nla_nest_end(skb, nla);
return 0;
}
@@ -596,10 +673,15 @@ static int __ipv4_tun_to_nlattr(struct sk_buff *skb,
if ((output->tun_flags & TUNNEL_OAM) &&
nla_put_flag(skb, OVS_TUNNEL_KEY_ATTR_OAM))
return -EMSGSIZE;
- if (tun_opts &&
- nla_put(skb, OVS_TUNNEL_KEY_ATTR_GENEVE_OPTS,
- swkey_tun_opts_len, tun_opts))
- return -EMSGSIZE;
+ if (tun_opts) {
+ if (output->tun_flags & TUNNEL_GENEVE_OPT &&
+ nla_put(skb, OVS_TUNNEL_KEY_ATTR_GENEVE_OPTS,
+ swkey_tun_opts_len, tun_opts))
+ return -EMSGSIZE;
+ else if (output->tun_flags & TUNNEL_VXLAN_OPT &&
+ vxlan_opt_to_nlattr(skb, tun_opts, swkey_tun_opts_len))
+ return -EMSGSIZE;
+ }
return 0;
}
@@ -680,7 +762,7 @@ static int metadata_from_nlattrs(struct sw_flow_match *match, u64 *attrs,
}
if (*attrs & (1 << OVS_KEY_ATTR_TUNNEL)) {
if (ipv4_tun_from_nlattr(a[OVS_KEY_ATTR_TUNNEL], match,
- is_mask, log))
+ is_mask, log) < 0)
return -EINVAL;
*attrs &= ~(1 << OVS_KEY_ATTR_TUNNEL);
}
@@ -1578,17 +1660,23 @@ static int validate_and_copy_set_tun(const struct nlattr *attr,
struct sw_flow_key key;
struct ovs_tunnel_info *tun_info;
struct nlattr *a;
- int err, start;
+ int err, start, opts_type;
ovs_match_init(&match, &key, NULL);
- err = ipv4_tun_from_nlattr(nla_data(attr), &match, false, log);
- if (err)
- return err;
+ opts_type = ipv4_tun_from_nlattr(nla_data(attr), &match, false, log);
+ if (opts_type < 0)
+ return opts_type;
if (key.tun_opts_len) {
- err = validate_and_copy_geneve_opts(&key);
- if (err < 0)
- return err;
+ switch (opts_type) {
+ case OVS_TUNNEL_KEY_ATTR_GENEVE_OPTS:
+ err = validate_and_copy_geneve_opts(&key);
+ if (err < 0)
+ return err;
+ break;
+ case OVS_TUNNEL_KEY_ATTR_VXLAN_OPTS:
+ break;
+ }
};
start = add_nested_action_start(sfa, OVS_ACTION_ATTR_SET, log);
diff --git a/net/openvswitch/vport-geneve.c b/net/openvswitch/vport-geneve.c
index 484864d..902ee4f 100644
--- a/net/openvswitch/vport-geneve.c
+++ b/net/openvswitch/vport-geneve.c
@@ -90,7 +90,7 @@ static void geneve_rcv(struct geneve_sock *gs, struct sk_buff *skb)
opts_len = geneveh->opt_len * 4;
- flags = TUNNEL_KEY | TUNNEL_OPTIONS_PRESENT |
+ flags = TUNNEL_KEY | TUNNEL_GENEVE_OPT |
(udp_hdr(skb)->check != 0 ? TUNNEL_CSUM : 0) |
(geneveh->oam ? TUNNEL_OAM : 0) |
(geneveh->critical ? TUNNEL_CRIT_OPT : 0);
diff --git a/net/openvswitch/vport-vxlan.c b/net/openvswitch/vport-vxlan.c
index 266c595..dbd6c75 100644
--- a/net/openvswitch/vport-vxlan.c
+++ b/net/openvswitch/vport-vxlan.c
@@ -40,6 +40,7 @@
#include "datapath.h"
#include "vport.h"
+#include "vport-vxlan.h"
/**
* struct vxlan_port - Keeps track of open UDP ports
@@ -49,6 +50,7 @@
struct vxlan_port {
struct vxlan_sock *vs;
char name[IFNAMSIZ];
+ u32 exts; /* VXLAN_EXT_* in <net/vxlan.h> */
};
static struct vport_ops ovs_vxlan_vport_ops;
@@ -63,16 +65,26 @@ static void vxlan_rcv(struct vxlan_sock *vs, struct sk_buff *skb,
struct vxlan_metadata *md)
{
struct ovs_tunnel_info tun_info;
+ struct vxlan_port *vxlan_port;
struct vport *vport = vs->data;
struct iphdr *iph;
+ struct ovs_vxlan_opts opts = {
+ .gbp = md->gbp,
+ };
__be64 key;
+ __be16 flags;
+
+ flags = TUNNEL_KEY;
+ vxlan_port = vxlan_vport(vport);
+ if (vxlan_port->exts & VXLAN_EXT_GBP)
+ flags |= TUNNEL_VXLAN_OPT;
/* Save outer tunnel values */
iph = ip_hdr(skb);
key = cpu_to_be64(ntohl(md->vni) >> 8);
ovs_flow_tun_info_init(&tun_info, iph,
udp_hdr(skb)->source, udp_hdr(skb)->dest,
- key, TUNNEL_KEY, NULL, 0);
+ key, flags, &opts, sizeof(opts));
ovs_vport_receive(vport, skb, &tun_info);
}
@@ -84,6 +96,21 @@ static int vxlan_get_options(const struct vport *vport, struct sk_buff *skb)
if (nla_put_u16(skb, OVS_TUNNEL_ATTR_DST_PORT, ntohs(dst_port)))
return -EMSGSIZE;
+
+ if (vxlan_port->exts) {
+ struct nlattr *exts;
+
+ exts = nla_nest_start(skb, OVS_TUNNEL_ATTR_EXTENSION);
+ if (!exts)
+ return -EMSGSIZE;
+
+ if (vxlan_port->exts & VXLAN_EXT_GBP &&
+ nla_put_flag(skb, OVS_VXLAN_EXT_GBP))
+ return -EMSGSIZE;
+
+ nla_nest_end(skb, exts);
+ }
+
return 0;
}
@@ -96,6 +123,31 @@ static void vxlan_tnl_destroy(struct vport *vport)
ovs_vport_deferred_free(vport);
}
+static const struct nla_policy exts_policy[OVS_VXLAN_EXT_MAX+1] = {
+ [OVS_VXLAN_EXT_GBP] = { .type = NLA_FLAG, },
+};
+
+static int vxlan_configure_exts(struct vport *vport, struct nlattr *attr)
+{
+ struct nlattr *exts[OVS_VXLAN_EXT_MAX+1];
+ struct vxlan_port *vxlan_port;
+ int err;
+
+ if (nla_len(attr) < sizeof(struct nlattr))
+ return -EINVAL;
+
+ err = nla_parse_nested(exts, OVS_VXLAN_EXT_MAX, attr, exts_policy);
+ if (err < 0)
+ return err;
+
+ vxlan_port = vxlan_vport(vport);
+
+ if (exts[OVS_VXLAN_EXT_GBP])
+ vxlan_port->exts |= VXLAN_EXT_GBP;
+
+ return 0;
+}
+
static struct vport *vxlan_tnl_create(const struct vport_parms *parms)
{
struct net *net = ovs_dp_get_net(parms->dp);
@@ -128,7 +180,17 @@ static struct vport *vxlan_tnl_create(const struct vport_parms *parms)
vxlan_port = vxlan_vport(vport);
strncpy(vxlan_port->name, parms->name, IFNAMSIZ);
- vs = vxlan_sock_add(net, htons(dst_port), vxlan_rcv, vport, true, 0, 0);
+ a = nla_find_nested(options, OVS_TUNNEL_ATTR_EXTENSION);
+ if (a) {
+ err = vxlan_configure_exts(vport, a);
+ if (err) {
+ ovs_vport_free(vport);
+ goto error;
+ }
+ }
+
+ vs = vxlan_sock_add(net, htons(dst_port), vxlan_rcv, vport, true, 0,
+ vxlan_port->exts);
if (IS_ERR(vs)) {
ovs_vport_free(vport);
return (void *)vs;
@@ -141,6 +203,20 @@ error:
return ERR_PTR(err);
}
+static int vxlan_ext_gbp(struct sk_buff *skb)
+{
+ const struct ovs_tunnel_info *tun_info;
+ const struct ovs_vxlan_opts *opts;
+
+ tun_info = OVS_CB(skb)->egress_tun_info;
+ opts = tun_info->options;
+
+ if (tun_info->options_len >= sizeof(*opts))
+ return opts->gbp;
+ else
+ return 0;
+}
+
static int vxlan_tnl_send(struct vport *vport, struct sk_buff *skb)
{
struct net *net = ovs_dp_get_net(vport->dp);
@@ -181,6 +257,7 @@ static int vxlan_tnl_send(struct vport *vport, struct sk_buff *skb)
src_port = udp_flow_src_port(net, skb, 0, 0, true);
md.vni = htonl(be64_to_cpu(tun_key->tun_id) << 8);
+ md.gbp = vxlan_ext_gbp(skb);
err = vxlan_xmit_skb(vxlan_port->vs, rt, skb,
fl.saddr, tun_key->ipv4_dst,
diff --git a/net/openvswitch/vport-vxlan.h b/net/openvswitch/vport-vxlan.h
new file mode 100644
index 0000000..4b08233e
--- /dev/null
+++ b/net/openvswitch/vport-vxlan.h
@@ -0,0 +1,11 @@
+#ifndef VPORT_VXLAN_H
+#define VPORT_VXLAN_H 1
+
+#include <linux/kernel.h>
+#include <linux/types.h>
+
+struct ovs_vxlan_opts {
+ __u32 gbp;
+};
+
+#endif
--
1.9.3
^ permalink raw reply related
* [PATCH 4/6] openvswitch: Rename GENEVE_TUN_OPTS() to TUN_METADATA_OPTS()
From: Thomas Graf @ 2015-01-12 12:26 UTC (permalink / raw)
To: davem, jesse, stephen, pshelar, therbert, alexei.starovoitov; +Cc: dev, netdev
In-Reply-To: <cover.1421064100.git.tgraf@suug.ch>
Also factors out Geneve validation code into a new separate function
validate_and_copy_geneve_opts().
A subsequent patch will introduce VXLAN options. Rename the existing
GENEVE_TUN_OPTS() to reflect its extended purpose of carrying generic
tunnel metadata options.
Signed-off-by: Thomas Graf <tgraf@suug.ch>
---
v2->v3:
- No change
v1->v2:
- Don't rename genev_tun_opt_from_nlattr() and keep it Geneve specific,
pointed out by Jesse.
- Factor out Geneve specific validation code into separate function as
requested by Jesse.
net/openvswitch/flow.c | 2 +-
net/openvswitch/flow.h | 14 ++++----
net/openvswitch/flow_netlink.c | 72 +++++++++++++++++++++++-------------------
3 files changed, 47 insertions(+), 41 deletions(-)
diff --git a/net/openvswitch/flow.c b/net/openvswitch/flow.c
index da2fae0..41f2dfd 100644
--- a/net/openvswitch/flow.c
+++ b/net/openvswitch/flow.c
@@ -691,7 +691,7 @@ int ovs_flow_key_extract(const struct ovs_tunnel_info *tun_info,
BUILD_BUG_ON((1 << (sizeof(tun_info->options_len) *
8)) - 1
> sizeof(key->tun_opts));
- memcpy(GENEVE_OPTS(key, tun_info->options_len),
+ memcpy(TUN_METADATA_OPTS(key, tun_info->options_len),
tun_info->options, tun_info->options_len);
key->tun_opts_len = tun_info->options_len;
} else {
diff --git a/net/openvswitch/flow.h b/net/openvswitch/flow.h
index a8b30f3..d3d0a40 100644
--- a/net/openvswitch/flow.h
+++ b/net/openvswitch/flow.h
@@ -53,7 +53,7 @@ struct ovs_key_ipv4_tunnel {
struct ovs_tunnel_info {
struct ovs_key_ipv4_tunnel tunnel;
- const struct geneve_opt *options;
+ const void *options;
u8 options_len;
};
@@ -61,10 +61,10 @@ struct ovs_tunnel_info {
* maximum size. This allows us to get the benefits of variable length
* matching for small options.
*/
-#define GENEVE_OPTS(flow_key, opt_len) \
- ((struct geneve_opt *)((flow_key)->tun_opts + \
- FIELD_SIZEOF(struct sw_flow_key, tun_opts) - \
- opt_len))
+#define TUN_METADATA_OFFSET(opt_len) \
+ (FIELD_SIZEOF(struct sw_flow_key, tun_opts) - opt_len)
+#define TUN_METADATA_OPTS(flow_key, opt_len) \
+ ((void *)((flow_key)->tun_opts + TUN_METADATA_OFFSET(opt_len)))
static inline void __ovs_flow_tun_info_init(struct ovs_tunnel_info *tun_info,
__be32 saddr, __be32 daddr,
@@ -73,7 +73,7 @@ static inline void __ovs_flow_tun_info_init(struct ovs_tunnel_info *tun_info,
__be16 tp_dst,
__be64 tun_id,
__be16 tun_flags,
- const struct geneve_opt *opts,
+ const void *opts,
u8 opts_len)
{
tun_info->tunnel.tun_id = tun_id;
@@ -105,7 +105,7 @@ static inline void ovs_flow_tun_info_init(struct ovs_tunnel_info *tun_info,
__be16 tp_dst,
__be64 tun_id,
__be16 tun_flags,
- const struct geneve_opt *opts,
+ const void *opts,
u8 opts_len)
{
__ovs_flow_tun_info_init(tun_info, iph->saddr, iph->daddr,
diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c
index d1eecf7..8980d32 100644
--- a/net/openvswitch/flow_netlink.c
+++ b/net/openvswitch/flow_netlink.c
@@ -432,8 +432,7 @@ static int genev_tun_opt_from_nlattr(const struct nlattr *a,
SW_FLOW_KEY_PUT(match, tun_opts_len, 0xff, true);
}
- opt_key_offset = (unsigned long)GENEVE_OPTS((struct sw_flow_key *)0,
- nla_len(a));
+ opt_key_offset = TUN_METADATA_OFFSET(nla_len(a));
SW_FLOW_KEY_MEMCPY_OFFSET(match, opt_key_offset, nla_data(a),
nla_len(a), is_mask);
return 0;
@@ -558,8 +557,7 @@ static int ipv4_tun_from_nlattr(const struct nlattr *attr,
static int __ipv4_tun_to_nlattr(struct sk_buff *skb,
const struct ovs_key_ipv4_tunnel *output,
- const struct geneve_opt *tun_opts,
- int swkey_tun_opts_len)
+ const void *tun_opts, int swkey_tun_opts_len)
{
if (output->tun_flags & TUNNEL_KEY &&
nla_put_be64(skb, OVS_TUNNEL_KEY_ATTR_ID, output->tun_id))
@@ -600,8 +598,7 @@ static int __ipv4_tun_to_nlattr(struct sk_buff *skb,
static int ipv4_tun_to_nlattr(struct sk_buff *skb,
const struct ovs_key_ipv4_tunnel *output,
- const struct geneve_opt *tun_opts,
- int swkey_tun_opts_len)
+ const void *tun_opts, int swkey_tun_opts_len)
{
struct nlattr *nla;
int err;
@@ -1148,10 +1145,10 @@ int ovs_nla_put_flow(const struct sw_flow_key *swkey,
goto nla_put_failure;
if ((swkey->tun_key.ipv4_dst || is_mask)) {
- const struct geneve_opt *opts = NULL;
+ const void *opts = NULL;
if (output->tun_key.tun_flags & TUNNEL_OPTIONS_PRESENT)
- opts = GENEVE_OPTS(output, swkey->tun_opts_len);
+ opts = TUN_METADATA_OPTS(output, swkey->tun_opts_len);
if (ipv4_tun_to_nlattr(skb, &output->tun_key, opts,
swkey->tun_opts_len))
@@ -1540,6 +1537,34 @@ void ovs_match_init(struct sw_flow_match *match,
}
}
+static int validate_and_copy_geneve_opts(struct sw_flow_key *key)
+{
+ struct geneve_opt *option;
+ int opts_len = key->tun_opts_len;
+ bool crit_opt = false;
+
+ option = (struct geneve_opt *)TUN_METADATA_OPTS(key, key->tun_opts_len);
+ while (opts_len > 0) {
+ int len;
+
+ if (opts_len < sizeof(*option))
+ return -EINVAL;
+
+ len = sizeof(*option) + option->length * 4;
+ if (len > opts_len)
+ return -EINVAL;
+
+ crit_opt |= !!(option->type & GENEVE_CRIT_OPT_TYPE);
+
+ option = (struct geneve_opt *)((u8 *)option + len);
+ opts_len -= len;
+ };
+
+ key->tun_key.tun_flags |= crit_opt ? TUNNEL_CRIT_OPT : 0;
+
+ return 0;
+}
+
static int validate_and_copy_set_tun(const struct nlattr *attr,
struct sw_flow_actions **sfa, bool log)
{
@@ -1555,28 +1580,9 @@ static int validate_and_copy_set_tun(const struct nlattr *attr,
return err;
if (key.tun_opts_len) {
- struct geneve_opt *option = GENEVE_OPTS(&key,
- key.tun_opts_len);
- int opts_len = key.tun_opts_len;
- bool crit_opt = false;
-
- while (opts_len > 0) {
- int len;
-
- if (opts_len < sizeof(*option))
- return -EINVAL;
-
- len = sizeof(*option) + option->length * 4;
- if (len > opts_len)
- return -EINVAL;
-
- crit_opt |= !!(option->type & GENEVE_CRIT_OPT_TYPE);
-
- option = (struct geneve_opt *)((u8 *)option + len);
- opts_len -= len;
- };
-
- key.tun_key.tun_flags |= crit_opt ? TUNNEL_CRIT_OPT : 0;
+ err = validate_and_copy_geneve_opts(&key);
+ if (err < 0)
+ return err;
};
start = add_nested_action_start(sfa, OVS_ACTION_ATTR_SET, log);
@@ -1597,9 +1603,9 @@ static int validate_and_copy_set_tun(const struct nlattr *attr,
* everything else will go away after flow setup. We can append
* it to tun_info and then point there.
*/
- memcpy((tun_info + 1), GENEVE_OPTS(&key, key.tun_opts_len),
- key.tun_opts_len);
- tun_info->options = (struct geneve_opt *)(tun_info + 1);
+ memcpy((tun_info + 1),
+ TUN_METADATA_OPTS(&key, key.tun_opts_len), key.tun_opts_len);
+ tun_info->options = (tun_info + 1);
} else {
tun_info->options = NULL;
}
--
1.9.3
^ permalink raw reply related
* [PATCH 2/6] vxlan: Group Policy extension
From: Thomas Graf @ 2015-01-12 12:26 UTC (permalink / raw)
To: davem, jesse, stephen, pshelar, therbert, alexei.starovoitov; +Cc: dev, netdev
In-Reply-To: <cover.1421064100.git.tgraf@suug.ch>
Implements supports for the Group Policy VXLAN extension [0] to provide
a lightweight and simple security label mechanism across network peers
based on VXLAN. The security context and associated metadata is mapped
to/from skb->mark. This allows further mapping to a SELinux context
using SECMARK, to implement ACLs directly with nftables, iptables, OVS,
tc, etc.
The group membership is defined by the lower 16 bits of skb->mark, the
upper 16 bits are used for flags.
SELinux allows to manage label to secure local resources. However,
distributed applications require ACLs to implemented across hosts. This
is typically achieved by matching on L2-L4 fields to identify the
original sending host and process on the receiver. On top of that,
netlabel and specifically CIPSO [1] allow to map security contexts to
universal labels. However, netlabel and CIPSO are relatively complex.
This patch provides a lightweight alternative for overlay network
environments with a trusted underlay. No additional control protocol
is required.
Host 1: Host 2:
Group A Group B Group B Group A
+-----+ +-------------+ +-------+ +-----+
| lxc | | SELinux CTX | | httpd | | VM |
+--+--+ +--+----------+ +---+---+ +--+--+
\---+---/ \----+---/
| |
+---+---+ +---+---+
| vxlan | | vxlan |
+---+---+ +---+---+
+------------------------------+
Backwards compatibility:
A VXLAN-GBP socket can receive standard VXLAN frames and will assign
the default group 0x0000 to such frames. A Linux VXLAN socket will
drop VXLAN-GBP frames. The extension is therefore disabled by default
and needs to be specifically enabled:
ip link add [...] type vxlan [...] gbp
In a mixed environment with VXLAN and VXLAN-GBP sockets, the GBP socket
must run on a separate port number.
Examples:
iptables:
host1# iptables -I OUTPUT -m owner --uid-owner 101 -j MARK --set-mark 0x200
host2# iptables -I INPUT -m mark --mark 0x200 -j DROP
OVS:
# ovs-ofctl add-flow br0 'in_port=1,actions=load:0x200->NXM_NX_TUN_GBP_ID[],NORMAL'
# ovs-ofctl add-flow br0 'in_port=2,tun_gbp_id=0x200,actions=drop'
[0] https://tools.ietf.org/html/draft-smith-vxlan-group-policy
[1] http://lwn.net/Articles/204905/
Signed-off-by: Thomas Graf <tgraf@suug.ch>
---
v2->v3:
- Removed empty struct vxlan_gbp as spotted by Alexei
v1->v2:
- split GBP header definition into separate struct vxlanhdr_gbp as requested
by Alexei
drivers/net/vxlan.c | 161 ++++++++++++++++++++++++++++++------------
include/net/vxlan.h | 70 ++++++++++++++++--
include/uapi/linux/if_link.h | 8 +++
net/openvswitch/vport-vxlan.c | 9 ++-
4 files changed, 195 insertions(+), 53 deletions(-)
diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
index 4d52aa9..b148739 100644
--- a/drivers/net/vxlan.c
+++ b/drivers/net/vxlan.c
@@ -132,6 +132,7 @@ struct vxlan_dev {
__u8 tos; /* TOS override */
__u8 ttl;
u32 flags; /* VXLAN_F_* in vxlan.h */
+ u32 exts; /* Enabled extensions */
struct work_struct sock_work;
struct work_struct igmp_join;
@@ -568,7 +569,8 @@ static struct sk_buff **vxlan_gro_receive(struct sk_buff **head, struct sk_buff
continue;
vh2 = (struct vxlanhdr *)(p->data + off_vx);
- if (vh->vx_vni != vh2->vx_vni) {
+ if (vh->vx_flags != vh2->vx_flags ||
+ vh->vx_vni != vh2->vx_vni) {
NAPI_GRO_CB(p)->same_flow = 0;
continue;
}
@@ -1095,6 +1097,7 @@ static int vxlan_udp_encap_recv(struct sock *sk, struct sk_buff *skb)
{
struct vxlan_sock *vs;
struct vxlanhdr *vxh;
+ struct vxlan_metadata md = {0};
/* Need Vxlan and inner Ethernet header to be present */
if (!pskb_may_pull(skb, VXLAN_HLEN))
@@ -1113,6 +1116,22 @@ static int vxlan_udp_encap_recv(struct sock *sk, struct sk_buff *skb)
if (vs->exts) {
if (!vxh->vni_present)
goto error_invalid_header;
+
+ if (vxh->gbp_present) {
+ struct vxlanhdr_gbp *gbp;
+
+ if (!(vs->exts & VXLAN_EXT_GBP))
+ goto error_invalid_header;
+
+ gbp = (struct vxlanhdr_gbp *)vxh;
+ md.gbp = ntohs(gbp->policy_id);
+
+ if (gbp->dont_learn)
+ md.gbp |= VXLAN_GBP_DONT_LEARN;
+
+ if (gbp->policy_applied)
+ md.gbp |= VXLAN_GBP_POLICY_APPLIED;
+ }
} else {
if (vxh->vx_flags != htonl(VXLAN_FLAGS) ||
(vxh->vx_vni & htonl(0xff)))
@@ -1122,7 +1141,8 @@ static int vxlan_udp_encap_recv(struct sock *sk, struct sk_buff *skb)
if (iptunnel_pull_header(skb, VXLAN_HLEN, htons(ETH_P_TEB)))
goto drop;
- vs->rcv(vs, skb, vxh->vx_vni);
+ md.vni = vxh->vx_vni;
+ vs->rcv(vs, skb, &md);
return 0;
drop:
@@ -1138,8 +1158,8 @@ error:
return 1;
}
-static void vxlan_rcv(struct vxlan_sock *vs,
- struct sk_buff *skb, __be32 vx_vni)
+static void vxlan_rcv(struct vxlan_sock *vs, struct sk_buff *skb,
+ struct vxlan_metadata *md)
{
struct iphdr *oip = NULL;
struct ipv6hdr *oip6 = NULL;
@@ -1150,7 +1170,7 @@ static void vxlan_rcv(struct vxlan_sock *vs,
int err = 0;
union vxlan_addr *remote_ip;
- vni = ntohl(vx_vni) >> 8;
+ vni = ntohl(md->vni) >> 8;
/* Is this VNI defined? */
vxlan = vxlan_vs_find_vni(vs, vni);
if (!vxlan)
@@ -1184,6 +1204,7 @@ static void vxlan_rcv(struct vxlan_sock *vs,
goto drop;
skb_reset_network_header(skb);
+ skb->mark = md->gbp;
if (oip6)
err = IP6_ECN_decapsulate(oip6, skb);
@@ -1533,15 +1554,57 @@ static bool route_shortcircuit(struct net_device *dev, struct sk_buff *skb)
return false;
}
+static int vxlan_build_hdr(struct sk_buff *skb, struct vxlan_sock *vs,
+ int min_headroom, struct vxlan_metadata *md)
+{
+ struct vxlanhdr *vxh;
+ int err;
+
+ /* Need space for new headers (invalidates iph ptr) */
+ err = skb_cow_head(skb, min_headroom);
+ if (unlikely(err)) {
+ kfree_skb(skb);
+ return err;
+ }
+
+ skb = vlan_hwaccel_push_inside(skb);
+ if (WARN_ON(!skb))
+ return -ENOMEM;
+
+ vxh = (struct vxlanhdr *)__skb_push(skb, sizeof(*vxh));
+ vxh->vx_flags = htonl(VXLAN_FLAGS);
+ vxh->vx_vni = md->vni;
+
+ if (vs->exts) {
+ if (vs->exts & VXLAN_EXT_GBP) {
+ struct vxlanhdr_gbp *gbp;
+
+ gbp = (struct vxlanhdr_gbp *)vxh;
+ vxh->gbp_present = 1;
+
+ if (md->gbp & VXLAN_GBP_DONT_LEARN)
+ gbp->dont_learn = 1;
+
+ if (md->gbp & VXLAN_GBP_POLICY_APPLIED)
+ gbp->policy_applied = 1;
+
+ gbp->policy_id = htons(md->gbp & VXLAN_GBP_ID_MASK);
+ }
+ }
+
+ skb_set_inner_protocol(skb, htons(ETH_P_TEB));
+
+ return 0;
+}
+
#if IS_ENABLED(CONFIG_IPV6)
static int vxlan6_xmit_skb(struct vxlan_sock *vs,
struct dst_entry *dst, struct sk_buff *skb,
struct net_device *dev, struct in6_addr *saddr,
struct in6_addr *daddr, __u8 prio, __u8 ttl,
- __be16 src_port, __be16 dst_port, __be32 vni,
- bool xnet)
+ __be16 src_port, __be16 dst_port,
+ struct vxlan_metadata *md, bool xnet)
{
- struct vxlanhdr *vxh;
int min_headroom;
int err;
bool udp_sum = !udp_get_no_check6_tx(vs->sock->sk);
@@ -1558,24 +1621,9 @@ static int vxlan6_xmit_skb(struct vxlan_sock *vs,
+ VXLAN_HLEN + sizeof(struct ipv6hdr)
+ (vlan_tx_tag_present(skb) ? VLAN_HLEN : 0);
- /* Need space for new headers (invalidates iph ptr) */
- err = skb_cow_head(skb, min_headroom);
- if (unlikely(err)) {
- kfree_skb(skb);
- goto err;
- }
-
- skb = vlan_hwaccel_push_inside(skb);
- if (WARN_ON(!skb)) {
- err = -ENOMEM;
+ err = vxlan_build_hdr(skb, vs, min_headroom, md);
+ if (err)
goto err;
- }
-
- vxh = (struct vxlanhdr *) __skb_push(skb, sizeof(*vxh));
- vxh->vx_flags = htonl(VXLAN_FLAGS);
- vxh->vx_vni = vni;
-
- skb_set_inner_protocol(skb, htons(ETH_P_TEB));
udp_tunnel6_xmit_skb(vs->sock, dst, skb, dev, saddr, daddr, prio,
ttl, src_port, dst_port);
@@ -1589,9 +1637,9 @@ err:
int vxlan_xmit_skb(struct vxlan_sock *vs,
struct rtable *rt, struct sk_buff *skb,
__be32 src, __be32 dst, __u8 tos, __u8 ttl, __be16 df,
- __be16 src_port, __be16 dst_port, __be32 vni, bool xnet)
+ __be16 src_port, __be16 dst_port,
+ struct vxlan_metadata *md, bool xnet)
{
- struct vxlanhdr *vxh;
int min_headroom;
int err;
bool udp_sum = !vs->sock->sk->sk_no_check_tx;
@@ -1604,22 +1652,9 @@ int vxlan_xmit_skb(struct vxlan_sock *vs,
+ VXLAN_HLEN + sizeof(struct iphdr)
+ (vlan_tx_tag_present(skb) ? VLAN_HLEN : 0);
- /* Need space for new headers (invalidates iph ptr) */
- err = skb_cow_head(skb, min_headroom);
- if (unlikely(err)) {
- kfree_skb(skb);
+ err = vxlan_build_hdr(skb, vs, min_headroom, md);
+ if (err)
return err;
- }
-
- skb = vlan_hwaccel_push_inside(skb);
- if (WARN_ON(!skb))
- return -ENOMEM;
-
- vxh = (struct vxlanhdr *) __skb_push(skb, sizeof(*vxh));
- vxh->vx_flags = htonl(VXLAN_FLAGS);
- vxh->vx_vni = vni;
-
- skb_set_inner_protocol(skb, htons(ETH_P_TEB));
return udp_tunnel_xmit_skb(vs->sock, rt, skb, src, dst, tos,
ttl, df, src_port, dst_port, xnet);
@@ -1679,6 +1714,7 @@ static void vxlan_xmit_one(struct sk_buff *skb, struct net_device *dev,
const struct iphdr *old_iph;
struct flowi4 fl4;
union vxlan_addr *dst;
+ struct vxlan_metadata md;
__be16 src_port = 0, dst_port;
u32 vni;
__be16 df = 0;
@@ -1749,11 +1785,12 @@ static void vxlan_xmit_one(struct sk_buff *skb, struct net_device *dev,
tos = ip_tunnel_ecn_encap(tos, old_iph, skb);
ttl = ttl ? : ip4_dst_hoplimit(&rt->dst);
+ md.vni = htonl(vni << 8);
+ md.gbp = skb->mark;
err = vxlan_xmit_skb(vxlan->vn_sock, rt, skb,
fl4.saddr, dst->sin.sin_addr.s_addr,
- tos, ttl, df, src_port, dst_port,
- htonl(vni << 8),
+ tos, ttl, df, src_port, dst_port, &md,
!net_eq(vxlan->net, dev_net(vxlan->dev)));
if (err < 0) {
/* skb is already freed. */
@@ -1806,10 +1843,12 @@ static void vxlan_xmit_one(struct sk_buff *skb, struct net_device *dev,
}
ttl = ttl ? : ip6_dst_hoplimit(ndst);
+ md.vni = htonl(vni << 8);
+ md.gbp = skb->mark;
err = vxlan6_xmit_skb(vxlan->vn_sock, ndst, skb,
dev, &fl6.saddr, &fl6.daddr, 0, ttl,
- src_port, dst_port, htonl(vni << 8),
+ src_port, dst_port, &md,
!net_eq(vxlan->net, dev_net(vxlan->dev)));
#endif
}
@@ -2210,6 +2249,11 @@ static const struct nla_policy vxlan_policy[IFLA_VXLAN_MAX + 1] = {
[IFLA_VXLAN_UDP_CSUM] = { .type = NLA_U8 },
[IFLA_VXLAN_UDP_ZERO_CSUM6_TX] = { .type = NLA_U8 },
[IFLA_VXLAN_UDP_ZERO_CSUM6_RX] = { .type = NLA_U8 },
+ [IFLA_VXLAN_EXTENSION] = { .type = NLA_NESTED },
+};
+
+static const struct nla_policy vxlan_ext_policy[IFLA_VXLAN_EXT_MAX + 1] = {
+ [IFLA_VXLAN_EXT_GBP] = { .type = NLA_FLAG, },
};
static int vxlan_validate(struct nlattr *tb[], struct nlattr *data[])
@@ -2246,6 +2290,18 @@ static int vxlan_validate(struct nlattr *tb[], struct nlattr *data[])
}
}
+ if (data[IFLA_VXLAN_EXTENSION]) {
+ int err;
+
+ err = nla_validate_nested(data[IFLA_VXLAN_EXTENSION],
+ IFLA_VXLAN_EXT_MAX, vxlan_ext_policy);
+ if (err < 0) {
+ pr_debug("invalid VXLAN extension configuration: %d\n",
+ err);
+ return -EINVAL;
+ }
+ }
+
return 0;
}
@@ -2400,6 +2456,18 @@ static void vxlan_sock_work(struct work_struct *work)
dev_put(vxlan->dev);
}
+static void configure_vxlan_exts(struct vxlan_dev *vxlan, struct nlattr *attr)
+{
+ struct nlattr *exts[IFLA_VXLAN_EXT_MAX+1];
+
+ /* Validated in vxlan_validate() */
+ if (nla_parse_nested(exts, IFLA_VXLAN_EXT_MAX, attr, NULL) < 0)
+ BUG();
+
+ if (exts[IFLA_VXLAN_EXT_GBP])
+ vxlan->exts |= VXLAN_EXT_GBP;
+}
+
static int vxlan_newlink(struct net *net, struct net_device *dev,
struct nlattr *tb[], struct nlattr *data[])
{
@@ -2525,6 +2593,9 @@ static int vxlan_newlink(struct net *net, struct net_device *dev,
nla_get_u8(data[IFLA_VXLAN_UDP_ZERO_CSUM6_RX]))
vxlan->flags |= VXLAN_F_UDP_ZERO_CSUM6_RX;
+ if (data[IFLA_VXLAN_EXTENSION])
+ configure_vxlan_exts(vxlan, data[IFLA_VXLAN_EXTENSION]);
+
if (vxlan_find_vni(net, vni, use_ipv6 ? AF_INET6 : AF_INET,
vxlan->dst_port)) {
pr_info("duplicate VNI %u\n", vni);
diff --git a/include/net/vxlan.h b/include/net/vxlan.h
index 3e98d31..66ec53c 100644
--- a/include/net/vxlan.h
+++ b/include/net/vxlan.h
@@ -11,13 +11,62 @@
#define VNI_HASH_BITS 10
#define VNI_HASH_SIZE (1<<VNI_HASH_BITS)
+/*
+ * VXLAN Group Based Policy Extension:
+ * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+ * |1|-|-|-|1|-|-|-|R|D|R|R|A|R|R|R| Group Policy ID |
+ * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+ * | VXLAN Network Identifier (VNI) | Reserved |
+ * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+ *
+ * D = Don't Learn bit. When set, this bit indicates that the egress
+ * VTEP MUST NOT learn the source address of the encapsulated frame.
+ *
+ * A = Indicates that the group policy has already been applied to
+ * this packet. Policies MUST NOT be applied by devices when the
+ * A bit is set.
+ *
+ * [0] https://tools.ietf.org/html/draft-smith-vxlan-group-policy
+ */
+struct vxlanhdr_gbp {
+ __u8 vx_flags;
+#ifdef __LITTLE_ENDIAN_BITFIELD
+ __u8 reserved_flags1:3,
+ policy_applied:1,
+ reserved_flags2:2,
+ dont_learn:1,
+ reserved_flags3:1;
+#elif defined(__BIG_ENDIAN_BITFIELD)
+ __u8 reserved_flags1:1,
+ dont_learn:1,
+ reserved_flags2:2,
+ policy_applied:1,
+ reserved_flags3:3;
+#else
+#error "Please fix <asm/byteorder.h>"
+#endif
+ __be16 policy_id;
+ __be32 vx_vni;
+};
+
+/* skb->mark mapping
+ *
+ * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+ * |R|R|R|R|R|R|R|R|R|D|R|R|A|R|R|R| Group Policy ID |
+ * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+ */
+#define VXLAN_GBP_DONT_LEARN (BIT(6) << 16)
+#define VXLAN_GBP_POLICY_APPLIED (BIT(3) << 16)
+#define VXLAN_GBP_ID_MASK (0xFFFF)
+
/* VXLAN protocol header:
* +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
- * |R|R|R|R|I|R|R|R| Reserved |
+ * |G|R|R|R|I|R|R|R| Reserved |
* +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
* | VXLAN Network Identifier (VNI) | Reserved |
* +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
*
+ * G = 1 Group Policy (VXLAN-GBP)
* I = 1 VXLAN Network Identifier (VNI) present
*/
struct vxlanhdr {
@@ -26,9 +75,11 @@ struct vxlanhdr {
#ifdef __LITTLE_ENDIAN_BITFIELD
__u8 reserved_flags1:3,
vni_present:1,
- reserved_flags2:4;
+ reserved_flags2:3,
+ gbp_present:1;
#elif defined(__BIG_ENDIAN_BITFIELD)
- __u8 reserved_flags2:4,
+ __u8 gbp_present:1,
+ reserved_flags2:3,
vni_present:1,
reserved_flags1:3;
#else
@@ -42,8 +93,16 @@ struct vxlanhdr {
__be32 vx_vni;
};
+struct vxlan_metadata {
+ __be32 vni;
+ u32 gbp;
+};
+
struct vxlan_sock;
-typedef void (vxlan_rcv_t)(struct vxlan_sock *vh, struct sk_buff *skb, __be32 key);
+typedef void (vxlan_rcv_t)(struct vxlan_sock *vh, struct sk_buff *skb,
+ struct vxlan_metadata *md);
+
+#define VXLAN_EXT_GBP BIT(0)
/* per UDP socket information */
struct vxlan_sock {
@@ -78,7 +137,8 @@ void vxlan_sock_release(struct vxlan_sock *vs);
int vxlan_xmit_skb(struct vxlan_sock *vs,
struct rtable *rt, struct sk_buff *skb,
__be32 src, __be32 dst, __u8 tos, __u8 ttl, __be16 df,
- __be16 src_port, __be16 dst_port, __be32 vni, bool xnet);
+ __be16 src_port, __be16 dst_port, struct vxlan_metadata *md,
+ bool xnet);
static inline netdev_features_t vxlan_features_check(struct sk_buff *skb,
netdev_features_t features)
diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
index f7d0d2d..9f07bf5 100644
--- a/include/uapi/linux/if_link.h
+++ b/include/uapi/linux/if_link.h
@@ -370,10 +370,18 @@ enum {
IFLA_VXLAN_UDP_CSUM,
IFLA_VXLAN_UDP_ZERO_CSUM6_TX,
IFLA_VXLAN_UDP_ZERO_CSUM6_RX,
+ IFLA_VXLAN_EXTENSION,
__IFLA_VXLAN_MAX
};
#define IFLA_VXLAN_MAX (__IFLA_VXLAN_MAX - 1)
+enum {
+ IFLA_VXLAN_EXT_UNSPEC,
+ IFLA_VXLAN_EXT_GBP,
+ __IFLA_VXLAN_EXT_MAX,
+};
+#define IFLA_VXLAN_EXT_MAX (__IFLA_VXLAN_EXT_MAX - 1)
+
struct ifla_vxlan_port_range {
__be16 low;
__be16 high;
diff --git a/net/openvswitch/vport-vxlan.c b/net/openvswitch/vport-vxlan.c
index d7c46b3..dd68c97 100644
--- a/net/openvswitch/vport-vxlan.c
+++ b/net/openvswitch/vport-vxlan.c
@@ -59,7 +59,8 @@ static inline struct vxlan_port *vxlan_vport(const struct vport *vport)
}
/* Called with rcu_read_lock and BH disabled. */
-static void vxlan_rcv(struct vxlan_sock *vs, struct sk_buff *skb, __be32 vx_vni)
+static void vxlan_rcv(struct vxlan_sock *vs, struct sk_buff *skb,
+ struct vxlan_metadata *md)
{
struct ovs_tunnel_info tun_info;
struct vport *vport = vs->data;
@@ -68,7 +69,7 @@ static void vxlan_rcv(struct vxlan_sock *vs, struct sk_buff *skb, __be32 vx_vni)
/* Save outer tunnel values */
iph = ip_hdr(skb);
- key = cpu_to_be64(ntohl(vx_vni) >> 8);
+ key = cpu_to_be64(ntohl(md->vni) >> 8);
ovs_flow_tun_info_init(&tun_info, iph,
udp_hdr(skb)->source, udp_hdr(skb)->dest,
key, TUNNEL_KEY, NULL, 0);
@@ -146,6 +147,7 @@ static int vxlan_tnl_send(struct vport *vport, struct sk_buff *skb)
struct vxlan_port *vxlan_port = vxlan_vport(vport);
__be16 dst_port = inet_sk(vxlan_port->vs->sock->sk)->inet_sport;
struct ovs_key_ipv4_tunnel *tun_key;
+ struct vxlan_metadata md;
struct rtable *rt;
struct flowi4 fl;
__be16 src_port;
@@ -178,12 +180,13 @@ static int vxlan_tnl_send(struct vport *vport, struct sk_buff *skb)
skb->ignore_df = 1;
src_port = udp_flow_src_port(net, skb, 0, 0, true);
+ md.vni = htonl(be64_to_cpu(tun_key->tun_id) << 8);
err = vxlan_xmit_skb(vxlan_port->vs, rt, skb,
fl.saddr, tun_key->ipv4_dst,
tun_key->ipv4_tos, tun_key->ipv4_ttl, df,
src_port, dst_port,
- htonl(be64_to_cpu(tun_key->tun_id) << 8),
+ &md,
false);
if (err < 0)
ip_rt_put(rt);
--
1.9.3
^ permalink raw reply related
* [PATCH 3/6] vxlan: Only bind to sockets with correct extensions enabled
From: Thomas Graf @ 2015-01-12 12:26 UTC (permalink / raw)
To: davem, jesse, stephen, pshelar, therbert, alexei.starovoitov; +Cc: dev, netdev
In-Reply-To: <cover.1421064100.git.tgraf@suug.ch>
A VXLAN net_device looking for an appropriate socket may only consider
a socket which has a matching set of extensions enabled. If the
extensions don't match, return a conflict to have the caller create a
distinct socket with distinct port.
The OVS VXLAN port is kept unaware of extensions at this point.
Signed-off-by: Thomas Graf <tgraf@suug.ch>
---
v2->v3:
- No change
v1->v2:
- Improved commit message, reported by Jesse
drivers/net/vxlan.c | 35 +++++++++++++++++++++--------------
include/net/vxlan.h | 2 +-
net/openvswitch/vport-vxlan.c | 2 +-
3 files changed, 23 insertions(+), 16 deletions(-)
diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
index b148739..61e1112 100644
--- a/drivers/net/vxlan.c
+++ b/drivers/net/vxlan.c
@@ -271,14 +271,15 @@ static inline struct vxlan_rdst *first_remote_rtnl(struct vxlan_fdb *fdb)
}
/* Find VXLAN socket based on network namespace, address family and UDP port */
-static struct vxlan_sock *vxlan_find_sock(struct net *net,
- sa_family_t family, __be16 port)
+static struct vxlan_sock *vxlan_find_sock(struct net *net, sa_family_t family,
+ __be16 port, u32 exts)
{
struct vxlan_sock *vs;
hlist_for_each_entry_rcu(vs, vs_head(net, port), hlist) {
if (inet_sk(vs->sock->sk)->inet_sport == port &&
- inet_sk(vs->sock->sk)->sk.sk_family == family)
+ inet_sk(vs->sock->sk)->sk.sk_family == family &&
+ vs->exts == exts)
return vs;
}
return NULL;
@@ -298,11 +299,12 @@ static struct vxlan_dev *vxlan_vs_find_vni(struct vxlan_sock *vs, u32 id)
/* Look up VNI in a per net namespace table */
static struct vxlan_dev *vxlan_find_vni(struct net *net, u32 id,
- sa_family_t family, __be16 port)
+ sa_family_t family, __be16 port,
+ u32 exts)
{
struct vxlan_sock *vs;
- vs = vxlan_find_sock(net, family, port);
+ vs = vxlan_find_sock(net, family, port, exts);
if (!vs)
return NULL;
@@ -1776,7 +1778,8 @@ static void vxlan_xmit_one(struct sk_buff *skb, struct net_device *dev,
ip_rt_put(rt);
dst_vxlan = vxlan_find_vni(vxlan->net, vni,
- dst->sa.sa_family, dst_port);
+ dst->sa.sa_family, dst_port,
+ vxlan->exts);
if (!dst_vxlan)
goto tx_error;
vxlan_encap_bypass(skb, vxlan, dst_vxlan);
@@ -1835,7 +1838,8 @@ static void vxlan_xmit_one(struct sk_buff *skb, struct net_device *dev,
dst_release(ndst);
dst_vxlan = vxlan_find_vni(vxlan->net, vni,
- dst->sa.sa_family, dst_port);
+ dst->sa.sa_family, dst_port,
+ vxlan->exts);
if (!dst_vxlan)
goto tx_error;
vxlan_encap_bypass(skb, vxlan, dst_vxlan);
@@ -2005,7 +2009,7 @@ static int vxlan_init(struct net_device *dev)
spin_lock(&vn->sock_lock);
vs = vxlan_find_sock(vxlan->net, ipv6 ? AF_INET6 : AF_INET,
- vxlan->dst_port);
+ vxlan->dst_port, vxlan->exts);
if (vs && atomic_add_unless(&vs->refcnt, 1, 0)) {
/* If we have a socket with same port already, reuse it */
vxlan_vs_add_dev(vs, vxlan);
@@ -2359,7 +2363,7 @@ static struct socket *vxlan_create_sock(struct net *net, bool ipv6,
/* Create new listen socket if needed */
static struct vxlan_sock *vxlan_socket_create(struct net *net, __be16 port,
vxlan_rcv_t *rcv, void *data,
- u32 flags)
+ u32 flags, u32 exts)
{
struct vxlan_net *vn = net_generic(net, vxlan_net_id);
struct vxlan_sock *vs;
@@ -2387,6 +2391,7 @@ static struct vxlan_sock *vxlan_socket_create(struct net *net, __be16 port,
atomic_set(&vs->refcnt, 1);
vs->rcv = rcv;
vs->data = data;
+ vs->exts = exts;
/* Initialize the vxlan udp offloads structure */
vs->udp_offloads.port = port;
@@ -2411,13 +2416,14 @@ static struct vxlan_sock *vxlan_socket_create(struct net *net, __be16 port,
struct vxlan_sock *vxlan_sock_add(struct net *net, __be16 port,
vxlan_rcv_t *rcv, void *data,
- bool no_share, u32 flags)
+ bool no_share, u32 flags,
+ u32 exts)
{
struct vxlan_net *vn = net_generic(net, vxlan_net_id);
struct vxlan_sock *vs;
bool ipv6 = flags & VXLAN_F_IPV6;
- vs = vxlan_socket_create(net, port, rcv, data, flags);
+ vs = vxlan_socket_create(net, port, rcv, data, flags, exts);
if (!IS_ERR(vs))
return vs;
@@ -2425,7 +2431,7 @@ struct vxlan_sock *vxlan_sock_add(struct net *net, __be16 port,
return vs;
spin_lock(&vn->sock_lock);
- vs = vxlan_find_sock(net, ipv6 ? AF_INET6 : AF_INET, port);
+ vs = vxlan_find_sock(net, ipv6 ? AF_INET6 : AF_INET, port, exts);
if (vs && ((vs->rcv != rcv) ||
!atomic_add_unless(&vs->refcnt, 1, 0)))
vs = ERR_PTR(-EBUSY);
@@ -2447,7 +2453,8 @@ static void vxlan_sock_work(struct work_struct *work)
__be16 port = vxlan->dst_port;
struct vxlan_sock *nvs;
- nvs = vxlan_sock_add(net, port, vxlan_rcv, NULL, false, vxlan->flags);
+ nvs = vxlan_sock_add(net, port, vxlan_rcv, NULL, false, vxlan->flags,
+ vxlan->exts);
spin_lock(&vn->sock_lock);
if (!IS_ERR(nvs))
vxlan_vs_add_dev(nvs, vxlan);
@@ -2597,7 +2604,7 @@ static int vxlan_newlink(struct net *net, struct net_device *dev,
configure_vxlan_exts(vxlan, data[IFLA_VXLAN_EXTENSION]);
if (vxlan_find_vni(net, vni, use_ipv6 ? AF_INET6 : AF_INET,
- vxlan->dst_port)) {
+ vxlan->dst_port, vxlan->exts)) {
pr_info("duplicate VNI %u\n", vni);
return -EEXIST;
}
diff --git a/include/net/vxlan.h b/include/net/vxlan.h
index 66ec53c..5ba49d5 100644
--- a/include/net/vxlan.h
+++ b/include/net/vxlan.h
@@ -130,7 +130,7 @@ struct vxlan_sock {
struct vxlan_sock *vxlan_sock_add(struct net *net, __be16 port,
vxlan_rcv_t *rcv, void *data,
- bool no_share, u32 flags);
+ bool no_share, u32 flags, u32 exts);
void vxlan_sock_release(struct vxlan_sock *vs);
diff --git a/net/openvswitch/vport-vxlan.c b/net/openvswitch/vport-vxlan.c
index dd68c97..266c595 100644
--- a/net/openvswitch/vport-vxlan.c
+++ b/net/openvswitch/vport-vxlan.c
@@ -128,7 +128,7 @@ static struct vport *vxlan_tnl_create(const struct vport_parms *parms)
vxlan_port = vxlan_vport(vport);
strncpy(vxlan_port->name, parms->name, IFNAMSIZ);
- vs = vxlan_sock_add(net, htons(dst_port), vxlan_rcv, vport, true, 0);
+ vs = vxlan_sock_add(net, htons(dst_port), vxlan_rcv, vport, true, 0, 0);
if (IS_ERR(vs)) {
ovs_vport_free(vport);
return (void *)vs;
--
1.9.3
^ permalink raw reply related
* [PATCH 1/6] vxlan: Allow for VXLAN extensions to be implemented
From: Thomas Graf @ 2015-01-12 12:26 UTC (permalink / raw)
To: davem, jesse, stephen, pshelar, therbert, alexei.starovoitov; +Cc: dev, netdev
In-Reply-To: <cover.1421064100.git.tgraf@suug.ch>
The VXLAN receive code is currently conservative in what it accepts and
will reject any frame that uses any of the reserved VXLAN protocol fields.
The VXLAN draft specifies that "reserved fields MUST be set to zero on
transmit and ignored on receive.".
Retain the current conservative parsing behaviour by default but allows
these fields to be used by VXLAN extensions which are explicitly enabled
on the VXLAN socket respectively VXLAN net_device.
Signed-off-by: Thomas Graf <tgraf@suug.ch>
---
v2->v3:
- No change
v1->v2:
- No change
drivers/net/vxlan.c | 29 +++++++++++++++++++----------
include/net/vxlan.h | 32 +++++++++++++++++++++++++++++---
2 files changed, 48 insertions(+), 13 deletions(-)
diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
index 2ab0922..4d52aa9 100644
--- a/drivers/net/vxlan.c
+++ b/drivers/net/vxlan.c
@@ -65,7 +65,7 @@
#define VXLAN_VID_MASK (VXLAN_N_VID - 1)
#define VXLAN_HLEN (sizeof(struct udphdr) + sizeof(struct vxlanhdr))
-#define VXLAN_FLAGS 0x08000000 /* struct vxlanhdr.vx_flags required value. */
+#define VXLAN_FLAGS 0x08000000 /* struct vxlanhdr.vx_flags default value. */
/* UDP port for VXLAN traffic.
* The IANA assigned port is 4789, but the Linux default is 8472
@@ -1100,22 +1100,28 @@ static int vxlan_udp_encap_recv(struct sock *sk, struct sk_buff *skb)
if (!pskb_may_pull(skb, VXLAN_HLEN))
goto error;
+ vs = rcu_dereference_sk_user_data(sk);
+ if (!vs)
+ goto drop;
+
/* Return packets with reserved bits set */
vxh = (struct vxlanhdr *)(udp_hdr(skb) + 1);
- if (vxh->vx_flags != htonl(VXLAN_FLAGS) ||
- (vxh->vx_vni & htonl(0xff))) {
- netdev_dbg(skb->dev, "invalid vxlan flags=%#x vni=%#x\n",
- ntohl(vxh->vx_flags), ntohl(vxh->vx_vni));
- goto error;
+
+ /* For backwards compatibility, only allow reserved fields to be
+ * used by VXLAN extensions if explicitly requested.
+ */
+ if (vs->exts) {
+ if (!vxh->vni_present)
+ goto error_invalid_header;
+ } else {
+ if (vxh->vx_flags != htonl(VXLAN_FLAGS) ||
+ (vxh->vx_vni & htonl(0xff)))
+ goto error_invalid_header;
}
if (iptunnel_pull_header(skb, VXLAN_HLEN, htons(ETH_P_TEB)))
goto drop;
- vs = rcu_dereference_sk_user_data(sk);
- if (!vs)
- goto drop;
-
vs->rcv(vs, skb, vxh->vx_vni);
return 0;
@@ -1124,6 +1130,9 @@ drop:
kfree_skb(skb);
return 0;
+error_invalid_header:
+ netdev_dbg(skb->dev, "invalid vxlan flags=%#x vni=%#x\n",
+ ntohl(vxh->vx_flags), ntohl(vxh->vx_vni));
error:
/* Return non vxlan pkt */
return 1;
diff --git a/include/net/vxlan.h b/include/net/vxlan.h
index 903461a..3e98d31 100644
--- a/include/net/vxlan.h
+++ b/include/net/vxlan.h
@@ -11,10 +11,35 @@
#define VNI_HASH_BITS 10
#define VNI_HASH_SIZE (1<<VNI_HASH_BITS)
-/* VXLAN protocol header */
+/* VXLAN protocol header:
+ * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+ * |R|R|R|R|I|R|R|R| Reserved |
+ * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+ * | VXLAN Network Identifier (VNI) | Reserved |
+ * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+ *
+ * I = 1 VXLAN Network Identifier (VNI) present
+ */
struct vxlanhdr {
- __be32 vx_flags;
- __be32 vx_vni;
+ union {
+ struct {
+#ifdef __LITTLE_ENDIAN_BITFIELD
+ __u8 reserved_flags1:3,
+ vni_present:1,
+ reserved_flags2:4;
+#elif defined(__BIG_ENDIAN_BITFIELD)
+ __u8 reserved_flags2:4,
+ vni_present:1,
+ reserved_flags1:3;
+#else
+#error "Please fix <asm/byteorder.h>"
+#endif
+ __u8 vx_reserved1;
+ __be16 vx_reserved2;
+ };
+ __be32 vx_flags;
+ };
+ __be32 vx_vni;
};
struct vxlan_sock;
@@ -25,6 +50,7 @@ struct vxlan_sock {
struct hlist_node hlist;
vxlan_rcv_t *rcv;
void *data;
+ u32 exts;
struct work_struct del_work;
struct socket *sock;
struct rcu_head rcu;
--
1.9.3
^ permalink raw reply related
* [PATCH 0/6 net-next v3] VXLAN Group Policy Extension
From: Thomas Graf @ 2015-01-12 12:26 UTC (permalink / raw)
To: davem, jesse, stephen, pshelar, therbert, alexei.starovoitov; +Cc: dev, netdev
Implements supports for the Group Policy VXLAN extension [0] to provide
a lightweight and simple security label mechanism across network peers
based on VXLAN. The security context and associated metadata is mapped
to/from skb->mark. This allows further mapping to a SELinux context
using SECMARK, to implement ACLs directly with nftables, iptables, OVS,
tc, etc.
The extension is disabled by default and should be run on a distinct
port in mixed Linux VXLAN VTEP environments. Liberal VXLAN VTEPs
which ignore unknown reserved bits will be able to receive VXLAN-GBP
frames.
Simple usage example:
10.1.1.1:
# ip link add vxlan0 type vxlan id 10 remote 10.1.1.2 gbp
# iptables -I OUTPUT -m owner --uid-owner 101 -j MARK --set-mark 0x200
10.1.1.2:
# ip link add vxlan0 type vxlan id 10 remote 10.1.1.1 gbp
# iptables -I INPUT -m mark --mark 0x200 -j DROP
iproute2 [1] and OVS [2] support will be provided in separate patches.
[0] https://tools.ietf.org/html/draft-smith-vxlan-group-policy
[1] https://github.com/tgraf/iproute2/tree/vxlan-gbp
[2] https://github.com/tgraf/ovs/tree/vxlan-gbp
Thomas Graf (6):
vxlan: Allow for VXLAN extensions to be implemented
vxlan: Group Policy extension
vxlan: Only bind to sockets with correct extensions enabled
openvswitch: Rename GENEVE_TUN_OPTS() to TUN_METADATA_OPTS()
openvswitch: Allow for any level of nesting in flow attributes
openvswitch: Support VXLAN Group Policy extension
drivers/net/vxlan.c | 225 ++++++++++++++++++++----------
include/net/ip_tunnels.h | 5 +-
include/net/vxlan.h | 98 +++++++++++++-
include/uapi/linux/if_link.h | 8 ++
include/uapi/linux/openvswitch.h | 11 ++
net/openvswitch/flow.c | 2 +-
net/openvswitch/flow.h | 14 +-
net/openvswitch/flow_netlink.c | 286 ++++++++++++++++++++++++++-------------
net/openvswitch/vport-geneve.c | 2 +-
net/openvswitch/vport-vxlan.c | 90 +++++++++++-
net/openvswitch/vport-vxlan.h | 11 ++
11 files changed, 569 insertions(+), 183 deletions(-)
create mode 100644 net/openvswitch/vport-vxlan.h
--
1.9.3
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox