Find_first_zero_bit considers BITS_PER_LONG bits at a time, and thus may
return a larger number than the maximum position argument if that position
is not a multiple of BITS_PER_LONG.
From: Julia Lawall <[email protected]>
Find_first_zero_bit considers BITS_PER_LONG bits at a time, and thus may
return a larger number than the maximum position argument if that position
is not a multiple of BITS_PER_LONG.
The semantic match that finds this problem is as follows:
(http://coccinelle.lip6.fr/)
// <smpl>
@@
expression e1,e2,e3;
statement S1,S2;
@@
e1 = find_first_zero_bit(e2,e3)
...
if (e1
- ==
+ >=
e3)
S1 else S2
// </smpl>
Signed-off-by: Julia Lawall <[email protected]>
---
drivers/infiniband/hw/qib/qib_file_ops.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff -u -p a/drivers/infiniband/hw/qib/qib_file_ops.c b/drivers/infiniband/hw/qib/qib_file_ops.c
--- a/drivers/infiniband/hw/qib/qib_file_ops.c
+++ b/drivers/infiniband/hw/qib/qib_file_ops.c
@@ -1187,7 +1187,7 @@ static void assign_ctxt_affinity(struct
int cpu;
cpu = find_first_zero_bit(qib_cpulist,
qib_cpulist_count);
- if (cpu == qib_cpulist_count)
+ if (cpu >= qib_cpulist_count)
qib_dev_err(dd,
"no cpus avail for affinity PID %u\n",
current->pid);
From: Julia Lawall <[email protected]>
Find_first_zero_bit considers BITS_PER_LONG bits at a time, and thus may
return a larger number than the maximum position argument if that position
is not a multiple of BITS_PER_LONG.
The semantic match that finds this problem is as follows:
(http://coccinelle.lip6.fr/)
// <smpl>
@@
expression e1,e2,e3;
statement S1,S2;
@@
e1 = find_first_zero_bit(e2,e3)
...
if (e1
- ==
+ >=
e3)
S1 else S2
// </smpl>
Signed-off-by: Julia Lawall <[email protected]>
---
drivers/net/wireless/ath/ath10k/htt_tx.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff -u -p a/drivers/net/wireless/ath/ath10k/htt_tx.c b/drivers/net/wireless/ath/ath10k/htt_tx.c
--- a/drivers/net/wireless/ath/ath10k/htt_tx.c
+++ b/drivers/net/wireless/ath/ath10k/htt_tx.c
@@ -64,7 +64,7 @@ int ath10k_htt_tx_alloc_msdu_id(struct a
msdu_id = find_first_zero_bit(htt->used_msdu_ids,
htt->max_num_pending_tx);
- if (msdu_id == htt->max_num_pending_tx)
+ if (msdu_id >= htt->max_num_pending_tx)
return -ENOBUFS;
ath10k_dbg(ATH10K_DBG_HTT, "htt tx alloc msdu_id %d\n", msdu_id);
From: Julia Lawall <[email protected]>
Find_first_zero_bit considers BITS_PER_LONG bits at a time, and thus may
return a larger number than the maximum position argument if that position
is not a multiple of BITS_PER_LONG.
The semantic match that finds this problem is as follows:
(http://coccinelle.lip6.fr/)
// <smpl>
@@
expression e1,e2,e3;
statement S1,S2;
@@
e1 = find_first_zero_bit(e2,e3)
...
if (e1
- ==
+ >=
e3)
S1 else S2
// </smpl>
Signed-off-by: Julia Lawall <[email protected]>
---
arch/blackfin/kernel/perf_event.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff -u -p a/arch/blackfin/kernel/perf_event.c b/arch/blackfin/kernel/perf_event.c
--- a/arch/blackfin/kernel/perf_event.c
+++ b/arch/blackfin/kernel/perf_event.c
@@ -354,7 +354,7 @@ static int bfin_pmu_add(struct perf_even
if (__test_and_set_bit(idx, cpuc->used_mask)) {
idx = find_first_zero_bit(cpuc->used_mask, MAX_HWEVENTS);
- if (idx == MAX_HWEVENTS)
+ if (idx >= MAX_HWEVENTS)
goto out;
__set_bit(idx, cpuc->used_mask);
From: Julia Lawall <[email protected]>
Find_first_zero_bit considers BITS_PER_LONG bits at a time, and thus may
return a larger number than the maximum position argument if that position
is not a multiple of BITS_PER_LONG.
The semantic match that finds this problem is as follows:
(http://coccinelle.lip6.fr/)
// <smpl>
@@
expression e1,e2,e3;
statement S1,S2;
@@
e1 = find_first_zero_bit(e2,e3)
...
if (e1
- ==
+ >=
e3)
S1 else S2
// </smpl>
Signed-off-by: Julia Lawall <[email protected]>
---
drivers/block/cciss.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff -u -p a/drivers/block/cciss.c b/drivers/block/cciss.c
--- a/drivers/block/cciss.c
+++ b/drivers/block/cciss.c
@@ -980,7 +980,7 @@ static CommandList_struct *cmd_alloc(ctl
do {
i = find_first_zero_bit(h->cmd_pool_bits, h->nr_cmds);
- if (i == h->nr_cmds)
+ if (i >= h->nr_cmds)
return NULL;
} while (test_and_set_bit(i, h->cmd_pool_bits) != 0);
c = h->cmd_pool + i;
From: Julia Lawall <[email protected]>
Find_first_zero_bit considers BITS_PER_LONG bits at a time, and thus may
return a larger number than the maximum position argument if that position
is not a multiple of BITS_PER_LONG.
The semantic match that finds this problem is as follows:
(http://coccinelle.lip6.fr/)
// <smpl>
@@
expression e1,e2,e3;
statement S1,S2;
@@
e1 = find_first_zero_bit(e2,e3)
...
if (e1
- ==
+ >=
e3)
S1 else S2
// </smpl>
Signed-off-by: Julia Lawall <[email protected]>
---
arch/s390/pci/pci.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff -u -p a/arch/s390/pci/pci.c b/arch/s390/pci/pci.c
--- a/arch/s390/pci/pci.c
+++ b/arch/s390/pci/pci.c
@@ -566,7 +566,7 @@ static int zpci_alloc_iomap(struct zpci_
spin_lock(&zpci_iomap_lock);
entry = find_first_zero_bit(zpci_iomap, ZPCI_IOMAP_MAX_ENTRIES);
- if (entry == ZPCI_IOMAP_MAX_ENTRIES) {
+ if (entry >= ZPCI_IOMAP_MAX_ENTRIES) {
spin_unlock(&zpci_iomap_lock);
return -ENOSPC;
}
@@ -746,7 +746,7 @@ static int zpci_alloc_domain(struct zpci
{
spin_lock(&zpci_domain_lock);
zdev->domain = find_first_zero_bit(zpci_domain, ZPCI_NR_DEVICES);
- if (zdev->domain == ZPCI_NR_DEVICES) {
+ if (zdev->domain >= ZPCI_NR_DEVICES) {
spin_unlock(&zpci_domain_lock);
return -ENOSPC;
}
From: Julia Lawall <[email protected]>
Find_first_zero_bit considers BITS_PER_LONG bits at a time, and thus may
return a larger number than the maximum position argument if that position
is not a multiple of BITS_PER_LONG.
The semantic match that finds this problem is as follows:
(http://coccinelle.lip6.fr/)
// <smpl>
@@
expression e1,e2,e3;
statement S1,S2;
@@
e1 = find_first_zero_bit(e2,e3)
...
if (e1
- ==
+ >=
e3)
S1 else S2
// </smpl>
Signed-off-by: Julia Lawall <[email protected]>
---
drivers/scsi/hpsa.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff -u -p a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
--- a/drivers/scsi/hpsa.c
+++ b/drivers/scsi/hpsa.c
@@ -4703,7 +4703,7 @@ static struct CommandList *cmd_alloc(str
spin_lock_irqsave(&h->lock, flags);
do {
i = find_first_zero_bit(h->cmd_pool_bits, h->nr_cmds);
- if (i == h->nr_cmds) {
+ if (i >= h->nr_cmds) {
spin_unlock_irqrestore(&h->lock, flags);
return NULL;
}
From: Julia Lawall <[email protected]>
Find_first_zero_bit considers BITS_PER_LONG bits at a time, and thus may
return a larger number than the maximum position argument if that position
is not a multiple of BITS_PER_LONG.
The semantic match that finds this problem is as follows:
(http://coccinelle.lip6.fr/)
// <smpl>
@@
expression e1,e2,e3;
statement S1,S2;
@@
e1 = find_first_zero_bit(e2,e3)
...
if (e1
- ==
+ >=
e3)
S1 else S2
// </smpl>
Signed-off-by: Julia Lawall <[email protected]>
---
drivers/staging/tidspbridge/rmgr/node.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff -u -p a/drivers/staging/tidspbridge/rmgr/node.c b/drivers/staging/tidspbridge/rmgr/node.c
--- a/drivers/staging/tidspbridge/rmgr/node.c
+++ b/drivers/staging/tidspbridge/rmgr/node.c
@@ -935,7 +935,7 @@ int node_connect(struct node_object *nod
node2_type == NODE_DAISSOCKET)) {
/* Find available pipe */
pipe_id = find_first_zero_bit(hnode_mgr->pipe_map, MAXPIPES);
- if (pipe_id == MAXPIPES) {
+ if (pipe_id >= MAXPIPES) {
status = -ECONNREFUSED;
goto out_unlock;
}
@@ -1008,7 +1008,7 @@ int node_connect(struct node_object *nod
status = -EINVAL;
goto out_unlock;
}
- if (chnl_id == CHNL_MAXCHANNELS) {
+ if (chnl_id >= CHNL_MAXCHANNELS) {
status = -ECONNREFUSED;
goto out_unlock;
}
From: Julia Lawall <[email protected]>
Find_first_zero_bit considers BITS_PER_LONG bits at a time, and thus may
return a larger number than the maximum position argument if that position
is not a multiple of BITS_PER_LONG.
The semantic match that finds this problem is as follows:
(http://coccinelle.lip6.fr/)
// <smpl>
@@
expression e1,e2,e3;
statement S1,S2;
@@
e1 = find_first_zero_bit(e2,e3)
...
if (e1
- ==
+ >=
e3)
S1 else S2
// </smpl>
Signed-off-by: Julia Lawall <[email protected]>
---
arch/arc/kernel/perf_event.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff -u -p a/arch/arc/kernel/perf_event.c b/arch/arc/kernel/perf_event.c
--- a/arch/arc/kernel/perf_event.c
+++ b/arch/arc/kernel/perf_event.c
@@ -205,7 +205,7 @@ static int arc_pmu_add(struct perf_event
if (__test_and_set_bit(idx, arc_pmu->used_mask)) {
idx = find_first_zero_bit(arc_pmu->used_mask,
arc_pmu->n_counters);
- if (idx == arc_pmu->n_counters)
+ if (idx >= arc_pmu->n_counters)
return -EAGAIN;
__set_bit(idx, arc_pmu->used_mask);
From: Julia Lawall <[email protected]>
Find_first_zero_bit considers BITS_PER_LONG bits at a time, and thus may
return a larger number than the maximum position argument if that position
is not a multiple of BITS_PER_LONG.
The semantic match that finds this problem is as follows:
(http://coccinelle.lip6.fr/)
// <smpl>
@@
expression e1,e2,e3;
statement S1,S2;
@@
e1 = find_first_zero_bit(e2,e3)
...
if (e1
- ==
+ >=
e3)
S1 else S2
// </smpl>
Signed-off-by: Julia Lawall <[email protected]>
---
drivers/video/fbdev/sh_mobile_meram.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff -u -p a/drivers/video/fbdev/sh_mobile_meram.c b/drivers/video/fbdev/sh_mobile_meram.c
--- a/drivers/video/fbdev/sh_mobile_meram.c
+++ b/drivers/video/fbdev/sh_mobile_meram.c
@@ -222,7 +222,7 @@ static int meram_plane_alloc(struct sh_m
unsigned long idx;
idx = find_first_zero_bit(&priv->used_icb, 28);
- if (idx == 28)
+ if (idx >= 28)
return -ENOMEM;
plane->cache = &priv->icbs[idx];
From: Julia Lawall <[email protected]>
Find_first_zero_bit considers BITS_PER_LONG bits at a time, and thus may
return a larger number than the maximum position argument if that position
is not a multiple of BITS_PER_LONG.
The semantic match that finds this problem is as follows:
(http://coccinelle.lip6.fr/)
// <smpl>
@@
expression e1,e2,e3;
statement S1,S2;
@@
e1 = find_first_zero_bit(e2,e3)
...
if (e1
- ==
+ >=
e3)
S1 else S2
// </smpl>
Signed-off-by: Julia Lawall <[email protected]>
---
arch/sh/kernel/perf_event.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff -u -p a/arch/sh/kernel/perf_event.c b/arch/sh/kernel/perf_event.c
--- a/arch/sh/kernel/perf_event.c
+++ b/arch/sh/kernel/perf_event.c
@@ -281,7 +281,7 @@ static int sh_pmu_add(struct perf_event
if (__test_and_set_bit(idx, cpuc->used_mask)) {
idx = find_first_zero_bit(cpuc->used_mask, sh_pmu->num_events);
- if (idx == sh_pmu->num_events)
+ if (idx >= sh_pmu->num_events)
goto out;
__set_bit(idx, cpuc->used_mask);
Hi Julia,
On Wed, Jun 4, 2014 at 11:07 AM, Julia Lawall <[email protected]> wrote:
> Find_first_zero_bit considers BITS_PER_LONG bits at a time, and thus may
> return a larger number than the maximum position argument if that position
> is not a multiple of BITS_PER_LONG.
Shouldn't this be fixed in find_first_zero_bit() instead?
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On Wed, 4 Jun 2014, Geert Uytterhoeven wrote:
> Hi Julia,
>
> On Wed, Jun 4, 2014 at 11:07 AM, Julia Lawall <[email protected]> wrote:
> > Find_first_zero_bit considers BITS_PER_LONG bits at a time, and thus may
> > return a larger number than the maximum position argument if that position
> > is not a multiple of BITS_PER_LONG.
>
> Shouldn't this be fixed in find_first_zero_bit() instead?
OK, I could do that as well. Most of the callers currently test with >=.
Should they be left as is, or changed to use ==?
julia
From: Julia Lawall
> On Wed, 4 Jun 2014, Geert Uytterhoeven wrote:
>
> > Hi Julia,
> >
> > On Wed, Jun 4, 2014 at 11:07 AM, Julia Lawall <[email protected]> wrote:
> > > Find_first_zero_bit considers BITS_PER_LONG bits at a time, and thus may
> > > return a larger number than the maximum position argument if that position
> > > is not a multiple of BITS_PER_LONG.
> >
> > Shouldn't this be fixed in find_first_zero_bit() instead?
>
> OK, I could do that as well. Most of the callers currently test with >=.
> Should they be left as is, or changed to use ==?
Do we want to add an extra test to find_first_zero_bit() and effectively
slow down all the calls - especially those where the length is a
multiple of 8 (probably the most common).
Maybe the documented return code should be changed to allow for the
existing behaviour.
David
On Wed, 4 Jun 2014, David Laight wrote:
> From: Julia Lawall
> > On Wed, 4 Jun 2014, Geert Uytterhoeven wrote:
> >
> > > Hi Julia,
> > >
> > > On Wed, Jun 4, 2014 at 11:07 AM, Julia Lawall <[email protected]> wrote:
> > > > Find_first_zero_bit considers BITS_PER_LONG bits at a time, and thus may
> > > > return a larger number than the maximum position argument if that position
> > > > is not a multiple of BITS_PER_LONG.
> > >
> > > Shouldn't this be fixed in find_first_zero_bit() instead?
> >
> > OK, I could do that as well. Most of the callers currently test with >=.
> > Should they be left as is, or changed to use ==?
>
> Do we want to add an extra test to find_first_zero_bit() and effectively
> slow down all the calls - especially those where the length is a
> multiple of 8 (probably the most common).
Currently, most of the calls test with >=, and most of the others seem to
need to (either the size value did not look like a multiple of anything in
particular, or it was eg read from a device).
Note that it is BITS_PER_LONG, so it seems like it is typically 32 or 64,
not 8.
> Maybe the documented return code should be changed to allow for the
> existing behaviour.
Sorry, I'm not sure to understand what you suggest here.
thanks,
julia
Hi Julia,
On Wed, Jun 4, 2014 at 11:52 AM, Julia Lawall <[email protected]> wrote:
>> Maybe the documented return code should be changed to allow for the
>> existing behaviour.
>
> Sorry, I'm not sure to understand what you suggest here.
include/asm-generic/bitops/find.h:
| /**
| * find_first_zero_bit - find the first cleared bit in a memory region
| * @addr: The address to start the search at
| * @size: The maximum number of bits to search
| *
| * Returns the bit number of the first cleared bit.
| * If no bits are zero, returns @size.
"If no bits are zero, returns @size or a number larger than @size."
| */
| extern unsigned long find_first_zero_bit(const unsigned long *addr,
| unsigned long size);
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On Wed, 4 Jun 2014, Geert Uytterhoeven wrote:
> Hi Julia,
>
> On Wed, Jun 4, 2014 at 11:52 AM, Julia Lawall <[email protected]> wrote:
> >> Maybe the documented return code should be changed to allow for the
> >> existing behaviour.
> >
> > Sorry, I'm not sure to understand what you suggest here.
>
> include/asm-generic/bitops/find.h:
>
> | /**
> | * find_first_zero_bit - find the first cleared bit in a memory region
> | * @addr: The address to start the search at
> | * @size: The maximum number of bits to search
> | *
> | * Returns the bit number of the first cleared bit.
> | * If no bits are zero, returns @size.
>
> "If no bits are zero, returns @size or a number larger than @size."
OK, thanks. I was only looking at the C code.
But the C code contains a loop that is followed by:
if (!size)
return result;
tmp = *p;
found_first:
tmp |= ~0UL << size;
if (tmp == ~0UL) /* Are any bits zero? */
return result + size; /* Nope. */
In the first return, it would seem that result == size. Could the second
one be changed to just return size? It should not hurt performance.
julia
>
> | */
> | extern unsigned long find_first_zero_bit(const unsigned long *addr,
> | unsigned long size);
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
> -- Linus Torvalds
>
Hi Julia,
On Wed, Jun 4, 2014 at 1:00 PM, Julia Lawall <[email protected]> wrote:
> OK, thanks. I was only looking at the C code.
>
> But the C code contains a loop that is followed by:
>
> if (!size)
> return result;
> tmp = *p;
>
> found_first:
> tmp |= ~0UL << size;
> if (tmp == ~0UL) /* Are any bits zero? */
> return result + size; /* Nope. */
>
> In the first return, it would seem that result == size. Could the second
> one be changed to just return size? It should not hurt performance.
"size" may have been changed between function entry and this line.
So you have to store it in a temporary.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On Wed, 4 Jun 2014, Julia Lawall wrote:
> From: Julia Lawall <[email protected]>
>
> Find_first_zero_bit considers BITS_PER_LONG bits at a time, and thus may
> return a larger number than the maximum position argument if that position
> is not a multiple of BITS_PER_LONG.
>
> The semantic match that finds this problem is as follows:
> (http://coccinelle.lip6.fr/)
>
> // <smpl>
> @@
> expression e1,e2,e3;
> statement S1,S2;
> @@
>
> e1 = find_first_zero_bit(e2,e3)
> ...
> if (e1
> - ==
> + >=
> e3)
> S1 else S2
> // </smpl>
>
> Signed-off-by: Julia Lawall <[email protected]>
>
> ---
> arch/s390/pci/pci.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff -u -p a/arch/s390/pci/pci.c b/arch/s390/pci/pci.c
> --- a/arch/s390/pci/pci.c
> +++ b/arch/s390/pci/pci.c
> @@ -566,7 +566,7 @@ static int zpci_alloc_iomap(struct zpci_
>
> spin_lock(&zpci_iomap_lock);
> entry = find_first_zero_bit(zpci_iomap, ZPCI_IOMAP_MAX_ENTRIES);
> - if (entry == ZPCI_IOMAP_MAX_ENTRIES) {
> + if (entry >= ZPCI_IOMAP_MAX_ENTRIES) {
> spin_unlock(&zpci_iomap_lock);
> return -ENOSPC;
> }
> @@ -746,7 +746,7 @@ static int zpci_alloc_domain(struct zpci
> {
> spin_lock(&zpci_domain_lock);
> zdev->domain = find_first_zero_bit(zpci_domain, ZPCI_NR_DEVICES);
> - if (zdev->domain == ZPCI_NR_DEVICES) {
> + if (zdev->domain >= ZPCI_NR_DEVICES) {
> spin_unlock(&zpci_domain_lock);
> return -ENOSPC;
> }
>
>
Thanks, applied.
Sebastian
On Wed, 4 Jun 2014, Geert Uytterhoeven wrote:
> Hi Julia,
>
> On Wed, Jun 4, 2014 at 1:00 PM, Julia Lawall <[email protected]> wrote:
> > OK, thanks. I was only looking at the C code.
> >
> > But the C code contains a loop that is followed by:
> >
> > if (!size)
> > return result;
> > tmp = *p;
> >
> > found_first:
> > tmp |= ~0UL << size;
> > if (tmp == ~0UL) /* Are any bits zero? */
> > return result + size; /* Nope. */
> >
> > In the first return, it would seem that result == size. Could the second
> > one be changed to just return size? It should not hurt performance.
>
> "size" may have been changed between function entry and this line.
> So you have to store it in a temporary.
Sorry, after reflection it seems that indeed size + result is always the
original size, so it is actually all of the code that uses >= that is
doing something unnecessary. == for the failure test is fine.
julia
From: Julia Lawall
> On Wed, 4 Jun 2014, Geert Uytterhoeven wrote:
>
> > Hi Julia,
> >
> > On Wed, Jun 4, 2014 at 1:00 PM, Julia Lawall <[email protected]> wrote:
> > > OK, thanks. I was only looking at the C code.
> > >
> > > But the C code contains a loop that is followed by:
> > >
> > > if (!size)
> > > return result;
> > > tmp = *p;
> > >
> > > found_first:
> > > tmp |= ~0UL << size;
> > > if (tmp == ~0UL) /* Are any bits zero? */
> > > return result + size; /* Nope. */
> > >
> > > In the first return, it would seem that result == size. Could the second
> > > one be changed to just return size? It should not hurt performance.
> >
> > "size" may have been changed between function entry and this line.
> > So you have to store it in a temporary.
>
> Sorry, after reflection it seems that indeed size + result is always the
> original size, so it is actually all of the code that uses >= that is
> doing something unnecessary. == for the failure test is fine.
There is nothing wrong with defensive coding.
The 'tmp |= ~0UL << size' ensures that the return value is 'correct'
when there are no bits set.
The function could have been defined so that this wasn't needed.
If you assume that the 'no zero bits' is unlikely, then checking the
return value from ffz() could well be slightly faster.
Not that anything is likely to notice.
David
> Find_first_zero_bit considers BITS_PER_LONG bits at a time, and thus may
> return a larger number than the maximum position argument if that position
> is not a multiple of BITS_PER_LONG.
>
> The semantic match that finds this problem is as follows:
> (http://coccinelle.lip6.fr/)
>
> // <smpl>
> @@
> expression e1,e2,e3;
> statement S1,S2;
> @@
>
> e1 = find_first_zero_bit(e2,e3)
> ...
> if (e1
> - ==
> + >=
> e3)
> S1 else S2
> // </smpl>
>
> Signed-off-by: Julia Lawall <[email protected]>
>
> ---
> drivers/block/cciss.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff -u -p a/drivers/block/cciss.c b/drivers/block/cciss.c
> --- a/drivers/block/cciss.c
> +++ b/drivers/block/cciss.c
> @@ -980,7 +980,7 @@ static CommandList_struct *cmd_alloc(ctl
>
> do {
> i = find_first_zero_bit(h->cmd_pool_bits, h->nr_cmds);
> - if (i == h->nr_cmds)
> + if (i >= h->nr_cmds)
> return NULL;
> } while (test_and_set_bit(i, h->cmd_pool_bits) != 0);
> c = h->cmd_pool + i;
Thanks. Ack.
You can add
Reviewed-by: Stephen M. Cameron <[email protected]>
to this patch if you want.
You might consider adding "Cc: [email protected]" into the
sign-off area as well.
-- steve
On Wed, Jun 04, 2014 at 11:07:56AM +0200, Julia Lawall wrote:
> From: Julia Lawall <[email protected]>
>
> Find_first_zero_bit considers BITS_PER_LONG bits at a time, and thus may
> return a larger number than the maximum position argument if that position
> is not a multiple of BITS_PER_LONG.
>
> The semantic match that finds this problem is as follows:
> (http://coccinelle.lip6.fr/)
>
> // <smpl>
> @@
> expression e1,e2,e3;
> statement S1,S2;
> @@
>
> e1 = find_first_zero_bit(e2,e3)
> ...
> if (e1
> - ==
> + >=
> e3)
> S1 else S2
> // </smpl>
>
> Signed-off-by: Julia Lawall <[email protected]>
>
> ---
> drivers/scsi/hpsa.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff -u -p a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
> --- a/drivers/scsi/hpsa.c
> +++ b/drivers/scsi/hpsa.c
> @@ -4703,7 +4703,7 @@ static struct CommandList *cmd_alloc(str
> spin_lock_irqsave(&h->lock, flags);
> do {
> i = find_first_zero_bit(h->cmd_pool_bits, h->nr_cmds);
> - if (i == h->nr_cmds) {
> + if (i >= h->nr_cmds) {
> spin_unlock_irqrestore(&h->lock, flags);
> return NULL;
> }
Thanks, Ack.
You can add
Reviewed-by: Stephen M. Cameron <[email protected]>
to this patch if you want.
You might also consider adding "Cc: [email protected]" to the sign-off area.
-- steve
On Wed, 4 Jun 2014, [email protected] wrote:
> On Wed, Jun 04, 2014 at 11:07:56AM +0200, Julia Lawall wrote:
> > From: Julia Lawall <[email protected]>
> >
> > Find_first_zero_bit considers BITS_PER_LONG bits at a time, and thus may
> > return a larger number than the maximum position argument if that position
> > is not a multiple of BITS_PER_LONG.
> >
> > The semantic match that finds this problem is as follows:
> > (http://coccinelle.lip6.fr/)
> >
> > // <smpl>
> > @@
> > expression e1,e2,e3;
> > statement S1,S2;
> > @@
> >
> > e1 = find_first_zero_bit(e2,e3)
> > ...
> > if (e1
> > - ==
> > + >=
> > e3)
> > S1 else S2
> > // </smpl>
> >
> > Signed-off-by: Julia Lawall <[email protected]>
> >
> > ---
> > drivers/scsi/hpsa.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff -u -p a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
> > --- a/drivers/scsi/hpsa.c
> > +++ b/drivers/scsi/hpsa.c
> > @@ -4703,7 +4703,7 @@ static struct CommandList *cmd_alloc(str
> > spin_lock_irqsave(&h->lock, flags);
> > do {
> > i = find_first_zero_bit(h->cmd_pool_bits, h->nr_cmds);
> > - if (i == h->nr_cmds) {
> > + if (i >= h->nr_cmds) {
> > spin_unlock_irqrestore(&h->lock, flags);
> > return NULL;
> > }
>
> Thanks, Ack.
>
> You can add
>
> Reviewed-by: Stephen M. Cameron <[email protected]>
>
> to this patch if you want.
>
> You might also consider adding "Cc: [email protected]" to the sign-off area.
Actually, it seems that the function can never overshoot the specified
limit. So the change is not needed.
julia
On Wed, 4 Jun 2014, [email protected] wrote:
> > Find_first_zero_bit considers BITS_PER_LONG bits at a time, and thus may
> > return a larger number than the maximum position argument if that position
> > is not a multiple of BITS_PER_LONG.
> >
> > The semantic match that finds this problem is as follows:
> > (http://coccinelle.lip6.fr/)
> >
> > // <smpl>
> > @@
> > expression e1,e2,e3;
> > statement S1,S2;
> > @@
> >
> > e1 = find_first_zero_bit(e2,e3)
> > ...
> > if (e1
> > - ==
> > + >=
> > e3)
> > S1 else S2
> > // </smpl>
> >
> > Signed-off-by: Julia Lawall <[email protected]>
> >
> > ---
> > drivers/block/cciss.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff -u -p a/drivers/block/cciss.c b/drivers/block/cciss.c
> > --- a/drivers/block/cciss.c
> > +++ b/drivers/block/cciss.c
> > @@ -980,7 +980,7 @@ static CommandList_struct *cmd_alloc(ctl
> >
> > do {
> > i = find_first_zero_bit(h->cmd_pool_bits, h->nr_cmds);
> > - if (i == h->nr_cmds)
> > + if (i >= h->nr_cmds)
> > return NULL;
> > } while (test_and_set_bit(i, h->cmd_pool_bits) != 0);
> > c = h->cmd_pool + i;
>
>
> Thanks. Ack.
>
> You can add
>
> Reviewed-by: Stephen M. Cameron <[email protected]>
>
> to this patch if you want.
>
> You might consider adding "Cc: [email protected]" into the
> sign-off area as well.
Likewise here, the change is not needed.
julia
On Wed, Jun 04, 2014 at 05:06:44PM +0200, Julia Lawall wrote:
>
>
> On Wed, 4 Jun 2014, [email protected] wrote:
>
> > On Wed, Jun 04, 2014 at 11:07:56AM +0200, Julia Lawall wrote:
> > > From: Julia Lawall <[email protected]>
> > >
> > > Find_first_zero_bit considers BITS_PER_LONG bits at a time, and thus may
> > > return a larger number than the maximum position argument if that position
> > > is not a multiple of BITS_PER_LONG.
> > >
> > > The semantic match that finds this problem is as follows:
> > > (http://coccinelle.lip6.fr/)
> > >
> > > // <smpl>
> > > @@
> > > expression e1,e2,e3;
> > > statement S1,S2;
> > > @@
> > >
> > > e1 = find_first_zero_bit(e2,e3)
> > > ...
> > > if (e1
> > > - ==
> > > + >=
> > > e3)
> > > S1 else S2
> > > // </smpl>
> > >
> > > Signed-off-by: Julia Lawall <[email protected]>
> > >
> > > ---
> > > drivers/scsi/hpsa.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff -u -p a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
> > > --- a/drivers/scsi/hpsa.c
> > > +++ b/drivers/scsi/hpsa.c
> > > @@ -4703,7 +4703,7 @@ static struct CommandList *cmd_alloc(str
> > > spin_lock_irqsave(&h->lock, flags);
> > > do {
> > > i = find_first_zero_bit(h->cmd_pool_bits, h->nr_cmds);
> > > - if (i == h->nr_cmds) {
> > > + if (i >= h->nr_cmds) {
> > > spin_unlock_irqrestore(&h->lock, flags);
> > > return NULL;
> > > }
> >
> > Thanks, Ack.
> >
> > You can add
> >
> > Reviewed-by: Stephen M. Cameron <[email protected]>
> >
> > to this patch if you want.
> >
> > You might also consider adding "Cc: [email protected]" to the sign-off area.
>
> Actually, it seems that the function can never overshoot the specified
> limit. So the change is not needed.
Well, that would explain why nobody has complained about it in all these years.
I figured that must have been the case at least on x86, but I thought maybe
other archictectures might behave differently, and the change seemed harmless
in any case.
-- steve
On 06/04/2014 08:51 AM, [email protected] wrote:
>> Find_first_zero_bit considers BITS_PER_LONG bits at a time, and thus may
>> return a larger number than the maximum position argument if that position
>> is not a multiple of BITS_PER_LONG.
>>
>> The semantic match that finds this problem is as follows:
>> (http://coccinelle.lip6.fr/)
>>
>> // <smpl>
>> @@
>> expression e1,e2,e3;
>> statement S1,S2;
>> @@
>>
>> e1 = find_first_zero_bit(e2,e3)
>> ...
>> if (e1
>> - ==
>> + >=
>> e3)
>> S1 else S2
>> // </smpl>
>>
>> Signed-off-by: Julia Lawall <[email protected]>
>>
>> ---
>> drivers/block/cciss.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff -u -p a/drivers/block/cciss.c b/drivers/block/cciss.c
>> --- a/drivers/block/cciss.c
>> +++ b/drivers/block/cciss.c
>> @@ -980,7 +980,7 @@ static CommandList_struct *cmd_alloc(ctl
>>
>> do {
>> i = find_first_zero_bit(h->cmd_pool_bits, h->nr_cmds);
>> - if (i == h->nr_cmds)
>> + if (i >= h->nr_cmds)
>> return NULL;
>> } while (test_and_set_bit(i, h->cmd_pool_bits) != 0);
>> c = h->cmd_pool + i;
>
>
> Thanks. Ack.
>
> You can add
>
> Reviewed-by: Stephen M. Cameron <[email protected]>
>
> to this patch if you want.
>
> You might consider adding "Cc: [email protected]" into the
> sign-off area as well.
There are two such instances in cciss.c, btw.
--
Jens Axboe
On Wed, 4 Jun 2014, Jens Axboe wrote:
> On 06/04/2014 08:51 AM, [email protected] wrote:
> >> Find_first_zero_bit considers BITS_PER_LONG bits at a time, and thus may
> >> return a larger number than the maximum position argument if that position
> >> is not a multiple of BITS_PER_LONG.
> >>
> >> The semantic match that finds this problem is as follows:
> >> (http://coccinelle.lip6.fr/)
> >>
> >> // <smpl>
> >> @@
> >> expression e1,e2,e3;
> >> statement S1,S2;
> >> @@
> >>
> >> e1 = find_first_zero_bit(e2,e3)
> >> ...
> >> if (e1
> >> - ==
> >> + >=
> >> e3)
> >> S1 else S2
> >> // </smpl>
> >>
> >> Signed-off-by: Julia Lawall <[email protected]>
> >>
> >> ---
> >> drivers/block/cciss.c | 2 +-
> >> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff -u -p a/drivers/block/cciss.c b/drivers/block/cciss.c
> >> --- a/drivers/block/cciss.c
> >> +++ b/drivers/block/cciss.c
> >> @@ -980,7 +980,7 @@ static CommandList_struct *cmd_alloc(ctl
> >>
> >> do {
> >> i = find_first_zero_bit(h->cmd_pool_bits, h->nr_cmds);
> >> - if (i == h->nr_cmds)
> >> + if (i >= h->nr_cmds)
> >> return NULL;
> >> } while (test_and_set_bit(i, h->cmd_pool_bits) != 0);
> >> c = h->cmd_pool + i;
> >
> >
> > Thanks. Ack.
> >
> > You can add
> >
> > Reviewed-by: Stephen M. Cameron <[email protected]>
> >
> > to this patch if you want.
> >
> > You might consider adding "Cc: [email protected]" into the
> > sign-off area as well.
>
> There are two such instances in cciss.c, btw.
Actually, there seem to be three, and I didn't find the other two because
the assignment is inlined into the test. But the patch isn't needed
anyway, because it turns out that the result never goes over the bound
value.
thanks,
julia
On 06/04/2014 10:59 AM, Julia Lawall wrote:
>
>
> On Wed, 4 Jun 2014, Jens Axboe wrote:
>
>> On 06/04/2014 08:51 AM, [email protected] wrote:
>>>> Find_first_zero_bit considers BITS_PER_LONG bits at a time, and thus may
>>>> return a larger number than the maximum position argument if that position
>>>> is not a multiple of BITS_PER_LONG.
>>>>
>>>> The semantic match that finds this problem is as follows:
>>>> (http://coccinelle.lip6.fr/)
>>>>
>>>> // <smpl>
>>>> @@
>>>> expression e1,e2,e3;
>>>> statement S1,S2;
>>>> @@
>>>>
>>>> e1 = find_first_zero_bit(e2,e3)
>>>> ...
>>>> if (e1
>>>> - ==
>>>> + >=
>>>> e3)
>>>> S1 else S2
>>>> // </smpl>
>>>>
>>>> Signed-off-by: Julia Lawall <[email protected]>
>>>>
>>>> ---
>>>> drivers/block/cciss.c | 2 +-
>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff -u -p a/drivers/block/cciss.c b/drivers/block/cciss.c
>>>> --- a/drivers/block/cciss.c
>>>> +++ b/drivers/block/cciss.c
>>>> @@ -980,7 +980,7 @@ static CommandList_struct *cmd_alloc(ctl
>>>>
>>>> do {
>>>> i = find_first_zero_bit(h->cmd_pool_bits, h->nr_cmds);
>>>> - if (i == h->nr_cmds)
>>>> + if (i >= h->nr_cmds)
>>>> return NULL;
>>>> } while (test_and_set_bit(i, h->cmd_pool_bits) != 0);
>>>> c = h->cmd_pool + i;
>>>
>>>
>>> Thanks. Ack.
>>>
>>> You can add
>>>
>>> Reviewed-by: Stephen M. Cameron <[email protected]>
>>>
>>> to this patch if you want.
>>>
>>> You might consider adding "Cc: [email protected]" into the
>>> sign-off area as well.
>>
>> There are two such instances in cciss.c, btw.
>
> Actually, there seem to be three, and I didn't find the other two because
> the assignment is inlined into the test. But the patch isn't needed
> anyway, because it turns out that the result never goes over the bound
> value.
I have always defensively programmed it, but it would make a shitty API
if it did.
--
Jens Axboe
> Subject: [PATCH 9/10] IB/qib: use safer test on the result of find_first_zero_bit
>
> From: Julia Lawall <[email protected]>
Thanks for the patch!
Roland, I'm marking this as stable since a memory corruption can occur in the _set_bit().
Cc: <[email protected]>
Acked-by: Mike Marciniszyn <[email protected]>
On Wed, 4 Jun 2014, Marciniszyn, Mike wrote:
> > Subject: [PATCH 9/10] IB/qib: use safer test on the result of find_first_zero_bit
> >
> > From: Julia Lawall <[email protected]>
>
> Thanks for the patch!
>
> Roland, I'm marking this as stable since a memory corruption can occur in the _set_bit().
No, it's not necessary. It turns out that the result cannot be greater
than the requested maximum value.
julia
>
> No, it's not necessary. It turns out that the result cannot be greater than the
> requested maximum value.
>
> Julia
Ok.
No stable then.
Mike
On 04/06/14 12:07, Julia Lawall wrote:
> From: Julia Lawall <[email protected]>
>
> Find_first_zero_bit considers BITS_PER_LONG bits at a time, and thus may
> return a larger number than the maximum position argument if that position
> is not a multiple of BITS_PER_LONG.
Thanks, queued for 3.17.
Tomi
On Mon, 23 Jun 2014, Tomi Valkeinen wrote:
> On 04/06/14 12:07, Julia Lawall wrote:
> > From: Julia Lawall <[email protected]>
> >
> > Find_first_zero_bit considers BITS_PER_LONG bits at a time, and thus may
> > return a larger number than the maximum position argument if that position
> > is not a multiple of BITS_PER_LONG.
>
> Thanks, queued for 3.17.
No, sorry you can drop it. It doesn't hurt anything, but it is not
necessary either. Returning a larger number is actually not possible.
julia
On 23/06/14 17:12, Julia Lawall wrote:
> On Mon, 23 Jun 2014, Tomi Valkeinen wrote:
>
>> On 04/06/14 12:07, Julia Lawall wrote:
>>> From: Julia Lawall <[email protected]>
>>>
>>> Find_first_zero_bit considers BITS_PER_LONG bits at a time, and thus may
>>> return a larger number than the maximum position argument if that position
>>> is not a multiple of BITS_PER_LONG.
>>
>> Thanks, queued for 3.17.
>
> No, sorry you can drop it. It doesn't hurt anything, but it is not
> necessary either. Returning a larger number is actually not possible.
Ok, dropped.
Tomi