[PATCH 1/2] soundwire: bus: Remove now outdated comments on no_pm IO

[PATCH 1/2] soundwire: bus: Remove now outdated comments on no_pm IO

[Charles Keepax]

Things have moved more towards end drivers using the no_pm versions of
the IO functions. See commits:

commit 167790abb90f ("soundwire: export sdw_write/read_no_pm functions")
commit 62dc9f3f2fd0 ("soundwire: bus: export sdw_nwrite_no_pm and
                      sdw_nread_no_pm functions")

As such this comment is now misleading, so remove it.

Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---
 drivers/soundwire/bus.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
index b6aca59c31300..3c67266f94834 100644
--- a/drivers/soundwire/bus.c
+++ b/drivers/soundwire/bus.c
@@ -384,9 +384,6 @@ int sdw_fill_msg(struct sdw_msg *msg, struct sdw_slave *slave,
 
 /*
  * Read/Write IO functions.
- * no_pm versions can only be called by the bus, e.g. while enumerating or
- * handling suspend-resume sequences.
- * all clients need to use the pm versions
  */
 
 int sdw_nread_no_pm(struct sdw_slave *slave, u32 addr, size_t count, u8 *val)
-- 
2.30.2

[PATCH 2/2] soundwire: bus: Update sdw_nread/nwrite_no_pm to handle page boundaries

[Charles Keepax]

Currently issuing a sdw_nread/nwrite_no_pm across a page boundary
will silently fail to write correctly as nothing updates the page
registers, meaning the same page of the chip will get rewritten
with each successive page of data.

As the sdw_msg structure contains page information it seems
reasonable that a single sdw_msg should always be within one
page. It is also mostly simpler to handle the paging at the
bus level rather than each master having to handle it in their
xfer_msg callback.

As such add handling to the bus code to split up a transfer into
multiple sdw_msg's when they go across page boundaries.

Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---
 drivers/soundwire/bus.c | 47 +++++++++++++++++++++++------------------
 1 file changed, 26 insertions(+), 21 deletions(-)

diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
index 3c67266f94834..bdd251e871694 100644
--- a/drivers/soundwire/bus.c
+++ b/drivers/soundwire/bus.c
@@ -386,37 +386,42 @@ int sdw_fill_msg(struct sdw_msg *msg, struct sdw_slave *slave,
  * Read/Write IO functions.
  */
 
-int sdw_nread_no_pm(struct sdw_slave *slave, u32 addr, size_t count, u8 *val)
+static int sdw_ntransfer_no_pm(struct sdw_slave *slave, u32 addr, u8 flags,
+			       size_t count, u8 *val)
 {
 	struct sdw_msg msg;
+	size_t size;
 	int ret;
 
-	ret = sdw_fill_msg(&msg, slave, addr, count,
-			   slave->dev_num, SDW_MSG_FLAG_READ, val);
-	if (ret < 0)
-		return ret;
+	while (count) {
+		// Only handle bytes up to next page boundary
+		size = min(count, (SDW_REGADDR + 1) - (addr & SDW_REGADDR));
 
-	ret = sdw_transfer(slave->bus, &msg);
-	if (slave->is_mockup_device)
-		ret = 0;
-	return ret;
+		ret = sdw_fill_msg(&msg, slave, addr, size, slave->dev_num, flags, val);
+		if (ret < 0)
+			return ret;
+
+		ret = sdw_transfer(slave->bus, &msg);
+		if (ret < 0 && !slave->is_mockup_device)
+			return ret;
+
+		addr += size;
+		val += size;
+		count -= size;
+	}
+
+	return 0;
+}
+
+int sdw_nread_no_pm(struct sdw_slave *slave, u32 addr, size_t count, u8 *val)
+{
+	return sdw_ntransfer_no_pm(slave, addr, SDW_MSG_FLAG_READ, count, val);
 }
 EXPORT_SYMBOL(sdw_nread_no_pm);
 
 int sdw_nwrite_no_pm(struct sdw_slave *slave, u32 addr, size_t count, const u8 *val)
 {
-	struct sdw_msg msg;
-	int ret;
-
-	ret = sdw_fill_msg(&msg, slave, addr, count,
-			   slave->dev_num, SDW_MSG_FLAG_WRITE, (u8 *)val);
-	if (ret < 0)
-		return ret;
-
-	ret = sdw_transfer(slave->bus, &msg);
-	if (slave->is_mockup_device)
-		ret = 0;
-	return ret;
+	return sdw_ntransfer_no_pm(slave, addr, SDW_MSG_FLAG_WRITE, count, (u8 *)val);
 }
 EXPORT_SYMBOL(sdw_nwrite_no_pm);
 
-- 
2.30.2

[PATCH 2/2] soundwire: bus: Update sdw_nread/nwrite_no_pm to handle page boundaries

[Charles Keepax via Alsa-devel]

[-- Attachment #0: Type: message/rfc822, Size: 7311 bytes --]

From: Charles Keepax <ckeepax@opensource.cirrus.com>
To: <vkoul@kernel.org>
Cc: yung-chuan.liao@linux.intel.com, pierre-louis.bossart@linux.intel.com, sanyog.r.kale@intel.com, alsa-devel@alsa-project.org, linux-kernel@vger.kernel.org, patches@opensource.cirrus.com
Subject: [PATCH 2/2] soundwire: bus: Update sdw_nread/nwrite_no_pm to handle page boundaries
Date: Thu, 16 Mar 2023 15:57:34 +0000
Message-ID: <20230316155734.3191577-2-ckeepax@opensource.cirrus.com>

Currently issuing a sdw_nread/nwrite_no_pm across a page boundary
will silently fail to write correctly as nothing updates the page
registers, meaning the same page of the chip will get rewritten
with each successive page of data.

As the sdw_msg structure contains page information it seems
reasonable that a single sdw_msg should always be within one
page. It is also mostly simpler to handle the paging at the
bus level rather than each master having to handle it in their
xfer_msg callback.

As such add handling to the bus code to split up a transfer into
multiple sdw_msg's when they go across page boundaries.

Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---
 drivers/soundwire/bus.c | 47 +++++++++++++++++++++++------------------
 1 file changed, 26 insertions(+), 21 deletions(-)

diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
index 3c67266f94834..bdd251e871694 100644
--- a/drivers/soundwire/bus.c
+++ b/drivers/soundwire/bus.c
@@ -386,37 +386,42 @@ int sdw_fill_msg(struct sdw_msg *msg, struct sdw_slave *slave,
  * Read/Write IO functions.
  */
 
-int sdw_nread_no_pm(struct sdw_slave *slave, u32 addr, size_t count, u8 *val)
+static int sdw_ntransfer_no_pm(struct sdw_slave *slave, u32 addr, u8 flags,
+			       size_t count, u8 *val)
 {
 	struct sdw_msg msg;
+	size_t size;
 	int ret;
 
-	ret = sdw_fill_msg(&msg, slave, addr, count,
-			   slave->dev_num, SDW_MSG_FLAG_READ, val);
-	if (ret < 0)
-		return ret;
+	while (count) {
+		// Only handle bytes up to next page boundary
+		size = min(count, (SDW_REGADDR + 1) - (addr & SDW_REGADDR));
 
-	ret = sdw_transfer(slave->bus, &msg);
-	if (slave->is_mockup_device)
-		ret = 0;
-	return ret;
+		ret = sdw_fill_msg(&msg, slave, addr, size, slave->dev_num, flags, val);
+		if (ret < 0)
+			return ret;
+
+		ret = sdw_transfer(slave->bus, &msg);
+		if (ret < 0 && !slave->is_mockup_device)
+			return ret;
+
+		addr += size;
+		val += size;
+		count -= size;
+	}
+
+	return 0;
+}
+
+int sdw_nread_no_pm(struct sdw_slave *slave, u32 addr, size_t count, u8 *val)
+{
+	return sdw_ntransfer_no_pm(slave, addr, SDW_MSG_FLAG_READ, count, val);
 }
 EXPORT_SYMBOL(sdw_nread_no_pm);
 
 int sdw_nwrite_no_pm(struct sdw_slave *slave, u32 addr, size_t count, const u8 *val)
 {
-	struct sdw_msg msg;
-	int ret;
-
-	ret = sdw_fill_msg(&msg, slave, addr, count,
-			   slave->dev_num, SDW_MSG_FLAG_WRITE, (u8 *)val);
-	if (ret < 0)
-		return ret;
-
-	ret = sdw_transfer(slave->bus, &msg);
-	if (slave->is_mockup_device)
-		ret = 0;
-	return ret;
+	return sdw_ntransfer_no_pm(slave, addr, SDW_MSG_FLAG_WRITE, count, (u8 *)val);
 }
 EXPORT_SYMBOL(sdw_nwrite_no_pm);
 
-- 
2.30.2

Re: [PATCH 2/2] soundwire: bus: Update sdw_nread/nwrite_no_pm to handle page boundaries

[Pierre-Louis Bossart]

On 3/16/23 10:57, Charles Keepax wrote:
> Currently issuing a sdw_nread/nwrite_no_pm across a page boundary
> will silently fail to write correctly as nothing updates the page
> registers, meaning the same page of the chip will get rewritten
> with each successive page of data.
> 
> As the sdw_msg structure contains page information it seems
> reasonable that a single sdw_msg should always be within one
> page. It is also mostly simpler to handle the paging at the
> bus level rather than each master having to handle it in their
> xfer_msg callback.
> 
> As such add handling to the bus code to split up a transfer into
> multiple sdw_msg's when they go across page boundaries.

This sounds good but we need to clarify that the multiple sdw_msg's will
not necessarily be sent one after the other, the msg_lock is held in the
sdw_transfer() function, so there should be no expectation that e.g. one
big chunk of firmware code can be sent without interruption.

I also wonder if we should have a lower bar than the page to avoid
hogging the bus with large read/write transactions. If there are
multiple devices on the same link and one of them signals an alert
status while a large transfer is on-going, the alert handling will
mechanically be delayed by up to a page - that's 32k reads/writes, isn't it?

> Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
> ---
>  drivers/soundwire/bus.c | 47 +++++++++++++++++++++++------------------
>  1 file changed, 26 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
> index 3c67266f94834..bdd251e871694 100644
> --- a/drivers/soundwire/bus.c
> +++ b/drivers/soundwire/bus.c
> @@ -386,37 +386,42 @@ int sdw_fill_msg(struct sdw_msg *msg, struct sdw_slave *slave,
>   * Read/Write IO functions.
>   */
>  
> -int sdw_nread_no_pm(struct sdw_slave *slave, u32 addr, size_t count, u8 *val)
> +static int sdw_ntransfer_no_pm(struct sdw_slave *slave, u32 addr, u8 flags,
> +			       size_t count, u8 *val)
>  {
>  	struct sdw_msg msg;
> +	size_t size;
>  	int ret;
>  
> -	ret = sdw_fill_msg(&msg, slave, addr, count,
> -			   slave->dev_num, SDW_MSG_FLAG_READ, val);
> -	if (ret < 0)
> -		return ret;
> +	while (count) {
> +		// Only handle bytes up to next page boundary
> +		size = min(count, (SDW_REGADDR + 1) - (addr & SDW_REGADDR));
>  
> -	ret = sdw_transfer(slave->bus, &msg);
> -	if (slave->is_mockup_device)
> -		ret = 0;
> -	return ret;
> +		ret = sdw_fill_msg(&msg, slave, addr, size, slave->dev_num, flags, val);
> +		if (ret < 0)
> +			return ret;
> +
> +		ret = sdw_transfer(slave->bus, &msg);
> +		if (ret < 0 && !slave->is_mockup_device)
> +			return ret;
> +
> +		addr += size;
> +		val += size;
> +		count -= size;
> +	}
> +
> +	return 0;
> +}
> +
> +int sdw_nread_no_pm(struct sdw_slave *slave, u32 addr, size_t count, u8 *val)
> +{
> +	return sdw_ntransfer_no_pm(slave, addr, SDW_MSG_FLAG_READ, count, val);
>  }
>  EXPORT_SYMBOL(sdw_nread_no_pm);
>  
>  int sdw_nwrite_no_pm(struct sdw_slave *slave, u32 addr, size_t count, const u8 *val)
>  {
> -	struct sdw_msg msg;
> -	int ret;
> -
> -	ret = sdw_fill_msg(&msg, slave, addr, count,
> -			   slave->dev_num, SDW_MSG_FLAG_WRITE, (u8 *)val);
> -	if (ret < 0)
> -		return ret;
> -
> -	ret = sdw_transfer(slave->bus, &msg);
> -	if (slave->is_mockup_device)
> -		ret = 0;
> -	return ret;
> +	return sdw_ntransfer_no_pm(slave, addr, SDW_MSG_FLAG_WRITE, count, (u8 *)val);
>  }
>  EXPORT_SYMBOL(sdw_nwrite_no_pm);
>  

Re: [PATCH 2/2] soundwire: bus: Update sdw_nread/nwrite_no_pm to handle page boundaries

[kernel test robot]

Hi Charles,

I love your patch! Perhaps something to improve:

[auto build test WARNING on linus/master]
[also build test WARNING on v6.3-rc2 next-20230316]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Charles-Keepax/soundwire-bus-Update-sdw_nread-nwrite_no_pm-to-handle-page-boundaries/20230317-000005
patch link:    https://lore.kernel.org/r/20230316155734.3191577-2-ckeepax%40opensource.cirrus.com
patch subject: [PATCH 2/2] soundwire: bus: Update sdw_nread/nwrite_no_pm to handle page boundaries
config: mips-randconfig-r022-20230312 (https://download.01.org/0day-ci/archive/20230317/202303170724.NdbQwtQo-lkp@intel.com/config)
compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project 67409911353323ca5edf2049ef0df54132fa1ca7)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install mips cross compiling tool for clang build
        # apt-get install binutils-mipsel-linux-gnu
        # https://github.com/intel-lab-lkp/linux/commit/6944d7175bd15a9a16a411c57f200d3bcecd3c00
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Charles-Keepax/soundwire-bus-Update-sdw_nread-nwrite_no_pm-to-handle-page-boundaries/20230317-000005
        git checkout 6944d7175bd15a9a16a411c57f200d3bcecd3c00
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=mips olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=mips SHELL=/bin/bash drivers/soundwire/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202303170724.NdbQwtQo-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/soundwire/bus.c:398:10: warning: comparison of distinct pointer types ('typeof (count) *' (aka 'unsigned int *') and 'typeof ((((((int)(sizeof(struct (unnamed struct at drivers/soundwire/bus.c:398:10))))) + (((~(((0UL)))) - ((((1UL))) << (0)) + 1) & (~(((0UL))) >> (32 - 1 - (14))))) + 1) - (addr & ((((int)(sizeof(struct (unnamed struct at drivers/soundwire/bus.c:398:10))))) + (((~(((0UL)))) - ((((1UL))) << (0)) + 1) & (~(((0UL))) >> (32 - 1 - (14))))))) *' (aka 'unsigned long *')) [-Wcompare-distinct-pointer-types]
                   size = min(count, (SDW_REGADDR + 1) - (addr & SDW_REGADDR));
                          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/minmax.h:67:19: note: expanded from macro 'min'
   #define min(x, y)       __careful_cmp(x, y, <)
                           ^~~~~~~~~~~~~~~~~~~~~~
   include/linux/minmax.h:36:24: note: expanded from macro '__careful_cmp'
           __builtin_choose_expr(__safe_cmp(x, y), \
                                 ^~~~~~~~~~~~~~~~
   include/linux/minmax.h:26:4: note: expanded from macro '__safe_cmp'
                   (__typecheck(x, y) && __no_side_effects(x, y))
                    ^~~~~~~~~~~~~~~~~
   include/linux/minmax.h:20:28: note: expanded from macro '__typecheck'
           (!!(sizeof((typeof(x) *)1 == (typeof(y) *)1)))
                      ~~~~~~~~~~~~~~ ^  ~~~~~~~~~~~~~~
   1 warning generated.


vim +398 drivers/soundwire/bus.c

   384	
   385	/*
   386	 * Read/Write IO functions.
   387	 */
   388	
   389	static int sdw_ntransfer_no_pm(struct sdw_slave *slave, u32 addr, u8 flags,
   390				       size_t count, u8 *val)
   391	{
   392		struct sdw_msg msg;
   393		size_t size;
   394		int ret;
   395	
   396		while (count) {
   397			// Only handle bytes up to next page boundary
 > 398			size = min(count, (SDW_REGADDR + 1) - (addr & SDW_REGADDR));
   399	
   400			ret = sdw_fill_msg(&msg, slave, addr, size, slave->dev_num, flags, val);
   401			if (ret < 0)
   402				return ret;
   403	
   404			ret = sdw_transfer(slave->bus, &msg);
   405			if (ret < 0 && !slave->is_mockup_device)
   406				return ret;
   407	
   408			addr += size;
   409			val += size;
   410			count -= size;
   411		}
   412	
   413		return 0;
   414	}
   415	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

Re: [PATCH 2/2] soundwire: bus: Update sdw_nread/nwrite_no_pm to handle page boundaries

[kernel test robot]

Hi Charles,

I love your patch! Perhaps something to improve:

[auto build test WARNING on linus/master]
[also build test WARNING on v6.3-rc2 next-20230316]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Charles-Keepax/soundwire-bus-Update-sdw_nread-nwrite_no_pm-to-handle-page-boundaries/20230317-000005
patch link:    https://lore.kernel.org/r/20230316155734.3191577-2-ckeepax%40opensource.cirrus.com
patch subject: [PATCH 2/2] soundwire: bus: Update sdw_nread/nwrite_no_pm to handle page boundaries
config: i386-randconfig-a011-20230313 (https://download.01.org/0day-ci/archive/20230317/202303170749.83Yd8Oh0-lkp@intel.com/config)
compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project f28c006a5895fc0e329fe15fead81e37457cb1d1)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/6944d7175bd15a9a16a411c57f200d3bcecd3c00
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Charles-Keepax/soundwire-bus-Update-sdw_nread-nwrite_no_pm-to-handle-page-boundaries/20230317-000005
        git checkout 6944d7175bd15a9a16a411c57f200d3bcecd3c00
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash drivers/soundwire/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202303170749.83Yd8Oh0-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/soundwire/bus.c:398:10: warning: comparison of distinct pointer types ('typeof (count) *' (aka 'unsigned int *') and 'typeof ((((((int)(sizeof(struct (unnamed struct at drivers/soundwire/bus.c:398:10))))) + (((~(((0UL)))) - ((((1UL))) << (0)) + 1) & (~(((0UL))) >> (32 - 1 - (14))))) + 1) - (addr & ((((int)(sizeof(struct (unnamed struct at drivers/soundwire/bus.c:398:10))))) + (((~(((0UL)))) - ((((1UL))) << (0)) + 1) & (~(((0UL))) >> (32 - 1 - (14))))))) *' (aka 'unsigned long *')) [-Wcompare-distinct-pointer-types]
                   size = min(count, (SDW_REGADDR + 1) - (addr & SDW_REGADDR));
                          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/minmax.h:67:19: note: expanded from macro 'min'
   #define min(x, y)       __careful_cmp(x, y, <)
                           ^~~~~~~~~~~~~~~~~~~~~~
   include/linux/minmax.h:36:24: note: expanded from macro '__careful_cmp'
           __builtin_choose_expr(__safe_cmp(x, y), \
                                 ^~~~~~~~~~~~~~~~
   include/linux/minmax.h:26:4: note: expanded from macro '__safe_cmp'
                   (__typecheck(x, y) && __no_side_effects(x, y))
                    ^~~~~~~~~~~~~~~~~
   include/linux/minmax.h:20:28: note: expanded from macro '__typecheck'
           (!!(sizeof((typeof(x) *)1 == (typeof(y) *)1)))
                      ~~~~~~~~~~~~~~ ^  ~~~~~~~~~~~~~~
   1 warning generated.


vim +398 drivers/soundwire/bus.c

   384	
   385	/*
   386	 * Read/Write IO functions.
   387	 */
   388	
   389	static int sdw_ntransfer_no_pm(struct sdw_slave *slave, u32 addr, u8 flags,
   390				       size_t count, u8 *val)
   391	{
   392		struct sdw_msg msg;
   393		size_t size;
   394		int ret;
   395	
   396		while (count) {
   397			// Only handle bytes up to next page boundary
 > 398			size = min(count, (SDW_REGADDR + 1) - (addr & SDW_REGADDR));
   399	
   400			ret = sdw_fill_msg(&msg, slave, addr, size, slave->dev_num, flags, val);
   401			if (ret < 0)
   402				return ret;
   403	
   404			ret = sdw_transfer(slave->bus, &msg);
   405			if (ret < 0 && !slave->is_mockup_device)
   406				return ret;
   407	
   408			addr += size;
   409			val += size;
   410			count -= size;
   411		}
   412	
   413		return 0;
   414	}
   415	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

Re: [PATCH 2/2] soundwire: bus: Update sdw_nread/nwrite_no_pm to handle page boundaries

[Charles Keepax]

On Thu, Mar 16, 2023 at 01:46:57PM -0500, Pierre-Louis Bossart wrote:
> 
> 
> On 3/16/23 10:57, Charles Keepax wrote:
> > Currently issuing a sdw_nread/nwrite_no_pm across a page boundary
> > will silently fail to write correctly as nothing updates the page
> > registers, meaning the same page of the chip will get rewritten
> > with each successive page of data.
> > 
> > As the sdw_msg structure contains page information it seems
> > reasonable that a single sdw_msg should always be within one
> > page. It is also mostly simpler to handle the paging at the
> > bus level rather than each master having to handle it in their
> > xfer_msg callback.
> > 
> > As such add handling to the bus code to split up a transfer into
> > multiple sdw_msg's when they go across page boundaries.
> 
> This sounds good but we need to clarify that the multiple sdw_msg's will
> not necessarily be sent one after the other, the msg_lock is held in the
> sdw_transfer() function, so there should be no expectation that e.g. one
> big chunk of firmware code can be sent without interruption.
> 

I will update the kdoc for nread/nwrite to specify that
transactions that cross a page boundry are not expected to be
atomic.

> I also wonder if we should have a lower bar than the page to avoid
> hogging the bus with large read/write transactions. If there are
> multiple devices on the same link and one of them signals an alert
> status while a large transfer is on-going, the alert handling will
> mechanically be delayed by up to a page - that's 32k reads/writes, isn't it?
> 

I think its 16k, but I would be inclined to say this is a
separate fix. The current code will tie up the bus for longer
than my fix does, since it only calls sdw_transfer once, and it
will write the wrong registers whilst doing it. Also to be clear
this wasn't found with super large transfers just medium sized
ones that happened to cross a page boundry.

If we want to add some transaction size capping that is really
a new feature, this patch a bug fix. I would also be inclined
to say if we are going to add transaction size capping, we
probably want some property to specify what we are capping to.

Thanks,
Charles

> > Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
> > ---
> >  drivers/soundwire/bus.c | 47 +++++++++++++++++++++++------------------
> >  1 file changed, 26 insertions(+), 21 deletions(-)
> > 
> > diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
> > index 3c67266f94834..bdd251e871694 100644
> > --- a/drivers/soundwire/bus.c
> > +++ b/drivers/soundwire/bus.c
> > @@ -386,37 +386,42 @@ int sdw_fill_msg(struct sdw_msg *msg, struct sdw_slave *slave,
> >   * Read/Write IO functions.
> >   */
> >  
> > -int sdw_nread_no_pm(struct sdw_slave *slave, u32 addr, size_t count, u8 *val)
> > +static int sdw_ntransfer_no_pm(struct sdw_slave *slave, u32 addr, u8 flags,
> > +			       size_t count, u8 *val)
> >  {
> >  	struct sdw_msg msg;
> > +	size_t size;
> >  	int ret;
> >  
> > -	ret = sdw_fill_msg(&msg, slave, addr, count,
> > -			   slave->dev_num, SDW_MSG_FLAG_READ, val);
> > -	if (ret < 0)
> > -		return ret;
> > +	while (count) {
> > +		// Only handle bytes up to next page boundary
> > +		size = min(count, (SDW_REGADDR + 1) - (addr & SDW_REGADDR));
> >  
> > -	ret = sdw_transfer(slave->bus, &msg);
> > -	if (slave->is_mockup_device)
> > -		ret = 0;
> > -	return ret;
> > +		ret = sdw_fill_msg(&msg, slave, addr, size, slave->dev_num, flags, val);
> > +		if (ret < 0)
> > +			return ret;
> > +
> > +		ret = sdw_transfer(slave->bus, &msg);
> > +		if (ret < 0 && !slave->is_mockup_device)
> > +			return ret;
> > +
> > +		addr += size;
> > +		val += size;
> > +		count -= size;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +int sdw_nread_no_pm(struct sdw_slave *slave, u32 addr, size_t count, u8 *val)
> > +{
> > +	return sdw_ntransfer_no_pm(slave, addr, SDW_MSG_FLAG_READ, count, val);
> >  }
> >  EXPORT_SYMBOL(sdw_nread_no_pm);
> >  
> >  int sdw_nwrite_no_pm(struct sdw_slave *slave, u32 addr, size_t count, const u8 *val)
> >  {
> > -	struct sdw_msg msg;
> > -	int ret;
> > -
> > -	ret = sdw_fill_msg(&msg, slave, addr, count,
> > -			   slave->dev_num, SDW_MSG_FLAG_WRITE, (u8 *)val);
> > -	if (ret < 0)
> > -		return ret;
> > -
> > -	ret = sdw_transfer(slave->bus, &msg);
> > -	if (slave->is_mockup_device)
> > -		ret = 0;
> > -	return ret;
> > +	return sdw_ntransfer_no_pm(slave, addr, SDW_MSG_FLAG_WRITE, count, (u8 *)val);
> >  }
> >  EXPORT_SYMBOL(sdw_nwrite_no_pm);
> >  

Re: [PATCH 2/2] soundwire: bus: Update sdw_nread/nwrite_no_pm to handle page boundaries

[Charles Keepax via Alsa-devel]

[-- Attachment #0: Type: message/rfc822, Size: 9304 bytes --]

From: Charles Keepax <ckeepax@opensource.cirrus.com>
To: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Cc: vkoul@kernel.org, yung-chuan.liao@linux.intel.com, sanyog.r.kale@intel.com, alsa-devel@alsa-project.org, linux-kernel@vger.kernel.org, patches@opensource.cirrus.com
Subject: Re: [PATCH 2/2] soundwire: bus: Update sdw_nread/nwrite_no_pm to handle page boundaries
Date: Fri, 17 Mar 2023 14:08:07 +0000
Message-ID: <20230317140807.GI68926@ediswmail.ad.cirrus.com>

On Thu, Mar 16, 2023 at 01:46:57PM -0500, Pierre-Louis Bossart wrote:
> 
> 
> On 3/16/23 10:57, Charles Keepax wrote:
> > Currently issuing a sdw_nread/nwrite_no_pm across a page boundary
> > will silently fail to write correctly as nothing updates the page
> > registers, meaning the same page of the chip will get rewritten
> > with each successive page of data.
> > 
> > As the sdw_msg structure contains page information it seems
> > reasonable that a single sdw_msg should always be within one
> > page. It is also mostly simpler to handle the paging at the
> > bus level rather than each master having to handle it in their
> > xfer_msg callback.
> > 
> > As such add handling to the bus code to split up a transfer into
> > multiple sdw_msg's when they go across page boundaries.
> 
> This sounds good but we need to clarify that the multiple sdw_msg's will
> not necessarily be sent one after the other, the msg_lock is held in the
> sdw_transfer() function, so there should be no expectation that e.g. one
> big chunk of firmware code can be sent without interruption.
> 

I will update the kdoc for nread/nwrite to specify that
transactions that cross a page boundry are not expected to be
atomic.

> I also wonder if we should have a lower bar than the page to avoid
> hogging the bus with large read/write transactions. If there are
> multiple devices on the same link and one of them signals an alert
> status while a large transfer is on-going, the alert handling will
> mechanically be delayed by up to a page - that's 32k reads/writes, isn't it?
> 

I think its 16k, but I would be inclined to say this is a
separate fix. The current code will tie up the bus for longer
than my fix does, since it only calls sdw_transfer once, and it
will write the wrong registers whilst doing it. Also to be clear
this wasn't found with super large transfers just medium sized
ones that happened to cross a page boundry.

If we want to add some transaction size capping that is really
a new feature, this patch a bug fix. I would also be inclined
to say if we are going to add transaction size capping, we
probably want some property to specify what we are capping to.

Thanks,
Charles

> > Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
> > ---
> >  drivers/soundwire/bus.c | 47 +++++++++++++++++++++++------------------
> >  1 file changed, 26 insertions(+), 21 deletions(-)
> > 
> > diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
> > index 3c67266f94834..bdd251e871694 100644
> > --- a/drivers/soundwire/bus.c
> > +++ b/drivers/soundwire/bus.c
> > @@ -386,37 +386,42 @@ int sdw_fill_msg(struct sdw_msg *msg, struct sdw_slave *slave,
> >   * Read/Write IO functions.
> >   */
> >  
> > -int sdw_nread_no_pm(struct sdw_slave *slave, u32 addr, size_t count, u8 *val)
> > +static int sdw_ntransfer_no_pm(struct sdw_slave *slave, u32 addr, u8 flags,
> > +			       size_t count, u8 *val)
> >  {
> >  	struct sdw_msg msg;
> > +	size_t size;
> >  	int ret;
> >  
> > -	ret = sdw_fill_msg(&msg, slave, addr, count,
> > -			   slave->dev_num, SDW_MSG_FLAG_READ, val);
> > -	if (ret < 0)
> > -		return ret;
> > +	while (count) {
> > +		// Only handle bytes up to next page boundary
> > +		size = min(count, (SDW_REGADDR + 1) - (addr & SDW_REGADDR));
> >  
> > -	ret = sdw_transfer(slave->bus, &msg);
> > -	if (slave->is_mockup_device)
> > -		ret = 0;
> > -	return ret;
> > +		ret = sdw_fill_msg(&msg, slave, addr, size, slave->dev_num, flags, val);
> > +		if (ret < 0)
> > +			return ret;
> > +
> > +		ret = sdw_transfer(slave->bus, &msg);
> > +		if (ret < 0 && !slave->is_mockup_device)
> > +			return ret;
> > +
> > +		addr += size;
> > +		val += size;
> > +		count -= size;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +int sdw_nread_no_pm(struct sdw_slave *slave, u32 addr, size_t count, u8 *val)
> > +{
> > +	return sdw_ntransfer_no_pm(slave, addr, SDW_MSG_FLAG_READ, count, val);
> >  }
> >  EXPORT_SYMBOL(sdw_nread_no_pm);
> >  
> >  int sdw_nwrite_no_pm(struct sdw_slave *slave, u32 addr, size_t count, const u8 *val)
> >  {
> > -	struct sdw_msg msg;
> > -	int ret;
> > -
> > -	ret = sdw_fill_msg(&msg, slave, addr, count,
> > -			   slave->dev_num, SDW_MSG_FLAG_WRITE, (u8 *)val);
> > -	if (ret < 0)
> > -		return ret;
> > -
> > -	ret = sdw_transfer(slave->bus, &msg);
> > -	if (slave->is_mockup_device)
> > -		ret = 0;
> > -	return ret;
> > +	return sdw_ntransfer_no_pm(slave, addr, SDW_MSG_FLAG_WRITE, count, (u8 *)val);
> >  }
> >  EXPORT_SYMBOL(sdw_nwrite_no_pm);
> >  

Re: [PATCH 2/2] soundwire: bus: Update sdw_nread/nwrite_no_pm to handle page boundaries

[Pierre-Louis Bossart]

On 3/17/23 09:08, Charles Keepax wrote:
> On Thu, Mar 16, 2023 at 01:46:57PM -0500, Pierre-Louis Bossart wrote:
>>
>>
>> On 3/16/23 10:57, Charles Keepax wrote:
>>> Currently issuing a sdw_nread/nwrite_no_pm across a page boundary
>>> will silently fail to write correctly as nothing updates the page
>>> registers, meaning the same page of the chip will get rewritten
>>> with each successive page of data.
>>>
>>> As the sdw_msg structure contains page information it seems
>>> reasonable that a single sdw_msg should always be within one
>>> page. It is also mostly simpler to handle the paging at the
>>> bus level rather than each master having to handle it in their
>>> xfer_msg callback.
>>>
>>> As such add handling to the bus code to split up a transfer into
>>> multiple sdw_msg's when they go across page boundaries.
>>
>> This sounds good but we need to clarify that the multiple sdw_msg's will
>> not necessarily be sent one after the other, the msg_lock is held in the
>> sdw_transfer() function, so there should be no expectation that e.g. one
>> big chunk of firmware code can be sent without interruption.
>>
> 
> I will update the kdoc for nread/nwrite to specify that
> transactions that cross a page boundry are not expected to be
> atomic.

Sounds good.

>> I also wonder if we should have a lower bar than the page to avoid
>> hogging the bus with large read/write transactions. If there are
>> multiple devices on the same link and one of them signals an alert
>> status while a large transfer is on-going, the alert handling will
>> mechanically be delayed by up to a page - that's 32k reads/writes, isn't it?
>>
> 
> I think its 16k, but I would be inclined to say this is a
> separate fix. The current code will tie up the bus for longer
> than my fix does, since it only calls sdw_transfer once, and it
> will write the wrong registers whilst doing it. Also to be clear
> this wasn't found with super large transfers just medium sized
> ones that happened to cross a page boundry.
> 
> If we want to add some transaction size capping that is really
> a new feature, this patch a bug fix. I would also be inclined
> to say if we are going to add transaction size capping, we
> probably want some property to specify what we are capping to.

Yes indeed, this would be a new feature. I think this should be a
manager property, depending on which peripherals are integrated and what
latencies are expected.

[PATCH 1/2] soundwire: bus: Remove now outdated comments on no_pm IO

[Charles Keepax via Alsa-devel]

[-- Attachment #0: Type: message/rfc822, Size: 5500 bytes --]

From: Charles Keepax <ckeepax@opensource.cirrus.com>
To: <vkoul@kernel.org>
Cc: yung-chuan.liao@linux.intel.com, pierre-louis.bossart@linux.intel.com, sanyog.r.kale@intel.com, alsa-devel@alsa-project.org, linux-kernel@vger.kernel.org, patches@opensource.cirrus.com
Subject: [PATCH 1/2] soundwire: bus: Remove now outdated comments on no_pm IO
Date: Thu, 16 Mar 2023 15:57:33 +0000
Message-ID: <20230316155734.3191577-1-ckeepax@opensource.cirrus.com>

Things have moved more towards end drivers using the no_pm versions of
the IO functions. See commits:

commit 167790abb90f ("soundwire: export sdw_write/read_no_pm functions")
commit 62dc9f3f2fd0 ("soundwire: bus: export sdw_nwrite_no_pm and
                      sdw_nread_no_pm functions")

As such this comment is now misleading, so remove it.

Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---
 drivers/soundwire/bus.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
index b6aca59c31300..3c67266f94834 100644
--- a/drivers/soundwire/bus.c
+++ b/drivers/soundwire/bus.c
@@ -384,9 +384,6 @@ int sdw_fill_msg(struct sdw_msg *msg, struct sdw_slave *slave,
 
 /*
  * Read/Write IO functions.
- * no_pm versions can only be called by the bus, e.g. while enumerating or
- * handling suspend-resume sequences.
- * all clients need to use the pm versions
  */
 
 int sdw_nread_no_pm(struct sdw_slave *slave, u32 addr, size_t count, u8 *val)
-- 
2.30.2