Message ID | 20221123211705.126900-1-nathan.morrison@timesys.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | spi: cadence-quadspi: Add upper limit safety check to baudrate divisor | expand |
Hi Nathan, Thanks for your contribution. However, there are a few issues that I would like you to address. On 24/11/22 02:47, Nathan Barrett-Morrison wrote: > While bringing up the cadence-quadspi driver on a customer board, > I discovered that the baud divisor calculation can exceed the > peripheral's maximum in some circumstances. This will prevent it. What is the peripheral's maximum? Is the peripheral a flash? Please define what you mean by "some circumstances". > > Signed-off-by: Nathan Barrett-Morrison <nathan.morrison@timesys.com> > --- > drivers/spi/spi-cadence-quadspi.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c > index 447230547945..250575fb7b0e 100644 > --- a/drivers/spi/spi-cadence-quadspi.c > +++ b/drivers/spi/spi-cadence-quadspi.c > @@ -1119,6 +1119,10 @@ static void cqspi_config_baudrate_div(struct cqspi_st *cqspi) > /* Recalculate the baudrate divisor based on QSPI specification. */ > div = DIV_ROUND_UP(ref_clk_hz, 2 * cqspi->sclk) - 1; > > + /* Maximum baud divisor */ > + if (div > CQSPI_REG_CONFIG_BAUD_MASK) I don't think comparing "greater than" with a MASK is atall a good idea. > + div = CQSPI_REG_CONFIG_BAUD_MASK; I would not encourage this either. > + > reg = readl(reg_base + CQSPI_REG_CONFIG); > reg &= ~(CQSPI_REG_CONFIG_BAUD_MASK << CQSPI_REG_CONFIG_BAUD_LSB); > reg |= (div & CQSPI_REG_CONFIG_BAUD_MASK) << CQSPI_REG_CONFIG_BAUD_LSB; Either come up with a better MACRO, or if what I understand is correct, the peripheral's max value will depend, well on the _peripheral_ in which case it is that "peripheral" driver's responsibility to properly tell the controller what to do. Again, I don't fully understand your situation is as in what is the peripheral you are using. So please elaborate on that. Importantly, I would suggest that you _NEVER_ compare ANY value to a MASK Macro. MASK Macros are meant to MASK bits.
On Thu, Nov 24, 2022 at 12:16:10PM +0530, Dhruva Gole wrote: > On 24/11/22 02:47, Nathan Barrett-Morrison wrote: > > + /* Maximum baud divisor */ > > + if (div > CQSPI_REG_CONFIG_BAUD_MASK) > I don't think comparing "greater than" with a MASK is atall a good idea. Why - it's checking that the calculated divisor can actually fit in the relevant register field which seems like a totally normal thing to do? > Again, I don't fully understand your situation is as in > what is the peripheral you are using. So please elaborate on that. 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. In this context the SPI controller is a peripheral within the SoC. > Importantly, I would suggest that you _NEVER_ compare ANY value to a > MASK Macro. MASK Macros are meant to MASK bits. It's very common to also use masks to identify when values have overflowed the values that can be written to a field.
On Wed, Nov 23, 2022 at 04:17:05PM -0500, Nathan Barrett-Morrison wrote: > + /* Maximum baud divisor */ > + if (div > CQSPI_REG_CONFIG_BAUD_MASK) > + div = CQSPI_REG_CONFIG_BAUD_MASK; This will fix the overflow of the divisor but it means that we'll be generating a faster clock than the device asked for which might lead to problems. We should at the very least warn, though returning an error would be safer. Ideally we'd be able to adjust the input clock to the SPI controller to allow us to divide out an appropriate clock but that's more disruptive.
Hi Mark & Dhruva, Your understanding is correct. This is just checking if the divisor field has exceed the bit field's full scale (0xF) in this case. This was observed when we had a reference block of 500MHz and a max SPI clock of 10MHz setting. 500000000/2*10000000 = 25 25 > 0xF (15) Would you like me to add a dev_err (or such) bailout error condition and resubmit? Sincerely, Nathan
On Thu, Nov 24, 2022 at 06:53:54AM -0500, Nathan Barrett-Morrison wrote:
> Would you like me to add a dev_err (or such) bailout error condition and resubmit?
Yes, please. A bit of rewording to clarify the commit log might help as
well.
Hi Mark, Thanks for your clarification. On 24/11/22 17:05, Mark Brown wrote: > On Thu, Nov 24, 2022 at 12:16:10PM +0530, Dhruva Gole wrote: >> On 24/11/22 02:47, Nathan Barrett-Morrison wrote: > >>> + /* Maximum baud divisor */ >>> + if (div > CQSPI_REG_CONFIG_BAUD_MASK) > >> I don't think comparing "greater than" with a MASK is atall a good idea. > > Why - it's checking that the calculated divisor can actually fit in the > relevant register field which seems like a totally normal thing to do? okay, it makes sense in the sense that it will cap the div rate to 0xF. > >> Again, I don't fully understand your situation is as in >> what is the peripheral you are using. So please elaborate on that. > > 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? > > In this context the SPI controller is a peripheral within the SoC. Ah okay, my understanding was that one would call a peripheral something that is connected via a SPI Bus to the SPI controller. > >> Importantly, I would suggest that you _NEVER_ compare ANY value to a >> MASK Macro. MASK Macros are meant to MASK bits. > > It's very common to also use masks to identify when values have > overflowed the values that can be written to a field. okay, this does make sense when the code doesn't mask the values before modifying the registers. However as I showed above, there is a masking done of div before setting the bits in the reg. I agree there is the other justification to use the BAUD_MASK macro to cap the div value to maximum if it is larger than maximum. However as you said as well, This will fix the overflow of the divisor but it means that we'll be generating a faster clock than the device asked for which might lead to problems. 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. As the commit said, "can exceed the peripheral's maximum in some circumstances. This will prevent it." The prevent it part does not seem to be special to his patch, because anyway we were masking the bits so the value wont exceed as such in register.
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).
diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c index 447230547945..250575fb7b0e 100644 --- a/drivers/spi/spi-cadence-quadspi.c +++ b/drivers/spi/spi-cadence-quadspi.c @@ -1119,6 +1119,10 @@ static void cqspi_config_baudrate_div(struct cqspi_st *cqspi) /* Recalculate the baudrate divisor based on QSPI specification. */ div = DIV_ROUND_UP(ref_clk_hz, 2 * cqspi->sclk) - 1; + /* Maximum baud divisor */ + if (div > CQSPI_REG_CONFIG_BAUD_MASK) + div = CQSPI_REG_CONFIG_BAUD_MASK; + reg = readl(reg_base + CQSPI_REG_CONFIG); reg &= ~(CQSPI_REG_CONFIG_BAUD_MASK << CQSPI_REG_CONFIG_BAUD_LSB); reg |= (div & CQSPI_REG_CONFIG_BAUD_MASK) << CQSPI_REG_CONFIG_BAUD_LSB;
While bringing up the cadence-quadspi driver on a customer board, I discovered that the baud divisor calculation can exceed the peripheral's maximum in some circumstances. This will prevent it. Signed-off-by: Nathan Barrett-Morrison <nathan.morrison@timesys.com> --- drivers/spi/spi-cadence-quadspi.c | 4 ++++ 1 file changed, 4 insertions(+)