On Sun, Apr 23, 2023 at 02:37:17PM -0700, Kelvin Cao wrote:
> Implement core PCI driver skeleton and register DMA engine callbacks.
I only noticed this now, but this sentence reads a bit odd. What does
it try to say?
> +struct chan_fw_regs {
> + u32 valid_en_se;
...
> + u16 cq_phase;
> +} __packed;
Everything here seems nicely naturally aligned, what is the reason
for the __packed attribute?
> +struct switchtec_dma_hw_se_desc {
> + u8 opc;
> + u8 ctrl;
> + __le16 tlp_setting;
> + __le16 rsvd1;
> + __le16 cid;
> + __le32 byte_cnt;
> + union {
> + __le32 saddr_lo;
> + __le32 widata_lo;
> + };
> + union {
> + __le32 saddr_hi;
> + __le32 widata_hi;
> + };
What is the point for unions of identical data types?
> + p = (int *)ce;
> + for (i = 0; i < sizeof(*ce)/4; i++) {
> + dev_err(chan_dev, "CE DW%d: 0x%08x\n", i,
> + le32_to_cpu((__force __le32)*p));
> + p++;
> + }
Why is this casting to an int that is never used and the back to CE?
Maybe a function that actually dumps the registers with names and
is type safe might be a better idea? If not just using
print_hex_dump would be a simpler, although that would always printk
in little endian representation (which might be easier to read anyway).
> + struct pci_dev *pdev;
> + struct switchtec_dma_chan *swdma_chan = to_switchtec_dma_chan(chan);
> + int rc;
> +
> + rcu_read_lock();
> + pdev = rcu_dereference(swdma_chan->swdma_dev->pdev);
> + rcu_read_unlock();
> +
> + if (pdev)
> + synchronize_irq(swdma_chan->irq);
At this point pdev might be freed as you're outside the RCU critical
section, and the irq number could have been reused.
> + switch (type) {
> + case MEMCPY:
> + if (len > SWITCHTEC_DESC_MAX_SIZE)
> + goto err_unlock;
> + break;
> + case WIMM:
> + if (len != 8)
> + break;
> +
> + if (dma_dst & ((1 << DMAENGINE_ALIGN_8_BYTES) - 1)) {
> + dev_err(chan_dev,
> + "QW WIMM dst addr 0x%08x_%08x not QW aligned!\n",
> + upper_32_bits(dma_dst), lower_32_bits(dma_dst));
> + goto err_unlock;
> + }
> + break;
> + default:
> + goto err_unlock;
> + }
IT looks like these checks could easily be done without the lock,
and in the respective callers.
> + if (type == MEMCPY) {
> + desc->hw->opc = SWITCHTEC_DMA_OPC_MEMCPY;
> + desc->hw->saddr_lo = cpu_to_le32(lower_32_bits(dma_src));
> + desc->hw->saddr_hi = cpu_to_le32(upper_32_bits(dma_src));
> + } else {
> + desc->hw->opc = SWITCHTEC_DMA_OPC_WRIMM;
> + desc->hw->widata_lo = cpu_to_le32(lower_32_bits(data));
> + desc->hw->widata_hi = cpu_to_le32(upper_32_bits(data));
> + }
... and then instead of the type I'd just pass the opcode directly,
simplifying the logic quite a bit.
> +static irqreturn_t switchtec_dma_isr(int irq, void *chan)
> +{
> + struct switchtec_dma_chan *swdma_chan = chan;
> +
> + if (swdma_chan->comp_ring_active)
> + tasklet_schedule(&swdma_chan->desc_task);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static irqreturn_t switchtec_dma_chan_status_isr(int irq, void *dma)
> +{
> + struct switchtec_dma_dev *swdma_dev = dma;
> +
> + tasklet_schedule(&swdma_dev->chan_status_task);
> +
> + return IRQ_HANDLED;
> +}
Same comment as last time - irq + tasklet seems quite hack and
inefficient over just using threaded interrupts. I'd like to see
a really good rationale for using it, and a Cc to the interrupt
maintainers that would love to kill off tasklets
> + addr = swdma_dev->bar + SWITCHTEC_DMAC_CHAN_CFG_STS_OFFSET;
> + addr += i * SWITCHTEC_DMA_CHAN_FW_REGS_SIZE;
> + chan_fw = (struct chan_fw_regs __iomem *)addr;
> +
> + addr = swdma_dev->bar + SWITCHTEC_DMAC_CHAN_CTRL_OFFSET;
> + addr += i * SWITCHTEC_DMA_CHAN_HW_REGS_SIZE;
> + chan_hw = (struct chan_hw_regs __iomem *)addr;
> +
> + swdma_dev->swdma_chans[i] = swdma_chan;
> + swdma_chan->mmio_chan_fw = chan_fw;
> + swdma_chan->mmio_chan_hw = chan_hw;
No need for the casts above. This could simply become:
swdma_chan->mmio_chan_fw =
swdma_dev->bar + SWITCHTEC_DMAC_CHAN_CFG_STS_OFFSET +
i * SWITCHTEC_DMA_CHAN_FW_REGS_SIZE;
swdma_chan->mmio_chan_hw =
swdma_dev->bar + SWITCHTEC_DMAC_CHAN_CTRL_OFFSET +
i * SWITCHTEC_DMA_CHAN_HW_REGS_SIZE;
> + rc = pause_reset_channel(swdma_chan);
> + if (rc)
> + goto free_and_exit;
> +
> + rcu_read_lock();
> + pdev = rcu_dereference(swdma_dev->pdev);
> + if (!pdev) {
> + rc = -ENODEV;
> + goto unlock_and_free;
> + }
The pdev can't go away while you're in ->probe as that is synchronized
vs ->remove and ->shutdown.
> + irq = pci_irq_vector(pdev, irq);
> + if (irq < 0) {
> + rc = irq;
> + goto unlock_and_free;
> + }
> +
> + rcu_read_unlock();
> +
> + rc = request_irq(irq, switchtec_dma_isr, 0, KBUILD_MODNAME, swdma_chan);
> + if (rc)
> + goto free_and_exit;
I'd just use pci_request_irq here.
> +#define SWITCHTEC_DMA_DEVICE(device_id) \
> + { \
> + .vendor = PCI_VENDOR_ID_MICROSEMI, \
> + .device = device_id, \
> + .subvendor = PCI_ANY_ID, \
> + .subdevice = PCI_ANY_ID, \
> + .class = PCI_CLASS_SYSTEM_OTHER << 8, \
> + .class_mask = 0xFFFFFFFF, \
> + }
> +
> +static const struct pci_device_id switchtec_dma_pci_tbl[] = {
> + SWITCHTEC_DMA_DEVICE(0x4000), /* PFX 100XG4 */
This should use the common PCI_DEVICE() macro instead, i.e.
PCI_DEVICE(PCI_VENDOR_ID_MICROSEMI, 0x4000), /* PFX 100XG4 */
...
