* [PATCH v3 1/3] staging: mt7621-mmc: Fix warning when reloading module with debug msgs enabled
2019-03-20 22:42 [PATCH v3 0/3] mt7621-mmc driver correctness fixes George Hilliard
@ 2019-03-20 22:42 ` George Hilliard
2019-03-20 22:42 ` [PATCH v3 2/3] staging: mt7621-mmc: Initialize completions a single time during probe George Hilliard
` (2 subsequent siblings)
3 siblings, 0 replies; 6+ messages in thread
From: George Hilliard @ 2019-03-20 22:42 UTC (permalink / raw)
To: Greg Kroah-Hartman; +Cc: linux-mips, linux-kernel, George Hilliard
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 <thirtythreeforty@gmail.com>
---
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
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH v3 2/3] staging: mt7621-mmc: Initialize completions a single time during probe
2019-03-20 22:42 [PATCH v3 0/3] mt7621-mmc driver correctness fixes George Hilliard
2019-03-20 22:42 ` [PATCH v3 1/3] staging: mt7621-mmc: Fix warning when reloading module with debug msgs enabled George Hilliard
@ 2019-03-20 22:42 ` George Hilliard
2019-03-22 14:19 ` Greg Kroah-Hartman
2019-03-20 22:42 ` [PATCH v3 3/3] staging: mt7621-mmc: Check for nonzero number of scatterlist entries George Hilliard
2019-03-22 14:20 ` [PATCH v3 0/3] mt7621-mmc driver correctness fixes Greg Kroah-Hartman
3 siblings, 1 reply; 6+ messages in thread
From: George Hilliard @ 2019-03-20 22:42 UTC (permalink / raw)
To: Greg Kroah-Hartman; +Cc: linux-mips, linux-kernel, George Hilliard
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 <thirtythreeforty@gmail.com>
---
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
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH v3 2/3] staging: mt7621-mmc: Initialize completions a single time during probe
2019-03-20 22:42 ` [PATCH v3 2/3] staging: mt7621-mmc: Initialize completions a single time during probe George Hilliard
@ 2019-03-22 14:19 ` Greg Kroah-Hartman
0 siblings, 0 replies; 6+ messages in thread
From: Greg Kroah-Hartman @ 2019-03-22 14:19 UTC (permalink / raw)
To: George Hilliard; +Cc: linux-mips, linux-kernel
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 <thirtythreeforty@gmail.com>
> ---
> 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 :(
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v3 3/3] staging: mt7621-mmc: Check for nonzero number of scatterlist entries
2019-03-20 22:42 [PATCH v3 0/3] mt7621-mmc driver correctness fixes George Hilliard
2019-03-20 22:42 ` [PATCH v3 1/3] staging: mt7621-mmc: Fix warning when reloading module with debug msgs enabled George Hilliard
2019-03-20 22:42 ` [PATCH v3 2/3] staging: mt7621-mmc: Initialize completions a single time during probe George Hilliard
@ 2019-03-20 22:42 ` George Hilliard
2019-03-22 14:20 ` [PATCH v3 0/3] mt7621-mmc driver correctness fixes Greg Kroah-Hartman
3 siblings, 0 replies; 6+ messages in thread
From: George Hilliard @ 2019-03-20 22:42 UTC (permalink / raw)
To: Greg Kroah-Hartman; +Cc: linux-mips, linux-kernel, George Hilliard
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 <thirtythreeforty@gmail.com>
---
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
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH v3 0/3] mt7621-mmc driver correctness fixes
2019-03-20 22:42 [PATCH v3 0/3] mt7621-mmc driver correctness fixes George Hilliard
` (2 preceding siblings ...)
2019-03-20 22:42 ` [PATCH v3 3/3] staging: mt7621-mmc: Check for nonzero number of scatterlist entries George Hilliard
@ 2019-03-22 14:20 ` Greg Kroah-Hartman
3 siblings, 0 replies; 6+ messages in thread
From: Greg Kroah-Hartman @ 2019-03-22 14:20 UTC (permalink / raw)
To: George Hilliard; +Cc: linux-mips, linux-kernel
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
^ permalink raw reply [flat|nested] 6+ messages in thread