* [PATCH 1/3] aqc111: fix endianness issue in aqc111_change_mtu @ 2019-05-09 9:08 Oliver Neukum 2019-05-09 9:08 ` [PATCH 2/3] aqc111: fix writing to the phy on BE Oliver Neukum ` (3 more replies) 0 siblings, 4 replies; 10+ messages in thread From: Oliver Neukum @ 2019-05-09 9:08 UTC (permalink / raw) To: dmitry.bezrukov, igor.russkikh, netdev; +Cc: Oliver Neukum If the MTU is large enough, the first write to the device is just repeated. On BE architectures, however, the first word of the command will be swapped a second time and garbage will be written. Avoid that. Signed-off-by: Oliver Neukum <oneukum@suse.com> --- drivers/net/usb/aqc111.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/net/usb/aqc111.c b/drivers/net/usb/aqc111.c index aff995be2a31..408df2d335e3 100644 --- a/drivers/net/usb/aqc111.c +++ b/drivers/net/usb/aqc111.c @@ -453,6 +453,8 @@ static int aqc111_change_mtu(struct net_device *net, int new_mtu) reg16 = 0x1420; else if (dev->net->mtu <= 16334) reg16 = 0x1A20; + else + return 0; aqc111_write16_cmd(dev, AQ_ACCESS_MAC, SFR_PAUSE_WATERLVL_LOW, 2, ®16); -- 2.16.4 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 2/3] aqc111: fix writing to the phy on BE 2019-05-09 9:08 [PATCH 1/3] aqc111: fix endianness issue in aqc111_change_mtu Oliver Neukum @ 2019-05-09 9:08 ` Oliver Neukum 2019-05-09 16:37 ` David Miller 2019-05-14 12:11 ` Igor Russkikh 2019-05-09 9:08 ` [PATCH 3/3] aqc111: fix double endianness swap " Oliver Neukum ` (2 subsequent siblings) 3 siblings, 2 replies; 10+ messages in thread From: Oliver Neukum @ 2019-05-09 9:08 UTC (permalink / raw) To: dmitry.bezrukov, igor.russkikh, netdev; +Cc: Oliver Neukum When writing to the phy on BE architectures an internal data structure was directly given, leading to it being byte swapped in the wrong way for the CPU in 50% of all cases. A temporary buffer must be used. Signed-off-by: Oliver Neukum <oneukum@suse.com> --- drivers/net/usb/aqc111.c | 23 +++++++++++++++++------ 1 file changed, 17 insertions(+), 6 deletions(-) diff --git a/drivers/net/usb/aqc111.c b/drivers/net/usb/aqc111.c index 408df2d335e3..599d560a8450 100644 --- a/drivers/net/usb/aqc111.c +++ b/drivers/net/usb/aqc111.c @@ -320,6 +320,7 @@ static int aqc111_get_link_ksettings(struct net_device *net, static void aqc111_set_phy_speed(struct usbnet *dev, u8 autoneg, u16 speed) { struct aqc111_data *aqc111_data = dev->driver_priv; + u32 phy_on_the_wire; aqc111_data->phy_cfg &= ~AQ_ADV_MASK; aqc111_data->phy_cfg |= AQ_PAUSE; @@ -361,7 +362,8 @@ static void aqc111_set_phy_speed(struct usbnet *dev, u8 autoneg, u16 speed) } } - aqc111_write32_cmd(dev, AQ_PHY_OPS, 0, 0, &aqc111_data->phy_cfg); + phy_on_the_wire = aqc111_data->phy_cfg; + aqc111_write32_cmd(dev, AQ_PHY_OPS, 0, 0, &phy_on_the_wire); } static int aqc111_set_link_ksettings(struct net_device *net, @@ -755,6 +757,7 @@ static void aqc111_unbind(struct usbnet *dev, struct usb_interface *intf) { struct aqc111_data *aqc111_data = dev->driver_priv; u16 reg16; + u32 phy_on_the_wire; /* Force bz */ reg16 = SFR_PHYPWR_RSTCTL_BZ; @@ -768,8 +771,9 @@ static void aqc111_unbind(struct usbnet *dev, struct usb_interface *intf) aqc111_data->phy_cfg &= ~AQ_ADV_MASK; aqc111_data->phy_cfg |= AQ_LOW_POWER; aqc111_data->phy_cfg &= ~AQ_PHY_POWER_EN; + phy_on_the_wire = aqc111_data->phy_cfg; aqc111_write32_cmd_nopm(dev, AQ_PHY_OPS, 0, 0, - &aqc111_data->phy_cfg); + &phy_on_the_wire); kfree(aqc111_data); } @@ -992,6 +996,7 @@ static int aqc111_reset(struct usbnet *dev) { struct aqc111_data *aqc111_data = dev->driver_priv; u8 reg8 = 0; + u32 phy_on_the_wire; dev->rx_urb_size = URB_SIZE; @@ -1004,8 +1009,9 @@ static int aqc111_reset(struct usbnet *dev) /* Power up ethernet PHY */ aqc111_data->phy_cfg = AQ_PHY_POWER_EN; + phy_on_the_wire = aqc111_data->phy_cfg; aqc111_write32_cmd(dev, AQ_PHY_OPS, 0, 0, - &aqc111_data->phy_cfg); + &phy_on_the_wire); /* Set the MAC address */ aqc111_write_cmd(dev, AQ_ACCESS_MAC, SFR_NODE_ID, ETH_ALEN, @@ -1036,6 +1042,7 @@ static int aqc111_stop(struct usbnet *dev) { struct aqc111_data *aqc111_data = dev->driver_priv; u16 reg16 = 0; + u32 phy_on_the_wire; aqc111_read16_cmd(dev, AQ_ACCESS_MAC, SFR_MEDIUM_STATUS_MODE, 2, ®16); @@ -1047,8 +1054,9 @@ static int aqc111_stop(struct usbnet *dev) /* Put PHY to low power*/ aqc111_data->phy_cfg |= AQ_LOW_POWER; + phy_on_the_wire = aqc111_data->phy_cfg; aqc111_write32_cmd(dev, AQ_PHY_OPS, 0, 0, - &aqc111_data->phy_cfg); + &phy_on_the_wire); netif_carrier_off(dev->net); @@ -1324,6 +1332,7 @@ static int aqc111_suspend(struct usb_interface *intf, pm_message_t message) u16 temp_rx_ctrl = 0x00; u16 reg16; u8 reg8; + u32 phy_on_the_wire; usbnet_suspend(intf, message); @@ -1395,12 +1404,14 @@ static int aqc111_suspend(struct usb_interface *intf, pm_message_t message) aqc111_write_cmd(dev, AQ_WOL_CFG, 0, 0, WOL_CFG_SIZE, &wol_cfg); + phy_on_the_wire = aqc111_data->phy_cfg; aqc111_write32_cmd(dev, AQ_PHY_OPS, 0, 0, - &aqc111_data->phy_cfg); + &phy_on_the_wire); } else { aqc111_data->phy_cfg |= AQ_LOW_POWER; + phy_on_the_wire = aqc111_data->phy_cfg; aqc111_write32_cmd(dev, AQ_PHY_OPS, 0, 0, - &aqc111_data->phy_cfg); + &phy_on_the_wire); /* Disable RX path */ aqc111_read16_cmd_nopm(dev, AQ_ACCESS_MAC, -- 2.16.4 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 2/3] aqc111: fix writing to the phy on BE 2019-05-09 9:08 ` [PATCH 2/3] aqc111: fix writing to the phy on BE Oliver Neukum @ 2019-05-09 16:37 ` David Miller 2019-05-14 12:11 ` Igor Russkikh 1 sibling, 0 replies; 10+ messages in thread From: David Miller @ 2019-05-09 16:37 UTC (permalink / raw) To: oneukum; +Cc: dmitry.bezrukov, igor.russkikh, netdev From: Oliver Neukum <oneukum@suse.com> Date: Thu, 9 May 2019 11:08:17 +0200 > When writing to the phy on BE architectures an internal data structure > was directly given, leading to it being byte swapped in the wrong > way for the CPU in 50% of all cases. A temporary buffer must be used. > > Signed-off-by: Oliver Neukum <oneukum@suse.com> Applied and queued up for -stable. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/3] aqc111: fix writing to the phy on BE 2019-05-09 9:08 ` [PATCH 2/3] aqc111: fix writing to the phy on BE Oliver Neukum 2019-05-09 16:37 ` David Miller @ 2019-05-14 12:11 ` Igor Russkikh 2019-05-14 13:14 ` Oliver Neukum 1 sibling, 1 reply; 10+ messages in thread From: Igor Russkikh @ 2019-05-14 12:11 UTC (permalink / raw) To: Oliver Neukum, Dmitry Bezrukov, netdev On 09.05.2019 12:08, Oliver Neukum wrote: > When writing to the phy on BE architectures an internal data structure > was directly given, leading to it being byte swapped in the wrong > way for the CPU in 50% of all cases. A temporary buffer must be used. > > Signed-off-by: Oliver Neukum <oneukum@suse.com> > --- > drivers/net/usb/aqc111.c | 23 +++++++++++++++++------ > 1 file changed, 17 insertions(+), 6 deletions(-) > > diff --git a/drivers/net/usb/aqc111.c b/drivers/net/usb/aqc111.c > index 408df2d335e3..599d560a8450 100644 > --- a/drivers/net/usb/aqc111.c > +++ b/drivers/net/usb/aqc111.c > @@ -320,6 +320,7 @@ static int aqc111_get_link_ksettings(struct net_device *net, > static void aqc111_set_phy_speed(struct usbnet *dev, u8 autoneg, u16 speed) > { > struct aqc111_data *aqc111_data = dev->driver_priv; > + u32 phy_on_the_wire; > > aqc111_data->phy_cfg &= ~AQ_ADV_MASK; > aqc111_data->phy_cfg |= AQ_PAUSE; > @@ -361,7 +362,8 @@ static void aqc111_set_phy_speed(struct usbnet *dev, u8 autoneg, u16 speed) > } > } > > - aqc111_write32_cmd(dev, AQ_PHY_OPS, 0, 0, &aqc111_data->phy_cfg); > + phy_on_the_wire = aqc111_data->phy_cfg; > + aqc111_write32_cmd(dev, AQ_PHY_OPS, 0, 0, &phy_on_the_wire); Hi Oliver, I see all write32_cmd and write16_cmd are using a temporary variable to do an internal cpu_to_le32. Why this extra temporary storage is needed? The question is actually for both 2nd and third patch. In all the cases BE machine will store temporary bswap conversion in tmp variable and will not actually touch actual field. Regards, Igor PS Sorry for sending this lately, had a long holiday weekend. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/3] aqc111: fix writing to the phy on BE 2019-05-14 12:11 ` Igor Russkikh @ 2019-05-14 13:14 ` Oliver Neukum 2019-05-15 14:16 ` Igor Russkikh 0 siblings, 1 reply; 10+ messages in thread From: Oliver Neukum @ 2019-05-14 13:14 UTC (permalink / raw) To: Igor Russkikh, Dmitry Bezrukov, netdev On Di, 2019-05-14 at 12:11 +0000, Igor Russkikh wrote: > On 09.05.2019 12:08, Oliver Neukum wrote: > > When writing to the phy on BE architectures an internal data structure > > was directly given, leading to it being byte swapped in the wrong > > way for the CPU in 50% of all cases. A temporary buffer must be used. > > > > Signed-off-by: Oliver Neukum <oneukum@suse.com> > > --- > > drivers/net/usb/aqc111.c | 23 +++++++++++++++++------ > > 1 file changed, 17 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/net/usb/aqc111.c b/drivers/net/usb/aqc111.c > > index 408df2d335e3..599d560a8450 100644 > > --- a/drivers/net/usb/aqc111.c > > +++ b/drivers/net/usb/aqc111.c > > @@ -320,6 +320,7 @@ static int aqc111_get_link_ksettings(struct net_device *net, > > static void aqc111_set_phy_speed(struct usbnet *dev, u8 autoneg, u16 speed) > > { > > struct aqc111_data *aqc111_data = dev->driver_priv; > > + u32 phy_on_the_wire; > > > > aqc111_data->phy_cfg &= ~AQ_ADV_MASK; > > aqc111_data->phy_cfg |= AQ_PAUSE; > > @@ -361,7 +362,8 @@ static void aqc111_set_phy_speed(struct usbnet *dev, u8 autoneg, u16 speed) > > } > > } > > > > - aqc111_write32_cmd(dev, AQ_PHY_OPS, 0, 0, &aqc111_data->phy_cfg); > > + phy_on_the_wire = aqc111_data->phy_cfg; > > + aqc111_write32_cmd(dev, AQ_PHY_OPS, 0, 0, &phy_on_the_wire); > > Hi Oliver, > > I see all write32_cmd and write16_cmd are using a temporary variable to do an > internal cpu_to_le32. Why this extra temporary storage is needed? > > The question is actually for both 2nd and third patch. > In all the cases BE machine will store temporary bswap conversion in tmp > variable and will not actually touch actual field. Hi, I am most terribly sorry. I overlooked the copy. Shall I revert or will you. Sorry Oliver ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/3] aqc111: fix writing to the phy on BE 2019-05-14 13:14 ` Oliver Neukum @ 2019-05-15 14:16 ` Igor Russkikh 0 siblings, 0 replies; 10+ messages in thread From: Igor Russkikh @ 2019-05-15 14:16 UTC (permalink / raw) To: Oliver Neukum, Dmitry Bezrukov, netdev >>> - aqc111_write32_cmd(dev, AQ_PHY_OPS, 0, 0, &aqc111_data->phy_cfg); >>> + phy_on_the_wire = aqc111_data->phy_cfg; >>> + aqc111_write32_cmd(dev, AQ_PHY_OPS, 0, 0, &phy_on_the_wire); >> >> Hi Oliver, >> >> I see all write32_cmd and write16_cmd are using a temporary variable to do an >> internal cpu_to_le32. Why this extra temporary storage is needed? >> >> The question is actually for both 2nd and third patch. >> In all the cases BE machine will store temporary bswap conversion in tmp >> variable and will not actually touch actual field. > > Hi, > > I am most terribly sorry. I overlooked the copy. Shall I revert or will > you. > > Sorry > Oliver Hi Oliver, I'll submit the reverts. Regards, Igor ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 3/3] aqc111: fix double endianness swap on BE 2019-05-09 9:08 [PATCH 1/3] aqc111: fix endianness issue in aqc111_change_mtu Oliver Neukum 2019-05-09 9:08 ` [PATCH 2/3] aqc111: fix writing to the phy on BE Oliver Neukum @ 2019-05-09 9:08 ` Oliver Neukum 2019-05-09 16:37 ` David Miller 2019-05-09 16:37 ` [PATCH 1/3] aqc111: fix endianness issue in aqc111_change_mtu David Miller 2019-05-14 12:37 ` Igor Russkikh 3 siblings, 1 reply; 10+ messages in thread From: Oliver Neukum @ 2019-05-09 9:08 UTC (permalink / raw) To: dmitry.bezrukov, igor.russkikh, netdev; +Cc: Oliver Neukum If you are using a function that does a swap in place, you cannot just reuse the buffer on the assumption that it has not been changed. Signed-off-by: Oliver Neukum <oneukum@suse.com> --- drivers/net/usb/aqc111.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/drivers/net/usb/aqc111.c b/drivers/net/usb/aqc111.c index 599d560a8450..b86c5ce9a92a 100644 --- a/drivers/net/usb/aqc111.c +++ b/drivers/net/usb/aqc111.c @@ -1428,7 +1428,7 @@ static int aqc111_resume(struct usb_interface *intf) { struct usbnet *dev = usb_get_intfdata(intf); struct aqc111_data *aqc111_data = dev->driver_priv; - u16 reg16; + u16 reg16, oldreg16; u8 reg8; netif_carrier_off(dev->net); @@ -1444,9 +1444,11 @@ static int aqc111_resume(struct usb_interface *intf) /* Configure RX control register => start operation */ reg16 = aqc111_data->rxctl; reg16 &= ~SFR_RX_CTL_START; + /* needs to be saved in case endianness is swapped */ + oldreg16 = reg16; aqc111_write16_cmd_nopm(dev, AQ_ACCESS_MAC, SFR_RX_CTL, 2, ®16); - reg16 |= SFR_RX_CTL_START; + reg16 = oldreg16 | SFR_RX_CTL_START; aqc111_write16_cmd_nopm(dev, AQ_ACCESS_MAC, SFR_RX_CTL, 2, ®16); aqc111_set_phy_speed(dev, aqc111_data->autoneg, -- 2.16.4 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 3/3] aqc111: fix double endianness swap on BE 2019-05-09 9:08 ` [PATCH 3/3] aqc111: fix double endianness swap " Oliver Neukum @ 2019-05-09 16:37 ` David Miller 0 siblings, 0 replies; 10+ messages in thread From: David Miller @ 2019-05-09 16:37 UTC (permalink / raw) To: oneukum; +Cc: dmitry.bezrukov, igor.russkikh, netdev From: Oliver Neukum <oneukum@suse.com> Date: Thu, 9 May 2019 11:08:18 +0200 > If you are using a function that does a swap in place, > you cannot just reuse the buffer on the assumption that it has > not been changed. > > Signed-off-by: Oliver Neukum <oneukum@suse.com> Applied and queued up for -stable. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/3] aqc111: fix endianness issue in aqc111_change_mtu 2019-05-09 9:08 [PATCH 1/3] aqc111: fix endianness issue in aqc111_change_mtu Oliver Neukum 2019-05-09 9:08 ` [PATCH 2/3] aqc111: fix writing to the phy on BE Oliver Neukum 2019-05-09 9:08 ` [PATCH 3/3] aqc111: fix double endianness swap " Oliver Neukum @ 2019-05-09 16:37 ` David Miller 2019-05-14 12:37 ` Igor Russkikh 3 siblings, 0 replies; 10+ messages in thread From: David Miller @ 2019-05-09 16:37 UTC (permalink / raw) To: oneukum; +Cc: dmitry.bezrukov, igor.russkikh, netdev From: Oliver Neukum <oneukum@suse.com> Date: Thu, 9 May 2019 11:08:16 +0200 > If the MTU is large enough, the first write to the device > is just repeated. On BE architectures, however, the first > word of the command will be swapped a second time and garbage > will be written. Avoid that. > > Signed-off-by: Oliver Neukum <oneukum@suse.com> Applied and queued up for -stable. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/3] aqc111: fix endianness issue in aqc111_change_mtu 2019-05-09 9:08 [PATCH 1/3] aqc111: fix endianness issue in aqc111_change_mtu Oliver Neukum ` (2 preceding siblings ...) 2019-05-09 16:37 ` [PATCH 1/3] aqc111: fix endianness issue in aqc111_change_mtu David Miller @ 2019-05-14 12:37 ` Igor Russkikh 3 siblings, 0 replies; 10+ messages in thread From: Igor Russkikh @ 2019-05-14 12:37 UTC (permalink / raw) To: Oliver Neukum, Dmitry Bezrukov, netdev On 09.05.2019 12:08, Oliver Neukum wrote: > If the MTU is large enough, the first write to the device > is just repeated. On BE architectures, however, the first > word of the command will be swapped a second time and garbage > will be written. Avoid that. > > Signed-off-by: Oliver Neukum <oneukum@suse.com> > --- > drivers/net/usb/aqc111.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/net/usb/aqc111.c b/drivers/net/usb/aqc111.c > index aff995be2a31..408df2d335e3 100644 > --- a/drivers/net/usb/aqc111.c > +++ b/drivers/net/usb/aqc111.c > @@ -453,6 +453,8 @@ static int aqc111_change_mtu(struct net_device *net, int new_mtu) > reg16 = 0x1420; > else if (dev->net->mtu <= 16334) > reg16 = 0x1A20; > + else > + return 0; > > aqc111_write16_cmd(dev, AQ_ACCESS_MAC, SFR_PAUSE_WATERLVL_LOW, > 2, ®16); > Since we specify max_mtu at `bind` time, this `else` will never happen. Only for readability that could be rewritten as - else if (dev->net->mtu <= 16334) + else reg16 = 0x1A20; Regards, Igor ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2019-05-15 14:16 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-05-09 9:08 [PATCH 1/3] aqc111: fix endianness issue in aqc111_change_mtu Oliver Neukum 2019-05-09 9:08 ` [PATCH 2/3] aqc111: fix writing to the phy on BE Oliver Neukum 2019-05-09 16:37 ` David Miller 2019-05-14 12:11 ` Igor Russkikh 2019-05-14 13:14 ` Oliver Neukum 2019-05-15 14:16 ` Igor Russkikh 2019-05-09 9:08 ` [PATCH 3/3] aqc111: fix double endianness swap " Oliver Neukum 2019-05-09 16:37 ` David Miller 2019-05-09 16:37 ` [PATCH 1/3] aqc111: fix endianness issue in aqc111_change_mtu David Miller 2019-05-14 12:37 ` Igor Russkikh
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).