diff mbox series

[3/4] soundwire: debugfs: Switch to sdw_read_no_pm

Message ID 20221114102956.914468-4-ckeepax@opensource.cirrus.com
State New
Headers
Series Minor SoundWire clean ups |

Commit Message

Charles Keepax Nov. 14, 2022, 10:29 a.m. UTC 
  It is rather inefficient to be constantly enabling/disabling the PM
runtime as we print out each individual register, switch to holding a PM
runtime reference across the whole register output.

Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---
 drivers/soundwire/debugfs.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

Comments

Pierre-Louis Bossart Nov. 14, 2022, 4:14 p.m. UTC | #1 
On 11/14/22 04:29, Charles Keepax wrote:
> It is rather inefficient to be constantly enabling/disabling the PM
> runtime as we print out each individual register, switch to holding a PM
> runtime reference across the whole register output.

the change is good, but technically the pm_runtime resume happens for
the first read and suspend with a delay if use_autosuspend() is enabled,
so presumably we'll see the same number of resume/suspend with the
existing code and the suggested change.

Maybe update the commit message to mention that we constantly change
reference counts, as you did in the next patch?

> 
> Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
> ---
>  drivers/soundwire/debugfs.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/soundwire/debugfs.c b/drivers/soundwire/debugfs.c
> index 49900cd207bc7..0718e9cda138a 100644
> --- a/drivers/soundwire/debugfs.c
> +++ b/drivers/soundwire/debugfs.c
> @@ -4,6 +4,7 @@
>  #include <linux/device.h>
>  #include <linux/debugfs.h>
>  #include <linux/mod_devicetable.h>
> +#include <linux/pm_runtime.h>
>  #include <linux/slab.h>
>  #include <linux/soundwire/sdw.h>
>  #include <linux/soundwire/sdw_registers.h>
> @@ -35,7 +36,7 @@ static ssize_t sdw_sprintf(struct sdw_slave *slave,
>  {
>  	int value;
>  
> -	value = sdw_read(slave, reg);
> +	value = sdw_read_no_pm(slave, reg);
>  
>  	if (value < 0)
>  		return scnprintf(buf + pos, RD_BUF - pos, "%3x\tXX\n", reg);
> @@ -55,6 +56,10 @@ static int sdw_slave_reg_show(struct seq_file *s_file, void *data)
>  	if (!buf)
>  		return -ENOMEM;
>  
> +	ret = pm_runtime_resume_and_get(&slave->dev);
> +	if (ret < 0 && ret != -EACCES)
> +		return ret;
> +
>  	ret = scnprintf(buf, RD_BUF, "Register  Value\n");
>  
>  	/* DP0 non-banked registers */
> @@ -112,6 +117,10 @@ static int sdw_slave_reg_show(struct seq_file *s_file, void *data)
>  	}
>  
>  	seq_printf(s_file, "%s", buf);
> +
> +	pm_runtime_mark_last_busy(&slave->dev);
> +	pm_runtime_put(&slave->dev);
> +
>  	kfree(buf);
>  
>  	return 0;
Charles Keepax Nov. 15, 2022, 10:14 a.m. UTC | #2 
On Mon, Nov 14, 2022 at 10:14:19AM -0600, Pierre-Louis Bossart wrote:
> 
> 
> On 11/14/22 04:29, Charles Keepax wrote:
> > It is rather inefficient to be constantly enabling/disabling the PM
> > runtime as we print out each individual register, switch to holding a PM
> > runtime reference across the whole register output.
> 
> the change is good, but technically the pm_runtime resume happens for
> the first read and suspend with a delay if use_autosuspend() is enabled,
> so presumably we'll see the same number of resume/suspend with the
> existing code and the suggested change.
> 
> Maybe update the commit message to mention that we constantly change
> reference counts, as you did in the next patch?

Yeah agree, I will respin the commit message.

Thanks,
Charles
Dan Carpenter Nov. 21, 2022, 10:50 a.m. UTC | #3 
Hi Charles,

https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Charles-Keepax/Minor-SoundWire-clean-ups/20221114-183333
patch link:    https://lore.kernel.org/r/20221114102956.914468-4-ckeepax%40opensource.cirrus.com
patch subject: [PATCH 3/4] soundwire: debugfs: Switch to sdw_read_no_pm
config: loongarch-randconfig-m041-20221114
compiler: loongarch64-linux-gcc (GCC) 12.1.0

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <error27@gmail.com>

smatch warnings:
drivers/soundwire/debugfs.c:61 sdw_slave_reg_show() warn: possible memory leak of 'buf'

vim +/buf +61 drivers/soundwire/debugfs.c

bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21   48  static int sdw_slave_reg_show(struct seq_file *s_file, void *data)
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21   49  {
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21   50  	struct sdw_slave *slave = s_file->private;
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21   51  	char *buf;
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21   52  	ssize_t ret;
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21   53  	int i, j;
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21   54  
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21   55  	buf = kzalloc(RD_BUF, GFP_KERNEL);
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21   56  	if (!buf)
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21   57  		return -ENOMEM;
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21   58  
f3345eda607ecc Charles Keepax       2022-11-14   59  	ret = pm_runtime_resume_and_get(&slave->dev);
f3345eda607ecc Charles Keepax       2022-11-14   60  	if (ret < 0 && ret != -EACCES)
f3345eda607ecc Charles Keepax       2022-11-14  @61  		return ret;

kfree(buf);

f3345eda607ecc Charles Keepax       2022-11-14   62  
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21   63  	ret = scnprintf(buf, RD_BUF, "Register  Value\n");
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21   64  
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21   65  	/* DP0 non-banked registers */
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21   66  	ret += scnprintf(buf + ret, RD_BUF - ret, "\nDP0\n");
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21   67  	for (i = SDW_DP0_INT; i <= SDW_DP0_PREPARECTRL; i++)
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21   68  		ret += sdw_sprintf(slave, buf, ret, i);
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21   69  
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21   70  	/* DP0 Bank 0 registers */
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21   71  	ret += scnprintf(buf + ret, RD_BUF - ret, "Bank0\n");
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21   72  	ret += sdw_sprintf(slave, buf, ret, SDW_DP0_CHANNELEN);
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21   73  	for (i = SDW_DP0_SAMPLECTRL1; i <= SDW_DP0_LANECTRL; i++)
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21   74  		ret += sdw_sprintf(slave, buf, ret, i);
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21   75  
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21   76  	/* DP0 Bank 1 registers */
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21   77  	ret += scnprintf(buf + ret, RD_BUF - ret, "Bank1\n");
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21   78  	ret += sdw_sprintf(slave, buf, ret,
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21   79  			SDW_DP0_CHANNELEN + SDW_BANK1_OFFSET);
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21   80  	for (i = SDW_DP0_SAMPLECTRL1 + SDW_BANK1_OFFSET;
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21   81  			i <= SDW_DP0_LANECTRL + SDW_BANK1_OFFSET; i++)
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21   82  		ret += sdw_sprintf(slave, buf, ret, i);
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21   83  
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21   84  	/* SCP registers */
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21   85  	ret += scnprintf(buf + ret, RD_BUF - ret, "\nSCP\n");
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21   86  	for (i = SDW_SCP_INT1; i <= SDW_SCP_BANKDELAY; i++)
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21   87  		ret += sdw_sprintf(slave, buf, ret, i);
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21   88  	for (i = SDW_SCP_DEVID_0; i <= SDW_SCP_DEVID_5; i++)
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21   89  		ret += sdw_sprintf(slave, buf, ret, i);
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21   90  
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21   91  	/*
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21   92  	 * SCP Bank 0/1 registers are read-only and cannot be
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21   93  	 * retrieved from the Slave. The Master typically keeps track
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21   94  	 * of the current frame size so the information can be found
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21   95  	 * in other places
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21   96  	 */
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21   97  
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21   98  	/* DP1..14 registers */
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21   99  	for (i = 1; SDW_VALID_PORT_RANGE(i); i++) {
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21  100  
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21  101  		/* DPi registers */
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21  102  		ret += scnprintf(buf + ret, RD_BUF - ret, "\nDP%d\n", i);
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21  103  		for (j = SDW_DPN_INT(i); j <= SDW_DPN_PREPARECTRL(i); j++)
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21  104  			ret += sdw_sprintf(slave, buf, ret, j);
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21  105  
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21  106  		/* DPi Bank0 registers */
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21  107  		ret += scnprintf(buf + ret, RD_BUF - ret, "Bank0\n");
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21  108  		for (j = SDW_DPN_CHANNELEN_B0(i);
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21  109  		     j <= SDW_DPN_LANECTRL_B0(i); j++)
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21  110  			ret += sdw_sprintf(slave, buf, ret, j);
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21  111  
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21  112  		/* DPi Bank1 registers */
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21  113  		ret += scnprintf(buf + ret, RD_BUF - ret, "Bank1\n");
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21  114  		for (j = SDW_DPN_CHANNELEN_B1(i);
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21  115  		     j <= SDW_DPN_LANECTRL_B1(i); j++)
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21  116  			ret += sdw_sprintf(slave, buf, ret, j);
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21  117  	}
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21  118  
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21  119  	seq_printf(s_file, "%s", buf);
f3345eda607ecc Charles Keepax       2022-11-14  120  
f3345eda607ecc Charles Keepax       2022-11-14  121  	pm_runtime_mark_last_busy(&slave->dev);
f3345eda607ecc Charles Keepax       2022-11-14  122  	pm_runtime_put(&slave->dev);
f3345eda607ecc Charles Keepax       2022-11-14  123  
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21  124  	kfree(buf);
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21  125  
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21  126  	return 0;
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21  127  }
bf03473d5bcc85 Pierre-Louis Bossart 2019-08-21  128  DEFINE_SHOW_ATTRIBUTE(sdw_slave_reg);
Charles Keepax Nov. 21, 2022, 1:13 p.m. UTC | #4 
On Mon, Nov 21, 2022 at 01:50:55PM +0300, Dan Carpenter wrote:
> Hi Charles,
> 
> https://git-scm.com/docs/git-format-patch#_base_tree_information]
> 
> url:    https://github.com/intel-lab-lkp/linux/commits/Charles-Keepax/Minor-SoundWire-clean-ups/20221114-183333
> patch link:    https://lore.kernel.org/r/20221114102956.914468-4-ckeepax%40opensource.cirrus.com
> patch subject: [PATCH 3/4] soundwire: debugfs: Switch to sdw_read_no_pm
> config: loongarch-randconfig-m041-20221114
> compiler: loongarch64-linux-gcc (GCC) 12.1.0
> 
> If you fix the issue, kindly add following tag where applicable
> | Reported-by: kernel test robot <lkp@intel.com>
> | Reported-by: Dan Carpenter <error27@gmail.com>
> 

Thanks for the spot I will fix this up and do a v3.

Thanks,
Charles
diff mbox series

Patch

diff --git a/drivers/soundwire/debugfs.c b/drivers/soundwire/debugfs.c
index 49900cd207bc7..0718e9cda138a 100644
--- a/drivers/soundwire/debugfs.c
+++ b/drivers/soundwire/debugfs.c
@@ -4,6 +4,7 @@ 
 #include <linux/device.h>
 #include <linux/debugfs.h>
 #include <linux/mod_devicetable.h>
+#include <linux/pm_runtime.h>
 #include <linux/slab.h>
 #include <linux/soundwire/sdw.h>
 #include <linux/soundwire/sdw_registers.h>
@@ -35,7 +36,7 @@  static ssize_t sdw_sprintf(struct sdw_slave *slave,
 {
 	int value;
 
-	value = sdw_read(slave, reg);
+	value = sdw_read_no_pm(slave, reg);
 
 	if (value < 0)
 		return scnprintf(buf + pos, RD_BUF - pos, "%3x\tXX\n", reg);
@@ -55,6 +56,10 @@  static int sdw_slave_reg_show(struct seq_file *s_file, void *data)
 	if (!buf)
 		return -ENOMEM;
 
+	ret = pm_runtime_resume_and_get(&slave->dev);
+	if (ret < 0 && ret != -EACCES)
+		return ret;
+
 	ret = scnprintf(buf, RD_BUF, "Register  Value\n");
 
 	/* DP0 non-banked registers */
@@ -112,6 +117,10 @@  static int sdw_slave_reg_show(struct seq_file *s_file, void *data)
 	}
 
 	seq_printf(s_file, "%s", buf);
+
+	pm_runtime_mark_last_busy(&slave->dev);
+	pm_runtime_put(&slave->dev);
+
 	kfree(buf);
 
 	return 0;