| Message ID | 20211031153135.5024-1-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | Mainlined, archived |
| Headers | show |
| Series | [v2] ACPI / PMIC: Fix intel_pmic_regs_handler() read accesses | expand |
On Sun, Oct 31, 2021 at 4:31 PM Hans de Goede <hdegoede@redhat.com> 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@kernel.org> > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > 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!
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) {
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@kernel.org> Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- Changes in v2: - Be consistent with capitalization of OpRegion --- drivers/acpi/pmic/intel_pmic.c | 51 +++++++++++++++++++--------------- 1 file changed, 28 insertions(+), 23 deletions(-)