+ 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.goodb...@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.goodb...@linaro.org>
>> >
>
