netdev - Re: [PATCH 10/11] net: macb: Add support for RP1's MACB variant

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: <ZsThCh45-WX_TDmP@apocalypse>
Date: Tue, 20 Aug 2024 20:31:38 +0200
From: Andrea della Porta <andrea.porta@...e.com>
To: Andrew Lunn <andrew@...n.ch>
Cc: Andrea della Porta <andrea.porta@...e.com>,
	Michael Turquette <mturquette@...libre.com>,
	Stephen Boyd <sboyd@...nel.org>, Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Florian Fainelli <florian.fainelli@...adcom.com>,
	Broadcom internal kernel review list <bcm-kernel-feedback-list@...adcom.com>,
	Linus Walleij <linus.walleij@...aro.org>,
	Catalin Marinas <catalin.marinas@....com>,
	Will Deacon <will@...nel.org>,
	Derek Kiernan <derek.kiernan@....com>,
	Dragan Cvetic <dragan.cvetic@....com>,
	Arnd Bergmann <arnd@...db.de>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Nicolas Ferre <nicolas.ferre@...rochip.com>,
	Claudiu Beznea <claudiu.beznea@...on.dev>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Saravana Kannan <saravanak@...gle.com>,
	Bjorn Helgaas <bhelgaas@...gle.com>, linux-clk@...r.kernel.org,
	devicetree@...r.kernel.org, linux-rpi-kernel@...ts.infradead.org,
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
	linux-gpio@...r.kernel.org, netdev@...r.kernel.org,
	linux-pci@...r.kernel.org, linux-arch@...r.kernel.org,
	Lee Jones <lee@...nel.org>, Stefan Wahren <wahrenst@....net>
Subject: Re: [PATCH 10/11] net: macb: Add support for RP1's MACB variant

Hi Andrew,

On 17:13 Tue 20 Aug     , Andrew Lunn wrote:
> > +static unsigned int txdelay = 35;
> > +module_param(txdelay, uint, 0644);
> 
> Networking does not like module parameters.
> 
> This is also unused in this patch! So i suggest you just delete it.
> 
> > +
> >  /* This structure is only used for MACB on SiFive FU540 devices */
> >  struct sifive_fu540_macb_mgmt {
> >  	void __iomem *reg;
> > @@ -334,7 +337,7 @@ static int macb_mdio_wait_for_idle(struct macb *bp)
> >  	u32 val;
> >  
> >  	return readx_poll_timeout(MACB_READ_NSR, bp, val, val & MACB_BIT(IDLE),
> > -				  1, MACB_MDIO_TIMEOUT);
> > +				  100, MACB_MDIO_TIMEOUT);
> >  }
>   
> Please take this patch out of the series, and break it up. This is one
> patch, with a good explanation why you need 1->100.
> 
> >  static int macb_mdio_read_c22(struct mii_bus *bus, int mii_id, int regnum)
> > @@ -493,6 +496,19 @@ static int macb_mdio_write_c45(struct mii_bus *bus, int mii_id,
> >  	return status;
> >  }
> >  
> > +static int macb_mdio_reset(struct mii_bus *bus)
> > +{
> > +	struct macb *bp = bus->priv;
> > +
> > +	if (bp->phy_reset_gpio) {
> > +		gpiod_set_value_cansleep(bp->phy_reset_gpio, 1);
> > +		msleep(bp->phy_reset_ms);
> > +		gpiod_set_value_cansleep(bp->phy_reset_gpio, 0);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >  static void macb_init_buffers(struct macb *bp)
> >  {
> >  	struct macb_queue *queue;
> > @@ -969,6 +985,7 @@ static int macb_mii_init(struct macb *bp)
> >  	bp->mii_bus->write = &macb_mdio_write_c22;
> >  	bp->mii_bus->read_c45 = &macb_mdio_read_c45;
> >  	bp->mii_bus->write_c45 = &macb_mdio_write_c45;
> > +	bp->mii_bus->reset = &macb_mdio_reset;
> 
> This is one patch.
> 
> >  	snprintf(bp->mii_bus->id, MII_BUS_ID_SIZE, "%s-%x",
> >  		 bp->pdev->name, bp->pdev->id);
> >  	bp->mii_bus->priv = bp;
> > @@ -1640,6 +1657,11 @@ static int macb_rx(struct macb_queue *queue, struct napi_struct *napi,
> >  
> >  		macb_init_rx_ring(queue);
> >  		queue_writel(queue, RBQP, queue->rx_ring_dma);
> > +#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
> > +		if (bp->hw_dma_cap & HW_DMA_CAP_64B)
> > +			macb_writel(bp, RBQPH,
> > +				    upper_32_bits(queue->rx_ring_dma));
> > +#endif
> 
> How does this affect a disto kernel? Do you actually need the #ifdef?
> What does bp->hw_dma_cap contain when CONFIG_ARCH_DMA_ADDR_T_64BIT is
> not defined?
> 
> Again, this should be a patch of its own, with a good commit message.
> 
> Interrupt coalescing should be a patch of its own, etc.
> 
>     Andrew
>

Thanks for the feedback, I agree on all the observations. Please do note
however that, as mentioned in the cover letter, this patch is not intended
to be included upstream and is provided just as a quick way for anyone
interested in testing the RP1 functionality using the Ethernet MAC. 
As such, this patch has not been polished nor splitted into manageable bits.
Ii'm taknge note of your comments however and will come back to them in a
future patch that deals specifically with macb.

Many thanks,
Andrea
 
> ---
> pw-bot: cr