* [PATCH v7 1/7] can: preserve skbuff protocol in can_put_echo_skb
2014-02-28 10:26 [PATCH v7 0/7] Add CAN FD infrastructure for CAN driver Oliver Hartkopp
@ 2014-02-28 10:26 ` Oliver Hartkopp
2014-02-28 10:26 ` [PATCH v7 2/7] can: only send bitrate data via netlink when available Oliver Hartkopp
` (5 subsequent siblings)
6 siblings, 0 replies; 12+ messages in thread
From: Oliver Hartkopp @ 2014-02-28 10:26 UTC (permalink / raw)
To: linux-can; +Cc: Oliver Hartkopp
The skbuff protocol value was formerly fixed/sanitized to ETH_P_CAN in
can_put_echo_skb(). With CAN FD this value has to be preserved.
This patch changes the hard assignment of the protocol value to a check of
valid protocol values for CAN and CAN FD.
Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
---
drivers/net/can/dev.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
index c0563f1..e1a3741 100644
--- a/drivers/net/can/dev.c
+++ b/drivers/net/can/dev.c
@@ -317,7 +317,9 @@ void can_put_echo_skb(struct sk_buff *skb, struct net_device *dev,
BUG_ON(idx >= priv->echo_skb_max);
/* check flag whether this packet has to be looped back */
- if (!(dev->flags & IFF_ECHO) || skb->pkt_type != PACKET_LOOPBACK) {
+ if (!(dev->flags & IFF_ECHO) || skb->pkt_type != PACKET_LOOPBACK ||
+ (skb->protocol != htons(ETH_P_CAN) &&
+ skb->protocol != htons(ETH_P_CANFD))) {
kfree_skb(skb);
return;
}
@@ -329,7 +331,6 @@ void can_put_echo_skb(struct sk_buff *skb, struct net_device *dev,
return;
/* make settings for echo to reduce code in irq context */
- skb->protocol = htons(ETH_P_CAN);
skb->pkt_type = PACKET_BROADCAST;
skb->ip_summed = CHECKSUM_UNNECESSARY;
skb->dev = dev;
--
1.9.0
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v7 2/7] can: only send bitrate data via netlink when available
2014-02-28 10:26 [PATCH v7 0/7] Add CAN FD infrastructure for CAN driver Oliver Hartkopp
2014-02-28 10:26 ` [PATCH v7 1/7] can: preserve skbuff protocol in can_put_echo_skb Oliver Hartkopp
@ 2014-02-28 10:26 ` Oliver Hartkopp
2014-02-28 10:26 ` [PATCH v7 3/7] can: move sanity check for bitrate and tq into can_get_bittiming Oliver Hartkopp
` (4 subsequent siblings)
6 siblings, 0 replies; 12+ messages in thread
From: Oliver Hartkopp @ 2014-02-28 10:26 UTC (permalink / raw)
To: linux-can; +Cc: Oliver Hartkopp
When setting the bitrate both can_calc_bittiming() and can_fixup_bittiming()
lead to the bitrate variable to be set, when a proper bit timing is available.
Only then the bitrate configuration is stored for the device, so checking for
priv->bittiming.bitrate is always sufficient.
Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
---
drivers/net/can/dev.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
index e1a3741..de04eac 100644
--- a/drivers/net/can/dev.c
+++ b/drivers/net/can/dev.c
@@ -606,7 +606,7 @@ int open_candev(struct net_device *dev)
{
struct can_priv *priv = netdev_priv(dev);
- if (!priv->bittiming.tq && !priv->bittiming.bitrate) {
+ if (!priv->bittiming.bitrate) {
netdev_err(dev, "bit-timing not yet defined\n");
return -EINVAL;
}
@@ -719,7 +719,8 @@ static size_t can_get_size(const struct net_device *dev)
struct can_priv *priv = netdev_priv(dev);
size_t size = 0;
- size += nla_total_size(sizeof(struct can_bittiming)); /* IFLA_CAN_BITTIMING */
+ if (priv->bittiming.bitrate) /* IFLA_CAN_BITTIMING */
+ size += nla_total_size(sizeof(struct can_bittiming));
if (priv->bittiming_const) /* IFLA_CAN_BITTIMING_CONST */
size += nla_total_size(sizeof(struct can_bittiming_const));
size += nla_total_size(sizeof(struct can_clock)); /* IFLA_CAN_CLOCK */
@@ -741,8 +742,9 @@ static int can_fill_info(struct sk_buff *skb, const struct net_device *dev)
if (priv->do_get_state)
priv->do_get_state(dev, &state);
- if (nla_put(skb, IFLA_CAN_BITTIMING,
- sizeof(priv->bittiming), &priv->bittiming) ||
+ if ((priv->bittiming.bitrate &&
+ nla_put(skb, IFLA_CAN_BITTIMING,
+ sizeof(priv->bittiming), &priv->bittiming)) ||
(priv->bittiming_const &&
nla_put(skb, IFLA_CAN_BITTIMING_CONST,
sizeof(*priv->bittiming_const), priv->bittiming_const)) ||
--
1.9.0
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v7 3/7] can: move sanity check for bitrate and tq into can_get_bittiming
2014-02-28 10:26 [PATCH v7 0/7] Add CAN FD infrastructure for CAN driver Oliver Hartkopp
2014-02-28 10:26 ` [PATCH v7 1/7] can: preserve skbuff protocol in can_put_echo_skb Oliver Hartkopp
2014-02-28 10:26 ` [PATCH v7 2/7] can: only send bitrate data via netlink when available Oliver Hartkopp
@ 2014-02-28 10:26 ` Oliver Hartkopp
2014-02-28 10:26 ` [PATCH v7 4/7] can: provide a separate bittiming_const parameter to bittiming functions Oliver Hartkopp
` (3 subsequent siblings)
6 siblings, 0 replies; 12+ messages in thread
From: Oliver Hartkopp @ 2014-02-28 10:26 UTC (permalink / raw)
To: linux-can; +Cc: Oliver Hartkopp
This patch moves a sanity check in order to have a second user for CAN FD.
Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
---
drivers/net/can/dev.c | 17 +++++++++++------
1 file changed, 11 insertions(+), 6 deletions(-)
diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
index de04eac..89fc798 100644
--- a/drivers/net/can/dev.c
+++ b/drivers/net/can/dev.c
@@ -263,12 +263,19 @@ static int can_get_bittiming(struct net_device *dev, struct can_bittiming *bt)
if (priv->bittiming_const) {
/* Non-expert mode? Check if the bitrate has been pre-defined */
- if (!bt->tq)
+ if (!bt->tq) {
/* Determine bit-timing parameters */
- err = can_calc_bittiming(dev, bt);
- else
+ if (bt->bitrate)
+ err = can_calc_bittiming(dev, bt);
+ else
+ return -EINVAL;
+ } else {
/* Check bit-timing params and calculate proper brp */
- err = can_fixup_bittiming(dev, bt);
+ if (!bt->bitrate)
+ err = can_fixup_bittiming(dev, bt);
+ else
+ return -EINVAL;
+ }
if (err)
return err;
}
@@ -667,8 +674,6 @@ static int can_changelink(struct net_device *dev,
if (dev->flags & IFF_UP)
return -EBUSY;
memcpy(&bt, nla_data(data[IFLA_CAN_BITTIMING]), sizeof(bt));
- if ((!bt.bitrate && !bt.tq) || (bt.bitrate && bt.tq))
- return -EINVAL;
err = can_get_bittiming(dev, &bt);
if (err)
return err;
--
1.9.0
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v7 4/7] can: provide a separate bittiming_const parameter to bittiming functions
2014-02-28 10:26 [PATCH v7 0/7] Add CAN FD infrastructure for CAN driver Oliver Hartkopp
` (2 preceding siblings ...)
2014-02-28 10:26 ` [PATCH v7 3/7] can: move sanity check for bitrate and tq into can_get_bittiming Oliver Hartkopp
@ 2014-02-28 10:26 ` Oliver Hartkopp
2014-02-28 10:46 ` Marc Kleine-Budde
2014-02-28 10:26 ` [PATCH v7 5/7] can: introduce the data bitrate configuration for CAN FD Oliver Hartkopp
` (2 subsequent siblings)
6 siblings, 1 reply; 12+ messages in thread
From: Oliver Hartkopp @ 2014-02-28 10:26 UTC (permalink / raw)
To: linux-can; +Cc: Oliver Hartkopp
As the bittiming calculation functions are to be used with different
bittiming_const structures for CAN and CAN FD the direct reference to
priv->bittiming_const inside these functions has to be removed.
Also simplify the return value generation in can_get_bittiming() as only
correct return values of can_[calc|fixup]_bittiming() lead to a return value
of zero. And moved the check for existing bittiming const to one place.
Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
---
drivers/net/can/dev.c | 59 ++++++++++++++++++++++-----------------------------
1 file changed, 25 insertions(+), 34 deletions(-)
diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
index 89fc798..c8cd528 100644
--- a/drivers/net/can/dev.c
+++ b/drivers/net/can/dev.c
@@ -99,10 +99,10 @@ static int can_update_spt(const struct can_bittiming_const *btc,
return 1000 * (tseg + 1 - *tseg2) / (tseg + 1);
}
-static int can_calc_bittiming(struct net_device *dev, struct can_bittiming *bt)
+static int can_calc_bittiming(struct net_device *dev, struct can_bittiming *bt,
+ const struct can_bittiming_const *btc)
{
struct can_priv *priv = netdev_priv(dev);
- const struct can_bittiming_const *btc = priv->bittiming_const;
long rate, best_rate = 0;
long best_error = 1000000000, error = 0;
int best_tseg = 0, best_brp = 0, brp = 0;
@@ -110,9 +110,6 @@ static int can_calc_bittiming(struct net_device *dev, struct can_bittiming *bt)
int spt_error = 1000, spt = 0, sampl_pt;
u64 v64;
- if (!priv->bittiming_const)
- return -ENOTSUPP;
-
/* Use CIA recommended sample points */
if (bt->sample_point) {
sampl_pt = bt->sample_point;
@@ -204,7 +201,8 @@ static int can_calc_bittiming(struct net_device *dev, struct can_bittiming *bt)
return 0;
}
#else /* !CONFIG_CAN_CALC_BITTIMING */
-static int can_calc_bittiming(struct net_device *dev, struct can_bittiming *bt)
+static int can_calc_bittiming(struct net_device *dev, struct can_bittiming *bt,
+ const struct can_bittiming_const *btc)
{
netdev_err(dev, "bit-timing calculation not available\n");
return -EINVAL;
@@ -217,16 +215,13 @@ static int can_calc_bittiming(struct net_device *dev, struct can_bittiming *bt)
* prescaler value brp. You can find more information in the header
* file linux/can/netlink.h.
*/
-static int can_fixup_bittiming(struct net_device *dev, struct can_bittiming *bt)
+static int can_fixup_bittiming(struct net_device *dev, struct can_bittiming *bt,
+ const struct can_bittiming_const *btc)
{
struct can_priv *priv = netdev_priv(dev);
- const struct can_bittiming_const *btc = priv->bittiming_const;
int tseg1, alltseg;
u64 brp64;
- if (!priv->bittiming_const)
- return -ENOTSUPP;
-
tseg1 = bt->prop_seg + bt->phase_seg1;
if (!bt->sjw)
bt->sjw = 1;
@@ -254,33 +249,29 @@ static int can_fixup_bittiming(struct net_device *dev, struct can_bittiming *bt)
return 0;
}
-static int can_get_bittiming(struct net_device *dev, struct can_bittiming *bt)
+static int can_get_bittiming(struct net_device *dev, struct can_bittiming *bt,
+ const struct can_bittiming_const *btc)
{
- struct can_priv *priv = netdev_priv(dev);
- int err;
+ int err = -EINVAL;
/* Check if the CAN device has bit-timing parameters */
- if (priv->bittiming_const) {
-
- /* Non-expert mode? Check if the bitrate has been pre-defined */
- if (!bt->tq) {
- /* Determine bit-timing parameters */
- if (bt->bitrate)
- err = can_calc_bittiming(dev, bt);
- else
- return -EINVAL;
- } else {
- /* Check bit-timing params and calculate proper brp */
- if (!bt->bitrate)
- err = can_fixup_bittiming(dev, bt);
- else
- return -EINVAL;
- }
- if (err)
- return err;
+ if (!btc)
+ return -ENOTSUPP;
+
+ if (!bt->tq) {
+ /* non-expert mode: Determine bit-timing parameters */
+ if (bt->bitrate)
+ err = can_calc_bittiming(dev, bt, btc);
+ } else {
+ /*
+ * expert mode: Check bit-timing parameters and
+ * calculate proper brp and a human readable bitrate
+ */
+ if (!bt->bitrate)
+ err = can_fixup_bittiming(dev, bt, btc);
}
- return 0;
+ return err;
}
/*
@@ -674,7 +665,7 @@ static int can_changelink(struct net_device *dev,
if (dev->flags & IFF_UP)
return -EBUSY;
memcpy(&bt, nla_data(data[IFLA_CAN_BITTIMING]), sizeof(bt));
- err = can_get_bittiming(dev, &bt);
+ err = can_get_bittiming(dev, &bt, priv->bittiming_const);
if (err)
return err;
memcpy(&priv->bittiming, &bt, sizeof(bt));
--
1.9.0
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v7 4/7] can: provide a separate bittiming_const parameter to bittiming functions
2014-02-28 10:26 ` [PATCH v7 4/7] can: provide a separate bittiming_const parameter to bittiming functions Oliver Hartkopp
@ 2014-02-28 10:46 ` Marc Kleine-Budde
2014-02-28 12:31 ` Oliver Hartkopp
0 siblings, 1 reply; 12+ messages in thread
From: Marc Kleine-Budde @ 2014-02-28 10:46 UTC (permalink / raw)
To: Oliver Hartkopp, linux-can
[-- Attachment #1: Type: text/plain, Size: 5403 bytes --]
On 02/28/2014 11:26 AM, Oliver Hartkopp wrote:
> As the bittiming calculation functions are to be used with different
> bittiming_const structures for CAN and CAN FD the direct reference to
> priv->bittiming_const inside these functions has to be removed.
>
> Also simplify the return value generation in can_get_bittiming() as only
> correct return values of can_[calc|fixup]_bittiming() lead to a return value
> of zero. And moved the check for existing bittiming const to one place.
>
> Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
> ---
> drivers/net/can/dev.c | 59 ++++++++++++++++++++++-----------------------------
> 1 file changed, 25 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
> index 89fc798..c8cd528 100644
> --- a/drivers/net/can/dev.c
> +++ b/drivers/net/can/dev.c
> @@ -99,10 +99,10 @@ static int can_update_spt(const struct can_bittiming_const *btc,
> return 1000 * (tseg + 1 - *tseg2) / (tseg + 1);
> }
>
> -static int can_calc_bittiming(struct net_device *dev, struct can_bittiming *bt)
> +static int can_calc_bittiming(struct net_device *dev, struct can_bittiming *bt,
> + const struct can_bittiming_const *btc)
> {
> struct can_priv *priv = netdev_priv(dev);
> - const struct can_bittiming_const *btc = priv->bittiming_const;
> long rate, best_rate = 0;
> long best_error = 1000000000, error = 0;
> int best_tseg = 0, best_brp = 0, brp = 0;
> @@ -110,9 +110,6 @@ static int can_calc_bittiming(struct net_device *dev, struct can_bittiming *bt)
> int spt_error = 1000, spt = 0, sampl_pt;
> u64 v64;
>
> - if (!priv->bittiming_const)
> - return -ENOTSUPP;
> -
> /* Use CIA recommended sample points */
> if (bt->sample_point) {
> sampl_pt = bt->sample_point;
> @@ -204,7 +201,8 @@ static int can_calc_bittiming(struct net_device *dev, struct can_bittiming *bt)
> return 0;
> }
> #else /* !CONFIG_CAN_CALC_BITTIMING */
> -static int can_calc_bittiming(struct net_device *dev, struct can_bittiming *bt)
> +static int can_calc_bittiming(struct net_device *dev, struct can_bittiming *bt,
> + const struct can_bittiming_const *btc)
> {
> netdev_err(dev, "bit-timing calculation not available\n");
> return -EINVAL;
> @@ -217,16 +215,13 @@ static int can_calc_bittiming(struct net_device *dev, struct can_bittiming *bt)
> * prescaler value brp. You can find more information in the header
> * file linux/can/netlink.h.
> */
> -static int can_fixup_bittiming(struct net_device *dev, struct can_bittiming *bt)
> +static int can_fixup_bittiming(struct net_device *dev, struct can_bittiming *bt,
> + const struct can_bittiming_const *btc)
> {
> struct can_priv *priv = netdev_priv(dev);
> - const struct can_bittiming_const *btc = priv->bittiming_const;
> int tseg1, alltseg;
> u64 brp64;
>
> - if (!priv->bittiming_const)
> - return -ENOTSUPP;
> -
> tseg1 = bt->prop_seg + bt->phase_seg1;
> if (!bt->sjw)
> bt->sjw = 1;
> @@ -254,33 +249,29 @@ static int can_fixup_bittiming(struct net_device *dev, struct can_bittiming *bt)
> return 0;
> }
>
> -static int can_get_bittiming(struct net_device *dev, struct can_bittiming *bt)
> +static int can_get_bittiming(struct net_device *dev, struct can_bittiming *bt,
> + const struct can_bittiming_const *btc)
> {
> - struct can_priv *priv = netdev_priv(dev);
> - int err;
> + int err = -EINVAL;
Keep err uninitialized and make an explicit else err = -EINVAL.... see
below:
>
> /* Check if the CAN device has bit-timing parameters */
> - if (priv->bittiming_const) {
> -
> - /* Non-expert mode? Check if the bitrate has been pre-defined */
> - if (!bt->tq) {
> - /* Determine bit-timing parameters */
> - if (bt->bitrate)
> - err = can_calc_bittiming(dev, bt);
> - else
> - return -EINVAL;
> - } else {
> - /* Check bit-timing params and calculate proper brp */
> - if (!bt->bitrate)
> - err = can_fixup_bittiming(dev, bt);
> - else
> - return -EINVAL;
> - }
> - if (err)
> - return err;
> + if (!btc)
> + return -ENOTSUPP;
> +
> + if (!bt->tq) {
> + /* non-expert mode: Determine bit-timing parameters */
> + if (bt->bitrate)
Why not:
if (!bt->tq && bt->bitrate) {
> + err = can_calc_bittiming(dev, bt, btc);
> + } else {
} else if (bt->tq && !bt->bitrate) {
> + /*
> + * expert mode: Check bit-timing parameters and
> + * calculate proper brp and a human readable bitrate
> + */
> + if (!bt->bitrate)
> + err = can_fixup_bittiming(dev, bt, btc);
> }
} else {
err = -EINVAL;
}
>
> - return 0;
> + return err;
> }
>
> /*
> @@ -674,7 +665,7 @@ static int can_changelink(struct net_device *dev,
> if (dev->flags & IFF_UP)
> return -EBUSY;
> memcpy(&bt, nla_data(data[IFLA_CAN_BITTIMING]), sizeof(bt));
> - err = can_get_bittiming(dev, &bt);
> + err = can_get_bittiming(dev, &bt, priv->bittiming_const);
> if (err)
> return err;
> memcpy(&priv->bittiming, &bt, sizeof(bt));
>
--
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: 242 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v7 4/7] can: provide a separate bittiming_const parameter to bittiming functions
2014-02-28 10:46 ` Marc Kleine-Budde
@ 2014-02-28 12:31 ` Oliver Hartkopp
2014-02-28 13:02 ` Marc Kleine-Budde
0 siblings, 1 reply; 12+ messages in thread
From: Oliver Hartkopp @ 2014-02-28 12:31 UTC (permalink / raw)
To: Marc Kleine-Budde, linux-can
On 28.02.2014 11:46, Marc Kleine-Budde wrote:
> On 02/28/2014 11:26 AM, Oliver Hartkopp wrote:
>> {
>> - struct can_priv *priv = netdev_priv(dev);
>> - int err;
>> + int err = -EINVAL;
>
> Keep err uninitialized and make an explicit else err = -EINVAL.... see
> below:
>
>> + if (!bt->tq) {
>> + /* non-expert mode: Determine bit-timing parameters */
>> + if (bt->bitrate)
>
> Why not:
> if (!bt->tq && bt->bitrate) {
>
>> + err = can_calc_bittiming(dev, bt, btc);
>> + } else {
>
> } else if (bt->tq && !bt->bitrate) {
>
>> + /*
>> + * expert mode: Check bit-timing parameters and
>> + * calculate proper brp and a human readable bitrate
>> + */
>> + if (!bt->bitrate)
>> + err = can_fixup_bittiming(dev, bt, btc);
>> }
> } else {
> err = -EINVAL;
> }
I don't have a strong preference on my implementation.
But IMHO your suggested changes only add additional code and additional
comparisons in the if statement.
I don't see a benefit in your suggestions :-(
Regards,
Oliver
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v7 4/7] can: provide a separate bittiming_const parameter to bittiming functions
2014-02-28 12:31 ` Oliver Hartkopp
@ 2014-02-28 13:02 ` Marc Kleine-Budde
2014-02-28 13:07 ` Oliver Hartkopp
0 siblings, 1 reply; 12+ messages in thread
From: Marc Kleine-Budde @ 2014-02-28 13:02 UTC (permalink / raw)
To: Oliver Hartkopp, linux-can
[-- Attachment #1: Type: text/plain, Size: 1726 bytes --]
On 02/28/2014 01:31 PM, Oliver Hartkopp wrote:
>
>
> On 28.02.2014 11:46, Marc Kleine-Budde wrote:
>> On 02/28/2014 11:26 AM, Oliver Hartkopp wrote:
>
>
>>> {
>>> - struct can_priv *priv = netdev_priv(dev);
>>> - int err;
>>> + int err = -EINVAL;
>>
>> Keep err uninitialized and make an explicit else err = -EINVAL.... see
>> below:
>>
>
>
>>> + if (!bt->tq) {
>>> + /* non-expert mode: Determine bit-timing parameters */
>>> + if (bt->bitrate)
>>
>> Why not:
>> if (!bt->tq && bt->bitrate) {
>>
>>> + err = can_calc_bittiming(dev, bt, btc);
>>> + } else {
>>
>> } else if (bt->tq && !bt->bitrate) {
>>
>>> + /*
>>> + * expert mode: Check bit-timing parameters and
>>> + * calculate proper brp and a human readable bitrate
>>> + */
>>> + if (!bt->bitrate)
>>> + err = can_fixup_bittiming(dev, bt, btc);
>>> }
>> } else {
>> err = -EINVAL;
>> }
>
> I don't have a strong preference on my implementation.
>
> But IMHO your suggested changes only add additional code and additional
> comparisons in the if statement.
>
> I don't see a benefit in your suggestions :-(
You have 3 seperate if()s and an implicit error value from the
initialisation. This is IMHO more straight forward:
if (!bt->tq && bt->bitrate)
err = can_calc_bittiming(dev, bt, btc);
else if (bt->tq && !bt->bitrate)
err = can_fixup_bittiming(dev, bt, btc);
else
err = -EINVAL;
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: 242 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v7 4/7] can: provide a separate bittiming_const parameter to bittiming functions
2014-02-28 13:02 ` Marc Kleine-Budde
@ 2014-02-28 13:07 ` Oliver Hartkopp
0 siblings, 0 replies; 12+ messages in thread
From: Oliver Hartkopp @ 2014-02-28 13:07 UTC (permalink / raw)
To: Marc Kleine-Budde, linux-can
On 28.02.2014 14:02, Marc Kleine-Budde wrote:
> On 02/28/2014 01:31 PM, Oliver Hartkopp wrote:
>>
>>
>> On 28.02.2014 11:46, Marc Kleine-Budde wrote:
>>> On 02/28/2014 11:26 AM, Oliver Hartkopp wrote:
>>
>>
>>>> {
>>>> - struct can_priv *priv = netdev_priv(dev);
>>>> - int err;
>>>> + int err = -EINVAL;
>>>
>>> Keep err uninitialized and make an explicit else err = -EINVAL.... see
>>> below:
>>>
>>
>>
>>>> + if (!bt->tq) {
>>>> + /* non-expert mode: Determine bit-timing parameters */
>>>> + if (bt->bitrate)
>>>
>>> Why not:
>>> if (!bt->tq && bt->bitrate) {
>>>
>>>> + err = can_calc_bittiming(dev, bt, btc);
>>>> + } else {
>>>
>>> } else if (bt->tq && !bt->bitrate) {
>>>
>>>> + /*
>>>> + * expert mode: Check bit-timing parameters and
>>>> + * calculate proper brp and a human readable bitrate
>>>> + */
>>>> + if (!bt->bitrate)
>>>> + err = can_fixup_bittiming(dev, bt, btc);
>>>> }
>>> } else {
>>> err = -EINVAL;
>>> }
>>
>> I don't have a strong preference on my implementation.
>>
>> But IMHO your suggested changes only add additional code and additional
>> comparisons in the if statement.
>>
>> I don't see a benefit in your suggestions :-(
>
> You have 3 seperate if()s and an implicit error value from the
> initialisation. This is IMHO more straight forward:
>
> if (!bt->tq && bt->bitrate)
> err = can_calc_bittiming(dev, bt, btc);
> else if (bt->tq && !bt->bitrate)
> err = can_fixup_bittiming(dev, bt, btc);
> else
> err = -EINVAL;
>
Ok - looks good too.
Will change this in the v8 patch series.
Regards,
Oliver
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v7 5/7] can: introduce the data bitrate configuration for CAN FD
2014-02-28 10:26 [PATCH v7 0/7] Add CAN FD infrastructure for CAN driver Oliver Hartkopp
` (3 preceding siblings ...)
2014-02-28 10:26 ` [PATCH v7 4/7] can: provide a separate bittiming_const parameter to bittiming functions Oliver Hartkopp
@ 2014-02-28 10:26 ` Oliver Hartkopp
2014-02-28 10:26 ` [PATCH v7 6/7] can: allow to change the device mtu for CAN FD capable devices Oliver Hartkopp
2014-02-28 10:26 ` [PATCH v7 7/7] can: add bittiming check at interface open for CAN FD Oliver Hartkopp
6 siblings, 0 replies; 12+ messages in thread
From: Oliver Hartkopp @ 2014-02-28 10:26 UTC (permalink / raw)
To: linux-can; +Cc: Oliver Hartkopp
As CAN FD offers a second bitrate for the data section of the CAN frame the
infrastructure for storing and configuring this second bitrate is introduced.
Improved the readability of the if-statement by inserting some newlines.
Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
---
drivers/net/can/dev.c | 45 +++++++++++++++++++++++++++++++++++++++-
include/linux/can/dev.h | 6 ++++--
include/uapi/linux/can/netlink.h | 2 ++
3 files changed, 50 insertions(+), 3 deletions(-)
diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
index c8cd528..b2b0a9b 100644
--- a/drivers/net/can/dev.c
+++ b/drivers/net/can/dev.c
@@ -647,6 +647,10 @@ static const struct nla_policy can_policy[IFLA_CAN_MAX + 1] = {
= { .len = sizeof(struct can_bittiming_const) },
[IFLA_CAN_CLOCK] = { .len = sizeof(struct can_clock) },
[IFLA_CAN_BERR_COUNTER] = { .len = sizeof(struct can_berr_counter) },
+ [IFLA_CAN_DATA_BITTIMING]
+ = { .len = sizeof(struct can_bittiming) },
+ [IFLA_CAN_DATA_BITTIMING_CONST]
+ = { .len = sizeof(struct can_bittiming_const) },
};
static int can_changelink(struct net_device *dev,
@@ -678,6 +682,27 @@ static int can_changelink(struct net_device *dev,
}
}
+ if (data[IFLA_CAN_DATA_BITTIMING]) {
+ struct can_bittiming dbt;
+
+ /* Do not allow changing bittiming while running */
+ if (dev->flags & IFF_UP)
+ return -EBUSY;
+ memcpy(&dbt, nla_data(data[IFLA_CAN_DATA_BITTIMING]),
+ sizeof(dbt));
+ err = can_get_bittiming(dev, &dbt, priv->data_bittiming_const);
+ if (err)
+ return err;
+ memcpy(&priv->data_bittiming, &dbt, sizeof(dbt));
+
+ if (priv->do_set_data_bittiming) {
+ /* Finally, set the bit-timing registers */
+ err = priv->do_set_data_bittiming(dev);
+ if (err)
+ return err;
+ }
+ }
+
if (data[IFLA_CAN_CTRLMODE]) {
struct can_ctrlmode *cm;
@@ -725,6 +750,10 @@ static size_t can_get_size(const struct net_device *dev)
size += nla_total_size(sizeof(u32)); /* IFLA_CAN_RESTART_MS */
if (priv->do_get_berr_counter) /* IFLA_CAN_BERR_COUNTER */
size += nla_total_size(sizeof(struct can_berr_counter));
+ if (priv->data_bittiming.bitrate) /* IFLA_CAN_DATA_BITTIMING */
+ size += nla_total_size(sizeof(struct can_bittiming));
+ if (priv->data_bittiming_const) /* IFLA_CAN_DATA_BITTIMING_CONST */
+ size += nla_total_size(sizeof(struct can_bittiming_const));
return size;
}
@@ -738,20 +767,34 @@ static int can_fill_info(struct sk_buff *skb, const struct net_device *dev)
if (priv->do_get_state)
priv->do_get_state(dev, &state);
+
if ((priv->bittiming.bitrate &&
nla_put(skb, IFLA_CAN_BITTIMING,
sizeof(priv->bittiming), &priv->bittiming)) ||
+
(priv->bittiming_const &&
nla_put(skb, IFLA_CAN_BITTIMING_CONST,
sizeof(*priv->bittiming_const), priv->bittiming_const)) ||
+
nla_put(skb, IFLA_CAN_CLOCK, sizeof(cm), &priv->clock) ||
nla_put_u32(skb, IFLA_CAN_STATE, state) ||
nla_put(skb, IFLA_CAN_CTRLMODE, sizeof(cm), &cm) ||
nla_put_u32(skb, IFLA_CAN_RESTART_MS, priv->restart_ms) ||
+
(priv->do_get_berr_counter &&
!priv->do_get_berr_counter(dev, &bec) &&
- nla_put(skb, IFLA_CAN_BERR_COUNTER, sizeof(bec), &bec)))
+ nla_put(skb, IFLA_CAN_BERR_COUNTER, sizeof(bec), &bec)) ||
+
+ (priv->data_bittiming.bitrate &&
+ nla_put(skb, IFLA_CAN_DATA_BITTIMING,
+ sizeof(priv->data_bittiming), &priv->data_bittiming)) ||
+
+ (priv->data_bittiming_const &&
+ nla_put(skb, IFLA_CAN_DATA_BITTIMING_CONST,
+ sizeof(*priv->data_bittiming_const),
+ priv->data_bittiming_const)))
return -EMSGSIZE;
+
return 0;
}
diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
index dc5f902..8adaee9 100644
--- a/include/linux/can/dev.h
+++ b/include/linux/can/dev.h
@@ -33,8 +33,9 @@ enum can_mode {
struct can_priv {
struct can_device_stats can_stats;
- struct can_bittiming bittiming;
- const struct can_bittiming_const *bittiming_const;
+ struct can_bittiming bittiming, data_bittiming;
+ const struct can_bittiming_const *bittiming_const,
+ *data_bittiming_const;
struct can_clock clock;
enum can_state state;
@@ -45,6 +46,7 @@ struct can_priv {
struct timer_list restart_timer;
int (*do_set_bittiming)(struct net_device *dev);
+ int (*do_set_data_bittiming)(struct net_device *dev);
int (*do_set_mode)(struct net_device *dev, enum can_mode mode);
int (*do_get_state)(const struct net_device *dev,
enum can_state *state);
diff --git a/include/uapi/linux/can/netlink.h b/include/uapi/linux/can/netlink.h
index df944ed..b41933d 100644
--- a/include/uapi/linux/can/netlink.h
+++ b/include/uapi/linux/can/netlink.h
@@ -122,6 +122,8 @@ enum {
IFLA_CAN_RESTART_MS,
IFLA_CAN_RESTART,
IFLA_CAN_BERR_COUNTER,
+ IFLA_CAN_DATA_BITTIMING,
+ IFLA_CAN_DATA_BITTIMING_CONST,
__IFLA_CAN_MAX
};
--
1.9.0
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v7 6/7] can: allow to change the device mtu for CAN FD capable devices
2014-02-28 10:26 [PATCH v7 0/7] Add CAN FD infrastructure for CAN driver Oliver Hartkopp
` (4 preceding siblings ...)
2014-02-28 10:26 ` [PATCH v7 5/7] can: introduce the data bitrate configuration for CAN FD Oliver Hartkopp
@ 2014-02-28 10:26 ` Oliver Hartkopp
2014-02-28 10:26 ` [PATCH v7 7/7] can: add bittiming check at interface open for CAN FD Oliver Hartkopp
6 siblings, 0 replies; 12+ messages in thread
From: Oliver Hartkopp @ 2014-02-28 10:26 UTC (permalink / raw)
To: linux-can; +Cc: Oliver Hartkopp
The configuration for CAN FD depends on CAN_CTRLMODE_FD enabled in the driver
specific ctrlmode_supported capabilities.
The configuration can be done either with the 'fd { on | off }' option in the
'ip' tool from iproute2 or by setting the CAN netdevice MTU to CAN_MTU (16) or
to CANFD_MTU (72).
Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
---
drivers/net/can/dev.c | 40 ++++++++++++++++++++++++++++++++++++++++
include/linux/can/dev.h | 1 +
include/uapi/linux/can/netlink.h | 1 +
3 files changed, 42 insertions(+)
diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
index b2b0a9b..f2825b4 100644
--- a/drivers/net/can/dev.c
+++ b/drivers/net/can/dev.c
@@ -595,6 +595,40 @@ void free_candev(struct net_device *dev)
EXPORT_SYMBOL_GPL(free_candev);
/*
+ * changing MTU and control mode for CAN/CANFD devices
+ */
+int can_change_mtu(struct net_device *dev, int new_mtu)
+{
+ struct can_priv *priv = netdev_priv(dev);
+
+ /* Do not allow changing the MTU while running */
+ if (dev->flags & IFF_UP)
+ return -EBUSY;
+
+ /* allow change of MTU according to the CANFD ability of the device */
+ switch (new_mtu) {
+
+ case CAN_MTU:
+ priv->ctrlmode &= ~CAN_CTRLMODE_FD;
+ break;
+
+ case CANFD_MTU:
+ if (!(priv->ctrlmode_supported & CAN_CTRLMODE_FD))
+ return -EINVAL;
+
+ priv->ctrlmode |= CAN_CTRLMODE_FD;
+ break;
+
+ default:
+ return -EINVAL;
+ }
+
+ dev->mtu = new_mtu;
+ return 0;
+}
+EXPORT_SYMBOL_GPL(can_change_mtu);
+
+/*
* Common open function when the device gets opened.
*
* This function should be called in the open function of the device
@@ -714,6 +748,12 @@ static int can_changelink(struct net_device *dev,
return -EOPNOTSUPP;
priv->ctrlmode &= ~cm->mask;
priv->ctrlmode |= cm->flags;
+
+ /* CAN_CTRLMODE_FD can only be set when driver supports FD */
+ if (priv->ctrlmode & CAN_CTRLMODE_FD)
+ dev->mtu = CANFD_MTU;
+ else
+ dev->mtu = CAN_MTU;
}
if (data[IFLA_CAN_RESTART_MS]) {
diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
index 8adaee9..3ce5e52 100644
--- a/include/linux/can/dev.h
+++ b/include/linux/can/dev.h
@@ -113,6 +113,7 @@ struct can_priv *safe_candev_priv(struct net_device *dev);
int open_candev(struct net_device *dev);
void close_candev(struct net_device *dev);
+int can_change_mtu(struct net_device *dev, int new_mtu);
int register_candev(struct net_device *dev);
void unregister_candev(struct net_device *dev);
diff --git a/include/uapi/linux/can/netlink.h b/include/uapi/linux/can/netlink.h
index b41933d..7e2e186 100644
--- a/include/uapi/linux/can/netlink.h
+++ b/include/uapi/linux/can/netlink.h
@@ -96,6 +96,7 @@ struct can_ctrlmode {
#define CAN_CTRLMODE_3_SAMPLES 0x04 /* Triple sampling mode */
#define CAN_CTRLMODE_ONE_SHOT 0x08 /* One-Shot mode */
#define CAN_CTRLMODE_BERR_REPORTING 0x10 /* Bus-error reporting */
+#define CAN_CTRLMODE_FD 0x20 /* CAN FD mode */
/*
* CAN device statistics
--
1.9.0
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v7 7/7] can: add bittiming check at interface open for CAN FD
2014-02-28 10:26 [PATCH v7 0/7] Add CAN FD infrastructure for CAN driver Oliver Hartkopp
` (5 preceding siblings ...)
2014-02-28 10:26 ` [PATCH v7 6/7] can: allow to change the device mtu for CAN FD capable devices Oliver Hartkopp
@ 2014-02-28 10:26 ` Oliver Hartkopp
6 siblings, 0 replies; 12+ messages in thread
From: Oliver Hartkopp @ 2014-02-28 10:26 UTC (permalink / raw)
To: linux-can; +Cc: Oliver Hartkopp
Additionally to have the second (data) bitrate available the data bitrate
has to be greater or equal to the arbitration bitrate in CAN FD.
Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
---
drivers/net/can/dev.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
index f2825b4..05b5dc2 100644
--- a/drivers/net/can/dev.c
+++ b/drivers/net/can/dev.c
@@ -643,6 +643,14 @@ int open_candev(struct net_device *dev)
return -EINVAL;
}
+ /* For CAN FD the data bitrate has to be >= the arbitration bitrate */
+ if ((priv->ctrlmode & CAN_CTRLMODE_FD) &&
+ (!priv->data_bittiming.bitrate ||
+ (priv->data_bittiming.bitrate < priv->bittiming.bitrate))) {
+ netdev_err(dev, "incorrect/missing data bit-timing\n");
+ return -EINVAL;
+ }
+
/* Switch carrier on if device was stopped while in bus-off state */
if (!netif_carrier_ok(dev))
netif_carrier_on(dev);
--
1.9.0
^ permalink raw reply related [flat|nested] 12+ messages in thread