| Message ID | 20190116161215.23656-1-hch@lst.de (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | a8cf59a6692c9c55a5a10257de97919fae6edef8 |
| Headers | show |
| Series | [for-5.0] scsi: communicate max segment size to the DMA mapping code | expand |
Christoph, > When a host driver sets a maximum segment size we should not only > propagate that setting to the block layer, which can merge segments, > but also to the DMA mapping layer which can merge segments as well. Applied to 5.0/scsi-fixes, thanks!
On 01/16/2019 05:12 PM, Christoph Hellwig wrote: > When a host driver sets a maximum segment size we should not only > propagate that setting to the block layer, which can merge segments, > but also to the DMA mapping layer which can merge segments as well. > > Fixes: 50c2e9107f ("scsi: introduce a max_segment_size host_template parameters") > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > drivers/ata/pata_macio.c | 9 ++++----- > drivers/ata/sata_inic162x.c | 22 +++++++++------------- > drivers/firewire/sbp2.c | 5 +---- > drivers/scsi/aacraid/linit.c | 9 ++++----- > drivers/scsi/scsi_lib.c | 4 ++-- > 5 files changed, 20 insertions(+), 29 deletions(-) > diff --git a/drivers/firewire/sbp2.c b/drivers/firewire/sbp2.c > index 09b845e90114..a785ffd5af89 100644 > --- a/drivers/firewire/sbp2.c > +++ b/drivers/firewire/sbp2.c > @@ -1144,10 +1144,6 @@ static int sbp2_probe(struct fw_unit *unit, const struct ieee1394_device_id *id) > if (device->is_local) > return -ENODEV; > > - if (dma_get_max_seg_size(device->card->device) > SBP2_MAX_SEG_SIZE) > - WARN_ON(dma_set_max_seg_size(device->card->device, > - SBP2_MAX_SEG_SIZE)); > - > shost = scsi_host_alloc(&scsi_driver_template, sizeof(*tgt)); > if (shost == NULL) > return -ENOMEM; > @@ -1610,6 +1606,7 @@ static struct scsi_host_template scsi_driver_template = { > .eh_abort_handler = sbp2_scsi_abort, > .this_id = -1, > .sg_tablesize = SG_ALL, > + .max_segment_size = SBP2_MAX_SEG_SIZE, > .can_queue = 1, > .sdev_attrs = sbp2_scsi_sysfs_attrs, > }; > diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c > index b13cc9288ba0..6d65ac584eba 100644 > --- a/drivers/scsi/scsi_lib.c > +++ b/drivers/scsi/scsi_lib.c > @@ -1842,8 +1842,8 @@ void __scsi_init_queue(struct Scsi_Host *shost, struct request_queue *q) > blk_queue_segment_boundary(q, shost->dma_boundary); > dma_set_seg_boundary(dev, shost->dma_boundary); > > - blk_queue_max_segment_size(q, > - min(shost->max_segment_size, dma_get_max_seg_size(dev))); > + blk_queue_max_segment_size(q, shost->max_segment_size); > + dma_set_max_seg_size(dev, shost->max_segment_size); Zfcp can only have max_segment_size of one page (4kB). Officially announced through dev.dma_parms since v2.6.35 https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/s390/scsi?id=683229845f1780b10041ee7a1043fc8f10061455. With Martin's 5.0/scsi-fixes which includes your above patch, I now get 64kB instead of 4kB for the respective queue limit because __scsi_ini_queue overwrites the dma parameter: [root@host:/sys/bus/ccw/drivers/zfcp/0.0.3c40/host5/rport-5:0-4/target5:0:4/5:0:4:10/block/sdi/queue](0)# cat max_segment_size 65536 To my surprise, I don't get IO errors with zfcp. Maybe my IO pattern does not cause too large segments to be created. I would have expected the FCP channel complaining rightly so if we pass segments larger than one page. Maybe the additional dma_boundary of pagesize-1 helped the too large max_segment_size to not become effective? A quick attempt to adapt zfcp to your patch would be to set scsi_host_template.max_segment_size = ZFCP_QDIO_SBALE_LEN. Ideas?
On Wed, Jan 23, 2019 at 02:45:49PM +0100, Steffen Maier wrote: > Zfcp can only have max_segment_size of one page (4kB). Officially announced > through dev.dma_parms since v2.6.35 > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/s390/scsi?id=683229845f1780b10041ee7a1043fc8f10061455. > > With Martin's 5.0/scsi-fixes which includes your above patch, I now get > 64kB instead of 4kB for the respective queue limit because __scsi_ini_queue > overwrites the dma parameter: Well, had the driver usd the proper dma_set_max_seg_size API instead of handcoding it I would have converted it.. > To my surprise, I don't get IO errors with zfcp. Maybe my IO pattern does > not cause too large segments to be created. I would have expected the FCP > channel complaining rightly so if we pass segments larger than one page. > Maybe the additional dma_boundary of pagesize-1 helped the too large > max_segment_size to not become effective? The driver already sets the dma_boundary field, which prevents from merging I/Os that span multiple pages, and thus limits each segment to a page or less. > A quick attempt to adapt zfcp to your patch would be to set > scsi_host_template.max_segment_size = ZFCP_QDIO_SBALE_LEN. I think we can just drop the dma_parms max_segment_size without replacement due to the dma_boundary. You do however still need to set up the dma_parms structure itself, as that is also used to communicate the boundary to the IOMMU. If would however be great if you moved that setup into the bus code instead of the driver, like we do for all other major hardware busses.
diff --git a/drivers/ata/pata_macio.c b/drivers/ata/pata_macio.c index 8cc9c429ad95..9e7fc302430f 100644 --- a/drivers/ata/pata_macio.c +++ b/drivers/ata/pata_macio.c @@ -915,6 +915,10 @@ static struct scsi_host_template pata_macio_sht = { .sg_tablesize = MAX_DCMDS, /* We may not need that strict one */ .dma_boundary = ATA_DMA_BOUNDARY, + /* Not sure what the real max is but we know it's less than 64K, let's + * use 64K minus 256 + */ + .max_segment_size = MAX_DBDMA_SEG, .slave_configure = pata_macio_slave_config, }; @@ -1044,11 +1048,6 @@ static int pata_macio_common_init(struct pata_macio_priv *priv, /* Make sure we have sane initial timings in the cache */ pata_macio_default_timings(priv); - /* Not sure what the real max is but we know it's less than 64K, let's - * use 64K minus 256 - */ - dma_set_max_seg_size(priv->dev, MAX_DBDMA_SEG); - /* Allocate libata host for 1 port */ memset(&pinfo, 0, sizeof(struct ata_port_info)); pmac_macio_calc_timing_masks(priv, &pinfo); diff --git a/drivers/ata/sata_inic162x.c b/drivers/ata/sata_inic162x.c index e0bcf9b2dab0..174e84ce4379 100644 --- a/drivers/ata/sata_inic162x.c +++ b/drivers/ata/sata_inic162x.c @@ -245,8 +245,15 @@ struct inic_port_priv { static struct scsi_host_template inic_sht = { ATA_BASE_SHT(DRV_NAME), - .sg_tablesize = LIBATA_MAX_PRD, /* maybe it can be larger? */ - .dma_boundary = INIC_DMA_BOUNDARY, + .sg_tablesize = LIBATA_MAX_PRD, /* maybe it can be larger? */ + + /* + * This controller is braindamaged. dma_boundary is 0xffff like others + * but it will lock up the whole machine HARD if 65536 byte PRD entry + * is fed. Reduce maximum segment size. + */ + .dma_boundary = INIC_DMA_BOUNDARY, + .max_segment_size = 65536 - 512, }; static const int scr_map[] = { @@ -868,17 +875,6 @@ static int inic_init_one(struct pci_dev *pdev, const struct pci_device_id *ent) return rc; } - /* - * This controller is braindamaged. dma_boundary is 0xffff - * like others but it will lock up the whole machine HARD if - * 65536 byte PRD entry is fed. Reduce maximum segment size. - */ - rc = dma_set_max_seg_size(&pdev->dev, 65536 - 512); - if (rc) { - dev_err(&pdev->dev, "failed to set the maximum segment size\n"); - return rc; - } - rc = init_controller(hpriv->mmio_base, hpriv->cached_hctl); if (rc) { dev_err(&pdev->dev, "failed to initialize controller\n"); diff --git a/drivers/firewire/sbp2.c b/drivers/firewire/sbp2.c index 09b845e90114..a785ffd5af89 100644 --- a/drivers/firewire/sbp2.c +++ b/drivers/firewire/sbp2.c @@ -1144,10 +1144,6 @@ static int sbp2_probe(struct fw_unit *unit, const struct ieee1394_device_id *id) if (device->is_local) return -ENODEV; - if (dma_get_max_seg_size(device->card->device) > SBP2_MAX_SEG_SIZE) - WARN_ON(dma_set_max_seg_size(device->card->device, - SBP2_MAX_SEG_SIZE)); - shost = scsi_host_alloc(&scsi_driver_template, sizeof(*tgt)); if (shost == NULL) return -ENOMEM; @@ -1610,6 +1606,7 @@ static struct scsi_host_template scsi_driver_template = { .eh_abort_handler = sbp2_scsi_abort, .this_id = -1, .sg_tablesize = SG_ALL, + .max_segment_size = SBP2_MAX_SEG_SIZE, .can_queue = 1, .sdev_attrs = sbp2_scsi_sysfs_attrs, }; diff --git a/drivers/scsi/aacraid/linit.c b/drivers/scsi/aacraid/linit.c index 634ddb90e7aa..7e56a11836c1 100644 --- a/drivers/scsi/aacraid/linit.c +++ b/drivers/scsi/aacraid/linit.c @@ -1747,11 +1747,10 @@ static int aac_probe_one(struct pci_dev *pdev, const struct pci_device_id *id) shost->max_sectors = (shost->sg_tablesize * 8) + 112; } - error = dma_set_max_seg_size(&pdev->dev, - (aac->adapter_info.options & AAC_OPT_NEW_COMM) ? - (shost->max_sectors << 9) : 65536); - if (error) - goto out_deinit; + if (aac->adapter_info.options & AAC_OPT_NEW_COMM) + shost->max_segment_size = shost->max_sectors << 9; + else + shost->max_segment_size = 65536; /* * Firmware printf works only with older firmware. diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c index b13cc9288ba0..6d65ac584eba 100644 --- a/drivers/scsi/scsi_lib.c +++ b/drivers/scsi/scsi_lib.c @@ -1842,8 +1842,8 @@ void __scsi_init_queue(struct Scsi_Host *shost, struct request_queue *q) blk_queue_segment_boundary(q, shost->dma_boundary); dma_set_seg_boundary(dev, shost->dma_boundary); - blk_queue_max_segment_size(q, - min(shost->max_segment_size, dma_get_max_seg_size(dev))); + blk_queue_max_segment_size(q, shost->max_segment_size); + dma_set_max_seg_size(dev, shost->max_segment_size); /* * Set a reasonable default alignment: The larger of 32-byte (dword),
When a host driver sets a maximum segment size we should not only propagate that setting to the block layer, which can merge segments, but also to the DMA mapping layer which can merge segments as well. Fixes: 50c2e9107f ("scsi: introduce a max_segment_size host_template parameters") Signed-off-by: Christoph Hellwig <hch@lst.de> --- drivers/ata/pata_macio.c | 9 ++++----- drivers/ata/sata_inic162x.c | 22 +++++++++------------- drivers/firewire/sbp2.c | 5 +---- drivers/scsi/aacraid/linit.c | 9 ++++----- drivers/scsi/scsi_lib.c | 4 ++-- 5 files changed, 20 insertions(+), 29 deletions(-)