Hi Kurt,
> Il 02/03/2021 19:49 Kurt Van Dijck <dev.k...@vandijck-laurijssen.be> ha
> scritto:
>
>
> On Sun, 28 Feb 2021 11:38:54 +0100, Dario Binacchi wrote:
> > Date: Sun, 28 Feb 2021 11:38:54 +0100
> > From: Dario Binacchi <dario...@libero.it>
> > To: linux-ker...@vger.kernel.org
> > Cc: Federico Vaga <federico.v...@gmail.com>, Alexander Stein
> > <alexander.st...@systec-electronic.com>, Dario Binacchi
> > <dario...@libero.it>, "David S. Miller" <da...@davemloft.net>, Jakub
> > Kicinski <k...@kernel.org>, Marc Kleine-Budde <m...@pengutronix.de>, Oliver
> > Hartkopp <socket...@hartkopp.net>, Vincent Mailhol
> > <mailhol.vinc...@wanadoo.fr>, Wolfgang Grandegger <w...@grandegger.com>,
> > YueHaibing <yuehaib...@huawei.com>, Zhang Qilong
> > <zhangqilo...@huawei.com>, linux-...@vger.kernel.org,
> > netdev@vger.kernel.org
> > Subject: [PATCH v3 5/6] can: c_can: prepare to up the message objects number
> > X-Mailer: git-send-email 2.17.1
> >
> > As pointed by commit c0a9f4d396c9 ("can: c_can: Reduce register access")
> > the "driver casts the 16 message objects in stone, which is completely
> > braindead as contemporary hardware has up to 128 message objects".
> >
> > The patch prepares the module to extend the number of message objects
> > beyond the 32 currently managed. This was achieved by transforming the
> > constants used to manage RX/TX messages into variables without changing
> > the driver policy.
> >
> > Signed-off-by: Dario Binacchi <dario...@libero.it>
> > Reported-by: kernel test robot <l...@intel.com>
> >
> > ---
> >
> > Changes in v3:
> > - Use unsigned int instead of int as type of the msg_obj_* fields
> > in the c_can_priv structure.
> > - Replace (u64)1 with 1UL in msg_obj_rx_mask setting.
> >
> > 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.
> >
> > drivers/net/can/c_can/c_can.c | 50 ++++++++++++++++----------
> > drivers/net/can/c_can/c_can.h | 23 ++++++------
> > drivers/net/can/c_can/c_can_pci.c | 2 +-
> > drivers/net/can/c_can/c_can_platform.c | 2 +-
> > 4 files changed, 43 insertions(+), 34 deletions(-)
> >
> > diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c
> > index 7081cfaf62e2..ede6f4d62095 100644
> > --- a/drivers/net/can/c_can/c_can.c
> > +++ b/drivers/net/can/c_can/c_can.c
> > @@ -173,9 +173,6 @@
> > /* Wait for ~1 sec for INIT bit */
> > #define INIT_WAIT_MS 1000
> >
> > -/* napi related */
> > -#define C_CAN_NAPI_WEIGHT C_CAN_MSG_OBJ_RX_NUM
> > -
> > /* c_can lec values */
> > enum c_can_lec_type {
> > LEC_NO_ERROR = 0,
> > @@ -325,7 +322,7 @@ static void c_can_setup_tx_object(struct net_device
> > *dev, int iface,
> > * first, i.e. clear the MSGVAL flag in the arbiter.
> > */
> > if (rtr != (bool)test_bit(idx, &priv->tx_dir)) {
> > - u32 obj = idx + C_CAN_MSG_OBJ_TX_FIRST;
> > + u32 obj = idx + priv->msg_obj_tx_first;
> >
> > c_can_inval_msg_object(dev, iface, obj);
> > change_bit(idx, &priv->tx_dir);
> > @@ -463,10 +460,10 @@ static netdev_tx_t c_can_start_xmit(struct sk_buff
> > *skb,
> > * prioritized. The lowest buffer number wins.
> > */
> > idx = fls(atomic_read(&priv->tx_active));
> > - obj = idx + C_CAN_MSG_OBJ_TX_FIRST;
> > + obj = idx + priv->msg_obj_tx_first;
> >
> > /* If this is the last buffer, stop the xmit queue */
> > - if (idx == C_CAN_MSG_OBJ_TX_NUM - 1)
> > + if (idx == priv->msg_obj_tx_num - 1)
> > netif_stop_queue(dev);
> > /*
> > * Store the message in the interface so we can call
> > @@ -549,17 +546,18 @@ static int c_can_set_bittiming(struct net_device *dev)
> > */
> > static void c_can_configure_msg_objects(struct net_device *dev)
> > {
> > + struct c_can_priv *priv = netdev_priv(dev);
> > int i;
> >
> > /* first invalidate all message objects */
> > - for (i = C_CAN_MSG_OBJ_RX_FIRST; i <= C_CAN_NO_OF_OBJECTS; i++)
> > + for (i = priv->msg_obj_rx_first; i <= priv->msg_obj_num; i++)
> > c_can_inval_msg_object(dev, IF_RX, i);
> >
> > /* setup receive message objects */
> > - for (i = C_CAN_MSG_OBJ_RX_FIRST; i < C_CAN_MSG_OBJ_RX_LAST; i++)
> > + for (i = priv->msg_obj_rx_first; i < priv->msg_obj_rx_last; i++)
> > c_can_setup_receive_object(dev, IF_RX, i, 0, 0, IF_MCONT_RCV);
> >
> > - c_can_setup_receive_object(dev, IF_RX, C_CAN_MSG_OBJ_RX_LAST, 0, 0,
> > + c_can_setup_receive_object(dev, IF_RX, priv->msg_obj_rx_last, 0, 0,
> > IF_MCONT_RCV_EOB);
> > }
> >
> > @@ -730,7 +728,7 @@ static void c_can_do_tx(struct net_device *dev)
> > while ((idx = ffs(pend))) {
> > idx--;
> > pend &= ~(1 << idx);
> > - obj = idx + C_CAN_MSG_OBJ_TX_FIRST;
> > + obj = idx + priv->msg_obj_tx_first;
> > c_can_inval_tx_object(dev, IF_TX, obj);
> > can_get_echo_skb(dev, idx, NULL);
> > bytes += priv->dlc[idx];
> > @@ -740,7 +738,7 @@ static void c_can_do_tx(struct net_device *dev)
> > /* Clear the bits in the tx_active mask */
> > atomic_sub(clr, &priv->tx_active);
> >
> > - if (clr & (1 << (C_CAN_MSG_OBJ_TX_NUM - 1)))
> > + if (clr & (1 << (priv->msg_obj_tx_num - 1)))
> > netif_wake_queue(dev);
> >
> > if (pkts) {
> > @@ -755,11 +753,11 @@ static void c_can_do_tx(struct net_device *dev)
> > * raced with the hardware or failed to readout all upper
> > * objects in the last run due to quota limit.
> > */
> > -static u32 c_can_adjust_pending(u32 pend)
> > +static u32 c_can_adjust_pending(u32 pend, u32 rx_mask)
> > {
> > u32 weight, lasts;
> >
> > - if (pend == RECEIVE_OBJECT_BITS)
> > + if (pend == rx_mask)
> > return pend;
> >
> > /*
> > @@ -862,8 +860,7 @@ static int c_can_do_rx_poll(struct net_device *dev, int
> > quota)
> > * It is faster to read only one 16bit register. This is only possible
> > * for a maximum number of 16 objects.
> > */
> > - BUILD_BUG_ON_MSG(C_CAN_MSG_OBJ_RX_LAST > 16,
> > - "Implementation does not support more message objects
> > than 16");
> > + WARN_ON(priv->msg_obj_rx_last > 16);
> >
> > while (quota > 0) {
> > if (!pend) {
> > @@ -874,7 +871,8 @@ static int c_can_do_rx_poll(struct net_device *dev, int
> > quota)
> > * If the pending field has a gap, handle the
> > * bits above the gap first.
> > */
> > - toread = c_can_adjust_pending(pend);
> > + toread = c_can_adjust_pending(pend,
> > + priv->msg_obj_rx_mask);
> > } else {
> > toread = pend;
> > }
> > @@ -1205,17 +1203,31 @@ static int c_can_close(struct net_device *dev)
> > return 0;
> > }
> >
> > -struct net_device *alloc_c_can_dev(void)
> > +struct net_device *alloc_c_can_dev(int msg_obj_num)
> > {
> > struct net_device *dev;
> > struct c_can_priv *priv;
> > + int msg_obj_tx_num = msg_obj_num / 2;
>
> IMO, a bigger tx queue is not usefull.
> A bigger rx queue however is.
This would not be good for my application.
I think it really depends on the type of application.
We can probably say that being able to size rx/tx queue
would be a useful feature.
Thanks and regards,
Dario
>
> My series last year took a fixed lenght of 8 for tx,
> and use the remaining as rx queue.
>
> >
> > - dev = alloc_candev(sizeof(struct c_can_priv), C_CAN_MSG_OBJ_TX_NUM);
> > + dev = alloc_candev(sizeof(*priv) + sizeof(u32) * msg_obj_tx_num,
> > + msg_obj_tx_num);
> > if (!dev)
> > return NULL;
> >
> > priv = netdev_priv(dev);
> > - netif_napi_add(dev, &priv->napi, c_can_poll, C_CAN_NAPI_WEIGHT);
> > + priv->msg_obj_num = msg_obj_num;
> > + priv->msg_obj_rx_num = msg_obj_num - msg_obj_tx_num;
> > + priv->msg_obj_rx_first = 1;
> > + priv->msg_obj_rx_last =
> > + priv->msg_obj_rx_first + priv->msg_obj_rx_num - 1;
> > + priv->msg_obj_rx_mask = (1UL << priv->msg_obj_rx_num) - 1;
> > +
> > + priv->msg_obj_tx_num = msg_obj_tx_num;
> > + priv->msg_obj_tx_first = priv->msg_obj_rx_last + 1;
> > + priv->msg_obj_tx_last =
> > + priv->msg_obj_tx_first + priv->msg_obj_tx_num - 1;
> > +
> > + netif_napi_add(dev, &priv->napi, c_can_poll, priv->msg_obj_rx_num);
> >
> > priv->dev = dev;
> > priv->can.bittiming_const = &c_can_bittiming_const;
> > diff --git a/drivers/net/can/c_can/c_can.h b/drivers/net/can/c_can/c_can.h
> > index 90d3d2e7a086..68295fab83d9 100644
> > --- a/drivers/net/can/c_can/c_can.h
> > +++ b/drivers/net/can/c_can/c_can.h
> > @@ -22,18 +22,7 @@
> > #ifndef C_CAN_H
> > #define C_CAN_H
> >
> > -/* message object split */
> > #define C_CAN_NO_OF_OBJECTS 32
> > -#define C_CAN_MSG_OBJ_RX_NUM 16
> > -#define C_CAN_MSG_OBJ_TX_NUM 16
> > -
> > -#define C_CAN_MSG_OBJ_RX_FIRST 1
> > -#define C_CAN_MSG_OBJ_RX_LAST (C_CAN_MSG_OBJ_RX_FIRST + \
> > - C_CAN_MSG_OBJ_RX_NUM - 1)
> > -
> > -#define C_CAN_MSG_OBJ_TX_FIRST (C_CAN_MSG_OBJ_RX_LAST + 1)
> > -
> > -#define RECEIVE_OBJECT_BITS 0x0000ffff
> >
> > enum reg {
> > C_CAN_CTRL_REG = 0,
> > @@ -193,6 +182,14 @@ struct c_can_priv {
> > struct napi_struct napi;
> > struct net_device *dev;
> > struct device *device;
> > + unsigned int msg_obj_num;
> > + unsigned int msg_obj_rx_num;
> > + unsigned int msg_obj_tx_num;
> > + unsigned int msg_obj_rx_first;
> > + unsigned int msg_obj_rx_last;
> > + unsigned int msg_obj_tx_first;
> > + unsigned int msg_obj_tx_last;
> > + u32 msg_obj_rx_mask;
> > atomic_t tx_active;
> > atomic_t sie_pending;
> > unsigned long tx_dir;
> > @@ -209,10 +206,10 @@ struct c_can_priv {
> > void (*raminit) (const struct c_can_priv *priv, bool enable);
> > u32 comm_rcv_high;
> > u32 rxmasked;
> > - u32 dlc[C_CAN_MSG_OBJ_TX_NUM];
> > + u32 dlc[];
> > };
> >
> > -struct net_device *alloc_c_can_dev(void);
> > +struct net_device *alloc_c_can_dev(int msg_obj_num);
> > void free_c_can_dev(struct net_device *dev);
> > int register_c_can_dev(struct net_device *dev);
> > void unregister_c_can_dev(struct net_device *dev);
> > diff --git a/drivers/net/can/c_can/c_can_pci.c
> > b/drivers/net/can/c_can/c_can_pci.c
> > index 406b4847e5dc..3752f68d095e 100644
> > --- a/drivers/net/can/c_can/c_can_pci.c
> > +++ b/drivers/net/can/c_can/c_can_pci.c
> > @@ -149,7 +149,7 @@ static int c_can_pci_probe(struct pci_dev *pdev,
> > }
> >
> > /* allocate the c_can device */
> > - dev = alloc_c_can_dev();
> > + dev = alloc_c_can_dev(C_CAN_NO_OF_OBJECTS);
> > if (!dev) {
> > ret = -ENOMEM;
> > goto out_iounmap;
> > diff --git a/drivers/net/can/c_can/c_can_platform.c
> > b/drivers/net/can/c_can/c_can_platform.c
> > index 05f425ceb53a..a5b9b1a93702 100644
> > --- a/drivers/net/can/c_can/c_can_platform.c
> > +++ b/drivers/net/can/c_can/c_can_platform.c
> > @@ -293,7 +293,7 @@ static int c_can_plat_probe(struct platform_device
> > *pdev)
> > }
> >
> > /* allocate the c_can device */
> > - dev = alloc_c_can_dev();
> > + dev = alloc_c_can_dev(C_CAN_NO_OF_OBJECTS);
> > if (!dev) {
> > ret = -ENOMEM;
> > goto exit;
> > --
> > 2.17.1
> >
