| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Mon, 20 Mar 2023 13:26:54 +0000
From: Yanchao Yang (杨彦超)
<Yanchao.Yang@...iatek.com>
To: "loic.poulain@...aro.org" <loic.poulain@...aro.org>
CC: Chris Feng (冯保林)
<Chris.Feng@...iatek.com>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-mediatek@...ts.infradead.org"
<linux-mediatek@...ts.infradead.org>,
Mingliang Xu (徐明亮)
<mingliang.xu@...iatek.com>,
Min Dong (董敏) <min.dong@...iatek.com>,
"linuxwwan@...el.com" <linuxwwan@...el.com>,
"m.chetan.kumar@...el.com" <m.chetan.kumar@...el.com>,
Liang Lu (吕亮) <liang.lu@...iatek.com>,
Haijun Liu (刘海军)
<haijun.liu@...iatek.com>,
Haozhe Chang (常浩哲)
<Haozhe.Chang@...iatek.com>,
Hua Yang (杨华) <Hua.Yang@...iatek.com>,
"ryazanov.s.a@...il.com" <ryazanov.s.a@...il.com>,
"kuba@...nel.org" <kuba@...nel.org>,
Aiden Wang (王咏麒)
<Aiden.Wang@...iatek.com>,
"edumazet@...gle.com" <edumazet@...gle.com>,
"pabeni@...hat.com" <pabeni@...hat.com>,
Ting Wang (王挺) <ting.wang@...iatek.com>,
"johannes@...solutions.net" <johannes@...solutions.net>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
Guohao Zhang (张国豪)
<Guohao.Zhang@...iatek.com>,
Felix Chen (陈非) <Felix.Chen@...iatek.com>,
Lambert Wang (王伟)
<Lambert.Wang@...iatek.com>,
"davem@...emloft.net" <davem@...emloft.net>,
Mingchuang Qiao (乔明闯)
<Mingchuang.Qiao@...iatek.com>,
Xiayu Zhang (张夏宇)
<Xiayu.Zhang@...iatek.com>
Subject: Re: [PATCH net-next v4 01/10] net: wwan: tmi: Add PCIe core
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@...iatek.com
> > 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@...iatek.com>
> > Signed-off-by: Ting Wang <ting.wang@...iatek.com>
> > ---
> > 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.
>
> [...]
>
> > +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.
>
> > + 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);
> > + 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?
>
> > @@ -0,0 +1,144 @@
> > +/* SPDX-License-Identifier: BSD-3-Clause-Clear
> > + *
> > + * Copyright (c) 2022, MediaTek Inc.
> > + */
> > +
> > +#ifndef __MTK_PCI_H__
> > +#define __MTK_PCI_H__
> > +
> > +#include <linux/pci.h>
> > +#include <linux/spinlock.h>
> > +
> > +#include "../mtk_dev.h"
> > +
> > +enum mtk_atr_type {
> > + ATR_PCI2AXI = 0,
> > + ATR_AXI2PCI
> > +};
>
> [...]
>
> Regards,
> Loic
Many thanks.
Yanchao.Yang
Powered by blists - more mailing lists