[PATCH net 1/1] igc: return an error if the mac type is unknown in igc_ptp_systim_to_hwtstamp()

[PATCH net 1/1] igc: return an error if the mac type is unknown in igc_ptp_systim_to_hwtstamp()

[Tony Nguyen]

From: Tom Rix <trix@redhat.com>

clang static analysis reports
drivers/net/ethernet/intel/igc/igc_ptp.c:673:3: warning: The left operand of
  '+' is a garbage value [core.UndefinedBinaryOperatorResult]
   ktime_add_ns(shhwtstamps.hwtstamp, adjust);
   ^            ~~~~~~~~~~~~~~~~~~~~

igc_ptp_systim_to_hwtstamp() silently returns without setting the hwtstamp
if the mac type is unknown.  This should be treated as an error.

Fixes: 81b055205e8b ("igc: Add support for RX timestamping")
Signed-off-by: Tom Rix <trix@redhat.com>
Reviewed-by: Simon Horman <simon.horman@corigine.com>
Acked-by: Sasha Neftin <sasha.neftin@intel.com>
Tested-by: Naama Meir <naamax.meir@linux.intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/igc/igc_ptp.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/intel/igc/igc_ptp.c b/drivers/net/ethernet/intel/igc/igc_ptp.c
index c34734d432e0..4e10ced736db 100644
--- a/drivers/net/ethernet/intel/igc/igc_ptp.c
+++ b/drivers/net/ethernet/intel/igc/igc_ptp.c
@@ -417,10 +417,12 @@ static int igc_ptp_verify_pin(struct ptp_clock_info *ptp, unsigned int pin,
  *
  * We need to convert the system time value stored in the RX/TXSTMP registers
  * into a hwtstamp which can be used by the upper level timestamping functions.
+ *
+ * Returns 0 on success.
  **/
-static void igc_ptp_systim_to_hwtstamp(struct igc_adapter *adapter,
-				       struct skb_shared_hwtstamps *hwtstamps,
-				       u64 systim)
+static int igc_ptp_systim_to_hwtstamp(struct igc_adapter *adapter,
+				      struct skb_shared_hwtstamps *hwtstamps,
+				      u64 systim)
 {
 	switch (adapter->hw.mac.type) {
 	case igc_i225:
@@ -430,8 +432,9 @@ static void igc_ptp_systim_to_hwtstamp(struct igc_adapter *adapter,
 						systim & 0xFFFFFFFF);
 		break;
 	default:
-		break;
+		return -EINVAL;
 	}
+	return 0;
 }
 
 /**
@@ -652,7 +655,8 @@ static void igc_ptp_tx_hwtstamp(struct igc_adapter *adapter)
 
 	regval = rd32(IGC_TXSTMPL);
 	regval |= (u64)rd32(IGC_TXSTMPH) << 32;
-	igc_ptp_systim_to_hwtstamp(adapter, &shhwtstamps, regval);
+	if (igc_ptp_systim_to_hwtstamp(adapter, &shhwtstamps, regval))
+		return;
 
 	switch (adapter->link_speed) {
 	case SPEED_10:
-- 
2.38.1

Re: [PATCH net 1/1] igc: return an error if the mac type is unknown in igc_ptp_systim_to_hwtstamp()

[Jiri Pirko]

Tue, Jan 31, 2023 at 10:54:37PM CET, anthony.l.nguyen@intel.com wrote:
>From: Tom Rix <trix@redhat.com>
>
>clang static analysis reports
>drivers/net/ethernet/intel/igc/igc_ptp.c:673:3: warning: The left operand of
>  '+' is a garbage value [core.UndefinedBinaryOperatorResult]
>   ktime_add_ns(shhwtstamps.hwtstamp, adjust);
>   ^            ~~~~~~~~~~~~~~~~~~~~
>
>igc_ptp_systim_to_hwtstamp() silently returns without setting the hwtstamp
>if the mac type is unknown.  This should be treated as an error.
>
>Fixes: 81b055205e8b ("igc: Add support for RX timestamping")
>Signed-off-by: Tom Rix <trix@redhat.com>
>Reviewed-by: Simon Horman <simon.horman@corigine.com>
>Acked-by: Sasha Neftin <sasha.neftin@intel.com>
>Tested-by: Naama Meir <naamax.meir@linux.intel.com>
>Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
>---
> drivers/net/ethernet/intel/igc/igc_ptp.c | 14 +++++++++-----
> 1 file changed, 9 insertions(+), 5 deletions(-)
>
>diff --git a/drivers/net/ethernet/intel/igc/igc_ptp.c b/drivers/net/ethernet/intel/igc/igc_ptp.c
>index c34734d432e0..4e10ced736db 100644
>--- a/drivers/net/ethernet/intel/igc/igc_ptp.c
>+++ b/drivers/net/ethernet/intel/igc/igc_ptp.c
>@@ -417,10 +417,12 @@ static int igc_ptp_verify_pin(struct ptp_clock_info *ptp, unsigned int pin,
>  *
>  * We need to convert the system time value stored in the RX/TXSTMP registers
>  * into a hwtstamp which can be used by the upper level timestamping functions.
>+ *
>+ * Returns 0 on success.
>  **/
>-static void igc_ptp_systim_to_hwtstamp(struct igc_adapter *adapter,
>-				       struct skb_shared_hwtstamps *hwtstamps,
>-				       u64 systim)
>+static int igc_ptp_systim_to_hwtstamp(struct igc_adapter *adapter,
>+				      struct skb_shared_hwtstamps *hwtstamps,
>+				      u64 systim)
> {
> 	switch (adapter->hw.mac.type) {
> 	case igc_i225:
>@@ -430,8 +432,9 @@ static void igc_ptp_systim_to_hwtstamp(struct igc_adapter *adapter,
> 						systim & 0xFFFFFFFF);
> 		break;
> 	default:
>-		break;
>+		return -EINVAL;
> 	}
>+	return 0;
> }
> 
> /**
>@@ -652,7 +655,8 @@ static void igc_ptp_tx_hwtstamp(struct igc_adapter *adapter)
> 
> 	regval = rd32(IGC_TXSTMPL);
> 	regval |= (u64)rd32(IGC_TXSTMPH) << 32;
>-	igc_ptp_systim_to_hwtstamp(adapter, &shhwtstamps, regval);
>+	if (igc_ptp_systim_to_hwtstamp(adapter, &shhwtstamps, regval))

Use variable to store the return value.


>+		return;
> 
> 	switch (adapter->link_speed) {
> 	case SPEED_10:
>-- 
>2.38.1
>

Re: [PATCH net 1/1] igc: return an error if the mac type is unknown in igc_ptp_systim_to_hwtstamp()

[Jakub Kicinski]

On Wed, 1 Feb 2023 10:12:27 +0100 Jiri Pirko wrote:
> >@@ -652,7 +655,8 @@ static void igc_ptp_tx_hwtstamp(struct igc_adapter *adapter)
> > 
> > 	regval = rd32(IGC_TXSTMPL);
> > 	regval |= (u64)rd32(IGC_TXSTMPH) << 32;
> >-	igc_ptp_systim_to_hwtstamp(adapter, &shhwtstamps, regval);
> >+	if (igc_ptp_systim_to_hwtstamp(adapter, &shhwtstamps, regval))  
> 
> Use variable to store the return value.

Is that a rule.. IDK.. there's probably worse code in this driver 🤷️
The return value can't be propagated further anyway, since this is 
a work.

Re: [PATCH net 1/1] igc: return an error if the mac type is unknown in igc_ptp_systim_to_hwtstamp()

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (master)
by Jakub Kicinski <kuba@kernel.org>:

On Tue, 31 Jan 2023 13:54:37 -0800 you wrote:
> From: Tom Rix <trix@redhat.com>
> 
> clang static analysis reports
> drivers/net/ethernet/intel/igc/igc_ptp.c:673:3: warning: The left operand of
>   '+' is a garbage value [core.UndefinedBinaryOperatorResult]
>    ktime_add_ns(shhwtstamps.hwtstamp, adjust);
>    ^            ~~~~~~~~~~~~~~~~~~~~
> 
> [...]

Here is the summary with links:
  - [net,1/1] igc: return an error if the mac type is unknown in igc_ptp_systim_to_hwtstamp()
    https://git.kernel.org/netdev/net/c/a2df8463e15c

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH net 1/1] igc: return an error if the mac type is unknown in igc_ptp_systim_to_hwtstamp()

[Jiri Pirko]

Wed, Feb 01, 2023 at 07:04:54PM CET, kuba@kernel.org wrote:
>On Wed, 1 Feb 2023 10:12:27 +0100 Jiri Pirko wrote:
>> >@@ -652,7 +655,8 @@ static void igc_ptp_tx_hwtstamp(struct igc_adapter *adapter)
>> > 
>> > 	regval = rd32(IGC_TXSTMPL);
>> > 	regval |= (u64)rd32(IGC_TXSTMPH) << 32;
>> >-	igc_ptp_systim_to_hwtstamp(adapter, &shhwtstamps, regval);
>> >+	if (igc_ptp_systim_to_hwtstamp(adapter, &shhwtstamps, regval))  
>> 
>> Use variable to store the return value.
>
>Is that a rule.. IDK.. there's probably worse code in this driver 🤷️

Well, it was in the past at least. I recall DaveM wanted it that way.
And I think it makes sense. Makes the code easier to follow.


>The return value can't be propagated further anyway, since this is 
>a work.