[PATCH v3 0/3] mt7621-mmc driver correctness fixes

[PATCH v3 0/3] mt7621-mmc driver correctness fixes

[George Hilliard]

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

[PATCH v3 1/3] staging: mt7621-mmc: Fix warning when reloading module with debug msgs enabled

[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

[PATCH v3 2/3] staging: mt7621-mmc: Initialize completions a single time during probe

[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

[PATCH v3 3/3] staging: mt7621-mmc: Check for nonzero number of scatterlist entries

[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

Re: [PATCH v3 2/3] staging: mt7621-mmc: Initialize completions a single time during probe

[Greg Kroah-Hartman]

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 :(

Re: [PATCH v3 0/3] mt7621-mmc driver correctness fixes

[Greg Kroah-Hartman]

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