This series improves up the patches that needed more work from v2 I sent
a couple days ago. BUG_ON()s have been removed.
Let me know if you don't want the 'Remove obsolete Kconfig flags' patch I sent
in the previous series; I didn't see any mail about it.
Thanks!
George
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, and log if the completions are not consumed as intended
(this is not a fatal problem, but should not go unnoticed).
Signed-off-by: George Hilliard <[email protected]>
---
v2: Rebased v1
v3: Removed BUG_ON() calls, and politely log; this failure won't kill us
drivers/staging/mt7621-mmc/sd.c | 15 +++++++++++----
1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/drivers/staging/mt7621-mmc/sd.c b/drivers/staging/mt7621-mmc/sd.c
index ff01872ab972..139533606863 100644
--- a/drivers/staging/mt7621-mmc/sd.c
+++ b/drivers/staging/mt7621-mmc/sd.c
@@ -466,7 +466,10 @@ 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
+ if(completion_done(&host->cmd_done))
+ dev_err(mmc_dev(host->mmc), "previous command was not handled\n");
sdr_set_bits(host->base + MSDC_INTEN, wints);
sdc_send_cmd(rawcmd, cmd->arg);
@@ -488,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);
@@ -672,7 +674,11 @@ 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
+ if(completion_done(&host->cmd_done))
+ dev_err(mmc_dev(host->mmc), "previous transfer was not handled\n");
/* start the command first*/
if (msdc_command_start(host, cmd, CMD_TIMEOUT) != 0)
@@ -691,7 +697,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);
@@ -1682,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 buffer descriptor setup loop is correct only if it is setting up at
least one bd struct. Besides, there is an error if dma_map_sg() returns
0, which is possible and must be handled.
Additionally, remove the BUG_ON() checking sglen, which is unnecessary
because we configure DMA with that constraint during init.
Signed-off-by: George Hilliard <[email protected]>
---
v2: Rebased v1
v3: Remove existing and added BUG_ON() in v2, and handle dma_map_sg
failure gracefully.
drivers/staging/mt7621-mmc/sd.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/mt7621-mmc/sd.c b/drivers/staging/mt7621-mmc/sd.c
index 139533606863..cf2ad06112eb 100644
--- a/drivers/staging/mt7621-mmc/sd.c
+++ b/drivers/staging/mt7621-mmc/sd.c
@@ -593,8 +593,6 @@ 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 */
-
gpd = dma->gpd;
bd = dma->bd;
@@ -687,6 +685,13 @@ static int msdc_do_request(struct mmc_host *mmc, struct mmc_request *mrq)
data->sg_count = dma_map_sg(mmc_dev(mmc), data->sg,
data->sg_len,
mmc_get_dma_dir(data));
+
+ if (data->sg_count == 0) {
+ dev_err(mmc_dev(host->mmc), "failed to map DMA for transfer\n");
+ data->error = -ENOMEM;
+ goto done;
+ }
+
msdc_dma_setup(host, &host->dma, data->sg,
data->sg_count);
--
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]>
---
v1: Initial attempt
v2: Rebased from v1
v3: Use #ifdef in header file, not implementation
drivers/staging/mt7621-mmc/dbg.c | 15 ++++++++++++---
drivers/staging/mt7621-mmc/dbg.h | 9 +++++++--
drivers/staging/mt7621-mmc/sd.c | 5 +++--
3 files changed, 22 insertions(+), 7 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..b6a09876d7ae 100644
--- a/drivers/staging/mt7621-mmc/dbg.h
+++ b/drivers/staging/mt7621-mmc/dbg.h
@@ -92,8 +92,13 @@ enum msdc_dbg {
#define DBG_EVT_MASK (DBG_EVT_ALL)
extern unsigned int sd_debug_zone[4];
-#define TAG "msdc"
-void msdc_debug_proc_init(void);
+#ifdef MT6575_SD_DEBUG
+void __init msdc_debug_proc_init(void);
+void __exit msdc_debug_proc_deinit(void);
+#else
+static inline void msdc_debug_proc_init(void) {}
+static inline void msdc_debug_proc_deinit(void) {}
+#endif
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 40f23e200c7a..ff01872ab972 100644
--- a/drivers/staging/mt7621-mmc/sd.c
+++ b/drivers/staging/mt7621-mmc/sd.c
@@ -1825,14 +1825,15 @@ static int __init mt_msdc_init(void)
return ret;
}
-#if defined(MT6575_SD_DEBUG)
msdc_debug_proc_init();
-#endif
+
return 0;
}
static void __exit mt_msdc_exit(void)
{
+ msdc_debug_proc_deinit();
+
platform_driver_unregister(&mt_msdc_driver);
}
--
2.21.0
On Wed, Mar 20, 2019 at 04:42:04PM -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, and log if the completions are not consumed as intended
> (this is not a fatal problem, but should not go unnoticed).
>
> Signed-off-by: George Hilliard <[email protected]>
> ---
> v2: Rebased v1
> v3: Removed BUG_ON() calls, and politely log; this failure won't kill us
>
> drivers/staging/mt7621-mmc/sd.c | 15 +++++++++++----
> 1 file changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/staging/mt7621-mmc/sd.c b/drivers/staging/mt7621-mmc/sd.c
> index ff01872ab972..139533606863 100644
> --- a/drivers/staging/mt7621-mmc/sd.c
> +++ b/drivers/staging/mt7621-mmc/sd.c
> @@ -466,7 +466,10 @@ 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
> + if(completion_done(&host->cmd_done))
> + dev_err(mmc_dev(host->mmc), "previous command was not handled\n");
Proper coding style please :(
On Wed, Mar 20, 2019 at 04:42:02PM -0600, George Hilliard wrote:
> This series improves up the patches that needed more work from v2 I sent
> a couple days ago. BUG_ON()s have been removed.
>
> Let me know if you don't want the 'Remove obsolete Kconfig flags' patch I sent
> in the previous series; I didn't see any mail about it.
If I didn't apply it already, please resend.
thanks,
greg k-h