Re: [PATCH] hw/net/imx_fec: return 0xffff when accessing non-existing PHY

[Jason Wang]

在 2021/5/3 上午12:14, Guenter Roeck 写道:
> On 5/2/21 9:09 AM, Bin Meng wrote:
> > On Mon, May 3, 2021 at 12:03 AM Guenter Roeck <linux@roeck-us.net> wrote:
> > > If a PHY does not exist, attempts to read from it should return 0xffff.
> > > Otherwise the Linux kernel will believe that a PHY is there and select
> > > the non-existing PHY. This in turn will result in network errors later
> > > on since the real PHY is not selected or configured.
> > >
> > > Since reading from or writing to a non-existing PHY is not an emulation
> > > error, replace guest error messages with traces.
> > >
> > > Fixes: 461c51ad4275 ("Add a phy-num property to the i.MX FEC emulator")
> > > Cc: Jean-Christophe Dubois <jcd@tribudubois.net>
> > > Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> > > ---
> > >   hw/net/imx_fec.c    | 8 +++-----
> > >   hw/net/trace-events | 2 ++
> > >   2 files changed, 5 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/hw/net/imx_fec.c b/hw/net/imx_fec.c
> > > index f03450c028..9c7035bc94 100644
> > > --- a/hw/net/imx_fec.c
> > > +++ b/hw/net/imx_fec.c
> > > @@ -283,9 +283,8 @@ static uint32_t imx_phy_read(IMXFECState *s, int reg)
> > >       uint32_t phy = reg / 32;
> > >
> > >       if (phy != s->phy_num) {
> > > -        qemu_log_mask(LOG_GUEST_ERROR, "[%s.phy]%s: Bad phy num %u\n",
> > > -                      TYPE_IMX_FEC, __func__, phy);
> > > -        return 0;
> > > +        trace_imx_phy_read_num(phy, s->phy_num);
> > > +        return 0xffff;
> > >       }
> > >
> > >       reg %= 32;
> > > @@ -345,8 +344,7 @@ static void imx_phy_write(IMXFECState *s, int reg, uint32_t 
> > > val)
> > >       uint32_t phy = reg / 32;
> > >
> > >       if (phy != s->phy_num) {
> > > -        qemu_log_mask(LOG_GUEST_ERROR, "[%s.phy]%s: Bad phy num %u\n",
> > > -                      TYPE_IMX_FEC, __func__, phy);
> > > +        trace_imx_phy_write_num(phy, s->phy_num);
> > >           return;
> > >       }
> > >
> > > diff --git a/hw/net/trace-events b/hw/net/trace-events
> > > index baf25ffa7e..ee77238d9e 100644
> > > --- a/hw/net/trace-events
> > > +++ b/hw/net/trace-events
> > > @@ -413,8 +413,10 @@ i82596_set_multicast(uint16_t count) "Added %d multicast 
> > > entries"
> > >   i82596_channel_attention(void *s) "%p: Received CHANNEL ATTENTION"
> > >
> > >   # imx_fec.c
> > > +imx_phy_read_num(int phy, int configured) "read request from unconfigured phy %d 
> > > (configured %d)"
> > nits: could we put this below imx_phy_read(), like you put
> > imx_phy_write_num after imx_phy_write?
> Sure, makes sense. I'll wait a bit for other feedback and then resend.


Ok, please send V2.

Thanks


> > >   imx_phy_read(uint32_t val, int phy, int reg) "0x%04"PRIx32" <= 
> > > phy[%d].reg[%d]"
> > >   imx_phy_write(uint32_t val, int phy, int reg) "0x%04"PRIx32" => 
> > > phy[%d].reg[%d]"
> > > +imx_phy_write_num(int phy, int configured) "write request to unconfigured phy %d 
> > > (configured %d)"
> > >   imx_phy_update_link(const char *s) "%s"
> > >   imx_phy_reset(void) ""
> > >   imx_fec_read_bd(uint64_t addr, int flags, int len, int data) "tx_bd 0x%"PRIx64" 
> > > flags 0x%04x len %d data 0x%08x"
> > > --
> > Otherwise,
> > Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
> > Tested-by: Bin Meng <bmeng.cn@gmail.com>
> Thanks!
>
> Guenter