[PATCH v2] ata: pata_macio: Use WARN instead of BUG

[PATCH v2] ata: pata_macio: Use WARN instead of BUG

[Michael Ellerman]

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.

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--;
-- 
2.46.0

Re: [PATCH v2] ata: pata_macio: Use WARN instead of BUG

[Damien Le Moal]

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 !

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v2] ata: pata_macio: Use WARN instead of BUG

[Sergei Shtylyov]

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

Re: [PATCH v2] ata: pata_macio: Use WARN instead of BUG

[Christoph Hellwig]

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.

Re: [PATCH v2] ata: pata_macio: Use WARN instead of BUG

[Sergei Shtylyov]

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

Re: [PATCH v2] ata: pata_macio: Use WARN instead of BUG

[Michael Ellerman]

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