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@xxxxxxxxx> 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@xxxxxxxxx>
> > Cc: stable@xxxxxxxxxxxxxxx # 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
> > };
> >
