Re: [PATCH net-next v3 01/10] net: wwan: tmi: Add PCIe core

["Yanchao Yang (杨彦超)" <Yanchao.Yang@mediatek.com>]

Hi Jakub,
On Thu, 2023-02-16 at 20:50 +0800, Yanchao Yang wrote:
> > 
> 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.
Update test result.
Both flags are deleted, then run the make command with 
"build in" and "build module" on a separate kernel tree. Both suffer
the same build error.
“drivers/net/wan/mediatek/pcie/mtk_pci.c:16:10: fatal error: mtk_fsm.h:
No such file or directory
 #include "mtk_fsm.h""
The reason is that all files are not placed in the same folder. The
driver named TMI needs a child folder, then needs these flags.

Any ideas or comments for this? Please help share it at your
convenience.

> > > +{
> > > +	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.

Any ideas or comments for this? Please help share it at your
convenience.

Many thanks.
yanchao.yang