> -----Original Message-----
> From: srinivas pandruvada <srinivas.pandruvada@xxxxxxxxxxxxxxx>
> Sent: Wednesday, December 22, 2021 11:16 PM
> To: Rafael J. Wysocki <rafael@xxxxxxxxxx>; Pawnikar, Sumeet R
> <sumeet.r.pawnikar@xxxxxxxxx>
> Cc: Daniel Lezcano <daniel.lezcano@xxxxxxxxxx>; Linux PM <linux-
> pm@xxxxxxxxxxxxxxx>; Linux Kernel Mailing List <linux-
> kernel@xxxxxxxxxxxxxxx>; Stable <stable@xxxxxxxxxxxxxxx>
> 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:
> > 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
>
Sure, let me follow this.
> 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.
>
Yes, I will update according to this.
> > > +{
> > > + 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
>
> > > + /* 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
> > > };
> > >
>
