Re: [PATCH net-next v4 01/10] net: wwan: tmi: Add PCIe core

[Loic Poulain]

Hello,


On Mon, 10 Apr 2023 at 14:16, Yanchao Yang (杨彦超)
<Yanchao.Yang@xxxxxxxxxxxx> wrote:
>
> Hi Loic,
>
> sorry for late response, please check following reply.
>
> On Mon, 2023-03-20 at 21:26 +0800, Yanchao Yang wrote:
> > Hi Loic,
> >
> > On Fri, 2023-03-17 at 13:34 +0100, Loic Poulain wrote:
> > > Hi Yanchao,
> > >
> > > On Fri, 17 Mar 2023 at 09:10, Yanchao Yang <
> > > yanchao.yang@xxxxxxxxxxxx
> > > > wrote:
> > > >
> > > > Registers the TMI device driver with the kernel. Set up all the
> > > > fundamental
> > > > configurations for the device: PCIe layer, Modem Host Cross Core
> > > > Interface
> > > > (MHCCIF), Reset Generation Unit (RGU), modem common control
> > > > operations and
> > > > build infrastructure.
> > > >
> > > > * PCIe layer code implements driver probe and removal, MSI-X
> > > > interrupt
> > > > initialization and de-initialization, and the way of resetting
> > > > the
> > > > device.
> > > > * MHCCIF provides interrupt channels to communicate events such
> > > > as
> > > > handshake,
> > > > PM and port enumeration.
> > > > * RGU provides interrupt channels to generate notifications from
> > > > the device
> > > > so that the TMI driver could get the device reset.
> > > > * Modem common control operations provide the basic read/write
> > > > functions of
> > > > the device's hardware registers, mask/unmask/get/clear functions
> > > > of
> > > > the
> > > > device's interrupt registers and inquiry functions of the
> > > > device's
> > > > status.
> > > >
> > > > Signed-off-by: Yanchao Yang <yanchao.yang@xxxxxxxxxxxx>
> > > > Signed-off-by: Ting Wang <ting.wang@xxxxxxxxxxxx>
> > > > ---
> > > >  drivers/net/wwan/Kconfig                 |  14 +
> > > >  drivers/net/wwan/Makefile                |   1 +
> > > >  drivers/net/wwan/mediatek/Makefile       |   8 +
> > > >  drivers/net/wwan/mediatek/mtk_dev.h      | 203 ++++++
> > > >  drivers/net/wwan/mediatek/pcie/mtk_pci.c | 887
> > > > +++++++++++++++++++++++
> > > >  drivers/net/wwan/mediatek/pcie/mtk_pci.h | 144 ++++
> > > >  drivers/net/wwan/mediatek/pcie/mtk_reg.h |  69 ++
> > > >  7 files changed, 1326 insertions(+)
> > > >  create mode 100644 drivers/net/wwan/mediatek/Makefile
> > > >  create mode 100644 drivers/net/wwan/mediatek/mtk_dev.h
> > > >  create mode 100644 drivers/net/wwan/mediatek/pcie/mtk_pci.c
> > > >  create mode 100644 drivers/net/wwan/mediatek/pcie/mtk_pci.h
> > > >  create mode 100644 drivers/net/wwan/mediatek/pcie/mtk_reg.h
> > > >
> > >
> > > [...]
> > >
> > > > +static int mtk_pci_get_virq_id(struct mtk_md_dev *mdev, int
> > > > irq_id)
> > > > +{
> > > > +       struct pci_dev *pdev = to_pci_dev(mdev->dev);
> > > > +       int nr = 0;
> > > > +
> > > > +       if (pdev->msix_enabled)
> > > > +               nr = irq_id % mdev->msi_nvecs;
> > > > +
> > > > +       return pci_irq_vector(pdev, nr);
> > > > +}
> > > > +
> > > > +static int mtk_pci_register_irq(struct mtk_md_dev *mdev, int
> > > > irq_id,
> > > > +                               int (*irq_cb)(int irq_id, void
> > > > *data), void *data)
> > > > +{
> > > > +       struct mtk_pci_priv *priv = mdev->hw_priv;
> > > > +
> > > > +       if (unlikely((irq_id < 0 || irq_id >= MTK_IRQ_CNT_MAX) ||
> > > > !irq_cb))
> > > > +               return -EINVAL;
> > > > +
> > > > +       if (priv->irq_cb_list[irq_id]) {
> > > > +               dev_err(mdev->dev,
> > > > +                       "Unable to register irq, irq_id=%d, it's
> > > > already been register by %ps.\n",
> > > > +                       irq_id, priv->irq_cb_list[irq_id]);
> > > > +               return -EFAULT;
> > > > +       }
> > > > +       priv->irq_cb_list[irq_id] = irq_cb;
> > > > +       priv->irq_cb_data[irq_id] = data;
> > >
> > > So it looks like you re-implement your own irq chip internally.
> > > What
> > > about creating a new irq-chip/domain for this (cf
> > > irq_domain_add_simple)?
> > > That would allow the client code to use the regular irq interface
> > > and
> > > helpers
> > > and it should simply code and improve its debuggability
> > > (/proc/irq...).
> >
> > We will check it and update you later.
> No, we don’t re-implement irq chip. After studying the irq_domain
> interface you suggest, the TMI driver leverages on MSI irq domain. We
> use pci_alloc_irq_vectors to allocate MSI-X irq desc and use
> pci_request_irq to bind interrupt sources with irq handlers.

What I mean is that you're implementing some interrupt muxing into
your driver, which could possibly be abstracted with a 'virtual' irq
domain and generic ops (e.g with proper irq domain your
mtk_pci_register_irq method could be replaced with the usual
request_irq), Think about it as future improvement, I assume we can go
with the current solution for now.

> > >
> > > [...]
> > >
> > > > +static int mtk_mhccif_register_evt(struct mtk_md_dev *mdev, u32
> > > > chs,
> > > > +                                  int (*evt_cb)(u32 status, void
> > > > *data), void *data)
> > > > +{
> > > > +       struct mtk_pci_priv *priv = mdev->hw_priv;
> > > > +       struct mtk_mhccif_cb *cb;
> > > > +       unsigned long flag;
> > > > +       int ret = 0;
> > > > +
> > > > +       if (!chs || !evt_cb)
> > > > +               return -EINVAL;
> > > > +
> > > > +       spin_lock_irqsave(&priv->mhccif_lock, flag);
> > >
> > > Why spinlock here and not mutex. AFAIU, you always take this lock
> > > in
> > > a
> > > non-atomic/process context.
> >
> > Currently, the function is only called in the FSM initialization and
> > PM(power management) initialization process. Both are atomic.
> > On the other hand, this registration function will operate the global
> > variables “mhccif_cb_list”, but it takes very little time. So, we
> > think
> > spinlock is preferred.
> Any ideas or comments for this? Please help share it at your
> convenience.

They do not seem atomic... moreover you're using the _irqsave variant
while you're never accessing the list from (hard|soft)irq context.

> >
> > >
> > > > +       list_for_each_entry(cb, &priv->mhccif_cb_list, entry) {
> > > > +               if (cb->chs & chs) {
> > > > +                       ret = -EFAULT;
> > > > +                       dev_err(mdev->dev,
> > > > +                               "Unable to register evt,
> > > > chs=0x%08X&0x%08X registered_cb=%ps\n",
> > > > +                               chs, cb->chs, cb->evt_cb);
> > > > +                       goto err;
> > > > +               }
> > > > +       }
> > > > +       cb = devm_kzalloc(mdev->dev, sizeof(*cb), GFP_ATOMIC);

Maybe alloc this outside the lock, so that you can use the 'regular'
GFP_KERNEL alloc.

> > > > +       if (!cb) {
> > > > +               ret = -ENOMEM;
> > > > +               goto err;
> > > > +       }
> > > > +       cb->evt_cb = evt_cb;
> > > > +       cb->data = data;
> > > > +       cb->chs = chs;
> > > > +       list_add_tail(&cb->entry, &priv->mhccif_cb_list);
> > > > +
> > > > +err:
> > > > +       spin_unlock_irqrestore(&priv->mhccif_lock, flag);
> > > > +
> > > > +       return ret;
> > > > +}
> > >
> > > [...]
> > >
> > > > +
> > > > +MODULE_LICENSE("GPL");
> > > > diff --git a/drivers/net/wwan/mediatek/pcie/mtk_pci.h
> > > > b/drivers/net/wwan/mediatek/pcie/mtk_pci.h
> > > > new file mode 100644
> > > > index 000000000000..b487ca9b302e
> > > > --- /dev/null
> > > > +++ b/drivers/net/wwan/mediatek/pcie/mtk_pci.h
> > >
> > > Why a separated header file, isn't the content (e.g. mtk_pci_priv)
> > > used only from mtk_pci.c?
> >
> > Do you mean that we should move all contents of “mtk_pci.h” into
> > “mtk_pci.c” directly? The “mtk_pci.h” seems to be redundant, right?
> Any ideas or comments for this? Please help share it at your
> convenience.

Yes, keep that in the  .c file.

Regards,
Loic