* Re: [PATCH 3/4] myri10ge - Driver core
From: Brice Goglin @ 2006-05-19 15:48 UTC (permalink / raw)
To: Andi Kleen; +Cc: Arnd Bergmann, netdev, gallatin, linux-kernel
In-Reply-To: <200605191309.30992.ak@suse.de>
Andi Kleen wrote:
> On Friday 19 May 2006 12:00, Arnd Bergmann wrote:
>
>> On Friday 19 May 2006 04:25, Brice Goglin wrote:
>>
>>> dev_mc_upload() from net/core/dev_mcast.c does
>>>
>>> spin_lock_bh(&dev->xmit_lock);
>>> __dev_mc_upload(dev);
>>>
>>> which calls dev->set_multicast_list(), which is
>>> myri10ge_set_multicast_list()
>>>
>>> which calls myri10ge_change_promisc
>>>
>>> which calls myri10ge_send_cmd
>>>
>> Hmm, if that is the only path where you call it, it may be
>> helpful to change myri10ge_change_promisc to call a special
>> atomic version of myri10ge_send_cmd then, while all others
>> use the regular version, which you can then convert to
>> do msleep as well.
>>
>
> Or just drop the xmit lock while you do that. As long as you
> handle races with ->start_xmit yourself it's ok
>
Looks like we can do that.
Thanks,
Brice
^ permalink raw reply
* [patch w/o fuzz] via-rhine: revert "change mdelay to msleep and remove from ISR path"
From: John W. Linville @ 2006-05-19 14:51 UTC (permalink / raw)
To: Roger Luethi, jeff; +Cc: Marco Berizzi, netdev
In-Reply-To: <20060519144124.GA30177@tuxdriver.com>
Revert previous patch with subject "change mdelay to msleep and remove
from ISR path". This patch seems to have caused bigger problems than
it solved, and it didn't solve much of a problem to begin with...
Signed-off-by: John W. Linville <linville@tuxdriver.com
---
Use this one, it applies w/o fuzz...
Discussion about backing-out this patch can be found here:
http://marc.theaimsgroup.com/?l=linux-netdev&m=114321570402396&w=2
The git commit associated w/ the original patch is:
6ba98d311d0a4ff7dc36d8f435ce60174f4c30ec
drivers/net/via-rhine.c | 34 +++-------------------------------
1 files changed, 3 insertions(+), 31 deletions(-)
diff --git a/drivers/net/via-rhine.c b/drivers/net/via-rhine.c
index a6dc53b..fdc2103 100644
--- a/drivers/net/via-rhine.c
+++ b/drivers/net/via-rhine.c
@@ -491,8 +491,6 @@ struct rhine_private {
u8 tx_thresh, rx_thresh;
struct mii_if_info mii_if;
- struct work_struct tx_timeout_task;
- struct work_struct check_media_task;
void __iomem *base;
};
@@ -500,8 +498,6 @@ static int mdio_read(struct net_device
static void mdio_write(struct net_device *dev, int phy_id, int location, int value);
static int rhine_open(struct net_device *dev);
static void rhine_tx_timeout(struct net_device *dev);
-static void rhine_tx_timeout_task(struct net_device *dev);
-static void rhine_check_media_task(struct net_device *dev);
static int rhine_start_tx(struct sk_buff *skb, struct net_device *dev);
static irqreturn_t rhine_interrupt(int irq, void *dev_instance, struct pt_regs *regs);
static void rhine_tx(struct net_device *dev);
@@ -856,12 +852,6 @@ #endif
if (rp->quirks & rqRhineI)
dev->features |= NETIF_F_SG|NETIF_F_HW_CSUM;
- INIT_WORK(&rp->tx_timeout_task,
- (void (*)(void *))rhine_tx_timeout_task, dev);
-
- INIT_WORK(&rp->check_media_task,
- (void (*)(void *))rhine_check_media_task, dev);
-
/* dev->name not defined before register_netdev()! */
rc = register_netdev(dev);
if (rc)
@@ -1108,11 +1098,6 @@ static void rhine_set_carrier(struct mii
netif_carrier_ok(mii->dev));
}
-static void rhine_check_media_task(struct net_device *dev)
-{
- rhine_check_media(dev, 0);
-}
-
static void init_registers(struct net_device *dev)
{
struct rhine_private *rp = netdev_priv(dev);
@@ -1166,8 +1151,8 @@ static void rhine_disable_linkmon(void _
if (quirks & rqRhineI) {
iowrite8(0x01, ioaddr + MIIRegAddr); // MII_BMSR
- /* Do not call from ISR! */
- msleep(1);
+ /* Can be called from ISR. Evil. */
+ mdelay(1);
/* 0x80 must be set immediately before turning it off */
iowrite8(0x80, ioaddr + MIICmd);
@@ -1257,16 +1242,6 @@ static int rhine_open(struct net_device
static void rhine_tx_timeout(struct net_device *dev)
{
struct rhine_private *rp = netdev_priv(dev);
-
- /*
- * Move bulk of work outside of interrupt context
- */
- schedule_work(&rp->tx_timeout_task);
-}
-
-static void rhine_tx_timeout_task(struct net_device *dev)
-{
- struct rhine_private *rp = netdev_priv(dev);
void __iomem *ioaddr = rp->base;
printk(KERN_WARNING "%s: Transmit timed out, status %4.4x, PHY status "
@@ -1677,7 +1652,7 @@ static void rhine_error(struct net_devic
spin_lock(&rp->lock);
if (intr_status & IntrLinkChange)
- schedule_work(&rp->check_media_task);
+ rhine_check_media(dev, 0);
if (intr_status & IntrStatsMax) {
rp->stats.rx_crc_errors += ioread16(ioaddr + RxCRCErrs);
rp->stats.rx_missed_errors += ioread16(ioaddr + RxMissed);
@@ -1927,9 +1902,6 @@ static int rhine_close(struct net_device
spin_unlock_irq(&rp->lock);
free_irq(rp->pdev->irq, dev);
-
- flush_scheduled_work();
-
free_rbufs(dev);
free_tbufs(dev);
free_ring(dev);
--
John W. Linville
linville@tuxdriver.com
^ permalink raw reply related
* [patch] via-rhine: revert "change mdelay to msleep and remove from ISR path"
From: John W. Linville @ 2006-05-19 14:41 UTC (permalink / raw)
To: Roger Luethi, jeff; +Cc: Marco Berizzi, netdev
In-Reply-To: <20060413203147.GA2874@k3.hellgate.ch>
Revert previous patch with subject "change mdelay to msleep and remove
from ISR path". This patch seems to have caused bigger problems than
it solved, and it didn't solve much of a problem to begin with...
Signed-off-by: John W. Linville <linville@tuxdriver.com
---
Discussion about backing-out this patch can be found here:
http://marc.theaimsgroup.com/?l=linux-netdev&m=114321570402396&w=2
The git commit associated w/ the original patch is:
6ba98d311d0a4ff7dc36d8f435ce60174f4c30ec
drivers/net/via-rhine.c | 34 +++-------------------------------
1 files changed, 3 insertions(+), 31 deletions(-)
diff --git a/drivers/net/via-rhine.c b/drivers/net/via-rhine.c
index 2418715..e7b4bc3 100644
--- a/drivers/net/via-rhine.c
+++ b/drivers/net/via-rhine.c
@@ -490,8 +490,6 @@ struct rhine_private {
u8 tx_thresh, rx_thresh;
struct mii_if_info mii_if;
- struct work_struct tx_timeout_task;
- struct work_struct check_media_task;
void __iomem *base;
};
@@ -499,8 +497,6 @@ static int mdio_read(struct net_device
static void mdio_write(struct net_device *dev, int phy_id, int location, int value);
static int rhine_open(struct net_device *dev);
static void rhine_tx_timeout(struct net_device *dev);
-static void rhine_tx_timeout_task(struct net_device *dev);
-static void rhine_check_media_task(struct net_device *dev);
static int rhine_start_tx(struct sk_buff *skb, struct net_device *dev);
static irqreturn_t rhine_interrupt(int irq, void *dev_instance, struct pt_regs *regs);
static void rhine_tx(struct net_device *dev);
@@ -855,12 +851,6 @@ #endif
if (rp->quirks & rqRhineI)
dev->features |= NETIF_F_SG|NETIF_F_HW_CSUM;
- INIT_WORK(&rp->tx_timeout_task,
- (void (*)(void *))rhine_tx_timeout_task, dev);
-
- INIT_WORK(&rp->check_media_task,
- (void (*)(void *))rhine_check_media_task, dev);
-
/* dev->name not defined before register_netdev()! */
rc = register_netdev(dev);
if (rc)
@@ -1087,11 +1077,6 @@ static void rhine_check_media(struct net
ioaddr + ChipCmd1);
}
-static void rhine_check_media_task(struct net_device *dev)
-{
- rhine_check_media(dev, 0);
-}
-
static void init_registers(struct net_device *dev)
{
struct rhine_private *rp = netdev_priv(dev);
@@ -1145,8 +1130,8 @@ static void rhine_disable_linkmon(void _
if (quirks & rqRhineI) {
iowrite8(0x01, ioaddr + MIIRegAddr); // MII_BMSR
- /* Do not call from ISR! */
- msleep(1);
+ /* Can be called from ISR. Evil. */
+ mdelay(1);
/* 0x80 must be set immediately before turning it off */
iowrite8(0x80, ioaddr + MIICmd);
@@ -1236,16 +1221,6 @@ static int rhine_open(struct net_device
static void rhine_tx_timeout(struct net_device *dev)
{
struct rhine_private *rp = netdev_priv(dev);
-
- /*
- * Move bulk of work outside of interrupt context
- */
- schedule_work(&rp->tx_timeout_task);
-}
-
-static void rhine_tx_timeout_task(struct net_device *dev)
-{
- struct rhine_private *rp = netdev_priv(dev);
void __iomem *ioaddr = rp->base;
printk(KERN_WARNING "%s: Transmit timed out, status %4.4x, PHY status "
@@ -1651,7 +1626,7 @@ static void rhine_error(struct net_devic
spin_lock(&rp->lock);
if (intr_status & IntrLinkChange)
- schedule_work(&rp->check_media_task);
+ rhine_check_media(dev, 0);
if (intr_status & IntrStatsMax) {
rp->stats.rx_crc_errors += ioread16(ioaddr + RxCRCErrs);
rp->stats.rx_missed_errors += ioread16(ioaddr + RxMissed);
@@ -1899,9 +1874,6 @@ static int rhine_close(struct net_device
spin_unlock_irq(&rp->lock);
free_irq(rp->pdev->irq, dev);
-
- flush_scheduled_work();
-
free_rbufs(dev);
free_tbufs(dev);
free_ring(dev);
--
John W. Linville
linville@tuxdriver.com
^ permalink raw reply related
* Re: [PATCH 3/4] myri10ge - Driver core
From: Brice Goglin @ 2006-05-19 14:39 UTC (permalink / raw)
To: Arnd Bergmann; +Cc: netdev, gallatin, linux-kernel
In-Reply-To: <200605180108.32949.arnd@arndb.de>
> dev_err?
>
[...]
> Could probably use dev_printk.
>
When the interface name is known, we prefer having the message look like
"myri10ge: eth2: something" since it might be easier to read than
""myri10ge: 00005:000e: something". The administrator usually knows the
name of the network interface better than its bus ID.
Brice
^ permalink raw reply
* [PATCH] eth1394: endian fixes
From: Alexey Dobriyan @ 2006-05-19 13:25 UTC (permalink / raw)
To: netdev; +Cc: linux1394-devel
Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
---
drivers/ieee1394/eth1394.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
--- a/drivers/ieee1394/eth1394.c
+++ 1/drivers/ieee1394/eth1394.c
@@ -935,7 +935,7 @@ static inline u16 ether1394_parse_encap(
*(u32*)arp_ptr = arp1394->sip; /* move sender IP addr */
arp_ptr += arp->ar_pln; /* skip over sender IP addr */
- if (arp->ar_op == 1)
+ if (arp->ar_op == htons(1))
/* just set ARP req target unique ID to 0 */
*((u64*)arp_ptr) = 0;
else
@@ -1395,7 +1395,7 @@ static inline void ether1394_arp_to_1394
/* We need to encapsulate the standard header with our own. We use the
* ethernet header's proto for our own. */
static inline unsigned int ether1394_encapsulate_prep(unsigned int max_payload,
- int proto,
+ __be16 proto,
union eth1394_hdr *hdr,
u16 dg_size, u16 dgl)
{
@@ -1626,7 +1626,7 @@ static int ether1394_tx (struct sk_buff
gfp_t kmflags = in_interrupt() ? GFP_ATOMIC : GFP_KERNEL;
struct eth1394hdr *eth;
struct eth1394_priv *priv = netdev_priv(dev);
- int proto;
+ __be16 proto;
unsigned long flags;
nodeid_t dest_node;
eth1394_tx_type tx_type;
^ permalink raw reply
* Re: MultiPath Routing
From: pravin b shelar @ 2006-05-19 13:21 UTC (permalink / raw)
To: David S. Miller; +Cc: Herbert Xu, netdev, Ravikiran G Thirumalai
>Christian Schmid <webmaster@rapidforum.com> wrote:
>>
>> Since this seems to be an undocumented feature.... How is this equal-cost multipath supposed to
>> work? I just want to send outgoing traffic over two gateways. I tried this:
>>
>> ip route replace default scope global nexthop via 192.168.0.1 dev eth1 weight 100 nexthop via
>> 192.168.0.100 dev eth1 weight 100
>>
>> It it doesnt work. It always uses the last for all download-users (around a few hundreds)
>
>Did you enable IP_ROUTE_MULTIPATH_CACHED? If so please disable it
>since it breaks multipath routing for forwarded traffic.
Hi ,
This was a bug reported by Christian Schmid long back. It is related to equal cost multipath caching on forwarded traffic. The solution suggested for this was
to disable equal-cost multipath caching. But It does not look like a clean solution.
I investigated the problem further and observed that, If multipath caching is "on" then all route entries are added to the route cache. This leads to selection of same
NIC for all route going through this ( forwarding ) sever. This is because we don't have selection logic for forwarded traffic in multipath caching code.
On digging this list further, I found a patch posted by Herbert, which disables multipath route cache entry addition for input case.
I tested this patch and everything works fine now.
for the patch ref: http://oss.sgi.com/projects/netdev/archive/2005-04/msg00232.html
So how about including this patch ?
Comments ?
Regards,
Pravin.
^ permalink raw reply
* [PATCH] irda: fixup type of ->lsap_state
From: Alexey Dobriyan @ 2006-05-19 11:42 UTC (permalink / raw)
To: netdev; +Cc: Samuel Ortiz
Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
---
--- a/include/net/irda/irlmp.h
+++ 1/include/net/irda/irlmp.h
@@ -112,7 +112,7 @@ struct lsap_cb {
struct timer_list watchdog_timer;
- IRLMP_STATE lsap_state; /* Connection state */
+ LSAP_STATE lsap_state; /* Connection state */
notify_t notify; /* Indication/Confirm entry points */
struct qos_info qos; /* QoS for this connection */
^ permalink raw reply
* [PATCH] irda: fix 16/32 bit confusion
From: Alexey Dobriyan @ 2006-05-19 11:35 UTC (permalink / raw)
To: netdev; +Cc: Samuel Ortiz
Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
---
--- a/net/irda/iriap.c
+++ 1/net/irda/iriap.c
@@ -544,7 +544,8 @@ static void iriap_getvaluebyclass_respon
{
struct sk_buff *tx_skb;
int n;
- __u32 tmp_be32, tmp_be16;
+ __u32 tmp_be32;
+ __be16 tmp_be16;
__u8 *fp;
IRDA_DEBUG(4, "%s()\n", __FUNCTION__);
^ permalink raw reply
* Re: [PATCH 3/4] myri10ge - Driver core
From: Andi Kleen @ 2006-05-19 11:09 UTC (permalink / raw)
To: Arnd Bergmann; +Cc: Brice Goglin, netdev, gallatin, linux-kernel
In-Reply-To: <200605191200.47132.arnd@arndb.de>
On Friday 19 May 2006 12:00, Arnd Bergmann wrote:
> On Friday 19 May 2006 04:25, Brice Goglin wrote:
> > dev_mc_upload() from net/core/dev_mcast.c does
> >
> > spin_lock_bh(&dev->xmit_lock);
> > __dev_mc_upload(dev);
> >
> > which calls dev->set_multicast_list(), which is
> > myri10ge_set_multicast_list()
> >
> > which calls myri10ge_change_promisc
> >
> > which calls myri10ge_send_cmd
>
> Hmm, if that is the only path where you call it, it may be
> helpful to change myri10ge_change_promisc to call a special
> atomic version of myri10ge_send_cmd then, while all others
> use the regular version, which you can then convert to
> do msleep as well.
Or just drop the xmit lock while you do that. As long as you
handle races with ->start_xmit yourself it's ok
-Andi
^ permalink raw reply
* Re: [PATCH 3/4] myri10ge - Driver core
From: Arnd Bergmann @ 2006-05-19 10:00 UTC (permalink / raw)
To: Brice Goglin; +Cc: netdev, gallatin, linux-kernel
In-Reply-To: <446D2CA7.2070009@myri.com>
On Friday 19 May 2006 04:25, Brice Goglin wrote:
> dev_mc_upload() from net/core/dev_mcast.c does
>
> spin_lock_bh(&dev->xmit_lock);
> __dev_mc_upload(dev);
>
> which calls dev->set_multicast_list(), which is
> myri10ge_set_multicast_list()
>
> which calls myri10ge_change_promisc
>
> which calls myri10ge_send_cmd
Hmm, if that is the only path where you call it, it may be
helpful to change myri10ge_change_promisc to call a special
atomic version of myri10ge_send_cmd then, while all others
use the regular version, which you can then convert to
do msleep as well.
Arnd <><
^ permalink raw reply
* Re: [PATCH 3/4] myri10ge - Driver core
From: Brice Goglin @ 2006-05-19 2:25 UTC (permalink / raw)
To: Arnd Bergmann; +Cc: netdev, gallatin, linux-kernel
In-Reply-To: <200605190355.11230.arnd@arndb.de>
Arnd Bergmann wrote:
> Am Friday 19 May 2006 01:56 schrieb Brice Goglin:
>
>> This place is actually the only one where we don't want to use msleep.
>> This function (myri10ge_send_cmd) might be called from various context
>> (spinlocked or not) and pass orders to the NIC whose processing time
>> depends a lot on the command. Of course, we don't have any place where a
>> long operation is passed from a spinlocked context :) But, we need the
>> tiny udelay granularity for the spinlocked case, and the long loop for
>> operations that are long to process in the NIC.
>>
>
> I don't see any spinlocks in your code and the function does not
> seem to be called from the interrupt handler or the softirq either.
> Maybe I'm missed something, but where is this ever called in an
> atomic context?
>
dev_mc_upload() from net/core/dev_mcast.c does
spin_lock_bh(&dev->xmit_lock);
__dev_mc_upload(dev);
which calls dev->set_multicast_list(), which is
myri10ge_set_multicast_list()
which calls myri10ge_change_promisc
which calls myri10ge_send_cmd
> Whenever you have a device associated with the message, it makes
> sense to use the dev_printk family of functions.
>
Ok, thanks.
> My understanding is that vendor IDs should go to the common file
> because they are likely to be used by multiple drivers whereas
> device IDs only need to be present in the one device driver for
> that particular device.
>
Make sense. I will change it.
Thanks again,
Brice
^ permalink raw reply
* Re: [PATCH 3/4] myri10ge - Driver core
From: Arnd Bergmann @ 2006-05-19 1:55 UTC (permalink / raw)
To: Brice Goglin; +Cc: netdev, gallatin, linux-kernel
In-Reply-To: <446D0994.8090103@myri.com>
Am Friday 19 May 2006 01:56 schrieb Brice Goglin:
> This place is actually the only one where we don't want to use msleep.
> This function (myri10ge_send_cmd) might be called from various context
> (spinlocked or not) and pass orders to the NIC whose processing time
> depends a lot on the command. Of course, we don't have any place where a
> long operation is passed from a spinlocked context :) But, we need the
> tiny udelay granularity for the spinlocked case, and the long loop for
> operations that are long to process in the NIC.
I don't see any spinlocks in your code and the function does not
seem to be called from the interrupt handler or the softirq either.
Maybe I'm missed something, but where is this ever called in an
atomic context?
> > missing printk level (KERN_DEBUG?). Could probably use dev_printk.
>
> When are we supposed to call dev_printk or not in such a driver ?
Whenever you have a device associated with the message, it makes
sense to use the dev_printk family of functions.
> >> +#define MYRI10GE_PCI_VENDOR_MYRICOM 0x14c1
> >> +#define MYRI10GE_PCI_DEVICE_Z8E 0x0008
> >
> > Shouldn't the vendor ID go to pci_ids.h?
>
> That's what I thought but i was told that the fashion these days is to
> keep the IDs with the driver that uses them. I'll happy to move as long
> as everybody agrees :)
My understanding is that vendor IDs should go to the common file
because they are likely to be used by multiple drivers whereas
device IDs only need to be present in the one device driver for
that particular device.
Arnd <><
^ permalink raw reply
* Re: 16/32 bit issues at nfulnl_recv_config
From: Patrick McHardy @ 2006-05-19 1:15 UTC (permalink / raw)
To: Alexey Dobriyan; +Cc: netdev, netfilter-devel
In-Reply-To: <20060518232050.GA31257@mipter.zuzino.mipt.ru>
[-- Attachment #1: Type: text/plain, Size: 1368 bytes --]
Alexey Dobriyan wrote:
> I'm talking about net/netfilter/nfnetlink_log.c::^nfulnl_recv_config
> below. I'm totally confused and beer supply almost ended, so can someone else
> also look at it...
>
> Put your attention at line 905:
>
> net/netfilter/nfnetlink_log.c:
>
> 902 if (nfula[NFULA_CFG_FLAGS-1]) {
> 903 u_int16_t flags =
> 904 *(u_int16_t *)NFA_DATA(nfula[NFULA_CFG_FLAGS-1]);
> 905 nfulnl_set_flags(inst, ntohl(flags));
>
>
> 1) Cast should be made to __be16, and history of fixing endian warning
> slightly above supports it.
> 2) Assuming __be16, ntohl(__be16) is whooops.
> 3) nfulnl_set_flags() wants something 16-bit wide as a second argument,
> so ntohling there is double whoops.
> 4) NFULNL_CFG_F_SEQ* defines fit into 16 bit and are host-endian;
> inst->flags is &'ed with host endian so it's probably host-endian and
>
> static int
> nfulnl_set_flags(struct nfulnl_instance *inst, u_int16_t flags)
> ^^^^^^^^^
> this should be made __be16.
> {
> spin_lock_bh(&inst->lock);
> inst->flags = ntohs(flags);
> spin_unlock_bh(&inst->lock);
> return 0;
> }
>
> What to do with lines 903-905 if it is.
I checked the userspace code, "flags" is indeed a u16 in network byte
order, so the ntohl in line 905 needs to become ntohs and the other
one can just go, as done by this patch.
[-- Attachment #2: x --]
[-- Type: text/plain, Size: 1292 bytes --]
[NETFILTER]: nfnetlink_log: fix byteorder confusion
flags is a u16, so use htons instead of htonl. Also avoid double
conversion.
Noticed by Alexey Dobriyan <adobriyan@gmail.com>
Signed-off-by: Patrick McHardy <kaber@trash.net>
---
commit ad1dcdc1f150f613ff8625fed88ed5c1d089d09a
tree fa34bb52998bd56144639dfe6b2c286b114ae59a
parent f372e5df6ab4cd1e1498489562af2095fb5aec7c
author Patrick McHardy <kaber@trash.net> Fri, 19 May 2006 03:12:08 +0200
committer Patrick McHardy <kaber@trash.net> Fri, 19 May 2006 03:12:08 +0200
net/netfilter/nfnetlink_log.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/net/netfilter/nfnetlink_log.c b/net/netfilter/nfnetlink_log.c
index c60273c..61cdda4 100644
--- a/net/netfilter/nfnetlink_log.c
+++ b/net/netfilter/nfnetlink_log.c
@@ -321,7 +321,7 @@ static int
nfulnl_set_flags(struct nfulnl_instance *inst, u_int16_t flags)
{
spin_lock_bh(&inst->lock);
- inst->flags = ntohs(flags);
+ inst->flags = flags;
spin_unlock_bh(&inst->lock);
return 0;
@@ -902,7 +902,7 @@ nfulnl_recv_config(struct sock *ctnl, st
if (nfula[NFULA_CFG_FLAGS-1]) {
u_int16_t flags =
*(u_int16_t *)NFA_DATA(nfula[NFULA_CFG_FLAGS-1]);
- nfulnl_set_flags(inst, ntohl(flags));
+ nfulnl_set_flags(inst, ntohs(flags));
}
out_put:
^ permalink raw reply related
* Re: [PATCH 3/4] myri10ge - Driver core
From: Brice Goglin @ 2006-05-18 23:56 UTC (permalink / raw)
To: Arnd Bergmann; +Cc: netdev, gallatin, linux-kernel
In-Reply-To: <200605180108.32949.arnd@arndb.de>
Arnd Bergmann wrote:
>> + for (sleep_total = 0;
>> + sleep_total < (15 * 1000) && response->result == 0xffffffff;
>> + sleep_total += 10) {
>> + udelay(10);
>> + }
>>
>
> udelay does not sleep. If you want to sleep, use msleep instead.
>
This place is actually the only one where we don't want to use msleep.
This function (myri10ge_send_cmd) might be called from various context
(spinlocked or not) and pass orders to the NIC whose processing time
depends a lot on the command. Of course, we don't have any place where a
long operation is passed from a spinlocked context :) But, we need the
tiny udelay granularity for the spinlocked case, and the long loop for
operations that are long to process in the NIC.
Concerning all the other places where you suggested to use msleep, you
were right.
> The __iomem variable need not be volatile.
As Roland pointed out, there was too many volatile in this code. We are
reworking this together with the sparse annotations.
>> + printk("myri10ge: %s: %s IRQ %d, tx bndry %d, fw %s, WC %s\n",
>> + netdev->name, (mgp->msi_enabled ? "MSI" : "xPIC"),
>> + pdev->irq, mgp->tx.boundary, mgp->fw_name,
>> + (mgp->mtrr >= 0 ? "Enabled" : "Disabled"));
>> +
>>
>
> missing printk level (KERN_DEBUG?). Could probably use dev_printk.
>
When are we supposed to call dev_printk or not in such a driver ?
>> +#define MYRI10GE_PCI_VENDOR_MYRICOM 0x14c1
>> +#define MYRI10GE_PCI_DEVICE_Z8E 0x0008
>>
>
> Shouldn't the vendor ID go to pci_ids.h?
>
That's what I thought but i was told that the fashion these days is to
keep the IDs with the driver that uses them. I'll happy to move as long
as everybody agrees :)
Thanks a lot for all your comments,
Brice
^ permalink raw reply
* Re: [PATCH 3/4] myri10ge - Driver core
From: Brice Goglin @ 2006-05-18 23:38 UTC (permalink / raw)
To: Roland Dreier; +Cc: netdev, gallatin, linux-kernel
In-Reply-To: <adairo446u3.fsf@cisco.com>
Roland Dreier wrote:
> Still some suspicious uses of volatile here.
>
> For example:
>
>
>> +struct myri10ge_priv {
>>
> ...
>
>> + volatile u8 __iomem *sram;
>>
>
> as far as I can see this is always used with proper __iomem accessors,
> often with casts to strip the volatile anyway. So why is volatile needed?
>
> I would suggest an audit of all uses of volatile in the driver, since
> "volatile" in drivers really should be read "there's probably a bug
> here, and if not something very tricky is going on." If there are any
> valid uses of volatile then a comment should explain why, so that
> future reviewers don't have to try and puzzle out which of the
> two possible translations of volatile is correct.
>
You are right, we audited the code and it looks like we don't need any
volatile.
Thanks,
Brice
^ permalink raw reply
* Re: [PATCH 2/4] myri10ge - Driver header files
From: Brice Goglin @ 2006-05-18 23:36 UTC (permalink / raw)
To: Randy.Dunlap; +Cc: netdev, gallatin, linux-kernel
In-Reply-To: <20060517152817.58a6d8bc.rdunlap@xenotime.net>
Randy.Dunlap wrote:
>> [PATCH 2/4] myri10ge - Driver header files
>>
>> myri10ge_mcp.h | 205 ++++++++++++++++++++++++++++++++++++++++++++++
>> myri10ge_mcp_gen_header.h | 58 +++++++++++++
>>
>
> Please use "diffstat -p 1 -w 70" is documented in
> Documentation/SubmittingPatches.
>
Ok, will do.
>> +/* 16 Bytes */
>>
> What is 16 bytes here?
>
>> +struct mcp_slot {
>> + u16 checksum;
>> + u16 length;
>> +};
>>
Looks like I have problems to compute the size of this type.
Thanks,
Brice
^ permalink raw reply
* 16/32 bit issues at nfulnl_recv_config
From: Alexey Dobriyan @ 2006-05-18 23:20 UTC (permalink / raw)
To: netdev; +Cc: netfilter-devel
I'm talking about net/netfilter/nfnetlink_log.c::^nfulnl_recv_config
below. I'm totally confused and beer supply almost ended, so can someone else
also look at it...
Put your attention at line 905:
net/netfilter/nfnetlink_log.c:
902 if (nfula[NFULA_CFG_FLAGS-1]) {
903 u_int16_t flags =
904 *(u_int16_t *)NFA_DATA(nfula[NFULA_CFG_FLAGS-1]);
905 nfulnl_set_flags(inst, ntohl(flags));
1) Cast should be made to __be16, and history of fixing endian warning
slightly above supports it.
2) Assuming __be16, ntohl(__be16) is whooops.
3) nfulnl_set_flags() wants something 16-bit wide as a second argument,
so ntohling there is double whoops.
4) NFULNL_CFG_F_SEQ* defines fit into 16 bit and are host-endian;
inst->flags is &'ed with host endian so it's probably host-endian and
static int
nfulnl_set_flags(struct nfulnl_instance *inst, u_int16_t flags)
^^^^^^^^^
this should be made __be16.
{
spin_lock_bh(&inst->lock);
inst->flags = ntohs(flags);
spin_unlock_bh(&inst->lock);
return 0;
}
What to do with lines 903-905 if it is.
Alexey "confused in Moscow" Dobriyan
^ permalink raw reply
* Re: about
From: Jackson Espinosa @ 2006-05-18 23:53 UTC (permalink / raw)
To: netdev
-S'ensationall revoolution in m'edicine!
-E'n'l'a'r'g'e your p''enis up to 10 cm or up to 4 inches!
-It's herbal solution what hasn't side effect, but has 100% guaranted results!
-Don't lose your chance and but know wihtout doubts, you will be impressed with results!
Clisk here: http://branairsystems.info
habitation parkish lifelike treadmill afoul habeas paoli none compensable seder
mule argive claus colloq cohomology herschel pistol seaman flounce
frontiersman jobholder deuterium obeisant lookout earphone belligerent eggplant barnet northeastern kamchatka
brush deprive pharmacology trapezoidal celebes dachshund vhf corsage
elan debra spurious brad jitterbugging diplomat freddie canaan
hundredth brother axiology gape constantinople aerosol transducer accession
^ permalink raw reply
* Re: [Bugme-new] [Bug 6579] New: Kernel Oops, IRDA stack related
From: Andrew Morton @ 2006-05-18 18:41 UTC (permalink / raw)
To: bugme-daemon; +Cc: netdev, Samuel Ortiz, Christoph Hellwig, zilvinas
In-Reply-To: <200605181710.k4IHA3PH021040@fire-2.osdl.org>
bugme-daemon@bugzilla.kernel.org wrote:
>
> http://bugzilla.kernel.org/show_bug.cgi?id=6579
>
> Summary: Kernel Oops, IRDA stack related
> Kernel Version: v2.6.16.16 + squashfs v3.0 patch
> Status: NEW
> Severity: normal
> Owner: drivers_other@kernel-bugs.osdl.org
> Submitter: zilvinas@gemtek.lt
>
>
> Most recent kernel where this bug did not occur:
> v2.6.16.16 - haven't tried earlier versions. Just now started using IRDA for
> phone management.
>
> Distribution:
> Debian Unstable/Uptodate. Kernel is vanilla v2.6.16.16 + squashfs 3.0 patch
> applied. Otherwise clean tree.
>
> Hardware Environment:
> Evo N800v,
> Software Environment:
> Linux huh 2.6.16.16-evo #1 Fri May 12 12:17:14 EEST 2006 i686 GNU/Linux
>
> Gnu C 4.0.4
> Gnu make 3.81
> binutils 2.16.91
> util-linux 2.12r
> mount 2.12r
> module-init-tools 3.2.2
> e2fsprogs 1.39-WIP
> reiserfsprogs 3.6.19
> xfsprogs 2.7.16
> pcmcia-cs 3.2.8
> nfs-utils 1.0.7
> Linux C Library 2.3.6
> Dynamic linker (ldd) 2.3.6
> Procps 3.2.6
> Net-tools 1.60
> Console-tools 0.2.3
> Sh-utils 5.94
> udev 092
> Modules Loaded option radeon drm nfsd lockd sunrpc ipv6 deflate
> zlib_deflate zlib_inflate twofish serpent aes blowfish des sha256 sha1 md5
> crypto_null dm_mod irnet ppp_generic slhc irlan ircomm_tty ircomm pl2303
> usbserial pcmcia firmware_class snd_intel8x0 snd_ac97_codec snd_ac97_bus
> snd_pcm_oss snd_mixer_oss irtty_sir sir_dev pcspkr snd_pcm snd_timer irda
> 8250_pnp ohci_hcd ide_cd crc_ccitt floppy ehci_hcd 8250 serial_core yenta_socket
> cdrom snd rsrc_nonstatic soundcore pcmcia_core snd_page_alloc intel_agp agpgart
>
> Problem Description:
>
> Recently I've started using IRDA, to manage phone/calendar & co on Nokia 6310
> phone. For this task I am using IRDA and xgnokii management software. Sometimes
> for no reason I am getting oopses at random addresses. System is not locking up
> or so. It always prints oops followed by messages :
>
> May 14 00:40:52 huh kernel: <1>Fixing recursive fault but reboot is needed!
>
> After reboot it takes again some time - ooops is reported at different location
> (mostly random, but backtrace is same).
>
> Steps to reproduce:
> It is quite simple, attempt to download phonebook, calendar, then attempt to
> upload. Sometimes it happens during logo download/upload.
>
> Oops will be added in the next comments.
>
It's hitting list_head poisoning in
irda_release->sock_def_wakeup->__wake_up_common.
Appears to be a post-2.6.16 regression.
^ permalink raw reply
* [PATCH 1a/5] sky2: allow dual port usage
From: Stephen Hemminger @ 2006-05-18 18:16 UTC (permalink / raw)
To: Jeff Garzik; +Cc: netdev
In-Reply-To: <20060517213801.841172000@localhost.localdomain>
If both ports are receiving on the SysKonnect dual port cards,
then it appears the bus interface unit can give an interrupt status
for frame before DMA has completed. This leads to bogus frames
and general confusion. This is why receive checksumming is also
messed up on dual port cards.
A workaround for the out of order receive problem is to eliminating
split transactions on PCI-X.
This version is based of the current linux-2.6.git including earlier
patch to disable dual ports.
Signed-off-by: Stephen Hemminger <shemminger@osdl.org>
--- sky2.orig/drivers/net/sky2.c 2006-05-18 11:08:10.000000000 -0700
+++ sky2/drivers/net/sky2.c 2006-05-18 11:11:48.000000000 -0700
@@ -1020,19 +1020,26 @@
struct sky2_hw *hw = sky2->hw;
unsigned port = sky2->port;
u32 ramsize, rxspace, imask;
- int err;
+ int cap, err = -ENOMEM;
struct net_device *otherdev = hw->dev[sky2->port^1];
- /* Block bringing up both ports at the same time on a dual port card.
- * There is an unfixed bug where receiver gets confused and picks up
- * packets out of order. Until this is fixed, prevent data corruption.
+ /*
+ * On dual port PCI-X card, there is an problem where status
+ * can be received out of order due to split transactions
*/
- if (otherdev && netif_running(otherdev)) {
- printk(KERN_INFO PFX "dual port support is disabled.\n");
- return -EBUSY;
- }
-
- err = -ENOMEM;
+ if (otherdev && netif_running(otherdev) &&
+ (cap = pci_find_capability(hw->pdev, PCI_CAP_ID_PCIX))) {
+ struct sky2_port *osky2 = netdev_priv(otherdev);
+ u16 cmd;
+
+ cmd = sky2_pci_read16(hw, cap + PCI_X_CMD);
+ cmd &= ~PCI_X_CMD_MAX_SPLIT;
+ sky2_pci_write16(hw, cap + PCI_X_CMD, cmd);
+
+ sky2->rx_csum = 0;
+ osky2->rx_csum = 0;
+ }
+
if (netif_msg_ifup(sky2))
printk(KERN_INFO PFX "%s: enabling interface\n", dev->name);
@@ -3078,12 +3085,7 @@
sky2->duplex = -1;
sky2->speed = -1;
sky2->advertising = sky2_supported_modes(hw);
-
- /* Receive checksum disabled for Yukon XL
- * because of observed problems with incorrect
- * values when multiple packets are received in one interrupt
- */
- sky2->rx_csum = (hw->chip_id != CHIP_ID_YUKON_XL);
+ sky2->rx_csum = 1;
spin_lock_init(&sky2->phy_lock);
sky2->tx_pending = TX_DEF_PENDING;
^ permalink raw reply
* Re: Please pull 'upstream' branch of wireless-2.6
From: Daniel Drake @ 2006-05-18 18:26 UTC (permalink / raw)
To: Daniel Drake, jeff, netdev, arjan
In-Reply-To: <20060518172844.GC21325@tuxdriver.com>
John W. Linville wrote:
> On Wed, May 17, 2006 at 10:23:34PM +0100, Daniel Drake wrote:
>> John W. Linville wrote:
>>> Daniel Drake:
>>> set_security implementation inside softmac
>> If it's not too late, can you drop this one?
>
> Well, it's your patch. If you want to withdraw it, I'll comply.
Please do. I didn't sign off or send it to you directly as I wasn't
asking for inclusion, but I'll make that clearer next time.
Thanks,
Daniel
^ permalink raw reply
* Re: Please pull 'upstream' branch of wireless-2.6
From: John W. Linville @ 2006-05-18 17:28 UTC (permalink / raw)
To: Daniel Drake; +Cc: jeff, netdev, arjan
In-Reply-To: <446B9456.5030308@gentoo.org>
On Wed, May 17, 2006 at 10:23:34PM +0100, Daniel Drake wrote:
> John W. Linville wrote:
> >Daniel Drake:
> > set_security implementation inside softmac
>
> If it's not too late, can you drop this one?
Well, it's your patch. If you want to withdraw it, I'll comply.
---
The following changes since commit 59d4b684fbe74b25ac8593f39d2aaa86aa485b1f:
John W. Linville:
Merge branch 'upstream-fixes' into upstream
are found in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/linville/wireless-2.6.git upstream
Arjan van de Ven:
unused exports in wireless drivers
drivers/net/wireless/hostap/hostap_80211_tx.c | 1 -
drivers/net/wireless/hostap/hostap_ap.c | 11 -----------
drivers/net/wireless/hostap/hostap_main.c | 2 --
3 files changed, 0 insertions(+), 14 deletions(-)
diff --git a/drivers/net/wireless/hostap/hostap_80211_tx.c b/drivers/net/wireless/hostap/hostap_80211_tx.c
index 06a5214..4a5be70 100644
--- a/drivers/net/wireless/hostap/hostap_80211_tx.c
+++ b/drivers/net/wireless/hostap/hostap_80211_tx.c
@@ -534,5 +534,4 @@ int hostap_master_start_xmit(struct sk_b
}
-EXPORT_SYMBOL(hostap_dump_tx_80211);
EXPORT_SYMBOL(hostap_master_start_xmit);
diff --git a/drivers/net/wireless/hostap/hostap_ap.c b/drivers/net/wireless/hostap/hostap_ap.c
index 06c3fa3..ba13125 100644
--- a/drivers/net/wireless/hostap/hostap_ap.c
+++ b/drivers/net/wireless/hostap/hostap_ap.c
@@ -3276,17 +3276,6 @@ EXPORT_SYMBOL(hostap_init_data);
EXPORT_SYMBOL(hostap_init_ap_proc);
EXPORT_SYMBOL(hostap_free_data);
EXPORT_SYMBOL(hostap_check_sta_fw_version);
-EXPORT_SYMBOL(hostap_handle_sta_tx);
-EXPORT_SYMBOL(hostap_handle_sta_release);
EXPORT_SYMBOL(hostap_handle_sta_tx_exc);
-EXPORT_SYMBOL(hostap_update_sta_ps);
-EXPORT_SYMBOL(hostap_handle_sta_rx);
-EXPORT_SYMBOL(hostap_is_sta_assoc);
-EXPORT_SYMBOL(hostap_is_sta_authorized);
-EXPORT_SYMBOL(hostap_add_sta);
-EXPORT_SYMBOL(hostap_update_rates);
-EXPORT_SYMBOL(hostap_add_wds_links);
-EXPORT_SYMBOL(hostap_wds_link_oper);
#ifndef PRISM2_NO_KERNEL_IEEE80211_MGMT
-EXPORT_SYMBOL(hostap_deauth_all_stas);
#endif /* PRISM2_NO_KERNEL_IEEE80211_MGMT */
diff --git a/drivers/net/wireless/hostap/hostap_main.c b/drivers/net/wireless/hostap/hostap_main.c
index 8dd4c44..93786f4 100644
--- a/drivers/net/wireless/hostap/hostap_main.c
+++ b/drivers/net/wireless/hostap/hostap_main.c
@@ -1125,11 +1125,9 @@ EXPORT_SYMBOL(hostap_set_auth_algs);
EXPORT_SYMBOL(hostap_dump_rx_header);
EXPORT_SYMBOL(hostap_dump_tx_header);
EXPORT_SYMBOL(hostap_80211_header_parse);
-EXPORT_SYMBOL(hostap_80211_prism_header_parse);
EXPORT_SYMBOL(hostap_80211_get_hdrlen);
EXPORT_SYMBOL(hostap_get_stats);
EXPORT_SYMBOL(hostap_setup_dev);
-EXPORT_SYMBOL(hostap_proc);
EXPORT_SYMBOL(hostap_set_multicast_list_queue);
EXPORT_SYMBOL(hostap_set_hostapd);
EXPORT_SYMBOL(hostap_set_hostapd_sta);
--
John W. Linville
linville@tuxdriver.com
^ permalink raw reply related
* Re: [PATCH] gre_in_range: 16/32 bit fix
From: Patrick McHardy @ 2006-05-18 14:47 UTC (permalink / raw)
To: Alexey Dobriyan; +Cc: netdev, netfilter-devel
In-Reply-To: <20060518133852.GA13516@mipter.zuzino.mipt.ru>
Alexey Dobriyan wrote:
> GRE keys are 16 bit.
Thanks, I've combined both patches and added them to me tree, will
push to Dave soon.
^ permalink raw reply
* [PATCH] gre_unique_tuple: 16/32 bit fixes
From: Alexey Dobriyan @ 2006-05-18 13:52 UTC (permalink / raw)
To: netdev
GRE keys are 16 bit.
Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
---
net/ipv4/netfilter/ip_nat_proto_gre.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
--- a/net/ipv4/netfilter/ip_nat_proto_gre.c
+++ 1/net/ipv4/netfilter/ip_nat_proto_gre.c
@@ -81,14 +81,14 @@ gre_unique_tuple(struct ip_conntrack_tup
min = 1;
range_size = 0xffff;
} else {
- min = ntohl(range->min.gre.key);
- range_size = ntohl(range->max.gre.key) - min + 1;
+ min = ntohs(range->min.gre.key);
+ range_size = ntohs(range->max.gre.key) - min + 1;
}
DEBUGP("min = %u, range_size = %u\n", min, range_size);
for (i = 0; i < range_size; i++, key++) {
- *keyptr = htonl(min + key % range_size);
+ *keyptr = htons(min + key % range_size);
if (!ip_nat_used_tuple(tuple, conntrack))
return 1;
}
^ permalink raw reply
* [PATCH] gre_in_range: 16/32 bit fix
From: Alexey Dobriyan @ 2006-05-18 13:38 UTC (permalink / raw)
To: netdev; +Cc: netfilter-devel
GRE keys are 16 bit.
Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
---
net/ipv4/netfilter/ip_nat_proto_gre.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
--- a/net/ipv4/netfilter/ip_nat_proto_gre.c
+++ 1/net/ipv4/netfilter/ip_nat_proto_gre.c
@@ -49,15 +49,15 @@ gre_in_range(const struct ip_conntrack_t
const union ip_conntrack_manip_proto *min,
const union ip_conntrack_manip_proto *max)
{
- u_int32_t key;
+ __be16 key;
if (maniptype == IP_NAT_MANIP_SRC)
key = tuple->src.u.gre.key;
else
key = tuple->dst.u.gre.key;
- return ntohl(key) >= ntohl(min->gre.key)
- && ntohl(key) <= ntohl(max->gre.key);
+ return ntohs(key) >= ntohs(min->gre.key)
+ && ntohs(key) <= ntohs(max->gre.key);
}
/* generate unique tuple ... */
^ 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