| Message ID | 20240819101913.489513-1-mpe@ellerman.id.au (mailing list archive) |
|---|---|
| State | Handled Elsewhere, archived |
| Headers | show |
| Series | [RFC] 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/19/24 19:19, 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> > --- > drivers/ata/pata_macio.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > Not sure if AC_ERR_OTHER is the right error code to use? Given that this would trigger if the command split has is buggy, I think that AC_ERR_SYSTEM would be better. Can you resend with the change and no "RFC" ? > > diff --git a/drivers/ata/pata_macio.c b/drivers/ata/pata_macio.c > index eaffa510de49..552e3ac0d391 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_OTHER; > > 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_OTHER; > > /* Convert the last command to an input/output */ > table--;
Damien Le Moal <dlemoal@kernel.org> writes: > On 8/19/24 19:19, 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> >> --- >> drivers/ata/pata_macio.c | 7 +++++-- >> 1 file changed, 5 insertions(+), 2 deletions(-) >> >> Not sure if AC_ERR_OTHER is the right error code to use? > > Given that this would trigger if the command split has is buggy, I think that > AC_ERR_SYSTEM would be better. Can you resend with the change and no "RFC" ? Will do. cheers
diff --git a/drivers/ata/pata_macio.c b/drivers/ata/pata_macio.c index eaffa510de49..552e3ac0d391 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_OTHER; 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_OTHER; /* 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(-) Not sure if AC_ERR_OTHER is the right error code to use?