On 20-06-21, 11:41, Sanjay R Mehta wrote: > +static irqreturn_t pt_core_irq_handler(int irq, void *data) > +{ > + struct pt_device *pt = data; > + struct pt_cmd_queue *cmd_q = &pt->cmd_q; > + u32 status; > + bool err = true; > + > + pt_core_disable_queue_interrupts(pt); > + > + status = ioread32(cmd_q->reg_interrupt_status); > + if (status) { > + cmd_q->int_status = status; > + cmd_q->q_status = ioread32(cmd_q->reg_status); > + cmd_q->q_int_status = ioread32(cmd_q->reg_int_status); > + > + /* On error, only save the first error value */ > + if ((status & INT_ERROR) && !cmd_q->cmd_error) { > + cmd_q->cmd_error = CMD_Q_ERROR(cmd_q->q_status); > + err = false; > + } > + > + /* Acknowledge the interrupt */ > + iowrite32(status, cmd_q->reg_interrupt_status); > + } > + > + pt_core_enable_queue_interrupts(pt); > + > + return err ? IRQ_HANDLED : IRQ_NONE; On err you should not return IRQ_NONE. IRQ_NONE means "interrupt was not from this device or was not handled" Error is handled here! > +static struct pt_device *pt_alloc_struct(struct device *dev) > +{ > + struct pt_device *pt; > + > + pt = devm_kzalloc(dev, sizeof(*pt), GFP_KERNEL); > + > + if (!pt) > + return NULL; > + pt->dev = dev; > + > + INIT_LIST_HEAD(&pt->cmd); > + > + snprintf(pt->name, MAX_PT_NAME_LEN, "pt-%s", dev_name(dev)); what is this name used for? Why not use dev_name everywhere? > +static int pt_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id) > +{ > + struct pt_device *pt; > + struct pt_msix *pt_msix; > + struct device *dev = &pdev->dev; > + void __iomem * const *iomap_table; > + int bar_mask; > + int ret = -ENOMEM; > + > + pt = pt_alloc_struct(dev); > + if (!pt) > + goto e_err; > + > + pt_msix = devm_kzalloc(dev, sizeof(*pt_msix), GFP_KERNEL); > + if (!pt_msix) > + goto e_err; > + > + pt->pt_msix = pt_msix; > + pt->dev_vdata = (struct pt_dev_vdata *)id->driver_data; > + if (!pt->dev_vdata) { > + ret = -ENODEV; > + dev_err(dev, "missing driver data\n"); > + goto e_err; > + } > + > + ret = pcim_enable_device(pdev); > + if (ret) { > + dev_err(dev, "pcim_enable_device failed (%d)\n", ret); > + goto e_err; > + } > + > + bar_mask = pci_select_bars(pdev, IORESOURCE_MEM); > + ret = pcim_iomap_regions(pdev, bar_mask, "ptdma"); > + if (ret) { > + dev_err(dev, "pcim_iomap_regions failed (%d)\n", ret); > + goto e_err; > + } > + > + iomap_table = pcim_iomap_table(pdev); > + if (!iomap_table) { > + dev_err(dev, "pcim_iomap_table failed\n"); > + ret = -ENOMEM; > + goto e_err; > + } > + > + pt->io_regs = iomap_table[pt->dev_vdata->bar]; > + if (!pt->io_regs) { > + dev_err(dev, "ioremap failed\n"); > + ret = -ENOMEM; > + goto e_err; > + } > + > + ret = pt_get_irqs(pt); > + if (ret) > + goto e_err; > + > + pci_set_master(pdev); > + > + ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(48)); > + if (ret) { > + ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32)); > + if (ret) { > + dev_err(dev, "dma_set_mask_and_coherent failed (%d)\n", > + ret); > + goto e_err; > + } > + } > + > + dev_set_drvdata(dev, pt); > + > + if (pt->dev_vdata) > + ret = pt_core_init(pt); > + > + if (ret) > + goto e_err; > + > + return 0; > + > +e_err: > + dev_err(dev, "initialization failed\n"); log the err code, that is very useful! > + /* Register addresses for queue */ > + void __iomem *reg_control; > + void __iomem *reg_tail_lo; > + void __iomem *reg_head_lo; > + void __iomem *reg_int_enable; > + void __iomem *reg_interrupt_status; > + void __iomem *reg_status; > + void __iomem *reg_int_status; > + void __iomem *reg_dma_status; > + void __iomem *reg_dma_read_status; > + void __iomem *reg_dma_write_status; this looks like pointer to registers, wont it make sense to keep base ptr and use offset to read..? Looking at pt_init_cmdq_regs(), i think that seems to be the case. Why waste so much memory by having so many pointers? > + u32 qcontrol; /* Cached control register */ > + > + /* Status values from job */ > + u32 int_status; > + u32 q_status; > + u32 q_int_status; > + u32 cmd_error; > +} ____cacheline_aligned; > + > +struct pt_device { > + struct list_head entry; > + > + unsigned int ord; Unused? -- ~Vinod