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;
Message-ID: <88b6dab2-87b1-34ef-b267-43933d79ab8e@ti.com>
Date:   Thu, 24 Nov 2022 17:57:10 +0530
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.4.2
Subject: Re: [PATCH] spi: cadence-quadspi: Add upper limit safety check to
 baudrate divisor
Content-Language: en-US
To:     Mark Brown <broonie@kernel.org>
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>
References: <20221123211705.126900-1-nathan.morrison@timesys.com>
 <9e5264fa-db1a-ed96-5fd8-cbfa4694b8bd@ti.com>
 <Y39XFzYJL3EmxSFF@sirena.org.uk>
From:   Dhruva Gole <d-gole@ti.com>
In-Reply-To: <Y39XFzYJL3EmxSFF@sirena.org.uk>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

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.


-- 
Thanks and Regards,
Dhruva Gole