On Tue 28 Mar 2023 at 12:32, Peter Hong <peter_hong@xxxxxxxxxxxxx> 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.
