Re: [PATCH] mac80211: fix order of flag descriptions

[Joe Perches]

On Fri, 2016-03-18 at 16:35 +, Luis de Bethencourt wrote:
> Fix order of mac80211_rx_flags description to match the enum.
> 
> Signed-off-by: Luis de Bethencourt 
> ---
> Hi,
> 
> I want ahead and fixed the order of the descriptions. checkpatch.pl was giving
> a warning to my previous patch and I had a hunch it was because the wrong 
> order
> breaks the parser. Indeed it does and with this patch below checkpatch.pl does
> not complain about this flag descriptions anymore.

checkpatch complains?  About what?

Re: [PATCH] mac80211: fix order of flag descriptions

[Luis de Bethencourt]

On 18/03/16 16:49, Johannes Berg wrote:
> On Fri, 2016-03-18 at 16:35 +, Luis de Bethencourt wrote:
>> Fix order of mac80211_rx_flags description to match the enum.
>>
>> Signed-off-by: Luis de Bethencourt 
>> ---
>> Hi,
>>
>> I want ahead and fixed the order of the descriptions. checkpatch.pl
>> was giving
>> a warning to my previous patch and I had a hunch it was because the
>> wrong order
>> breaks the parser. Indeed it does and with this patch below
>> checkpatch.pl does
>> not complain about this flag descriptions anymore.
>>
> Huh. I think we should fix checkpatch.pl instead. While the current
> order isn't likely really good, I believe kernel-doc will output the
> documentation in the order it's listed, and that can be useful for the
> documentation output to group related things, even if their bits may be
> further apart.
> 
> johannes
> 

I agree checkpatch.pl should be fixed. I can look into it. No promises
though, it has been a long time since I look at Perl code.

I understand the logic of grouping the documentation in logical blocks.
It is unfortunate though that this means the enum and documentation won't
match, which makes reading the code harder.

In this particular case I don't see the order of the documentation broken
because flags are being grouped.

Thanks for the review :)
Luis

Re: [PATCH] mac80211: fix order of flag descriptions

[Joe Perches]

On Fri, 2016-03-18 at 17:40 +, Luis de Bethencourt wrote:
> On 18/03/16 17:29, Joe Perches wrote:
> > On Fri, 2016-03-18 at 16:35 +, Luis de Bethencourt wrote:
> > > Fix order of mac80211_rx_flags description to match the enum.
[]
> > > I want ahead and fixed the order of the descriptions. checkpatch.pl was 
> > > giving
> > > a warning to my previous patch and I had a hunch it was because the wrong 
> > > order
> > > breaks the parser. Indeed it does and with this patch below checkpatch.pl 
> > > does
> > > not complain about this flag descriptions anymore.
> > checkpatch complains?  About what?
> warning: Enum value 'RX_FLAG_DUP_VALIDATED' not described in enum 
> 'mac80211_rx_flags'

That's not a checkpatch warning.

cheers, Joe

[PATCH] mac80211: fix order of flag descriptions

[Luis de Bethencourt]

Fix order of mac80211_rx_flags description to match the enum.

Signed-off-by: Luis de Bethencourt 
---
Hi,

I want ahead and fixed the order of the descriptions. checkpatch.pl was giving
a warning to my previous patch and I had a hunch it was because the wrong order
breaks the parser. Indeed it does and with this patch below checkpatch.pl does
not complain about this flag descriptions anymore.

Thanks,
Luis

 include/net/mac80211.h | 28 ++--
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index 42c8cde..4b215a3 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -992,17 +992,13 @@ ieee80211_tx_info_clear_status(struct ieee80211_tx_info 
*info)
  * @RX_FLAG_MMIC_ERROR: Michael MIC error was reported on this frame.
  * Use together with %RX_FLAG_MMIC_STRIPPED.
  * @RX_FLAG_DECRYPTED: This frame was decrypted in hardware.
+ * @RX_FLAG_MACTIME_PLCP_START: The timestamp passed in the RX status (@mactime
+ * field) is valid and contains the time the SYNC preamble was received.
  * @RX_FLAG_MMIC_STRIPPED: the Michael MIC is stripped off this frame,
  * verification has been done by the hardware.
  * @RX_FLAG_IV_STRIPPED: The IV/ICV are stripped from this frame.
  * If this flag is set, the stack cannot do any replay detection
  * hence the driver or hardware will have to do that.
- * @RX_FLAG_PN_VALIDATED: Currently only valid for CCMP/GCMP frames, this
- * flag indicates that the PN was verified for replay protection.
- * Note that this flag is also currently only supported when a frame
- * is also decrypted (ie. @RX_FLAG_DECRYPTED must be set)
- * @RX_FLAG_DUP_VALIDATED: Set this flag for the driver to check for packet
- * duplication by itself.
  * @RX_FLAG_FAILED_FCS_CRC: Set this flag if the FCS check failed on
  * the frame.
  * @RX_FLAG_FAILED_PLCP_CRC: Set this flag if the PCLP check failed on
@@ -1011,14 +1007,8 @@ ieee80211_tx_info_clear_status(struct ieee80211_tx_info 
*info)
  * field) is valid and contains the time the first symbol of the MPDU
  * was received. This is useful in monitor mode and for proper IBSS
  * merging.
- * @RX_FLAG_MACTIME_END: The timestamp passed in the RX status (@mactime
- * field) is valid and contains the time the last symbol of the MPDU
- * (including FCS) was received.
- * @RX_FLAG_MACTIME_PLCP_START: The timestamp passed in the RX status (@mactime
- * field) is valid and contains the time the SYNC preamble was received.
  * @RX_FLAG_SHORTPRE: Short preamble was used for this frame
  * @RX_FLAG_HT: HT MCS was used and rate_idx is MCS index
- * @RX_FLAG_VHT: VHT MCS was used and rate_index is MCS index
  * @RX_FLAG_40MHZ: HT40 (40 MHz) was used
  * @RX_FLAG_SHORT_GI: Short guard interval was used
  * @RX_FLAG_NO_SIGNAL_VAL: The signal strength value is not present.
@@ -1029,6 +1019,12 @@ ieee80211_tx_info_clear_status(struct ieee80211_tx_info 
*info)
  * @RX_FLAG_AMPDU_DETAILS: A-MPDU details are known, in particular the 
reference
  * number (@ampdu_reference) must be populated and be a distinct number for
  * each A-MPDU
+ * @RX_FLAG_PN_VALIDATED: Currently only valid for CCMP/GCMP frames, this
+ * flag indicates that the PN was verified for replay protection.
+ * Note that this flag is also currently only supported when a frame
+ * is also decrypted (ie. @RX_FLAG_DECRYPTED must be set)
+ * @RX_FLAG_DUP_VALIDATED: Set this flag for the driver to check for packet
+ * duplication by itself.
  * @RX_FLAG_AMPDU_LAST_KNOWN: last subframe is known, should be set on all
  * subframes of a single A-MPDU
  * @RX_FLAG_AMPDU_IS_LAST: this subframe is the last subframe of the A-MPDU
@@ -1036,8 +1032,10 @@ ieee80211_tx_info_clear_status(struct ieee80211_tx_info 
*info)
  * on this subframe
  * @RX_FLAG_AMPDU_DELIM_CRC_KNOWN: The delimiter CRC field is known (the CRC
  * is stored in the @ampdu_delimiter_crc field)
- * @RX_FLAG_MIC_STRIPPED: The mic was stripped of this packet. Decryption was
- * done by the hardware
+ * @RX_FLAG_MACTIME_END: The timestamp passed in the RX status (@mactime
+ * field) is valid and contains the time the last symbol of the MPDU
+ * (including FCS) was received.
+ * @RX_FLAG_VHT: VHT MCS was used and rate_index is MCS index
  * @RX_FLAG_LDPC: LDPC was used
  * @RX_FLAG_ONLY_MONITOR: Report frame only to monitor interfaces without
  * processing it in any regular way.
@@ -1062,6 +1060,8 @@ ieee80211_tx_info_clear_status(struct ieee80211_tx_info 
*info)
  * @RX_FLAG_RADIOTAP_VENDOR_DATA: This frame contains vendor-specific
  * radiotap data in the skb->data (before the frame) as described by
  * the  ieee80211_vendor_radiotap.
+ * @RX_FLAG_MIC_STRIPPED: The mic was stripped of this packet. Decryption was
+ * done by the hardware
  */
 enum mac80211_rx_flags {
RX_FLAG_MMIC_ERROR  = 

Re: [PATCH] mac80211: fix order of flag descriptions

[Luis de Bethencourt]

On 18/03/16 17:46, Joe Perches wrote:
> On Fri, 2016-03-18 at 17:40 +, Luis de Bethencourt wrote:
>> On 18/03/16 17:29, Joe Perches wrote:
>>> On Fri, 2016-03-18 at 16:35 +, Luis de Bethencourt wrote:
 Fix order of mac80211_rx_flags description to match the enum.
> []
 I want ahead and fixed the order of the descriptions. checkpatch.pl was 
 giving
 a warning to my previous patch and I had a hunch it was because the wrong 
 order
 breaks the parser. Indeed it does and with this patch below checkpatch.pl 
 does
 not complain about this flag descriptions anymore.
>>> checkpatch complains?  About what?
>> warning: Enum value 'RX_FLAG_DUP_VALIDATED' not described in enum 
>> 'mac80211_rx_flags'
> 
> That's not a checkpatch warning.
> 
> cheers, Joe
> 

Oh no!

Joe is right. That isn't a checkpatch warning, but checkpatch just printing the 
line
in my commit message that goes over 75 characters. Which I kept that big to 
keep the
format from make htmldocs.

I still think the order of the documentation should match the enum regardless, 
it is
nicer for developers reading the code.

Sorry :( my bad for misinterpreting checkpatch's output.

Thanks you Joe for correcting me,
Luis 

Re: [PATCH] mac80211: fix order of flag descriptions

[Luis de Bethencourt]

On 18/03/16 17:29, Joe Perches wrote:
> On Fri, 2016-03-18 at 16:35 +, Luis de Bethencourt wrote:
>> Fix order of mac80211_rx_flags description to match the enum.
>>
>> Signed-off-by: Luis de Bethencourt 
>> ---
>> Hi,
>>
>> I want ahead and fixed the order of the descriptions. checkpatch.pl was 
>> giving
>> a warning to my previous patch and I had a hunch it was because the wrong 
>> order
>> breaks the parser. Indeed it does and with this patch below checkpatch.pl 
>> does
>> not complain about this flag descriptions anymore.
> 
> checkpatch complains?  About what?
> 

warning: Enum value 'RX_FLAG_DUP_VALIDATED' not described in enum 
'mac80211_rx_flags'

Is the warning checkpatch.pl gives against my first patch (add doc for
RX_FLAG_DUP_VALIDATED flag). This is silenced by reordering the descriptions to 
match
the order in the enum.

Thanks,
Luis

Re: [PATCH] mac80211: fix order of flag descriptions

[Johannes Berg]

On Fri, 2016-03-18 at 16:35 +, Luis de Bethencourt wrote:
> Fix order of mac80211_rx_flags description to match the enum.
> 
> Signed-off-by: Luis de Bethencourt 
> ---
> Hi,
> 
> I want ahead and fixed the order of the descriptions. checkpatch.pl
> was giving
> a warning to my previous patch and I had a hunch it was because the
> wrong order
> breaks the parser. Indeed it does and with this patch below
> checkpatch.pl does
> not complain about this flag descriptions anymore.
> 
Huh. I think we should fix checkpatch.pl instead. While the current
order isn't likely really good, I believe kernel-doc will output the
documentation in the order it's listed, and that can be useful for the
documentation output to group related things, even if their bits may be
further apart.

johannes

Re: [PATCH] mac80211: fix order of flag descriptions

[Luis de Bethencourt]

On 18/03/16 17:46, Luis de Bethencourt wrote:
> On 18/03/16 16:49, Johannes Berg wrote:
>> On Fri, 2016-03-18 at 16:35 +, Luis de Bethencourt wrote:
>>> Fix order of mac80211_rx_flags description to match the enum.
>>>
>>> Signed-off-by: Luis de Bethencourt 
>>> ---
>>> Hi,
>>>
>>> I want ahead and fixed the order of the descriptions. checkpatch.pl
>>> was giving
>>> a warning to my previous patch and I had a hunch it was because the
>>> wrong order
>>> breaks the parser. Indeed it does and with this patch below
>>> checkpatch.pl does
>>> not complain about this flag descriptions anymore.
>>>
>> Huh. I think we should fix checkpatch.pl instead. While the current
>> order isn't likely really good, I believe kernel-doc will output the
>> documentation in the order it's listed, and that can be useful for the
>> documentation output to group related things, even if their bits may be
>> further apart.
>>
>> johannes
>>
> 
> I agree checkpatch.pl should be fixed. I can look into it. No promises
> though, it has been a long time since I look at Perl code.
> 
> I understand the logic of grouping the documentation in logical blocks.
> It is unfortunate though that this means the enum and documentation won't
> match, which makes reading the code harder.
> 
> In this particular case I don't see the order of the documentation broken
> because flags are being grouped.
> 
> Thanks for the review :)
> Luis
> 

Sorry Johannes,

Update. checkpatch doesn't need fixing and it does see the documentation
independently of the order. It was my mistake.

Joe Perches pointed it out in this email branch:
https://lkml.org/lkml/2016/3/18/532


Thanks for looking into this, sorry for my blunder.
Luis