Re: kernel BUG at ide-cd.c:1726 in 2.6.24-03863-g0ba6c33 && -g8561b089
> Could you try the patch below and give me all boot messages again? Sure, no problem, see below for full log (I updated to the latest git, which seems to have some other unrelated problems with things timing out earlier in the boot, but it does get to the ide-cd init); here's the relevant looking debug info: [ 163.990058] ide-cd: rq still having bio: dev hda: type=2, flags=114c8 [ 163.990060] [ 163.990061] sector 1987647, nr/cnr 0/0 [ 163.990063] bio 8101c418c200, biotail 8101c418c200, buffer , data , len 158 [ 163.990064] cdb: 12 00 00 00 fe 00 00 00 00 00 00 00 00 00 00 00 [ 163.990069] backup: data_len=158 bi_size=158 [ 163.994021] ide-cd: rq still having bio: dev hda: type=2, flags=114c8 [ 163.994023] [ 163.994024] sector 1987647, nr/cnr 0/0 [ 163.994025] bio 8101c418c200, biotail 8101c418c200, buffer , data , len 158 [ 163.994027] cdb: 12 01 00 00 fe 00 00 00 00 00 00 00 00 00 00 00 [ 163.994032] backup: data_len=158 bi_size=158 > This patch displays debug messages against requests still having bio, > then tries to unlink all bios from the rq before the rq is completed. > So your system may be able to continue to work correctly > after displaying debug messages. By the way, even with your first debug patch, I had no problem using the box once it booted. I didn't try the CD-ROM but everything else seemed fine. Also, I saw the same issue on a different box (Dell SC1435 with serverworks IDE): [1.367648] Uniform Multi-Platform E-IDE driver Revision: 7.00alpha2 [1.371649] ide: Assuming 33MHz system bus speed for PIO modes; override with idebus=xx [2.612417] SvrWks HT1000: IDE controller (0x1166:0x0214 rev 0x00) at PCI slot :00:02.1 [2.612439] SvrWks HT1000: not 100% native mode: will probe irqs later [2.616427] ide0: BM-DMA at 0x08c0-0x08c7, BIOS settings: hda:DMA, hdb:pio [2.623505] Probing IDE interface ide0... [4.037070] hda: PHILIPS DVD-ROM SDR089, ATAPI CD/DVD-ROM drive [4.041801] hda: host max PIO4 wanted PIO255(auto-tune) selected PIO4 [4.058447] hda: UDMA/33 mode selected [4.061278] ide0 at 0x1f0-0x1f7,0x3f6 on irq 14 [4.632739] hda: ATAPI 24X DVD-ROM drive, 2048kB Cache ... [8.631615] ide-cd: rq still having bio: dev hda: type=2, flags=114c8 [8.631615] [8.631615] sector 2867855, nr/cnr 0/0 [8.631615] bio 81021fba3f40, biotail 81021fba3f40, buffer , data , len 158 [8.631615] cdb: 12 00 00 00 fe 00 00 00 00 00 00 00 00 00 00 00 [8.660505] ide-cd: rq still having bio: dev hda: type=2, flags=114c8 [8.660505] [8.660505] sector 2867855, nr/cnr 0/0 [8.660505] bio 81021fba3f40, biotail 81021fba3f40, buffer , data , len 62 [8.660505] cdb: 12 00 00 00 fe 00 00 00 00 00 00 00 00 00 00 00 Let me know if you'd like debug info from that too. [0.00] Linux version 2.6.24 ([EMAIL PROTECTED]) (gcc version 4.2.3 20080114 (prerelease) (Debian 4.2.2-7)) #5 SMP Wed Jan 30 18:25:58 PST 2008 [0.00] Command line: root=/dev/sda1 ro console=ttyS0,38400n8 [0.00] BIOS-provided physical RAM map: [0.00] BIOS-e820: - 0009fc00 (usable) [0.00] BIOS-e820: 0009fc00 - 000a (reserved) [0.00] BIOS-e820: 000e8000 - 0010 (reserved) [0.00] BIOS-e820: 0010 - 9fff (usable) [0.00] BIOS-e820: 9fff - 9fffe000 (ACPI data) [0.00] BIOS-e820: 9fffe000 - a000 (ACPI NVS) [0.00] BIOS-e820: fec0 - fec01000 (reserved) [0.00] BIOS-e820: fee0 - fee01000 (reserved) [0.00] BIOS-e820: ff70 - 0001 (reserved) [0.00] BIOS-e820: 0001 - 00024500 (usable) [0.00] Entering add_active_range(0, 0, 159) 0 entries of 3200 used [0.00] Entering add_active_range(0, 256, 655344) 1 entries of 3200 used [0.00] Entering add_active_range(0, 1048576, 2379776) 2 entries of 3200 used [0.00] end_pfn_map = 2379776 [0.00] DMI 2.3 present. [0.00] ACPI: RSDP 000FA190, 0024 (r2 ACPIAM) [0.00] ACPI: XSDT 9FFF0100, 0044 (r1 A M I OEMXSDT 12000505 MSFT 97) [0.00] ACPI: FACP 9FFF0290, 00F4 (r3 A M I OEMFACP 12000505 MSFT 97) [0.00] ACPI Warning (tbfadt-0442): Optional field "Gpe1Block" has zero address or length: 44A0/0 [20070126] [0.00] ACPI: DSDT 9FFF0440, 5544 (r1 1HQC8 1HQC80033 INTL 2002026) [0.00] ACPI: FACS 9FFFE000, 0040 [0.00] ACPI: APIC 9FFF0390, 00A4 (r1 A M I OEMAPIC 12000505 MSFT 97)
Re: [git patches 1/2] warnings: attack valid cases spotted by warnings
> Hey, I appreciate it, but I really do have to warn you that I did this all
> blind, and just meant for it to be a "I think this kind of direction is
> more productive" thing. I'm not going to guarantee that it works at all.
Oh, understood, and I'm definitely planning on taking your patch as
inspiration and a kick in the pants to finally clean up the code
rather than trying to use it as is. Obviously it was a quick hack
from someone that doesn't know the hardware -- for example
handle_raddr_seg() is at least misnamed, since UD work requests don't
have raddr segments.
But if you're going to take the time to work on the code, then I feel
like it's only polite not to drop your work on the floor. Just like I
hope someone who works on IB and who is usually sending me stuff to
merge would give a patch from me some attention.
BTW, I noticed one interesting thing while starting on this cleanup.
I wanted to make sure that the generated code didn't change with the
first step, and I actually discovered that the patch below seems to
make the generated code *better*, maybe because some gcc alias
analysis doesn't get as paranoid without the void *:
add/remove: 0/0 grow/shrink: 0/7 up/down: 0/-85 (-85)
function old new delta
mthca_arbel_post_srq_recv373 369 -4
mthca_arbel_post_receive 570 562 -8
mthca_tavor_post_srq_recv520 508 -12
mthca_tavor_post_send 13441330 -14
mthca_arbel_post_send 14811467 -14
mlx4_ib_post_send 15981582 -16
mthca_tavor_post_receive 792 775 -17
And here's the patch itself -- I think this is a reasonable size step
to break things up into. I assume that this is the basic form of the
cleanup that you're proposing?
diff --git a/drivers/infiniband/hw/mlx4/qp.c b/drivers/infiniband/hw/mlx4/qp.c
index 8d09aa3..a59c7f0 100644
--- a/drivers/infiniband/hw/mlx4/qp.c
+++ b/drivers/infiniband/hw/mlx4/qp.c
@@ -1183,6 +1183,14 @@ static int mlx4_wq_overflow(struct mlx4_ib_wq *wq, int
nreq, struct ib_cq *ib_cq
return cur + nreq >= wq->max_post;
}
+static void set_data_seg(struct mlx4_wqe_data_seg *dseg,
+struct ib_sge *sg)
+{
+ dseg->byte_count = cpu_to_be32(sg->length);
+ dseg->lkey = cpu_to_be32(sg->lkey);
+ dseg->addr = cpu_to_be64(sg->addr);
+}
+
int mlx4_ib_post_send(struct ib_qp *ibqp, struct ib_send_wr *wr,
struct ib_send_wr **bad_wr)
{
@@ -1313,12 +1321,7 @@ int mlx4_ib_post_send(struct ib_qp *ibqp, struct
ib_send_wr *wr,
}
for (i = 0; i < wr->num_sge; ++i) {
- ((struct mlx4_wqe_data_seg *) wqe)->byte_count =
- cpu_to_be32(wr->sg_list[i].length);
- ((struct mlx4_wqe_data_seg *) wqe)->lkey =
- cpu_to_be32(wr->sg_list[i].lkey);
- ((struct mlx4_wqe_data_seg *) wqe)->addr =
- cpu_to_be64(wr->sg_list[i].addr);
+ set_data_seg(wqe, wr->sg_list + i);
wqe += sizeof (struct mlx4_wqe_data_seg);
size += sizeof (struct mlx4_wqe_data_seg) / 16;
diff --git a/drivers/infiniband/hw/mthca/mthca_qp.c
b/drivers/infiniband/hw/mthca/mthca_qp.c
index 0e9ef24..2548250 100644
--- a/drivers/infiniband/hw/mthca/mthca_qp.c
+++ b/drivers/infiniband/hw/mthca/mthca_qp.c
@@ -1740,13 +1740,8 @@ int mthca_tavor_post_send(struct ib_qp *ibqp, struct
ib_send_wr *wr,
}
for (i = 0; i < wr->num_sge; ++i) {
- ((struct mthca_data_seg *) wqe)->byte_count =
- cpu_to_be32(wr->sg_list[i].length);
- ((struct mthca_data_seg *) wqe)->lkey =
- cpu_to_be32(wr->sg_list[i].lkey);
- ((struct mthca_data_seg *) wqe)->addr =
- cpu_to_be64(wr->sg_list[i].addr);
- wqe += sizeof (struct mthca_data_seg);
+ mthca_set_data_seg(wqe, wr->sg_list + i);
+ wqe += sizeof (struct mthca_data_seg);
size += sizeof (struct mthca_data_seg) / 16;
}
@@ -1869,13 +1864,8 @@ int mthca_tavor_post_receive(struct ib_qp *ibqp, struct
ib_recv_wr *wr,
}
for (i = 0; i < wr->num_sge; ++i) {
- ((struct mthca_data_seg *) wqe)->byte_count =
- cpu_to_be32(wr->sg_list[i].length);
- ((struct mthca_data_seg *) wqe)->lkey =
- cpu_to_be32(wr->sg_list[i].lkey);
- ((struct mthca_data_seg *) wqe)->addr =
- cpu_to_b
Re: [git patches 1/2] warnings: attack valid cases spotted by warnings
> Quite frankly, I don't quite understand where you get those enormous balls > you have, that you can then talk about how ugly it is to just add a "= 0" > that shuts up a compiler warning. That's the _least_ ugly part of the > whole damn function! The clanking when I walk annoys people in the office too... But you're right. It is stupid of me to make such a big deal about this. My excuse is that I've seen those warnings so many times and actually given them more thought than they deserve, and I really felt that Jeff's change makes the admittedly already ugly code a tiny little bit worse. > Anyway, here's a totally untested cleanup that compiles but probably > doesn't work, because I didn't check that I did the right thing with all > the pointer arithmetic (ie when I change "wqe" to a real structure pointer > instead of just a "void *", maybe I left some pointer arithmetic around > that expected it to work as a byte pointer, but now really works on the > whole structure size instead). Given that you took the time to do this, I'll get the patch into a working state and apply it. And maybe split it into reviewable chunks while I'm at it ;) Thanks, Roland - To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [git patches 1/2] warnings: attack valid cases spotted by warnings
> > So setting a variable to something meaningless (guaranteeing that a
> > garbage value is used in case of a bug) just to shut up a warning makes
> > no sense -- it's no safer than leaving the code as is.
>
> Wrong.
>
> It's safer for two reasons:
> - now everybody will see the *same* behaviour
> - the "meaningless value" is guaranteed to not be a security leak
>
> but the whole "shut up bogus warnings" is the best reason.
>
> So it *is* safer than leaving the code as-is.
OK, fair enough. What I said wasn't quite right, but in my case I
think neither of your reasons really applies, since the uninitialized
variable would be written into some hardware control block, so the
effect would probably still be random even if the value is the same
and the information leak doesn't really matter.
Anyway, I think that in this case it's not too hard to show that the
variable really can't be used uninitialized, so I prefer the smaller
generated code from uninitialized_var() (plus a comment explaining why
that's safe).
> Of course, usually the best approach is to rewrite the code to be simpler,
> so that even gcc sees that something is obviously initialized. Sadly,
> people seldom do the right thing, and sometimes gcc just blows incredibly
> hard.
In this case the code is basically
u32 x;
for (n = 0; cond; ++n) {
...
if (!n)
x = something;
...
}
if (n) {
...
use(x);
...
}
and gcc still warns...
- R.
-
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [git patches 1/2] warnings: attack valid cases spotted by warnings
I think this patch (on top of the previous one) actually makes the
code clearer, and also makes it smaller:
add/remove: 0/0 grow/shrink: 0/3 up/down: 0/-41 (-41)
function old new delta
mthca_tavor_post_send 13441335 -9
mthca_tavor_post_receive 792 776 -16
mthca_arbel_post_send 14811465 -16
So I'll test this a bit and probably merge it.
---
diff --git a/drivers/infiniband/hw/mthca/mthca_qp.c
b/drivers/infiniband/hw/mthca/mthca_qp.c
index 0e9ef24..186bade 100644
--- a/drivers/infiniband/hw/mthca/mthca_qp.c
+++ b/drivers/infiniband/hw/mthca/mthca_qp.c
@@ -1590,13 +1590,14 @@ int mthca_tavor_post_send(struct ib_qp *ibqp, struct
ib_send_wr *wr,
int nreq;
int i;
int size;
- int size0 = 0;
/*
-* f0 is only used if nreq != 0, and f0 will be initialized
-* the first time through the main loop, since size0 == 0 the
-* first time through. So nreq cannot become non-zero without
-* initializing f0, and f0 is in fact never used uninitialized.
+* f0 and size0 are only used if nreq != 0, and they will
+* always be initialized the first time through the main loop
+* before nreq is incremented. So nreq cannot become non-zero
+* without initializing f0 and size0, and they are in fact
+* never used uninitialized.
*/
+ int uninitialized_var(size0);
u32 uninitialized_var(f0);
int ind;
u8 op0 = 0;
@@ -1774,11 +1775,11 @@ int mthca_tavor_post_send(struct ib_qp *ibqp, struct
ib_send_wr *wr,
mthca_opcode[wr->opcode]);
wmb();
((struct mthca_next_seg *) prev_wqe)->ee_nds =
- cpu_to_be32((size0 ? 0 : MTHCA_NEXT_DBD) | size |
+ cpu_to_be32((nreq ? 0 : MTHCA_NEXT_DBD) | size |
((wr->send_flags & IB_SEND_FENCE) ?
MTHCA_NEXT_FENCE : 0));
- if (!size0) {
+ if (!nreq) {
size0 = size;
op0 = mthca_opcode[wr->opcode];
f0= wr->send_flags & IB_SEND_FENCE ?
@@ -1828,7 +1829,14 @@ int mthca_tavor_post_receive(struct ib_qp *ibqp, struct
ib_recv_wr *wr,
int nreq;
int i;
int size;
- int size0 = 0;
+ /*
+* size0 is only used if nreq != 0, and it will always be
+* initialized the first time through the main loop before
+* nreq is incremented. So nreq cannot become non-zero
+* without initializing size0, and it is in fact never used
+* uninitialized.
+*/
+ int uninitialized_var(size0);
int ind;
void *wqe;
void *prev_wqe;
@@ -1887,7 +1895,7 @@ int mthca_tavor_post_receive(struct ib_qp *ibqp, struct
ib_recv_wr *wr,
((struct mthca_next_seg *) prev_wqe)->ee_nds =
cpu_to_be32(MTHCA_NEXT_DBD | size);
- if (!size0)
+ if (!nreq)
size0 = size;
++ind;
@@ -1909,7 +1917,6 @@ int mthca_tavor_post_receive(struct ib_qp *ibqp, struct
ib_recv_wr *wr,
qp->rq.next_ind = ind;
qp->rq.head += MTHCA_TAVOR_MAX_WQES_PER_RECV_DB;
- size0 = 0;
}
}
@@ -1951,13 +1958,14 @@ int mthca_arbel_post_send(struct ib_qp *ibqp, struct
ib_send_wr *wr,
int nreq;
int i;
int size;
- int size0 = 0;
/*
-* f0 is only used if nreq != 0, and f0 will be initialized
-* the first time through the main loop, since size0 == 0 the
-* first time through. So nreq cannot become non-zero without
-* initializing f0, and f0 is in fact never used uninitialized.
+* f0 and size0 are only used if nreq != 0, and they will
+* always be initialized the first time through the main loop
+* before nreq is incremented. So nreq cannot become non-zero
+* without initializing f0 and size0, and they are in fact
+* never used uninitialized.
*/
+ int uninitialized_var(size0);
u32 uninitialized_var(f0);
int ind;
u8 op0 = 0;
@@ -1978,7 +1986,6 @@ int mthca_arbel_post_send(struct ib_qp *ibqp, struct
ib_send_wr *wr,
doorbell[1] = cpu_to_be32((qp->qpn << 8) | size0);
qp->sq.head += MTHCA_ARBEL_MAX_WQES_PER_SEND_DB;
- size0 = 0;
/*
* Make sure that descriptors are written before
@@ -2163,7 +2170,7 @@ int mthca_arbel_post_send(struct ib_qp *ibqp, struct
ib_send_wr *wr,
((wr->send_flags & IB_SEND_FENCE) ?
Re: [git patches 1/2] warnings: attack valid cases spotted by warnings
> I don't buy that performance argument, in this case. You are already
> dirtying the same cacheline with other variable initializations.
>
> Like I noted in the changeset description (hey, this is precisely why
> I included it, so that we could have this discussion), IMO the flow of
> control makes it not only impossible for the compiler to understand
> the full value range of 'f0', but also difficult for humans as well.
>
> I could perhaps understand initializing the variable to some poison
> value rather than zero, but IMO the code is stronger with f0 set to a
> sane value.
The more I think about it, the less sense initializing f0 to 0 makes.
The whole problem with an uninitialized variable is that a random
value from the stack might be used. So setting a variable to
something meaningless (guaranteeing that a garbage value is used in
case of a bug) just to shut up a warning makes no sense -- it's no
safer than leaving the code as is. uninitialized_var() gets rid of
the warning, saves a little text and instruction cache, and documents
things better.
(BTW, I agree the code is a little confusing as written. I think
things could be cleaned up and made more efficient by getting rid of
the initialization of size0 too -- I'll look at doing that)
Anyway, I queued this up for my next merge with Linus:
commit 6d7d080e9f7cd535a8821efd3835c5cfa5223ab6
Author: Roland Dreier <[EMAIL PROTECTED]>
Date: Tue Jul 17 19:30:51 2007 -0700
IB/mthca: Use uninitialized_var() for f0
Commit 9db48926 ("drivers/infiniband/hw/mthca/mthca_qp: kill uninit'd
var warning") added "= 0" to the declarations of f0 to shut up gcc
warnings. However, there's no point in making the code bigger by
initializing f0 to a random value just to get rid of a warning;
setting f0 to 0 is no safer than just using uninitialized_var(), which
documents the situation better and gives smaller code too. For example,
on x86_64:
add/remove: 0/0 grow/shrink: 0/2 up/down: 0/-16 (-16)
function old new delta
mthca_tavor_post_send 13521344 -8
mthca_arbel_post_send 14891481 -8
Signed-off-by: Roland Dreier <[EMAIL PROTECTED]>
diff --git a/drivers/infiniband/hw/mthca/mthca_qp.c
b/drivers/infiniband/hw/mthca/mthca_qp.c
index 11f1d99..0e9ef24 100644
--- a/drivers/infiniband/hw/mthca/mthca_qp.c
+++ b/drivers/infiniband/hw/mthca/mthca_qp.c
@@ -1591,7 +1591,13 @@ int mthca_tavor_post_send(struct ib_qp *ibqp, struct
ib_send_wr *wr,
int i;
int size;
int size0 = 0;
- u32 f0 = 0;
+ /*
+* f0 is only used if nreq != 0, and f0 will be initialized
+* the first time through the main loop, since size0 == 0 the
+* first time through. So nreq cannot become non-zero without
+* initializing f0, and f0 is in fact never used uninitialized.
+*/
+ u32 uninitialized_var(f0);
int ind;
u8 op0 = 0;
@@ -1946,7 +1952,13 @@ int mthca_arbel_post_send(struct ib_qp *ibqp, struct
ib_send_wr *wr,
int i;
int size;
int size0 = 0;
- u32 f0 = 0;
+ /*
+* f0 is only used if nreq != 0, and f0 will be initialized
+* the first time through the main loop, since size0 == 0 the
+* first time through. So nreq cannot become non-zero without
+* initializing f0, and f0 is in fact never used uninitialized.
+*/
+ u32 uninitialized_var(f0);
int ind;
u8 op0 = 0;
-
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [git patches 1/2] warnings: attack valid cases spotted by warnings
> drivers/infiniband/hw/mthca/mthca_qp: kill uninit'd var warning > > drivers/infiniband/hw/mthca/mthca_qp.c: In function > âmthca_tavor_post_sendâ: > drivers/infiniband/hw/mthca/mthca_qp.c:1594: warning: âf0â may be > used > uninitialized in this function > drivers/infiniband/hw/mthca/mthca_qp.c: In function > âmthca_arbel_post_sendâ: > drivers/infiniband/hw/mthca/mthca_qp.c:1949: warning: âf0â may be > used > uninitialized in this function > > Initializing 'f0' is not strictly necessary in either case, AFAICS. > > I was considering use of uninitialized_var(), but looking at the > complex flow of control in each function, I feel it is wiser and > safer to simply zero the var and be certain of ourselves. > > Signed-off-by: Jeff Garzik <[EMAIL PROTECTED]> I don't really like this. These functions are in the hottest, most latency-sensitive code path of this driver, which is used by people who care about nanoseconds. I'm quite confident that the code is correct as written, and it really feels wrong to me to add bloat to the fastpath just to cover up a shortcoming of gcc. And I don't like using uninitialized_var() here for a similar reason... the functions are complex and I would prefer to avoid hiding future bugs that may creep in. In fact adding the initialization to 0 has a similar effect, since it shuts up the compiler even if the logic in the function gets screwed up. On the other hand I definitely sympathize with the desire to have a warning-free build so that real warnings are more visible. I guess I could live with the uninitialized_var() patch, although I would feel a little sad. - R. - To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Another libata TODO item
Jeff> Look at net drivers. Theres no real infrastructure beyond Jeff> bit tests and printks. I wouldn't call that a subsystem, Jeff> so, I wouldn't call this one such either. Well, scsi_logging.h isn't much of a subsystem either. - R. - To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Another libata TODO item
Jeff> In any case, I also contine to be skeptical of in-kernel Jeff> logging subsystems. Aren't you proposing a libata logging subsystem? - R. - To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [BK PATCHES] 2.6.x libata fixes (mostly)
Prakash> If I am not totally mistaken this is not gcc4 friendly Prakash> code. (lvalue thing...) Actually you misread the code slightly. It's a little subtle, but code like *(__le32 *)prd = cpu_to_le32(len); is not using a cast as an lvalue. It's dereferencing a cast and as such is totally correct, idiomatic and clean C. - Roland - To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
