netdev - Re: [PATCH net-next v12 13/15] net: stmmac: dwmac-loongson: Add Loongson GNET support

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <150b03ff-70b5-488a-b5e6-5f74b6398b20@loongson.cn>
Date: Tue, 7 May 2024 21:35:24 +0800
From: Yanteng Si <siyanteng@...ngson.cn>
To: Serge Semin <fancer.lancer@...il.com>
Cc: andrew@...n.ch, hkallweit1@...il.com, peppe.cavallaro@...com,
 alexandre.torgue@...s.st.com, joabreu@...opsys.com, Jose.Abreu@...opsys.com,
 chenhuacai@...nel.org, linux@...linux.org.uk, guyinggang@...ngson.cn,
 netdev@...r.kernel.org, chris.chenfeiyang@...il.com, siyanteng01@...il.com
Subject: Re: [PATCH net-next v12 13/15] net: stmmac: dwmac-loongson: Add
 Loongson GNET support

Hi Serge,

在 2024/5/6 18:39, Serge Semin 写道:
> On Thu, Apr 25, 2024 at 09:11:36PM +0800, Yanteng Si wrote:
>> ...
>>   
>> +static int loongson_dwmac_config_msi(struct pci_dev *pdev,
>> +				     struct plat_stmmacenet_data *plat,
>> +				     struct stmmac_resources *res,
>> +				     struct device_node *np)
>> +{
>> +	int i, ret, vecs;
>> +
>> +	vecs = roundup_pow_of_two(CHANNEL_NUM * 2 + 1);
>> +	ret = pci_alloc_irq_vectors(pdev, vecs, vecs, PCI_IRQ_MSI);
>> +	if (ret < 0) {
>> +		dev_info(&pdev->dev,
>> +			 "MSI enable failed, Fallback to legacy interrupt\n");
>> +		return loongson_dwmac_config_legacy(pdev, plat, res, np);
>> +	}
>> +
>> +	res->irq = pci_irq_vector(pdev, 0);
>> +	res->wol_irq = 0;
>> +
>> +	/* INT NAME | MAC | CH7 rx | CH7 tx | ... | CH0 rx | CH0 tx |
>> +	 * --------- ----- -------- --------  ...  -------- --------
>> +	 * IRQ NUM  |  0  |   1    |   2    | ... |   15   |   16   |
>> +	 */
>> +	for (i = 0; i < CHANNEL_NUM; i++) {
>> +		res->rx_irq[CHANNEL_NUM - 1 - i] =
>> +			pci_irq_vector(pdev, 1 + i * 2);
>> +		res->tx_irq[CHANNEL_NUM - 1 - i] =
>> +			pci_irq_vector(pdev, 2 + i * 2);
>> +	}
>> +
>> +	plat->flags |= STMMAC_FLAG_MULTI_MSI_EN;
>> +
>> +	return 0;
>> +}
>> +
>> ...
>>   static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>>   {
>>   	struct plat_stmmacenet_data *plat;
>>   	int ret, i, bus_id, phy_mode;
>>   	struct stmmac_pci_info *info;
>>   	struct stmmac_resources res;
>> +	struct loongson_data *ld;
>>   	struct device_node *np;
>>   
>>   	np = dev_of_node(&pdev->dev);
>> @@ -122,10 +460,12 @@ static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id
>>   		return -ENOMEM;
>>   
>>   	plat->dma_cfg = devm_kzalloc(&pdev->dev, sizeof(*plat->dma_cfg), GFP_KERNEL);
>> -	if (!plat->dma_cfg) {
>> -		ret = -ENOMEM;
>> -		goto err_put_node;
>> -	}
>> +	if (!plat->dma_cfg)
>> +		return -ENOMEM;
>> +
>> +	ld = devm_kzalloc(&pdev->dev, sizeof(*ld), GFP_KERNEL);
>> +	if (!ld)
>> +		return -ENOMEM;
>>   
>>   	/* Enable pci device */
>>   	ret = pci_enable_device(pdev);
>> @@ -171,14 +511,34 @@ static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id
>>   		plat->phy_interface = phy_mode;
>>   	}
>>   
>> -	pci_enable_msi(pdev);
>> +	plat->bsp_priv = ld;
>> +	plat->setup = loongson_dwmac_setup;
>> +	ld->dev = &pdev->dev;
>> +
>>   	memset(&res, 0, sizeof(res));
>>   	res.addr = pcim_iomap_table(pdev)[0];
>> +	ld->gmac_verion = readl(res.addr + GMAC_VERSION) & 0xff;
>> +
>> +	switch (ld->gmac_verion) {
>> +	case LOONGSON_DWMAC_CORE_1_00:
>> +		plat->rx_queues_to_use = CHANNEL_NUM;
>> +		plat->tx_queues_to_use = CHANNEL_NUM;
>> +
>> +		/* Only channel 0 supports checksum,
>> +		 * so turn off checksum to enable multiple channels.
>> +		 */
>> +		for (i = 1; i < CHANNEL_NUM; i++)
>> +			plat->tx_queues_cfg[i].coe_unsupported = 1;
>>   
>> -	plat->tx_queues_to_use = 1;
>> -	plat->rx_queues_to_use = 1;
>> +		ret = loongson_dwmac_config_msi(pdev, plat, &res, np);
>> +		break;
>> +	default:	/* 0x35 device and 0x37 device. */
>> +		plat->tx_queues_to_use = 1;
>> +		plat->rx_queues_to_use = 1;
>>   
>> -	ret = loongson_dwmac_config_legacy(pdev, plat, &res, np);
>> +		ret = loongson_dwmac_config_legacy(pdev, plat, &res, np);
>> +		break;
>> +	}
>>   
> Let's now talk about this change.
>
> First of all, one more time. You can't miss the return value check
> because if any of the IRQ config method fails then the driver won't
> work! The first change that introduces the problem is in the patch
> [PATCH net-next v12 11/15] net: stmmac: dwmac-loongson: Add loongson_dwmac_config_legacy
OK!
>
> Second, as I already mentioned in another message sent to this patch
> you are missing the PCI MSI IRQs freeing in the cleanup-on-error path
> and in the device/driver remove() function. It's definitely wrong.
You are right! I will do it.
> Thirdly, you said that the node-pointer is now optional and introduced
> the patch
> [PATCH net-next v12 10/15] net: stmmac: dwmac-loongson: Add full PCI support
> If so and the DT-based setting up isn't mandatory then I would
> suggest to proceed with the entire so called legacy setups only if the
> node-pointer has been found, otherwise the pure PCI-based setup would
> be performed. So the patches 10-13 (in your v12 order) would look

In this case, MSI will not be enabled when the node-pointer is found.

.


In fact, a large fraction of 2k devices are DT-based, of course, many 
are PCI-based.


Thanks,

Yanteng