Received-SPF: pass (google.com: domain of linux-kernel+bounces-100719-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Date: Tue, 12 Mar 2024 19:42:16 +0000
From: "Russell King (Oracle)" <linux@armlinux.org.uk>
To: Doug Berger <opendmb@gmail.com>
Cc: Florian Fainelli <f.fainelli@gmail.com>,
	Daniil Dulov <d.dulov@aladdin.ru>, Jakub Kicinski <kuba@kernel.org>,
	Florian Fainelli <florian.fainelli@broadcom.com>,
	Broadcom internal kernel review list <bcm-kernel-feedback-list@broadcom.com>,
	Andrew Lunn <andrew@lunn.ch>,
	Heiner Kallweit <hkallweit1@gmail.com>,
	"David S. Miller" <davem@davemloft.net>,
	Eric Dumazet <edumazet@google.com>, Paolo Abeni <pabeni@redhat.com>,
	netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
	lvc-project@linuxtesting.org
Subject: Re: [PATCH net-next] net: phy: mdio-bcm-unimac: Cast denominator to
 unsigned long to avoid overflow
Message-ID: <ZfCwGF3JRiFdJ353@shell.armlinux.org.uk>
References: <20240312165358.7712-1-d.dulov@aladdin.ru>
 <ZfCOb4x/+41y+SW3@shell.armlinux.org.uk>
 <df295be9-d33e-45d2-914f-c9c1554e5ac0@gmail.com>
 <b1acf9d0-872c-487a-9938-6d667959d0d3@gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <b1acf9d0-872c-487a-9938-6d667959d0d3@gmail.com>
Sender: Russell King (Oracle) <linux@armlinux.org.uk>

On Tue, Mar 12, 2024 at 12:23:20PM -0700, Doug Berger wrote:
> On 3/12/2024 10:23 AM, Florian Fainelli wrote:
> > On 3/12/24 10:18, Russell King (Oracle) wrote:
> > > On Tue, Mar 12, 2024 at 07:53:58PM +0300, Daniil Dulov wrote:
> > > > The expression priv->clk_freq * 2 can lead to overflow that will cause
> > > > a division by zero. So, let's cast it to unsigned long to avoid it.
> > > 
> > > How does casting this help? "unsigned long" can still be 32-bit.
> > > Maybe unimac_mdio_probe() should be validating the value it read from
> > > DT won't overflow? I suspect that a value of 2.1GHz is way too large
> > > for this property in any case.
> > > 
> > > https://en.wikipedia.org/wiki/Management_Data_Input/Output#Electrical_specification
> > > 
> > > (note, this driver is clause-22 only.)
> > > 
> > 
> > Had commented on the previous version (not sure why this was not
> > prefixed with v2) that the maximum clock frequency for this clock is
> > 250MHz, the driver could check that to prevent for an overflow, most
> > certainly.
> 
> Could also use:
> -	div = (rate / (2 * priv->clk_freq)) - 1;
> +	div = ((rate / priv->clk_freq) >> 1) - 1;
> which is mathematically equivalent without the risk of overflow.

What's the point when the maximum clock frequency that the driver should
allow fits within u32, nay u28?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!