Re: [PATCH v2] ACPI / PMIC: Fix intel_pmic_regs_handler() read accesses

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Sun, Oct 31, 2021 at 4:31 PM Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
>
> The handling of PMIC register reads through writing 0 to address 4
> of the OpRegion is wrong. Instead of returning the read value
> through the value64, which is a no-op for function == ACPI_WRITE calls,
> store the value and then on a subsequent function == ACPI_READ with
> address == 3 (the address for the value field of the OpRegion)
> return the stored value.
>
> This has been tested on a Xiaomi Mi Pad 2 and makes the ACPI battery dev
> there mostly functional (unfortunately there are still other issues).
>
> Here are the SET() / GET() functions of the PMIC ACPI device,
> which use this OpRegion, which clearly show the new behavior to
> be correct:
>
> OperationRegion (REGS, 0x8F, Zero, 0x50)
> Field (REGS, ByteAcc, NoLock, Preserve)
> {
>     CLNT,   8,
>     SA,     8,
>     OFF,    8,
>     VAL,    8,
>     RWM,    8
> }
>
> Method (GET, 3, Serialized)
> {
>     If ((AVBE == One))
>     {
>         CLNT = Arg0
>         SA = Arg1
>         OFF = Arg2
>         RWM = Zero
>         If ((AVBG == One))
>         {
>             GPRW = Zero
>         }
>     }
>
>     Return (VAL) /* \_SB_.PCI0.I2C7.PMI5.VAL_ */
> }
>
> Method (SET, 4, Serialized)
> {
>     If ((AVBE == One))
>     {
>         CLNT = Arg0
>         SA = Arg1
>         OFF = Arg2
>         VAL = Arg3
>         RWM = One
>         If ((AVBG == One))
>         {
>             GPRW = One
>         }
>     }
> }
>
> Fixes: 0afa877a5650 ("ACPI / PMIC: intel: add REGS operation region support")
> Cc: Felipe Balbi <balbi@xxxxxxxxxx>
> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
> ---
> Changes in v2:
> - Be consistent with capitalization of OpRegion
> ---
>  drivers/acpi/pmic/intel_pmic.c | 51 +++++++++++++++++++---------------
>  1 file changed, 28 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/acpi/pmic/intel_pmic.c b/drivers/acpi/pmic/intel_pmic.c
> index a371f273f99d..9cde299eba88 100644
> --- a/drivers/acpi/pmic/intel_pmic.c
> +++ b/drivers/acpi/pmic/intel_pmic.c
> @@ -211,31 +211,36 @@ static acpi_status intel_pmic_regs_handler(u32 function,
>                 void *handler_context, void *region_context)
>  {
>         struct intel_pmic_opregion *opregion = region_context;
> -       int result = 0;
> +       int result = -EINVAL;
> +
> +       if (function == ACPI_WRITE) {
> +               switch (address) {
> +               case 0:
> +                       return AE_OK;
> +               case 1:
> +                       opregion->ctx.addr |= (*value64 & 0xff) << 8;
> +                       return AE_OK;
> +               case 2:
> +                       opregion->ctx.addr |= *value64 & 0xff;
> +                       return AE_OK;
> +               case 3:
> +                       opregion->ctx.val = *value64 & 0xff;
> +                       return AE_OK;
> +               case 4:
> +                       if (*value64) {
> +                               result = regmap_write(opregion->regmap, opregion->ctx.addr,
> +                                                     opregion->ctx.val);
> +                       } else {
> +                               result = regmap_read(opregion->regmap, opregion->ctx.addr,
> +                                                    &opregion->ctx.val);
> +                       }
> +                       opregion->ctx.addr = 0;
> +               }
> +       }
>
> -       switch (address) {
> -       case 0:
> -               return AE_OK;
> -       case 1:
> -               opregion->ctx.addr |= (*value64 & 0xff) << 8;
> -               return AE_OK;
> -       case 2:
> -               opregion->ctx.addr |= *value64 & 0xff;
> +       if (function == ACPI_READ && address == 3) {
> +               *value64 = opregion->ctx.val;
>                 return AE_OK;
> -       case 3:
> -               opregion->ctx.val = *value64 & 0xff;
> -               return AE_OK;
> -       case 4:
> -               if (*value64) {
> -                       result = regmap_write(opregion->regmap, opregion->ctx.addr,
> -                                             opregion->ctx.val);
> -               } else {
> -                       result = regmap_read(opregion->regmap, opregion->ctx.addr,
> -                                            &opregion->ctx.val);
> -                       if (result == 0)
> -                               *value64 = opregion->ctx.val;
> -               }
> -               memset(&opregion->ctx, 0x00, sizeof(opregion->ctx));
>         }
>
>         if (result < 0) {
> --

Applied as 5.16-rc material, thanks!



[Index of Archives]     [Linux IBM ACPI]     [Linux Power Management]     [Linux Kernel]     [Linux Laptop]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]

  Powered by Linux