On Thursday 09 August 2018 07:35:33 Christoph Hellwig wrote:
> On Wed, Aug 08, 2018 at 10:30:19PM +0200, Ondrej Zary wrote:
> > Then it crashes with null-pointer dereference in aha1542_reset.
>
> Must be the dma_unmap. Updated patch below:
>
> ---
> From 98a9c770430cde972923838b990b2b9cf7b95816 Mon Sep 17 00:00:00 2001
> From: Christoph Hellwig <hch@xxxxxx>
> Date: Thu, 2 Aug 2018 14:22:46 +0200
> Subject: aha1542: convert to DMA mapping API
>
> aha1542 is one of the last users of the legacy isa_*_to_bus APIs, which
> also isn't portable enough. Convert it to the proper DMA mapping API.
>
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>
> ---
> drivers/scsi/aha1542.c | 126 +++++++++++++++++++++++++++++------------
> 1 file changed, 91 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/scsi/aha1542.c b/drivers/scsi/aha1542.c
> index 41add33e3f1f..d09e19dde177 100644
> --- a/drivers/scsi/aha1542.c
> +++ b/drivers/scsi/aha1542.c
> @@ -58,8 +58,15 @@ struct aha1542_hostdata {
> int aha1542_last_mbi_used;
> int aha1542_last_mbo_used;
> struct scsi_cmnd *int_cmds[AHA1542_MAILBOXES];
> - struct mailbox mb[2 * AHA1542_MAILBOXES];
> - struct ccb ccb[AHA1542_MAILBOXES];
> + struct mailbox *mb;
> + dma_addr_t mb_handle;
> + struct ccb *ccb;
> + dma_addr_t ccb_handle;
> +};
> +
> +struct aha1542_cmd {
> + struct chain *chain;
> + dma_addr_t chain_handle;
> };
>
> static inline void aha1542_intr_reset(u16 base)
> @@ -233,6 +240,21 @@ static int aha1542_test_port(struct Scsi_Host *sh)
> return 1;
> }
>
> +static void aha1542_free_cmd(struct scsi_cmnd *cmd)
> +{
> + struct aha1542_cmd *acmd = scsi_cmd_priv(cmd);
> + struct device *dev = cmd->device->host->dma_dev;
> + size_t len = scsi_sg_count(cmd) * sizeof(struct chain);
> +
> + if (acmd->chain) {
> + dma_unmap_single(dev, acmd->chain_handle, len, DMA_TO_DEVICE);
> + kfree(acmd->chain);
> + }
> +
> + acmd->chain = NULL;
> + scsi_dma_unmap(cmd);
> +}
> +
> static irqreturn_t aha1542_interrupt(int irq, void *dev_id)
> {
> struct Scsi_Host *sh = dev_id;
> @@ -303,7 +325,7 @@ static irqreturn_t aha1542_interrupt(int irq, void *dev_id)
> return IRQ_HANDLED;
> };
>
> - mbo = (scsi2int(mb[mbi].ccbptr) - (isa_virt_to_bus(&ccb[0]))) / sizeof(struct ccb);
> + mbo = (scsi2int(mb[mbi].ccbptr) - aha1542->ccb_handle) / sizeof(struct ccb);
> mbistatus = mb[mbi].status;
> mb[mbi].status = 0;
> aha1542->aha1542_last_mbi_used = mbi;
> @@ -331,8 +353,7 @@ static irqreturn_t aha1542_interrupt(int irq, void *dev_id)
> return IRQ_HANDLED;
> }
> my_done = tmp_cmd->scsi_done;
> - kfree(tmp_cmd->host_scribble);
> - tmp_cmd->host_scribble = NULL;
> + aha1542_free_cmd(tmp_cmd);
> /* Fetch the sense data, and tuck it away, in the required slot. The
> Adaptec automatically fetches it, and there is no guarantee that
> we will still have it in the cdb when we come back */
> @@ -369,6 +390,7 @@ static irqreturn_t aha1542_interrupt(int irq, void *dev_id)
>
> static int aha1542_queuecommand(struct Scsi_Host *sh, struct scsi_cmnd *cmd)
> {
> + struct aha1542_cmd *acmd = scsi_cmd_priv(cmd);
> struct aha1542_hostdata *aha1542 = shost_priv(sh);
> u8 direction;
> u8 target = cmd->device->id;
> @@ -378,7 +400,6 @@ static int aha1542_queuecommand(struct Scsi_Host *sh, struct scsi_cmnd *cmd)
> int mbo, sg_count;
> struct mailbox *mb = aha1542->mb;
> struct ccb *ccb = aha1542->ccb;
> - struct chain *cptr;
>
> if (*cmd->cmnd == REQUEST_SENSE) {
> /* Don't do the command - we have the sense data already */
> @@ -398,15 +419,17 @@ static int aha1542_queuecommand(struct Scsi_Host *sh, struct scsi_cmnd *cmd)
> print_hex_dump_bytes("command: ", DUMP_PREFIX_NONE, cmd->cmnd, cmd->cmd_len);
> }
> #endif
> - if (bufflen) { /* allocate memory before taking host_lock */
> - sg_count = scsi_sg_count(cmd);
> - cptr = kmalloc_array(sg_count, sizeof(*cptr),
> - GFP_KERNEL | GFP_DMA);
> - if (!cptr)
> - return SCSI_MLQUEUE_HOST_BUSY;
> - } else {
> - sg_count = 0;
> - cptr = NULL;
> + sg_count = scsi_dma_map(cmd);
> + if (sg_count) {
> + size_t len = sg_count * sizeof(struct chain);
> +
> + acmd->chain = kmalloc(len, GFP_DMA);
> + if (!acmd->chain)
> + goto out_unmap;
> + acmd->chain_handle = dma_map_single(sh->dma_dev, acmd->chain,
> + len, DMA_TO_DEVICE);
> + if (dma_mapping_error(sh->dma_dev, acmd->chain_handle))
> + goto out_free_chain;
> }
>
> /* Use the outgoing mailboxes in a round-robin fashion, because this
> @@ -437,7 +460,8 @@ static int aha1542_queuecommand(struct Scsi_Host *sh, struct scsi_cmnd *cmd)
> shost_printk(KERN_DEBUG, sh, "Sending command (%d %p)...", mbo, cmd->scsi_done);
> #endif
>
> - any2scsi(mb[mbo].ccbptr, isa_virt_to_bus(&ccb[mbo])); /* This gets trashed for some reason */
> + /* This gets trashed for some reason */
> + any2scsi(mb[mbo].ccbptr, aha1542->ccb_handle + mbo * sizeof(ccb));
>
> memset(&ccb[mbo], 0, sizeof(struct ccb));
>
> @@ -456,21 +480,18 @@ static int aha1542_queuecommand(struct Scsi_Host *sh, struct scsi_cmnd *cmd)
> int i;
>
> ccb[mbo].op = 2; /* SCSI Initiator Command w/scatter-gather */
> - cmd->host_scribble = (void *)cptr;
> scsi_for_each_sg(cmd, sg, sg_count, i) {
> - any2scsi(cptr[i].dataptr, isa_page_to_bus(sg_page(sg))
> - + sg->offset);
> - any2scsi(cptr[i].datalen, sg->length);
> + any2scsi(acmd->chain[i].dataptr, sg_dma_address(sg));
> + any2scsi(acmd->chain[i].datalen, sg_dma_len(sg));
> };
> any2scsi(ccb[mbo].datalen, sg_count * sizeof(struct chain));
> - any2scsi(ccb[mbo].dataptr, isa_virt_to_bus(cptr));
> + any2scsi(ccb[mbo].dataptr, acmd->chain_handle);
> #ifdef DEBUG
> - shost_printk(KERN_DEBUG, sh, "cptr %p: ", cptr);
> - print_hex_dump_bytes("cptr: ", DUMP_PREFIX_NONE, cptr, 18);
> + shost_printk(KERN_DEBUG, sh, "cptr %p: ", acmd->chain);
> + print_hex_dump_bytes("cptr: ", DUMP_PREFIX_NONE, acmd->chain, 18);
> #endif
> } else {
> ccb[mbo].op = 0; /* SCSI Initiator Command */
> - cmd->host_scribble = NULL;
> any2scsi(ccb[mbo].datalen, 0);
> any2scsi(ccb[mbo].dataptr, 0);
> };
> @@ -488,24 +509,29 @@ static int aha1542_queuecommand(struct Scsi_Host *sh, struct scsi_cmnd *cmd)
> spin_unlock_irqrestore(sh->host_lock, flags);
>
> return 0;
> +out_free_chain:
> + kfree(acmd->chain);
> + acmd->chain = NULL;
> +out_unmap:
> + scsi_dma_unmap(cmd);
> + return SCSI_MLQUEUE_HOST_BUSY;
> }
>
> /* Initialize mailboxes */
> static void setup_mailboxes(struct Scsi_Host *sh)
> {
> struct aha1542_hostdata *aha1542 = shost_priv(sh);
> - int i;
> - struct mailbox *mb = aha1542->mb;
> - struct ccb *ccb = aha1542->ccb;
> -
> u8 mb_cmd[5] = { CMD_MBINIT, AHA1542_MAILBOXES, 0, 0, 0};
> + int i;
>
> for (i = 0; i < AHA1542_MAILBOXES; i++) {
> - mb[i].status = mb[AHA1542_MAILBOXES + i].status = 0;
> - any2scsi(mb[i].ccbptr, isa_virt_to_bus(&ccb[i]));
> + aha1542->mb[i].status = 0;
> + any2scsi(aha1542->mb[i].ccbptr,
> + aha1542->ccb_handle + i * sizeof(struct ccb));
> + aha1542->mb[AHA1542_MAILBOXES + i].status = 0;
> };
> aha1542_intr_reset(sh->io_port); /* reset interrupts, so they don't block */
> - any2scsi((mb_cmd + 2), isa_virt_to_bus(mb));
> + any2scsi(mb_cmd + 2, aha1542->mb_handle);
> if (aha1542_out(sh->io_port, mb_cmd, 5))
> shost_printk(KERN_ERR, sh, "failed setting up mailboxes\n");
> aha1542_intr_reset(sh->io_port);
> @@ -739,11 +765,26 @@ static struct Scsi_Host *aha1542_hw_init(struct scsi_host_template *tpnt, struct
> if (aha1542->bios_translation == BIOS_TRANSLATION_25563)
> shost_printk(KERN_INFO, sh, "Using extended bios translation\n");
>
> + if (dma_set_mask_and_coherent(pdev, DMA_BIT_MASK(24)) < 0)
> + goto unregister;
> +
> + aha1542->mb = dma_alloc_coherent(pdev,
> + AHA1542_MAILBOXES * 2 * sizeof(struct mailbox),
> + &aha1542->mb_handle, GFP_KERNEL);
> + if (!aha1542->mb)
> + goto unregister;
> +
> + aha1542->ccb = dma_alloc_coherent(pdev,
> + AHA1542_MAILBOXES * sizeof(struct ccb),
> + &aha1542->ccb_handle, GFP_KERNEL);
> + if (!aha1542->ccb)
> + goto free_mb;
> +
> setup_mailboxes(sh);
>
> if (request_irq(sh->irq, aha1542_interrupt, 0, "aha1542", sh)) {
> shost_printk(KERN_ERR, sh, "Unable to allocate IRQ.\n");
> - goto unregister;
> + goto free_ccb;
> }
> if (sh->dma_channel != 0xFF) {
> if (request_dma(sh->dma_channel, "aha1542")) {
> @@ -762,11 +803,18 @@ static struct Scsi_Host *aha1542_hw_init(struct scsi_host_template *tpnt, struct
> scsi_scan_host(sh);
>
> return sh;
> +
> free_dma:
> if (sh->dma_channel != 0xff)
> free_dma(sh->dma_channel);
> free_irq:
> free_irq(sh->irq, sh);
> +free_ccb:
> + dma_free_coherent(pdev, AHA1542_MAILBOXES * sizeof(struct ccb),
> + aha1542->ccb, aha1542->ccb_handle);
> +free_mb:
> + dma_free_coherent(pdev, AHA1542_MAILBOXES * 2 * sizeof(struct mailbox),
> + aha1542->mb, aha1542->mb_handle);
> unregister:
> scsi_host_put(sh);
> release:
> @@ -777,9 +825,16 @@ static struct Scsi_Host *aha1542_hw_init(struct scsi_host_template *tpnt, struct
>
> static int aha1542_release(struct Scsi_Host *sh)
> {
> + struct aha1542_hostdata *aha1542 = shost_priv(sh);
> + struct device *dev = sh->dma_dev;
> +
> scsi_remove_host(sh);
> if (sh->dma_channel != 0xff)
> free_dma(sh->dma_channel);
> + dma_free_coherent(dev, AHA1542_MAILBOXES * sizeof(struct ccb),
> + aha1542->ccb, aha1542->ccb_handle);
> + dma_free_coherent(dev, AHA1542_MAILBOXES * 2 * sizeof(struct mailbox),
> + aha1542->mb, aha1542->mb_handle);
> if (sh->irq)
> free_irq(sh->irq, sh);
> if (sh->io_port && sh->n_io_port)
> @@ -826,7 +881,8 @@ static int aha1542_dev_reset(struct scsi_cmnd *cmd)
>
> aha1542->aha1542_last_mbo_used = mbo;
>
> - any2scsi(mb[mbo].ccbptr, isa_virt_to_bus(&ccb[mbo])); /* This gets trashed for some reason */
> + /* This gets trashed for some reason */
> + any2scsi(mb[mbo].ccbptr, aha1542->ccb_handle + mbo * sizeof(ccb));
>
> memset(&ccb[mbo], 0, sizeof(struct ccb));
>
> @@ -901,8 +957,7 @@ static int aha1542_reset(struct scsi_cmnd *cmd, u8 reset_cmd)
> */
> continue;
> }
> - kfree(tmp_cmd->host_scribble);
> - tmp_cmd->host_scribble = NULL;
> + aha1542_free_cmd(tmp_cmd);
> aha1542->int_cmds[i] = NULL;
> aha1542->mb[i].status = 0;
> }
> @@ -946,6 +1001,7 @@ static struct scsi_host_template driver_template = {
> .module = THIS_MODULE,
> .proc_name = "aha1542",
> .name = "Adaptec 1542",
> + .cmd_size = sizeof(struct aha1542_cmd),
> .queuecommand = aha1542_queuecommand,
> .eh_device_reset_handler= aha1542_dev_reset,
> .eh_bus_reset_handler = aha1542_bus_reset,
This patch fixes the problem and makes aha1542 work after your DMA mapping API
conversion:
diff --git a/drivers/scsi/aha1542.c b/drivers/scsi/aha1542.c
index d09e19dde177..ffe3b9881ce1 100644
--- a/drivers/scsi/aha1542.c
+++ b/drivers/scsi/aha1542.c
@@ -461,7 +461,7 @@ static int aha1542_queuecommand(struct Scsi_Host *sh, struct scsi_cmnd *cmd)
#endif
/* This gets trashed for some reason */
- any2scsi(mb[mbo].ccbptr, aha1542->ccb_handle + mbo * sizeof(ccb));
+ any2scsi(mb[mbo].ccbptr, aha1542->ccb_handle + mbo * sizeof(struct ccb));
memset(&ccb[mbo], 0, sizeof(struct ccb));
--
Ondrej Zary
