[PATCH] mmc: owl_mmc: Do not dereference data before NULL check

[Andrew Goodbody]

On 05/08/2025 05:00, Peng Fan wrote:
> On Thu, Jul 31, 2025 at 12:11:47PM +0100, Andrew Goodbody wrote:
>> In owl_mmc_prepare_data there is a NULL check for the pointer data but
>> it happens after data has already been dereferenced. Refactor the code
>> so that the NULL check happens before any code dereferences data.
>>
>> This issue was found by Smatch.
>>
>> Signed-off-by: Andrew Goodbody <andrew.goodbody at linaro.org>
>> ---
>> drivers/mmc/owl_mmc.c | 18 +++++++++---------
>> 1 file changed, 9 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/mmc/owl_mmc.c b/drivers/mmc/owl_mmc.c
>> index bd4906f58e7..c18807b1f49 100644
>> --- a/drivers/mmc/owl_mmc.c
>> +++ b/drivers/mmc/owl_mmc.c
>> @@ -135,19 +135,19 @@ static void owl_mmc_prepare_data(struct owl_mmc_priv *priv,
>>
>> 	setbits_le32(priv->reg_base + OWL_REG_SD_EN, OWL_SD_EN_BSEL);
>>
>> -	writel(data->blocks, priv->reg_base + OWL_REG_SD_BLK_NUM);
>> -	writel(data->blocksize, priv->reg_base + OWL_REG_SD_BLK_SIZE);
>> -	total = data->blocksize * data->blocks;
>> -
>> -	if (total < 512)
>> -		writel(total, priv->reg_base + OWL_REG_SD_BUF_SIZE);
>> -	else
>> -		writel(512, priv->reg_base + OWL_REG_SD_BUF_SIZE);
>> -
>> 	/* DMA STOP */
>> 	writel(0x0, SD_DMA_CHANNEL(priv->dma_channel, 0) + DMA_START);
>>
>> 	if (data) {
>> +		writel(data->blocks, priv->reg_base + OWL_REG_SD_BLK_NUM);
>> +		writel(data->blocksize, priv->reg_base + OWL_REG_SD_BLK_SIZE);
>> +		total = data->blocksize * data->blocks;
>> +
>> +		if (total < 512)
>> +			writel(total, priv->reg_base + OWL_REG_SD_BUF_SIZE);
>> +		else
>> +			writel(512, priv->reg_base + OWL_REG_SD_BUF_SIZE);
>> +
> 
> This piece code is moved to after 'DMA STOP', so code logic is changed.
> This may not be expected.

Are you familiar with this hardware or is there anyone on the list who 
is that could comment on the preferred order of actions here?

This was changed for two reasons. Firstly and of least importance, it 
makes the code tidier by needing just a single NULL check for 'data'. 
Secondly I wonder about the correctness of the original code. It seems a 
little strange to me to be writing to the device registers while the DMA 
is possibly active. This could potentially lead to undesired effects. I 
would expect that stopping the DMA before changing the registers would 
be a safer order to perform these actions. However I do admit that this 
is just speculation on my part and that this order could be fine or even 
necessary for some reason, but that would be.
Also, of course, in the case that 'data' is NULL, the writes that were 
before the call to stop the DMA cannot be executed as they dereference 
'data'.

So yes this is a bit of a speculative change but I think it would be OK.
But if you would rather I rework it to keep the ordering as before then 
I will do that. Please let me know.

Andrew

> Thanks,
> Peng
> 
>> 		if (data->flags == MMC_DATA_READ) {
>> 			buf = (ulong) (data->dest);
>> 			owl_dma_config(priv, (ulong) priv->reg_base +
>>
>> ---
>> base-commit: 79f3e77133bd7248e4579827effc13f97a32a8a8
>> change-id: 20250731-owl_mmc-ef16e32b68fd
>>
>> Best regards,
>> -- 
>> Andrew Goodbody <andrew.goodbody at linaro.org>
>>