| 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
| ||
|
Date: Fri, 28 Jun 2024 12:02:50 +0200
From: Lorenzo Bianconi <lorenzo@...nel.org>
To: Simon Horman <horms@...nel.org>
Cc: netdev@...r.kernel.org, nbd@....name, lorenzo.bianconi83@...il.com,
davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
pabeni@...hat.com, conor@...nel.org,
linux-arm-kernel@...ts.infradead.org, robh+dt@...nel.org,
krzysztof.kozlowski+dt@...aro.org, conor+dt@...nel.org,
devicetree@...r.kernel.org, catalin.marinas@....com,
will@...nel.org, upstream@...oha.com,
angelogioacchino.delregno@...labora.com,
benjamin.larsson@...exis.eu, rkannoth@...vell.com,
sgoutham@...vell.com, andrew@...n.ch
Subject: Re: [PATCH v3 net-next 2/2] net: airoha: Introduce ethernet support
for EN7581 SoC
> On Sun, Jun 23, 2024 at 06:19:57PM +0200, Lorenzo Bianconi wrote:
> > Add airoha_eth driver in order to introduce ethernet support for
> > Airoha EN7581 SoC available on EN7581 development board (en7581-evb).
> > en7581-evb networking architecture is composed by airoha_eth as mac
> > controller (cpu port) and a mt7530 dsa based switch.
> > EN7581 mac controller is mainly composed by Frame Engine (FE) and
> > QoS-DMA (QDMA) modules. FE is used for traffic offloading (just basic
> > functionalities are supported now) while QDMA is used for DMA operation
> > and QOS functionalities between mac layer and the dsa switch (hw QoS is
> > not available yet and it will be added in the future).
> > Currently only hw lan features are available, hw wan will be added with
> > subsequent patches.
> >
> > Tested-by: Benjamin Larsson <benjamin.larsson@...exis.eu>
> > Signed-off-by: Lorenzo Bianconi <lorenzo@...nel.org>
>
> Hi Lorenzo,
>
> Some minor nits from my side.
Hi Simon,
thx for the review.
>
> ...
>
> > diff --git a/drivers/net/ethernet/mediatek/airoha_eth.c b/drivers/net/ethernet/mediatek/airoha_eth.c
>
> ...
>
> > +#define airoha_fe_rr(eth, offset) airoha_rr((eth)->fe_regs, (offset))
> > +#define airoha_fe_wr(eth, offset, val) airoha_wr((eth)->fe_regs, (offset), (val))
> > +#define airoha_fe_rmw(eth, offset, mask, val) airoha_rmw((eth)->fe_regs, (offset), (mask), (val))
> > +#define airoha_fe_set(eth, offset, val) airoha_rmw((eth)->fe_regs, (offset), 0, (val))
> > +#define airoha_fe_clear(eth, offset, val) airoha_rmw((eth)->fe_regs, (offset), (val), 0)
> > +
> > +#define airoha_qdma_rr(eth, offset) airoha_rr((eth)->qdma_regs, (offset))
> > +#define airoha_qdma_wr(eth, offset, val) airoha_wr((eth)->qdma_regs, (offset), (val))
> > +#define airoha_qdma_rmw(eth, offset, mask, val) airoha_rmw((eth)->qdma_regs, (offset), (mask), (val))
> > +#define airoha_qdma_set(eth, offset, val) airoha_rmw((eth)->qdma_regs, (offset), 0, (val))
> > +#define airoha_qdma_clear(eth, offset, val) airoha_rmw((eth)->qdma_regs, (offset), (val), 0)
>
> nit: Please consider line-wrapping the above so lines are 80 columns wide
> or less, which is still preferred in Networking code.
>
> Flagged by checkpatch.pl --max-line-length=80
ack, I will fix it in v4.
>
> ...
>
> > +static netdev_tx_t airoha_dev_xmit(struct sk_buff *skb,
> > + struct net_device *dev)
> > +{
> > + struct skb_shared_info *sinfo = skb_shinfo(skb);
> > + struct airoha_eth *eth = netdev_priv(dev);
> > + int i, qid = skb_get_queue_mapping(skb);
> > + u32 nr_frags, msg0 = 0, msg1;
> > + u32 len = skb_headlen(skb);
> > + struct netdev_queue *txq;
> > + struct airoha_queue *q;
> > + void *data = skb->data;
> > + u16 index;
> > +
> > + if (skb->ip_summed == CHECKSUM_PARTIAL)
> > + msg0 |= FIELD_PREP(QDMA_ETH_TXMSG_TCO_MASK, 1) |
> > + FIELD_PREP(QDMA_ETH_TXMSG_UCO_MASK, 1) |
> > + FIELD_PREP(QDMA_ETH_TXMSG_ICO_MASK, 1);
> > +
> > + /* TSO: fill MSS info in tcp checksum field */
> > + if (skb_is_gso(skb)) {
> > + if (skb_cow_head(skb, 0))
> > + goto error;
> > +
> > + if (sinfo->gso_type & (SKB_GSO_TCPV4 | SKB_GSO_TCPV6)) {
> > + tcp_hdr(skb)->check = cpu_to_be16(sinfo->gso_size);
>
> Probably we could do better with an appropriate helper - perhaps
> there is one I couldn't find one - but I think you need a cast here
> to keep Sparse happy.
>
> Something like this (completely untested!):
>
> tcp_hdr(skb)->check =
> (__force __sum16)cpu_to_be16(sinfo->gso_size);
>
> ...
ack, I will fix it in v4.
>
> > +static int airoha_probe(struct platform_device *pdev)
> > +{
> > + struct device_node *np = pdev->dev.of_node;
> > + struct net_device *dev;
> > + struct airoha_eth *eth;
> > + int err;
> > +
> > + dev = devm_alloc_etherdev_mqs(&pdev->dev, sizeof(*eth),
> > + AIROHA_NUM_TX_RING, AIROHA_NUM_RX_RING);
> > + if (!dev) {
> > + dev_err(&pdev->dev, "alloc_etherdev failed\n");
> > + return -ENOMEM;
> > + }
> > +
> > + eth = netdev_priv(dev);
> > + eth->net_dev = dev;
> > +
> > + err = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));
> > + if (err) {
> > + dev_err(&pdev->dev, "failed configuring DMA mask\n");
> > + return err;
> > + }
> > +
> > + eth->fe_regs = devm_platform_ioremap_resource_byname(pdev, "fe");
> > + if (IS_ERR(eth->fe_regs))
> > + return dev_err_probe(&pdev->dev, PTR_ERR(eth->fe_regs),
> > + "failed to iomap fe regs\n");
> > +
> > + eth->qdma_regs = devm_platform_ioremap_resource_byname(pdev, "qdma0");
> > + if (IS_ERR(eth->qdma_regs))
> > + return dev_err_probe(&pdev->dev, PTR_ERR(eth->qdma_regs),
> > + "failed to iomap qdma regs\n");
> > +
> > + eth->rsts[0].id = "fe";
> > + eth->rsts[1].id = "pdma";
> > + eth->rsts[2].id = "qdma";
> > + err = devm_reset_control_bulk_get_exclusive(&pdev->dev,
> > + ARRAY_SIZE(eth->rsts),
> > + eth->rsts);
> > + if (err) {
> > + dev_err(&pdev->dev, "failed to get bulk reset lines\n");
> > + return err;
> > + }
> > +
> > + eth->xsi_rsts[0].id = "xsi-mac";
> > + eth->xsi_rsts[1].id = "hsi0-mac";
> > + eth->xsi_rsts[2].id = "hsi1-mac";
> > + eth->xsi_rsts[3].id = "hsi-mac";
> > + eth->xsi_rsts[4].id = "xfp-mac";
> > + err = devm_reset_control_bulk_get_exclusive(&pdev->dev,
> > + ARRAY_SIZE(eth->xsi_rsts),
> > + eth->xsi_rsts);
> > + if (err) {
> > + dev_err(&pdev->dev, "failed to get bulk xsi reset lines\n");
> > + return err;
> > + }
> > +
> > + spin_lock_init(ð->irq_lock);
> > + eth->irq = platform_get_irq(pdev, 0);
> > + if (eth->irq < 0) {
> > + dev_err(&pdev->dev, "failed reading irq line\n");
>
> Coccinelle says:
>
> .../airoha_eth.c:1698:2-9: line 1698 is redundant because platform_get_irq() already prints an error
>
> ...
ack, I will fix it in v4.
>
> > +const struct of_device_id of_airoha_match[] = {
> > + { .compatible = "airoha,en7581-eth" },
> > + { /* sentinel */ }
> > +};
>
> of_airoha_match appears to only be used in this file.
> If so, it should be static.
>
> Flagged by Sparse.
ack, I will fix it in v4.
>
> > +
> > +static struct platform_driver airoha_driver = {
> > + .probe = airoha_probe,
> > + .remove_new = airoha_remove,
> > + .driver = {
> > + .name = KBUILD_MODNAME,
> > + .of_match_table = of_airoha_match,
> > + },
> > +};
> > +module_platform_driver(airoha_driver);
> > +
> > +MODULE_LICENSE("GPL");
> > +MODULE_AUTHOR("Lorenzo Bianconi <lorenzo@...nel.org>");
> > +MODULE_DESCRIPTION("Ethernet driver for Airoha SoC");
>
> > diff --git a/drivers/net/ethernet/mediatek/airoha_eth.h b/drivers/net/ethernet/mediatek/airoha_eth.h
> > new file mode 100644
> > index 000000000000..f7b984be4d60
> > --- /dev/null
> > +++ b/drivers/net/ethernet/mediatek/airoha_eth.h
> > @@ -0,0 +1,793 @@
> > +// SPDX-License-Identifier: GPL-2.0
>
> The correct SPDX header comment style for .h (but not .c) files is /* ... */
>
> https://docs.kernel.org/6.9/process/license-rules.html#license-identifier-syntax
>
> Flagged by checkpatch
ack, I will fix it in v4.
Regards,
Lorenzo
>
> ...
Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)
Powered by blists - more mailing lists