Re: Bad idea -- was Re: PATCH 2.3.99.1.4: net-init alignment fix

From: Jeff Garzik (jgarzik@mandrakesoft.com)
Date: Sun Mar 19 2000 - 12:24:10 EST


Donald Becker wrote:
> The old method was to put the padding at the end of the private structure with
> code like
> int pad0, pad1; /* Pad for alignment. */

If you are doing this you don't understand how gcc works. There are
other ways.

> The pci-skeleton driver and many others use
>
> { /* Make certain elements e.g. descriptor lists are aligned. */
> void *mem = kmalloc(sizeof(*np) + PRIV_ALIGN, GFP_KERNEL);
> if ( ! mem) {
> ....
> }
> dev->priv = np = (void *)(((long)mem + PRIV_ALIGN) & ~PRIV_ALIGN);
> memset(np, 0, sizeof(*np));
> np->priv_addr = mem;
> }

Now init_etherdev() does this for you, thankfully. makes drivers easier
to maintain, LESS prone to error, and more clean.

> This was a bug in some of the drivers, but has since been fixed. Well, at
> least in my versions, which have been explicitly rejected because they
> contain backwards-compatibility code..

Paraphrasing from a past thread: "...since they were rejected, I
removed back compat code as requested, and they were still rejected."
Pick your argument, please. :) This is the nature of Linux kernel and
iterative development. Patches get rejected. You don't take it
personally, you work with Linus and others to resolve the issues.

As I have personally pointed out to you several times, the correct way
to do backward compat is have crud like the following in a header
somewhere:

#if old-kernel-version
#define new-kernel-feature { compatibility code }
#endif

That way your code works verbatim on new drivers, and simply requires a
compatibility header and module to work under older kernels.

Take a look at the attachment below: kcompat24.h (as modified by Jakub
Jelinek(sp?)), and kcompat24.c. This illustrate the, IMNSHO, correct
approach to backwards compatibility. It leaves the native code CLEAN
and READABLE, which does not describe at all your drivers before
backward compat crud was removed.

        Jeff

-- 
Jeff Garzik              | My to-do list is a function
Building 1024            | which approaches infinity.
MandrakeSoft, Inc.       |

attached mail follows:


> I agree heartily ! See attachment, which is my attempt. Note that the > code has a bug I haven't found yet. > > I do not think Linus would accept such a header in the current kernel > source tree, because it would simply be a duplication of code elsewhere > in the tree. However, I do think there is a need for a common kernel > compatibility module, which allows near-seamless porting of 2.4.0 > drivers to 2.2.x and 2.0.x kernels. dhinds and Donald Becker both > appear to have similar compatibility code written -- but for opposite > purpose -- keeping old code working with new kernels, instead of the > (IMHO better) method of keeping new drivers working with older kernels.

Agreed, but IMHO such header/.c file combo has to be done properly, that means one should go and study the exact kernel revision when a particular interface change happened etc., go up to say 2.0.0 because people are still running 2.0.x kernels from time to time and maintainers would be whinning if that compat stuff goes away. But maybe it would be good to come with these to l-k and ask people for suggestions on what has to be added/#ifdefed etc., then afterwards we should test a few drivers in 2.3.4x, 2.2.15, 2.0.xx.

> > The very last hunk of the patch adds a naked call to > pci-free-consistent. But maybe that was covered by a compatibility > ifdef I missed...

All these are defined at the beginning of the file if kernel < 2.3.47.

Here is a little bit updated kcompat24.h header, but IMHO it still has way to go before it can be actually usable. I have just added dynamic DMA stuff defines and modified your pci_resource_start/end definitions because base_address[] used to include the bottom bits which have to be masked out.

#ifndef __KCOMPAT24_H__ #define __KCOMPAT24_H__

#include <linux/version.h>

#if LINUX_VERSION_CODE < KERNEL_VERSION(2,3,0)

#include <asm/spinlock.h>

#ifdef KCOMPAT_INCLUDED #define KCOMPINC static #else #define KCOMPINC #endif

#define net_device device

#define __exit #define __exitdata #define __devinit #define __devinitdata #define __devexit #define __devexitdata

/* Not sure what version aliases were introduced in, but certainly in 2.91.66. */ #ifdef MODULE #if __GNUC__ > 2 || (__GNUC__ == 2 && __GNUC_MINOR__ >= 91) #define module_init(x) int init_module(void) __attribute__((alias(#x))); #define module_exit(x) void cleanup_module(void) __attribute__((alias(#x))); #else #define module_init(x) int init_module(void) { return x(); } #define module_exit(x) void cleanup_module(void) { x(); } #endif #else #define module_init(x) #define module_exit(x) #endif

#define MODULE_DEVICE_TABLE(foo,bar)

/* * Insert a new entry before the specified head.. */ static __inline__ void list_add_tail(struct list_head *new, struct list_head *head) { __list_add(new, head->prev, head); }

#define IORESOURCE_IO 0x00000100 /* Resource type */ #define IORESOURCE_MEM 0x00000200

#define request_region compat_request_region #define request_mem_region compat_request_region #define release_mem_region release_region

/* New-style probing supporting hot-pluggable devices */

#define PCI_PM_CTRL 4 /* PM control and status register */ #define PCI_PM_CTRL_STATE_MASK 0x0003 /* Current power state (D0 to D3) */

#define PCI_ANY_ID (~0)

#define PCI_GET_DRIVER_DATA pci_compat_get_driver_data #define PCI_SET_DRIVER_DATA pci_compat_set_driver_data

#define pci_enable_device pci_compat_enable_device #define pci_register_driver pci_compat_register_driver #define pci_unregister_driver pci_compat_unregister_driver

#define pci_dev_g(n) list_entry(n, struct pci_dev, global_list) #define pci_dev_b(n) list_entry(n, struct pci_dev, bus_list)

#define pci_for_each_dev(dev) \ for(dev = pci_devices; dev; dev = dev->next)

#define pci_resource_start(dev,bar) \ (((dev)->base_address[(bar)] & PCI_BASE_ADDRESS_SPACE) ? \ ((dev)->base_address[(bar)] & PCI_BASE_ADDRESS_IO_MASK) : \ ((dev)->base_address[(bar)] & PCI_BASE_ADDRESS_MEM_MASK)) #define pci_resource_end(dev,bar) \ (pci_resource_start(dev,bar) + pci_compat_get_size((dev),(bar))) #define pci_resource_flags(dev,bar) (pci_compat_get_flags((dev),(bar)))

struct pci_device_id { unsigned int vendor, device; /* Vendor and device ID or PCI_ANY_ID */ unsigned int subvendor, subdevice; /* Subsystem ID's or PCI_ANY_ID */ unsigned int class, class_mask; /* (class,subclass,prog-if) triplet */ unsigned long driver_data; /* Data private to the driver */ };

struct pci_driver { struct list_head node; struct pci_dev *dev; char *name; const struct pci_device_id *id_table; /* NULL if wants all devices */ int (*probe)(struct pci_dev *dev, const struct pci_device_id *id); /* New device inserted */ void (*remove)(struct pci_dev *dev); /* Device removed (NULL if not a hot-plug capable driver) */ void (*suspend)(struct pci_dev *dev); /* Device suspended */ void (*resume)(struct pci_dev *dev); /* Device woken up */ };

/* * */ KCOMPINC const struct pci_device_id * pci_compat_match_device(const struct pci_device_id *ids, struct pci_dev *dev); KCOMPINC int pci_compat_register_driver(struct pci_driver *drv); KCOMPINC void pci_compat_unregister_driver(struct pci_driver *drv); KCOMPINC unsigned long pci_compat_get_size (struct pci_dev *dev, int n_base); KCOMPINC int pci_compat_get_flags (struct pci_dev *dev, int n_base); KCOMPINC int pci_compat_set_power_state(struct pci_dev *dev, int new_state); KCOMPINC int pci_compat_enable_device(struct pci_dev *dev); KCOMPINC int pci_compat_find_capability(struct pci_dev *dev, int cap); KCOMPINC void *compat_request_region (unsigned long start, unsigned long n, const char *name); KCOMPINC void * pci_compat_get_driver_data (struct pci_dev *dev); KCOMPINC void pci_compat_set_driver_data (struct pci_dev *dev, void *driver_data);

#else /* if kernel version >= 2.3.0 */

#include <linux/spinlock.h>

#ifndef pci_resource_start #define pci_resource_start(dev,bar) ((dev)->resource[(bar)].start) #define pci_resource_end(dev,bar) ((dev)->resource[(bar)].end) #define pci_resource_flags(dev,bar) ((dev)->resource[(bar)].flags) #endif /* !pci_resource_start */

#ifndef PCI_GET_DRIVER_DATA #define PCI_GET_DRIVER_DATA(pdev) ((pdev)->driver_data) #define PCI_SET_DRIVER_DATA(pdev,data) (((pdev)->driver_data) = (data)) #endif /* PCI_GET_DRIVER_DATA */

#endif /* kernel version <=> 2.3.0 */

#if LINUX_VERSION_CODE < KERNEL_VERSION(2,3,47) typedef __u32 dma_addr_t;

/* Pure 2^n version of get_order */ extern __inline__ int __compat_get_order(unsigned long size) { int order;

size = (size-1) >> (PAGE_SHIFT-1); order = -1; do { size >>= 1; order++; } while (size); return order; }

extern __inline__ void * pci_alloc_consistent(struct pci_dev *hwdev, size_t size, dma_addr_t *dma_handle) { void *ret; int gfp = GFP_ATOMIC;

if (hwdev == NULL) gfp |= GFP_DMA; ret = (void *)__get_free_pages(gfp, __compat_get_order(size));

if (ret != NULL) { memset(ret, 0, size); *dma_handle = virt_to_bus(ret); } return ret; }

extern __inline__ void pci_free_consistent(struct pci_dev *hwdev, size_t size, void *vaddr, dma_addr_t dma_handle) { free_pages((unsigned long)vaddr, __compat_get_order(size)); }

#define pci_unmap_single(cookie, address, size, dir) #define pci_map_sg(cookie, sg, nents, dir) (nents) #define pci_unmap_sg(cookie, sg, nents, dir) #define sg_dma_len(slp) ((slp)->length) #define pci_map_single(cookie, address, size, dir) virt_to_bus(address) #define sg_dma_address(slp) virt_to_bus((slp)->address) #define PCI_DMA_BIDIRECTIONAL 0 #define PCI_DMA_TODEVICE 1 #define PCI_DMA_FROMDEVICE 2 #define PCI_DMA_NONE 3 #define scsi_to_pci_dma_dir(dir) PCI_DMA_BIDIRECTIONAL #endif /* version < v2.3.47 */

#ifdef KCOMPAT_INCLUDED #include "kcompat24.c" #endif

#endif /* __KCOMPAT24_H__ */

Cheers, Jakub ___________________________________________________________________ Jakub Jelinek | jakub@redhat.com | http://sunsite.mff.cuni.cz/~jj Linux version 2.3.47 on a sparc64 machine (1343.49 BogoMips) ___________________________________________________________________

/* * $Id: kcompat24.c,v 1.5 2000/02/08 17:52:13 jgarzik Exp $ * * PCI Bus Services, see include/linux/pci.h for further explanation. * * Copyright 1993 -- 1997 Drew Eckhardt, Frederic Potter, * David Mosberger-Tang * * Copyright 1997 -- 2000 Martin Mares <mj@suse.cz> */

#include <linux/version.h>

#if LINUX_VERSION_CODE < KERNEL_VERSION(2,3,0)

#ifndef KCOMPAT_INCLUDED #include <linux/module.h> #endif

#include <linux/kernel.h> #include <linux/list.h> #include <linux/pci.h> #include <linux/ioport.h> #include "kcompat24.h"

/* * Registration of PCI drivers and handling of hot-pluggable devices. */

static LIST_HEAD(pci_drivers);

struct pci_driver_mapping { struct pci_dev *dev; struct pci_driver *drv; void *driver_data; };

#define PCI_MAX_MAPPINGS 64 static struct pci_driver_mapping drvmap [PCI_MAX_MAPPINGS] = { { NULL, } , };

KCOMPINC void * pci_compat_get_driver_data (struct pci_dev *dev) { int i; for (i = 0; i < PCI_MAX_MAPPINGS; i++) if (drvmap[i].dev == dev) return drvmap[i].driver_data; return NULL; }

KCOMPINC void pci_compat_set_driver_data (struct pci_dev *dev, void *driver_data) { int i; for (i = 0; i < PCI_MAX_MAPPINGS; i++) if (drvmap[i].dev == dev) { drvmap[i].driver_data = driver_data; return; } }

KCOMPINC const struct pci_device_id * pci_compat_match_device(const struct pci_device_id *ids, struct pci_dev *dev) { u16 subsystem_vendor, subsystem_device;

pci_read_config_word(dev, PCI_SUBSYSTEM_VENDOR_ID, &subsystem_vendor); pci_read_config_word(dev, PCI_SUBSYSTEM_ID, &subsystem_device);

while (ids->vendor || ids->subvendor || ids->class_mask) { if ((ids->vendor == PCI_ANY_ID || ids->vendor == dev->vendor) && (ids->device == PCI_ANY_ID || ids->device == dev->device) && (ids->subvendor == PCI_ANY_ID || ids->subvendor == subsystem_vendor) && (ids->subdevice == PCI_ANY_ID || ids->subdevice == subsystem_device) && !((ids->class ^ dev->class) & ids->class_mask)) return ids; ids++; } return NULL; }

static int pci_announce_device(struct pci_driver *drv, struct pci_dev *dev) { const struct pci_device_id *id;

if (drv->id_table) { id = pci_compat_match_device(drv->id_table, dev); if (!id) return 0; } else id = NULL; if (drv->probe(dev, id) >= 0) { int i; for (i = 0; i < PCI_MAX_MAPPINGS; i++) { if (!drvmap[i].dev) { drvmap[i].dev = dev; drvmap[i].drv = drv; return 1; } } } return 0; }

KCOMPINC int pci_compat_register_driver(struct pci_driver *drv) { struct pci_dev *dev; int count = 0, found, i;

list_add_tail(&drv->node, &pci_drivers); pci_for_each_dev(dev) { found = 0; for (i = 0; i < PCI_MAX_MAPPINGS && !found; i++) if (drvmap[i].dev == dev) found = 1; if (!found) count += pci_announce_device(drv, dev); } return count; }

KCOMPINC void pci_compat_unregister_driver(struct pci_driver *drv) { struct pci_dev *dev; int i, found;

list_del(&drv->node); pci_for_each_dev(dev) { found = 0; for (i = 0; i < PCI_MAX_MAPPINGS && !found; i++) if (drvmap[i].dev == dev) found = 1; if (found) { if (drv->remove) drv->remove(dev); drvmap[i].dev = NULL; } } }

KCOMPINC unsigned long pci_compat_get_size (struct pci_dev *dev, int n_base) { u32 l, sz; int reg = PCI_BASE_ADDRESS_0 + (n_base << 2);

pci_read_config_dword (dev, reg, &l); if (l == 0xffffffff) return 0;

pci_write_config_dword (dev, reg, ~0); pci_read_config_dword (dev, reg, &sz); pci_write_config_dword (dev, reg, l);

if (!sz || sz == 0xffffffff) return 0; if ((l & PCI_BASE_ADDRESS_SPACE) == PCI_BASE_ADDRESS_SPACE_MEMORY) { sz = ~(sz & PCI_BASE_ADDRESS_MEM_MASK); } else { sz = ~(sz & PCI_BASE_ADDRESS_IO_MASK) & 0xffff; } return sz; }

KCOMPINC int pci_compat_get_flags (struct pci_dev *dev, int n_base) { unsigned long foo = dev->base_address[n_base] & PCI_BASE_ADDRESS_SPACE; int flags = 0; if (foo == 0) flags |= IORESOURCE_MEM; if (foo == 1) flags |= IORESOURCE_IO; return flags; }

/* * Set power management state of a device. For transitions from state D3 * it isn't as straightforward as one could assume since many devices forget * their configuration space during wakeup. Returns old power state. */ KCOMPINC int pci_compat_set_power_state(struct pci_dev *dev, int new_state) { u32 base[5], romaddr; u16 pci_command, pwr_command; u8 pci_latency, pci_cacheline; int i, old_state; int pm = pci_compat_find_capability(dev, PCI_CAP_ID_PM);

if (!pm) return 0; pci_read_config_word(dev, pm + PCI_PM_CTRL, &pwr_command); old_state = pwr_command & PCI_PM_CTRL_STATE_MASK; if (old_state == new_state) return old_state; if (old_state == 3) { pci_read_config_word(dev, PCI_COMMAND, &pci_command); pci_write_config_word(dev, PCI_COMMAND, pci_command & ~(PCI_COMMAND_IO | PCI_COMMAND_MEMORY)); for (i = 0; i < 5; i++) pci_read_config_dword(dev, PCI_BASE_ADDRESS_0 + i*4, &base[i]); pci_read_config_dword(dev, PCI_ROM_ADDRESS, &romaddr); pci_read_config_byte(dev, PCI_LATENCY_TIMER, &pci_latency); pci_read_config_byte(dev, PCI_CACHE_LINE_SIZE, &pci_cacheline); pci_write_config_word(dev, pm + PCI_PM_CTRL, new_state); for (i = 0; i < 5; i++) pci_write_config_dword(dev, PCI_BASE_ADDRESS_0 + i*4, base[i]); pci_write_config_dword(dev, PCI_ROM_ADDRESS, romaddr); pci_write_config_byte(dev, PCI_INTERRUPT_LINE, dev->irq); pci_write_config_byte(dev, PCI_CACHE_LINE_SIZE, pci_cacheline); pci_write_config_byte(dev, PCI_LATENCY_TIMER, pci_latency); pci_write_config_word(dev, PCI_COMMAND, pci_command); } else pci_write_config_word(dev, pm + PCI_PM_CTRL, (pwr_command & ~PCI_PM_CTRL_STATE_MASK) | new_state); return old_state; }

/* * Initialize device before it's used by a driver. Ask low-level code * to enable I/O and memory. Wake up the device if it was suspended. * Beware, this function can fail. */ KCOMPINC int pci_compat_enable_device(struct pci_dev *dev) { #if 0 pci_compat_set_power_state(dev, 0); #endif return 0; }

KCOMPINC int pci_compat_find_capability(struct pci_dev *dev, int cap) { u16 status; u8 pos, id; int ttl = 48;

pci_read_config_word(dev, PCI_STATUS, &status); if (!(status & PCI_STATUS_CAP_LIST)) return 0; pci_read_config_byte(dev, PCI_CAPABILITY_LIST, &pos); while (ttl-- && pos >= 0x40) { pos &= ~3; pci_read_config_byte(dev, pos + PCI_CAP_LIST_ID, &id); if (id == 0xff) break; if (id == cap) return pos; pci_read_config_byte(dev, pos + PCI_CAP_LIST_NEXT, &pos); } return 0; }

/* note - assumes you only test for NULL, and not * actually care about the return value */ KCOMPINC void *compat_request_region (unsigned long start, unsigned long n, const char *name) { if (check_region (start, n) != 0) return NULL; request_region (start, n, name); return (void *) 1; }

#ifndef KCOMPAT_INCLUDED EXPORT_SYMBOL(pci_compat_match_device); EXPORT_SYMBOL(pci_compat_register_driver); EXPORT_SYMBOL(pci_compat_unregister_driver); EXPORT_SYMBOL(pci_compat_get_size); EXPORT_SYMBOL(pci_compat_get_flags); EXPORT_SYMBOL(pci_compat_set_power_state); EXPORT_SYMBOL(pci_compat_enable_device); EXPORT_SYMBOL(pci_compat_find_capability); EXPORT_SYMBOL(pci_compat_get_driver_data); EXPORT_SYMBOL(pci_compat_set_driver_data); #endif

#endif /* kernel version < 2.3.0 */

- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.rutgers.edu Please read the FAQ at http://www.tux.org/lkml/



This archive was generated by hypermail 2b29 : Thu Mar 23 2000 - 21:00:26 EST