On Wed, 2021-12-22 at 17:56 +0100, Rafael J. Wysocki wrote:
On Wed, Dec 22, 2021 at 5:53 PM Sumeet Pawnikar sumeet.r.pawnikar@intel.com wrote:
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 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 sumeet.r.pawnikar@intel.com 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
2. There is one issue in the code below.
[...]
+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.
+{ + 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.
Thanks, Srinivas
+ /* 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 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 };