netdev - 回覆: [net-next 3/3] net: ftgmac100: Support for AST2700

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:
 <SEYPR06MB5134FCCB102F13EA968F81869D5B2@SEYPR06MB5134.apcprd06.prod.outlook.com>
Date: Thu, 14 Nov 2024 02:19:44 +0000
From: Jacky Chou <jacky_chou@...eedtech.com>
To: Andrew Lunn <andrew@...n.ch>
CC: "andrew+netdev@...n.ch" <andrew+netdev@...n.ch>, "davem@...emloft.net"
	<davem@...emloft.net>, "edumazet@...gle.com" <edumazet@...gle.com>,
	"kuba@...nel.org" <kuba@...nel.org>, "pabeni@...hat.com" <pabeni@...hat.com>,
	"robh@...nel.org" <robh@...nel.org>, "krzk+dt@...nel.org"
	<krzk+dt@...nel.org>, "conor+dt@...nel.org" <conor+dt@...nel.org>,
	"p.zabel@...gutronix.de" <p.zabel@...gutronix.de>, "ratbert@...aday-tech.com"
	<ratbert@...aday-tech.com>, "netdev@...r.kernel.org"
	<netdev@...r.kernel.org>, "devicetree@...r.kernel.org"
	<devicetree@...r.kernel.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>
Subject:
 回覆: [net-next 3/3] net: ftgmac100: Support for AST2700

Hi Andrew,

Thank you for your reply.

> >  	/* Setup RX ring buffer base */
> > -	iowrite32(priv->rxdes_dma, priv->base +
> FTGMAC100_OFFSET_RXR_BADR);
> > +	iowrite32(lower_32_bits(priv->rxdes_dma), priv->base +
> FTGMAC100_OFFSET_RXR_BADR);
> > +	iowrite32(upper_32_bits(priv->rxdes_dma), priv->base +
> > +FTGMAC100_OFFSET_RXR_BADDR_HIGH);
> 
> This appears to write to registers which older generations do not have. Is this
> safe? Is it defined in the datasheet what happens when you write to reserved
> registers?

These registers that add in AST2700 do not exist in older generations.
I have verified that these addresses can be accessed on older generations, 
like AST2600, etc. They are no impact.

> 
> >  	/* Store DMA address into RX desc */
> > -	rxdes->rxdes3 = cpu_to_le32(map);
> > +	rxdes->rxdes2 = FIELD_PREP(FTGMAC100_RXDES2_RXBUF_BADR_HI,
> upper_32_bits(map));
> > +	rxdes->rxdes3 = lower_32_bits(map);
> 
> Maybe update the comment:
>         unsigned int    rxdes3; /* not used by HW */

The rxdes3 fills in the packet buffer address. HW will use it to do DMA to put 
packet content from receiving side.

> 
> Also, should its type be changed to __le32 ?

Agree. I will add this on next version.

> 
> > -	map = le32_to_cpu(rxdes->rxdes3);
> > +	map = le32_to_cpu(rxdes->rxdes3) | ((rxdes->rxdes2 &
> > +FTGMAC100_RXDES2_RXBUF_BADR_HI) << 16);
> 
> Is this safe? You have to assume older generation of devices will return 42 in
> rxdes3, since it is not used by the hardware.

Why does it need to return 42 in rxdes3?
The packet buffer address of the RX descriptor is used in both software and hardware.

> 
> >  	/* Mark the end of the ring */
> >  	rxdes->rxdes0 |= cpu_to_le32(priv->rxdes0_edorr_mask);
> > @@ -1249,7 +1266,6 @@ static int ftgmac100_poll(struct napi_struct *napi,
> int budget)
> >  		more = ftgmac100_rx_packet(priv, &work_done);
> >  	} while (more && work_done < budget);
> >
> > -
> >  	/* The interrupt is telling us to kick the MAC back to life
> >  	 * after an RX overflow
> >  	 */
> > @@ -1339,7 +1355,6 @@ static void ftgmac100_reset(struct ftgmac100
> *priv)
> >  	if (priv->mii_bus)
> >  		mutex_lock(&priv->mii_bus->mdio_lock);
> >
> > -
> >  	/* Check if the interface is still up */
> >  	if (!netif_running(netdev))
> >  		goto bail;
> > @@ -1438,7 +1453,6 @@ static void ftgmac100_adjust_link(struct
> > net_device *netdev)
> >
> >  	if (netdev->phydev)
> >  		mutex_lock(&netdev->phydev->lock);
> > -
> >  }
> 
> There are quite a few whitespace changes like this in this patch. Please remove
> them. If they are important, put them into a patch of there own.

Agree, I will adjust this part on next version.

> 
> > +	dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64));
> 
> This can fail, you should check the return value.

Agree. I will add to check the return value on next version.

Thanks,
Jacky