netdev - Re: [PATCH v6] ethernet/arc/arc_emac

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] [day] [month] [year] [list]

Date:	Fri, 21 Jun 2013 07:05:44 +0000
From:	Alexey Brodkin <Alexey.Brodkin@...opsys.com>
To:	Andy Shevchenko <andy.shevchenko@...il.com>
CC:	netdev <netdev@...r.kernel.org>,
	Francois Romieu <romieu@...zoreil.com>,
	Joe Perches <joe@...ches.com>,
	Vineet Gupta <Vineet.Gupta1@...opsys.com>,
	Mischa Jonker <Mischa.Jonker@...opsys.com>,
	Arnd Bergmann <arnd@...db.de>,
	Grant Likely <grant.likely@...aro.org>,
	Rob Herring <rob.herring@...xeda.com>,
	Paul Gortmaker <paul.gortmaker@...driver.com>,
	"David S. Miller" <davem@...emloft.net>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Devicetree Discuss <devicetree-discuss@...ts.ozlabs.org>,
	Florian Fainelli <florian@...nwrt.org>
Subject: Re: [PATCH v6] ethernet/arc/arc_emac - Add new driver

On 06/20/2013 08:00 PM, Andy Shevchenko wrote:
> On Thu, Jun 20, 2013 at 2:53 PM, Alexey Brodkin
>> +               if ((info & FOR_EMAC) || !txbd->data)
>
> Redundant internal braces. Up to you.

I left them so priorities of operations will be obvious.

>> +#define FRST_OR_LAST_MASK      (FRST_MASK | LAST_MASK)
>
> You may re-use it in *_tx_clean() above.
> Also, please use full English word FIRST. Actually is there any
> conflicts with existing drivers? Should it be with [ARC_]EMAC prefix?

Re-used it in 2 mentioned places. And changed FRST with FIRST.
I don't see defines with the same name anywhere else.

>
>> +               /* Make sure pointer to data buffer is set */
>> +               mb();
>
> I'm not so familiar with memory barriers, but is this somehow differ
> from smp_mb()?
> Which one suits better? And what about the other places in this file?

Since for now driver itself is not SMP-safe so I don't see any reason 
for "smp_" barriers. Later I plan to add SMP compatibility and that this 
point barriers will be replaced as well.

Another thing is I switched to "wmb()" (which is a barrier fror write in 
particular) instead of generic "mb()".

>> +       /* Allocate Tx BD's similar to Rx BD's. All Tx BD's owned by CPU */
>> +       bd = priv->txbd;
>> +       for (i = 0; i < TX_BD_NUM; i++) {
>> +               bd->data = 0;
>> +               bd->info = 0;
>> +               bd++;
>> +       }
>
> Looks like
> memset(priv->txbd, 0, siezeof(*priv->txbd) * TX_BD_NUM);

Correct.

>> +       /* Get "info" of the next BD */
>
>> +       /* Get PHY from device tree */
>> +       phy_node = of_parse_phandle(pdev->dev.of_node, "phy", 0);
>
>> +       /* Get EMAC registers base address from device tree */
>> +       err = of_address_to_resource(pdev->dev.of_node, 0, &res_regs);
>
>> +       /* Get CPU clock frequency from device tree */
>> +       if (of_property_read_u32(pdev->dev.of_node, "clock-frequency",
>> +                                &clock_frequency)) {
>
>> +       /* Get IRQ from device tree */
>> +       err = of_irq_to_resource(pdev->dev.of_node, 0, &res_irq);
>
> So, you don't like the idea to get as much as possible before
> alloc_netdev(), don't you?

Ok, I did this change)))

Regards,
Alexey
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html