On Sun, Feb 09, 2025 at 01:09:06PM +0100, Lorenzo Bianconi wrote:
> +static irqreturn_t airoha_npu_wdt_handler(int irq, void *core_instance)
> +{
> + struct airoha_npu_core *core = core_instance;
> + struct airoha_npu *npu = core->npu;
> + int c = core - &npu->cores[0];
> + u32 val;
> +
> + airoha_npu_rmw(npu, REG_WDT_TIMER_CTRL(c), 0, WDT_INTR_MASK);
> + val = airoha_npu_rr(npu, REG_WDT_TIMER_CTRL(c));
> + if (FIELD_GET(WDT_EN_MASK, val))
> + schedule_work(&core->wdt_work);
> +
> + return IRQ_HANDLED;
> +}
> +
> +struct airoha_npu *airoha_npu_init(struct airoha_eth *eth)
> +{
> + struct reserved_mem *rmem;
> + int i, irq, err = -ENODEV;
> + struct airoha_npu *npu;
> + struct device_node *np;
> +
> + npu = devm_kzalloc(eth->dev, sizeof(*npu), GFP_KERNEL);
> + if (!npu)
> + return ERR_PTR(-ENOMEM);
> +
> + npu->np = of_parse_phandle(eth->dev->of_node, "airoha,npu", 0);
> + if (!npu->np)
> + return ERR_PTR(-ENODEV);
Why? The property is not required, so how can missing property fail the
probe?
This is also still unnecessary ABI break without explanation/reasoning.
> +
> + npu->pdev = of_find_device_by_node(npu->np);
> + if (!npu->pdev)
> + goto error_of_node_put;
You should also add device link and probably try_module_get. See
qcom,ice (patch for missing try_module_get is on the lists).
> +
> + get_device(&npu->pdev->dev);
Why? of_find_device_by_node() does it.
> +
> + npu->base = devm_platform_ioremap_resource(npu->pdev, 0);
> + if (IS_ERR(npu->base))
> + goto error_put_dev;
> +
> + np = of_parse_phandle(npu->np, "memory-region", 0);
> + if (!np)
> + goto error_put_dev;
> +
> + rmem = of_reserved_mem_lookup(np);
> + of_node_put(np);
> +
> + if (!rmem)
> + goto error_put_dev;
> +
> + irq = platform_get_irq(npu->pdev, 0);
> + if (irq < 0) {
> + err = irq;
> + goto error_put_dev;
> + }
> +
> + err = devm_request_irq(&npu->pdev->dev, irq, airoha_npu_mbox_handler,
> + IRQF_SHARED, "airoha-npu-mbox", npu);
> + if (err)
> + goto error_put_dev;
> +
> + for (i = 0; i < ARRAY_SIZE(npu->cores); i++) {
> + struct airoha_npu_core *core = &npu->cores[i];
> +
> + spin_lock_init(&core->lock);
> + core->npu = npu;
> +
> + irq = platform_get_irq(npu->pdev, i + 1);
> + if (irq < 0) {
> + err = irq;
> + goto error_put_dev;
> + }
This is all confusing. Why are you requesting IRQs for other - the npu -
device? That device driver is responsible for its interrupts, not you
here. This breaks encapsulation. And what do you do if the other device
starts handling interrupts on its own? This is really unexpected to see
here.
> +
> + err = devm_request_irq(&npu->pdev->dev, irq,
> + airoha_npu_wdt_handler, IRQF_SHARED,
> + "airoha-npu-wdt", core);
> + if (err)
> + goto error_put_dev;
> +
> + INIT_WORK(&core->wdt_work, airoha_npu_wdt_work);
> + }
> +
> + if (dma_set_coherent_mask(&npu->pdev->dev, 0xbfffffff))
> + dev_err(&npu->pdev->dev,
> + "failed coherent DMA configuration\n");
> +
> + err = airoha_npu_run_firmware(npu, rmem);
> + if (err)
> + goto error_put_dev;
> +
> + airoha_npu_wr(npu, REG_CR_NPU_MIB(10),
> + rmem->base + NPU_EN7581_FIRMWARE_RV32_MAX_SIZE);
> + airoha_npu_wr(npu, REG_CR_NPU_MIB(11), 0x40000); /* SRAM 256K */
> + airoha_npu_wr(npu, REG_CR_NPU_MIB(12), 0);
> + airoha_npu_wr(npu, REG_CR_NPU_MIB(21), 1);
> + msleep(100);
> +
> + /* setting booting address */
> + for (i = 0; i < AIROHA_NPU_NUM_CORES; i++)
> + airoha_npu_wr(npu, REG_CR_BOOT_BASE(i), rmem->base);
> + usleep_range(1000, 2000);
> +
> + /* enable NPU cores */
> + /* do not start core3 since it is used for WiFi offloading */
> + airoha_npu_wr(npu, REG_CR_BOOT_CONFIG, 0xf7);
> + airoha_npu_wr(npu, REG_CR_BOOT_TRIGGER, 0x1);
> + msleep(100);
> +
> + return npu;
> +
> +error_put_dev:
> + put_device(&npu->pdev->dev);
Missing platform_device_put()
> +error_of_node_put:
> + of_node_put(npu->np);
> +
> + return ERR_PTR(err);
> +}
> +
> +void airoha_npu_deinit(struct airoha_npu *npu)
> +{
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(npu->cores); i++)
> + cancel_work_sync(&npu->cores[i].wdt_work);
> +
Leaking device put.
> + put_device(&npu->pdev->dev);
> + of_node_put(npu->np);
Best regards,
Krzysztof
