[PATCH] can: m_can: add support for handling arbitration error

[PATCH] can: m_can: add support for handling arbitration error

[Pankaj Sharma]

The Bosch MCAN hardware (3.1.0 and above) supports interrupt flag to
detect Protocol error in arbitration phase.

Transmit error statistics is currently not updated from the MCAN driver.
Protocol error in arbitration phase is a TX error and the network
statistics should be updated accordingly.

The member "tx_error" of "struct net_device_stats" should be incremented
as arbitration is a transmit protocol error. Also "arbitration_lost" of
"struct can_device_stats" should be incremented to report arbitration
lost.

Signed-off-by: Pankaj Sharma <pankj.sharma@samsung.com>
Signed-off-by: Sriram Dash <sriram.dash@samsung.com>
---
 drivers/net/can/m_can/m_can.c | 38 +++++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
index b95b382eb308..7efafee0eec8 100644
--- a/drivers/net/can/m_can/m_can.c
+++ b/drivers/net/can/m_can/m_can.c
@@ -778,6 +778,39 @@ static inline bool is_lec_err(u32 psr)
 	return psr && (psr != LEC_UNUSED);
 }
 
+static inline bool is_protocol_err(u32 irqstatus)
+{
+	if (irqstatus & IR_ERR_LEC_31X)
+		return 1;
+	else
+		return 0;
+}
+
+static int m_can_handle_protocol_error(struct net_device *dev, u32 irqstatus)
+{
+	struct net_device_stats *stats = &dev->stats;
+	struct m_can_priv *priv = netdev_priv(dev);
+	struct can_frame *cf;
+	struct sk_buff *skb;
+
+	/* propagate the error condition to the CAN stack */
+	skb = alloc_can_err_skb(dev, &cf);
+	if (unlikely(!skb))
+		return 0;
+
+	if (priv->version >= 31 && (irqstatus & IR_PEA)) {
+		netdev_dbg(dev, "Protocol error in Arbitration fail\n");
+		stats->tx_errors++;
+		priv->can.can_stats.arbitration_lost++;
+		cf->can_id |= CAN_ERR_LOSTARB;
+		cf->data[0] |= CAN_ERR_LOSTARB_UNSPEC;
+	}
+
+	netif_receive_skb(skb);
+
+	return 1;
+}
+
 static int m_can_handle_bus_errors(struct net_device *dev, u32 irqstatus,
 				   u32 psr)
 {
@@ -792,6 +825,11 @@ static int m_can_handle_bus_errors(struct net_device *dev, u32 irqstatus,
 	    is_lec_err(psr))
 		work_done += m_can_handle_lec_err(dev, psr & LEC_UNUSED);
 
+	/* handle protocol errors in arbitration phase */
+	if ((priv->can.ctrlmode & CAN_CTRLMODE_BERR_REPORTING) &&
+	    is_protocol_err(irqstatus))
+		work_done += m_can_handle_protocol_error(dev, irqstatus);
+
 	/* other unproccessed error interrupts */
 	m_can_handle_other_err(dev, irqstatus);
 
-- 
2.17.1

Re: [PATCH] can: m_can: add support for handling arbitration error

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 3051 bytes --]

On 10/14/19 1:34 PM, Pankaj Sharma wrote:
> The Bosch MCAN hardware (3.1.0 and above) supports interrupt flag to
> detect Protocol error in arbitration phase.
> 
> Transmit error statistics is currently not updated from the MCAN driver.
> Protocol error in arbitration phase is a TX error and the network
> statistics should be updated accordingly.
> 
> The member "tx_error" of "struct net_device_stats" should be incremented
> as arbitration is a transmit protocol error. Also "arbitration_lost" of
> "struct can_device_stats" should be incremented to report arbitration
> lost.
> 
> Signed-off-by: Pankaj Sharma <pankj.sharma@samsung.com>
> Signed-off-by: Sriram Dash <sriram.dash@samsung.com>
> ---
>  drivers/net/can/m_can/m_can.c | 38 +++++++++++++++++++++++++++++++++++
>  1 file changed, 38 insertions(+)
> 
> diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
> index b95b382eb308..7efafee0eec8 100644
> --- a/drivers/net/can/m_can/m_can.c
> +++ b/drivers/net/can/m_can/m_can.c
> @@ -778,6 +778,39 @@ static inline bool is_lec_err(u32 psr)
>  	return psr && (psr != LEC_UNUSED);
>  }
>  
> +static inline bool is_protocol_err(u32 irqstatus)

please add the comon m_can_ prefix

> +{
> +	if (irqstatus & IR_ERR_LEC_31X)
> +		return 1;
> +	else
> +		return 0;
> +}
> +
> +static int m_can_handle_protocol_error(struct net_device *dev, u32 irqstatus)
> +{
> +	struct net_device_stats *stats = &dev->stats;
> +	struct m_can_priv *priv = netdev_priv(dev);
> +	struct can_frame *cf;
> +	struct sk_buff *skb;
> +
> +	/* propagate the error condition to the CAN stack */
> +	skb = alloc_can_err_skb(dev, &cf);
> +	if (unlikely(!skb))
> +		return 0;

please handle the stats, even if the allocation of the skb fails.

> +
> +	if (priv->version >= 31 && (irqstatus & IR_PEA)) {
> +		netdev_dbg(dev, "Protocol error in Arbitration fail\n");
> +		stats->tx_errors++;
> +		priv->can.can_stats.arbitration_lost++;
> +		cf->can_id |= CAN_ERR_LOSTARB;
> +		cf->data[0] |= CAN_ERR_LOSTARB_UNSPEC;
> +	}
> +
> +	netif_receive_skb(skb);
> +
> +	return 1;
> +}
> +
>  static int m_can_handle_bus_errors(struct net_device *dev, u32 irqstatus,
>  				   u32 psr)
>  {
> @@ -792,6 +825,11 @@ static int m_can_handle_bus_errors(struct net_device *dev, u32 irqstatus,
>  	    is_lec_err(psr))
>  		work_done += m_can_handle_lec_err(dev, psr & LEC_UNUSED);
>  
> +	/* handle protocol errors in arbitration phase */
> +	if ((priv->can.ctrlmode & CAN_CTRLMODE_BERR_REPORTING) &&
> +	    is_protocol_err(irqstatus))
> +		work_done += m_can_handle_protocol_error(dev, irqstatus);
> +
>  	/* other unproccessed error interrupts */
>  	m_can_handle_other_err(dev, irqstatus);
>  
> 

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] can: m_can: add support for handling arbitration error

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2683 bytes --]

Hi Pankaj,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on net/master]
[cannot apply to v5.4-rc3 next-20191011]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Pankaj-Sharma/can-m_can-add-support-for-handling-arbitration-error/20191014-193532
config: sparc64-allmodconfig (attached as .config)
compiler: sparc64-linux-gcc (GCC) 7.4.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=7.4.0 make.cross ARCH=sparc64 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   drivers/net/can/m_can/m_can.c: In function 'm_can_handle_protocol_error':
>> drivers/net/can/m_can/m_can.c:800:10: error: dereferencing pointer to incomplete type 'struct m_can_priv'
     if (priv->version >= 31 && (irqstatus & IR_PEA)) {
             ^~
   drivers/net/can/m_can/m_can.c: In function 'm_can_handle_bus_errors':
   drivers/net/can/m_can/m_can.c:828:7: error: 'priv' undeclared (first use in this function); did you mean 'pid'?
     if ((priv->can.ctrlmode & CAN_CTRLMODE_BERR_REPORTING) &&
          ^~~~
          pid
   drivers/net/can/m_can/m_can.c:828:7: note: each undeclared identifier is reported only once for each function it appears in

vim +800 drivers/net/can/m_can/m_can.c

   787	
   788	static int m_can_handle_protocol_error(struct net_device *dev, u32 irqstatus)
   789	{
   790		struct net_device_stats *stats = &dev->stats;
   791		struct m_can_priv *priv = netdev_priv(dev);
   792		struct can_frame *cf;
   793		struct sk_buff *skb;
   794	
   795		/* propagate the error condition to the CAN stack */
   796		skb = alloc_can_err_skb(dev, &cf);
   797		if (unlikely(!skb))
   798			return 0;
   799	
 > 800		if (priv->version >= 31 && (irqstatus & IR_PEA)) {
   801			netdev_dbg(dev, "Protocol error in Arbitration fail\n");
   802			stats->tx_errors++;
   803			priv->can.can_stats.arbitration_lost++;
   804			cf->can_id |= CAN_ERR_LOSTARB;
   805			cf->data[0] |= CAN_ERR_LOSTARB_UNSPEC;
   806		}
   807	
   808		netif_receive_skb(skb);
   809	
   810		return 1;
   811	}
   812	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 59100 bytes --]

Re: [PATCH] can: m_can: add support for handling arbitration error

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2683 bytes --]

Hi Pankaj,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on net/master]
[cannot apply to v5.4-rc3 next-20191011]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Pankaj-Sharma/can-m_can-add-support-for-handling-arbitration-error/20191014-193532
config: sparc64-allmodconfig (attached as .config)
compiler: sparc64-linux-gcc (GCC) 7.4.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=7.4.0 make.cross ARCH=sparc64 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   drivers/net/can/m_can/m_can.c: In function 'm_can_handle_protocol_error':
>> drivers/net/can/m_can/m_can.c:800:10: error: dereferencing pointer to incomplete type 'struct m_can_priv'
     if (priv->version >= 31 && (irqstatus & IR_PEA)) {
             ^~
   drivers/net/can/m_can/m_can.c: In function 'm_can_handle_bus_errors':
   drivers/net/can/m_can/m_can.c:828:7: error: 'priv' undeclared (first use in this function); did you mean 'pid'?
     if ((priv->can.ctrlmode & CAN_CTRLMODE_BERR_REPORTING) &&
          ^~~~
          pid
   drivers/net/can/m_can/m_can.c:828:7: note: each undeclared identifier is reported only once for each function it appears in

vim +800 drivers/net/can/m_can/m_can.c

   787	
   788	static int m_can_handle_protocol_error(struct net_device *dev, u32 irqstatus)
   789	{
   790		struct net_device_stats *stats = &dev->stats;
   791		struct m_can_priv *priv = netdev_priv(dev);
   792		struct can_frame *cf;
   793		struct sk_buff *skb;
   794	
   795		/* propagate the error condition to the CAN stack */
   796		skb = alloc_can_err_skb(dev, &cf);
   797		if (unlikely(!skb))
   798			return 0;
   799	
 > 800		if (priv->version >= 31 && (irqstatus & IR_PEA)) {
   801			netdev_dbg(dev, "Protocol error in Arbitration fail\n");
   802			stats->tx_errors++;
   803			priv->can.can_stats.arbitration_lost++;
   804			cf->can_id |= CAN_ERR_LOSTARB;
   805			cf->data[0] |= CAN_ERR_LOSTARB_UNSPEC;
   806		}
   807	
   808		netif_receive_skb(skb);
   809	
   810		return 1;
   811	}
   812	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 59100 bytes --]

Re: [PATCH] can: m_can: add support for handling arbitration error

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2750 bytes --]

Hi Pankaj,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on net/master]
[cannot apply to v5.4-rc3 next-20191011]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Pankaj-Sharma/can-m_can-add-support-for-handling-arbitration-error/20191014-193532
config: sparc64-allmodconfig (attached as .config)
compiler: sparc64-linux-gcc (GCC) 7.4.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=7.4.0 make.cross ARCH=sparc64 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   drivers/net/can/m_can/m_can.c: In function 'm_can_handle_protocol_error':
>> drivers/net/can/m_can/m_can.c:800:10: error: dereferencing pointer to incomplete type 'struct m_can_priv'
     if (priv->version >= 31 && (irqstatus & IR_PEA)) {
             ^~
   drivers/net/can/m_can/m_can.c: In function 'm_can_handle_bus_errors':
   drivers/net/can/m_can/m_can.c:828:7: error: 'priv' undeclared (first use in this function); did you mean 'pid'?
     if ((priv->can.ctrlmode & CAN_CTRLMODE_BERR_REPORTING) &&
          ^~~~
          pid
   drivers/net/can/m_can/m_can.c:828:7: note: each undeclared identifier is reported only once for each function it appears in

vim +800 drivers/net/can/m_can/m_can.c

   787	
   788	static int m_can_handle_protocol_error(struct net_device *dev, u32 irqstatus)
   789	{
   790		struct net_device_stats *stats = &dev->stats;
   791		struct m_can_priv *priv = netdev_priv(dev);
   792		struct can_frame *cf;
   793		struct sk_buff *skb;
   794	
   795		/* propagate the error condition to the CAN stack */
   796		skb = alloc_can_err_skb(dev, &cf);
   797		if (unlikely(!skb))
   798			return 0;
   799	
 > 800		if (priv->version >= 31 && (irqstatus & IR_PEA)) {
   801			netdev_dbg(dev, "Protocol error in Arbitration fail\n");
   802			stats->tx_errors++;
   803			priv->can.can_stats.arbitration_lost++;
   804			cf->can_id |= CAN_ERR_LOSTARB;
   805			cf->data[0] |= CAN_ERR_LOSTARB_UNSPEC;
   806		}
   807	
   808		netif_receive_skb(skb);
   809	
   810		return 1;
   811	}
   812	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 59100 bytes --]

Re: [PATCH] can: m_can: add support for handling arbitration error

Hi Pankaj,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on net/master]
[cannot apply to v5.4-rc3 next-20191014]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Pankaj-Sharma/can-m_can-add-support-for-handling-arbitration-error/20191014-193532

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>


coccinelle warnings: (new ones prefixed by >>)

>> drivers/net/can/m_can/m_can.c:783:9-10: WARNING: return of 0/1 in function 'is_protocol_err' with return type bool

Please review and possibly fold the followup patch.

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

Re: [PATCH] can: m_can: add support for handling arbitration error

[kbuild test robot]

Hi Pankaj,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on net/master]
[cannot apply to v5.4-rc3 next-20191014]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Pankaj-Sharma/can-m_can-add-support-for-handling-arbitration-error/20191014-193532

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>


coccinelle warnings: (new ones prefixed by >>)

>> drivers/net/can/m_can/m_can.c:783:9-10: WARNING: return of 0/1 in function 'is_protocol_err' with return type bool

Please review and possibly fold the followup patch.

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

Re: [PATCH] can: m_can: add support for handling arbitration error

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1020 bytes --]

Hi Pankaj,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on net/master]
[cannot apply to v5.4-rc3 next-20191014]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Pankaj-Sharma/can-m_can-add-support-for-handling-arbitration-error/20191014-193532

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>


coccinelle warnings: (new ones prefixed by >>)

>> drivers/net/can/m_can/m_can.c:783:9-10: WARNING: return of 0/1 in function 'is_protocol_err' with return type bool

Please review and possibly fold the followup patch.

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[PATCH] can: m_can: fix boolreturn.cocci warnings

[kbuild test robot]

From: kbuild test robot <lkp@intel.com>

drivers/net/can/m_can/m_can.c:783:9-10: WARNING: return of 0/1 in function 'is_protocol_err' with return type bool

 Return statements in functions returning bool should use
 true/false instead of 1/0.
Generated by: scripts/coccinelle/misc/boolreturn.cocci

Fixes: 46946163ac61 ("can: m_can: add support for handling arbitration error")
CC: Pankaj Sharma <pankj.sharma@samsung.com>
Signed-off-by: kbuild test robot <lkp@intel.com>
---

url:    https://github.com/0day-ci/linux/commits/Pankaj-Sharma/can-m_can-add-support-for-handling-arbitration-error/20191014-193532

 m_can.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

--- a/drivers/net/can/m_can/m_can.c
+++ b/drivers/net/can/m_can/m_can.c
@@ -780,9 +780,9 @@ static inline bool is_lec_err(u32 psr)
 static inline bool is_protocol_err(u32 irqstatus)
 {
 	if (irqstatus & IR_ERR_LEC_31X)
-		return 1;
+		return true;
 	else
-		return 0;
+		return false;
 }
 
 static int m_can_handle_protocol_error(struct net_device *dev, u32 irqstatus)

[PATCH] can: m_can: fix boolreturn.cocci warnings

[kbuild test robot]

From: kbuild test robot <lkp@intel.com>

drivers/net/can/m_can/m_can.c:783:9-10: WARNING: return of 0/1 in function 'is_protocol_err' with return type bool

 Return statements in functions returning bool should use
 true/false instead of 1/0.
Generated by: scripts/coccinelle/misc/boolreturn.cocci

Fixes: 46946163ac61 ("can: m_can: add support for handling arbitration error")
CC: Pankaj Sharma <pankj.sharma@samsung.com>
Signed-off-by: kbuild test robot <lkp@intel.com>
---

url:    https://github.com/0day-ci/linux/commits/Pankaj-Sharma/can-m_can-add-support-for-handling-arbitration-error/20191014-193532

 m_can.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

--- a/drivers/net/can/m_can/m_can.c
+++ b/drivers/net/can/m_can/m_can.c
@@ -780,9 +780,9 @@ static inline bool is_lec_err(u32 psr)
 static inline bool is_protocol_err(u32 irqstatus)
 {
 	if (irqstatus & IR_ERR_LEC_31X)
-		return 1;
+		return true;
 	else
-		return 0;
+		return false;
 }
 
 static int m_can_handle_protocol_error(struct net_device *dev, u32 irqstatus)

[PATCH] can: m_can: fix boolreturn.cocci warnings

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1080 bytes --]

From: kbuild test robot <lkp@intel.com>

drivers/net/can/m_can/m_can.c:783:9-10: WARNING: return of 0/1 in function 'is_protocol_err' with return type bool

 Return statements in functions returning bool should use
 true/false instead of 1/0.
Generated by: scripts/coccinelle/misc/boolreturn.cocci

Fixes: 46946163ac61 ("can: m_can: add support for handling arbitration error")
CC: Pankaj Sharma <pankj.sharma@samsung.com>
Signed-off-by: kbuild test robot <lkp@intel.com>
---

url:    https://github.com/0day-ci/linux/commits/Pankaj-Sharma/can-m_can-add-support-for-handling-arbitration-error/20191014-193532

 m_can.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

--- a/drivers/net/can/m_can/m_can.c
+++ b/drivers/net/can/m_can/m_can.c
@@ -780,9 +780,9 @@ static inline bool is_lec_err(u32 psr)
 static inline bool is_protocol_err(u32 irqstatus)
 {
 	if (irqstatus & IR_ERR_LEC_31X)
-		return 1;
+		return true;
 	else
-		return 0;
+		return false;
 }
 
 static int m_can_handle_protocol_error(struct net_device *dev, u32 irqstatus)

Re: [PATCH] can: m_can: add support for handling arbitration error

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 3410 bytes --]

Hi Pankaj,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on net/master]
[cannot apply to v5.4-rc3 next-20191014]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Pankaj-Sharma/can-m_can-add-support-for-handling-arbitration-error/20191014-193532
config: x86_64-fedora-25 (attached as .config)
compiler: gcc-7 (Debian 7.4.0-13) 7.4.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   drivers/net/can/m_can/m_can.c: In function 'm_can_handle_protocol_error':
>> drivers/net/can/m_can/m_can.c:800:10: error: dereferencing pointer to incomplete type 'struct m_can_priv'
     if (priv->version >= 31 && (irqstatus & IR_PEA)) {
             ^~
   drivers/net/can/m_can/m_can.c: In function 'm_can_handle_bus_errors':
>> drivers/net/can/m_can/m_can.c:828:7: error: 'priv' undeclared (first use in this function); did you mean 'pid'?
     if ((priv->can.ctrlmode & CAN_CTRLMODE_BERR_REPORTING) &&
          ^~~~
          pid
   drivers/net/can/m_can/m_can.c:828:7: note: each undeclared identifier is reported only once for each function it appears in

vim +800 drivers/net/can/m_can/m_can.c

   787	
   788	static int m_can_handle_protocol_error(struct net_device *dev, u32 irqstatus)
   789	{
   790		struct net_device_stats *stats = &dev->stats;
   791		struct m_can_priv *priv = netdev_priv(dev);
   792		struct can_frame *cf;
   793		struct sk_buff *skb;
   794	
   795		/* propagate the error condition to the CAN stack */
   796		skb = alloc_can_err_skb(dev, &cf);
   797		if (unlikely(!skb))
   798			return 0;
   799	
 > 800		if (priv->version >= 31 && (irqstatus & IR_PEA)) {
   801			netdev_dbg(dev, "Protocol error in Arbitration fail\n");
   802			stats->tx_errors++;
   803			priv->can.can_stats.arbitration_lost++;
   804			cf->can_id |= CAN_ERR_LOSTARB;
   805			cf->data[0] |= CAN_ERR_LOSTARB_UNSPEC;
   806		}
   807	
   808		netif_receive_skb(skb);
   809	
   810		return 1;
   811	}
   812	
   813	static int m_can_handle_bus_errors(struct net_device *dev, u32 irqstatus,
   814					   u32 psr)
   815	{
   816		struct m_can_classdev *cdev = netdev_priv(dev);
   817		int work_done = 0;
   818	
   819		if (irqstatus & IR_RF0L)
   820			work_done += m_can_handle_lost_msg(dev);
   821	
   822		/* handle lec errors on the bus */
   823		if ((cdev->can.ctrlmode & CAN_CTRLMODE_BERR_REPORTING) &&
   824		    is_lec_err(psr))
   825			work_done += m_can_handle_lec_err(dev, psr & LEC_UNUSED);
   826	
   827		/* handle protocol errors in arbitration phase */
 > 828		if ((priv->can.ctrlmode & CAN_CTRLMODE_BERR_REPORTING) &&
   829		    is_protocol_err(irqstatus))
   830			work_done += m_can_handle_protocol_error(dev, irqstatus);
   831	
   832		/* other unproccessed error interrupts */
   833		m_can_handle_other_err(dev, irqstatus);
   834	
   835		return work_done;
   836	}
   837	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 51182 bytes --]

Re: [PATCH] can: m_can: add support for handling arbitration error

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 3410 bytes --]

Hi Pankaj,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on net/master]
[cannot apply to v5.4-rc3 next-20191014]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Pankaj-Sharma/can-m_can-add-support-for-handling-arbitration-error/20191014-193532
config: x86_64-fedora-25 (attached as .config)
compiler: gcc-7 (Debian 7.4.0-13) 7.4.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   drivers/net/can/m_can/m_can.c: In function 'm_can_handle_protocol_error':
>> drivers/net/can/m_can/m_can.c:800:10: error: dereferencing pointer to incomplete type 'struct m_can_priv'
     if (priv->version >= 31 && (irqstatus & IR_PEA)) {
             ^~
   drivers/net/can/m_can/m_can.c: In function 'm_can_handle_bus_errors':
>> drivers/net/can/m_can/m_can.c:828:7: error: 'priv' undeclared (first use in this function); did you mean 'pid'?
     if ((priv->can.ctrlmode & CAN_CTRLMODE_BERR_REPORTING) &&
          ^~~~
          pid
   drivers/net/can/m_can/m_can.c:828:7: note: each undeclared identifier is reported only once for each function it appears in

vim +800 drivers/net/can/m_can/m_can.c

   787	
   788	static int m_can_handle_protocol_error(struct net_device *dev, u32 irqstatus)
   789	{
   790		struct net_device_stats *stats = &dev->stats;
   791		struct m_can_priv *priv = netdev_priv(dev);
   792		struct can_frame *cf;
   793		struct sk_buff *skb;
   794	
   795		/* propagate the error condition to the CAN stack */
   796		skb = alloc_can_err_skb(dev, &cf);
   797		if (unlikely(!skb))
   798			return 0;
   799	
 > 800		if (priv->version >= 31 && (irqstatus & IR_PEA)) {
   801			netdev_dbg(dev, "Protocol error in Arbitration fail\n");
   802			stats->tx_errors++;
   803			priv->can.can_stats.arbitration_lost++;
   804			cf->can_id |= CAN_ERR_LOSTARB;
   805			cf->data[0] |= CAN_ERR_LOSTARB_UNSPEC;
   806		}
   807	
   808		netif_receive_skb(skb);
   809	
   810		return 1;
   811	}
   812	
   813	static int m_can_handle_bus_errors(struct net_device *dev, u32 irqstatus,
   814					   u32 psr)
   815	{
   816		struct m_can_classdev *cdev = netdev_priv(dev);
   817		int work_done = 0;
   818	
   819		if (irqstatus & IR_RF0L)
   820			work_done += m_can_handle_lost_msg(dev);
   821	
   822		/* handle lec errors on the bus */
   823		if ((cdev->can.ctrlmode & CAN_CTRLMODE_BERR_REPORTING) &&
   824		    is_lec_err(psr))
   825			work_done += m_can_handle_lec_err(dev, psr & LEC_UNUSED);
   826	
   827		/* handle protocol errors in arbitration phase */
 > 828		if ((priv->can.ctrlmode & CAN_CTRLMODE_BERR_REPORTING) &&
   829		    is_protocol_err(irqstatus))
   830			work_done += m_can_handle_protocol_error(dev, irqstatus);
   831	
   832		/* other unproccessed error interrupts */
   833		m_can_handle_other_err(dev, irqstatus);
   834	
   835		return work_done;
   836	}
   837	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 51182 bytes --]

Re: [PATCH] can: m_can: add support for handling arbitration error

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 3500 bytes --]

Hi Pankaj,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on net/master]
[cannot apply to v5.4-rc3 next-20191014]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Pankaj-Sharma/can-m_can-add-support-for-handling-arbitration-error/20191014-193532
config: x86_64-fedora-25 (attached as .config)
compiler: gcc-7 (Debian 7.4.0-13) 7.4.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   drivers/net/can/m_can/m_can.c: In function 'm_can_handle_protocol_error':
>> drivers/net/can/m_can/m_can.c:800:10: error: dereferencing pointer to incomplete type 'struct m_can_priv'
     if (priv->version >= 31 && (irqstatus & IR_PEA)) {
             ^~
   drivers/net/can/m_can/m_can.c: In function 'm_can_handle_bus_errors':
>> drivers/net/can/m_can/m_can.c:828:7: error: 'priv' undeclared (first use in this function); did you mean 'pid'?
     if ((priv->can.ctrlmode & CAN_CTRLMODE_BERR_REPORTING) &&
          ^~~~
          pid
   drivers/net/can/m_can/m_can.c:828:7: note: each undeclared identifier is reported only once for each function it appears in

vim +800 drivers/net/can/m_can/m_can.c

   787	
   788	static int m_can_handle_protocol_error(struct net_device *dev, u32 irqstatus)
   789	{
   790		struct net_device_stats *stats = &dev->stats;
   791		struct m_can_priv *priv = netdev_priv(dev);
   792		struct can_frame *cf;
   793		struct sk_buff *skb;
   794	
   795		/* propagate the error condition to the CAN stack */
   796		skb = alloc_can_err_skb(dev, &cf);
   797		if (unlikely(!skb))
   798			return 0;
   799	
 > 800		if (priv->version >= 31 && (irqstatus & IR_PEA)) {
   801			netdev_dbg(dev, "Protocol error in Arbitration fail\n");
   802			stats->tx_errors++;
   803			priv->can.can_stats.arbitration_lost++;
   804			cf->can_id |= CAN_ERR_LOSTARB;
   805			cf->data[0] |= CAN_ERR_LOSTARB_UNSPEC;
   806		}
   807	
   808		netif_receive_skb(skb);
   809	
   810		return 1;
   811	}
   812	
   813	static int m_can_handle_bus_errors(struct net_device *dev, u32 irqstatus,
   814					   u32 psr)
   815	{
   816		struct m_can_classdev *cdev = netdev_priv(dev);
   817		int work_done = 0;
   818	
   819		if (irqstatus & IR_RF0L)
   820			work_done += m_can_handle_lost_msg(dev);
   821	
   822		/* handle lec errors on the bus */
   823		if ((cdev->can.ctrlmode & CAN_CTRLMODE_BERR_REPORTING) &&
   824		    is_lec_err(psr))
   825			work_done += m_can_handle_lec_err(dev, psr & LEC_UNUSED);
   826	
   827		/* handle protocol errors in arbitration phase */
 > 828		if ((priv->can.ctrlmode & CAN_CTRLMODE_BERR_REPORTING) &&
   829		    is_protocol_err(irqstatus))
   830			work_done += m_can_handle_protocol_error(dev, irqstatus);
   831	
   832		/* other unproccessed error interrupts */
   833		m_can_handle_other_err(dev, irqstatus);
   834	
   835		return work_done;
   836	}
   837	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 51182 bytes --]

Re: [PATCH] can: m_can: fix boolreturn.cocci warnings

[Simon Horman]

On Mon, Oct 14, 2019 at 11:04:28PM +0800, kbuild test robot wrote:
> From: kbuild test robot <lkp@intel.com>
> 
> drivers/net/can/m_can/m_can.c:783:9-10: WARNING: return of 0/1 in function 'is_protocol_err' with return type bool
> 
>  Return statements in functions returning bool should use
>  true/false instead of 1/0.
> Generated by: scripts/coccinelle/misc/boolreturn.cocci
> 
> Fixes: 46946163ac61 ("can: m_can: add support for handling arbitration error")
> CC: Pankaj Sharma <pankj.sharma@samsung.com>
> Signed-off-by: kbuild test robot <lkp@intel.com>
> ---
> 
> url:    https://github.com/0day-ci/linux/commits/Pankaj-Sharma/can-m_can-add-support-for-handling-arbitration-error/20191014-193532
> 
>  m_can.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> --- a/drivers/net/can/m_can/m_can.c
> +++ b/drivers/net/can/m_can/m_can.c
> @@ -780,9 +780,9 @@ static inline bool is_lec_err(u32 psr)
>  static inline bool is_protocol_err(u32 irqstatus)
>  {
>  	if (irqstatus & IR_ERR_LEC_31X)
> -		return 1;
> +		return true;
>  	else
> -		return 0;
> +		return false;
>  }
>  
>  static int m_can_handle_protocol_error(struct net_device *dev, u32 irqstatus)
> 

<2c>
Perhaps the following is a nicer way to express this (completely untested):

	return !!(irqstatus & IR_ERR_LEC_31X);
</2c>

Re: [PATCH] can: m_can: fix boolreturn.cocci warnings

[Simon Horman]

[-- Attachment #1: Type: text/plain, Size: 1351 bytes --]

On Mon, Oct 14, 2019 at 11:04:28PM +0800, kbuild test robot wrote:
> From: kbuild test robot <lkp@intel.com>
> 
> drivers/net/can/m_can/m_can.c:783:9-10: WARNING: return of 0/1 in function 'is_protocol_err' with return type bool
> 
>  Return statements in functions returning bool should use
>  true/false instead of 1/0.
> Generated by: scripts/coccinelle/misc/boolreturn.cocci
> 
> Fixes: 46946163ac61 ("can: m_can: add support for handling arbitration error")
> CC: Pankaj Sharma <pankj.sharma@samsung.com>
> Signed-off-by: kbuild test robot <lkp@intel.com>
> ---
> 
> url:    https://github.com/0day-ci/linux/commits/Pankaj-Sharma/can-m_can-add-support-for-handling-arbitration-error/20191014-193532
> 
>  m_can.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> --- a/drivers/net/can/m_can/m_can.c
> +++ b/drivers/net/can/m_can/m_can.c
> @@ -780,9 +780,9 @@ static inline bool is_lec_err(u32 psr)
>  static inline bool is_protocol_err(u32 irqstatus)
>  {
>  	if (irqstatus & IR_ERR_LEC_31X)
> -		return 1;
> +		return true;
>  	else
> -		return 0;
> +		return false;
>  }
>  
>  static int m_can_handle_protocol_error(struct net_device *dev, u32 irqstatus)
> 

<2c>
Perhaps the following is a nicer way to express this (completely untested):

	return !!(irqstatus & IR_ERR_LEC_31X);
</2c>

Re: [PATCH] can: m_can: fix boolreturn.cocci warnings

[Jeroen Hofstee]

Hi,

On 10/15/19 7:57 AM, Simon Horman wrote:
> On Mon, Oct 14, 2019 at 11:04:28PM +0800, kbuild test robot wrote:
>> From: kbuild test robot <lkp@intel.com>
>>
>> drivers/net/can/m_can/m_can.c:783:9-10: WARNING: return of 0/1 in function 'is_protocol_err' with return type bool
>>
>>   Return statements in functions returning bool should use
>>   true/false instead of 1/0.
>> Generated by: scripts/coccinelle/misc/boolreturn.cocci
>>
>> Fixes: 46946163ac61 ("can: m_can: add support for handling arbitration error")
>> CC: Pankaj Sharma <pankj.sharma@samsung.com>
>> Signed-off-by: kbuild test robot <lkp@intel.com>
>> ---
>>
>> url:    https://github.com/0day-ci/linux/commits/Pankaj-Sharma/can-m_can-add-support-for-handling-arbitration-error/20191014-193532
>>
>>   m_can.c |    4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> --- a/drivers/net/can/m_can/m_can.c
>> +++ b/drivers/net/can/m_can/m_can.c
>> @@ -780,9 +780,9 @@ static inline bool is_lec_err(u32 psr)
>>   static inline bool is_protocol_err(u32 irqstatus)
>>   {
>>   	if (irqstatus & IR_ERR_LEC_31X)
>> -		return 1;
>> +		return true;
>>   	else
>> -		return 0;
>> +		return false;
>>   }
>>   
>>   static int m_can_handle_protocol_error(struct net_device *dev, u32 irqstatus)
>>
> <2c>
> Perhaps the following is a nicer way to express this (completely untested):
>
> 	return !!(irqstatus & IR_ERR_LEC_31X);
> </2c>


Really...., !! for bool / _Bool types? why not simply:

static inline bool is_protocol_err(u32 irqstatus)
	return irqstatus & IR_ERR_LEC_31X;
}

Regards,
Jeroen

Re: [PATCH] can: m_can: fix boolreturn.cocci warnings

[Simon Horman]

On Tue, Oct 15, 2019 at 06:37:54AM +0000, Jeroen Hofstee wrote:
> Hi,
> 
> On 10/15/19 7:57 AM, Simon Horman wrote:
> > On Mon, Oct 14, 2019 at 11:04:28PM +0800, kbuild test robot wrote:
> >> From: kbuild test robot <lkp@intel.com>
> >>
> >> drivers/net/can/m_can/m_can.c:783:9-10: WARNING: return of 0/1 in function 'is_protocol_err' with return type bool
> >>
> >>   Return statements in functions returning bool should use
> >>   true/false instead of 1/0.
> >> Generated by: scripts/coccinelle/misc/boolreturn.cocci
> >>
> >> Fixes: 46946163ac61 ("can: m_can: add support for handling arbitration error")
> >> CC: Pankaj Sharma <pankj.sharma@samsung.com>
> >> Signed-off-by: kbuild test robot <lkp@intel.com>
> >> ---
> >>
> >> url:    https://github.com/0day-ci/linux/commits/Pankaj-Sharma/can-m_can-add-support-for-handling-arbitration-error/20191014-193532
> >>
> >>   m_can.c |    4 ++--
> >>   1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> --- a/drivers/net/can/m_can/m_can.c
> >> +++ b/drivers/net/can/m_can/m_can.c
> >> @@ -780,9 +780,9 @@ static inline bool is_lec_err(u32 psr)
> >>   static inline bool is_protocol_err(u32 irqstatus)
> >>   {
> >>   	if (irqstatus & IR_ERR_LEC_31X)
> >> -		return 1;
> >> +		return true;
> >>   	else
> >> -		return 0;
> >> +		return false;
> >>   }
> >>   
> >>   static int m_can_handle_protocol_error(struct net_device *dev, u32 irqstatus)
> >>
> > <2c>
> > Perhaps the following is a nicer way to express this (completely untested):
> >
> > 	return !!(irqstatus & IR_ERR_LEC_31X);
> > </2c>
> 
> 
> Really...., !! for bool / _Bool types? why not simply:
> 
> static inline bool is_protocol_err(u32 irqstatus)
> 	return irqstatus & IR_ERR_LEC_31X;
> }

Good point, silly me.

Re: [PATCH] can: m_can: fix boolreturn.cocci warnings

[Simon Horman]

[-- Attachment #1: Type: text/plain, Size: 1787 bytes --]

On Tue, Oct 15, 2019 at 06:37:54AM +0000, Jeroen Hofstee wrote:
> Hi,
> 
> On 10/15/19 7:57 AM, Simon Horman wrote:
> > On Mon, Oct 14, 2019 at 11:04:28PM +0800, kbuild test robot wrote:
> >> From: kbuild test robot <lkp@intel.com>
> >>
> >> drivers/net/can/m_can/m_can.c:783:9-10: WARNING: return of 0/1 in function 'is_protocol_err' with return type bool
> >>
> >>   Return statements in functions returning bool should use
> >>   true/false instead of 1/0.
> >> Generated by: scripts/coccinelle/misc/boolreturn.cocci
> >>
> >> Fixes: 46946163ac61 ("can: m_can: add support for handling arbitration error")
> >> CC: Pankaj Sharma <pankj.sharma@samsung.com>
> >> Signed-off-by: kbuild test robot <lkp@intel.com>
> >> ---
> >>
> >> url:    https://github.com/0day-ci/linux/commits/Pankaj-Sharma/can-m_can-add-support-for-handling-arbitration-error/20191014-193532
> >>
> >>   m_can.c |    4 ++--
> >>   1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> --- a/drivers/net/can/m_can/m_can.c
> >> +++ b/drivers/net/can/m_can/m_can.c
> >> @@ -780,9 +780,9 @@ static inline bool is_lec_err(u32 psr)
> >>   static inline bool is_protocol_err(u32 irqstatus)
> >>   {
> >>   	if (irqstatus & IR_ERR_LEC_31X)
> >> -		return 1;
> >> +		return true;
> >>   	else
> >> -		return 0;
> >> +		return false;
> >>   }
> >>   
> >>   static int m_can_handle_protocol_error(struct net_device *dev, u32 irqstatus)
> >>
> > <2c>
> > Perhaps the following is a nicer way to express this (completely untested):
> >
> > 	return !!(irqstatus & IR_ERR_LEC_31X);
> > </2c>
> 
> 
> Really...., !! for bool / _Bool types? why not simply:
> 
> static inline bool is_protocol_err(u32 irqstatus)
> 	return irqstatus & IR_ERR_LEC_31X;
> }

Good point, silly me.

Re: [PATCH] can: m_can: fix boolreturn.cocci warnings

[Jeroen Hofstee]

Hello Simon,

On 10/15/19 9:13 AM, Simon Horman wrote:
> On Tue, Oct 15, 2019 at 06:37:54AM +0000, Jeroen Hofstee wrote:
>> Hi,
>>
>> On 10/15/19 7:57 AM, Simon Horman wrote:
>>> On Mon, Oct 14, 2019 at 11:04:28PM +0800, kbuild test robot wrote:
>>>> From: kbuild test robot <lkp@intel.com>
>>>>
>>>> drivers/net/can/m_can/m_can.c:783:9-10: WARNING: return of 0/1 in function 'is_protocol_err' with return type bool
>>>>
>>>>    Return statements in functions returning bool should use
>>>>    true/false instead of 1/0.
>>>> Generated by: scripts/coccinelle/misc/boolreturn.cocci
>>>>
>>>> Fixes: 46946163ac61 ("can: m_can: add support for handling arbitration error")
>>>> CC: Pankaj Sharma <pankj.sharma@samsung.com>
>>>> Signed-off-by: kbuild test robot <lkp@intel.com>
>>>> ---
>>>>
>>>> url:    https://github.com/0day-ci/linux/commits/Pankaj-Sharma/can-m_can-add-support-for-handling-arbitration-error/20191014-193532
>>>>
>>>>    m_can.c |    4 ++--
>>>>    1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> --- a/drivers/net/can/m_can/m_can.c
>>>> +++ b/drivers/net/can/m_can/m_can.c
>>>> @@ -780,9 +780,9 @@ static inline bool is_lec_err(u32 psr)
>>>>    static inline bool is_protocol_err(u32 irqstatus)
>>>>    {
>>>>    	if (irqstatus & IR_ERR_LEC_31X)
>>>> -		return 1;
>>>> +		return true;
>>>>    	else
>>>> -		return 0;
>>>> +		return false;
>>>>    }
>>>>    
>>>>    static int m_can_handle_protocol_error(struct net_device *dev, u32 irqstatus)
>>>>
>>> <2c>
>>> Perhaps the following is a nicer way to express this (completely untested):
>>>
>>> 	return !!(irqstatus & IR_ERR_LEC_31X);
>>> </2c>
>>
>> Really...., !! for bool / _Bool types? why not simply:
>>
>> static inline bool is_protocol_err(u32 irqstatus)
>> 	return irqstatus & IR_ERR_LEC_31X;
>> }
> Good point, silly me.


For clarity, I am commenting on the suggestion made by
the tool, not the patch itself..

Regards,

Jeroen

RE: [PATCH] can: m_can: add support for handling arbitration error

[pankj.sharma]

> From: Marc Kleine-Budde <mkl@pengutronix.de>
> Subject: Re: [PATCH] can: m_can: add support for handling arbitration error
> 
> On 10/14/19 1:34 PM, Pankaj Sharma wrote:
> > The Bosch MCAN hardware (3.1.0 and above) supports interrupt flag to
> > detect Protocol error in arbitration phase.
> >
> > Transmit error statistics is currently not updated from the MCAN driver.
> > Protocol error in arbitration phase is a TX error and the network
> > statistics should be updated accordingly.
> >
> > The member "tx_error" of "struct net_device_stats" should be
> > incremented as arbitration is a transmit protocol error. Also
> > "arbitration_lost" of "struct can_device_stats" should be incremented
> > to report arbitration lost.
> >
> > Signed-off-by: Pankaj Sharma <pankj.sharma@samsung.com>
> > Signed-off-by: Sriram Dash <sriram.dash@samsung.com>
> > ---
> >  drivers/net/can/m_can/m_can.c | 38
> > +++++++++++++++++++++++++++++++++++
> >  1 file changed, 38 insertions(+)
> >
> > diff --git a/drivers/net/can/m_can/m_can.c
> > b/drivers/net/can/m_can/m_can.c index b95b382eb308..7efafee0eec8
> > 100644
> > --- a/drivers/net/can/m_can/m_can.c
> > +++ b/drivers/net/can/m_can/m_can.c
> > @@ -778,6 +778,39 @@ static inline bool is_lec_err(u32 psr)
> >  	return psr && (psr != LEC_UNUSED);
> >  }
> >
> > +static inline bool is_protocol_err(u32 irqstatus)
> 
> please add the comon m_can_ prefix

Alright. Will change the name "is_protocol_err" to "m_can_ is_protocol_err"

> 
> > +{
> > +	if (irqstatus & IR_ERR_LEC_31X)
> > +		return 1;
> > +	else
> > +		return 0;
> > +}
> > +
> > +static int m_can_handle_protocol_error(struct net_device *dev, u32
> > +irqstatus) {
> > +	struct net_device_stats *stats = &dev->stats;
> > +	struct m_can_priv *priv = netdev_priv(dev);
> > +	struct can_frame *cf;
> > +	struct sk_buff *skb;
> > +
> > +	/* propagate the error condition to the CAN stack */
> > +	skb = alloc_can_err_skb(dev, &cf);
> > +	if (unlikely(!skb))
> > +		return 0;
> 
> please handle the stats, even if the allocation of the skb fails.

Alright. Will do as follows
+       if (unlikely(!skb)) {
+               netdev_dbg(dev, "allocation of skb failed\n");
+               stats->tx_errors++;
+               return 0;
+       }

> 
> > +
> > +	if (priv->version >= 31 && (irqstatus & IR_PEA)) {
> > +		netdev_dbg(dev, "Protocol error in Arbitration fail\n");
> > +		stats->tx_errors++;
> > +		priv->can.can_stats.arbitration_lost++;
> > +		cf->can_id |= CAN_ERR_LOSTARB;
> > +		cf->data[0] |= CAN_ERR_LOSTARB_UNSPEC;
> > +	}
> > +
> > +	netif_receive_skb(skb);
> > +
> > +	return 1;
> > +}
> > +
> >  static int m_can_handle_bus_errors(struct net_device *dev, u32 irqstatus,
> >  				   u32 psr)
> >  {
> > @@ -792,6 +825,11 @@ static int m_can_handle_bus_errors(struct
> net_device *dev, u32 irqstatus,
> >  	    is_lec_err(psr))
> >  		work_done += m_can_handle_lec_err(dev, psr & LEC_UNUSED);
> >
> > +	/* handle protocol errors in arbitration phase */
> > +	if ((priv->can.ctrlmode & CAN_CTRLMODE_BERR_REPORTING) &&
> > +	    is_protocol_err(irqstatus))
> > +		work_done += m_can_handle_protocol_error(dev, irqstatus);
> > +
> >  	/* other unproccessed error interrupts */
> >  	m_can_handle_other_err(dev, irqstatus);
> >
> >
> 
> Marc
> 
> --
> Pengutronix e.K.                  | Marc Kleine-Budde           |
> Industrial Linux Solutions        | Phone: +49-231-2826-924     |
> Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
> Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |

RE: [PATCH] can: m_can: add support for handling arbitration error

[pankj.sharma]

> From: Marc Kleine-Budde <mkl@pengutronix.de>
> Subject: Re: [PATCH] can: m_can: add support for handling arbitration error
> 
> On 10/14/19 1:34 PM, Pankaj Sharma wrote:
> > The Bosch MCAN hardware (3.1.0 and above) supports interrupt flag to
> > detect Protocol error in arbitration phase.
> >
> > Transmit error statistics is currently not updated from the MCAN driver.
> > Protocol error in arbitration phase is a TX error and the network
> > statistics should be updated accordingly.
> >
> > The member "tx_error" of "struct net_device_stats" should be
> > incremented as arbitration is a transmit protocol error. Also
> > "arbitration_lost" of "struct can_device_stats" should be incremented
> > to report arbitration lost.
> >
> > Signed-off-by: Pankaj Sharma <pankj.sharma@samsung.com>
> > Signed-off-by: Sriram Dash <sriram.dash@samsung.com>
> > ---
> >  drivers/net/can/m_can/m_can.c | 38
> > +++++++++++++++++++++++++++++++++++
> >  1 file changed, 38 insertions(+)
> >
> > diff --git a/drivers/net/can/m_can/m_can.c
> > b/drivers/net/can/m_can/m_can.c index b95b382eb308..7efafee0eec8
> > 100644
> > --- a/drivers/net/can/m_can/m_can.c
> > +++ b/drivers/net/can/m_can/m_can.c
> > @@ -778,6 +778,39 @@ static inline bool is_lec_err(u32 psr)
> >  	return psr && (psr != LEC_UNUSED);
> >  }
> >
> > +static inline bool is_protocol_err(u32 irqstatus)
> 
> please add the comon m_can_ prefix

Alright. Will change the name "is_protocol_err" to "m_can_ is_protocol_err"

> 
> > +{
> > +	if (irqstatus & IR_ERR_LEC_31X)
> > +		return 1;
> > +	else
> > +		return 0;
> > +}
> > +
> > +static int m_can_handle_protocol_error(struct net_device *dev, u32
> > +irqstatus) {
> > +	struct net_device_stats *stats = &dev->stats;
> > +	struct m_can_priv *priv = netdev_priv(dev);
> > +	struct can_frame *cf;
> > +	struct sk_buff *skb;
> > +
> > +	/* propagate the error condition to the CAN stack */
> > +	skb = alloc_can_err_skb(dev, &cf);
> > +	if (unlikely(!skb))
> > +		return 0;
> 
> please handle the stats, even if the allocation of the skb fails.

Alright. Will do as follows
+       if (unlikely(!skb)) {
+               netdev_dbg(dev, "allocation of skb failed\n");
+               stats->tx_errors++;
+               return 0;
+       }

> 
> > +
> > +	if (priv->version >= 31 && (irqstatus & IR_PEA)) {
> > +		netdev_dbg(dev, "Protocol error in Arbitration fail\n");
> > +		stats->tx_errors++;
> > +		priv->can.can_stats.arbitration_lost++;
> > +		cf->can_id |= CAN_ERR_LOSTARB;
> > +		cf->data[0] |= CAN_ERR_LOSTARB_UNSPEC;
> > +	}
> > +
> > +	netif_receive_skb(skb);
> > +
> > +	return 1;
> > +}
> > +
> >  static int m_can_handle_bus_errors(struct net_device *dev, u32 irqstatus,
> >  				   u32 psr)
> >  {
> > @@ -792,6 +825,11 @@ static int m_can_handle_bus_errors(struct
> net_device *dev, u32 irqstatus,
> >  	    is_lec_err(psr))
> >  		work_done += m_can_handle_lec_err(dev, psr & LEC_UNUSED);
> >
> > +	/* handle protocol errors in arbitration phase */
> > +	if ((priv->can.ctrlmode & CAN_CTRLMODE_BERR_REPORTING) &&
> > +	    is_protocol_err(irqstatus))
> > +		work_done += m_can_handle_protocol_error(dev, irqstatus);
> > +
> >  	/* other unproccessed error interrupts */
> >  	m_can_handle_other_err(dev, irqstatus);
> >
> >
> 
> Marc
> 
> --
> Pengutronix e.K.                  | Marc Kleine-Budde           |
> Industrial Linux Solutions        | Phone: +49-231-2826-924     |
> Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
> Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |

RE: [PATCH] can: m_can: fix boolreturn.cocci warnings

[pankj.sharma]

> From: Jeroen Hofstee <jhofstee@victronenergy.com>
> Subject: Re: [PATCH] can: m_can: fix boolreturn.cocci warnings
> 
> Hello Simon,
> 
> On 10/15/19 9:13 AM, Simon Horman wrote:
> > On Tue, Oct 15, 2019 at 06:37:54AM +0000, Jeroen Hofstee wrote:
> >> Hi,
> >>
> >> On 10/15/19 7:57 AM, Simon Horman wrote:
> >>> On Mon, Oct 14, 2019 at 11:04:28PM +0800, kbuild test robot wrote:
> >>>> From: kbuild test robot <lkp@intel.com>
> >>>>
> >>>> drivers/net/can/m_can/m_can.c:783:9-10: WARNING: return of 0/1 in
> >>>> function 'is_protocol_err' with return type bool
> >>>>
> >>>>    Return statements in functions returning bool should use
> >>>>    true/false instead of 1/0.
> >>>> Generated by: scripts/coccinelle/misc/boolreturn.cocci
> >>>>
> >>>> Fixes: 46946163ac61 ("can: m_can: add support for handling
> >>>> arbitration error")
> >>>> CC: Pankaj Sharma <pankj.sharma@samsung.com>
> >>>> Signed-off-by: kbuild test robot <lkp@intel.com>
> >>>> ---
> >>>>
> >>>> url:    https://github.com/0day-ci/linux/commits/Pankaj-Sharma/can-
> m_can-add-support-for-handling-arbitration-error/20191014-193532
> >>>>
> >>>>    m_can.c |    4 ++--
> >>>>    1 file changed, 2 insertions(+), 2 deletions(-)
> >>>>
> >>>> --- a/drivers/net/can/m_can/m_can.c
> >>>> +++ b/drivers/net/can/m_can/m_can.c
> >>>> @@ -780,9 +780,9 @@ static inline bool is_lec_err(u32 psr)
> >>>>    static inline bool is_protocol_err(u32 irqstatus)
> >>>>    {
> >>>>    	if (irqstatus & IR_ERR_LEC_31X)
> >>>> -		return 1;
> >>>> +		return true;
> >>>>    	else
> >>>> -		return 0;
> >>>> +		return false;
> >>>>    }
> >>>>
> >>>>    static int m_can_handle_protocol_error(struct net_device *dev,
> >>>> u32 irqstatus)
> >>>>
> >>> <2c>
> >>> Perhaps the following is a nicer way to express this (completely untested):
> >>>
> >>> 	return !!(irqstatus & IR_ERR_LEC_31X); </2c>
> >>
> >> Really...., !! for bool / _Bool types? why not simply:
> >>
> >> static inline bool is_protocol_err(u32 irqstatus)
> >> 	return irqstatus & IR_ERR_LEC_31X;

Thank you. Will Modify in v2.

> >> }
> > Good point, silly me.
> 
> 
> For clarity, I am commenting on the suggestion made by the tool, not the patch
> itself..
> 
> Regards,
> 
> Jeroen

RE: [PATCH] can: m_can: fix boolreturn.cocci warnings

[pankj.sharma]

> From: Jeroen Hofstee <jhofstee@victronenergy.com>
> Subject: Re: [PATCH] can: m_can: fix boolreturn.cocci warnings
> 
> Hello Simon,
> 
> On 10/15/19 9:13 AM, Simon Horman wrote:
> > On Tue, Oct 15, 2019 at 06:37:54AM +0000, Jeroen Hofstee wrote:
> >> Hi,
> >>
> >> On 10/15/19 7:57 AM, Simon Horman wrote:
> >>> On Mon, Oct 14, 2019 at 11:04:28PM +0800, kbuild test robot wrote:
> >>>> From: kbuild test robot <lkp@intel.com>
> >>>>
> >>>> drivers/net/can/m_can/m_can.c:783:9-10: WARNING: return of 0/1 in
> >>>> function 'is_protocol_err' with return type bool
> >>>>
> >>>>    Return statements in functions returning bool should use
> >>>>    true/false instead of 1/0.
> >>>> Generated by: scripts/coccinelle/misc/boolreturn.cocci
> >>>>
> >>>> Fixes: 46946163ac61 ("can: m_can: add support for handling
> >>>> arbitration error")
> >>>> CC: Pankaj Sharma <pankj.sharma@samsung.com>
> >>>> Signed-off-by: kbuild test robot <lkp@intel.com>
> >>>> ---
> >>>>
> >>>> url:    https://github.com/0day-ci/linux/commits/Pankaj-Sharma/can-
> m_can-add-support-for-handling-arbitration-error/20191014-193532
> >>>>
> >>>>    m_can.c |    4 ++--
> >>>>    1 file changed, 2 insertions(+), 2 deletions(-)
> >>>>
> >>>> --- a/drivers/net/can/m_can/m_can.c
> >>>> +++ b/drivers/net/can/m_can/m_can.c
> >>>> @@ -780,9 +780,9 @@ static inline bool is_lec_err(u32 psr)
> >>>>    static inline bool is_protocol_err(u32 irqstatus)
> >>>>    {
> >>>>    	if (irqstatus & IR_ERR_LEC_31X)
> >>>> -		return 1;
> >>>> +		return true;
> >>>>    	else
> >>>> -		return 0;
> >>>> +		return false;
> >>>>    }
> >>>>
> >>>>    static int m_can_handle_protocol_error(struct net_device *dev,
> >>>> u32 irqstatus)
> >>>>
> >>> <2c>
> >>> Perhaps the following is a nicer way to express this (completely untested):
> >>>
> >>> 	return !!(irqstatus & IR_ERR_LEC_31X); </2c>
> >>
> >> Really...., !! for bool / _Bool types? why not simply:
> >>
> >> static inline bool is_protocol_err(u32 irqstatus)
> >> 	return irqstatus & IR_ERR_LEC_31X;

Thank you. Will Modify in v2.

> >> }
> > Good point, silly me.
> 
> 
> For clarity, I am commenting on the suggestion made by the tool, not the patch
> itself..
> 
> Regards,
> 
> Jeroen

Re: [PATCH] can: m_can: fix boolreturn.cocci warnings

[pankj.sharma]

[-- Attachment #1: Type: text/plain, Size: 2277 bytes --]

> From: Jeroen Hofstee <jhofstee@victronenergy.com>
> Subject: Re: [PATCH] can: m_can: fix boolreturn.cocci warnings
> 
> Hello Simon,
> 
> On 10/15/19 9:13 AM, Simon Horman wrote:
> > On Tue, Oct 15, 2019 at 06:37:54AM +0000, Jeroen Hofstee wrote:
> >> Hi,
> >>
> >> On 10/15/19 7:57 AM, Simon Horman wrote:
> >>> On Mon, Oct 14, 2019 at 11:04:28PM +0800, kbuild test robot wrote:
> >>>> From: kbuild test robot <lkp@intel.com>
> >>>>
> >>>> drivers/net/can/m_can/m_can.c:783:9-10: WARNING: return of 0/1 in
> >>>> function 'is_protocol_err' with return type bool
> >>>>
> >>>>    Return statements in functions returning bool should use
> >>>>    true/false instead of 1/0.
> >>>> Generated by: scripts/coccinelle/misc/boolreturn.cocci
> >>>>
> >>>> Fixes: 46946163ac61 ("can: m_can: add support for handling
> >>>> arbitration error")
> >>>> CC: Pankaj Sharma <pankj.sharma@samsung.com>
> >>>> Signed-off-by: kbuild test robot <lkp@intel.com>
> >>>> ---
> >>>>
> >>>> url:    https://github.com/0day-ci/linux/commits/Pankaj-Sharma/can-
> m_can-add-support-for-handling-arbitration-error/20191014-193532
> >>>>
> >>>>    m_can.c |    4 ++--
> >>>>    1 file changed, 2 insertions(+), 2 deletions(-)
> >>>>
> >>>> --- a/drivers/net/can/m_can/m_can.c
> >>>> +++ b/drivers/net/can/m_can/m_can.c
> >>>> @@ -780,9 +780,9 @@ static inline bool is_lec_err(u32 psr)
> >>>>    static inline bool is_protocol_err(u32 irqstatus)
> >>>>    {
> >>>>    	if (irqstatus & IR_ERR_LEC_31X)
> >>>> -		return 1;
> >>>> +		return true;
> >>>>    	else
> >>>> -		return 0;
> >>>> +		return false;
> >>>>    }
> >>>>
> >>>>    static int m_can_handle_protocol_error(struct net_device *dev,
> >>>> u32 irqstatus)
> >>>>
> >>> <2c>
> >>> Perhaps the following is a nicer way to express this (completely untested):
> >>>
> >>> 	return !!(irqstatus & IR_ERR_LEC_31X); </2c>
> >>
> >> Really...., !! for bool / _Bool types? why not simply:
> >>
> >> static inline bool is_protocol_err(u32 irqstatus)
> >> 	return irqstatus & IR_ERR_LEC_31X;

Thank you. Will Modify in v2.

> >> }
> > Good point, silly me.
> 
> 
> For clarity, I am commenting on the suggestion made by the tool, not the patch
> itself..
> 
> Regards,
> 
> Jeroen