Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <5bdee736-7868-81c3-e63f-a28787bd0007@fintek.com.tw>
Date:   Tue, 28 Mar 2023 11:18:44 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.9.0
Subject: Re: [PATCH V3] can: usb: f81604: add Fintek F81604 support
Content-Language: en-US
To:     Vincent MAILHOL <mailhol.vincent@wanadoo.fr>
CC:     <wg@grandegger.com>, <mkl@pengutronix.de>,
        <michal.swiatkowski@linux.intel.com>,
        <Steen.Hegelund@microchip.com>, <davem@davemloft.net>,
        <edumazet@google.com>, <kuba@kernel.org>, <pabeni@redhat.com>,
        <frank.jungclaus@esd.eu>, <linux-kernel@vger.kernel.org>,
        <linux-can@vger.kernel.org>, <netdev@vger.kernel.org>,
        <hpeter+linux_kernel@gmail.com>
References: <20230327051048.11589-1-peter_hong@fintek.com.tw>
 <CAMZ6Rq+ps1tLii1VfYyAqfD4ck_TGWBUo_ouK_vLfhoNEg-BPg@mail.gmail.com>
From:   Peter Hong <peter_hong@fintek.com.tw>
In-Reply-To: <CAMZ6Rq+ps1tLii1VfYyAqfD4ck_TGWBUo_ouK_vLfhoNEg-BPg@mail.gmail.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

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 ?

>> +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.


>> +       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.

>> +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.

>> +               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,
so I'll remove CAN_CTRLMODE_CC_LEN8_DLC in future patch.

Thanks,