linux-kernel - RE: [PATCH v5 2/2] mmc: Add mmc driver for Sunplus SP7021

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 31 Mar 2022 09:27:27 +0000
From:   Tony Huang 黃懷厚 <tony.huang@...plus.com>
To:     Arnd Bergmann <arnd@...db.de>,
        Tony Huang <tonyhuang.sunplus@...il.com>
CC:     Ulf Hansson <ulf.hansson@...aro.org>,
        Rob Herring <robh+dt@...nel.org>,
        "lhjeff911@...il.com" <lhjeff911@...il.com>,
        linux-mmc <linux-mmc@...r.kernel.org>,
        DTML <devicetree@...r.kernel.org>,
        "Linux Kernel Mailing List" <linux-kernel@...r.kernel.org>,
        Philipp Zabel <p.zabel@...gutronix.de>,
        Wells Lu 呂芳騰 <wells.lu@...plus.com>,
        Lh Kuo 郭力豪 <lh.Kuo@...plus.com>
Subject: RE: [PATCH v5 2/2] mmc: Add mmc driver for Sunplus SP7021

Dear Arnd:

> > Add mmc driver for Sunplus SP7021
> >
> > Signed-off-by: Tony Huang <tonyhuang.sunplus@...il.com>
> 
> There should be a description of the device in the changelog, not just the same
> text as the subject.

OK, I will add description.

> > +static void spmmc_request(struct mmc_host *mmc, struct mmc_request
> > +*mrq) {
> > +       struct spmmc_host *host = mmc_priv(mmc);
> > +       struct mmc_data *data;
> > +       struct mmc_command *cmd;
> > +       int ret;
> > +
> > +       ret = mutex_lock_interruptible(&host->mrq_lock);
> > +       if (ret)
> > +               return;
> 
> I don't think it's valid to just return here when you get a signal. If nothing can
> handle the signal, doesn't it just hang?
> 
> It also appears that you don't release the mutex until the tasklet runs, but it is
> not valid to release a mutex from a different context.
> 
> You should get a warning about this when running a kernel with lockdep
> enabled at compile time. Please rework the locking to make this work.
> 
	Reomve code:
    ret = mutex_lock_interruptible(&host->mrq_lock);
    if (ret)
         return;

	Below is my modification:
.    mutex_lock(&host->mrq_lock);


> > +#endif /* ifdef CONFIG_PM_RUNTIME */
> > +
> > +static const struct dev_pm_ops spmmc_pm_ops = {
> > +       SET_SYSTEM_SLEEP_PM_OPS(spmmc_pm_suspend,
> spmmc_pm_resume)
> > +#ifdef CONFIG_PM_RUNTIME
> > +       SET_RUNTIME_PM_OPS(spmmc_pm_runtime_suspend,
> > +spmmc_pm_runtime_resume, NULL) #endif }; #endif /* ifdef CONFIG_PM
> */
> 
> It's better to use SYSTEM_SLEEP_PM_OPS/RUNTIME_PM_OPS instead of the
> SET_ version, then you can remove all the #ifdef checks.
> 

	I use SYSTEM_SLEEP_PM_OPS/RUNTIME_PM_OPS. 								
	Compile shows error. Error: implicit declaration of function ? ? SYSTEM_SLEEP_PM_OPS? ? Did you mean ? ? SET_SYSTEM_SLEEP_PM_OPS? ?		[-Werror=implicit-function-declaration]								
									
	I reference other mmc driver.								
	Below is my modification:
    Compiler is pass.								
									
	#ifdef CONFIG_PM_SLEEP								
	static int spmmc_pm_suspend(struct device *dev)								
	{								
		pm_runtime_force_suspend(dev);							
									
		return 0;							
	}								
									
	static int spmmc_pm_resume(struct device *dev)								
	{								
		pm_runtime_force_resume(dev);							
									
		return 0;							
	}								
	#endif								
									
	#ifdef CONFIG_PM								
	static int spmmc_pm_runtime_suspend(struct device *dev)								
	{								
		struct spmmc_host *host;							
									
		host = dev_get_drvdata(dev);							
		clk_disable(host->clk);							
									
		return 0;							
	}								
									
	static int spmmc_pm_runtime_resume(struct device *dev)								
	{								
		struct spmmc_host *host;							
									
		host = dev_get_drvdata(dev);							
									
		return clk_enable(host->clk);							
	}								
	#endif								
									
	static const struct dev_pm_ops spmmc_pm_ops = {								
		SET_SYSTEM_SLEEP_PM_OPS(spmmc_pm_suspend, spmmc_pm_resume)							
		SET_RUNTIME_PM_OPS(spmmc_pm_runtime_suspend, spmmc_pm_runtime_resume, NULL)							
	};