Re: [PATCH] cli()/sti() fix for drivers/net/depca.c

Denis Vlasenko (vda@port.imtp.ilyichevsk.odessa.ua)
Thu, 3 Oct 2002 10:55:23 -0200


On 2 October 2002 20:12, Francois Romieu wrote:
> Denis Vlasenko <vda@port.imtp.ilyichevsk.odessa.ua> :
> [...]
>
> > 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 */ - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/