| 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
| ||
|
Message-ID: <CAMZdPi9_xYO_MQ0BpxcqDci761uu=ZoczGMg81qkEDeOsP6apw@mail.gmail.com>
Date: Fri, 17 Mar 2023 13:34:32 +0100
From: Loic Poulain <loic.poulain@...aro.org>
To: Yanchao Yang <yanchao.yang@...iatek.com>
Cc: Sergey Ryazanov <ryazanov.s.a@...il.com>,
Johannes Berg <johannes@...solutions.net>,
"David S . Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>,
Paolo Abeni <pabeni@...hat.com>,
netdev ML <netdev@...r.kernel.org>,
kernel ML <linux-kernel@...r.kernel.org>,
Intel experts <linuxwwan@...el.com>,
Chetan <m.chetan.kumar@...el.com>,
MTK ML <linux-mediatek@...ts.infradead.org>,
Liang Lu <liang.lu@...iatek.com>,
Haijun Liu <haijun.liu@...iatek.com>,
Hua Yang <hua.yang@...iatek.com>,
Ting Wang <ting.wang@...iatek.com>,
Felix Chen <felix.chen@...iatek.com>,
Mingliang Xu <mingliang.xu@...iatek.com>,
Min Dong <min.dong@...iatek.com>,
Aiden Wang <aiden.wang@...iatek.com>,
Guohao Zhang <guohao.zhang@...iatek.com>,
Chris Feng <chris.feng@...iatek.com>,
Lambert Wang <lambert.wang@...iatek.com>,
Mingchuang Qiao <mingchuang.qiao@...iatek.com>,
Xiayu Zhang <xiayu.zhang@...iatek.com>,
Haozhe Chang <haozhe.chang@...iatek.com>
Subject: Re: [PATCH net-next v4 01/10] net: wwan: tmi: Add PCIe core
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...).
[...]
> +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.
> + 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?
> @@ -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
Powered by blists - more mailing lists