Gavin, we really appreciate the time you put into reviewing and improving the patches.
I have done some more testing with focus on the EoE part and in one setup I have now observed that the EtherCAT-EoE thread can end up in an forever uninterruptable sleep. I can reproduce this in both the patch serie from 20160502/20160610 and in the newest patch serie from 20160613. I will investigate this further. A few comments to below patches: Patch 0037: The versioning is a bit tricky and to be safe we have to use version 3.17 where the detect_deadlock argument for sure is dropped. If using the RT kernel patch, the argument must be dropped from 3.14.34-rt32, but it is not possible to do a check on the rt32 version part that is just given by a tag. I will update the patch to use version 3.17 and maybe also allow the detect_deadlock argument to be dropped via configure.ac so we can support the Linux Real time versions from 3.14.34-rt32. Patch 0032: Yes, now I recall that we discussed this about a year ago and it is actually the commit message that is a bit wrong as we do actually enter INIT + ERROR after the Master has requested PREOP. I will update the commit message to make this a bit more clear. Patch 0034: Yes, I should have known that this of cause is unnecessary for the read part (as I have previously worked with the SII cache part). I will update the patch and remove this part again. Thanks, Knud From: Gavin Lambert [mailto:gav...@compacsort.com] Sent: 13. juni 2016 08:34 To: Knud Baastrup; Florian Pose; 'David Page' Cc: etherlab-dev@etherlab.org<mailto:etherlab-dev@etherlab.org> Subject: RE: [PATCH] Default branch patchset re-applied on 5a70ff Ok, I've looked through the new patches now. Attached is my refresh of them. Mostly it's just inclusion of a series file and cleaning up the commit messages to be HG-safe (HG doesn't quite like some of the things that git adds, such as diffstats and a few other artefacts). Other changes include: * Adopted the filenames from Knud's set. * Replaced 0037-Breaking-change-rt_mutx_lock_interruptible-calls-for.patch with a version that isn't a breaking change (assuming the version numbers in the commit message were correct; I haven't verified this). This could probably be folded into patch 0004, but I left it separate for clarity. * Added 0040-rescan-check-revision.patch. This modifies patch 0013 in four ways: 1. The SII cache-and-reuse behaviour can be disabled via -disable-sii-cache at configure time, rather than requiring modifying a header file. 2. The revision number is also verified before using the cached version (this resolves some issues when the device firmware is upgraded). 3. If both the alias and serial are read as 0, it will no longer bother reading the vendor/product/revision, as it is now known that the SII is not in the cache. 4. Several similar states are consolidated into one. * Added 0041-load-sii-from-file.patch, from Graeme Foot's recent patch; but I've made the following modifications: 1. The functionality is disabled by default. 2. At configure time, you can use -enable-sii-override to activate it, using the standard udev/hotplug lookup process. 3. At configure time, you can use -enable-sii-override=/lib/firmware (or another path) to activate it using a direct loading method. 4. A bug where it would fail to load 6 words of the SII when not loading from a file has been corrected. 5. Several places where it exited the state prematurely have been corrected. 6. It will cooperate as expected with patch 0013, although note that it's not as efficient as it could be (it will reload some of the values that patch 0013 already read when checking the SII cache; but trying to improve this would make the code really awkward). * Added 0042-load-sii-from-file-async.patch, which makes the above firmware loading asynchronous so it doesn't block the master thread, since that makes me uncomfortable. Again this could be folded into the prior patch but I left it separate so that the differences are apparent and easier to review. I'm also pondering whether the firmware loading stuff (or perhaps even the whole SII loading stuff including the reuse cache) should be broken out into a separate sub-FSM file. It's not really complicated but it's a bit wordy, particularly with the direct-loading support included. Open to suggestions here. * (Note these additions aren't breaking changes, but I didn't want to renumber the patches just because of that. They should be able to apply prior to the breaking change patches if you want to try it that way.) Some other thoughts about the new patches (I haven't modified anything for these): * 0032-Correction-to-Clear-slave-mailboxes-after-a-re-scan.patch: As previously mentioned, the stated circumstances should never happen, as a slave is not permitted to refuse to enter INIT. But the patch itself is probably harmless. * 0034-Tool-Withdraw-EEPROM-control-for-SII-read-write.patch: This seems like it should have some conditioning on EC_SII_ASSIGN, since unless that is enabled the SII should always be assigned to ECAT anyway. Additionally the changes to CommandSiiRead.cpp are completely unnecessary (or don't work as intended), as this doesn't access the device - it reads the SII contents as cached during scanning (even before patch 0013 happened). I also wonder if perhaps the mode switch on writing should be done in the state machines instead, so that it can be switched to ECAT only just prior to the write and then restored afterwards. Please let me know if you have any questions or suggestions, or if there's a different way I can present these that would help them get merged faster. :) From: Gavin Lambert Sent: Sunday, 12 June 2016 01:25 To: Knud Baastrup <k...@deif.com<mailto:k...@deif.com>>; Florian Pose <f...@igh.de<mailto:f...@igh.de>>; 'David Page' <dave.p...@gleeble.com<mailto:dave.p...@gleeble.com>> Subject: Re: [PATCH] Default branch patchset re-applied on 5a70ff Heh, looks like you beat me to it - I was in the process of rebasing to the same point (along with some additional tweaks - in particular I'm testing the loadable SII patch Graeme provided a few weeks ago. Sadly it has some bugs but I think I have it nearly ready). I'm away from my test setup ATM but I'll have a look at these next week. Sent from my Samsung Galaxy smartphone. -------- Original message -------- From: Knud Baastrup <k...@deif.com<mailto:k...@deif.com>> Date: 11/06/2016 00:55 (GMT+12:00) To: Florian Pose <f...@igh.de<mailto:f...@igh.de>>, 'David Page' <dave.p...@gleeble.com<mailto:dave.p...@gleeble.com>>, Gavin Lambert <gavin.lamb...@compacsort.com<mailto:gavin.lamb...@compacsort.com>> Cc: etherlab-dev@etherlab.org<mailto:etherlab-dev@etherlab.org> Subject: [PATCH] Default branch patchset re-applied on 5a70ff Hi all, I have re-applied the patch serie from Gavin Lambert on the latest commit on default branch (5a70ff from 2016-05-04) and as well added some additional (minor) patches (see the short explanation in the patch it-selves). I have placed the "Breaking change" patches in the end and done some extensive testing including all patches up to and including patch 0037. The tests have involved a lot of EoE traffic tests that all passed successfully. The patches are created with GIT (and named according to the commit message) but I believe they can easily be applied on HG as well. I have as well cleaned the patches for all white-space related issues. The patch serie is quite long and a bit cumbersome to maintain so I hope we soon can get more of them included on the default branch. * 0001-Distributed-Clock-fixes-and-helpers.patch (same as 0001-graemef-dc_fixes_and_helpers.patch) * 0002-Distributed-Clock-fixes-from-Jun-Yuan.patch (same as 0002-junyuan-dc_sync_issues.patch) * 0003-Do-not-reuse-the-index-of-a-pending-datagram.patch (same as 0003-frank-index-reuse.patch) * 0004-If-enable-rtmutex-use-rtmutexes-instead-of-semaphore.patch (same as 0004-gavinl-Semaphores-replaced-with-mutexes.patch) * 0005-Support-for-multiple-mailbox-protocols.patch (same as 0005-knud-Support-for-multiple-mailbox-protocols.patch) * 0006-Await-SDO-dictionary-to-be-fetched.patch (same as 0006-knud-Await-SDO-dictionary-to-be-fetched.patch) * 0007-Protection-of-external-datagram-queue.patch (same as 0007-knud-Protection-of-external-datagram-queue.patch) * 0008-Clear-slave-mailboxes-after-a-re-scan.patch (same as 0008-knud-Clear-slave-mailboxes-after-a-re-scan.patch) * 0009-Skip-output-statistics-during-re-scan.patch (same as 0009-knud-Skip-output-statistics-during-re-scan.patch) * 0010-Sdo-directory-now-only-fetched-on-request.patch (same as 0010-knud-Sdo-directory-now-only-fetched-on-request.patch) * 0011-Master-locks-to-avoid-corrupted-datagram-queue.patch (same as 0011-knud-Master-locks-to-avoid-corrupted-datagram-queue.patch) * 0012-Ignore-mailbox-settings-if-corrupted-sii-file.patch (same as 0012-knud-Ignore-mailbox-settings-if-corrupted-sii-file.patch) * 0013-Improved-EtherCAT-rescan-performance.patch (same as 0013-knud-Improved-EtherCAT-rescan-performance.patch) * 0014-Fix-NOHZ-local_softirq_pending-08-warning.patch (same as 0016-knud-Fix-NOHZ-local_softirq_pending-08-warning.patch) * 0015-EoE-processing-is-now-only-allowed-in-state-PREOP.patch (same as 0017-knud-EoE-processing-is-now-only-allowed-in-state-PREOP-SA.patch) * 0016-Prevent-abandoning-the-mailbox-state-machines-early-.patch (same as 0101-gavinl-mbox-finish-sm.patch) * 0017-Make-busy-logging-a-little-less-irritating.patch (same as 0102-gavinl-sdo_logging_verbosity.patch) * 0018-Clear-configuration-on-deactivate-even-if-not-activa.patch (same as 0103-gavinl-clear_on_deactivate.patch) * 0019-After-a-comms-interruption-allow-SAFEOP-OP-directly-.patch (same as 0104-gavinl-quick_op_watchdog.patch) * 0020-Adding-some-more-state-to-avoid-calling-the-more-exp.patch (same as 0105-gavinl-more_state.patch) * 0021-Detect-bypassed-ports-timestamp-not-updated.patch (same as 0106-gavinl-topology_bypass.patch) * 0022-Calculate-most-likely-upstream-port-for-each-slave.patch (same as 0107-gavinl-topology_upstream.patch) * 0023-Print-redundancy-device-name-with-ring-positions-as-.patch (same as0108-gavinl-redundant_device.patch) * 0024-RX-does-not-reset-watchdog-breaks-redundancy.patch (same as 0109-gavinl-e1000e_watchdog.patch) * 0025-Add-command-to-request-hardware-reboot-for-slaves-th.patch (same as 0110-gavinl-reboot.patch) * 0026-Add-register-read-write-support.patch (same as 0111-gavinl-reg_readwrite.patch) * 0027-Display-more-info-about-the-register-requests-in-pro.patch (same as 0112-gavinl-reg_req_info.patch) * 0028-Support-SDO-upload-via-complete-access.patch (same as 0113-gavinl-sdo_complete_upload.patch) * 0029-Disable-DC-SYNC-before-updating-a-running-slave-s-sy.patch (same as 0116-gavinl-dc-sync-vs-sys-time.patch) * 0030-Use-call-back-functions.patch (NEW) * 0031-Added-newline-to-syslog-message-MAC-address-derived.patch (NEW) * 0032-Correction-to-Clear-slave-mailboxes-after-a-re-scan.patch (NEW) * 0033-Corrected-debug-level-for-Aborting-dictionary.patch (NEW) * 0034-Tool-Withdraw-EEPROM-control-for-SII-read-write.patch (NEW) * 0035-Add-support-for-bringup-up-network-interface-when-st.patch (NEW) * 0036-Reduced-printing-to-avoid-syslog-spam.patch (NEW) * 0037-Breaking-change-rt_mutx_lock_interruptible-calls-for.patch (NEW) * 0038-Breaking-change-require-data-size-to-be-specified-in.patch (same as 0114-gavinl-sdo_write_size.patch) * 0039-Breaking-change-Support-for-SDO-complete-access-in-e.patch (same as 0115-gavinl-sdo_async_complete.patch) Venlig hilsen / Best regards, Knud Baastrup DEIF Wind Power Technology SW Developer Direct.: +45 9614 8458 E-mail: k...@deif.com<mailto:k...@deif.com> --------------------------------------------------------------- DEIF A/S Frisenborgvej 33 DK-7800 Skive Tel.: +45 9614 9614 Fax: +45 9614 9615 www.deifwindpower.com<http://www.deifwindpower.com/> "This email and any files transmitted with it are confidential and intended solely for the use of the individual or entity to whom they are addressed. Please notify the sender immediately by e-mail if you have received this e-mail by mistake and delete this e-mail from your system. If you are not the intended recipient you are notified that disclosing, copying, distributing or taking any action in reliance on the contents of this information is strictly prohibited"
_______________________________________________ etherlab-dev mailing list etherlab-dev@etherlab.org http://lists.etherlab.org/mailman/listinfo/etherlab-dev
