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

[Peng Fan]

+ Amit

On Tue, Aug 05, 2025 at 10:22:10AM +0100, Andrew Goodbody wrote:
>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?

Amit may help comment 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.

I am fine with the changes, but need Amit who is the author of the driver
to give an Ack

Thanks,
Peng

>
>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>
>> > 
>