zkkkk12 commented on code in PR #16999: URL: https://github.com/apache/nuttx/pull/16999#discussion_r3413922150
########## arch/arm/src/stm32h7/stm32_mdio.c: ########## @@ -0,0 +1,232 @@ +/**************************************************************************** + * arch/arm/src/stm32h7/stm32_mdio.c + * + * SPDX-License-Identifier: Apache-2.0 + * + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. The + * ASF licenses this file to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance with the + * License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations + * under the License. + * + ****************************************************************************/ + +/**************************************************************************** + * Included Files + ****************************************************************************/ + +#include <arm_internal.h> + +/**************************************************************************** + * Pre-processor Definitions + ****************************************************************************/ + +#ifdef CONFIG_STM32H7_ETHMAC_REGDEBUG +static uint32_t stm32_getreg(uint32_t addr); +static void stm32_putreg(uint32_t val, uint32_t addr); +static void stm32_checksetup(void); +#else +# define stm32_getreg(addr) getreg32(addr) +# define stm32_putreg(val,addr) putreg32(val,addr) +# define stm32_checksetup() +#endif + +/**************************************************************************** + * Included Files + ****************************************************************************/ + +#include <nuttx/mutex.h> +#include <nuttx/config.h> + +#include <debug.h> +#include <errno.h> +#include <inttypes.h> + +#include "stm32_mdio.h" +#include "hardware/stm32_ethernet.h" + +/**************************************************************************** + * Private Types + ****************************************************************************/ + +struct stm32_mdio_bus_s +{ + struct mdio_lowerhalf_s *lower; + + /* MDIO bus timeout in milliseconds */ + + int timeout; +}; + +/**************************************************************************** + * Private Function Prototypes + ****************************************************************************/ + +static int stm32_c22_read(struct mdio_lowerhalf_s *dev, uint8_t phydev, + uint8_t regaddr, uint16_t *value); + +static int stm32_c22_write(struct mdio_lowerhalf_s *dev, uint8_t phydev, + uint8_t regaddr, uint16_t value); + +/**************************************************************************** + * Private Data + ****************************************************************************/ + +const struct mdio_ops_s g_stm32_mdio_ops = +{ + .read = stm32_c22_read, + .write = stm32_c22_write, + .reset = NULL, +}; + +struct mdio_lowerhalf_s g_stm32_mdio_lowerhalf = +{ + .ops = &g_stm32_mdio_ops +}; + +struct stm32_mdio_bus_s g_stm32_mdio_bus = +{ + .lower = &g_stm32_mdio_lowerhalf, + .timeout = 10 +}; + +/**************************************************************************** + * Private Functions + ****************************************************************************/ + +static int stm32_c22_read(struct mdio_lowerhalf_s *dev, uint8_t phydev, + uint8_t regaddr, uint16_t *value) +{ + int to; + uint32_t regval; + + int retval = -ETIMEDOUT; + struct stm32_mdio_bus_s *priv = (struct stm32_mdio_bus_s *)dev; Review Comment: Hi @tinnedkarma Thanks for the explanation. Agreed on the root cause: the callback gets an mdio_lowerhalf_s *, but the original code cast it to stm32_mdio_bus_s while lower was a pointer to a separate object, so timeout read out of bounds. container_of is the right fix. I'd go with option 2 (embed mdio_lowerhalf_s in stm32_mdio_bus_s + container_of): It's the standard NuttX upcast pattern — extensible, compile-time type-checked, and supports multiple instances out of the box. And as you noted yourself, the heap concern doesn't really apply: stm32_mdio_bus_s is static/global, and the only heap object is the opaque mdio_bus_s handle from mdio_register(), regardless of embed vs. pointer. On option 1: if it means moving timeout into the common struct mdio_lowerhalf_s, I'd avoid that — it's the arch-agnostic contract, and per-arch extras (timeout, controller handle, port, …) diverge and shouldn't live there. If it just means making timeout a compile-time constant so STM32 needs no wrapper, that's fine for the single-instance case, but option 2 gives one pattern that maps cleanly onto any arch. So my vote is option 2 — same goal of keeping things simple, but the generality of the interface is worth it here. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
