Re: PATCH: straighten out the IDE layer locking and add hotplug
From: Bartlomiej Zolnierkiewicz
Date: Tue Aug 17 2004 - 08:19:42 EST
more comments, please read
On Sunday 15 August 2004 17:13, Alan Cox wrote:
> There really isnt any sane way to break this patch down because all the
> changes are interlinked so closely.
it turns out that it _really_ must be splitted out on invidual changes,
this is possible and needed because changes are fairly complicated
> We now
> - enforce and document an actual lock order
> - use a seperate semaphore to get sleep-read/spin-write locking for
> the lists that need it
> - Fix the driver list locking
> - Fix the hwif list locking
> - Remove the impossible to fix and unused stuff for passing
> a device between drivers
> - Change ide_unregister into ide_unregister_hwif as it now takes
> a hwif pointer
> - Fix various cases we wrongly dropped locks
> - Split configured/present so that we can do unregister right
> - Call ->remove() callbacks
>
> This patch leaves pci and ide-cs broken, patches to move them to the new
> locking follow
>
>
> diff -u --new-file --recursive --exclude-from /usr/src/exclude
> linux.vanilla-2.6.8-rc3/drivers/ide/ide.c linux-2.6.8-rc3/drivers/ide/ide.c
> --- linux.vanilla-2.6.8-rc3/drivers/ide/ide.c 2004-08-09 15:51:00.000000000
> +0100 +++ linux-2.6.8-rc3/drivers/ide/ide.c 2004-08-12 17:55:05.000000000
> +0100 @@ -448,31 +515,46 @@
> return -ENXIO;
> }
>
> +/*
> + * drives_lock protects the list of drives, drivers lock the
> + * list of drivers. Currently nobody takes both at once.
> + * drivers_sem guards the drivers_list for readers that may
> + * sleep. It must be taken before drivers_lock. Take drivers_sem
> + * before ide_setting_sem and idecfg_sem before either of the
> + * others.
> + */
> +
> static spinlock_t drives_lock = SPIN_LOCK_UNLOCKED;
> +static DECLARE_MUTEX(drivers_sem);
Could you please explain why is it needed - what is it trying to fix?
> static spinlock_t drivers_lock = SPIN_LOCK_UNLOCKED;
> +
> static LIST_HEAD(drivers);
>
> -/* Iterator */
> +/* Iterator for the driver list. */
> +
> static void *m_start(struct seq_file *m, loff_t *pos)
> {
> struct list_head *p;
> loff_t l = *pos;
> - spin_lock(&drivers_lock);
> + down(&drivers_sem);
> list_for_each(p, &drivers)
> if (!l--)
> return list_entry(p, ide_driver_t, drivers);
> return NULL;
> }
> +
> static void *m_next(struct seq_file *m, void *v, loff_t *pos)
> {
> struct list_head *p = ((ide_driver_t *)v)->drivers.next;
> (*pos)++;
> return p==&drivers ? NULL : list_entry(p, ide_driver_t, drivers);
> }
> +
> static void m_stop(struct seq_file *m, void *v)
> {
> - spin_unlock(&drivers_lock);
> + up(&drivers_sem);
> }
> +
> static int show_driver(struct seq_file *m, void *v)
> {
> ide_driver_t *driver = v;
> @@ -515,6 +597,7 @@
> * MMIO leaves it to the controller driver,
> * PIO will migrate this way over time.
> */
> +
> int ide_hwif_request_regions(ide_hwif_t *hwif)
> {
> unsigned long addr;
> @@ -564,6 +647,7 @@
> * importantly our caller should be doing this so we need to
> * restructure this as a helper function for drivers.
> */
> +
> void ide_hwif_release_regions(ide_hwif_t *hwif)
> {
> u32 i = 0;
> @@ -581,7 +665,15 @@
> release_region(hwif->io_ports[i], 1);
> }
>
> -/* restore hwif to a sane state */
> +/**
> + * ide_hwif_restore - restore hwif to template
> + * @hwif: hwif to update
> + * @tmp_hwif: template
> + *
> + * Restore hwif to a default state by copying most settngs
> + * from the template.
> + */
> +
> static void ide_hwif_restore(ide_hwif_t *hwif, ide_hwif_t *tmp_hwif)
> {
> hwif->hwgroup = tmp_hwif->hwgroup;
> @@ -678,15 +771,16 @@
> }
>
> /**
> - * ide_unregister - free an ide interface
> - * @index: index of interface (will change soon to a pointer)
> + * __ide_unregister_hwif - free an ide interface
> + * @hwif: interface to unregister
> *
> * Perform the final unregister of an IDE interface. At the moment
> * we don't refcount interfaces so this will also get split up.
> *
> * Locking:
> - * The caller must not hold the IDE locks
> - * The drive present/vanishing is not yet properly locked
> + * The caller must not hold the IDE locks except for ide_cfg_sem
> + * which must be held.
> + *
> * Take care with the callbacks. These have been split to avoid
> * deadlocking the IDE layer. The shutdown callback is called
> * before we take the lock and free resources. It is up to the
> @@ -695,31 +789,43 @@
> * isnt yet done btw). After we commit to the final kill we
> * call the cleanup callback with the ide locks held.
> *
> + * An interface can be in four states we care about
> + * - It can be busy (drive or driver thinks its active). No unload
> + * - It can be unconfigured - which means its already gone
> + * - It can be configured and present - a full interface
> + * - It can be configured and not present - pci configured but no drives
> + * so logically absent.
> + *
> * Unregister restores the hwif structures to the default state.
> - * This is raving bonkers.
> */
>
> -void ide_unregister(unsigned int index)
> +int __ide_unregister_hwif(ide_hwif_t *hwif)
> {
> ide_drive_t *drive;
> - ide_hwif_t *hwif, *g, *tmp_hwif;
> + ide_hwif_t *g;
> ide_hwgroup_t *hwgroup;
> int irq_count = 0, unit, i;
> + int index;
> + static ide_hwif_t tmp_hwif; /* Serialized by locking */
> + int ret = 0;
> + int was_present;
>
> - BUG_ON(index >= MAX_HWIFS);
> -
> - tmp_hwif = kmalloc(sizeof(*tmp_hwif), GFP_KERNEL|__GFP_NOFAIL);
> - if (!tmp_hwif) {
> - printk(KERN_ERR "%s: unable to allocate memory\n", __FUNCTION__);
> - return;
> - }
> -
> + index = hwif->index;
> +
> BUG_ON(in_interrupt());
> BUG_ON(irqs_disabled());
> - down(&ide_cfg_sem);
> +
> + /* Make sure nobody sneaks in via the proc interface */
> + down(&ide_setting_sem);
> +
> + /* Now ensure nobody gets in for I/O while we clean up and
> + do the busy check. If busy is set then the device is still
> + open and we must stop */
> spin_lock_irq(&ide_lock);
> - hwif = &ide_hwifs[index];
> - if (!hwif->present)
> +
> + printk("Unregister %d\n", index);
> +
> + if (!hwif->configured)
> goto abort;
> for (unit = 0; unit < MAX_DRIVES; ++unit) {
> drive = &hwif->drives[unit];
> @@ -729,10 +835,18 @@
> goto abort;
> drive->dead = 1;
> }
> + /*
> + * Protect against new users. From this point the hwif
> + * is not present so cannot be opened by a new I/O source.
> + * This also invalidates key driven access from procfs
> + */
> +
> + was_present = hwif->present;
> hwif->present = 0;
>
> spin_unlock_irq(&ide_lock);
> -
> + up(&ide_setting_sem);
> +
> for (unit = 0; unit < MAX_DRIVES; ++unit) {
> drive = &hwif->drives[unit];
> if (!drive->present)
> @@ -743,27 +857,36 @@
> #ifdef CONFIG_PROC_FS
> destroy_proc_ide_drives(hwif);
> #endif
> -
> + spin_lock_irq(&ide_lock);
> +
> hwgroup = hwif->hwgroup;
> - /*
> - * free the irq if we were the only hwif using it
> - */
> - g = hwgroup->hwif;
> - do {
> - if (g->irq == hwif->irq)
> - ++irq_count;
> - g = g->next;
> - } while (g != hwgroup->hwif);
> - if (irq_count == 1)
> + if(hwgroup)
> + {
> + /*
> + * free the irq if we were the only hwif using it
> + */
> + g = hwgroup->hwif;
> + do {
> + if (g->irq == hwif->irq)
> + ++irq_count;
> + g = g->next;
> + } while (g != hwgroup->hwif);
> + }
> + spin_unlock_irq(&ide_lock);
new race(s) here because of not holding ide_cfg_sem anylonger,
see init_irq() in ide-probe.c and ide_cfg_sem comments in ide.h
some other driver may accessing this hwif->hwgroup without holding ide_lock
- probably will result in a crash few lines below when we try to free this
hwgroup
> + if (irq_count == 1 && hwgroup)
> free_irq(hwif->irq, hwgroup);
>
> - spin_lock_irq(&ide_lock);
> /*
> * Note that we only release the standard ports,
> * and do not even try to handle any extra ports
> * allocated for weird IDE interface chipsets.
> + *
> + * FIXME: should defer this I think
> */
> - ide_hwif_release_regions(hwif);
> +
> + if(was_present)
> + ide_hwif_release_regions(hwif);
>
> /*
> * Remove us from the hwgroup, and free
> @@ -777,6 +900,14 @@
> }
> if (!drive->present)
> continue;
> +
> + /*
> + * The hwgroup chain is IRQ touched. We must protect
> + * walking this from an IDE event for another device
> + * in the chain
> + */
> +
> + spin_lock_irq(&ide_lock);
> if (drive == drive->next) {
> /* special case: last drive from hwgroup. */
> BUG_ON(hwgroup->drive != drive);
> @@ -793,51 +924,73 @@
> hwgroup->hwif = HWIF(hwgroup->drive);
> }
> }
> + spin_unlock_irq(&ide_lock);
> +
> + /*
> + * The rest of the cleanup is private
> + */
> +
> BUG_ON(hwgroup->drive == drive);
> if (drive->id != NULL) {
> kfree(drive->id);
> drive->id = NULL;
> }
> drive->present = 0;
> - /* Messed up locking ... */
> - spin_unlock_irq(&ide_lock);
> blk_cleanup_queue(drive->queue);
> device_unregister(&drive->gendev);
> down(&drive->gendev_rel_sem);
> - spin_lock_irq(&ide_lock);
> drive->queue = NULL;
> }
> - if (hwif->next == hwif) {
> - BUG_ON(hwgroup->hwif != hwif);
> - kfree(hwgroup);
> - } else {
> - /* There is another interface in hwgroup.
> - * Unlink us, and set hwgroup->drive and ->hwif to
> - * something sane.
> - */
> - g = hwgroup->hwif;
> - while (g->next != hwif)
> - g = g->next;
> - g->next = hwif->next;
> - if (hwgroup->hwif == hwif) {
> - /* Chose a random hwif for hwgroup->hwif.
> - * It's guaranteed that there are no drives
> - * left in the hwgroup.
> +
> + /*
> + * Lock against hwgroup walkers including interrupts off other
> + * IDE devices wile we unhook ourselves.
> + */
> +
> + spin_lock_irq(&ide_lock);
> +
> + if (hwgroup)
> + {
> + if (hwif->next == hwif) {
> + BUG_ON(hwgroup->hwif != hwif);
> + kfree(hwgroup);
> + } else {
> + /* There is another interface in hwgroup.
> + * Unlink us, and set hwgroup->drive and ->hwif to
> + * something sane.
> */
> - BUG_ON(hwgroup->drive != NULL);
> - hwgroup->hwif = g;
> + g = hwgroup->hwif;
> + while (g->next != hwif)
> + g = g->next;
> + g->next = hwif->next;
> + if (hwgroup->hwif == hwif) {
> + /* Chose a random hwif for hwgroup->hwif.
> + * It's guaranteed that there are no drives
> + * left in the hwgroup.
> + */
> + BUG_ON(hwgroup->drive != NULL);
> + hwgroup->hwif = g;
> + }
> + BUG_ON(hwgroup->hwif == hwif);
> }
> - BUG_ON(hwgroup->hwif == hwif);
> }
> -
> - /* More messed up locking ... */
> spin_unlock_irq(&ide_lock);
> - device_unregister(&hwif->gendev);
> - down(&hwif->gendev_rel_sem);
> +
> + /*
> + * PCI interfaces with no devices don't exist in the device
> + * tree so don't unregister them.
> + */
> +
> + if(was_present)
> + {
> + device_unregister(&hwif->gendev);
> + down(&hwif->gendev_rel_sem);
> + }
>
> /*
> * Remove us from the kernel's knowledge
> */
> +
> blk_unregister_region(MKDEV(hwif->major, 0), MAX_DRIVES<<PARTN_BITS);
> for (i = 0; i < MAX_DRIVES; i++) {
> struct gendisk *disk = hwif->drives[i].disk;
> @@ -845,8 +998,16 @@
> put_disk(disk);
> }
> unregister_blkdev(hwif->major, hwif->name);
> +
> spin_lock_irq(&ide_lock);
>
> + /*
> + * Let the driver free up private objects
> + */
> +
> + if(hwif->remove)
> + hwif->remove(hwif);
> +
> if (hwif->dma_base) {
> (void) ide_release_dma(hwif);
>
> @@ -858,24 +1019,70 @@
> hwif->dma_vendor3 = 0;
> hwif->dma_prdtable = 0;
> }
> +
> + hwif->chipset = ide_unknown;
this breaks (half-working) HDIO_SCAN_HWIF ioctl
and can change ordering of IDE devices in some situations
- many host drivers look at hwif->chipset during init
> /* copy original settings */
> - *tmp_hwif = *hwif;
> + tmp_hwif = *hwif;
>
> /* restore hwif data to pristine status */
> init_hwif_data(hwif, index);
> init_hwif_default(hwif, index);
> +
> + hwif->key++;
> + hwif->configured = 0;
hwif->key is not restored in ide_hwif_restore() so it will be
always == 1 for once unregistered hwifs due to init_hwif_data()
Have you tested 'hwif->key' infrastructure?
> - ide_hwif_restore(hwif, tmp_hwif);
> + ide_hwif_restore(hwif, &tmp_hwif);
> +
> + spin_unlock_irq(&ide_lock);
> + return 0;
>
> abort:
> + if(hwif->configured)
> + {
> + printk("Unregister %d fail %d %d\n", index, drive->usage,
> DRIVER(drive)->busy); + ret = -EBUSY;
> + }
> + else
> + {
> + printk("No such hwif!\n");
> + ret = -ENOENT;
> + }
> spin_unlock_irq(&ide_lock);
> - up(&ide_cfg_sem);
> + up(&ide_setting_sem);
> + return ret;
> +}
> +
> +EXPORT_SYMBOL_GPL(__ide_unregister_hwif);
>
> - kfree(tmp_hwif);
> +
> +/**
> + * ide_unregister_hwif - free an ide interface
> + * @hwif: interface to unregister
> + *
> + * Perform the final unregister of an IDE interface. At the moment
> + * we don't refcount interfaces so this will also get split up.
> + * Unregister restores the hwif structures to the default state.
> + *
> + * No locks should be held on entry. When an unregister must
> + * be done atomically with a register see __ide_unregister_hwif
> + * and hold the ide_cfg_sem yourself.
> + */
> +
> +int ide_unregister_hwif(ide_hwif_t *hwif)
> +{
> + int ret;
> +
> + /* This protects two things. Firstly it serializes the
> + shutdown sequence, secondly it protects us from
> + races while we are killing off a device */
> + down(&ide_cfg_sem);
> + ret = __ide_unregister_hwif(hwif);
> + up(&ide_cfg_sem);
> + return ret;
> }
>
> -EXPORT_SYMBOL(ide_unregister);
> +EXPORT_SYMBOL_GPL(ide_unregister_hwif);
>
>
> /**
> @@ -931,15 +1138,25 @@
> */
> }
>
> -/*
> - * Register an IDE interface, specifying exactly the registers etc
> - * Set init=1 iff calling before probes have taken place.
> +/**
> + * ide_register_hw - register IDE interface
> + * @hw: hardware registers
> + * @hwifp: pointer to returned hwif
> + *
> + * Register an IDE interface, specifying exactly the registers etc
> + * Set init=1 iff calling before probes have taken place. The
> + * ide_cfg_sem protects this against races.
> + *
> + * Returns -1 on error.
> */
> +
> int ide_register_hw (hw_regs_t *hw, ide_hwif_t **hwifp)
> {
> int index, retry = 1;
> ide_hwif_t *hwif;
>
> + down(&ide_cfg_sem);
> +
> do {
> for (index = 0; index < MAX_HWIFS; ++index) {
> hwif = &ide_hwifs[index];
> @@ -950,28 +1167,37 @@
> hwif = &ide_hwifs[index];
> if (hwif->hold)
> continue;
> - if ((!hwif->present && !hwif->mate && !initializing) ||
> + if ((!hwif->configured && !hwif->mate && !initializing) ||
> (!hwif->hw.io_ports[IDE_DATA_OFFSET] && initializing))
> goto found;
> }
> + /* FIXME- this check should die as should the retry loop */
> for (index = 0; index < MAX_HWIFS; index++)
> - ide_unregister(index);
> + {
> + hwif = &ide_hwifs[index];
> + __ide_unregister_hwif(hwif);
> + }
> } while (retry--);
> +
> + up(&ide_cfg_sem);
> return -1;
> found:
> - if (hwif->present)
> - ide_unregister(index);
> + /* FIXME: do we really need this case */
> + if (hwif->configured)
> + __ide_unregister_hwif(hwif);
> else if (!hwif->hold) {
> init_hwif_data(hwif, index);
> init_hwif_default(hwif, index);
> }
> - if (hwif->present)
> + if (hwif->configured)
> return -1;
> + hwif->configured = 1;
> memcpy(&hwif->hw, hw, sizeof(*hw));
> memcpy(hwif->io_ports, hwif->hw.io_ports, sizeof(hwif->hw.io_ports));
> hwif->irq = hw->irq;
> hwif->noprobe = 0;
> hwif->chipset = hw->chipset;
> + up(&ide_cfg_sem);
>
> if (!initializing) {
> probe_hwif_init(hwif);
> @@ -1008,21 +1234,21 @@
> * @set: setting
> *
> * Removes the setting named from the device if it is present.
> - * The function takes the settings_lock to protect against
> - * parallel changes. This function must not be called from IRQ
> - * context. Returns 0 on success or -1 on failure.
> + * This function must not be called from IRQ context. Returns 0
> + * on success or -1 on failure.
> *
> * BUGS: This code is seriously over-engineered. There is also
> * magic about how the driver specific features are setup. If
> * a driver is attached we assume the driver settings are auto
> * remove.
> + *
> + * The caller must hold settings_lock
> */
>
> int ide_add_setting (ide_drive_t *drive, const char *name, int rw, int
> read_ioctl, int write_ioctl, int data_type, int min, int max, int
> mul_factor, int div_factor, void *data, ide_procset_t *set) {
> ide_settings_t **p = (ide_settings_t **) &drive->settings, *setting =
> NULL;
>
> - down(&ide_setting_sem);
> while ((*p) && strcmp((*p)->name, name) < 0)
> p = &((*p)->next);
> if ((setting = kmalloc(sizeof(*setting), GFP_KERNEL)) == NULL)
> @@ -1046,10 +1272,8 @@
> if (drive->driver != &idedefault_driver)
> setting->auto_remove = 1;
> *p = setting;
> - up(&ide_setting_sem);
> return 0;
> abort:
> - up(&ide_setting_sem);
> if (setting)
> kfree(setting);
> return -1;
> @@ -1058,7 +1282,7 @@
> EXPORT_SYMBOL(ide_add_setting);
this breaks locking for IDE device drivers which also call ide_add_setting()
but they are not holding ide_setting_sem
> /**
> - * __ide_remove_setting - remove an ide setting option
> + * ide_remove_setting - remove an ide setting option
> * @drive: drive to use
> * @name: setting name
> *
> @@ -1066,7 +1290,7 @@
> * The caller must hold the setting semaphore.
> */
>
> -static void __ide_remove_setting (ide_drive_t *drive, char *name)
> +static void ide_remove_setting (ide_drive_t *drive, char *name)
> {
> ide_settings_t **p, *setting;
>
> @@ -1144,7 +1368,7 @@
> setting = drive->settings;
> while (setting) {
> if (setting->auto_remove) {
> - __ide_remove_setting(drive, setting->name);
> + ide_remove_setting(drive, setting->name);
> goto repeat;
> }
> setting = setting->next;
> @@ -1322,25 +1556,19 @@
> return err;
> }
>
> -int ide_atapi_to_scsi (ide_drive_t *drive, int arg)
> -{
> - if (drive->media == ide_disk) {
> - drive->scsi = 0;
> - return 0;
> - }
> -
> - if (DRIVER(drive)->cleanup(drive)) {
> - drive->scsi = 0;
> - return 0;
> - }
> -
> - drive->scsi = (u8) arg;
> - ata_attach(drive);
> - return 0;
> -}
>
> +/**
> + * ide_add_generic_settings - generic /proc settings
> + * @drive: drive being configured
> + *
> + * Add the generic parts of the system settings to the /proc files
> + * for this IDE device. The caller must not be holding the settings_sem
> + * .lock
> + */
> +
> void ide_add_generic_settings (ide_drive_t *drive)
> {
> + down(&ide_setting_sem);
> /*
> * drive setting name read/write access read ioctl write
> ioctl data type min max mul_factor div_factor data pointer set
> function */
> @@ -1353,10 +1581,17 @@
>
> ide_add_setting(drive, "init_speed", SETTING_RW, -1, -1, TYPE_BYT
>E, 0, 70, 1, 1, &drive->init_speed, NULL);
> ide_add_setting(drive, "current_speed", SETTING_RW, -1, -1, TYPE_BY
>TE, 0, 70, 1, 1, &drive->current_speed, set_xfer_rate);
> ide_add_setting(drive, "number", SETTING_RW, -1, -1, TYPE_BYTE, 0,
> 3, 1, 1, &drive->dn, NULL); - if (drive->media != ide_disk)
> - ide_add_setting(drive, "ide-scsi", SETTING_RW, -1, HDIO_SET_IDE_SC
>SI, TYPE_BYTE, 0, 1, 1, 1, &drive->scsi, ide_atapi_to_scsi); +
> + up(&ide_setting_sem);
> }
>
> +/**
> + * system_bus_clock - clock guess
> + *
> + * External version of the bus clock guess used by old old IDE drivers
> + * for things like VLB timings. Should not be used.
> + */
> +
> int system_bus_clock (void)
> {
> return((int) ((!system_bus_speed) ? ide_system_bus_speed() :
> system_bus_speed )); @@ -1364,52 +1599,42 @@
>
> EXPORT_SYMBOL(system_bus_clock);
>
> -/*
> - * Locking is badly broken here - since way back. That sucker is
> - * root-only, but that's not an excuse... The real question is what
> - * exclusion rules do we want here.
> +/**
> + * ata_attach - attach an ATA/ATAPI device
> + * @drive: drive to attach
> + *
> + * Takes a drive that is as yet not assigned to any midlayer IDE
> + * module and figures out which driver would like to own it. If
> + * nobody claims the driver then it is automatically attached
> + * to the default driver used for unclaimed objects.
> + *
> + * A return of zero indicates attachment to a driver, of one
> + * attachment to the default driver
> + *
> + * Takes the driver list lock and the ide_settings semaphore.
> */
> -int ide_replace_subdriver (ide_drive_t *drive, const char *driver)
> -{
> - if (!drive->present || drive->usage || drive->dead)
> - goto abort;
> - if (DRIVER(drive)->cleanup(drive))
> - goto abort;
> - strlcpy(drive->driver_req, driver, sizeof(drive->driver_req));
> - if (ata_attach(drive)) {
> - spin_lock(&drives_lock);
> - list_del_init(&drive->list);
> - spin_unlock(&drives_lock);
> - drive->driver_req[0] = 0;
> - ata_attach(drive);
> - } else {
> - drive->driver_req[0] = 0;
> - }
> - if (DRIVER(drive)!= &idedefault_driver && !strcmp(DRIVER(drive)->name,
> driver)) - return 0;
> -abort:
> - return 1;
> -}
> -
> +
> int ata_attach(ide_drive_t *drive)
> {
> struct list_head *p;
> - spin_lock(&drivers_lock);
> + down(&drivers_sem);
> + down(&ide_setting_sem);
> list_for_each(p, &drivers) {
> ide_driver_t *driver = list_entry(p, ide_driver_t, drivers);
> if (!try_module_get(driver->owner))
> continue;
> - spin_unlock(&drivers_lock);
> if (driver->attach(drive) == 0) {
> module_put(driver->owner);
> drive->gendev.driver = &driver->gen_driver;
> + up(&ide_setting_sem);
> + up(&drivers_sem);
> return 0;
> }
> - spin_lock(&drivers_lock);
> module_put(driver->owner);
> }
> drive->gendev.driver = &idedefault_driver.gen_driver;
> - spin_unlock(&drivers_lock);
> + up(&ide_setting_sem);
> + up(&drivers_sem);
> if(idedefault_driver.attach(drive) != 0)
> panic("ide: default attach failed");
> return 1;
> @@ -1549,9 +1774,11 @@
> }
> case HDIO_UNREGISTER_HWIF:
> if (!capable(CAP_SYS_RAWIO)) return -EACCES;
> - /* (arg > MAX_HWIFS) checked in function */
> - ide_unregister(arg);
> - return 0;
> + if(arg > MAX_HWIFS || arg < 0)
> + return -EINVAL;
> + if(ide_hwifs[arg].pci_dev)
> + return -EINVAL;
Why this change? It prohibits all IDE PCI drivers and ide-cs
from using HDIO_UNREGISTER_HWIF ioctl (which is half-working for IDE PCI
because next call to HDIO_SCAN_HWIF will clear hwif out due to hwif->hold ==
0 but it is not the case for ide-cs).
I hate HDIO_SCAN_HWIF and HDIO_UNREGISTER_HWIF and I still think we should
remove them - I waited with such changes for 2.7 but this plan failed becuase
there won't be 2.7 soon. :/
> + return ide_unregister_hwif(&ide_hwifs[arg]);
> case HDIO_SET_NICE:
> if (!capable(CAP_SYS_ADMIN)) return -EACCES;
> if (arg != (arg & ((1 << IDE_NICE_DSC_OVERLAP) | (1 << IDE_NICE_1))))
> @@ -2199,9 +2426,23 @@
>
> EXPORT_SYMBOL(ide_register_subdriver);
>
> +/**
> + * ide_unregister_subdriver - disconnect drive from driver
> + * @drive: drive to unplug
> + *
> + * Disconnect a drive from the driver it was attached to and then
> + * clean up the various proc files and other objects attached to it.
> + * Takes ide_sem, ide_lock, and drive_lock. Caller must hold none of
> + * the locks.
> + *
> + * No locking versus subdriver unload because we are moving to the
> + * default driver anyway. Wants double checking.
> + */
> +
> int ide_unregister_subdriver (ide_drive_t *drive)
> {
> unsigned long flags;
> + ide_proc_entry_t *dir;
>
> down(&ide_setting_sem);
> spin_lock_irqsave(&ide_lock, flags);
> @@ -2210,13 +2451,14 @@
> up(&ide_setting_sem);
> return 1;
> }
> + dir = DRIVER(drive)->proc;
> + drive->driver = &idedefault_driver;
> + spin_unlock_irqrestore(&ide_lock, flags);
> #ifdef CONFIG_PROC_FS
> - ide_remove_proc_entries(drive->proc, DRIVER(drive)->proc);
> + ide_remove_proc_entries(drive->proc, dir);
> ide_remove_proc_entries(drive->proc, generic_subdriver_entries);
> #endif
> auto_remove_settings(drive);
> - drive->driver = &idedefault_driver;
> - spin_unlock_irqrestore(&ide_lock, flags);
> up(&ide_setting_sem);
> spin_lock(&drives_lock);
> list_del_init(&drive->list);
> @@ -2234,6 +2476,18 @@
> return 0;
> }
>
> +/**
> + * ide_register_driver - new driver loaded
> + * @driver: the IDE driver module
> + *
> + * Register a new driver module and then scan the devices
> + * on the IDE bus in case any should be attached to the
> + * driver we have just attached. If so attach them
> + *
> + * Takes the drivers and drives lock. Should take the
> + * ide_sem but doesn't - FIXME
> + */
> +
> int ide_register_driver(ide_driver_t *driver)
> {
> struct list_head list;
> @@ -2242,9 +2496,11 @@
>
> setup_driver_defaults(driver);
>
> + down(&drivers_sem);
> spin_lock(&drivers_lock);
> list_add(&driver->drivers, &drivers);
> spin_unlock(&drivers_lock);
> + up(&drivers_sem);
>
> INIT_LIST_HEAD(&list);
> spin_lock(&drives_lock);
> @@ -2265,13 +2521,25 @@
>
> EXPORT_SYMBOL(ide_register_driver);
>
> +/**
> + * ide_unregister_driver - IDE module unload
> + * @driver: IDE driver module
> + *
> + * Unload a driver module and reattach any devices to whatever
> + * driver claims them next (typically the default driver). Takes
> + * the drivers lock, and called functions will take the
> + * IDE setting semaphore.
> + */
> +
> void ide_unregister_driver(ide_driver_t *driver)
> {
> ide_drive_t *drive;
>
> + down(&drivers_sem);
> spin_lock(&drivers_lock);
> list_del(&driver->drivers);
> spin_unlock(&drivers_lock);
> + up(&drivers_sem);
>
> driver_unregister(&driver->gen_driver);
>
> @@ -2385,7 +2653,8 @@
> int index;
>
> for (index = 0; index < MAX_HWIFS; ++index) {
> - ide_unregister(index);
> + if(ide_unregister_hwif(&ide_hwifs[index]))
> + printk(KERN_ERR "ide: unload yet busy!\n");
> if (ide_hwifs[index].dma_base)
> (void) ide_release_dma(&ide_hwifs[index]);
> }
> diff -u --new-file --recursive --exclude-from /usr/src/exclude
> linux.vanilla-2.6.8-rc3/include/linux/ide.h
> linux-2.6.8-rc3/include/linux/ide.h ---
> linux.vanilla-2.6.8-rc3/include/linux/ide.h 2004-08-09 15:50:59.000000000
> +0100 +++ linux-2.6.8-rc3/include/linux/ide.h 2004-08-12 16:45:17.000000000
> +0100 @@ -1503,9 +1509,11 @@
> extern void ide_pci_unregister_driver(struct pci_driver *driver);
> extern void ide_pci_setup_ports(struct pci_dev *dev, struct
> ide_pci_device_s *d, int autodma, int pciirq, ata_index_t *index); extern
> void ide_setup_pci_noise (struct pci_dev *dev, struct ide_pci_device_s *d);
> +extern void ide_pci_remove_hwifs(struct pci_dev *dev);
>
> extern void default_hwif_iops(ide_hwif_t *);
> extern void default_hwif_mmiops(ide_hwif_t *);
> +extern void removed_hwif_iops(ide_hwif_t *);
> extern void default_hwif_transport(ide_hwif_t *);
>
> int ide_register_driver(ide_driver_t *driver);
> @@ -1603,8 +1611,9 @@
> #endif
>
> extern int ide_hwif_request_regions(ide_hwif_t *hwif);
> -extern void ide_hwif_release_regions(ide_hwif_t* hwif);
> -extern void ide_unregister (unsigned int index);
> +extern void ide_hwif_release_regions(ide_hwif_t *hwif);
> +extern int ide_unregister_hwif(ide_hwif_t *hwif);
> +extern int __ide_unregister_hwif(ide_hwif_t *hwif);
>
> extern int probe_hwif_init(ide_hwif_t *);
>
>
> Signed-off-by: Alan Cox <alan@xxxxxxxxxx>
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/