Messages in this thread | | | Date | Tue, 14 Feb 2023 20:22:29 -0800 | From | Jakub Kicinski <> | Subject | Re: [PATCH net-next v3 01/10] net: wwan: tmi: Add PCIe core |
| |
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?
> +#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.
> +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
> + 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
> + 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.
> +} > + > +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() ?
> + 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.
> +#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?
> + 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)));
parenthesis unnecessary
> + > + 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.
> + 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--) > + 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.
> +{ > + 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.
> + pci_disable_pcie_error_reporting(pdev); > + pci_clear_master(pdev); > + 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.
> + 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
> + 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.
> + 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.
| |