[PATCH] fix mapping of chassitype to system.formfactor

[David Zeuthen]

On Tue, 2006-05-09 at 00:45 +0200, Danny Kukawka wrote:
> On Monday 08 May 2006 17:54, David Zeuthen wrote:
> > On Fri, 2006-05-05 at 17:48 +0200, Danny Kukawka wrote:
> [...]
> > and I'm not sure why we need locking.
> 
> I couldn't find a other 100% secure way to avoid this problem without a 
> onetime lock. 
> 
> > Maybe the ACPI/PMU/APM code is setting system.formfactor when it
> > shouldn't (e.g. in step 1 rather than step 2). Also, what about
> > synthesizing the events we need *after* we added the computer object?
> > Any chance you can look into that?
> 
> No, I think the ACPI/APM/PMU code is correct. 

The thing is that acpi_synthesize() sets a property on the computer
object. This function is called by acpi_synthesize_hotplug_events which
is in turn called by osspec_probe() even before the helper
hald-probe-smbios is invoked. 

> The problem is, that the 
> information for mapping smbios info to system.formfactor are from the prober 
> which call dmidecode and need to parse the information from this output to 
> fill the needed keys. IMO you can't say how long this need, because the 
> prober run while HAL do other things (like synthesizing ACPI events/devices) 
> without a lock you can't be sure that a other part write information while 
> this code do the mapping. I debugged this 'race' and the info from ACPI acpi 
> is written before the mapping start. I see no other way to fix this atm.

The thinking is

 1. A minimal computer object is created in the TDL

 2. All hotplug events are synthesized; e.g. create a long list of
    hotplug events to process

    Notably, while synthesizing we're allowed to change stuff on the
    computer object in the TDL, like system.formfactor - this is
    intended because when synthesizing all events we gather a lot of
    information and e.g. we say if the computer has battery bays then
    it must be a laptop. Which I think is sane.

    (notably hald/linux2/apm.c is wrong; it sets system.formfactor in
     the wrong place; e.g. needs to be set while synthesizing the event;
     that needs to be fixed)

 3. Continue building the computer object, e.g. run helpers for looking
    at the smbios.

 4. Merge fdi files; notably they may now depend on system.formfactor
    being correct.

 5. Run callouts for computer

 6. Add computer to GDL

 7. Process all hotplug events in order [1]

So.. I'm not sure we're doing anything wrong. This looks right, yes?

As far as I can see, at worst computer_probing_pcbios_helper_done()
needs to get some more smarts so as to not overwrite system.formfactor
in the event system.formfactor is already 'laptop' because one of the
ACPI/PMU/APM hotplug synthesizers set it that way. 

The code itself looks broken

  for (i = 0; chassis_map[i] != NULL; i += 2) {
    if (strcmp (chassis_map[i], chassis_type) == 0) {
      /* check if the key is 'unknown' to prevent overwrite keys */
      if (strcmp (hal_device_property_get_string (d, "system.formfactor"), "unknown"))
        hal_device_property_set_string (d, "system.formfactor", chassis_map[i+1]);
      break;
    }
  }

The problem here is that we only set system.formfactor if it's not
"unknown" already. Someone likely forgot the "== 0" part or how
strcmp(3) works :-). It's much better to be verbose, binary operators
are cheap these days and the code makes it look like strcmp returns a
boolean.

Maybe it should look like this:

  /* don't attempt to modify system.formfactor unless it's already "unknown" /*
  if (strcmp (hal_device_property_get_string (d, "system.formfactor"), "unknown") == 0) {
    for (i = 0; chassis_map[i] != NULL; i += 2) {
      if (strcmp (chassis_map[i], chassis_type) == 0) {
        hal_device_property_set_string (d, "system.formfactor", chassis_map[i+1]);
        break;
      }
    }
  }

What is the original problem that the patch was trying to address? Does
my proposed fix above fix it?

    David

[1] : some day in the future this may be done in parallel.