Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 24 Nov 2022 12:41:54 +0000
From:   Mark Brown <broonie@kernel.org>
To:     Dhruva Gole <d-gole@ti.com>
Cc:     Nathan Barrett-Morrison <nathan.morrison@timesys.com>,
        greg.malysa@timesys.com,
        "open list:SPI SUBSYSTEM" <linux-spi@vger.kernel.org>,
        open list <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] spi: cadence-quadspi: Add upper limit safety check to
 baudrate divisor
Message-ID: <Y39mkl+9W8S6ZzOk@sirena.org.uk>
References: <20221123211705.126900-1-nathan.morrison@timesys.com>
 <9e5264fa-db1a-ed96-5fd8-cbfa4694b8bd@ti.com>
 <Y39XFzYJL3EmxSFF@sirena.org.uk>
 <88b6dab2-87b1-34ef-b267-43933d79ab8e@ti.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
        protocol="application/pgp-signature"; boundary="BSJOWebIfHRbS2Ss"
Content-Disposition: inline
In-Reply-To: <88b6dab2-87b1-34ef-b267-43933d79ab8e@ti.com>
Precedence: bulk


--BSJOWebIfHRbS2Ss
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Thu, Nov 24, 2022 at 05:57:10PM +0530, Dhruva Gole wrote:
> On 24/11/22 17:05, Mark Brown wrote:

> > As far as I can tell the issue here is that the device is asking for a
> > rate which requires a larger divisor than the controller can support but
> > the driver doesn't do any bounds checking so it just writes the
> > calculated divisor out to the hardware, corrupting any adjacent fields.

> but, I am not sure it would anyway corrupt any adjacent bits,

> The code
> reg |= (div & CQSPI_REG_CONFIG_BAUD_MASK) << CQSPI_REG_CONFIG_BAUD_LSB

> does mask the div value to ensure bits ONLY in CQSPI_REG_CONFIG_BAUD_MASK
> region are set and nothing else right?

Yes, that'd avoid corrupting adjacent bits (though it'd still be making
things worse in that it makes the divider smaller).

> I believe a simple warning is enough, and better not touch the div variable
> because it seems unnecessary. We already have a mask to take care of masking
> the appropriate bits.

That'd still leave the clock driven too fast which could break things,
going for the maximum divider would mitigate this (though an error would
be even safer).

--BSJOWebIfHRbS2Ss
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQEzBAABCgAdFiEEreZoqmdXGLWf4p/qJNaLcl1Uh9AFAmN/ZpEACgkQJNaLcl1U
h9AvPAf9G6Ky3lmquOrNVgL0eNAxmRCd4YVvOTlKeIHNJrNv6TT320J/DzrVN4w+
9ktTV3ZW4wsBcN1P3zz6nz8lwIofuAK+exw90LI2BduuLQ99UWguHRTZ5Boe9LDf
xnFPHZmYF3HwIDC+S854MAKeEv99++By+wMtrhTBzv/OMF1OPynedrcmhNLYDZF6
V8Hca5elzIKlOI0jF8GZ1tYllW50vvdNbgsCVpD5cUxtlsZDOuqLuH40xmS6DZ6A
87YYdtrvk4LbTpVumgtypvnzECMDx+Jya1OSjqcRrEY9UDFklb2EX0RUJspLMpA7
A2qJeq9A2Gf0/ULY7DAUK3T2I34JeA==
=ZHIr
-----END PGP SIGNATURE-----

--BSJOWebIfHRbS2Ss--