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: <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