These are my first patches for network drivers.
Please comment if I'm doing something wrong.
Compile tested. Testers with hw wanted.
--
vda
diff -u --recursive linux-2.5.38orig/drivers/net/depca.c linux-2.5.38/drivers/net/depca.c
--- linux-2.5.38orig/drivers/net/depca.c Sun Sep 22 04:25:10 2002
+++ linux-2.5.38/drivers/net/depca.c Wed Oct 2 01:16:57 2002
@@ -1910,6 +1910,7 @@
struct depca_ioctl *ioc = (struct depca_ioctl *) &rq->ifr_data;
int i, status = 0;
u_long ioaddr = dev->base_addr;
+ unsigned long flags;
union {
u8 addr[(HASH_TABLE_LEN * ETH_ALEN)];
u16 sval[(HASH_TABLE_LEN * ETH_ALEN) >> 1];
@@ -1999,18 +2000,19 @@
break;
case DEPCA_GET_STATS: /* Get the driver statistics */
- cli();
+
+ spin_lock_irqsave(&lp->lock, flags);
ioc->len = sizeof(lp->pktStats);
if (copy_to_user(ioc->data, &lp->pktStats, ioc->len))
status = -EFAULT;
- sti();
+ spin_unlock_irqrestore(&lp->lock, flags);
break;
case DEPCA_CLR_STATS: /* Zero out the driver statistics */
if (!capable(CAP_NET_ADMIN)) return -EPERM;
- cli();
+ spin_lock_irqsave(&lp->lock, flags);
memset(&lp->pktStats, 0, sizeof(lp->pktStats));
- sti();
+ spin_unlock_irqrestore(&lp->lock, flags);
break;
case DEPCA_GET_REG: /* Get the DEPCA Registers */
Denis Vlasenko <[email protected]> :
> diff -u --recursive linux-2.5.38orig/drivers/net/depca.c linux-2.5.38/drivers/net/depca.c
> --- linux-2.5.38orig/drivers/net/depca.c Sun Sep 22 04:25:10 2002
> +++ linux-2.5.38/drivers/net/depca.c Wed Oct 2 01:16:57 2002
[...]
> @@ -1999,18 +2000,19 @@
> break;
>
> case DEPCA_GET_STATS: /* Get the driver statistics */
> - cli();
> +
> + spin_lock_irqsave(&lp->lock, flags);
> ioc->len = sizeof(lp->pktStats);
> if (copy_to_user(ioc->data, &lp->pktStats, ioc->len))
> status = -EFAULT;
> - sti();
> + spin_unlock_irqrestore(&lp->lock, flags);
- copy_to_user() may sleep. Sleeping with spinlock held hurts badly.
- on SMP, pktStat can be updated while the copy progresses, see depca_rx().
--
Ueimor
On 2 October 2002 18:40, Francois Romieu wrote:
> Denis Vlasenko <[email protected]> :
> > diff -u --recursive linux-2.5.38orig/drivers/net/depca.c
> > linux-2.5.38/drivers/net/depca.c ---
> > linux-2.5.38orig/drivers/net/depca.c Sun Sep 22 04:25:10 2002 +++
> > linux-2.5.38/drivers/net/depca.c Wed Oct 2 01:16:57 2002
>
> [...]
>
> > @@ -1999,18 +2000,19 @@
> > break;
> >
> > case DEPCA_GET_STATS: /* Get the driver statistics */
> > - cli();
> > +
> > + spin_lock_irqsave(&lp->lock, flags);
> > ioc->len = sizeof(lp->pktStats);
> > if (copy_to_user(ioc->data, &lp->pktStats, ioc->len))
> > status = -EFAULT;
> > - sti();
> > + spin_unlock_irqrestore(&lp->lock, flags);
Did I say it's my first network patch? :-)
> - copy_to_user() may sleep. Sleeping with spinlock held hurts badly.
Ho to do it properly? Make a copy on stack under lock, release lock,
proceed with copy_to_user? That's 88 bytes at least...
> - on SMP, pktStat can be updated while the copy progresses, see depca_rx().
Should I place these pktStat updates under lp->lock?
--
vda
Denis Vlasenko <[email protected]> :
[...]
> Ho to do it properly? Make a copy on stack under lock, release lock,
> proceed with copy_to_user? That's 88 bytes at least...
Please see ETHTOOL_GSTATS usage in drivers/net/8139cp.c.
> > - on SMP, pktStat can be updated while the copy progresses, see depca_rx().
>
> Should I place these pktStat updates under lp->lock?
You may.
depca_rx() looks strange:
buf = skb_put(skb, len);
[...]
netif_rx(skb);
[...]
if (buf[0] & ...)
--
Ueimor
On 2 October 2002 20:12, Francois Romieu wrote:
> Denis Vlasenko <[email protected]> :
> [...]
>
> > Ho to do it properly? Make a copy on stack under lock, release
> > lock, proceed with copy_to_user? That's 88 bytes at least...
>
> Please see ETHTOOL_GSTATS usage in drivers/net/8139cp.c.
This:
tmp_stats = kmalloc(sz, GFP_KERNEL);
if (!tmp_stats) return -ENOMEM;
memset(tmp_stats, 0, sz);
i = 0;
tmp_stats[i++] = le64_to_cpu(cp->nic_stats->tx_ok);
tmp_stats[i++] = le64_to_cpu(cp->nic_stats->rx_ok);
...
tmp_stats[i++] = le16_to_cpu(cp->nic_stats->tx_underrun);
tmp_stats[i++] = cp->cp_stats.rx_frags;
if (i != CP_NUM_STATS) BUG();
i = copy_to_user(useraddr + sizeof(estats), tmp_stats, sz);
kfree(tmp_stats);
kmalloc() isn't very fast, but I suppose ioctl is not that critical.
> > > - on SMP, pktStat can be updated while the copy progresses, see
> > > depca_rx().
> >
> > Should I place these pktStat updates under lp->lock?
>
> You may.
>
> depca_rx() looks strange:
> buf = skb_put(skb, len);
> [...]
> netif_rx(skb);
> [...]
> if (buf[0] & ...)
I'd say this network stuff is a bit cryptic for untrained eye :-)
What's strange with that code?
BTW, is there some (downloadable) book on Linux networking
internals?
--
vda
On 2 October 2002 20:12, Francois Romieu wrote:
> Denis Vlasenko <[email protected]> :
> [...]
>
> > Ho to do it properly? Make a copy on stack under lock, release
> > lock, proceed with copy_to_user? That's 88 bytes at least...
>
> Please see ETHTOOL_GSTATS usage in drivers/net/8139cp.c.
>
> > > - on SMP, pktStat can be updated while the copy progresses, see
> > > depca_rx().
> >
> > Should I place these pktStat updates under lp->lock?
>
> You may.
It's already under the lock: depca_rx is called from depca_interrupt
which acqures the lock.
New patch attached. Testers with hw wanted ;-)
--
vda
diff -u --recursive linux-2.5.40org/drivers/net/depca.c linux-2.5.40/drivers/net/depca.c
--- linux-2.5.40org/drivers/net/depca.c Tue Oct 1 05:07:01 2002
+++ linux-2.5.40/drivers/net/depca.c Thu Oct 3 10:52:09 2002
@@ -951,6 +951,7 @@
}
+/* Called with lp->lock held */
static int
depca_rx(struct net_device *dev)
{
@@ -1052,6 +1053,7 @@
/*
** Buffer sent - check for buffer errors.
+** Called with lp->lock held
*/
static int
depca_tx(struct net_device *dev)
@@ -1910,6 +1912,7 @@
struct depca_ioctl *ioc = (struct depca_ioctl *) &rq->ifr_data;
int i, status = 0;
u_long ioaddr = dev->base_addr;
+ unsigned long flags;
union {
u8 addr[(HASH_TABLE_LEN * ETH_ALEN)];
u16 sval[(HASH_TABLE_LEN * ETH_ALEN) >> 1];
@@ -1998,19 +2001,27 @@
set_multicast_list(dev);
break;
- case DEPCA_GET_STATS: /* Get the driver statistics */
- cli();
+ case DEPCA_GET_STATS: { /* Get the driver statistics */
+ typeof(lp->pktStats) *tmp_stats =
+ kmalloc(sizeof(lp->pktStats), GFP_KERNEL);
+ if (!tmp_stats) return -ENOMEM;
+
+ spin_lock_irqsave(&lp->lock, flags);
+ memcpy(tmp_stats, &lp->pktStats, sizeof(lp->pktStats));
+ spin_unlock_irqrestore(&lp->lock, flags);
+
ioc->len = sizeof(lp->pktStats);
- if (copy_to_user(ioc->data, &lp->pktStats, ioc->len))
+ if (copy_to_user(ioc->data, tmp_stats, sizeof(lp->pktStats)))
status = -EFAULT;
- sti();
+ kfree(tmp_stats);
break;
+ }
case DEPCA_CLR_STATS: /* Zero out the driver statistics */
if (!capable(CAP_NET_ADMIN)) return -EPERM;
- cli();
+ spin_lock_irqsave(&lp->lock, flags);
memset(&lp->pktStats, 0, sizeof(lp->pktStats));
- sti();
+ spin_unlock_irqrestore(&lp->lock, flags);
break;
case DEPCA_GET_REG: /* Get the DEPCA Registers */
Denis Vlasenko <[email protected]> :
[...]
> > depca_rx() looks strange:
> > buf = skb_put(skb, len);
> > [...]
> > netif_rx(skb);
> > [...]
> > if (buf[0] & ...)
>
> I'd say this network stuff is a bit cryptic for untrained eye :-)
> What's strange with that code?
One shouldn't assume the buffer is available once it was passed to
netif_rx().
--
Ueimor