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 <[email protected]>
Signed-off-by: Sriram Dash <[email protected]>
---
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
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 <[email protected]>
> Signed-off-by: Sriram Dash <[email protected]>
> ---
> 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 |
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 <[email protected]>
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
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 <[email protected]>
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
From: kbuild test robot <[email protected]>
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 <[email protected]>
Signed-off-by: kbuild test robot <[email protected]>
---
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)
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 <[email protected]>
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
On Mon, Oct 14, 2019 at 11:04:28PM +0800, kbuild test robot wrote:
> From: kbuild test robot <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: kbuild test robot <[email protected]>
> ---
>
> 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>
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 <[email protected]>
>>
>> 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 <[email protected]>
>> Signed-off-by: kbuild test robot <[email protected]>
>> ---
>>
>> 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
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 <[email protected]>
> >>
> >> 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 <[email protected]>
> >> Signed-off-by: kbuild test robot <[email protected]>
> >> ---
> >>
> >> 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.
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 <[email protected]>
>>>>
>>>> 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 <[email protected]>
>>>> Signed-off-by: kbuild test robot <[email protected]>
>>>> ---
>>>>
>>>> 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
> From: Marc Kleine-Budde <[email protected]>
> 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 <[email protected]>
> > Signed-off-by: Sriram Dash <[email protected]>
> > ---
> > 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 |
> From: Jeroen Hofstee <[email protected]>
> 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 <[email protected]>
> >>>>
> >>>> 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 <[email protected]>
> >>>> Signed-off-by: kbuild test robot <[email protected]>
> >>>> ---
> >>>>
> >>>> 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