Re: PATCH: straighten out the IDE layer locking and add hotplug

[Alan Cox]

On Tue, Aug 17, 2004 at 03:12:26PM +0200, Bartlomiej Zolnierkiewicz wrote:
> 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

Will take a look. 
 
> > +
> > +	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

The existing code didn't allow reuse of the hwif. It leaked it forever
each time because the chipset was left randomly set to pci. ide_unknown is
used by the scanning code in various places to mean "reusable". It has no
impact on ordering I can see because you don't hotplug ide until after
the boot sequence is complete. Once you do hotplug well the order is already
intriguing although it will preserve the pre setup legacy interfaces.

> 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?

I've done some basic testing on things like unload invalidate. It would
have missed the unload/reload one. As discussed earlier I'm trying some
alternatives - refcount turns out to get really horrible because unless
we switch to refcounting -everything- in one go we get unregisters that
can randomly fail as busy depending on /proc and other accesses, and
code that isnt meant to cope with this.

> >  		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

No. Follow the locking. You have to move that locking outwards and I
already did so. Remember ata_attach is only safe under the setting sem.
The attach methods should always have ben called under setting_sem but
were not. Now they are so they in turn call the setting functions safely.

> > -			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).

It's unsafe for the PCI case. Its also unsafe for every other case. Thats
why I have a fixme tagged on it 8)

> 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. :/

Once you have drive and controller hot plug you don't need them. Until then
some laptop users rely on them. I'd prefer to ignore the issue (its a
privileged code path) until the hotplug is there, or patch it up by
allowing unregister only of SCAN_HWIF added hwifs ?

Alan

-
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/