From: Arvid Brodin <Arvid.Brodin@xdin•com>
To: Joe Perches <joe@perches•com>
Cc: "netdev@vger•kernel.org" <netdev@vger•kernel.org>,
Stephen Hemminger <shemminger@vyatta•com>,
Alexey Kuznetsov <kuznet@ms2•inr.ac.ru>,
Javier Boticario <jboticario@gmail•com>,
Bruno Ferreira <balferreira@googlemail•com>
Subject: Re: [RFC v2 1/2] net/hsr: Add support for IEC 62439-3 High-availability Seamless Redundancy
Date: Wed, 4 Jul 2012 22:02:16 +0000 [thread overview]
Message-ID: <4FF4BD68.1090304@xdin.com> (raw)
In-Reply-To: <1341361824.1993.16.camel@joe2Laptop>
On 2012-07-04 02:30, Joe Perches wrote:
> On Wed, 2012-07-04 at 00:12 +0000, Arvid Brodin wrote:
>> The kernel patch.
>
> []
What does this mean (the "[]")?
>
>> diff --git a/net/hsr/hsr_device.c b/net/hsr/hsr_device.c
>
>> @@ -0,0 +1,531 @@
>
>> +static int is_admin_up(struct net_device *dev)
>> +{
>> + return (dev->flags & IFF_UP);
>> +}
>> +
>> +static int is_operstate_up(struct net_device *dev)
>> +{
>> + return (dev->operstate == IF_OPER_UP);
>> +}
>
> bool?
Yep, didn't know the bool type existed.
>
>> +static void __hsr_set_operstate(struct net_device *dev, int transition)
>> +{
>> + if (dev->operstate != transition) {
>> +/*
>> + switch (transition) {
>> + case IF_OPER_UP:
>> + printk(KERN_INFO "%s: new operstate is IF_OPER_UP\n", dev->name);
>
> netdev_info(dev, "new operstate is IF_OPER_UP\n");
>
>> + break;
>> + default:
>> + printk(KERN_INFO "%s: new operstate is !IF_OPER_UP (%d)\n", dev->name, transition);
>
> etc.
>
>> +void hsr_set_operstate(struct net_device *hsr_dev, struct net_device *slave1,
>> + struct net_device *slave2)
>> +{
>> + if (!is_admin_up(hsr_dev)) {
>> + __hsr_set_operstate(hsr_dev, IF_OPER_DOWN);
>> + return;
>> + }
>> +/*
>> + printk(KERN_INFO "Slave1/2 operstate: %d/%d\n",
>> + slave1->operstate, slave2->operstate);
>> +*/
>
> Please remove commented out code.
I intended to do so when I send the patch of course. I didn't know it would be so frowned
upon in a RFC, I thought it would be enough to note the existence of the commented out
code under known problems, as I did in RFC part 0. Lesson learned!
>
>> +static void restore_slaves(struct net_device *hsr_dev)
>> +{
>> + struct hsr_priv *hsr_priv;
>> + struct net_device *slave[2];
>> + int i;
>> + int res;
>> +
>> + hsr_priv = netdev_priv(hsr_dev);
>> + for (i = 0; i < 2; i++)
>> + slave[i] = hsr_priv->slave_data[i].dev;
>> +
>> + rtnl_lock();
>> +
>> + /* Restore promiscuity */
>> + for (i = 0; i < 2; i++) {
>> + if (!hsr_priv->slave_data[i].promisc)
>> + continue;
>> + res = dev_set_promiscuity(slave[i],
>> + -hsr_priv->slave_data[i].promisc);
>> + if (res)
>> + pr_info("HSR: Cannot restore promiscuity (%s, %d)\n",
>> + slave[i]->name,
>> + res);
>
> shouldn't this just be:
>
> netdev_info(slave[i], "cannot restore promiscuity: %d\n",
> res);
>
> If you must use pr_<level> please add
> #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> before any include and let the printk subsystem
> add MODNAME as a prefix.
>
>
>> +static int check_slave_ok(struct net_device *dev)
>> +{
>> + /* Don't allow HSR on non-ethernet like devices */
>> + if ((dev->flags & IFF_LOOPBACK) || (dev->type != ARPHRD_ETHER) ||
>> + (dev->addr_len != ETH_ALEN)) {
>> + pr_info("%s: Cannot enslave loopback or non-ethernet device\n",
>> + dev->name);
>
> netdev_info(dev, "Cannot enslave...");
>
>> + return -EINVAL;
>> + }
>> +
>> + /* Don't allow enslaving hsr devices */
>> + if (is_hsr_master(dev)) {
>> + pr_info("%s: Don't try to create trees of hsr devices!\n",
>> + dev->name);
>
>
> netdev_err(dev, "Cannot create trees of hsr devices\n");
>
>
>> +int hsr_dev_finalize(struct net_device *hsr_dev, struct net_device *slave[2])
>> +{
>
>> + /* Set hsr_dev's MAC address to that of mac_slave1 */
>> + memcpy(hsr_dev->dev_addr, hsr_priv->slave_data[0].dev->dev_addr,
>> + hsr_dev->addr_len);
>
> ETH_ALEN?
>
>> diff --git a/net/hsr/hsr_device.h b/net/hsr/hsr_device.h
> []
>> @@ -0,0 +1,27 @@
>
>> +void hsr_dev_setup(struct net_device *dev);
>> +int hsr_dev_finalize(struct net_device *hsr_dev, struct net_device *slave[2]);
>> +void hsr_set_operstate(struct net_device *hsr_dev, struct net_device *slave1,
>> + struct net_device *slave2);
>
> please align arguments immediately after the open parenthesis.
>
>> diff --git a/net/hsr/hsr_framereg.c b/net/hsr/hsr_framereg.c
> []
>> +static struct node_entry *find_node_by_AddrA(struct list_head *node_db,
>> + unsigned char addr[ETH_ALEN])
>
> static struct node_entry *find_node_by_AddrA(struct list_head *node_db,
> unsigned char addr[ETH_ALEN])
> []
>> +static struct node_entry *find_node_by_AddrB(struct list_head *node_db,
>> + unsigned char addr[ETH_ALEN])
>
> Alignment...
>
>> +int framereg_merge_node(struct hsr_priv *hsr_priv, enum hsr_dev_idx dev_idx,
>> + struct sk_buff *skb)
>> +{
> []
>> + node = find_node_by_AddrA(&hsr_priv->node_db, hsr_stag->MacAddressA);
>> + if (!node) {
>> + rcu_read_unlock();
>> + found = 0;
>> + node = kmalloc(sizeof(*node), GFP_ATOMIC);
>
> why GFP_ATOMIC?
This function is (indirectly) called by the receive callback for packet type ETH_P_HSR
(hsr_rcv() in hsr_main.c). If I recall correctly, I tried GFP_KERNEL first but the kernel
complained over sleeping in atomic context. I'll check it out again.
>
>> + if (!node)
>> + return -ENOMEM;
>> +
>> + memcpy(node->MacAddressA, hsr_stag->MacAddressA, ETH_ALEN);
>> + memcpy(node->MacAddressB, ethhdr->h_source, ETH_ALEN);
>> +
>> + for (i = 0; i < HSR_MAX_SLAVE; i++)
>> + node->time_in[i] = 0;
>> + for (i = 0; i < HSR_MAX_DEV; i++)
>> + node->seq_out[i] = 0;
>> +/*
>> + printk(KERN_INFO "HSR: Added node %pM / %pM\n",
>> + node->MacAddressA,
>> + node->MacAddressB);
>> +*/
>
> Please remove commented out code here and everywhere else...
>
> [too long, stopped reading]
>
Thank you for your time. I will take care of these issues when I get back from my vacation. :)
--
Arvid Brodin | Consultant (Linux)
XDIN AB | Jan Stenbecks Torg 17 | SE-164 40 Kista | Sweden | xdin.com
next prev parent reply other threads:[~2012-07-04 22:02 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-07-04 0:12 [RFC v2 1/2] net/hsr: Add support for IEC 62439-3 High-availability Seamless Redundancy Arvid Brodin
2012-07-04 0:30 ` Joe Perches
2012-07-04 22:02 ` Arvid Brodin [this message]
2012-08-16 19:12 ` [RFC v3 0/1] " Arvid Brodin
2012-08-16 19:17 ` [RFC v3 1/1] " Arvid Brodin
2012-08-16 20:30 ` David Miller
2012-08-16 21:16 ` Arvid Brodin
2012-08-16 21:46 ` David Miller
2012-10-12 17:11 ` [RFC v4 1/1] net/hsr: Support for the HSR protocol (IEC:2010 / HSR v0) Arvid Brodin
2012-10-12 18:57 ` Stephen Hemminger
2012-10-12 21:39 ` Joe Perches
2012-07-04 4:20 ` [RFC v2 1/2] net/hsr: Add support for IEC 62439-3 High-availability Seamless Redundancy Stephen Hemminger
2012-07-04 22:34 ` Arvid Brodin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4FF4BD68.1090304@xdin.com \
--to=arvid.brodin@xdin$(echo .)com \
--cc=balferreira@googlemail$(echo .)com \
--cc=jboticario@gmail$(echo .)com \
--cc=joe@perches$(echo .)com \
--cc=kuznet@ms2$(echo .)inr.ac.ru \
--cc=netdev@vger$(echo .)kernel.org \
--cc=shemminger@vyatta$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox