i40evf: add missing kernel-doc argument
Subsystems (2)
PATCH v2 1/6 i40evf: add missing kernel-doc argument
From: Jean Sacren <[email protected]>
@flush has been missing since the inception of i40evf_irq_enable(). Add it for the kernel doc.
---
v2: For the whole series, drop the first two due to the simple mistake.
For this patch, no changes except for the serial number.
drivers/net/ethernet/intel/i40evf/i40evf_main.c | 1 + 1 file changed, 1 insertion(+)
drivers/net/ethernet/intel/i40evf/i40evf_main.c +1 -0
@@ -282,6 +282,7 @@ static void i40evf_fire_sw_int(struct i40evf_adapter *adapter, u32 mask)/** * i40evf_irq_enable - Enable default interrupt generation settings * @adapter: board private structure +* @flush: boolean value whether to run rd32() **/ void i40evf_irq_enable (struct i40evf_adapter *adapter,bool flush) {
PATCH v2 2/6 i40e: add missing kernel-doc argument
From: Jean Sacren <[email protected]>
The following kernell-doc arguments for their respective functions are missing:
1) @cd_type_cmd_tso_mss for i40e_tso();
2) @cd_type_cmd_tso_mss for i40e_tsyn();
3) @tx_ring for i40e_tx_enable_csum().
Add them all for the kernel-doc requirement.
---
v2: No changes except for the serial number.
drivers/net/ethernet/intel/i40e/i40e_txrx.c | 3 +++ 1 file changed, 3 insertions(+)
drivers/net/ethernet/intel/i40e/i40e_txrx.c +3 -0
@@ -2187,6 +2187,7 @@ out:* @tx_ring: ptr to the ring to send * @skb: ptr to the skb we're sending * @hdr_len: ptr to the size of the packet header +* @cd_type_cmd_tso_mss: ptr to u64 object * @cd_tunneling: ptr to context descriptor bits * * Returns 0 if no TSO can happen, 1 if tso is going, or error
@@ -2246,6 +2247,7 @@ static int i40e_tso(struct i40e_ring *tx_ring, struct sk_buff *skb,* @tx_ring: ptr to the ring to send * @skb: ptr to the skb we're sending * @tx_flags: the collected send information +* @cd_type_cmd_tso_mss: ptr to u64 object * * Returns 0 if no Tx timestamp can happen and 1 if the timestamp will happen **/
@@ -2288,6 +2290,7 @@ static int i40e_tsyn(struct i40e_ring *tx_ring, struct sk_buff *skb,* @tx_flags: pointer to Tx flags currently set * @td_cmd: Tx descriptor command bits to set * @td_offset: Tx descriptor header offsets to set +* @tx_ring: Tx descriptor ring * @cd_tunneling: ptr to context desc bits **/ static void i40e_tx_enable_csum(struct sk_buff *skb, u32 *tx_flags,
Bowers
PATCH v2 2/6 i40e: add missing kernel-doc argument
Quoted 17 lines
-----Original Message-----
From: Intel-wired-lan [mailto:intel-wired-lan-bounces at lists.osuosl.org] On Behalf Of Jean Sacren
Sent: Tuesday, October 13, 2015 12:06 AM
To: intel-wired-lan at lists.osuosl.org
Subject: [Intel-wired-lan] [next-queue v2 2/6] i40e: add missing kernel-doc argumentFrom: Jean Sacren <[email protected]>
The following kernell-doc arguments for their respective functions are missing:
1) @cd_type_cmd_tso_mss for
i40e_tso();
2) @cd_type_cmd_tso_mss fori40e_tsyn();
3) @tx_ring fori40e_tx_enable_csum().
Quoted 8 lines
Add them all for the kernel-doc requirement.
Signed-off-by Jean Sacren <[email protected]>---
v2: No changes except for the serial number.drivers/net/ethernet/intel/i40e/i40e_txrx.c | 3 +++ 1 file changed, 3 insertions(+)
Patch code changes correctly applied
PATCH v2 2/6 i40e: add missing kernel-doc argument
In reply to Jεan SacrenQuoted 9 lines · On Tue, 13 Oct 2015 01:06:28 -0600 J?an Sacren <[email protected]> wrote:
From: Jean Sacren <[email protected]>
The following kernell-doc arguments for their respective functions are missing:
1) @cd_type_cmd_tso_mss for
i40e_tso();
2) @cd_type_cmd_tso_mss fori40e_tsyn();
3) @tx_ring fori40e_tx_enable_csum().
Quoted 5 lines
Add them all for the kernel-doc requirement.
Signed-off-by Jean Sacren <[email protected]>---
v2: No changes except for the serial number.
PATCH v2 3/6 i40e: clean up the entire function exit path
In reply to Jεan SacrenFrom: Jean Sacren <[email protected]>
In i40e_tsyn(), by using a label of 'no_timestamp', we will be able to clearly denote a slew of circumstances that return with 0, which is much better as a default exit than the one that returns with 1.
With the above change, in the last check, we will be able to spare the 'else' branch. Further, the whole branch for returning 1 shall be put together for clarity.
Add a comment next to returning 1 in comparison to no_timestamp label.
---
v2: No changes except for the serial number.
drivers/net/ethernet/intel/i40e/i40e_txrx.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-)
drivers/net/ethernet/intel/i40e/i40e_txrx.c +9 -9
@@ -2257,31 +2257,31 @@ static int i40e_tsyn(struct i40e_ring *tx_ring, struct sk_buff *skb,struct i40e_pf *pf;if (likely (!(skb_shinfo (skb)->tx_flags &SKBTX_HW_TSTAMP ))) -return 0 ; + goto no_timestamp;/* Tx timestamps cannot be sampled when doing TSO */ if (tx_flags &I40E_TX_FLAGS_TSO ) -return 0 ; + goto no_timestamp;/* only timestamp the outbound packet if the user has requested it and * we are not already transmitting a packet to be timestamped */ pf =i40e_netdev_to_pf (tx_ring->netdev);if (!(pf->flags &I40E_FLAG_PTP )) -return 0 ; + goto no_timestamp;if (pf->ptp_tx && !test_and_set_bit_lock (__I40E_PTP_TX_IN_PROGRESS, &pf->state)) {skb_shinfo (skb)->tx_flags |=SKBTX_IN_PROGRESS ; pf->ptp_tx_skb =skb_get (skb); - }else { -return 0 ; - } + *cd_type_cmd_tso_mss |= (u64 )I40E_TX_CTX_DESC_TSYN << +I40E_TXD_CTX_QW1_CMD_SHIFT ; - *cd_type_cmd_tso_mss |= (u64 )I40E_TX_CTX_DESC_TSYN << -I40E_TXD_CTX_QW1_CMD_SHIFT ; +return 1 ;/* Tx timestamp will happen */ + } -return 1 ; +no_timestamp: +return 0 ; }/**
Bowers
PATCH v2 3/6 i40e: clean up the entire function exit path
Quoted 18 lines
-----Original Message-----
From: Intel-wired-lan [mailto:intel-wired-lan-bounces at lists.osuosl.org] On Behalf Of Jean Sacren
Sent: Tuesday, October 13, 2015 12:06 AM
To: intel-wired-lan at lists.osuosl.org
Subject: [Intel-wired-lan] [next-queue v2 3/6] i40e: clean up the entire function exit pathFrom: Jean Sacren <[email protected]>
In
i40e_tsyn(), by using a label of 'no_timestamp', we will be able to clearly denote a slew of circumstances that return with 0, which is much better as a default exit than the one that returns with 1.With the above change, in the last check, we will be able to spare the 'else' branch. Further, the whole branch for returning 1 shall be put together for clarity.
Quoted 8 lines
Add a comment next to returning 1 in comparison to
no_timestamplabel.Signed-off-by Jean Sacren <[email protected]>---
v2: No changes except for the serial number.drivers/net/ethernet/intel/i40e/i40e_txrx.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-)
Patch code changes correctly applied
PATCH v2 3/6 i40e: clean up the entire function exit path
In reply to Jεan SacrenQuoted 5 lines · On Tue, 13 Oct 2015 01:06:29 -0600 J?an Sacren <[email protected]> wrote:
From: Jean Sacren <[email protected]>
In
i40e_tsyn(), by using a label of 'no_timestamp', we will be able to clearly denote a slew of circumstances that return with 0, which is much better as a default exit than the one that returns with 1.
What's the motivation for making this change? Just code clean up?
BTW thanks for the patches, I realize you spent a good amount of time finding the issues/reviewing code.
Quoted 4 lines
With the above change, in the last check, we will be able to spare the 'else' branch. Further, the whole branch for returning 1 shall be put together for clarity.
Does sparing the "else branch" help anything? I'm not quite sure what the point of this change is beyond just code churn. If it is deemed better this way by someone else, then fine, and I'll ACK it, but the change itself really seems unnecessary.
Quoted 59 lines
Add a comment next to returning 1 in comparison to
no_timestamplabel.Signed-off-by Jean Sacren <[email protected]>---
v2: No changes except for the serial number.drivers/net/ethernet/intel/i40e/i40e_txrx.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-)drivers/net/ethernet/intel/i40e/i40e_txrx.c +9
-9@@ -2257,31 +2257,31 @@ static int i40e_tsyn(struct i40e_ring *tx_ring, struct sk_buff *skb,struct i40e_pf *pf;if (likely (!(skb_shinfo (skb)->tx_flags &SKBTX_HW_TSTAMP ))) -return 0 ; + goto no_timestamp;/* Tx timestamps cannot be sampled when doing TSO */ if (tx_flags &I40E_TX_FLAGS_TSO ) -return 0 ; + goto no_timestamp;/* only timestamp the outbound packet if the user has requested it and * we are not already transmitting a packet to be timestamped */ pf =i40e_netdev_to_pf (tx_ring->netdev);if (!(pf->flags &I40E_FLAG_PTP )) -return 0 ; + goto no_timestamp;if (pf->ptp_tx && !test_and_set_bit_lock (__I40E_PTP_TX_IN_PROGRESS, &pf->state)) {skb_shinfo (skb)->tx_flags |=SKBTX_IN_PROGRESS ; pf->ptp_tx_skb =skb_get (skb); - }else { -return 0 ; - } + *cd_type_cmd_tso_mss |= (u64 )I40E_TX_CTX_DESC_TSYN << +I40E_TXD_CTX_QW1_CMD_SHIFT ; - *cd_type_cmd_tso_mss |= (u64 )I40E_TX_CTX_DESC_TSYN << -I40E_TXD_CTX_QW1_CMD_SHIFT ; +return 1 ;/* Tx timestamp will happen */ + } -return 1 ; +no_timestamp: +return 0 ; }/** _______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan at lists.osuosl.org
http://lists.osuosl.org/mailman/listinfo/intel-wired-lan
Tantilov
PATCH v2 3/6 i40e: clean up the entire function exit path
Quoted 25 lines
-----Original Message-----
From: Intel-wired-lan [mailto:intel-wired-lan-bounces at lists.osuosl.org] On Behalf Of Jesse Brandeburg
Sent: Tuesday, October 13, 2015 9:59 AM
To: J?an Sacren
Cc: intel-wired-lan at lists.osuosl.org
Subject: Re: [Intel-wired-lan] [next-queue v2 3/6] i40e: clean up the entire function exit pathQuoted 5 lines · On Tue, 13 Oct 2015 01:06:29 -0600 J?an Sacren <[email protected]> wrote:
From: Jean Sacren <[email protected]>
In
i40e_tsyn(), by using a label of 'no_timestamp', we will be able to clearly denote a slew of circumstances that return with 0, which is much better as a default exit than the one that returns with 1.What's the motivation for making this change? Just code clean up?
BTW thanks for the patches, I realize you spent a good amount of time finding the issues/reviewing code.
Quoted 1 line
Quoted 8 lines
Quoted 3 lines
With the above change, in the last check, we will be able to spare the 'else' branch. Further, the whole branch for returning 1 shall be put together for clarity.
Does sparing the "else branch" help anything? I'm not quite sure what the point of this change is beyond just code churn. If it is deemed better this way by someone else, then fine, and I'll ACK it, but the change itself really seems unnecessary.
Actually this patch goes against guidelines of the coding style:
https://www.kernel.org/doc/Documentation/CodingStyle
Specifically the following paragraph:
The goto statement comes in handy when a function exits from multiple locations and some common work such as cleanup has to be done. If there is no cleanup needed then just return directly.
So this is a clear NACK IMHO.
Thanks,
Emil
PATCH v2 4/6 i40evf: clean up local variable initialization
In reply to Jεan SacrenFrom: Jean Sacren <[email protected]>
In i40evf_msix_aq(), the first two lines of rd32() are mainly to clear the registers. If we initialize 'val' at this point, it will be overwritten immediately. We shall simply discard the return value here.
When we initialize 'val', we might as well include the mask in one step.
---
v2: No changes except for the serial number.
drivers/net/ethernet/intel/i40evf/i40evf_main.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
drivers/net/ethernet/intel/i40evf/i40evf_main.c +4 -4
@@ -308,11 +308,11 @@ static irqreturn_t i40evf_msix_aq(int irq, void *data)u32 val;/* handle non-queue interrupts, these reads clear the registers */ - val =rd32 (hw,I40E_VFINT_ICR01 ); - val =rd32 (hw,I40E_VFINT_ICR0_ENA1 ); +rd32 (hw,I40E_VFINT_ICR01 ); +rd32 (hw,I40E_VFINT_ICR0_ENA1 ); - val =rd32 (hw,I40E_VFINT_DYN_CTL01 ); - val = val |I40E_VFINT_DYN_CTL01_CLEARPBA_MASK ; + val =rd32 (hw,I40E_VFINT_DYN_CTL01 ) | +I40E_VFINT_DYN_CTL01_CLEARPBA_MASK ;wr32 (hw,I40E_VFINT_DYN_CTL01 , val);/* schedule work on the private workqueue */
Bowers
PATCH v2 4/6 i40evf: clean up local variable initialization
Quoted 14 lines
-----Original Message-----
From: Intel-wired-lan [mailto:intel-wired-lan-bounces at lists.osuosl.org] On Behalf Of Jean Sacren
Sent: Tuesday, October 13, 2015 12:07 AM
To: intel-wired-lan at lists.osuosl.org
Subject: [Intel-wired-lan] [next-queue v2 4/6] i40evf: clean up local variable initializationFrom: Jean Sacren <[email protected]>
In
i40evf_msix_aq(), the first two lines ofrd32()are mainly to clear the registers. If we initialize 'val' at this point, it will be overwritten immediately. We shall simply discard the return value here.
Quoted 8 lines
When we initialize 'val', we might as well include the mask in one step.
Signed-off-by Jean Sacren <[email protected]>---
v2: No changes except for the serial number.drivers/net/ethernet/intel/i40evf/i40evf_main.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
Patch code changes correctly applied
PATCH v2 5/6 i40e: clean up local variable initialization
In reply to Jεan SacrenFrom: Jean Sacren <[email protected]>
In both i40e_calc_nvm_checksum() and i40e_update_nvm_checksum(), the local variables designated by 'ret_code' are overwritten immediately. As such, they should merely be declared.
---
v2: No changes except for the serial number.
drivers/net/ethernet/intel/i40e/i40e_nvm.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/net/ethernet/intel/i40e/i40e_nvm.c +2 -2
@@ -484,7 +484,7 @@ static i40e_status i40e_write_nvm_aq(struct i40e_hw *hw, u8 module_pointer,static i40e_status i40e_calc_nvm_checksum (struct i40e_hw *hw,u16 *checksum) { -i40e_status ret_code =0 ; +i40e_status ret_code;struct i40e_virt_mem vmem;u16 pcie_alt_module =0 ;u16 checksum_local =0 ;
@@ -564,7 +564,7 @@ i40e_calc_nvm_checksum_exit:**/ i40e_status i40e_update_nvm_checksum (struct i40e_hw *hw) { -i40e_status ret_code =0 ; +i40e_status ret_code;u16 checksum;__le16 le_sum;
Bowers
PATCH v2 5/6 i40e: clean up local variable initialization
Quoted 14 lines
-----Original Message-----
From: Intel-wired-lan [mailto:intel-wired-lan-bounces at lists.osuosl.org] On Behalf Of Jean Sacren
Sent: Tuesday, October 13, 2015 12:07 AM
To: intel-wired-lan at lists.osuosl.org
Subject: [Intel-wired-lan] [next-queue v2 5/6] i40e: clean up local variable initializationFrom: Jean Sacren <[email protected]>
In both
i40e_calc_nvm_checksum()andi40e_update_nvm_checksum(), the local variables designated by 'ret_code' are overwritten immediately. As such, they should merely be declared.
Quoted 6 lines
Signed-off-by Jean Sacren <[email protected]>---
v2: No changes except for the serial number.drivers/net/ethernet/intel/i40e/i40e_nvm.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
Patch code changes correctly applied
PATCH v2 6/6 i40e: fix unconditional execution of cpu_to_le16()
In reply to Jεan SacrenFrom: Jean Sacren <[email protected]>
The commit 3092e5e4cc79 ("i40e: add little endian conversion for checksum") fixed the checksum bug on big-endian architecture.
But we should not execute cpu_to_le16() unconditionally. Thus, put cpu_to_le16() under certain condition.
---
v2: Update patch serial number and add CC to Paul M Stillwell Jr.
drivers/net/ethernet/intel/i40e/i40e_nvm.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
drivers/net/ethernet/intel/i40e/i40e_nvm.c +3 -2
@@ -569,10 +569,11 @@ i40e_status i40e_update_nvm_checksum(struct i40e_hw *hw)__le16 le_sum; ret_code =i40e_calc_nvm_checksum (hw, &checksum); - le_sum =cpu_to_le16 (checksum); -if (!ret_code) +if (!ret_code) { + le_sum =cpu_to_le16 (checksum); ret_code =i40e_write_nvm_aq (hw,0x00 ,I40E_SR_SW_CHECKSUM_WORD ,1 , &le_sum, true); + }return ret_code; }
Bowers
PATCH v2 6/6 i40e: fix unconditional execution of cpu_to_le16()
Quoted 17 lines
-----Original Message-----
From: Intel-wired-lan [mailto:intel-wired-lan-bounces at lists.osuosl.org] On Behalf Of Jean Sacren
Sent: Tuesday, October 13, 2015 12:07 AM
To: intel-wired-lan at lists.osuosl.org
Cc: Stillwell Jr, Paul M
Subject: [Intel-wired-lan] [next-queue v2 6/6] i40e: fix unconditional execution ofcpu_to_le16()From: Jean Sacren <[email protected]>
The commit
3092e5e4cc79("i40e: add little endian conversion for checksum") fixed the checksum bug on big-endian architecture.But we should not execute
cpu_to_le16()unconditionally. Thus, putcpu_to_le16()under certain condition.
Quoted 8 lines
Signed-off-by Jean Sacren <[email protected]>Cc Jesse Brandeburg <[email protected]>Cc Paul M Stillwell Jr <[email protected]>---
v2: Update patch serial number and add CC to Paul M Stillwell Jr.drivers/net/ethernet/intel/i40e/i40e_nvm.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
Patch code changes correctly applied
Bowers
PATCH v2 1/6 i40evf: add missing kernel-doc argument
In reply to Jεan SacrenQuoted 13 lines
-----Original Message-----
From: Intel-wired-lan [mailto:intel-wired-lan-bounces at lists.osuosl.org] On Behalf Of Jean Sacren
Sent: Tuesday, October 13, 2015 12:06 AM
To: intel-wired-lan at lists.osuosl.org
Subject: [Intel-wired-lan] [next-queue v2 1/6] i40evf: add missing kernel-doc argumentFrom: Jean Sacren <[email protected]>
@flush has been missing since the inception of
i40evf_irq_enable(). Add it for the kernel doc.
Quoted 8 lines
Signed-off-by Jean Sacren <[email protected]>---
v2: For the whole series, drop the first two due to the simple mistake.For this patch, no changes except for the serial number.
drivers/net/ethernet/intel/i40evf/i40evf_main.c | 1 + 1 file changed, 1 insertion(+)
Patch code changes correctly applied
PATCH v2 1/6 i40evf: add missing kernel-doc argument
In reply to Jεan SacrenQuoted 6 lines · On Tue, 13 Oct 2015 01:06:27 -0600 J?an Sacren <[email protected]> wrote:
From: Jean Sacren <[email protected]>
@flush has been missing since the inception of
i40evf_irq_enable(). Add it for the kernel doc.Signed-off-by Jean Sacren <[email protected]>