Hi Vincent,
On Sun, Jan 29, 2023 at 12:00:48AM +0900, Vincent MAILHOL wrote:
> Hi Markus,
>
> I did not have time for a deep review yet. But so far, I didn't find
> anything odd aside from two nitpicks.
>
> On Thu. 26 Jan 2023 à 04:53, Markus Schneider-Pargmann
> <msp@xxxxxxxxxxxx> a écrit :
> >
> > m_can supports submitting mulitple transmits with one register write.
> ^^^^^^^^
> multiple
>
> > This is an interesting option to reduce the number of SPI transfers for
> > peripheral chips.
> >
> > The m_can_tx_op is extended with a bool that signals if it is the last
> > transmission and the submit should be executed immediately.
> >
> > The worker then writes the skb to the FIFO and submits it only if the
> > submit bool is set. If it isn't set, the worker will write the next skb
> > which is waiting in the workqueue to the FIFO, etc.
> >
> > Signed-off-by: Markus Schneider-Pargmann <msp@xxxxxxxxxxxx>
> > ---
> >
> > Notes:
> > Notes:
> > - I ran into lost messages in the receive FIFO when using this
> > implementation. I guess this only shows up with my test setup in
> > loopback mode and maybe not enough CPU power.
> > - I put this behind the tx-frames ethtool coalescing option as we do
> > wait before submitting packages but it is something different than the
> > tx-frames-irq option. I am not sure if this is the correct option,
> > please let me know.
> >
> > drivers/net/can/m_can/m_can.c | 55 ++++++++++++++++++++++++++++++++---
> > drivers/net/can/m_can/m_can.h | 6 ++++
> > 2 files changed, 57 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
> > index c6a09369d1aa..99bfcfec3775 100644
> > --- a/drivers/net/can/m_can/m_can.c
> > +++ b/drivers/net/can/m_can/m_can.c
> > @@ -1504,6 +1504,9 @@ static int m_can_start(struct net_device *dev)
> > if (ret)
> > return ret;
> >
> > + netdev_queue_set_dql_min_limit(netdev_get_tx_queue(cdev->net, 0),
> > + cdev->tx_max_coalesced_frames);
> > +
> > cdev->can.state = CAN_STATE_ERROR_ACTIVE;
> >
> > m_can_enable_all_interrupts(cdev);
> > @@ -1813,8 +1816,13 @@ static netdev_tx_t m_can_tx_handler(struct m_can_classdev *cdev,
> > */
> > can_put_echo_skb(skb, dev, putidx, frame_len);
> >
> > - /* Enable TX FIFO element to start transfer */
> > - m_can_write(cdev, M_CAN_TXBAR, (1 << putidx));
> > + if (cdev->is_peripheral) {
> > + /* Delay enabling TX FIFO element */
> > + cdev->tx_peripheral_submit |= BIT(putidx);
> > + } else {
> > + /* Enable TX FIFO element to start transfer */
> > + m_can_write(cdev, M_CAN_TXBAR, BIT(putidx));
> > + }
> > cdev->tx_fifo_putidx = (++cdev->tx_fifo_putidx >= cdev->can.echo_skb_max ?
> > 0 : cdev->tx_fifo_putidx);
> > }
> > @@ -1827,6 +1835,17 @@ static netdev_tx_t m_can_tx_handler(struct m_can_classdev *cdev,
> > return NETDEV_TX_BUSY;
> > }
> >
> > +static void m_can_tx_submit(struct m_can_classdev *cdev)
> > +{
> > + if (cdev->version == 30)
> > + return;
> > + if (!cdev->is_peripheral)
> > + return;
> > +
> > + m_can_write(cdev, M_CAN_TXBAR, cdev->tx_peripheral_submit);
> > + cdev->tx_peripheral_submit = 0;
> > +}
> > +
> > static void m_can_tx_work_queue(struct work_struct *ws)
> > {
> > struct m_can_tx_op *op = container_of(ws, struct m_can_tx_op, work);
> > @@ -1835,11 +1854,15 @@ static void m_can_tx_work_queue(struct work_struct *ws)
> >
> > op->skb = NULL;
> > m_can_tx_handler(cdev, skb);
> > + if (op->submit)
> > + m_can_tx_submit(cdev);
> > }
> >
> > -static void m_can_tx_queue_skb(struct m_can_classdev *cdev, struct sk_buff *skb)
> > +static void m_can_tx_queue_skb(struct m_can_classdev *cdev, struct sk_buff *skb,
> > + bool submit)
> > {
> > cdev->tx_ops[cdev->next_tx_op].skb = skb;
> > + cdev->tx_ops[cdev->next_tx_op].submit = submit;
> > queue_work(cdev->tx_wq, &cdev->tx_ops[cdev->next_tx_op].work);
> >
> > ++cdev->next_tx_op;
> > @@ -1851,6 +1874,7 @@ static netdev_tx_t m_can_start_peripheral_xmit(struct m_can_classdev *cdev,
> > struct sk_buff *skb)
> > {
> > netdev_tx_t err;
> > + bool submit;
> >
> > if (cdev->can.state == CAN_STATE_BUS_OFF) {
> > m_can_clean(cdev->net);
> > @@ -1861,7 +1885,15 @@ static netdev_tx_t m_can_start_peripheral_xmit(struct m_can_classdev *cdev,
> > if (err != NETDEV_TX_OK)
> > return err;
> >
> > - m_can_tx_queue_skb(cdev, skb);
> > + ++cdev->nr_txs_without_submit;
> > + if (cdev->nr_txs_without_submit >= cdev->tx_max_coalesced_frames ||
> > + !netdev_xmit_more()) {
> > + cdev->nr_txs_without_submit = 0;
> > + submit = true;
> > + } else {
> > + submit = false;
> > + }
> > + m_can_tx_queue_skb(cdev, skb, submit);
> >
> > return NETDEV_TX_OK;
> > }
> > @@ -1993,6 +2025,7 @@ static int m_can_get_coalesce(struct net_device *dev,
> >
> > ec->rx_max_coalesced_frames_irq = cdev->rx_max_coalesced_frames_irq;
> > ec->rx_coalesce_usecs_irq = cdev->rx_coalesce_usecs_irq;
> > + ec->tx_max_coalesced_frames = cdev->tx_max_coalesced_frames;
> > ec->tx_max_coalesced_frames_irq = cdev->tx_max_coalesced_frames_irq;
> > ec->tx_coalesce_usecs_irq = cdev->tx_coalesce_usecs_irq;
> >
> > @@ -2037,6 +2070,18 @@ static int m_can_set_coalesce(struct net_device *dev,
> > netdev_err(dev, "tx-frames-irq and tx-usecs-irq can only be set together\n");
> > return -EINVAL;
> > }
> > + if (ec->tx_max_coalesced_frames > cdev->mcfg[MRAM_TXE].num) {
> > + netdev_err(dev, "tx-frames (%u) greater than the TX event FIFO (%u)\n",
> ^^^^
> ^^^^
> Avoid parenthesis.
> Ref: https://www.kernel.org/doc/html/latest/process/coding-style.html#printing-kernel-messages:
>
> Printing numbers in parentheses (%d) adds no value and should be avoided.
>
> This comment applies to other patches in the series as well.
Thank you for having a look and pointing this out, I wasn't aware of
that. I will fix these for the next version.
Best,
Markus
>
> > + ec->tx_max_coalesced_frames,
> > + cdev->mcfg[MRAM_TXE].num);
> > + return -EINVAL;
> > + }
> > + if (ec->tx_max_coalesced_frames > cdev->mcfg[MRAM_TXB].num) {
> > + netdev_err(dev, "tx-frames (%u) greater than the TX FIFO (%u)\n",
> > + ec->tx_max_coalesced_frames,
> > + cdev->mcfg[MRAM_TXB].num);
> > + return -EINVAL;
> > + }
> > if (ec->rx_coalesce_usecs_irq != 0 && ec->tx_coalesce_usecs_irq != 0 &&
> > ec->rx_coalesce_usecs_irq != ec->tx_coalesce_usecs_irq) {
> > netdev_err(dev, "rx-usecs-irq (%u) needs to be equal to tx-usecs-irq (%u) if both are enabled\n",
> > @@ -2047,6 +2092,7 @@ static int m_can_set_coalesce(struct net_device *dev,
> >
> > cdev->rx_max_coalesced_frames_irq = ec->rx_max_coalesced_frames_irq;
> > cdev->rx_coalesce_usecs_irq = ec->rx_coalesce_usecs_irq;
> > + cdev->tx_max_coalesced_frames = ec->tx_max_coalesced_frames;
> > cdev->tx_max_coalesced_frames_irq = ec->tx_max_coalesced_frames_irq;
> > cdev->tx_coalesce_usecs_irq = ec->tx_coalesce_usecs_irq;
> >
> > @@ -2064,6 +2110,7 @@ static const struct ethtool_ops m_can_ethtool_ops = {
> > .supported_coalesce_params = ETHTOOL_COALESCE_RX_USECS_IRQ |
> > ETHTOOL_COALESCE_RX_MAX_FRAMES_IRQ |
> > ETHTOOL_COALESCE_TX_USECS_IRQ |
> > + ETHTOOL_COALESCE_TX_MAX_FRAMES |
> > ETHTOOL_COALESCE_TX_MAX_FRAMES_IRQ,
> > .get_ts_info = ethtool_op_get_ts_info,
> > .get_coalesce = m_can_get_coalesce,
> > diff --git a/drivers/net/can/m_can/m_can.h b/drivers/net/can/m_can/m_can.h
> > index bfef2c89e239..e209de81b5a4 100644
> > --- a/drivers/net/can/m_can/m_can.h
> > +++ b/drivers/net/can/m_can/m_can.h
> > @@ -74,6 +74,7 @@ struct m_can_tx_op {
> > struct m_can_classdev *cdev;
> > struct work_struct work;
> > struct sk_buff *skb;
> > + bool submit;
> > };
> >
> > struct m_can_classdev {
> > @@ -103,6 +104,7 @@ struct m_can_classdev {
> > u32 active_interrupts;
> > u32 rx_max_coalesced_frames_irq;
> > u32 rx_coalesce_usecs_irq;
> > + u32 tx_max_coalesced_frames;
> > u32 tx_max_coalesced_frames_irq;
> > u32 tx_coalesce_usecs_irq;
> >
> > @@ -117,6 +119,10 @@ struct m_can_classdev {
> > int tx_fifo_size;
> > int next_tx_op;
> >
> > + int nr_txs_without_submit;
> > + /* bitfield of fifo elements that will be submitted together */
> > + u32 tx_peripheral_submit;
> > +
> > struct mram_cfg mcfg[MRAM_CFG_NUM];
> > };
