[PR] Minor net error handling improvements [nuttx]

["jpaali (via GitHub)" <gi...@apache.org>]

jpaali opened a new pull request, #15450:
URL: https://github.com/apache/nuttx/pull/15450

   ## Summary
   
   This PR includes two minor networking related error handling improvements.
   
   drivers/net/ksz9477.c:
   Return values of register writes are not checked in ksz9477_init() 
   which may lead to misbehaving ksz9477 ethernet switch in case of configuration failure.
   Check return values of register writes and abort init in case of error.
   
   arch/risc-v/src/mpfs/mpfs_ethernet.c:
   Error is not returned in mpfs_phyinit() in case of mpfs_i2cbus_initialize() failure.
   Return error in case of failure.
   
   ## Impact
   
   Gives possibility to react to initialization failures in ksz9477 / mpfs_ethernet.
   Check return values of register writes in drivers/net/ksz9477.c
   Return error value in case of i2c bus initialization failure in arch/risc-v/src/mpfs/mpfs_ethernet.c
   
   ## Testing
   
   Tested with custom mpfs board with ksz9477 ethernet switch.
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Minor net error handling improvements [nuttx]

["nuttxpr (via GitHub)" <gi...@apache.org>]

nuttxpr commented on PR #15450:
URL: https://github.com/apache/nuttx/pull/15450#issuecomment-2574807030

   [**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues)
   
   
   
   Yes, this PR generally meets the NuttX requirements, but could be improved.  Specifically, it's missing details in the Impact and Testing sections.
   
   
   Here's a breakdown of what's good and what needs improvement:
   
   **Good:**
   
   * **Clear Summary:** The summary explains the "why" (error handling improvements), "what" (checking return values), and "how" (aborting init on error, returning error codes).
   * **Concise Description:** The writing is clear and to the point.
   
   
   **Needs Improvement:**
   
   * **Impact - Be More Specific:**  While you mention the impact, you should be more explicit.  Instead of just saying "Gives possibility to react...", describe *how* the user would react.  For example: "Applications using the KSZ9477 driver can now check the return value of the initialization function and handle failures appropriately, such as retrying initialization or logging the error. Previously, failures would go unnoticed, potentially leading to network connectivity issues."  Similar detail is needed for the `mpfs_phyinit` change.  Answer all the "Impact on..." questions explicitly (even with "NO").
   
   * **Testing - Insufficient Detail:**
       * **Build Host:** Specify your OS, CPU architecture, and compiler version (e.g., "Linux, x86_64, GCC 12.2.1").
       * **Target:** Be very specific about your target.  "Custom mpfs board" isn't enough.  What's the SoC? What's the board configuration? (e.g., "Microchip MPFS-ICICLE-KIT with KSZ9477 configured via I2C").
       * **Logs:** You *must* provide logs.  "Tested with..." is not sufficient.  Show logs demonstrating the previous behavior (errors being ignored) and the new behavior (errors being handled).  Even simple logs showing successful initialization after the change are better than nothing.  Ideally, you should also try to *induce* failures (e.g., disconnect the KSZ9477) to demonstrate that your error handling works as intended.
   
   **Example Improvement for Testing Section:**
   
   ```
   Testing
   
   I confirm that changes are verified on local setup and works as intended:
   * Build Host(s): Linux, x86_64, GCC 12.2.1
   * Target(s): Microchip MPFS-ICICLE-KIT with KSZ9477 configured via I2C.
   
   Testing logs before change (KSZ9477 simulated failure - cable unplugged):
   
   ```
   [date/time] Network initialization...
   [date/time] KSZ9477 initialization complete (no error indication, despite failure).
   [date/time] Network connection failed.
   ```
   
   Testing logs after change (KSZ9477 simulated failure - cable unplugged):
   
   ```
   [date/time] Network initialization...
   [date/time] KSZ9477 initialization failed: -EIO (I/O error).
   [date/time] Network initialization aborted.
   ```
   
   Testing logs after change (successful initialization):
   
   ```
   [date/time] Network initialization...
   [date/time] KSZ9477 initialization complete.
   [date/time] Network connection successful.
   ```
   ```
   
   
   By adding these details, the PR will be much stronger and easier for reviewers to evaluate.
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Minor net error handling improvements [nuttx]

["xiaoxiang781216 (via GitHub)" <gi...@apache.org>]

xiaoxiang781216 merged PR #15450:
URL: https://github.com/apache/nuttx/pull/15450


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org