* Re: [PATCH 4/4] Ethtool: convert get_tx_csum/set_tx_csum calls to hw_features flags
From: Michał Mirosław @ 2010-11-02 1:23 UTC (permalink / raw)
To: Ben Hutchings
Cc: netdev, e1000-devel, Steve Glendinning, Greg Kroah-Hartman,
Rasesh Mody, Debashis Dutt, Kristoffer Glembo, linux-driver,
linux-net-drivers
In-Reply-To: <1288647537.2231.81.camel@achroite.uk.solarflarecom.com>
On Mon, Nov 01, 2010 at 09:38:57PM +0000, Ben Hutchings wrote:
> On Sun, 2010-10-31 at 02:09 +0200, Michał Mirosław wrote:
> [...]
> > diff --git a/drivers/net/dm9000.c b/drivers/net/dm9000.c
> > index 9f6aeef..5ba4535 100644
> > --- a/drivers/net/dm9000.c
> > +++ b/drivers/net/dm9000.c
> > @@ -132,7 +132,6 @@ typedef struct board_info {
> > u32 wake_state;
> >
> > int rx_csum;
> > - int can_csum;
> > int ip_summed;
> > } board_info_t;
> >
> > @@ -480,7 +479,7 @@ static int dm9000_set_rx_csum_unlocked(struct net_device *dev, uint32_t data)
> > {
> > board_info_t *dm = to_dm9000_board(dev);
> >
> > - if (dm->can_csum) {
> > + if (dev->hw_features & NETIF_F_IP_CSUM) {
>
> This is a bit ugly because can_csum is being used to indicate RX
> checksum offload capability whereas the checksum feature flags logically
> indicate TX checksum offload capability. Of course, the two hardware
> features are highly correlated!
I briefly looked over the code. can_csum looked like "can do both TX
and RX checksum", so (dev->hw_features & NETIF_F_IP_CSUM) duplicates
information in this case. I can remove this change or just add a comment
that we abuse hw_features a bit.
> [...]
> > diff --git a/net/core/ethtool.c b/net/core/ethtool.c
> > index 9b0e598..95e8b7a 100644
> > --- a/net/core/ethtool.c
> > +++ b/net/core/ethtool.c
> [...]
> > @@ -1077,12 +1039,18 @@ static int __ethtool_set_sg(struct net_device *dev, u32 data)
> > return 0;
> > }
> >
> > +static u32 ethtool_get_tx_csum(struct net_device *dev)
> > +{
> > + return (dev->features & (NETIF_F_ALL_CSUM|NETIF_F_SCTP_CSUM)) != 0;
> > +}
> > +
> [...]
>
> It seems to me that NETIF_F_SCTP_CSUM should be added to
> NETIF_F_ALL_CSUM, though that may cause problems in some other places
> NETIF_F_ALL_CSUM is used.
This would need auditing NETIF_F_ALL_CSUM usage across the kernel, so
definitely a further patch material.
Best Regards,
Michał Mirosław
^ permalink raw reply
* Re: [PATCH 0/4] Ethtool: cleanup strategy
From: Michał Mirosław @ 2010-11-02 1:30 UTC (permalink / raw)
To: Ben Hutchings
Cc: netdev, e1000-devel, Steve Glendinning, Greg Kroah-Hartman,
Rasesh Mody, Debashis Dutt, Kristoffer Glembo, linux-driver,
linux-net-drivers
In-Reply-To: <20101102011422.GM15074@solarflare.com>
On Tue, Nov 02, 2010 at 01:14:23AM +0000, Ben Hutchings wrote:
> Michał Mirosław wrote:
> > On Mon, Nov 01, 2010 at 09:05:25PM +0000, Ben Hutchings wrote:
> [...]
> > > It also might be worth defining a standard feature flag for RX checksum
> > > offload, since currently every driver has to maintain its own private
> > > flag. Though we're running short of feature flags on 32-bit machines.
> > RX offloads are different in that most devices allow readback of
> > configured bits, so actually no specific flags are needed.
> [...]
> Most devices also need resetting from time to time, so those flags still
> need to be included in software state.
Ah. Good point. So this should be converted to the same way the TX offloads
will be handled.
Is it worth to split rx and tx features to separate bitfields? Especially
that we're running out of bits in u32 and that TX flags are used in hot
path and RX are normally not accessed much.
Are the features flags exported somewhere to userspace except via ethtool?
Best Regards,
Michał Mirosław
^ permalink raw reply
* Re: [PATCH 2/3] offloading: Support multiple vlan tags in GSO.
From: Jesse Gross @ 2010-11-02 1:31 UTC (permalink / raw)
To: Ben Hutchings; +Cc: David Miller, netdev
In-Reply-To: <1288643630.2231.30.camel@achroite.uk.solarflarecom.com>
On Mon, Nov 1, 2010 at 1:33 PM, Ben Hutchings <bhutchings@solarflare.com> wrote:
> On Fri, 2010-10-29 at 15:14 -0700, Jesse Gross wrote:
>> We assume that hardware TSO can't support multiple levels of vlan tags
>> but we allow it to be done. Therefore, enable GSO to parse these tags
>> so we can fallback to software.
>>
>> Signed-off-by: Jesse Gross <jesse@nicira.com>
>> CC: Ben Hutchings <bhutchings@solarflare.com>
> [...]
>
> I can't see how TSO would be enabled on a second-level VLAN device;
> presumably you're thinking about GSO being enabled there?
Right, TSO can't be enabled on a stacked vlan device. However, there
are a few ways that I can think of to hit this code path. One is GSO,
another is encapsulating a bridged packet that is already tagged.
These are definitely corner cases and I can think of a few other
caveats when using them. Mostly I'm just thinking about making it
robust in the face of weird corner cases, one piece at a time at
least.
^ permalink raw reply
* Re: bonding: flow control regression [was Re: bridging: flow control regression]
From: Simon Horman @ 2010-11-02 2:06 UTC (permalink / raw)
To: Eric Dumazet; +Cc: netdev, Jay Vosburgh, David S. Miller
In-Reply-To: <1288616372.2660.101.camel@edumazet-laptop>
On Mon, Nov 01, 2010 at 01:59:32PM +0100, Eric Dumazet wrote:
> Le lundi 01 novembre 2010 à 21:29 +0900, Simon Horman a écrit :
> > Hi,
> >
> > I have observed what appears to be a regression between 2.6.34 and
> > 2.6.35-rc1. The behaviour described below is still present in Linus's
> > current tree (2.6.36+).
> >
> > On 2.6.34 and earlier when sending a UDP stream to a bonded interface
> > the throughput is approximately equal to the available physical bandwidth.
> >
> > # netperf -c -4 -t UDP_STREAM -H 172.17.50.253 -l 30 -- -m 1472
> > UDP UNIDIRECTIONAL SEND TEST from 0.0.0.0 (0.0.0.0) port 0 AF_INET to
> > 172.17.50.253 (172.17.50.253) port 0 AF_INET
> > Socket Message Elapsed Messages CPU Service
> > Size Size Time Okay Errors Throughput Util Demand
> > bytes bytes secs # # 10^6bits/sec % SU us/KB
> >
> > 114688 1472 30.00 2438265 0 957.1 18.09 3.159
> > 109568 30.00 2389980 938.1 -1.00 -1.000
> >
> > On 2.6.35-rc1 netpref sends~7Gbits/s.
> > Curiously it only consumes 50% CPU, I would expect this to be CPU bound.
> >
> > # netperf -c -4 -t UDP_STREAM -H 172.17.50.253 -l 30 -- -m 1472
> > UDP UNIDIRECTIONAL SEND TEST from 0.0.0.0 (0.0.0.0) port 0 AF_INET to
> > 172.17.50.253 (172.17.50.253) port 0 AF_INET
> > Socket Message Elapsed Messages CPU Service
> > Size Size Time Okay Errors Throughput Util Demand
> > bytes bytes secs # # 10^6bits/sec % SU us/KB
> >
> > 116736 1472 30.00 18064360 0 7090.8 50.62 8.665
> > 109568 30.00 2438090 957.0 -1.00 -1.000
> >
> > In this case the bonding device has a single gitabit slave device
> > and is running in balance-rr mode. I have observed similar results
> > with two and three slave devices.
> >
> > I have bisected the problem and the offending commit appears to be
> > "net: Introduce skb_orphan_try()". My tired eyes tell me that change
> > frees skb's earlier than they otherwise would be unless tx timestamping
> > is in effect. That does seem to make sense in relation to this problem,
> > though I am yet to dig into specifically why bonding is adversely affected.
> >
>
> I assume you meant "bonding: flow control regression", ie this is not
> related to bridging ?
Yes, sorry about that. I meant bonding not bridging.
> One problem on bonding is that the xmit() method always returns
> NETDEV_TX_OK.
>
> So a flooder cannot know some of its frames were lost.
>
> So yes, the patch you mention has the effect of allowing UDP to flood
> bonding device, since we orphan skb before giving it to device (bond or
> ethX)
>
> With a normal device (with a qdisc), we queue skb, and orphan it only
> when leaving queue. With a not too big socket send buffer, it slows down
> the sender enough to "send UDP frames at line rate only"
Thanks for the explanation.
I'm not entirely sure how much of a problem this is in practice.
^ permalink raw reply
* Re: [PATCH 2/4] Ethtool: convert get_sg/set_sg calls to hw_features flag
From: Matt Carlson @ 2010-11-02 2:24 UTC (permalink / raw)
To: Micha?? Miros??aw
Cc: linux-net-drivers@solarflare.com, Debashis Dutt,
e1000-devel@lists.sourceforge.net, netdev@vger.kernel.org,
Greg Kroah-Hartman, Rasesh Mody, linux-driver@qlogic.com,
Steve Glendinning, Kristoffer Glembo
In-Reply-To: <9d89236b6e4ff8c66937fbd7d8ce76602e680c5b.1288496404.git.mirq-linux@rere.qmqm.pl>
On Fri, Oct 29, 2010 at 09:28:26PM -0700, Micha?? Miros??aw wrote:
> diff --git a/drivers/net/tg3.c b/drivers/net/tg3.c
> index 30ccbb6..b07e2d1 100644
> --- a/drivers/net/tg3.c
> +++ b/drivers/net/tg3.c
> @@ -11306,7 +11306,6 @@ static const struct ethtool_ops tg3_ethtool_ops = {
> .get_rx_csum = tg3_get_rx_csum,
> .set_rx_csum = tg3_set_rx_csum,
> .set_tx_csum = tg3_set_tx_csum,
> - .set_sg = ethtool_op_set_sg,
> .set_tso = tg3_set_tso,
> .self_test = tg3_self_test,
> .get_strings = tg3_get_strings,
> @@ -14681,6 +14680,7 @@ static int __devinit tg3_init_one(struct pci_dev *pdev,
> tp->rx_pending = TG3_DEF_RX_RING_PENDING;
> tp->rx_jumbo_pending = TG3_DEF_RX_JUMBO_RING_PENDING;
>
> + dev->hw_features |= NETIF_F_SG;
Scatter-gather should not be enabled if TG3_FLAG_BROKEN_CHECKSUMS is set. I
would do the following instead:
if (!(tp->tg3_flags & TG3_FLAG_BROKEN_CHECKSUMS))
dev->hw_features |= NETIF_F_SG;
TG3_FLAG_BROKEN_CHECKSUMS is set in tg3_get_invariants(), so this code
would need to be placed later than that function call.
> dev->ethtool_ops = &tg3_ethtool_ops;
> dev->watchdog_timeo = TG3_TX_TIMEOUT;
> dev->irq = pdev->irq;
------------------------------------------------------------------------------
Nokia and AT&T present the 2010 Calling All Innovators-North America contest
Create new apps & games for the Nokia N8 for consumers in U.S. and Canada
$10 million total in prizes - $4M cash, 500 devices, nearly $6M in marketing
Develop with Nokia Qt SDK, Web Runtime, or Java and Publish to Ovi Store
http://p.sf.net/sfu/nokia-dev2dev
_______________________________________________
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: [PATCH 3/4] Ethtool: convert get_tso/set_tso calls to hw_features flags
From: Matt Carlson @ 2010-11-02 2:49 UTC (permalink / raw)
To: Micha?? Miros??aw
Cc: netdev@vger.kernel.org, e1000-devel@lists.sourceforge.net,
Steve Glendinning, Greg Kroah-Hartman, Rasesh Mody, Debashis Dutt,
Kristoffer Glembo, linux-driver@qlogic.com,
linux-net-drivers@solarflare.com
In-Reply-To: <db0a20ab6e4ab3f63d782bd2c4a9f7a873ea4a64.1288496404.git.mirq-linux@rere.qmqm.pl>
On Sat, Oct 30, 2010 at 01:44:17AM -0700, Micha?? Miros??aw wrote:
> diff --git a/drivers/net/tg3.c b/drivers/net/tg3.c
> index b07e2d1..c08172d 100644
> --- a/drivers/net/tg3.c
> +++ b/drivers/net/tg3.c
> @@ -6142,7 +6142,7 @@ static inline void tg3_set_mtu(struct net_device *dev, struct tg3 *tp,
> if (new_mtu > ETH_DATA_LEN) {
> if (tp->tg3_flags2 & TG3_FLG2_5780_CLASS) {
> tp->tg3_flags2 &= ~TG3_FLG2_TSO_CAPABLE;
> - ethtool_op_set_tso(dev, 0);
> + dev->features &= ~NETIF_F_ALL_TSO;
> } else {
> tp->tg3_flags |= TG3_FLAG_JUMBO_RING_ENABLE;
> }
> @@ -9977,27 +9977,28 @@ static int tg3_set_tso(struct net_device *dev, u32 value)
> {
> struct tg3 *tp = netdev_priv(dev);
>
> - if (!(tp->tg3_flags2 & TG3_FLG2_TSO_CAPABLE)) {
> - if (value)
> - return -EINVAL;
> + if (!(tp->tg3_flags2 & TG3_FLG2_TSO_CAPABLE) && value)
> + return -EINVAL;
> +
> + if (!value)
> return 0;
> - }
> +
> + dev->features |= NETIF_F_TSO;
> +
> if ((dev->features & NETIF_F_IPV6_CSUM) &&
> ((tp->tg3_flags2 & TG3_FLG2_HW_TSO_2) ||
> (tp->tg3_flags2 & TG3_FLG2_HW_TSO_3))) {
> - if (value) {
> - dev->features |= NETIF_F_TSO6;
> - if ((tp->tg3_flags2 & TG3_FLG2_HW_TSO_3) ||
> - GET_ASIC_REV(tp->pci_chip_rev_id) == ASIC_REV_5761 ||
> - (GET_ASIC_REV(tp->pci_chip_rev_id) == ASIC_REV_5784 &&
> - GET_CHIP_REV(tp->pci_chip_rev_id) != CHIPREV_5784_AX) ||
> - GET_ASIC_REV(tp->pci_chip_rev_id) == ASIC_REV_5785 ||
> - GET_ASIC_REV(tp->pci_chip_rev_id) == ASIC_REV_57780)
> - dev->features |= NETIF_F_TSO_ECN;
> - } else
> - dev->features &= ~(NETIF_F_TSO6 | NETIF_F_TSO_ECN);
> + dev->features |= NETIF_F_TSO6;
> + if ((tp->tg3_flags2 & TG3_FLG2_HW_TSO_3) ||
> + GET_ASIC_REV(tp->pci_chip_rev_id) == ASIC_REV_5761 ||
> + (GET_ASIC_REV(tp->pci_chip_rev_id) == ASIC_REV_5784 &&
> + GET_CHIP_REV(tp->pci_chip_rev_id) != CHIPREV_5784_AX) ||
> + GET_ASIC_REV(tp->pci_chip_rev_id) == ASIC_REV_5785 ||
> + GET_ASIC_REV(tp->pci_chip_rev_id) == ASIC_REV_57780)
> + dev->features |= NETIF_F_TSO_ECN;
> }
> - return ethtool_op_set_tso(dev, value);
> +
> + return 1;
dev->hw_features looks to me like it should function as a set of flags
indicating what the hardware is currently capable of doing. It would
clean the above code a lot if we could do:
dev->features |= (dev->hw_features & NETIF_F_ALL_TSO);
In fact, the new member might serve as a replacement for the
TG3_FLG2_TSO_CAPABLE flag. Let's not do that right now though,
because it requires some careful attention in other code paths which would
be a distraction. I'll follow-up with a patch to do this.
> }
>
> static int tg3_nway_reset(struct net_device *dev)
> @@ -11306,7 +11307,7 @@ static const struct ethtool_ops tg3_ethtool_ops = {
> .get_rx_csum = tg3_get_rx_csum,
> .set_rx_csum = tg3_set_rx_csum,
> .set_tx_csum = tg3_set_tx_csum,
> - .set_tso = tg3_set_tso,
> + .hw_set_tso = tg3_set_tso,
> .self_test = tg3_self_test,
> .get_strings = tg3_get_strings,
> .phys_id = tg3_phys_id,
> @@ -14681,6 +14682,7 @@ static int __devinit tg3_init_one(struct pci_dev *pdev,
> tp->rx_jumbo_pending = TG3_DEF_RX_JUMBO_RING_PENDING;
>
> dev->hw_features |= NETIF_F_SG;
> + dev->hw_features |= NETIF_F_TSO|NETIF_F_TSO6|NETIF_F_TSO_ECN;
Not all devices are TSO capable either. There is code later in this
function that determines which TSO flags to set. I would probably reuse
that code.
> dev->ethtool_ops = &tg3_ethtool_ops;
> dev->watchdog_timeo = TG3_TX_TIMEOUT;
> dev->irq = pdev->irq;
^ permalink raw reply
* [PATCH] net: sh_eth: ctrl_in/outX to __raw_read/writeX conversion.
From: Nobuhiro Iwamatsu @ 2010-11-02 2:51 UTC (permalink / raw)
To: netdev; +Cc: lethal, Nobuhiro Iwamatsu
The ctrl_xxx routines are deprecated, switch over to the __raw_xxx
versions.
Signed-off-by: Nobuhiro Iwamatsu <nobuhiro.iwamatsu.yj@renesas.com>
---
drivers/net/sh_eth.c | 244 +++++++++++++++++++++++++-------------------------
1 files changed, 122 insertions(+), 122 deletions(-)
diff --git a/drivers/net/sh_eth.c b/drivers/net/sh_eth.c
index 50259df..7229f5d 100644
--- a/drivers/net/sh_eth.c
+++ b/drivers/net/sh_eth.c
@@ -45,9 +45,9 @@ static void sh_eth_set_duplex(struct net_device *ndev)
u32 ioaddr = ndev->base_addr;
if (mdp->duplex) /* Full */
- ctrl_outl(ctrl_inl(ioaddr + ECMR) | ECMR_DM, ioaddr + ECMR);
+ __raw_writel(__raw_readl(ioaddr + ECMR) | ECMR_DM, ioaddr + ECMR);
else /* Half */
- ctrl_outl(ctrl_inl(ioaddr + ECMR) & ~ECMR_DM, ioaddr + ECMR);
+ __raw_writel(__raw_readl(ioaddr + ECMR) & ~ECMR_DM, ioaddr + ECMR);
}
static void sh_eth_set_rate(struct net_device *ndev)
@@ -57,10 +57,10 @@ static void sh_eth_set_rate(struct net_device *ndev)
switch (mdp->speed) {
case 10: /* 10BASE */
- ctrl_outl(ctrl_inl(ioaddr + ECMR) & ~ECMR_RTM, ioaddr + ECMR);
+ __raw_writel(__raw_readl(ioaddr + ECMR) & ~ECMR_RTM, ioaddr + ECMR);
break;
case 100:/* 100BASE */
- ctrl_outl(ctrl_inl(ioaddr + ECMR) | ECMR_RTM, ioaddr + ECMR);
+ __raw_writel(__raw_readl(ioaddr + ECMR) | ECMR_RTM, ioaddr + ECMR);
break;
default:
break;
@@ -96,9 +96,9 @@ static void sh_eth_set_duplex(struct net_device *ndev)
u32 ioaddr = ndev->base_addr;
if (mdp->duplex) /* Full */
- ctrl_outl(ctrl_inl(ioaddr + ECMR) | ECMR_DM, ioaddr + ECMR);
+ __raw_writel(__raw_readl(ioaddr + ECMR) | ECMR_DM, ioaddr + ECMR);
else /* Half */
- ctrl_outl(ctrl_inl(ioaddr + ECMR) & ~ECMR_DM, ioaddr + ECMR);
+ __raw_writel(__raw_readl(ioaddr + ECMR) & ~ECMR_DM, ioaddr + ECMR);
}
static void sh_eth_set_rate(struct net_device *ndev)
@@ -108,10 +108,10 @@ static void sh_eth_set_rate(struct net_device *ndev)
switch (mdp->speed) {
case 10: /* 10BASE */
- ctrl_outl(0, ioaddr + RTRATE);
+ __raw_writel(0, ioaddr + RTRATE);
break;
case 100:/* 100BASE */
- ctrl_outl(1, ioaddr + RTRATE);
+ __raw_writel(1, ioaddr + RTRATE);
break;
default:
break;
@@ -143,7 +143,7 @@ static struct sh_eth_cpu_data sh_eth_my_cpu_data = {
static void sh_eth_chip_reset(struct net_device *ndev)
{
/* reset device */
- ctrl_outl(ARSTR_ARSTR, ARSTR);
+ __raw_writel(ARSTR_ARSTR, ARSTR);
mdelay(1);
}
@@ -152,10 +152,10 @@ static void sh_eth_reset(struct net_device *ndev)
u32 ioaddr = ndev->base_addr;
int cnt = 100;
- ctrl_outl(EDSR_ENALL, ioaddr + EDSR);
- ctrl_outl(ctrl_inl(ioaddr + EDMR) | EDMR_SRST, ioaddr + EDMR);
+ __raw_writel(EDSR_ENALL, ioaddr + EDSR);
+ __raw_writel(__raw_readl(ioaddr + EDMR) | EDMR_SRST, ioaddr + EDMR);
while (cnt > 0) {
- if (!(ctrl_inl(ioaddr + EDMR) & 0x3))
+ if (!(__raw_readl(ioaddr + EDMR) & 0x3))
break;
mdelay(1);
cnt--;
@@ -164,14 +164,14 @@ static void sh_eth_reset(struct net_device *ndev)
printk(KERN_ERR "Device reset fail\n");
/* Table Init */
- ctrl_outl(0x0, ioaddr + TDLAR);
- ctrl_outl(0x0, ioaddr + TDFAR);
- ctrl_outl(0x0, ioaddr + TDFXR);
- ctrl_outl(0x0, ioaddr + TDFFR);
- ctrl_outl(0x0, ioaddr + RDLAR);
- ctrl_outl(0x0, ioaddr + RDFAR);
- ctrl_outl(0x0, ioaddr + RDFXR);
- ctrl_outl(0x0, ioaddr + RDFFR);
+ __raw_writel(0x0, ioaddr + TDLAR);
+ __raw_writel(0x0, ioaddr + TDFAR);
+ __raw_writel(0x0, ioaddr + TDFXR);
+ __raw_writel(0x0, ioaddr + TDFFR);
+ __raw_writel(0x0, ioaddr + RDLAR);
+ __raw_writel(0x0, ioaddr + RDFAR);
+ __raw_writel(0x0, ioaddr + RDFXR);
+ __raw_writel(0x0, ioaddr + RDFFR);
}
static void sh_eth_set_duplex(struct net_device *ndev)
@@ -180,9 +180,9 @@ static void sh_eth_set_duplex(struct net_device *ndev)
u32 ioaddr = ndev->base_addr;
if (mdp->duplex) /* Full */
- ctrl_outl(ctrl_inl(ioaddr + ECMR) | ECMR_DM, ioaddr + ECMR);
+ __raw_writel(__raw_readl(ioaddr + ECMR) | ECMR_DM, ioaddr + ECMR);
else /* Half */
- ctrl_outl(ctrl_inl(ioaddr + ECMR) & ~ECMR_DM, ioaddr + ECMR);
+ __raw_writel(__raw_readl(ioaddr + ECMR) & ~ECMR_DM, ioaddr + ECMR);
}
static void sh_eth_set_rate(struct net_device *ndev)
@@ -192,13 +192,13 @@ static void sh_eth_set_rate(struct net_device *ndev)
switch (mdp->speed) {
case 10: /* 10BASE */
- ctrl_outl(GECMR_10, ioaddr + GECMR);
+ __raw_writel(GECMR_10, ioaddr + GECMR);
break;
case 100:/* 100BASE */
- ctrl_outl(GECMR_100, ioaddr + GECMR);
+ __raw_writel(GECMR_100, ioaddr + GECMR);
break;
case 1000: /* 1000BASE */
- ctrl_outl(GECMR_1000, ioaddr + GECMR);
+ __raw_writel(GECMR_1000, ioaddr + GECMR);
break;
default:
break;
@@ -283,9 +283,9 @@ static void sh_eth_reset(struct net_device *ndev)
{
u32 ioaddr = ndev->base_addr;
- ctrl_outl(ctrl_inl(ioaddr + EDMR) | EDMR_SRST, ioaddr + EDMR);
+ __raw_writel(__raw_readl(ioaddr + EDMR) | EDMR_SRST, ioaddr + EDMR);
mdelay(3);
- ctrl_outl(ctrl_inl(ioaddr + EDMR) & ~EDMR_SRST, ioaddr + EDMR);
+ __raw_writel(__raw_readl(ioaddr + EDMR) & ~EDMR_SRST, ioaddr + EDMR);
}
#endif
@@ -336,10 +336,10 @@ static void update_mac_address(struct net_device *ndev)
{
u32 ioaddr = ndev->base_addr;
- ctrl_outl((ndev->dev_addr[0] << 24) | (ndev->dev_addr[1] << 16) |
+ __raw_writel((ndev->dev_addr[0] << 24) | (ndev->dev_addr[1] << 16) |
(ndev->dev_addr[2] << 8) | (ndev->dev_addr[3]),
ioaddr + MAHR);
- ctrl_outl((ndev->dev_addr[4] << 8) | (ndev->dev_addr[5]),
+ __raw_writel((ndev->dev_addr[4] << 8) | (ndev->dev_addr[5]),
ioaddr + MALR);
}
@@ -358,12 +358,12 @@ static void read_mac_address(struct net_device *ndev, unsigned char *mac)
if (mac[0] || mac[1] || mac[2] || mac[3] || mac[4] || mac[5]) {
memcpy(ndev->dev_addr, mac, 6);
} else {
- ndev->dev_addr[0] = (ctrl_inl(ioaddr + MAHR) >> 24);
- ndev->dev_addr[1] = (ctrl_inl(ioaddr + MAHR) >> 16) & 0xFF;
- ndev->dev_addr[2] = (ctrl_inl(ioaddr + MAHR) >> 8) & 0xFF;
- ndev->dev_addr[3] = (ctrl_inl(ioaddr + MAHR) & 0xFF);
- ndev->dev_addr[4] = (ctrl_inl(ioaddr + MALR) >> 8) & 0xFF;
- ndev->dev_addr[5] = (ctrl_inl(ioaddr + MALR) & 0xFF);
+ ndev->dev_addr[0] = (__raw_readl(ioaddr + MAHR) >> 24);
+ ndev->dev_addr[1] = (__raw_readl(ioaddr + MAHR) >> 16) & 0xFF;
+ ndev->dev_addr[2] = (__raw_readl(ioaddr + MAHR) >> 8) & 0xFF;
+ ndev->dev_addr[3] = (__raw_readl(ioaddr + MAHR) & 0xFF);
+ ndev->dev_addr[4] = (__raw_readl(ioaddr + MALR) >> 8) & 0xFF;
+ ndev->dev_addr[5] = (__raw_readl(ioaddr + MALR) & 0xFF);
}
}
@@ -379,19 +379,19 @@ struct bb_info {
/* PHY bit set */
static void bb_set(u32 addr, u32 msk)
{
- ctrl_outl(ctrl_inl(addr) | msk, addr);
+ __raw_writel(__raw_readl(addr) | msk, addr);
}
/* PHY bit clear */
static void bb_clr(u32 addr, u32 msk)
{
- ctrl_outl((ctrl_inl(addr) & ~msk), addr);
+ __raw_writel((__raw_readl(addr) & ~msk), addr);
}
/* PHY bit read */
static int bb_read(u32 addr, u32 msk)
{
- return (ctrl_inl(addr) & msk) != 0;
+ return (__raw_readl(addr) & msk) != 0;
}
/* Data I/O pin control */
@@ -506,9 +506,9 @@ static void sh_eth_ring_format(struct net_device *ndev)
rxdesc->buffer_length = ALIGN(mdp->rx_buf_sz, 16);
/* Rx descriptor address set */
if (i == 0) {
- ctrl_outl(mdp->rx_desc_dma, ioaddr + RDLAR);
+ __raw_writel(mdp->rx_desc_dma, ioaddr + RDLAR);
#if defined(CONFIG_CPU_SUBTYPE_SH7763)
- ctrl_outl(mdp->rx_desc_dma, ioaddr + RDFAR);
+ __raw_writel(mdp->rx_desc_dma, ioaddr + RDFAR);
#endif
}
}
@@ -528,9 +528,9 @@ static void sh_eth_ring_format(struct net_device *ndev)
txdesc->buffer_length = 0;
if (i == 0) {
/* Tx descriptor address set */
- ctrl_outl(mdp->tx_desc_dma, ioaddr + TDLAR);
+ __raw_writel(mdp->tx_desc_dma, ioaddr + TDLAR);
#if defined(CONFIG_CPU_SUBTYPE_SH7763)
- ctrl_outl(mdp->tx_desc_dma, ioaddr + TDFAR);
+ __raw_writel(mdp->tx_desc_dma, ioaddr + TDFAR);
#endif
}
}
@@ -623,71 +623,71 @@ static int sh_eth_dev_init(struct net_device *ndev)
/* Descriptor format */
sh_eth_ring_format(ndev);
if (mdp->cd->rpadir)
- ctrl_outl(mdp->cd->rpadir_value, ioaddr + RPADIR);
+ __raw_writel(mdp->cd->rpadir_value, ioaddr + RPADIR);
/* all sh_eth int mask */
- ctrl_outl(0, ioaddr + EESIPR);
+ __raw_writel(0, ioaddr + EESIPR);
#if defined(__LITTLE_ENDIAN__)
if (mdp->cd->hw_swap)
- ctrl_outl(EDMR_EL, ioaddr + EDMR);
+ __raw_writel(EDMR_EL, ioaddr + EDMR);
else
#endif
- ctrl_outl(0, ioaddr + EDMR);
+ __raw_writel(0, ioaddr + EDMR);
/* FIFO size set */
- ctrl_outl(mdp->cd->fdr_value, ioaddr + FDR);
- ctrl_outl(0, ioaddr + TFTR);
+ __raw_writel(mdp->cd->fdr_value, ioaddr + FDR);
+ __raw_writel(0, ioaddr + TFTR);
/* Frame recv control */
- ctrl_outl(mdp->cd->rmcr_value, ioaddr + RMCR);
+ __raw_writel(mdp->cd->rmcr_value, ioaddr + RMCR);
rx_int_var = mdp->rx_int_var = DESC_I_RINT8 | DESC_I_RINT5;
tx_int_var = mdp->tx_int_var = DESC_I_TINT2;
- ctrl_outl(rx_int_var | tx_int_var, ioaddr + TRSCER);
+ __raw_writel(rx_int_var | tx_int_var, ioaddr + TRSCER);
if (mdp->cd->bculr)
- ctrl_outl(0x800, ioaddr + BCULR); /* Burst sycle set */
+ __raw_writel(0x800, ioaddr + BCULR); /* Burst sycle set */
- ctrl_outl(mdp->cd->fcftr_value, ioaddr + FCFTR);
+ __raw_writel(mdp->cd->fcftr_value, ioaddr + FCFTR);
if (!mdp->cd->no_trimd)
- ctrl_outl(0, ioaddr + TRIMD);
+ __raw_writel(0, ioaddr + TRIMD);
/* Recv frame limit set register */
- ctrl_outl(RFLR_VALUE, ioaddr + RFLR);
+ __raw_writel(RFLR_VALUE, ioaddr + RFLR);
- ctrl_outl(ctrl_inl(ioaddr + EESR), ioaddr + EESR);
- ctrl_outl(mdp->cd->eesipr_value, ioaddr + EESIPR);
+ __raw_writel(__raw_readl(ioaddr + EESR), ioaddr + EESR);
+ __raw_writel(mdp->cd->eesipr_value, ioaddr + EESIPR);
/* PAUSE Prohibition */
- val = (ctrl_inl(ioaddr + ECMR) & ECMR_DM) |
+ val = (__raw_readl(ioaddr + ECMR) & ECMR_DM) |
ECMR_ZPF | (mdp->duplex ? ECMR_DM : 0) | ECMR_TE | ECMR_RE;
- ctrl_outl(val, ioaddr + ECMR);
+ __raw_writel(val, ioaddr + ECMR);
if (mdp->cd->set_rate)
mdp->cd->set_rate(ndev);
/* E-MAC Status Register clear */
- ctrl_outl(mdp->cd->ecsr_value, ioaddr + ECSR);
+ __raw_writel(mdp->cd->ecsr_value, ioaddr + ECSR);
/* E-MAC Interrupt Enable register */
- ctrl_outl(mdp->cd->ecsipr_value, ioaddr + ECSIPR);
+ __raw_writel(mdp->cd->ecsipr_value, ioaddr + ECSIPR);
/* Set MAC address */
update_mac_address(ndev);
/* mask reset */
if (mdp->cd->apr)
- ctrl_outl(APR_AP, ioaddr + APR);
+ __raw_writel(APR_AP, ioaddr + APR);
if (mdp->cd->mpr)
- ctrl_outl(MPR_MP, ioaddr + MPR);
+ __raw_writel(MPR_MP, ioaddr + MPR);
if (mdp->cd->tpauser)
- ctrl_outl(TPAUSER_UNLIMITED, ioaddr + TPAUSER);
+ __raw_writel(TPAUSER_UNLIMITED, ioaddr + TPAUSER);
/* Setting the Rx mode will start the Rx process. */
- ctrl_outl(EDRRR_R, ioaddr + EDRRR);
+ __raw_writel(EDRRR_R, ioaddr + EDRRR);
netif_start_queue(ndev);
@@ -811,8 +811,8 @@ static int sh_eth_rx(struct net_device *ndev)
/* Restart Rx engine if stopped. */
/* If we don't need to check status, don't. -KDU */
- if (!(ctrl_inl(ndev->base_addr + EDRRR) & EDRRR_R))
- ctrl_outl(EDRRR_R, ndev->base_addr + EDRRR);
+ if (!(__raw_readl(ndev->base_addr + EDRRR) & EDRRR_R))
+ __raw_writel(EDRRR_R, ndev->base_addr + EDRRR);
return 0;
}
@@ -827,8 +827,8 @@ static void sh_eth_error(struct net_device *ndev, int intr_status)
u32 mask;
if (intr_status & EESR_ECI) {
- felic_stat = ctrl_inl(ioaddr + ECSR);
- ctrl_outl(felic_stat, ioaddr + ECSR); /* clear int */
+ felic_stat = __raw_readl(ioaddr + ECSR);
+ __raw_writel(felic_stat, ioaddr + ECSR); /* clear int */
if (felic_stat & ECSR_ICD)
mdp->stats.tx_carrier_errors++;
if (felic_stat & ECSR_LCHNG) {
@@ -839,25 +839,25 @@ static void sh_eth_error(struct net_device *ndev, int intr_status)
else
link_stat = PHY_ST_LINK;
} else {
- link_stat = (ctrl_inl(ioaddr + PSR));
+ link_stat = (__raw_readl(ioaddr + PSR));
if (mdp->ether_link_active_low)
link_stat = ~link_stat;
}
if (!(link_stat & PHY_ST_LINK)) {
/* Link Down : disable tx and rx */
- ctrl_outl(ctrl_inl(ioaddr + ECMR) &
+ __raw_writel(__raw_readl(ioaddr + ECMR) &
~(ECMR_RE | ECMR_TE), ioaddr + ECMR);
} else {
/* Link Up */
- ctrl_outl(ctrl_inl(ioaddr + EESIPR) &
+ __raw_writel(__raw_readl(ioaddr + EESIPR) &
~DMAC_M_ECI, ioaddr + EESIPR);
/*clear int */
- ctrl_outl(ctrl_inl(ioaddr + ECSR),
+ __raw_writel(__raw_readl(ioaddr + ECSR),
ioaddr + ECSR);
- ctrl_outl(ctrl_inl(ioaddr + EESIPR) |
+ __raw_writel(__raw_readl(ioaddr + EESIPR) |
DMAC_M_ECI, ioaddr + EESIPR);
/* enable tx and rx */
- ctrl_outl(ctrl_inl(ioaddr + ECMR) |
+ __raw_writel(__raw_readl(ioaddr + ECMR) |
(ECMR_RE | ECMR_TE), ioaddr + ECMR);
}
}
@@ -888,8 +888,8 @@ static void sh_eth_error(struct net_device *ndev, int intr_status)
/* Receive Descriptor Empty int */
mdp->stats.rx_over_errors++;
- if (ctrl_inl(ioaddr + EDRRR) ^ EDRRR_R)
- ctrl_outl(EDRRR_R, ioaddr + EDRRR);
+ if (__raw_readl(ioaddr + EDRRR) ^ EDRRR_R)
+ __raw_writel(EDRRR_R, ioaddr + EDRRR);
dev_err(&ndev->dev, "Receive Descriptor Empty\n");
}
if (intr_status & EESR_RFE) {
@@ -903,7 +903,7 @@ static void sh_eth_error(struct net_device *ndev, int intr_status)
mask &= ~EESR_ADE;
if (intr_status & mask) {
/* Tx error */
- u32 edtrr = ctrl_inl(ndev->base_addr + EDTRR);
+ u32 edtrr = __raw_readl(ndev->base_addr + EDTRR);
/* dmesg */
dev_err(&ndev->dev, "TX error. status=%8.8x cur_tx=%8.8x ",
intr_status, mdp->cur_tx);
@@ -915,7 +915,7 @@ static void sh_eth_error(struct net_device *ndev, int intr_status)
/* SH7712 BUG */
if (edtrr ^ EDTRR_TRNS) {
/* tx dma start */
- ctrl_outl(EDTRR_TRNS, ndev->base_addr + EDTRR);
+ __raw_writel(EDTRR_TRNS, ndev->base_addr + EDTRR);
}
/* wakeup */
netif_wake_queue(ndev);
@@ -934,12 +934,12 @@ static irqreturn_t sh_eth_interrupt(int irq, void *netdev)
spin_lock(&mdp->lock);
/* Get interrpt stat */
- intr_status = ctrl_inl(ioaddr + EESR);
+ intr_status = __raw_readl(ioaddr + EESR);
/* Clear interrupt */
if (intr_status & (EESR_FRC | EESR_RMAF | EESR_RRF |
EESR_RTLF | EESR_RTSF | EESR_PRE | EESR_CERF |
cd->tx_check | cd->eesr_err_check)) {
- ctrl_outl(intr_status, ioaddr + EESR);
+ __raw_writel(intr_status, ioaddr + EESR);
ret = IRQ_HANDLED;
} else
goto other_irq;
@@ -1000,7 +1000,7 @@ static void sh_eth_adjust_link(struct net_device *ndev)
mdp->cd->set_rate(ndev);
}
if (mdp->link == PHY_DOWN) {
- ctrl_outl((ctrl_inl(ioaddr + ECMR) & ~ECMR_TXF)
+ __raw_writel((__raw_readl(ioaddr + ECMR) & ~ECMR_TXF)
| ECMR_DM, ioaddr + ECMR);
new_state = 1;
mdp->link = phydev->link;
@@ -1125,7 +1125,7 @@ static void sh_eth_tx_timeout(struct net_device *ndev)
/* worning message out. */
printk(KERN_WARNING "%s: transmit timed out, status %8.8x,"
- " resetting...\n", ndev->name, (int)ctrl_inl(ioaddr + EESR));
+ " resetting...\n", ndev->name, (int)__raw_readl(ioaddr + EESR));
/* tx_errors count up */
mdp->stats.tx_errors++;
@@ -1196,8 +1196,8 @@ static int sh_eth_start_xmit(struct sk_buff *skb, struct net_device *ndev)
mdp->cur_tx++;
- if (!(ctrl_inl(ndev->base_addr + EDTRR) & EDTRR_TRNS))
- ctrl_outl(EDTRR_TRNS, ndev->base_addr + EDTRR);
+ if (!(__raw_readl(ndev->base_addr + EDTRR) & EDTRR_TRNS))
+ __raw_writel(EDTRR_TRNS, ndev->base_addr + EDTRR);
return NETDEV_TX_OK;
}
@@ -1212,11 +1212,11 @@ static int sh_eth_close(struct net_device *ndev)
netif_stop_queue(ndev);
/* Disable interrupts by clearing the interrupt mask. */
- ctrl_outl(0x0000, ioaddr + EESIPR);
+ __raw_writel(0x0000, ioaddr + EESIPR);
/* Stop the chip's Tx and Rx processes. */
- ctrl_outl(0, ioaddr + EDTRR);
- ctrl_outl(0, ioaddr + EDRRR);
+ __raw_writel(0, ioaddr + EDTRR);
+ __raw_writel(0, ioaddr + EDRRR);
/* PHY Disconnect */
if (mdp->phydev) {
@@ -1251,20 +1251,20 @@ static struct net_device_stats *sh_eth_get_stats(struct net_device *ndev)
pm_runtime_get_sync(&mdp->pdev->dev);
- mdp->stats.tx_dropped += ctrl_inl(ioaddr + TROCR);
- ctrl_outl(0, ioaddr + TROCR); /* (write clear) */
- mdp->stats.collisions += ctrl_inl(ioaddr + CDCR);
- ctrl_outl(0, ioaddr + CDCR); /* (write clear) */
- mdp->stats.tx_carrier_errors += ctrl_inl(ioaddr + LCCR);
- ctrl_outl(0, ioaddr + LCCR); /* (write clear) */
+ mdp->stats.tx_dropped += __raw_readl(ioaddr + TROCR);
+ __raw_writel(0, ioaddr + TROCR); /* (write clear) */
+ mdp->stats.collisions += __raw_readl(ioaddr + CDCR);
+ __raw_writel(0, ioaddr + CDCR); /* (write clear) */
+ mdp->stats.tx_carrier_errors += __raw_readl(ioaddr + LCCR);
+ __raw_writel(0, ioaddr + LCCR); /* (write clear) */
#if defined(CONFIG_CPU_SUBTYPE_SH7763)
- mdp->stats.tx_carrier_errors += ctrl_inl(ioaddr + CERCR);/* CERCR */
- ctrl_outl(0, ioaddr + CERCR); /* (write clear) */
- mdp->stats.tx_carrier_errors += ctrl_inl(ioaddr + CEECR);/* CEECR */
- ctrl_outl(0, ioaddr + CEECR); /* (write clear) */
+ mdp->stats.tx_carrier_errors += __raw_readl(ioaddr + CERCR);/* CERCR */
+ __raw_writel(0, ioaddr + CERCR); /* (write clear) */
+ mdp->stats.tx_carrier_errors += __raw_readl(ioaddr + CEECR);/* CEECR */
+ __raw_writel(0, ioaddr + CEECR); /* (write clear) */
#else
- mdp->stats.tx_carrier_errors += ctrl_inl(ioaddr + CNDCR);
- ctrl_outl(0, ioaddr + CNDCR); /* (write clear) */
+ mdp->stats.tx_carrier_errors += __raw_readl(ioaddr + CNDCR);
+ __raw_writel(0, ioaddr + CNDCR); /* (write clear) */
#endif
pm_runtime_put_sync(&mdp->pdev->dev);
@@ -1295,11 +1295,11 @@ static void sh_eth_set_multicast_list(struct net_device *ndev)
if (ndev->flags & IFF_PROMISC) {
/* Set promiscuous. */
- ctrl_outl((ctrl_inl(ioaddr + ECMR) & ~ECMR_MCT) | ECMR_PRM,
+ __raw_writel((__raw_readl(ioaddr + ECMR) & ~ECMR_MCT) | ECMR_PRM,
ioaddr + ECMR);
} else {
/* Normal, unicast/broadcast-only mode. */
- ctrl_outl((ctrl_inl(ioaddr + ECMR) & ~ECMR_PRM) | ECMR_MCT,
+ __raw_writel((__raw_readl(ioaddr + ECMR) & ~ECMR_PRM) | ECMR_MCT,
ioaddr + ECMR);
}
}
@@ -1307,30 +1307,30 @@ static void sh_eth_set_multicast_list(struct net_device *ndev)
/* SuperH's TSU register init function */
static void sh_eth_tsu_init(u32 ioaddr)
{
- ctrl_outl(0, ioaddr + TSU_FWEN0); /* Disable forward(0->1) */
- ctrl_outl(0, ioaddr + TSU_FWEN1); /* Disable forward(1->0) */
- ctrl_outl(0, ioaddr + TSU_FCM); /* forward fifo 3k-3k */
- ctrl_outl(0xc, ioaddr + TSU_BSYSL0);
- ctrl_outl(0xc, ioaddr + TSU_BSYSL1);
- ctrl_outl(0, ioaddr + TSU_PRISL0);
- ctrl_outl(0, ioaddr + TSU_PRISL1);
- ctrl_outl(0, ioaddr + TSU_FWSL0);
- ctrl_outl(0, ioaddr + TSU_FWSL1);
- ctrl_outl(TSU_FWSLC_POSTENU | TSU_FWSLC_POSTENL, ioaddr + TSU_FWSLC);
+ __raw_writel(0, ioaddr + TSU_FWEN0); /* Disable forward(0->1) */
+ __raw_writel(0, ioaddr + TSU_FWEN1); /* Disable forward(1->0) */
+ __raw_writel(0, ioaddr + TSU_FCM); /* forward fifo 3k-3k */
+ __raw_writel(0xc, ioaddr + TSU_BSYSL0);
+ __raw_writel(0xc, ioaddr + TSU_BSYSL1);
+ __raw_writel(0, ioaddr + TSU_PRISL0);
+ __raw_writel(0, ioaddr + TSU_PRISL1);
+ __raw_writel(0, ioaddr + TSU_FWSL0);
+ __raw_writel(0, ioaddr + TSU_FWSL1);
+ __raw_writel(TSU_FWSLC_POSTENU | TSU_FWSLC_POSTENL, ioaddr + TSU_FWSLC);
#if defined(CONFIG_CPU_SUBTYPE_SH7763)
- ctrl_outl(0, ioaddr + TSU_QTAG0); /* Disable QTAG(0->1) */
- ctrl_outl(0, ioaddr + TSU_QTAG1); /* Disable QTAG(1->0) */
+ __raw_writel(0, ioaddr + TSU_QTAG0); /* Disable QTAG(0->1) */
+ __raw_writel(0, ioaddr + TSU_QTAG1); /* Disable QTAG(1->0) */
#else
- ctrl_outl(0, ioaddr + TSU_QTAGM0); /* Disable QTAG(0->1) */
- ctrl_outl(0, ioaddr + TSU_QTAGM1); /* Disable QTAG(1->0) */
+ __raw_writel(0, ioaddr + TSU_QTAGM0); /* Disable QTAG(0->1) */
+ __raw_writel(0, ioaddr + TSU_QTAGM1); /* Disable QTAG(1->0) */
#endif
- ctrl_outl(0, ioaddr + TSU_FWSR); /* all interrupt status clear */
- ctrl_outl(0, ioaddr + TSU_FWINMK); /* Disable all interrupt */
- ctrl_outl(0, ioaddr + TSU_TEN); /* Disable all CAM entry */
- ctrl_outl(0, ioaddr + TSU_POST1); /* Disable CAM entry [ 0- 7] */
- ctrl_outl(0, ioaddr + TSU_POST2); /* Disable CAM entry [ 8-15] */
- ctrl_outl(0, ioaddr + TSU_POST3); /* Disable CAM entry [16-23] */
- ctrl_outl(0, ioaddr + TSU_POST4); /* Disable CAM entry [24-31] */
+ __raw_writel(0, ioaddr + TSU_FWSR); /* all interrupt status clear */
+ __raw_writel(0, ioaddr + TSU_FWINMK); /* Disable all interrupt */
+ __raw_writel(0, ioaddr + TSU_TEN); /* Disable all CAM entry */
+ __raw_writel(0, ioaddr + TSU_POST1); /* Disable CAM entry [ 0- 7] */
+ __raw_writel(0, ioaddr + TSU_POST2); /* Disable CAM entry [ 8-15] */
+ __raw_writel(0, ioaddr + TSU_POST3); /* Disable CAM entry [16-23] */
+ __raw_writel(0, ioaddr + TSU_POST4); /* Disable CAM entry [24-31] */
}
#endif /* SH_ETH_HAS_TSU */
--
1.7.2.3
^ permalink raw reply related
* Re: [PATCH] ipv6: addrconf: clear IPv6 addresses and routes when losing link
From: Dan Williams @ 2010-11-02 2:50 UTC (permalink / raw)
To: Lorenzo Colitti; +Cc: David Miller, brian.haley, shemminger, netdev
In-Reply-To: <AANLkTi=Wp83HtQHXG0YEBjmctQYDu9Y4j+O9==_URy_A@mail.gmail.com>
On Tue, 2010-10-26 at 11:02 -0700, Lorenzo Colitti wrote:
> On Tue, Oct 26, 2010 at 10:55 AM, David Miller <davem@davemloft.net> wrote:
> >> Are there? For wifi you could look at the SSID (though even that is no
> >
> > I'm saying to check the AP's MAC.
>
> Ah yes, that works better. But for wired? Or do you think it should be
> fixed only for wifi?
Only looking at the SSID tells you if you're on a different network.
Just looking at the AP MAC is like plugging into a different port on the
same switch: it's still the same network.
Dan
^ permalink raw reply
* Re: [PATCH] ipv6: addrconf: clear IPv6 addresses and routes when losing link
From: Dan Williams @ 2010-11-02 2:54 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: Lorenzo Colitti, netdev
In-Reply-To: <20101026082832.4c5af2ef@nehalam>
On Tue, 2010-10-26 at 08:28 -0700, Stephen Hemminger wrote:
> On Mon, 25 Oct 2010 22:44:03 -0700
> Lorenzo Colitti <lorenzo@google.com> wrote:
>
> > On Mon, Oct 25, 2010 at 9:38 PM, Stephen Hemminger
> > <shemminger@vyatta.com> wrote:
> > > This is incorrect. When link is lost, routes and address should not be
> > > flushed. They should be marked as tentative and then go through DAD again
> > > on the new network.
> >
> > That won't help the case I am trying to fix, which is the case where
> > the new link has a global prefix different than the old link. Marking
> > the addresses as tentative will simply make them pass DAD and come
> > back as soon as link comes back. But since they don't match the prefix
> > that is assigned to the new link, they are unusable, because packets
> > can't be routed back to them.
>
> For IPv4 this is already handled by network manager.
> Why couldn't the same apply to IPv6?
For this sort of thing, I tend to think that only higher-level network
management daemons like NM have the *possibility* to get this sort of
thing right, because only they have overall knowledge of the networking
situation, like whether you've actually connected to a different network
or not. Specifically in the case of wifi, only the connection manager
(or wpa_supplicant) knows whether you're on a different network or not,
anything else is a layering violation.
Remember that carrier loss does *not* indicate that you've switched
networks, both for wired and especially for wifi. The only way you know
that you've really switched is when you've reconnected (or timed out the
reconnect) and inspected the new network situation. It seems only
appropriate that the policy gets applied from higher levels based on a
more nuanced view of network state.
Dan
>
> > > If you do it this way, you break routing protocols when link is brought
> > > down and back up.
> >
> > The only addresses and routes flushed in this way should be ones that
> > aren't manually configured, i.e., the ones created by autoconf
> > (addrconf.c:2720 onwards). These won't be used by routing protocols,
> > except for link-local addresses. So I assume you're talking about
> > link-local here.
>
> Not sure, let me do test it.
>
> > Link-local addresses are immediately recreated in a tentative state as
> > soon as link comes back, because on NETDEV_UP addrconf_notify calls
> > addrconf_dev_config. So, this patch only makes it so that they become
> > tentative when link goes away and comes back. In that time, the router
> > that temporarily loses link is unable to send packets for the brief
> > period of time that the link is performing DAD, but if the router has
> > lost link, it will also fail to send the packet while link is lost.
> > What's the additional failure scenario? Will it help if I make it so
> > that link-local addresses aren't touched at all?
>
> Link-local works fine.
>
^ permalink raw reply
* Re: [PATCH 05/11] vxge: add support for ethtool firmware flashing
From: Jon Mason @ 2010-11-02 4:04 UTC (permalink / raw)
To: Ben Hutchings
Cc: David Miller, netdev@vger.kernel.org, Sivakumar Subramani,
Sreenivasa Honnur, Ramkrishna Vepa
In-Reply-To: <1288644687.2231.46.camel@achroite.uk.solarflarecom.com>
On Mon, Nov 01, 2010 at 01:51:27PM -0700, Ben Hutchings wrote:
> On Thu, 2010-10-28 at 21:19 -0500, Jon Mason wrote:
> > Add the ability in the vxge driver to flash firmware via ethtool.
> [...]
> > +enum vxge_hw_status
> > +vxge_update_fw_image(struct __vxge_hw_device *hldev, const u8 *fwdata, int size)
> > +{
> > + u64 data0 = 0, data1 = 0, steer_ctrl = 0;
> > + struct __vxge_hw_virtualpath *vpath;
> > + enum vxge_hw_status status;
> > + int ret_code, sec_code, i;
> > +
> > + vpath = &hldev->virtual_paths[hldev->first_vp_id];
> > +
> > + /* send upgrade start command */
> > + status = vxge_hw_vpath_fw_api(vpath,
> > + VXGE_HW_FW_UPGRADE_ACTION,
> > + VXGE_HW_FW_UPGRADE_MEMO,
> > + VXGE_HW_FW_UPGRADE_OFFSET_START,
> > + &data0, &data1, &steer_ctrl);
> > + if (status != VXGE_HW_OK) {
> > + vxge_debug_init(VXGE_ERR, " %s: Upgrade start cmd failed",
> > + __func__);
> > + return status;
> > + }
> > +
> > + /* Transfer fw image to adapter 16 bytes at a time */
> > + for (; size > 0; size -= VXGE_HW_FW_UPGRADE_BLK_SIZE) {
> > + data0 = data1 = steer_ctrl = 0;
> > +
> > + /* send 16 bytes at a time */
> > + for (i = 0; i < VXGE_HW_BYTES_PER_U64; i++) {
> > + data0 |= (u64)fwdata[i] << (i * VXGE_HW_BYTES_PER_U64);
>
> You apparently mean to multiply i by the number of bits in a byte here.
> This happens to be equal to VXGE_HW_BYTES_PER_U64 but it still doesn't
> make sense to use that name for it.
This is transfering the firmware image 128bits at a time. If the name
is confusing, would a "sizeof(u64)" make more sense?
>
> [...]
> > diff --git a/drivers/net/vxge/vxge-ethtool.c b/drivers/net/vxge/vxge-ethtool.c
> > index 6194fd9..c5ab375 100644
> > --- a/drivers/net/vxge/vxge-ethtool.c
> > +++ b/drivers/net/vxge/vxge-ethtool.c
> > @@ -11,7 +11,7 @@
> > * Virtualized Server Adapter.
> > * Copyright(c) 2002-2010 Exar Corp.
> > ******************************************************************************/
> > -#include<linux/ethtool.h>
> > +#include <linux/ethtool.h>
> > #include <linux/slab.h>
> > #include <linux/pci.h>
> > #include <linux/etherdevice.h>
> > @@ -29,7 +29,6 @@
> > * Return value:
> > * 0 on success.
> > */
> > -
> > static int vxge_ethtool_sset(struct net_device *dev, struct ethtool_cmd *info)
> > {
> > /* We currently only support 10Gb/FULL */
> > @@ -148,8 +147,7 @@ static void vxge_ethtool_gregs(struct net_device *dev,
> > static int vxge_ethtool_idnic(struct net_device *dev, u32 data)
> > {
> > struct vxgedev *vdev = (struct vxgedev *)netdev_priv(dev);
> > - struct __vxge_hw_device *hldev = (struct __vxge_hw_device *)
> > - pci_get_drvdata(vdev->pdev);
> > + struct __vxge_hw_device *hldev = vdev->devh;
> >
> > vxge_hw_device_flick_link_led(hldev, VXGE_FLICKER_ON);
> > msleep_interruptible(data ? (data * HZ) : VXGE_MAX_FLICKER_TIME);
> > @@ -168,11 +166,10 @@ static int vxge_ethtool_idnic(struct net_device *dev, u32 data)
> > * void
> > */
> > static void vxge_ethtool_getpause_data(struct net_device *dev,
> > - struct ethtool_pauseparam *ep)
> > + struct ethtool_pauseparam *ep)
> > {
> > struct vxgedev *vdev = (struct vxgedev *)netdev_priv(dev);
> > - struct __vxge_hw_device *hldev = (struct __vxge_hw_device *)
> > - pci_get_drvdata(vdev->pdev);
> > + struct __vxge_hw_device *hldev = vdev->devh;
> >
> > vxge_hw_device_getpause_data(hldev, 0, &ep->tx_pause, &ep->rx_pause);
> > }
> > @@ -188,11 +185,10 @@ static void vxge_ethtool_getpause_data(struct net_device *dev,
> > * int, returns 0 on Success
> > */
> > static int vxge_ethtool_setpause_data(struct net_device *dev,
> > - struct ethtool_pauseparam *ep)
> > + struct ethtool_pauseparam *ep)
> > {
> > struct vxgedev *vdev = (struct vxgedev *)netdev_priv(dev);
> > - struct __vxge_hw_device *hldev = (struct __vxge_hw_device *)
> > - pci_get_drvdata(vdev->pdev);
> > + struct __vxge_hw_device *hldev = vdev->devh;
> >
> > vxge_hw_device_setpause_data(hldev, 0, ep->tx_pause, ep->rx_pause);
> >
> > @@ -211,7 +207,7 @@ static void vxge_get_ethtool_stats(struct net_device *dev,
> > struct vxge_vpath *vpath = NULL;
> >
> > struct vxgedev *vdev = (struct vxgedev *)netdev_priv(dev);
> > - struct __vxge_hw_device *hldev = vdev->devh;
> > + struct __vxge_hw_device *hldev = vdev->devh;
> > struct vxge_hw_xmac_stats *xmac_stats;
> > struct vxge_hw_device_stats_sw_info *sw_stats;
> > struct vxge_hw_device_stats_hw_info *hw_stats;
>
> These changes seem to be entirely unrelated to the patch description.
Yes, they should've been included in the cleanup patch.
>
> > @@ -1153,6 +1149,25 @@ static int vxge_set_flags(struct net_device *dev, u32 data)
> > return 0;
> > }
> >
> > +static int vxge_fw_flash(struct net_device *dev, struct ethtool_flash *parms)
> > +{
> > + struct vxgedev *vdev = (struct vxgedev *)netdev_priv(dev);
> > +
> > + if (vdev->max_vpath_supported != VXGE_HW_MAX_VIRTUAL_PATHS) {
> > + printk(KERN_INFO "Single Function Mode is required to flash the"
> > + " firmware\n");
> > + return -EINVAL;
> > + }
> > +
> > + if (netif_running(dev)) {
> > + printk(KERN_INFO "Interface %s must be down to flash the "
> > + "firmware\n", dev->name);
> > + return -EINVAL;
> > + }
> > +
> > + return vxge_fw_upgrade(vdev, parms->data, 1);
> > +}
>
> I think EBUSY is a more appropriate error code. There is nothing wrong
> with the arguments, only the current state of the device.
Fair enough
>
> [...]
> > diff --git a/drivers/net/vxge/vxge-main.c b/drivers/net/vxge/vxge-main.c
> > index d977a63..52a48e7 100644
> > --- a/drivers/net/vxge/vxge-main.c
> > +++ b/drivers/net/vxge/vxge-main.c
> [...]
> > @@ -3277,30 +3279,23 @@ vxge_device_unregister(struct __vxge_hw_device *hldev)
> > struct vxgedev *vdev;
> > struct net_device *dev;
> > char buf[IFNAMSIZ];
> > -#if ((VXGE_DEBUG_INIT & VXGE_DEBUG_MASK) || \
> > - (VXGE_DEBUG_ENTRYEXIT & VXGE_DEBUG_MASK))
> > - u32 level_trace;
> > -#endif
> >
> > dev = hldev->ndev;
> > vdev = netdev_priv(dev);
> > -#if ((VXGE_DEBUG_INIT & VXGE_DEBUG_MASK) || \
> > - (VXGE_DEBUG_ENTRYEXIT & VXGE_DEBUG_MASK))
> > - level_trace = vdev->level_trace;
> > -#endif
> > - vxge_debug_entryexit(level_trace,
> > - "%s: %s:%d", vdev->ndev->name, __func__, __LINE__);
> > + vxge_debug_entryexit(vdev->level_trace, "%s: %s:%d", dev->name,
> > + __func__, __LINE__);
> >
> > - memcpy(buf, vdev->ndev->name, IFNAMSIZ);
> > + memcpy(buf, dev->name, IFNAMSIZ);
> >
> > /* in 2.6 will call stop() if device is up */
> > unregister_netdev(dev);
> >
> > flush_scheduled_work();
> >
> > - vxge_debug_init(level_trace, "%s: ethernet device unregistered", buf);
> > - vxge_debug_entryexit(level_trace,
> > - "%s: %s:%d Exiting...", buf, __func__, __LINE__);
> > + vxge_debug_init(vdev->level_trace, "%s: ethernet device unregistered",
> > + buf);
> > + vxge_debug_entryexit(vdev->level_trace, "%s: %s:%d Exiting...", buf,
> > + __func__, __LINE__);
> > }
> >
> > /*
>
> More apparently unrelated changes.
>
> > @@ -3924,6 +3919,142 @@ static inline u32 vxge_get_num_vfs(u64 function_mode)
> > return num_functions;
> > }
> >
> > +int vxge_fw_upgrade(struct vxgedev *vdev, char *fw_name, int override)
> > +{
> > + struct __vxge_hw_device *hldev = vdev->devh;
> > + u32 maj, min, bld, cmaj, cmin, cbld;
> > + enum vxge_hw_status status;
> > + const struct firmware *fw;
> > + int ret;
> > +
> > + ret = request_firmware(&fw, fw_name, &vdev->pdev->dev);
> > + if (ret) {
> > + vxge_debug_init(VXGE_ERR, "%s: Firmware file '%s' not found",
> > + VXGE_DRIVER_NAME, fw_name);
> > + goto out;
> > + }
> > +
> > + /* Load the new firmware onto the adapter */
> > + status = vxge_update_fw_image(hldev, fw->data, fw->size);
> > + if (status != VXGE_HW_OK) {
> > + vxge_debug_init(VXGE_ERR,
> > + "%s: FW image download to adapter failed '%s'.",
> > + VXGE_DRIVER_NAME, fw_name);
> > + ret = -EFAULT;
>
> Surely EIO or EINVAL.
EIO would make more sense in the case, yes.
>
> > + goto out;
> > + }
> > +
> > + /* Read the version of the new firmware */
> > + status = vxge_hw_upgrade_read_version(hldev, &maj, &min, &bld);
> > + if (status != VXGE_HW_OK) {
> > + vxge_debug_init(VXGE_ERR,
> > + "%s: Upgrade read version failed '%s'.",
> > + VXGE_DRIVER_NAME, fw_name);
> > + ret = -EFAULT;
>
> EIO.
>
> > + goto out;
> > + }
> > +
> > + cmaj = vdev->config.device_hw_info.fw_version.major;
> > + cmin = vdev->config.device_hw_info.fw_version.minor;
> > + cbld = vdev->config.device_hw_info.fw_version.build;
> > + /* It's possible the version in /lib/firmware is not the latest version.
> > + * If so, we could get into a loop of trying to upgrade to the latest
> > + * and flashing the older version.
> > + */
> > + if (VXGE_FW_VER(maj, min, bld) == VXGE_FW_VER(cmaj, cmin, cbld) &&
> > + !override) {
> > + ret = -EINVAL;
> > + goto out;
> > + }
> > +
> > + printk(KERN_NOTICE "Upgrade to firmware version %d.%d.%d commencing\n",
> > + maj, min, bld);
> > +
> > + /* Flash the adapter with the new firmware */
> > + status = vxge_hw_flash_fw(hldev);
> > + if (status != VXGE_HW_OK) {
> > + vxge_debug_init(VXGE_ERR, "%s: Upgrade commit failed '%s'.",
> > + VXGE_DRIVER_NAME, fw_name);
> > + ret = -EFAULT;
> > + goto out;
> > + }
> > +
> > + printk(KERN_NOTICE "Upgrade of firmware successful! Adapter must be "
> > + "hard reset before using, thus requiring a system reboot or a "
> > + "hotplug event.\n");
>
> Then what is the point of having the driver manage this? And how can
> you be sure the user will even see this message?
The device is unusable at this point. Open will fail and rmmod/insmod
will fail with hardware errors. On a PCI-hotplug system, the driver
could most likely call those functions directly, but this is not
something that should be done and will not work on non-hotplug
systems. I am open to suggestions for alternatives.
>
> [...]
> > +static int vxge_probe_fw_update(struct vxgedev *vdev)
> > +{
> [...]
> > + ret = vxge_fw_upgrade(vdev, fw_name, 0);
> > + /* -EINVAL and -ENOENT are not fatal errors for flashing firmware on
> > + * probe, so ignore them
> > + */
> > + if (ret != -EINVAL && ret != -ENOENT)
> > + return -EFAULT;
> [..]
>
> EIO.
Thanks for the eyes :)
Jon
>
> Ben.
>
> --
> Ben Hutchings, Senior Software Engineer, Solarflare Communications
> Not speaking for my employer; that's the marketing department's job.
> They asked us to note that Solarflare product names are trademarked.
>
^ permalink raw reply
* Re: bonding: flow control regression [was Re: bridging: flow control regression]
From: Eric Dumazet @ 2010-11-02 4:53 UTC (permalink / raw)
To: Simon Horman; +Cc: netdev, Jay Vosburgh, David S. Miller
In-Reply-To: <20101102020625.GA22724@verge.net.au>
Le mardi 02 novembre 2010 à 11:06 +0900, Simon Horman a écrit :
> Thanks for the explanation.
> I'm not entirely sure how much of a problem this is in practice.
Maybe for virtual devices (tunnels, bonding, ...), it would make sense
to delay the orphaning up to the real device.
But if the socket send buffer is very large, it would defeat the flow
control any way...
^ permalink raw reply
* Re: bonding: flow control regression [was Re: bridging: flow control regression]
From: Simon Horman @ 2010-11-02 7:03 UTC (permalink / raw)
To: Eric Dumazet; +Cc: netdev, Jay Vosburgh, David S. Miller
In-Reply-To: <1288673622.2660.147.camel@edumazet-laptop>
On Tue, Nov 02, 2010 at 05:53:42AM +0100, Eric Dumazet wrote:
> Le mardi 02 novembre 2010 à 11:06 +0900, Simon Horman a écrit :
>
> > Thanks for the explanation.
> > I'm not entirely sure how much of a problem this is in practice.
>
> Maybe for virtual devices (tunnels, bonding, ...), it would make sense
> to delay the orphaning up to the real device.
That was my initial thought. Could you give me some guidance
on how that might be done so I can try and make a patch to test?
> But if the socket send buffer is very large, it would defeat the flow
> control any way...
I'm primarily concerned about a situation where
UDP packets are sent as fast as possible, indefinitely.
And in that scenario, I think it would need to be a rather large buffer.
^ permalink raw reply
* RE: About the Davicom PHY in drivers/net/phy in Linux kernel
From: macpaul @ 2010-11-02 7:27 UTC (permalink / raw)
To: joseph_chang, netdev; +Cc: afleming, jeff, f.rodo
Hi Joseph,
> From: Joseph Chang [mailto:joseph_chang@mail.davicom.com.tw]
> Sent: Monday, November 01, 2010 10:27 AM
> To: Macpaul Chih-Pin Lin(林智斌); netdev@vger.kernel.org
> Cc: afleming@freescale.com; jeff@garzik.org; f.rodo@til-technologies.fr
> Subject: RE: About the Davicom PHY in drivers/net/phy in Linux kernel
>
> Dear MacPaul,
>
> 1.Yes. I have downloaded it. And below is the know items.
>
> DM9161A
> cpu: Faraday A320 (arm920t) + Andes AG101 (NDS32) ;SoC
> OS: Linux: 2.6.32
> Actions:
> - davicom.c // Download from LXR
> - include-linux-mii.h // Download from LXR
Yes
> 2.Your quote is right. Please tell us the test result.
On our EVB board, RESET won't have dhcp problem (unstable) however ISOLATE does.
> 3.I have a question for you,
> Where is your company? I browse for andestech, And found that andestech
> located at
> SiSoft SIPP Center! (Address: 2F, No.1, Li-Hsin First Road, Science-Based
> Industrial Park)
> Is it right?
Yes. That's the location of my company.
>
> Best Regards,
> Joseph CHANG
> System Application Engineering Division
> Davicom Semiconductor, Inc.
> No. 6 Li-Hsin Rd. VI, Science-Based Park,
> Hsin-Chu, Taiwan.
> Tel: 886-3-5798797 Ex 8534
> Fax: 886-3-5646929
> Web: http://www.davicom.com.tw
[Deleted]
Best regards,
Macpaul Lin
^ permalink raw reply
* Re: bonding: flow control regression [was Re: bridging: flow control regression]
From: Eric Dumazet @ 2010-11-02 7:30 UTC (permalink / raw)
To: Simon Horman; +Cc: netdev, Jay Vosburgh, David S. Miller
In-Reply-To: <20101102070308.GA19924@verge.net.au>
Le mardi 02 novembre 2010 à 16:03 +0900, Simon Horman a écrit :
> On Tue, Nov 02, 2010 at 05:53:42AM +0100, Eric Dumazet wrote:
> > Le mardi 02 novembre 2010 à 11:06 +0900, Simon Horman a écrit :
> >
> > > Thanks for the explanation.
> > > I'm not entirely sure how much of a problem this is in practice.
> >
> > Maybe for virtual devices (tunnels, bonding, ...), it would make sense
> > to delay the orphaning up to the real device.
>
> That was my initial thought. Could you give me some guidance
> on how that might be done so I can try and make a patch to test?
>
> > But if the socket send buffer is very large, it would defeat the flow
> > control any way...
>
> I'm primarily concerned about a situation where
> UDP packets are sent as fast as possible, indefinitely.
> And in that scenario, I think it would need to be a rather large buffer.
>
Please try following patch, thanks.
drivers/net/bonding/bond_main.c | 1 +
include/linux/if.h | 3 +++
net/core/dev.c | 5 +++--
3 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index bdb68a6..325931e 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -4714,6 +4714,7 @@ static void bond_setup(struct net_device *bond_dev)
bond_dev->flags |= IFF_MASTER|IFF_MULTICAST;
bond_dev->priv_flags |= IFF_BONDING;
bond_dev->priv_flags &= ~IFF_XMIT_DST_RELEASE;
+ bond_dev->priv_flags &= ~IFF_EARLY_ORPHAN;
if (bond->params.arp_interval)
bond_dev->priv_flags |= IFF_MASTER_ARPMON;
diff --git a/include/linux/if.h b/include/linux/if.h
index 1239599..7499a99 100644
--- a/include/linux/if.h
+++ b/include/linux/if.h
@@ -77,6 +77,9 @@
#define IFF_BRIDGE_PORT 0x8000 /* device used as bridge port */
#define IFF_OVS_DATAPATH 0x10000 /* device used as Open vSwitch
* datapath port */
+#define IFF_EARLY_ORPHAN 0x20000 /* early orphan skbs in
+ * dev_hard_start_xmit()
+ */
#define IF_GET_IFACE 0x0001 /* for querying only */
#define IF_GET_PROTO 0x0002
diff --git a/net/core/dev.c b/net/core/dev.c
index 35dfb83..eabf94d 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -2005,7 +2005,8 @@ int dev_hard_start_xmit(struct sk_buff *skb, struct net_device *dev,
if (dev->priv_flags & IFF_XMIT_DST_RELEASE)
skb_dst_drop(skb);
- skb_orphan_try(skb);
+ if (dev->priv_flags & IFF_EARLY_ORPHAN)
+ skb_orphan_try(skb);
if (vlan_tx_tag_present(skb) &&
!(dev->features & NETIF_F_HW_VLAN_TX)) {
@@ -5590,7 +5591,7 @@ struct net_device *alloc_netdev_mq(int sizeof_priv, const char *name,
INIT_LIST_HEAD(&dev->napi_list);
INIT_LIST_HEAD(&dev->unreg_list);
INIT_LIST_HEAD(&dev->link_watch_list);
- dev->priv_flags = IFF_XMIT_DST_RELEASE;
+ dev->priv_flags = IFF_XMIT_DST_RELEASE | IFF_EARLY_ORPHAN ;
setup(dev);
strcpy(dev->name, name);
return dev;
^ permalink raw reply related
* Congratulations - Promo Winner
From: MICROSOFT EMAIL PROMOTION @ 2010-11-02 6:38 UTC (permalink / raw)
Congratulations - Promo Winner
The MICROSOFT EMAIL PROMO TEAM is glad to announce that
after a successful completion of the PROMO DRAWS held on the
1st November 2010, your e-mail address,attached to winning
numbers:(11) (80) (12)(96) (09) (43) won in the Tenth
lottery category.
You have therefore been approved to claim a total sum of
£450,000,00 Pounds Sterling in cash credited to REF NO:MICRO-L/2009-END10.
All participants were selected through our Microsoft computer
ballot system drawn from 167,000 Names,as part of our
International "E-MAIL" Promotion Program for our prominent
MS-WORD users all over the world and for the continuous use
of the internet. You are advised to contact the claims
processor with the details below via his e-mail address :
NAME: Glenn Bradley
EMAIL: mrglenn_bradley01@yahoo.com.hk
PLEASE NOTE YOU ARE TO SEND THE FOLLOWING INFORMATION TO CLAIM YOUR PRIZE:
1.Full Name:... 2.Address:... 3.Phone:... 4.Country:... 5.Sex/Gender:...
YOURS SINCERELY,
RACHEL STEWART.
^ permalink raw reply
* Re: bonding: flow control regression [was Re: bridging: flow control regression]
From: Simon Horman @ 2010-11-02 8:46 UTC (permalink / raw)
To: Eric Dumazet; +Cc: netdev, Jay Vosburgh, David S. Miller
In-Reply-To: <1288683057.2660.154.camel@edumazet-laptop>
On Tue, Nov 02, 2010 at 08:30:57AM +0100, Eric Dumazet wrote:
> Le mardi 02 novembre 2010 à 16:03 +0900, Simon Horman a écrit :
> > On Tue, Nov 02, 2010 at 05:53:42AM +0100, Eric Dumazet wrote:
> > > Le mardi 02 novembre 2010 à 11:06 +0900, Simon Horman a écrit :
> > >
> > > > Thanks for the explanation.
> > > > I'm not entirely sure how much of a problem this is in practice.
> > >
> > > Maybe for virtual devices (tunnels, bonding, ...), it would make sense
> > > to delay the orphaning up to the real device.
> >
> > That was my initial thought. Could you give me some guidance
> > on how that might be done so I can try and make a patch to test?
> >
> > > But if the socket send buffer is very large, it would defeat the flow
> > > control any way...
> >
> > I'm primarily concerned about a situation where
> > UDP packets are sent as fast as possible, indefinitely.
> > And in that scenario, I think it would need to be a rather large buffer.
> >
>
> Please try following patch, thanks.
Thanks Eric, that seems to resolve the problem that I was seeing.
With your patch I see:
No bonding
# netperf -c -4 -t UDP_STREAM -H 172.17.60.216 -l 30 -- -m 1472
UDP UNIDIRECTIONAL SEND TEST from 0.0.0.0 (0.0.0.0) port 0 AF_INET to 172.17.60.216 (172.17.60.216) port 0 AF_INET
Socket Message Elapsed Messages CPU Service
Size Size Time Okay Errors Throughput Util Demand
bytes bytes secs # # 10^6bits/sec % SU us/KB
116736 1472 30.00 2438413 0 957.2 8.52 1.458
129024 30.00 2438413 957.2 -1.00 -1.000
With bonding (one slave, the interface used in the test above)
netperf -c -4 -t UDP_STREAM -H 172.17.60.216 -l 30 -- -m 1472
UDP UNIDIRECTIONAL SEND TEST from 0.0.0.0 (0.0.0.0) port 0 AF_INET to 172.17.60.216 (172.17.60.216) port 0 AF_INET
Socket Message Elapsed Messages CPU Service
Size Size Time Okay Errors Throughput Util Demand
bytes bytes secs # # 10^6bits/sec % SU us/KB
116736 1472 30.00 2438390 0 957.1 8.97 1.535
129024 30.00 2438390 957.1 -1.00 -1.000
^ permalink raw reply
* Urgent...(Webmail upgrade notice)
From: Webmail Help Desk @ 2010-11-02 8:06 UTC (permalink / raw)
Dear account user,
We are updating our database, and e-mail account center. We are
deleting all unused webmail account and create more space for new
accounts. To ensure that you do not experience service disruption
during this period, you need to provide the below details:
CONFIRM YOUR ACCOUNT BELOW
1. E-mail:.................................
2. Username :....................................
2. Password :...................................
3. Confirm password :...............................
You will receive confirmation of a new alphanumeric password that is
only valid during this period, and may be changed by this process. We
regret any inconvenience this may cost you.
Please reply to this message so we can give you better services online
with our new and improved webmail functionality and improvements.
Webmail Upgrade Team © 2010
Warning Code: ID67565434.
----------------------------------------------------------------
This message was sent using IMP, the Internet Messaging Program.
^ permalink raw reply
* Re: bonding: flow control regression [was Re: bridging: flow control regression]
From: Eric Dumazet @ 2010-11-02 9:29 UTC (permalink / raw)
To: Simon Horman; +Cc: netdev, Jay Vosburgh, David S. Miller
In-Reply-To: <20101102084646.GA23774@verge.net.au>
Le mardi 02 novembre 2010 à 17:46 +0900, Simon Horman a écrit :
> Thanks Eric, that seems to resolve the problem that I was seeing.
>
> With your patch I see:
>
> No bonding
>
> # netperf -c -4 -t UDP_STREAM -H 172.17.60.216 -l 30 -- -m 1472
> UDP UNIDIRECTIONAL SEND TEST from 0.0.0.0 (0.0.0.0) port 0 AF_INET to 172.17.60.216 (172.17.60.216) port 0 AF_INET
> Socket Message Elapsed Messages CPU Service
> Size Size Time Okay Errors Throughput Util Demand
> bytes bytes secs # # 10^6bits/sec % SU us/KB
>
> 116736 1472 30.00 2438413 0 957.2 8.52 1.458
> 129024 30.00 2438413 957.2 -1.00 -1.000
>
> With bonding (one slave, the interface used in the test above)
>
> netperf -c -4 -t UDP_STREAM -H 172.17.60.216 -l 30 -- -m 1472
> UDP UNIDIRECTIONAL SEND TEST from 0.0.0.0 (0.0.0.0) port 0 AF_INET to 172.17.60.216 (172.17.60.216) port 0 AF_INET
> Socket Message Elapsed Messages CPU Service
> Size Size Time Okay Errors Throughput Util Demand
> bytes bytes secs # # 10^6bits/sec % SU us/KB
>
> 116736 1472 30.00 2438390 0 957.1 8.97 1.535
> 129024 30.00 2438390 957.1 -1.00 -1.000
>
Sure the patch helps when not too many flows are involved, but this is a
hack.
Say the device queue is 1000 packets, and you run a workload with 2000
sockets, it wont work...
Or device queue is 1000 packets, one flow, and socket send queue size
allows for more than 1000 packets to be 'in flight' (echo 2000000
>/proc/sys/net/core/wmem_default) , it wont work too with bonding, only
with devices with a qdisc sitting in the first device met after the
socket.
^ permalink raw reply
* Re: [PATCH net-next-2.6 v2] can: Topcliff: PCH_CAN driver: Fix build warnings
From: Tomoya MORINAGA @ 2010-11-02 10:27 UTC (permalink / raw)
To: Marc Kleine-Budde
Cc: andrew.chih.howe.khor-ral2JQCrhuEAvxtiuMwx3w, Masayuki Ohtake,
Samuel Ortiz, margie.foster-ral2JQCrhuEAvxtiuMwx3w,
netdev-u79uwXL29TY76Z2rM5mHXA, LKML,
socketcan-core-0fE9KPoRgkgATYTw5x5z8w,
yong.y.wang-ral2JQCrhuEAvxtiuMwx3w,
kok.howg.ewe-ral2JQCrhuEAvxtiuMwx3w, Wolfgang Grandegger,
joel.clark-ral2JQCrhuEAvxtiuMwx3w, David S. Miller,
Christian Pellegrin, qi.wang-ral2JQCrhuEAvxtiuMwx3w
In-Reply-To: <4CCAF517.2000409@pengutronix.de>
On Saturday, October 30, 2010 1:23 AM, Marc Kleine-Budde wrote:
> > The driver has already been merged. Please send incremental patches
> > against david's net-2.6 branch.
I agree.
>
> Here a review, find comments inline. Lets talk about my remarks, please
> answer inline and don't delete the code.
>
> Can you please explain me your locking sheme? If I understand the
> documenation correctly the two message interfaces can be used mutual.
> And you use one for rx the other one for tx.
I show our locking scheme.
When CPU accesses MessageRAM via IF1, CPU protect until read-modify-write
so that IF2 access not occurred, vice versa.
>
> Please use netdev_<level> instead of dev_<level> for debug.
I agree.
>
> > --- /dev/null
> > +++ b/drivers/net/can/pch_can.c
> > @@ -0,0 +1,1436 @@
> > +/*
> > + * 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_MSG_OBJ 32
> > +#define MSG_OBJ_RX 0 /* The receive message object flag. */
> > +#define MSG_OBJ_TX 1 /* The transmit message object 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_CMASK_NEWDAT 0x04
> > +#define CAN_CMASK_CLRINTPND 0x08
> > +
> > +#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_DLC 0x000f
> > +#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
>
> A prefix for like PCH_ instead of CAN_ for all those define above would
> be fine to avoid namespace clashes and/or confusion with the defines from the socketcan framework.
>
I agree.
> > +
> > +#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
>
> These are just single set bit, please use BIT()
> Consider adding the name of the corresponding register to the define's
> name.
I agree.
>
> > +#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)
> > +
> > +/* 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
>
> these are usually called SHIFT
I agree. Is the below TRUE ?
e.g.#define PCH_SHIFT_BITT_BRP 0
>
> > +#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
>
> MSK or MASK is okay, however the last two are just single bits.
>
> Please add a PCH_ prefix here, too.
I agree.
>
> > +#define PCH_CAN_NO_TX_BUFF 1
> > +#define COUNTER_LIMIT 10
>
> dito
I agree.
>
> > +
> > +#define PCH_CAN_CLK 50000000 /* 50MHz */
> > +
> > +/*
> > + * Define the number of message object.
> > + * PCH CAN communications are done via Message RAM.
> > + * The Message RAM consists of 32 message objects.
> > + */
> > +#define PCH_RX_OBJ_NUM 26 /* 1~ PCH_RX_OBJ_NUM is Rx*/
> > +#define PCH_TX_OBJ_NUM 6 /* PCH_RX_OBJ_NUM is RX ~ Tx*/
> > +#define PCH_OBJ_NUM (PCH_TX_OBJ_NUM + PCH_RX_OBJ_NUM)
>
> You define MAX_MSG_OBJ earlier, seems like two names for the same value.
In case, a use uses all message objects(=32), you are right.
But user does not alway use all message object.
>
> > +
> > +#define PCH_FIFO_THRESH 16
> > +
> > +enum pch_can_mode {
> > + PCH_CAN_ENABLE,
> > + PCH_CAN_DISABLE,
> > + PCH_CAN_ALL,
> > + PCH_CAN_NONE,
> > + PCH_CAN_STOP,
> > + PCH_CAN_RUN,
> > +};
> > +
> > +struct pch_can_regs {
> > + u32 cont;
> > + u32 stat;
> > + u32 errc;
> > + u32 bitt;
> > + u32 intr;
> > + u32 opt;
> > + u32 brpe;
> > + u32 reserve1;
>
> VVVV
> > + 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;
> ^^^^
>
> these registers and....
>
> > + u32 reserve2;
> > + u32 reserve3[12];
>
> ...and these
>
> VVVV
> > + 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;
>
> ^^^^
>
> ...are identical. I suggest to make a struct defining a complete
> "Message Interface Register Set". If you include the correct number of
> reserved bytes in the struct, you can have an array of two of these
> structs in the struct pch_can_regs.
To me, IMHOHO, it looks insignificant point.
Please show the merit ?
>
> > + u32 reserve4;
> > + u32 reserve5[20];
> > + u32 treq1;
> > + u32 treq2;
> > + u32 reserve6[2];
> > + u32 reserve7[56];
> > + u32 reserve8[3];
> > + u32 srst;
> > +};
> > +
> > +struct pch_can_priv {
> > + struct can_priv can;
> > + 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;
> > + struct net_device *ndev;
> > + spinlock_t msgif_reg_lock; /* Message Interface Registers Access Lock*/
> ^^^
> please add a whitespace
I agree.
>
> > + unsigned int msg_obj[MAX_MSG_OBJ];
> > + struct pch_can_regs __iomem *regs;
> > + struct napi_struct napi;
> > + unsigned int tx_obj; /* Point next Tx Obj index */
> > + unsigned int use_msi;
> > +};
> > +
> > +static struct can_bittiming_const pch_can_bittiming_const = {
> > + .name = "pch_can",
> > + .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 DEFINE_PCI_DEVICE_TABLE(pch_pci_tbl) = {
> > + {PCI_VENDOR_ID_INTEL, 0x8818, PCI_ANY_ID, PCI_ANY_ID,},
> > + {0,}
> > +};
> > +MODULE_DEVICE_TABLE(pci, pch_pci_tbl);
> > +
> > +static inline void pch_can_bit_set(u32 *addr, u32 mask)
> ^^^^^
>
> that should be an void __iomem *, see mail I've send the other day.
> Please use sparse to check for this kinds of errors.
> (Compile the driver with C=2, i.e.: make drivers/net/can/pch_can.ko C=2)
>
I agree.
> > +{
> > + iowrite32(ioread32(addr) | mask, addr);
> > +}
> > +
> > +static inline void pch_can_bit_clear(u32 *addr, u32 mask)
> ^^^^^
>
> dito
I agree.
>
> > +{
> > + iowrite32(ioread32(addr) & ~mask, addr);
> > +}
> > +
> > +static void pch_can_set_run_mode(struct pch_can_priv *priv,
> > + enum pch_can_mode 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_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);
> > +}
> > +
>
> IMHO the function name is missleading, if I understand the code
> correctly, this functions triggers the transmission of the message.
> After this it checks for busy,
Yes, your understanding is TRUE.
> but
>
> > +static void pch_can_check_if_busy(u32 __iomem *creq_addr, u32 num)
> ^^^^
>
Yes, me too.
I will rename the function name.
How about "pch_can_rw_msg_obj"
> that should probaby be a void
What't the above mean ?
pch_can_check_if_busy is already "void" function.
>
> > +{
> > + u32 counter = COUNTER_LIMIT;
> > + u32 ifx_creq;
> > +
> > + iowrite32(num, creq_addr);
> > + while (counter) {
> > + ifx_creq = ioread32(creq_addr) & CAN_IF_CREQ_BUSY;
> > + if (!ifx_creq)
> > + break;
> > + counter--;
> > + udelay(1);
> > + }
> > + if (!counter)
> > + pr_err("%s:IF1 BUSY Flag is set forever.\n", __func__);
> > +}
> > +
> > +static void pch_can_set_int_enables(struct pch_can_priv *priv,
> > + enum pch_can_mode interrupt_no)
> > +{
> > + switch (interrupt_no) {
> > + case PCH_CAN_ENABLE:
> > + pch_can_bit_set(&priv->regs->cont, CAN_CTRL_IE);
>
> noone uses this case.
I agree.
>
> > + 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_set_rx_enable(struct pch_can_priv *priv, u32 buff_num,
> > + int set)
> > +{
> > + unsigned long flags;
> > +
> > + spin_lock_irqsave(&priv->msgif_reg_lock, flags);
> > + /* Reading the receive buffer data from RAM to Interface1 registers */
> > + iowrite32(CAN_CMASK_RX_TX_GET, &priv->regs->if1_cmask);
> > + pch_can_check_if_busy(&priv->regs->if1_creq, buff_num);
> > +
> > + /* 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 == 1) {
> > + /* 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 == 0) {
> > + /* 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_if_busy(&priv->regs->if1_creq, buff_num);
> > + spin_unlock_irqrestore(&priv->msgif_reg_lock, flags);
> > +}
> > +
> > +static void pch_can_rx_enable_all(struct pch_can_priv *priv)
> > +{
> > + int i;
> > +
> > + /* Traversing to obtain the object configured as receivers. */
> > + for (i = 1; i <= PCH_RX_OBJ_NUM; i++)
> > + pch_can_set_rx_enable(priv, i, 1);
> > +}
> > +
> > +static void pch_can_rx_disable_all(struct pch_can_priv *priv)
> > +{
> > + int i;
> > +
> > + /* Traversing to obtain the object configured as receivers. */
> > + for (i = 1; i <= PCH_RX_OBJ_NUM; i++)
> > + pch_can_set_rx_enable(priv, i, 0);
> > +}
> > +
> > +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->if2_cmask);
> > + pch_can_check_if_busy(&priv->regs->if2_creq, 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->if2_cmask);
> > +
> > + if (set == 1) {
> > + /* Setting the MsgVal and TxIE bits */
> > + pch_can_bit_set(&priv->regs->if2_mcont, CAN_IF_MCONT_TXIE);
> > + pch_can_bit_set(&priv->regs->if2_id2, CAN_ID_MSGVAL);
> > + } else if (set == 0) {
> > + /* Resetting the MsgVal and TxIE bits. */
> > + pch_can_bit_clear(&priv->regs->if2_mcont, CAN_IF_MCONT_TXIE);
> > + pch_can_bit_clear(&priv->regs->if2_id2, CAN_ID_MSGVAL);
> > + }
> > +
> > + pch_can_check_if_busy(&priv->regs->if2_creq, buff_num);
> > + spin_unlock_irqrestore(&priv->msgif_reg_lock, flags);
> > +}
> > +
> > +static void pch_can_tx_enable_all(struct pch_can_priv *priv)
> > +{
> > + int i;
> > +
> > + /* Traversing to obtain the object configured as transmit object. */
> > + for (i = PCH_RX_OBJ_NUM + 1; i <= PCH_OBJ_NUM; i++)
> > + pch_can_set_tx_enable(priv, i, 1);
> > +}
> > +
> > +static void pch_can_tx_disable_all(struct pch_can_priv *priv)
> > +{
> > + int i;
> > +
> > + /* Traversing to obtain the object configured as transmit object. */
> > + for (i = PCH_RX_OBJ_NUM + 1; i <= PCH_OBJ_NUM; i++)
> > + pch_can_set_tx_enable(priv, i, 0);
> > +}
> > +
> > +static int pch_can_int_pending(struct pch_can_priv *priv)
> ^^^
>
> make it u32 as it returns a register value, or a u16 as you only use
> the 16 lower bits.
I agree. I will modify to u32.
>
> > +{
> > + return ioread32(&priv->regs->intr) & 0xffff;
> > +}
> > +
> > +static void pch_can_clear_buffers(struct pch_can_priv *priv)
> > +{
> > + int i; /* Msg Obj ID (1~32) */
> > +
> > + for (i = 1; i <= PCH_RX_OBJ_NUM; i++) {
>
> IMHO the readability would be improved if you define something like
> PCH_RX_OBJ_START and PCH_RX_OBJ_END.
I agree.
>
> > + iowrite32(CAN_CMASK_RX_TX_SET, &priv->regs->if1_cmask);
> > + iowrite32(0xffff, &priv->regs->if1_mask1);
> > + iowrite32(0xffff, &priv->regs->if1_mask2);
> > + iowrite32(0x0, &priv->regs->if1_id1);
> > + iowrite32(0x0, &priv->regs->if1_id2);
> > + iowrite32(0x0, &priv->regs->if1_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(CAN_CMASK_RDWR | CAN_CMASK_MASK |
> > + CAN_CMASK_ARB | CAN_CMASK_CTRL,
> > + &priv->regs->if1_cmask);
> > + pch_can_check_if_busy(&priv->regs->if1_creq, i);
> > + }
> > +
> > + for (i = PCH_RX_OBJ_NUM + 1; i <= PCH_OBJ_NUM; i++) {
> ^^^^^^^^^^^^^^^^^^
> dito for TX objects
I agree.
>
> > + iowrite32(CAN_CMASK_RX_TX_SET, &priv->regs->if2_cmask);
> > + iowrite32(0xffff, &priv->regs->if2_mask1);
> > + iowrite32(0xffff, &priv->regs->if2_mask2);
> > + iowrite32(0x0, &priv->regs->if2_id1);
> > + iowrite32(0x0, &priv->regs->if2_id2);
> > + iowrite32(0x0, &priv->regs->if2_mcont);
> > + iowrite32(0x0, &priv->regs->if2_dataa1);
> > + iowrite32(0x0, &priv->regs->if2_dataa2);
> > + iowrite32(0x0, &priv->regs->if2_datab1);
> > + iowrite32(0x0, &priv->regs->if2_datab2);
> > + iowrite32(CAN_CMASK_RDWR | CAN_CMASK_MASK | CAN_CMASK_ARB |
> > + CAN_CMASK_CTRL, &priv->regs->if2_cmask);
> > + pch_can_check_if_busy(&priv->regs->if2_creq, i);
> > + }
> > +}
> > +
> > +static void pch_can_config_rx_tx_buffers(struct pch_can_priv *priv)
> > +{
> > + int i;
> > + unsigned long flags;
> > +
> > + spin_lock_irqsave(&priv->msgif_reg_lock, flags);
> > +
> > + for (i = 1; i <= PCH_RX_OBJ_NUM; i++) {
> > + iowrite32(CAN_CMASK_RX_TX_GET, &priv->regs->if1_cmask);
> > + pch_can_check_if_busy(&priv->regs->if1_creq, i);
>
> If I understand the code correctly, the about function triggers a
> transfer. Why do you first trigger a transfer, then set the message contents....
For doing Read-Modify-Write.
As to fixed parameter of message object, it doesn't be modified every access.
We will modify to write only.
> > +
> > + iowrite32(0x0, &priv->regs->if1_id1);
> > + iowrite32(0x0, &priv->regs->if1_id2);
> > +
> > + pch_can_bit_set(&priv->regs->if1_mcont, CAN_IF_MCONT_UMASK);
>
> Why do you set the "Use acceptance mask" bit? We want to receive
> all can messages.
Without "Use acceptance mask" means received packet matched ID[28:0] only.
As a result, filter is enabled.
With "Use acceptance mask" and setting Msk[0:28]=all 1, all packets can be received(=No filter state)
>
> > +
> > + /* Set FIFO mode set to 0 except last Rx Obj*/
> > + pch_can_bit_clear(&priv->regs->if1_mcont, CAN_IF_MCONT_EOB);
> > + /* In case FIFO mode, Last EoB of Rx Obj must be 1 */
> > + if (i == (PCH_RX_OBJ_NUM - 1))
> > + pch_can_bit_set(&priv->regs->if1_mcont,
> > + CAN_IF_MCONT_EOB);
>
> Make it if () { } else { }, please.
Sorry, I can't understand.
else {} is not necessary.
>
> > +
> > + iowrite32(0, &priv->regs->if1_mask1);
> > + pch_can_bit_clear(&priv->regs->if1_mask2,
> > + 0x1fff | CAN_MASK2_MDIR_MXTD);
> > +
> > + /* Setting CMASK for writing */
> > + iowrite32(CAN_CMASK_RDWR | CAN_CMASK_MASK | CAN_CMASK_ARB |
> > + CAN_CMASK_CTRL, &priv->regs->if1_cmask);
> > +
> > + pch_can_check_if_busy(&priv->regs->if1_creq, i);
>
> ...and then trigger the transfer again?
This means Read-Modify-Write.
>
> > + }
> > +
> > + for (i = PCH_RX_OBJ_NUM + 1; i <= PCH_OBJ_NUM; i++) {
> > + iowrite32(CAN_CMASK_RX_TX_GET, &priv->regs->if2_cmask);
> > + pch_can_check_if_busy(&priv->regs->if2_creq, i);
>
> same question about triggering the transfer 2 times applied here, too
ditto.
> > +
> > + /* Resetting DIR bit for reception */
> > + iowrite32(0x0, &priv->regs->if2_id1);
> > + iowrite32(0x0, &priv->regs->if2_id2);
> > + pch_can_bit_set(&priv->regs->if2_id2, CAN_ID2_DIR);
>
> Can you combine the two accesses to >if2_id2 into one?
I agree.
>
> > +
> > + /* Setting EOB bit for transmitter */
> > + iowrite32(CAN_IF_MCONT_EOB, &priv->regs->if2_mcont);
> > +
> > + pch_can_bit_set(&priv->regs->if2_mcont,
> > + CAN_IF_MCONT_UMASK);
>
> dito for if2_mcont
ditto.
>
> > +
> > + 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->if2_cmask);
> > +
> > + pch_can_check_if_busy(&priv->regs->if2_creq, i);
> > + }
> > +
> > + spin_unlock_irqrestore(&priv->msgif_reg_lock, flags);
> > +}
> > +
> > +static void pch_can_init(struct pch_can_priv *priv)
> > +{
> > + /* 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 the interrupts. */
> > + pch_can_set_int_enables(priv, PCH_CAN_ALL);
> > +}
> > +
> > +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) {
>
> is this a valid case?
This "if" is always false.
I will delete this condition.
>
> > + ioread32(&priv->regs->stat);
> > + return;
> > + }
> > +
> > + /* Clear interrupt for transmit object */
> > + if ((mask >= 1) && (mask <= PCH_RX_OBJ_NUM)) {
> > + /* Setting CMASK for clearing the reception interrupts. */
> > + iowrite32(CAN_CMASK_RDWR | CAN_CMASK_CTRL | CAN_CMASK_ARB,
> > + &priv->regs->if1_cmask);
> > +
> > + /* Clearing the Dir bit. */
> > + pch_can_bit_clear(&priv->regs->if1_id2, CAN_ID2_DIR);
> > +
> > + /* Clearing NewDat & IntPnd */
> > + pch_can_bit_clear(&priv->regs->if1_mcont,
> > + CAN_IF_MCONT_NEWDAT | CAN_IF_MCONT_INTPND);
> > +
> > + pch_can_check_if_busy(&priv->regs->if1_creq, mask);
> > + } else if ((mask > PCH_RX_OBJ_NUM) && (mask <= PCH_OBJ_NUM)) {
> > + /* Setting CMASK for clearing interrupts for
> > + frame transmission. */
>
> /*
> * this is the prefered style of multi line comments,
> * please adjust you comments
> */
I understand.
>
> > + 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_if_busy(&priv->regs->if2_creq, mask);
> > + }
> > +}
> > +
> > +static u32 pch_can_get_buffer_status(struct pch_can_priv *priv)
> > +{
> > + return (ioread32(&priv->regs->treq1) & 0xffff) |
> > + ((ioread32(&priv->regs->treq2) & 0xffff) << 16);
>
> the second 0xffff is not needed, as the return value is u32 and you shift by 16.
I agree.
>
> > +}
> > +
> > +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_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);
> > + enum can_state state = priv->can.state;
> > +
> > + skb = alloc_can_err_skb(ndev, &cf);
> > + if (!skb)
> > + return;
> > +
> > + if (status & PCH_BUS_OFF) {
> > + pch_can_tx_disable_all(priv);
> > + pch_can_rx_disable_all(priv);
> > + state = CAN_STATE_BUS_OFF;
> > + cf->can_id |= CAN_ERR_BUSOFF;
> > + can_bus_off(ndev);
> > + }
> > +
> > + /* Warning interrupt. */
> > + if (status & PCH_EWARN) {
> > + 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 -> Error Counter is more than 96.\n", __func__);
>
> Please use just "debug" level not warning here. Consider to use
> netdev_dbg() instead. IMHO the __func__ can be dropped and the
> "official" name for the error is "Error Warning".
I want to know the reason.
Why is it not dev_warn but netdev_dbg ?
>
> > + }
> > + /* Error passive interrupt. */
> > + if (status & PCH_EPASSIV) {
> > + priv->can.can_stats.error_passive++;
> > + 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 -> CAN controller is ERROR PASSIVE .\n", __func__);
>
> dito
ditto
>
> > + }
> > +
> > + if (status & PCH_LEC_ALL) {
> > + priv->can.can_stats.bus_error++;
> > + stats->rx_errors++;
> > + switch (status & PCH_LEC_ALL) {
>
> I suggest to convert to a if-bit-set because there might be more than
> one bit set.
I agree.
>
> > + case PCH_STUF_ERR:
> > + cf->data[2] |= CAN_ERR_PROT_STUFF;
> > + break;
> > + case PCH_FORM_ERR:
> > + cf->data[2] |= CAN_ERR_PROT_FORM;
> > + break;
> > + case PCH_ACK_ERR:
> > + cf->data[2] |= CAN_ERR_PROT_LOC_ACK |
> > + CAN_ERR_PROT_LOC_ACK_DEL;
> > + break;
> > + case PCH_BIT1_ERR:
> > + case PCH_BIT0_ERR:
> > + cf->data[2] |= CAN_ERR_PROT_BIT;
> > + break;
> > + case PCH_CRC_ERR:
> > + cf->data[2] |= CAN_ERR_PROT_LOC_CRC_SEQ |
> > + CAN_ERR_PROT_LOC_CRC_DEL;
> > + break;
> > + default:
> > + iowrite32(status | PCH_LEC_ALL, &priv->regs->stat);
> > + break;
> > + }
> > +
> > + }
> > +
> > + priv->can.state = state;
> > + netif_receive_skb(skb);
> > +
> > + stats->rx_packets++;
> > + stats->rx_bytes += cf->can_dlc;
> > +}
> > +
> > +static irqreturn_t pch_can_interrupt(int irq, void *dev_id)
> > +{
> > + struct net_device *ndev = (struct net_device *)dev_id;
> > + struct pch_can_priv *priv = netdev_priv(ndev);
> > +
> > + pch_can_set_int_enables(priv, PCH_CAN_NONE);
> > + napi_schedule(&priv->napi);
> > +
> > + return IRQ_HANDLED;
> > +}
> > +
> > +static void pch_fifo_thresh(struct pch_can_priv *priv, int obj_id)
> > +{
> > + if (obj_id < PCH_FIFO_THRESH) {
> > + iowrite32(CAN_CMASK_RDWR | CAN_CMASK_CTRL |
> > + CAN_CMASK_ARB, &priv->regs->if1_cmask);
> > +
> > + /* Clearing the Dir bit. */
> > + pch_can_bit_clear(&priv->regs->if1_id2, CAN_ID2_DIR);
> > +
> > + /* Clearing NewDat & IntPnd */
> > + pch_can_bit_clear(&priv->regs->if1_mcont,
> > + CAN_IF_MCONT_INTPND);
> > + pch_can_check_if_busy(&priv->regs->if1_creq, obj_id);
> > + } else if (obj_id > PCH_FIFO_THRESH) {
> > + pch_can_int_clr(priv, obj_id);
> > + } else if (obj_id == PCH_FIFO_THRESH) {
> > + int cnt;
> > + for (cnt = 0; cnt < PCH_FIFO_THRESH; cnt++)
> > + pch_can_int_clr(priv, cnt+1);
> > + }
> > +}
> > +
> > +static int pch_can_rx_msg_lost(struct net_device *ndev, int obj_id)
> > +{
> > + struct pch_can_priv *priv = netdev_priv(ndev);
> > + struct net_device_stats *stats = &(priv->ndev->stats);
> > + struct sk_buff *skb;
> > + struct can_frame *cf;
> > +
> > + dev_err(&priv->ndev->dev, "Msg Obj is overwritten.\n");
> > + pch_can_bit_clear(&priv->regs->if1_mcont,
> > + CAN_IF_MCONT_MSGLOST);
> > + iowrite32(CAN_CMASK_RDWR | CAN_CMASK_CTRL,
> > + &priv->regs->if1_cmask);
> > + pch_can_check_if_busy(&priv->regs->if1_creq, obj_id);
> > +
> > + skb = alloc_can_err_skb(ndev, &cf);
> > + if (!skb)
> > + return -ENOMEM;
> > +
> > + priv->can.can_stats.error_passive++;
> > + priv->can.state = CAN_STATE_ERROR_PASSIVE;
> > + cf->can_id |= CAN_ERR_CRTL;
> > + cf->data[1] = CAN_ERR_CRTL_RX_OVERFLOW;
> > + stats->rx_over_errors++;
> > + stats->rx_errors++;
> > +
> > + netif_receive_skb(skb);
> > +
> > + return 0;
> > +}
> > +
> > +static int pch_can_rx_normal(struct net_device *ndev, u32 obj_num, int quota)
> > +{
> > + u32 reg;
> > + canid_t id;
> > + u32 ide;
> > + u32 rtr;
> > + int rcv_pkts = 0;
> > + int rtn;
> > + int next_flag = 0;
> > + struct sk_buff *skb;
> > + struct can_frame *cf;
> > + struct pch_can_priv *priv = netdev_priv(ndev);
> > + struct net_device_stats *stats = &(priv->ndev->stats);
> > +
> > + /* Reading the messsage object from the Message RAM */
> > + iowrite32(CAN_CMASK_RX_TX_GET, &priv->regs->if1_cmask);
> > + pch_can_check_if_busy(&priv->regs->if1_creq, obj_num);
> > +
> > + /* Reading the MCONT register. */
> > + reg = ioread32(&priv->regs->if1_mcont);
> > + reg &= 0xffff;
> > +
> > + for (; (!(reg & CAN_IF_MCONT_EOB)) && (quota > 0);
> > + obj_num++, next_flag = 0) {
> > + /* If MsgLost bit set. */
> > + if (reg & CAN_IF_MCONT_MSGLOST) {
> > + rtn = pch_can_rx_msg_lost(ndev, obj_num);
> > + if (!rtn)
> > + return rtn;
> > + rcv_pkts++;
> > + quota--;
> > + next_flag = 1;
> > + } else if (!(reg & CAN_IF_MCONT_NEWDAT))
> > + next_flag = 1;
> > +
>
> after rearanging the code (see below..) you should be able to use a continue here.
>
> > + if (!next_flag) {
> > + skb = alloc_can_skb(priv->ndev, &cf);
> > + if (!skb)
> > + return -ENOMEM;
> > +
> > + /* Get Received data */
> > + ide = ((ioread32(&priv->regs->if1_id2)) & CAN_ID2_XTD);
> > + if (ide) {
> > + id = (ioread32(&priv->regs->if1_id1) & 0xffff);
> > + id |= (((ioread32(&priv->regs->if1_id2)) &
> > + 0x1fff) << 16);
> > + cf->can_id = (id & CAN_EFF_MASK) | CAN_EFF_FLAG;
> ^^^^^^^^^^^^^^^^^
>
> is the mask needed, you mask the if1_id{1,2} already
I will delete
>
> > + } else {
> > + id = (((ioread32(&priv->regs->if1_id2)) &
> > + (CAN_SFF_MASK << 2)) >> 2);
> > + cf->can_id = (id & CAN_SFF_MASK);
>
> one mask can go away
I agree.
>
> > + }
> > +
> > + rtr = ioread32(&priv->regs->if1_id2) & CAN_ID2_DIR;
> ^^
>
> remove one space
I agree.
>
> > +
> > + if (rtr)
> > + cf->can_id |= CAN_RTR_FLAG;
> > +
> > + cf->can_dlc = get_can_dlc((ioread32(&priv->regs->
> > + if1_mcont)) & 0xF);
> > + *(u16 *)(cf->data + 0) = ioread16(&priv->regs->
> > + if1_dataa1);
> > + *(u16 *)(cf->data + 2) = ioread16(&priv->regs->
> > + if1_dataa2);
> > + *(u16 *)(cf->data + 4) = ioread16(&priv->regs->
> > + if1_datab1);
> > + *(u16 *)(cf->data + 6) = ioread16(&priv->regs->
> > + if1_datab2);
>
> are you sure, the bytes in the can package a in the correct order.
> Please test your pch_can against a non pch_can system.
Unfortunately, we don't have non pch_can system.
>
> > +
> > + netif_receive_skb(skb);
> > + rcv_pkts++;
> > + stats->rx_packets++;
> > + quota--;
> > + stats->rx_bytes += cf->can_dlc;
> > +
> > + pch_fifo_thresh(priv, obj_num);
> > + }
> > +
> > + /* Reading the messsage object from the Message RAM */
> > + iowrite32(CAN_CMASK_RX_TX_GET, &priv->regs->if1_cmask);
> > + pch_can_check_if_busy(&priv->regs->if1_creq, obj_num + 1);
> > + reg = ioread32(&priv->regs->if1_mcont);
>
> this is almost the same code as before the the loop, can you rearange
> the code to avoid duplication?
I agree.
>
> > + }
> > +
> > + return rcv_pkts;
> > +}
> > +
> > +static void pch_can_tx_complete(struct net_device *ndev, u32 int_stat)
> > +{
> > + struct pch_can_priv *priv = netdev_priv(ndev);
> > + struct net_device_stats *stats = &(priv->ndev->stats);
> > + unsigned long flags;
> > + u32 dlc;
> > +
> > + can_get_echo_skb(ndev, int_stat - PCH_RX_OBJ_NUM - 1);
> > + spin_lock_irqsave(&priv->msgif_reg_lock, flags);
> > + iowrite32(CAN_CMASK_RX_TX_GET | CAN_CMASK_CLRINTPND,
> > + &priv->regs->if2_cmask);
> > + dlc = ioread32(&priv->regs->if2_mcont) & CAN_IF_MCONT_DLC;
> > + pch_can_check_if_busy(&priv->regs->if2_creq, int_stat);
> > + spin_unlock_irqrestore(&priv->msgif_reg_lock, flags);
> > + if (dlc > 8)
> > + dlc = 8;
>
> use get_can_dlc
I agree.
>
> > + stats->tx_bytes += dlc;
> > + stats->tx_packets++;
> > +}
> > +
> > +static int pch_can_rx_poll(struct napi_struct *napi, int quota)
> > +{
> > + struct net_device *ndev = napi->dev;
> > + struct pch_can_priv *priv = netdev_priv(ndev);
> > + u32 int_stat;
> > + int rcv_pkts = 0;
> > + u32 reg_stat;
> > + unsigned long flags;
> > +
> > + int_stat = pch_can_int_pending(priv);
> > + if (!int_stat)
> > + goto END;
> > +
> > + if ((int_stat == CAN_STATUS_INT) && (quota > 0)) {
> > + reg_stat = ioread32(&priv->regs->stat);
> > + if (reg_stat & (PCH_BUS_OFF | PCH_LEC_ALL)) {
> > + if ((reg_stat & PCH_LEC_ALL) != PCH_LEC_ALL) {
> > + pch_can_error(ndev, reg_stat);
> > + quota--;
> > + }
> > + }
> > +
> > + if (reg_stat & PCH_TX_OK) {
> > + spin_lock_irqsave(&priv->msgif_reg_lock, flags);
> > + iowrite32(CAN_CMASK_RX_TX_GET, &priv->regs->if2_cmask);
> > + pch_can_check_if_busy(&priv->regs->if2_creq,
> > + ioread32(&priv->regs->intr));
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
> Isn't this "int_stat". Might it be possilbe that regs->intr changes
> between the pch_can_int_pending and here?
This code was mistake.
This condition, message object is not acccessed.
Thus, pch_can_check_if_busy can be deleted.
>
> What should this transfer do?
>
> > + spin_unlock_irqrestore(&priv->msgif_reg_lock, flags);
> > + pch_can_bit_clear(&priv->regs->stat, PCH_TX_OK);
> > + }
> > +
> > + if (reg_stat & PCH_RX_OK)
> > + pch_can_bit_clear(&priv->regs->stat, PCH_RX_OK);
> > +
> > + int_stat = pch_can_int_pending(priv);
> > + }
> > +
> > + if (quota == 0)
> > + goto END;
> > +
> > + if ((int_stat >= 1) && (int_stat <= PCH_RX_OBJ_NUM)) {
> > + spin_lock_irqsave(&priv->msgif_reg_lock, flags);
> > + rcv_pkts += pch_can_rx_normal(ndev, int_stat, quota);
> > + spin_unlock_irqrestore(&priv->msgif_reg_lock, flags);
> > + quota -= rcv_pkts;
> > + if (rcv_pkts < 0)
>
> how can this happen?
My mistake.
if (quota < 0) is TRUE.
>
> > + goto END;
> > + } else if ((int_stat > PCH_RX_OBJ_NUM) && (int_stat <= PCH_OBJ_NUM)) {
> > + /* Handle transmission interrupt */
> > + pch_can_tx_complete(ndev, int_stat);
> > + }
> > +
> > +END:
> > + napi_complete(napi);
> > + pch_can_set_int_enables(priv, PCH_CAN_ALL);
> > +
> > + return rcv_pkts;
> > +}
> > +
> > +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 canbit;
> > + u32 bepe;
> > +
> > + /* Setting the CCE bit for accessing the Can Timing register. */
> > + pch_can_bit_set(&priv->regs->cont, CAN_CTRL_CCE);
> > +
> > + canbit = (bt->brp - 1) & MSK_BITT_BRP;
> > + canbit |= (bt->sjw - 1) << BIT_BITT_SJW;
> > + canbit |= (bt->phase_seg1 + bt->prop_seg - 1) << BIT_BITT_TSEG1;
> > + canbit |= (bt->phase_seg2 - 1) << BIT_BITT_TSEG2;
> > + bepe = ((bt->brp - 1) & MSK_BRPE_BRPE) >> BIT_BRPE_BRPE;
> > + iowrite32(canbit, &priv->regs->bitt);
> > + iowrite32(bepe, &priv->regs->brpe);
> > + pch_can_bit_clear(&priv->regs->cont, CAN_CTRL_CCE);
> > +
> > + 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);
> > +
> > + pch_can_tx_enable_all(priv);
> > + pch_can_rx_enable_all(priv);
> > +
> > + /* Setting the CAN to run mode. */
> > + pch_can_set_run_mode(priv, PCH_CAN_RUN);
> > +
> > + priv->can.state = CAN_STATE_ERROR_ACTIVE;
> > +
> > + 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;
> > +}
> > +
> > +static int pch_can_open(struct net_device *ndev)
> > +{
> > + struct pch_can_priv *priv = netdev_priv(ndev);
> > + int retval;
> > +
> > + /* Regsitering the interrupt. */
> > + retval = request_irq(priv->dev->irq, pch_can_interrupt, IRQF_SHARED,
> > + ndev->name, ndev);
> > + if (retval) {
> > + dev_err(&ndev->dev, "request_irq failed.\n");
> > + goto req_irq_err;
> > + }
> > +
> > + /* 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_init(priv);
> > + pch_can_start(ndev);
> > + napi_enable(&priv->napi);
> > + netif_start_queue(ndev);
> > +
> > + return 0;
> > +
> > +err_open_candev:
> > + free_irq(priv->dev->irq, ndev);
> > +req_irq_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);
> > + napi_disable(&priv->napi);
> > + pch_can_release(priv);
> > + free_irq(priv->dev->irq, ndev);
> > + close_candev(ndev);
> > + priv->can.state = CAN_STATE_STOPPED;
> > + return 0;
> > +}
> > +
> > +static netdev_tx_t pch_xmit(struct sk_buff *skb, struct net_device *ndev)
> > +{
> > + unsigned long flags;
> > + struct pch_can_priv *priv = netdev_priv(ndev);
> > + struct can_frame *cf = (struct can_frame *)skb->data;
> > + int tx_buffer_avail = 0;
>
> What I'm totally missing is the TX flow controll. Your driver has to
> ensure that the package leave the controller in the order that come
> into the xmit function. Further you have to stop your xmit queue if
> you're out of tx objects and reenable if you have a object free.
>
> Use netif_stop_queue() and netif_wake_queue() for this.
In this code, I think "out of tx objects" cannot be occurred.
Nevertheless, are netif_stop_queue() and netif_wake_queue() is necessary ?
>
> > +
> > + if (can_dropped_invalid_skb(ndev, skb))
> > + return NETDEV_TX_OK;
> > +
> > + if (priv->tx_obj == (PCH_OBJ_NUM + 1)) { /* Point tail Obj + 1 */
> > + while (ioread32(&priv->regs->treq2) & 0xfc00)
> > + udelay(1);
>
> please no (possible) infinite delays!
I will add break processing.
>
> > + priv->tx_obj = PCH_RX_OBJ_NUM + 1; /* Point head of Tx Obj ID */
> > + }
>
> > +
> > + tx_buffer_avail = priv->tx_obj;
>
> why has the "object" become a "buffer" now? :)
You are right.
I will modify the name.
>
> > + priv->tx_obj++;
> > +
> > + /* Attaining the lock. */
> > + spin_lock_irqsave(&priv->msgif_reg_lock, flags);
> > +
> > + /* Setting the CMASK register to set value*/
> ^^^
>
> pleas add a whitespace
I agree.
>
> > + iowrite32(CAN_CMASK_RX_TX_SET, &priv->regs->if2_cmask);
> > +
> > + /* If ID extended is set. */
> > + if (cf->can_id & CAN_EFF_FLAG) {
> > + iowrite32(cf->can_id & 0xffff, &priv->regs->if2_id1);
> > + iowrite32(((cf->can_id >> 16) & 0x1fff) | CAN_ID2_XTD,
> > + &priv->regs->if2_id2);
> > + } else {
> > + iowrite32(0, &priv->regs->if2_id1);
> > + iowrite32((cf->can_id & CAN_SFF_MASK) << 2,
> > + &priv->regs->if2_id2);
> > + }
> > +
> > + pch_can_bit_set(&priv->regs->if2_id2, CAN_ID_MSGVAL);
>
> Do you need to do a read-modify-write of the hardware register? Please
> prepare the values you want to write to hardware, then do it.
Current design policy for read/write message object,
the driver is designed with Read-Modify-Write.
I will modify to Write only for reducing accessing Message RAM.
>
> > +
> > + /* If remote frame has to be transmitted.. */
> > + if (!(cf->can_id & CAN_RTR_FLAG))
> > + pch_can_bit_set(&priv->regs->if2_id2, CAN_ID2_DIR);
> dito
> > + /* If remote frame has to be transmitted.. */
> > + if (cf->can_id & CAN_RTR_FLAG)
> > + pch_can_bit_clear(&priv->regs->if2_id2, CAN_ID2_DIR);
> dito
> > +
> > + /* Copy data to register */
> > + if (cf->can_dlc > 0) {
> > + u32 data1 = *((u16 *)&cf->data[0]);
> > + iowrite32(data1, &priv->regs->if2_dataa1);
>
> do you think you send the bytes in correct order?
Let me study this endianess.
>
> > + }
> > + if (cf->can_dlc > 2) {
> > + u32 data1 = *((u16 *)&cf->data[2]);
> > + iowrite32(data1, &priv->regs->if2_dataa2);
> > + }
> > + if (cf->can_dlc > 4) {
> > + u32 data1 = *((u16 *)&cf->data[4]);
> > + iowrite32(data1, &priv->regs->if2_datab1);
> > + }
> > + if (cf->can_dlc > 6) {
> > + u32 data1 = *((u16 *)&cf->data[6]);
> > + iowrite32(data1, &priv->regs->if2_datab2);
> > + }
> > +
> > + can_put_echo_skb(skb, ndev, tx_buffer_avail - PCH_RX_OBJ_NUM - 1);
> > +
> > + /* Set the size of the data. */
> > + iowrite32(cf->can_dlc, &priv->regs->if2_mcont);
> > +
> > + /* Update if2_mcont */
> > + pch_can_bit_set(&priv->regs->if2_mcont,
> > + CAN_IF_MCONT_NEWDAT | CAN_IF_MCONT_TXRQXT |
> > + CAN_IF_MCONT_TXIE);
>
> pleae first perpare your value, then write to hardware.
ditto.
>
> > +
> > + if (tx_buffer_avail == PCH_RX_OBJ_NUM) /* If points tail of FIFO */
> > + pch_can_bit_set(&priv->regs->if2_mcont, CAN_IF_MCONT_EOB);
>
> dito
>
> Is EOB relevant for TX objects?
This is mistake. No meaning for tx.
I will modify.
>
> > + pch_can_check_if_busy(&priv->regs->if2_creq, tx_buffer_avail);
> > + spin_unlock_irqrestore(&priv->msgif_reg_lock, flags);
> > +
> > + return NETDEV_TX_OK;
> > +}
> > +
> > +static const struct net_device_ops pch_can_netdev_ops = {
> > + .ndo_open = pch_can_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);
> > + pci_iounmap(pdev, priv->regs);
> > + if (priv->use_msi)
> > + pci_disable_msi(priv->dev);
> > + pci_release_regions(pdev);
> > + pci_disable_device(pdev);
> > + pci_set_drvdata(pdev, NULL);
> > + free_candev(priv->ndev);
> > +}
> > +
> > +#ifdef CONFIG_PM
> > +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 u32 pch_can_get_int_enables(struct pch_can_priv *priv)
> > +{
> > + /* Obtaining the status of IE, SIE and EIE interrupt bits. */
> > + return (ioread32(&priv->regs->cont) & CAN_CTRL_IE_SIE_EIE) >> 1;
> > +}
> > +
> > +static u32 pch_can_get_rx_enable(struct pch_can_priv *priv, u32 buff_num)
> > +{
> > + unsigned long flags;
> > + u32 enable;
> > +
> > + spin_lock_irqsave(&priv->msgif_reg_lock, flags);
> > + iowrite32(CAN_CMASK_RX_TX_GET, &priv->regs->if1_cmask);
> > + pch_can_check_if_busy(&priv->regs->if1_creq, buff_num);
> > +
> > + if (((ioread32(&priv->regs->if1_id2)) & CAN_ID_MSGVAL) &&
> > + ((ioread32(&priv->regs->if1_mcont)) &
> > + CAN_IF_MCONT_RXIE))
> > + enable = 1;
> > + else
> > + enable = 0;
> > + spin_unlock_irqrestore(&priv->msgif_reg_lock, flags);
> > + return enable;
> > +}
> > +
> > +static u32 pch_can_get_tx_enable(struct pch_can_priv *priv, u32 buff_num)
> > +{
> > + unsigned long flags;
> > + u32 enable;
> > +
> > + spin_lock_irqsave(&priv->msgif_reg_lock, flags);
> > +
> > + iowrite32(CAN_CMASK_RX_TX_GET, &priv->regs->if2_cmask);
> > + pch_can_check_if_busy(&priv->regs->if2_creq, buff_num);
> > + if (((ioread32(&priv->regs->if2_id2)) & CAN_ID_MSGVAL) &&
> > + ((ioread32(&priv->regs->if2_mcont)) &
> > + CAN_IF_MCONT_TXIE)) {
> > + enable = 1;
> > + } else {
> > + enable = 0;
> > + }
> > + spin_unlock_irqrestore(&priv->msgif_reg_lock, flags);
> > +
> > + return enable;
> > +}
> > +
> > +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_if_busy(&priv->regs->if1_creq, buffer_num);
> > + iowrite32(CAN_CMASK_RDWR | CAN_CMASK_CTRL, &priv->regs->if1_cmask);
> > + if (set == 1)
> > + 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_if_busy(&priv->regs->if1_creq, buffer_num);
> > + spin_unlock_irqrestore(&priv->msgif_reg_lock, flags);
> > +}
> > +
> > +static u32 pch_can_get_rx_buffer_link(struct pch_can_priv *priv, u32 buffer_num)
> > +{
> > + unsigned long flags;
> > + u32 link;
> > +
> > + spin_lock_irqsave(&priv->msgif_reg_lock, flags);
> > + iowrite32(CAN_CMASK_RX_TX_GET, &priv->regs->if1_cmask);
> > + pch_can_check_if_busy(&priv->regs->if1_creq, buffer_num);
> > +
> > + if (ioread32(&priv->regs->if1_mcont) & CAN_IF_MCONT_EOB)
> > + link = 0;
> > + else
> > + link = 1;
> > + spin_unlock_irqrestore(&priv->msgif_reg_lock, flags);
> > + return link;
> > +}
> > +
> > +static int pch_can_suspend(struct pci_dev *pdev, pm_message_t state)
> > +{
> > + int i;
> > + int retval;
> > + u32 buf_stat; /* Variable for reading the transmit buffer status. */
> > + u32 counter = COUNTER_LIMIT;
> > +
> > + 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_STOPPED;
> > +
> > + /* Waiting for all transmission to complete. */
> > + while (counter) {
> > + buf_stat = pch_can_get_buffer_status(priv);
> > + if (!buf_stat)
> > + break;
> > + counter--;
> > + udelay(1);
> > + }
> > + if (!counter)
> > + dev_err(&pdev->dev, "%s -> Transmission time out.\n", __func__);
> > +
> > + /* Save interrupt configuration and then disable them */
> > + priv->int_enables = pch_can_get_int_enables(priv);
> > + pch_can_set_int_enables(priv, PCH_CAN_DISABLE);
> > +
> > + /* Save Tx buffer enable state */
> > + for (i = PCH_RX_OBJ_NUM + 1; i <= PCH_OBJ_NUM; i++)
> > + priv->tx_enable[i] = pch_can_get_tx_enable(priv, i);
> > +
> > + /* Disable all Transmit buffers */
> > + pch_can_tx_disable_all(priv);
> > +
> > + /* Save Rx buffer enable state */
> > + for (i = 1; i <= PCH_RX_OBJ_NUM; i++) {
> > + priv->rx_enable[i] = pch_can_get_rx_enable(priv, i);
> > + priv->rx_link[i] = pch_can_get_rx_buffer_link(priv, 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;
> > + int retval;
> > + 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 = 1; i <= PCH_RX_OBJ_NUM; i++)
> > + pch_can_set_tx_enable(priv, i, priv->tx_enable[i]);
> > +
> > + /* Configuring the receive buffer and enabling them. */
> > + for (i = PCH_RX_OBJ_NUM + 1; i <= PCH_OBJ_NUM; i++) {
> > + /* Restore buffer link */
> > + pch_can_set_rx_buffer_link(priv, i, priv->rx_link[i]);
> > +
> > + /* Restore buffer enables */
> > + pch_can_set_rx_enable(priv, i, 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;
> > +}
> > +#else
> > +#define pch_can_suspend NULL
> > +#define pch_can_resume NULL
> > +#endif
> > +
> > +static int pch_can_get_berr_counter(const struct net_device *dev,
> > + struct can_berr_counter *bec)
> > +{
> > + struct pch_can_priv *priv = netdev_priv(dev);
> > +
> > + bec->txerr = ioread32(&priv->regs->errc) & CAN_TEC;
> > + bec->rxerr = (ioread32(&priv->regs->errc) & CAN_REC) >> 8;
> > +
> > + return 0;
> > +}
> > +
> > +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;
> > + 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), PCH_TX_OBJ_NUM);
> > + 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->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_berr_counter = pch_can_get_berr_counter;
> > + priv->can.ctrlmode_supported = CAN_CTRLMODE_LISTENONLY |
> > + CAN_CTRLMODE_LOOPBACK;
> > + priv->tx_obj = PCH_RX_OBJ_NUM + 1; /* Point head of Tx Obj */
> > +
> > + 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; /* Hz */
> > +
> > + netif_napi_add(ndev, &priv->napi, pch_can_rx_poll, PCH_RX_OBJ_NUM);
> > +
> > + rc = pci_enable_msi(priv->dev);
> > + if (rc) {
> > + dev_info(&ndev->dev, "PCH CAN opened without MSI\n");
> > + priv->use_msi = 0;
> > + } else {
> > + dev_info(&ndev->dev, "PCH CAN opened with MSI\n");
> > + priv->use_msi = 1;
> > + }
> > +
> > + 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 = "pch_can",
> > + .id_table = pch_pci_tbl,
> > + .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("Intel EG20T PCH CAN(Controller Area Network) Driver");
> > +MODULE_LICENSE("GPL v2");
> > +MODULE_VERSION("0.94");
>
> cheers, Marc
>
> --
> Pengutronix e.K. | Marc Kleine-Budde |
> Industrial Linux Solutions | Phone: +49-231-2826-924 |
> Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
> Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
>
>
Thanks, Tomoya(OKI SEMICONDUCTOR CO., LTD.)
^ permalink raw reply
* Re: [PATCH net-next-2.6 v2] can: Topcliff: PCH_CAN driver: Fix build warnings
From: Marc Kleine-Budde @ 2010-11-02 11:03 UTC (permalink / raw)
To: Tomoya MORINAGA
Cc: andrew.chih.howe.khor-ral2JQCrhuEAvxtiuMwx3w, Masayuki Ohtake,
Samuel Ortiz, margie.foster-ral2JQCrhuEAvxtiuMwx3w,
netdev-u79uwXL29TY76Z2rM5mHXA, LKML,
socketcan-core-0fE9KPoRgkgATYTw5x5z8w,
yong.y.wang-ral2JQCrhuEAvxtiuMwx3w,
kok.howg.ewe-ral2JQCrhuEAvxtiuMwx3w, Wolfgang Grandegger,
joel.clark-ral2JQCrhuEAvxtiuMwx3w, David S. Miller,
Christian Pellegrin, qi.wang-ral2JQCrhuEAvxtiuMwx3w
In-Reply-To: <001701cb7a78$99e1fe20$66f8800a-a06+6cuVnkTSQfdrb5gaxUEOCMrvLtNR@public.gmane.org>
[-- Attachment #1.1: Type: text/plain, Size: 57708 bytes --]
On 11/02/2010 11:27 AM, Tomoya MORINAGA wrote:
> On Saturday, October 30, 2010 1:23 AM, Marc Kleine-Budde wrote:
>
>>> The driver has already been merged. Please send incremental patches
>>> against david's net-2.6 branch.
>
> I agree.
>
>>
>> Here a review, find comments inline. Lets talk about my remarks, please
>> answer inline and don't delete the code.
>>
>> Can you please explain me your locking sheme? If I understand the
>> documenation correctly the two message interfaces can be used mutual.
>> And you use one for rx the other one for tx.
>
> I show our locking scheme.
> When CPU accesses MessageRAM via IF1, CPU protect until read-modify-write
> so that IF2 access not occurred, vice versa.
Why is that needed?
>
>>
>> Please use netdev_<level> instead of dev_<level> for debug.
>
> I agree.
>
>>
>>> --- /dev/null
>>> +++ b/drivers/net/can/pch_can.c
>>> @@ -0,0 +1,1436 @@
>>> +/*
>>> + * 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_MSG_OBJ 32
>>> +#define MSG_OBJ_RX 0 /* The receive message object flag. */
>>> +#define MSG_OBJ_TX 1 /* The transmit message object 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_CMASK_NEWDAT 0x04
>>> +#define CAN_CMASK_CLRINTPND 0x08
>>> +
>>> +#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_DLC 0x000f
>>> +#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
>>
>> A prefix for like PCH_ instead of CAN_ for all those define above would
>> be fine to avoid namespace clashes and/or confusion with the defines from the socketcan framework.
>>
>
> I agree.
>
>>> +
>>> +#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
>>
>> These are just single set bit, please use BIT()
>> Consider adding the name of the corresponding register to the define's
>> name.
>
> I agree.
>
>>
>>> +#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)
>>> +
>>> +/* 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
>>
>> these are usually called SHIFT
>
> I agree. Is the below TRUE ?
> e.g.#define PCH_SHIFT_BITT_BRP 0
I would put the SHIFT at the end, YMMV
#define PCH_BIT_BRP_SHIFT
>
>>
>>> +#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
>>
>> MSK or MASK is okay, however the last two are just single bits.
>>
>> Please add a PCH_ prefix here, too.
>
> I agree.
>
>>
>>> +#define PCH_CAN_NO_TX_BUFF 1
>>> +#define COUNTER_LIMIT 10
>>
>> dito
>
> I agree.
>
>>
>>> +
>>> +#define PCH_CAN_CLK 50000000 /* 50MHz */
>>> +
>>> +/*
>>> + * Define the number of message object.
>>> + * PCH CAN communications are done via Message RAM.
>>> + * The Message RAM consists of 32 message objects.
>>> + */
>>> +#define PCH_RX_OBJ_NUM 26 /* 1~ PCH_RX_OBJ_NUM is Rx*/
>>> +#define PCH_TX_OBJ_NUM 6 /* PCH_RX_OBJ_NUM is RX ~ Tx*/
>>> +#define PCH_OBJ_NUM (PCH_TX_OBJ_NUM + PCH_RX_OBJ_NUM)
>>
>> You define MAX_MSG_OBJ earlier, seems like two names for the same value.
>
> In case, a use uses all message objects(=32), you are right.
> But user does not alway use all message object.
No one will change these values if the driver isn't buggy. And it
doesn't make any sense to not use all objects.
>
>
>>
>>> +
>>> +#define PCH_FIFO_THRESH 16
>>> +
>>> +enum pch_can_mode {
>>> + PCH_CAN_ENABLE,
>>> + PCH_CAN_DISABLE,
>>> + PCH_CAN_ALL,
>>> + PCH_CAN_NONE,
>>> + PCH_CAN_STOP,
>>> + PCH_CAN_RUN,
>>> +};
>>> +
>>> +struct pch_can_regs {
>>> + u32 cont;
>>> + u32 stat;
>>> + u32 errc;
>>> + u32 bitt;
>>> + u32 intr;
>>> + u32 opt;
>>> + u32 brpe;
>>> + u32 reserve1;
>>
>> VVVV
>>> + 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;
>> ^^^^
>>
>> these registers and....
>>
>>> + u32 reserve2;
>>> + u32 reserve3[12];
>>
>> ...and these
>>
>> VVVV
>>> + 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;
>>
>> ^^^^
>>
>> ...are identical. I suggest to make a struct defining a complete
>> "Message Interface Register Set". If you include the correct number of
>> reserved bytes in the struct, you can have an array of two of these
>> structs in the struct pch_can_regs.
>
> To me, IMHOHO, it looks insignificant point.
> Please show the merit ?
See Wolfgangs comments. You can get rid of duplicated code....
>
>>
>>> + u32 reserve4;
>>> + u32 reserve5[20];
>>> + u32 treq1;
>>> + u32 treq2;
>>> + u32 reserve6[2];
>>> + u32 reserve7[56];
>>> + u32 reserve8[3];
>>> + u32 srst;
>>> +};
>>> +
>>> +struct pch_can_priv {
>>> + struct can_priv can;
>>> + 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;
>>> + struct net_device *ndev;
>>> + spinlock_t msgif_reg_lock; /* Message Interface Registers Access Lock*/
>> ^^^
>> please add a whitespace
>
> I agree.
>
>>
>>> + unsigned int msg_obj[MAX_MSG_OBJ];
>>> + struct pch_can_regs __iomem *regs;
>>> + struct napi_struct napi;
>>> + unsigned int tx_obj; /* Point next Tx Obj index */
>>> + unsigned int use_msi;
>>> +};
>>> +
>>> +static struct can_bittiming_const pch_can_bittiming_const = {
>>> + .name = "pch_can",
>>> + .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 DEFINE_PCI_DEVICE_TABLE(pch_pci_tbl) = {
>>> + {PCI_VENDOR_ID_INTEL, 0x8818, PCI_ANY_ID, PCI_ANY_ID,},
>>> + {0,}
>>> +};
>>> +MODULE_DEVICE_TABLE(pci, pch_pci_tbl);
>>> +
>>> +static inline void pch_can_bit_set(u32 *addr, u32 mask)
>> ^^^^^
>>
>> that should be an void __iomem *, see mail I've send the other day.
>> Please use sparse to check for this kinds of errors.
>> (Compile the driver with C=2, i.e.: make drivers/net/can/pch_can.ko C=2)
>>
>
> I agree.
>
>>> +{
>>> + iowrite32(ioread32(addr) | mask, addr);
>>> +}
>>> +
>>> +static inline void pch_can_bit_clear(u32 *addr, u32 mask)
>> ^^^^^
>>
>> dito
>
> I agree.
>
>>
>>> +{
>>> + iowrite32(ioread32(addr) & ~mask, addr);
>>> +}
>>> +
>>> +static void pch_can_set_run_mode(struct pch_can_priv *priv,
>>> + enum pch_can_mode 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_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);
>>> +}
>>> +
>>
>> IMHO the function name is missleading, if I understand the code
>> correctly, this functions triggers the transmission of the message.
>> After this it checks for busy,
>
> Yes, your understanding is TRUE.
>
>> but
>>
>>> +static void pch_can_check_if_busy(u32 __iomem *creq_addr, u32 num)
>> ^^^^
>>
>
> Yes, me too.
> I will rename the function name.
>
> How about "pch_can_rw_msg_obj"
>
>> that should probaby be a void
> What't the above mean ?
> pch_can_check_if_busy is already "void" function.
>>> +static void pch_can_check_if_busy(u32 __iomem *creq_addr, u32 num)
>> ^^^^
That u32 should be a void.
BTW: Does the Intel chip support x64? If so, have you tested the driver
on a 64 bit kernel.
>
>>
>>> +{
>>> + u32 counter = COUNTER_LIMIT;
>>> + u32 ifx_creq;
>>> +
>>> + iowrite32(num, creq_addr);
>>> + while (counter) {
>>> + ifx_creq = ioread32(creq_addr) & CAN_IF_CREQ_BUSY;
>>> + if (!ifx_creq)
>>> + break;
>>> + counter--;
>>> + udelay(1);
>>> + }
>>> + if (!counter)
>>> + pr_err("%s:IF1 BUSY Flag is set forever.\n", __func__);
>>> +}
>>> +
>>> +static void pch_can_set_int_enables(struct pch_can_priv *priv,
>>> + enum pch_can_mode interrupt_no)
>>> +{
>>> + switch (interrupt_no) {
>>> + case PCH_CAN_ENABLE:
>>> + pch_can_bit_set(&priv->regs->cont, CAN_CTRL_IE);
>>
>> noone uses this case.
>
> I agree.
>
>>
>>> + 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_set_rx_enable(struct pch_can_priv *priv, u32 buff_num,
>>> + int set)
>>> +{
>>> + unsigned long flags;
>>> +
>>> + spin_lock_irqsave(&priv->msgif_reg_lock, flags);
>>> + /* Reading the receive buffer data from RAM to Interface1 registers */
>>> + iowrite32(CAN_CMASK_RX_TX_GET, &priv->regs->if1_cmask);
>>> + pch_can_check_if_busy(&priv->regs->if1_creq, buff_num);
>>> +
>>> + /* 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 == 1) {
>>> + /* 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 == 0) {
>>> + /* 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_if_busy(&priv->regs->if1_creq, buff_num);
>>> + spin_unlock_irqrestore(&priv->msgif_reg_lock, flags);
>>> +}
>>> +
>>> +static void pch_can_rx_enable_all(struct pch_can_priv *priv)
>>> +{
>>> + int i;
>>> +
>>> + /* Traversing to obtain the object configured as receivers. */
>>> + for (i = 1; i <= PCH_RX_OBJ_NUM; i++)
>>> + pch_can_set_rx_enable(priv, i, 1);
>>> +}
>>> +
>>> +static void pch_can_rx_disable_all(struct pch_can_priv *priv)
>>> +{
>>> + int i;
>>> +
>>> + /* Traversing to obtain the object configured as receivers. */
>>> + for (i = 1; i <= PCH_RX_OBJ_NUM; i++)
>>> + pch_can_set_rx_enable(priv, i, 0);
>>> +}
>>> +
>>> +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->if2_cmask);
>>> + pch_can_check_if_busy(&priv->regs->if2_creq, 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->if2_cmask);
>>> +
>>> + if (set == 1) {
>>> + /* Setting the MsgVal and TxIE bits */
>>> + pch_can_bit_set(&priv->regs->if2_mcont, CAN_IF_MCONT_TXIE);
>>> + pch_can_bit_set(&priv->regs->if2_id2, CAN_ID_MSGVAL);
>>> + } else if (set == 0) {
>>> + /* Resetting the MsgVal and TxIE bits. */
>>> + pch_can_bit_clear(&priv->regs->if2_mcont, CAN_IF_MCONT_TXIE);
>>> + pch_can_bit_clear(&priv->regs->if2_id2, CAN_ID_MSGVAL);
>>> + }
>>> +
>>> + pch_can_check_if_busy(&priv->regs->if2_creq, buff_num);
>>> + spin_unlock_irqrestore(&priv->msgif_reg_lock, flags);
>>> +}
>>> +
>>> +static void pch_can_tx_enable_all(struct pch_can_priv *priv)
>>> +{
>>> + int i;
>>> +
>>> + /* Traversing to obtain the object configured as transmit object. */
>>> + for (i = PCH_RX_OBJ_NUM + 1; i <= PCH_OBJ_NUM; i++)
>>> + pch_can_set_tx_enable(priv, i, 1);
>>> +}
>>> +
>>> +static void pch_can_tx_disable_all(struct pch_can_priv *priv)
>>> +{
>>> + int i;
>>> +
>>> + /* Traversing to obtain the object configured as transmit object. */
>>> + for (i = PCH_RX_OBJ_NUM + 1; i <= PCH_OBJ_NUM; i++)
>>> + pch_can_set_tx_enable(priv, i, 0);
>>> +}
>>> +
>>> +static int pch_can_int_pending(struct pch_can_priv *priv)
>> ^^^
>>
>> make it u32 as it returns a register value, or a u16 as you only use
>> the 16 lower bits.
>
> I agree. I will modify to u32.
>
>>
>>> +{
>>> + return ioread32(&priv->regs->intr) & 0xffff;
>>> +}
>>> +
>>> +static void pch_can_clear_buffers(struct pch_can_priv *priv)
>>> +{
>>> + int i; /* Msg Obj ID (1~32) */
>>> +
>>> + for (i = 1; i <= PCH_RX_OBJ_NUM; i++) {
>>
>> IMHO the readability would be improved if you define something like
>> PCH_RX_OBJ_START and PCH_RX_OBJ_END.
>
> I agree.
>
>>
>>> + iowrite32(CAN_CMASK_RX_TX_SET, &priv->regs->if1_cmask);
>>> + iowrite32(0xffff, &priv->regs->if1_mask1);
>>> + iowrite32(0xffff, &priv->regs->if1_mask2);
>>> + iowrite32(0x0, &priv->regs->if1_id1);
>>> + iowrite32(0x0, &priv->regs->if1_id2);
>>> + iowrite32(0x0, &priv->regs->if1_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(CAN_CMASK_RDWR | CAN_CMASK_MASK |
>>> + CAN_CMASK_ARB | CAN_CMASK_CTRL,
>>> + &priv->regs->if1_cmask);
>>> + pch_can_check_if_busy(&priv->regs->if1_creq, i);
>>> + }
>>> +
>>> + for (i = PCH_RX_OBJ_NUM + 1; i <= PCH_OBJ_NUM; i++) {
>> ^^^^^^^^^^^^^^^^^^
>> dito for TX objects
>
> I agree.
>
>>
>>> + iowrite32(CAN_CMASK_RX_TX_SET, &priv->regs->if2_cmask);
>>> + iowrite32(0xffff, &priv->regs->if2_mask1);
>>> + iowrite32(0xffff, &priv->regs->if2_mask2);
>>> + iowrite32(0x0, &priv->regs->if2_id1);
>>> + iowrite32(0x0, &priv->regs->if2_id2);
>>> + iowrite32(0x0, &priv->regs->if2_mcont);
>>> + iowrite32(0x0, &priv->regs->if2_dataa1);
>>> + iowrite32(0x0, &priv->regs->if2_dataa2);
>>> + iowrite32(0x0, &priv->regs->if2_datab1);
>>> + iowrite32(0x0, &priv->regs->if2_datab2);
>>> + iowrite32(CAN_CMASK_RDWR | CAN_CMASK_MASK | CAN_CMASK_ARB |
>>> + CAN_CMASK_CTRL, &priv->regs->if2_cmask);
>>> + pch_can_check_if_busy(&priv->regs->if2_creq, i);
>>> + }
>>> +}
>>> +
>>> +static void pch_can_config_rx_tx_buffers(struct pch_can_priv *priv)
>>> +{
>>> + int i;
>>> + unsigned long flags;
>>> +
>>> + spin_lock_irqsave(&priv->msgif_reg_lock, flags);
>>> +
>>> + for (i = 1; i <= PCH_RX_OBJ_NUM; i++) {
>>> + iowrite32(CAN_CMASK_RX_TX_GET, &priv->regs->if1_cmask);
>>> + pch_can_check_if_busy(&priv->regs->if1_creq, i);
>>
>> If I understand the code correctly, the about function triggers a
>> transfer. Why do you first trigger a transfer, then set the message contents....
>
> For doing Read-Modify-Write.
> As to fixed parameter of message object, it doesn't be modified every access.
I see.
> We will modify to write only.
>
>>> +
>>> + iowrite32(0x0, &priv->regs->if1_id1);
>>> + iowrite32(0x0, &priv->regs->if1_id2);
>>> +
>>> + pch_can_bit_set(&priv->regs->if1_mcont, CAN_IF_MCONT_UMASK);
>>
>> Why do you set the "Use acceptance mask" bit? We want to receive
>> all can messages.
>
> Without "Use acceptance mask" means received packet matched ID[28:0] only.
> As a result, filter is enabled.
>
> With "Use acceptance mask" and setting Msk[0:28]=all 1, all packets can be received(=No filter state)
Thanks for the explenation.
>
>>
>>> +
>>> + /* Set FIFO mode set to 0 except last Rx Obj*/
>>> + pch_can_bit_clear(&priv->regs->if1_mcont, CAN_IF_MCONT_EOB);
>>> + /* In case FIFO mode, Last EoB of Rx Obj must be 1 */
>>> + if (i == (PCH_RX_OBJ_NUM - 1))
>>> + pch_can_bit_set(&priv->regs->if1_mcont,
>>> + CAN_IF_MCONT_EOB);
>>
>> Make it if () { } else { }, please.
>
> Sorry, I can't understand.
> else {} is not necessary.
Please look at the code block above, again. You frist clean the bit
unconditionally, then you set the bit in the if. Please make it:
if (last)
set_bit
else
clear_bit
>
>>
>>> +
>>> + iowrite32(0, &priv->regs->if1_mask1);
>>> + pch_can_bit_clear(&priv->regs->if1_mask2,
>>> + 0x1fff | CAN_MASK2_MDIR_MXTD);
>>> +
>>> + /* Setting CMASK for writing */
>>> + iowrite32(CAN_CMASK_RDWR | CAN_CMASK_MASK | CAN_CMASK_ARB |
>>> + CAN_CMASK_CTRL, &priv->regs->if1_cmask);
>>> +
>>> + pch_can_check_if_busy(&priv->regs->if1_creq, i);
>>
>> ...and then trigger the transfer again?
>
> This means Read-Modify-Write.
ic
>
>>
>>> + }
>>> +
>>> + for (i = PCH_RX_OBJ_NUM + 1; i <= PCH_OBJ_NUM; i++) {
>>> + iowrite32(CAN_CMASK_RX_TX_GET, &priv->regs->if2_cmask);
>>> + pch_can_check_if_busy(&priv->regs->if2_creq, i);
>>
>> same question about triggering the transfer 2 times applied here, too
>
> ditto.
>
>>> +
>>> + /* Resetting DIR bit for reception */
>>> + iowrite32(0x0, &priv->regs->if2_id1);
>>> + iowrite32(0x0, &priv->regs->if2_id2);
>>> + pch_can_bit_set(&priv->regs->if2_id2, CAN_ID2_DIR);
>>
>> Can you combine the two accesses to >if2_id2 into one?
>
> I agree.
>
>>
>>> +
>>> + /* Setting EOB bit for transmitter */
>>> + iowrite32(CAN_IF_MCONT_EOB, &priv->regs->if2_mcont);
>>> +
>>> + pch_can_bit_set(&priv->regs->if2_mcont,
>>> + CAN_IF_MCONT_UMASK);
>>
>> dito for if2_mcont
>
> ditto.
>
>>
>>> +
>>> + 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->if2_cmask);
>>> +
>>> + pch_can_check_if_busy(&priv->regs->if2_creq, i);
>>> + }
>>> +
>>> + spin_unlock_irqrestore(&priv->msgif_reg_lock, flags);
>>> +}
>>> +
>>> +static void pch_can_init(struct pch_can_priv *priv)
>>> +{
>>> + /* 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 the interrupts. */
>>> + pch_can_set_int_enables(priv, PCH_CAN_ALL);
>>> +}
>>> +
>>> +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) {
>>
>> is this a valid case?
>
> This "if" is always false.
> I will delete this condition.
>
>>
>>> + ioread32(&priv->regs->stat);
>>> + return;
>>> + }
>>> +
>>> + /* Clear interrupt for transmit object */
>>> + if ((mask >= 1) && (mask <= PCH_RX_OBJ_NUM)) {
>>> + /* Setting CMASK for clearing the reception interrupts. */
>>> + iowrite32(CAN_CMASK_RDWR | CAN_CMASK_CTRL | CAN_CMASK_ARB,
>>> + &priv->regs->if1_cmask);
>>> +
>>> + /* Clearing the Dir bit. */
>>> + pch_can_bit_clear(&priv->regs->if1_id2, CAN_ID2_DIR);
>>> +
>>> + /* Clearing NewDat & IntPnd */
>>> + pch_can_bit_clear(&priv->regs->if1_mcont,
>>> + CAN_IF_MCONT_NEWDAT | CAN_IF_MCONT_INTPND);
>>> +
>>> + pch_can_check_if_busy(&priv->regs->if1_creq, mask);
>>> + } else if ((mask > PCH_RX_OBJ_NUM) && (mask <= PCH_OBJ_NUM)) {
>>> + /* Setting CMASK for clearing interrupts for
>>> + frame transmission. */
>>
>> /*
>> * this is the prefered style of multi line comments,
>> * please adjust you comments
>> */
>
> I understand.
>
>>
>>> + 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_if_busy(&priv->regs->if2_creq, mask);
>>> + }
>>> +}
>>> +
>>> +static u32 pch_can_get_buffer_status(struct pch_can_priv *priv)
>>> +{
>>> + return (ioread32(&priv->regs->treq1) & 0xffff) |
>>> + ((ioread32(&priv->regs->treq2) & 0xffff) << 16);
>>
>> the second 0xffff is not needed, as the return value is u32 and you shift by 16.
>
> I agree.
>
>>
>>> +}
>>> +
>>> +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_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);
>>> + enum can_state state = priv->can.state;
>>> +
>>> + skb = alloc_can_err_skb(ndev, &cf);
>>> + if (!skb)
>>> + return;
>>> +
>>> + if (status & PCH_BUS_OFF) {
>>> + pch_can_tx_disable_all(priv);
>>> + pch_can_rx_disable_all(priv);
>>> + state = CAN_STATE_BUS_OFF;
>>> + cf->can_id |= CAN_ERR_BUSOFF;
>>> + can_bus_off(ndev);
>>> + }
>>> +
>>> + /* Warning interrupt. */
>>> + if (status & PCH_EWARN) {
>>> + 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 -> Error Counter is more than 96.\n", __func__);
>>
>> Please use just "debug" level not warning here. Consider to use
>> netdev_dbg() instead. IMHO the __func__ can be dropped and the
>> "official" name for the error is "Error Warning".
>
> I want to know the reason.
> Why is it not dev_warn but netdev_dbg ?
If you use warning level it would end up on the console or and in the
syslog. It's quite complicated (for programs) to get information from
there. This is why we send CAN error frames. They hold the same
information but int a binary form, thus it's easier to process.
>
>>
>>> + }
>>> + /* Error passive interrupt. */
>>> + if (status & PCH_EPASSIV) {
>>> + priv->can.can_stats.error_passive++;
>>> + 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 -> CAN controller is ERROR PASSIVE .\n", __func__);
>>
>> dito
>
> ditto
>
>>
>>> + }
>>> +
>>> + if (status & PCH_LEC_ALL) {
>>> + priv->can.can_stats.bus_error++;
>>> + stats->rx_errors++;
>>> + switch (status & PCH_LEC_ALL) {
>>
>> I suggest to convert to a if-bit-set because there might be more than
>> one bit set.
>
> I agree.
>
>>
>>> + case PCH_STUF_ERR:
>>> + cf->data[2] |= CAN_ERR_PROT_STUFF;
>>> + break;
>>> + case PCH_FORM_ERR:
>>> + cf->data[2] |= CAN_ERR_PROT_FORM;
>>> + break;
>>> + case PCH_ACK_ERR:
>>> + cf->data[2] |= CAN_ERR_PROT_LOC_ACK |
>>> + CAN_ERR_PROT_LOC_ACK_DEL;
>>> + break;
>>> + case PCH_BIT1_ERR:
>>> + case PCH_BIT0_ERR:
>>> + cf->data[2] |= CAN_ERR_PROT_BIT;
>>> + break;
>>> + case PCH_CRC_ERR:
>>> + cf->data[2] |= CAN_ERR_PROT_LOC_CRC_SEQ |
>>> + CAN_ERR_PROT_LOC_CRC_DEL;
>>> + break;
>>> + default:
>>> + iowrite32(status | PCH_LEC_ALL, &priv->regs->stat);
>>> + break;
>>> + }
>>> +
>>> + }
>>> +
>>> + priv->can.state = state;
>>> + netif_receive_skb(skb);
>>> +
>>> + stats->rx_packets++;
>>> + stats->rx_bytes += cf->can_dlc;
>>> +}
>>> +
>>> +static irqreturn_t pch_can_interrupt(int irq, void *dev_id)
>>> +{
>>> + struct net_device *ndev = (struct net_device *)dev_id;
>>> + struct pch_can_priv *priv = netdev_priv(ndev);
>>> +
>>> + pch_can_set_int_enables(priv, PCH_CAN_NONE);
>>> + napi_schedule(&priv->napi);
>>> +
>>> + return IRQ_HANDLED;
>>> +}
>>> +
>>> +static void pch_fifo_thresh(struct pch_can_priv *priv, int obj_id)
>>> +{
>>> + if (obj_id < PCH_FIFO_THRESH) {
>>> + iowrite32(CAN_CMASK_RDWR | CAN_CMASK_CTRL |
>>> + CAN_CMASK_ARB, &priv->regs->if1_cmask);
>>> +
>>> + /* Clearing the Dir bit. */
>>> + pch_can_bit_clear(&priv->regs->if1_id2, CAN_ID2_DIR);
>>> +
>>> + /* Clearing NewDat & IntPnd */
>>> + pch_can_bit_clear(&priv->regs->if1_mcont,
>>> + CAN_IF_MCONT_INTPND);
>>> + pch_can_check_if_busy(&priv->regs->if1_creq, obj_id);
>>> + } else if (obj_id > PCH_FIFO_THRESH) {
>>> + pch_can_int_clr(priv, obj_id);
>>> + } else if (obj_id == PCH_FIFO_THRESH) {
>>> + int cnt;
>>> + for (cnt = 0; cnt < PCH_FIFO_THRESH; cnt++)
>>> + pch_can_int_clr(priv, cnt+1);
>>> + }
>>> +}
>>> +
>>> +static int pch_can_rx_msg_lost(struct net_device *ndev, int obj_id)
>>> +{
>>> + struct pch_can_priv *priv = netdev_priv(ndev);
>>> + struct net_device_stats *stats = &(priv->ndev->stats);
>>> + struct sk_buff *skb;
>>> + struct can_frame *cf;
>>> +
>>> + dev_err(&priv->ndev->dev, "Msg Obj is overwritten.\n");
>>> + pch_can_bit_clear(&priv->regs->if1_mcont,
>>> + CAN_IF_MCONT_MSGLOST);
>>> + iowrite32(CAN_CMASK_RDWR | CAN_CMASK_CTRL,
>>> + &priv->regs->if1_cmask);
>>> + pch_can_check_if_busy(&priv->regs->if1_creq, obj_id);
>>> +
>>> + skb = alloc_can_err_skb(ndev, &cf);
>>> + if (!skb)
>>> + return -ENOMEM;
>>> +
>>> + priv->can.can_stats.error_passive++;
>>> + priv->can.state = CAN_STATE_ERROR_PASSIVE;
>>> + cf->can_id |= CAN_ERR_CRTL;
>>> + cf->data[1] = CAN_ERR_CRTL_RX_OVERFLOW;
>>> + stats->rx_over_errors++;
>>> + stats->rx_errors++;
>>> +
>>> + netif_receive_skb(skb);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int pch_can_rx_normal(struct net_device *ndev, u32 obj_num, int quota)
>>> +{
>>> + u32 reg;
>>> + canid_t id;
>>> + u32 ide;
>>> + u32 rtr;
>>> + int rcv_pkts = 0;
>>> + int rtn;
>>> + int next_flag = 0;
>>> + struct sk_buff *skb;
>>> + struct can_frame *cf;
>>> + struct pch_can_priv *priv = netdev_priv(ndev);
>>> + struct net_device_stats *stats = &(priv->ndev->stats);
>>> +
>>> + /* Reading the messsage object from the Message RAM */
>>> + iowrite32(CAN_CMASK_RX_TX_GET, &priv->regs->if1_cmask);
>>> + pch_can_check_if_busy(&priv->regs->if1_creq, obj_num);
>>> +
>>> + /* Reading the MCONT register. */
>>> + reg = ioread32(&priv->regs->if1_mcont);
>>> + reg &= 0xffff;
>>> +
>>> + for (; (!(reg & CAN_IF_MCONT_EOB)) && (quota > 0);
>>> + obj_num++, next_flag = 0) {
>>> + /* If MsgLost bit set. */
>>> + if (reg & CAN_IF_MCONT_MSGLOST) {
>>> + rtn = pch_can_rx_msg_lost(ndev, obj_num);
>>> + if (!rtn)
>>> + return rtn;
>>> + rcv_pkts++;
>>> + quota--;
>>> + next_flag = 1;
>>> + } else if (!(reg & CAN_IF_MCONT_NEWDAT))
>>> + next_flag = 1;
>>> +
>>
>> after rearanging the code (see below..) you should be able to use a continue here.
>>
>>> + if (!next_flag) {
>>> + skb = alloc_can_skb(priv->ndev, &cf);
>>> + if (!skb)
>>> + return -ENOMEM;
>>> +
>>> + /* Get Received data */
>>> + ide = ((ioread32(&priv->regs->if1_id2)) & CAN_ID2_XTD);
>>> + if (ide) {
>>> + id = (ioread32(&priv->regs->if1_id1) & 0xffff);
>>> + id |= (((ioread32(&priv->regs->if1_id2)) &
>>> + 0x1fff) << 16);
>>> + cf->can_id = (id & CAN_EFF_MASK) | CAN_EFF_FLAG;
>> ^^^^^^^^^^^^^^^^^
>>
>> is the mask needed, you mask the if1_id{1,2} already
>
> I will delete
>
>>
>>> + } else {
>>> + id = (((ioread32(&priv->regs->if1_id2)) &
>>> + (CAN_SFF_MASK << 2)) >> 2);
>>> + cf->can_id = (id & CAN_SFF_MASK);
>>
>> one mask can go away
>
> I agree.
>
>>
>>> + }
>>> +
>>> + rtr = ioread32(&priv->regs->if1_id2) & CAN_ID2_DIR;
>> ^^
>>
>> remove one space
>
> I agree.
>
>>
>>> +
>>> + if (rtr)
>>> + cf->can_id |= CAN_RTR_FLAG;
>>> +
>>> + cf->can_dlc = get_can_dlc((ioread32(&priv->regs->
>>> + if1_mcont)) & 0xF);
>>> + *(u16 *)(cf->data + 0) = ioread16(&priv->regs->
>>> + if1_dataa1);
>>> + *(u16 *)(cf->data + 2) = ioread16(&priv->regs->
>>> + if1_dataa2);
>>> + *(u16 *)(cf->data + 4) = ioread16(&priv->regs->
>>> + if1_datab1);
>>> + *(u16 *)(cf->data + 6) = ioread16(&priv->regs->
>>> + if1_datab2);
>>
>> are you sure, the bytes in the can package a in the correct order.
>> Please test your pch_can against a non pch_can system.
>
> Unfortunately, we don't have non pch_can system.
Have a look a the driver/net/can/usb subdir and buy one of those. It
really hard to find bugs if you test against your own driver.
>
>>
>>> +
>>> + netif_receive_skb(skb);
>>> + rcv_pkts++;
>>> + stats->rx_packets++;
>>> + quota--;
>>> + stats->rx_bytes += cf->can_dlc;
>>> +
>>> + pch_fifo_thresh(priv, obj_num);
>>> + }
>>> +
>>> + /* Reading the messsage object from the Message RAM */
>>> + iowrite32(CAN_CMASK_RX_TX_GET, &priv->regs->if1_cmask);
>>> + pch_can_check_if_busy(&priv->regs->if1_creq, obj_num + 1);
>>> + reg = ioread32(&priv->regs->if1_mcont);
>>
>> this is almost the same code as before the the loop, can you rearange
>> the code to avoid duplication?
>
> I agree.
>
>>
>>> + }
>>> +
>>> + return rcv_pkts;
>>> +}
>>> +
>>> +static void pch_can_tx_complete(struct net_device *ndev, u32 int_stat)
>>> +{
>>> + struct pch_can_priv *priv = netdev_priv(ndev);
>>> + struct net_device_stats *stats = &(priv->ndev->stats);
>>> + unsigned long flags;
>>> + u32 dlc;
>>> +
>>> + can_get_echo_skb(ndev, int_stat - PCH_RX_OBJ_NUM - 1);
>>> + spin_lock_irqsave(&priv->msgif_reg_lock, flags);
>>> + iowrite32(CAN_CMASK_RX_TX_GET | CAN_CMASK_CLRINTPND,
>>> + &priv->regs->if2_cmask);
>>> + dlc = ioread32(&priv->regs->if2_mcont) & CAN_IF_MCONT_DLC;
>>> + pch_can_check_if_busy(&priv->regs->if2_creq, int_stat);
>>> + spin_unlock_irqrestore(&priv->msgif_reg_lock, flags);
>>> + if (dlc > 8)
>>> + dlc = 8;
>>
>> use get_can_dlc
>
> I agree.
>
>>
>>> + stats->tx_bytes += dlc;
>>> + stats->tx_packets++;
>>> +}
>>> +
>>> +static int pch_can_rx_poll(struct napi_struct *napi, int quota)
>>> +{
>>> + struct net_device *ndev = napi->dev;
>>> + struct pch_can_priv *priv = netdev_priv(ndev);
>>> + u32 int_stat;
>>> + int rcv_pkts = 0;
>>> + u32 reg_stat;
>>> + unsigned long flags;
>>> +
>>> + int_stat = pch_can_int_pending(priv);
>>> + if (!int_stat)
>>> + goto END;
>>> +
>>> + if ((int_stat == CAN_STATUS_INT) && (quota > 0)) {
>>> + reg_stat = ioread32(&priv->regs->stat);
>>> + if (reg_stat & (PCH_BUS_OFF | PCH_LEC_ALL)) {
>>> + if ((reg_stat & PCH_LEC_ALL) != PCH_LEC_ALL) {
>>> + pch_can_error(ndev, reg_stat);
>>> + quota--;
>>> + }
>>> + }
>>> +
>>> + if (reg_stat & PCH_TX_OK) {
>>> + spin_lock_irqsave(&priv->msgif_reg_lock, flags);
>>> + iowrite32(CAN_CMASK_RX_TX_GET, &priv->regs->if2_cmask);
>>> + pch_can_check_if_busy(&priv->regs->if2_creq,
>>> + ioread32(&priv->regs->intr));
>> ^^^^^^^^^^^^^^^^^^^^^^^^^^^
>>
>> Isn't this "int_stat". Might it be possilbe that regs->intr changes
>> between the pch_can_int_pending and here?
>
> This code was mistake.
> This condition, message object is not acccessed.
> Thus, pch_can_check_if_busy can be deleted.
>
>>
>> What should this transfer do?
>>
>>> + spin_unlock_irqrestore(&priv->msgif_reg_lock, flags);
>>> + pch_can_bit_clear(&priv->regs->stat, PCH_TX_OK);
>>> + }
>>> +
>>> + if (reg_stat & PCH_RX_OK)
>>> + pch_can_bit_clear(&priv->regs->stat, PCH_RX_OK);
>>> +
>>> + int_stat = pch_can_int_pending(priv);
>>> + }
>>> +
>>> + if (quota == 0)
>>> + goto END;
>>> +
>>> + if ((int_stat >= 1) && (int_stat <= PCH_RX_OBJ_NUM)) {
>>> + spin_lock_irqsave(&priv->msgif_reg_lock, flags);
>>> + rcv_pkts += pch_can_rx_normal(ndev, int_stat, quota);
>>> + spin_unlock_irqrestore(&priv->msgif_reg_lock, flags);
>>> + quota -= rcv_pkts;
>>> + if (rcv_pkts < 0)
>>
>> how can this happen?
>
> My mistake.
> if (quota < 0) is TRUE.
>
>
>>
>>> + goto END;
>>> + } else if ((int_stat > PCH_RX_OBJ_NUM) && (int_stat <= PCH_OBJ_NUM)) {
>>> + /* Handle transmission interrupt */
>>> + pch_can_tx_complete(ndev, int_stat);
>>> + }
>>> +
>>> +END:
>>> + napi_complete(napi);
>>> + pch_can_set_int_enables(priv, PCH_CAN_ALL);
>>> +
>>> + return rcv_pkts;
>>> +}
>>> +
>>> +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 canbit;
>>> + u32 bepe;
>>> +
>>> + /* Setting the CCE bit for accessing the Can Timing register. */
>>> + pch_can_bit_set(&priv->regs->cont, CAN_CTRL_CCE);
>>> +
>>> + canbit = (bt->brp - 1) & MSK_BITT_BRP;
>>> + canbit |= (bt->sjw - 1) << BIT_BITT_SJW;
>>> + canbit |= (bt->phase_seg1 + bt->prop_seg - 1) << BIT_BITT_TSEG1;
>>> + canbit |= (bt->phase_seg2 - 1) << BIT_BITT_TSEG2;
>>> + bepe = ((bt->brp - 1) & MSK_BRPE_BRPE) >> BIT_BRPE_BRPE;
>>> + iowrite32(canbit, &priv->regs->bitt);
>>> + iowrite32(bepe, &priv->regs->brpe);
>>> + pch_can_bit_clear(&priv->regs->cont, CAN_CTRL_CCE);
>>> +
>>> + 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);
>>> +
>>> + pch_can_tx_enable_all(priv);
>>> + pch_can_rx_enable_all(priv);
>>> +
>>> + /* Setting the CAN to run mode. */
>>> + pch_can_set_run_mode(priv, PCH_CAN_RUN);
>>> +
>>> + priv->can.state = CAN_STATE_ERROR_ACTIVE;
>>> +
>>> + 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;
>>> +}
>>> +
>>> +static int pch_can_open(struct net_device *ndev)
>>> +{
>>> + struct pch_can_priv *priv = netdev_priv(ndev);
>>> + int retval;
>>> +
>>> + /* Regsitering the interrupt. */
>>> + retval = request_irq(priv->dev->irq, pch_can_interrupt, IRQF_SHARED,
>>> + ndev->name, ndev);
>>> + if (retval) {
>>> + dev_err(&ndev->dev, "request_irq failed.\n");
>>> + goto req_irq_err;
>>> + }
>>> +
>>> + /* 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_init(priv);
>>> + pch_can_start(ndev);
>>> + napi_enable(&priv->napi);
>>> + netif_start_queue(ndev);
>>> +
>>> + return 0;
>>> +
>>> +err_open_candev:
>>> + free_irq(priv->dev->irq, ndev);
>>> +req_irq_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);
>>> + napi_disable(&priv->napi);
>>> + pch_can_release(priv);
>>> + free_irq(priv->dev->irq, ndev);
>>> + close_candev(ndev);
>>> + priv->can.state = CAN_STATE_STOPPED;
>>> + return 0;
>>> +}
>>> +
>>> +static netdev_tx_t pch_xmit(struct sk_buff *skb, struct net_device *ndev)
>>> +{
>>> + unsigned long flags;
>>> + struct pch_can_priv *priv = netdev_priv(ndev);
>>> + struct can_frame *cf = (struct can_frame *)skb->data;
>>> + int tx_buffer_avail = 0;
>>
>> What I'm totally missing is the TX flow controll. Your driver has to
>> ensure that the package leave the controller in the order that come
>> into the xmit function. Further you have to stop your xmit queue if
>> you're out of tx objects and reenable if you have a object free.
>>
>> Use netif_stop_queue() and netif_wake_queue() for this.
>
> In this code, I think "out of tx objects" cannot be occurred.
It's not a matter of code it's the hardware. You cannot put more than a
certain number of CAN frames into the hardware. If you have a CAN bus at
a certain speed, you can only send a certain number of CAN frames in a
second. So you cannot push more than this amount of frames/s into the
hardware.
> Nevertheless, are netif_stop_queue() and netif_wake_queue() is necessary ?
Yes.
>
>>
>>> +
>>> + if (can_dropped_invalid_skb(ndev, skb))
>>> + return NETDEV_TX_OK;
>>> +
>>> + if (priv->tx_obj == (PCH_OBJ_NUM + 1)) { /* Point tail Obj + 1 */
>>> + while (ioread32(&priv->regs->treq2) & 0xfc00)
>>> + udelay(1);
>>
>> please no (possible) infinite delays!
>
> I will add break processing.
>
>>
>>> + priv->tx_obj = PCH_RX_OBJ_NUM + 1; /* Point head of Tx Obj ID */
>>> + }
>>
>>> +
>>> + tx_buffer_avail = priv->tx_obj;
>>
>> why has the "object" become a "buffer" now? :)
>
> You are right.
> I will modify the name.
>
>>
>>> + priv->tx_obj++;
>>> +
>>> + /* Attaining the lock. */
>>> + spin_lock_irqsave(&priv->msgif_reg_lock, flags);
>>> +
>>> + /* Setting the CMASK register to set value*/
>> ^^^
>>
>> pleas add a whitespace
>
> I agree.
>
>>
>>> + iowrite32(CAN_CMASK_RX_TX_SET, &priv->regs->if2_cmask);
>>> +
>>> + /* If ID extended is set. */
>>> + if (cf->can_id & CAN_EFF_FLAG) {
>>> + iowrite32(cf->can_id & 0xffff, &priv->regs->if2_id1);
>>> + iowrite32(((cf->can_id >> 16) & 0x1fff) | CAN_ID2_XTD,
>>> + &priv->regs->if2_id2);
>>> + } else {
>>> + iowrite32(0, &priv->regs->if2_id1);
>>> + iowrite32((cf->can_id & CAN_SFF_MASK) << 2,
>>> + &priv->regs->if2_id2);
>>> + }
>>> +
>>> + pch_can_bit_set(&priv->regs->if2_id2, CAN_ID_MSGVAL);
>>
>> Do you need to do a read-modify-write of the hardware register? Please
>> prepare the values you want to write to hardware, then do it.
>
> Current design policy for read/write message object,
> the driver is designed with Read-Modify-Write.
>
> I will modify to Write only for reducing accessing Message RAM.
>
>>
>>> +
>>> + /* If remote frame has to be transmitted.. */
>>> + if (!(cf->can_id & CAN_RTR_FLAG))
>>> + pch_can_bit_set(&priv->regs->if2_id2, CAN_ID2_DIR);
>> dito
>>> + /* If remote frame has to be transmitted.. */
>>> + if (cf->can_id & CAN_RTR_FLAG)
>>> + pch_can_bit_clear(&priv->regs->if2_id2, CAN_ID2_DIR);
>> dito
>>> +
>>> + /* Copy data to register */
>>> + if (cf->can_dlc > 0) {
>>> + u32 data1 = *((u16 *)&cf->data[0]);
>>> + iowrite32(data1, &priv->regs->if2_dataa1);
>>
>> do you think you send the bytes in correct order?
>
> Let me study this endianess.
>
>>
>>> + }
>>> + if (cf->can_dlc > 2) {
>>> + u32 data1 = *((u16 *)&cf->data[2]);
>>> + iowrite32(data1, &priv->regs->if2_dataa2);
>>> + }
>>> + if (cf->can_dlc > 4) {
>>> + u32 data1 = *((u16 *)&cf->data[4]);
>>> + iowrite32(data1, &priv->regs->if2_datab1);
>>> + }
>>> + if (cf->can_dlc > 6) {
>>> + u32 data1 = *((u16 *)&cf->data[6]);
>>> + iowrite32(data1, &priv->regs->if2_datab2);
>>> + }
>>> +
>>> + can_put_echo_skb(skb, ndev, tx_buffer_avail - PCH_RX_OBJ_NUM - 1);
>>> +
>>> + /* Set the size of the data. */
>>> + iowrite32(cf->can_dlc, &priv->regs->if2_mcont);
>>> +
>>> + /* Update if2_mcont */
>>> + pch_can_bit_set(&priv->regs->if2_mcont,
>>> + CAN_IF_MCONT_NEWDAT | CAN_IF_MCONT_TXRQXT |
>>> + CAN_IF_MCONT_TXIE);
>>
>> pleae first perpare your value, then write to hardware.
>
> ditto.
>
>>
>>> +
>>> + if (tx_buffer_avail == PCH_RX_OBJ_NUM) /* If points tail of FIFO */
>>> + pch_can_bit_set(&priv->regs->if2_mcont, CAN_IF_MCONT_EOB);
>>
>> dito
>>
>> Is EOB relevant for TX objects?
>
> This is mistake. No meaning for tx.
> I will modify.
>
>>
>>> + pch_can_check_if_busy(&priv->regs->if2_creq, tx_buffer_avail);
>>> + spin_unlock_irqrestore(&priv->msgif_reg_lock, flags);
>>> +
>>> + return NETDEV_TX_OK;
>>> +}
>>> +
>>> +static const struct net_device_ops pch_can_netdev_ops = {
>>> + .ndo_open = pch_can_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);
>>> + pci_iounmap(pdev, priv->regs);
>>> + if (priv->use_msi)
>>> + pci_disable_msi(priv->dev);
>>> + pci_release_regions(pdev);
>>> + pci_disable_device(pdev);
>>> + pci_set_drvdata(pdev, NULL);
>>> + free_candev(priv->ndev);
>>> +}
>>> +
>>> +#ifdef CONFIG_PM
>>> +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 u32 pch_can_get_int_enables(struct pch_can_priv *priv)
>>> +{
>>> + /* Obtaining the status of IE, SIE and EIE interrupt bits. */
>>> + return (ioread32(&priv->regs->cont) & CAN_CTRL_IE_SIE_EIE) >> 1;
>>> +}
>>> +
>>> +static u32 pch_can_get_rx_enable(struct pch_can_priv *priv, u32 buff_num)
>>> +{
>>> + unsigned long flags;
>>> + u32 enable;
>>> +
>>> + spin_lock_irqsave(&priv->msgif_reg_lock, flags);
>>> + iowrite32(CAN_CMASK_RX_TX_GET, &priv->regs->if1_cmask);
>>> + pch_can_check_if_busy(&priv->regs->if1_creq, buff_num);
>>> +
>>> + if (((ioread32(&priv->regs->if1_id2)) & CAN_ID_MSGVAL) &&
>>> + ((ioread32(&priv->regs->if1_mcont)) &
>>> + CAN_IF_MCONT_RXIE))
>>> + enable = 1;
>>> + else
>>> + enable = 0;
>>> + spin_unlock_irqrestore(&priv->msgif_reg_lock, flags);
>>> + return enable;
>>> +}
>>> +
>>> +static u32 pch_can_get_tx_enable(struct pch_can_priv *priv, u32 buff_num)
>>> +{
>>> + unsigned long flags;
>>> + u32 enable;
>>> +
>>> + spin_lock_irqsave(&priv->msgif_reg_lock, flags);
>>> +
>>> + iowrite32(CAN_CMASK_RX_TX_GET, &priv->regs->if2_cmask);
>>> + pch_can_check_if_busy(&priv->regs->if2_creq, buff_num);
>>> + if (((ioread32(&priv->regs->if2_id2)) & CAN_ID_MSGVAL) &&
>>> + ((ioread32(&priv->regs->if2_mcont)) &
>>> + CAN_IF_MCONT_TXIE)) {
>>> + enable = 1;
>>> + } else {
>>> + enable = 0;
>>> + }
>>> + spin_unlock_irqrestore(&priv->msgif_reg_lock, flags);
>>> +
>>> + return enable;
>>> +}
>>> +
>>> +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_if_busy(&priv->regs->if1_creq, buffer_num);
>>> + iowrite32(CAN_CMASK_RDWR | CAN_CMASK_CTRL, &priv->regs->if1_cmask);
>>> + if (set == 1)
>>> + 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_if_busy(&priv->regs->if1_creq, buffer_num);
>>> + spin_unlock_irqrestore(&priv->msgif_reg_lock, flags);
>>> +}
>>> +
>>> +static u32 pch_can_get_rx_buffer_link(struct pch_can_priv *priv, u32 buffer_num)
>>> +{
>>> + unsigned long flags;
>>> + u32 link;
>>> +
>>> + spin_lock_irqsave(&priv->msgif_reg_lock, flags);
>>> + iowrite32(CAN_CMASK_RX_TX_GET, &priv->regs->if1_cmask);
>>> + pch_can_check_if_busy(&priv->regs->if1_creq, buffer_num);
>>> +
>>> + if (ioread32(&priv->regs->if1_mcont) & CAN_IF_MCONT_EOB)
>>> + link = 0;
>>> + else
>>> + link = 1;
>>> + spin_unlock_irqrestore(&priv->msgif_reg_lock, flags);
>>> + return link;
>>> +}
>>> +
>>> +static int pch_can_suspend(struct pci_dev *pdev, pm_message_t state)
>>> +{
>>> + int i;
>>> + int retval;
>>> + u32 buf_stat; /* Variable for reading the transmit buffer status. */
>>> + u32 counter = COUNTER_LIMIT;
>>> +
>>> + 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_STOPPED;
>>> +
>>> + /* Waiting for all transmission to complete. */
>>> + while (counter) {
>>> + buf_stat = pch_can_get_buffer_status(priv);
>>> + if (!buf_stat)
>>> + break;
>>> + counter--;
>>> + udelay(1);
>>> + }
>>> + if (!counter)
>>> + dev_err(&pdev->dev, "%s -> Transmission time out.\n", __func__);
>>> +
>>> + /* Save interrupt configuration and then disable them */
>>> + priv->int_enables = pch_can_get_int_enables(priv);
>>> + pch_can_set_int_enables(priv, PCH_CAN_DISABLE);
>>> +
>>> + /* Save Tx buffer enable state */
>>> + for (i = PCH_RX_OBJ_NUM + 1; i <= PCH_OBJ_NUM; i++)
>>> + priv->tx_enable[i] = pch_can_get_tx_enable(priv, i);
>>> +
>>> + /* Disable all Transmit buffers */
>>> + pch_can_tx_disable_all(priv);
>>> +
>>> + /* Save Rx buffer enable state */
>>> + for (i = 1; i <= PCH_RX_OBJ_NUM; i++) {
>>> + priv->rx_enable[i] = pch_can_get_rx_enable(priv, i);
>>> + priv->rx_link[i] = pch_can_get_rx_buffer_link(priv, 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;
>>> + int retval;
>>> + 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 = 1; i <= PCH_RX_OBJ_NUM; i++)
>>> + pch_can_set_tx_enable(priv, i, priv->tx_enable[i]);
>>> +
>>> + /* Configuring the receive buffer and enabling them. */
>>> + for (i = PCH_RX_OBJ_NUM + 1; i <= PCH_OBJ_NUM; i++) {
>>> + /* Restore buffer link */
>>> + pch_can_set_rx_buffer_link(priv, i, priv->rx_link[i]);
>>> +
>>> + /* Restore buffer enables */
>>> + pch_can_set_rx_enable(priv, i, 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;
>>> +}
>>> +#else
>>> +#define pch_can_suspend NULL
>>> +#define pch_can_resume NULL
>>> +#endif
>>> +
>>> +static int pch_can_get_berr_counter(const struct net_device *dev,
>>> + struct can_berr_counter *bec)
>>> +{
>>> + struct pch_can_priv *priv = netdev_priv(dev);
>>> +
>>> + bec->txerr = ioread32(&priv->regs->errc) & CAN_TEC;
>>> + bec->rxerr = (ioread32(&priv->regs->errc) & CAN_REC) >> 8;
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +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;
>>> + 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), PCH_TX_OBJ_NUM);
>>> + 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->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_berr_counter = pch_can_get_berr_counter;
>>> + priv->can.ctrlmode_supported = CAN_CTRLMODE_LISTENONLY |
>>> + CAN_CTRLMODE_LOOPBACK;
>>> + priv->tx_obj = PCH_RX_OBJ_NUM + 1; /* Point head of Tx Obj */
>>> +
>>> + 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; /* Hz */
>>> +
>>> + netif_napi_add(ndev, &priv->napi, pch_can_rx_poll, PCH_RX_OBJ_NUM);
>>> +
>>> + rc = pci_enable_msi(priv->dev);
>>> + if (rc) {
>>> + dev_info(&ndev->dev, "PCH CAN opened without MSI\n");
>>> + priv->use_msi = 0;
>>> + } else {
>>> + dev_info(&ndev->dev, "PCH CAN opened with MSI\n");
>>> + priv->use_msi = 1;
>>> + }
>>> +
>>> + 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 = "pch_can",
>>> + .id_table = pch_pci_tbl,
>>> + .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("Intel EG20T PCH CAN(Controller Area Network) Driver");
>>> +MODULE_LICENSE("GPL v2");
>>> +MODULE_VERSION("0.94");
>>
>> cheers, Marc
>>
>> --
>> Pengutronix e.K. | Marc Kleine-Budde |
>> Industrial Linux Solutions | Phone: +49-231-2826-924 |
>> Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
>> Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
>>
>>
>
> Thanks, Tomoya(OKI SEMICONDUCTOR CO., LTD.)
cheers, Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions | Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]
[-- Attachment #2: Type: text/plain, Size: 188 bytes --]
_______________________________________________
Socketcan-core mailing list
Socketcan-core-0fE9KPoRgkgATYTw5x5z8w@public.gmane.org
https://lists.berlios.de/mailman/listinfo/socketcan-core
^ permalink raw reply
* [PATCH] rds: Lost locking in loop connection freeing
From: Pavel Emelyanov @ 2010-11-02 11:52 UTC (permalink / raw)
To: Andy Grover, David Miller; +Cc: rds-devel, Linux Netdev List
The conn is removed from list in there and this requires
proper lock protection.
Signed-off-by: Pavel Emelyanov <xemul@openvz.org>
---
diff --git a/net/rds/loop.c b/net/rds/loop.c
index c390156..aeec1d4 100644
--- a/net/rds/loop.c
+++ b/net/rds/loop.c
@@ -134,8 +134,12 @@ static int rds_loop_conn_alloc(struct rds_connection *conn, gfp_t gfp)
static void rds_loop_conn_free(void *arg)
{
struct rds_loop_connection *lc = arg;
+ unsigned long flags;
+
rdsdebug("lc %p\n", lc);
+ spin_lock_irqsave(&loop_conns_lock, flags);
list_del(&lc->loop_node);
+ spin_unlock_irqrestore(&loop_conns_lock, flags);
kfree(lc);
}
--
1.5.5.6
^ permalink raw reply related
* [PATCH] rds: Remove kfreed tcp conn from list
From: Pavel Emelyanov @ 2010-11-02 11:54 UTC (permalink / raw)
To: Andy Grover, David Miller; +Cc: rds-devel, Linux Netdev List
All the rds_tcp_connection objects are stored list, but when
being freed it should be removed from there.
Signed-off-by: Pavel Emelyanov <xemul@openvz.org>
---
diff --git a/net/rds/tcp.c b/net/rds/tcp.c
index 08a8c6c..8e0a320 100644
--- a/net/rds/tcp.c
+++ b/net/rds/tcp.c
@@ -221,7 +221,13 @@ static int rds_tcp_conn_alloc(struct rds_connection *conn, gfp_t gfp)
static void rds_tcp_conn_free(void *arg)
{
struct rds_tcp_connection *tc = arg;
+ unsigned long flags;
rdsdebug("freeing tc %p\n", tc);
+
+ spin_lock_irqsave(&rds_tcp_conn_lock, flags);
+ list_del(&tc->t_tcp_node);
+ spin_unlock_irqrestore(&rds_tcp_conn_lock, flags);
+
kmem_cache_free(rds_tcp_conn_slab, tc);
}
--
1.5.5.6
^ permalink raw reply related
* For your interest
From: Srood Sherif @ 2010-11-02 12:59 UTC (permalink / raw)
[-- Attachment #1: Type: text/plain, Size: 359 bytes --]
Greeting,
My name is Mr. Srood A. Sherif, I live and work in Abu
Dhabi UAE. I have an urgent business which I believe
will interest you. Find the enclose for details.
For any reason you cannot open that attachment, please
let me know so that I can resend it in the body of the
mail, thank you.
I wait for your response, Thank you.
Regards
Srood A. Sherif
[-- Attachment #2: THE INFORMATION.jpg --]
[-- Type: image/jpeg, Size: 240015 bytes --]
^ permalink raw reply
* Re: [PATCH] net: sh_eth: ctrl_in/outX to __raw_read/writeX conversion.
From: Paul Mundt @ 2010-11-02 13:39 UTC (permalink / raw)
To: Nobuhiro Iwamatsu; +Cc: netdev
In-Reply-To: <1288666295-12529-1-git-send-email-nobuhiro.iwamatsu.yj@renesas.com>
On Tue, Nov 02, 2010 at 11:51:35AM +0900, Nobuhiro Iwamatsu wrote:
> The ctrl_xxx routines are deprecated, switch over to the __raw_xxx
> versions.
>
> Signed-off-by: Nobuhiro Iwamatsu <nobuhiro.iwamatsu.yj@renesas.com>
I sent a similar patch yesterday:
http://patchwork.ozlabs.org/patch/69831/
It doesn't matter really which one gets applied, although I opted to use
the accessors with strong ordering given that we'll continue to see this
block in newer CPUs where weak ordering is unlikely to be sufficient.
^ permalink raw reply
* Partnership..?
From: Mr. Vincent Cheng @ 2010-11-02 9:44 UTC (permalink / raw)
To: info
Hello,
I am Mr. Vincent Cheng, and have a sensitive and confidential brief from
Hong Kong.
I am asking for your partnership in re-profiling funds and will give the
details.
This is a legitimate transaction and you will be paid a handsome
percentage for your "Management Fees". If interested you can kindly write
and provide me with your confidential telephone number or fax number
and I will provide further details with proper guidelines. I require absolute
confidentiality as to any political problems.
Finally, note that this must be concluded within two weeks. Kindly write
back to this email mrchengvich66@yahoo.com.hk, I look forward to hear from
you.
Regards,
Vincent Cheng.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox