Re: [RFC][PATCH] New ACPI-WMI driver for shuttle machines

[Herton Ronaldo Krzesinski <herton@mandriva.com.br>]

[-- Attachment #1: Type: text/plain, Size: 2955 bytes --]

On Wed, 1 Dec 2010 14:14:22 -0200
Herton Ronaldo Krzesinski <herton@mandriva.com.br> wrote:
> Hi,
> 
> > > +static acpi_status wmi_setget_mtd(struct shuttle_cmd *scmd, u32 **res)
> > > +{
> > > +       acpi_status status;
> > > +       union acpi_object *obj;
> > > +       struct acpi_buffer input;
> > > +       static DEFINE_MUTEX(mtd_lock);
> > > +       struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL };
> > > +
> > > +       input.length = sizeof(struct shuttle_cmd);
> > > +       scmd->hdr = 0xec00;
> > > +       input.pointer = (u8 *) scmd;
> > > +
> > > +       mutex_lock(&mtd_lock);
> > > +       status = wmi_evaluate_method(SHUTTLE_WMI_SETGET_GUID, 0, 2,
> > > +                                    &input, &output);
> > > +       mutex_unlock(&mtd_lock);
> > 
> > I'm not sure why you need a mutex here ?
> 
> I need mutex because of the way the vendor implemented things in its wmi
> interface. If I don't serialize the access to it, when sending commands
> in parallel some of them will fail, for example:
> 
> # echo 1 > webcam & rfkill unblock <idx> & echo 1 > touchpad
> 
> some of the echos or rfkill will fail. This happens because the vendor used a
> common buffer/variable to store parameters in AML code, from the DSDT:
> 
> Name (AC00, Buffer (0x28)
> ...
> CreateDWordField (AC00, Zero, SAC0)
> CreateDWordField (AC00, 0x04, SAC1)
> ...
> CreateByteField (AC00, Zero, SA00)
> CreateByteField (AC00, One, SA01)
> CreateByteField (AC00, 0x02, SA02)
> CreateByteField (AC00, 0x03, SA03)
> CreateByteField (AC00, 0x04, SA04)
> ...
> Method (WMBC, 3, NotSerialized)
> {
>     If (LEqual (Arg1, One))
>     {
>          Return (GETC (Arg0))
>     }
> 
>     If (LEqual (Arg1, 0x02))
>     {
>          Return (SETC (Arg0, Arg2))
>     }
> ...
> Method (SETC, 2, NotSerialized)
> {
>     If (LEqual (Arg0, Zero))
>     {
>          Store (Arg1, AC00)
>          OEMF (AC00)
>          Return (SAC0)
>     }
> ...
> Method (OEMF, 1, NotSerialized)
> {
>     If (LEqual (SAC1, 0xEC000300))
>     {
>          Store (0x73, DBG8)
>          Store ("LS", SAC0)
>          Return (SAC0)
>     }
> 
>     If (LEqual (SAC1, 0xEC000000))
>     {
>          WKBC (SA00, SA01, SA02, SA03)
>          Return (SAC0)
>     }
> ...
> 
> 
> Note that it uses only buffer AC00 to store and read the
> parameters, so parallel calls will overwrite the value in AC00
> (referenced as SAC?? and SA?? in some places) and some commands
> will fail or have unpredicted behaviour.
> 
> Placing the lock prevents the parallel echo/rfkill command I placed
> above as example to fail, all writes/commands are sent successfuly.
> 
> I'm attaching here DSDT of both machines I have for you to see the
> entire code.

Too long reply and forgot to attach it :P
here it goes the two gzipped.

-- 
[]'s
Herton

[-- Attachment #2: DSDT-DA18IE.dsl.gz --]
[-- Type: application/x-gzip, Size: 20796 bytes --]

[-- Attachment #3: DSDT-DA18IM.dsl.gz --]
[-- Type: application/x-gzip, Size: 18424 bytes --]