> On Sat, Jun 29, 2024 at 05:01:38PM +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@xxxxxxxxxx>
> > Signed-off-by: Lorenzo Bianconi <lorenzo@xxxxxxxxxx>
>
> Hi Lorenzo,
>
> Some minor feedback from my side.
Hi Simon,
>
> > +static void airoha_qdma_set_irqmask(struct airoha_eth *eth, int index,
> > + u32 clear, u32 set)
> > +{
> > + unsigned long flags;
> > +
> > + if (WARN_ON_ONCE(index >= ARRAY_SIZE(eth->irqmask)))
> > + return;
> > +
> > + spin_lock_irqsave(ð->irq_lock, flags);
> > +
> > + eth->irqmask[index] &= ~clear;
> > + eth->irqmask[index] |= set;
> > + airoha_qdma_wr(eth, REG_INT_ENABLE(index), eth->irqmask[index]);
> > + /* Read irq_enable register in order to guarantee the update above
> > + * completes in the spinlock critical section.
> > + */
> > + airoha_rr(eth, REG_INT_ENABLE(index));
>
> airoha_rr() expects an __iomem pointer as it's first argument,
> but the type of eth is struct airoha_eth *eth.
>
> Should this be using airoha_qdma_rr() instead?
ack, right. Thx for pointing this out. I will fix it in v5.
>
> Flagged by Sparse.
>
> > +
> > + spin_unlock_irqrestore(ð->irq_lock, flags);
> > +}
>
> ...
>
> > +static void airoha_ethtool_get_strings(struct net_device *dev, u32 sset,
> > + u8 *data)
> > +{
> > + int i;
> > +
> > + if (sset != ETH_SS_STATS)
> > + return;
> > +
> > + for (i = 0; i < ARRAY_SIZE(airoha_ethtool_stats_name); i++) {
> > + memcpy(data + i * ETH_GSTRING_LEN,
> > + airoha_ethtool_stats_name[i], ETH_GSTRING_LEN);
> > + }
> > +
> > + data += ETH_GSTRING_LEN * ARRAY_SIZE(airoha_ethtool_stats_name);
> > + page_pool_ethtool_stats_get_strings(data);
> > +}
>
> W=1 allmodconfig builds on x86_64 with gcc-13 complain about the use
> of memcpy above because the source is (often?) less than ETH_GSTRING_LEN
> bytes long.
>
> I think the preferred solution is to use ethtool_puts(),
> something like this (compile tested only!):
>
> @@ -2291,12 +2291,9 @@ static void airoha_ethtool_get_strings(struct net_device *dev, u32 sset,
> if (sset != ETH_SS_STATS)
> return;
>
> - for (i = 0; i < ARRAY_SIZE(airoha_ethtool_stats_name); i++) {
> - memcpy(data + i * ETH_GSTRING_LEN,
> - airoha_ethtool_stats_name[i], ETH_GSTRING_LEN);
> - }
> + for (i = 0; i < ARRAY_SIZE(airoha_ethtool_stats_name); i++)
> + ethtool_puts(&data, airoha_ethtool_stats_name[i]);
>
> - data += ETH_GSTRING_LEN * ARRAY_SIZE(airoha_ethtool_stats_name);
> page_pool_ethtool_stats_get_strings(data);
> }
>
ack, I will fix it in v5.
>
> ...
>
> > +static int airoha_alloc_gdm_port(struct airoha_eth *eth, struct device_node *np)
> > +{
> > + const __be32 *id_ptr = of_get_property(np, "reg", NULL);
> > + struct net_device *dev;
> > + struct airoha_gdm_port *port;
>
> nit: reverse xmas tree
ack, I will fix it in v5.
Regards,
Lorenzo
>
> > + int err, index;
> > + u32 id;
>
> ...
>
> --
> pw-bot: changes-requested
Attachment:
signature.asc
Description: PGP signature
