Damien Le Moal <dlemoal@xxxxxxxxxx> writes:
> On 8/19/24 19:17, Michael Ellerman wrote:
>> Kolbjørn and Jonáš reported that their 32-bit PowerMacs were crashing
>> in pata-macio since commit 09fe2bfa6b83 ("ata: pata_macio: Fix
>> max_segment_size with PAGE_SIZE == 64K").
>>
>> For example:
>>
>> kernel BUG at drivers/ata/pata_macio.c:544!
>> Oops: Exception in kernel mode, sig: 5 [#1]
>> BE PAGE_SIZE=4K MMU=Hash SMP NR_CPUS=2 DEBUG_PAGEALLOC PowerMac
>> ...
>> NIP pata_macio_qc_prep+0xf4/0x190
>> LR pata_macio_qc_prep+0xfc/0x190
>> Call Trace:
>> 0xc1421660 (unreliable)
>> ata_qc_issue+0x14c/0x2d4
>> __ata_scsi_queuecmd+0x200/0x53c
>> ata_scsi_queuecmd+0x50/0xe0
>> scsi_queue_rq+0x788/0xb1c
>> __blk_mq_issue_directly+0x58/0xf4
>> blk_mq_plug_issue_direct+0x8c/0x1b4
>> blk_mq_flush_plug_list.part.0+0x584/0x5e0
>> __blk_flush_plug+0xf8/0x194
>> __submit_bio+0x1b8/0x2e0
>> submit_bio_noacct_nocheck+0x230/0x304
>> btrfs_work_helper+0x200/0x338
>> process_one_work+0x1a8/0x338
>> worker_thread+0x364/0x4c0
>> kthread+0x100/0x104
>> start_kernel_thread+0x10/0x14
>>
>> That commit increased max_segment_size to 64KB, with the justification
>> that the SCSI core was already using that size when PAGE_SIZE == 64KB,
>> and that there was existing logic to split over-sized requests.
>>
>> However with a sufficiently large request, the splitting logic causes
>> each sg to be split into two commands in the DMA table, leading to
>> overflow of the DMA table, triggering the BUG_ON().
>>
>> With default settings the bug doesn't trigger, because the request size
>> is limited by max_sectors_kb == 1280, however max_sectors_kb can be
>> increased, and apparently some distros do that by default using udev
>> rules.
>>
>> Fix the bug for 4KB kernels by reverting to the old max_segment_size.
>>
>> For 64KB kernels the sg_tablesize needs to be halved, to allow for the
>> possibility that each sg will be split into two.
>>
>> Fixes: 09fe2bfa6b83 ("ata: pata_macio: Fix max_segment_size with PAGE_SIZE == 64K")
>> Cc: stable@xxxxxxxxxxxxxxx # v6.10+
>> Reported-by: Kolbjørn Barmen <linux-ppc@xxxxxxxx>
>> Closes: https://lore.kernel.org/all/62d248bb-e97a-25d2-bcf2-9160c518cae5@xxxxxxxx/
>> Reported-by: Jonáš Vidra <vidra@xxxxxxxxxxxxxxxx>
>> Closes: https://lore.kernel.org/all/3b6441b8-06e6-45da-9e55-f92f2c86933e@xxxxxxxxxxxxxxxx/
>> Tested-by: Kolbjørn Barmen <linux-ppc@xxxxxxxx>
>> Signed-off-by: Michael Ellerman <mpe@xxxxxxxxxxxxxx>
>> ---
>> drivers/ata/pata_macio.c | 23 +++++++++++++++--------
>> 1 file changed, 15 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/ata/pata_macio.c b/drivers/ata/pata_macio.c
>> index 1b85e8bf4ef9..eaffa510de49 100644
>> --- a/drivers/ata/pata_macio.c
>> +++ b/drivers/ata/pata_macio.c
>> @@ -208,6 +208,19 @@ static const char* macio_ata_names[] = {
>> /* Don't let a DMA segment go all the way to 64K */
>> #define MAX_DBDMA_SEG 0xff00
>>
>> +#ifdef CONFIG_PAGE_SIZE_64KB
>> +/*
>> + * The SCSI core requires the segment size to cover at least a page, so
>> + * for 64K page size kernels it must be at least 64K. However the
>> + * hardware can't handle 64K, so pata_macio_qc_prep() will split large
>> + * requests. To handle the split requests the tablesize must be halved.
>> + */
>> +#define MAX_SEGMENT_SIZE SZ_64K
>> +#define SG_TABLESIZE (MAX_DCMDS / 2)
>> +#else
>> +#define MAX_SEGMENT_SIZE MAX_DBDMA_SEG
>> +#define SG_TABLESIZE MAX_DCMDS
>> +#endif
>
> These names are rather generic and could clash with some core layer ditions. So
> maybe prefix the macro names with PATA_MACIO_ ?
> Also please tab-align the values to make this a little easier to read.
OK.
> Other than this, looks good to me.
OK thanks, will send a v2.
cheers
