-----Original Message-----
From: srinivas pandruvada <srinivas.pandruvada@linux.intel.com>
Sent: Wednesday, December 22, 2021 11:16 PM
To: Rafael J. Wysocki <rafael@kernel.org>; Pawnikar, Sumeet R
[off-list ref]
Cc: Daniel Lezcano <redacted>; Linux PM <linux-
pm@vger.kernel.org>; Linux Kernel Mailing List <linux-
kernel@vger.kernel.org>; Stable [off-list ref]
Subject: Re: [PATCH] thermal/drivers/int340x: add functions for mbox read
and write commands

On Wed, 2021-12-22 at 17:56 +0100, Rafael J. Wysocki wrote:

quoted

On Wed, Dec 22, 2021 at 5:53 PM Sumeet Pawnikar
[off-list ref] wrote:

quoted

The existing mail mechanism only supports writing of workload types.
But mailbox command for RFIM (cmd = 0x08) also requires write
operation which was ignored. This results in failing to store RFI
restriction.
This requires enhancing mailbox writes for non workload commands
also.
So, remove the check for MBOX_CMD_WORKLOAD_TYPE_WRITE in

mailbox

quoted

quoted

write, with this other write commands also can be supoorted. But at
the same time, we have to make sure that there is no impact on read
commands, by not writing anything in mailbox data register.
To properly implement, add two separate functions for mbox read and
write command for processor thermal workload command type. This
helps to differentiate the read and write workload command types
while sending mbox command.

Fixes: 5d6fbc96bd36 ("thermal/drivers/int340x: processor_thermal:
Export additional attributes")
Signed-off-by: Sumeet Pawnikar <redacted>
Cc: stable@vger.kernel.org # 5.14+

This requires an ACK from Srinivas.

I found some more issues in this patch after my prior internal review.

1. Subject of patch should be what is this patch for rather than how.
Something like:
Fix RFIM mailbox write commands

Sure, let me follow this. 

2. There is one issue in the code below.

quoted

quoted

---

[...]

quoted

quoted

+static int send_mbox_read_cmd(struct pci_dev *pdev, u16 id, u32
data, u64 *resp)

There is no use of "data" argument for read  after spilt of read and write
commands, if you look at the code before.

Yes, I will update according to this. 

quoted

quoted

+{
+       struct proc_thermal_device *proc_priv;
+       u32 reg_data;
+       int ret;

-               if (!cmd_resp)
-                       break;
+       proc_priv = pci_get_drvdata(pdev);

-               if (cmd_id == MBOX_CMD_WORKLOAD_TYPE_READ)
-                       *cmd_resp = readl((void __iomem *)
(proc_priv->mmio_base + MBOX_OFFSET_DATA));
-               else
-                       *cmd_resp = readq((void __iomem *)
(proc_priv->mmio_base + MBOX_OFFSET_DATA));
+       mutex_lock(&mbox_lock);

-               break;
-       } while (--retries);
+       ret = wait_for_mbox_ready(proc_priv);
+       if (ret)
+               goto unlock_mbox;
+
+       writel(data, (proc_priv->mmio_base + MBOX_OFFSET_DATA));

The above is not required for read. This is what we wanted to avoid for
reads.

Let me remove this and test new changes and post V2 for review.

Thanks,
Sumeet.

Thanks,
Srinivas

quoted

quoted

+       /* Write command register */
+       reg_data = BIT_ULL(MBOX_BUSY_BIT) | id;
+       writel(reg_data, (proc_priv->mmio_base +
MBOX_OFFSET_INTERFACE));
+
+       ret = wait_for_mbox_ready(proc_priv);
+       if (ret)
+               goto unlock_mbox;
+
+       if (id == MBOX_CMD_WORKLOAD_TYPE_READ)
+               *resp = readl(proc_priv->mmio_base +
MBOX_OFFSET_DATA);
+       else
+               *resp = readq(proc_priv->mmio_base +
MBOX_OFFSET_DATA);

 unlock_mbox:
        mutex_unlock(&mbox_lock);
        return ret;
 }

-int processor_thermal_send_mbox_cmd(struct pci_dev *pdev, u16
cmd_id, u32 cmd_data, u64 *cmd_resp)
+int processor_thermal_send_mbox_read_cmd(struct pci_dev *pdev,

u16

quoted

quoted

id, u32 data, u64 *resp)
 {
-       return send_mbox_cmd(pdev, cmd_id, cmd_data, cmd_resp);
+       return send_mbox_read_cmd(pdev, id, data, resp);
 }
-EXPORT_SYMBOL_GPL(processor_thermal_send_mbox_cmd);
+EXPORT_SYMBOL_NS_GPL(processor_thermal_send_mbox_read_cmd,
INT340X_THERMAL);
+
+int processor_thermal_send_mbox_write_cmd(struct pci_dev *pdev,
u16 id, u32 data)
+{
+       return send_mbox_write_cmd(pdev, id, data); }
+EXPORT_SYMBOL_NS_GPL(processor_thermal_send_mbox_write_cmd,
INT340X_THERMAL);

 /* List of workload types */
 static const char * const workload_types[] = { @@ -104,7 +126,6 @@
static const char * const workload_types[] = {
        NULL
 };