[Accton] Check TX_DISABLE support before access in onlp_sfpi_control_… by eric271110 · Pull Request #194 · accton/OpenNetworkLinux

eric271110 · 2026-06-15T00:50:50Z

…get/set

platform:
as7535_28xb
as7926-40xfb
as7946-30xb
as7946-74xkb
as9716_32d
as9736_64d
as9737-32db
as9817-64nb
as9817-64
as9926-24

merge from [Accton][as9726-32d] Check TX_DISABLE support before access in onlp_sfpi_control_get/set (#191)

Flat-memory CMIS modules (typically passive DACs / simple AOCs) only have lower memory + page 00h. The page select register is silently discarded, so a request for page 01h or 10h actually still reads page 00h vendor info, returning plausible-looking but meaningless bytes.
control_get previously did not check the TX Disable Supported advertising bit at all; it walked straight to page 10h and read byte 0x82. control_set checked the advertising bit but missed the flat-memory case.

Both paths now:

Read lower-memory byte 0x02 bit 7 (Flat_mem) first. Flat-memory modules return ONLP_STATUS_E_UNSUPPORTED immediately, without touching the page select registers.
For paged modules, check the TX Disable Supported advertising bit on page 01h byte 0x9B bit 1, mirroring the existing set path.
control_get's restore block preserves E_UNSUPPORTED instead of overwriting it with E_INTERNAL.

Verified on a real switch via libonlp.onlp_sfp_control_get(): an unsupported / flat-memory module now returns -10 (E_UNSUPPORTED) instead of a stale lane-mask byte.

…get/set platform: as7535_28xb as7926-40xfb as7946-30xb as7946-74xkb as9716_32d as9736_64d as9737-32db as9817-64nb as9817-64 as9926-24 merge from [Accton][as9726-32d] Check TX_DISABLE support before access in onlp_sfpi_control_get/set (accton#191) 1. Flat-memory CMIS modules (typically passive DACs / simple AOCs) only have lower memory + page 00h. The page select register is silently discarded, so a request for page 01h or 10h actually still reads page 00h vendor info, returning plausible-looking but meaningless bytes. 2. control_get previously did not check the TX Disable Supported advertising bit at all; it walked straight to page 10h and read byte 0x82. control_set checked the advertising bit but missed the flat-memory case. Both paths now: - Read lower-memory byte 0x02 bit 7 (Flat_mem) first. Flat-memory modules return ONLP_STATUS_E_UNSUPPORTED immediately, without touching the page select registers. - For paged modules, check the TX Disable Supported advertising bit on page 01h byte 0x9B bit 1, mirroring the existing set path. - control_get's restore block preserves E_UNSUPPORTED instead of overwriting it with E_INTERNAL. Verified on a real switch via libonlp.onlp_sfp_control_get(): an unsupported / flat-memory module now returns -10 (E_UNSUPPORTED) instead of a stale lane-mask byte. Signed-off-by: Eric Yang <eric_yang@accton.com>

platform: as7535_28xb as7926-40xfb as7946-30xb as7946-74xkb as9716_32d as9736_64d as9737-32db as9817-64nb as9817-64 as9726-32d as9926-24 Signed-off-by: Eric Yang <eric_yang@accton.com>

platform: as9926-24db Signed-off-by: Eric Yang <eric_yang@accton.com>

Page 01h (Advertising) is unbanked, so writing the bank-select register before switching to it has no effect. Page 10h (Lane Control) is banked, so the bank-select must be set before switching to it. Move the QSFP_DD_EEPROM_OFFSET_BANK_SELECT write into the TX_DISABLE-supported branch, immediately before the page-select to QSFP_DD_PAGE_LANE_CTRL, in both the set and get paths. platform: as7535-28xb as7926-40xfb as7946-30xb as7946-74xkb as9716_32d as9726-32d as9736-64d as9737-32db as9817-64 as9817-64-nb as9926-24d as9926-24db Signed-off-by: Eric Yang <eric_yang@accton.com>

…sary error logs merge from [Accton][as9726-32d] Route SFP errors to syslog, drop unsupported-state logs accton#197 platform: as7535-28xb as7926-40xfb as7946-30xb as7946-74xkb as9716_32d as9736-64d as9737-32db as9817-64-nb as9817-64 as9926-24d as9926-24db Changes: - Replace AIM_LOG_ERROR with syslog(LOG_ERR, ...) for consistent logging - Remove flat-memory module unsupported log messages - Remove tx_disable not supported log messages - Add #include <syslog.h> Signed-off-by: Eric Yang <eric_yang@accton.com>

vincentchiang-ec

test

vincentchiang-ec

Nice port overall — flat_mem detection, advertising-bit gating, bank-select order and the syslog cleanup all landed cleanly on most platforms. Two systematic issues showed up during review, and I've left inline notes at each site.

1. Restore-write clobbers E_UNSUPPORTED / OK (5 platforms × set+get = 10 sites). The final writeb(page_select, ADMIN_INFO) in the QSFP-DD block sets rv = E_INTERNAL on failure, which overwrites the rv = E_UNSUPPORTED we just carefully set for flat-memory / non-advertising modules (and also flips a successful write to E_INTERNAL). Regresses the exact behaviour PR #191 was written to introduce. Reference fix pattern is in as9726-32d sfpi.c (restore: label + rv = (rv == E_UNSUPPORTED) ? rv : E_INTERNAL) — please mirror it. Affected: as9716_32d, as9736-64d, as9926-24d, as9926-24db, as7926-40xfb.

2. as9817-64 / as9817-64-nb early-return pattern (2 platforms × set+get = 4 sites). These files use return E_INTERNAL inline in each error branch instead of the restore: label. The E_UNSUPPORTED paths (return E_UNSUPPORTED after restore) are fine — but the final "success" restore does return E_INTERNAL on restore failure, so a successful set/get becomes E_INTERNAL if only the ADMIN_INFO write itself fails. Same fix idea: either preserve the pre-restore return value or drop the failure-→ E_INTERNAL on the restore-only branch.

3. Missing & 0xff mask on *value = tx_disable (as9817-64 / as9817-64-nb only). Everywhere else the port applies *value = (tx_dis & 0xff) to avoid the sign-extension from dev_readb's int return; these two platforms still assign the raw signed int, so a byte with bit7 set will surface as a negative value in value. Trivial mask fix.

Nothing else looks off — flat-mem check offsets, page 01h advertising bit, bank-select-before-page-10h all match the reference on every platform.

vincentchiang-ec · 2026-07-08T08:01:04Z

-                                AIM_LOG_ERROR("Unable to write tx_disable status to port(%d): write page to eeprom fail\r\n", 
+                                syslog(LOG_ERR, "Unable to write tx_disable status to port(%d): write page to eeprom fail", 
                                        port);
                                rv = ONLP_STATUS_E_INTERNAL;


Restore write clobbers E_UNSUPPORTED (control_set path). When the TX_DISABLE_SUPPORT advertising bit is clear a few lines above, rv is set to E_UNSUPPORTED; this block then unconditionally overwrites it to E_INTERNAL if the ADMIN_INFO restore itself fails. Same thing happens on the successful write path (turns rv = OK into E_INTERNAL).

Suggested fix — mirror the as9726-32d pattern in this repo:

restore: if ((onlp_sfpi_dev_writeb(port, PORT_EEPROM_DEVADDR, QSFP_DD_EEPROM_OFFSET_PAGE_SELECT, QSFP_DD_PAGE_ADMIN_INFO)) < 0) { syslog(LOG_ERR, "Failed to restore Page Select to Admin Info on port(%d)!", port); } if (rv < 0) { rv = (rv == ONLP_STATUS_E_UNSUPPORTED) ? rv : ONLP_STATUS_E_INTERNAL; }

Fork missed this platform — same pattern as the four flagged in the other comments.

vincentchiang-ec · 2026-07-08T08:01:04Z

-                        AIM_LOG_ERROR("Unable to read tx_disable status from port(%d): write page to eeprom fail\r\n", 
+                        syslog(LOG_ERR, "Unable to read tx_disable status from port(%d): write page to eeprom fail", 
                                port);
                        rv = ONLP_STATUS_E_INTERNAL;


Same restore-clobber issue on the control_get path — see the control_set comment above for the reference fix.

vincentchiang-ec · 2026-07-08T08:01:04Z

-                                    AIM_LOG_ERROR("Unable to write tx_disable status to port(%d): write page to eeprom fail\r\n", 
+                                    syslog(LOG_ERR, "Unable to write tx_disable status to port(%d): write page to eeprom fail", 
                                            port);
                                    rv = ONLP_STATUS_E_INTERNAL;


Restore write clobbers E_UNSUPPORTED (control_set path). When the earlier else { rv = ONLP_STATUS_E_UNSUPPORTED; } fires (advertising bit clear), this block promotes it to E_INTERNAL if the ADMIN_INFO restore itself fails. Also flips a successful write path (rv = OK) to E_INTERNAL.

Suggested fix — mirror the as9726-32d reference:

restore: if ((onlp_sfpi_dev_writeb(port, PORT_EEPROM_DEVADDR, QSFP_DD_EEPROM_OFFSET_PAGE_SELECT, QSFP_DD_PAGE_ADMIN_INFO)) < 0) { syslog(LOG_ERR, "Failed to restore Page Select to Admin Info on port(%d)!", port); } if (rv < 0) { rv = (rv == ONLP_STATUS_E_UNSUPPORTED) ? rv : ONLP_STATUS_E_INTERNAL; }

vincentchiang-ec · 2026-07-08T08:01:04Z

-                                AIM_LOG_ERROR("Unable to read tx_disable status from port(%d): write page to eeprom fail\r\n", 
+                                syslog(LOG_ERR, "Unable to read tx_disable status from port(%d): write page to eeprom fail", 
                                        port);
                                rv = ONLP_STATUS_E_INTERNAL;


Same restore-clobber issue on the control_get path — see the control_set comment above.

vincentchiang-ec · 2026-07-08T08:01:04Z

-							AIM_LOG_ERROR("Unable to write tx_disable status to port(%d): write page to eeprom fail\r\n",
+							syslog(LOG_ERR, "Unable to write tx_disable status to port(%d): write page to eeprom fail",
 									port);
 							rv = ONLP_STATUS_E_INTERNAL;


Restore write clobbers E_UNSUPPORTED (control_set). Please mirror the as9726-32d restore: label + rv = (rv == E_UNSUPPORTED) ? rv : E_INTERNAL pattern — same story as the as9716_32d comment.

vincentchiang-ec · 2026-07-08T08:01:04Z

-                                AIM_LOG_ERROR("Unable to read tx_disabled status from port(%d) : write page to eeprom fail\r\n", 
+                                syslog(LOG_ERR, "Unable to read tx_disabled status from port(%d) : write page to eeprom fail", 
                                        port);
                                return ONLP_STATUS_E_INTERNAL;


Successful control_get reported as E_INTERNAL when the restore write fails — same pattern as line 332 on the set side. Same fix suggestion.

vincentchiang-ec · 2026-07-08T08:01:04Z

                                if(onlp_sfpi_dev_writeb(port, PORT_EEPROM_DEVADDR, QSFP_DD_EEPROM_OFFSET_PAGE_SELECT, QSFP_DD_PAGE_ADMIN_INFO) <0){
-                                    AIM_LOG_ERROR("Unable to write tx_disable status to port(%d): write page to eeprom fail\r\n", port);
+                                    syslog(LOG_ERR, "Unable to write tx_disable status to port(%d): write page to eeprom fail", port);
                                    return ONLP_STATUS_E_INTERNAL;


Successful control_set reported as E_INTERNAL when the restore write fails. Every earlier error branch already return E_INTERNAL after its own restore, so this final restore only runs on the OK path — returning E_INTERNAL here turns a real success into a caller-visible failure. Same fix idea as the as9817-64 comment: either drop the return E_INTERNAL here or adopt the restore: label pattern from as9726-32d.

vincentchiang-ec · 2026-07-08T08:01:04Z

+                                    syslog(LOG_ERR, "Unable to read tx_disabled status from port(%d) : read TX disable from eeprom fail", port);
+                                    return ONLP_STATUS_E_INTERNAL;
+                                }
+                                *value = tx_disable;


Missing & 0xff mask on *value = tx_disable. Sign-extension bug — dev_readb returns int and a byte with bit 7 set surfaces as a negative value. Please apply the *value = (tx_disable & 0xff) mask used elsewhere in this PR.

vincentchiang-ec · 2026-07-08T08:01:04Z

                            if(onlp_sfpi_dev_writeb(port, PORT_EEPROM_DEVADDR, QSFP_DD_EEPROM_OFFSET_PAGE_SELECT, QSFP_DD_PAGE_ADMIN_INFO) <0){
-                                AIM_LOG_ERROR("Unable to read tx_disabled status from port(%d) : write page to eeprom fail\r\n", port);
+                                syslog(LOG_ERR, "Unable to read tx_disabled status from port(%d) : write page to eeprom fail", port);
                                return ONLP_STATUS_E_INTERNAL;


Successful control_get reported as E_INTERNAL when the restore write fails — same pattern as line 314. Same fix suggestion.

vincentchiang-ec · 2026-07-08T08:01:04Z

+                                syslog(LOG_ERR, "Unable to read tx_disabled status from port(%d) : read TX disable from eeprom fail", port);
                                return ONLP_STATUS_E_INTERNAL;
                            }
                            *value = tx_disable;


Same missing & 0xff mask on *value = tx_disable (QSFP+ path).

vincentchiang-ec · 2026-07-08T08:05:37Z

Follow-up context on the review above — I want to be fair about how much of this bug PR #194 actually introduced, since re-reading the pre-PR code changed my picture:

as9726-32d was already safe before PR #191 landed. It has used the goto restore + rv = (rv == E_UNSUPPORTED) ? rv : E_INTERNAL pattern for a long time. That's why it works as the reference here — PR #191 only added the flat-memory early-exit on top of an already-safe restore block.

The other 11 platforms use a different structure (inline writeb(ADMIN_INFO) with rv = E_INTERNAL on failure). Splitting the finding against that:

Path	Introduced by PR #194?
set — advertising bit clear → `E_UNSUPPORTED` clobbered	No, pre-existing. The 5 flagged platforms already had this bug on `master`; PR #194 didn't fix it (and I don't think it was expected to).
set — new flat-memory `E_UNSUPPORTED` clobbered	Yes, because PR #194 adds this new path into the already-broken restore.
get — `E_UNSUPPORTED` clobbered	Yes. `control_get` didn't produce `E_UNSUPPORTED` before, so this is fully a PR #194 introduction.
as9817-64 / as9817-64-nb — OK → `E_INTERNAL` on restore fail	No, pre-existing early-return structure.

So the practical takeaway isn't "PR #194 introduced all these bugs" — it's more that porting the flat-memory check via the restore: label + preserve-E_UNSUPPORTED pattern (i.e. actually mirroring as9726-32d structurally, not just its offsets) would fix both the newly-introduced and the pre-existing cases in one shot. That's the real value of adopting the reference pattern here — the fix isn't scope-creep, it's the same edit that makes the port complete.

(as9817-64 / as9817-64-nb are separate — their early-return structure is fine for E_UNSUPPORTED; only the OK-path return E_INTERNAL on restore-failure needs adjusting.)

…tten by page restore failure The final ADMIN_INFO page restore write could previously mask a successful or E_UNSUPPORTED result. This fix ensures the real result is preserved by refactoring the QSFP-DD TX_DISABLE control_set/get flow to the goto restore pattern. Also added value masks to prevent sign extension (QSFP-DD & 0xff, QSFP/QSFP+ & 0xf). Affected platforms: - as7926-40xfb - as9716_32d - as9736-64d - as9737-32db - as9817-64 - as9817-64-nb - as9926-24d - as9926-24db Signed-off-by: Eric Yang <eric_yang@accton.com>

eric271110 · 2026-07-13T07:22:42Z

@vincentchiang-ec

I've addressed the issues you pointed out above.

I also noticed that the missing & 0xff mask on tx_disable is a common issue rather than being limited to the originally flagged platforms, so I've added the mask across all platforms.

The following platforms have been refactored to use the goto restore structure:

as7926-40xfb
as9716_32d
as9736-64d
as9737-32db
as9817-64
as9817-64-nb
as9926-24d
as9926-24db

Thanks!

akenliu · 2026-07-17T06:11:44Z

+
+                restore:
+                    if (onlp_sfpi_dev_writeb(port, PORT_EEPROM_DEVADDR, QSFP_DD_EEPROM_OFFSET_PAGE_SELECT, QSFP_DD_PAGE_ADMIN_INFO) < 0) {
+                        syslog(LOG_ERR, "Unable to read tx_disable status from port(%d): write page to eeprom fail",


this action is not "Unable to read tx_disable status from port(%d)"

akenliu · 2026-07-17T06:14:00Z

+
+                    restore:
+                        if (onlp_sfpi_dev_writeb(port, PORT_EEPROM_DEVADDR, QSFP_DD_EEPROM_OFFSET_PAGE_SELECT, QSFP_DD_PAGE_ADMIN_INFO) < 0) {
+                            syslog(LOG_ERR, "Unable to write tx_disable status to port(%d): write page to eeprom fail",


this action is not "Unable to read tx_disable status from port(%d)"

akenliu · 2026-07-17T06:17:00Z

                    }
                    if (rv < 0) {
-                        AIM_LOG_ERROR("Unable to write tx_disable status to port(%d)\r\n", port);
+                        syslog(LOG_ERR, "Unable to write tx_disable status to port(%d)", port);


Do we need this error log?
We already have another error log before "goto restore"

eric271110 force-pushed the add_sfp_functionality_linux6.1 branch from 087b196 to 08284d2 Compare June 15, 2026 09:10

eric271110 added 4 commits July 2, 2026 17:57

[Accton] remove redundant CMIS bank setting in control_get/set

95acf33

platform: as7535_28xb as7926-40xfb as7946-30xb as7946-74xkb as9716_32d as9736_64d as9737-32db as9817-64nb as9817-64 as9726-32d as9926-24 Signed-off-by: Eric Yang <eric_yang@accton.com>

[Accton]Add SFP functionality (LP mode, reset, TX disable)

2dc2a4d

platform: as9926-24db Signed-off-by: Eric Yang <eric_yang@accton.com>

eric271110 force-pushed the add_sfp_functionality_linux6.1 branch from b1fd057 to 1756289 Compare July 2, 2026 10:03

vincentchiang-ec reviewed Jul 8, 2026

View reviewed changes

eric271110 force-pushed the add_sfp_functionality_linux6.1 branch 4 times, most recently from 42814f4 to 9e430f2 Compare July 10, 2026 09:07

eric271110 force-pushed the add_sfp_functionality_linux6.1 branch from 9e430f2 to 096e616 Compare July 13, 2026 03:34

akenliu reviewed Jul 17, 2026

View reviewed changes

Uh oh!

Conversation

eric271110 commented Jun 15, 2026

Uh oh!

vincentchiang-ec left a comment

Choose a reason for hiding this comment

Uh oh!

vincentchiang-ec left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vincentchiang-ec commented Jul 8, 2026

Uh oh!

eric271110 commented Jul 13, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants