| Message ID | 20240820030407.627785-1-mpe@ellerman.id.au (mailing list archive) |
|---|---|
| State | Handled Elsewhere, archived |
| Headers | show |
| Series | [v2] ata: pata_macio: Use WARN instead of BUG | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/github-powerpc_sparse | success | Successfully ran 4 jobs. |
| snowpatch_ozlabs/github-powerpc_clang | success | Successfully ran 5 jobs. |
| snowpatch_ozlabs/github-powerpc_kernel_qemu | success | Successfully ran 21 jobs. |
On 8/20/24 12:04 PM, Michael Ellerman wrote: > The overflow/underflow conditions in pata_macio_qc_prep() should never > happen. But if they do there's no need to kill the system entirely, a > WARN and failing the IO request should be sufficient and might allow the > system to keep running. > > Signed-off-by: Michael Ellerman <mpe@ellerman.id.au> Applied to for-6.11-fixes. Thanks !
On 8/20/24 6:04 AM, Michael Ellerman wrote: > The overflow/underflow conditions in pata_macio_qc_prep() should never > happen. But if they do there's no need to kill the system entirely, a > WARN and failing the IO request should be sufficient and might allow the > system to keep running. WARN*() can kill your system with panic_on_warn -- Android is particularly fond of this kernel parameter but I guess it's not your case... :-) Greg KH usually advices against using these macros. :-) > Signed-off-by: Michael Ellerman <mpe@ellerman.id.au> [...] Please do CC me on the PATA driver patches! This one circumvented my review (again)... :-/ MBR, Sergey
On Thu, Aug 22, 2024 at 12:13:52AM +0300, Sergei Shtylyov wrote: > On 8/20/24 6:04 AM, Michael Ellerman wrote: > > > The overflow/underflow conditions in pata_macio_qc_prep() should never > > happen. But if they do there's no need to kill the system entirely, a > > WARN and failing the IO request should be sufficient and might allow the > > system to keep running. > > WARN*() can kill your system with panic_on_warn -- Android is particularly > fond of this kernel parameter but I guess it's not your case... :-) > Greg KH usually advices against using these macros. :-) And in this case he is simply totally wrong. The whole poing of WARN_ON is to have a standardized way to assert conditions.
On 8/22/24 5:59 AM, Christoph Hellwig wrote: [...] >>> The overflow/underflow conditions in pata_macio_qc_prep() should never >>> happen. But if they do there's no need to kill the system entirely, a >>> WARN and failing the IO request should be sufficient and might allow the >>> system to keep running. >> >> WARN*() can kill your system with panic_on_warn -- Android is particularly >> fond of this kernel parameter but I guess it's not your case... :-) >> Greg KH usually advices against using these macros. :-) > > And in this case he is simply totally wrong. The whole poing of WARN_ON Greg does have a point: on billions of Linux systems (Android phones) that all use panic_on_warn=1, WARN*() is pretty much equivalent to panic()... :-/ > is to have a standardized way to assert conditions. Hm, makes me remember assert() in C aborts a program... :-) MBR, Sergey
Sergei Shtylyov <sergei.shtylyov@gmail.com> writes: > On 8/20/24 6:04 AM, Michael Ellerman wrote: > >> The overflow/underflow conditions in pata_macio_qc_prep() should never >> happen. But if they do there's no need to kill the system entirely, a >> WARN and failing the IO request should be sufficient and might allow the >> system to keep running. > > WARN*() can kill your system with panic_on_warn -- Android is particularly > fond of this kernel parameter but I guess it's not your case... :-) > Greg KH usually advices against using these macros. :-) Yeah, but in this case it's replacing BUG with WARN, so I figure it's clearly an improvement. Also if someone is running with panic_on_warn then they *want* their system to panic if anything strange happens, which is the case here. >> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au> > [...] > > Please do CC me on the PATA driver patches! This one circumvented my review > (again)... :-/ Oops sorry, I think I just grabbed the Cc's from the report. I'll use get_maintainer.pl in future. cheers
diff --git a/drivers/ata/pata_macio.c b/drivers/ata/pata_macio.c index 1cb8d24b088f..f2f36e55a1f4 100644 --- a/drivers/ata/pata_macio.c +++ b/drivers/ata/pata_macio.c @@ -554,7 +554,8 @@ static enum ata_completion_errors pata_macio_qc_prep(struct ata_queued_cmd *qc) while (sg_len) { /* table overflow should never happen */ - BUG_ON (pi++ >= MAX_DCMDS); + if (WARN_ON_ONCE(pi >= MAX_DCMDS)) + return AC_ERR_SYSTEM; len = (sg_len < MAX_DBDMA_SEG) ? sg_len : MAX_DBDMA_SEG; table->command = cpu_to_le16(write ? OUTPUT_MORE: INPUT_MORE); @@ -566,11 +567,13 @@ static enum ata_completion_errors pata_macio_qc_prep(struct ata_queued_cmd *qc) addr += len; sg_len -= len; ++table; + ++pi; } } /* Should never happen according to Tejun */ - BUG_ON(!pi); + if (WARN_ON_ONCE(!pi)) + return AC_ERR_SYSTEM; /* Convert the last command to an input/output */ table--;
The overflow/underflow conditions in pata_macio_qc_prep() should never happen. But if they do there's no need to kill the system entirely, a WARN and failing the IO request should be sufficient and might allow the system to keep running. Signed-off-by: Michael Ellerman <mpe@ellerman.id.au> --- drivers/ata/pata_macio.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) v2: Use AC_ERR_SYSTEM as suggested by Damien.