[PATCH bluetooth-next] mac802154: rename seq to sequence_number

[PATCH bluetooth-next] mac802154: rename seq to sequence_number

[Varka Bhadram]

This patch rename ieee802154_hdr member seq to sequence_number.

Signed-off-by: Varka Bhadram <varkab@cdac.in>
---
 include/net/ieee802154_netdev.h |    2 +-
 net/ieee802154/header_ops.c     |    2 +-
 net/mac802154/iface.c           |    2 +-
 net/mac802154/rx.c              |    2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/include/net/ieee802154_netdev.h b/include/net/ieee802154_netdev.h
index 2c10a9f..9c95a74 100644
--- a/include/net/ieee802154_netdev.h
+++ b/include/net/ieee802154_netdev.h
@@ -88,7 +88,7 @@ struct ieee802154_hdr_fc {
 
 struct ieee802154_hdr {
 	struct ieee802154_hdr_fc fc;
-	u8 seq;
+	u8 sequence_number;
 	struct ieee802154_addr source;
 	struct ieee802154_addr dest;
 	struct ieee802154_sechdr sec;
diff --git a/net/ieee802154/header_ops.c b/net/ieee802154/header_ops.c
index a051b69..9c3291b 100644
--- a/net/ieee802154/header_ops.c
+++ b/net/ieee802154/header_ops.c
@@ -90,7 +90,7 @@ ieee802154_hdr_push(struct sk_buff *skb, const struct ieee802154_hdr *hdr)
 	int rc;
 	struct ieee802154_hdr_fc fc = hdr->fc;
 
-	buf[pos++] = hdr->seq;
+	buf[pos++] = hdr->sequence_number;
 
 	fc.dest_addr_mode = hdr->dest.mode;
 
diff --git a/net/mac802154/iface.c b/net/mac802154/iface.c
index 416de90..0bc0f44 100644
--- a/net/mac802154/iface.c
+++ b/net/mac802154/iface.c
@@ -372,7 +372,7 @@ static int mac802154_header_create(struct sk_buff *skb,
 	hdr.fc.type = cb->type;
 	hdr.fc.security_enabled = cb->secen;
 	hdr.fc.ack_request = cb->ackreq;
-	hdr.seq = atomic_inc_return(&dev->ieee802154_ptr->dsn) & 0xFF;
+	hdr.sequence_number = atomic_inc_return(&dev->ieee802154_ptr->dsn) & 0xFF;
 
 	if (mac802154_set_header_security(sdata, &hdr, cb) < 0)
 		return -EINVAL;
diff --git a/net/mac802154/rx.c b/net/mac802154/rx.c
index 5a258c1..48781a6 100644
--- a/net/mac802154/rx.c
+++ b/net/mac802154/rx.c
@@ -142,7 +142,7 @@ ieee802154_parse_frame_start(struct sk_buff *skb, struct ieee802154_hdr *hdr)
 	skb->mac_len = hlen;
 
 	pr_debug("fc: %04x dsn: %02x\n", le16_to_cpup((__le16 *)&hdr->fc),
-		 hdr->seq);
+		 hdr->sequence_number);
 
 	cb->type = hdr->fc.type;
 	cb->ackreq = hdr->fc.ack_request;
-- 
1.7.9.5

Re: [PATCH bluetooth-next] mac802154: rename seq to sequence_number

[Stefan Schmidt]

Hello.

On 25/06/15 08:31, Varka Bhadram wrote:
> This patch rename ieee802154_hdr member seq to sequence_number.

Any good reason for this? I think seq is quite clear in this context and 
making it sequence_number has no real benefit.

If others disagree I'm fine to let that one in though. No hard feelings 
about it.

regards
Stefan Schmidt
> Signed-off-by: Varka Bhadram <varkab@cdac.in>
> ---
>   include/net/ieee802154_netdev.h |    2 +-
>   net/ieee802154/header_ops.c     |    2 +-
>   net/mac802154/iface.c           |    2 +-
>   net/mac802154/rx.c              |    2 +-
>   4 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/include/net/ieee802154_netdev.h b/include/net/ieee802154_netdev.h
> index 2c10a9f..9c95a74 100644
> --- a/include/net/ieee802154_netdev.h
> +++ b/include/net/ieee802154_netdev.h
> @@ -88,7 +88,7 @@ struct ieee802154_hdr_fc {
>   
>   struct ieee802154_hdr {
>   	struct ieee802154_hdr_fc fc;
> -	u8 seq;
> +	u8 sequence_number;
>   	struct ieee802154_addr source;
>   	struct ieee802154_addr dest;
>   	struct ieee802154_sechdr sec;
> diff --git a/net/ieee802154/header_ops.c b/net/ieee802154/header_ops.c
> index a051b69..9c3291b 100644
> --- a/net/ieee802154/header_ops.c
> +++ b/net/ieee802154/header_ops.c
> @@ -90,7 +90,7 @@ ieee802154_hdr_push(struct sk_buff *skb, const struct ieee802154_hdr *hdr)
>   	int rc;
>   	struct ieee802154_hdr_fc fc = hdr->fc;
>   
> -	buf[pos++] = hdr->seq;
> +	buf[pos++] = hdr->sequence_number;
>   
>   	fc.dest_addr_mode = hdr->dest.mode;
>   
> diff --git a/net/mac802154/iface.c b/net/mac802154/iface.c
> index 416de90..0bc0f44 100644
> --- a/net/mac802154/iface.c
> +++ b/net/mac802154/iface.c
> @@ -372,7 +372,7 @@ static int mac802154_header_create(struct sk_buff *skb,
>   	hdr.fc.type = cb->type;
>   	hdr.fc.security_enabled = cb->secen;
>   	hdr.fc.ack_request = cb->ackreq;
> -	hdr.seq = atomic_inc_return(&dev->ieee802154_ptr->dsn) & 0xFF;
> +	hdr.sequence_number = atomic_inc_return(&dev->ieee802154_ptr->dsn) & 0xFF;
>   
>   	if (mac802154_set_header_security(sdata, &hdr, cb) < 0)
>   		return -EINVAL;
> diff --git a/net/mac802154/rx.c b/net/mac802154/rx.c
> index 5a258c1..48781a6 100644
> --- a/net/mac802154/rx.c
> +++ b/net/mac802154/rx.c
> @@ -142,7 +142,7 @@ ieee802154_parse_frame_start(struct sk_buff *skb, struct ieee802154_hdr *hdr)
>   	skb->mac_len = hlen;
>   
>   	pr_debug("fc: %04x dsn: %02x\n", le16_to_cpup((__le16 *)&hdr->fc),
> -		 hdr->seq);
> +		 hdr->sequence_number);
>   
>   	cb->type = hdr->fc.type;
>   	cb->ackreq = hdr->fc.ack_request;

Re: [PATCH bluetooth-next] mac802154: rename seq to sequence_number

[Phoebe Buckheister]

On Thu, June 25, 2015 9:29 am, Stefan Schmidt wrote:
> Hello.
>
> On 25/06/15 08:31, Varka Bhadram wrote:
>> This patch rename ieee802154_hdr member seq to sequence_number.
>
> Any good reason for this? I think seq is quite clear in this context and
> making it sequence_number has no real benefit.
>
> If others disagree I'm fine to let that one in though. No hard feelings
> about it.

Don't see the in renaming this either.

> regards
> Stefan Schmidt
>> Signed-off-by: Varka Bhadram <varkab@cdac.in>
>> ---
>>   include/net/ieee802154_netdev.h |    2 +-
>>   net/ieee802154/header_ops.c     |    2 +-
>>   net/mac802154/iface.c           |    2 +-
>>   net/mac802154/rx.c              |    2 +-
>>   4 files changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/include/net/ieee802154_netdev.h
>> b/include/net/ieee802154_netdev.h
>> index 2c10a9f..9c95a74 100644
>> --- a/include/net/ieee802154_netdev.h
>> +++ b/include/net/ieee802154_netdev.h
>> @@ -88,7 +88,7 @@ struct ieee802154_hdr_fc {
>>
>>   struct ieee802154_hdr {
>>   	struct ieee802154_hdr_fc fc;
>> -	u8 seq;
>> +	u8 sequence_number;
>>   	struct ieee802154_addr source;
>>   	struct ieee802154_addr dest;
>>   	struct ieee802154_sechdr sec;
>> diff --git a/net/ieee802154/header_ops.c b/net/ieee802154/header_ops.c
>> index a051b69..9c3291b 100644
>> --- a/net/ieee802154/header_ops.c
>> +++ b/net/ieee802154/header_ops.c
>> @@ -90,7 +90,7 @@ ieee802154_hdr_push(struct sk_buff *skb, const struct
>> ieee802154_hdr *hdr)
>>   	int rc;
>>   	struct ieee802154_hdr_fc fc = hdr->fc;
>>
>> -	buf[pos++] = hdr->seq;
>> +	buf[pos++] = hdr->sequence_number;
>>
>>   	fc.dest_addr_mode = hdr->dest.mode;
>>
>> diff --git a/net/mac802154/iface.c b/net/mac802154/iface.c
>> index 416de90..0bc0f44 100644
>> --- a/net/mac802154/iface.c
>> +++ b/net/mac802154/iface.c
>> @@ -372,7 +372,7 @@ static int mac802154_header_create(struct sk_buff
>> *skb,
>>   	hdr.fc.type = cb->type;
>>   	hdr.fc.security_enabled = cb->secen;
>>   	hdr.fc.ack_request = cb->ackreq;
>> -	hdr.seq = atomic_inc_return(&dev->ieee802154_ptr->dsn) & 0xFF;
>> +	hdr.sequence_number = atomic_inc_return(&dev->ieee802154_ptr->dsn) &
>> 0xFF;
>>
>>   	if (mac802154_set_header_security(sdata, &hdr, cb) < 0)
>>   		return -EINVAL;
>> diff --git a/net/mac802154/rx.c b/net/mac802154/rx.c
>> index 5a258c1..48781a6 100644
>> --- a/net/mac802154/rx.c
>> +++ b/net/mac802154/rx.c
>> @@ -142,7 +142,7 @@ ieee802154_parse_frame_start(struct sk_buff *skb,
>> struct ieee802154_hdr *hdr)
>>   	skb->mac_len = hlen;
>>
>>   	pr_debug("fc: %04x dsn: %02x\n", le16_to_cpup((__le16 *)&hdr->fc),
>> -		 hdr->seq);
>> +		 hdr->sequence_number);
>>
>>   	cb->type = hdr->fc.type;
>>   	cb->ackreq = hdr->fc.ack_request;
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-wpan" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH bluetooth-next] mac802154: rename seq to sequence_number

[Varka Bhadram]

Hi Stefan,

On 06/25/2015 12:59 PM, Stefan Schmidt wrote:
> Hello.
>
> On 25/06/15 08:31, Varka Bhadram wrote:
>> This patch rename ieee802154_hdr member seq to sequence_number.
>
> Any good reason for this? I think seq is quite clear in this context 
> and making it sequence_number has no real benefit.
>
> If others disagree I'm fine to let that one in though. No hard 
> feelings about it.
>
No good reason. For me *sequence_number* is more clear than *seq*
while looking at the IEEE-802.15.4 header structure.

Right now I am working on new parsing style on frame. I did this work as part of that.

-- 
Best regards,
Varka Bhadram.

Re: [PATCH bluetooth-next] mac802154: rename seq to sequence_number

[Varka Bhadram]

Hi Phoebe Buckheister,

On 06/25/2015 01:13 PM, Phoebe Buckheister wrote:
> On Thu, June 25, 2015 9:29 am, Stefan Schmidt wrote:
>> Hello.
>>
>> On 25/06/15 08:31, Varka Bhadram wrote:
>>> This patch rename ieee802154_hdr member seq to sequence_number.
>> Any good reason for this? I think seq is quite clear in this context and
>> making it sequence_number has no real benefit.
>>
>> If others disagree I'm fine to let that one in though. No hard feelings
>> about it.
> Don't see the in renaming this either.

I didn't get your point. ?

-- 
Best regards,
Varka Bhadram.

Re: [PATCH bluetooth-next] mac802154: rename seq to sequence_number

[Phoebe Buckheister]

On Thu, June 25, 2015 10:23 am, Varka Bhadram wrote:
> Hi Phoebe Buckheister,
>
> On 06/25/2015 01:13 PM, Phoebe Buckheister wrote:
>> On Thu, June 25, 2015 9:29 am, Stefan Schmidt wrote:
>>> Hello.
>>>
>>> On 25/06/15 08:31, Varka Bhadram wrote:
>>>> This patch rename ieee802154_hdr member seq to sequence_number.
>>> Any good reason for this? I think seq is quite clear in this context
>>> and
>>> making it sequence_number has no real benefit.
>>>
>>> If others disagree I'm fine to let that one in though. No hard feelings
>>> about it.
>> Don't see the in renaming this either.
>
> I didn't get your point. ?

The "point" went missing. I don't see why renaming this symbol is
necessary or useful. Like Stefan said, it is pretty clear from context,
and sequence_number is kind of long too.

> --
> Best regards,
> Varka Bhadram.
>
>

Re: [PATCH bluetooth-next] mac802154: rename seq to sequence_number

[Varka Bhadram]

Hello,

On 06/25/2015 02:10 PM, Phoebe Buckheister wrote:

> On Thu, June 25, 2015 10:23 am, Varka Bhadram wrote:
>> Hi Phoebe Buckheister,
>>
>> On 06/25/2015 01:13 PM, Phoebe Buckheister wrote:
>>> On Thu, June 25, 2015 9:29 am, Stefan Schmidt wrote:
>>>> Hello.
>>>>
>>>> On 25/06/15 08:31, Varka Bhadram wrote:
>>>>> This patch rename ieee802154_hdr member seq to sequence_number.
>>>> Any good reason for this? I think seq is quite clear in this context
>>>> and
>>>> making it sequence_number has no real benefit.
>>>>
>>>> If others disagree I'm fine to let that one in though. No hard feelings
>>>> about it.
>>> Don't see the in renaming this either.
>> I didn't get your point. ?
> The "point" went missing. I don't see why renaming this symbol is
> necessary or useful. Like Stefan said, it is pretty clear from context,
> and sequence_number is kind of long too.
>
As part of the rework on frame parsing, Alex used this naming convention [1].

If you think that it's too long, can we use *seq_num* instead of *seq* ?

[1]: https://github.com/linux-wpan/linux-wpan-next/blob/wpan_rework_rfc/include/net/ieee802154.h#L160

-- 
Varka Bhadram.

Re: [PATCH bluetooth-next] mac802154: rename seq to sequence_number

[Phoebe Buckheister]

On Thu, June 25, 2015 11:23 am, Varka Bhadram wrote:
> Hello,
>
> On 06/25/2015 02:10 PM, Phoebe Buckheister wrote:
>
>> On Thu, June 25, 2015 10:23 am, Varka Bhadram wrote:
>>> Hi Phoebe Buckheister,
>>>
>>> On 06/25/2015 01:13 PM, Phoebe Buckheister wrote:
>>>> On Thu, June 25, 2015 9:29 am, Stefan Schmidt wrote:
>>>>> Hello.
>>>>>
>>>>> On 25/06/15 08:31, Varka Bhadram wrote:
>>>>>> This patch rename ieee802154_hdr member seq to sequence_number.
>>>>> Any good reason for this? I think seq is quite clear in this context
>>>>> and
>>>>> making it sequence_number has no real benefit.
>>>>>
>>>>> If others disagree I'm fine to let that one in though. No hard
>>>>> feelings
>>>>> about it.
>>>> Don't see the in renaming this either.
>>> I didn't get your point. ?
>> The "point" went missing. I don't see why renaming this symbol is
>> necessary or useful. Like Stefan said, it is pretty clear from context,
>> and sequence_number is kind of long too.
>>
> As part of the rework on frame parsing, Alex used this naming convention
> [1].
>
> If you think that it's too long, can we use *seq_num* instead of *seq* ?

Sure. Just avoid overly long names when a shorter one will do.
Still not a friend of remaining that now if it'll be replaced anyway.
>
> [1]:
> https://github.com/linux-wpan/linux-wpan-next/blob/wpan_rework_rfc/include/net/ieee802154.h#L160
>
> --
> Varka Bhadram.
>
>

Re: [PATCH bluetooth-next] mac802154: rename seq to sequence_number

[Stefan Schmidt]

Hello.

On 25/06/15 11:23, Varka Bhadram wrote:
> Hello,
>
> On 06/25/2015 02:10 PM, Phoebe Buckheister wrote:
>
>> On Thu, June 25, 2015 10:23 am, Varka Bhadram wrote:
>>> Hi Phoebe Buckheister,
>>>
>>> On 06/25/2015 01:13 PM, Phoebe Buckheister wrote:
>>>> On Thu, June 25, 2015 9:29 am, Stefan Schmidt wrote:
>>>>> Hello.
>>>>>
>>>>> On 25/06/15 08:31, Varka Bhadram wrote:
>>>>>> This patch rename ieee802154_hdr member seq to sequence_number.
>>>>> Any good reason for this? I think seq is quite clear in this context
>>>>> and
>>>>> making it sequence_number has no real benefit.
>>>>>
>>>>> If others disagree I'm fine to let that one in though. No hard 
>>>>> feelings
>>>>> about it.
>>>> Don't see the in renaming this either.
>>> I didn't get your point. ?
>> The "point" went missing. I don't see why renaming this symbol is
>> necessary or useful. Like Stefan said, it is pretty clear from context,
>> and sequence_number is kind of long too.
>>
> As part of the rework on frame parsing, Alex used this naming 
> convention [1].
>
> If you think that it's too long, can we use *seq_num* instead of *seq* ?
>
> [1]: 
> https://github.com/linux-wpan/linux-wpan-next/blob/wpan_rework_rfc/include/net/ieee802154.h#L160
>

I still miss the benefit of this change. What else do you think seq 
would stand for in the context of the ieee802154 header?

Guess we can leave that up to Alex. Seq_num would be a compromise 
considering the length.

regards
Stefan Schmidt

Re: [PATCH bluetooth-next] mac802154: rename seq to sequence_number

[Alexander Aring]

Hi,

On Thu, Jun 25, 2015 at 02:53:18PM +0530, Varka Bhadram wrote:
> Hello,
> 
> On 06/25/2015 02:10 PM, Phoebe Buckheister wrote:
> 
> >On Thu, June 25, 2015 10:23 am, Varka Bhadram wrote:
> >>Hi Phoebe Buckheister,
> >>
> >>On 06/25/2015 01:13 PM, Phoebe Buckheister wrote:
> >>>On Thu, June 25, 2015 9:29 am, Stefan Schmidt wrote:
> >>>>Hello.
> >>>>
> >>>>On 25/06/15 08:31, Varka Bhadram wrote:
> >>>>>This patch rename ieee802154_hdr member seq to sequence_number.
> >>>>Any good reason for this? I think seq is quite clear in this context
> >>>>and
> >>>>making it sequence_number has no real benefit.
> >>>>
> >>>>If others disagree I'm fine to let that one in though. No hard feelings
> >>>>about it.
> >>>Don't see the in renaming this either.
> >>I didn't get your point. ?
> >The "point" went missing. I don't see why renaming this symbol is
> >necessary or useful. Like Stefan said, it is pretty clear from context,
> >and sequence_number is kind of long too.
> >
> As part of the rework on frame parsing, Alex used this naming convention [1].
> 
> If you think that it's too long, can we use *seq_num* instead of *seq* ?
> 

Yea, shame on me. The rework dev branch was just fun for me, I was
looking for mechanism like (nl802154, cfg802154, (frame parsing?)) which
wireless/mac80211 do.

I think I deleted for that the complete actual frame parsing stuff. Then
I grab the 802.15.4 standard and simple use the naming convention from
there.

Nevertheless in the C programmers nature is to use short names. I am
fine with "seq", maybe add comments that seq means sequence_number.

I need also to remember that doing things in a dirty dev branch and
bringing things mainline are complete different parts. Bringing things
mainline will be a longer process that all people are fine with the new
solution. Maybe we should first start a new mailing thread to talking
about the new strategies and listing the pros and cons of each solution.


Another point is: when you try to bring this frame parsing stuff
mainline (which is similar like mac80211 stuff), I don't ack patches
until we have some working llsec state in nl802154 and wpan-tools.

Because the llsec module is a core part of the frame parsing stuff.
The actual state for doing the frame parsing stuff in the control block
of skb (skb->cb (mac_cb)) will occur many side effects when we changing
something in there, which I can't ack at the moment without a working
llsec layer.


The second thing is: don't make per day 1-2 patches. If you plan to do
something big, then send patch series (please 20 patches at maximum per
patch series).

- Alex

Re: [PATCH bluetooth-next] mac802154: rename seq to sequence_number

[Varka Bhadram]

Hi,

On 06/25/2015 05:46 PM, Alexander Aring wrote:

> Hi,
>
> On Thu, Jun 25, 2015 at 02:53:18PM +0530, Varka Bhadram wrote:
>> Hello,
>>
>> On 06/25/2015 02:10 PM, Phoebe Buckheister wrote:
>>
>>> On Thu, June 25, 2015 10:23 am, Varka Bhadram wrote:
>>>> Hi Phoebe Buckheister,
>>>>
>>>> On 06/25/2015 01:13 PM, Phoebe Buckheister wrote:
>>>>> On Thu, June 25, 2015 9:29 am, Stefan Schmidt wrote:
>>>>>> Hello.
>>>>>>
>>>>>> On 25/06/15 08:31, Varka Bhadram wrote:
>>>>>>> This patch rename ieee802154_hdr member seq to sequence_number.
>>>>>> Any good reason for this? I think seq is quite clear in this context
>>>>>> and
>>>>>> making it sequence_number has no real benefit.
>>>>>>
>>>>>> If others disagree I'm fine to let that one in though. No hard feelings
>>>>>> about it.
>>>>> Don't see the in renaming this either.
>>>> I didn't get your point. ?
>>> The "point" went missing. I don't see why renaming this symbol is
>>> necessary or useful. Like Stefan said, it is pretty clear from context,
>>> and sequence_number is kind of long too.
>>>
>> As part of the rework on frame parsing, Alex used this naming convention [1].
>>
>> If you think that it's too long, can we use *seq_num* instead of *seq* ?
>>
> Yea, shame on me. The rework dev branch was just fun for me, I was
> looking for mechanism like (nl802154, cfg802154, (frame parsing?)) which
> wireless/mac80211 do.
>
> I think I deleted for that the complete actual frame parsing stuff. Then
> I grab the 802.15.4 standard and simple use the naming convention from
> there.
>
> Nevertheless in the C programmers nature is to use short names. I am
> fine with "seq", maybe add comments that seq means sequence_number.
>
> I need also to remember that doing things in a dirty dev branch and
> bringing things mainline are complete different parts. Bringing things
> mainline will be a longer process that all people are fine with the new
> solution. Maybe we should first start a new mailing thread to talking
> about the new strategies and listing the pros and cons of each solution.
>
>
> Another point is: when you try to bring this frame parsing stuff
> mainline (which is similar like mac80211 stuff), I don't ack patches
> until we have some working llsec state in nl802154 and wpan-tools.
>
> Because the llsec module is a core part of the frame parsing stuff.
> The actual state for doing the frame parsing stuff in the control block
> of skb (skb->cb (mac_cb)) will occur many side effects when we changing
> something in there, which I can't ack at the moment without a working
> llsec layer.
>
>
> The second thing is: don't make per day 1-2 patches. If you plan to do
> something big, then send patch series (please 20 patches at maximum per
> patch series).
>
> - Alex

Noted. Please drop it.

Thanks.

-- 
Varka Bhadram.