[PATCH 0/2] soundwire: bus: Prevent infinite loop in sdw_ch_mask_to_ch()

[PATCH 0/2] soundwire: bus: Prevent infinite loop in sdw_ch_mask_to_ch()

[Richard Fitzgerald]

sdw_ch_mask_to_ch() would loop forever if bit 31 of the mask was set.
The entire function is unnecessary because the existing standard
function hweight32() already does the same thing, and hweight32() is
safe. But the change has been made in two steps.

Richard Fitzgerald (2):
  soundwire: bus: Prevent infinite loop in sdw_ch_mask_to_ch()
  soundwire: bandwidth allocation: Use hweight32() to calculate set bits

 drivers/soundwire/bus.h                          | 11 -----------
 drivers/soundwire/generic_bandwidth_allocation.c |  3 ++-
 2 files changed, 2 insertions(+), 12 deletions(-)

-- 
2.30.2

[PATCH 1/2] soundwire: bus: Prevent infinite loop in sdw_ch_mask_to_ch()

[Richard Fitzgerald]

Define the ch_mask argument of sdw_ch_mask_to_ch() as an unsigned
so that the shift right is guaranteed to eventually make the
value of ch_mask==0.

Previously ch_mask was defined as a signed int, but a right
shift of a signed value preserves the sign bit. So if the sign
bit was 1, ch_mask would never become 0 and the for loop would
be infinite.

Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
---
 drivers/soundwire/bus.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/soundwire/bus.h b/drivers/soundwire/bus.h
index 7631ef5e71fb..28bedc919b78 100644
--- a/drivers/soundwire/bus.h
+++ b/drivers/soundwire/bus.h
@@ -160,7 +160,7 @@ int sdw_fill_msg(struct sdw_msg *msg, struct sdw_slave *slave,
 		 u32 addr, size_t count, u16 dev_num, u8 flags, u8 *buf);
 
 /* Retrieve and return channel count from channel mask */
-static inline int sdw_ch_mask_to_ch(int ch_mask)
+static inline int sdw_ch_mask_to_ch(unsigned int ch_mask)
 {
 	int c = 0;
 
-- 
2.30.2

Re: [PATCH 1/2] soundwire: bus: Prevent infinite loop in sdw_ch_mask_to_ch()

[Pierre-Louis Bossart]

On 2/2/23 09:42, Richard Fitzgerald wrote:
> Define the ch_mask argument of sdw_ch_mask_to_ch() as an unsigned
> so that the shift right is guaranteed to eventually make the
> value of ch_mask==0.
> 
> Previously ch_mask was defined as a signed int, but a right
> shift of a signed value preserves the sign bit. So if the sign
> bit was 1, ch_mask would never become 0 and the for loop would
> be infinite.
> Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
> ---
>  drivers/soundwire/bus.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/soundwire/bus.h b/drivers/soundwire/bus.h
> index 7631ef5e71fb..28bedc919b78 100644
> --- a/drivers/soundwire/bus.h
> +++ b/drivers/soundwire/bus.h
> @@ -160,7 +160,7 @@ int sdw_fill_msg(struct sdw_msg *msg, struct sdw_slave *slave,
>  		 u32 addr, size_t count, u16 dev_num, u8 flags, u8 *buf);
>  
>  /* Retrieve and return channel count from channel mask */
> -static inline int sdw_ch_mask_to_ch(int ch_mask)
> +static inline int sdw_ch_mask_to_ch(unsigned int ch_mask)
>  {
>  	int c = 0;
>  

This patch1 is fine, but you remove this function in patch2, so is this
patch needed at all?

-/* Retrieve and return channel count from channel mask */
-static inline int sdw_ch_mask_to_ch(unsigned int ch_mask)
-{
-	int c = 0;
-
-	for (c = 0; ch_mask; ch_mask >>= 1)
-		c += ch_mask & 1;
-
-	return c;
-}
-

Re: [PATCH 1/2] soundwire: bus: Prevent infinite loop in sdw_ch_mask_to_ch()

[Richard Fitzgerald]

On 03/02/2023 14:35, Pierre-Louis Bossart wrote:
> 
> 
> On 2/2/23 09:42, Richard Fitzgerald wrote:
>> Define the ch_mask argument of sdw_ch_mask_to_ch() as an unsigned
>> so that the shift right is guaranteed to eventually make the
>> value of ch_mask==0.
>>
>> Previously ch_mask was defined as a signed int, but a right
>> shift of a signed value preserves the sign bit. So if the sign
>> bit was 1, ch_mask would never become 0 and the for loop would
>> be infinite.
>> Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
>> ---
>>   drivers/soundwire/bus.h | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/soundwire/bus.h b/drivers/soundwire/bus.h
>> index 7631ef5e71fb..28bedc919b78 100644
>> --- a/drivers/soundwire/bus.h
>> +++ b/drivers/soundwire/bus.h
>> @@ -160,7 +160,7 @@ int sdw_fill_msg(struct sdw_msg *msg, struct sdw_slave *slave,
>>   		 u32 addr, size_t count, u16 dev_num, u8 flags, u8 *buf);
>>   
>>   /* Retrieve and return channel count from channel mask */
>> -static inline int sdw_ch_mask_to_ch(int ch_mask)
>> +static inline int sdw_ch_mask_to_ch(unsigned int ch_mask)
>>   {
>>   	int c = 0;
>>   
> 
> This patch1 is fine, but you remove this function in patch2, so is this
> patch needed at all?
> 
> -/* Retrieve and return channel count from channel mask */
> -static inline int sdw_ch_mask_to_ch(unsigned int ch_mask)
> -{
> -	int c = 0;
> -
> -	for (c = 0; ch_mask; ch_mask >>= 1)
> -		c += ch_mask & 1;
> -
> -	return c;
> -}
> -

I'm happy to squash them, I did it in two steps so it didn't get
overlooked that there's a bugfix happening.

Re: [PATCH 1/2] soundwire: bus: Prevent infinite loop in sdw_ch_mask_to_ch()

[Vinod Koul]

On 03-02-23, 16:18, Richard Fitzgerald wrote:
> On 03/02/2023 14:35, Pierre-Louis Bossart wrote:
> > 
> > 
> > On 2/2/23 09:42, Richard Fitzgerald wrote:
> > > Define the ch_mask argument of sdw_ch_mask_to_ch() as an unsigned
> > > so that the shift right is guaranteed to eventually make the
> > > value of ch_mask==0.
> > > 
> > > Previously ch_mask was defined as a signed int, but a right
> > > shift of a signed value preserves the sign bit. So if the sign
> > > bit was 1, ch_mask would never become 0 and the for loop would
> > > be infinite.
> > > Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
> > > ---
> > >   drivers/soundwire/bus.h | 2 +-
> > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/soundwire/bus.h b/drivers/soundwire/bus.h
> > > index 7631ef5e71fb..28bedc919b78 100644
> > > --- a/drivers/soundwire/bus.h
> > > +++ b/drivers/soundwire/bus.h
> > > @@ -160,7 +160,7 @@ int sdw_fill_msg(struct sdw_msg *msg, struct sdw_slave *slave,
> > >   		 u32 addr, size_t count, u16 dev_num, u8 flags, u8 *buf);
> > >   /* Retrieve and return channel count from channel mask */
> > > -static inline int sdw_ch_mask_to_ch(int ch_mask)
> > > +static inline int sdw_ch_mask_to_ch(unsigned int ch_mask)
> > >   {
> > >   	int c = 0;
> > 
> > This patch1 is fine, but you remove this function in patch2, so is this
> > patch needed at all?
> > 
> > -/* Retrieve and return channel count from channel mask */
> > -static inline int sdw_ch_mask_to_ch(unsigned int ch_mask)
> > -{
> > -	int c = 0;
> > -
> > -	for (c = 0; ch_mask; ch_mask >>= 1)
> > -		c += ch_mask & 1;
> > -
> > -	return c;
> > -}
> > -
> 
> I'm happy to squash them, I did it in two steps so it didn't get
> overlooked that there's a bugfix happening.

I think this case is fine to squash and send as a single patch while
documenting that we are fixing the bug in the log

Thanks
-- 
~Vinod

[PATCH 2/2] soundwire: bandwidth allocation: Use hweight32() to calculate set bits

[Richard Fitzgerald]

Replace the call to sdw_ch_mask_to_ch() with a call to hweight32().

sdw_ch_mask_to_ch() is counting the number of set bits. The hweight()
family of functions already do this, and they have an advantage of
using a bit-counting instruction if it is available on the target CPU.

Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
---
 drivers/soundwire/bus.h                          | 11 -----------
 drivers/soundwire/generic_bandwidth_allocation.c |  3 ++-
 2 files changed, 2 insertions(+), 12 deletions(-)

diff --git a/drivers/soundwire/bus.h b/drivers/soundwire/bus.h
index 28bedc919b78..4ce8d708a39c 100644
--- a/drivers/soundwire/bus.h
+++ b/drivers/soundwire/bus.h
@@ -159,17 +159,6 @@ int sdw_transfer_defer(struct sdw_bus *bus, struct sdw_msg *msg,
 int sdw_fill_msg(struct sdw_msg *msg, struct sdw_slave *slave,
 		 u32 addr, size_t count, u16 dev_num, u8 flags, u8 *buf);
 
-/* Retrieve and return channel count from channel mask */
-static inline int sdw_ch_mask_to_ch(unsigned int ch_mask)
-{
-	int c = 0;
-
-	for (c = 0; ch_mask; ch_mask >>= 1)
-		c += ch_mask & 1;
-
-	return c;
-}
-
 /* Fill transport parameter data structure */
 static inline void sdw_fill_xport_params(struct sdw_transport_params *params,
 					 int port_num, bool grp_ctrl_valid,
diff --git a/drivers/soundwire/generic_bandwidth_allocation.c b/drivers/soundwire/generic_bandwidth_allocation.c
index f7c66083a4dd..ea3e8ef408e4 100644
--- a/drivers/soundwire/generic_bandwidth_allocation.c
+++ b/drivers/soundwire/generic_bandwidth_allocation.c
@@ -6,6 +6,7 @@
  *
  */
 
+#include <linux/bitops.h>
 #include <linux/device.h>
 #include <linux/module.h>
 #include <linux/mod_devicetable.h>
@@ -54,7 +55,7 @@ static void sdw_compute_slave_ports(struct sdw_master_runtime *m_rt,
 		slave_total_ch = 0;
 
 		list_for_each_entry(p_rt, &s_rt->port_list, port_node) {
-			ch = sdw_ch_mask_to_ch(p_rt->ch_mask);
+			ch = hweight32(p_rt->ch_mask);
 
 			sdw_fill_xport_params(&p_rt->transport_params,
 					      p_rt->num, false,
-- 
2.30.2