Message-ID: <2e518c17bf54298a2108de75fcd35aaf2b3397d3.camel@mediatek.com>
Date:   Thu, 16 Feb 2023 12:50:44 +0000
From:   Yanchao Yang (杨彦超) 
        <Yanchao.Yang@...iatek.com>
To:     "kuba@...nel.org" <kuba@...nel.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>,
        Xiayu Zhang (张夏宇) 
        <Xiayu.Zhang@...iatek.com>,
        "loic.poulain@...aro.org" <loic.poulain@...aro.org>,
        "pabeni@...hat.com" <pabeni@...hat.com>,
        "edumazet@...gle.com" <edumazet@...gle.com>,
        Ting Wang (王挺) <ting.wang@...iatek.com>,
        "johannes@...solutions.net" <johannes@...solutions.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Aiden Wang (王咏麒) 
        <Aiden.Wang@...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>,
        Guohao Zhang (张国豪) 
        <Guohao.Zhang@...iatek.com>
Subject: Re: [PATCH net-next v3 01/10] net: wwan: tmi: Add PCIe core

Hi Jakub,

On Tue, 2023-02-14 at 20:22 -0800, Jakub Kicinski wrote:
> On Sat, 11 Feb 2023 16:37:23 +0800 Yanchao Yang wrote:
> > +ccflags-y += -I$(srctree)/$(src)/
> > +ccflags-y += -I$(srctree)/$(src)/pcie/
> 
> Do you really need these flags?
We will check and update if it's really redundant soon.
> 
> > +#ifndef _MTK_COMMON_H
> > +#define _MTK_COMMON_H
> > +
> > +#include <linux/device.h>
> > +
> > +#define MTK_UEVENT_INFO_LEN 128
> > +
> > +/* MTK uevent */
> > +enum mtk_uevent_id {
> > +	MTK_UEVENT_FSM = 1,
> > +	MTK_UEVENT_MAX
> > +};
> > +
> > +static inline void mtk_uevent_notify(struct device *dev, enum
> > mtk_uevent_id id, const char *info)
> > +{
> > +	char buf[MTK_UEVENT_INFO_LEN];
> > +	char *ext[2] = {NULL, NULL};
> > +
> > +	snprintf(buf, MTK_UEVENT_INFO_LEN, "%s:event_id=%d, info=%s",
> > +		 dev->kobj.name, id, info);
> > +	ext[0] = buf;
> > +	kobject_uevent_env(&dev->kobj, KOBJ_CHANGE, ext);
> > +}
> 
> What is this for? It's not used in this patch.
Ok, remove it from patch1 and add it back to patch5.
> 
> > +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;
> 
> avoid defensive programming
Ok, remove it.
> 
> > +	spin_lock_irqsave(&priv->mhccif_lock, flag);
> > +	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;
> > +}
> > +
> > +static int mtk_mhccif_unregister_evt(struct mtk_md_dev *mdev, u32
> > chs)
> > +{
> > +	struct mtk_pci_priv *priv = mdev->hw_priv;
> > +	struct mtk_mhccif_cb *cb, *next;
> > +	unsigned long flag;
> > +	int ret = 0;
> > +
> > +	if (!chs)
> > +		return -EINVAL;
> 
> avoid defensive programming
Ok, remove it.
> 
> > +	spin_lock_irqsave(&priv->mhccif_lock, flag);
> > +	list_for_each_entry_safe(cb, next, &priv->mhccif_cb_list,
> > entry) {
> > +		if (cb->chs == chs) {
> > +			list_del(&cb->entry);
> > +			devm_kfree(mdev->dev, cb);
> > +			goto out;
> > +		}
> > +	}
> > +	ret = -EFAULT;
> > +	dev_warn(mdev->dev, "Unable to unregister evt, no chs=0x%08X
> > has been registered.\n", chs);
> > +out:
> > +	spin_unlock_irqrestore(&priv->mhccif_lock, flag);
> > +
> > +	return ret;
> > +}
> > +static void mtk_mhccif_clear_evt(struct mtk_md_dev *mdev, u32 chs)
> > +{
> > +	struct mtk_pci_priv *priv = mdev->hw_priv;
> > +
> > +	mtk_pci_write32(mdev, priv->cfg->mhccif_rc_base_addr
> > +		+ MHCCIF_EP2RC_SW_INT_ACK, chs);
> 
> + goes at the end of the previous line, and the continuation line
> should be aligned to (
> Please fix everywhere.
Ok, modify as shown below:
> +	mtk_pci_write32(mdev, priv->cfg-
>mhccif_rc_base_addr +
> +		               MHCCIF_EP2RC_SW_INT_ACK,
chs);
> 
> > +}
> > +
> > +static int mtk_mhccif_send_evt(struct mtk_md_dev *mdev, u32 ch)
> > +{
> > +	struct mtk_pci_priv *priv = mdev->hw_priv;
> > +	u32 rc_base;
> > +
> > +	rc_base = priv->cfg->mhccif_rc_base_addr;
> > +	/* Only allow one ch to be triggered at a time */
> > +	if ((ch & (ch - 1)) || !ch) {
> 
> is_power_of_2() ?
Ok, use kernel API instead
> +	/* Only allow one ch to be triggered at a
time */
> +	if (!is_power_of_2(ch)) {
> 
> > +		dev_err(mdev->dev, "Unsupported ext evt ch=0x%08X\n",
> > ch);
> > +		return -EINVAL;
> > +	}
> > +
> > +	mtk_pci_write32(mdev, rc_base + MHCCIF_RC2EP_SW_BSY, ch);
> > +	mtk_pci_write32(mdev, rc_base + MHCCIF_RC2EP_SW_TCHNUM, ffs(ch)
> > - 1);
> > +
> > +	return 0;
> > +}
> > +
> > +static u32 mtk_mhccif_get_evt_status(struct mtk_md_dev *mdev)
> > +{
> > +	struct mtk_pci_priv *priv = mdev->hw_priv;
> > +
> > +	return mtk_pci_read32(mdev, priv->cfg->mhccif_rc_base_addr +
> > MHCCIF_EP2RC_SW_INT_STS);
> > +}
> > +
> > +static int mtk_pci_acpi_reset(struct mtk_md_dev *mdev, char
> > *fn_name)
> > +{
> > +#ifdef CONFIG_ACPI
> > +	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
> > +	acpi_status acpi_ret;
> > +	acpi_handle handle;
> > +	int ret = 0;
> > +
> > +	handle = ACPI_HANDLE(mdev->dev);
> > +	if (!handle) {
> > +		dev_err(mdev->dev, "Unsupported, acpi handle isn't
> > found\n");
> > +		ret = -ENODEV;
> > +		goto err;
> > +	}
> > +	if (!acpi_has_method(handle, fn_name)) {
> > +		dev_err(mdev->dev, "Unsupported, _RST method isn't
> > found\n");
> > +		ret = -ENODEV;
> > +		goto err;
> > +	}
> > +	acpi_ret = acpi_evaluate_object(handle, fn_name, NULL,
> > &buffer);
> > +	if (ACPI_FAILURE(acpi_ret)) {
> > +		dev_err(mdev->dev, "Failed to execute %s method: %s\n",
> > +			fn_name,
> > +			acpi_format_exception(acpi_ret));
> > +		ret = -EFAULT;
> > +		goto err;
> > +	}
> > +	dev_info(mdev->dev, "FLDR execute successfully\n");
> > +	acpi_os_free(buffer.pointer);
> > +err:
> > +	return ret;
> > +#else
> > +	dev_err(mdev->dev, "Unsupported, CONFIG ACPI hasn't been set to
> > 'y'\n");
> > +	return -ENODEV;
> 
> If driver needs ACPI it should be a dependency in Kconfig.
Ok, add the dependency in Kconfig.
> 
> > +#endif
> > +}
> > +static irqreturn_t mtk_pci_irq_msix(int irq, void *data)
> > +{
> > +	struct mtk_pci_irq_desc *irq_desc = data;
> > +	struct mtk_md_dev *mdev = irq_desc->mdev;
> > +	struct mtk_pci_priv *priv;
> > +	u32 irq_state, irq_enable;
> > +
> > +	priv = mdev->hw_priv;
> > +	irq_state = mtk_pci_mac_read32(priv,
> > REG_MSIX_ISTATUS_HOST_GRP0_0);
> > +	irq_enable = mtk_pci_mac_read32(priv,
> > REG_IMASK_HOST_MSIX_GRP0_0);
> > +	dev_dbg(mdev->dev, "irq_state=0x%08X, irq_enable=0x%08X,
> > msix_bits=0x%08X\n",
> > +		irq_state, irq_enable, irq_desc->msix_bits);
> > +	irq_state &= irq_enable;
> > +
> > +	if (unlikely(!irq_state) ||
> > +	    unlikely(!((irq_state % GENMASK(priv->irq_cnt - 1, 0)) &
> > irq_desc->msix_bits)))
> 
> Are you sure the modulo is correct here?
Yes, sure. We have tested it and confirm related test cases pass.
> 
> > +		return IRQ_NONE;
> > +
> > +	/* Mask the bit and user needs to unmask by itself */
> > +	mtk_pci_mac_write32(priv, REG_IMASK_HOST_MSIX_CLR_GRP0_0,
> > irq_state & (~BIT(30)));
> mtk_cldma.c
> parenthesis unnecessary
Ok, modify as shown below:
> +	mtk_pci_mac_write32(priv,
REG_IMASK_HOST_MSIX_CLR_GRP0_0, irq_state & ~BIT(30));
> 
> > +
> > +	return mtk_pci_irq_handler(mdev, irq_state);
> > +}
> > +
> > +static int mtk_pci_request_irq_msix(struct mtk_md_dev *mdev, int
> > irq_cnt_allocated)
> > +{
> > +	struct mtk_pci_priv *priv = mdev->hw_priv;
> > +	struct mtk_pci_irq_desc *irq_desc;
> > +	struct pci_dev *pdev;
> > +	int irq_cnt;
> > +	int ret, i;
> > +
> > +	/* calculate the nearest 2's power number */
> > +	irq_cnt = BIT(fls(irq_cnt_allocated) - 1);
> > +	pdev = to_pci_dev(mdev->dev);
> > +	irq_desc = priv->irq_desc;
> > +	for (i = 0; i < irq_cnt; i++) {
> > +		irq_desc[i].mdev = mdev;
> > +		irq_desc[i].msix_bits = BIT(i);
> > +		snprintf(irq_desc[i].name, MTK_IRQ_NAME_LEN, "msix%d-
> > %s", i, mdev->dev_str);
> 
> please use pci_name() instead of your custom format stored in
> dev_str.
Ok, modify as shown below:
snprintf(irq_desc[i].name, MTK_IRQ_NAME_LEN,
"msix%d-%s", i, pci_name(to_pci_dev(mdev->dev));
> 
> > +		ret = pci_request_irq(pdev, i, mtk_pci_irq_msix, NULL,
> > +				      &irq_desc[i], irq_desc[i].name);
> > +		if (ret) {
> > +			dev_err(mdev->dev, "Failed to request %s:
> > ret=%d\n", irq_desc[i].name, ret);
> > +			for (i--; i >= 0; i--)mtk_cldma.c
> > +				pci_free_irq(pdev, i, &irq_desc[i]);
> > +			return ret;
> > +		}
> > +	}
> > +	priv->irq_cnt = irq_cnt;
> > +	priv->irq_type = PCI_IRQ_MSIX;
> > +
> > +	if (irq_cnt != MTK_IRQ_CNT_MAX)
> > +		mtk_pci_set_msix_merged(priv, irq_cnt);
> > +
> > +	return 0;
> > +}
> > +
> > +static int mtk_pci_request_irq(struct mtk_md_dev *mdev, int
> > max_irq_cnt, int irq_type)
> 
> irq_type is always PCI_IRQ_MSIX in this series
> and max_irq_cnt is MTK_IRQ_CNT_MAX. Use those directly.
Ok, will modify this part as shown below:
static int
mtk_pci_request_irq(struct mtk_md_dev *mdev)

in mtk_pci_probe()
ret =
mtk_pci_request_irq(mdev);
if (ret)
	goto err_request_irq;
> 
> > +{
> > +	struct pci_dev *pdev = to_pci_dev(mdev->dev);
> > +	int irq_cnt;
> > +
> > +	irq_cnt = pci_alloc_irq_vectors(pdev, MTK_IRQ_CNT_MIN,
> > max_irq_cnt, irq_type);
> > +	mdev->msi_nvecs = irq_cnt;
> > +
> > +	if (irq_cnt < MTK_IRQ_CNT_MIN) {
> > +		dev_err(mdev->dev,
> > +			"Unable to alloc pci irq vectors. ret=%d
> > maxirqcnt=%d irqtype=0x%x\n",
> > +			irq_cnt, max_irq_cnt, irq_type);
> > +		return -EFAULT;
> > +	}
> > +
> > +	return mtk_pci_request_irq_msix(mdev, irq_cnt);
> > +}
> > +
> > +static void mtk_pci_free_irq(struct mtk_md_dev *mdev)
> > +{
> > +	struct pci_dev *pdev = to_pci_dev(mdev->dev);
> > +	struct mtk_pci_priv *priv = mdev->hw_priv;
> > +	int i;
> > +
> > +	for (i = 0; i < priv->irq_cnt; i++)
> > +		pci_free_irq(pdev, i, &priv->irq_desc[i]);
> > +
> > +	pci_free_irq_vectors(pdev);
> > +}
> > +
> > +static void mtk_pci_save_state(struct mtk_md_dev *mdev)
> > +{
> > +	struct pci_dev *pdev = to_pci_dev(mdev->dev);
> > +	struct mtk_pci_priv *priv = mdev->hw_priv;
> > +	int ltr, l1ss;
> > +
> > +	pci_save_state(pdev);
> > +	/* save ltr */
> > +	ltr = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_LTR);
> > +	if (ltr) {
> > +		pci_read_config_word(pdev, ltr + PCI_LTR_MAX_SNOOP_LAT,
> > +				     &priv->ltr_max_snoop_lat);
> > +		pci_read_config_word(pdev, ltr +
> > PCI_LTR_MAX_NOSNOOP_LAT,
> > +				     &priv->ltr_max_nosnoop_lat);
> > +	}
> > +	/* save l1ss */
> > +	l1ss = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_L1SS);
> > +	if (l1ss) {
> > +		pci_read_config_dword(pdev, l1ss + PCI_L1SS_CTL1,
> > &priv->l1ss_ctl1);
> > +		pci_read_config_dword(pdev, l1ss + PCI_L1SS_CTL2,
> > &priv->l1ss_ctl2);
> > +	}
> > +}
> > +	dev_info(mdev->dev, "Probe done hw_ver=0x%x\n", mdev->hw_ver);
> > +	return 0;
> > +
> > +err_save_state:
> 
> Labels should be named after action they perform, not where they jump
> from. Please fix this everywhere.
We can found similar samples in kernel codes, naming the label per
where jump from…
ex. pci-sysfs.c
shall we apply this rule to our driver?
I
t's mandatory or nice to have.
> 
> > +	pci_disable_pcie_error_reporting(pdev);
> > +	pci_clear_master(pdev);mtk_cldma.c
> > +	mtk_pci_free_irq(mdev);
> > +err_request_irq:
> > +	mtk_mhccif_exit(mdev);
> > +err_mhccif_init:
> > +err_atr_init:
> > +	mtk_pci_bar_exit(mdev);
> > +err_bar_init:
> > +err_set_dma_mask:
> > +	pci_disable_device(pdev);
> > +err_enable_pdev:
> > +	devm_kfree(dev, priv);
> > +err_alloc_priv:
> > +	devm_kfree(dev, mdev);
> > +err_alloc_mdev:
> > +	dev_err(dev, "Failed to probe device, ret=%d\n", ret);
> 
> I believe core already prints this sort of a message. 
> Please double check.
Ok, remove the redundant warning trace.
> 
> > +	return ret;
> > +}
> > +
> > +static void mtk_pci_remove(struct pci_dev *pdev)
> > +{
> > +	struct mtk_md_dev *mdev = pci_get_drvdata(pdev);
> > +	struct mtk_pci_priv *priv = mdev->hw_priv;
> > +	struct device *dev = &pdev->dev;
> > +	int ret;
> > +
> > +	mtk_pci_mask_irq(mdev, priv->mhccif_irq_id);
> > +	pci_disable_pcie_error_reporting(pdev);
> 
> The explicit error reporting calls should be removed, please 
> see f26e58bf6f54
Ok, remove it ”pci_disable_pcie_error_reporting(pdev);“.
> 
> > +	ret = mtk_pci_fldr(mdev);
> > +	if (ret)
> > +		mtk_mhccif_send_evt(mdev, EXT_EVT_H2D_DEVICE_RESET);
> > +
> > +	pci_clear_master(pdev);
> > +	mtk_mhccif_exit(mdev);
> > +	mtk_pci_free_irq(mdev);
> > +	mtk_pci_bar_exit(mdev);
> > +	pci_disable_device(pdev);
> > +	pci_load_and_free_saved_state(pdev, &priv->saved_state);
> > +	devm_kfree(dev, priv);
> > +	devm_kfree(dev, mdev);
> 
> Why are you using devm_ if you call kfree explicitly anyway?
> You can save some memory by using kfree() directly.
devm_kzalloc(), devm_ioremap_resource(), devm_request_irq(),
devm_gpio_request(), devm_clk_get()…..
They will be freed automatically
when corresponding device is freed, so you don’t have to free them
explicitly. This also make probe error easier to handle. Is it ok?
> 
> > +	u16 ltr_max_snoop_lat;
> > +	u16 ltr_max_nosnoop_lat;
> > +	u32 l1ss_ctl1;
> > +	u32 l1ss_ctl2;
> 
> These 4 registers seem to be saved but never used.
Ok, remove it from patch1.

Many thanks.
yanchao.yang

Powered by blists - more mailing lists