mbox series

[v2,0/6] can: c_can: add support to 64 message objects

Message ID 20210225215155.30509-1-dariobin@libero.it
Headers show
Series can: c_can: add support to 64 message objects | expand

Message

Dario Binacchi Feb. 25, 2021, 9:51 p.m. UTC 
The D_CAN controller supports up to 128 messages. Until now the driver
only managed 32 messages although Sitara processors and DRA7 SOC can
handle 64.

The series was tested on a beaglebone board.

Note:
I have not changed the type of tx_field (belonging to the c_can_priv
structure) to atomic64_t because I think the atomic_t type has size
of at least 32 bits on x86 and arm, which is enough to handle 64
messages.
http://marc.info/?l=linux-can&m=139746476821294&w=2 reports the results
of tests performed just on x86 and arm architectures.

Changes in v2:
- Fix compiling error reported by kernel test robot.
- Add Reported-by tag.
- Pass larger size to alloc_candev() routine to avoid an additional
  memory allocation/deallocation.
- Add message objects number to PCI driver data.

Dario Binacchi (6):
  can: c_can: remove unused code
  can: c_can: fix indentation
  can: c_can: fix control interface used by c_can_do_tx
  can: c_can: use 32-bit write to set arbitration register
  can: c_can: prepare to up the message objects number
  can: c_can: add support to 64 message objects

 drivers/net/can/c_can/c_can.c          | 77 +++++++++++++++-----------
 drivers/net/can/c_can/c_can.h          | 32 +++++------
 drivers/net/can/c_can/c_can_pci.c      |  6 +-
 drivers/net/can/c_can/c_can_platform.c |  6 +-
 4 files changed, 68 insertions(+), 53 deletions(-)

Comments

Marc Kleine-Budde Feb. 26, 2021, 8:43 a.m. UTC | #1 
On 25.02.2021 22:51:55, Dario Binacchi wrote:
> --- a/drivers/net/can/c_can/c_can.h

> +++ b/drivers/net/can/c_can/c_can.h

> @@ -22,8 +22,6 @@

>  #ifndef C_CAN_H

>  #define C_CAN_H

>  

> -#define C_CAN_NO_OF_OBJECTS	32

> -

>  enum reg {

>  	C_CAN_CTRL_REG = 0,

>  	C_CAN_CTRL_EX_REG,

> @@ -61,6 +59,7 @@ enum reg {

>  	C_CAN_NEWDAT2_REG,

>  	C_CAN_INTPND1_REG,

>  	C_CAN_INTPND2_REG,

> +	C_CAN_INTPND3_REG,

>  	C_CAN_MSGVAL1_REG,

>  	C_CAN_MSGVAL2_REG,

>  	C_CAN_FUNCTION_REG,

> @@ -122,6 +121,7 @@ static const u16 __maybe_unused reg_map_d_can[] = {

>  	[C_CAN_NEWDAT2_REG]	= 0x9E,

>  	[C_CAN_INTPND1_REG]	= 0xB0,

>  	[C_CAN_INTPND2_REG]	= 0xB2,

> +	[C_CAN_INTPND3_REG]	= 0xB4,

>  	[C_CAN_MSGVAL1_REG]	= 0xC4,

>  	[C_CAN_MSGVAL2_REG]	= 0xC6,

>  	[C_CAN_IF1_COMREQ_REG]	= 0x100,

> @@ -161,6 +161,7 @@ struct raminit_bits {

>  

>  struct c_can_driver_data {

>  	enum c_can_dev_id id;

> +	int msg_obj_num;


unsigned int

>  

>  	/* RAMINIT register description. Optional. */

>  	const struct raminit_bits *raminit_bits; /* Array of START/DONE bit positions */

> diff --git a/drivers/net/can/c_can/c_can_pci.c b/drivers/net/can/c_can/c_can_pci.c

> index 3752f68d095e..2cb98ccd04d7 100644

> --- a/drivers/net/can/c_can/c_can_pci.c

> +++ b/drivers/net/can/c_can/c_can_pci.c

> @@ -31,6 +31,8 @@ enum c_can_pci_reg_align {

>  struct c_can_pci_data {

>  	/* Specify if is C_CAN or D_CAN */

>  	enum c_can_dev_id type;

> +	/* Number of message objects */

> +	int msg_obj_num;


unsigned int


Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |
Marc Kleine-Budde Feb. 26, 2021, 8:44 a.m. UTC | #2 
On 25.02.2021 22:51:52, Dario Binacchi wrote:
> According to commit 640916db2bf7 ("can: c_can: Make it SMP safe") let RX use

> IF1 (i.e. IF_RX) and TX use IF2 (i.e. IF_TX).


Is this a fix?

Marc

> 

> Signed-off-by: Dario Binacchi <dariobin@libero.it>

> ---

> 

> (no changes since v1)

> 

>  drivers/net/can/c_can/c_can.c | 2 +-

>  1 file changed, 1 insertion(+), 1 deletion(-)

> 

> diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c

> index dbcc1c1c92d6..69526c3a671c 100644

> --- a/drivers/net/can/c_can/c_can.c

> +++ b/drivers/net/can/c_can/c_can.c

> @@ -732,7 +732,7 @@ static void c_can_do_tx(struct net_device *dev)

>  		idx--;

>  		pend &= ~(1 << idx);

>  		obj = idx + C_CAN_MSG_OBJ_TX_FIRST;

> -		c_can_inval_tx_object(dev, IF_RX, obj);

> +		c_can_inval_tx_object(dev, IF_TX, obj);

>  		can_get_echo_skb(dev, idx, NULL);

>  		bytes += priv->dlc[idx];

>  		pkts++;

> -- 

> 2.17.1

> 

> 


-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |
Dario Binacchi Feb. 28, 2021, 10:35 a.m. UTC | #3 
Hi Marc,

> Il 26/02/2021 09:44 Marc Kleine-Budde <mkl@pengutronix.de> ha scritto:

> 

>  

> On 25.02.2021 22:51:52, Dario Binacchi wrote:

> > According to commit 640916db2bf7 ("can: c_can: Make it SMP safe") let RX use

> > IF1 (i.e. IF_RX) and TX use IF2 (i.e. IF_TX).

> 

> Is this a fix?

> 


I think that If I consider what is described in the 640916db2bf7 commit, using 
the IF_RX interface in a tx routine is wrong.

Thanks and regards
Dario

> Marc

> 

> > 

> > Signed-off-by: Dario Binacchi <dariobin@libero.it>

> > ---

> > 

> > (no changes since v1)

> > 

> >  drivers/net/can/c_can/c_can.c | 2 +-

> >  1 file changed, 1 insertion(+), 1 deletion(-)

> > 

> > diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c

> > index dbcc1c1c92d6..69526c3a671c 100644

> > --- a/drivers/net/can/c_can/c_can.c

> > +++ b/drivers/net/can/c_can/c_can.c

> > @@ -732,7 +732,7 @@ static void c_can_do_tx(struct net_device *dev)

> >  		idx--;

> >  		pend &= ~(1 << idx);

> >  		obj = idx + C_CAN_MSG_OBJ_TX_FIRST;

> > -		c_can_inval_tx_object(dev, IF_RX, obj);

> > +		c_can_inval_tx_object(dev, IF_TX, obj);

> >  		can_get_echo_skb(dev, idx, NULL);

> >  		bytes += priv->dlc[idx];

> >  		pkts++;

> > -- 

> > 2.17.1

> > 

> > 

> 

> -- 

> Pengutronix e.K.                 | Marc Kleine-Budde           |

> Embedded Linux                   | https://www.pengutronix.de  |

> Vertretung West/Dortmund         | Phone: +49-231-2826-924     |

> Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |
Marc Kleine-Budde March 1, 2021, 11:36 a.m. UTC | #4 
On 28.02.2021 11:35:31, Dario Binacchi wrote:
> > On 25.02.2021 22:51:52, Dario Binacchi wrote:

> > > According to commit 640916db2bf7 ("can: c_can: Make it SMP safe") let RX use

> > > IF1 (i.e. IF_RX) and TX use IF2 (i.e. IF_TX).

> > 

> > Is this a fix?

> > 

> 

> I think that If I consider what is described in the 640916db2bf7

> commit, using the IF_RX interface in a tx routine is wrong.


Yes, IF_RX is used in c_can_do_tx(), but that's called from
c_can_poll(), which runs ins NAPI.

As far as I understand 640916db2bf7 ("can: c_can: Make it SMP safe")
fixes the race condition that c_can_poll() and c_can_start_xmit() both
access the same IF. See again the patch description:

| The hardware has two message control interfaces, but the code only uses the
| first one. So on SMP the following can be observed:
| 
| CPU0            CPU1
| rx_poll()
|   write IF1     xmit()
|                 write IF1
|   write IF1

It's not 100% accurate, as the race condition is not just
c_can_do_rx_poll() against the c_can_start_xmit(), but the whole
c_can_poll() against c_can_start_xmit().

If you think my analysis is correct, please update the patch and add a
comment to clarify why IF_RX is used instead of changing it to IF_TX.

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |
Dario Binacchi March 1, 2021, 5:38 p.m. UTC | #5 
> Il 01/03/2021 12:36 Marc Kleine-Budde <mkl@pengutronix.de> ha scritto:

> 

>  

> On 28.02.2021 11:35:31, Dario Binacchi wrote:

> > > On 25.02.2021 22:51:52, Dario Binacchi wrote:

> > > > According to commit 640916db2bf7 ("can: c_can: Make it SMP safe") let RX use

> > > > IF1 (i.e. IF_RX) and TX use IF2 (i.e. IF_TX).

> > > 

> > > Is this a fix?

> > > 

> > 

> > I think that If I consider what is described in the 640916db2bf7

> > commit, using the IF_RX interface in a tx routine is wrong.

> 

> Yes, IF_RX is used in c_can_do_tx(), but that's called from

> c_can_poll(), which runs ins NAPI.


Yes, you are right. I was misled by the name of the function.

> 

> As far as I understand 640916db2bf7 ("can: c_can: Make it SMP safe")

> fixes the race condition that c_can_poll() and c_can_start_xmit() both

> access the same IF. See again the patch description:

> 

> | The hardware has two message control interfaces, but the code only uses the

> | first one. So on SMP the following can be observed:

> | 

> | CPU0            CPU1

> | rx_poll()

> |   write IF1     xmit()

> |                 write IF1

> |   write IF1

> 

> It's not 100% accurate, as the race condition is not just

> c_can_do_rx_poll() against the c_can_start_xmit(), but the whole

> c_can_poll() against c_can_start_xmit().

> 

> If you think my analysis is correct, please update the patch and add a

> comment to clarify why IF_RX is used instead of changing it to IF_TX.


I agree with you, I'll do it.

Thanks and regards,
Dario

> 

> regards,

> Marc

> 

> -- 

> Pengutronix e.K.                 | Marc Kleine-Budde           |

> Embedded Linux                   | https://www.pengutronix.de  |

> Vertretung West/Dortmund         | Phone: +49-231-2826-924     |

> Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |