Re: RFC: device attachment/detachment

[Christoph Egger <Christoph_Egger%gmx.de@localhost>]

Quentin Garnier wrote:
> On Sat, May 02, 2009 at 12:56:43AM +0200, Christoph Egger wrote:
>> Drivers attaches in the device tree independent if their attachment
>> function failed or not. The autoconf framework isn't aware of this,
>> because the attachment function doesn't return an error code.
>>
>> dyoung@ started to work on the drivers to detach the drivers
>> on shutdown/reboot. The drivers' detach function doesn't know if
>> the attachment failed or not. It only can carefully
>> check which resource was allocated and which not and free the
>> allocated ones.
> 
> No matter what happens, one has to be careful not to free resources it
> didn't alloc.  I don't see how anything could change that.
> 
>> Attached patch modifies the driver's attachment function to
>> return an error code, zero for success.
> 
> While I understand where this comes from, autoconf(9) has more short-
> comings than just that one, and fixing them will require touching every
> single driver in the tree, too.  I'd rather see that done just once.

Can you enlist all shortcomings, please ?


> [...]
>> @@ -1377,6 +1381,15 @@ config_attach_loc(device_t parent, cfdat
>>                      }
>>              }
>>      }
>> +
>> +#if defined(SPLASHSCREEN) && defined(SPLASHSCREEN_PROGRESS)
>> +    if (splash_progress_state)
>> +            splash_progress_update(splash_progress_state);
>> +#endif
>> +    error = (*dev->dv_cfattach->ca_attach)(parent, dev, aux);
>> +    if (error)
>> +            goto fail;
>> +
>>  #ifdef __HAVE_DEVICE_REGISTER
>>      device_register(dev, aux);
>>  #endif
>> @@ -1388,17 +1401,18 @@ config_attach_loc(device_t parent, cfdat
>>      if (splash_progress_state)
>>              splash_progress_update(splash_progress_state);
>>  #endif
>> -    (*dev->dv_cfattach->ca_attach)(parent, dev, aux);
>> -#if defined(SPLASHSCREEN) && defined(SPLASHSCREEN_PROGRESS)
>> -    if (splash_progress_state)
>> -            splash_progress_update(splash_progress_state);
>> -#endif
> [...]
>> @@ -1435,14 +1453,23 @@ config_attach_pseudo(cfdata_t cf)
>>  
>>      config_devlink(dev);
>>  
>> +    error = (*dev->dv_cfattach->ca_attach)(ROOT, dev, NULL);
>> +    if (error)
>> +            goto fail;
>> +
>>  #if 0       /* XXXJRT not yet */
>>  #ifdef __HAVE_DEVICE_REGISTER
>>      device_register(dev, NULL);     /* like a root node */
>>  #endif
>>  #endif
>> -    (*dev->dv_cfattach->ca_attach)(ROOT, dev, NULL);
> 
> You can't move the attach callbacks around the call to
> device_register().  (I'll grant you that it is #if 0'd in the pseudo-
> device case, but it might not stay that way eventually.)
> 
> device_register() has to happen before the attach callback because the
> system will typically set properties for the attach callback to use
> during that call.  I don't know how device_register() users could
> react if the device they've just messed with disappear immediately.  It
> has to be checked carefully.

On x86, device_register() is used to set bootdev. When prompted for
the boot device, the failed devices shouldn't be listed.
When other platforms do other things that need to be undone, then
we need a device_deregister() for autoconf's error path.

Christoph