| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 6 Jan 2022 20:19:39 +0200
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: Gabriel Somlo <gsomlo@...il.com>
Cc: Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
Rob Herring <robh+dt@...nel.org>,
devicetree <devicetree@...r.kernel.org>,
Ulf Hansson <ulf.hansson@...aro.org>,
linux-mmc <linux-mmc@...r.kernel.org>,
Karol Gugala <kgugala@...micro.com>,
Mateusz Holenko <mholenko@...micro.com>, krakoczy@...micro.com,
mdudek@...ernships.antmicro.com, paulus@...abs.org,
Joel Stanley <joel@....id.au>,
Stafford Horne <shorne@...il.com>,
Geert Uytterhoeven <geert@...ux-m68k.org>,
david.abdurachmanov@...ive.com, florent@...oy-digital.fr,
Randy Dunlap <rdunlap@...radead.org>
Subject: Re: [PATCH v6 3/3] mmc: Add driver for LiteX's LiteSDCard interface
On Thu, Jan 6, 2022 at 7:48 PM Gabriel Somlo <gsomlo@...il.com> wrote:
>
> LiteX (https://github.com/enjoy-digital/litex) is a SoC framework
> that targets FPGAs. LiteSDCard is a small footprint, configurable
> SDCard core commonly used in LiteX designs.
>
> The driver was first written in May 2020 and has been maintained
> cooperatively by the LiteX community. Thanks to all contributors!
> +config MMC_LITEX
> + tristate "LiteX MMC Host Controller support"
> + depends on OF
> + depends on PPC_MICROWATT || LITEX || COMPILE_TEST
> + help
> + This selects support for the MMC Host Controller found in LiteX SoCs.
> +
> + If unsure, say N.
What would be the module name if built as a module?
...
> +/*
> + * LiteX LiteSDCard driver
> + *
> + * Copyright (C) 2019-2020 Antmicro <contact@...micro.com>
> + * Copyright (C) 2019-2020 Kamil Rakoczy <krakoczy@...micro.com>
> + * Copyright (C) 2019-2020 Maciej Dudek <mdudek@...ernships.antmicro.com>
> + * Copyright (C) 2020 Paul Mackerras <paulus@...abs.org>
> + * Copyright (C) 2020-2021 Gabriel Somlo <gsomlo@...il.com>
> + *
Redundant blank line.
> + */
...
> +#include <linux/module.h>
> +#include <linux/litex.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/clk.h>
> +#include <linux/interrupt.h>
> +#include <linux/iopoll.h>
> +#include <linux/mmc/sd.h>
> +#include <linux/mmc/mmc.h>
> +#include <linux/mmc/host.h>
> +#include <linux/delay.h>
> +#include <linux/dma-mapping.h>
Perhaps keep it sorted?
It will easily show, for example, absence of bits.h.
...
> + ret = readx_poll_timeout(litex_read8, reg, evt, (evt & SD_BIT_DONE),
Too many parentheses.
> + SD_SLEEP_US, SD_TIMEOUT_US);
> + if (ret || (evt & SD_BIT_TIMEOUT))
Redundant second condition. If you want +1 iteration, increase timeout.
> + return -ETIMEDOUT;
Why shadowed error code?
...
> + pr_err("%s: unknown error evt=%x\n", __func__, evt);
Use dev_err().
...
> + /* Wait for an interrupt if we have an interrupt and either there is
> + * data to be transferred, or if the card can report busy via DAT0.
> + */
This comment style is for the net subsystem, for others we use
/*
* Starting here...
*/
Fix it everywhere in your code.
...
> + reg = host->sdcore + LITEX_CORE_CMDRSP;
> + for (i = 0; i < 4; i++) {
> + host->resp[i] = litex_read32(reg);
> + reg += sizeof(u32);
> + }
Isn't it memcpy_fromio()?
...
> + if (!host->app_cmd && cmd == SD_SEND_RELATIVE_ADDR)
> + host->rca = (host->resp[3] >> 16) & 0xffff;
Are you expecting a 32-bit value to be bigger than 2^32-1?
...
> + div = min(max(div, 2U), 256U);
clamp_t() / clamp_val() ?
...
> + ret = platform_get_irq_optional(host->dev, 0);
> + if (ret == -ENXIO || ret == 0) {
> + dev_warn(dev, "Failed to get IRQ, using polling\n");
> + goto use_polling;
> + }
> + if (ret < 0)
> + return ret; /* e.g., deferred probe */
> + host->irq = ret;
Can it be rather written as
ret = platform_get_irq_optional(host->dev, 0);
if (ret < 0 && ret != -ENXIO)
return ret;
if (ret > 0)
host->irq = ret;
else {
dev_warn(dev, "Failed to get IRQ, using polling\n");
goto use_polling;
}
?
...
> +use_polling:
> + host->mmc->caps |= MMC_CAP_NEEDS_POLL;
> + host->irq = 0;
Isn't it 0 by default?
...
> + mmc = mmc_alloc_host(sizeof(struct litex_mmc_host), &pdev->dev);
> + /* NOTE: defaults to max_[req,seg]_size=PAGE_SIZE, max_blk_size=512,
> + * and max_blk_count accordingly set to 8;
> + * If for some reason we need to modify max_blk_count, we must also
> + * re-calculate `max_[req,seg]_size = max_blk_size * max_blk_count;`
> + */
Can you rather not split code by this comment. It makes sense to be above, no?
> + if (!mmc)
> + return -ENOMEM;
...
> + /* set default sd_clk frequency range based on empirical observations
> + * of LiteSDCard gateware behavior on typical SDCard media
> + */
Start sentences from capital letters and keep proper style of
multi-line comments.
...
> +err:
> + mmc_free_host(mmc);
> + return ret;
This...
> +}
> +
> +static int litex_mmc_remove(struct platform_device *pdev)
> +{
> + struct litex_mmc_host *host = dev_get_drvdata(&pdev->dev);
> +
> + if (host->irq > 0)
> + free_irq(host->irq, host->mmc);
> + mmc_remove_host(host->mmc);
> + mmc_free_host(host->mmc);
...and this have ordering issues. You mixed devm_*() with non-devm_*()
APIs in the wrong way.
Also, I haven't noticed the free_irq() call in the error path of
->probe(). Isn't it missed?
> + return 0;
> +}
...
> + .of_match_table = of_match_ptr(litex_match),
Wrong usage of of_match_ptr().
--
With Best Regards,
Andy Shevchenko
Powered by blists - more mailing lists