| Message ID | 20230202154212.1098736-2-rf@opensource.cirrus.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | soundwire: bus: Prevent infinite loop in sdw_ch_mask_to_ch() | expand |
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
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;
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(-)