Date:   Tue, 28 Mar 2023 13:49:05 +0900
From:   Vincent MAILHOL <mailhol.vincent@...adoo.fr>
To:     Peter Hong <peter_hong@...tek.com.tw>
Cc:     wg@...ndegger.com, mkl@...gutronix.de,
        michal.swiatkowski@...ux.intel.com, Steen.Hegelund@...rochip.com,
        davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
        pabeni@...hat.com, frank.jungclaus@....eu,
        linux-kernel@...r.kernel.org, linux-can@...r.kernel.org,
        netdev@...r.kernel.org, hpeter+linux_kernel@...il.com
Subject: Re: [PATCH V3] can: usb: f81604: add Fintek F81604 support

On Tue 28 Mar 2023 at 12:32, Peter Hong <peter_hong@...tek.com.tw> wrote:
> Hi Vincent,
>
> Vincent MAILHOL 於 2023/3/27 下午 06:27 寫道:
> > eff->id is a 32 bit value. It is not aligned. So, you must always use
> > {get|set}_unaligned_be32() to manipulate this value.
> > N.B. on x86 architecture, unaligned access is fine, but some other
> > architecture may throw a fault. Read this for more details:
> >
> >    https://docs.kernel.org/arm/mem_alignment.html
>
> for the consistency of the code, could I also add get/put_unaligned_be16
> in SFF
> sections ?

It is not needed. OK to mix.

> >> +static int f81604_set_reset_mode(struct net_device *netdev)
> >> +{
> >> +       struct f81604_port_priv *priv = netdev_priv(netdev);
> >> +       int status, i;
> >> +       u8 tmp;
> >> +
> >> +       /* disable interrupts */
> >> +       status = f81604_set_sja1000_register(priv->dev, netdev->dev_id,
> >> +                                            SJA1000_IER, IRQ_OFF);
> >> +       if (status)
> >> +               return status;
> >> +
> >> +       for (i = 0; i < F81604_SET_DEVICE_RETRY; i++) {
> > Thanks for removing F81604_USB_MAX_RETRY.
> >
> > Yet, I still would like to understand why you need one hundred tries?
> > Is this some paranoiac safenet? Or does the device really need so many
> > attempts to operate reliably? If those are needed, I would like to
> > understand the root cause.
>
> This section is copy from sja1000.c. In my test, the operation/reset may
> retry 1 times.
> I'll reduce it from 100 to 10 times.

Is it because the device is not ready? Does this only appear at
startup or at random?

>
> >> +       int status, len;
> >> +
> >> +       if (can_dropped_invalid_skb(netdev, skb))
> >> +               return NETDEV_TX_OK;
> >> +
> >> +       netif_stop_queue(netdev);
> > In your driver, you send the CAN frames one at a time and wait for the
> > rx_handler to restart the queue. This approach dramatically degrades
> > the throughput. Is this a device limitation? Is the device not able to
> > manage more than one frame at a time?
> >
>
> This device will not NAK on TX frame not complete, it only NAK on TX
> endpoint
> memory not processed, so we'll send next frame unitl TX complete(TI)
> interrupt
> received.
>
> The device can polling status register via TX/RX endpoint, but it's more
> complex.
> We'll plan to do it when first driver landing in mainstream.

OK for me to have this as a next step. Marc, what do you think?

> >> +static int f81604_set_termination(struct net_device *netdev, u16 term)
> >> +{
> >> +       struct f81604_port_priv *port_priv = netdev_priv(netdev);
> >> +       struct f81604_priv *priv;
> >> +       u8 mask, data = 0;
> >> +       int r;
> >> +
> >> +       priv = usb_get_intfdata(port_priv->intf);
> >> +
> >> +       if (netdev->dev_id == 0)
> >> +               mask = F81604_CAN0_TERM;
> >> +       else
> >> +               mask = F81604_CAN1_TERM;
> >> +
> >> +       if (term == F81604_TERMINATION_ENABLED)
> >> +               data = mask;
> >> +
> >> +       mutex_lock(&priv->mutex);
> > Did you witness a race condition?
> >
> > As far as I know, this call back is only called while the network
> > stack big kernel lock (a.k.a. rtnl_lock) is being hold.
> > If you have doubt, try adding a:
> >
> >    ASSERT_RTNL()
> >
> > If this assert works, then another mutex is not needed.
>
> It had added ASSERT_RTNL() into f81604_set_termination(). It only assert
> in f81604_probe() -> f81604_set_termination(), not called via ip command:
>      ip link set dev can0 type can termination 120
>      ip link set dev can0 type can termination 0
>
> so I'll still use mutex on here.

Sorry, do you mean that the assert throws warnings for f81604_probe()
-> f81604_set_termination() but that it is OK (no warning) for ip
command?

I did not see that you called f81604_set_termination() internally.
Indeed, rtnl_lock is not held in probe(). But I think it is still OK.
In f81604_probe() you call f81604_set_termination() before
register_candev(). If the device is not yet registered,
f81604_set_termination() can not yet be called via ip command. Can you
describe more precisely where you think there is a concurrency issue?
I still do not see it.

> >> +               port_priv->can.do_get_berr_counter = f81604_get_berr_counter;
> >> +               port_priv->can.ctrlmode_supported =
> >> +                       CAN_CTRLMODE_LISTENONLY | CAN_CTRLMODE_3_SAMPLES |
> >> +                       CAN_CTRLMODE_ONE_SHOT | CAN_CTRLMODE_BERR_REPORTING |
> >> +                       CAN_CTRLMODE_CC_LEN8_DLC | CAN_CTRLMODE_PRESUME_ACK;
> > Did you test the CAN_CTRLMODE_CC_LEN8_DLC feature? Did you confirm
> > that you can send and receive DLC greater than 8?
>
> Sorry, I had misunderstand the define. This device is only support 0~8
> data length,
  ^^^^^^^^^^^

Data length or Data Length Code (DLC)? Classical CAN maximum data
length is 8 but maximum DLC is 15 (and DLC 8 to 15 mean a data length
of 8).

> so I'll remove CAN_CTRLMODE_CC_LEN8_DLC in future patch.

Powered by blists - more mailing lists