| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <Z6t7SuFurbGwJjt_@lore-desk>
Date: Tue, 11 Feb 2025 17:31:06 +0100
From: Lorenzo Bianconi <lorenzo@...nel.org>
To: Krzysztof Kozlowski <krzk@...nel.org>
Cc: Andrew Lunn <andrew+netdev@...n.ch>,
"David S. Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
Felix Fietkau <nbd@....name>, Sean Wang <sean.wang@...iatek.com>,
Matthias Brugger <matthias.bgg@...il.com>,
AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>,
Philipp Zabel <p.zabel@...gutronix.de>,
Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley <conor+dt@...nel.org>,
"Chester A. Unal" <chester.a.unal@...nc9.com>,
Daniel Golle <daniel@...rotopia.org>,
DENG Qingfang <dqfext@...il.com>, Andrew Lunn <andrew@...n.ch>,
Vladimir Oltean <olteanv@...il.com>, netdev@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org,
linux-mediatek@...ts.infradead.org, devicetree@...r.kernel.org,
upstream@...oha.com
Subject: Re: [PATCH net-next v3 13/16] net: airoha: Introduce PPE
initialization via NPU
> 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?
similar to mtk_wed device, airoha_npu is not modeled as a standalone driver,
but it is part of the airoha_eth driver. If you think it is better, I can
rework it implementing a dedicated driver for it. What do you think?
>
> This is also still unnecessary ABI break without explanation/reasoning.
At the moment if airoha_npu_init() fails (e.g. if the npu node is not present),
it will not cause any failure in airoha_hw_init() (so in the core ethernet
driver probing).
>
> > +
> > + 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).
thx for the pointer, I will take a look to it.
>
> > +
> > + get_device(&npu->pdev->dev);
>
> Why? of_find_device_by_node() does it.
ack, I will fix 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.
As pointed out above, there is no other driver for airoha_npu at the moment,
but I am fine to implement it.
Regards,
Lorenzo
>
> > +
> > + 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
>
Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)
Powered by blists - more mailing lists