ssb_wait_bit was designed for only one-bit bitmasks.
People start using it for multi-bit bitmasks. Make the "set" case
is safe for this. The "unset" case is already safe.
This does not change behavior of the current code.
Signed-off-by: Michael Buesch <[email protected]>
---
Index: wireless-testing/drivers/ssb/main.c
===================================================================
--- wireless-testing.orig/drivers/ssb/main.c 2010-12-30 15:49:39.235946210 +0100
+++ wireless-testing/drivers/ssb/main.c 2011-02-15 00:17:35.727816704 +0100
@@ -1192,10 +1192,10 @@ void ssb_device_enable(struct ssb_device
}
EXPORT_SYMBOL(ssb_device_enable);
-/* Wait for a bit in a register to get set or unset.
+/* Wait for bitmask in a register to get set or cleared.
* timeout is in units of ten-microseconds */
-static int ssb_wait_bit(struct ssb_device *dev, u16 reg, u32 bitmask,
- int timeout, int set)
+static int ssb_wait_bits(struct ssb_device *dev, u16 reg, u32 bitmask,
+ int timeout, int set)
{
int i;
u32 val;
@@ -1203,7 +1203,7 @@ static int ssb_wait_bit(struct ssb_devic
for (i = 0; i < timeout; i++) {
val = ssb_read32(dev, reg);
if (set) {
- if (val & bitmask)
+ if ((val & bitmask) == bitmask)
return 0;
} else {
if (!(val & bitmask))
@@ -1227,8 +1227,8 @@ void ssb_device_disable(struct ssb_devic
reject = ssb_tmslow_reject_bitmask(dev);
ssb_write32(dev, SSB_TMSLOW, reject | SSB_TMSLOW_CLOCK);
- ssb_wait_bit(dev, SSB_TMSLOW, reject, 1000, 1);
- ssb_wait_bit(dev, SSB_TMSHIGH, SSB_TMSHIGH_BUSY, 1000, 0);
+ ssb_wait_bits(dev, SSB_TMSLOW, reject, 1000, 1);
+ ssb_wait_bits(dev, SSB_TMSHIGH, SSB_TMSHIGH_BUSY, 1000, 0);
ssb_write32(dev, SSB_TMSLOW,
SSB_TMSLOW_FGC | SSB_TMSLOW_CLOCK |
reject | SSB_TMSLOW_RESET |
--
Greetings Michael.
On Tue, 2011-02-15 at 00:44 +0100, Rafał Miłecki wrote:
> 2011/2/15 Michael Büsch <[email protected]>:
> > ssb_wait_bit was designed for only one-bit bitmasks.
> > People start using it for multi-bit bitmasks. Make the "set" case
> > is safe for this. The "unset" case is already safe.
>
> Thanks :)
This should not change the behavior of your patch, though,
as you only add an "unset" case, which was already correct.
So this doesn't fix an actual bug but rather hardens the code
for future use.
--
Greetings Michael.
On Tue, Feb 15, 2011 at 12:21 AM, Michael B?sch <[email protected]> wrote:
> ssb_wait_bit was designed for only one-bit bitmasks.
> People start using it for multi-bit bitmasks. Make the "set" case
> is safe for this. The "unset" case is already safe.
>
> This does not change behavior of the current code.
>
> Signed-off-by: Michael Buesch <[email protected]>
>
> ---
>
> Index: wireless-testing/drivers/ssb/main.c
> ===================================================================
> --- wireless-testing.orig/drivers/ssb/main.c ? ?2010-12-30 15:49:39.235946210 +0100
> +++ wireless-testing/drivers/ssb/main.c 2011-02-15 00:17:35.727816704 +0100
> @@ -1192,10 +1192,10 @@ void ssb_device_enable(struct ssb_device
> ?}
> ?EXPORT_SYMBOL(ssb_device_enable);
>
> -/* Wait for a bit in a register to get set or unset.
> +/* Wait for bitmask in a register to get set or cleared.
> ?* timeout is in units of ten-microseconds */
> -static int ssb_wait_bit(struct ssb_device *dev, u16 reg, u32 bitmask,
> - ? ? ? ? ? ? ? ? ? ? ? int timeout, int set)
> +static int ssb_wait_bits(struct ssb_device *dev, u16 reg, u32 bitmask,
> + ? ? ? ? ? ? ? ? ? ? ? ?int timeout, int set)
While you are at it, you may want to change this to "bool set".
> ?{
> ? ? ? ?int i;
> ? ? ? ?u32 val;
> @@ -1203,7 +1203,7 @@ static int ssb_wait_bit(struct ssb_devic
> ? ? ? ?for (i = 0; i < timeout; i++) {
> ? ? ? ? ? ? ? ?val = ssb_read32(dev, reg);
> ? ? ? ? ? ? ? ?if (set) {
> - ? ? ? ? ? ? ? ? ? ? ? if (val & bitmask)
> + ? ? ? ? ? ? ? ? ? ? ? if ((val & bitmask) == bitmask)
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?return 0;
> ? ? ? ? ? ? ? ?} else {
> ? ? ? ? ? ? ? ? ? ? ? ?if (!(val & bitmask))
> @@ -1227,8 +1227,8 @@ void ssb_device_disable(struct ssb_devic
>
> ? ? ? ?reject = ssb_tmslow_reject_bitmask(dev);
> ? ? ? ?ssb_write32(dev, SSB_TMSLOW, reject | SSB_TMSLOW_CLOCK);
> - ? ? ? ssb_wait_bit(dev, SSB_TMSLOW, reject, 1000, 1);
> - ? ? ? ssb_wait_bit(dev, SSB_TMSHIGH, SSB_TMSHIGH_BUSY, 1000, 0);
> + ? ? ? ssb_wait_bits(dev, SSB_TMSLOW, reject, 1000, 1);
> + ? ? ? ssb_wait_bits(dev, SSB_TMSHIGH, SSB_TMSHIGH_BUSY, 1000, 0);
> ? ? ? ?ssb_write32(dev, SSB_TMSLOW,
> ? ? ? ? ? ? ? ? ? ?SSB_TMSLOW_FGC | SSB_TMSLOW_CLOCK |
> ? ? ? ? ? ? ? ? ? ?reject | SSB_TMSLOW_RESET |
>
> --
> Greetings Michael.
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
> the body of a message to [email protected]
> More majordomo info at ?http://vger.kernel.org/majordomo-info.html
>
--
Vista: [V]iruses, [I]ntruders, [S]pyware, [T]rojans and [A]dware. :-)
On Tue, 2011-02-15 at 01:03 +0100, Gábor Stefanik wrote:
> > -static int ssb_wait_bit(struct ssb_device *dev, u16 reg, u32 bitmask,
> > - int timeout, int set)
> > +static int ssb_wait_bits(struct ssb_device *dev, u16 reg, u32 bitmask,
> > + int timeout, int set)
>
> While you are at it, you may want to change this to "bool set".
Please feel free to submit a separate patch on top of this.
--
Greetings Michael.
2011/2/15 Michael Büsch <[email protected]>:
> ssb_wait_bit was designed for only one-bit bitmasks.
> People start using it for multi-bit bitmasks. Make the "set" case
> is safe for this. The "unset" case is already safe.
Thanks :)
--
Rafał
W dniu 15 lutego 2011 00:59 użytkownik Michael Büsch <[email protected]> napisał:
> On Tue, 2011-02-15 at 00:44 +0100, Rafał Miłecki wrote:
>> 2011/2/15 Michael Büsch <[email protected]>:
>> > ssb_wait_bit was designed for only one-bit bitmasks.
>> > People start using it for multi-bit bitmasks. Make the "set" case
>> > is safe for this. The "unset" case is already safe.
>>
>> Thanks :)
>
> This should not change the behavior of your patch, though,
> as you only add an "unset" case, which was already correct.
>
> So this doesn't fix an actual bug but rather hardens the code
> for future use.
Yes, I can see that :)
--
Rafał