Thanks for your input Peter, > The IMXRT SocketCAN driver is based on the S32K1XX driver which had limited > amount of mailboxes. > So they had to be cleared as quick as possible to make sure we don't drop > packets if all mailboxes get full. I did get that from the git history, however I hope we can find a way to not unnecessarily cripple newer more capable hardware while still supporting the more limited case.
> > On the IMXRT it's different since in non-FD mode you've got a lot of > mailboxes. > Indeed the solution would be to schedule a workqueue to empty the mailboxes > and then instead of net_trylock do a net_lock so the workqueue suspends if > doesn't get the lock but when it gets it immediately clears the lock. > Still with the workqueue you've to be careful that you don't drop packets. > > The better solution is to utilize the new features of the IOB buffer rewrite. > I haven't look into the details but it allows to pre-allocate IOB so that the > workqueue/irq immediately can push the packets to the network. > This would work with hardware a low amount of mailboxes and would even open > the possibility to use the DMA as well to transfer the CAN packets. I did look into it a bit, and it is possible to use the IOB buffers to handle the "temporary storage", or rather remove the need for it. Unfortunately at this point we have a released software based on 10.2 (before the IOB rewrite of the network layer), and need to implement this as a bugfix to this version. I did review the changes between latest master and 10.2, and even though things have changed so an IOB can be passed from the low-level driver to the upper levels, the SocketCAN drivers all still does net_trylock and calls can_input/can_callback from interrupt contexts, which means they miss notifying the socket recv-code in can_recvmsg. How does the following sound for a way forward: - I do implement (locally in our repo) the change to move from in-interrupt processing to workqueue based processing, can_input/can_callback is changed to expect being called from workqueue and therefor can use net_lock() etc which handles both the re-ordering and stall issues right now. - Once we bump our local NuttX version, which is expected within a few months, I'll forward-port this patch and provide it to upstream. It will then work out of the box for processors such as iMXRT that have many mailboxes. For other examples such as S32K1XX, or in all as an optimization, the driver can allocate IOBs and in the interrupt queue the data to IOB within the ISR, then the workqueue passes the IOB(-chain) to the upper layers. I could implement it for all SocketCAN drivers, its not that many, but would require help to test on non-iMXRT as I don't have access to them. What I don't want to do is implement a local solution for now, then we bump NuttX and forward port it but the solution is outright rejected if I try to submit it upstream. Tweaks, adjustments etc sure but the general approach I hope can be sound. Regards Marten Svanfeldt
