Patchwork pch_can: fix error passive level test

login
register
mail settings
Submitter Xi Wang
Date Dec. 12, 2011, 7:16 a.m.
Message ID <1323674180-16916-1-git-send-email-xi.wang@gmail.com>
Download mbox | patch
Permalink /patch/130649/
State Awaiting Upstream
Delegated to: David Miller
Headers show

Comments

Xi Wang - Dec. 12, 2011, 7:16 a.m.
The test (((errc & PCH_REC) >> 8) > 127) would always be false because
the receive error counter ((errc & PCH_REC) >> 8) is at most 127, where
PCH_REC is defined as 0x7f00.  To test whether the receive error counter
has reached the error passive level, the RP bit (15) should be used.

Signed-off-by: Xi Wang <xi.wang@gmail.com>
---
 drivers/net/can/pch_can.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)
Wolfgang Grandegger - Dec. 12, 2011, 8:05 a.m.
On 12/12/2011 08:16 AM, Xi Wang wrote:
> The test (((errc & PCH_REC) >> 8) > 127) would always be false because
> the receive error counter ((errc & PCH_REC) >> 8) is at most 127, where
> PCH_REC is defined as 0x7f00.  To test whether the receive error counter
> has reached the error passive level, the RP bit (15) should be used.
> 
> Signed-off-by: Xi Wang <xi.wang@gmail.com>

Acked-by: Wolfgang Grandegger <wg@grandegger.com>

The C_CAN driver, which supports the same CAN controller, does handle
the error passive state correctly. This reminds me to get rid of pch_can
in favor of C_CAN sooner than later.

Thanks,

Wolfgang.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Marc Kleine-Budde - Dec. 12, 2011, 9:17 a.m.
On 12/12/2011 09:05 AM, Wolfgang Grandegger wrote:
> On 12/12/2011 08:16 AM, Xi Wang wrote:
>> The test (((errc & PCH_REC) >> 8) > 127) would always be false because
>> the receive error counter ((errc & PCH_REC) >> 8) is at most 127, where
>> PCH_REC is defined as 0x7f00.  To test whether the receive error counter
>> has reached the error passive level, the RP bit (15) should be used.
>>
>> Signed-off-by: Xi Wang <xi.wang@gmail.com>
> 
> Acked-by: Wolfgang Grandegger <wg@grandegger.com>

Is this patch a candidate for stable?

> The C_CAN driver, which supports the same CAN controller, does handle
> the error passive state correctly. This reminds me to get rid of pch_can
> in favor of C_CAN sooner than later.

+1

Marc
Wolfgang Grandegger - Dec. 12, 2011, 9:31 a.m.
On 12/12/2011 10:17 AM, Marc Kleine-Budde wrote:
> On 12/12/2011 09:05 AM, Wolfgang Grandegger wrote:
>> On 12/12/2011 08:16 AM, Xi Wang wrote:
>>> The test (((errc & PCH_REC) >> 8) > 127) would always be false because
>>> the receive error counter ((errc & PCH_REC) >> 8) is at most 127, where
>>> PCH_REC is defined as 0x7f00.  To test whether the receive error counter
>>> has reached the error passive level, the RP bit (15) should be used.
>>>
>>> Signed-off-by: Xi Wang <xi.wang@gmail.com>
>>
>> Acked-by: Wolfgang Grandegger <wg@grandegger.com>
> 
> Is this patch a candidate for stable?

You mean for the "net" branch? Yes, I think so.

Wolfgang.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Marc Kleine-Budde - Dec. 12, 2011, 9:39 a.m.
On 12/12/2011 10:31 AM, Wolfgang Grandegger wrote:
> On 12/12/2011 10:17 AM, Marc Kleine-Budde wrote:
>> On 12/12/2011 09:05 AM, Wolfgang Grandegger wrote:
>>> On 12/12/2011 08:16 AM, Xi Wang wrote:
>>>> The test (((errc & PCH_REC) >> 8) > 127) would always be false because
>>>> the receive error counter ((errc & PCH_REC) >> 8) is at most 127, where
>>>> PCH_REC is defined as 0x7f00.  To test whether the receive error counter
>>>> has reached the error passive level, the RP bit (15) should be used.
>>>>
>>>> Signed-off-by: Xi Wang <xi.wang@gmail.com>
>>>
>>> Acked-by: Wolfgang Grandegger <wg@grandegger.com>
>>
>> Is this patch a candidate for stable?
> 
> You mean for the "net" branch? Yes, I think so.

Even for all trees which contain this driver (in a working version),
which is v2.6.38 and newer.

Marc
Wolfgang Grandegger - Dec. 12, 2011, 9:50 a.m.
On 12/12/2011 10:39 AM, Marc Kleine-Budde wrote:
> On 12/12/2011 10:31 AM, Wolfgang Grandegger wrote:
>> On 12/12/2011 10:17 AM, Marc Kleine-Budde wrote:
>>> On 12/12/2011 09:05 AM, Wolfgang Grandegger wrote:
>>>> On 12/12/2011 08:16 AM, Xi Wang wrote:
>>>>> The test (((errc & PCH_REC) >> 8) > 127) would always be false because
>>>>> the receive error counter ((errc & PCH_REC) >> 8) is at most 127, where
>>>>> PCH_REC is defined as 0x7f00.  To test whether the receive error counter
>>>>> has reached the error passive level, the RP bit (15) should be used.
>>>>>
>>>>> Signed-off-by: Xi Wang <xi.wang@gmail.com>
>>>>
>>>> Acked-by: Wolfgang Grandegger <wg@grandegger.com>
>>>
>>> Is this patch a candidate for stable?
>>
>> You mean for the "net" branch? Yes, I think so.
> 
> Even for all trees which contain this driver (in a working version),
> which is v2.6.38 and newer.

OK, Well, it's not a serious fix, at least.

Wolfgang.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Xi Wang - Jan. 7, 2012, 11:47 p.m.
On Dec 12, 2011, at 4:50 AM, Wolfgang Grandegger wrote:
> On 12/12/2011 10:39 AM, Marc Kleine-Budde wrote:
>> 
>> Even for all trees which contain this driver (in a working version),
>> which is v2.6.38 and newer.
> 
> OK, Well, it's not a serious fix, at least.

Should I resend the patch and cc to stable?  Thanks.

- xi
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/net/can/pch_can.c b/drivers/net/can/pch_can.c
index d11fbb2..6edc25e 100644
--- a/drivers/net/can/pch_can.c
+++ b/drivers/net/can/pch_can.c
@@ -66,6 +66,7 @@ 
 #define PCH_IF_CREQ_BUSY	BIT(15)
 
 #define PCH_STATUS_INT		0x8000
+#define PCH_RP			0x00008000
 #define PCH_REC			0x00007f00
 #define PCH_TEC			0x000000ff
 
@@ -527,7 +528,7 @@  static void pch_can_error(struct net_device *ndev, u32 status)
 		priv->can.can_stats.error_passive++;
 		state = CAN_STATE_ERROR_PASSIVE;
 		cf->can_id |= CAN_ERR_CRTL;
-		if (((errc & PCH_REC) >> 8) > 127)
+		if (errc & PCH_RP)
 			cf->data[1] |= CAN_ERR_CRTL_RX_PASSIVE;
 		if ((errc & PCH_TEC) > 127)
 			cf->data[1] |= CAN_ERR_CRTL_TX_PASSIVE;