[resend,PATCHv2] staging: batman-adv: remove useless addr_to_string()
Commit Message
Since all *printf() methods in the kernel understand '%pM' modifier the
conversion to the string is useless beforehand.
Additionally this patch decreases batman_if structure by 20 bytes.
Signed-off-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: Marek Lindner <lindner_marek@yahoo.de>
Cc: Simon Wunderlich <siwu@hrz.tu-chemnitz.de>
Cc: Andrew Lunn <andrew@lunn.ch>
---
drivers/staging/batman-adv/hard-interface.c | 2 --
drivers/staging/batman-adv/main.c | 5 -----
drivers/staging/batman-adv/main.h | 2 --
drivers/staging/batman-adv/originator.c | 16 ++++++----------
drivers/staging/batman-adv/routing.c | 4 ++--
drivers/staging/batman-adv/send.c | 8 ++++----
drivers/staging/batman-adv/types.h | 1 -
drivers/staging/batman-adv/vis.c | 18 ++++++------------
8 files changed, 18 insertions(+), 38 deletions(-)
Comments
On Wed, Oct 20, 2010 at 06:47:44PM +0300, Andy Shevchenko wrote:
> Since all *printf() methods in the kernel understand '%pM' modifier the
> conversion to the string is useless beforehand.
>
> Additionally this patch decreases batman_if structure by 20 bytes.
Thanks for your patch. I have problems with compiling due to other patches in
the queue. I will fix that and recommend it as patch for 2.6.38.
Best regards,
Sven
On Wed, Oct 20, 2010 at 10:51:15PM +0200, Sven Eckelmann wrote:
> On Wed, Oct 20, 2010 at 06:47:44PM +0300, Andy Shevchenko wrote:
> > Since all *printf() methods in the kernel understand '%pM' modifier the
> > conversion to the string is useless beforehand.
> >
> > Additionally this patch decreases batman_if structure by 20 bytes.
>
> Thanks for your patch. I have problems with compiling due to other patches in
> the queue. I will fix that and recommend it as patch for 2.6.38.
What do you mean by this? It applies just fine to my tree, so why can't
I take it now?
thanks,
greg k-h
On Wed, Oct 20, 2010 at 01:57:49PM -0700, Greg KH wrote:
> On Wed, Oct 20, 2010 at 10:51:15PM +0200, Sven Eckelmann wrote:
> > On Wed, Oct 20, 2010 at 06:47:44PM +0300, Andy Shevchenko wrote:
> > > Since all *printf() methods in the kernel understand '%pM' modifier the
> > > conversion to the string is useless beforehand.
> > >
> > > Additionally this patch decreases batman_if structure by 20 bytes.
> >
> > Thanks for your patch. I have problems with compiling due to other patches in
> > the queue. I will fix that and recommend it as patch for 2.6.38.
>
> What do you mean by this? It applies just fine to my tree, so why can't
> I take it now?
If you want then do so, but the stuff in batman-adv's master must be fixed so
they have to apply the v3 version of the patch and not the v2 version Andy
sent.
Best regards,
Sven
On Thu, Oct 21, 2010 at 12:16:37AM +0200, Sven Eckelmann wrote:
> On Wed, Oct 20, 2010 at 01:57:49PM -0700, Greg KH wrote:
> > On Wed, Oct 20, 2010 at 10:51:15PM +0200, Sven Eckelmann wrote:
> > > On Wed, Oct 20, 2010 at 06:47:44PM +0300, Andy Shevchenko wrote:
> > > > Since all *printf() methods in the kernel understand '%pM' modifier the
> > > > conversion to the string is useless beforehand.
> > > >
> > > > Additionally this patch decreases batman_if structure by 20 bytes.
> > >
> > > Thanks for your patch. I have problems with compiling due to other patches in
> > > the queue. I will fix that and recommend it as patch for 2.6.38.
> >
> > What do you mean by this? It applies just fine to my tree, so why can't
> > I take it now?
>
> If you want then do so, but the stuff in batman-adv's master must be fixed so
> they have to apply the v3 version of the patch and not the v2 version Andy
> sent.
That's one of the problems with having an out-of-tree tree. Please
don't do that at all anymore.
I'll go apply this patch to mine, and you can handle any merge issues if
you continue to wish to keep an external tree (hint, I STRONGLY
recommend that you do not, for these reasons and many others.)
thanks,
greg k-h
Greg KH wrote:
> On Thu, Oct 21, 2010 at 12:16:37AM +0200, Sven Eckelmann wrote:
> > On Wed, Oct 20, 2010 at 01:57:49PM -0700, Greg KH wrote:
> > > On Wed, Oct 20, 2010 at 10:51:15PM +0200, Sven Eckelmann wrote:
> > > > On Wed, Oct 20, 2010 at 06:47:44PM +0300, Andy Shevchenko wrote:
> > > > > Since all *printf() methods in the kernel understand '%pM' modifier
> > > > > the conversion to the string is useless beforehand.
> > > > >
> > > > > Additionally this patch decreases batman_if structure by 20 bytes.
> > > >
> > > > Thanks for your patch. I have problems with compiling due to other
> > > > patches in the queue. I will fix that and recommend it as patch for
> > > > 2.6.38.
> > >
> > > What do you mean by this? It applies just fine to my tree, so why
> > > can't I take it now?
> >
> > If you want then do so, but the stuff in batman-adv's master must be
> > fixed so they have to apply the v3 version of the patch and not the v2
> > version Andy sent.
>
> That's one of the problems with having an out-of-tree tree. Please
> don't do that at all anymore.
I don't see a difference in a in-tree tree and and out-of-tree tree when
applying patches somewhere else out of order. In both situations we have a
merge conflict (not that the scm says "omg, i cannot merge it" but that the
thing doesn't compile after the merge). Not that it would care much that there
is a merge conflict - it only has to be resolved one way or the other. And I
don't force the submitter to do it (I couldn't do it anyway), but try to help
to resolve it for the actual maintainer.
I always thought that even when the source is in the kernel (or in staging)
that there are still a maintainer responsible for it. That this person has to
go through the patches and look if they do whatever they claim to do and that
this isn't against what the original implementation had to do or should do.
In case of batman-adv those maintainers are called by the names Marek Lindner
and Simon Wunderlich. None of those names sound like GregKH or random guy (aka
me). I know that you are the staging maintainer and you have more to say, but
wouldn't it be more healthy that the guys who know more details about it can
take a look at the non critical stuff?
And the only reason I proposed (not forced, only recommended) that will be
send later for inclusion in 2.6.38 was that you told us that the day a new
kernel (2.6.36) gets released is too late to get new stuff (other than fixes)
in your tree for 2.6.37.
> I'll go apply this patch to mine, and you can handle any merge issues if
> you continue to wish to keep an external tree (hint, I STRONGLY
> recommend that you do not, for these reasons and many others.)
The development of batman-adv is mainly done by people which need it
externally - so out of kernel. That means if we are not allowed anymore to
have some kind of external tree that we can use, it must be done the other way
around aka compat-wireless like and without the ability to test experimental
stuff with the community unless by sending it to you and reverting it before a
new linux release is made. Otherwise we would only have a external tree which
is in another form (quilt, loose patches, ...) and would be nothing different
than what we have currently.
The current way is not to say that we come first, but to keep things
organized. Take for example Andy's patch - it was in my own opinion to late
for 2.6.37 and it was nothing real critical (but nevertheless quite helpful).
So queuing it up with the stuff for 2.6.38 seemed to be a good idea. In that
time also the affected other code parts could be fixed by the actual author
and everything flows back to you as a complete package.
And in my own opinion the external tree was quite helpful to see incorrect
merge resolutions...
Everything above is my own opinion and does not strictly reflect the positions
and policies of the batman-adv project or the actual maintainers.
thanks,
Sven
On Thu, Oct 21, 2010 at 02:41:37PM +0200, Sven Eckelmann wrote:
> Greg KH wrote:
> > On Thu, Oct 21, 2010 at 12:16:37AM +0200, Sven Eckelmann wrote:
> > > On Wed, Oct 20, 2010 at 01:57:49PM -0700, Greg KH wrote:
> > > > On Wed, Oct 20, 2010 at 10:51:15PM +0200, Sven Eckelmann wrote:
> > > > > On Wed, Oct 20, 2010 at 06:47:44PM +0300, Andy Shevchenko wrote:
> > > > > > Since all *printf() methods in the kernel understand '%pM' modifier
> > > > > > the conversion to the string is useless beforehand.
> > > > > >
> > > > > > Additionally this patch decreases batman_if structure by 20 bytes.
> > > > >
> > > > > Thanks for your patch. I have problems with compiling due to other
> > > > > patches in the queue. I will fix that and recommend it as patch for
> > > > > 2.6.38.
> > > >
> > > > What do you mean by this? It applies just fine to my tree, so why
> > > > can't I take it now?
> > >
> > > If you want then do so, but the stuff in batman-adv's master must be
> > > fixed so they have to apply the v3 version of the patch and not the v2
> > > version Andy sent.
> >
> > That's one of the problems with having an out-of-tree tree. Please
> > don't do that at all anymore.
>
> I don't see a difference in a in-tree tree and and out-of-tree tree when
> applying patches somewhere else out of order. In both situations we have a
> merge conflict (not that the scm says "omg, i cannot merge it" but that the
> thing doesn't compile after the merge).
Not true at all, the in-linux-next tree builds just fine with this
patch. In fact, it's now in linux-next already.
> I always thought that even when the source is in the kernel (or in staging)
> that there are still a maintainer responsible for it. That this person has to
> go through the patches and look if they do whatever they claim to do and that
> this isn't against what the original implementation had to do or should do.
Yes, but sometimes, especially for trivial patches, the maintainer is
routed around and patches go in through other trees.
Remember a maintainer is not someone who can say "no" to all patches
that comes in, sorry, we don't work that way.
> > I'll go apply this patch to mine, and you can handle any merge issues if
> > you continue to wish to keep an external tree (hint, I STRONGLY
> > recommend that you do not, for these reasons and many others.)
>
> The development of batman-adv is mainly done by people which need it
> externally - so out of kernel. That means if we are not allowed anymore to
> have some kind of external tree that we can use, it must be done the other way
> around aka compat-wireless like and without the ability to test experimental
> stuff with the community unless by sending it to you and reverting it before a
> new linux release is made. Otherwise we would only have a external tree which
> is in another form (quilt, loose patches, ...) and would be nothing different
> than what we have currently.
As your stuff is now in the kernel, I would recommend dropping your
external tree. It will only cause more problems in the future.
thanks,
greg k-h
Greg KH wrote:
> On Thu, Oct 21, 2010 at 02:41:37PM +0200, Sven Eckelmann wrote:
> > Greg KH wrote:
> > > On Thu, Oct 21, 2010 at 12:16:37AM +0200, Sven Eckelmann wrote:
> > > > On Wed, Oct 20, 2010 at 01:57:49PM -0700, Greg KH wrote:
> > > > > On Wed, Oct 20, 2010 at 10:51:15PM +0200, Sven Eckelmann wrote:
> > > > > > On Wed, Oct 20, 2010 at 06:47:44PM +0300, Andy Shevchenko wrote:
> > > > > > > Since all *printf() methods in the kernel understand '%pM'
> > > > > > > modifier the conversion to the string is useless beforehand.
> > > > > > >
> > > > > > > Additionally this patch decreases batman_if structure by 20
> > > > > > > bytes.
> > > > > >
> > > > > > Thanks for your patch. I have problems with compiling due to
> > > > > > other patches in the queue. I will fix that and recommend it as
> > > > > > patch for 2.6.38.
> > > > >
> > > > > What do you mean by this? It applies just fine to my tree, so why
> > > > > can't I take it now?
> > > >
> > > > If you want then do so, but the stuff in batman-adv's master must be
> > > > fixed so they have to apply the v3 version of the patch and not the
> > > > v2 version Andy sent.
> > >
> > > That's one of the problems with having an out-of-tree tree. Please
> > > don't do that at all anymore.
> >
> > I don't see a difference in a in-tree tree and and out-of-tree tree when
> > applying patches somewhere else out of order. In both situations we have
> > a merge conflict (not that the scm says "omg, i cannot merge it" but
> > that the thing doesn't compile after the merge).
>
> Not true at all, the in-linux-next tree builds just fine with this
> patch. In fact, it's now in linux-next already.
He? I never said that it breaks stuff in your staging tree.
> > I always thought that even when the source is in the kernel (or in
> > staging) that there are still a maintainer responsible for it. That this
> > person has to go through the patches and look if they do whatever they
> > claim to do and that this isn't against what the original implementation
> > had to do or should do.
>
> Yes, but sometimes, especially for trivial patches, the maintainer is
> routed around and patches go in through other trees.
>
> Remember a maintainer is not someone who can say "no" to all patches
> that comes in, sorry, we don't work that way.
What? I no batman-adv maintainer said no to patches on the lkml or other linux
related mailing lists as far as I can remember, but postponed them or
recommended changes.
There were patches dropped in the past which didn't make sense or created more
problems than they solved - but that was even before batman-adv entered
staging.
thanks,
Sven
@@ -159,8 +159,6 @@ static bool hardif_is_iface_up(struct batman_if *batman_if)
static void update_mac_addresses(struct batman_if *batman_if)
{
- addr_to_string(batman_if->addr_str, batman_if->net_dev->dev_addr);
-
memcpy(((struct batman_packet *)(batman_if->packet_buff))->orig,
batman_if->net_dev->dev_addr, ETH_ALEN);
memcpy(((struct batman_packet *)(batman_if->packet_buff))->prev_sender,
@@ -145,11 +145,6 @@ void dec_module_count(void)
module_put(THIS_MODULE);
}
-int addr_to_string(char *buff, uint8_t *addr)
-{
- return sprintf(buff, "%pM", addr);
-}
-
/* returns 1 if they are the same originator */
int compare_orig(void *data1, void *data2)
@@ -58,7 +58,6 @@
#define PACKBUFF_SIZE 2000
#define LOG_BUF_LEN 8192 /* has to be a power of 2 */
-#define ETH_STR_LEN 20
#define VIS_INTERVAL 5000 /* 5 seconds */
@@ -136,7 +135,6 @@ int mesh_init(struct net_device *soft_iface);
void mesh_free(struct net_device *soft_iface);
void inc_module_count(void);
void dec_module_count(void);
-int addr_to_string(char *buff, uint8_t *addr);
int compare_orig(void *data1, void *data2);
int choose_orig(void *data, int32_t size);
int is_my_mac(uint8_t *addr);
@@ -315,7 +315,6 @@ int orig_seq_print_text(struct seq_file *seq, void *offset)
int last_seen_secs;
int last_seen_msecs;
unsigned long flags;
- char orig_str[ETH_STR_LEN], router_str[ETH_STR_LEN];
if ((!bat_priv->primary_if) ||
(bat_priv->primary_if->if_status != IF_ACTIVE)) {
@@ -329,10 +328,10 @@ int orig_seq_print_text(struct seq_file *seq, void *offset)
net_dev->name);
}
- seq_printf(seq, "[B.A.T.M.A.N. adv %s%s, MainIF/MAC: %s/%s (%s)]\n",
+ seq_printf(seq, "[B.A.T.M.A.N. adv %s%s, MainIF/MAC: %s/%pM (%s)]\n",
SOURCE_VERSION, REVISION_VERSION_STR,
bat_priv->primary_if->net_dev->name,
- bat_priv->primary_if->addr_str, net_dev->name);
+ bat_priv->primary_if->net_dev->dev_addr, net_dev->name);
seq_printf(seq, " %-15s %s (%s/%i) %17s [%10s]: %20s ...\n",
"Originator", "last-seen", "#", TQ_MAX_VALUE, "Nexthop",
"outgoingIF", "Potential nexthops");
@@ -349,21 +348,18 @@ int orig_seq_print_text(struct seq_file *seq, void *offset)
if (orig_node->router->tq_avg == 0)
continue;
- addr_to_string(orig_str, orig_node->orig);
- addr_to_string(router_str, orig_node->router->addr);
last_seen_secs = jiffies_to_msecs(jiffies -
orig_node->last_valid) / 1000;
last_seen_msecs = jiffies_to_msecs(jiffies -
orig_node->last_valid) % 1000;
- seq_printf(seq, "%-17s %4i.%03is (%3i) %17s [%10s]:",
- orig_str, last_seen_secs, last_seen_msecs,
- orig_node->router->tq_avg, router_str,
+ seq_printf(seq, "%pM %4i.%03is (%3i) %pM [%10s]:",
+ orig_node->orig, last_seen_secs, last_seen_msecs,
+ orig_node->router->tq_avg, orig_node->router->addr,
orig_node->router->if_incoming->net_dev->name);
list_for_each_entry(neigh_node, &orig_node->neigh_list, list) {
- addr_to_string(orig_str, neigh_node->addr);
- seq_printf(seq, " %17s (%3i)", orig_str,
+ seq_printf(seq, " %pM (%3i)", neigh_node->addr,
neigh_node->tq_avg);
}
@@ -554,11 +554,11 @@ void receive_bat_packet(struct ethhdr *ethhdr,
batman_packet->orig) ? 1 : 0);
bat_dbg(DBG_BATMAN, bat_priv,
- "Received BATMAN packet via NB: %pM, IF: %s [%s] "
+ "Received BATMAN packet via NB: %pM, IF: %s [%pM] "
"(from OG: %pM, via prev OG: %pM, seqno %d, tq %d, "
"TTL %d, V %d, IDF %d)\n",
ethhdr->h_source, if_incoming->net_dev->name,
- if_incoming->addr_str, batman_packet->orig,
+ if_incoming->net_dev->dev_addr, batman_packet->orig,
batman_packet->prev_sender, batman_packet->seqno,
batman_packet->tq, batman_packet->ttl, batman_packet->version,
has_directlink_flag);
@@ -135,13 +135,13 @@ static void send_packet_to_if(struct forw_packet *forw_packet,
"Forwarding"));
bat_dbg(DBG_BATMAN, bat_priv,
"%s %spacket (originator %pM, seqno %d, TQ %d, TTL %d,"
- " IDF %s) on interface %s [%s]\n",
+ " IDF %s) on interface %s [%pM]\n",
fwd_str, (packet_num > 0 ? "aggregated " : ""),
batman_packet->orig, ntohl(batman_packet->seqno),
batman_packet->tq, batman_packet->ttl,
(batman_packet->flags & DIRECTLINK ?
"on" : "off"),
- batman_if->net_dev->name, batman_if->addr_str);
+ batman_if->net_dev->name, batman_if->net_dev->dev_addr);
buff_pos += sizeof(struct batman_packet) +
(batman_packet->num_hna * ETH_ALEN);
@@ -186,12 +186,12 @@ static void send_packet(struct forw_packet *forw_packet)
/* FIXME: what about aggregated packets ? */
bat_dbg(DBG_BATMAN, bat_priv,
"%s packet (originator %pM, seqno %d, TTL %d) "
- "on interface %s [%s]\n",
+ "on interface %s [%pM]\n",
(forw_packet->own ? "Sending own" : "Forwarding"),
batman_packet->orig, ntohl(batman_packet->seqno),
batman_packet->ttl,
forw_packet->if_incoming->net_dev->name,
- forw_packet->if_incoming->addr_str);
+ forw_packet->if_incoming->net_dev->dev_addr);
/* skb is only used once and than forw_packet is free'd */
send_skb_packet(forw_packet->skb, forw_packet->if_incoming,
@@ -37,7 +37,6 @@ struct batman_if {
struct list_head list;
int16_t if_num;
char if_status;
- char addr_str[ETH_STR_LEN];
struct net_device *net_dev;
atomic_t seqno;
atomic_t frag_seqno;
@@ -130,15 +130,13 @@ static ssize_t vis_data_read_prim_sec(char *buff, struct hlist_head *if_list)
{
struct if_list_entry *entry;
struct hlist_node *pos;
- char tmp_addr_str[ETH_STR_LEN];
size_t len = 0;
hlist_for_each_entry(entry, pos, if_list, list) {
if (entry->primary)
len += sprintf(buff + len, "PRIMARY, ");
else {
- addr_to_string(tmp_addr_str, entry->addr);
- len += sprintf(buff + len, "SEC %s, ", tmp_addr_str);
+ len += sprintf(buff + len, "SEC %pM, ", entry->addr);
}
}
@@ -165,14 +163,12 @@ static size_t vis_data_count_prim_sec(struct hlist_head *if_list)
static ssize_t vis_data_read_entry(char *buff, struct vis_info_entry *entry,
uint8_t *src, bool primary)
{
- char to[18];
-
/* maximal length: max(4+17+2, 3+17+1+3+2) == 26 */
- addr_to_string(to, entry->dest);
if (primary && entry->quality == 0)
- return sprintf(buff, "HNA %s, ", to);
+ return sprintf(buff, "HNA %pM, ", entry->dest);
else if (compare_orig(entry->src, src))
- return sprintf(buff, "TQ %s %d, ", to, entry->quality);
+ return sprintf(buff, "TQ %pM %d, ", entry->dest,
+ entry->quality);
return 0;
}
@@ -190,7 +186,6 @@ int vis_seq_print_text(struct seq_file *seq, void *offset)
struct if_list_entry *entry;
struct hlist_node *pos, *n;
int i;
- char tmp_addr_str[ETH_STR_LEN];
unsigned long flags;
int vis_server = atomic_read(&bat_priv->vis_mode);
size_t buff_pos, buf_size;
@@ -255,9 +250,8 @@ int vis_seq_print_text(struct seq_file *seq, void *offset)
}
hlist_for_each_entry(entry, pos, &vis_if_list, list) {
- addr_to_string(tmp_addr_str, entry->addr);
- buff_pos += sprintf(buff + buff_pos, "%s,",
- tmp_addr_str);
+ buff_pos += sprintf(buff + buff_pos, "%pM,",
+ entry->addr);
for (i = 0; i < packet->entries; i++)
buff_pos += vis_data_read_entry(buff + buff_pos,