netdev - Re: [PATCH net-next v8 07/11] net: stmmac: dwmac-loongson: Add multi-channel supports for loongson

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: <tr65rdtph43gtccnwymjfkaoumzuwc574cbzxfh2q3ipoip2eo@rzzrwtbp5m6v>
Date: Fri, 22 Mar 2024 21:47:35 +0300
From: Serge Semin <fancer.lancer@...il.com>
To: Yanteng Si <siyanteng@...ngson.cn>
Cc: hkallweit1@...il.com, andrew@...n.ch, peppe.cavallaro@...com, 
	alexandre.torgue@...s.st.com, joabreu@...opsys.com, Jose.Abreu@...opsys.com, 
	chenhuacai@...ngson.cn, linux@...linux.org.uk, guyinggang@...ngson.cn, 
	netdev@...r.kernel.org, chris.chenfeiyang@...il.com
Subject: Re: [PATCH net-next v8 07/11] net: stmmac: dwmac-loongson: Add
 multi-channel supports for loongson

On Fri, Mar 22, 2024 at 06:36:20PM +0800, Yanteng Si wrote:
> > > > > > +{
> > > > > > +	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);
> > > > > In what conditions is this possible? Will the
> > > > > loongson_dwmac_config_legacy() method work in that case? Did you test
> > > > > it out?
> I need to wait for special hardware and PMON for this.  Please give me some
> time.
> 
> > Then those platforms will _require_ to have the DT-node specified. This
> > will define the DT-bindings which I doubt you imply here. Am I wrong?
> > 
> > Once again have you tested the loongson_dwmac_config_legacy() method
> > working in the case of the pci_alloc_irq_vectors() failure?
> 
> Yes!  I have tested it, it works in single channel mode.
> 
> dmesg:
> 
> [    3.935203] mdio_bus stmmac-18:02: attached PHY driver [unbound]
> (mii_bus:phy_addr=stmmac-18:02, irq=POLL)
> [    3.945625] dwmac-loongson-pci 0000:00:03.1: MSI enable failed, Fallback to
> legacy interrupt
> [    3.954175] dwmac-loongson-pci 0000:00:03.1: User ID: 0xd1, Synopsys ID: 0x10
> [    3.973676] dwmac-loongson-pci 0000:00:03.1: DMA HW capability register supported
> [    3.981135] dwmac-loongson-pci 0000:00:03.1: RX Checksum Offload Engine supported
> 
> cat /proc/interrupt:
> 
> 43:          0          0   PCH PIC  16  ahci[0000:00:08.0]
>   44:          0          0   PCH PIC  12  enp0s3f0
>   45:          0          0   PCH PIC  14  enp0s3f1
>   46:      16233          0   PCH PIC  17  enp0s3f2
>   47:      12698          0   PCH PIC  48  xhci-hcd:usb1
> 
> 
> the irq number 46 is the falkback legacy irq.

Ok. Thanks. You can do that in a bit more clever manner. Like this:

+static int loongson_dwmac_config_multi_msi(struct pci_dev *pdev,
+					   struct plat_stmmacenet_data *plat,
+					   struct stmmac_resources *res)
+{
+	int i, ret, vecs;
+
+	/* INT NAME | MAC | CH7 rx | CH7 tx | ... | CH0 rx | CH0 tx |
+	 * --------- ----- -------- --------  ...  -------- --------
+	 * IRQ NUM  |  0  |   1    |   2    | ... |   15   |   16   |
+	 */
+	vecs = plat->rx_queues_to_use + plat->tx_queues_to_use + 1;
+	ret = pci_alloc_irq_vectors(pdev, 1, vecs, PCI_IRQ_MSI | PCI_IRQ_LEGACY);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "Failed to allocate PCI IRQs\n");
+		return ret;
+	} else if (ret >= vecs) {
+		for (i = 0; i < plat->rx_queues_to_use; i++) {
+			res->rx_irq[CHANNELS_NUM - 1 - i] =
+				pci_irq_vector(pdev, 1 + i * 2);
+		}
+		for (i = 0; i < plat->tx_queues_to_use; i++) {
+			res->tx_irq[CHANNELS_NUM - 1 - i] =
+				pci_irq_vector(pdev, 2 + i * 2);
+		}
+
+		plat->flags |= STMMAC_FLAG_MULTI_MSI_EN;
+	}
+
+	res->irq = pci_irq_vector(pdev, 0);
+
+	return 0;
+}

Thus in case if for some reason you were able to allocate less MSI
IRQs than required you'll still be able to use them. The legacy IRQ
will be also available in case if MSI failed to be allocated.

-Serge(y)

> 
> > 	
> > 
> > > > > > +	}
> > > > > > +
> > > > > > +	plat->rx_queues_to_use = channel_num;
> > > > > > +	plat->tx_queues_to_use = channel_num;
> > > > > This is supposed to be initialized in the setup() methods. Please move
> > > > > it to the dedicated patch.
> > > > No, referring to my previous reply, only the 0x10 gnet device has 8 channels,
> > > > and the 0x37 device has a single channel.
> > Yes. You have a perfectly suitable method for it. It's
> > loongson_gnet_data(). Init the number of channels there based on the
> > value read from the GMAC_VERSION.SNPSVER field. Thus the
> > loongson_gnet_config_multi_msi() will get to be more coherent setting
> > up the MSI IRQs only.
> You are right!  it works well.
> 
> 
> Thanks,
> 
> Yanteng
> 
>