double-check me?

Maybe you guys can spot something I'm missing here.  alloc_netdev is 
supposed to guarantee that dev->priv is aligned by 32 bytes:

struct net_device *alloc_netdev(int sizeof_priv, const char *mask,
                                        void (*setup)(struct net_device *))
{
         struct net_device *dev;
         int alloc_size;

         /* ensure 32-byte alignment of the private area */
         alloc_size = sizeof (*dev) + sizeof_priv + 31;

         dev = (struct net_device *) kmalloc (alloc_size, GFP_KERNEL);
         if (dev == NULL)
         {
                 printk(KERN_ERR "alloc_dev: Unable to allocate device 
memory.\n"
);
                 return NULL;
         }

         memset(dev, 0, alloc_size);

         if (sizeof_priv)
                 dev->priv = (void *) (((long)(dev + 1) + 31) & ~31);


Now... shouldn't that last line of code be "dev + 1 + sizeof(*dev)" ?

It seems to work 2.[456] for a long time, so I am doubting myself... 
surely it would have caused memory corruption or something by now if I 
have really found a bug.

	Jeff

Jeff Garzik [off-list ref] writes:

Maybe you guys can spot something I'm missing here.  alloc_netdev is
supposed to guarantee that dev->priv is aligned by 32 bytes:

         struct net_device *dev;

         if (sizeof_priv)
                 dev->priv = (void *) (((long)(dev + 1) + 31) & ~31);


Now... shouldn't that last line of code be "dev + 1 + sizeof(*dev)" ?

It seems to work 2.[456] for a long time, so I am doubting
myself... surely it would have caused memory corruption or something
by now if I have really found a bug.

Looks ok... not very readable, though.

dev + 1 = ((u8*)dev) + sizeof(dev) = pointer to end of net_device struct.

(X + 31) & ~31 makes sure X is 32-bytes aligned (0-31 bytes are added).
Hope the alloc_size has enough space for this.

31 should be better 0x1F I think.

I also like sizeof(struct net_device) more than sizeof(*dev).

Why do you want "+ 1" if you add sizeof(*dev)?
-- 
Krzysztof Halasa
Network Administrator

jgarzik@pobox.com said:

         if (sizeof_priv)
                 dev->priv = (void *) (((long)(dev + 1) + 31) & ~31);


Now... shouldn't that last line of code be "dev + 1 + sizeof(*dev)" ?

are you missing that the "dev + 1" pointer arithmetic is already adding
sizeof(*dev) to dev, rather than just 1 byte? "(dev + 1)" is a pointer
to the private area after the actual struct net_device, and
"((long)(dev + 1) + 31)" adds 31 bytes of padding. The final "& ~31"
chops off any excess padding from the 31 that was added and actually
aligns the pointer.

Seems right to me, but I'm not used to playing alignment tricks.

Jason

Krzysztof Halasa wrote:

Jeff Garzik [off-list ref] writes:

quoted

Maybe you guys can spot something I'm missing here.  alloc_netdev is
supposed to guarantee that dev->priv is aligned by 32 bytes:

        struct net_device *dev;

        if (sizeof_priv)
                dev->priv = (void *) (((long)(dev + 1) + 31) & ~31);


Now... shouldn't that last line of code be "dev + 1 + sizeof(*dev)" ?

It seems to work 2.[456] for a long time, so I am doubting
myself... surely it would have caused memory corruption or something
by now if I have really found a bug.

Looks ok... not very readable, though.

dev + 1 = ((u8*)dev) + sizeof(dev) = pointer to end of net_device struct.

C pointer arithmatic.  Ok, duh, thanks :)

	Jeff, too used to void pointer arith

jgarzik@pobox.com said:

C pointer arithmatic.  Ok, duh, thanks :)
	Jeff, too used to void pointer arith

Don't get _too_ used to it. arithmetic on (void *) is a gcc extension.
it's undefined in ansi C.

Jason

On Mon, Aug 18, 2003 at 02:38:50PM +0000, Jason Lunz wrote:

jgarzik@pobox.com said:

quoted

C pointer arithmatic.  Ok, duh, thanks :)
	Jeff, too used to void pointer arith

Don't get _too_ used to it. arithmetic on (void *) is a gcc extension.
it's undefined in ansi C.

Yes, I know.

It's used extensively in the kernel, however.

	Jeff