Because of this change, the driver now expects a pinctrl device
reference in the mmc controller's device tree node; without it, it will
bail out. This could break existing setups that don't specify it
because it "just worked" up until now. So currently I just let the old
behavior fall away because this is a staging driver. But if this is a
problem, the old behavior could be added back as a fallback.
This is version 2 of a patchset that I requested feedback for about a
month ago. Please review as if they are a new patchset; all the patches
were rebased several times and a couple new correctness fixes added.
The TODO list is largely unchanged, aside from the couple of TODO
comments in the code that I have addressed. Ultimately, I think this
driver could potentially be merged with the "real" mtk-mmc driver as the
TODO suggests, but someone who is more familiar with the IP core will
have to do that. Mediatek documentation (that I can find) is very
sparse.
This is ready to merge if there is no other feedback!
From George Hilliard <[email protected]> # This line is ignored.
From: George Hilliard <[email protected]>
Reply-To:
Subject: [PATCH v2 00/11] mt7621-mmc: Various correctness fixes
In-Reply-To:
The compiler complains:
drivers/staging/mt7621-mmc/dbg.c: In function ‘msdc_debug_proc_write’:
drivers/staging/mt7621-mmc/dbg.c:237:12: warning: unused variable ‘size’ [-Wunused-variable]
int mode, size;
^~~~
drivers/staging/mt7621-mmc/dbg.c:237:6: warning: unused variable ‘mode’ [-Wunused-variable]
int mode, size;
^~~~
Remove these declarations.
Signed-off-by: George Hilliard <[email protected]>
---
drivers/staging/mt7621-mmc/dbg.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/staging/mt7621-mmc/dbg.c b/drivers/staging/mt7621-mmc/dbg.c
index 6b59ac8b323a..fda3ba38ba37 100644
--- a/drivers/staging/mt7621-mmc/dbg.c
+++ b/drivers/staging/mt7621-mmc/dbg.c
@@ -232,7 +232,6 @@ static ssize_t msdc_debug_proc_write(struct file *file,
int cmd, p1, p2;
int id, zone;
- int mode, size;
if (count == 0)
return -1;
--
2.21.0
A failure while processing the start command could cause dma_unmap_sg()
to be called without first calling dma_map_sg().
Since calling dma_unmap_sg() is only needed when data != NULL, move the
unmap call into the corresponding if {} block.
Signed-off-by: George Hilliard <[email protected]>
---
drivers/staging/mt7621-mmc/sd.c | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/drivers/staging/mt7621-mmc/sd.c b/drivers/staging/mt7621-mmc/sd.c
index b52e0284ea8e..f14ff75cc4b7 100644
--- a/drivers/staging/mt7621-mmc/sd.c
+++ b/drivers/staging/mt7621-mmc/sd.c
@@ -695,7 +695,7 @@ static int msdc_do_request(struct mmc_host *mmc, struct mmc_request *mrq)
/* then wait command done */
if (msdc_command_resp(host, cmd, 1, CMD_TIMEOUT) != 0)
- goto done;
+ goto unmap;
/* for read, the data coming too fast, then CRC error
* start DMA no business with CRC.
@@ -732,18 +732,17 @@ static int msdc_do_request(struct mmc_host *mmc, struct mmc_request *mrq)
/* Last: stop transfer */
if (data->stop) {
if (msdc_do_command(host, data->stop, 0, CMD_TIMEOUT) != 0)
- goto done;
+ goto unmap;
}
- }
-done:
- if (data) {
+unmap:
host->data = NULL;
dma_unmap_sg(mmc_dev(mmc), data->sg, data->sg_len,
mmc_get_dma_dir(data));
host->blksz = 0;
}
+done:
if (mrq->cmd->error)
host->error = 0x001;
if (mrq->data && mrq->data->error)
--
2.21.0
There is no need to delay notifying the mmc layer. Schedule the delayed
work to run immediately.
Signed-off-by: George Hilliard <[email protected]>
---
drivers/staging/mt7621-mmc/sd.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/mt7621-mmc/sd.c b/drivers/staging/mt7621-mmc/sd.c
index df85888f3a03..f64d51c8bd42 100644
--- a/drivers/staging/mt7621-mmc/sd.c
+++ b/drivers/staging/mt7621-mmc/sd.c
@@ -1352,7 +1352,7 @@ static irqreturn_t msdc_irq(int irq, void *dev_id)
if (intsts & MSDC_INT_CDSC) {
if (host->mmc->caps & MMC_CAP_NEEDS_POLL)
return IRQ_HANDLED;
- schedule_delayed_work(&host->card_delaywork, HZ);
+ schedule_delayed_work(&host->card_delaywork, 0);
/* tuning when plug card ? */
}
--
2.21.0
This is set once during initialization and never changed. Don't bother
setting it again in the interrupt handler.
Signed-off-by: George Hilliard <[email protected]>
---
drivers/staging/mt7621-mmc/sd.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/staging/mt7621-mmc/sd.c b/drivers/staging/mt7621-mmc/sd.c
index 31c5b44bd29f..df85888f3a03 100644
--- a/drivers/staging/mt7621-mmc/sd.c
+++ b/drivers/staging/mt7621-mmc/sd.c
@@ -209,7 +209,6 @@ static void msdc_tasklet_card(struct work_struct *work)
host->card_inserted = inserted;
if (!host->suspend) {
- host->mmc->f_max = HOST_MAX_MCLK;
mmc_detect_change(host->mmc, msecs_to_jiffies(20));
}
--
2.21.0
The driver previously grabbed the SD pins for itself, ignoring the pin
controller. Remove this, and allow the pinctrl subsystem to set up the
pins using the device tree mappings. This allows this driver to work on
related devices that have a different pin controller mapping, such as
the MT7688. The hardcoded bit index was incorrect on that device.
The driver now needs a pin controller reference in its device tree node:
sdhci: /* ... */ {
compatible = "ralink,mt7620-sdhci";
pinctrl-names = "default";
pinctrl-0 = <&sdhci_pins>;
// ...
};
This change could break SD controller functionality on existing devices
whose device trees do not specify a pin controller and state for the SD
node.
Signed-off-by: George Hilliard <[email protected]>
---
drivers/staging/mt7621-mmc/sd.c | 6 ------
1 file changed, 6 deletions(-)
diff --git a/drivers/staging/mt7621-mmc/sd.c b/drivers/staging/mt7621-mmc/sd.c
index 306b3b46f7c9..4380b321bc73 100644
--- a/drivers/staging/mt7621-mmc/sd.c
+++ b/drivers/staging/mt7621-mmc/sd.c
@@ -1820,12 +1820,6 @@ static struct platform_driver mt_msdc_driver = {
static int __init mt_msdc_init(void)
{
int ret;
- u32 reg;
-
- // Set the pins for sdxc to sdxc mode
- //FIXME: this should be done by pinctl and not by the sd driver
- reg = readl((void __iomem *)(RALINK_SYSCTL_BASE + 0x60)) & ~(0x3 << 18);
- writel(reg, (void __iomem *)(RALINK_SYSCTL_BASE + 0x60));
ret = platform_driver_register(&mt_msdc_driver);
if (ret) {
--
2.21.0
The module was initializing completions whenever it was going to wait on
them, and not when the completion was allocated. This is incorrect
according to the completion docs:
Calling init_completion() on the same completion object twice is
most likely a bug [...]
Re-initialization is also unnecessary because the module never uses
complete_all(). Fix this by only ever initializing the completion a
single time.
Signed-off-by: George Hilliard <[email protected]>
---
drivers/staging/mt7621-mmc/sd.c | 13 +++++++++----
1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/drivers/staging/mt7621-mmc/sd.c b/drivers/staging/mt7621-mmc/sd.c
index 89fbc0a1dec7..c272aa780719 100644
--- a/drivers/staging/mt7621-mmc/sd.c
+++ b/drivers/staging/mt7621-mmc/sd.c
@@ -467,7 +467,9 @@ static unsigned int msdc_command_start(struct msdc_host *host,
host->cmd = cmd;
host->cmd_rsp = resp;
- init_completion(&host->cmd_done);
+ // The completion should have been consumed by the previous command
+ // response handler, because the mmc requests should be serialized
+ BUG_ON(completion_done(&host->cmd_done));
sdr_set_bits(host->base + MSDC_INTEN, wints);
sdc_send_cmd(rawcmd, cmd->arg);
@@ -489,7 +491,6 @@ static unsigned int msdc_command_resp(struct msdc_host *host,
MSDC_INT_ACMD19_DONE;
BUG_ON(in_interrupt());
- //init_completion(&host->cmd_done);
//sdr_set_bits(host->base + MSDC_INTEN, wints);
spin_unlock(&host->lock);
@@ -673,7 +674,10 @@ static int msdc_do_request(struct mmc_host *mmc, struct mmc_request *mrq)
//msdc_clr_fifo(host); /* no need */
msdc_dma_on(); /* enable DMA mode first!! */
- init_completion(&host->xfer_done);
+
+ // The completion should have been consumed by the previous xfer
+ // response handler, because the mmc requests should be serialized
+ BUG_ON(completion_done(&host->xfer_done));
/* start the command first*/
if (msdc_command_start(host, cmd, CMD_TIMEOUT) != 0)
@@ -692,7 +696,6 @@ static int msdc_do_request(struct mmc_host *mmc, struct mmc_request *mrq)
/* for read, the data coming too fast, then CRC error
* start DMA no business with CRC.
*/
- //init_completion(&host->xfer_done);
msdc_dma_start(host);
spin_unlock(&host->lock);
@@ -1684,6 +1687,8 @@ static int msdc_drv_probe(struct platform_device *pdev)
}
msdc_init_gpd_bd(host, &host->dma);
+ init_completion(&host->cmd_done);
+ init_completion(&host->xfer_done);
INIT_DELAYED_WORK(&host->card_delaywork, msdc_tasklet_card);
spin_lock_init(&host->lock);
msdc_init_hw(host);
--
2.21.0
The kernel complained:
[ 510.277151] WARNING: CPU: 0 PID: 395 at fs/proc/generic.c:360 proc_register+0xf0/0x108
[ 510.292891] proc_dir_entry '/proc/msdc_debug' already registered
when doing a modprobe/rmmod/modprobe of this module if debug messages
are compiled in. Fix this by removing the proc entry when the module is
unloaded.
Signed-off-by: George Hilliard <[email protected]>
---
drivers/staging/mt7621-mmc/dbg.c | 15 ++++++++++++---
drivers/staging/mt7621-mmc/dbg.h | 3 ++-
drivers/staging/mt7621-mmc/sd.c | 4 ++++
3 files changed, 18 insertions(+), 4 deletions(-)
diff --git a/drivers/staging/mt7621-mmc/dbg.c b/drivers/staging/mt7621-mmc/dbg.c
index fda3ba38ba37..2310f3bcc16e 100644
--- a/drivers/staging/mt7621-mmc/dbg.c
+++ b/drivers/staging/mt7621-mmc/dbg.c
@@ -294,9 +294,18 @@ static const struct file_operations msdc_debug_fops = {
.release = single_release,
};
-void msdc_debug_proc_init(void)
+// Keep ahold of the proc entry we create so it can be freed during
+// module removal
+struct proc_dir_entry *msdc_debug_proc_entry;
+
+void __init msdc_debug_proc_init(void)
{
- proc_create("msdc_debug", 0660, NULL, &msdc_debug_fops);
+ msdc_debug_proc_entry = proc_create("msdc_debug", 0660,
+ NULL, &msdc_debug_fops);
+}
+
+void __exit msdc_debug_proc_deinit(void)
+{
+ proc_remove(msdc_debug_proc_entry);
}
-EXPORT_SYMBOL_GPL(msdc_debug_proc_init);
#endif
diff --git a/drivers/staging/mt7621-mmc/dbg.h b/drivers/staging/mt7621-mmc/dbg.h
index 2d447b2d92ae..d100324aa3fe 100644
--- a/drivers/staging/mt7621-mmc/dbg.h
+++ b/drivers/staging/mt7621-mmc/dbg.h
@@ -93,7 +93,8 @@ enum msdc_dbg {
extern unsigned int sd_debug_zone[4];
#define TAG "msdc"
-void msdc_debug_proc_init(void);
+void __init msdc_debug_proc_init(void);
+void __exit msdc_debug_proc_deinit(void);
u32 msdc_time_calc(u32 old_L32, u32 old_H32, u32 new_L32, u32 new_H32);
void msdc_performance(u32 opcode, u32 sizes, u32 bRx, u32 ticks);
diff --git a/drivers/staging/mt7621-mmc/sd.c b/drivers/staging/mt7621-mmc/sd.c
index 9074848a8251..306b3b46f7c9 100644
--- a/drivers/staging/mt7621-mmc/sd.c
+++ b/drivers/staging/mt7621-mmc/sd.c
@@ -1841,6 +1841,10 @@ static int __init mt_msdc_init(void)
static void __exit mt_msdc_exit(void)
{
+#if defined(MT6575_SD_DEBUG)
+ msdc_debug_proc_deinit();
+#endif
+
platform_driver_unregister(&mt_msdc_driver);
}
--
2.21.0
These comments don't contain useful code or alternate implementation
ideas. Remove them.
Signed-off-by: George Hilliard <[email protected]>
---
drivers/staging/mt7621-mmc/mt6575_sd.h | 3 ---
drivers/staging/mt7621-mmc/sd.c | 2 --
2 files changed, 5 deletions(-)
diff --git a/drivers/staging/mt7621-mmc/mt6575_sd.h b/drivers/staging/mt7621-mmc/mt6575_sd.h
index 038a484a9476..8b6cf17175cd 100644
--- a/drivers/staging/mt7621-mmc/mt6575_sd.h
+++ b/drivers/staging/mt7621-mmc/mt6575_sd.h
@@ -39,8 +39,6 @@
#include <linux/bitops.h>
#include <linux/mmc/host.h>
-// #include <mach/mt6575_reg_base.h> /* --- by chhung */
-
/*--------------------------------------------------------------------------*/
/* Common Definition */
/*--------------------------------------------------------------------------*/
@@ -418,7 +416,6 @@ struct msdc_host {
int error;
spinlock_t lock; /* mutex */
- struct semaphore sem;
u32 blksz; /* host block size */
void __iomem *base; /* host base address */
diff --git a/drivers/staging/mt7621-mmc/sd.c b/drivers/staging/mt7621-mmc/sd.c
index 049b312131a9..9074848a8251 100644
--- a/drivers/staging/mt7621-mmc/sd.c
+++ b/drivers/staging/mt7621-mmc/sd.c
@@ -100,7 +100,6 @@ static int cd_active_low = 1;
struct msdc_hw msdc0_hw = {
.clk_src = 0,
.flags = MSDC_CD_PIN_EN | MSDC_REMOVABLE,
-// .flags = MSDC_WP_PIN_EN | MSDC_CD_PIN_EN | MSDC_REMOVABLE,
};
/* end of +++ */
@@ -1669,7 +1668,6 @@ static int msdc_drv_probe(struct platform_device *pdev)
host->timeout_clks = DEFAULT_DTOC * 65536;
host->mrq = NULL;
- //init_MUTEX(&host->sem); /* we don't need to support multiple threads access */
dma_coerce_mask_and_coherent(mmc_dev(mmc), DMA_BIT_MASK(32));
--
2.21.0
When waiting on completions, use the _io variant so the caller is
charged as using I/O.
This should have no effect on the module's functionality, only improve
CPU accounting.
Signed-off-by: George Hilliard <[email protected]>
---
drivers/staging/mt7621-mmc/sd.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/mt7621-mmc/sd.c b/drivers/staging/mt7621-mmc/sd.c
index 4380b321bc73..89fbc0a1dec7 100644
--- a/drivers/staging/mt7621-mmc/sd.c
+++ b/drivers/staging/mt7621-mmc/sd.c
@@ -493,7 +493,7 @@ static unsigned int msdc_command_resp(struct msdc_host *host,
//sdr_set_bits(host->base + MSDC_INTEN, wints);
spin_unlock(&host->lock);
- if (!wait_for_completion_timeout(&host->cmd_done, 10 * timeout)) {
+ if (!wait_for_completion_io_timeout(&host->cmd_done, 10 * timeout)) {
dev_err(mmc_dev(host->mmc),
"%d -> XXX CMD<%d> wait_for_completion timeout ARG<0x%.8x>\n",
host->id, opcode, cmd->arg);
@@ -696,7 +696,7 @@ static int msdc_do_request(struct mmc_host *mmc, struct mmc_request *mrq)
msdc_dma_start(host);
spin_unlock(&host->lock);
- if (!wait_for_completion_timeout(&host->xfer_done, DAT_TIMEOUT)) {
+ if (!wait_for_completion_io_timeout(&host->xfer_done, DAT_TIMEOUT)) {
dev_err(mmc_dev(host->mmc),
"%d -> XXX CMD<%d> wait xfer_done<%d> timeout!!\n",
host->id, cmd->opcode,
--
2.21.0
Obvious typo. It is specified as BURST in the datasheet.
Signed-off-by: George Hilliard <[email protected]>
---
drivers/staging/mt7621-mmc/mt6575_sd.h | 10 +++++-----
drivers/staging/mt7621-mmc/sd.c | 4 ++--
2 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/staging/mt7621-mmc/mt6575_sd.h b/drivers/staging/mt7621-mmc/mt6575_sd.h
index 8b6cf17175cd..7ee3a9ed328a 100644
--- a/drivers/staging/mt7621-mmc/mt6575_sd.h
+++ b/drivers/staging/mt7621-mmc/mt6575_sd.h
@@ -53,10 +53,10 @@
#define MSDC_BUS_4BITS (1)
#define MSDC_BUS_8BITS (2)
-#define MSDC_BRUST_8B (3)
-#define MSDC_BRUST_16B (4)
-#define MSDC_BRUST_32B (5)
-#define MSDC_BRUST_64B (6)
+#define MSDC_BURST_8B (3)
+#define MSDC_BURST_16B (4)
+#define MSDC_BURST_32B (5)
+#define MSDC_BURST_64B (6)
#define MSDC_PIN_PULL_NONE (0)
#define MSDC_PIN_PULL_DOWN (1)
@@ -280,7 +280,7 @@ enum {
#define MSDC_DMA_CTRL_RESUME (0x1 << 2) /* W */
#define MSDC_DMA_CTRL_MODE (0x1 << 8) /* RW */
#define MSDC_DMA_CTRL_LASTBUF (0x1 << 10) /* RW */
-#define MSDC_DMA_CTRL_BRUSTSZ (0x7 << 12) /* RW */
+#define MSDC_DMA_CTRL_BURSTSZ (0x7 << 12) /* RW */
#define MSDC_DMA_CTRL_XFERSZ (0xffffUL << 16)/* RW */
/* MSDC_DMA_CFG mask */
diff --git a/drivers/staging/mt7621-mmc/sd.c b/drivers/staging/mt7621-mmc/sd.c
index f64d51c8bd42..b52e0284ea8e 100644
--- a/drivers/staging/mt7621-mmc/sd.c
+++ b/drivers/staging/mt7621-mmc/sd.c
@@ -626,8 +626,8 @@ static void msdc_dma_setup(struct msdc_host *host, struct msdc_dma *dma,
}
sdr_set_field(host->base + MSDC_DMA_CFG, MSDC_DMA_CFG_DECSEN, 1);
- sdr_set_field(host->base + MSDC_DMA_CTRL, MSDC_DMA_CTRL_BRUSTSZ,
- MSDC_BRUST_64B);
+ sdr_set_field(host->base + MSDC_DMA_CTRL, MSDC_DMA_CTRL_BURSTSZ,
+ MSDC_BURST_64B);
sdr_set_field(host->base + MSDC_DMA_CTRL, MSDC_DMA_CTRL_MODE, 1);
writel(PHYSADDR((u32)dma->gpd_addr), host->base + MSDC_DMA_SA);
--
2.21.0
The buffer descriptor setup loop is correct only if it is setting up at
least one bd struct. Besides, there is an error somewhere if
dma_map_sg() returns 0. So add a paranoid check for this condition.
Signed-off-by: George Hilliard <[email protected]>
---
drivers/staging/mt7621-mmc/sd.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/staging/mt7621-mmc/sd.c b/drivers/staging/mt7621-mmc/sd.c
index c272aa780719..31c5b44bd29f 100644
--- a/drivers/staging/mt7621-mmc/sd.c
+++ b/drivers/staging/mt7621-mmc/sd.c
@@ -593,7 +593,12 @@ static void msdc_dma_setup(struct msdc_host *host, struct msdc_dma *dma,
struct bd *bd;
u32 j;
- BUG_ON(sglen > MAX_BD_NUM); /* not support currently */
+ // Shouldn't happen; we configure the mmc host layer
+ // based on MAX_BD_NUM:
+ BUG_ON(sglen > MAX_BD_NUM);
+ // Correct setup below requires at least one bd
+ // (and dma_map_sg should not return 0):
+ BUG_ON(sglen == 0);
gpd = dma->gpd;
bd = dma->bd;
--
2.21.0
On Mon, Mar 18, 2019 at 08:20:04PM -0600, George Hilliard wrote:
> The kernel complained:
>
> [ 510.277151] WARNING: CPU: 0 PID: 395 at fs/proc/generic.c:360 proc_register+0xf0/0x108
> [ 510.292891] proc_dir_entry '/proc/msdc_debug' already registered
>
> when doing a modprobe/rmmod/modprobe of this module if debug messages
> are compiled in. Fix this by removing the proc entry when the module is
> unloaded.
>
> Signed-off-by: George Hilliard <[email protected]>
> ---
> drivers/staging/mt7621-mmc/dbg.c | 15 ++++++++++++---
> drivers/staging/mt7621-mmc/dbg.h | 3 ++-
> drivers/staging/mt7621-mmc/sd.c | 4 ++++
> 3 files changed, 18 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/staging/mt7621-mmc/dbg.c b/drivers/staging/mt7621-mmc/dbg.c
> index fda3ba38ba37..2310f3bcc16e 100644
> --- a/drivers/staging/mt7621-mmc/dbg.c
> +++ b/drivers/staging/mt7621-mmc/dbg.c
> @@ -294,9 +294,18 @@ static const struct file_operations msdc_debug_fops = {
> .release = single_release,
> };
>
> -void msdc_debug_proc_init(void)
> +// Keep ahold of the proc entry we create so it can be freed during
> +// module removal
> +struct proc_dir_entry *msdc_debug_proc_entry;
> +
> +void __init msdc_debug_proc_init(void)
> {
> - proc_create("msdc_debug", 0660, NULL, &msdc_debug_fops);
> + msdc_debug_proc_entry = proc_create("msdc_debug", 0660,
> + NULL, &msdc_debug_fops);
> +}
> +
> +void __exit msdc_debug_proc_deinit(void)
> +{
> + proc_remove(msdc_debug_proc_entry);
> }
> -EXPORT_SYMBOL_GPL(msdc_debug_proc_init);
> #endif
> diff --git a/drivers/staging/mt7621-mmc/dbg.h b/drivers/staging/mt7621-mmc/dbg.h
> index 2d447b2d92ae..d100324aa3fe 100644
> --- a/drivers/staging/mt7621-mmc/dbg.h
> +++ b/drivers/staging/mt7621-mmc/dbg.h
> @@ -93,7 +93,8 @@ enum msdc_dbg {
>
> extern unsigned int sd_debug_zone[4];
> #define TAG "msdc"
> -void msdc_debug_proc_init(void);
> +void __init msdc_debug_proc_init(void);
> +void __exit msdc_debug_proc_deinit(void);
>
> u32 msdc_time_calc(u32 old_L32, u32 old_H32, u32 new_L32, u32 new_H32);
> void msdc_performance(u32 opcode, u32 sizes, u32 bRx, u32 ticks);
> diff --git a/drivers/staging/mt7621-mmc/sd.c b/drivers/staging/mt7621-mmc/sd.c
> index 9074848a8251..306b3b46f7c9 100644
> --- a/drivers/staging/mt7621-mmc/sd.c
> +++ b/drivers/staging/mt7621-mmc/sd.c
> @@ -1841,6 +1841,10 @@ static int __init mt_msdc_init(void)
>
> static void __exit mt_msdc_exit(void)
> {
> +#if defined(MT6575_SD_DEBUG)
> + msdc_debug_proc_deinit();
> +#endif
You shouldn't need a #ifdef in .c code. In the .h file provide an
"empty" function for this if the option is not defined. Then you can
just call this function in the .c file no matter what.
thanks,
greg k-h
On Mon, Mar 18, 2019 at 08:20:08PM -0600, George Hilliard wrote:
> The buffer descriptor setup loop is correct only if it is setting up at
> least one bd struct. Besides, there is an error somewhere if
> dma_map_sg() returns 0. So add a paranoid check for this condition.
>
> Signed-off-by: George Hilliard <[email protected]>
> ---
> drivers/staging/mt7621-mmc/sd.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/staging/mt7621-mmc/sd.c b/drivers/staging/mt7621-mmc/sd.c
> index c272aa780719..31c5b44bd29f 100644
> --- a/drivers/staging/mt7621-mmc/sd.c
> +++ b/drivers/staging/mt7621-mmc/sd.c
> @@ -593,7 +593,12 @@ static void msdc_dma_setup(struct msdc_host *host, struct msdc_dma *dma,
> struct bd *bd;
> u32 j;
>
> - BUG_ON(sglen > MAX_BD_NUM); /* not support currently */
> + // Shouldn't happen; we configure the mmc host layer
> + // based on MAX_BD_NUM:
> + BUG_ON(sglen > MAX_BD_NUM);
> + // Correct setup below requires at least one bd
> + // (and dma_map_sg should not return 0):
> + BUG_ON(sglen == 0);
Same here, no new BUG_ON(). To quote one kernel developer, "BUG_ON()
means that the programmer is lazy" :)
thanks,
greg k-h
On Mon, Mar 18, 2019 at 08:20:07PM -0600, George Hilliard wrote:
> The module was initializing completions whenever it was going to wait on
> them, and not when the completion was allocated. This is incorrect
> according to the completion docs:
>
> Calling init_completion() on the same completion object twice is
> most likely a bug [...]
>
> Re-initialization is also unnecessary because the module never uses
> complete_all(). Fix this by only ever initializing the completion a
> single time.
>
> Signed-off-by: George Hilliard <[email protected]>
> ---
> drivers/staging/mt7621-mmc/sd.c | 13 +++++++++----
> 1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/staging/mt7621-mmc/sd.c b/drivers/staging/mt7621-mmc/sd.c
> index 89fbc0a1dec7..c272aa780719 100644
> --- a/drivers/staging/mt7621-mmc/sd.c
> +++ b/drivers/staging/mt7621-mmc/sd.c
> @@ -467,7 +467,9 @@ static unsigned int msdc_command_start(struct msdc_host *host,
> host->cmd = cmd;
> host->cmd_rsp = resp;
>
> - init_completion(&host->cmd_done);
> + // The completion should have been consumed by the previous command
> + // response handler, because the mmc requests should be serialized
> + BUG_ON(completion_done(&host->cmd_done));
Adding new BUG_ON() calls is not ok. That crashes the machine and will
break everyone. If this really is a potential error that could happen,
then handle it. If not, then do not even put this there, it's not
needed.
All of the BUG_ON() entries need to be removed in the end from this code
anyway.
thanks,
greg k-h
On Mon, Mar 18, 2019 at 08:20:01PM -0600, George Hilliard wrote:
> Because of this change, the driver now expects a pinctrl device
> reference in the mmc controller's device tree node; without it, it will
> bail out. This could break existing setups that don't specify it
> because it "just worked" up until now. So currently I just let the old
> behavior fall away because this is a staging driver. But if this is a
> problem, the old behavior could be added back as a fallback.
>
> This is version 2 of a patchset that I requested feedback for about a
> month ago. Please review as if they are a new patchset; all the patches
> were rebased several times and a couple new correctness fixes added.
>
> The TODO list is largely unchanged, aside from the couple of TODO
> comments in the code that I have addressed. Ultimately, I think this
> driver could potentially be merged with the "real" mtk-mmc driver as the
> TODO suggests, but someone who is more familiar with the IP core will
> have to do that. Mediatek documentation (that I can find) is very
> sparse.
>
> This is ready to merge if there is no other feedback!
>
> >From George Hilliard <[email protected]> # This line is ignored.
> From: George Hilliard <[email protected]>
> Reply-To:
> Subject: [PATCH v2 00/11] mt7621-mmc: Various correctness fixes
> In-Reply-To:
>
>
No subject for this email?