[PATCH 0/2] soundwire: bus: Prevent infinite loop in sdw_ch_mask_to_ch()
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(-)
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;
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; -} -
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.
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
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,
participants (3)
-
Pierre-Louis Bossart -
Richard Fitzgerald -
Vinod Koul