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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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





[Index of Archives]     [Linux Kernel]     [Kernel Development Newbies]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Hiking]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux