* [PATCH RFC v5 3/6] ARM: pxa: Convert Spitz CF power control to GPIO descriptors
From: Duje Mihanović @ 2023-10-04 14:56 UTC (permalink / raw)
To: Daniel Mack, Haojian Zhuang, Robert Jarzmik, Russell King,
Alan Stern, Greg Kroah-Hartman, Linus Walleij,
Bartosz Golaszewski, Andy Shevchenko, Dmitry Torokhov, Mark Brown
Cc: linux-arm-kernel, linux-kernel, linux-usb, linux-gpio,
linux-input, linux-spi, Duje Mihanović
In-Reply-To: <20231004-pxa-gpio-v5-0-d99ae6fceea8@skole.hr>
Sharp's Spitz board still uses the legacy GPIO interface for controlling
the power supply to its CF and SD card slots.
Convert it to use the GPIO descriptor interface.
Acked-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Duje Mihanović <duje.mihanovic@skole.hr>
---
arch/arm/mach-pxa/spitz.c | 20 ++++++++++++++++++--
1 file changed, 18 insertions(+), 2 deletions(-)
diff --git a/arch/arm/mach-pxa/spitz.c b/arch/arm/mach-pxa/spitz.c
index b6a4085e9fb0..965354e64c68 100644
--- a/arch/arm/mach-pxa/spitz.c
+++ b/arch/arm/mach-pxa/spitz.c
@@ -133,6 +133,10 @@ static unsigned long spitz_pin_config[] __initdata = {
* Scoop GPIO expander
******************************************************************************/
#if defined(CONFIG_SHARP_SCOOP) || defined(CONFIG_SHARP_SCOOP_MODULE)
+GPIO_LOOKUP_SINGLE(spitz_card_pwr_ctrl_gpio_table, "pxa2xx-mci.0",
+ "sharp-scoop", SPITZ_GPIO_CF_POWER, "cf_power",
+ GPIO_ACTIVE_HIGH);
+
/* SCOOP Device #1 */
static struct resource spitz_scoop_1_resources[] = {
[0] = {
@@ -190,6 +194,7 @@ struct platform_device spitz_scoop_2_device = {
static void __init spitz_scoop_init(void)
{
platform_device_register(&spitz_scoop_1_device);
+ gpiod_add_lookup_table(&spitz_card_pwr_ctrl_gpio_table);
/* Akita doesn't have the second SCOOP chip */
if (!machine_is_akita())
@@ -201,9 +206,18 @@ static void __maybe_unused spitz_card_pwr_ctrl(uint8_t enable, uint8_t new_cpr)
{
unsigned short cpr;
unsigned long flags;
+ struct gpio_desc *cf_power;
+
+ cf_power = gpiod_get(&pxa_device_mci.dev, "cf_power", GPIOD_ASIS);
+ if (IS_ERR(cf_power)) {
+ dev_err(&pxa_device_mci.dev,
+ "failed to get power control GPIO with %ld\n",
+ PTR_ERR(cf_power));
+ return;
+ }
if (new_cpr & 0x7) {
- gpio_set_value(SPITZ_GPIO_CF_POWER, 1);
+ gpiod_direction_output(cf_power, 1);
mdelay(5);
}
@@ -222,8 +236,10 @@ static void __maybe_unused spitz_card_pwr_ctrl(uint8_t enable, uint8_t new_cpr)
if (!(cpr & 0x7)) {
mdelay(1);
- gpio_set_value(SPITZ_GPIO_CF_POWER, 0);
+ gpiod_direction_output(cf_power, 0);
}
+
+ gpiod_put(cf_power);
}
#else
--
2.42.0
^ permalink raw reply related
* [PATCH RFC v5 0/6] ARM: pxa: GPIO descriptor conversions
From: Duje Mihanović @ 2023-10-04 14:56 UTC (permalink / raw)
To: Daniel Mack, Haojian Zhuang, Robert Jarzmik, Russell King,
Alan Stern, Greg Kroah-Hartman, Linus Walleij,
Bartosz Golaszewski, Andy Shevchenko, Dmitry Torokhov, Mark Brown
Cc: linux-arm-kernel, linux-kernel, linux-usb, linux-gpio,
linux-input, linux-spi, Duje Mihanović
Hello,
Small series to convert some of the board files in the mach-pxa directory
to use the new GPIO descriptor interface.
Most notably, the am200epd, am300epd and Spitz matrix keypad among
others are not converted in this series.
Signed-off-by: Duje Mihanović <duje.mihanovic@skole.hr>
---
Changes in v5:
- Address maintainer comments:
- Rename "reset generator" GPIO to "reset"
- Rename ads7846_wait_for_sync() to ads7846_wait_for_sync_gpio()
- Properly bail out when requesting USB host GPIO fails
- Use dev_err_probe() when requesting touchscreen sync GPIO fails
- Use static gpio_desc for gumstix bluetooth reset
- Pulse gumstix bluetooth reset line correctly (assert, then deassert)
- Fix style issue in ads7846_wait_for_sync_gpio()
- Update trailers
- Link to v4: https://lore.kernel.org/r/20231001-pxa-gpio-v4-0-0f3b975e6ed5@skole.hr
Changes in v4:
- Address maintainer comments:
- Move wait_for_sync() from spitz.c to driver
- Register LED platform device before getting its gpiod-s
- Add Linus' Reviewed-by
- Link to v3: https://lore.kernel.org/r/20230929-pxa-gpio-v3-0-af8d5e5d1f34@skole.hr
Changes in v3:
- Address maintainer comments:
- Use GPIO_LOOKUP_IDX for LEDs
- Drop unnecessary NULL assignments
- Don't give up on *all* SPI devices if hsync cannot be set up
- Add Linus' Acked-by
- Link to v2: https://lore.kernel.org/r/20230926-pxa-gpio-v2-0-984464d165dd@skole.hr
Changes in v2:
- Address maintainer comments:
- Change mentions of function to function()
- Drop cast in OHCI driver dev_warn() call
- Use %pe in OHCI and reset drivers
- Use GPIO _optional() API in OHCI driver
- Drop unnecessary not-null check in OHCI driver
- Use pr_err() instead of printk() in reset driver
- Rebase on v6.6-rc3
- Link to v1: https://lore.kernel.org/r/20230924-pxa-gpio-v1-0-2805b87d8894@skole.hr
---
Duje Mihanović (6):
ARM: pxa: Convert Spitz OHCI to GPIO descriptors
ARM: pxa: Convert Spitz LEDs to GPIO descriptors
ARM: pxa: Convert Spitz CF power control to GPIO descriptors
ARM: pxa: Convert reset driver to GPIO descriptors
ARM: pxa: Convert gumstix Bluetooth to GPIO descriptors
input: ads7846: Move wait_for_sync() logic to driver
arch/arm/mach-pxa/gumstix.c | 22 ++++++------
arch/arm/mach-pxa/reset.c | 39 +++++++--------------
arch/arm/mach-pxa/reset.h | 3 +-
arch/arm/mach-pxa/spitz.c | 69 +++++++++++++++++++++++++------------
drivers/input/touchscreen/ads7846.c | 22 ++++++++----
drivers/usb/host/ohci-pxa27x.c | 7 ++++
include/linux/spi/ads7846.h | 1 -
7 files changed, 94 insertions(+), 69 deletions(-)
---
base-commit: 6465e260f48790807eef06b583b38ca9789b6072
change-id: 20230807-pxa-gpio-3ce25d574814
Best regards,
--
Duje Mihanović <duje.mihanovic@skole.hr>
^ permalink raw reply
* Re: [PATCH v2] Input: powermate - fix use-after-free in powermate_config_complete
From: Dmitry Torokhov @ 2023-10-04 14:30 UTC (permalink / raw)
To: Javier Carrasco; +Cc: linux-input, linux-kernel, syzbot+0434ac83f907a1dbdd1e
In-Reply-To: <20230916-topic-powermate_use_after_free-v2-1-505f49019f2f@gmail.com>
Hi Javier,
On Sun, Oct 01, 2023 at 05:35:54PM +0200, Javier Carrasco wrote:
> syzbot has found a use-after-free bug [1] in the powermate driver. This
> happens when the device is disconnected, which leads to a memory free
> from the powermate_device struct.
> When an asynchronous control message completes after the kfree and its
> callback is invoked, the lock does not exist anymore and hence the bug.
>
> Use usb_kill_urb() on pm->config to cancel any in-progress requests upon
> device disconnection. Given that this action is already done on pm->irq,
> reorder the code to have both calls after the call to
> input_unregister_device(), which is the most common approach.
>
> [1] https://syzkaller.appspot.com/bug?extid=0434ac83f907a1dbdd1e
>
> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
> Reported-by: syzbot+0434ac83f907a1dbdd1e@syzkaller.appspotmail.com
> ---
> Changes in v2:
> - Use usb_kill_urb() on pm->config upon device disconnection.
> - Link to v1: https://lore.kernel.org/r/20230916-topic-powermate_use_after_free-v1-1-2ffa46652869@gmail.com
> ---
> drivers/input/misc/powermate.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/input/misc/powermate.c b/drivers/input/misc/powermate.c
> index c1c733a9cb89..c0aea75eb087 100644
> --- a/drivers/input/misc/powermate.c
> +++ b/drivers/input/misc/powermate.c
> @@ -423,8 +423,9 @@ static void powermate_disconnect(struct usb_interface *intf)
> usb_set_intfdata(intf, NULL);
> if (pm) {
> pm->requires_update = 0;
> - usb_kill_urb(pm->irq);
No, you do not want interrupts coming in while input device is being
destroyed, this one needs to stay where it is.
> input_unregister_device(pm->input);
> + usb_kill_urb(pm->irq);
> + usb_kill_urb(pm->config);
> usb_free_urb(pm->irq);
> usb_free_urb(pm->config);
> powermate_free_buffers(interface_to_usbdev(intf), pm);
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH] Input: goodix - Ensure int GPIO is in input for gpio_count == 1 && gpio_int_idx == 0 case
From: Dmitry Torokhov @ 2023-10-04 14:26 UTC (permalink / raw)
To: Hans de Goede; +Cc: Bastien Nocera, linux-input, Michael Smith
In-Reply-To: <20231003215144.69527-1-hdegoede@redhat.com>
On Tue, Oct 03, 2023 at 11:51:44PM +0200, Hans de Goede wrote:
> Add a special case for gpio_count == 1 && gpio_int_idx == 0 to
> goodix_add_acpi_gpio_mappings().
>
> It seems that on newer x86/ACPI devices the reset and irq GPIOs are no
> longer listed as GPIO resources instead there is only 1 GpioInt resource
> and _PS0 does the whole reset sequence for us.
>
> This means that we must call acpi_device_fix_up_power() on these devices
> to ensure that the chip is reset before we try to use it.
>
> This part was already fixed in commit 3de93e6ed2df ("Input: goodix - call
> acpi_device_fix_up_power() in some cases") by adding a call to
> acpi_device_fix_up_power() to the generic "Unexpected ACPI resources"
> catch all.
>
> But it turns out that this case on some hw needs some more special
> handling. Specifically the firmware may bootup with the IRQ pin in
> output mode. The reset sequence from ACPI _PS0 (executed by
> acpi_device_fix_up_power()) should put the pin in input mode,
> but the GPIO subsystem has cached the direction at bootup, causing
> request_irq() to fail due to gpiochip_lock_as_irq() failure:
>
> [ 9.119864] Goodix-TS i2c-GDIX1002:00: Unexpected ACPI resources: gpio_count 1, gpio_int_idx 0
> [ 9.317443] Goodix-TS i2c-GDIX1002:00: ID 911, version: 1060
> [ 9.321902] input: Goodix Capacitive TouchScreen as /devices/pci0000:00/0000:00:17.0/i2c_designware.4/i2c-5/i2c-GDIX1002:00/input/input8
> [ 9.327840] gpio gpiochip0: (INT3453:00): gpiochip_lock_as_irq: tried to flag a GPIO set as output for IRQ
> [ 9.327856] gpio gpiochip0: (INT3453:00): unable to lock HW IRQ 26 for IRQ
> [ 9.327861] genirq: Failed to request resources for GDIX1002:00 (irq 131) on irqchip intel-gpio
> [ 9.327912] Goodix-TS i2c-GDIX1002:00: request IRQ failed: -5
>
> Fix this by adding a special case for gpio_count == 1 && gpio_int_idx == 0
> which adds an ACPI GPIO lookup table for the int GPIO even though we cannot
> use it for reset purposes (as there is no reset GPIO).
>
> Adding the lookup will make the gpiod_int = gpiod_get(..., GPIOD_IN) call
> succeed, which will explicitly set the direction to input fixing the issue.
>
> Note this re-uses the acpi_goodix_int_first_gpios[] lookup table, since
> there is only 1 GPIO in the ACPI resources the reset entry in that
> lookup table will amount to a no-op.
>
> Reported-and-tested-by: Michael Smith <1973.mjsmith@gmail.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Applied, thank you.
--
Dmitry
^ permalink raw reply
* Re: [PATCH] Input: i8042 - add Fujitsu Lifebook E5411 to i8042 quirk table
From: Dmitry Torokhov @ 2023-10-04 14:24 UTC (permalink / raw)
To: Szilard Fabian; +Cc: linux-kernel, linux-input
In-Reply-To: <20231004011749.101789-1-szfabian@bluemarch.art>
On Wed, Oct 04, 2023 at 01:17:54AM +0000, Szilard Fabian wrote:
> In the initial boot stage the integrated keyboard of Fujitsu Lifebook E5411
> refuses to work and it's not possible to type for example a dm-crypt
> passphrase without the help of an external keyboard.
>
> i8042.nomux kernel parameter resolves this issue but using that a PS/2
> mouse is detected. This input device is unused even when the i2c-hid-acpi
> kernel module is blacklisted making the integrated ELAN touchpad
> (04F3:308A) not working at all.
>
> Since the integrated touchpad is managed by the i2c_designware input
> driver in the Linux kernel and you can't find a PS/2 mouse port on the
> computer I think it's safe to not use the PS/2 mouse port at all.
>
> Signed-off-by: Szilard Fabian <szfabian@bluemarch.art>
Applied, thank you.
--
Dmitry
^ permalink raw reply
* Re: [PATCH] Input: elantech - fix fast_reconnect callback in ps2 mode
From: Dmitry Torokhov @ 2023-10-04 14:10 UTC (permalink / raw)
To: Jeffery Miller
Cc: regressions, benjamin.tissoires, linux, Andrew Duggan,
Andrew Duggan, loic.poulain, Greg Kroah-Hartman, linux-input,
linux-kernel
In-Reply-To: <20231004005729.3943515-1-jefferymiller@google.com>
Hi Jeffery,
On Tue, Oct 03, 2023 at 07:57:24PM -0500, Jeffery Miller wrote:
> Make `elantech_setup_ps2` set a compatible fast_reconnect pointer
> when its ps2 mode is used.
>
> When an SMBus connection is attempted and fails `psmouse_smbus_init`
> sets fast_reconnect to `psmouse_smbus_reconnect`.
> `psmouse_smbus_reconnect` expects `psmouse->private` to be
> `struct psmouse_smbus_dev` but `elantech_setup_ps2` replaces
> it with its private data. This was causing an issue on resume
> since psmouse_smbus_reconnect was being called while in ps2, not SMBus
> mode.
>
> This was uncovered by commit 92e24e0e57f7 ("Input: psmouse - add delay when
> deactivating for SMBus mode")
Nice find, thank you.
>
> Closes:
> Link:https://lore.kernel.org/all/ca0109fa-c64b-43c1-a651-75b294d750a1@leemhuis.info/
> Reported-by: Thorsten Leemhuis <linux@leemhuis.info>
>
> Signed-off-by: Jeffery Miller <jefferymiller@google.com>
> ---
>
> The other callbacks set in psmouse_smbus_init are already replaced.
> Should fast_reconnect be set to `elantech_reconnect` instead?
No, doing PS/2 Elantech reinitialization is not a "fast" operation, as
it takes a while to communicate with/query the device, so we should not
be using elantech_reconnect() as a "fast" reconnect handler.
In fact, now that I think about it more, we should rework the original
patch that added the delay, so that we do not wait these 30 msec in the
"fast" reconnect handler. It turns out your original approach was
better, but we should not be using retries, but rather the existing
reset_delay_ms already defined in rmi platform data. I would appreciate
if you try the draft patch at the end of this email (to be applied after
reverting your original one adding the delay in psmouse-smbus.c).
>
>
> drivers/input/mouse/elantech.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/input/mouse/elantech.c b/drivers/input/mouse/elantech.c
> index 2118b2075f43..4e38229404b4 100644
> --- a/drivers/input/mouse/elantech.c
> +++ b/drivers/input/mouse/elantech.c
> @@ -2114,6 +2114,7 @@ static int elantech_setup_ps2(struct psmouse *psmouse,
> psmouse->protocol_handler = elantech_process_byte;
> psmouse->disconnect = elantech_disconnect;
> psmouse->reconnect = elantech_reconnect;
> + psmouse->fast_reconnect = NULL;
I think we need a similar change in synaptics.c as that one also can
fall back to PS/2 mode.
Thanks!
---
diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
index ada299ec5bba..6ccc4a099b51 100644
--- a/drivers/input/mouse/synaptics.c
+++ b/drivers/input/mouse/synaptics.c
@@ -1752,6 +1752,7 @@ static int synaptics_create_intertouch(struct psmouse *psmouse,
psmouse_matches_pnp_id(psmouse, topbuttonpad_pnp_ids) &&
!SYN_CAP_EXT_BUTTONS_STICK(info->ext_cap_10);
const struct rmi_device_platform_data pdata = {
+ .reset_delay_ms = 30,
.sensor_pdata = {
.sensor_type = rmi_sensor_touchpad,
.axis_align.flip_y = true,
diff --git a/drivers/input/rmi4/rmi_smbus.c b/drivers/input/rmi4/rmi_smbus.c
index 7059a2762aeb..b0b099b5528a 100644
--- a/drivers/input/rmi4/rmi_smbus.c
+++ b/drivers/input/rmi4/rmi_smbus.c
@@ -235,12 +235,29 @@ static void rmi_smb_clear_state(struct rmi_smb_xport *rmi_smb)
static int rmi_smb_enable_smbus_mode(struct rmi_smb_xport *rmi_smb)
{
- int retval;
+ struct i2c_client *client = rmi_smb->client;
+ int smbus_version;
+
+ /*
+ * psmouse driver resets the controller, we only need to wait
+ * to give the firmware chance to fully reinitialize.
+ */
+ if (rmi_smb->xport.pdata.reset_delay_ms)
+ msleep(rmi_smb->xport.pdata.reset_delay_ms);
/* we need to get the smbus version to activate the touchpad */
- retval = rmi_smb_get_version(rmi_smb);
- if (retval < 0)
- return retval;
+ smbus_version = rmi_smb_get_version(rmi_smb);
+ if (smbus_version < 0)
+ return smbus_version;
+
+ rmi_dbg(RMI_DEBUG_XPORT, &client->dev, "Smbus version is %d",
+ smbus_version);
+
+ if (smbus_version != 2 && smbus_version != 3) {
+ dev_err(&client->dev, "Unrecognized SMB version %d\n",
+ smbus_version);
+ return -ENODEV;
+ }
return 0;
}
@@ -253,11 +270,10 @@ static int rmi_smb_reset(struct rmi_transport_dev *xport, u16 reset_addr)
rmi_smb_clear_state(rmi_smb);
/*
- * we do not call the actual reset command, it has to be handled in
- * PS/2 or there will be races between PS/2 and SMBus.
- * PS/2 should ensure that a psmouse_reset is called before
- * intializing the device and after it has been removed to be in a known
- * state.
+ * We do not call the actual reset command, it has to be handled in
+ * PS/2 or there will be races between PS/2 and SMBus. PS/2 should
+ * ensure that a psmouse_reset is called before initializing the
+ * device and after it has been removed to be in a known state.
*/
return rmi_smb_enable_smbus_mode(rmi_smb);
}
@@ -272,7 +288,6 @@ static int rmi_smb_probe(struct i2c_client *client)
{
struct rmi_device_platform_data *pdata = dev_get_platdata(&client->dev);
struct rmi_smb_xport *rmi_smb;
- int smbus_version;
int error;
if (!pdata) {
@@ -311,18 +326,9 @@ static int rmi_smb_probe(struct i2c_client *client)
rmi_smb->xport.proto_name = "smb";
rmi_smb->xport.ops = &rmi_smb_ops;
- smbus_version = rmi_smb_get_version(rmi_smb);
- if (smbus_version < 0)
- return smbus_version;
-
- rmi_dbg(RMI_DEBUG_XPORT, &client->dev, "Smbus version is %d",
- smbus_version);
-
- if (smbus_version != 2 && smbus_version != 3) {
- dev_err(&client->dev, "Unrecognized SMB version %d\n",
- smbus_version);
- return -ENODEV;
- }
+ error = rmi_smb_enable_smbus_mode(rmi_smb);
+ if (error)
+ return error;
i2c_set_clientdata(client, rmi_smb);
--
Dmitry
^ permalink raw reply related
* Re: [PATCH v2 12/16] platform/x86/amd/pmf: Add PMF-AMDGPU get interface
From: Ilpo Järvinen @ 2023-10-04 12:49 UTC (permalink / raw)
To: Shyam Sundar S K
Cc: Hans de Goede, markgross, basavaraj.natikar, jikos,
benjamin.tissoires, alexander.deucher, christian.koenig,
Xinhui.Pan, airlied, daniel, Patil.Reddy, mario.limonciello,
platform-driver-x86, linux-input, amd-gfx, dri-devel
In-Reply-To: <20230930083715.2050863-13-Shyam-sundar.S-k@amd.com>
On Sat, 30 Sep 2023, Shyam Sundar S K wrote:
> In order to provide GPU inputs to TA for the Smart PC solution to work, we
> need to have interface between the PMF driver and the AMDGPU driver.
>
> Add the initial code path for get interface from AMDGPU.
>
> Co-developed-by: Mario Limonciello <mario.limonciello@amd.com>
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> @@ -355,6 +356,21 @@ static int amd_pmf_get_bios_buffer(struct amd_pmf_dev *dev)
> return amd_pmf_start_policy_engine(dev);
> }
>
> +static int amd_pmf_get_gpu_handle(struct pci_dev *pdev, void *data)
> +{
> + struct amd_pmf_dev *dev = data;
> +
> + if (pdev->vendor == PCI_VENDOR_ID_ATI && pdev->devfn == 0) {
> + /* get the amdgpu handle from the pci root after walking through the pci bus */
I can see from the code that you assign to amdgpu handle so this comment
added no information.
It doesn't really answer at all why you're doing this second step. Based
on the give parameters to pci_get_device(), it looks as if you're asking
for the same device you already have in pdev to be searched to you.
> + dev->gfx_data.gpu_dev = pci_get_device(pdev->vendor, pdev->device, NULL);
> + if (dev->gfx_data.gpu_dev) {
> + pci_dev_put(pdev);
> + return 1; /* stop walking */
> + }
> + }
> + return 0; /* continue walking */
> +}
> +
> static int amd_pmf_amdtee_ta_match(struct tee_ioctl_version_data *ver, const void *data)
> {
> return ver->impl_id == TEE_IMPL_ID_AMDTEE;
> @@ -451,6 +467,15 @@ int amd_pmf_init_smart_pc(struct amd_pmf_dev *dev)
> INIT_DELAYED_WORK(&dev->pb_work, amd_pmf_invoke_cmd);
> amd_pmf_set_dram_addr(dev);
> amd_pmf_get_bios_buffer(dev);
> +
> + /* get amdgpu handle */
> + pci_walk_bus(dev->root->bus, amd_pmf_get_gpu_handle, dev);
> + if (!dev->gfx_data.gpu_dev)
> + dev_err(dev->dev, "GPU handle not found!\n");
> +
> + if (!amd_pmf_gpu_init(&dev->gfx_data))
> + dev->gfx_data.gpu_dev_en = true;
> +
--
i.
^ permalink raw reply
* Re: [PATCH v2 11/16] platform/x86/amd/pmf: dump policy binary data
From: Ilpo Järvinen @ 2023-10-04 12:32 UTC (permalink / raw)
To: Shyam Sundar S K
Cc: Hans de Goede, markgross, basavaraj.natikar, jikos,
benjamin.tissoires, alexander.deucher, christian.koenig,
Xinhui.Pan, airlied, daniel, Patil.Reddy, mario.limonciello,
platform-driver-x86, linux-input, amd-gfx, dri-devel
In-Reply-To: <20230930083715.2050863-12-Shyam-sundar.S-k@amd.com>
On Sat, 30 Sep 2023, Shyam Sundar S K wrote:
> Sometimes policy binary retrieved from the BIOS maybe incorrect that can
> end up in failing to enable the Smart PC solution feature.
>
> Use print_hex_dump_debug() to dump the policy binary in hex, so that we
> debug the issues related to the binary even before sending that to TA.
>
> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> ---
> drivers/platform/x86/amd/pmf/tee-if.c | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/drivers/platform/x86/amd/pmf/tee-if.c b/drivers/platform/x86/amd/pmf/tee-if.c
> index 01f974b55a6a..d16bdecfd43a 100644
> --- a/drivers/platform/x86/amd/pmf/tee-if.c
> +++ b/drivers/platform/x86/amd/pmf/tee-if.c
> @@ -290,6 +290,9 @@ static ssize_t amd_pmf_get_pb_data(struct file *filp, const char __user *buf,
> if (copy_from_user(dev->policy_buf, buf, dev->policy_sz))
> return -EFAULT;
>
> + print_hex_dump_debug("(pb): ", DUMP_PREFIX_OFFSET, 16, 1, dev->policy_buf,
> + dev->policy_sz, false);
> +
> ret = amd_pmf_start_policy_engine(dev);
> if (ret)
> return -EINVAL;
> @@ -341,6 +344,10 @@ static int amd_pmf_get_bios_buffer(struct amd_pmf_dev *dev)
> return -ENOMEM;
>
> memcpy(dev->policy_buf, dev->policy_base, dev->policy_sz);
> +#ifdef CONFIG_AMD_PMF_DEBUG
> + print_hex_dump_debug("(pb): ", DUMP_PREFIX_OFFSET, 16, 1, dev->policy_buf,
> + dev->policy_sz, false);
> +#endif
Create a wrapper for print_hex_dump_debug into #ifdef and #else blocks for
this too so you don't need the ifdef here.
--
i.
^ permalink raw reply
* Re: [PATCH v2 09/16] platform/x86/amd/pmf: Add facility to dump TA inputs
From: Ilpo Järvinen @ 2023-10-04 12:27 UTC (permalink / raw)
To: Shyam Sundar S K
Cc: Hans de Goede, markgross, basavaraj.natikar, jikos,
benjamin.tissoires, alexander.deucher, christian.koenig,
Xinhui.Pan, airlied, daniel, Patil.Reddy, mario.limonciello,
platform-driver-x86, linux-input, amd-gfx, dri-devel
In-Reply-To: <20230930083715.2050863-10-Shyam-sundar.S-k@amd.com>
On Sat, 30 Sep 2023, Shyam Sundar S K wrote:
> PMF driver sends constant inputs to TA which its gets via the other
> subsystems in the kernel. To debug certain TA issues knowing what inputs
> being sent to TA becomes critical. Add debug facility to the driver which
> can isolate Smart PC and TA related issues.
>
> Also, make source_as_str() as non-static function as this helper is
> required outside of sps.c file.
>
> Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> ---
> drivers/platform/x86/amd/pmf/pmf.h | 3 +++
> drivers/platform/x86/amd/pmf/spc.c | 37 +++++++++++++++++++++++++++
> drivers/platform/x86/amd/pmf/sps.c | 2 +-
> drivers/platform/x86/amd/pmf/tee-if.c | 1 +
> 4 files changed, 42 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/platform/x86/amd/pmf/pmf.h b/drivers/platform/x86/amd/pmf/pmf.h
> index 34778192432e..2ad5ece47601 100644
> --- a/drivers/platform/x86/amd/pmf/pmf.h
> +++ b/drivers/platform/x86/amd/pmf/pmf.h
> @@ -595,6 +595,7 @@ int apmf_get_static_slider_granular(struct amd_pmf_dev *pdev,
> bool is_pprof_balanced(struct amd_pmf_dev *pmf);
> int amd_pmf_power_slider_update_event(struct amd_pmf_dev *dev);
>
> +const char *source_as_str(unsigned int state);
Too generic name, add prefix to the name.
--
i.
> int apmf_update_fan_idx(struct amd_pmf_dev *pdev, bool manual, u32 idx);
> int amd_pmf_set_sps_power_limits(struct amd_pmf_dev *pmf);
> @@ -625,4 +626,6 @@ int apmf_check_smart_pc(struct amd_pmf_dev *pmf_dev);
>
> /* Smart PC - TA interfaces */
> void amd_pmf_populate_ta_inputs(struct amd_pmf_dev *dev, struct ta_pmf_enact_table *in);
> +void amd_pmf_dump_ta_inputs(struct amd_pmf_dev *dev, struct ta_pmf_enact_table *in);
> +
> #endif /* PMF_H */
> diff --git a/drivers/platform/x86/amd/pmf/spc.c b/drivers/platform/x86/amd/pmf/spc.c
> index 3113bde051d9..3aee78629cce 100644
> --- a/drivers/platform/x86/amd/pmf/spc.c
> +++ b/drivers/platform/x86/amd/pmf/spc.c
> @@ -14,6 +14,43 @@
> #include <linux/units.h>
> #include "pmf.h"
>
> +#ifdef CONFIG_AMD_PMF_DEBUG
> +static const char *ta_slider_as_str(unsigned int state)
> +{
> + switch (state) {
> + case TA_BEST_PERFORMANCE:
> + return "PERFORMANCE";
> + case TA_BETTER_PERFORMANCE:
> + return "BALANCED";
> + case TA_BEST_BATTERY:
> + return "POWER_SAVER";
> + default:
> + return "Unknown TA Slider State";
> + }
> +}
> +
> +void amd_pmf_dump_ta_inputs(struct amd_pmf_dev *dev, struct ta_pmf_enact_table *in)
> +{
> + dev_dbg(dev->dev, "==== TA inputs START ====\n");
> + dev_dbg(dev->dev, "Slider State : %s\n", ta_slider_as_str(in->ev_info.power_slider));
> + dev_dbg(dev->dev, "Power Source : %s\n", source_as_str(in->ev_info.power_source));
> + dev_dbg(dev->dev, "Battery Percentage : %u\n", in->ev_info.bat_percentage);
> + dev_dbg(dev->dev, "Designed Battery Capacity : %u\n", in->ev_info.bat_design);
> + dev_dbg(dev->dev, "Fully Charged Capacity : %u\n", in->ev_info.full_charge_capacity);
> + dev_dbg(dev->dev, "Drain Rate : %d\n", in->ev_info.drain_rate);
> + dev_dbg(dev->dev, "Socket Power : %u\n", in->ev_info.socket_power);
> + dev_dbg(dev->dev, "Skin Temperature : %u\n", in->ev_info.skin_temperature);
> + dev_dbg(dev->dev, "Avg C0 Residency : %u\n", in->ev_info.avg_c0residency);
> + dev_dbg(dev->dev, "Max C0 Residency : %u\n", in->ev_info.max_c0residency);
> + dev_dbg(dev->dev, "GFX Busy : %u\n", in->ev_info.gfx_busy);
> + dev_dbg(dev->dev, "Connected Display Count : %u\n", in->ev_info.monitor_count);
> + dev_dbg(dev->dev, "LID State : %s\n", in->ev_info.lid_state ? "Close" : "Open");
> + dev_dbg(dev->dev, "==== TA inputs END ====\n");
> +}
> +#else
> +void amd_pmf_dump_ta_inputs(struct amd_pmf_dev *dev, struct ta_pmf_enact_table *in) {}
> +#endif
> +
> static void amd_pmf_get_smu_info(struct amd_pmf_dev *dev, struct ta_pmf_enact_table *in)
> {
> u16 max, avg = 0;
> diff --git a/drivers/platform/x86/amd/pmf/sps.c b/drivers/platform/x86/amd/pmf/sps.c
> index a70e67749be3..13e36b52dfe8 100644
> --- a/drivers/platform/x86/amd/pmf/sps.c
> +++ b/drivers/platform/x86/amd/pmf/sps.c
> @@ -27,7 +27,7 @@ static const char *slider_as_str(unsigned int state)
> }
> }
>
> -static const char *source_as_str(unsigned int state)
> +const char *source_as_str(unsigned int state)
> {
> switch (state) {
> case POWER_SOURCE_AC:
> diff --git a/drivers/platform/x86/amd/pmf/tee-if.c b/drivers/platform/x86/amd/pmf/tee-if.c
> index 961011530c1b..b0711b2f8c8f 100644
> --- a/drivers/platform/x86/amd/pmf/tee-if.c
> +++ b/drivers/platform/x86/amd/pmf/tee-if.c
> @@ -187,6 +187,7 @@ static int amd_pmf_invoke_cmd_enact(struct amd_pmf_dev *dev)
> }
>
> if (ta_sm->pmf_result == TA_PMF_TYPE_SUCCESS && out->actions_count) {
> + amd_pmf_dump_ta_inputs(dev, in);
> dev_dbg(dev->dev, "action count:%u result:%x\n", out->actions_count,
> ta_sm->pmf_result);
> amd_pmf_apply_policies(dev, out);
>
^ permalink raw reply
* Re: [PATCH v2 08/16] platform/x86/amd/pmf: Add support to update system state
From: Ilpo Järvinen @ 2023-10-04 12:23 UTC (permalink / raw)
To: Shyam Sundar S K
Cc: Hans de Goede, markgross, basavaraj.natikar, jikos,
benjamin.tissoires, alexander.deucher, christian.koenig,
Xinhui.Pan, airlied, daniel, Patil.Reddy, mario.limonciello,
platform-driver-x86, linux-input, amd-gfx, dri-devel,
kernel test robot
In-Reply-To: <20230930083715.2050863-9-Shyam-sundar.S-k@amd.com>
On Sat, 30 Sep 2023, Shyam Sundar S K wrote:
> PMF driver based on the output actions from the TA can request to update
> the system states like entering s0i3, lock screen etc. by generating
> an uevent. Based on the udev rules set in the userspace the event id
> matching the uevent shall get updated accordingly using the systemctl.
>
> Sample udev rules under Documentation/admin-guide/pmf.rst.
>
> Reported-by: kernel test robot <lkp@intel.com>
> Closes: https://lore.kernel.org/oe-kbuild-all/202309260515.5XbR6N0g-lkp@intel.com/
Please don't put lkp tags for patches that are still under development
(even if the email you get misleadingly instructs you to). Only use them
when you fix code that's already in tree based on LKP's report.
> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> ---
> Documentation/admin-guide/index.rst | 1 +
> Documentation/admin-guide/pmf.rst | 25 ++++++++++++++++
> drivers/platform/x86/amd/pmf/pmf.h | 9 ++++++
> drivers/platform/x86/amd/pmf/tee-if.c | 41 ++++++++++++++++++++++++++-
> 4 files changed, 75 insertions(+), 1 deletion(-)
> create mode 100644 Documentation/admin-guide/pmf.rst
>
> diff --git a/Documentation/admin-guide/index.rst b/Documentation/admin-guide/index.rst
> index 43ea35613dfc..fb40a1f6f79e 100644
> --- a/Documentation/admin-guide/index.rst
> +++ b/Documentation/admin-guide/index.rst
> @@ -119,6 +119,7 @@ configure specific aspects of kernel behavior to your liking.
> parport
> perf-security
> pm/index
> + pmf
> pnp
> rapidio
> ras
> diff --git a/Documentation/admin-guide/pmf.rst b/Documentation/admin-guide/pmf.rst
> new file mode 100644
> index 000000000000..90072add511e
> --- /dev/null
> +++ b/Documentation/admin-guide/pmf.rst
> @@ -0,0 +1,25 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +
> +Set udev rules for PMF Smart PC Builder
> +---------------------------------------
> +
> +AMD PMF(Platform Management Framework) Smart PC Solution builder has to set the system states
> +like S0i3, Screen lock, hibernate etc, based on the output actions provided by the PMF
> +TA (Trusted Application).
> +
> +In order for this to work the PMF driver generates a uevent for userspace to react to. Below are
> +sample udev rules that can facilitate this experience when a machine has PMF Smart PC solution builder
> +enabled.
> +
> +Please add the following line(s) to
> +``/etc/udev/rules.d/99-local.rules``::
> +
> + DRIVERS=="amd-pmf", ACTION=="change", ENV{EVENT_ID}=="1", RUN+="/usr/bin/systemctl suspend"
> + DRIVERS=="amd-pmf", ACTION=="change", ENV{EVENT_ID}=="2", RUN+="/usr/bin/systemctl hibernate"
> + DRIVERS=="amd-pmf", ACTION=="change", ENV{EVENT_ID}=="3", RUN+="/bin/loginctl lock-sessions"
> +
> +EVENT_ID values:
> +1= Put the system to S0i3/S2Idle
> +2= Put the system to hibernate
> +3= Lock the screen
> +
> diff --git a/drivers/platform/x86/amd/pmf/pmf.h b/drivers/platform/x86/amd/pmf/pmf.h
> index d5e410c41e81..34778192432e 100644
> --- a/drivers/platform/x86/amd/pmf/pmf.h
> +++ b/drivers/platform/x86/amd/pmf/pmf.h
> @@ -73,6 +73,7 @@
> #define PMF_POLICY_STT_MIN 6
> #define PMF_POLICY_STT_SKINTEMP_APU 7
> #define PMF_POLICY_STT_SKINTEMP_HS2 8
> +#define PMF_POLICY_SYSTEM_STATE 9
> #define PMF_POLICY_P3T 38
>
> /* TA macros */
> @@ -439,6 +440,13 @@ struct apmf_dyn_slider_output {
> } __packed;
>
> /* Smart PC - TA internals */
> +enum system_state {
> + SYSTEM_STATE__S0i3 = 1,
> + SYSTEM_STATE__S4,
> + SYSTEM_STATE__SCREEN_LOCK,
> + SYSTEM_STATE__MAX
> +};
> +
> enum ta_slider {
> TA_BEST_BATTERY, /* Best Battery */
> TA_BETTER_BATTERY, /* Better Battery */
> @@ -470,6 +478,7 @@ enum ta_pmf_error_type {
> };
>
> struct pmf_action_table {
> + enum system_state system_state;
> unsigned long spl; /* in mW */
> unsigned long sppt; /* in mW */
> unsigned long sppt_apuonly; /* in mW */
> diff --git a/drivers/platform/x86/amd/pmf/tee-if.c b/drivers/platform/x86/amd/pmf/tee-if.c
> index 315e3d2eacdf..961011530c1b 100644
> --- a/drivers/platform/x86/amd/pmf/tee-if.c
> +++ b/drivers/platform/x86/amd/pmf/tee-if.c
> @@ -24,6 +24,20 @@ MODULE_PARM_DESC(pb_actions_ms, "Policy binary actions sampling frequency (defau
> static const uuid_t amd_pmf_ta_uuid = UUID_INIT(0x6fd93b77, 0x3fb8, 0x524d,
> 0xb1, 0x2d, 0xc5, 0x29, 0xb1, 0x3d, 0x85, 0x43);
>
> +static const char *amd_pmf_uevent_as_str(unsigned int state)
> +{
> + switch (state) {
> + case SYSTEM_STATE__S0i3:
> + return "S0i3";
> + case SYSTEM_STATE__S4:
> + return "S4";
> + case SYSTEM_STATE__SCREEN_LOCK:
> + return "SCREEN_LOCK";
> + default:
> + return "Unknown Smart PC event";
> + }
> +}
> +
> static void amd_pmf_prepare_args(struct amd_pmf_dev *dev, int cmd,
> struct tee_ioctl_invoke_arg *arg,
> struct tee_param *param)
> @@ -42,9 +56,23 @@ static void amd_pmf_prepare_args(struct amd_pmf_dev *dev, int cmd,
> param[0].u.memref.shm_offs = 0;
> }
>
> +static int amd_pmf_update_uevents(struct amd_pmf_dev *dev, u16 event)
> +{
> + char *envp[2] = {};
> +
> + envp[0] = kasprintf(GFP_KERNEL, "EVENT_ID=%d", event);
> + if (!envp[0])
> + return -EINVAL;
> +
> + kobject_uevent_env(&dev->dev->kobj, KOBJ_CHANGE, envp);
> +
> + kfree(envp[0]);
> + return 0;
> +}
> +
> static void amd_pmf_apply_policies(struct amd_pmf_dev *dev, struct ta_pmf_enact_result *out)
> {
> - unsigned long val;
> + unsigned long val, event = 0;
> int idx;
>
> for (idx = 0; idx < out->actions_count; idx++) {
> @@ -113,6 +141,17 @@ static void amd_pmf_apply_policies(struct amd_pmf_dev *dev, struct ta_pmf_enact_
> dev->prev_data->p3t_limit = val;
> }
> break;
> +
> + case PMF_POLICY_SYSTEM_STATE:
> + event = val + 1;
> + if (dev->prev_data->system_state != event) {
> + amd_pmf_update_uevents(dev, event);
> + dev_dbg(dev->dev, "update SYSTEM_STATE : %s\n",
> + amd_pmf_uevent_as_str(event));
> + /* reset the previous recorded state to zero */
> + dev->prev_data->system_state = 0;
No, a comment won't help you here. As is, system_state is constant 0 so
it's entirely unnecessary to keep that value at all. In fact, the comment
is wrong because there never was "previously recorder state" in
->system_state to begin with since it's either initialized to zero on
alloc or reset to zero by this line.
So what are you trying to achieve here with this ->system_state variable?
--
i.
^ permalink raw reply
* Re: [PATCH v2 06/16] platform/x86/amd/pmf: Add support to get inputs from other subsystems
From: Ilpo Järvinen @ 2023-10-04 12:14 UTC (permalink / raw)
To: Shyam Sundar S K
Cc: Hans de Goede, markgross, basavaraj.natikar, jikos,
benjamin.tissoires, alexander.deucher, christian.koenig,
Xinhui.Pan, airlied, daniel, Patil.Reddy, mario.limonciello,
platform-driver-x86, linux-input, amd-gfx, dri-devel
In-Reply-To: <20230930083715.2050863-7-Shyam-sundar.S-k@amd.com>
On Sat, 30 Sep 2023, Shyam Sundar S K wrote:
> PMF driver sends changing inputs from each subystem to TA for evaluating
> the conditions in the policy binary.
>
> Add initial support of plumbing in the PMF driver for Smart PC to get
> information from other subsystems in the kernel.
>
> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> ---
> drivers/platform/x86/amd/pmf/Makefile | 2 +-
> drivers/platform/x86/amd/pmf/pmf.h | 18 ++++
> drivers/platform/x86/amd/pmf/spc.c | 119 ++++++++++++++++++++++++++
> drivers/platform/x86/amd/pmf/tee-if.c | 3 +
> 4 files changed, 141 insertions(+), 1 deletion(-)
> create mode 100644 drivers/platform/x86/amd/pmf/spc.c
>
> diff --git a/drivers/platform/x86/amd/pmf/Makefile b/drivers/platform/x86/amd/pmf/Makefile
> index d2746ee7369f..6b26e48ce8ad 100644
> --- a/drivers/platform/x86/amd/pmf/Makefile
> +++ b/drivers/platform/x86/amd/pmf/Makefile
> @@ -7,4 +7,4 @@
> obj-$(CONFIG_AMD_PMF) += amd-pmf.o
> amd-pmf-objs := core.o acpi.o sps.o \
> auto-mode.o cnqf.o \
> - tee-if.o
> + tee-if.o spc.o
> diff --git a/drivers/platform/x86/amd/pmf/pmf.h b/drivers/platform/x86/amd/pmf/pmf.h
> index 6f4b6f4ecee4..60b11455dadf 100644
> --- a/drivers/platform/x86/amd/pmf/pmf.h
> +++ b/drivers/platform/x86/amd/pmf/pmf.h
> @@ -149,6 +149,21 @@ struct smu_pmf_metrics {
> u16 infra_gfx_maxfreq; /* in MHz */
> u16 skin_temp; /* in centi-Celsius */
> u16 device_state;
> + u16 curtemp; /* in centi-Celsius */
> + u16 filter_alpha_value;
> + u16 avg_gfx_clkfrequency;
> + u16 avg_fclk_frequency;
> + u16 avg_gfx_activity;
> + u16 avg_socclk_frequency;
> + u16 avg_vclk_frequency;
> + u16 avg_vcn_activity;
> + u16 avg_dram_reads;
> + u16 avg_dram_writes;
> + u16 avg_socket_power;
> + u16 avg_core_power[2];
> + u16 avg_core_c0residency[16];
> + u16 spare1;
> + u32 metrics_counter;
> } __packed;
>
> enum amd_stt_skin_temp {
> @@ -595,4 +610,7 @@ extern const struct attribute_group cnqf_feature_attribute_group;
> int amd_pmf_init_smart_pc(struct amd_pmf_dev *dev);
> void amd_pmf_deinit_smart_pc(struct amd_pmf_dev *dev);
> int apmf_check_smart_pc(struct amd_pmf_dev *pmf_dev);
> +
> +/* Smart PC - TA interfaces */
> +void amd_pmf_populate_ta_inputs(struct amd_pmf_dev *dev, struct ta_pmf_enact_table *in);
> #endif /* PMF_H */
> diff --git a/drivers/platform/x86/amd/pmf/spc.c b/drivers/platform/x86/amd/pmf/spc.c
> new file mode 100644
> index 000000000000..3113bde051d9
> --- /dev/null
> +++ b/drivers/platform/x86/amd/pmf/spc.c
> @@ -0,0 +1,119 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * AMD Platform Management Framework Driver - Smart PC Capabilities
> + *
> + * Copyright (c) 2023, Advanced Micro Devices, Inc.
> + * All Rights Reserved.
> + *
> + * Authors: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> + * Patil Rajesh Reddy <Patil.Reddy@amd.com>
> + */
> +
> +#include <acpi/button.h>
> +#include <linux/power_supply.h>
> +#include <linux/units.h>
> +#include "pmf.h"
> +
> +static void amd_pmf_get_smu_info(struct amd_pmf_dev *dev, struct ta_pmf_enact_table *in)
> +{
> + u16 max, avg = 0;
> + int i;
> +
> + memset(dev->buf, 0, sizeof(dev->m_table));
> + amd_pmf_send_cmd(dev, SET_TRANSFER_TABLE, 0, 7, NULL);
> + memcpy(&dev->m_table, dev->buf, sizeof(dev->m_table));
> +
> + in->ev_info.socket_power = dev->m_table.apu_power + dev->m_table.dgpu_power;
> + in->ev_info.skin_temperature = dev->m_table.skin_temp;
> +
> + /* get the avg C0 residency of all the cores */
> + for (i = 0; i < ARRAY_SIZE(dev->m_table.avg_core_c0residency); i++)
> + avg += dev->m_table.avg_core_c0residency[i];
> +
> + /* get the max C0 residency of all the cores */
> + max = dev->m_table.avg_core_c0residency[0];
> + for (i = 1; i < ARRAY_SIZE(dev->m_table.avg_core_c0residency); i++) {
> + if (dev->m_table.avg_core_c0residency[i] > max)
> + max = dev->m_table.avg_core_c0residency[i];
> + }
My comments weren't either answered adequately or changes made here.
Please check the v1 comments. I hope it's not because you feel hurry to
get the next version out...
I'm still unsure if the u16 thing can overflow because I don't know what's
the max value for avg_core_c0residency[i].
> +
> + in->ev_info.avg_c0residency = avg / ARRAY_SIZE(dev->m_table.avg_core_c0residency);
> + in->ev_info.max_c0residency = max;
> + in->ev_info.gfx_busy = dev->m_table.avg_gfx_activity;
> +}
> +
> +static const char * const pmf_battery_supply_name[] = {
> + "BATT",
> + "BAT0",
> +};
> +
> +static int get_battery_prop(enum power_supply_property prop)
> +{
> + union power_supply_propval value;
> + struct power_supply *psy;
> + int i, ret = -EINVAL;
> +
> + for (i = 0; i < ARRAY_SIZE(pmf_battery_supply_name); i++) {
> + psy = power_supply_get_by_name(pmf_battery_supply_name[i]);
> + if (!psy)
> + continue;
> +
> + ret = power_supply_get_property(psy, prop, &value);
> + if (ret) {
> + power_supply_put(psy);
> + return ret;
> + }
> + }
> +
> + return value.intval;
> +}
> +
> +static int amd_pmf_get_battery_info(struct amd_pmf_dev *dev, struct ta_pmf_enact_table *in)
> +{
> + int val;
> +
> + val = get_battery_prop(POWER_SUPPLY_PROP_PRESENT);
> + if (val != 1)
> + return -EINVAL;
> +
> + in->ev_info.bat_percentage = get_battery_prop(POWER_SUPPLY_PROP_CAPACITY);
> + /* all values in mWh metrics */
> + in->ev_info.bat_design = get_battery_prop(POWER_SUPPLY_PROP_ENERGY_FULL_DESIGN) / MILLI;
> + in->ev_info.full_charge_capacity = get_battery_prop(POWER_SUPPLY_PROP_ENERGY_FULL) / MILLI;
> + in->ev_info.drain_rate = get_battery_prop(POWER_SUPPLY_PROP_POWER_NOW) / MILLI;
There are defines specifically for watts so you should use them rather
than generic MILLI.
--
i.
^ permalink raw reply
* Re: [PATCH v2 02/16] platform/x86/amd/pmf: Add support PMF-TA interaction
From: Ilpo Järvinen @ 2023-10-04 12:02 UTC (permalink / raw)
To: Shyam Sundar S K
Cc: Hans de Goede, markgross, basavaraj.natikar, jikos,
benjamin.tissoires, alexander.deucher, christian.koenig,
Xinhui.Pan, airlied, daniel, Patil.Reddy, mario.limonciello,
platform-driver-x86, linux-input, amd-gfx, dri-devel
In-Reply-To: <20230930083715.2050863-3-Shyam-sundar.S-k@amd.com>
On Sat, 30 Sep 2023, Shyam Sundar S K wrote:
> PMF TA (Trusted Application) loads via the TEE environment into the
> AMD ASP.
>
> PMF-TA supports two commands:
> 1) Init: Initialize the TA with the PMF Smart PC policy binary and
> start the policy engine. A policy is a combination of inputs and
> outputs, where;
> - the inputs are the changing dynamics of the system like the user
> behaviour, system heuristics etc.
> - the outputs, which are the actions to be set on the system which
> lead to better power management and enhanced user experience.
>
> PMF driver acts as a central manager in this case to supply the
> inputs required to the TA (either by getting the information from
> the other kernel subsystems or from userland)
>
> 2) Enact: Enact the output actions from the TA. The action could be
> applying a new thermal limit to boost/throttle the power limits or
> change system behavior.
>
> Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> ---
> drivers/platform/x86/amd/pmf/pmf.h | 10 +++
> drivers/platform/x86/amd/pmf/tee-if.c | 97 ++++++++++++++++++++++++++-
> 2 files changed, 106 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/platform/x86/amd/pmf/pmf.h b/drivers/platform/x86/amd/pmf/pmf.h
> index 02460c2a31ea..e0837799f521 100644
> --- a/drivers/platform/x86/amd/pmf/pmf.h
> +++ b/drivers/platform/x86/amd/pmf/pmf.h
> @@ -59,6 +59,9 @@
> #define ARG_NONE 0
> #define AVG_SAMPLE_SIZE 3
>
> +/* TA macros */
> +#define PMF_TA_IF_VERSION_MAJOR 1
> +
> /* AMD PMF BIOS interfaces */
> struct apmf_verify_interface {
> u16 size;
> @@ -184,6 +187,7 @@ struct amd_pmf_dev {
> struct tee_shm *fw_shm_pool;
> u32 session_id;
> void *shbuf;
> + struct delayed_work pb_work;
> bool smart_pc_enabled;
> };
>
> @@ -395,6 +399,12 @@ struct apmf_dyn_slider_output {
> struct apmf_cnqf_power_set ps[APMF_CNQF_MAX];
> } __packed;
>
> +/* cmd ids for TA communication */
> +enum ta_pmf_command {
> + TA_PMF_COMMAND_POLICY_BUILDER_INITIALIZE,
> + TA_PMF_COMMAND_POLICY_BUILDER_ENACT_POLICIES,
> +};
> +
> struct ta_pmf_shared_memory {
> int command_id;
> int resp_id;
> diff --git a/drivers/platform/x86/amd/pmf/tee-if.c b/drivers/platform/x86/amd/pmf/tee-if.c
> index 4db80ca59a11..1b3985cd7c08 100644
> --- a/drivers/platform/x86/amd/pmf/tee-if.c
> +++ b/drivers/platform/x86/amd/pmf/tee-if.c
> @@ -13,9 +13,96 @@
> #include "pmf.h"
>
> #define MAX_TEE_PARAM 4
> +
> +/* Policy binary actions sampling frequency (in ms) */
> +static int pb_actions_ms = 1000;
MSEC_PER_SEC (from #include <linux/time.h>, don't include the vdso one).
> +#ifdef CONFIG_AMD_PMF_DEBUG
> +module_param(pb_actions_ms, int, 0644);
> +MODULE_PARM_DESC(pb_actions_ms, "Policy binary actions sampling frequency (default = 1000ms)");
> +#endif
--
i.
^ permalink raw reply
* Re: [PATCH v2 04/16] platform/x86/amd/pmf: Add support for PMF Policy Binary
From: Ilpo Järvinen @ 2023-10-04 12:00 UTC (permalink / raw)
To: Shyam Sundar S K
Cc: Hans de Goede, markgross, basavaraj.natikar, jikos,
benjamin.tissoires, alexander.deucher, christian.koenig,
Xinhui.Pan, airlied, daniel, Patil.Reddy, mario.limonciello,
platform-driver-x86, linux-input, amd-gfx, dri-devel
In-Reply-To: <20230930083715.2050863-5-Shyam-sundar.S-k@amd.com>
On Sat, 30 Sep 2023, Shyam Sundar S K wrote:
> PMF Policy binary is a encrypted and signed binary that will be part
> of the BIOS. PMF driver via the ACPI interface checks the existence
> of Smart PC bit. If the advertised bit is found, PMF driver walks
> the acpi namespace to find out the policy binary size and the address
> which has to be passed to the TA during the TA init sequence.
>
> The policy binary is comprised of inputs (or the events) and outputs
> (or the actions). With the PMF ecosystem, OEMs generate the policy
> binary (or could be multiple binaries) that contains a supported set
> of inputs and outputs which could be specifically carved out for each
> usage segment (or for each user also) that could influence the system
> behavior either by enriching the user experience or/and boost/throttle
> power limits.
>
> Once the TA init command succeeds, the PMF driver sends the changing
> events in the current environment to the TA for a constant sampling
> frequency time (the event here could be a lid close or open) and
> if the policy binary has corresponding action built within it, the
> TA sends the action for it in the subsequent enact command.
>
> If the inputs sent to the TA has no output defined in the policy
> binary generated by OEMs, there will be no action to be performed
> by the PMF driver.
>
> Example policies:
>
> 1) if slider is performance ; set the SPL to 40W
> Here PMF driver registers with the platform profile interface and
> when the slider position is changed, PMF driver lets the TA know
> about this. TA sends back an action to update the Sustained
> Power Limit (SPL). PMF driver updates this limit via the PMFW mailbox.
>
> 2) if user_away ; then lock the system
> Here PMF driver hooks to the AMD SFH driver to know the user presence
> and send the inputs to TA and if the condition is met, the TA sends
> the action of locking the system. PMF driver generates a uevent and
> based on the udev rule in the userland the system gets locked with
> systemctl.
>
> The intent here is to provide the OEM's to make a policy to lock the
> system when the user is away ; but the userland can make a choice to
> ignore it.
>
> and so on.
>
> The OEMs will have an utility to create numerous such policies and
> the policies shall be reviewed by AMD before signing and encrypting
> them. Policies are shared between operating systems to have seemless user
> experience.
>
> Since all this action has to happen via the "amdtee" driver, currently
> there is no caller for it in the kernel which can load the amdtee driver.
> Without amdtee driver loading onto the system the "tee" calls shall fail
> from the PMF driver. Hence an explicit "request_module" has been added
> to address this.
>
> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> ---
> drivers/platform/x86/amd/pmf/Kconfig | 1 +
> drivers/platform/x86/amd/pmf/acpi.c | 37 +++++++
> drivers/platform/x86/amd/pmf/core.c | 12 +++
> drivers/platform/x86/amd/pmf/pmf.h | 135 ++++++++++++++++++++++++
> drivers/platform/x86/amd/pmf/tee-if.c | 141 +++++++++++++++++++++++++-
> 5 files changed, 324 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/platform/x86/amd/pmf/Kconfig b/drivers/platform/x86/amd/pmf/Kconfig
> index 3064bc8ea167..437b78c6d1c5 100644
> --- a/drivers/platform/x86/amd/pmf/Kconfig
> +++ b/drivers/platform/x86/amd/pmf/Kconfig
> @@ -9,6 +9,7 @@ config AMD_PMF
> depends on POWER_SUPPLY
> depends on AMD_NB
> select ACPI_PLATFORM_PROFILE
> + depends on AMDTEE
> help
> This driver provides support for the AMD Platform Management Framework.
> The goal is to enhance end user experience by making AMD PCs smarter,
> diff --git a/drivers/platform/x86/amd/pmf/acpi.c b/drivers/platform/x86/amd/pmf/acpi.c
> index 3fc5e4547d9f..d0512af2cd42 100644
> --- a/drivers/platform/x86/amd/pmf/acpi.c
> +++ b/drivers/platform/x86/amd/pmf/acpi.c
> @@ -286,6 +286,43 @@ int apmf_install_handler(struct amd_pmf_dev *pmf_dev)
> return 0;
> }
>
> +static acpi_status apmf_walk_resources(struct acpi_resource *res, void *data)
> +{
> + struct amd_pmf_dev *dev = data;
> +
> + switch (res->type) {
> + case ACPI_RESOURCE_TYPE_ADDRESS64:
> + dev->policy_addr = res->data.address64.address.minimum;
> + dev->policy_sz = res->data.address64.address.address_length;
> + break;
> + case ACPI_RESOURCE_TYPE_FIXED_MEMORY32:
> + dev->policy_addr = res->data.fixed_memory32.address;
> + dev->policy_sz = res->data.fixed_memory32.address_length;
> + break;
> + }
> +
> + if (!dev->policy_addr || dev->policy_sz > POLICY_BUF_MAX_SZ || dev->policy_sz == 0) {
> + pr_err("Incorrect Policy params, possibly a SBIOS bug\n");
> + return AE_ERROR;
> + }
> +
> + return AE_OK;
> +}
> +
> +int apmf_check_smart_pc(struct amd_pmf_dev *pmf_dev)
> +{
> + acpi_handle ahandle = ACPI_HANDLE(pmf_dev->dev);
> + acpi_status status;
> +
> + status = acpi_walk_resources(ahandle, METHOD_NAME__CRS, apmf_walk_resources, pmf_dev);
> + if (ACPI_FAILURE(status)) {
> + dev_err(pmf_dev->dev, "acpi_walk_resources failed\n");
> + return status;
> + }
> +
> + return 0;
> +}
> +
> void apmf_acpi_deinit(struct amd_pmf_dev *pmf_dev)
> {
> acpi_handle ahandle = ACPI_HANDLE(pmf_dev->dev);
> diff --git a/drivers/platform/x86/amd/pmf/core.c b/drivers/platform/x86/amd/pmf/core.c
> index 678dce4fea08..787f25511191 100644
> --- a/drivers/platform/x86/amd/pmf/core.c
> +++ b/drivers/platform/x86/amd/pmf/core.c
> @@ -384,6 +384,18 @@ static int amd_pmf_probe(struct platform_device *pdev)
> return -ENOMEM;
>
> dev->dev = &pdev->dev;
> + err = apmf_check_smart_pc(dev);
> + if (!err) {
> + /* in order for Smart PC solution to work it has a hard dependency
> + * on the amdtee driver to be loaded first even before the PMF driver
> + * loads. PMF ASL has a _CRS method that advertises the existence
> + * of Smart PC bit. If this information is present, use this to
> + * explicitly probe the amdtee driver, so that "tee" plumbing is done
> + * before the PMF Smart PC init happens.
> + */
But please follow no-text on /* line formatting for multiline comments.
Also start with a capital letter.
> + if (request_module("amdtee"))
Are you aware that this won't give you very strong guarantees about
anything if request_module()'s function comments is to be believed?
If that's all what you're after, MODULE_SOFTDEP("pre: amdtee"); is
probably enough (and I unfortunately don't know the answer how to do it if
you want something stronger than that when you don't directly depend on
the symbols of the other module).
> + pr_err("Failed to load amdtee. PMF Smart PC not enabled!\n");
> + }
>
> rdev = pci_get_domain_bus_and_slot(0, 0, PCI_DEVFN(0, 0));
> if (!rdev || !pci_match_id(pmf_pci_ids, rdev)) {
> diff --git a/drivers/platform/x86/amd/pmf/pmf.h b/drivers/platform/x86/amd/pmf/pmf.h
> index 3930b8ed8333..6f4b6f4ecee4 100644
> --- a/drivers/platform/x86/amd/pmf/pmf.h
> +++ b/drivers/platform/x86/amd/pmf/pmf.h
> @@ -14,6 +14,11 @@
> #include <linux/acpi.h>
> #include <linux/platform_profile.h>
>
> +#define POLICY_BUF_MAX_SZ 0x4b000
> +#define POLICY_SIGN_COOKIE 0x31535024
> +#define POLICY_COOKIE_OFFSET 0x10
> +#define POLICY_COOKIE_LEN 0x14
> +
> /* APMF Functions */
> #define APMF_FUNC_VERIFY_INTERFACE 0
> #define APMF_FUNC_GET_SYS_PARAMS 1
> @@ -59,8 +64,20 @@
> #define ARG_NONE 0
> #define AVG_SAMPLE_SIZE 3
>
> +/* Policy Actions */
> +#define PMF_POLICY_SPL 2
> +#define PMF_POLICY_SPPT 3
> +#define PMF_POLICY_FPPT 4
> +#define PMF_POLICY_SPPT_APU_ONLY 5
> +#define PMF_POLICY_STT_MIN 6
> +#define PMF_POLICY_STT_SKINTEMP_APU 7
> +#define PMF_POLICY_STT_SKINTEMP_HS2 8
> +
> /* TA macros */
> #define PMF_TA_IF_VERSION_MAJOR 1
> +#define TA_PMF_ACTION_MAX 32
> +#define TA_PMF_UNDO_MAX 8
> +#define MAX_OPERATION_PARAMS 4
>
> /* AMD PMF BIOS interfaces */
> struct apmf_verify_interface {
> @@ -183,11 +200,16 @@ struct amd_pmf_dev {
> bool cnqf_supported;
> struct notifier_block pwr_src_notifier;
> /* Smart PC solution builder */
> + unsigned char *policy_buf;
> + u32 policy_sz;
> struct tee_context *tee_ctx;
> struct tee_shm *fw_shm_pool;
> u32 session_id;
> void *shbuf;
> struct delayed_work pb_work;
> + struct pmf_action_table *prev_data;
> + u64 policy_addr;
> + void *policy_base;
> bool smart_pc_enabled;
> };
>
> @@ -399,17 +421,129 @@ struct apmf_dyn_slider_output {
> struct apmf_cnqf_power_set ps[APMF_CNQF_MAX];
> } __packed;
>
> +/* Smart PC - TA internals */
> +enum ta_slider {
> + TA_BEST_BATTERY, /* Best Battery */
> + TA_BETTER_BATTERY, /* Better Battery */
> + TA_BETTER_PERFORMANCE, /* Better Performance */
> + TA_BEST_PERFORMANCE, /* Best Performance */
Align comments.
> + TA_MAX,
> +};
> +
> /* cmd ids for TA communication */
> enum ta_pmf_command {
> TA_PMF_COMMAND_POLICY_BUILDER_INITIALIZE,
> TA_PMF_COMMAND_POLICY_BUILDER_ENACT_POLICIES,
> };
>
> +enum ta_pmf_error_type {
> + TA_PMF_TYPE_SUCCESS,
> + TA_PMF_ERROR_TYPE_GENERIC,
> + TA_PMF_ERROR_TYPE_CRYPTO,
> + TA_PMF_ERROR_TYPE_CRYPTO_VALIDATE,
> + TA_PMF_ERROR_TYPE_CRYPTO_VERIFY_OEM,
> + TA_PMF_ERROR_TYPE_POLICY_BUILDER,
> + TA_PMF_ERROR_TYPE_PB_CONVERT,
> + TA_PMF_ERROR_TYPE_PB_SETUP,
> + TA_PMF_ERROR_TYPE_PB_ENACT,
> + TA_PMF_ERROR_TYPE_ASD_GET_DEVICE_INFO,
> + TA_PMF_ERROR_TYPE_ASD_GET_DEVICE_PCIE_INFO,
> + TA_PMF_ERROR_TYPE_SYS_DRV_FW_VALIDATION,
> + TA_PMF_ERROR_TYPE_MAX,
> +};
> +
> +struct pmf_action_table {
> + unsigned long spl; /* in mW */
> + unsigned long sppt; /* in mW */
> + unsigned long sppt_apuonly; /* in mW */
> + unsigned long fppt; /* in mW */
> + unsigned long stt_minlimit; /* in mW */
> + unsigned long stt_skintemp_apu; /* in C */
> + unsigned long stt_skintemp_hs2; /* in C */
Ditto.
> +};
> +
> +/* Input conditions */
> +struct ta_pmf_condition_info {
> + u32 power_source;
> + u32 bat_percentage;
> + u32 power_slider;
> + u32 lid_state;
> + bool user_present;
> + u32 rsvd1[2];
> + u32 monitor_count;
> + u32 rsvd2[2];
> + u32 bat_design;
> + u32 full_charge_capacity;
> + int drain_rate;
> + bool user_engaged;
> + u32 device_state;
> + u32 socket_power;
> + u32 skin_temperature;
> + u32 rsvd3[5];
> + u32 ambient_light;
> + u32 length;
> + u32 avg_c0residency;
> + u32 max_c0residency;
> + u32 s0i3_entry;
> + u32 gfx_busy;
> + u32 rsvd4[7];
> + bool camera_state;
> + u32 workload_type;
> + u32 display_type;
> + u32 display_state;
> + u32 rsvd5[150];
> +};
> +
> +struct ta_pmf_load_policy_table {
> + u32 table_size;
> + u8 table[POLICY_BUF_MAX_SZ];
> +};
> +
> +/* TA initialization params */
> +struct ta_pmf_init_table {
> + u32 frequency; /* SMU sampling frequency */
> + bool validate;
> + bool sku_check;
> + bool metadata_macrocheck;
> + struct ta_pmf_load_policy_table policies_table;
> +};
> +
> +/* Everything the TA needs to Enact Policies */
> +struct ta_pmf_enact_table {
> + struct ta_pmf_condition_info ev_info;
> + u32 name;
> +};
> +
> +struct ta_pmf_action {
> + u32 action_index;
> + u32 value;
> +};
> +
> +/* output actions from TA */
> +struct ta_pmf_enact_result {
> + u32 actions_count;
> + struct ta_pmf_action actions_list[TA_PMF_ACTION_MAX];
> + u32 undo_count;
> + struct ta_pmf_action undo_list[TA_PMF_UNDO_MAX];
> +};
> +
> +union ta_pmf_input {
> + struct ta_pmf_enact_table enact_table;
> + struct ta_pmf_init_table init_table;
> +};
> +
> +union ta_pmf_output {
> + struct ta_pmf_enact_result policy_apply_table;
> + u32 rsvd[906];
This is some size (SZ_1K?) - sizeof(ta_pmf_enact_result)? I don't know if
compiler would like such a construct though in the array declaration. If
the compiler isn't complaining it would be the most informative way to
state the size but if it's not happy, a comment might be useful.
> +};
> +
> struct ta_pmf_shared_memory {
> int command_id;
> int resp_id;
> u32 pmf_result;
> u32 if_version;
> + union ta_pmf_output pmf_output;
> + union ta_pmf_input pmf_input;
> };
>
> /* Core Layer */
> @@ -460,4 +594,5 @@ extern const struct attribute_group cnqf_feature_attribute_group;
> /* Smart PC builder Layer*/
> int amd_pmf_init_smart_pc(struct amd_pmf_dev *dev);
> void amd_pmf_deinit_smart_pc(struct amd_pmf_dev *dev);
> +int apmf_check_smart_pc(struct amd_pmf_dev *pmf_dev);
> #endif /* PMF_H */
> diff --git a/drivers/platform/x86/amd/pmf/tee-if.c b/drivers/platform/x86/amd/pmf/tee-if.c
> index 1b3985cd7c08..15aa6e6e1050 100644
> --- a/drivers/platform/x86/amd/pmf/tee-if.c
> +++ b/drivers/platform/x86/amd/pmf/tee-if.c
> @@ -42,9 +42,77 @@ static void amd_pmf_prepare_args(struct amd_pmf_dev *dev, int cmd,
> param[0].u.memref.shm_offs = 0;
> }
>
> +static void amd_pmf_apply_policies(struct amd_pmf_dev *dev, struct ta_pmf_enact_result *out)
> +{
> + unsigned long val;
> + int idx;
> +
> + for (idx = 0; idx < out->actions_count; idx++) {
> + val = out->actions_list[idx].value;
> + switch (out->actions_list[idx].action_index) {
> + case PMF_POLICY_SPL:
> + if (dev->prev_data->spl != val) {
> + amd_pmf_send_cmd(dev, SET_SPL, false, val, NULL);
> + dev_dbg(dev->dev, "update SPL : %lu\n", val);
> + dev->prev_data->spl = val;
Well, I'd have expected you to go u32 (and %u) here as isn't
out->actions_list[idx].value u32? And amd_pmf_send_cmd also takes u32.
So unsigned long looks quite inconsistent and wasteful.
> + }
> + break;
> +
> + case PMF_POLICY_SPPT:
> + if (dev->prev_data->sppt != val) {
> + amd_pmf_send_cmd(dev, SET_SPPT, false, val, NULL);
> + dev_dbg(dev->dev, "update SPPT : %lu\n", val);
> + dev->prev_data->sppt = val;
> + }
> + break;
> +
> + case PMF_POLICY_FPPT:
> + if (dev->prev_data->fppt != val) {
> + amd_pmf_send_cmd(dev, SET_FPPT, false, val, NULL);
> + dev_dbg(dev->dev, "update FPPT : %lu\n", val);
> + dev->prev_data->fppt = val;
> + }
> + break;
> +
> + case PMF_POLICY_SPPT_APU_ONLY:
> + if (dev->prev_data->sppt_apuonly != val) {
> + amd_pmf_send_cmd(dev, SET_SPPT_APU_ONLY, false, val, NULL);
> + dev_dbg(dev->dev, "update SPPT_APU_ONLY : %lu\n", val);
> + dev->prev_data->sppt_apuonly = val;
> + }
> + break;
> +
> + case PMF_POLICY_STT_MIN:
> + if (dev->prev_data->stt_minlimit != val) {
> + amd_pmf_send_cmd(dev, SET_STT_MIN_LIMIT, false, val, NULL);
> + dev_dbg(dev->dev, "update STT_MIN : %lu\n", val);
> + dev->prev_data->stt_minlimit = val;
> + }
> + break;
> +
> + case PMF_POLICY_STT_SKINTEMP_APU:
> + if (dev->prev_data->stt_skintemp_apu != val) {
> + amd_pmf_send_cmd(dev, SET_STT_LIMIT_APU, false, val, NULL);
> + dev_dbg(dev->dev, "update STT_SKINTEMP_APU : %lu\n", val);
> + dev->prev_data->stt_skintemp_apu = val;
> + }
> + break;
> +
> + case PMF_POLICY_STT_SKINTEMP_HS2:
> + if (dev->prev_data->stt_skintemp_hs2 != val) {
> + amd_pmf_send_cmd(dev, SET_STT_LIMIT_HS2, false, val, NULL);
> + dev_dbg(dev->dev, "update STT_SKINTEMP_HS2 : %lu\n", val);
> + dev->prev_data->stt_skintemp_hs2 = val;
> + }
> + break;
> + }
> + }
> +}
> +
> static int amd_pmf_invoke_cmd_enact(struct amd_pmf_dev *dev)
> {
> struct ta_pmf_shared_memory *ta_sm = NULL;
> + struct ta_pmf_enact_result *out = NULL;
> struct tee_param param[MAX_TEE_PARAM];
> struct tee_ioctl_invoke_arg arg;
> int ret = 0;
> @@ -52,7 +120,10 @@ static int amd_pmf_invoke_cmd_enact(struct amd_pmf_dev *dev)
> if (!dev->tee_ctx)
> return -ENODEV;
>
> + memset(dev->shbuf, 0, dev->policy_sz);
> ta_sm = (struct ta_pmf_shared_memory *)dev->shbuf;
> + out = &ta_sm->pmf_output.policy_apply_table;
> +
> memset(ta_sm, 0, sizeof(struct ta_pmf_shared_memory));
> ta_sm->command_id = TA_PMF_COMMAND_POLICY_BUILDER_ENACT_POLICIES;
> ta_sm->if_version = PMF_TA_IF_VERSION_MAJOR;
> @@ -65,6 +136,12 @@ static int amd_pmf_invoke_cmd_enact(struct amd_pmf_dev *dev)
> return -EINVAL;
> }
>
> + if (ta_sm->pmf_result == TA_PMF_TYPE_SUCCESS && out->actions_count) {
> + dev_dbg(dev->dev, "action count:%u result:%x\n", out->actions_count,
> + ta_sm->pmf_result);
> + amd_pmf_apply_policies(dev, out);
> + }
> +
> return 0;
> }
>
> @@ -72,6 +149,7 @@ static int amd_pmf_invoke_cmd_init(struct amd_pmf_dev *dev)
> {
> struct ta_pmf_shared_memory *ta_sm = NULL;
> struct tee_param param[MAX_TEE_PARAM];
> + struct ta_pmf_init_table *in = NULL;
> struct tee_ioctl_invoke_arg arg;
> int ret = 0;
>
> @@ -80,10 +158,20 @@ static int amd_pmf_invoke_cmd_init(struct amd_pmf_dev *dev)
> return -ENODEV;
> }
>
> + dev_dbg(dev->dev, "Policy Binary size: %u bytes\n", dev->policy_sz);
> + memset(dev->shbuf, 0, dev->policy_sz);
> ta_sm = (struct ta_pmf_shared_memory *)dev->shbuf;
> + in = &ta_sm->pmf_input.init_table;
> +
> ta_sm->command_id = TA_PMF_COMMAND_POLICY_BUILDER_INITIALIZE;
> ta_sm->if_version = PMF_TA_IF_VERSION_MAJOR;
> + in->metadata_macrocheck = false;
> + in->sku_check = false;
> + in->validate = true;
> + in->frequency = pb_actions_ms;
> + in->policies_table.table_size = dev->policy_sz;
>
> + memcpy(in->policies_table.table, dev->policy_buf, dev->policy_sz);
> amd_pmf_prepare_args(dev, TA_PMF_COMMAND_POLICY_BUILDER_INITIALIZE, &arg, param);
>
> ret = tee_client_invoke_func(dev->tee_ctx, &arg, param);
> @@ -103,6 +191,47 @@ static void amd_pmf_invoke_cmd(struct work_struct *work)
> schedule_delayed_work(&dev->pb_work, msecs_to_jiffies(pb_actions_ms));
> }
>
> +static int amd_pmf_start_policy_engine(struct amd_pmf_dev *dev)
> +{
> + u32 cookie, length;
> + int res;
> +
> + cookie = readl(dev->policy_buf + POLICY_COOKIE_OFFSET);
> + length = readl(dev->policy_buf + POLICY_COOKIE_LEN);
> +
> + if (cookie != POLICY_SIGN_COOKIE || !length)
> + return -EINVAL;
> +
> + /* update the actual length */
> + dev->policy_sz = length + 512;
> + res = amd_pmf_invoke_cmd_init(dev);
> + if (res == TA_PMF_TYPE_SUCCESS) {
> + /* now its safe to announce that smart pc is enabled */
> + dev->smart_pc_enabled = 1;
> + schedule_delayed_work(&dev->pb_work, msecs_to_jiffies(pb_actions_ms * 3));
Why * 3? Explain in comment if you feel it's necessary as it's not
obvious.
--
i.
^ permalink raw reply
* Re: [PATCH v2 01/16] platform/x86/amd/pmf: Add PMF TEE interface
From: Ilpo Järvinen @ 2023-10-04 11:27 UTC (permalink / raw)
To: Shyam Sundar S K
Cc: Hans de Goede, markgross, basavaraj.natikar, jikos,
benjamin.tissoires, alexander.deucher, christian.koenig,
Xinhui.Pan, airlied, daniel, Patil.Reddy, mario.limonciello,
platform-driver-x86, linux-input, amd-gfx, dri-devel
In-Reply-To: <20230930083715.2050863-2-Shyam-sundar.S-k@amd.com>
On Sat, 30 Sep 2023, Shyam Sundar S K wrote:
> AMD PMF driver loads the PMF TA (Trusted Application) into the AMD
> ASP's (AMD Security Processor) TEE (Trusted Execution Environment).
>
> PMF Trusted Application is a secured firmware placed under
> /lib/firmware/amdtee gets loaded only when the TEE environment is
> initialized. Add the initial code path to build these pipes.
>
> Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> ---
> drivers/platform/x86/amd/pmf/Makefile | 3 +-
> drivers/platform/x86/amd/pmf/core.c | 11 ++-
> drivers/platform/x86/amd/pmf/pmf.h | 16 ++++
> drivers/platform/x86/amd/pmf/tee-if.c | 112 ++++++++++++++++++++++++++
> 4 files changed, 138 insertions(+), 4 deletions(-)
> create mode 100644 drivers/platform/x86/amd/pmf/tee-if.c
>
> diff --git a/drivers/platform/x86/amd/pmf/Makefile b/drivers/platform/x86/amd/pmf/Makefile
> index fdededf54392..d2746ee7369f 100644
> --- a/drivers/platform/x86/amd/pmf/Makefile
> +++ b/drivers/platform/x86/amd/pmf/Makefile
> @@ -6,4 +6,5 @@
>
> obj-$(CONFIG_AMD_PMF) += amd-pmf.o
> amd-pmf-objs := core.o acpi.o sps.o \
> - auto-mode.o cnqf.o
> + auto-mode.o cnqf.o \
> + tee-if.o
> diff --git a/drivers/platform/x86/amd/pmf/core.c b/drivers/platform/x86/amd/pmf/core.c
> index 78ed3ee22555..68f1389dda3e 100644
> --- a/drivers/platform/x86/amd/pmf/core.c
> +++ b/drivers/platform/x86/amd/pmf/core.c
> @@ -309,8 +309,11 @@ static void amd_pmf_init_features(struct amd_pmf_dev *dev)
> dev_dbg(dev->dev, "SPS enabled and Platform Profiles registered\n");
> }
>
> - /* Enable Auto Mode */
> - if (is_apmf_func_supported(dev, APMF_FUNC_AUTO_MODE)) {
> + if (amd_pmf_init_smart_pc(dev)) {
> + /* Enable Smart PC Solution builder */
> + dev_dbg(dev->dev, "Smart PC Solution Enabled\n");
> + } else if (is_apmf_func_supported(dev, APMF_FUNC_AUTO_MODE)) {
> + /* Enable Auto Mode */
> amd_pmf_init_auto_mode(dev);
> dev_dbg(dev->dev, "Auto Mode Init done\n");
> } else if (is_apmf_func_supported(dev, APMF_FUNC_DYN_SLIDER_AC) ||
> @@ -330,7 +333,9 @@ static void amd_pmf_deinit_features(struct amd_pmf_dev *dev)
> amd_pmf_deinit_sps(dev);
> }
>
> - if (is_apmf_func_supported(dev, APMF_FUNC_AUTO_MODE)) {
> + if (dev->smart_pc_enabled) {
> + amd_pmf_deinit_smart_pc(dev);
> + } else if (is_apmf_func_supported(dev, APMF_FUNC_AUTO_MODE)) {
> amd_pmf_deinit_auto_mode(dev);
> } else if (is_apmf_func_supported(dev, APMF_FUNC_DYN_SLIDER_AC) ||
> is_apmf_func_supported(dev, APMF_FUNC_DYN_SLIDER_DC)) {
> diff --git a/drivers/platform/x86/amd/pmf/pmf.h b/drivers/platform/x86/amd/pmf/pmf.h
> index deba88e6e4c8..02460c2a31ea 100644
> --- a/drivers/platform/x86/amd/pmf/pmf.h
> +++ b/drivers/platform/x86/amd/pmf/pmf.h
> @@ -179,6 +179,12 @@ struct amd_pmf_dev {
> bool cnqf_enabled;
> bool cnqf_supported;
> struct notifier_block pwr_src_notifier;
> + /* Smart PC solution builder */
> + struct tee_context *tee_ctx;
> + struct tee_shm *fw_shm_pool;
> + u32 session_id;
> + void *shbuf;
> + bool smart_pc_enabled;
> };
>
> struct apmf_sps_prop_granular {
> @@ -389,6 +395,13 @@ struct apmf_dyn_slider_output {
> struct apmf_cnqf_power_set ps[APMF_CNQF_MAX];
> } __packed;
>
> +struct ta_pmf_shared_memory {
> + int command_id;
> + int resp_id;
> + u32 pmf_result;
> + u32 if_version;
> +};
> +
> /* Core Layer */
> int apmf_acpi_init(struct amd_pmf_dev *pmf_dev);
> void apmf_acpi_deinit(struct amd_pmf_dev *pmf_dev);
> @@ -433,4 +446,7 @@ void amd_pmf_deinit_cnqf(struct amd_pmf_dev *dev);
> int amd_pmf_trans_cnqf(struct amd_pmf_dev *dev, int socket_power, ktime_t time_lapsed_ms);
> extern const struct attribute_group cnqf_feature_attribute_group;
>
> +/* Smart PC builder Layer*/
> +int amd_pmf_init_smart_pc(struct amd_pmf_dev *dev);
> +void amd_pmf_deinit_smart_pc(struct amd_pmf_dev *dev);
> #endif /* PMF_H */
> diff --git a/drivers/platform/x86/amd/pmf/tee-if.c b/drivers/platform/x86/amd/pmf/tee-if.c
> new file mode 100644
> index 000000000000..4db80ca59a11
> --- /dev/null
> +++ b/drivers/platform/x86/amd/pmf/tee-if.c
> @@ -0,0 +1,112 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * AMD Platform Management Framework Driver - TEE Interface
> + *
> + * Copyright (c) 2023, Advanced Micro Devices, Inc.
> + * All Rights Reserved.
> + *
> + * Author: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> + */
> +
> +#include <linux/tee_drv.h>
> +#include <linux/uuid.h>
> +#include "pmf.h"
> +
> +#define MAX_TEE_PARAM 4
> +static const uuid_t amd_pmf_ta_uuid = UUID_INIT(0x6fd93b77, 0x3fb8, 0x524d,
> + 0xb1, 0x2d, 0xc5, 0x29, 0xb1, 0x3d, 0x85, 0x43);
> +
> +static int amd_pmf_amdtee_ta_match(struct tee_ioctl_version_data *ver, const void *data)
> +{
> + return ver->impl_id == TEE_IMPL_ID_AMDTEE;
> +}
> +
> +static int amd_pmf_ta_open_session(struct tee_context *ctx, u32 *id)
> +{
> + struct tee_ioctl_open_session_arg sess_arg = {};
> + int rc;
> +
> + export_uuid(sess_arg.uuid, &amd_pmf_ta_uuid);
> + sess_arg.clnt_login = TEE_IOCTL_LOGIN_PUBLIC;
> + sess_arg.num_params = 0;
> +
> + rc = tee_client_open_session(ctx, &sess_arg, NULL);
This will probably fail build if CONFIG_TEE is not set as nothing
is added here to require it?
--
i.
^ permalink raw reply
* Re: [PATCH v2 03/16] platform/x86/amd/pmf: Change return type of amd_pmf_set_dram_addr()
From: Ilpo Järvinen @ 2023-10-04 11:03 UTC (permalink / raw)
To: Shyam Sundar S K
Cc: Hans de Goede, markgross, basavaraj.natikar, jikos,
benjamin.tissoires, alexander.deucher, christian.koenig,
Xinhui.Pan, airlied, daniel, Patil.Reddy, mario.limonciello,
platform-driver-x86, linux-input, amd-gfx, dri-devel
In-Reply-To: <20230930083715.2050863-4-Shyam-sundar.S-k@amd.com>
On Sat, 30 Sep 2023, Shyam Sundar S K wrote:
> In the current code, the metrics table information was required only
> for auto-mode or CnQF at a given time. Hence keeping the return type
> of amd_pmf_set_dram_addr() as static made sense.
>
> But with the addition of Smart PC builder feature, the metrics table
> information has to be shared by the Smart PC also and this feature
> resides outside of core.c.
>
> To make amd_pmf_set_dram_addr() visible outside of core.c make it
> as a non-static function and move the allocation of memory for
> metrics table from amd_pmf_init_metrics_table() to amd_pmf_set_dram_addr()
> as amd_pmf_set_dram_addr() is the common function to set the DRAM
> address.
>
> Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> ---
> drivers/platform/x86/amd/pmf/core.c | 26 ++++++++++++++++++--------
> drivers/platform/x86/amd/pmf/pmf.h | 1 +
> 2 files changed, 19 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/platform/x86/amd/pmf/core.c b/drivers/platform/x86/amd/pmf/core.c
> index 68f1389dda3e..678dce4fea08 100644
> --- a/drivers/platform/x86/amd/pmf/core.c
> +++ b/drivers/platform/x86/amd/pmf/core.c
> @@ -251,29 +251,35 @@ static const struct pci_device_id pmf_pci_ids[] = {
> { }
> };
>
> -static void amd_pmf_set_dram_addr(struct amd_pmf_dev *dev)
> +int amd_pmf_set_dram_addr(struct amd_pmf_dev *dev)
> {
> u64 phys_addr;
> u32 hi, low;
>
> + /* Get Metrics Table Address */
> + dev->buf = kzalloc(sizeof(dev->m_table), GFP_KERNEL);
> + if (!dev->buf)
> + return -ENOMEM;
> +
> phys_addr = virt_to_phys(dev->buf);
> hi = phys_addr >> 32;
> low = phys_addr & GENMASK(31, 0);
>
> amd_pmf_send_cmd(dev, SET_DRAM_ADDR_HIGH, 0, hi, NULL);
> amd_pmf_send_cmd(dev, SET_DRAM_ADDR_LOW, 0, low, NULL);
> +
> + return 0;
> }
>
> int amd_pmf_init_metrics_table(struct amd_pmf_dev *dev)
> {
> - /* Get Metrics Table Address */
> - dev->buf = kzalloc(sizeof(dev->m_table), GFP_KERNEL);
> - if (!dev->buf)
> - return -ENOMEM;
> + int ret;
>
> INIT_DELAYED_WORK(&dev->work_buffer, amd_pmf_get_metrics);
>
> - amd_pmf_set_dram_addr(dev);
> + ret = amd_pmf_set_dram_addr(dev);
> + if (ret)
> + return ret;
>
> /*
> * Start collecting the metrics data after a small delay
> @@ -287,9 +293,13 @@ int amd_pmf_init_metrics_table(struct amd_pmf_dev *dev)
> static int amd_pmf_resume_handler(struct device *dev)
> {
> struct amd_pmf_dev *pdev = dev_get_drvdata(dev);
> + int ret;
>
> - if (pdev->buf)
> - amd_pmf_set_dram_addr(pdev);
> + if (pdev->buf) {
> + ret = amd_pmf_set_dram_addr(pdev);
Won't this now leak the previous ->buf?
> + if (ret)
> + return ret;
> + }
>
> return 0;
> }
> diff --git a/drivers/platform/x86/amd/pmf/pmf.h b/drivers/platform/x86/amd/pmf/pmf.h
> index e0837799f521..3930b8ed8333 100644
> --- a/drivers/platform/x86/amd/pmf/pmf.h
> +++ b/drivers/platform/x86/amd/pmf/pmf.h
> @@ -421,6 +421,7 @@ int amd_pmf_init_metrics_table(struct amd_pmf_dev *dev);
> int amd_pmf_get_power_source(void);
> int apmf_install_handler(struct amd_pmf_dev *pmf_dev);
> int apmf_os_power_slider_update(struct amd_pmf_dev *dev, u8 flag);
> +int amd_pmf_set_dram_addr(struct amd_pmf_dev *dev);
>
> /* SPS Layer */
> int amd_pmf_get_pprof_modes(struct amd_pmf_dev *pmf);
>
--
i.
^ permalink raw reply
* Re: [PATCH v2 02/16] platform/x86/amd/pmf: Add support PMF-TA interaction
From: Ilpo Järvinen @ 2023-10-04 10:56 UTC (permalink / raw)
To: Shyam Sundar S K
Cc: Hans de Goede, markgross, basavaraj.natikar, jikos,
benjamin.tissoires, alexander.deucher, christian.koenig,
Xinhui.Pan, airlied, daniel, Patil.Reddy, mario.limonciello,
platform-driver-x86, linux-input, amd-gfx, dri-devel
In-Reply-To: <20230930083715.2050863-3-Shyam-sundar.S-k@amd.com>
On Sat, 30 Sep 2023, Shyam Sundar S K wrote:
> PMF TA (Trusted Application) loads via the TEE environment into the
> AMD ASP.
>
> PMF-TA supports two commands:
> 1) Init: Initialize the TA with the PMF Smart PC policy binary and
> start the policy engine. A policy is a combination of inputs and
> outputs, where;
> - the inputs are the changing dynamics of the system like the user
> behaviour, system heuristics etc.
> - the outputs, which are the actions to be set on the system which
> lead to better power management and enhanced user experience.
>
> PMF driver acts as a central manager in this case to supply the
> inputs required to the TA (either by getting the information from
> the other kernel subsystems or from userland)
>
> 2) Enact: Enact the output actions from the TA. The action could be
> applying a new thermal limit to boost/throttle the power limits or
> change system behavior.
>
> Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> ---
> drivers/platform/x86/amd/pmf/pmf.h | 10 +++
> drivers/platform/x86/amd/pmf/tee-if.c | 97 ++++++++++++++++++++++++++-
> 2 files changed, 106 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/platform/x86/amd/pmf/pmf.h b/drivers/platform/x86/amd/pmf/pmf.h
> index 02460c2a31ea..e0837799f521 100644
> --- a/drivers/platform/x86/amd/pmf/pmf.h
> +++ b/drivers/platform/x86/amd/pmf/pmf.h
> @@ -59,6 +59,9 @@
> #define ARG_NONE 0
> #define AVG_SAMPLE_SIZE 3
>
> +/* TA macros */
> +#define PMF_TA_IF_VERSION_MAJOR 1
> +
> /* AMD PMF BIOS interfaces */
> struct apmf_verify_interface {
> u16 size;
> @@ -184,6 +187,7 @@ struct amd_pmf_dev {
> struct tee_shm *fw_shm_pool;
> u32 session_id;
> void *shbuf;
> + struct delayed_work pb_work;
> bool smart_pc_enabled;
> };
>
> @@ -395,6 +399,12 @@ struct apmf_dyn_slider_output {
> struct apmf_cnqf_power_set ps[APMF_CNQF_MAX];
> } __packed;
>
> +/* cmd ids for TA communication */
> +enum ta_pmf_command {
> + TA_PMF_COMMAND_POLICY_BUILDER_INITIALIZE,
> + TA_PMF_COMMAND_POLICY_BUILDER_ENACT_POLICIES,
> +};
> +
> struct ta_pmf_shared_memory {
> int command_id;
> int resp_id;
> diff --git a/drivers/platform/x86/amd/pmf/tee-if.c b/drivers/platform/x86/amd/pmf/tee-if.c
> index 4db80ca59a11..1b3985cd7c08 100644
> --- a/drivers/platform/x86/amd/pmf/tee-if.c
> +++ b/drivers/platform/x86/amd/pmf/tee-if.c
> @@ -13,9 +13,96 @@
> #include "pmf.h"
>
> #define MAX_TEE_PARAM 4
> +
> +/* Policy binary actions sampling frequency (in ms) */
> +static int pb_actions_ms = 1000;
> +#ifdef CONFIG_AMD_PMF_DEBUG
> +module_param(pb_actions_ms, int, 0644);
> +MODULE_PARM_DESC(pb_actions_ms, "Policy binary actions sampling frequency (default = 1000ms)");
> +#endif
> +
> static const uuid_t amd_pmf_ta_uuid = UUID_INIT(0x6fd93b77, 0x3fb8, 0x524d,
> 0xb1, 0x2d, 0xc5, 0x29, 0xb1, 0x3d, 0x85, 0x43);
>
> +static void amd_pmf_prepare_args(struct amd_pmf_dev *dev, int cmd,
> + struct tee_ioctl_invoke_arg *arg,
> + struct tee_param *param)
> +{
> + memset(arg, 0, sizeof(*arg));
> + memset(param, 0, MAX_TEE_PARAM * sizeof(*param));
> +
> + arg->func = cmd;
> + arg->session = dev->session_id;
> + arg->num_params = MAX_TEE_PARAM;
> +
> + /* Fill invoke cmd params */
> + param[0].u.memref.size = sizeof(struct ta_pmf_shared_memory);
> + param[0].attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INOUT;
> + param[0].u.memref.shm = dev->fw_shm_pool;
> + param[0].u.memref.shm_offs = 0;
> +}
> +
> +static int amd_pmf_invoke_cmd_enact(struct amd_pmf_dev *dev)
> +{
> + struct ta_pmf_shared_memory *ta_sm = NULL;
> + struct tee_param param[MAX_TEE_PARAM];
> + struct tee_ioctl_invoke_arg arg;
> + int ret = 0;
> +
> + if (!dev->tee_ctx)
> + return -ENODEV;
> +
> + ta_sm = (struct ta_pmf_shared_memory *)dev->shbuf;
Don't cast from void * to a typed pointer, it's unnecessary as compiler
will handle that for you.
> + memset(ta_sm, 0, sizeof(struct ta_pmf_shared_memory));
sizeof(*ta_sm) to be on the safer side of things.
> + ta_sm->command_id = TA_PMF_COMMAND_POLICY_BUILDER_ENACT_POLICIES;
> + ta_sm->if_version = PMF_TA_IF_VERSION_MAJOR;
> +
> + amd_pmf_prepare_args(dev, TA_PMF_COMMAND_POLICY_BUILDER_ENACT_POLICIES, &arg, param);
> +
> + ret = tee_client_invoke_func(dev->tee_ctx, &arg, param);
> + if (ret < 0 || arg.ret != 0) {
> + dev_err(dev->dev, "TEE enact cmd failed. err: %x, ret:%x\n", arg.ret, ret);
-Exx code should be printed as %x
> + return -EINVAL;
This overrides the original error code if ret < 0.
> + }
> +
> + return 0;
> +}
> +
> +static int amd_pmf_invoke_cmd_init(struct amd_pmf_dev *dev)
> +{
> + struct ta_pmf_shared_memory *ta_sm = NULL;
> + struct tee_param param[MAX_TEE_PARAM];
> + struct tee_ioctl_invoke_arg arg;
> + int ret = 0;
> +
> + if (!dev->tee_ctx) {
> + dev_err(dev->dev, "Failed to get TEE context\n");
> + return -ENODEV;
> + }
> +
> + ta_sm = (struct ta_pmf_shared_memory *)dev->shbuf;
Unnecessary cast from void *.
> + ta_sm->command_id = TA_PMF_COMMAND_POLICY_BUILDER_INITIALIZE;
> + ta_sm->if_version = PMF_TA_IF_VERSION_MAJOR;
> +
> + amd_pmf_prepare_args(dev, TA_PMF_COMMAND_POLICY_BUILDER_INITIALIZE, &arg, param);
> +
> + ret = tee_client_invoke_func(dev->tee_ctx, &arg, param);
> + if (ret < 0 || arg.ret != 0) {
> + dev_err(dev->dev, "Failed to invoke TEE init cmd. err: %x, ret:%x\n", arg.ret, ret);
> + return -EINVAL;
As per earlier comments (x2).
> + }
> +
> + return ta_sm->pmf_result;
> +}
> +
> +static void amd_pmf_invoke_cmd(struct work_struct *work)
> +{
> + struct amd_pmf_dev *dev = container_of(work, struct amd_pmf_dev, pb_work.work);
> +
> + amd_pmf_invoke_cmd_enact(dev);
> + schedule_delayed_work(&dev->pb_work, msecs_to_jiffies(pb_actions_ms));
> +}
> +
> static int amd_pmf_amdtee_ta_match(struct tee_ioctl_version_data *ver, const void *data)
> {
> return ver->impl_id == TEE_IMPL_ID_AMDTEE;
> @@ -103,10 +190,18 @@ static void amd_pmf_tee_deinit(struct amd_pmf_dev *dev)
>
> int amd_pmf_init_smart_pc(struct amd_pmf_dev *dev)
> {
> - return amd_pmf_tee_init(dev);
> + int ret;
> +
> + ret = amd_pmf_tee_init(dev);
> + if (ret)
> + return ret;
> +
> + INIT_DELAYED_WORK(&dev->pb_work, amd_pmf_invoke_cmd);
> + return 0;
> }
>
> void amd_pmf_deinit_smart_pc(struct amd_pmf_dev *dev)
> {
> + cancel_delayed_work_sync(&dev->pb_work);
> amd_pmf_tee_deinit(dev);
> }
>
--
i.
^ permalink raw reply
* Re: [PATCH v2 01/16] platform/x86/amd/pmf: Add PMF TEE interface
From: Ilpo Järvinen @ 2023-10-04 10:50 UTC (permalink / raw)
To: Shyam Sundar S K
Cc: Hans de Goede, markgross, basavaraj.natikar, jikos,
benjamin.tissoires, alexander.deucher, christian.koenig,
Xinhui.Pan, airlied, daniel, Patil.Reddy, mario.limonciello,
platform-driver-x86, linux-input, amd-gfx, dri-devel
In-Reply-To: <20230930083715.2050863-2-Shyam-sundar.S-k@amd.com>
On Sat, 30 Sep 2023, Shyam Sundar S K wrote:
> AMD PMF driver loads the PMF TA (Trusted Application) into the AMD
> ASP's (AMD Security Processor) TEE (Trusted Execution Environment).
>
> PMF Trusted Application is a secured firmware placed under
> /lib/firmware/amdtee gets loaded only when the TEE environment is
> initialized. Add the initial code path to build these pipes.
>
> Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> ---
> drivers/platform/x86/amd/pmf/Makefile | 3 +-
> drivers/platform/x86/amd/pmf/core.c | 11 ++-
> drivers/platform/x86/amd/pmf/pmf.h | 16 ++++
> drivers/platform/x86/amd/pmf/tee-if.c | 112 ++++++++++++++++++++++++++
> 4 files changed, 138 insertions(+), 4 deletions(-)
> create mode 100644 drivers/platform/x86/amd/pmf/tee-if.c
>
> diff --git a/drivers/platform/x86/amd/pmf/Makefile b/drivers/platform/x86/amd/pmf/Makefile
> index fdededf54392..d2746ee7369f 100644
> --- a/drivers/platform/x86/amd/pmf/Makefile
> +++ b/drivers/platform/x86/amd/pmf/Makefile
> @@ -6,4 +6,5 @@
>
> obj-$(CONFIG_AMD_PMF) += amd-pmf.o
> amd-pmf-objs := core.o acpi.o sps.o \
> - auto-mode.o cnqf.o
> + auto-mode.o cnqf.o \
> + tee-if.o
> diff --git a/drivers/platform/x86/amd/pmf/core.c b/drivers/platform/x86/amd/pmf/core.c
> index 78ed3ee22555..68f1389dda3e 100644
> --- a/drivers/platform/x86/amd/pmf/core.c
> +++ b/drivers/platform/x86/amd/pmf/core.c
> @@ -309,8 +309,11 @@ static void amd_pmf_init_features(struct amd_pmf_dev *dev)
> dev_dbg(dev->dev, "SPS enabled and Platform Profiles registered\n");
> }
>
> - /* Enable Auto Mode */
> - if (is_apmf_func_supported(dev, APMF_FUNC_AUTO_MODE)) {
> + if (amd_pmf_init_smart_pc(dev)) {
> + /* Enable Smart PC Solution builder */
> + dev_dbg(dev->dev, "Smart PC Solution Enabled\n");
> + } else if (is_apmf_func_supported(dev, APMF_FUNC_AUTO_MODE)) {
> + /* Enable Auto Mode */
I'm pretty certain neither of these two comments add any information to
what's readily visible from the code itself so they can be dropped.
> amd_pmf_init_auto_mode(dev);
> dev_dbg(dev->dev, "Auto Mode Init done\n");
> } else if (is_apmf_func_supported(dev, APMF_FUNC_DYN_SLIDER_AC) ||
> @@ -330,7 +333,9 @@ static void amd_pmf_deinit_features(struct amd_pmf_dev *dev)
> amd_pmf_deinit_sps(dev);
> }
>
> - if (is_apmf_func_supported(dev, APMF_FUNC_AUTO_MODE)) {
> + if (dev->smart_pc_enabled) {
> + amd_pmf_deinit_smart_pc(dev);
> + } else if (is_apmf_func_supported(dev, APMF_FUNC_AUTO_MODE)) {
> amd_pmf_deinit_auto_mode(dev);
> } else if (is_apmf_func_supported(dev, APMF_FUNC_DYN_SLIDER_AC) ||
> is_apmf_func_supported(dev, APMF_FUNC_DYN_SLIDER_DC)) {
> diff --git a/drivers/platform/x86/amd/pmf/pmf.h b/drivers/platform/x86/amd/pmf/pmf.h
> index deba88e6e4c8..02460c2a31ea 100644
> --- a/drivers/platform/x86/amd/pmf/pmf.h
> +++ b/drivers/platform/x86/amd/pmf/pmf.h
> @@ -179,6 +179,12 @@ struct amd_pmf_dev {
> bool cnqf_enabled;
> bool cnqf_supported;
> struct notifier_block pwr_src_notifier;
> + /* Smart PC solution builder */
> + struct tee_context *tee_ctx;
> + struct tee_shm *fw_shm_pool;
> + u32 session_id;
> + void *shbuf;
> + bool smart_pc_enabled;
> };
>
> struct apmf_sps_prop_granular {
> @@ -389,6 +395,13 @@ struct apmf_dyn_slider_output {
> struct apmf_cnqf_power_set ps[APMF_CNQF_MAX];
> } __packed;
>
> +struct ta_pmf_shared_memory {
> + int command_id;
> + int resp_id;
> + u32 pmf_result;
> + u32 if_version;
> +};
> +
> /* Core Layer */
> int apmf_acpi_init(struct amd_pmf_dev *pmf_dev);
> void apmf_acpi_deinit(struct amd_pmf_dev *pmf_dev);
> @@ -433,4 +446,7 @@ void amd_pmf_deinit_cnqf(struct amd_pmf_dev *dev);
> int amd_pmf_trans_cnqf(struct amd_pmf_dev *dev, int socket_power, ktime_t time_lapsed_ms);
> extern const struct attribute_group cnqf_feature_attribute_group;
>
> +/* Smart PC builder Layer*/
Missing space.
> +int amd_pmf_init_smart_pc(struct amd_pmf_dev *dev);
> +void amd_pmf_deinit_smart_pc(struct amd_pmf_dev *dev);
> #endif /* PMF_H */
> diff --git a/drivers/platform/x86/amd/pmf/tee-if.c b/drivers/platform/x86/amd/pmf/tee-if.c
> new file mode 100644
> index 000000000000..4db80ca59a11
> --- /dev/null
> +++ b/drivers/platform/x86/amd/pmf/tee-if.c
> @@ -0,0 +1,112 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * AMD Platform Management Framework Driver - TEE Interface
> + *
> + * Copyright (c) 2023, Advanced Micro Devices, Inc.
> + * All Rights Reserved.
> + *
> + * Author: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> + */
> +
> +#include <linux/tee_drv.h>
> +#include <linux/uuid.h>
> +#include "pmf.h"
> +
> +#define MAX_TEE_PARAM 4
> +static const uuid_t amd_pmf_ta_uuid = UUID_INIT(0x6fd93b77, 0x3fb8, 0x524d,
> + 0xb1, 0x2d, 0xc5, 0x29, 0xb1, 0x3d, 0x85, 0x43);
> +
> +static int amd_pmf_amdtee_ta_match(struct tee_ioctl_version_data *ver, const void *data)
> +{
> + return ver->impl_id == TEE_IMPL_ID_AMDTEE;
> +}
> +
> +static int amd_pmf_ta_open_session(struct tee_context *ctx, u32 *id)
> +{
> + struct tee_ioctl_open_session_arg sess_arg = {};
> + int rc;
> +
> + export_uuid(sess_arg.uuid, &amd_pmf_ta_uuid);
> + sess_arg.clnt_login = TEE_IOCTL_LOGIN_PUBLIC;
> + sess_arg.num_params = 0;
> +
> + rc = tee_client_open_session(ctx, &sess_arg, NULL);
> + if (rc < 0 || sess_arg.ret != 0) {
> + pr_err("Failed to open TEE session err:%#x, ret:%#x\n", sess_arg.ret, rc);
Print normal -Exx error codes as %d, not %x (rc). I don't know what would
be best to do with sess_arg.ret, TEEC_ERROR_* look like errnos (negative
values) manually converted into u32.
> + rc = -EINVAL;
If rc < 0, I think you should just pass the error code on.
> + } else {
> + *id = sess_arg.session;
> + }
> +
> + return rc;
> +}
> +
> +static int amd_pmf_tee_init(struct amd_pmf_dev *dev)
> +{
> + int ret;
> + u32 size;
> +
> + /* Open context with TEE driver */
Too obvious comment to stay, it's what the code already says on the next
line so there's little point to repeat something this obvious in the
comments.
> + dev->tee_ctx = tee_client_open_context(NULL, amd_pmf_amdtee_ta_match, NULL, NULL);
> + if (IS_ERR(dev->tee_ctx)) {
> + dev_err(dev->dev, "Failed to open TEE context\n");
> + return PTR_ERR(dev->tee_ctx);
> + }
> +
> + /* Open session with PMF Trusted App */
Remove this one too.
> + ret = amd_pmf_ta_open_session(dev->tee_ctx, &dev->session_id);
> + if (ret) {
> + dev_err(dev->dev, "Failed to open TA session (%d)\n", ret);
> + ret = -EINVAL;
> + goto out_ctx;
> + }
> +
> + size = sizeof(struct ta_pmf_shared_memory);
> + dev->fw_shm_pool = tee_shm_alloc_kernel_buf(dev->tee_ctx, size);
> + if (IS_ERR(dev->fw_shm_pool)) {
> + dev_err(dev->dev, "Failed to alloc TEE shared memory\n");
> + ret = PTR_ERR(dev->fw_shm_pool);
> + goto out_sess;
> + }
> +
> + dev->shbuf = tee_shm_get_va(dev->fw_shm_pool, 0);
> + if (IS_ERR(dev->shbuf)) {
> + dev_err(dev->dev, "Failed to get TEE virtual address\n");
> + ret = PTR_ERR(dev->shbuf);
> + goto out_shm;
> + }
> + dev_dbg(dev->dev, "TEE init done\n");
> +
> + return 0;
> +
> +out_shm:
> + tee_shm_free(dev->fw_shm_pool);
> +out_sess:
> + tee_client_close_session(dev->tee_ctx, dev->session_id);
> +out_ctx:
> + tee_client_close_context(dev->tee_ctx);
> +
> + return ret;
> +}
> +
> +static void amd_pmf_tee_deinit(struct amd_pmf_dev *dev)
> +{
> + /* Free the shared memory pool */
> + tee_shm_free(dev->fw_shm_pool);
> +
> + /* close the existing session with PMF TA*/
Missing space.
> + tee_client_close_session(dev->tee_ctx, dev->session_id);
> +
> + /* close the context with TEE driver */
> + tee_client_close_context(dev->tee_ctx);
> +}
> +
> +int amd_pmf_init_smart_pc(struct amd_pmf_dev *dev)
> +{
> + return amd_pmf_tee_init(dev);
> +}
> +
> +void amd_pmf_deinit_smart_pc(struct amd_pmf_dev *dev)
> +{
> + amd_pmf_tee_deinit(dev);
> +}
>
--
i.
^ permalink raw reply
* Re: [PATCH v2 2/5] dt-bindings: input/touchscreen: Add compatible for IST3038B
From: Karel Balej @ 2023-10-04 8:32 UTC (permalink / raw)
To: Krzysztof Kozlowski, Markuss Broks, Dmitry Torokhov, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Henrik Rydberg, linux-input,
devicetree, linux-kernel, ~postmarketos/upstreaming
Cc: Duje Mihanović, Karel Balej
In-Reply-To: <b14654a4-7cbc-4027-8456-73efe214498d@linaro.org>
Hello, Krzysztof,
On Wed Oct 4, 2023 at 10:09 AM CEST, Krzysztof Kozlowski wrote:
> On 03/10/2023 15:34, karelb@gimli.ms.mff.cuni.cz wrote:
> > From: Karel Balej <karelb@gimli.ms.mff.cuni.cz>
> >
> > From: Markuss Broks <markuss.broks@gmail.com>
>
> This does not look right. Please apply it to your tree and see the
> result. You cannot have two From fields,
I am aware of this problem and I have explained it in a follow-up
message to the cover-letter, which should have reached you too - was it
not so?
The patches themselves should be fine though, so I plan to wait for some
feedback and fix this either in v3 or in a resend before application.
Best regards,
K. B.
^ permalink raw reply
* Re: [PATCH v2 2/5] dt-bindings: input/touchscreen: Add compatible for IST3038B
From: Krzysztof Kozlowski @ 2023-10-04 8:09 UTC (permalink / raw)
To: karelb, Markuss Broks, Dmitry Torokhov, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Henrik Rydberg, linux-input,
devicetree, linux-kernel, ~postmarketos/upstreaming
Cc: Duje Mihanović, Karel Balej
In-Reply-To: <20231003133440.4696-3-karelb@gimli.ms.mff.cuni.cz>
On 03/10/2023 15:34, karelb@gimli.ms.mff.cuni.cz wrote:
> From: Karel Balej <karelb@gimli.ms.mff.cuni.cz>
>
> From: Markuss Broks <markuss.broks@gmail.com>
This does not look right. Please apply it to your tree and see the
result. You cannot have two From fields,
>
> Imagis IST3038B is a variant (firmware?) of Imagis IST3038 IC,
> add the compatible for it to the IST3038C bindings.
>
> Signed-off-by: Markuss Broks <markuss.broks@gmail.com>
> Signed-off-by: Karel Balej <balejk@matfyz.cz>
Best regards,
Krzysztof
^ permalink raw reply
* Rozszerzenie Programu Mój Prąd 5.0
From: Kamil Lasek @ 2023-10-04 7:40 UTC (permalink / raw)
To: linux-input
Szanowni Państwo!
W ramach nowej edycji programu Mój Prąd mogą otrzymać Państwo dofinansowanie na zakup i montaż fotowoltaiki i/lub magazynu energii. Maksymalna kwota dofinansowania wynosi 58 tys. zł.
Jako firma wyspecjalizowana w tym zakresie zajmiemy się Państwa wnioskiem o dofinansowanie oraz instalacją i serwisem dopasowanych do Państwa budynku paneli słonecznych.
Będę wdzięczny za informację czy są Państwo zainteresowani.
Pozdrawiam,
Kamil Lasek
^ permalink raw reply
* Re: [PATCH v2 5/5] input/touchscreen: imagis: add support for IST3032C
From: kernel test robot @ 2023-10-04 6:42 UTC (permalink / raw)
To: karelb, Markuss Broks, Dmitry Torokhov, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Henrik Rydberg, linux-input,
devicetree, linux-kernel, ~postmarketos/upstreaming
Cc: oe-kbuild-all, Duje Mihanović, Karel Balej
In-Reply-To: <20231003133440.4696-6-karelb@gimli.ms.mff.cuni.cz>
Hi,
kernel test robot noticed the following build warnings:
[auto build test WARNING on dtor-input/next]
[also build test WARNING on dtor-input/for-linus robh/for-next linus/master v6.6-rc4 next-20231003]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/karelb-gimli-ms-mff-cuni-cz/input-touchscreen-imagis-Correct-the-maximum-touch-area-value/20231003-213739
base: https://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git next
patch link: https://lore.kernel.org/r/20231003133440.4696-6-karelb%40gimli.ms.mff.cuni.cz
patch subject: [PATCH v2 5/5] input/touchscreen: imagis: add support for IST3032C
config: x86_64-randconfig-004-20231004 (https://download.01.org/0day-ci/archive/20231004/202310041406.DCcjrpLd-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231004/202310041406.DCcjrpLd-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202310041406.DCcjrpLd-lkp@intel.com/
All warnings (new ones prefixed by >>):
drivers/input/touchscreen/imagis.c:383:39: warning: 'imagis_3038c_data' defined but not used [-Wunused-const-variable=]
383 | static const struct imagis_properties imagis_3038c_data = {
| ^~~~~~~~~~~~~~~~~
drivers/input/touchscreen/imagis.c:375:39: warning: 'imagis_3038b_data' defined but not used [-Wunused-const-variable=]
375 | static const struct imagis_properties imagis_3038b_data = {
| ^~~~~~~~~~~~~~~~~
>> drivers/input/touchscreen/imagis.c:368:39: warning: 'imagis_3032c_data' defined but not used [-Wunused-const-variable=]
368 | static const struct imagis_properties imagis_3032c_data = {
| ^~~~~~~~~~~~~~~~~
vim +/imagis_3032c_data +368 drivers/input/touchscreen/imagis.c
367
> 368 static const struct imagis_properties imagis_3032c_data = {
369 .interrupt_msg_cmd = IST3038C_REG_INTR_MESSAGE,
370 .touch_coord_cmd = IST3038C_REG_TOUCH_COORD,
371 .whoami_cmd = IST3038C_REG_CHIPID,
372 .whoami_val = IST3032C_WHOAMI,
373 };
374
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply
* Re: [PATCH] Input: elantech - fix fast_reconnect callback in ps2 mode
From: Thorsten Leemhuis @ 2023-10-04 6:34 UTC (permalink / raw)
To: Greg Kroah-Hartman, Jeffery Miller
Cc: Dmitry Torokhov, regressions, benjamin.tissoires, Andrew Duggan,
Andrew Duggan, loic.poulain, linux-input, linux-kernel
In-Reply-To: <2023100418-clapping-driven-bc09@gregkh>
On 04.10.23 08:19, Greg Kroah-Hartman wrote:
> On Tue, Oct 03, 2023 at 07:57:24PM -0500, Jeffery Miller wrote:
>> Make `elantech_setup_ps2` set a compatible fast_reconnect pointer
>> when its ps2 mode is used.
>>
>> When an SMBus connection is attempted and fails `psmouse_smbus_init`
>> sets fast_reconnect to `psmouse_smbus_reconnect`.
>> `psmouse_smbus_reconnect` expects `psmouse->private` to be
>> `struct psmouse_smbus_dev` but `elantech_setup_ps2` replaces
>> it with its private data. This was causing an issue on resume
>> since psmouse_smbus_reconnect was being called while in ps2, not SMBus
>> mode.
>>
>> This was uncovered by commit 92e24e0e57f7 ("Input: psmouse - add delay when
>> deactivating for SMBus mode")
>>
>> Closes:
>> Link:https://lore.kernel.org/all/ca0109fa-c64b-43c1-a651-75b294d750a1@leemhuis.info/
>> Reported-by: Thorsten Leemhuis <linux@leemhuis.info>
>>
>> Signed-off-by: Jeffery Miller <jefferymiller@google.com>
>> ---
>>
>> The other callbacks set in psmouse_smbus_init are already replaced.
>> Should fast_reconnect be set to `elantech_reconnect` instead?
>
> What commit id does this fix?
Good point, yes, it also needs this:
Fixes: 92e24e0e57f72e ("Input: psmouse - add delay when deactivating for
SMBus mode")
> Should it also have a cc: stable tag?
Not that I can see, as that commit was merged for 6.6 and not backported
(no idea why Jeffery CCed the stable list, maybe I'm missing something)
Ciao, Thorsten
^ permalink raw reply
* Re: [PATCH] Input: elantech - fix fast_reconnect callback in ps2 mode
From: Greg Kroah-Hartman @ 2023-10-04 6:19 UTC (permalink / raw)
To: Jeffery Miller
Cc: Dmitry Torokhov, regressions, benjamin.tissoires, linux,
Andrew Duggan, Andrew Duggan, loic.poulain, linux-input,
linux-kernel
In-Reply-To: <20231004005729.3943515-1-jefferymiller@google.com>
On Tue, Oct 03, 2023 at 07:57:24PM -0500, Jeffery Miller wrote:
> Make `elantech_setup_ps2` set a compatible fast_reconnect pointer
> when its ps2 mode is used.
>
> When an SMBus connection is attempted and fails `psmouse_smbus_init`
> sets fast_reconnect to `psmouse_smbus_reconnect`.
> `psmouse_smbus_reconnect` expects `psmouse->private` to be
> `struct psmouse_smbus_dev` but `elantech_setup_ps2` replaces
> it with its private data. This was causing an issue on resume
> since psmouse_smbus_reconnect was being called while in ps2, not SMBus
> mode.
>
> This was uncovered by commit 92e24e0e57f7 ("Input: psmouse - add delay when
> deactivating for SMBus mode")
>
> Closes:
> Link:https://lore.kernel.org/all/ca0109fa-c64b-43c1-a651-75b294d750a1@leemhuis.info/
> Reported-by: Thorsten Leemhuis <linux@leemhuis.info>
>
> Signed-off-by: Jeffery Miller <jefferymiller@google.com>
> ---
>
> The other callbacks set in psmouse_smbus_init are already replaced.
> Should fast_reconnect be set to `elantech_reconnect` instead?
What commit id does this fix? Should it also have a cc: stable tag?
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH] Input: elantech - fix fast_reconnect callback in ps2 mode
From: Thorsten Leemhuis @ 2023-10-04 5:07 UTC (permalink / raw)
To: Jeffery Miller, Dmitry Torokhov
Cc: regressions, benjamin.tissoires, Andrew Duggan, Andrew Duggan,
loic.poulain, Greg Kroah-Hartman, linux-input, linux-kernel
In-Reply-To: <20231004005729.3943515-1-jefferymiller@google.com>
On 04.10.23 02:57, Jeffery Miller wrote:
> Make `elantech_setup_ps2` set a compatible fast_reconnect pointer
> when its ps2 mode is used.
>
> When an SMBus connection is attempted and fails `psmouse_smbus_init`
> sets fast_reconnect to `psmouse_smbus_reconnect`.
> `psmouse_smbus_reconnect` expects `psmouse->private` to be
> `struct psmouse_smbus_dev` but `elantech_setup_ps2` replaces
> it with its private data. This was causing an issue on resume
> since psmouse_smbus_reconnect was being called while in ps2, not SMBus
> mode.
>
> This was uncovered by commit 92e24e0e57f7 ("Input: psmouse - add delay when
> deactivating for SMBus mode")
Ahh, thx for investigating. This fixes things for me. Feel free to add:
Tested-by: Thorsten Leemhuis <linux@leemhuis.info>
> Closes:
> Link:https://lore.kernel.org/all/ca0109fa-c64b-43c1-a651-75b294d750a1@leemhuis.info/
> Reported-by: Thorsten Leemhuis <linux@leemhuis.info>
Side note: something went sideways here, it afaics should look something
like this:
"""
> Reported-by: Thorsten Leemhuis <linux@leemhuis.info>
> Closes:
https://lore.kernel.org/all/ca0109fa-c64b-43c1-a651-75b294d750a1@leemhuis.info/
"""
Thx again. Ciao, Thorsten
^ permalink raw reply
* Re: [PATCH v2 3/5] input/touchscreen: imagis: Add support for Imagis IST3038B
From: kernel test robot @ 2023-10-04 2:50 UTC (permalink / raw)
To: karelb, Markuss Broks, Dmitry Torokhov, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Henrik Rydberg, linux-input,
devicetree, linux-kernel, ~postmarketos/upstreaming
Cc: oe-kbuild-all, Duje Mihanović, Karel Balej
In-Reply-To: <20231003133440.4696-4-karelb@gimli.ms.mff.cuni.cz>
Hi,
kernel test robot noticed the following build warnings:
[auto build test WARNING on dtor-input/next]
[also build test WARNING on dtor-input/for-linus robh/for-next linus/master v6.6-rc4 next-20231003]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/karelb-gimli-ms-mff-cuni-cz/input-touchscreen-imagis-Correct-the-maximum-touch-area-value/20231003-213739
base: https://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git next
patch link: https://lore.kernel.org/r/20231003133440.4696-4-karelb%40gimli.ms.mff.cuni.cz
patch subject: [PATCH v2 3/5] input/touchscreen: imagis: Add support for Imagis IST3038B
config: x86_64-randconfig-004-20231004 (https://download.01.org/0day-ci/archive/20231004/202310041036.tddy1jGm-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231004/202310041036.tddy1jGm-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202310041036.tddy1jGm-lkp@intel.com/
All warnings (new ones prefixed by >>):
>> drivers/input/touchscreen/imagis.c:374:39: warning: 'imagis_3038c_data' defined but not used [-Wunused-const-variable=]
374 | static const struct imagis_properties imagis_3038c_data = {
| ^~~~~~~~~~~~~~~~~
>> drivers/input/touchscreen/imagis.c:366:39: warning: 'imagis_3038b_data' defined but not used [-Wunused-const-variable=]
366 | static const struct imagis_properties imagis_3038b_data = {
| ^~~~~~~~~~~~~~~~~
vim +/imagis_3038c_data +374 drivers/input/touchscreen/imagis.c
365
> 366 static const struct imagis_properties imagis_3038b_data = {
367 .interrupt_msg_cmd = IST3038B_REG_STATUS,
368 .touch_coord_cmd = IST3038B_REG_STATUS,
369 .whoami_cmd = IST3038B_REG_CHIPID,
370 .whoami_val = IST3038B_WHOAMI,
371 .protocol_b = true,
372 };
373
> 374 static const struct imagis_properties imagis_3038c_data = {
375 .interrupt_msg_cmd = IST3038C_REG_INTR_MESSAGE,
376 .touch_coord_cmd = IST3038C_REG_TOUCH_COORD,
377 .whoami_cmd = IST3038C_REG_CHIPID,
378 .whoami_val = IST3038C_WHOAMI,
379 };
380
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox