linux-kernel - Re: [PATCH] net: ethernet: stmmac: add ARP management

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 18 Jan 2017 01:14:08 +0200
From:   Andy Shevchenko <andy.shevchenko@...il.com>
To:     Christophe Roullier <christophe.roullier@...com>
Cc:     Alexandre Torgue <alexandre.torgue@...com>,
        Giuseppe Cavallaro <peppe.cavallaro@...com>,
        netdev <netdev@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] net: ethernet: stmmac: add ARP management

On Tue, Jan 17, 2017 at 6:56 PM, Christophe Roullier
<christophe.roullier@...com> wrote:

> +static int dwmac4_arp_enable(struct mac_device_info *hw)
> +{
> +       void __iomem *ioaddr = hw->pcsr;

__iomem *config = hw->pcsr + GMAC_CONFIG;

> +       u32 value = readl(ioaddr + GMAC_CONFIG);
> +
> +       value |= GMAC_CONFIG_ARPEN;
> +
> +       writel(value, ioaddr + GMAC_CONFIG);

u32 value;

value = readl();
writel(value | ...);

?

> +
> +       value = readl(ioaddr + GMAC_CONFIG);
> +
> +       return !!(value & GMAC_CONFIG_ARPEN);
> +}

> +/* Set ARP Address */
> +static void dwmac4_set_arp_addr(void __iomem *ioaddr, bool set, u32 addr)
> +{

__iomem *arp_addr = ioaddr + GMAC_ARP_ADDR;

> +       u32 value;
> +
> +       value = readl(ioaddr + GMAC_ARP_ADDR);

Care to explain why do you need dummy readl() here?

> +
> +       if (set) {
> +               /* set arp address */
> +               value = addr;
> +       } else {
> +               /* unset arp address */
> +               value = 0;
> +       }

value = set ? addr : 0;


> +
> +       writel(value, ioaddr + GMAC_ARP_ADDR);
> +       value = readl(ioaddr + GMAC_ARP_ADDR);
> +}


> +       if ((priv->plat->arp_en) && (priv->dma_cap.arpoffsel)) {
> +               ret = priv->hw->mac->arp_en(priv->hw);
> +               if (!ret) {

Hmm... Most would expect

if (ret) {
 doing something
} else {
 doing something else
}

> +                       pr_warn(" ARP feature disabled\n");
> +               } else {

> +                       pr_info(" ARP feature enabled\n");

Wouldn't be too noisy?

pr_* -> dev_*

> +                       /* Copy MAC addr into MAC_ARP_ADDRESS register*/
> +                       priv->hw->dma->set_arp_addr(priv->ioaddr, 1,
> +                                                   priv->dev->dev_addr);
> +               }
> +       }

-- 
With Best Regards,
Andy Shevchenko