| Message ID | 20210502160326.1196252-1-linux@roeck-us.net |
|---|---|
| State | New |
| Headers | show |
| Series | hw/net/imx_fec: return 0xffff when accessing non-existing PHY | expand |
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? > 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>
On 5/2/21 6:03 PM, Guenter Roeck 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(-) Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
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. >> 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
在 2021/5/3 上午12:03, Guenter Roeck 写道: > 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> Applied. Thanks > --- > 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)" > 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"
在 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 >
On Thu, May 06, 2021 at 10:24:52AM +0800, Jason Wang wrote: [ ... ] > Ok, please send V2. > You should have it by now. Please let me know if it got lost. Thanks, Guenter
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)" 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"
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(-)