Re: [PATCH] thermal/drivers/int340x: add functions for mbox read and write commands

[srinivas pandruvada]

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
> >  };
> >