* PATCH net-next 1/2] ipv4: introduce __ip_dev_find()
From: Eric Dumazet @ 2010-09-30 13:31 UTC (permalink / raw)
To: David Miller; +Cc: netdev
ip_dev_find(net, addr) finds a device given an IPv4 source address and
takes a reference on it.
Introduce __ip_dev_find(), taking a third argument, to optionally take
the device reference. Callers not asking the reference to be taken
should be in an rcu_read_lock() protected section.
Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
include/linux/inetdevice.h | 7 ++++++-
net/ipv4/fib_frontend.c | 32 +++++++++++++++++++-------------
2 files changed, 25 insertions(+), 14 deletions(-)
diff --git a/include/linux/inetdevice.h b/include/linux/inetdevice.h
index 1ec09bb..ccd5b07 100644
--- a/include/linux/inetdevice.h
+++ b/include/linux/inetdevice.h
@@ -159,7 +159,12 @@ struct in_ifaddr {
extern int register_inetaddr_notifier(struct notifier_block *nb);
extern int unregister_inetaddr_notifier(struct notifier_block *nb);
-extern struct net_device *ip_dev_find(struct net *net, __be32 addr);
+extern struct net_device *__ip_dev_find(struct net *net, __be32 addr, bool devref);
+static inline struct net_device *ip_dev_find(struct net *net, __be32 addr)
+{
+ return __ip_dev_find(net, addr, true);
+}
+
extern int inet_addr_onlink(struct in_device *in_dev, __be32 a, __be32 b);
extern int devinet_ioctl(struct net *net, unsigned int cmd, void __user *);
extern void devinet_init(void);
diff --git a/net/ipv4/fib_frontend.c b/net/ipv4/fib_frontend.c
index 981f3c5..4a69a95 100644
--- a/net/ipv4/fib_frontend.c
+++ b/net/ipv4/fib_frontend.c
@@ -147,34 +147,40 @@ static void fib_flush(struct net *net)
rt_cache_flush(net, -1);
}
-/*
- * Find the first device with a given source address.
+/**
+ * __ip_dev_find - find the first device with a given source address.
+ * @net: the net namespace
+ * @addr: the source address
+ * @devref: if true, take a reference on the found device
+ *
+ * If a caller uses devref=false, it should be protected by RCU
*/
-
-struct net_device * ip_dev_find(struct net *net, __be32 addr)
+struct net_device *__ip_dev_find(struct net *net, __be32 addr, bool devref)
{
- struct flowi fl = { .nl_u = { .ip4_u = { .daddr = addr } },
- .flags = FLOWI_FLAG_MATCH_ANY_IIF };
- struct fib_result res;
+ struct flowi fl = {
+ .nl_u = {
+ .ip4_u = {
+ .daddr = addr
+ }
+ },
+ .flags = FLOWI_FLAG_MATCH_ANY_IIF
+ };
+ struct fib_result res = { 0 };
struct net_device *dev = NULL;
-#ifdef CONFIG_IP_MULTIPLE_TABLES
- res.r = NULL;
-#endif
-
if (fib_lookup(net, &fl, &res))
return NULL;
if (res.type != RTN_LOCAL)
goto out;
dev = FIB_RES_DEV(res);
- if (dev)
+ if (dev && devref)
dev_hold(dev);
out:
fib_res_put(&res);
return dev;
}
-EXPORT_SYMBOL(ip_dev_find);
+EXPORT_SYMBOL(__ip_dev_find);
/*
* Find address type as if only "dev" was present in the system. If
^ permalink raw reply related
* pull request: wireless-2.6 2010-09-29
From: John W. Linville @ 2010-09-29 20:33 UTC (permalink / raw)
To: davem; +Cc: linux-wireless, netdev, linux-kernel
Dave,
Here are two more fixes intended for 2.6.36. One fixes a user after
free error, the other fixes a reported regression (bug 17722). Both are
reasonably small and well documented in the commit logs.
Please let me know if there are problems!
Thanks,
John
---
The following changes since commit 01db403cf99f739f86903314a489fb420e0e254f:
tcp: Fix >4GB writes on 64-bit. (2010-09-27 20:24:54 -0700)
are available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/linville/wireless-2.6.git master
Florian Mickler (1):
iwl3945: queue the right work if the scan needs to be aborted
Johannes Berg (1):
mac80211: fix use-after-free
drivers/net/wireless/iwlwifi/iwl-agn-lib.c | 2 +-
drivers/net/wireless/iwlwifi/iwl3945-base.c | 2 +-
net/mac80211/rx.c | 4 ----
3 files changed, 2 insertions(+), 6 deletions(-)
diff --git a/drivers/net/wireless/iwlwifi/iwl-agn-lib.c b/drivers/net/wireless/iwlwifi/iwl-agn-lib.c
index 9dd9e64..8fd00a6 100644
--- a/drivers/net/wireless/iwlwifi/iwl-agn-lib.c
+++ b/drivers/net/wireless/iwlwifi/iwl-agn-lib.c
@@ -1411,7 +1411,7 @@ void iwlagn_request_scan(struct iwl_priv *priv, struct ieee80211_vif *vif)
clear_bit(STATUS_SCAN_HW, &priv->status);
clear_bit(STATUS_SCANNING, &priv->status);
/* inform mac80211 scan aborted */
- queue_work(priv->workqueue, &priv->scan_completed);
+ queue_work(priv->workqueue, &priv->abort_scan);
}
int iwlagn_manage_ibss_station(struct iwl_priv *priv,
diff --git a/drivers/net/wireless/iwlwifi/iwl3945-base.c b/drivers/net/wireless/iwlwifi/iwl3945-base.c
index 59a308b..d31661c 100644
--- a/drivers/net/wireless/iwlwifi/iwl3945-base.c
+++ b/drivers/net/wireless/iwlwifi/iwl3945-base.c
@@ -3018,7 +3018,7 @@ void iwl3945_request_scan(struct iwl_priv *priv, struct ieee80211_vif *vif)
clear_bit(STATUS_SCANNING, &priv->status);
/* inform mac80211 scan aborted */
- queue_work(priv->workqueue, &priv->scan_completed);
+ queue_work(priv->workqueue, &priv->abort_scan);
}
static void iwl3945_bg_restart(struct work_struct *data)
diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
index fa0f37e..2862428 100644
--- a/net/mac80211/rx.c
+++ b/net/mac80211/rx.c
@@ -2199,9 +2199,6 @@ static void ieee80211_rx_cooked_monitor(struct ieee80211_rx_data *rx,
struct net_device *prev_dev = NULL;
struct ieee80211_rx_status *status = IEEE80211_SKB_RXCB(skb);
- if (status->flag & RX_FLAG_INTERNAL_CMTR)
- goto out_free_skb;
-
if (skb_headroom(skb) < sizeof(*rthdr) &&
pskb_expand_head(skb, sizeof(*rthdr), 0, GFP_ATOMIC))
goto out_free_skb;
@@ -2260,7 +2257,6 @@ static void ieee80211_rx_cooked_monitor(struct ieee80211_rx_data *rx,
} else
goto out_free_skb;
- status->flag |= RX_FLAG_INTERNAL_CMTR;
return;
out_free_skb:
--
John W. Linville Someday the world will need a hero, and you
linville@tuxdriver.com might be all we have. Be ready.
^ permalink raw reply related
* Re: Packet time delays on multi-core systems
From: Eric Dumazet @ 2010-09-30 12:46 UTC (permalink / raw)
To: Alexey Vlasov; +Cc: Linux Kernel Mailing List, netdev
In-Reply-To: <20100930123048.GA4094@beaver.vrungel.ru>
Le jeudi 30 septembre 2010 à 16:30 +0400, Alexey Vlasov a écrit :
> On Wed, Sep 29, 2010 at 11:45:21PM +0200, Eric Dumazet wrote:
> > But if you send SYN packets in the same time, (logged), this might
> > slow
> > down the reception (and answers) of ICMP frames. LOG target can be
> > quite
> > expensive...
>
> Yes, it's clear that some slow down can appear, but 100 ms is too much,
> and this happens at 200 SYN packets in 2 minutes just as in my example.
> On old servers where some tx/rx are missing in NIC card I don't see
> such a situation even at more then 1000 SYN-packets per sec.
Because all cpus were servicing interrupts, which was good for your
needs. Things apparently changed with 2.6.32.
You have a multiqueue NIC, but using a single CPU to handle the
workload.
>
> > Is using other rules gives same problem ?
> >
> > iptables -A OUTPUT -p tcp -m tcp --dport 80 --tcp-flags
>
> No, only LOG gives such a scheme.
>
^ permalink raw reply
* Re: Packet time delays on multi-core systems
From: Eric Dumazet @ 2010-09-30 12:44 UTC (permalink / raw)
To: Alexey Vlasov; +Cc: Linux Kernel Mailing List, netdev
In-Reply-To: <20100930122321.GA1575@beaver.vrungel.ru>
Le jeudi 30 septembre 2010 à 16:23 +0400, Alexey Vlasov a écrit :
> On Thu, Sep 30, 2010 at 08:33:52AM +0200, Eric Dumazet wrote:
> > Le jeudi 30 septembre 2010 ?? 10:24 +0400, Alexey Vlasov a ??crit :
> > > Here I found some dude with the same problem:
> > > http://lkml.org/lkml/2010/7/9/340
> > >
> >
> > In your opinion its the same problem.
> >
> > But the description you gave is completely different.
> >
> > You have time skew only when activating a particular iptables rule.
>
> Well I put interrups from NIC, namely tx/rx query, to different
> processors and got normal pings by adding LOG rule.
>
> I also found that overruns is constantly growing, I don't know if these are connected.
> RX packets:2831439546 errors:0 dropped:134726 overruns:947671733 frame:0
> TX packets:2880849825 errors:0 dropped:0 overruns:0 carrier:0
>
> Rather strange that only one processor was involved, even in top was
> clear that ksoftirqd eats the first processor up to 100%.
>
OK, because only CPU0 gets interrupts of all queues.
> Here goes the typical distribution of interrups on new servers:
> CPU0 CPU1 CPU2 CPU3 ... CPU23
> 752: 11 0 0 0 ... 0 PCI-MSI-edge eth0
> 753: 2799366721 0 0 0 ... 0 PCI-MSI-edge eth0-rx3
> 754: 2821840553 0 0 0 ... 0 PCI-MSI-edge eth0-rx2
> 755: 2786117044 0 0 0 ... 0 PCI-MSI-edge eth0-rx1
> 756: 2896099336 0 0 0 ... 0 PCI-MSI-edge eth0-rx0
> 757: 1808404680 0 0 0 ... 0 PCI-MSI-edge eth0-tx3
> 758: 1797855130 0 0 0 ... 0 PCI-MSI-edge eth0-tx2
> 759: 1807222032 0 0 0 ... 0 PCI-MSI-edge eth0-tx1
> 760: 1820309360 0 0 0 ... 0 PCI-MSI-edge eth0-tx0
>
echo 01 >/proc/irq/*/eth0-rx0/../smp_affinity
echo 02 >/proc/irq/*/eth0-rx1/../smp_affinity
echo 04 >/proc/irq/*/eth0-rx2/../smp_affinity
echo 08 >/proc/irq/*/eth0-rx3/../smp_affinity
cat /proc/irq/*/eth0-rx0/../smp_affinity
cat /proc/irq/*/eth0-rx1/../smp_affinity
cat /proc/irq/*/eth0-rx2/../smp_affinity
cat /proc/irq/*/eth0-rx3/../smp_affinity
> On the old ones:
> CPU0 CPU1 CPU2 ... CPU8
> 502: 522320256 522384039 522327386 ... 522380267 PCI-MSI-edge eth0
>
What network driver is it (newbox), was it (old box) ?
If you switch to 2.6.35, you can use RPS to dispatch packets to several
cpu, in the case interrupt affinity could not be changed (all interrupts
still handled by CPU0)
^ permalink raw reply
* Re: Packet time delays on multi-core systems
From: Alexey Vlasov @ 2010-09-30 12:30 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Linux Kernel Mailing List, netdev
In-Reply-To: <1285796721.5211.156.camel@edumazet-laptop>
On Wed, Sep 29, 2010 at 11:45:21PM +0200, Eric Dumazet wrote:
> But if you send SYN packets in the same time, (logged), this might
> slow
> down the reception (and answers) of ICMP frames. LOG target can be
> quite
> expensive...
Yes, it's clear that some slow down can appear, but 100 ms is too much,
and this happens at 200 SYN packets in 2 minutes just as in my example.
On old servers where some tx/rx are missing in NIC card I don't see
such a situation even at more then 1000 SYN-packets per sec.
> Is using other rules gives same problem ?
>
> iptables -A OUTPUT -p tcp -m tcp --dport 80 --tcp-flags
No, only LOG gives such a scheme.
--
BRGDS. Alexey Vlasov.
^ permalink raw reply
* Re: Packet time delays on multi-core systems
From: Alexey Vlasov @ 2010-09-30 12:23 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Linux Kernel Mailing List, netdev
In-Reply-To: <1285828432.5211.812.camel@edumazet-laptop>
On Thu, Sep 30, 2010 at 08:33:52AM +0200, Eric Dumazet wrote:
> Le jeudi 30 septembre 2010 ?? 10:24 +0400, Alexey Vlasov a ??crit :
> > Here I found some dude with the same problem:
> > http://lkml.org/lkml/2010/7/9/340
> >
>
> In your opinion its the same problem.
>
> But the description you gave is completely different.
>
> You have time skew only when activating a particular iptables rule.
Well I put interrups from NIC, namely tx/rx query, to different
processors and got normal pings by adding LOG rule.
I also found that overruns is constantly growing, I don't know if these are connected.
RX packets:2831439546 errors:0 dropped:134726 overruns:947671733 frame:0
TX packets:2880849825 errors:0 dropped:0 overruns:0 carrier:0
Rather strange that only one processor was involved, even in top was
clear that ksoftirqd eats the first processor up to 100%.
Here goes the typical distribution of interrups on new servers:
CPU0 CPU1 CPU2 CPU3 ... CPU23
752: 11 0 0 0 ... 0 PCI-MSI-edge eth0
753: 2799366721 0 0 0 ... 0 PCI-MSI-edge eth0-rx3
754: 2821840553 0 0 0 ... 0 PCI-MSI-edge eth0-rx2
755: 2786117044 0 0 0 ... 0 PCI-MSI-edge eth0-rx1
756: 2896099336 0 0 0 ... 0 PCI-MSI-edge eth0-rx0
757: 1808404680 0 0 0 ... 0 PCI-MSI-edge eth0-tx3
758: 1797855130 0 0 0 ... 0 PCI-MSI-edge eth0-tx2
759: 1807222032 0 0 0 ... 0 PCI-MSI-edge eth0-tx1
760: 1820309360 0 0 0 ... 0 PCI-MSI-edge eth0-tx0
On the old ones:
CPU0 CPU1 CPU2 ... CPU8
502: 522320256 522384039 522327386 ... 522380267 PCI-MSI-edge eth0
--
BRGDS. Alexey Vlasov.
^ permalink raw reply
* Re: VLAN packets silently dropped in promiscuous mode
From: Eric Dumazet @ 2010-09-30 12:16 UTC (permalink / raw)
To: Patrick McHardy; +Cc: Roger Luethi, David Miller, Jesse Gross, netdev
In-Reply-To: <4CA46B72.2090100@trash.net>
Le jeudi 30 septembre 2010 à 12:50 +0200, Patrick McHardy a écrit :
> This should be fine as long as the packets are properly marked
> with PACKET_OTHERHOST.
Ah thanks Patrick for the tip !
I tested following patch on tg3 and it is doing the right thing this
time.
[PATCH] vlan: dont drop packets from unknown vlans in promiscuous mode
Roger Luethi noticed packets for unknown VLANs getting silently dropped
even in promiscuous mode.
Check for promiscuous mode in __vlan_hwaccel_rx() and vlan_gro_common()
before drops.
As suggested by Patrick, mark such packets to have skb->pkt_type set to
PACKET_OTHERHOST to make sure they are dropped by IP stack.
Reported-by: Roger Luethi <rl@hellgate.ch>
Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
CC: Patrick McHardy <kaber@trash.net>
---
net/8021q/vlan_core.c | 14 ++++++++++----
1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/net/8021q/vlan_core.c b/net/8021q/vlan_core.c
index 01ddb04..0eb96f7 100644
--- a/net/8021q/vlan_core.c
+++ b/net/8021q/vlan_core.c
@@ -24,8 +24,11 @@ int __vlan_hwaccel_rx(struct sk_buff *skb, struct vlan_group *grp,
if (vlan_dev)
skb->dev = vlan_dev;
- else if (vlan_id)
- goto drop;
+ else if (vlan_id) {
+ if (!(skb->dev->flags & IFF_PROMISC))
+ goto drop;
+ skb->pkt_type = PACKET_OTHERHOST;
+ }
return (polling ? netif_receive_skb(skb) : netif_rx(skb));
@@ -102,8 +105,11 @@ vlan_gro_common(struct napi_struct *napi, struct vlan_group *grp,
if (vlan_dev)
skb->dev = vlan_dev;
- else if (vlan_id)
- goto drop;
+ else if (vlan_id) {
+ if (!(skb->dev->flags & IFF_PROMISC))
+ goto drop;
+ skb->pkt_type = PACKET_OTHERHOST;
+ }
for (p = napi->gro_list; p; p = p->next) {
NAPI_GRO_CB(p)->same_flow =
^ permalink raw reply related
* Re: [PATCH] ipv4: remove all rt cache entries on UNREGISTER event
From: Nicolas Dichtel @ 2010-09-30 11:49 UTC (permalink / raw)
To: Eric Dumazet; +Cc: netdev, Octavian Purdila
In-Reply-To: <1285751929.2615.30.camel@edumazet-laptop>
Patch works well with my case.
In fact, it's more proper to returns an error to the daemon to let it
knows that packet was not sent.
Acked-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
Regards,
Nicolas
Eric Dumazet wrote:
> I found following patch was enough to avoid route being created if
> device is down. This is still racy and needs more thinking.
>
>
> diff --git a/net/ipv4/route.c b/net/ipv4/route.c
> index ac6559c..1ee0b1a 100644
> --- a/net/ipv4/route.c
> +++ b/net/ipv4/route.c
> @@ -2586,9 +2586,10 @@ static int ip_route_output_slow(struct net *net, struct rtable **rp,
> goto out;
>
> /* RACE: Check return value of inet_select_addr instead. */
> - if (__in_dev_get_rtnl(dev_out) == NULL) {
> + if (!(dev_out->flags & IFF_UP) || __in_dev_get_rtnl(dev_out) == NULL) {
> dev_put(dev_out);
> - goto out; /* Wrong error code */
> + err = -ENETUNREACH;
> + goto out;
> }
>
> if (ipv4_is_local_multicast(oldflp->fl4_dst) ||
>
>
^ permalink raw reply
* Re: [PATCH] nf_nat_snmp: fix checksum calculation (v3)
From: Patrick McHardy @ 2010-09-30 11:03 UTC (permalink / raw)
To: 王韬; +Cc: akpm, bugzilla-daemon, bugme-daemon, netdev, shemminger
In-Reply-To: <AANLkTi=FbjWdAVfmoBK8E21gDEqxYyDsUyoJsGAfRumW@mail.gmail.com>
On 30.09.2010 10:25, 王韬 wrote:
> Dear Patrick
>
> I am Clark Wang. I have one question is that which
> version of kernel will contain this patch.
Its currently contained in the net-2.6.git tree, so it should be
in final .36 release.
^ permalink raw reply
* Re: VLAN packets silently dropped in promiscuous mode
From: Patrick McHardy @ 2010-09-30 10:50 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Roger Luethi, David Miller, Jesse Gross, netdev
In-Reply-To: <1285843234.2615.278.camel@edumazet-laptop>
On 30.09.2010 12:40, Eric Dumazet wrote:
> Le jeudi 30 septembre 2010 à 11:55 +0200, Eric Dumazet a écrit :
>> Le jeudi 30 septembre 2010 à 11:16 +0200, Eric Dumazet a écrit :
>>
>>> Agreed
>>>
>>> Could you try following patch, based on net-next-2.6 ?
>>
>> Here is the official patch I cooked for net-2.6 (linux-2.6)
>>
>> I tested it successfully with a tg3 NIC.
>>
>> Thanks
>>
>> [PATCH] vlan: dont drop packets from unknown vlans in promiscuous mode
>>
>> Roger Luethi noticed packets for unknown VLANs getting silently dropped
>> even in promiscuous mode.
>>
>> Check for promiscuous mode in __vlan_hwaccel_rx() and vlan_gro_common()
>> before drops.
>>
>> Reported-by: Roger Luethi <rl@hellgate.ch>
>> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
>> ---
>> net/8021q/vlan_core.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/net/8021q/vlan_core.c b/net/8021q/vlan_core.c
>> index 01ddb04..ba502b4 100644
>> --- a/net/8021q/vlan_core.c
>> +++ b/net/8021q/vlan_core.c
>> @@ -24,7 +24,7 @@ int __vlan_hwaccel_rx(struct sk_buff *skb, struct vlan_group *grp,
>>
>> if (vlan_dev)
>> skb->dev = vlan_dev;
>> - else if (vlan_id)
>> + else if (vlan_id && !(skb->dev->flags & IFF_PROMISC))
>> goto drop;
>>
>> return (polling ? netif_receive_skb(skb) : netif_rx(skb));
>> @@ -102,7 +102,7 @@ vlan_gro_common(struct napi_struct *napi, struct vlan_group *grp,
>>
>> if (vlan_dev)
>> skb->dev = vlan_dev;
>> - else if (vlan_id)
>> + else if (vlan_id && !(skb->dev->flags & IFF_PROMISC))
>> goto drop;
>>
>> for (p = napi->gro_list; p; p = p->next) {
>>
>
>
> Hmm, packets are delivered not only to tcpdump but also on other stacks,
> on ethX.
>
> So this is a domain violation.
This should be fine as long as the packets are properly marked
with PACKET_OTHERHOST.
^ permalink raw reply
* Re: VLAN packets silently dropped in promiscuous mode
From: Eric Dumazet @ 2010-09-30 10:40 UTC (permalink / raw)
To: Roger Luethi; +Cc: David Miller, Jesse Gross, netdev, Patrick McHardy
In-Reply-To: <1285840532.2615.196.camel@edumazet-laptop>
Le jeudi 30 septembre 2010 à 11:55 +0200, Eric Dumazet a écrit :
> Le jeudi 30 septembre 2010 à 11:16 +0200, Eric Dumazet a écrit :
>
> > Agreed
> >
> > Could you try following patch, based on net-next-2.6 ?
>
> Here is the official patch I cooked for net-2.6 (linux-2.6)
>
> I tested it successfully with a tg3 NIC.
>
> Thanks
>
> [PATCH] vlan: dont drop packets from unknown vlans in promiscuous mode
>
> Roger Luethi noticed packets for unknown VLANs getting silently dropped
> even in promiscuous mode.
>
> Check for promiscuous mode in __vlan_hwaccel_rx() and vlan_gro_common()
> before drops.
>
> Reported-by: Roger Luethi <rl@hellgate.ch>
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> ---
> net/8021q/vlan_core.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/net/8021q/vlan_core.c b/net/8021q/vlan_core.c
> index 01ddb04..ba502b4 100644
> --- a/net/8021q/vlan_core.c
> +++ b/net/8021q/vlan_core.c
> @@ -24,7 +24,7 @@ int __vlan_hwaccel_rx(struct sk_buff *skb, struct vlan_group *grp,
>
> if (vlan_dev)
> skb->dev = vlan_dev;
> - else if (vlan_id)
> + else if (vlan_id && !(skb->dev->flags & IFF_PROMISC))
> goto drop;
>
> return (polling ? netif_receive_skb(skb) : netif_rx(skb));
> @@ -102,7 +102,7 @@ vlan_gro_common(struct napi_struct *napi, struct vlan_group *grp,
>
> if (vlan_dev)
> skb->dev = vlan_dev;
> - else if (vlan_id)
> + else if (vlan_id && !(skb->dev->flags & IFF_PROMISC))
> goto drop;
>
> for (p = napi->gro_list; p; p = p->next) {
>
Hmm, packets are delivered not only to tcpdump but also on other stacks,
on ethX.
So this is a domain violation.
Patch is not applicable as is.
^ permalink raw reply
* Re: VLAN packets silently dropped in promiscuous mode
From: Eric Dumazet @ 2010-09-30 9:55 UTC (permalink / raw)
To: Roger Luethi, David Miller; +Cc: Jesse Gross, netdev, Patrick McHardy
In-Reply-To: <1285838215.2615.126.camel@edumazet-laptop>
Le jeudi 30 septembre 2010 à 11:16 +0200, Eric Dumazet a écrit :
> Agreed
>
> Could you try following patch, based on net-next-2.6 ?
Here is the official patch I cooked for net-2.6 (linux-2.6)
I tested it successfully with a tg3 NIC.
Thanks
[PATCH] vlan: dont drop packets from unknown vlans in promiscuous mode
Roger Luethi noticed packets for unknown VLANs getting silently dropped
even in promiscuous mode.
Check for promiscuous mode in __vlan_hwaccel_rx() and vlan_gro_common()
before drops.
Reported-by: Roger Luethi <rl@hellgate.ch>
Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
net/8021q/vlan_core.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/net/8021q/vlan_core.c b/net/8021q/vlan_core.c
index 01ddb04..ba502b4 100644
--- a/net/8021q/vlan_core.c
+++ b/net/8021q/vlan_core.c
@@ -24,7 +24,7 @@ int __vlan_hwaccel_rx(struct sk_buff *skb, struct vlan_group *grp,
if (vlan_dev)
skb->dev = vlan_dev;
- else if (vlan_id)
+ else if (vlan_id && !(skb->dev->flags & IFF_PROMISC))
goto drop;
return (polling ? netif_receive_skb(skb) : netif_rx(skb));
@@ -102,7 +102,7 @@ vlan_gro_common(struct napi_struct *napi, struct vlan_group *grp,
if (vlan_dev)
skb->dev = vlan_dev;
- else if (vlan_id)
+ else if (vlan_id && !(skb->dev->flags & IFF_PROMISC))
goto drop;
for (p = napi->gro_list; p; p = p->next) {
^ permalink raw reply related
* Re: [PATCH] de2104x: disable media debug messages by default
From: Michał Mirosław @ 2010-09-30 9:46 UTC (permalink / raw)
To: Ondrej Zary; +Cc: jgarzik, netdev, Kernel development list
In-Reply-To: <201009282018.57547.linux@rainbow-software.org>
2010/9/28 Ondrej Zary <linux@rainbow-software.org>:
> Print media debug messages only when HW debug is enabled.
>
> Signed-off-by: Ondrej Zary <linux@rainbow-software.org>
>
> --- linux-2.6.36-rc3-/drivers/net/tulip/de2104x.c 2010-09-28 19:50:51.000000000 +0200
> +++ linux-2.6.36-rc3/drivers/net/tulip/de2104x.c 2010-09-28 20:05:34.000000000 +0200
> @@ -948,8 +948,9 @@ static void de_set_media (struct de_priv
> else
> macmode &= ~FullDuplex;
>
> - if (netif_msg_link(de)) {
> + if (netif_msg_link(de))
> dev_info(&de->dev->dev, "set link %s\n", media_name[media]);
You can use netif_info(de, link, de->dev, ...) instead and get 'ethX:
' prefix for free.
> + if (netif_msg_hw(de)) {
> dev_info(&de->dev->dev, "mode 0x%x, sia 0x%x,0x%x,0x%x,0x%x\n",
> dr32(MacMode), dr32(SIAStatus),
> dr32(CSR13), dr32(CSR14), dr32(CSR15));
Same here.
Best Regards,
Michał Mirosław
^ permalink raw reply
* Re: Something wrong with tx/rx/sg/gso with dhclient etc (Was: Linux 2.6.35.6/e1000e does not receive replies from DHCP server, 2.6.33 works)
From: Sami Farin @ 2010-09-30 9:44 UTC (permalink / raw)
To: Tantilov, Emil S; +Cc: Sami Farin, e1000-devel, linux-kernel, netdev
In-Reply-To: <EA929A9653AAE14F841771FB1DE5A13660222E867B@rrsmsx501.amr.corp.intel.com>
On Wed, Sep 29, 2010 at 17:40:08 -0600, Tantilov, Emil S wrote:
> Sami Farin wrote:
> > False alarm, 2.6.35.6+latest git e1000e does not work any better.
> > I was just lucky.
>
> Is your `latest git` from Linus or net-next tree?
Linus.
>All of our latest patches go into net-next, so if you haven't already - give it a try and see if it resolves your issue.
Maybe later =)
> >
> > One thing what was common, when it works, I get this line a little
> > time before dhclient start working:
> >
> > eth0: IPv6 duplicate address fe80::219:d1ff:fe00:5f01 detected!
>
> I have a system with the same device ID (it is not the same board) and could not reproduce any issues with DHCP on 2.6.35.6 kernel.
Okay, thanks for trying.
> Aside from checking the latest net-next tree, there are some other things to look into:
>
> 1. Is AMT enabled - there is usually a manageability tab/option in the BIOS. If you have that option try enabling/disabling it and see if it makes a difference.
I believe I haven't used AMT, but I check that option.
> 2. Make sure your BIOS is up to date.
Latest .ISO update which worked was CO6079P from Aug 2008, maybe I can
get the USB boot to work..
> If any of the above does not help your situation please file a bug at e1000.sf.net and include the following information:
I find it odd I stop seeing the reply packets just like that..
Can this be e1000e bug/feature or something else?
For example, I did "make oldconfig" in net-next, and:
----------------
CONFIG_NETFILTER_XT_TARGET_CHECKSUM:
This option adds a `CHECKSUM' target, which can be used in the iptables mangle table.
You can use this target to compute and fill in the checksum in
a packet that lacks a checksum. This is particularly useful,
if you need to work around old applications such as dhcp clients, <<=== here
that do not work well with checksum offloads, but don't want to disable
checksum offload in your device.
----------------
But not even tcpdump sees the packets.. shouldn't it see them despite
rx/tx settings? What could be eating the packets?
FYI, I just played with "rx off tx off sg off gso off" and
"rx on tx on sg on gso on", when the options were all on, I could not
even get ARP reply from my router! Two seconds after I turned them off,
all start working. Also dhclient worked—now that I tried—with all off.
I believe there is something fishy in rx, tx, sg and/or gso features,
which worked in 2.6.33 AFAICT. Am I sounding ambigue? ;)
> 1. lspci -vvv
> 2. ethtool -e eth0
> 3. there is a tool call ethregs which you can download from this site. If you can include the output of ethregs -s 00:19.0
> 4. kernel config
> 5. anything that you think may be related - like setup, type of traffic etc.
>
> Thanks,
> Emil
--
Do what you love because life is too short for anything else.
------------------------------------------------------------------------------
Start uncovering the many advantages of virtual appliances
and start using them to simplify application deployment and
accelerate your shift to cloud computing.
http://p.sf.net/sfu/novell-sfdev2dev
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel® Ethernet, visit http://communities.intel.com/community/wired
^ permalink raw reply
* Re: VLAN packets silently dropped in promiscuous mode
From: Eric Dumazet @ 2010-09-30 9:16 UTC (permalink / raw)
To: Roger Luethi; +Cc: Jesse Gross, netdev, Patrick McHardy
In-Reply-To: <20100930080703.GA10827@core.hellgate.ch>
Le jeudi 30 septembre 2010 à 10:07 +0200, Roger Luethi a écrit :
> On Wed, 29 Sep 2010 10:44:26 -0700, Jesse Gross wrote:
> > On Wed, Sep 29, 2010 at 4:37 AM, Roger Luethi <rl@hellgate.ch> wrote:
> > > I noticed packets for unknown VLANs getting silently dropped even in
> > > promiscuous mode (this is true only for the hardware accelerated path).
> > > netif_nit_deliver was introduced specifically to prevent that, but the
> > > function gets called only _after_ packets from unknown VLANs have been
> > > dropped.
> >
> > Some drivers are fixing this on a case by case basis by disabling
> > hardware accelerated VLAN stripping when in promiscuous mode, i.e.:
> > http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=5f6c01819979afbfec7e0b15fe52371b8eed87e8
> >
> > However, at this point it is more or less random which drivers do
> > this. It would obviously be much better if it were consistent.
>
> My understanding is this. Hardware VLAN tagging and stripping can always be
> enabled. The kernel passes 802.1Q information along with the stripped
> header to libpcap which reassembles the original header where necessary.
> Works for me.
>
> Hardware VLAN filtering, on the other hand, must be disabled in promiscuous
> mode. But doing that in the driver makes no difference now as the current
> VLAN code drops the packets so preserved before they are passed to the pcap
> interface. That appears to be a bug.
Agreed
Could you try following patch, based on net-next-2.6 ?
diff --git a/net/8021q/vlan_core.c b/net/8021q/vlan_core.c
index 0eb486d..fabdedb 100644
--- a/net/8021q/vlan_core.c
+++ b/net/8021q/vlan_core.c
@@ -101,7 +101,7 @@ vlan_gro_common(struct napi_struct *napi, struct vlan_group *grp,
if (vlan_dev)
skb->dev = vlan_dev;
- else if (vlan_id)
+ else if (vlan_id && !(skb->dev->flags & IFF_PROMISC))
goto drop;
for (p = napi->gro_list; p; p = p->next) {
^ permalink raw reply related
* Re: [MeeGo-Dev][PATCH v3] Topcliff: Update PCH_CAN driver to 2.6.35
From: Wolfgang Grandegger @ 2010-09-30 9:10 UTC (permalink / raw)
To: Masayuki Ohtak
Cc: andrew.chih.howe.khor-ral2JQCrhuEAvxtiuMwx3w, Samuel Ortiz,
margie.foster-ral2JQCrhuEAvxtiuMwx3w,
netdev-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
socketcan-core-0fE9KPoRgkgATYTw5x5z8w,
yong.y.wang-ral2JQCrhuEAvxtiuMwx3w,
kok.howg.ewe-ral2JQCrhuEAvxtiuMwx3w,
joel.clark-ral2JQCrhuEAvxtiuMwx3w, Tomoya MORINAGA,
meego-dev-WXzIur8shnEAvxtiuMwx3w, David S. Miller,
Christian Pellegrin, qi.wang-ral2JQCrhuEAvxtiuMwx3w
In-Reply-To: <4C9C7C6F.1000003-ECg8zkTtlr0C6LszWs/t0g@public.gmane.org>
Hi Ohtake,
here comes my review, sorry for delay.
On 09/24/2010 12:24 PM, Masayuki Ohtak wrote:
> Hi Wolfgang and Marc,
>
> We have modified a pretty amount of our driver based on other accepted Socket CAN driver.
> Additionally, We have reduced the number of lines 3601 to 1444.
Much better, but I believe it could be reduced even further.
> Please check below.
>
> Thanks, Ohtake(OKISemi)
>
> ---
> CAN driver of Topcliff PCH
>
> Topcliff PCH is the platform controller hub that is going to be used in
> Intel's upcoming general embedded platform. All IO peripherals in
> Topcliff PCH are actually devices sitting on AMBA bus.
> Topcliff PCH has CAN I/F. This driver enables CAN function.
>
> Signed-off-by: Masayuki Ohtake <masa-korg-ECg8zkTtlr0C6LszWs/t0g@public.gmane.org>
> ---
> drivers/net/can/Kconfig | 8 +
> drivers/net/can/Makefile | 1 +
> drivers/net/can/pch_can.c | 1444 +++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 1453 insertions(+), 0 deletions(-)
> create mode 100644 drivers/net/can/pch_can.c
>
> diff --git a/drivers/net/can/Kconfig b/drivers/net/can/Kconfig
> index 2c5227c..5c98a20 100644
> --- a/drivers/net/can/Kconfig
> +++ b/drivers/net/can/Kconfig
> @@ -73,6 +73,14 @@ config CAN_JANZ_ICAN3
> This driver can also be built as a module. If so, the module will be
> called janz-ican3.ko.
>
> +config PCH_CAN
> + tristate "PCH CAN"
> + depends on CAN_DEV
> + ---help---
> + This driver is for PCH CAN of Topcliff which is an IOH for x86
> + embedded processor.
> + This driver can access CAN bus.
> +
> source "drivers/net/can/mscan/Kconfig"
>
> source "drivers/net/can/sja1000/Kconfig"
> diff --git a/drivers/net/can/Makefile b/drivers/net/can/Makefile
> index 9047cd0..3ddc6a7 100644
> --- a/drivers/net/can/Makefile
> +++ b/drivers/net/can/Makefile
> @@ -16,5 +16,6 @@ obj-$(CONFIG_CAN_TI_HECC) += ti_hecc.o
> obj-$(CONFIG_CAN_MCP251X) += mcp251x.o
> obj-$(CONFIG_CAN_BFIN) += bfin_can.o
> obj-$(CONFIG_CAN_JANZ_ICAN3) += janz-ican3.o
> +obj-$(CONFIG_PCH_CAN) += pch_can.o
Please provide patches against David Millers "net-next-2.6" GIT tree and
use the prefix "can: " in your subject next time. See
http://svn.berlios.de/wsvn/socketcan/trunk/README.submitting-patches
for further information.
> ccflags-$(CONFIG_CAN_DEBUG_DEVICES) := -DDEBUG
> diff --git a/drivers/net/can/pch_can.c b/drivers/net/can/pch_can.c
> new file mode 100644
> index 0000000..8c1731b
> --- /dev/null
> +++ b/drivers/net/can/pch_can.c
> @@ -0,0 +1,1444 @@
> +/*
> + * Copyright (C) 1999 - 2010 Intel Corporation.
> + * Copyright (C) 2010 OKI SEMICONDUCTOR Co., LTD.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; version 2 of the License.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307, USA.
> + */
> +
> +#include <linux/interrupt.h>
> +#include <linux/delay.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/sched.h>
> +#include <linux/pci.h>
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/types.h>
> +#include <linux/errno.h>
> +#include <linux/netdevice.h>
> +#include <linux/skbuff.h>
> +#include <linux/can.h>
> +#include <linux/can/dev.h>
> +#include <linux/can/error.h>
> +
> +#define MAX_BITRATE 0x3e8
Dead code? At least it's not used anywhere.
> +
> +#define MAX_MSG_OBJ 32
> +#define MSG_OBJ_RX 0 /* The receive message object flag. */
> +#define MSG_OBJ_TX 1 /* The transmit message object flag. */
> +
> +#define ENABLE 1 /* The enable flag */
> +#define DISABLE 0 /* The disable flag */
> +#define CAN_CTRL_INIT 0x0001 /* The INIT bit of CANCONT register. */
> +#define CAN_CTRL_IE 0x0002 /* The IE bit of CAN control register */
> +#define CAN_CTRL_IE_SIE_EIE 0x000e
> +#define CAN_CTRL_CCE 0x0040
> +#define CAN_CTRL_OPT 0x0080 /* The OPT bit of CANCONT register. */
> +#define CAN_OPT_SILENT 0x0008 /* The Silent bit of CANOPT reg. */
> +#define CAN_OPT_LBACK 0x0010 /* The LoopBack bit of CANOPT reg. */
> +#define CAN_CMASK_RX_TX_SET 0x00f3
> +#define CAN_CMASK_RX_TX_GET 0x0073
> +#define CAN_CMASK_ALL 0xff
> +#define CAN_CMASK_RDWR 0x80
> +#define CAN_CMASK_ARB 0x20
> +#define CAN_CMASK_CTRL 0x10
> +#define CAN_CMASK_MASK 0x40
> +
> +#define CAN_IF_MCONT_NEWDAT 0x8000
> +#define CAN_IF_MCONT_INTPND 0x2000
> +#define CAN_IF_MCONT_UMASK 0x1000
> +#define CAN_IF_MCONT_TXIE 0x0800
> +#define CAN_IF_MCONT_RXIE 0x0400
> +#define CAN_IF_MCONT_RMTEN 0x0200
> +#define CAN_IF_MCONT_TXRQXT 0x0100
> +#define CAN_IF_MCONT_EOB 0x0080
> +#define CAN_IF_MCONT_MSGLOST 0x4000
> +#define CAN_MASK2_MDIR_MXTD 0xc000
> +#define CAN_ID2_DIR 0x2000
> +#define CAN_ID_MSGVAL 0x8000
> +
> +#define CAN_STATUS_INT 0x8000
> +#define CAN_IF_CREQ_BUSY 0x8000
> +#define CAN_ID2_XTD 0x4000
> +
> +#define CAN_REC 0x00007f00
> +#define CAN_TEC 0x000000ff
> +
> +#define PCH_RX_OK 0x00000010
> +#define PCH_TX_OK 0x00000008
> +#define PCH_BUS_OFF 0x00000080
> +#define PCH_EWARN 0x00000040
> +#define PCH_EPASSIV 0x00000020
> +#define PCH_LEC0 0x00000001
> +#define PCH_LEC1 0x00000002
> +#define PCH_LEC2 0x00000004
> +#define PCH_LEC_ALL (PCH_LEC0 | PCH_LEC1 | PCH_LEC2)
> +#define PCH_STUF_ERR PCH_LEC0
> +#define PCH_FORM_ERR PCH_LEC1
> +#define PCH_ACK_ERR (PCH_LEC0 | PCH_LEC1)
> +#define PCH_BIT1_ERR PCH_LEC2
> +#define PCH_BIT0_ERR (PCH_LEC0 | PCH_LEC2)
> +#define PCH_CRC_ERR (PCH_LEC1 | PCH_LEC2)
enum {
PCH_LEC_STUF_ERR = 0,
PCH_LEC_FORM_ERR,
PCH_LEC_ACK_ERR,
...
PCH_LEC_ALL
};
Seems more appropriate. More comments below.
> +
> +/* bit position of certain controller bits. */
> +#define BIT_BITT_BRP 0
> +#define BIT_BITT_SJW 6
> +#define BIT_BITT_TSEG1 8
> +#define BIT_BITT_TSEG2 12
> +#define BIT_IF1_MCONT_RXIE 10
> +#define BIT_IF2_MCONT_TXIE 11
> +#define BIT_BRPE_BRPE 6
> +#define BIT_ES_TXERRCNT 0
> +#define BIT_ES_RXERRCNT 8
> +#define MSK_BITT_BRP 0x3f
> +#define MSK_BITT_SJW 0xc0
> +#define MSK_BITT_TSEG1 0xf00
> +#define MSK_BITT_TSEG2 0x7000
> +#define MSK_BRPE_BRPE 0x3c0
> +#define MSK_BRPE_GET 0x0f
> +#define MSK_CTRL_IE_SIE_EIE 0x07
> +#define MSK_MCONT_TXIE 0x08
> +#define MSK_MCONT_RXIE 0x10
> +#define PCH_CAN_NO_TX_BUFF 1
> +#define PCI_DEVICE_ID_INTEL_PCH1_CAN 0x8818
> +#define COUNTER_LIMIT 0xFFFF
Keep alignment?
> +#define PCH_CAN_CLK 50000 /* 50MHz */
Please specify it in Hz already here.
> +
> +/* Total 32 OBJs */
> +#define PCH_RX_OBJ_NUM 1
> +#define PCH_TX_OBJ_NUM 1
> +#define PCH_OBJ_NUM (PCH_TX_OBJ_NUM + PCH_RX_OBJ_NUM)
Please explain biefly what message object are use for what purpose.
Either here or in the initialization code.
> +
> +#define PCH_CAN_ACTIVE 0
> +#define PCH_CAN_LISTEN 1
> +#define PCH_CAN_STOP 0
> +#define PCH_CAN_RUN 1
> +
> +#define PCH_CAN_ENABLE 0
> +#define PCH_CAN_DISABLE 1
> +#define PCH_CAN_ALL 2
> +#define PCH_CAN_NONE 3
The above are used in switch case and should therefore be anonymous
enums. I suggested to remove them because I'm not a real friend of the
helper functions which are just called *once*.
> +
> +struct pch_can_regs {
> + u32 cont;
> + u32 stat;
> + u32 errc;
> + u32 bitt;
> + u32 intr;
> + u32 opt;
> + u32 brpe;
> + u32 reserve1;
> + u32 if1_creq;
> + u32 if1_cmask;
> + u32 if1_mask1;
> + u32 if1_mask2;
> + u32 if1_id1;
> + u32 if1_id2;
> + u32 if1_mcont;
> + u32 if1_dataa1;
> + u32 if1_dataa2;
> + u32 if1_datab1;
> + u32 if1_datab2;
> + u32 reserve2;
> + u32 reserve3[12];
> + u32 if2_creq;
> + u32 if2_cmask;
> + u32 if2_mask1;
> + u32 if2_mask2;
> + u32 if2_id1;
> + u32 if2_id2;
> + u32 if2_mcont;
> + u32 if2_dataa1;
> + u32 if2_dataa2;
> + u32 if2_datab1;
> + u32 if2_datab2;
> + u32 reserve4;
> + u32 reserve5[20];
> + u32 treq1;
> + u32 treq2;
> + u32 reserve6[2];
> + u32 reserve7[56];
> + u32 reserve8[3];
> + u32 srst;
> +};
Nice.
> +struct pch_can_priv {
> + struct can_priv can;
> + void __iomem *base;
> + unsigned int can_num;
> + struct pci_dev *dev;
> + unsigned int tx_enable[MAX_MSG_OBJ];
> + unsigned int rx_enable[MAX_MSG_OBJ];
> + unsigned int rx_link[MAX_MSG_OBJ];
> + unsigned int int_enables;
> + unsigned int int_stat;
> + unsigned int bus_off_interrupt;
> + struct net_device *ndev;
> + spinlock_t msgif_reg_lock; /* Message Interface Registers Access Lock*/
> + unsigned int msg_obj[MAX_MSG_OBJ];
> + struct pch_can_regs *regs;
Please add __iomem. Do you need both, regs *and* base?
> +};
> +
> +static struct can_bittiming_const pch_can_bittiming_const = {
> + .name = KBUILD_MODNAME,
Not sure what KBUILD_MODNAME is. Should be "pch_can", the name of the
driver.
> + .tseg1_min = 1,
> + .tseg1_max = 16,
> + .tseg2_min = 1,
> + .tseg2_max = 8,
> + .sjw_max = 4,
> + .brp_min = 1,
> + .brp_max = 1024, /* 6bit + extended 4bit */
> + .brp_inc = 1,
> +};
> +
> +static const struct pci_device_id pch_can_pcidev_id[] __devinitdata = {
> + {PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_PCH1_CAN)},
> + {}
> +};
Please use DEFINE_PCI_DEVICE_TABLE.
> +static inline void pch_can_bit_set(u32 *addr, u32 mask)
> +{
> + iowrite32((ioread32(addr) | mask), addr);
Outer brackets not needed!
> +}
> +
> +static inline void pch_can_bit_clear(u32 *addr, u32 mask)
> +{
> + iowrite32((ioread32(addr) & ~(mask)), addr);
Ditto.
> +}
> +
> +static void pch_can_set_run_mode(struct pch_can_priv *priv, u32 mode)
> +{
> + switch (mode) {
> + case PCH_CAN_RUN:
> + pch_can_bit_clear(&(priv->regs)->cont, CAN_CTRL_INIT);
> + break;
> +
> + case PCH_CAN_STOP:
> + pch_can_bit_set(&(priv->regs)->cont, CAN_CTRL_INIT);
> + break;
> +
> + default:
> + dev_err(&priv->ndev->dev, "%s -> Invalid Mode.\n", __func__);
> + break;
> + }
> +}
> +
> +static void pch_can_get_run_mode(struct pch_can_priv *priv, u32 *mode)
> +{
> + u32 reg_val = ioread32(&(priv->regs)->cont);
I don't think you need the brackets around "priv->regs". Therefore I
suggest s/&(priv->regs)/&priv->regs/ for the whole file.
> +
> + if (reg_val & CAN_CTRL_INIT)
> + *mode = PCH_CAN_STOP;
> + else
> + *mode = PCH_CAN_RUN;
> +}
These are the helper functions I complained about above. And reg_val is
not really needed.
> +static void pch_can_set_optmode(struct pch_can_priv *priv)
> +{
> + u32 reg_val = ioread32(&(priv->regs)->opt);
> +
> + if (priv->can.ctrlmode & CAN_CTRLMODE_LISTENONLY)
> + reg_val |= CAN_OPT_SILENT;
> +
> + if (priv->can.ctrlmode & CAN_CTRLMODE_LOOPBACK)
> + reg_val |= CAN_OPT_LBACK;
> +
> + pch_can_bit_set(&(priv->regs)->cont, CAN_CTRL_OPT);
> + iowrite32(reg_val, &(priv->regs)->opt);
> +}
> +
> +static void pch_can_set_int_custom(struct pch_can_priv *priv)
> +{
> + /* Clearing the IE, SIE and EIE bits of Can control register. */
> + pch_can_bit_clear(&(priv->regs)->cont, CAN_CTRL_IE_SIE_EIE);
> +
> + /* Appropriately setting them. */
> + pch_can_bit_set(&(priv->regs)->cont,
> + ((priv->int_enables & MSK_CTRL_IE_SIE_EIE) << 1));
> +}
> +
> +/* This function retrieves interrupt enabled for the CAN device. */
> +static void pch_can_get_int_enables(struct pch_can_priv *priv, u32 *enables)
> +{
> + u32 reg_ctrl_val = ioread32(&(priv->regs)->cont);
> +
> + /* Obtaining the status of IE, SIE and EIE interrupt bits. */
> + *enables = ((reg_ctrl_val & CAN_CTRL_IE_SIE_EIE) >> 1);
Do you really need an extra variable?
> +}
> +
> +static void pch_can_set_int_enables(struct pch_can_priv *priv, u32 interrupt_no)
> +{
> + switch (interrupt_no) {
> + case PCH_CAN_ENABLE:
> + pch_can_bit_set(&(priv->regs)->cont, CAN_CTRL_IE);
> + break;
> +
> + case PCH_CAN_DISABLE:
> + pch_can_bit_clear(&(priv->regs)->cont, CAN_CTRL_IE);
> + break;
> +
> + case PCH_CAN_ALL:
> + pch_can_bit_set(&(priv->regs)->cont, CAN_CTRL_IE_SIE_EIE);
> + break;
> +
> + case PCH_CAN_NONE:
> + pch_can_bit_clear(&(priv->regs)->cont, CAN_CTRL_IE_SIE_EIE);
> + break;
> +
> + default:
> + dev_err(&priv->ndev->dev, "Invalid interrupt number.\n");
> + break;
> + }
> +}
> +
> +static void pch_can_check_if1_busy(struct pch_can_priv *priv, u32 num)
> +{
> + u32 counter = COUNTER_LIMIT;
> + u32 if1_creq;
> +
> + iowrite32(num, &(priv->regs)->if1_creq);
> + while (counter) {
> + if1_creq = (ioread32(&(priv->regs)->if1_creq)) &
> + CAN_IF_CREQ_BUSY;
> + if (!if1_creq)
> + break;
> + counter--;
> + }
> + if (!counter)
> + dev_err(&priv->ndev->dev, "IF1 BUSY Flag is set forever.\n");
Please use a defined delay for the above timeout. How long does it
usually take the bit to toggle? A small delay, e.g. udelay(1) could be
fine. This function is called in the time critical path!
> +}
> +
> +static void pch_can_check_if2_busy(struct pch_can_priv *priv, u32 num)
> +{
> + u32 counter = COUNTER_LIMIT;
> + u32 if2_creq;
> +
> + iowrite32(num, &(priv->regs)->if2_creq);
> + while (counter) {
> + if2_creq = (ioread32(&(priv->regs)->if2_creq)) &
> + CAN_IF_CREQ_BUSY;
> + if (!if2_creq)
> + break;
> + counter--;
> + }
> + if (!counter)
> + dev_err(&priv->ndev->dev, "IF2 BUSY Flag is set forever.\n");
> +}
Duplicated code!
> +static void pch_can_set_rx_enable(struct pch_can_priv *priv, u32 buff_num,
> + u32 set)
> +{
> + unsigned long flags;
> +
> + spin_lock_irqsave(&priv->msgif_reg_lock, flags);
> + /*Reading the receive buffer data from RAM to Interface1 registers */
Space after /* ?
> + iowrite32(CAN_CMASK_RX_TX_GET, &(priv->regs)->if1_cmask);
> + pch_can_check_if1_busy(priv, buff_num); /* Read from MsgRAN */
> +
> + /* Setting the IF1MASK1 register to access MsgVal and RxIE bits */
> + iowrite32((CAN_CMASK_RDWR | CAN_CMASK_ARB | CAN_CMASK_CTRL),
> + (&(priv->regs)->if1_cmask));
> +
> + if (set == ENABLE) {
> + /* Setting the MsgVal and RxIE bits */
> + pch_can_bit_set(&(priv->regs)->if1_mcont, CAN_IF_MCONT_RXIE);
> + pch_can_bit_set(&(priv->regs)->if1_id2, CAN_ID_MSGVAL);
> +
> + } else if (set == DISABLE) {
> + /* Resetting the MsgVal and RxIE bits */
> + pch_can_bit_clear(&(priv->regs)->if1_mcont, CAN_IF_MCONT_RXIE);
> + pch_can_bit_clear(&(priv->regs)->if1_id2, CAN_ID_MSGVAL);
> + }
> +
> + pch_can_check_if1_busy(priv, buff_num); /* Write to MsgRAM */
> + spin_unlock_irqrestore(&priv->msgif_reg_lock, flags);
> +}
> +
> +static void pch_can_rx_enable_all(struct pch_can_priv *priv)
> +{
> + u32 i;
> +
> + /* Traversing to obtain the object configured as receivers. */
> + for (i = 0; i < PCH_OBJ_NUM; i++) {
> + if (priv->msg_obj[i] == MSG_OBJ_RX)
> + pch_can_set_rx_enable(priv, i + 1, ENABLE);
> + }
> +}
> +
> +static void pch_can_rx_disable_all(struct pch_can_priv *priv)
> +{
> + u32 i;
> +
> + /* Traversing to obtain the object configured as receivers. */
> + for (i = 0; i < PCH_OBJ_NUM; i++) {
> + if (priv->msg_obj[i] == MSG_OBJ_RX)
> + pch_can_set_rx_enable(priv, (i + 1), DISABLE);
> + }
> +}
> +
> +static void pch_can_set_tx_enable(struct pch_can_priv *priv, u32 buff_num,
> + u32 set)
> +{
> + unsigned long flags;
> +
> + spin_lock_irqsave(&priv->msgif_reg_lock, flags);
> + /* Reading the Msg buffer from Message RAM to Interface2 registers. */
> + iowrite32(CAN_CMASK_RX_TX_GET, (&(priv->regs)->if1_cmask));
> + pch_can_check_if1_busy(priv, buff_num);
> +
> + /* Setting the IF2CMASK register for accessing the
> + MsgVal and TxIE bits */
> + iowrite32((CAN_CMASK_RDWR | CAN_CMASK_ARB | CAN_CMASK_CTRL),
> + (&(priv->regs)->if1_cmask));
> +
> + if (set == ENABLE) {
> + /* Setting the MsgVal and TxIE bits */
> + pch_can_bit_set(&(priv->regs)->if1_mcont, CAN_IF_MCONT_TXIE);
> + pch_can_bit_set(&(priv->regs)->if1_id2, CAN_ID_MSGVAL);
> + } else if (set == DISABLE) {
> + /* Resetting the MsgVal and TxIE bits. */
> + pch_can_bit_clear(&(priv->regs)->if1_mcont, CAN_IF_MCONT_TXIE);
> + pch_can_bit_clear(&(priv->regs)->if1_id2, CAN_ID_MSGVAL);
> + }
> +
> + pch_can_check_if1_busy(priv, buff_num); /* Write to MsgRAM */
> + spin_unlock_irqrestore(&priv->msgif_reg_lock, flags);
> +}
> +
> +static void pch_can_tx_enable_all(struct pch_can_priv *priv)
> +{
> + u32 i;
> +
> + /* Traversing to obtain the object configured as transmit object. */
> + for (i = 0; i < PCH_OBJ_NUM; i++) {
> + if (priv->msg_obj[i] == MSG_OBJ_TX)
> + pch_can_set_tx_enable(priv, (i + 1), ENABLE);
> + }
> +}
> +
> +static void pch_can_tx_disable_all(struct pch_can_priv *priv)
> +{
> + u32 i;
> +
> + /* Traversing to obtain the object configured as transmit object. */
> + for (i = 0; i < PCH_OBJ_NUM; i++) {
> + if (priv->msg_obj[i] == MSG_OBJ_TX)
> + pch_can_set_tx_enable(priv, (i + 1), DISABLE);
> + }
> +}
> +
> +static void pch_can_get_rx_enable(struct pch_can_priv *priv, u32 buff_num,
> + u32 *enable)
> +{
> + unsigned long flags;
> +
> + spin_lock_irqsave(&priv->msgif_reg_lock, flags);
> + iowrite32(CAN_CMASK_RX_TX_GET, (&(priv->regs)->if1_cmask));
> + pch_can_check_if1_busy(priv, buff_num);
> +
> + if (((ioread32(&(priv->regs)->if1_id2)) & CAN_ID_MSGVAL) &&
> + ((ioread32(&(priv->regs)->if1_mcont)) &
> + CAN_IF_MCONT_RXIE))
> + *enable = ENABLE;
> + else
> + *enable = DISABLE;
> + spin_unlock_irqrestore(&priv->msgif_reg_lock, flags);
> +}
> +
> +static void pch_can_get_tx_enable(struct pch_can_priv *priv, u32 buff_num,
> + u32 *enable)
> +{
> + unsigned long flags;
> +
> + spin_lock_irqsave(&priv->msgif_reg_lock, flags);
> + iowrite32(CAN_CMASK_RX_TX_GET, &(priv->regs)->if1_cmask);
> + pch_can_check_if1_busy(priv, buff_num);
> +
> + if (((ioread32(&(priv->regs)->if1_id2)) & CAN_ID_MSGVAL) &&
> + ((ioread32(&(priv->regs)->if1_mcont)) &
> + CAN_IF_MCONT_TXIE)) {
> + *enable = ENABLE;
> + } else {
> + *enable = DISABLE;
> + }
> + spin_unlock_irqrestore(&priv->msgif_reg_lock, flags);
> +}
> +
> +static int pch_can_int_pending(struct pch_can_priv *priv)
> +{
> + return ioread32(&(priv->regs)->intr) & 0xffff;
> +}
> +
> +static void pch_can_set_rx_buffer_link(struct pch_can_priv *priv,
> + u32 buffer_num, u32 set)
> +{
> + unsigned long flags;
> +
> + spin_lock_irqsave(&priv->msgif_reg_lock, flags);
> + iowrite32(CAN_CMASK_RX_TX_GET, &(priv->regs)->if1_cmask);
> + pch_can_check_if1_busy(priv, buffer_num);
> + iowrite32((CAN_CMASK_RDWR | CAN_CMASK_CTRL), &(priv->regs)->if1_cmask);
> + if (set == ENABLE)
> + pch_can_bit_clear(&(priv->regs)->if1_mcont, CAN_IF_MCONT_EOB);
> + else
> + pch_can_bit_set(&(priv->regs)->if1_mcont, CAN_IF_MCONT_EOB);
> +
> + pch_can_check_if1_busy(priv, buffer_num);
> + spin_unlock_irqrestore(&priv->msgif_reg_lock, flags);
> +}
> +
> +static void pch_can_get_rx_buffer_link(struct pch_can_priv *priv,
> + u32 buffer_num, u32 *link)
> +{
> + u32 reg_val;
Really needed?
> + unsigned long flags;
> +
> + spin_lock_irqsave(&priv->msgif_reg_lock, flags);
> + iowrite32(CAN_CMASK_RX_TX_GET, &(priv->regs)->if1_cmask);
> + pch_can_check_if1_busy(priv, buffer_num);
> +
> + reg_val = ioread32(&(priv->regs)->if1_mcont);
> + if (reg_val & CAN_IF_MCONT_EOB)
> + *link = DISABLE;
> + else
> + *link = ENABLE;
> + spin_unlock_irqrestore(&priv->msgif_reg_lock, flags);
> +}
> +
> +static void pch_can_clear_buffers(struct pch_can_priv *priv)
> +{
> + u32 i;
> + u32 rx_buff_num;
> + u32 tx_buff_num;
Really needed?
> +
> + iowrite32(CAN_CMASK_RX_TX_SET, &(priv->regs)->if1_cmask);
> + iowrite32(CAN_CMASK_RX_TX_SET, &(priv->regs)->if2_cmask);
> + iowrite32(0xffff, &(priv->regs)->if1_mask1);
> + iowrite32(0xffff, &(priv->regs)->if1_mask2);
> + iowrite32(0xffff, &(priv->regs)->if2_mask1);
> + iowrite32(0xffff, &(priv->regs)->if2_mask2);
> +
> + iowrite32(0x0, &(priv->regs)->if1_id1);
> + iowrite32(0x0, &(priv->regs)->if1_id2);
> + iowrite32(0x0, &(priv->regs)->if2_id1);
> + iowrite32(0x0, &(priv->regs)->if2_id2);
> + iowrite32(0x0, &(priv->regs)->if1_mcont);
> + iowrite32(0x0, &(priv->regs)->if2_mcont);
> + iowrite32(0x0, &(priv->regs)->if1_dataa1);
> + iowrite32(0x0, &(priv->regs)->if1_dataa2);
> + iowrite32(0x0, &(priv->regs)->if1_datab1);
> + iowrite32(0x0, &(priv->regs)->if1_datab2);
> + iowrite32(0x0, &(priv->regs)->if2_dataa1);
> + iowrite32(0x0, &(priv->regs)->if2_dataa2);
> + iowrite32(0x0, &(priv->regs)->if2_datab1);
> + iowrite32(0x0, &(priv->regs)->if2_datab2);
> +
> + for (i = 1; i <= (MAX_MSG_OBJ / 2); i++) {
> + rx_buff_num = 2 * i;
> + tx_buff_num = (2 * i) - 1;
> +
> + iowrite32(rx_buff_num, &(priv->regs)->if1_creq);
> + iowrite32(tx_buff_num, &(priv->regs)->if2_creq);
> +
> + mdelay(10);
> + }
> +}
> +
> +static void pch_can_config_rx_tx_buffers(struct pch_can_priv *priv)
> +{
> + u32 i;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&priv->msgif_reg_lock, flags);
> + /* For accssing MsgVal, ID and EOB bit */
> + iowrite32((CAN_CMASK_RDWR | CAN_CMASK_ARB | CAN_CMASK_CTRL),
> + (&(priv->regs)->if1_cmask));
> + iowrite32((CAN_CMASK_RDWR | CAN_CMASK_ARB | CAN_CMASK_CTRL),
> + (&(priv->regs)->if2_cmask));
> + iowrite32(0x0, (&(priv->regs)->if1_id1));
> + iowrite32(0x0, (&(priv->regs)->if1_id2));
> +
> + /* Resetting DIR bit for reception */
> + iowrite32(0x0, (&(priv->regs)->if2_id1));
> +
> + /* Setting DIR bit for transmission */
> + iowrite32((CAN_ID2_DIR | (0x7ff << 2)),
> + (&(priv->regs)->if2_id2));
> +
> + /* Setting EOB bit for receiver */
> + iowrite32(CAN_IF_MCONT_EOB, &(priv->regs)->if1_mcont);
> +
> + /* Setting EOB bit for transmitter */
> + iowrite32(CAN_IF_MCONT_EOB, (&(priv->regs)->if2_mcont));
> +
> + for (i = 0; i < PCH_OBJ_NUM; i++) {
> + if (priv->msg_obj[i] == MSG_OBJ_RX)
> + pch_can_check_if1_busy(priv, i + 1);
> + else if (priv->msg_obj[i] == MSG_OBJ_TX)
> + pch_can_check_if2_busy(priv, i + 1);
> + else
> + dev_err(&priv->ndev->dev, "Invalid OBJ\n");
> + }
> +
> + for (i = 0; i < PCH_OBJ_NUM; i++) {
> + if (priv->msg_obj[i] == MSG_OBJ_RX) {
> + iowrite32(CAN_CMASK_RX_TX_GET,
> + &(priv->regs)->if1_cmask);
> + pch_can_check_if1_busy(priv, i+1);
> +
> + pch_can_bit_clear(&(priv->regs)->if1_id2, 0x1fff);
> + pch_can_bit_clear(&(priv->regs)->if1_id2, CAN_ID2_XTD);
Could'nt it be set just by one call?
> + iowrite32(0, (&(priv->regs)->if1_id1));
> + pch_can_bit_set(&(priv->regs)->if1_id2, 0);
> + pch_can_bit_set(&(priv->regs)->if1_mcont,
> + CAN_IF_MCONT_UMASK);
> + pch_can_bit_set(&(priv->regs)->if2_mcont,
> + CAN_IF_MCONT_UMASK);
> +
> + iowrite32(0, &(priv->regs)->if1_mask1);
> + pch_can_bit_clear(&(priv->regs)->if1_mask2, 0x1fff);
> + pch_can_bit_clear(&(priv->regs)->if1_mask2,
> + CAN_MASK2_MDIR_MXTD);
> +
> + iowrite32(0, &(priv->regs)->if2_mask1);
> + pch_can_bit_clear(&(priv->regs)->if2_mask2, 0x1fff);
> +
> + /* Setting CMASK for writing */
> + iowrite32((CAN_CMASK_RDWR | CAN_CMASK_MASK |
> + CAN_CMASK_ARB | CAN_CMASK_CTRL),
> + (&(priv->regs)->if1_cmask));
> +
> + pch_can_check_if1_busy(priv, i+1);
> + }
> + }
> + spin_unlock_irqrestore(&priv->msgif_reg_lock, flags);
> +}
> +
> +static void pch_can_open(struct pch_can_priv *priv)
Probably pch_can_init is the better name.
> +{
> + /* Stopping the Can device. */
> + pch_can_set_run_mode(priv, PCH_CAN_STOP);
> +
> + /* Clearing all the message object buffers. */
> + pch_can_clear_buffers(priv);
> +
> + /* Configuring the respective message object as either rx/tx object. */
> + pch_can_config_rx_tx_buffers(priv);
> +
> + /* Enabling all receive objects. */
> + pch_can_rx_enable_all(priv);
> +
> + /* Enabling all transmit objects. */
> + pch_can_tx_enable_all(priv);
> +
> + /* Enabling the interrupts. */
> + pch_can_set_int_enables(priv, PCH_CAN_ALL);
> +
> + /* Setting the CAN to run mode. */
> + pch_can_set_run_mode(priv, PCH_CAN_RUN);
Hm, you start the controller here... more later.
> +}
> +
> +static void pch_can_release(struct pch_can_priv *priv)
> +{
> + /* Stooping the CAN device. */
> + pch_can_set_run_mode(priv, PCH_CAN_STOP);
> +
> + /* Disabling the interrupts. */
> + pch_can_set_int_enables(priv, PCH_CAN_NONE);
> +
> + /* Disabling all the receive object. */
> + pch_can_rx_disable_all(priv);
> +
> + /* Disabling all the transmit object. */
> + pch_can_tx_disable_all(priv);
> +}
> +
> +/* This function clears interrupt(s) from the CAN device. */
> +static void pch_can_int_clr(struct pch_can_priv *priv, u32 mask)
> +{
> + if (mask == CAN_STATUS_INT) {
> + ioread32(&(priv->regs)->stat);
> + return;
> + }
> +
> + /* Clear interrupt for transmit object */
> + if (priv->msg_obj[mask - 1] == MSG_OBJ_TX) {
> + /* Setting CMASK for clearing interrupts for
> + frame transmission. */
> + iowrite32((CAN_CMASK_RDWR | CAN_CMASK_CTRL | CAN_CMASK_ARB),
> + (&(priv->regs)->if2_cmask));
> +
> + /* Resetting the ID registers. */
> + pch_can_bit_set(&(priv->regs)->if2_id2,
> + (CAN_ID2_DIR | (0x7ff << 2)));
> + iowrite32(0x0, (&(priv->regs)->if2_id1));
> +
> + /* Claring NewDat, TxRqst & IntPnd */
> + pch_can_bit_clear(&(priv->regs)->if2_mcont,
> + (CAN_IF_MCONT_NEWDAT | CAN_IF_MCONT_INTPND |
> + CAN_IF_MCONT_TXRQXT));
> + pch_can_check_if2_busy(priv, mask);
> + }
> + /* Clear interrupt for receive object */
> + else if (priv->msg_obj[mask - 1] == MSG_OBJ_RX) {
Should be "} else if ..."
> + /* Setting CMASK for clearing the reception interrupts. */
> + iowrite32((CAN_CMASK_RDWR | CAN_CMASK_CTRL | CAN_CMASK_ARB),
> + (&(priv->regs)->if2_cmask));
> +
> + /* Clearing the Dir bit. */
> + pch_can_bit_clear(&(priv->regs)->if2_id2, CAN_ID2_DIR);
> +
> + /* Clearing NewDat & IntPnd */
> + pch_can_bit_clear(&(priv->regs)->if2_mcont,
> + (CAN_IF_MCONT_NEWDAT | CAN_IF_MCONT_INTPND));
> +
> + pch_can_check_if2_busy(priv, mask);
> + }
> +}
> +
> +static int pch_can_get_buffer_status(struct pch_can_priv *priv)
> +{
> + u32 reg_treq1;
> + u32 reg_treq2;
Really needed?
> +
> + /* Reading the transmission request registers. */
> + reg_treq1 = (ioread32(&(priv->regs)->treq1) & 0xffff);
> + reg_treq2 = ((ioread32(&(priv->regs)->treq2) & 0xffff) << 16);
> +
> + return reg_treq1 | reg_treq2;
> +}
> +
> +static void pch_can_reset(struct pch_can_priv *priv)
> +{
> + /* write to sw reset register */
> + iowrite32(1, (&(priv->regs)->srst));
> + iowrite32(0, (&(priv->regs)->srst));
> +}
> +
> +static void pch_can_msg_obj(struct net_device *ndev, u32 status)
> +{
> + struct pch_can_priv *priv = netdev_priv(ndev);
> + u32 reg;
> + struct sk_buff *skb;
> + struct can_frame *cf;
> + canid_t id;
> + u32 ide;
> + u32 rtr;
> + int i, j;
> + struct net_device_stats *stats = &(priv->ndev->stats);
> +
> + /* Reading the messsage object from the Message RAM */
> + iowrite32(CAN_CMASK_RX_TX_GET, &(priv->regs)->if2_cmask);
> + pch_can_check_if2_busy(priv, status);
> +
> + /* Reading the MCONT register. */
> + reg = ioread32(&(priv->regs)->if2_mcont);
> + reg &= 0xffff;
> +
> + /* If MsgLost bit set. */
> + if (reg & CAN_IF_MCONT_MSGLOST) {
> + pch_can_bit_clear(&(priv->regs)->if2_mcont,
> + CAN_IF_MCONT_MSGLOST);
> + dev_err(&priv->ndev->dev, "Msg Obj is overwritten.\n");
That should create an error message as well.
> + }
> + /* Read the direction bit for determination of remote frame . */
> + rtr = (ioread32((&(priv->regs)->if2_id2)) & CAN_ID2_DIR);
> + /* Clearing interrupts. */
> + pch_can_int_clr(priv, status);
> + /* Hanlde reception interrupt */
Typo!
> + if (priv->msg_obj[status - 1] == MSG_OBJ_RX) {
> + if (!(reg & CAN_IF_MCONT_NEWDAT)) {
> + dev_err(&priv->ndev->dev, "MCONT_NEWDAT isn't SET.\n");
> + return;
> + }
> + skb = alloc_can_skb(priv->ndev, &cf);
> + if (!skb)
> + return;
> +
> + ide = ((ioread32(&(priv->regs)->if2_id2)) & CAN_ID2_XTD) >> 14;
> + if (ide) {
> + id = (ioread32(&(priv->regs)->if2_id1) & 0xffff);
> + id |= (((ioread32(&(priv->regs)->if2_id2)) &
> + 0x1fff) << 16);
> + cf->can_id = (id & CAN_EFF_MASK) | CAN_EFF_FLAG;
> + } else {
> + id = (((ioread32(&(priv->regs)->if2_id2)) &
> + (CAN_SFF_MASK << 2)) >> 2);
> + cf->can_id = (id & CAN_SFF_MASK);
> + }
> +
> + if (rtr) {
> + cf->can_dlc = 0;
> + cf->can_id |= CAN_RTR_FLAG;
> + } else {
> + cf->can_dlc = ((ioread32(&(priv->regs)->if2_mcont)) &
> + 0x0f);
> + }
> +
> + /* Reading back the data. */
> + for (i = 0, j = 0; i < cf->can_dlc; j++) {
> + reg = ioread32(&(priv->regs)->if2_dataa1 + j*4);
> + cf->data[i++] = cpu_to_le32(reg & 0xff);
> + if (i == cf->can_dlc)
> + break;
> + cf->data[i++] = cpu_to_le32((reg & (0xff << 8)) >> 8);
> + }
> + netif_rx(skb);
> + stats->rx_packets++;
> + stats->rx_bytes += cf->can_dlc;
> + } else if (priv->msg_obj[status - 1] == MSG_OBJ_TX) {
> + /* Hanlde transmission interrupt */
Typo!
> + can_get_echo_skb(priv->ndev, 0);
> + netif_wake_queue(priv->ndev);
> + }
> +}
> +
> +static void pch_can_error(struct net_device *ndev, u32 status)
> +{
> + struct sk_buff *skb;
> + struct pch_can_priv *priv = netdev_priv(ndev);
> + struct can_frame *cf;
> + u32 errc;
> + struct net_device_stats *stats = &(priv->ndev->stats);
> +
> + skb = alloc_can_err_skb(ndev, &cf);
> + if (!skb) {
> + dev_err(&ndev->dev, "%s -> No memory.\n", __func__);
Please drop the error message.
> + return;
> + }
> +
> + if (status & PCH_BUS_OFF) {
> + if (!priv->bus_off_interrupt) {
> + pch_can_tx_disable_all(priv);
> + pch_can_rx_disable_all(priv);
> +
> + priv->can.state = CAN_STATE_BUS_OFF;
> + cf->can_id |= CAN_ERR_BUSOFF;
> + can_bus_off(ndev);
> +
> + priv->bus_off_interrupt = 1;
> + pch_can_set_run_mode(priv, PCH_CAN_RUN);
Hm, you automatically restart the contoller after a bus-off. That's not
the intended behaviour. It's up to the user to define how and when the
device should recover from bus-off. For further information read
http://lxr.linux.no/#linux+v2.6.35.7/Documentation/networking/can.txt#L767
> + }
> + }
> + /* Warning interrupt. */
> + if (status & PCH_EWARN) {
> + priv->can.state = CAN_STATE_ERROR_WARNING;
> + priv->can.can_stats.error_warning++;
> + cf->can_id |= CAN_ERR_CRTL;
> + errc = ioread32((&(priv->regs)->errc));
> + if (((errc & CAN_REC) >> 8) > 96)
> + cf->data[1] |= CAN_ERR_CRTL_RX_WARNING;
> + if ((errc & CAN_TEC) > 96)
> + cf->data[1] |= CAN_ERR_CRTL_TX_WARNING;
> + dev_warn(&ndev->dev, "%s -> Warning interrupt.\n", __func__);
> + }
> + /* Error passive interrupt. */
> + if (status & PCH_EPASSIV) {
> + priv->can.can_stats.error_passive++;
> + priv->can.state = CAN_STATE_ERROR_PASSIVE;
> + cf->can_id |= CAN_ERR_CRTL;
> + errc = ioread32((&(priv->regs)->errc));
> + if (((errc & CAN_REC) >> 8) > 127)
> + cf->data[1] |= CAN_ERR_CRTL_RX_PASSIVE;
> + if ((errc & CAN_TEC) > 127)
> + cf->data[1] |= CAN_ERR_CRTL_TX_PASSIVE;
> + dev_err(&ndev->dev,
> + "%s -> Error interrupt.\n", __func__);
> + }
> +
> + if (status & PCH_STUF_ERR)
> + cf->data[2] |= CAN_ERR_PROT_STUFF;
> +
> + if (status & PCH_FORM_ERR)
> + cf->data[2] |= CAN_ERR_PROT_FORM;
> +
> + if (status & PCH_ACK_ERR)
> + cf->data[2] |= CAN_ERR_PROT_LOC_ACK | CAN_ERR_PROT_LOC_ACK_DEL;
> +
> + if ((status & PCH_BIT1_ERR) || (status & PCH_BIT0_ERR))
> + cf->data[2] |= CAN_ERR_PROT_BIT;
> +
> + if (status & PCH_CRC_ERR)
> + cf->data[2] |= CAN_ERR_PROT_LOC_CRC_SEQ |
> + CAN_ERR_PROT_LOC_CRC_DEL;
> +
> + if (status & PCH_LEC_ALL)
> + iowrite32(status | PCH_LEC_ALL,
> + &(priv->regs)->stat);
A bit-wise test of the above values is wrong, I believe. Please use the
switch statement instead.
> +
> + stats->rx_packets++;
> + stats->rx_bytes += cf->can_dlc;
> + netif_rx(skb);
> +}
> +
> +static irqreturn_t pch_can_handler(int irq, void *dev_id)
A better name making clear that it's the interrupt handler would be nice.
> +{
> + u32 int_stat;
> + u32 reg_stat;
> + struct net_device *ndev = (struct net_device *)dev_id;
> + struct pch_can_priv *priv = netdev_priv(ndev);
> + int_stat = pch_can_int_pending(priv);
> +
> + if (!int_stat)
> + return IRQ_NONE;
> +
> + if (int_stat == CAN_STATUS_INT) {
> + reg_stat = ioread32((&(priv->regs)->stat));
> + if (reg_stat & (PCH_BUS_OFF | PCH_LEC_ALL | PCH_EWARN |
> + PCH_EPASSIV)) {
> + if ((reg_stat & PCH_LEC_ALL) != PCH_LEC_ALL)
> + pch_can_error(ndev, reg_stat);
> + }
> +
> + /* Recover from Bus Off */
> + if (!reg_stat && priv->bus_off_interrupt) {
> + priv->bus_off_interrupt = 0;
> + pch_can_tx_enable_all(priv);
> + pch_can_rx_enable_all(priv);
> +
> + dev_info(&priv->ndev->dev, "BusOff stage recovered.\n");
Bogus bus-off handling, more later...
> + }
> +
> + if (reg_stat & PCH_RX_OK)
> + pch_can_bit_clear(&(priv->regs)->stat, PCH_RX_OK);
> +
> + if (reg_stat & PCH_TX_OK)
> + pch_can_bit_clear(&(priv->regs)->stat, PCH_TX_OK);
Could be done in one call, I think.
> + int_stat = pch_can_int_pending(priv);
> + }
> +
> + if ((int_stat > 0) && (int_stat <= MAX_MSG_OBJ))
> + pch_can_msg_obj(ndev, int_stat);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static int pch_set_bittiming(struct net_device *ndev)
> +{
> + struct pch_can_priv *priv = netdev_priv(ndev);
> + const struct can_bittiming *bt = &priv->can.bittiming;
> + u32 curr_mode;
> + u32 reg1; /* CANBIT */
> + u32 reg2; /* BEPE */
Why not "u32 canbit" then?
> + u32 brp;
> +
> + pch_can_get_run_mode(priv, &curr_mode);
> + if (curr_mode == PCH_CAN_RUN)
> + pch_can_set_run_mode(priv, PCH_CAN_STOP);
The device is stopped when this function is called. Please remove.
> +
> + /* Setting the CCE bit for accessing the Can Timing register. */
> + pch_can_bit_set(&(priv->regs)->cont, CAN_CTRL_CCE);
> +
> + brp = (bt->tq) / (1000000/PCH_CAN_CLK) - 1;
> + reg1 = brp & MSK_BITT_BRP;
> + reg1 |= (bt->sjw - 1) << BIT_BITT_SJW;
> + reg1 |= (bt->phase_seg1 + bt->prop_seg - 1) << BIT_BITT_TSEG1;
> + reg1 |= (bt->phase_seg2 - 1) << BIT_BITT_TSEG2;
> + reg2 = (brp & MSK_BRPE_BRPE) >> BIT_BRPE_BRPE;
> + iowrite32(reg1, (&(priv->regs)->bitt));
> + iowrite32(reg2, (&(priv->regs)->brpe));
> + pch_can_bit_clear(&(priv->regs)->cont, CAN_CTRL_CCE);
> +
> + if (curr_mode == PCH_CAN_RUN)
> + pch_can_set_run_mode(priv, PCH_CAN_RUN);
Ditto.
> + return 0;
> +}
> +
> +static void pch_can_start(struct net_device *ndev)
> +{
> + struct pch_can_priv *priv = netdev_priv(ndev);
> +
> + if (priv->can.state != CAN_STATE_STOPPED)
> + pch_can_reset(priv);
> +
> + pch_set_bittiming(ndev);
> + pch_can_set_optmode(priv);
> + priv->can.state = CAN_STATE_ERROR_ACTIVE;
Hm, where do you really start the controller. I'm missing
pch_can_set_run_mode(priv, PCH_CAN_RUN).
> + return;
> +}
> +
> +static int pch_can_do_set_mode(struct net_device *ndev, enum can_mode mode)
> +{
> + int ret = 0;
> +
> + switch (mode) {
> + case CAN_MODE_START:
> + pch_can_start(ndev);
> + netif_wake_queue(ndev);
> + break;
> + default:
> + ret = -EOPNOTSUPP;
> + break;
> + }
> +
> + return ret;
> +}
Note that this function is called when the device will recover from bus-off.
> +static int pch_can_get_state(const struct net_device *ndev,
> + enum can_state *state)
> +{
> + struct pch_can_priv *priv = netdev_priv(ndev);
> +
> + *state = priv->can.state;
> + return 0;
> +}
There is no need for that function as the driver handles state changes
in the interrupt handler.
> +static int pch_open(struct net_device *ndev)
That's confussing! Please use the prefix pch_can throught this file.
> +{
> + struct pch_can_priv *priv = netdev_priv(ndev);
> + int retval;
> +
> + pch_can_open(priv);
This function already starts the controller, which is too *early*.
> +
> + retval = pci_enable_msi(priv->dev);
> + if (retval) {
> + dev_err(&ndev->dev, "Unable to allocate MSI ret=%d\n", retval);
> + goto pci_en_msi_err;
> + }
> +
> + /* Regsitering the interrupt. */
> + retval = request_irq(priv->dev->irq, pch_can_handler, IRQF_SHARED,
> + ndev->name, ndev);
> + if (retval) {
> + dev_err(&ndev->dev, "request_irq failed.\n");
> + goto req_irq_err;
> + }
> +
> + /* Assuming that no bus off interrupt. */
> + priv->bus_off_interrupt = 0;
> +
> + /* Open common can device */
> + retval = open_candev(ndev);
> + if (retval) {
> + dev_err(ndev->dev.parent, "open_candev() failed %d\n", retval);
> + goto err_open_candev;
> + }
> +
> + pch_can_start(ndev);
Thde above function should finally start the controller.
> + netif_start_queue(ndev);
> +
> + return 0;
> +
> +err_open_candev:
> + free_irq(priv->dev->irq, ndev);
> +req_irq_err:
> + pci_disable_msi(priv->dev);
> +pci_en_msi_err:
> + pch_can_release(priv);
> +
> + return retval;
> +}
> +
> +static int pch_close(struct net_device *ndev)
> +{
> + struct pch_can_priv *priv = netdev_priv(ndev);
> +
> + netif_stop_queue(ndev);
> + pch_can_release(priv);
> + free_irq(priv->dev->irq, ndev);
> + pci_disable_msi(priv->dev);
> + close_candev(ndev);
> + priv->can.state = CAN_STATE_STOPPED;
> + return 0;
> +}
> +
> +static int pch_get_free_msg_obj(struct net_device *ndev)
> +{
> + u32 buffer_status = 0;
> + u32 tx_disable_counter = 0;
> + u32 tx_buffer_avail = 0;
> + u32 status;
> + s32 i;
> + struct pch_can_priv *priv = netdev_priv(ndev);
> +
> + /* Getting the message object status. */
> + buffer_status = (u32) pch_can_get_buffer_status(priv);
> +
> + /* Getting the free transmit message object. */
> + for (i = 0; i < PCH_OBJ_NUM; i++) {
> + if ((priv->msg_obj[i] == MSG_OBJ_TX)) {
> + /* Checking whether the object is enabled. */
> + pch_can_get_tx_enable(priv, i + 1, &status);
> + if (status == ENABLE) {
> + if (!((buffer_status >> i) & 1)) {
> + tx_buffer_avail = (i + 1);
> + break;
> + }
> + } else {
> + tx_disable_counter++;
> + }
> + }
> + }
> +
> + /* If no transmit object available. */
> + if (!tx_buffer_avail) {
> + /* If no object is enabled. */
> + if ((tx_disable_counter == PCH_TX_OBJ_NUM)) {
> + dev_err(&ndev->dev, "All tx buffers are disabled.\n");
> + return -EPERM;
> + } else {
> + dev_err(&ndev->dev, "%s:No tx buf free.\n", __func__);
> + return -PCH_CAN_NO_TX_BUFF;
> + }
> + }
> + return tx_buffer_avail;
> +}
> +
> +static netdev_tx_t pch_xmit(struct sk_buff *skb, struct net_device *ndev)
> +{
> + canid_t id;
> + u32 id1 = 0;
> + u32 id2 = 0;
Need these values to be preset?
> + u32 run_mode;
> + u32 i, j;
It's common to use type "int" for the usual incrementer value... as you
do in other places as well. Please check!
> + unsigned long flags;
> + struct pch_can_priv *priv = netdev_priv(ndev);
> + struct can_frame *cf = (struct can_frame *)skb->data;
> + struct net_device_stats *stats = &ndev->stats;
> + u32 tx_buffer_avail = 0;
> +
> + if (can_dropped_invalid_skb(ndev, skb))
> + return NETDEV_TX_OK;
> +
> + /* Getting the current CAN mode. */
> + pch_can_get_run_mode(priv, &run_mode);
> + if (run_mode != PCH_CAN_RUN) {
> + dev_err(&ndev->dev, "CAN stopped on transmit attempt.\n");
> + return -EPERM;
> + }
Can this happen? I think this check can be removed. Anyway, -EPERM is
not a valid return value for that function.
> +
> + tx_buffer_avail = pch_get_free_msg_obj(ndev);
> + if (tx_buffer_avail < 0)
> + return tx_buffer_avail;
Wrong return value?
> +
> + /* Attaining the lock. */
> + spin_lock_irqsave(&priv->msgif_reg_lock, flags);
> +
> + /* Reading the Msg Obj from the Msg RAM to the Interface register. */
> + iowrite32(CAN_CMASK_RX_TX_GET, &(priv->regs)->if1_cmask);
> + pch_can_check_if1_busy(priv, tx_buffer_avail);
> +
> + /* Setting the CMASK register. */
> + pch_can_bit_set(&(priv->regs)->if1_cmask, CAN_CMASK_ALL);
> +
> + /* If ID extended is set. */
> + if (cf->can_id & CAN_EFF_FLAG) {
> + id = cf->can_id & CAN_EFF_MASK;
> + id1 = id & 0xffff;
> + id2 = ((id & (0x1fff << 16)) >> 16) | CAN_ID2_XTD;
Please use some more macro definitions for the sake of readability.
> + } else {
> + id = cf->can_id & CAN_SFF_MASK;
> + id1 = 0;
> + id2 = ((id & CAN_SFF_MASK) << 2);
> + }
> + pch_can_bit_clear(&(priv->regs)->if1_id1, 0xffff);
> + pch_can_bit_clear(&(priv->regs)->if1_id2, 0x1fff | CAN_ID2_XTD);
> + pch_can_bit_set(&(priv->regs)->if1_id1, id1);
> + pch_can_bit_set(&(priv->regs)->if1_id2, id2);
> +
> + /* If remote frame has to be transmitted.. */
> + if (cf->can_id & CAN_RTR_FLAG)
> + pch_can_bit_clear(&(priv->regs)->if1_id2, CAN_ID2_DIR);
> +
> + for (i = 0, j = 0; i < cf->can_dlc; j++) {
> + iowrite32(le32_to_cpu(cf->data[i++]),
> + (&(priv->regs)->if1_dataa1) + j*4);
> + if (i == cf->can_dlc)
> + break;
> + iowrite32(le32_to_cpu(cf->data[i++] << 8),
> + (&(priv->regs)->if1_dataa1) + j*4);
> + }
> + can_put_echo_skb(skb, ndev, 0);
> +
> + /* Updating the size of the data. */
> + pch_can_bit_clear(&(priv->regs)->if1_mcont, 0x0f);
> + pch_can_bit_set(&(priv->regs)->if1_mcont, cf->can_dlc);
> +
> + /* Clearing IntPend, NewDat & TxRqst */
> + pch_can_bit_clear(&(priv->regs)->if1_mcont,
> + (CAN_IF_MCONT_NEWDAT | CAN_IF_MCONT_INTPND |
> + CAN_IF_MCONT_TXRQXT));
> +
> + /* Setting NewDat, TxRqst bits */
> + pch_can_bit_set(&(priv->regs)->if1_mcont,
> + (CAN_IF_MCONT_NEWDAT | CAN_IF_MCONT_TXRQXT));
> +
> + pch_can_check_if1_busy(priv, tx_buffer_avail);
> + spin_unlock_irqrestore(&priv->msgif_reg_lock, flags);
> +
> + stats->tx_bytes += cf->can_dlc;
> + stats->tx_packets++;
That shoould be incremented when the TX done interrupt is handled.
> +
> + return NETDEV_TX_OK;
> +}
> +
> +static const struct net_device_ops pch_can_netdev_ops = {
> + .ndo_open = pch_open,
> + .ndo_stop = pch_close,
> + .ndo_start_xmit = pch_xmit,
> +};
> +
> +static void __devexit pch_can_remove(struct pci_dev *pdev)
> +{
> + struct net_device *ndev = pci_get_drvdata(pdev);
> + struct pch_can_priv *priv = netdev_priv(ndev);
> +
> + unregister_candev(priv->ndev);
> + free_candev(priv->ndev);
> + pci_iounmap(pdev, priv->base);
> + pci_release_regions(pdev);
> + pci_disable_device(pdev);
> + pci_set_drvdata(pdev, NULL);
> + pch_can_reset(priv);
> +}
> +
> +#ifdef CONFIG_PM
> +static int pch_can_suspend(struct pci_dev *pdev, pm_message_t state)
> +{
> + int i; /* Counter variable. */
> + int retval; /* Return value. */
> + u32 buf_stat; /* Variable for reading the transmit buffer status. */
> + u32 counter = 0xFFFFFF;
> +
> + struct net_device *dev = pci_get_drvdata(pdev);
> + struct pch_can_priv *priv = netdev_priv(dev);
> +
> + /* Stop the CAN controller */
> + pch_can_set_run_mode(priv, PCH_CAN_STOP);
> +
> + /* Indicate that we are aboutto/in suspend */
> + priv->can.state = CAN_STATE_SLEEPING;
> +
> + /* Waiting for all transmission to complete. */
> + while (counter) {
> + buf_stat = pch_can_get_buffer_status(priv);
> + if (!buf_stat)
> + break;
> + counter--;
> + }
> + if (!counter)
> + dev_err(&pdev->dev, "%s -> Transmission time out.\n", __func__);
Timeout without defined delay!
> + /* Save interrupt configuration and then disable them */
> + pch_can_get_int_enables(priv, &(priv->int_enables));
> + pch_can_set_int_enables(priv, PCH_CAN_DISABLE);
> +
> + /* Save Tx buffer enable state */
> + for (i = 0; i < PCH_OBJ_NUM; i++) {
> + if (priv->msg_obj[i] == MSG_OBJ_TX)
> + pch_can_get_tx_enable(priv, (i + 1),
> + &(priv->tx_enable[i]));
> + }
> +
> + /* Disable all Transmit buffers */
> + pch_can_tx_disable_all(priv);
> +
> + /* Save Rx buffer enable state */
> + for (i = 0; i < PCH_OBJ_NUM; i++) {
> + if (priv->msg_obj[i] == MSG_OBJ_RX) {
> + pch_can_get_rx_enable(priv, (i + 1),
> + &(priv->rx_enable[i]));
> + pch_can_get_rx_buffer_link(priv, (i + 1),
> + &(priv->rx_link[i]));
> + }
> + }
> +
> + /* Disable all Receive buffers */
> + pch_can_rx_disable_all(priv);
> + retval = pci_save_state(pdev);
> + if (retval) {
> + dev_err(&pdev->dev, "pci_save_state failed.\n");
> + } else {
> + pci_enable_wake(pdev, PCI_D3hot, 0);
> + pci_disable_device(pdev);
> + pci_set_power_state(pdev, pci_choose_state(pdev, state));
> + }
> +
> + return retval;
> +}
> +
> +static int pch_can_resume(struct pci_dev *pdev)
> +{
> + int i; /* Counter variable. */
> + int retval; /* Return variable. */
> + struct net_device *dev = pci_get_drvdata(pdev);
> + struct pch_can_priv *priv = netdev_priv(dev);
> +
> + pci_set_power_state(pdev, PCI_D0);
> + pci_restore_state(pdev);
> + retval = pci_enable_device(pdev);
> + if (retval) {
> + dev_err(&pdev->dev, "pci_enable_device failed.\n");
> + return retval;
> + }
> +
> + pci_enable_wake(pdev, PCI_D3hot, 0);
> +
> + priv->can.state = CAN_STATE_ERROR_ACTIVE;
> +
> + /* Disabling all interrupts. */
> + pch_can_set_int_enables(priv, PCH_CAN_DISABLE);
> +
> + /* Setting the CAN device in Stop Mode. */
> + pch_can_set_run_mode(priv, PCH_CAN_STOP);
> +
> + /* Configuring the transmit and receive buffers. */
> + pch_can_config_rx_tx_buffers(priv);
> +
> + /* Restore the CAN state */
> + pch_set_bittiming(dev);
> +
> + /* Listen/Active */
> + pch_can_set_optmode(priv);
> +
> + /* Enabling the transmit buffer. */
> + for (i = 0; i < PCH_OBJ_NUM; i++) {
> + if (priv->msg_obj[i] == MSG_OBJ_TX) {
> + pch_can_set_tx_enable(priv, i + 1,
> + priv->tx_enable[i]);
> + }
> + }
> +
> + /* Configuring the receive buffer and enabling them. */
> + for (i = 0; i < PCH_OBJ_NUM; i++) {
> + if (priv->msg_obj[i] == MSG_OBJ_RX) {
> + /* Restore buffer link */
> + pch_can_set_rx_buffer_link(priv, i + 1,
> + priv->rx_link[i]);
> +
> + /* Restore buffer enables */
> + pch_can_set_rx_enable(priv, i + 1, priv->rx_enable[i]);
> + }
> + }
> +
> + /* Enable CAN Interrupts */
> + pch_can_set_int_custom(priv);
> +
> + /* Restore Run Mode */
> + pch_can_set_run_mode(priv, PCH_CAN_RUN);
> +
> + return retval;
> +}
Are the suspend and resume functions tested?
> +#else
> +#define pch_can_suspend NULL
> +#define pch_can_resume NULL
> +#endif
Add empty line here
> +static int __devinit pch_can_probe(struct pci_dev *pdev,
> + const struct pci_device_id *id)
> +{
> + struct net_device *ndev;
> + struct pch_can_priv *priv;
> + int rc;
> + int index;
> + void __iomem *addr;
> +
> + rc = pci_enable_device(pdev);
> + if (rc) {
> + dev_err(&pdev->dev, "Failed pci_enable_device %d\n", rc);
> + goto probe_exit_endev;
> + }
> +
> + rc = pci_request_regions(pdev, KBUILD_MODNAME);
> + if (rc) {
> + dev_err(&pdev->dev, "Failed pci_request_regions %d\n", rc);
> + goto probe_exit_pcireq;
> + }
> +
> + addr = pci_iomap(pdev, 1, 0);
> + if (!addr) {
> + rc = -EIO;
> + dev_err(&pdev->dev, "Failed pci_iomap\n");
> + goto probe_exit_ipmap;
> + }
> +
> + ndev = alloc_candev(sizeof(struct pch_can_priv), 1);
> + if (!ndev) {
> + rc = -ENOMEM;
> + dev_err(&pdev->dev, "Failed alloc_candev\n");
> + goto probe_exit_alloc_candev;
> + }
> +
> + priv = netdev_priv(ndev);
> + priv->ndev = ndev;
> + priv->base = addr;
> + priv->regs = addr;
> + priv->dev = pdev;
> + priv->can.bittiming_const = &pch_can_bittiming_const;
> + priv->can.do_set_mode = pch_can_do_set_mode;
> + priv->can.do_get_state = pch_can_get_state;
Not needed! See above.
Could you please also implement do_get_berr_counter().
> + priv->can.ctrlmode_supported = CAN_CTRLMODE_LISTENONLY |
> + CAN_CTRLMODE_LOOPBACK;
> + ndev->irq = pdev->irq;
> + ndev->flags |= IFF_ECHO;
> +
> + pci_set_drvdata(pdev, ndev);
> + SET_NETDEV_DEV(ndev, &pdev->dev);
> + ndev->netdev_ops = &pch_can_netdev_ops;
> +
> + priv->can.clock.freq = PCH_CAN_CLK * 1000; /* Hz to KHz) */
> + for (index = 0; index < PCH_RX_OBJ_NUM;)
> + priv->msg_obj[index++] = MSG_OBJ_RX;
> +
> + for (index = index; index < PCH_OBJ_NUM;)
> + priv->msg_obj[index++] = MSG_OBJ_TX;
> +
> + rc = register_candev(ndev);
> + if (rc) {
> + dev_err(&pdev->dev, "Failed register_candev %d\n", rc);
> + goto probe_exit_reg_candev;
> + }
> +
> + return 0;
> +
> +probe_exit_reg_candev:
> + free_candev(ndev);
> +probe_exit_alloc_candev:
> + pci_iounmap(pdev, addr);
> +probe_exit_ipmap:
> + pci_release_regions(pdev);
> +probe_exit_pcireq:
> + pci_disable_device(pdev);
> +probe_exit_endev:
> + return rc;
> +}
> +
> +static struct pci_driver pch_can_pcidev = {
> + .name = KBUILD_MODNAME,
> + .id_table = pch_can_pcidev_id,
> + .probe = pch_can_probe,
> + .remove = __devexit_p(pch_can_remove),
> + .suspend = pch_can_suspend,
> + .resume = pch_can_resume,
> +};
> +
> +static int __init pch_can_pci_init(void)
> +{
> + return pci_register_driver(&pch_can_pcidev);
> +}
> +module_init(pch_can_pci_init);
> +
> +static void __exit pch_can_pci_exit(void)
> +{
> + pci_unregister_driver(&pch_can_pcidev);
> +}
> +module_exit(pch_can_pci_exit);
> +
> +MODULE_DESCRIPTION("Controller Area Network Driver");
> +MODULE_LICENSE("GPL");
GPL v2 ?
> +MODULE_VERSION("0.94");
> +MODULE_DEVICE_TABLE(pci, pch_can_pcidev_id);
Please add it below the declaration of pch_can_pcidev_id.
In this driver you are using just *one* RX object. This means that the
CPU must handle new messages as quickly as possible otherwise message
losses will happen, right?. For sure, this will not make user's happy.
Any chance to use more RX objects in FIFO mode?
Thanks,
Wolfgang.
^ permalink raw reply
* [PATCH] Phonet: restore flow control credits when sending fails
From: Kumar A Sanghvi @ 2010-09-30 8:33 UTC (permalink / raw)
To: netdev, davem, remi.denis-courmont, eric.dumazet
Cc: gulshan.karmani, Kumar Sanghvi, Linus Walleij
From: Rémi Denis-Courmont <remi.denis-courmont@nokia.com>
This patch restores the below flow control patch submitted by Rémi
Denis-Courmont, which accidentaly got lost due to Pipe controller patch
on Phonet.
commit 1a98214feef2221cd7c24b17cd688a5a9d85b2ea
Author: Rémi Denis-Courmont <remi.denis-courmont@nokia.com>
Date: Mon Aug 30 12:57:03 2010 +0000
Phonet: restore flow control credits when sending fails
Signed-off-by: Rémi Denis-Courmont <remi.denis-courmont@nokia.com>
Signed-off-by: David S. Miller <davem@davemloft.net>
Signed-off-by: Kumar Sanghvi <kumar.sanghvi@stericsson.com>
Acked-by: Linus Walleij <linus.walleij@stericsson.com>
---
net/phonet/pep.c | 10 ++++++++--
1 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/net/phonet/pep.c b/net/phonet/pep.c
index 9746c6d..aa3d870 100644
--- a/net/phonet/pep.c
+++ b/net/phonet/pep.c
@@ -1289,6 +1289,7 @@ static int pipe_skb_send(struct sock *sk, struct sk_buff *skb)
{
struct pep_sock *pn = pep_sk(sk);
struct pnpipehdr *ph;
+ int err;
#ifdef CONFIG_PHONET_PIPECTRLR
struct sockaddr_pn spn = {
.spn_family = AF_PHONET,
@@ -1315,10 +1316,15 @@ static int pipe_skb_send(struct sock *sk, struct sk_buff *skb)
ph->message_id = PNS_PIPE_DATA;
ph->pipe_handle = pn->pipe_handle;
#ifdef CONFIG_PHONET_PIPECTRLR
- return pn_skb_send(sk, skb, &spn);
+ err = pn_skb_send(sk, skb, &spn);
#else
- return pn_skb_send(sk, skb, &pipe_srv);
+ err = pn_skb_send(sk, skb, &pipe_srv);
#endif
+
+ if (err && pn_flow_safe(pn->tx_fc))
+ atomic_inc(&pn->tx_credits);
+ return err;
+
}
static int pep_sendmsg(struct kiocb *iocb, struct sock *sk,
--
1.7.2.dirty
^ permalink raw reply related
* Re: [PATCH 4/4] Phonet: restore flow control credits when sending fails
From: Kumar SANGHVI @ 2010-09-30 8:31 UTC (permalink / raw)
To: netdev@vger.kernel.org, davem@davemloft.net,
remi.denis-courmont@nokia.com, "eric.dumazet@gmail.co
Cc: Gulshan KARMANI, Linus WALLEIJ
In-Reply-To: <1285835105-20293-1-git-send-email-kumar.sanghvi@stericsson.com>
Hi All,
On Thu, Sep 30, 2010 at 10:25:05 +0200, Kumar A SANGHVI wrote:
> From: Rémi Denis-Courmont <remi.denis-courmont@nokia.com>
>
> This patch restores the below flow control patch submitted by Rémi
> Denis-Courmont, which accidentaly got lost due to Pipe controller patch
> on Phonet.
>
> commit 1a98214feef2221cd7c24b17cd688a5a9d85b2ea
> Author: Rémi Denis-Courmont <remi.denis-courmont@nokia.com>
> Date: Mon Aug 30 12:57:03 2010 +0000
>
> Phonet: restore flow control credits when sending fails
>
> Signed-off-by: Rémi Denis-Courmont <remi.denis-courmont@nokia.com>
> Signed-off-by: David S. Miller <davem@davemloft.net>
>
> Signed-off-by: Kumar Sanghvi <kumar.sanghvi@stericsson.com>
> Acked-by: Linus Walleij <linus.walleij@stericsson.com>
Please discard this.
I will send a new patch.
Thanks,
Kumar.
^ permalink raw reply
* [PATCH 4/4] Phonet: restore flow control credits when sending fails
From: Kumar A Sanghvi @ 2010-09-30 8:25 UTC (permalink / raw)
To: netdev, davem, remi.denis-courmont, eric.dumazet
Cc: gulshan.karmani, Kumar Sanghvi, Linus Walleij
From: Rémi Denis-Courmont <remi.denis-courmont@nokia.com>
This patch restores the below flow control patch submitted by Rémi
Denis-Courmont, which accidentaly got lost due to Pipe controller patch
on Phonet.
commit 1a98214feef2221cd7c24b17cd688a5a9d85b2ea
Author: Rémi Denis-Courmont <remi.denis-courmont@nokia.com>
Date: Mon Aug 30 12:57:03 2010 +0000
Phonet: restore flow control credits when sending fails
Signed-off-by: Rémi Denis-Courmont <remi.denis-courmont@nokia.com>
Signed-off-by: David S. Miller <davem@davemloft.net>
Signed-off-by: Kumar Sanghvi <kumar.sanghvi@stericsson.com>
Acked-by: Linus Walleij <linus.walleij@stericsson.com>
---
net/phonet/pep.c | 10 ++++++++--
1 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/net/phonet/pep.c b/net/phonet/pep.c
index 9746c6d..aa3d870 100644
--- a/net/phonet/pep.c
+++ b/net/phonet/pep.c
@@ -1289,6 +1289,7 @@ static int pipe_skb_send(struct sock *sk, struct sk_buff *skb)
{
struct pep_sock *pn = pep_sk(sk);
struct pnpipehdr *ph;
+ int err;
#ifdef CONFIG_PHONET_PIPECTRLR
struct sockaddr_pn spn = {
.spn_family = AF_PHONET,
@@ -1315,10 +1316,15 @@ static int pipe_skb_send(struct sock *sk, struct sk_buff *skb)
ph->message_id = PNS_PIPE_DATA;
ph->pipe_handle = pn->pipe_handle;
#ifdef CONFIG_PHONET_PIPECTRLR
- return pn_skb_send(sk, skb, &spn);
+ err = pn_skb_send(sk, skb, &spn);
#else
- return pn_skb_send(sk, skb, &pipe_srv);
+ err = pn_skb_send(sk, skb, &pipe_srv);
#endif
+
+ if (err && pn_flow_safe(pn->tx_fc))
+ atomic_inc(&pn->tx_credits);
+ return err;
+
}
static int pep_sendmsg(struct kiocb *iocb, struct sock *sk,
--
1.7.2.dirty
^ permalink raw reply related
* Re: VLAN packets silently dropped in promiscuous mode
From: Roger Luethi @ 2010-09-30 8:07 UTC (permalink / raw)
To: Jesse Gross; +Cc: netdev, Patrick McHardy
In-Reply-To: <AANLkTi=QAoVH9KU8FAH6iscWt_mKnZpU3P9QqgFRLqXM@mail.gmail.com>
On Wed, 29 Sep 2010 10:44:26 -0700, Jesse Gross wrote:
> On Wed, Sep 29, 2010 at 4:37 AM, Roger Luethi <rl@hellgate.ch> wrote:
> > I noticed packets for unknown VLANs getting silently dropped even in
> > promiscuous mode (this is true only for the hardware accelerated path).
> > netif_nit_deliver was introduced specifically to prevent that, but the
> > function gets called only _after_ packets from unknown VLANs have been
> > dropped.
>
> Some drivers are fixing this on a case by case basis by disabling
> hardware accelerated VLAN stripping when in promiscuous mode, i.e.:
> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=5f6c01819979afbfec7e0b15fe52371b8eed87e8
>
> However, at this point it is more or less random which drivers do
> this. It would obviously be much better if it were consistent.
My understanding is this. Hardware VLAN tagging and stripping can always be
enabled. The kernel passes 802.1Q information along with the stripped
header to libpcap which reassembles the original header where necessary.
Works for me.
Hardware VLAN filtering, on the other hand, must be disabled in promiscuous
mode. But doing that in the driver makes no difference now as the current
VLAN code drops the packets so preserved before they are passed to the pcap
interface. That appears to be a bug.
Roger
^ permalink raw reply
* [net-next-2.6 PATCH 2/3] e1000e: reset PHY after errors detected on 82574 Si
From: Jeff Kirsher @ 2010-09-30 7:39 UTC (permalink / raw)
To: davem
Cc: netdev, gospo, bphilips, Jesse Brandeburg, Carolyn Wyborny,
Jeff Kirsher
In-Reply-To: <20100930073814.13378.4212.stgit@localhost.localdomain>
From: Carolyn Wyborny <carolyn.wyborny@intel.com>
Some errors can be induced in the PHY via environmental testing
(specifically extreme temperature changes and electro static
discharge testing), and in the case of the PHY hanging due to
this input, this detects the problem and resets to continue.
This issue only applies to 82574 silicon.
Signed-off-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
Signed-off-by: Carolyn Wyborny <carolyn.wyborny@intel.com>
Tested-by: Jeff Pieper <jeffrey.e.pieper@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
drivers/net/e1000e/82571.c | 40 +++++++++++++++++++++++++++++++++++++++-
drivers/net/e1000e/e1000.h | 3 +++
drivers/net/e1000e/netdev.c | 25 +++++++++++++++++++++++++
3 files changed, 67 insertions(+), 1 deletions(-)
diff --git a/drivers/net/e1000e/82571.c b/drivers/net/e1000e/82571.c
index ca663f1..db5ef6e 100644
--- a/drivers/net/e1000e/82571.c
+++ b/drivers/net/e1000e/82571.c
@@ -52,6 +52,10 @@
(ID_LED_DEF1_DEF2))
#define E1000_GCR_L1_ACT_WITHOUT_L0S_RX 0x08000000
+#define E1000_BASE1000T_STATUS 10
+#define E1000_IDLE_ERROR_COUNT_MASK 0xFF
+#define E1000_RECEIVE_ERROR_COUNTER 21
+#define E1000_RECEIVE_ERROR_MAX 0xFFFF
#define E1000_NVM_INIT_CTRL2_MNGM 0x6000 /* Manageability Operation Mode mask */
@@ -1243,6 +1247,39 @@ static s32 e1000_led_on_82574(struct e1000_hw *hw)
}
/**
+ * e1000_check_phy_82574 - check 82574 phy hung state
+ * @hw: pointer to the HW structure
+ *
+ * Returns whether phy is hung or not
+ **/
+bool e1000_check_phy_82574(struct e1000_hw *hw)
+{
+ u16 status_1kbt = 0;
+ u16 receive_errors = 0;
+ bool phy_hung = false;
+ s32 ret_val = 0;
+
+ /*
+ * Read PHY Receive Error counter first, if its is max - all F's then
+ * read the Base1000T status register If both are max then PHY is hung.
+ */
+ ret_val = e1e_rphy(hw, E1000_RECEIVE_ERROR_COUNTER, &receive_errors);
+
+ if (ret_val)
+ goto out;
+ if (receive_errors == E1000_RECEIVE_ERROR_MAX) {
+ ret_val = e1e_rphy(hw, E1000_BASE1000T_STATUS, &status_1kbt);
+ if (ret_val)
+ goto out;
+ if ((status_1kbt & E1000_IDLE_ERROR_COUNT_MASK) ==
+ E1000_IDLE_ERROR_COUNT_MASK)
+ phy_hung = true;
+ }
+out:
+ return phy_hung;
+}
+
+/**
* e1000_setup_link_82571 - Setup flow control and link settings
* @hw: pointer to the HW structure
*
@@ -1858,7 +1895,8 @@ struct e1000_info e1000_82574_info = {
| FLAG_RX_CSUM_ENABLED
| FLAG_HAS_SMART_POWER_DOWN
| FLAG_HAS_AMT
- | FLAG_HAS_CTRLEXT_ON_LOAD,
+ | FLAG_HAS_CTRLEXT_ON_LOAD
+ | FLAG2_CHECK_PHY_HANG,
.pba = 36,
.max_hw_frame_size = DEFAULT_JUMBO,
.get_variants = e1000_get_variants_82571,
diff --git a/drivers/net/e1000e/e1000.h b/drivers/net/e1000e/e1000.h
index 5ec0af5..da3b82f 100644
--- a/drivers/net/e1000e/e1000.h
+++ b/drivers/net/e1000e/e1000.h
@@ -397,6 +397,7 @@ struct e1000_adapter {
struct work_struct print_hang_task;
bool idle_check;
+ int phy_hang_count;
};
struct e1000_info {
@@ -453,6 +454,7 @@ struct e1000_info {
#define FLAG2_HAS_PHY_STATS (1 << 4)
#define FLAG2_HAS_EEE (1 << 5)
#define FLAG2_DMA_BURST (1 << 6)
+#define FLAG2_CHECK_PHY_HANG (1 << 7)
#define E1000_RX_DESC_PS(R, i) \
(&(((union e1000_rx_desc_packet_split *)((R).desc))[i]))
@@ -630,6 +632,7 @@ extern s32 e1000_get_phy_info_ife(struct e1000_hw *hw);
extern s32 e1000_check_polarity_ife(struct e1000_hw *hw);
extern s32 e1000_phy_force_speed_duplex_ife(struct e1000_hw *hw);
extern s32 e1000_check_polarity_igp(struct e1000_hw *hw);
+extern bool e1000_check_phy_82574(struct e1000_hw *hw);
static inline s32 e1000_phy_hw_reset(struct e1000_hw *hw)
{
diff --git a/drivers/net/e1000e/netdev.c b/drivers/net/e1000e/netdev.c
index 1aa4228..a8b55ab 100644
--- a/drivers/net/e1000e/netdev.c
+++ b/drivers/net/e1000e/netdev.c
@@ -4060,6 +4060,28 @@ static void e1000e_enable_receives(struct e1000_adapter *adapter)
}
}
+static void e1000e_check_82574_phy_workaround(struct e1000_adapter *adapter)
+{
+ struct e1000_hw *hw = &adapter->hw;
+
+ if (!(adapter->flags2 & FLAG2_CHECK_PHY_HANG))
+ return;
+
+ /*
+ * With 82574 controllers, PHY needs to be checked periodically
+ * for hung state and reset, if two calls return true
+ */
+ if (e1000_check_phy_82574(hw))
+ adapter->phy_hang_count++;
+ else
+ adapter->phy_hang_count = 0;
+
+ if (adapter->phy_hang_count > 1) {
+ adapter->phy_hang_count = 0;
+ schedule_work(&adapter->reset_task);
+ }
+}
+
/**
* e1000_watchdog - Timer Call-back
* @data: pointer to adapter cast into an unsigned long
@@ -4295,6 +4317,9 @@ link_up:
if (e1000e_get_laa_state_82571(hw))
e1000e_rar_set(hw, adapter->hw.mac.addr, 0);
+ if (adapter->flags2 & FLAG2_CHECK_PHY_HANG)
+ e1000e_check_82574_phy_workaround(adapter);
+
/* Reset the timer */
if (!test_bit(__E1000_DOWN, &adapter->state))
mod_timer(&adapter->watchdog_timer,
^ permalink raw reply related
* Re: [PATCH] net: Fix IPv6 PMTU disc. w/ asymmetric routes
From: David Miller @ 2010-09-30 7:41 UTC (permalink / raw)
To: zenczykowski; +Cc: netdev, yoshfuji
In-Reply-To: <AANLkTi=9THOcD9FieK3uy635C1kNDc=uEdtDe4qm1WU6@mail.gmail.com>
From: Maciej Żenczykowski <zenczykowski@gmail.com>
Date: Tue, 28 Sep 2010 15:37:26 -0700
> * I still think that handling the saddr == NULL ie. INADDR_ANY case is
> entirely superfluous, since it doesn't actually iterate through all
> possible source addresses. With IPv6 there can be many, many possible
> source addresses (just think of link local vs global public vs privacy
> addresses and then tack on 6to4 and mobility, etc... for example I see
> 13 ipv6 addresses on eth0 on my desktop at home, 12 of them globally
> reachable).
I only have %100 confidence in the reasoning behind why ipv4 handles
things this way, so I'll discuss this in those terms and then try
to tie it into the ipv6 side.
When we are looking up an ipv4 output route, there are 2 "source
address" objects.
1) The one specified in the "struct flowi" for the lookup
(the flp->fl4_src passed into ip_route_output_flow) which
is also the one that ends up in the routing cache entry's
->fl.fl4_src member.
2) The one contained in the routing cache entry's specification.
Ie. rth->rt_src
These are distinct. #1 is what is used to hash and find a matching
routing cache entry.
Since a source address of INADDR_ANY is allowed for routing lookups,
routing cache entries for the same daddr/saddr pair can exist in more
than one hash chains.
Therefore, if we didn't iterate over INADDR_ANY and the specific
address in the icmp PMTU message, we'd miss some routing cache
entries.
Look at the PMTU loops in ipv4 ip_rt_frag_needed():
for (k = 0; k < 2; k++) {
for (i = 0; i < 2; i++) {
unsigned hash = rt_hash(daddr, skeys[i], ikeys[k],
rt_genid(net));
("ANY vs. specific" ifindex and saddr are used for the hash
computation)
if (rth->fl.fl4_dst != daddr ||
rth->fl.fl4_src != skeys[i] ||
rth->rt_dst != daddr ||
rth->rt_src != iph->saddr ||
rth->fl.oif != ikeys[k] ||
rth->fl.iif != 0 ||
(and for the routing cache entry flow member comparisons)
But the routing cache entry "rt_src" member is compared always to
"iph->saddr", it doesn't use the "ANY vs. specific" skey[] value.
Unless ipv6 does not allow INADDR_ANY source address specifications
during route lookups, it ought to have the same issue too.
My understanding is that ipv6 uses a two-layered tree based scheme,
one layer to key off of the source address and one layer to key off
of the destination address.
So it seems to me that the lookups would have the same aliasing issue
that ipv4 does, and thus require checking both the specific saddr
and also the saddr INADDR_ANY.
Maybe the problem is that the ipv6 side uses the same saddr for both
the lookup and the entry comparison in these PMTU code paths? Does it
not allow specifying them seperately as the ipv4 PMTU (and incidently
the RT redirect) code paths do?
Or is this not an issue on the ipv6 side for some reason?
^ permalink raw reply
* [net-next-2.6 PATCH 3/3] e1000e: 82579 performance improvements
From: Jeff Kirsher @ 2010-09-30 7:39 UTC (permalink / raw)
To: davem; +Cc: netdev, gospo, bphilips, Bruce Allan, Jeff Kirsher
In-Reply-To: <20100930073814.13378.4212.stgit@localhost.localdomain>
From: Bruce Allan <bruce.w.allan@intel.com>
The initial support for 82579 was tuned poorly for performance. Adjust the
packet buffer allocation appropriately for both standard and jumbo frames;
and for jumbo frames increase the receive descriptor pre-fetch, disable
adaptive interrupt moderation and set the DMA latency tolerance.
Signed-off-by: Bruce Allan <bruce.w.allan@intel.com>
Tested-by: Jeff Pieper <jeffrey.e.pieper@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
drivers/net/e1000e/e1000.h | 1 +
drivers/net/e1000e/ich8lan.c | 2 +-
drivers/net/e1000e/netdev.c | 50 +++++++++++++++++++++++++++++++++++++-----
3 files changed, 46 insertions(+), 7 deletions(-)
diff --git a/drivers/net/e1000e/e1000.h b/drivers/net/e1000e/e1000.h
index da3b82f..8a79bbd 100644
--- a/drivers/net/e1000e/e1000.h
+++ b/drivers/net/e1000e/e1000.h
@@ -455,6 +455,7 @@ struct e1000_info {
#define FLAG2_HAS_EEE (1 << 5)
#define FLAG2_DMA_BURST (1 << 6)
#define FLAG2_CHECK_PHY_HANG (1 << 7)
+#define FLAG2_DISABLE_AIM (1 << 8)
#define E1000_RX_DESC_PS(R, i) \
(&(((union e1000_rx_desc_packet_split *)((R).desc))[i]))
diff --git a/drivers/net/e1000e/ich8lan.c b/drivers/net/e1000e/ich8lan.c
index 57b5435..e3374d9 100644
--- a/drivers/net/e1000e/ich8lan.c
+++ b/drivers/net/e1000e/ich8lan.c
@@ -3986,7 +3986,7 @@ struct e1000_info e1000_pch2_info = {
| FLAG_APME_IN_WUC,
.flags2 = FLAG2_HAS_PHY_STATS
| FLAG2_HAS_EEE,
- .pba = 18,
+ .pba = 26,
.max_hw_frame_size = DEFAULT_JUMBO,
.get_variants = e1000_get_variants_ich8lan,
.mac_ops = &ich8_mac_ops,
diff --git a/drivers/net/e1000e/netdev.c b/drivers/net/e1000e/netdev.c
index a8b55ab..c194804 100644
--- a/drivers/net/e1000e/netdev.c
+++ b/drivers/net/e1000e/netdev.c
@@ -2290,6 +2290,11 @@ static void e1000_set_itr(struct e1000_adapter *adapter)
goto set_itr_now;
}
+ if (adapter->flags2 & FLAG2_DISABLE_AIM) {
+ new_itr = 0;
+ goto set_itr_now;
+ }
+
adapter->tx_itr = e1000_update_itr(adapter,
adapter->tx_itr,
adapter->total_tx_packets,
@@ -2338,7 +2343,10 @@ set_itr_now:
if (adapter->msix_entries)
adapter->rx_ring->set_itr = 1;
else
- ew32(ITR, 1000000000 / (new_itr * 256));
+ if (new_itr)
+ ew32(ITR, 1000000000 / (new_itr * 256));
+ else
+ ew32(ITR, 0);
}
}
@@ -2920,7 +2928,7 @@ static void e1000_configure_rx(struct e1000_adapter *adapter)
/* irq moderation */
ew32(RADV, adapter->rx_abs_int_delay);
- if (adapter->itr_setting != 0)
+ if ((adapter->itr_setting != 0) && (adapter->itr != 0))
ew32(ITR, 1000000000 / (adapter->itr * 256));
ctrl_ext = er32(CTRL_EXT);
@@ -2965,11 +2973,13 @@ static void e1000_configure_rx(struct e1000_adapter *adapter)
* packet size is equal or larger than the specified value (in 8 byte
* units), e.g. using jumbo frames when setting to E1000_ERT_2048
*/
- if (adapter->flags & FLAG_HAS_ERT) {
+ if ((adapter->flags & FLAG_HAS_ERT) ||
+ (adapter->hw.mac.type == e1000_pch2lan)) {
if (adapter->netdev->mtu > ETH_DATA_LEN) {
u32 rxdctl = er32(RXDCTL(0));
ew32(RXDCTL(0), rxdctl | 0x3);
- ew32(ERT, E1000_ERT_2048 | (1 << 13));
+ if (adapter->flags & FLAG_HAS_ERT)
+ ew32(ERT, E1000_ERT_2048 | (1 << 13));
/*
* With jumbo frames and early-receive enabled,
* excessive C-state transition latencies result in
@@ -3232,9 +3242,35 @@ void e1000e_reset(struct e1000_adapter *adapter)
fc->low_water = 0x05048;
fc->pause_time = 0x0650;
fc->refresh_time = 0x0400;
+ if (adapter->netdev->mtu > ETH_DATA_LEN) {
+ pba = 14;
+ ew32(PBA, pba);
+ }
break;
}
+ /*
+ * Disable Adaptive Interrupt Moderation if 2 full packets cannot
+ * fit in receive buffer and early-receive not supported.
+ */
+ if (adapter->itr_setting & 0x3) {
+ if (((adapter->max_frame_size * 2) > (pba << 10)) &&
+ !(adapter->flags & FLAG_HAS_ERT)) {
+ if (!(adapter->flags2 & FLAG2_DISABLE_AIM)) {
+ dev_info(&adapter->pdev->dev,
+ "Interrupt Throttle Rate turned off\n");
+ adapter->flags2 |= FLAG2_DISABLE_AIM;
+ ew32(ITR, 0);
+ }
+ } else if (adapter->flags2 & FLAG2_DISABLE_AIM) {
+ dev_info(&adapter->pdev->dev,
+ "Interrupt Throttle Rate turned on\n");
+ adapter->flags2 &= ~FLAG2_DISABLE_AIM;
+ adapter->itr = 20000;
+ ew32(ITR, 1000000000 / (adapter->itr * 256));
+ }
+ }
+
/* Allow time for pending master requests to run */
mac->ops.reset_hw(hw);
@@ -3553,7 +3589,8 @@ static int e1000_open(struct net_device *netdev)
e1000_update_mng_vlan(adapter);
/* DMA latency requirement to workaround early-receive/jumbo issue */
- if (adapter->flags & FLAG_HAS_ERT)
+ if ((adapter->flags & FLAG_HAS_ERT) ||
+ (adapter->hw.mac.type == e1000_pch2lan))
pm_qos_add_request(&adapter->netdev->pm_qos_req,
PM_QOS_CPU_DMA_LATENCY,
PM_QOS_DEFAULT_VALUE);
@@ -3662,7 +3699,8 @@ static int e1000_close(struct net_device *netdev)
if (adapter->flags & FLAG_HAS_AMT)
e1000_release_hw_control(adapter);
- if (adapter->flags & FLAG_HAS_ERT)
+ if ((adapter->flags & FLAG_HAS_ERT) ||
+ (adapter->hw.mac.type == e1000_pch2lan))
pm_qos_remove_request(&adapter->netdev->pm_qos_req);
pm_runtime_put_sync(&pdev->dev);
^ permalink raw reply related
* [net-next-2.6 PATCH 1/3] e1000e: use hardware writeback batching
From: Jeff Kirsher @ 2010-09-30 7:38 UTC (permalink / raw)
To: davem; +Cc: netdev, gospo, bphilips, Jesse Brandeburg, Jeff Kirsher
From: Jesse Brandeburg <jesse.brandeburg@intel.com>
Most e1000e parts support batching writebacks. The problem with this is
that when some of the TADV or TIDV timers are not set, Tx can sit forever.
This is solved in this patch with write flushes using the Flush Partial
Descriptors (FPD) bit in TIDV and RDTR.
This improves bus utilization and removes partial writes on e1000e,
particularly from 82571 parts in S5500 chipset based machines.
Only ES2LAN and 82571/2 parts are included in this optimization, to reduce
testing load.
Signed-off-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
Tested-by: Emil Tantilov <emil.s.tantilov@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
drivers/net/e1000e/82571.c | 6 +++--
drivers/net/e1000e/defines.h | 2 ++
drivers/net/e1000e/e1000.h | 28 ++++++++++++++++++++++
drivers/net/e1000e/es2lan.c | 1 +
drivers/net/e1000e/netdev.c | 53 ++++++++++++++++++++++++++++++++++++++++++
drivers/net/e1000e/param.c | 2 --
6 files changed, 88 insertions(+), 4 deletions(-)
diff --git a/drivers/net/e1000e/82571.c b/drivers/net/e1000e/82571.c
index d3d4a57..ca663f1 100644
--- a/drivers/net/e1000e/82571.c
+++ b/drivers/net/e1000e/82571.c
@@ -1801,7 +1801,8 @@ struct e1000_info e1000_82571_info = {
| FLAG_RESET_OVERWRITES_LAA /* errata */
| FLAG_TARC_SPEED_MODE_BIT /* errata */
| FLAG_APME_CHECK_PORT_B,
- .flags2 = FLAG2_DISABLE_ASPM_L1, /* errata 13 */
+ .flags2 = FLAG2_DISABLE_ASPM_L1 /* errata 13 */
+ | FLAG2_DMA_BURST,
.pba = 38,
.max_hw_frame_size = DEFAULT_JUMBO,
.get_variants = e1000_get_variants_82571,
@@ -1819,7 +1820,8 @@ struct e1000_info e1000_82572_info = {
| FLAG_RX_CSUM_ENABLED
| FLAG_HAS_CTRLEXT_ON_LOAD
| FLAG_TARC_SPEED_MODE_BIT, /* errata */
- .flags2 = FLAG2_DISABLE_ASPM_L1, /* errata 13 */
+ .flags2 = FLAG2_DISABLE_ASPM_L1 /* errata 13 */
+ | FLAG2_DMA_BURST,
.pba = 38,
.max_hw_frame_size = DEFAULT_JUMBO,
.get_variants = e1000_get_variants_82571,
diff --git a/drivers/net/e1000e/defines.h b/drivers/net/e1000e/defines.h
index 93b3bed..d3f7a9c 100644
--- a/drivers/net/e1000e/defines.h
+++ b/drivers/net/e1000e/defines.h
@@ -446,7 +446,9 @@
/* Transmit Descriptor Control */
#define E1000_TXDCTL_PTHRESH 0x0000003F /* TXDCTL Prefetch Threshold */
+#define E1000_TXDCTL_HTHRESH 0x00003F00 /* TXDCTL Host Threshold */
#define E1000_TXDCTL_WTHRESH 0x003F0000 /* TXDCTL Writeback Threshold */
+#define E1000_TXDCTL_GRAN 0x01000000 /* TXDCTL Granularity */
#define E1000_TXDCTL_FULL_TX_DESC_WB 0x01010000 /* GRAN=1, WTHRESH=1 */
#define E1000_TXDCTL_MAX_TX_DESC_PREFETCH 0x0100001F /* GRAN=1, PTHRESH=31 */
/* Enable the counting of desc. still to be processed. */
diff --git a/drivers/net/e1000e/e1000.h b/drivers/net/e1000e/e1000.h
index f9a31c8..5ec0af5 100644
--- a/drivers/net/e1000e/e1000.h
+++ b/drivers/net/e1000e/e1000.h
@@ -153,6 +153,33 @@ struct e1000_info;
/* Time to wait before putting the device into D3 if there's no link (in ms). */
#define LINK_TIMEOUT 100
+#define DEFAULT_RDTR 0
+#define DEFAULT_RADV 8
+#define BURST_RDTR 0x20
+#define BURST_RADV 0x20
+
+/*
+ * in the case of WTHRESH, it appears at least the 82571/2 hardware
+ * writes back 4 descriptors when WTHRESH=5, and 3 descriptors when
+ * WTHRESH=4, and since we want 64 bytes at a time written back, set
+ * it to 5
+ */
+#define E1000_TXDCTL_DMA_BURST_ENABLE \
+ (E1000_TXDCTL_GRAN | /* set descriptor granularity */ \
+ E1000_TXDCTL_COUNT_DESC | \
+ (5 << 16) | /* wthresh must be +1 more than desired */\
+ (1 << 8) | /* hthresh */ \
+ 0x1f) /* pthresh */
+
+#define E1000_RXDCTL_DMA_BURST_ENABLE \
+ (0x01000000 | /* set descriptor granularity */ \
+ (4 << 16) | /* set writeback threshold */ \
+ (4 << 8) | /* set prefetch threshold */ \
+ 0x20) /* set hthresh */
+
+#define E1000_TIDV_FPD (1 << 31)
+#define E1000_RDTR_FPD (1 << 31)
+
enum e1000_boards {
board_82571,
board_82572,
@@ -425,6 +452,7 @@ struct e1000_info {
#define FLAG2_DISABLE_ASPM_L1 (1 << 3)
#define FLAG2_HAS_PHY_STATS (1 << 4)
#define FLAG2_HAS_EEE (1 << 5)
+#define FLAG2_DMA_BURST (1 << 6)
#define E1000_RX_DESC_PS(R, i) \
(&(((union e1000_rx_desc_packet_split *)((R).desc))[i]))
diff --git a/drivers/net/e1000e/es2lan.c b/drivers/net/e1000e/es2lan.c
index 45aebb4..24f8ac9 100644
--- a/drivers/net/e1000e/es2lan.c
+++ b/drivers/net/e1000e/es2lan.c
@@ -1494,6 +1494,7 @@ struct e1000_info e1000_es2_info = {
| FLAG_APME_CHECK_PORT_B
| FLAG_DISABLE_FC_PAUSE_TIME /* errata */
| FLAG_TIPG_MEDIUM_FOR_80003ESLAN,
+ .flags2 = FLAG2_DMA_BURST,
.pba = 38,
.max_hw_frame_size = DEFAULT_JUMBO,
.get_variants = e1000_get_variants_80003es2lan,
diff --git a/drivers/net/e1000e/netdev.c b/drivers/net/e1000e/netdev.c
index c69563c..1aa4228 100644
--- a/drivers/net/e1000e/netdev.c
+++ b/drivers/net/e1000e/netdev.c
@@ -2650,6 +2650,26 @@ static void e1000_configure_tx(struct e1000_adapter *adapter)
/* Tx irq moderation */
ew32(TADV, adapter->tx_abs_int_delay);
+ if (adapter->flags2 & FLAG2_DMA_BURST) {
+ u32 txdctl = er32(TXDCTL(0));
+ txdctl &= ~(E1000_TXDCTL_PTHRESH | E1000_TXDCTL_HTHRESH |
+ E1000_TXDCTL_WTHRESH);
+ /*
+ * set up some performance related parameters to encourage the
+ * hardware to use the bus more efficiently in bursts, depends
+ * on the tx_int_delay to be enabled,
+ * wthresh = 5 ==> burst write a cacheline (64 bytes) at a time
+ * hthresh = 1 ==> prefetch when one or more available
+ * pthresh = 0x1f ==> prefetch if internal cache 31 or less
+ * BEWARE: this seems to work but should be considered first if
+ * there are tx hangs or other tx related bugs
+ */
+ txdctl |= E1000_TXDCTL_DMA_BURST_ENABLE;
+ ew32(TXDCTL(0), txdctl);
+ /* erratum work around: set txdctl the same for both queues */
+ ew32(TXDCTL(1), txdctl);
+ }
+
/* Program the Transmit Control Register */
tctl = er32(TCTL);
tctl &= ~E1000_TCTL_CT;
@@ -2872,6 +2892,29 @@ static void e1000_configure_rx(struct e1000_adapter *adapter)
e1e_flush();
msleep(10);
+ if (adapter->flags2 & FLAG2_DMA_BURST) {
+ /*
+ * set the writeback threshold (only takes effect if the RDTR
+ * is set). set GRAN=1 and write back up to 0x4 worth, and
+ * enable prefetching of 0x20 rx descriptors
+ * granularity = 01
+ * wthresh = 04,
+ * hthresh = 04,
+ * pthresh = 0x20
+ */
+ ew32(RXDCTL(0), E1000_RXDCTL_DMA_BURST_ENABLE);
+ ew32(RXDCTL(1), E1000_RXDCTL_DMA_BURST_ENABLE);
+
+ /*
+ * override the delay timers for enabling bursting, only if
+ * the value was not set by the user via module options
+ */
+ if (adapter->rx_int_delay == DEFAULT_RDTR)
+ adapter->rx_int_delay = BURST_RDTR;
+ if (adapter->rx_abs_int_delay == DEFAULT_RADV)
+ adapter->rx_abs_int_delay = BURST_RADV;
+ }
+
/* set the Receive Delay Timer Register */
ew32(RDTR, adapter->rx_int_delay);
@@ -4235,6 +4278,16 @@ link_up:
/* Force detection of hung controller every watchdog period */
adapter->detect_tx_hung = 1;
+ /* flush partial descriptors to memory before detecting tx hang */
+ if (adapter->flags2 & FLAG2_DMA_BURST) {
+ ew32(TIDV, adapter->tx_int_delay | E1000_TIDV_FPD);
+ ew32(RDTR, adapter->rx_int_delay | E1000_RDTR_FPD);
+ /*
+ * no need to flush the writes because the timeout code does
+ * an er32 first thing
+ */
+ }
+
/*
* With 82571 controllers, LAA may be overwritten due to controller
* reset from the other port. Set the appropriate LAA in RAR[0]
diff --git a/drivers/net/e1000e/param.c b/drivers/net/e1000e/param.c
index 34aeec1..3d36911 100644
--- a/drivers/net/e1000e/param.c
+++ b/drivers/net/e1000e/param.c
@@ -91,7 +91,6 @@ E1000_PARAM(TxAbsIntDelay, "Transmit Absolute Interrupt Delay");
* Valid Range: 0-65535
*/
E1000_PARAM(RxIntDelay, "Receive Interrupt Delay");
-#define DEFAULT_RDTR 0
#define MAX_RXDELAY 0xFFFF
#define MIN_RXDELAY 0
@@ -101,7 +100,6 @@ E1000_PARAM(RxIntDelay, "Receive Interrupt Delay");
* Valid Range: 0-65535
*/
E1000_PARAM(RxAbsIntDelay, "Receive Absolute Interrupt Delay");
-#define DEFAULT_RADV 8
#define MAX_RXABSDELAY 0xFFFF
#define MIN_RXABSDELAY 0
^ permalink raw reply related
* [net-next-2.6 PATCH] ixgbe: fix link issues and panic with shared interrupts for 82598
From: Jeff Kirsher @ 2010-09-30 7:35 UTC (permalink / raw)
To: davem; +Cc: netdev, gospo, bphilips, Emil Tantilov, Jeff Kirsher
From: Emil Tantilov <emil.s.tantilov@intel.com>
Fix possible panic/hang with shared Legacy interrupts by not enabling
interrupts when interface is down.
Also fixes an intermittent link by enabling LSC upon exit from ixgbe_intr()
This patch adds flags to ixgbe_irq_enable() to allow for some flexibility
when enabling interrupts.
Signed-off-by: Emil Tantilov <emil.s.tantilov@intel.com>
Tested-by: Stephen Ko <stephen.s.ko@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
drivers/net/ixgbe/ixgbe_main.c | 36 ++++++++++++++++++++++++++----------
1 files changed, 26 insertions(+), 10 deletions(-)
diff --git a/drivers/net/ixgbe/ixgbe_main.c b/drivers/net/ixgbe/ixgbe_main.c
index c35185c..c35e13c 100644
--- a/drivers/net/ixgbe/ixgbe_main.c
+++ b/drivers/net/ixgbe/ixgbe_main.c
@@ -2233,7 +2233,8 @@ static void ixgbe_set_itr(struct ixgbe_adapter *adapter)
* ixgbe_irq_enable - Enable default interrupt generation settings
* @adapter: board private structure
**/
-static inline void ixgbe_irq_enable(struct ixgbe_adapter *adapter)
+static inline void ixgbe_irq_enable(struct ixgbe_adapter *adapter, bool queues,
+ bool flush)
{
u32 mask;
@@ -2254,8 +2255,10 @@ static inline void ixgbe_irq_enable(struct ixgbe_adapter *adapter)
mask |= IXGBE_EIMS_FLOW_DIR;
IXGBE_WRITE_REG(&adapter->hw, IXGBE_EIMS, mask);
- ixgbe_irq_enable_queues(adapter, ~0);
- IXGBE_WRITE_FLUSH(&adapter->hw);
+ if (queues)
+ ixgbe_irq_enable_queues(adapter, ~0);
+ if (flush)
+ IXGBE_WRITE_FLUSH(&adapter->hw);
if (adapter->num_vfs > 32) {
u32 eitrsel = (1 << (adapter->num_vfs - 32)) - 1;
@@ -2277,7 +2280,7 @@ static irqreturn_t ixgbe_intr(int irq, void *data)
u32 eicr;
/*
- * Workaround for silicon errata. Mask the interrupts
+ * Workaround for silicon errata on 82598. Mask the interrupts
* before the read of EICR.
*/
IXGBE_WRITE_REG(hw, IXGBE_EIMC, IXGBE_IRQ_CLEAR_MASK);
@@ -2286,10 +2289,15 @@ static irqreturn_t ixgbe_intr(int irq, void *data)
* therefore no explict interrupt disable is necessary */
eicr = IXGBE_READ_REG(hw, IXGBE_EICR);
if (!eicr) {
- /* shared interrupt alert!
+ /*
+ * shared interrupt alert!
* make sure interrupts are enabled because the read will
- * have disabled interrupts due to EIAM */
- ixgbe_irq_enable(adapter);
+ * have disabled interrupts due to EIAM
+ * finish the workaround of silicon errata on 82598. Unmask
+ * the interrupt that we masked before the EICR read.
+ */
+ if (!test_bit(__IXGBE_DOWN, &adapter->state))
+ ixgbe_irq_enable(adapter, true, true);
return IRQ_NONE; /* Not our interrupt */
}
@@ -2313,6 +2321,14 @@ static irqreturn_t ixgbe_intr(int irq, void *data)
__napi_schedule(&(q_vector->napi));
}
+ /*
+ * re-enable link(maybe) and non-queue interrupts, no flush.
+ * ixgbe_poll will re-enable the queue interrupts
+ */
+
+ if (!test_bit(__IXGBE_DOWN, &adapter->state))
+ ixgbe_irq_enable(adapter, false, false);
+
return IRQ_HANDLED;
}
@@ -3048,7 +3064,7 @@ static void ixgbe_vlan_rx_kill_vid(struct net_device *netdev, u16 vid)
vlan_group_set_device(adapter->vlgrp, vid, NULL);
if (!test_bit(__IXGBE_DOWN, &adapter->state))
- ixgbe_irq_enable(adapter);
+ ixgbe_irq_enable(adapter, true, true);
/* remove VID from filter table */
hw->mac.ops.set_vfta(&adapter->hw, vid, pool_ndx, false);
@@ -3145,7 +3161,7 @@ static void ixgbe_vlan_rx_register(struct net_device *netdev,
ixgbe_vlan_rx_add_vid(netdev, 0);
if (!test_bit(__IXGBE_DOWN, &adapter->state))
- ixgbe_irq_enable(adapter);
+ ixgbe_irq_enable(adapter, true, true);
}
static void ixgbe_restore_vlan(struct ixgbe_adapter *adapter)
@@ -3546,7 +3562,7 @@ static int ixgbe_up_complete(struct ixgbe_adapter *adapter)
/* clear any pending interrupts, may auto mask */
IXGBE_READ_REG(hw, IXGBE_EICR);
- ixgbe_irq_enable(adapter);
+ ixgbe_irq_enable(adapter, true, true);
/*
* If this adapter has a fan, check to see if we had a failure
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox