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

[Bartlomiej Zolnierkiewicz]

one more comment

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

this is dubious for many non PCI drivers which use ide_register_hw() to only
claim/fill ide_hwifs[] entry but actual probing is done later by ide-generic 
driver - we end up with hwif->present == 0 and hwif->configured == 1
and if ide_register_hw() will try to unregister such hwif it will possibly 
crash (because we now check for ->configured not ->present in 
ide_unregister_hwif) - you've correctly noticed in the FIXMEs that we 
shouldn't be unregistering hwifs in ide_register_hw() but this has some 
side-effects (breaks HDIO_SCAN_HWIF for all non default/generic hwifs
and can change ordering in some rare situations) - we should go that way but 
we need to be fully aware of results of this change

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