* Re: RFC: crash in fib6_clean_all() while loading ipv6 module
From: David Miller @ 2013-09-09 17:27 UTC (permalink / raw)
To: mkubecek; +Cc: netdev
In-Reply-To: <20130909100515.GA16056@unicorn.suse.cz>
From: Michal Kubecek <mkubecek@suse.cz>
Date: Mon, 9 Sep 2013 12:05:15 +0200
> This could be prevented by setting a flag when ip6_route_init() is
> complete and not calling fib6_run_gc() from ndisc_netdev_event() until
> the flag is set. However, I don't like the idea of adding a test which
> will be useful only in a short window while loading ipv6 module.
Please just initialize the parts of ipv6 in the correct order necessary
to prevent this problem.
I stronly dislike using flags when it's simply an initialization
ordering problem. It's just like registering a device interrupt
handle before the driver's data structures are properly setup.
^ permalink raw reply
* [PATCH 1/1] bridge: fix message_age_timer calculation
From: Chris Healy @ 2013-09-09 16:56 UTC (permalink / raw)
To: Stephen Hemminger, David S. Miller; +Cc: netdev, bridge, buytenh
This changes the message_age_timer calculation to use the BPDU's max age as opposed to the local bridge's max age. This is in accordance with section 8.6.2.3.2 Step 2 of the 802.1D-1998 sprecification.
With the current implementation, when running with very large bridge diameters, convergance will not always occur even if a root bridge is configured to have a longer max age.
Tested successfully on bridge diameters of ~200.
Signed-off-by: Chris Healy <cphealy@gmail.com>
---
net/bridge/br_stp.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/bridge/br_stp.c b/net/bridge/br_stp.c
index 1c0a50f..f1887ba 100644
--- a/net/bridge/br_stp.c
+++ b/net/bridge/br_stp.c
@@ -209,7 +209,7 @@ static void br_record_config_information(struct net_bridge_port *p,
p->designated_age = jiffies - bpdu->message_age;
mod_timer(&p->message_age_timer, jiffies
- + (p->br->max_age - bpdu->message_age));
+ + (bpdu->max_age - bpdu->message_age));
}
/* called under bridge lock */
--
1.8.1.2
^ permalink raw reply related
* Re: rtnl_lock deadlock on 3.10
From: Steve Wise @ 2013-09-09 16:48 UTC (permalink / raw)
To: Shawn Bohrer, roland-BHEL68pLQRGGvPXPguhicg
Cc: Bart Van Assche, Shawn Bohrer, Or Gerlitz, Cong Wang,
netdev-u79uwXL29TY76Z2rM5mHXA, linux-rdma-u79uwXL29TY76Z2rM5mHXA,
swise-ut6Up61K2wZBDgjK7y7TUQ
In-Reply-To: <20130906231901.GB10419-/vebjAlq/uFE7V8Yqttd03bhEEblAqRIDbRjUBewulXQT0dZR+AlfA@public.gmane.org>
On 9/6/2013 6:19 PM, Shawn Bohrer wrote:
> On Thu, Sep 05, 2013 at 10:14:51AM -0500, Steve Wise wrote:
>> Roland, what do you think?
>>
>> As I've said, I think we should go ahead with using the rtnl lock in
>> the core. Is there a complete patch available for review? looks
>> like the original was a partial fix.
> I guess I should realize that when no one jumps at fixing my issues
> for me that they probably aren't simple to fix. The solution that
> Cong proposed was to acquire rtnl_lock() before acquiring the
> infiniband device_mutex, and his partial patch did that in
> ib_register_client(). The problem is that you would also need to do
> that in ib_unregister_client(), ib_register_device(), and
> ib_unregister_device(), and that brings us back to the original
> problem which was that cxgb3 was holding the rtnl_lock() when it
> called ib_register_device(). Thus with the proposed fix I believe
> cxgb3 would already be holding the rtnl_lock() and then call
> ib_register_device() which would try to acquire the rtnl_lock() again
> and deadlock for a different reason.
>
> Actually how does this currently work? ib_register_device() calls
> client->add() for each client in the list which should call
> ipoib_add_one() which calls register_netdev(). Shouldn't that also
> deadlock in the cxgb3 case?
cxgb3 is an iWARP device and doesn't support IPoIB.
>
> Also while digging through this I think I see another bug which is
> that ipoib_dev_cleanup() can be called from ipoib_add_port() but in
> the current code ipoib_add_port() is not holding the rtnl_lock() which
> appears to be a requirement of ipoib_dev_cleanup().
>
> Sigh... I'm going to stop looking at this for now and hopefully
> someone can propose a better solution to this issue.
I can help with this, but I'm waiting for Roland to chime in.
Steve.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH net-next v4 1/6] bonding: simplify and use RCU protection for 3ad xmit path
From: Ding Tianhong @ 2013-09-09 14:53 UTC (permalink / raw)
To: Veaceslav Falico
Cc: Ding Tianhong, Nikolay Aleksandrov, David S. Miller, Netdev
In-Reply-To: <20130909095752.GC2048@redhat.com>
于 2013/9/9 17:57, Veaceslav Falico 写道:
> On Mon, Sep 09, 2013 at 04:58:29PM +0800, Ding Tianhong wrote:
>> On 2013/9/8 14:05, Ding Tianhong wrote:
>>
>> Hi Veaceslav and Nik:
>>
>> please take a moment to reveiw the function just modify for
>> bond_XXX_rcu,
>> and give me some advice. thanks for the help again.:)
>>
>> +#define bond_first_slave_rcu(bond) \
>> + list_first_or_null_rcu(&(bond)->slave_list, struct slave, list);
>> +#define bond_last_slave_rcu(bond) \
>> + ({struct list_head *__slave_list = &(bond)->slave_list; \
>> + struct list_head __rcu *__prev = \
>> + (*((struct list_head __rcu **)(&(__slave_list)->prev)));\
>> + likely(__slave_list != __prev) ? \
>> + container_of(__prev, struct slave, list) : NULL;})
>
> Please take a look at Nikolay's reply to my RCU email -
> http://www.spinics.net/lists/netdev/msg249805.html . And mine also, to
> his
> email. In short - RCU doesn't guarantee ->prev, so better take the
> approach
> of eliminating bond_last/prev_slave completely.
>
yes, I see the message, the list_del_rcu will make the slave->list
->prev = LIST_POISON2,
the bond->slave_list will not be set to the messae, the prev will point
a slave->list or itself,
so I think it will be ok here, please correct me if I miss something.
Best Regards
Ding
>> +
>> #define bond_is_first_slave(bond, pos) ((pos)->list.prev ==
>> &(bond)->slave_list)
>> #define bond_is_last_slave(bond, pos) ((pos)->list.next ==
>> &(bond)->slave_list)
>>
>> @@ -93,6 +117,29 @@
>> (bond_is_first_slave(bond, pos) ? bond_last_slave(bond) : \
>> bond_to_slave((pos)->list.prev))
>>
>> +/* Since bond_first/last_slave_rcu can return NULL, these can return
>> NULL too */
>> +#define bond_next_slave_rcu(bond, pos) \
>> + ({struct list_head *__slave_list = &(bond)->slave_list; \
>> + struct list_head __rcu *__next = list_next_rcu(__slave_list); \
>> + struct list_head *__pos_list = &(pos)->list; \
>> + struct list_head __rcu *__pos_next = list_next_rcu(__pos_list); \
>> + likely(__pos_next != __slave_list) ? \
>> + container_of(__pos_next, struct slave, list) : \
>> + container_of(__next, struct slave, list); \
>> + })
>
> Nice, but can be shortened - we know that pos won't go away.
OK, clean it soon.
>
>> +
>> +#define bond_prev_slave_rcu(bond, pos) \
>> + ({struct list_head *__slave_list = &(bond)->slave_list; \
>> + struct list_head __rcu *__prev = \
>> + (*((struct list_head __rcu **)(&(__slave_list)->prev)));\
>> + struct list_head *__pos_list = &(pos)->list; \
>> + struct list_head __rcu *__pos_prev = (__pos_list->prev
>> !=LIST_POISON2) ? \
yes, the pos->list will be set to LIST_POISON2 by list_del_rcu, so I add
a check for it, But
take the approach of eliminating bond_last/prev_slave completely is a
wise decision, I agree.
>> + (*((struct list_head __rcu **)(&(__pos_list)->prev))) : NULL; \
>> + likely(__pos_prev != __slave_list) ? \
>> + ((__pos_prev) ? list_entry_rcu(__pos_prev, struct slave, list) :
>> NULL;) : \
>> + (list_entry_rcu(__prev, struct slave, list)); \
>> + })
>
> Same remark as above about prev.
>
>> +
>>
>>
>> -#define bond_for_each_slave_from(bond, pos, cnt, start) \
>> - for (cnt = 0, pos = start; pos && cnt < (bond)->slave_cnt; \
>> - cnt++, pos = bond_next_slave(bond, pos))
>> -
>> +#define bond_for_each_slave_from(bond, pos, start) \
>> + for (pos = start; pos; (pos = bond_next_slave(bond, pos)) != start ? \
>> + (pos) : (pos = NULL))
>> +
>> +#define bond_for_each_slave_from_rcu(bond, pos, start) \
yes, it is a little tedious. I think it could be more easier and shorter.
>> + for ({struct list_head *__start = &(start)->list; \
>> + struct list_head *__slave_list = &(bond)->slave_list; \
>> + pos = list_entry_rcu(__start, struct slave, list);}; \
>> + pos; \
the only way to get out of the loop is that pos is NULL.
>> + {struct list_head __rcu *__next = list_next_rcu(pos->next); \
>> + __next != __slave_list ? \
>> + __next : __next = list_next_rcu(__next->next); \
first, check whether the pos->next is the last one in the slave_list, if
it does, get the
first slave of the bond->slave_list.
>>
>> + __next != __start ? \
>> + pos = list_entry_rcu(__next, struct slave, list) : \
>> + pos = NULL; \
second, check whether the pos is reach the start, if not, continue,
otherwise, the pos
will be set to NULL, so break the loop.
>> + })
>
> Jeez, I don't even want to review it. It's too complex and too hard to
> maintain, even if it works. Can you please make something
> shorter/easier to
> understand?
>
Best Regards.
Ding
>> +
>>
>> Best regards
>> Ding
>>
>>
>>>> --
>>>> To unsubscribe from this list: send the line "unsubsc ribe netdev" in
>>>> the body of a message to majordomo@vger.kernel.org
>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>>
>>>
>>>
>>> .
>>>
>>
>>
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply
* Re: [PATCH net] bnx2x: Fix configuration of doorbell block
From: Eric Dumazet @ 2013-09-09 14:17 UTC (permalink / raw)
To: Ariel Elior; +Cc: David Miller, netdev, Eilon Greenstein
In-Reply-To: <1378727487-6921-1-git-send-email-ariele@broadcom.com>
On Mon, 2013-09-09 at 14:51 +0300, Ariel Elior wrote:
> As part of VF RSS feature doorbell block was configured not to use dpm, but
> a small part of configuration was left out, preventing the driver from sending
> tx messages to the device. This patch adds the missing configuration.
>
> Reported-by: Eric Dumazet <eric.dumazet@gmil.com>
> Signed-off-by: Ariel Elior <ariele@broadcom.com>
> Signed-off-by: Eilon Greenstein <eilong@broadcom.com>
> ---
> drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c | 1 +
> drivers/net/ethernet/broadcom/bnx2x/bnx2x_sriov.c | 3 ---
> 2 files changed, 1 insertions(+), 3 deletions(-)
Thanks for fixing this.
Tested-by: Eric Dumazet <edumazet@google.com>
^ permalink raw reply
* Re: [PATCH net] net: sctp: fix bug in sctp_poll for SOCK_SELECT_ERR_QUEUE
From: Vlad Yasevich @ 2013-09-09 13:56 UTC (permalink / raw)
To: Daniel Borkmann; +Cc: davem, netdev, linux-sctp, Jacob Keller
In-Reply-To: <1378565099-20987-1-git-send-email-dborkman@redhat.com>
On 09/07/2013 10:44 AM, Daniel Borkmann wrote:
> If we do not add braces around ...
>
> mask |= POLLERR |
> sock_flag(sk, SOCK_SELECT_ERR_QUEUE) ? POLLPRI : 0;
>
> ... then this condition always evaluates to true as POLLERR is
> defined as 8 and binary or'd with whatever result comes out of
> sock_flag(). Hence instead of (X | Y) ? A : B, transform it into
> X | (Y ? A : B). Unfortunatelty, commit 8facd5fb73 ("net: fix
> smatch warnings inside datagram_poll") forgot about SCTP. :-(
>
> Introduced by 7d4c04fc170 ("net: add option to enable error queue
> packets waking select").
>
> Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
> Cc Jacob Keller <jacob.e.keller@intel.com>
Acked-by: Vlad Yasevich <vyasevich@gmail.com>
-vlad
> ---
> net/sctp/socket.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> index d5d5882..5462bbb 100644
> --- a/net/sctp/socket.c
> +++ b/net/sctp/socket.c
> @@ -6176,7 +6176,7 @@ unsigned int sctp_poll(struct file *file, struct socket *sock, poll_table *wait)
> /* Is there any exceptional events? */
> if (sk->sk_err || !skb_queue_empty(&sk->sk_error_queue))
> mask |= POLLERR |
> - sock_flag(sk, SOCK_SELECT_ERR_QUEUE) ? POLLPRI : 0;
> + (sock_flag(sk, SOCK_SELECT_ERR_QUEUE) ? POLLPRI : 0);
> if (sk->sk_shutdown & RCV_SHUTDOWN)
> mask |= POLLRDHUP | POLLIN | POLLRDNORM;
> if (sk->sk_shutdown == SHUTDOWN_MASK)
>
^ permalink raw reply
* Re: TSQ accounting skb->truesize degrades throughput for large packets
From: Eric Dumazet @ 2013-09-09 13:47 UTC (permalink / raw)
To: Jason Wang
Cc: Zoltan Kiss, Wei Liu, Jonathan Davies, Ian Campbell, netdev,
xen-devel, Michael S. Tsirkin
In-Reply-To: <522D9466.6020205@redhat.com>
On Mon, 2013-09-09 at 17:27 +0800, Jason Wang wrote:
> Virtio-net orphan the skb in .ndo_start_xmit() so TSQ can not throttle
> packets in device accurately, and it also can't do BQL. Does this means
> TSQ should be disabled for virtio-net?
>
If skb are orphaned, there is no way TSQ can work at all.
It is already disabled, so why do you want to disable it ?
^ permalink raw reply
* Re: [PATCH] bnx2x: avoid atomic allocations during initialization
From: Michal Schmidt @ 2013-09-09 12:20 UTC (permalink / raw)
To: Dmitry Kravkov
Cc: David Miller, netdev@vger.kernel.org, Ariel Elior,
Eilon Greenstein
In-Reply-To: <504C9EFCA2D0054393414C9CB605C37F20D90E3D@SJEXCHMB06.corp.ad.broadcom.com>
On 09/07/2013 03:45 AM, Dmitry Kravkov wrote:
> Once you allocated the memory during initialization , you will most
> probably fail to allocate its replacement during RX handling (on this
> machine).
Why do you think this would happen "most probably"? I would instead
expect the VM subsystem to respond to the memory pressure created by the
initial GFP_KERNEL allocations by freeing some memory to allow the
future atomic allocations to succeed.
Michal
^ permalink raw reply
* Re: [PATCH stable] ipv6: restrict neighbor entry creation to output flow
From: Jiri Pirko @ 2013-09-09 12:17 UTC (permalink / raw)
To: David Miller; +Cc: hannes, mleitner, netdev, dbanerje, yoshfuji
In-Reply-To: <20130815.155454.417440388188230172.davem@davemloft.net>
Fri, Aug 16, 2013 at 12:54:54AM CEST, davem@davemloft.net wrote:
>From: Hannes Frederic Sowa <hannes@stressinduktion.org>
>Date: Wed, 14 Aug 2013 17:00:54 +0200
>
>> On Wed, Aug 14, 2013 at 10:53:27AM -0300, Marcelo Ricardo Leitner wrote:
>>> This patch is based on 3.2.y branch, the one used by reported. Please let me
>>> know if it should be different. Thanks.
>>>
>>> ---8<---
>>>
>>> Commit 0d6a77079c475033cb622c07c5a880b392ef664e introduced a regression on
>>> which routes to local delivery would not work anymore. Like this:
>>>
>>> $ ip -6 route add local 2001::/64 dev lo
>>> $ ping6 -c1 2001::9
>>> PING 2001::9(2001::9) 56 data bytes
>>> ping: sendmsg: Invalid argument
>>>
>>> As this is a local delivery, that commit would not allow the creation of a
>>> neighbor entry and thus the packet cannot be sent.
>>>
>>> But as TPROXY scenario actually needs to avoid the neighbor entry creation only
>>> for input flow, this patch now limits previous patch to input flow, keeping
>>> output as before that patch.
>>>
>>> Reported-by: Debabrata Banerjee <dbavatar@gmail.com>
>>> Signed-off-by: Marcelo Ricardo Leitner <mleitner@redhat.com>
>>> CC: Hannes Frederic Sowa <hannes@stressinduktion.org>
>>
>> Looks good, thanks Marcelo!
>>
>> Acked-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
>>
>> David, this patch is for all stable kernels except the 3.10 series.
>> It does not apply cleanly throughout the whole longterm kernels but the
>> changes should not be too difficult to adapt. Do you take care of this
>> or can we do something to ease this process?
>
>I've queued it up for -stable, thanks.
Hi Dave.
When do you plan to push this to stable maintainers?
Thanks!
Jiri
^ permalink raw reply
* [PATCH net] bnx2x: Fix configuration of doorbell block
From: Ariel Elior @ 2013-09-09 11:51 UTC (permalink / raw)
To: David Miller; +Cc: netdev, Eilon Greenstein, Eric Dumazet, Ariel Elior
As part of VF RSS feature doorbell block was configured not to use dpm, but
a small part of configuration was left out, preventing the driver from sending
tx messages to the device. This patch adds the missing configuration.
Reported-by: Eric Dumazet <eric.dumazet@gmil.com>
Signed-off-by: Ariel Elior <ariele@broadcom.com>
Signed-off-by: Eilon Greenstein <eilong@broadcom.com>
---
drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c | 1 +
drivers/net/ethernet/broadcom/bnx2x/bnx2x_sriov.c | 3 ---
2 files changed, 1 insertions(+), 3 deletions(-)
diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
index 634a793..2f8dbbb 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
@@ -7645,6 +7645,7 @@ static int bnx2x_init_hw_func(struct bnx2x *bp)
bnx2x_init_block(bp, BLOCK_TM, init_phase);
bnx2x_init_block(bp, BLOCK_DORQ, init_phase);
+ REG_WR(bp, DORQ_REG_MODE_ACT, 1); /* no dpm */
bnx2x_iov_init_dq(bp);
diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_sriov.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_sriov.c
index b26eb83..2604b62 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_sriov.c
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_sriov.c
@@ -1756,9 +1756,6 @@ void bnx2x_iov_init_dq(struct bnx2x *bp)
REG_WR(bp, DORQ_REG_VF_TYPE_MIN_MCID_0, 0);
REG_WR(bp, DORQ_REG_VF_TYPE_MAX_MCID_0, 0x1ffff);
- /* set the number of VF allowed doorbells to the full DQ range */
- REG_WR(bp, DORQ_REG_VF_NORM_MAX_CID_COUNT, 0x20000);
-
/* set the VF doorbell threshold */
REG_WR(bp, DORQ_REG_VF_USAGE_CT_LIMIT, 4);
}
--
1.7.1
^ permalink raw reply related
* Re: [PATCH 09/16] c_can: expicit 32bit access on D_CAN to message buffer data register
From: Marc Kleine-Budde @ 2013-09-09 11:20 UTC (permalink / raw)
To: Benedikt Spranger
Cc: netdev, Alexander Frank, Sebastian Andrzej Siewior,
Holger Dengler, linux-can@vger.kernel.org
In-Reply-To: <1378711513-2548-10-git-send-email-b.spranger@linutronix.de>
[-- Attachment #1: Type: text/plain, Size: 2170 bytes --]
On 09/09/2013 09:25 AM, Benedikt Spranger wrote:
> change the 16bit access of ARB1_REG and DATA1/2_REG to a 32bit access.
>
> Signed-off-by: Benedikt Spranger <b.spranger@linutronix.de>
> ---
> drivers/net/can/c_can/c_can.c | 19 ++++++++++---------
> 1 file changed, 10 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c
> index 4b94f2d..c573399 100644
> --- a/drivers/net/can/c_can/c_can.c
> +++ b/drivers/net/can/c_can/c_can.c
> @@ -360,7 +360,6 @@ static inline void c_can_object_put(struct net_device *dev,
> static void c_can_write_msg_object(struct net_device *dev,
> int iface, struct can_frame *frame, int objno)
> {
> - int i;
> u32 flags = IF_ARB_MSGVAL;
> unsigned int id;
> struct c_can_priv *priv = netdev_priv(dev);
> @@ -376,15 +375,17 @@ static void c_can_write_msg_object(struct net_device *dev,
>
> id |= flags;
>
> - priv->write_reg(priv, C_CAN_IFACE(ARB1_REG, iface),
> - IFX_WRITE_LOW_16BIT(id));
> - priv->write_reg(priv, C_CAN_IFACE(ARB2_REG, iface),
> - IFX_WRITE_HIGH_16BIT(id));
> + c_can_writereg32(priv, C_CAN_IFACE(ARB1_REG, iface),
> + IFX_WRITE_HIGH_16BIT(id),
> + IFX_WRITE_LOW_16BIT(id));
> +
> + c_can_writereg32(priv, C_CAN_IFACE(DATA1_REG, iface),
> + frame->data[2] | frame->data[3] << 8,
> + frame->data[0] | frame->data[1] << 8);
You can use something like beXX_to_cpup((__be32 *)&cf->data[0]) here.
>
> - for (i = 0; i < frame->can_dlc; i += 2) {
> - priv->write_reg(priv, C_CAN_IFACE(DATA1_REG, iface) + i / 2,
> - frame->data[i] | (frame->data[i + 1] << 8));
> - }
> + c_can_writereg32(priv, C_CAN_IFACE(DATA3_REG, iface),
> + frame->data[6] | frame->data[7] << 8,
> + frame->data[4] | frame->data[5] << 8);
You write here the upper 32 bit unconditionally, the original code doesn't.
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: 259 bytes --]
^ permalink raw reply
* Re: [PATCH 04/16] c_can: fix receive buffer configuration
From: Marc Kleine-Budde @ 2013-09-09 10:51 UTC (permalink / raw)
To: Benedikt Spranger
Cc: netdev, Alexander Frank, Sebastian Andrzej Siewior,
Holger Dengler, linux-can@vger.kernel.org
In-Reply-To: <1378711513-2548-5-git-send-email-b.spranger@linutronix.de>
[-- Attachment #1: Type: text/plain, Size: 2132 bytes --]
On 09/09/2013 09:25 AM, Benedikt Spranger wrote:
> The former implementation lacks some initialization of receive buffer.
> First the maximal buffer length was 0. The Buffer direction was not set
> and the mask register a reserverd flag was cleared.
>
> Signed-off-by: Benedikt Spranger <b.spranger@linutronix.de>
> ---
> drivers/net/can/c_can/c_can.c | 19 ++++++++++++++-----
> 1 file changed, 14 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c
> index bdb7bcd..886163f 100644
> --- a/drivers/net/can/c_can/c_can.c
> +++ b/drivers/net/can/c_can/c_can.c
> @@ -116,6 +116,11 @@
> IF_COMM_CONTROL | IF_COMM_TXRQST | \
> IF_COMM_DATAA | IF_COMM_DATAB)
>
> +/* IFx mask */
> +#define IF_MASK_MXTD BIT(31)
> +#define IF_MASK_MDIR BIT(30)
> +#define IF_MASK_RES BIT(29)
> +
> /* IFx arbitration */
> #define IF_ARB_MSGVAL BIT(31)
> #define IF_ARB_MSGXTD BIT(30)
> @@ -653,11 +658,15 @@ static void c_can_configure_msg_objects(struct net_device *dev)
>
> /* setup receive message objects */
> for (i = C_CAN_MSG_OBJ_RX_FIRST; i < C_CAN_MSG_OBJ_RX_LAST; i++)
> - c_can_setup_receive_object(dev, 0, i, 0, 0,
> - (IF_MCONT_RXIE | IF_MCONT_UMASK) & ~IF_MCONT_EOB);
> -
> - c_can_setup_receive_object(dev, 0, C_CAN_MSG_OBJ_RX_LAST, 0, 0,
> - IF_MCONT_EOB | IF_MCONT_RXIE | IF_MCONT_UMASK);
> + c_can_setup_receive_object(dev, 0, i,
> + IF_MASK_MDIR | IF_MASK_RES, 0,
> + IF_MCONT_UMASK | IF_MCONT_RXIE |
> + IF_MCONT_DLC_MAX);
DLC_MAX should be added in this patch. not in 3.
> +
> + c_can_setup_receive_object(dev, 0, C_CAN_MSG_OBJ_RX_LAST,
> + IF_MASK_MDIR | IF_MASK_RES, 0,
> + IF_MCONT_UMASK | IF_MCONT_EOB |
> + IF_MCONT_RXIE | IF_MCONT_DLC_MAX);
> }
>
> /*
>
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions | Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]
^ permalink raw reply
* Re: [PATCH 12/16] c_can: Add flag to disable automatic retransmission of CAN frames
From: Marc Kleine-Budde @ 2013-09-09 10:34 UTC (permalink / raw)
To: Benedikt Spranger
Cc: netdev, Alexander Frank, Sebastian Andrzej Siewior,
Holger Dengler, linux-can@vger.kernel.org
In-Reply-To: <522DA145.9090507@pengutronix.de>
[-- Attachment #1: Type: text/plain, Size: 718 bytes --]
On 09/09/2013 12:21 PM, Marc Kleine-Budde wrote:
> On 09/09/2013 09:25 AM, Benedikt Spranger wrote:
>> The C_CAN/D_CAN controler can automatic retransmit CAN after arbitration loss.
>> Add a flag to CAN ctrlmode set the appropiate behaivior of this feature.
>
> We already have this flag, it's called CAN_CTRLMODE_ONE_SHOT.
BTW: Have a look at commit:
ee6f098 can: c_can: disable one shot mode until driver is fixed
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: 259 bytes --]
^ permalink raw reply
* Re: [PATCH 16/16] c_can: fix TX packet accounting
From: Marc Kleine-Budde @ 2013-09-09 10:31 UTC (permalink / raw)
To: Benedikt Spranger
Cc: netdev, Alexander Frank, Sebastian Andrzej Siewior,
Holger Dengler, linux-can@vger.kernel.org
In-Reply-To: <1378711513-2548-17-git-send-email-b.spranger@linutronix.de>
[-- Attachment #1: Type: text/plain, Size: 1980 bytes --]
On 09/09/2013 09:25 AM, Benedikt Spranger wrote:
> Due to the integration of the FlexCard support the TX stats accounting got
> lost. Readd the accounting.
Please squash into the appropriate patch.
Marc
> Signed-off-by: Benedikt Spranger <b.spranger@linutronix.de>
> ---
> drivers/net/can/c_can/c_can.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c
> index 92c1444..b409cf2 100644
> --- a/drivers/net/can/c_can/c_can.c
> +++ b/drivers/net/can/c_can/c_can.c
> @@ -619,6 +619,7 @@ static netdev_tx_t c_can_start_xmit(struct sk_buff *skb,
> u32 msg_obj_no;
> struct c_can_priv *priv = netdev_priv(dev);
> struct can_frame *frame = (struct can_frame *)skb->data;
> + struct net_device_stats *stats = &dev->stats;
> int tx_fifo, txie_off;
>
> if (can_dropped_invalid_skb(dev, skb))
> @@ -664,6 +665,9 @@ static netdev_tx_t c_can_start_xmit(struct sk_buff *skb,
> ctrl = readl(priv->base + FC_TXFIFO_CTRL);
> ctrl |= FC_TXFIFO_CTRL_REQ;
> writel(ctrl, priv->base + FC_TXFIFO_CTRL);
> +
> + stats->tx_bytes += frame->can_dlc;
> + stats->tx_packets++;
> kfree_skb(skb);
> } else {
> msg_obj_no = get_tx_next_msg_obj(priv);
> @@ -672,6 +676,8 @@ static netdev_tx_t c_can_start_xmit(struct sk_buff *skb,
> c_can_write_msg_object(dev, 0, frame, msg_obj_no, txie_off);
> priv->tx_next++;
>
> + stats->tx_bytes += frame->can_dlc;
> + stats->tx_packets++;
> can_put_echo_skb(skb, dev, msg_obj_no - C_CAN_MSG_OBJ_TX_FIRST);
> /* we have to stop the queue in case of a wrap around or
> * if the next TX message object is still in use
>
--
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: 259 bytes --]
^ permalink raw reply
* Re: [PATCH 15/16] flexcard: can: Configure CAN loopback packages (TXACK)
From: Marc Kleine-Budde @ 2013-09-09 10:29 UTC (permalink / raw)
To: Benedikt Spranger
Cc: netdev, Alexander Frank, Sebastian Andrzej Siewior,
Holger Dengler, linux-can@vger.kernel.org
In-Reply-To: <1378711513-2548-16-git-send-email-b.spranger@linutronix.de>
[-- Attachment #1: Type: text/plain, Size: 3416 bytes --]
On 09/09/2013 09:25 AM, Benedikt Spranger wrote:
> Signed-off-by: Holger Dengler <dengler@linutronix.de>
I'm missing your S-o-b and a patch description. I'm finding some
uncommon use of frame->can_dlc.
Marc
> ---
> drivers/net/can/c_can/c_can.c | 23 ++++++++++++++++-------
> 1 file changed, 16 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c
> index 670a6b1..92c1444 100644
> --- a/drivers/net/can/c_can/c_can.c
> +++ b/drivers/net/can/c_can/c_can.c
> @@ -358,11 +358,13 @@ static inline void c_can_object_put(struct net_device *dev,
> }
>
> static void c_can_write_msg_object(struct net_device *dev,
> - int iface, struct can_frame *frame, int objno)
> + int iface, struct can_frame *frame, int objno,
> + int txie_off)
> {
> u32 flags = IF_ARB_MSGVAL;
> unsigned int id;
> struct c_can_priv *priv = netdev_priv(dev);
> + u32 int_en;
>
> if (!(frame->can_id & CAN_RTR_FLAG))
> flags |= IF_ARB_TRANSMIT;
> @@ -388,9 +390,10 @@ static void c_can_write_msg_object(struct net_device *dev,
> frame->data[4] | frame->data[5] << 8);
>
> /* enable interrupt for this message object */
> - priv->write_reg(priv, C_CAN_IFACE(MSGCTRL_REG, iface),
> - IF_MCONT_TXIE | IF_MCONT_TXRQST | IF_MCONT_EOB |
> - frame->can_dlc);
> + int_en = frame->can_dlc;
> + if (!txie_off)
> + int_en |= IF_MCONT_TXIE | IF_MCONT_TXRQST | IF_MCONT_EOB;
> + priv->write_reg(priv, C_CAN_IFACE(MSGCTRL_REG, iface), int_en);
> c_can_object_put(dev, iface, objno, IF_COMM_ALL);
> }
>
> @@ -616,16 +619,17 @@ static netdev_tx_t c_can_start_xmit(struct sk_buff *skb,
> u32 msg_obj_no;
> struct c_can_priv *priv = netdev_priv(dev);
> struct can_frame *frame = (struct can_frame *)skb->data;
> - int tx_fifo;
> + int tx_fifo, txie_off;
>
> if (can_dropped_invalid_skb(dev, skb))
> return NETDEV_TX_OK;
>
> tx_fifo = frame->can_dlc & FC_TXFIFO_FLAG;
> + txie_off = frame->can_dlc & FC_TXACKOFF_FLAG;
> frame->can_dlc &= FC_TXFIFO_DLC_MASK;
>
> if (tx_fifo) {
> - u32 id, *data, ctrl;
> + u32 id, *data, ctrl, conf;
>
> if (readl(priv->base + FC_TXFIFO_STAT) &
> FC_TXFIFO_STAT_FULL) {
> @@ -652,6 +656,11 @@ static netdev_tx_t c_can_start_xmit(struct sk_buff *skb,
> writel(data[1], priv->base + FC_TXFIFO_MSGDB);
> }
>
> + conf = readl(priv->base + FC_TXFIFO_CONF);
> + conf &= ~FC_TXFIFO_CONF_TXACK;
> + conf |= (txie_off ? 0 : FC_TXFIFO_CONF_TXACK);
> + writel(conf, priv->base + FC_TXFIFO_CONF);
> +
> ctrl = readl(priv->base + FC_TXFIFO_CTRL);
> ctrl |= FC_TXFIFO_CTRL_REQ;
> writel(ctrl, priv->base + FC_TXFIFO_CTRL);
> @@ -660,7 +669,7 @@ static netdev_tx_t c_can_start_xmit(struct sk_buff *skb,
> msg_obj_no = get_tx_next_msg_obj(priv);
>
> /* prepare message object for transmission */
> - c_can_write_msg_object(dev, 0, frame, msg_obj_no);
> + c_can_write_msg_object(dev, 0, frame, msg_obj_no, txie_off);
> priv->tx_next++;
>
> can_put_echo_skb(skb, dev, msg_obj_no - C_CAN_MSG_OBJ_TX_FIRST);
>
--
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: 259 bytes --]
^ permalink raw reply
* Re: [PATCH 13/16] c_can: flexcard: add ioctl to reset FIFO message object
From: Marc Kleine-Budde @ 2013-09-09 10:24 UTC (permalink / raw)
To: Benedikt Spranger
Cc: netdev, Alexander Frank, Sebastian Andrzej Siewior,
Holger Dengler, linux-can@vger.kernel.org
In-Reply-To: <1378711513-2548-14-git-send-email-b.spranger@linutronix.de>
[-- Attachment #1: Type: text/plain, Size: 1638 bytes --]
On 09/09/2013 09:25 AM, Benedikt Spranger wrote:
> The FlexCard FIFO reset implementation did not reset the FIFO message object.
> Since the FIFO configuration is done from userspace a function to reset the
> FIFO message object is needed. Add an ioctl to reset the FIFO message object
> from userspace.
Can you elaborate this a bit.
> Signed-off-by: Benedikt Spranger <b.spranger@linutronix.de>
>
> ---
> drivers/net/can/c_can/c_can.c | 14 ++++++++++++++
> 1 file changed, 14 insertions(+)
>
> diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c
> index 51ca2a6..670a6b1 100644
> --- a/drivers/net/can/c_can/c_can.c
> +++ b/drivers/net/can/c_can/c_can.c
> @@ -1447,7 +1447,21 @@ void free_c_can_dev(struct net_device *dev)
> }
> EXPORT_SYMBOL_GPL(free_c_can_dev);
>
> +static int c_can_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
> +{
> + switch (cmd) {
> + case SIOCDEVPRIVATE:
> + c_can_inval_msg_object(dev, 0, FC_TXFIFO_MO);
> + break;
What happens if you call this ioctl on non flexcard HW?
> + default:
> + return -ENOIOCTLCMD;
> + }
> +
> + return 0;
> +}
> +
> static const struct net_device_ops c_can_netdev_ops = {
> + .ndo_do_ioctl = c_can_ioctl,
> .ndo_open = c_can_open,
> .ndo_stop = c_can_close,
> .ndo_start_xmit = c_can_start_xmit,
>
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: 259 bytes --]
^ permalink raw reply
* Re: [PATCH 12/16] c_can: Add flag to disable automatic retransmission of CAN frames
From: Marc Kleine-Budde @ 2013-09-09 10:21 UTC (permalink / raw)
To: Benedikt Spranger
Cc: netdev, Alexander Frank, Sebastian Andrzej Siewior,
Holger Dengler, linux-can@vger.kernel.org
In-Reply-To: <1378711513-2548-13-git-send-email-b.spranger@linutronix.de>
[-- Attachment #1: Type: text/plain, Size: 3408 bytes --]
On 09/09/2013 09:25 AM, Benedikt Spranger wrote:
> The C_CAN/D_CAN controler can automatic retransmit CAN after arbitration loss.
> Add a flag to CAN ctrlmode set the appropiate behaivior of this feature.
We already have this flag, it's called CAN_CTRLMODE_ONE_SHOT.
Marc
> Signed-off-by: Benedikt Spranger <b.spranger@linutronix.de>
> ---
> drivers/net/can/c_can/c_can.c | 16 +++++++++-------
> include/uapi/linux/can/netlink.h | 2 ++
> 2 files changed, 11 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c
> index 02f7c89..51ca2a6 100644
> --- a/drivers/net/can/c_can/c_can.c
> +++ b/drivers/net/can/c_can/c_can.c
> @@ -785,33 +785,35 @@ static void c_can_configure_msg_objects(struct net_device *dev, int invalidate)
> static int c_can_chip_config(struct net_device *dev)
> {
> struct c_can_priv *priv = netdev_priv(dev);
> + u16 reg;
> int ret;
>
> - /* enable automatic retransmission */
> - priv->write_reg(priv, C_CAN_CTRL_REG,
> - CONTROL_ENABLE_AR);
> + if (priv->can.ctrlmode & CAN_CTRLMODE_DAR)
> + reg = CONTROL_DISABLE_AR;
> + else
> + reg = CONTROL_ENABLE_AR;
>
> if ((priv->can.ctrlmode & CAN_CTRLMODE_LISTENONLY) &&
> (priv->can.ctrlmode & CAN_CTRLMODE_LOOPBACK)) {
> /* loopback + silent mode : useful for hot self-test */
> priv->write_reg(priv, C_CAN_CTRL_REG, CONTROL_EIE |
> - CONTROL_SIE | CONTROL_IE | CONTROL_TEST);
> + reg | CONTROL_SIE | CONTROL_IE | CONTROL_TEST);
> priv->write_reg(priv, C_CAN_TEST_REG,
> TEST_LBACK | TEST_SILENT);
> } else if (priv->can.ctrlmode & CAN_CTRLMODE_LOOPBACK) {
> /* loopback mode : useful for self-test function */
> priv->write_reg(priv, C_CAN_CTRL_REG, CONTROL_EIE |
> - CONTROL_SIE | CONTROL_IE | CONTROL_TEST);
> + reg | CONTROL_SIE | CONTROL_IE | CONTROL_TEST);
> priv->write_reg(priv, C_CAN_TEST_REG, TEST_LBACK);
> } else if (priv->can.ctrlmode & CAN_CTRLMODE_LISTENONLY) {
> /* silent mode : bus-monitoring mode */
> priv->write_reg(priv, C_CAN_CTRL_REG, CONTROL_EIE |
> - CONTROL_SIE | CONTROL_IE | CONTROL_TEST);
> + reg | CONTROL_SIE | CONTROL_IE | CONTROL_TEST);
> priv->write_reg(priv, C_CAN_TEST_REG, TEST_SILENT);
> } else
> /* normal mode*/
> priv->write_reg(priv, C_CAN_CTRL_REG,
> - CONTROL_EIE | CONTROL_SIE | CONTROL_IE);
> + reg | CONTROL_EIE | CONTROL_SIE | CONTROL_IE);
>
> /* configure message objects */
> c_can_configure_msg_objects(dev, 1);
> diff --git a/include/uapi/linux/can/netlink.h b/include/uapi/linux/can/netlink.h
> index 14966dd..fdce5af 100644
> --- a/include/uapi/linux/can/netlink.h
> +++ b/include/uapi/linux/can/netlink.h
> @@ -88,6 +88,8 @@ 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_DAR 0x20 /* Disable Automatic
> + * Retransmission */
>
> /*
> * CAN device statistics
>
--
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: 259 bytes --]
^ permalink raw reply
* Re: [PATCH 11/16] c_can: stop netqueue if hardware is busy
From: Marc Kleine-Budde @ 2013-09-09 10:21 UTC (permalink / raw)
To: Benedikt Spranger
Cc: netdev, Alexander Frank, Sebastian Andrzej Siewior,
Holger Dengler, linux-can@vger.kernel.org
In-Reply-To: <1378711513-2548-12-git-send-email-b.spranger@linutronix.de>
[-- Attachment #1: Type: text/plain, Size: 796 bytes --]
On 09/09/2013 09:25 AM, Benedikt Spranger wrote:
> Unlike other network devices the FlexCard do not support interrupts on TX.
> Emulate the TX interrupt by polling the MOTRX registers and restart the
> netqueue if one or more message object is available for transtition of CAN
> frames.
>
> Signed-off-by: Benedikt Spranger <b.spranger@linutronix.de>
It's not onlynetif_wake_queue() that you have to call on TX-complete.
Have a look at c_can_do_tx(), better call it, or trigger napi from your
timer.
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: 259 bytes --]
^ permalink raw reply
* Re: [PATCH 11/16] c_can: stop netqueue if hardware is busy
From: Marc Kleine-Budde @ 2013-09-09 10:05 UTC (permalink / raw)
To: Benedikt Spranger
Cc: netdev, Alexander Frank, Sebastian Andrzej Siewior,
Holger Dengler, linux-can@vger.kernel.org
In-Reply-To: <1378711513-2548-12-git-send-email-b.spranger@linutronix.de>
[-- Attachment #1: Type: text/plain, Size: 4985 bytes --]
On 09/09/2013 09:25 AM, Benedikt Spranger wrote:
> Unlike other network devices the FlexCard do not support interrupts on TX.
> Emulate the TX interrupt by polling the MOTRX registers and restart the
> netqueue if one or more message object is available for transtition of CAN
> frames.
>
> Signed-off-by: Benedikt Spranger <b.spranger@linutronix.de>
> ---
> drivers/net/can/c_can/c_can.c | 61 ++++++++++++++++++++++++++++++++++++++++---
> drivers/net/can/c_can/c_can.h | 4 +++
> 2 files changed, 62 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c
> index 46d741d..02f7c89 100644
> --- a/drivers/net/can/c_can/c_can.c
> +++ b/drivers/net/can/c_can/c_can.c
> @@ -550,18 +550,66 @@ static void c_can_inval_msg_object(struct net_device *dev, int iface, int objno)
>
> static inline int c_can_is_next_tx_obj_busy(struct c_can_priv *priv, int objno)
> {
> - int val = c_can_read_reg32(priv, C_CAN_TXRQST1_REG);
> + int val = c_can_read_reg32(priv, C_CAN_TXRQST1_REG + (objno / 32) * 4);
>
> /*
> * as transmission request register's bit n-1 corresponds to
> * message object n, we need to handle the same properly.
> */
> - if (val & (1 << (objno - 1)))
> + if (val & (1 << ((objno % 32) - 1)))
> return 1;
>
> return 0;
> }
>
> +static void c_can_motrx_add_timer(struct net_device *dev)
> +{
> + struct c_can_priv *priv = netdev_priv(dev);
> +
> + priv->timer.expires = round_jiffies_relative(1);
> + add_timer(&priv->timer);
> +}
> +
> +static void c_can_motrx_poll(unsigned long data)
> +{
> + struct net_device *dev = (struct net_device *)data;
> + struct c_can_priv *priv = netdev_priv(dev);
> + u32 val, empty = 0;
> + int i;
> +
> + for (i = 0; i < C_CAN_MOTRX_NR; i++) {
> + val = c_can_read_reg32(priv, C_CAN_TXRQST1_REG + i * 4);
> +
> + if (val < priv->motrx[i]) {
> + netif_wake_queue(dev);
> + return;
> + }
> +
> + empty |= val;
> + }
> +
> + if (!empty) {
> + netif_wake_queue(dev);
> + return;
> + }
> +
> + c_can_motrx_add_timer(dev);
> +}
> +
> +static void c_can_motrx_monitor(struct net_device *dev)
> +{
> + struct c_can_priv *priv = netdev_priv(dev);
> + int i;
> +
> + if (priv->type != BOSCH_D_CAN_FLEXCARD)
> + return;
> +
> + for (i = 0; i < C_CAN_MOTRX_NR; i++)
> + priv->motrx[i] = c_can_read_reg32(priv,
> + C_CAN_TXRQST1_REG + i * 4);
> + c_can_motrx_add_timer(dev);
> +}
> +
> static netdev_tx_t c_can_start_xmit(struct sk_buff *skb,
> struct net_device *dev)
> {
> @@ -582,6 +630,7 @@ static netdev_tx_t c_can_start_xmit(struct sk_buff *skb,
> if (readl(priv->base + FC_TXFIFO_STAT) &
> FC_TXFIFO_STAT_FULL) {
> netif_stop_queue(dev);
> + c_can_motrx_monitor(dev);
> return NETDEV_TX_BUSY;
> }
>
> @@ -619,8 +668,10 @@ static netdev_tx_t c_can_start_xmit(struct sk_buff *skb,
> * if the next TX message object is still in use
> */
> if (c_can_is_next_tx_obj_busy(priv, get_tx_next_msg_obj(priv))
> - || ((priv->tx_next & C_CAN_NEXT_MSG_OBJ_MASK) == 0))
> + || ((priv->tx_next & C_CAN_NEXT_MSG_OBJ_MASK) == 0)) {
> netif_stop_queue(dev);
> + c_can_motrx_monitor(dev);
> + }
> }
>
> return NETDEV_TX_OK;
> @@ -1272,6 +1323,7 @@ static int c_can_close(struct net_device *dev)
> {
> struct c_can_priv *priv = netdev_priv(dev);
>
> + del_timer_sync(&priv->timer);
> netif_stop_queue(dev);
> napi_disable(&priv->napi);
> c_can_stop(dev);
> @@ -1306,6 +1358,9 @@ struct net_device *alloc_c_can_dev(void)
> CAN_CTRLMODE_LISTENONLY |
> CAN_CTRLMODE_BERR_REPORTING;
> spin_lock_init(&priv->lock);
> + priv->timer.function = c_can_motrx_poll;
> + priv->timer.data = (unsigned long) dev;
> + init_timer_deferrable(&priv->timer);
>
> return dev;
> }
> diff --git a/drivers/net/can/c_can/c_can.h b/drivers/net/can/c_can/c_can.h
> index beea437..6d0f805 100644
> --- a/drivers/net/can/c_can/c_can.h
> +++ b/drivers/net/can/c_can/c_can.h
> @@ -153,6 +153,8 @@ enum c_can_dev_id {
> BOSCH_D_CAN_FLEXCARD,
> };
>
> +#define C_CAN_MOTRX_NR 1
Are there reasons to have C_CAN_MOTRX_NR != 1? If not, please remove the
array and make code more clean.
> /* c_can private data structure */
> struct c_can_priv {
> struct can_priv can; /* must be the first member */
> @@ -176,6 +178,8 @@ struct c_can_priv {
> unsigned int instance;
> void (*raminit) (const struct c_can_priv *priv, bool enable);
> spinlock_t lock;
> + struct timer_list timer;
> + u32 motrx[C_CAN_MOTRX_NR];
> };
>
> struct net_device *alloc_c_can_dev(void);
>
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: 259 bytes --]
^ permalink raw reply
* RFC: crash in fib6_clean_all() while loading ipv6 module
From: Michal Kubecek @ 2013-09-09 10:05 UTC (permalink / raw)
To: netdev
Hello,
Two of our customers encountered a crash in fib6_clean_all() when
booting their system:
[ 12.408421] BUG: unable to handle kernel NULL pointer dereference at (null)
[ 12.408424] IP: [<ffffffffa02566b4>] fib6_clean_all+0x34/0xd0 [ipv6_lib]
[ 12.408434] PGD c3590f067 PUD c29073067 PMD 0
[ 12.408436] Oops: 0000 [#1] SMP
[ 12.408439] CPU 1
[ 12.408440] Modules linked in: processor(+) ipv6(+) thermal_sys ipv6_lib
ahci(+) ixgbe(+) ehci_hcd libahci hwmon igb(+) libata usbcore i2c_i801 dca
iTCO_wdt pcspkr scsi_mod e1000e i2c_core usb_common iTCO_vendor_support
rtc_cmos container ptp pps_core button mdio
[ 12.408449] Supported: Yes
[ 12.408449]
[ 12.408451] Pid: 211, comm: modprobe Not tainted 3.0.76-0.11-default #1 ...
[ 12.408453] RIP: 0010:[<ffffffffa02566b4>] [<ffffffffa02566b4>] fib6_clean_all+0x34/0xd0 [ipv6_lib]
[ 12.408460] RSP: 0018:ffff880c1e983b98 EFLAGS: 00010246
[ 12.408462] RAX: 0000000000000000 RBX: ffffffffffffffff RCX: 0000000000000000
[ 12.408463] RDX: 0000000000000000 RSI: ffffffffa0254f90 RDI: ffffffff81a72280
[ 12.408464] RBP: ffffffff81a72280 R08: 0000000000000000 R09: 000000003520aec8
[ 12.408466] R10: 000000005f000c28 R11: 000000003520aec8 R12: 0000000000000000
[ 12.408467] R13: ffffffff81a72280 R14: 0000000000000000 R15: ffffffffa0254f90
[ 12.408469] FS: 00007fb2039f1700(0000) GS:ffff880c7f040000(0000) knlGS:0000000000000000
[ 12.408470] CS: 0010 DS: 0000 ES: 0000 CR0: 000000008005003b
[ 12.408471] CR2: 0000000000000000 CR3: 0000000c2904e000 CR4: 00000000001407e0
[ 12.408473] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[ 12.408474] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
[ 12.408476] Process modprobe (pid: 211, threadinfo ffff880c1e982000, task ffff880c3242c540)
[ 12.408477] Stack:
[ 12.408478] 0000000000000000 0000000000000000 0000000000000000 ffffffff8146544e
[ 12.408480] 000000000006a9d0 0000000000000000 000000003520aec8 000000005f000c28
[ 12.408483] 000000003520aec8 0000000000000000 0000000000000000 0000000000000000
[ 12.408485] Call Trace:
[ 12.408507] [<ffffffffa02567a3>] fib6_run_gc+0x53/0xf0 [ipv6_lib]
[ 12.408524] [<ffffffffa025c5c6>] ndisc_netdev_event+0x186/0x190 [ipv6_lib]
[ 12.408541] [<ffffffff81460437>] notifier_call_chain+0x37/0x70
[ 12.408547] [<ffffffff813a6e52>] dev_addr_add+0x52/0x90
[ 12.408556] [<ffffffffa032e3e9>] ixgbe_probe+0x8c9/0xd10 [ixgbe]
[ 12.408565] [<ffffffff8127d5d4>] local_pci_probe+0x54/0xe0
[ 12.408570] [<ffffffff8127d740>] __pci_device_probe+0xe0/0xf0
[ 12.408573] [<ffffffff8127e953>] pci_device_probe+0x33/0x60
[ 12.408578] [<ffffffff8132ddda>] really_probe+0x7a/0x260
[ 12.408581] [<ffffffff8132e023>] driver_probe_device+0x63/0xc0
[ 12.408585] [<ffffffff8132e113>] __driver_attach+0x93/0xa0
[ 12.408588] [<ffffffff8132d468>] bus_for_each_dev+0x88/0xb0
[ 12.408591] [<ffffffff8132cbb5>] bus_add_driver+0x155/0x2a0
[ 12.408595] [<ffffffff8132e769>] driver_register+0x79/0x170
[ 12.408599] [<ffffffff8127ebe8>] __pci_register_driver+0x58/0xe0
[ 12.408604] [<ffffffff810001cb>] do_one_initcall+0x3b/0x180
[ 12.408610] [<ffffffff810a02df>] sys_init_module+0xcf/0x240
[ 12.408615] [<ffffffff81464592>] system_call_fastpath+0x16/0x1b
The crash is caused by fib6_clean_all() dereferencing a null pointer in
init_net.ipv6.fib_table_hash because ndisc_netdev_event() handler which
calls fib6_run_gc() is registered by ndisc_init() but the relevant data
structures aren't initialized until ip6_route_init() is called. If a
device intialization falls into this window, it emits NETDEV_CHANGEADDR,
leading to a crash.
This could be prevented by setting a flag when ip6_route_init() is
complete and not calling fib6_run_gc() from ndisc_netdev_event() until
the flag is set. However, I don't like the idea of adding a test which
will be useful only in a short window while loading ipv6 module.
The only other actions ndisc_netdev_event() does are flushing the neigh
table (which should be empty at the moment) and calling
ndisc_send_unsol_na() (which should do nothing as there is no IPv6
address assigned yet). Thus I believe taking the netdev event handler
registration call out of ndisc_init() into a separate function, say
ndisc_late_init(), which would be called after ip6_route_init() should
be a safe and efficient way to avoid the race condition.
Before I submit the patch, I wanted to ask if someone can see a problem
with this solution (or a better solution).
Michal Kubecek
^ permalink raw reply
* Re: [PATCH net-next v4 1/6] bonding: simplify and use RCU protection for 3ad xmit path
From: Veaceslav Falico @ 2013-09-09 9:57 UTC (permalink / raw)
To: Ding Tianhong; +Cc: Nikolay Aleksandrov, David S. Miller, Netdev
In-Reply-To: <522D8DB5.1030302@huawei.com>
On Mon, Sep 09, 2013 at 04:58:29PM +0800, Ding Tianhong wrote:
>On 2013/9/8 14:05, Ding Tianhong wrote:
>
>Hi Veaceslav and Nik:
>
>please take a moment to reveiw the function just modify for bond_XXX_rcu,
>and give me some advice. thanks for the help again.:)
>
>+#define bond_first_slave_rcu(bond) \
>+ list_first_or_null_rcu(&(bond)->slave_list, struct slave, list);
>+#define bond_last_slave_rcu(bond) \
>+ ({struct list_head *__slave_list = &(bond)->slave_list; \
>+ struct list_head __rcu *__prev = \
>+ (*((struct list_head __rcu **)(&(__slave_list)->prev)));\
>+ likely(__slave_list != __prev) ? \
>+ container_of(__prev, struct slave, list) : NULL;})
Please take a look at Nikolay's reply to my RCU email -
http://www.spinics.net/lists/netdev/msg249805.html . And mine also, to his
email. In short - RCU doesn't guarantee ->prev, so better take the approach
of eliminating bond_last/prev_slave completely.
>+
> #define bond_is_first_slave(bond, pos) ((pos)->list.prev == &(bond)->slave_list)
> #define bond_is_last_slave(bond, pos) ((pos)->list.next == &(bond)->slave_list)
>
>@@ -93,6 +117,29 @@
> (bond_is_first_slave(bond, pos) ? bond_last_slave(bond) : \
> bond_to_slave((pos)->list.prev))
>
>+/* Since bond_first/last_slave_rcu can return NULL, these can return NULL too */
>+#define bond_next_slave_rcu(bond, pos) \
>+ ({struct list_head *__slave_list = &(bond)->slave_list; \
>+ struct list_head __rcu *__next = list_next_rcu(__slave_list); \
>+ struct list_head *__pos_list = &(pos)->list; \
>+ struct list_head __rcu *__pos_next = list_next_rcu(__pos_list); \
>+ likely(__pos_next != __slave_list) ? \
>+ container_of(__pos_next, struct slave, list) : \
>+ container_of(__next, struct slave, list); \
>+ })
Nice, but can be shortened - we know that pos won't go away.
>+
>+#define bond_prev_slave_rcu(bond, pos) \
>+ ({struct list_head *__slave_list = &(bond)->slave_list; \
>+ struct list_head __rcu *__prev = \
>+ (*((struct list_head __rcu **)(&(__slave_list)->prev)));\
>+ struct list_head *__pos_list = &(pos)->list; \
>+ struct list_head __rcu *__pos_prev = (__pos_list->prev != LIST_POISON2) ? \
>+ (*((struct list_head __rcu **)(&(__pos_list)->prev))) : NULL; \
>+ likely(__pos_prev != __slave_list) ? \
>+ ((__pos_prev) ? list_entry_rcu(__pos_prev, struct slave, list) : NULL;) : \
>+ (list_entry_rcu(__prev, struct slave, list)); \
>+ })
Same remark as above about prev.
>+
>
>
>-#define bond_for_each_slave_from(bond, pos, cnt, start) \
>- for (cnt = 0, pos = start; pos && cnt < (bond)->slave_cnt; \
>- cnt++, pos = bond_next_slave(bond, pos))
>-
>+#define bond_for_each_slave_from(bond, pos, start) \
>+ for (pos = start; pos; (pos = bond_next_slave(bond, pos)) != start ? \
>+ (pos) : (pos = NULL))
>+
>+#define bond_for_each_slave_from_rcu(bond, pos, start) \
>+ for ({struct list_head *__start = &(start)->list; \
>+ struct list_head *__slave_list = &(bond)->slave_list; \
>+ pos = list_entry_rcu(__start, struct slave, list);}; \
>+ pos; \
>+ {struct list_head __rcu *__next = list_next_rcu(pos->next); \
>+ __next != __slave_list ? \
>+ __next : __next = list_next_rcu(__next->next); \
>+ __next != __start ? \
>+ pos = list_entry_rcu(__next, struct slave, list) : \
>+ pos = NULL; \
>+ })
Jeez, I don't even want to review it. It's too complex and too hard to
maintain, even if it works. Can you please make something shorter/easier to
understand?
>+
>
>Best regards
>Ding
>
>
>>> --
>>> To unsubscribe from this list: send the line "unsubsc ribe netdev" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>
>>
>>
>> .
>>
>
>
^ permalink raw reply
* Re: [PATCH 10/16] c_can: add 16bit align 32bit access functions
From: Marc Kleine-Budde @ 2013-09-09 9:57 UTC (permalink / raw)
To: Benedikt Spranger
Cc: netdev, Alexander Frank, Sebastian Andrzej Siewior,
Holger Dengler, linux-can@vger.kernel.org
In-Reply-To: <1378711513-2548-11-git-send-email-b.spranger@linutronix.de>
[-- Attachment #1: Type: text/plain, Size: 3941 bytes --]
On 09/09/2013 09:25 AM, Benedikt Spranger wrote:
> The FlexCard only allows 32bit access to DCAN registers, otherwise the
> register value can be wraped up. Add a new helper function to access
> registers 16bit aligned but 32bit access.
You are modifying code you have previously added. You should shuffle
your patches that this is not necessary.
> Signed-off-by: Benedikt Spranger <b.spranger@linutronix.de>
> ---
> drivers/net/can/c_can/c_can.c | 1 +
> drivers/net/can/c_can/c_can.h | 1 +
> drivers/net/can/c_can/c_can_platform.c | 38 ++++++++++++++++++++++++++++++++--
> 3 files changed, 38 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c
> index c573399..46d741d 100644
> --- a/drivers/net/can/c_can/c_can.c
> +++ b/drivers/net/can/c_can/c_can.c
> @@ -1305,6 +1305,7 @@ struct net_device *alloc_c_can_dev(void)
> priv->can.ctrlmode_supported = CAN_CTRLMODE_LOOPBACK |
> CAN_CTRLMODE_LISTENONLY |
> CAN_CTRLMODE_BERR_REPORTING;
> + spin_lock_init(&priv->lock);
>
> return dev;
> }
> diff --git a/drivers/net/can/c_can/c_can.h b/drivers/net/can/c_can/c_can.h
> index 9f0eda8..beea437 100644
> --- a/drivers/net/can/c_can/c_can.h
> +++ b/drivers/net/can/c_can/c_can.h
> @@ -175,6 +175,7 @@ struct c_can_priv {
> u32 __iomem *raminit_ctrlreg;
> unsigned int instance;
> void (*raminit) (const struct c_can_priv *priv, bool enable);
> + spinlock_t lock;
> };
>
> struct net_device *alloc_c_can_dev(void);
> diff --git a/drivers/net/can/c_can/c_can_platform.c b/drivers/net/can/c_can/c_can_platform.c
> index 43a3e3f..c6c1eb4 100644
> --- a/drivers/net/can/c_can/c_can_platform.c
> +++ b/drivers/net/can/c_can/c_can_platform.c
> @@ -58,6 +58,40 @@ static void c_can_plat_write_reg_aligned_to_16bit(struct c_can_priv *priv,
> writew(val, priv->base + priv->regs[index]);
> }
>
> +#define ALIGN_DOWN(p, a) ((typeof(p))round_down((unsigned long)(p), (a)))
> +
> +static u16 c_can_plat_read_16bit_align_32bit_access(struct c_can_priv *priv,
> + enum reg index)
> +{
> + void *addr = priv->base + priv->regs[index];
Please compile with C=2. Should be void __iomem *.
> + u32 reg;
> + reg = readl(ALIGN_DOWN(addr, 4));
> +
> + return IS_ALIGNED((unsigned long)addr, 4) ?
> + reg & 0xffff :
> + reg >> 16;
> +}
> +
> +static void c_can_plat_write_16bit_align_32bit_access(struct c_can_priv *priv,
> + enum reg index, u16 val)
> +{
> + unsigned long flags;
> + void *addr = priv->base + priv->regs[index];
dito
> + u32 reg;
> +
> + spin_lock_irqsave(&priv->lock, flags);
> + reg = readl(ALIGN_DOWN(addr, 4));
> + if (IS_ALIGNED((unsigned long)addr, 4)) {
> + reg &= 0xffff0000;
> + reg |= val;
> + } else {
> + reg &= 0xffff;
> + reg |= val << 16;
> + }
> + writel(reg, ALIGN_DOWN(addr, 4));
> + spin_unlock_irqrestore(&priv->lock, flags);
> +}
> +
> static u16 c_can_plat_read_reg_aligned_to_32bit(struct c_can_priv *priv,
> enum reg index)
> {
> @@ -317,8 +351,8 @@ static int c_can_plat_probe(struct platform_device *pdev)
> case BOSCH_D_CAN_FLEXCARD:
> priv->regs = reg_map_d_can;
> priv->can.ctrlmode_supported |= CAN_CTRLMODE_3_SAMPLES;
> - priv->read_reg = c_can_plat_read_reg_aligned_to_16bit;
> - priv->write_reg = c_can_plat_write_reg_aligned_to_16bit;
> + priv->read_reg = c_can_plat_read_16bit_align_32bit_access;
> + priv->write_reg = c_can_plat_write_16bit_align_32bit_access;
> priv->instance = pdev->id;
>
> res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
>
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: 259 bytes --]
^ permalink raw reply
* Re: [PATCH 08/16] c_can: Add FlexCard CAN TX fifo support
From: Marc Kleine-Budde @ 2013-09-09 9:47 UTC (permalink / raw)
To: Benedikt Spranger
Cc: netdev, Alexander Frank, Sebastian Andrzej Siewior,
Holger Dengler, linux-can@vger.kernel.org
In-Reply-To: <1378711513-2548-9-git-send-email-b.spranger@linutronix.de>
[-- Attachment #1: Type: text/plain, Size: 5416 bytes --]
On 09/09/2013 09:25 AM, Benedikt Spranger wrote:
> The FlexCard DCAN implementation contains a specialized TX fifo function.
> Add the TX support for this function.
This patch looks a bit fishy. Is this compatible with existing
c_can/d_can hardware. It looks like you "use" frame->dlc to transport
some information.
> Signed-off-by: Benedikt Spranger <b.spranger@linutronix.de>
> ---
> drivers/net/can/c_can/c_can.c | 77 ++++++++++++++++++++++++++++++++++++-------
> 1 file changed, 65 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c
> index 39e2bb0..4b94f2d 100644
> --- a/drivers/net/can/c_can/c_can.c
> +++ b/drivers/net/can/c_can/c_can.c
> @@ -41,6 +41,7 @@
> #include <linux/can/error.h>
> #include <linux/can/led.h>
>
> +#include <linux/flexcard.h>
> #include "c_can.h"
>
> /* Number of interface registers */
> @@ -566,24 +567,60 @@ static netdev_tx_t c_can_start_xmit(struct sk_buff *skb,
> u32 msg_obj_no;
> struct c_can_priv *priv = netdev_priv(dev);
> struct can_frame *frame = (struct can_frame *)skb->data;
> + int tx_fifo;
>
> if (can_dropped_invalid_skb(dev, skb))
> return NETDEV_TX_OK;
>
> - msg_obj_no = get_tx_next_msg_obj(priv);
> + tx_fifo = frame->can_dlc & FC_TXFIFO_FLAG;
Can you describe what you have encoded into to frame->can_dlc?
> + frame->can_dlc &= FC_TXFIFO_DLC_MASK;
>
> - /* prepare message object for transmission */
> - c_can_write_msg_object(dev, 0, frame, msg_obj_no);
> - can_put_echo_skb(skb, dev, msg_obj_no - C_CAN_MSG_OBJ_TX_FIRST);
> + if (tx_fifo) {
> + u32 id, *data, ctrl;
>
> - /*
> - * we have to stop the queue in case of a wrap around or
> - * if the next TX message object is still in use
> - */
> - priv->tx_next++;
> - if (c_can_is_next_tx_obj_busy(priv, get_tx_next_msg_obj(priv)) ||
> - (priv->tx_next & C_CAN_NEXT_MSG_OBJ_MASK) == 0)
> - netif_stop_queue(dev);
> + if (readl(priv->base + FC_TXFIFO_STAT) &
> + FC_TXFIFO_STAT_FULL) {
> + netif_stop_queue(dev);
> + return NETDEV_TX_BUSY;
> + }
> +
> + if (frame->can_id & CAN_EFF_FLAG) {
> + id = frame->can_id & CAN_EFF_MASK;
> + id |= FC_TXFIFO_MSGID_EXT;
> + } else {
> + id = frame->can_id & CAN_SFF_MASK;
> + /* StdID is left alligned */
> + id <<= FC_TXFIFO_MSGID_STDID_SHIFT;
> + }
> +
> + writel(id, priv->base + FC_TXFIFO_MSGID);
> + writel(frame->can_dlc, priv->base + FC_TXFIFO_MSGCTRL);
> +
> + if (frame->can_dlc) {
> + data = (u32 *) frame->data;
> + writel(data[0], priv->base + FC_TXFIFO_MSGDA);
> + writel(data[1], priv->base + FC_TXFIFO_MSGDB);
> + }
> +
> + ctrl = readl(priv->base + FC_TXFIFO_CTRL);
> + ctrl |= FC_TXFIFO_CTRL_REQ;
> + writel(ctrl, priv->base + FC_TXFIFO_CTRL);
> + kfree_skb(skb);
> + } else {
> + msg_obj_no = get_tx_next_msg_obj(priv);
> +
> + /* prepare message object for transmission */
> + c_can_write_msg_object(dev, 0, frame, msg_obj_no);
> + priv->tx_next++;
> +
> + can_put_echo_skb(skb, dev, msg_obj_no - C_CAN_MSG_OBJ_TX_FIRST);
> + /* we have to stop the queue in case of a wrap around or
> + * if the next TX message object is still in use
> + */
> + if (c_can_is_next_tx_obj_busy(priv, get_tx_next_msg_obj(priv))
> + || ((priv->tx_next & C_CAN_NEXT_MSG_OBJ_MASK) == 0))
> + netif_stop_queue(dev);
> + }
>
> return NETDEV_TX_OK;
> }
> @@ -683,6 +720,8 @@ static void c_can_configure_msg_objects(struct net_device *dev, int invalidate)
> IF_MASK_MDIR | IF_MASK_RES, 0,
> IF_MCONT_UMASK | IF_MCONT_EOB |
> IF_MCONT_RXIE | IF_MCONT_DLC_MAX);
> +
> + c_can_inval_msg_object(dev, 0, FC_TXFIFO_MO);
> }
>
> /*
> @@ -740,8 +779,13 @@ static int c_can_chip_config(struct net_device *dev)
> static int c_can_start(struct net_device *dev)
> {
> struct c_can_priv *priv = netdev_priv(dev);
> + u32 conf;
> int ret;
>
> + conf = readl(priv->base + FC_TXFIFO_CONF);
> + conf |= FC_TXFIFO_CONF_EN;
> + writel(conf, priv->base + FC_TXFIFO_CONF);
> +
> /* basic c_can configuration */
> ret = c_can_chip_config(dev);
> if (ret)
> @@ -762,6 +806,11 @@ out:
> static void c_can_stop(struct net_device *dev)
> {
> struct c_can_priv *priv = netdev_priv(dev);
> + u32 conf;
> +
> + conf = readl(priv->base + FC_TXFIFO_CONF);
> + conf &= ~FC_TXFIFO_CONF_EN;
> + writel(conf, priv->base + FC_TXFIFO_CONF);
>
> /* disable all interrupts */
> c_can_enable_all_interrupts(priv, DISABLE_ALL_INTERRUPTS);
> @@ -1350,6 +1399,8 @@ int register_c_can_dev(struct net_device *dev)
> struct c_can_priv *priv = netdev_priv(dev);
> int err;
>
> + writel(0, priv->base + FC_TXFIFO_CONF);
> +
> c_can_pm_runtime_enable(priv);
>
> dev->flags |= IFF_ECHO; /* we support local echo */
> @@ -1369,6 +1420,8 @@ void unregister_c_can_dev(struct net_device *dev)
> {
> struct c_can_priv *priv = netdev_priv(dev);
>
> + writel(0, priv->base + FC_TXFIFO_CONF);
> +
> unregister_candev(dev);
>
> c_can_pm_runtime_disable(priv);
>
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: 259 bytes --]
^ permalink raw reply
* Re: [PATCH 06/16] c_can: consider set bittiming may fail
From: Marc Kleine-Budde @ 2013-09-09 9:39 UTC (permalink / raw)
To: Benedikt Spranger
Cc: netdev, Alexander Frank, Sebastian Andrzej Siewior,
Holger Dengler
In-Reply-To: <1378711513-2548-7-git-send-email-b.spranger@linutronix.de>
[-- Attachment #1: Type: text/plain, Size: 2682 bytes --]
On 09/09/2013 09:25 AM, Benedikt Spranger wrote:
> While setting the bittiming the C_CAN/D_CAN may fail. Do not bring up the
> CAN interface in this case.
>
> Signed-off-by: Benedikt Spranger <b.spranger@linutronix.de>
> ---
> drivers/net/can/c_can/c_can.c | 25 ++++++++++++++++++++-----
> 1 file changed, 20 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c
> index b3cfb85..3fa5347 100644
> --- a/drivers/net/can/c_can/c_can.c
> +++ b/drivers/net/can/c_can/c_can.c
> @@ -689,9 +689,10 @@ static void c_can_configure_msg_objects(struct net_device *dev)
> * - set operating mode
> * - configure message objects
> */
> -static void c_can_chip_config(struct net_device *dev)
> +static int c_can_chip_config(struct net_device *dev)
> {
> struct c_can_priv *priv = netdev_priv(dev);
> + int ret;
>
> /* enable automatic retransmission */
> priv->write_reg(priv, C_CAN_CTRL_REG,
> @@ -726,15 +727,20 @@ static void c_can_chip_config(struct net_device *dev)
> priv->write_reg(priv, C_CAN_STS_REG, LEC_UNUSED);
>
> /* set bittiming params */
> - c_can_set_bittiming(dev);
> + ret = c_can_set_bittiming(dev);
> +
> + return ret;
Maybe just:
return c_can_set_bittiming(dev);
> }
>
> -static void c_can_start(struct net_device *dev)
> +static int c_can_start(struct net_device *dev)
> {
> struct c_can_priv *priv = netdev_priv(dev);
> + int ret;
>
> /* basic c_can configuration */
> - c_can_chip_config(dev);
> + ret = c_can_chip_config(dev);
> + if (ret)
> + goto out;
No need for a goto here, IMHO.
>
> priv->can.state = CAN_STATE_ERROR_ACTIVE;
>
> @@ -743,6 +749,9 @@ static void c_can_start(struct net_device *dev)
>
> /* enable status change, error and module interrupts */
> c_can_enable_all_interrupts(priv, ENABLE_ALL_INTERRUPTS);
> +
> +out:
> + return ret;
> }
>
> static void c_can_stop(struct net_device *dev)
> @@ -758,9 +767,15 @@ static void c_can_stop(struct net_device *dev)
>
> static int c_can_set_mode(struct net_device *dev, enum can_mode mode)
> {
> + int ret;
> +
> switch (mode) {
> case CAN_MODE_START:
> - c_can_start(dev);
> + ret = c_can_start(dev);
> + if (ret) {
> + c_can_stop(dev);
> + return ret;
> + }
> netif_wake_queue(dev);
> break;
> default:
>
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: 259 bytes --]
^ permalink raw reply
* Re: [PATCH 05/16] c_can: use 32 bit access for D_CAN
From: Marc Kleine-Budde @ 2013-09-09 9:37 UTC (permalink / raw)
To: Benedikt Spranger
Cc: netdev, Alexander Frank, Sebastian Andrzej Siewior,
Holger Dengler, linux-can@vger.kernel.org
In-Reply-To: <1378711513-2548-6-git-send-email-b.spranger@linutronix.de>
[-- Attachment #1: Type: text/plain, Size: 5047 bytes --]
On 09/09/2013 09:25 AM, Benedikt Spranger wrote:
> Other than the C_CAN 16 bit memory interface the D_CAN uses a 32bit memory
> interface. This causes some trouble while accessing the IFxCMR register in
> two 16bit chunks. Use 32bit access on D_CAN.
>
> Signed-off-by: Benedikt Spranger <b.spranger@linutronix.de>
> ---
> drivers/net/can/c_can/c_can.c | 56 +++++++++++++++++++++++++++----------------
> 1 file changed, 35 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c
> index 886163f..b3cfb85 100644
> --- a/drivers/net/can/c_can/c_can.c
> +++ b/drivers/net/can/c_can/c_can.c
> @@ -262,11 +262,32 @@ static inline int get_tx_echo_msg_obj(const struct c_can_priv *priv)
>
> static u32 c_can_read_reg32(struct c_can_priv *priv, enum reg index)
What about introducing priv->{read,write}_reg32 function pointers?
> {
> - u32 val = priv->read_reg(priv, index);
> - val |= ((u32) priv->read_reg(priv, index + 1)) << 16;
> + u32 val;
> +
> + if (priv->type == BOSCH_D_CAN || priv->type == BOSCH_D_CAN_FLEXCARD) {
> + val = readl(priv->base + priv->regs[index]);
> + } else {
> + val = priv->read_reg(priv, index);
> + val |= ((u32) priv->read_reg(priv, index + 1)) << 16;
> + }
> return val;
> }
>
> +static void c_can_writereg32(struct c_can_priv *priv, enum reg index,
> + u16 high, u16 low)
> +{
If we decide not to introduce read/write 32 bit function pointers,
please rename your function to c_can_write_reg32() to match the pattern
of the read_reg32() function.
As you have converted some of the potential users of write_reg32() to
work with a single 32 bit value instead of two 16 bits, I think it's
better to call this function with a single 32 bite value.
> + u32 val;
> +
> + if (priv->type == BOSCH_D_CAN || priv->type == BOSCH_D_CAN_FLEXCARD) {
> + val = high << 16;
> + val |= low;
> + writel(val, priv->base + priv->regs[index]);
> + } else {
> + priv->write_reg(priv, index + 1, high);
> + priv->write_reg(priv, index, low);
> + }
> +}
> +
> static void c_can_enable_all_interrupts(struct c_can_priv *priv,
> int enable)
> {
> @@ -309,10 +330,8 @@ static inline void c_can_object_get(struct net_device *dev,
> * register and message RAM must be complete in 6 CAN-CLK
> * period.
> */
> - priv->write_reg(priv, C_CAN_IFACE(COMMSK_REG, iface),
> - IFX_WRITE_LOW_16BIT(mask));
> - priv->write_reg(priv, C_CAN_IFACE(COMREQ_REG, iface),
> - IFX_WRITE_LOW_16BIT(objno));
> + c_can_writereg32(priv, C_CAN_IFACE(COMREQ_REG, iface),
> + IFX_WRITE_LOW_16BIT(mask), IFX_WRITE_LOW_16BIT(objno));
>
> if (c_can_msg_obj_is_busy(priv, iface))
> netdev_err(dev, "timed out in object get\n");
> @@ -329,9 +348,8 @@ static inline void c_can_object_put(struct net_device *dev,
> * register and message RAM must be complete in 6 CAN-CLK
> * period.
> */
> - priv->write_reg(priv, C_CAN_IFACE(COMMSK_REG, iface),
> - (IF_COMM_WR | IFX_WRITE_LOW_16BIT(mask)));
> - priv->write_reg(priv, C_CAN_IFACE(COMREQ_REG, iface),
> + c_can_writereg32(priv, C_CAN_IFACE(COMREQ_REG, iface),
> + IF_COMM_WR | IFX_WRITE_LOW_16BIT(mask),
> IFX_WRITE_LOW_16BIT(objno));
>
> if (c_can_msg_obj_is_busy(priv, iface))
> @@ -496,23 +514,19 @@ static void c_can_setup_receive_object(struct net_device *dev, int iface,
> {
> struct c_can_priv *priv = netdev_priv(dev);
>
> - priv->write_reg(priv, C_CAN_IFACE(MASK1_REG, iface),
> - IFX_WRITE_LOW_16BIT(mask));
> -
> /* According to C_CAN documentation, the reserved bit
> * in IFx_MASK2 register is fixed 1
> */
> - priv->write_reg(priv, C_CAN_IFACE(MASK2_REG, iface),
> - IFX_WRITE_HIGH_16BIT(mask) | BIT(13));
> -
> + c_can_writereg32(priv, C_CAN_IFACE(MASK1_REG, iface),
> + IFX_WRITE_HIGH_16BIT(mask) | BIT(13),
> + IFX_WRITE_LOW_16BIT(mask));
> id |= IF_ARB_MSGVAL;
> - priv->write_reg(priv, C_CAN_IFACE(ARB1_REG, iface),
> - IFX_WRITE_LOW_16BIT(id));
> - priv->write_reg(priv, C_CAN_IFACE(ARB2_REG, iface),
> - (IFX_WRITE_HIGH_16BIT(id)));
> + c_can_writereg32(priv, C_CAN_IFACE(ARB1_REG, iface),
> + IFX_WRITE_HIGH_16BIT(id), IFX_WRITE_LOW_16BIT(id));
> + c_can_writereg32(priv, C_CAN_IFACE(MSGCTRL_REG, iface), 0, mcont);
>
> - priv->write_reg(priv, C_CAN_IFACE(MSGCTRL_REG, iface), mcont);
> - c_can_object_put(dev, iface, objno, IF_COMM_ALL & ~IF_COMM_TXRQST);
> + c_can_object_put(dev, iface, objno, IF_COMM_WR | IF_COMM_MASK |
> + IF_COMM_ARB | IF_COMM_CONTROL | IF_COMM_CLR_INT_PND);
I think the last change is unrelated.
>
> netdev_dbg(dev, "obj no:%d, msgval:0x%08x\n", objno,
> c_can_read_reg32(priv, C_CAN_MSGVAL1_REG));
>
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: 259 bytes --]
^ 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