| Message ID | 20240819101755.489078-1-mpe@ellerman.id.au |
|---|---|
| State | New |
| Headers | show |
| Series | ata: pata_macio: Fix DMA table overflow | expand |
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@vger.kernel.org # v6.10+ > Reported-by: Kolbjørn Barmen <linux-ppc@kolla.no> > Closes: https://lore.kernel.org/all/62d248bb-e97a-25d2-bcf2-9160c518cae5@kolla.no/ > Reported-by: Jonáš Vidra <vidra@ufal.mff.cuni.cz> > Closes: https://lore.kernel.org/all/3b6441b8-06e6-45da-9e55-f92f2c86933e@ufal.mff.cuni.cz/ > Tested-by: Kolbjørn Barmen <linux-ppc@kolla.no> > Signed-off-by: Michael Ellerman <mpe@ellerman.id.au> > --- > 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. Other than this, looks good to me. > > /* > * Wait 1s for disk to answer on IDE bus after a hard reset > @@ -912,16 +925,10 @@ static int pata_macio_do_resume(struct pata_macio_priv *priv) > > static const struct scsi_host_template pata_macio_sht = { > __ATA_BASE_SHT(DRV_NAME), > - .sg_tablesize = MAX_DCMDS, > + .sg_tablesize = SG_TABLESIZE, > /* We may not need that strict one */ > .dma_boundary = ATA_DMA_BOUNDARY, > - /* > - * The SCSI core requires the segment size to cover at least a page, so > - * for 64K page size kernels this must be at least 64K. However the > - * hardware can't handle 64K, so pata_macio_qc_prep() will split large > - * requests. > - */ > - .max_segment_size = SZ_64K, > + .max_segment_size = MAX_SEGMENT_SIZE, > .device_configure = pata_macio_device_configure, > .sdev_groups = ata_common_sdev_groups, > .can_queue = ATA_DEF_QUEUE,
Damien Le Moal <dlemoal@kernel.org> 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@vger.kernel.org # v6.10+ >> Reported-by: Kolbjørn Barmen <linux-ppc@kolla.no> >> Closes: https://lore.kernel.org/all/62d248bb-e97a-25d2-bcf2-9160c518cae5@kolla.no/ >> Reported-by: Jonáš Vidra <vidra@ufal.mff.cuni.cz> >> Closes: https://lore.kernel.org/all/3b6441b8-06e6-45da-9e55-f92f2c86933e@ufal.mff.cuni.cz/ >> Tested-by: Kolbjørn Barmen <linux-ppc@kolla.no> >> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au> >> --- >> 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
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 /* * Wait 1s for disk to answer on IDE bus after a hard reset @@ -912,16 +925,10 @@ static int pata_macio_do_resume(struct pata_macio_priv *priv) static const struct scsi_host_template pata_macio_sht = { __ATA_BASE_SHT(DRV_NAME), - .sg_tablesize = MAX_DCMDS, + .sg_tablesize = SG_TABLESIZE, /* We may not need that strict one */ .dma_boundary = ATA_DMA_BOUNDARY, - /* - * The SCSI core requires the segment size to cover at least a page, so - * for 64K page size kernels this must be at least 64K. However the - * hardware can't handle 64K, so pata_macio_qc_prep() will split large - * requests. - */ - .max_segment_size = SZ_64K, + .max_segment_size = MAX_SEGMENT_SIZE, .device_configure = pata_macio_device_configure, .sdev_groups = ata_common_sdev_groups, .can_queue = ATA_DEF_QUEUE,
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@vger.kernel.org # v6.10+ Reported-by: Kolbjørn Barmen <linux-ppc@kolla.no> Closes: https://lore.kernel.org/all/62d248bb-e97a-25d2-bcf2-9160c518cae5@kolla.no/ Reported-by: Jonáš Vidra <vidra@ufal.mff.cuni.cz> Closes: https://lore.kernel.org/all/3b6441b8-06e6-45da-9e55-f92f2c86933e@ufal.mff.cuni.cz/ Tested-by: Kolbjørn Barmen <linux-ppc@kolla.no> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au> --- drivers/ata/pata_macio.c | 23 +++++++++++++++-------- 1 file changed, 15 insertions(+), 8 deletions(-)