RE: [PATCH net-next 0/8] net: dsa: microchip: DSA driver support for LAN937x switch

[Woojung.Huh]

Hi Florian,

Wish you a happy and safe new year.

Thanks for your time to review new patches.
 > It is great to see a new switch from Microchip being submitted for
> review. One thing that has bothered me as a DSA maintainer before though
> is that we have seen Microchip contribute new DSA drivers which are
> always welcome, however the maintenance and bug fixing of these drivers
> was spotty, thus leading to external contributors to take on the tasks
> of fixing bugs. Do you have a stronger commitment now to stay involved
> with reviewing/fixing bugs affecting Microchip DSA drivers and to a
> larger extent the framework itself?
Admit that Microchip's activities on community, especially on DSA drivers, 
was not active for a while.  We are going to do our best to get involved more
on community including reviewing and frameworks. You might already start
seeing review and comments on community from Microchip recently.
 
> Could you also feed back to your hardware organization to settle on a
> tag format that is not a snowflake? Almost *every* switch you have has a
> different tagging format, this is absurd. All other vendors in tree have
> been able to settle on at most 2 or 3 different tagging formats over
> their switching product life span (for some vendors this dates back 20
> years ago).
Understand this point too. Actually, those products are developed over time.
Sometime it is not avoidable to add new stuff.
But, Yes, it would be better to design ahead with reserved fields.

Thanks again on your reviews and comments, will gear up on DSA works.

Best Regards,
Woojung

RE: [PATCH net-next v2 4/4] net: usb: smsc: fix warning reported by kbuild test robot

[Woojung.Huh]

> This patch fixes following warning reported by kbuild test robot:
> 
>  In function ‘memcpy’,
>  inlined from ‘smsc75xx_init_mac_address’ at 
> drivers/net/usb/smsc75xx.c:778:3,
>  inlined from ‘smsc75xx_bind’ at drivers/net/usb/smsc75xx.c:1501:2:
>  ./include/linux/string.h:355:9: warning: argument 2 null where non-null 
> expected [-Wnonnull]
>return __builtin_memcpy(p, q, size);
>   ^~~~
>  drivers/net/usb/smsc75xx.c: In function ‘smsc75xx_bind’:
>  ./include/linux/string.h:355:9: note: in a call to built-in function 
> ‘__builtin_memcpy’
> 
> I've replaced the offending memcpy with ether_addr_copy, because I'm
> 100% sure, that of_get_mac_address can't return NULL as it returns valid
> pointer or ERR_PTR encoded value, nothing else.
> 
> I'm hesitant to just change IS_ERR into IS_ERR_OR_NULL check, as this
> would make the warning disappear also, but it would be confusing to
> check for impossible return value just to make a compiler happy.
> 
> Fixes: adfb3cb2c52e ("net: usb: support of_get_mac_address new ERR_PTR error")
> Reported-by: kbuild test robot 
> Signed-off-by: Petr Štetiar 
> ---

Reviewed-by: Woojung Huh 

RE: [PATCH] MAINTAINERS: normalize Woojung Huh's email address

[Woojung.Huh]

> MAINTAINERS contains a lower-case and upper-case variant of
> Woojung Huh' s email address.
> 
> Only keep the lower-case variant in MAINTAINERS.
> 
> Signed-off-by: Lukas Bulwahn 

Acked-by: Woojung Huh 

RE: [PATCH] MAINTAINERS: Add a maintainer for MSCC MIPS SoCs

[Woojung.Huh]

> -Original Message-
> From: Alexandre Belloni 
> Sent: Tuesday, December 18, 2018 9:27 AM
> To: James Hogan ; Paul Burton 
> Cc: Ralf Baechle ; linux-kernel@vger.kernel.org; 
> linux-m...@linux-mips.org;
> UNGLinuxDriver ; Alexandre Belloni
> 
> Subject: [PATCH] MAINTAINERS: Add a maintainer for MSCC MIPS SoCs
> 
> Microsemi has been bought by Microchip and Microchip is supporting those
> SoCs.
> 
> Signed-off-by: Alexandre Belloni 
> ---
>  MAINTAINERS | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index f4855974f325..50223cba6ddb 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -9801,8 +9801,9 @@ F:  drivers/dma/at_xdmac.c
> 
>  MICROSEMI MIPS SOCS
>  M:   Alexandre Belloni 
> +M:   Microchip Linux Driver Support 
>  L:   linux-m...@linux-mips.org
> -S:   Maintained
> +S:   Supported
>  F:   arch/mips/generic/board-ocelot.c
>  F:   arch/mips/configs/generic/board-ocelot.config
>  F:   arch/mips/boot/dts/mscc/
> --
> 2.20.0

Acked-by: Woojung Huh 

RE: [PATCH net-next] MAINTAINERS: Add a maintainer for Microsemi switches

[Woojung.Huh]

> -Original Message-
> From: Alexandre Belloni 
> Sent: Tuesday, December 18, 2018 9:26 AM
> To: David S . Miller 
> Cc: UNGLinuxDriver ; net...@vger.kernel.org; 
> linux-
> ker...@vger.kernel.org; Alexandre Belloni 
> Subject: [PATCH net-next] MAINTAINERS: Add a maintainer for Microsemi switches
> 
> Microsemi has been bought by Microchip and Microchip is supporting those
> switches.
> 
> Signed-off-by: Alexandre Belloni 
> ---
>  MAINTAINERS | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 50223cba6ddb..48abe982aed6 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -9823,6 +9823,7 @@ F:  Documentation/scsi/smartpqi.txt
> 
>  MICROSEMI ETHERNET SWITCH DRIVER
>  M:   Alexandre Belloni 
> +M:   Microchip Linux Driver Support 
>  L:   net...@vger.kernel.org
>  S:   Supported
>  F:   drivers/net/ethernet/mscc/
> --
> 2.20.0

Acked-by: Woojung Huh 

RE: [RFC net-next 4/5] net: phy: Add support for IEEE standard test modes

[Woojung.Huh]

Hi Florian,

> Well, the way the code is structure is that if you call that function
> with a test mode value that is not part of the standard set, it returns
> -EOPNOTSUPP, so if your particular PHY driver wants to "overlay"
> standard and non-standard modes, it can by using that hint.
> 
> This should work even if we have more standard test modes in the future
> because the test modes are dynamically fetched by user-space using the
> ETH_GSTRINGS ioctl().
> 
> Does that cover what you had in mind?
Basically, agree on your explanation.

My idea was making genphy_set_test() more expandable for other test modes
because it would be a good place to add more standard test modes later.

No problem to keep current codes.

Thanks.
Woojung

RE: [RFC net-next 4/5] net: phy: Add support for IEEE standard test modes

[Woojung.Huh]

Hi Florian,

> Well, the way the code is structure is that if you call that function
> with a test mode value that is not part of the standard set, it returns
> -EOPNOTSUPP, so if your particular PHY driver wants to "overlay"
> standard and non-standard modes, it can by using that hint.
> 
> This should work even if we have more standard test modes in the future
> because the test modes are dynamically fetched by user-space using the
> ETH_GSTRINGS ioctl().
> 
> Does that cover what you had in mind?
Basically, agree on your explanation.

My idea was making genphy_set_test() more expandable for other test modes
because it would be a good place to add more standard test modes later.

No problem to keep current codes.

Thanks.
Woojung

RE: [RFC net-next 4/5] net: phy: Add support for IEEE standard test modes

[Woojung.Huh]

Hi Florian,

> Not sure I completely understand your suggestion, do you mean that I
> should break down the body of that function above such that there are
> per-speed lower level functions? Something like the pseudo-code below:
> 
> genphy_set_test() {
>   switch (mode) {
>   case PHY_STD_TEST_MODE_100BASET2_1:
>   ..
>   case PHY_STD_TEST_MODE_100BASET2_3:
>   return genphy_set_100baset2();
> 
>   case PHY_STD_TEST_MODE_1000BASET_1:
>   ..
>   case PHY_STD_TEST_MODE_1000BASET_4:
>   return genphy_set_1000baset();
> 
>   case PHY_STD_TEST_MODE_8021BWQCQ_1:
>   return genphy_set_100baset1();
> 
> }
Yes, I should write pseudo code. Sorry about confusion.
User can override this function or expand to other modes.

Thanks.
Woojung

RE: [RFC net-next 4/5] net: phy: Add support for IEEE standard test modes

[Woojung.Huh]

Hi Florian,

> Not sure I completely understand your suggestion, do you mean that I
> should break down the body of that function above such that there are
> per-speed lower level functions? Something like the pseudo-code below:
> 
> genphy_set_test() {
>   switch (mode) {
>   case PHY_STD_TEST_MODE_100BASET2_1:
>   ..
>   case PHY_STD_TEST_MODE_100BASET2_3:
>   return genphy_set_100baset2();
> 
>   case PHY_STD_TEST_MODE_1000BASET_1:
>   ..
>   case PHY_STD_TEST_MODE_1000BASET_4:
>   return genphy_set_1000baset();
> 
>   case PHY_STD_TEST_MODE_8021BWQCQ_1:
>   return genphy_set_100baset1();
> 
> }
Yes, I should write pseudo code. Sorry about confusion.
User can override this function or expand to other modes.

Thanks.
Woojung

RE: [RFC net-next 4/5] net: phy: Add support for IEEE standard test modes

[Woojung.Huh]

Hi Florian,

> diff --git a/drivers/net/phy/phy-tests.c b/drivers/net/phy/phy-tests.c
...
> +/* genphy_set_test - Make a PHY enter one of the standard IEEE defined
> + * test modes
> + * @phydev: the PHY device instance
> + * @test: the desired test mode
> + * @data: test specific data (none)
> + *
> + * This function makes the designated @phydev enter the desired standard
> + * 100BaseT2 or 1000BaseT test mode as defined in IEEE 802.3-2012 section TWO
> + * and THREE under 32.6.1.2.1 and 40.6.1.1.2 respectively
> + */
> +int genphy_set_test(struct phy_device *phydev,
> + struct ethtool_phy_test *test, const u8 *data)
> +{
> + u16 shift, base, bmcr = 0;
> + int ret;
> +
> + /* Exit test mode */
> + if (test->mode == PHY_STD_TEST_MODE_NORMAL) {
> + ret = phy_read(phydev, MII_CTRL1000);
> + if (ret < 0)
> + return ret;
> +
> + ret &= ~GENMASK(15, 13);
> +
> + return phy_write(phydev, MII_CTRL1000, ret);
> + }
> +
> + switch (test->mode) {
> + case PHY_STD_TEST_MODE_100BASET2_1:
> + case PHY_STD_TEST_MODE_100BASET2_2:
> + case PHY_STD_TEST_MODE_100BASET2_3:
> + if (!(phydev->supported & PHY_100BT_FEATURES))
> + return -EOPNOTSUPP;
> +
> + shift = 14;
> + base = test->mode - PHY_STD_TEST_MODE_NORMAL;
> + bmcr = BMCR_SPEED100;
> + break;
> +
> + case PHY_STD_TEST_MODE_1000BASET_1:
> + case PHY_STD_TEST_MODE_1000BASET_2:
> + case PHY_STD_TEST_MODE_1000BASET_3:
> + case PHY_STD_TEST_MODE_1000BASET_4:
> + if (!(phydev->supported & PHY_1000BT_FEATURES))
> + return -EOPNOTSUPP;
> +
> + shift = 13;
> + base = test->mode - PHY_STD_TEST_MODE_100BASET2_MAX;
> + bmcr = BMCR_SPEED1000;
> + break;
> +
> + default:
> + /* Let an upper driver deal with additional modes it may
> +  * support
> +  */
> + return -EOPNOTSUPP;
> + }
> +
> + /* Force speed and duplex */
> + ret = phy_write(phydev, MII_BMCR, bmcr | BMCR_FULLDPLX);
> + if (ret < 0)
> + return ret;
> +
> + /* Set the desired test mode bit */
> + return phy_write(phydev, MII_CTRL1000, (test->mode + base) << shift);
> +}
For now, these are for 100B-T2 & 1000B-TX.
But, other speeds such as 802.3bw/bq/cq have very similar format,
how about make  phy_write() to BMCR & CTRL1000 as another function call per 
speed?

Thanks.
Woojung

RE: [RFC net-next 4/5] net: phy: Add support for IEEE standard test modes

[Woojung.Huh]

Hi Florian,

> diff --git a/drivers/net/phy/phy-tests.c b/drivers/net/phy/phy-tests.c
...
> +/* genphy_set_test - Make a PHY enter one of the standard IEEE defined
> + * test modes
> + * @phydev: the PHY device instance
> + * @test: the desired test mode
> + * @data: test specific data (none)
> + *
> + * This function makes the designated @phydev enter the desired standard
> + * 100BaseT2 or 1000BaseT test mode as defined in IEEE 802.3-2012 section TWO
> + * and THREE under 32.6.1.2.1 and 40.6.1.1.2 respectively
> + */
> +int genphy_set_test(struct phy_device *phydev,
> + struct ethtool_phy_test *test, const u8 *data)
> +{
> + u16 shift, base, bmcr = 0;
> + int ret;
> +
> + /* Exit test mode */
> + if (test->mode == PHY_STD_TEST_MODE_NORMAL) {
> + ret = phy_read(phydev, MII_CTRL1000);
> + if (ret < 0)
> + return ret;
> +
> + ret &= ~GENMASK(15, 13);
> +
> + return phy_write(phydev, MII_CTRL1000, ret);
> + }
> +
> + switch (test->mode) {
> + case PHY_STD_TEST_MODE_100BASET2_1:
> + case PHY_STD_TEST_MODE_100BASET2_2:
> + case PHY_STD_TEST_MODE_100BASET2_3:
> + if (!(phydev->supported & PHY_100BT_FEATURES))
> + return -EOPNOTSUPP;
> +
> + shift = 14;
> + base = test->mode - PHY_STD_TEST_MODE_NORMAL;
> + bmcr = BMCR_SPEED100;
> + break;
> +
> + case PHY_STD_TEST_MODE_1000BASET_1:
> + case PHY_STD_TEST_MODE_1000BASET_2:
> + case PHY_STD_TEST_MODE_1000BASET_3:
> + case PHY_STD_TEST_MODE_1000BASET_4:
> + if (!(phydev->supported & PHY_1000BT_FEATURES))
> + return -EOPNOTSUPP;
> +
> + shift = 13;
> + base = test->mode - PHY_STD_TEST_MODE_100BASET2_MAX;
> + bmcr = BMCR_SPEED1000;
> + break;
> +
> + default:
> + /* Let an upper driver deal with additional modes it may
> +  * support
> +  */
> + return -EOPNOTSUPP;
> + }
> +
> + /* Force speed and duplex */
> + ret = phy_write(phydev, MII_BMCR, bmcr | BMCR_FULLDPLX);
> + if (ret < 0)
> + return ret;
> +
> + /* Set the desired test mode bit */
> + return phy_write(phydev, MII_CTRL1000, (test->mode + base) << shift);
> +}
For now, these are for 100B-T2 & 1000B-TX.
But, other speeds such as 802.3bw/bq/cq have very similar format,
how about make  phy_write() to BMCR & CTRL1000 as another function call per 
speed?

Thanks.
Woojung

RE: [PATCH 3/4] lan78xx: Read LED modes from Device Tree

[Woojung.Huh]

> > @@ -2097,6 +2098,25 @@ static int lan78xx_phy_init(struct lan78xx_net *dev)
> > (void)lan78xx_set_eee(dev->net, );
> > }
> >
> > +   if (!of_property_read_u32_array(dev->udev->dev.of_node,
> > +   "microchip,led-modes",
> > +   led_modes, ARRAY_SIZE(led_modes))) {
> > +   u32 reg;
> > +   int i;
> > +
> > +   reg = phy_read(phydev, 0x1d);
> > +   for (i = 0; i < ARRAY_SIZE(led_modes); i++) {
> > +   reg &= ~(0xf << (i * 4));
> > +   reg |= (led_modes[i] & 0xf) << (i * 4);
> > +   }
> > +   (void)phy_write(phydev, 0x1d, reg);
> 
> Poking PHY registers directly from the MAC driver is not always a good
> idea. This MAC driver does that in a few places :-(
Agree but, some are for workaround unfortunately.

> What do we know about the PHY? It is built into the device or is it
> external? If it is external, how do you know the LED register is at
> 0x1d?
This register is not defined in include/linux/microchipphy.h. :(
Also agree that there parts should be applied to internal PHY only.

RE: [PATCH 3/4] lan78xx: Read LED modes from Device Tree

[Woojung.Huh]

> > @@ -2097,6 +2098,25 @@ static int lan78xx_phy_init(struct lan78xx_net *dev)
> > (void)lan78xx_set_eee(dev->net, );
> > }
> >
> > +   if (!of_property_read_u32_array(dev->udev->dev.of_node,
> > +   "microchip,led-modes",
> > +   led_modes, ARRAY_SIZE(led_modes))) {
> > +   u32 reg;
> > +   int i;
> > +
> > +   reg = phy_read(phydev, 0x1d);
> > +   for (i = 0; i < ARRAY_SIZE(led_modes); i++) {
> > +   reg &= ~(0xf << (i * 4));
> > +   reg |= (led_modes[i] & 0xf) << (i * 4);
> > +   }
> > +   (void)phy_write(phydev, 0x1d, reg);
> 
> Poking PHY registers directly from the MAC driver is not always a good
> idea. This MAC driver does that in a few places :-(
Agree but, some are for workaround unfortunately.

> What do we know about the PHY? It is built into the device or is it
> external? If it is external, how do you know the LED register is at
> 0x1d?
This register is not defined in include/linux/microchipphy.h. :(
Also agree that there parts should be applied to internal PHY only.

RE: [PATCH] lan78xx: Connect phy early

[Woojung.Huh]

Hi Alexander,

Thanks for patch. We will look into it if there is any corner case
Such as plug in/out while operations.

Woojung

> -Original Message-
> From: Alexander Graf [mailto:ag...@suse.de]
> Sent: Wednesday, March 14, 2018 10:55 AM
> To: Woojung Huh - C21699 
> Cc: UNGLinuxDriver ; net...@vger.kernel.org; 
> linux-
> u...@vger.kernel.org; linux-kernel@vger.kernel.org; Thomas Bogendoerfer
> ; Phil Elwell 
> Subject: [PATCH] lan78xx: Connect phy early
> 
> When using wicked with a lan78xx device attached to the system, we
> end up with ethtool commands issued on the device before an ifup
> got issued. That lead to the following crash:
> 
> Unable to handle kernel NULL pointer dereference at virtual address 
> 039c
> pgd = 800035b3
> [039c] *pgd=
> Internal error: Oops: 9604 [#1] SMP
> Modules linked in: [...]
> Supported: Yes
> CPU: 3 PID: 638 Comm: wickedd Tainted: GE  
> 4.12.14-0-default #1
> Hardware name: raspberrypi rpi/rpi, BIOS 2018.03-rc2 02/21/2018
> task: 800035e74180 task.stack: 800036718000
> PC is at phy_ethtool_ksettings_get+0x20/0x98
> LR is at lan78xx_get_link_ksettings+0x44/0x60 [lan78xx]
> pc : [] lr : [] pstate: 2005
> sp : 80003671bb20
> x29: 80003671bb20 x28: 800035e74180
> x27: 08912000 x26: 001d
> x25: 0124 x24: 08f74d00
> x23: 004000114809 x22: 
> x21: 80003671bbd0 x20: 
> x19: 80003671bbd0 x18: 040d
> x17: 0001 x16: 
> x15:  x14: 
> x13:  x12: 0020
> x11: 0101010101010101 x10: fefefefefefefeff
> x9 : 7f7f7f7f7f7f7f7f x8 : fefefeff31677364
> x7 : 80808080 x6 : 80003671bc9c
> x5 : 80003671b9f8 x4 : 80002c296190
> x3 :  x2 : 
> x1 : 80003671bbd0 x0 : 80003671bc00
> Process wickedd (pid: 638, stack limit = 0x800036718000)
> Call trace:
> Exception stack(0x80003671b9e0 to 0x80003671bb20)
> b9e0: 80003671bc00 80003671bbd0  
> ba00: 80002c296190 80003671b9f8 80003671bc9c 80808080
> ba20: fefefeff31677364 7f7f7f7f7f7f7f7f fefefefefefefeff 0101010101010101
> ba40: 0020   
> ba60:  0001 040d 80003671bbd0
> ba80:  80003671bbd0  004000114809
> baa0: 08f74d00 0124 001d 08912000
> bac0: 800035e74180 80003671bb20 00dcca84 80003671bb20
> bae0: 086f7f30 2005 80002c296000 800035223900
> bb00:   80003671bb20 086f7f30
> [] phy_ethtool_ksettings_get+0x20/0x98
> [] lan78xx_get_link_ksettings+0x44/0x60 [lan78xx]
> [] ethtool_get_settings+0x68/0x210
> [] dev_ethtool+0x214/0x2180
> [] dev_ioctl+0x400/0x630
> [] sock_do_ioctl+0x70/0x88
> [] sock_ioctl+0x208/0x368
> [] do_vfs_ioctl+0xb0/0x848
> [] SyS_ioctl+0x8c/0xa8
> Exception stack(0x80003671bec0 to 0x80003671c000)
> bec0: 0009 8946 f4e841d0 aa0032687465
> bee0: fa2319d4 f4e841d4 32687465 32687465
> bf00: 001d 7f7fff7f7f7f7f7f 72606b622e71ff4c 7f7f7f7f7f7f7f7f
> bf20: 0101010101010101 0020  7f510c68
> bf40: 7f6a9d18 7f44ce30 040d 7f6f98f0
> bf60: f4e842c0 0001 fa2c2e00 7f6ab000
> bf80: f4e842c0 7f62a000 fa2b9f20 fa2c2e00
> bfa0: f4e84818 f4e841a0 7f5ad0cc f4e841a0
> bfc0: 7f44ce3c 8000 0009 001d
> bfe0:    
> 
> The culprit is quite simple: The driver tries to access the phy left and 
> right,
> but only actually has a working reference to it when the device is up.
> 
> The fix thus is quite simple too: Get a reference to the phy on probe already
> and keep it even when the device is going down.
> 
> With this patch applied, I can successfully run wicked on my system and bring
> the interface up and down as many times as I want, without getting NULL 
> pointer
> dereferences in between.
> 
> Signed-off-by: Alexander Graf 
> ---
>  drivers/net/usb/lan78xx.c | 21 ++---
>  1 file changed, 10 insertions(+), 11 deletions(-)
> 
> diff --git 

RE: [PATCH] lan78xx: Connect phy early

[Woojung.Huh]

Hi Alexander,

Thanks for patch. We will look into it if there is any corner case
Such as plug in/out while operations.

Woojung

> -Original Message-
> From: Alexander Graf [mailto:ag...@suse.de]
> Sent: Wednesday, March 14, 2018 10:55 AM
> To: Woojung Huh - C21699 
> Cc: UNGLinuxDriver ; net...@vger.kernel.org; 
> linux-
> u...@vger.kernel.org; linux-kernel@vger.kernel.org; Thomas Bogendoerfer
> ; Phil Elwell 
> Subject: [PATCH] lan78xx: Connect phy early
> 
> When using wicked with a lan78xx device attached to the system, we
> end up with ethtool commands issued on the device before an ifup
> got issued. That lead to the following crash:
> 
> Unable to handle kernel NULL pointer dereference at virtual address 
> 039c
> pgd = 800035b3
> [039c] *pgd=
> Internal error: Oops: 9604 [#1] SMP
> Modules linked in: [...]
> Supported: Yes
> CPU: 3 PID: 638 Comm: wickedd Tainted: GE  
> 4.12.14-0-default #1
> Hardware name: raspberrypi rpi/rpi, BIOS 2018.03-rc2 02/21/2018
> task: 800035e74180 task.stack: 800036718000
> PC is at phy_ethtool_ksettings_get+0x20/0x98
> LR is at lan78xx_get_link_ksettings+0x44/0x60 [lan78xx]
> pc : [] lr : [] pstate: 2005
> sp : 80003671bb20
> x29: 80003671bb20 x28: 800035e74180
> x27: 08912000 x26: 001d
> x25: 0124 x24: 08f74d00
> x23: 004000114809 x22: 
> x21: 80003671bbd0 x20: 
> x19: 80003671bbd0 x18: 040d
> x17: 0001 x16: 
> x15:  x14: 
> x13:  x12: 0020
> x11: 0101010101010101 x10: fefefefefefefeff
> x9 : 7f7f7f7f7f7f7f7f x8 : fefefeff31677364
> x7 : 80808080 x6 : 80003671bc9c
> x5 : 80003671b9f8 x4 : 80002c296190
> x3 :  x2 : 
> x1 : 80003671bbd0 x0 : 80003671bc00
> Process wickedd (pid: 638, stack limit = 0x800036718000)
> Call trace:
> Exception stack(0x80003671b9e0 to 0x80003671bb20)
> b9e0: 80003671bc00 80003671bbd0  
> ba00: 80002c296190 80003671b9f8 80003671bc9c 80808080
> ba20: fefefeff31677364 7f7f7f7f7f7f7f7f fefefefefefefeff 0101010101010101
> ba40: 0020   
> ba60:  0001 040d 80003671bbd0
> ba80:  80003671bbd0  004000114809
> baa0: 08f74d00 0124 001d 08912000
> bac0: 800035e74180 80003671bb20 00dcca84 80003671bb20
> bae0: 086f7f30 2005 80002c296000 800035223900
> bb00:   80003671bb20 086f7f30
> [] phy_ethtool_ksettings_get+0x20/0x98
> [] lan78xx_get_link_ksettings+0x44/0x60 [lan78xx]
> [] ethtool_get_settings+0x68/0x210
> [] dev_ethtool+0x214/0x2180
> [] dev_ioctl+0x400/0x630
> [] sock_do_ioctl+0x70/0x88
> [] sock_ioctl+0x208/0x368
> [] do_vfs_ioctl+0xb0/0x848
> [] SyS_ioctl+0x8c/0xa8
> Exception stack(0x80003671bec0 to 0x80003671c000)
> bec0: 0009 8946 f4e841d0 aa0032687465
> bee0: fa2319d4 f4e841d4 32687465 32687465
> bf00: 001d 7f7fff7f7f7f7f7f 72606b622e71ff4c 7f7f7f7f7f7f7f7f
> bf20: 0101010101010101 0020  7f510c68
> bf40: 7f6a9d18 7f44ce30 040d 7f6f98f0
> bf60: f4e842c0 0001 fa2c2e00 7f6ab000
> bf80: f4e842c0 7f62a000 fa2b9f20 fa2c2e00
> bfa0: f4e84818 f4e841a0 7f5ad0cc f4e841a0
> bfc0: 7f44ce3c 8000 0009 001d
> bfe0:    
> 
> The culprit is quite simple: The driver tries to access the phy left and 
> right,
> but only actually has a working reference to it when the device is up.
> 
> The fix thus is quite simple too: Get a reference to the phy on probe already
> and keep it even when the device is going down.
> 
> With this patch applied, I can successfully run wicked on my system and bring
> the interface up and down as many times as I want, without getting NULL 
> pointer
> dereferences in between.
> 
> Signed-off-by: Alexander Graf 
> ---
>  drivers/net/usb/lan78xx.c | 21 ++---
>  1 file changed, 10 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
> index 60a604cc7647..931cc124ab0c 100644
> --- a/drivers/net/usb/lan78xx.c
> 

RE: [PATCH] lan78xx: Use common error handling code in lan78xx_phy_init()

[Woojung.Huh]

> From: SF Markus Elfring [mailto:elfr...@users.sourceforge.net]
> Sent: Saturday, October 28, 2017 4:57 PM
> To: net...@vger.kernel.org; linux-...@vger.kernel.org; UNGLinuxDriver;
> Woojung Huh - C21699
> Cc: LKML; kernel-janit...@vger.kernel.org
> Subject: [PATCH] lan78xx: Use common error handling code in
> lan78xx_phy_init()
> 
> From: Markus Elfring 
> Date: Sat, 28 Oct 2017 22:42:52 +0200
> 
> * Add a jump target so that a specific error message is stored only once
>   at the end of this function implementation.
> 
> * Replace two calls of the function "netdev_err" by goto statements.
> 
> * Adjust two condition checks.
> 
> This issue was detected by using the Coccinelle software.
> 
> Signed-off-by: Markus Elfring 
> ---

Reviewed-by: Woojung Huh 

RE: [PATCH] lan78xx: Use common error handling code in lan78xx_phy_init()

[Woojung.Huh]

> From: SF Markus Elfring [mailto:elfr...@users.sourceforge.net]
> Sent: Saturday, October 28, 2017 4:57 PM
> To: net...@vger.kernel.org; linux-...@vger.kernel.org; UNGLinuxDriver;
> Woojung Huh - C21699
> Cc: LKML; kernel-janit...@vger.kernel.org
> Subject: [PATCH] lan78xx: Use common error handling code in
> lan78xx_phy_init()
> 
> From: Markus Elfring 
> Date: Sat, 28 Oct 2017 22:42:52 +0200
> 
> * Add a jump target so that a specific error message is stored only once
>   at the end of this function implementation.
> 
> * Replace two calls of the function "netdev_err" by goto statements.
> 
> * Adjust two condition checks.
> 
> This issue was detected by using the Coccinelle software.
> 
> Signed-off-by: Markus Elfring 
> ---

Reviewed-by: Woojung Huh 

RE: [PATCH net-next 2/2] net: dsa: lan9303: Learn addresses on CPU port when bridged

[Woojung.Huh]

Hi Egil,

> >> @@ -62,7 +80,10 @@ static struct sk_buff *lan9303_xmit(struct sk_buff
> *skb,
> >> struct net_device *dev)
> >>
> >>lan9303_tag = (u16 *)(skb->data + 2 * ETH_ALEN);
> >>lan9303_tag[0] = htons(ETH_P_8021Q);
> >> -  lan9303_tag[1] = htons(dp->index | BIT(4));
> >> +  lan9303_tag[1] = lan9303_tx_use_arl(dp, skb->data) ?
> >
> > How about using skb_mac_header(skb) than skb->data?
> >
> >> +  LAN9303_TAG_TX_USE_ALR :
> >> +  dp->index |
> >
> 
> I am not the expert here.
> 
> I see that skb_mac_header() is (skb->head + skb->mac_header). So it will
> cost a few nano seconds per packet. Not the end of the world though.
> But I see that other net/dsa/tag_*.c use skb->data, assuming that
> skb->data point to mac header.
> 

Revisited skb_mac_header(). It is basically skb->data after math.
Understand that it would be extra steps than referring skb->data directly.
Unless no one comments on this, please keep first patch.

Thanks.
Woojung

RE: [PATCH net-next 2/2] net: dsa: lan9303: Learn addresses on CPU port when bridged

[Woojung.Huh]

Hi Egil,

> >> @@ -62,7 +80,10 @@ static struct sk_buff *lan9303_xmit(struct sk_buff
> *skb,
> >> struct net_device *dev)
> >>
> >>lan9303_tag = (u16 *)(skb->data + 2 * ETH_ALEN);
> >>lan9303_tag[0] = htons(ETH_P_8021Q);
> >> -  lan9303_tag[1] = htons(dp->index | BIT(4));
> >> +  lan9303_tag[1] = lan9303_tx_use_arl(dp, skb->data) ?
> >
> > How about using skb_mac_header(skb) than skb->data?
> >
> >> +  LAN9303_TAG_TX_USE_ALR :
> >> +  dp->index |
> >
> 
> I am not the expert here.
> 
> I see that skb_mac_header() is (skb->head + skb->mac_header). So it will
> cost a few nano seconds per packet. Not the end of the world though.
> But I see that other net/dsa/tag_*.c use skb->data, assuming that
> skb->data point to mac header.
> 

Revisited skb_mac_header(). It is basically skb->data after math.
Understand that it would be extra steps than referring skb->data directly.
Unless no one comments on this, please keep first patch.

Thanks.
Woojung

RE: [PATCH net-next 2/2] net: dsa: lan9303: Learn addresses on CPU port when bridged

[Woojung.Huh]

Hi Egil,

> +static inline int lan9303_tx_use_arl(struct dsa_port *dp, u8 *dest_addr)
> +{
> + struct lan9303 *chip = dp->ds->priv;
> +
> + return chip->is_bridged && !ether_addr_equal(dest_addr,
> eth_stp_addr);
> +}
> 
>  static struct sk_buff *lan9303_xmit(struct sk_buff *skb, struct net_device
> *dev)
>  {
> @@ -62,7 +80,10 @@ static struct sk_buff *lan9303_xmit(struct sk_buff *skb,
> struct net_device *dev)
> 
>   lan9303_tag = (u16 *)(skb->data + 2 * ETH_ALEN);
>   lan9303_tag[0] = htons(ETH_P_8021Q);
> - lan9303_tag[1] = htons(dp->index | BIT(4));
> + lan9303_tag[1] = lan9303_tx_use_arl(dp, skb->data) ?

How about using skb_mac_header(skb) than skb->data?

> + LAN9303_TAG_TX_USE_ALR :
> + dp->index |

Thanks.
Woojung

RE: [PATCH net-next 2/2] net: dsa: lan9303: Learn addresses on CPU port when bridged

[Woojung.Huh]

Hi Egil,

> +static inline int lan9303_tx_use_arl(struct dsa_port *dp, u8 *dest_addr)
> +{
> + struct lan9303 *chip = dp->ds->priv;
> +
> + return chip->is_bridged && !ether_addr_equal(dest_addr,
> eth_stp_addr);
> +}
> 
>  static struct sk_buff *lan9303_xmit(struct sk_buff *skb, struct net_device
> *dev)
>  {
> @@ -62,7 +80,10 @@ static struct sk_buff *lan9303_xmit(struct sk_buff *skb,
> struct net_device *dev)
> 
>   lan9303_tag = (u16 *)(skb->data + 2 * ETH_ALEN);
>   lan9303_tag[0] = htons(ETH_P_8021Q);
> - lan9303_tag[1] = htons(dp->index | BIT(4));
> + lan9303_tag[1] = lan9303_tx_use_arl(dp, skb->data) ?

How about using skb_mac_header(skb) than skb->data?

> + LAN9303_TAG_TX_USE_ALR :
> + dp->index |

Thanks.
Woojung

RE: [PATCH net] net: dsa: mv88e6060: fix switch MAC address

[Woojung.Huh]

> -Original Message-
> From: netdev-ow...@vger.kernel.org [mailto:netdev-
> ow...@vger.kernel.org] On Behalf Of Vivien Didelot
> Sent: Friday, October 13, 2017 1:39 PM
> To: net...@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org; ker...@savoirfairelinux.com; David S.
> Miller; Florian Fainelli; Andrew Lunn; David Laight; Vivien Didelot
> Subject: [PATCH net] net: dsa: mv88e6060: fix switch MAC address
> 
> The 88E6060 Ethernet switch always transmits the multicast bit of the
> switch MAC address as a zero. It re-uses the corresponding bit 8 of the
> register "Switch MAC Address Register Bytes 0 & 1" for "DiffAddr".
> 
> If the "DiffAddr" bit is 0, then all ports transmit the same source
> address. If it is set to 1, then bit 2:0 are used for the port number.
> 
> The mv88e6060 driver is currently wrongly shifting the MAC address byte
> 0 by 9. To fix this, shift it by 8 as usual and clear its bit 0.
> 
> Signed-off-by: Vivien Didelot 
> ---

Reviewed-by: Woojung Huh 

- Woojung

RE: [PATCH net] net: dsa: mv88e6060: fix switch MAC address

[Woojung.Huh]

> -Original Message-
> From: netdev-ow...@vger.kernel.org [mailto:netdev-
> ow...@vger.kernel.org] On Behalf Of Vivien Didelot
> Sent: Friday, October 13, 2017 1:39 PM
> To: net...@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org; ker...@savoirfairelinux.com; David S.
> Miller; Florian Fainelli; Andrew Lunn; David Laight; Vivien Didelot
> Subject: [PATCH net] net: dsa: mv88e6060: fix switch MAC address
> 
> The 88E6060 Ethernet switch always transmits the multicast bit of the
> switch MAC address as a zero. It re-uses the corresponding bit 8 of the
> register "Switch MAC Address Register Bytes 0 & 1" for "DiffAddr".
> 
> If the "DiffAddr" bit is 0, then all ports transmit the same source
> address. If it is set to 1, then bit 2:0 are used for the port number.
> 
> The mv88e6060 driver is currently wrongly shifting the MAC address byte
> 0 by 9. To fix this, shift it by 8 as usual and clear its bit 0.
> 
> Signed-off-by: Vivien Didelot 
> ---

Reviewed-by: Woojung Huh 

- Woojung

RE: [PATCH net-next v2 2/4] net: dsa: mv88e6060: setup random mac address

[Woojung.Huh]

Hi Vivien,

> >> > +REG_WRITE(REG_GLOBAL, GLOBAL_MAC_01, (addr[0] << 9) |
> >> addr[1]);
> >>
> >> Is that supposed to be 9 ?
> >
> > Looks like it.
> > Check
> http://www.marvell.com/switching/assets/marvell_linkstreet_88E6060_data
> sheet.pdf
> 
> Hum, David is correct, there is a bug in the driver which needs to be
> addressed first. MAC address bit 40 is addr[0] & 0x1, thus we must
> shift byte 0 by 8 and mask it against 0xfe.
> 
> I'll respin this serie including a fix for both net and net-next.

Yes, you are right. Missed description about bit 40.

Thanks.
Woojung

RE: [PATCH net-next v2 2/4] net: dsa: mv88e6060: setup random mac address

[Woojung.Huh]

Hi Vivien,

> >> > +REG_WRITE(REG_GLOBAL, GLOBAL_MAC_01, (addr[0] << 9) |
> >> addr[1]);
> >>
> >> Is that supposed to be 9 ?
> >
> > Looks like it.
> > Check
> http://www.marvell.com/switching/assets/marvell_linkstreet_88E6060_data
> sheet.pdf
> 
> Hum, David is correct, there is a bug in the driver which needs to be
> addressed first. MAC address bit 40 is addr[0] & 0x1, thus we must
> shift byte 0 by 8 and mask it against 0xfe.
> 
> I'll respin this serie including a fix for both net and net-next.

Yes, you are right. Missed description about bit 40.

Thanks.
Woojung

RE: [PATCH net-next v2 2/4] net: dsa: mv88e6060: setup random mac address

[Woojung.Huh]

> From: Vivien Didelot
> > Sent: 13 October 2017 02:41
> > As for mv88e6xxx, setup the switch from within the mv88e6060 driver with
> > a random MAC address, and remove the .set_addr implementation.
> >
> > Signed-off-by: Vivien Didelot 
> > ---
> >  drivers/net/dsa/mv88e6060.c | 30 +++---
> >  1 file changed, 19 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/net/dsa/mv88e6060.c b/drivers/net/dsa/mv88e6060.c
> > index 621cdc46ad81..2f9d5e6a0f97 100644
> > --- a/drivers/net/dsa/mv88e6060.c
> > +++ b/drivers/net/dsa/mv88e6060.c
> ...
> > +   REG_WRITE(REG_GLOBAL, GLOBAL_MAC_01, (addr[0] << 9) |
> addr[1]);
> 
> Is that supposed to be 9 ?

Looks like it.
Check 
http://www.marvell.com/switching/assets/marvell_linkstreet_88E6060_datasheet.pdf

Woojung

RE: [PATCH net-next v2 2/4] net: dsa: mv88e6060: setup random mac address

[Woojung.Huh]

> From: Vivien Didelot
> > Sent: 13 October 2017 02:41
> > As for mv88e6xxx, setup the switch from within the mv88e6060 driver with
> > a random MAC address, and remove the .set_addr implementation.
> >
> > Signed-off-by: Vivien Didelot 
> > ---
> >  drivers/net/dsa/mv88e6060.c | 30 +++---
> >  1 file changed, 19 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/net/dsa/mv88e6060.c b/drivers/net/dsa/mv88e6060.c
> > index 621cdc46ad81..2f9d5e6a0f97 100644
> > --- a/drivers/net/dsa/mv88e6060.c
> > +++ b/drivers/net/dsa/mv88e6060.c
> ...
> > +   REG_WRITE(REG_GLOBAL, GLOBAL_MAC_01, (addr[0] << 9) |
> addr[1]);
> 
> Is that supposed to be 9 ?

Looks like it.
Check 
http://www.marvell.com/switching/assets/marvell_linkstreet_88E6060_datasheet.pdf

Woojung

RE: [PATCH v2 net-next 1/2] net: dsa: lan9303: Move tag setup to new lan9303_setup_tagging

[Woojung.Huh]

>  @@ -644,6 +648,10 @@ static int lan9303_setup(struct dsa_switch *ds)
>   return -EINVAL;
>   }
> 
>  +ret = lan9303_setup_tagging(chip);
>  +if (ret)
>  +dev_err(chip->dev, "failed to setup port tagging %d\n", 
>  ret);
>  +
> >>> Still move on when error happens?
> >>>
> >> Good question. I just followed the pattern from the original function,
> >> which was not made by me. Actually I did once reflect on whether this
> >> was the correct way. Perhaps it could be argued that it is better to
> >> allow the device to come up, so the problem can be investigated?
> > Maybe depends on severity of setting?
> > BTW, lan9303_setup() still returns ZERO at the end?
> I did quick survey of the _setup functions of the other dsa drivers.
> Some return on error, some ignore errors.
> If you think so, I can make a v3 series that return on error. Otherwise
> I leave it as it is.

Unless Andrew, Vivien or Florian raises flag, I guess it will be fine as-is.

Thanks.
Woojung

RE: [PATCH v2 net-next 1/2] net: dsa: lan9303: Move tag setup to new lan9303_setup_tagging

[Woojung.Huh]

>  @@ -644,6 +648,10 @@ static int lan9303_setup(struct dsa_switch *ds)
>   return -EINVAL;
>   }
> 
>  +ret = lan9303_setup_tagging(chip);
>  +if (ret)
>  +dev_err(chip->dev, "failed to setup port tagging %d\n", 
>  ret);
>  +
> >>> Still move on when error happens?
> >>>
> >> Good question. I just followed the pattern from the original function,
> >> which was not made by me. Actually I did once reflect on whether this
> >> was the correct way. Perhaps it could be argued that it is better to
> >> allow the device to come up, so the problem can be investigated?
> > Maybe depends on severity of setting?
> > BTW, lan9303_setup() still returns ZERO at the end?
> I did quick survey of the _setup functions of the other dsa drivers.
> Some return on error, some ignore errors.
> If you think so, I can make a v3 series that return on error. Otherwise
> I leave it as it is.

Unless Andrew, Vivien or Florian raises flag, I guess it will be fine as-is.

Thanks.
Woojung

RE: [PATCH v2 net-next 1/2] net: dsa: lan9303: Move tag setup to new lan9303_setup_tagging

[Woojung.Huh]

> > Specific reason to use val then using
> LAN9303_BM_EGRSS_PORT_TYPE_SPECIAL_TAG_PORT0
> > like previous line?
> >
> Specific reason was to please a reviewer that did not like my
> indenting in first version. I did not agree with him, but since
> nobody else spoke up, I changed the code.
Got it. Missed previous patch/comment.

> >> @@ -644,6 +648,10 @@ static int lan9303_setup(struct dsa_switch *ds)
> >>return -EINVAL;
> >>}
> >>
> >> +  ret = lan9303_setup_tagging(chip);
> >> +  if (ret)
> >> +  dev_err(chip->dev, "failed to setup port tagging %d\n", ret);
> >> +
> > Still move on when error happens?
> >
> Good question. I just followed the pattern from the original function,
> which was not made by me. Actually I did once reflect on whether this
> was the correct way. Perhaps it could be argued that it is better to
> allow the device to come up, so the problem can be investigated?
Maybe depends on severity of setting?
BTW, lan9303_setup() still returns ZERO at the end?

Thanks.
Woojung

RE: [PATCH v2 net-next 1/2] net: dsa: lan9303: Move tag setup to new lan9303_setup_tagging

[Woojung.Huh]

> > Specific reason to use val then using
> LAN9303_BM_EGRSS_PORT_TYPE_SPECIAL_TAG_PORT0
> > like previous line?
> >
> Specific reason was to please a reviewer that did not like my
> indenting in first version. I did not agree with him, but since
> nobody else spoke up, I changed the code.
Got it. Missed previous patch/comment.

> >> @@ -644,6 +648,10 @@ static int lan9303_setup(struct dsa_switch *ds)
> >>return -EINVAL;
> >>}
> >>
> >> +  ret = lan9303_setup_tagging(chip);
> >> +  if (ret)
> >> +  dev_err(chip->dev, "failed to setup port tagging %d\n", ret);
> >> +
> > Still move on when error happens?
> >
> Good question. I just followed the pattern from the original function,
> which was not made by me. Actually I did once reflect on whether this
> was the correct way. Perhaps it could be argued that it is better to
> allow the device to come up, so the problem can be investigated?
Maybe depends on severity of setting?
BTW, lan9303_setup() still returns ZERO at the end?

Thanks.
Woojung

RE: [PATCH v2 net-next 1/2] net: dsa: lan9303: Move tag setup to new lan9303_setup_tagging

[Woojung.Huh]

> +/* forward special tagged packets from port 0 to port 1 *or* port 2 */
> +static int lan9303_setup_tagging(struct lan9303 *chip)
> +{
> + int ret;
> + u32 val;
> + /* enable defining the destination port via special VLAN tagging
> +  * for port 0
> +  */
> + ret = lan9303_write_switch_reg(chip,
> LAN9303_SWE_INGRESS_PORT_TYPE,
> +
> LAN9303_SWE_INGRESS_PORT_TYPE_VLAN);
> + if (ret)
> + return ret;
> +
> + /* tag incoming packets at port 1 and 2 on their way to port 0 to be
> +  * able to discover their source port
> +  */
> + val = LAN9303_BM_EGRSS_PORT_TYPE_SPECIAL_TAG_PORT0;
> + return lan9303_write_switch_reg(chip,
> LAN9303_BM_EGRSS_PORT_TYPE, val);
Specific reason to use val then using 
LAN9303_BM_EGRSS_PORT_TYPE_SPECIAL_TAG_PORT0
like previous line?

> @@ -644,6 +648,10 @@ static int lan9303_setup(struct dsa_switch *ds)
>   return -EINVAL;
>   }
> 
> + ret = lan9303_setup_tagging(chip);
> + if (ret)
> + dev_err(chip->dev, "failed to setup port tagging %d\n", ret);
> +
Still move on when error happens?

>   ret = lan9303_separate_ports(chip);
>   if (ret)
>   dev_err(chip->dev, "failed to separate ports %d\n", ret);
> --
> 2.11.0

- Woojung

RE: [PATCH v2 net-next 1/2] net: dsa: lan9303: Move tag setup to new lan9303_setup_tagging

[Woojung.Huh]

> +/* forward special tagged packets from port 0 to port 1 *or* port 2 */
> +static int lan9303_setup_tagging(struct lan9303 *chip)
> +{
> + int ret;
> + u32 val;
> + /* enable defining the destination port via special VLAN tagging
> +  * for port 0
> +  */
> + ret = lan9303_write_switch_reg(chip,
> LAN9303_SWE_INGRESS_PORT_TYPE,
> +
> LAN9303_SWE_INGRESS_PORT_TYPE_VLAN);
> + if (ret)
> + return ret;
> +
> + /* tag incoming packets at port 1 and 2 on their way to port 0 to be
> +  * able to discover their source port
> +  */
> + val = LAN9303_BM_EGRSS_PORT_TYPE_SPECIAL_TAG_PORT0;
> + return lan9303_write_switch_reg(chip,
> LAN9303_BM_EGRSS_PORT_TYPE, val);
Specific reason to use val then using 
LAN9303_BM_EGRSS_PORT_TYPE_SPECIAL_TAG_PORT0
like previous line?

> @@ -644,6 +648,10 @@ static int lan9303_setup(struct dsa_switch *ds)
>   return -EINVAL;
>   }
> 
> + ret = lan9303_setup_tagging(chip);
> + if (ret)
> + dev_err(chip->dev, "failed to setup port tagging %d\n", ret);
> +
Still move on when error happens?

>   ret = lan9303_separate_ports(chip);
>   if (ret)
>   dev_err(chip->dev, "failed to separate ports %d\n", ret);
> --
> 2.11.0

- Woojung

RE: [PATCH v1 RFC 1/7] Replace license with GPL

[Woojung.Huh]

> Subject: [PATCH v1 RFC 1/7] Replace license with GPL
> 
> From: Tristram Ha 
> 
> Replace license with GPL.
> 
> Signed-off-by: Tristram Ha 

Reviewed-by: Woojung Huh 

RE: [PATCH v1 RFC 1/7] Replace license with GPL

[Woojung.Huh]

> Subject: [PATCH v1 RFC 1/7] Replace license with GPL
> 
> From: Tristram Ha 
> 
> Replace license with GPL.
> 
> Signed-off-by: Tristram Ha 

Reviewed-by: Woojung Huh 

RE: [PATCH RFC 3/5] Add KSZ8795 switch driver

[Woojung.Huh]

> > > > @@ -0,0 +1,2066 @@
> > > > +/*
> > > > + * Microchip KSZ8795 switch driver
> > > > + *
> > > > + * Copyright (C) 2017 Microchip Technology Inc.
> > > > + * Tristram Ha 
> > > > + *
> > > > + * Permission to use, copy, modify, and/or distribute this software for
> any
> > > > + * purpose with or without fee is hereby granted, provided that the
> above
> > > > + * copyright notice and this permission notice appear in all copies.
> > > > + *
> > > > + * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS
> ALL
> > > WARRANTIES
> > > > + * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED
> WARRANTIES
> > > OF
> > > > + * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR
> BE
> > > LIABLE FOR
> > > > + * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR
> ANY
> > > DAMAGES
> > > > + * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS,
> WHETHER
> > > IN AN
> > > > + * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION,
> > > ARISING OUT OF
> > > > + * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS
> SOFTWARE.
> > > > + */
> > >
> > > This is not exactly GPL, right? But tagging below says it is
> > > GPL. Please fix one.
> > >
> >
> > This boilerplate paragraph was copied from the KSZ9477 driver, although I
> did
> > wonder why this was used.
> 
> Hi Tristram
> 
> Please can you talk to your legal people and see if this can be
> replaced with the standard GPL text?
This should be replaced to GPL. These text copied from drivers/net/dsa/b53/*.
Will submit patches of drivers/net/dsa/microchip/*

- Woojung

RE: [PATCH RFC 3/5] Add KSZ8795 switch driver

[Woojung.Huh]

> > > > @@ -0,0 +1,2066 @@
> > > > +/*
> > > > + * Microchip KSZ8795 switch driver
> > > > + *
> > > > + * Copyright (C) 2017 Microchip Technology Inc.
> > > > + * Tristram Ha 
> > > > + *
> > > > + * Permission to use, copy, modify, and/or distribute this software for
> any
> > > > + * purpose with or without fee is hereby granted, provided that the
> above
> > > > + * copyright notice and this permission notice appear in all copies.
> > > > + *
> > > > + * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS
> ALL
> > > WARRANTIES
> > > > + * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED
> WARRANTIES
> > > OF
> > > > + * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR
> BE
> > > LIABLE FOR
> > > > + * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR
> ANY
> > > DAMAGES
> > > > + * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS,
> WHETHER
> > > IN AN
> > > > + * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION,
> > > ARISING OUT OF
> > > > + * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS
> SOFTWARE.
> > > > + */
> > >
> > > This is not exactly GPL, right? But tagging below says it is
> > > GPL. Please fix one.
> > >
> >
> > This boilerplate paragraph was copied from the KSZ9477 driver, although I
> did
> > wonder why this was used.
> 
> Hi Tristram
> 
> Please can you talk to your legal people and see if this can be
> replaced with the standard GPL text?
This should be replaced to GPL. These text copied from drivers/net/dsa/b53/*.
Will submit patches of drivers/net/dsa/microchip/*

- Woojung

RE: [PATCH] DSA support for Micrel KSZ8895

[Woojung.Huh]

Pavel,

Thanks for update and sorry about email format (due to web-access version)
I'll do review when getting back to office later this week.

- Woojung

From: Pavel Machek [pa...@denx.de]
Sent: Sunday, August 27, 2017 8:36 AM
To: Woojung Huh - C21699; nathan.leigh.con...@gmail.com
Cc: vivien.dide...@savoirfairelinux.com; f.faine...@gmail.com; 
net...@vger.kernel.org; linux-kernel@vger.kernel.org; tristram...@micrel.com; 
and...@lunn.ch; pa...@denx.de
Subject: [PATCH] DSA support for Micrel KSZ8895

Hi!

So I fought with the driver a bit more, and now I have something that
kind-of-works.

"great great hack" belows worries me.

Yeah, disabled code needs to be removed before merge.

No, tag_ksz part probably is not acceptable. Do you see solution
better than just copying it into tag_ksz1 file?

Any more comments, etc?

Help would be welcome.

RE: [PATCH] DSA support for Micrel KSZ8895

[Woojung.Huh]

Pavel,

Thanks for update and sorry about email format (due to web-access version)
I'll do review when getting back to office later this week.

- Woojung

From: Pavel Machek [pa...@denx.de]
Sent: Sunday, August 27, 2017 8:36 AM
To: Woojung Huh - C21699; nathan.leigh.con...@gmail.com
Cc: vivien.dide...@savoirfairelinux.com; f.faine...@gmail.com; 
net...@vger.kernel.org; linux-kernel@vger.kernel.org; tristram...@micrel.com; 
and...@lunn.ch; pa...@denx.de
Subject: [PATCH] DSA support for Micrel KSZ8895

Hi!

So I fought with the driver a bit more, and now I have something that
kind-of-works.

"great great hack" belows worries me.

Yeah, disabled code needs to be removed before merge.

No, tag_ksz part probably is not acceptable. Do you see solution
better than just copying it into tag_ksz1 file?

Any more comments, etc?

Help would be welcome.

RE: DSA support for Micrel KSZ8895

[Woojung.Huh]

Pavel,

> > I'll forward your email to our support.
> > AFAIK, KSZ8895 has different register mapping from KSZ9477,
> > it will be more than ID changes in current driver.
> 
> More than ID changes, indeed. As layout is completely different, it
> looks like different source file will be needed for support.
> 
> I'm not nearly there; but I can ifconfig lanX up, already, and perform
> some pings.
> 
> Any ideas how to do the work in a way to minimize code duplication are
> welcome...

Which version do you use to create patch?
Getting error when applying patch to the latest net-next.

- Woojung

RE: DSA support for Micrel KSZ8895

[Woojung.Huh]

Pavel,

> > I'll forward your email to our support.
> > AFAIK, KSZ8895 has different register mapping from KSZ9477,
> > it will be more than ID changes in current driver.
> 
> More than ID changes, indeed. As layout is completely different, it
> looks like different source file will be needed for support.
> 
> I'm not nearly there; but I can ifconfig lanX up, already, and perform
> some pings.
> 
> Any ideas how to do the work in a way to minimize code duplication are
> welcome...

Which version do you use to create patch?
Getting error when applying patch to the latest net-next.

- Woojung

RE: DSA support for Micrel KSZ8895

[Woojung.Huh]

> > Hi!
> >
> > I've got hardware with KSZ8895, and I'd like to use switch ports as
> > separate ethernet cards. I believe that means DSA support.
> >
> > And there are even patches available from microchip... unfortunately
> > they are in strange form and for v3.18.
> >
> >
> http://www.microchip.com/SWLibraryWeb/product.aspx?product=KSZ8895
> %20Software%20Linux%203.18
> >
> > Is there newer version of the driver available somewhere? Is the
> > driver good starting point, or should I start with something else?
> 
> Hi Pavel
> 
> Woojung is the expert here. His DSA driver for the 9477 is a nice
> clean driver.
> 
> Have you compared the 8895 to the 9477. Are they similar? Could the
> existing 9477 be extended to support the 8895?
> 
>Andrew

Hi Pavel,

I'll forward your email to our support.
AFAIK, KSZ8895 has different register mapping from KSZ9477,
it will be more than ID changes in current driver.

Thanks.
Woojung

RE: DSA support for Micrel KSZ8895

[Woojung.Huh]

> > Hi!
> >
> > I've got hardware with KSZ8895, and I'd like to use switch ports as
> > separate ethernet cards. I believe that means DSA support.
> >
> > And there are even patches available from microchip... unfortunately
> > they are in strange form and for v3.18.
> >
> >
> http://www.microchip.com/SWLibraryWeb/product.aspx?product=KSZ8895
> %20Software%20Linux%203.18
> >
> > Is there newer version of the driver available somewhere? Is the
> > driver good starting point, or should I start with something else?
> 
> Hi Pavel
> 
> Woojung is the expert here. His DSA driver for the 9477 is a nice
> clean driver.
> 
> Have you compared the 8895 to the 9477. Are they similar? Could the
> existing 9477 be extended to support the 8895?
> 
>Andrew

Hi Pavel,

I'll forward your email to our support.
AFAIK, KSZ8895 has different register mapping from KSZ9477,
it will be more than ID changes in current driver.

Thanks.
Woojung

RE: [PATCH net-next 06/11] net: dsa: debugfs: add port registers

[Woojung.Huh]

Vivien,

> Subject: [PATCH net-next 06/11] net: dsa: debugfs: add port registers
> 
> Add a debug filesystem "regs" entry to query a port's hardware registers
> through the .get_regs_len and .get_regs_len switch operations.
> 
> This is very convenient because it allows one to dump the registers of
> DSA links, which are not exposed to userspace.
This series will be very useful to get various debug information.
Do you have any plan to expand 32bit register access and switch-level registers
In addition to port-level registers?

> +static void dsa_debugfs_regs_read_count(struct dsa_switch *ds, int id,
> + struct seq_file *seq, int count)
> +{
> + u16 data[count * ETH_GSTRING_LEN];
I think this should be u16 data[count] because it is value of registers.

> + struct ethtool_regs regs;
> + int i;
> +
> + ds->ops->get_regs(ds, id, , data);
> +
> + for (i = 0; i < count / 2; i++)
> + seq_printf(seq, "%2d: %04x\n", i, data[i]);
> +}
> +
> +static int dsa_debugfs_regs_read(struct dsa_switch *ds, int id,
> +  struct seq_file *seq)
> +{
> + int count;
> +
> + if (!ds->ops->get_regs_len || !ds->ops->get_regs)
> + return -EOPNOTSUPP;
> +
> + count = ds->ops->get_regs_len(ds, id);
> + if (count < 0)
> + return count;
Because get_regs_len returns length than count per mv88e6xxx/chip.c,
it requires some math before passing to dsa_debugfs_regs_read_count().

It does "count/2" in dsa-debugfs_regs_read_count(), however,
As commented above, "count" is used at "u16 data[...]".
So, it would be nice to match with name. Ie, count or size/length.

Thanks.
Woojung

RE: [PATCH net-next 06/11] net: dsa: debugfs: add port registers

[Woojung.Huh]

Vivien,

> Subject: [PATCH net-next 06/11] net: dsa: debugfs: add port registers
> 
> Add a debug filesystem "regs" entry to query a port's hardware registers
> through the .get_regs_len and .get_regs_len switch operations.
> 
> This is very convenient because it allows one to dump the registers of
> DSA links, which are not exposed to userspace.
This series will be very useful to get various debug information.
Do you have any plan to expand 32bit register access and switch-level registers
In addition to port-level registers?

> +static void dsa_debugfs_regs_read_count(struct dsa_switch *ds, int id,
> + struct seq_file *seq, int count)
> +{
> + u16 data[count * ETH_GSTRING_LEN];
I think this should be u16 data[count] because it is value of registers.

> + struct ethtool_regs regs;
> + int i;
> +
> + ds->ops->get_regs(ds, id, , data);
> +
> + for (i = 0; i < count / 2; i++)
> + seq_printf(seq, "%2d: %04x\n", i, data[i]);
> +}
> +
> +static int dsa_debugfs_regs_read(struct dsa_switch *ds, int id,
> +  struct seq_file *seq)
> +{
> + int count;
> +
> + if (!ds->ops->get_regs_len || !ds->ops->get_regs)
> + return -EOPNOTSUPP;
> +
> + count = ds->ops->get_regs_len(ds, id);
> + if (count < 0)
> + return count;
Because get_regs_len returns length than count per mv88e6xxx/chip.c,
it requires some math before passing to dsa_debugfs_regs_read_count().

It does "count/2" in dsa-debugfs_regs_read_count(), however,
As commented above, "count" is used at "u16 data[...]".
So, it would be nice to match with name. Ie, count or size/length.

Thanks.
Woojung

RE: [PATCH net] net: dsa: ksz: fix skb freeing

[Woojung.Huh]

> The DSA layer frees the original skb when an xmit function returns NULL,
> meaning an error occurred. But if the tagging code copied the original
> skb, it is responsible of freeing the copy if an error occurs.
> 
> The ksz tagging code currently has two issues: if skb_put_padto fails,
> the skb copy is not freed, and the original skb will be freed twice.
> 
> To fix that, move skb_put_padto inside both branches of the skb_tailroom
> condition, before freeing the original skb, and free the copy on error.
> 
> Signed-off-by: Vivien Didelot 
Reviewed-by: Woojung Huh 

RE: [PATCH net] net: dsa: ksz: fix skb freeing

[Woojung.Huh]

> The DSA layer frees the original skb when an xmit function returns NULL,
> meaning an error occurred. But if the tagging code copied the original
> skb, it is responsible of freeing the copy if an error occurs.
> 
> The ksz tagging code currently has two issues: if skb_put_padto fails,
> the skb copy is not freed, and the original skb will be freed twice.
> 
> To fix that, move skb_put_padto inside both branches of the skb_tailroom
> condition, before freeing the original skb, and free the copy on error.
> 
> Signed-off-by: Vivien Didelot 
Reviewed-by: Woojung Huh 

RE: [PATCH][net-next] net: dsa: make function ksz_rcv static

[Woojung.Huh]

> function ksz_rcv can be made static as it does not need to be
> in global scope. Reformat arguments to make it checkpatch warning
> free too.
> 
> Cleans up sparse warning: "symbol 'ksz_rcv' was not declared. Should
> it be static?"
> 
> Signed-off-by: Colin Ian King 

Reviewed-by: Woojung Huh 

RE: [PATCH][net-next] net: dsa: make function ksz_rcv static

[Woojung.Huh]

> function ksz_rcv can be made static as it does not need to be
> in global scope. Reformat arguments to make it checkpatch warning
> free too.
> 
> Cleans up sparse warning: "symbol 'ksz_rcv' was not declared. Should
> it be static?"
> 
> Signed-off-by: Colin Ian King 

Reviewed-by: Woojung Huh 

RE: [PATCH 1/1] net: usb: set error code when usb_alloc_urb fails

[Woojung.Huh]

> Signed-off-by: Pan Bian 
> ---
>  drivers/net/usb/lan78xx.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
> index db558b8..f33460c 100644
> --- a/drivers/net/usb/lan78xx.c
> +++ b/drivers/net/usb/lan78xx.c
> @@ -3395,6 +3395,7 @@ static int lan78xx_probe(struct usb_interface *intf,
>   if (buf) {
>   dev->urb_intr = usb_alloc_urb(0, GFP_KERNEL);
>   if (!dev->urb_intr) {
> + ret = -ENOMEM;
>   kfree(buf);
>   goto out3;
>   } else {
> --
> 1.9.1
> 
Acked-by: Woojung Huh 

RE: [PATCH 1/1] net: usb: set error code when usb_alloc_urb fails

[Woojung.Huh]

> Signed-off-by: Pan Bian 
> ---
>  drivers/net/usb/lan78xx.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
> index db558b8..f33460c 100644
> --- a/drivers/net/usb/lan78xx.c
> +++ b/drivers/net/usb/lan78xx.c
> @@ -3395,6 +3395,7 @@ static int lan78xx_probe(struct usb_interface *intf,
>   if (buf) {
>   dev->urb_intr = usb_alloc_urb(0, GFP_KERNEL);
>   if (!dev->urb_intr) {
> + ret = -ENOMEM;
>   kfree(buf);
>   goto out3;
>   } else {
> --
> 1.9.1
> 
Acked-by: Woojung Huh 

RE: [PATCH 15/15] net: usb: lan78xx: Utilize phy_ethtool_nway_reset

[Woojung.Huh]

> Signed-off-by: Florian Fainelli 
> ---
>  drivers/net/usb/lan78xx.c | 7 +--
>  1 file changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
> index bcd9010c1f27..cf2857fa938f 100644
> --- a/drivers/net/usb/lan78xx.c
> +++ b/drivers/net/usb/lan78xx.c
> @@ -1447,11 +1447,6 @@ static u32 lan78xx_get_link(struct net_device *net)
>   return net->phydev->link;
>  }
> 
> -static int lan78xx_nway_reset(struct net_device *net)
> -{
> - return phy_start_aneg(net->phydev);
> -}
> -
>  static void lan78xx_get_drvinfo(struct net_device *net,
>   struct ethtool_drvinfo *info)
>  {
> @@ -1655,7 +1650,7 @@ static int lan78xx_set_pause(struct net_device
> *net,
> 
>  static const struct ethtool_ops lan78xx_ethtool_ops = {
>   .get_link   = lan78xx_get_link,
> - .nway_reset = lan78xx_nway_reset,
> + .nway_reset = phy_ethtool_nway_reset,
>   .get_drvinfo= lan78xx_get_drvinfo,
>   .get_msglevel   = lan78xx_get_msglevel,
>   .set_msglevel   = lan78xx_set_msglevel,
> --
> 2.9.3
Acked-by: Woojung Huh 

RE: [PATCH 15/15] net: usb: lan78xx: Utilize phy_ethtool_nway_reset

[Woojung.Huh]

> Signed-off-by: Florian Fainelli 
> ---
>  drivers/net/usb/lan78xx.c | 7 +--
>  1 file changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
> index bcd9010c1f27..cf2857fa938f 100644
> --- a/drivers/net/usb/lan78xx.c
> +++ b/drivers/net/usb/lan78xx.c
> @@ -1447,11 +1447,6 @@ static u32 lan78xx_get_link(struct net_device *net)
>   return net->phydev->link;
>  }
> 
> -static int lan78xx_nway_reset(struct net_device *net)
> -{
> - return phy_start_aneg(net->phydev);
> -}
> -
>  static void lan78xx_get_drvinfo(struct net_device *net,
>   struct ethtool_drvinfo *info)
>  {
> @@ -1655,7 +1650,7 @@ static int lan78xx_set_pause(struct net_device
> *net,
> 
>  static const struct ethtool_ops lan78xx_ethtool_ops = {
>   .get_link   = lan78xx_get_link,
> - .nway_reset = lan78xx_nway_reset,
> + .nway_reset = phy_ethtool_nway_reset,
>   .get_drvinfo= lan78xx_get_drvinfo,
>   .get_msglevel   = lan78xx_get_msglevel,
>   .set_msglevel   = lan78xx_set_msglevel,
> --
> 2.9.3
Acked-by: Woojung Huh 

RE: [PATCH 0/2] lan78xx: Remove trailing underscores from macros

[Woojung.Huh]

> Joe Perches (2):
>   lan78xx: Remove locally defined trailing underscores from defines and uses
>   microchipphy.h and uses: Remove trailing underscores from defines and
> uses
> 
>  drivers/net/phy/microchip.c  |4 +-
>  drivers/net/usb/lan78xx.c|  368 +++
>  drivers/net/usb/lan78xx.h| 1068 +-
> 
>  include/linux/microchipphy.h |   72 +--
>  4 files changed, 756 insertions(+), 756 deletions(-)

Because there is no specific rule how to name defines, I'm not sure it is worth 
to change 1000+ lines.
It may be better to set guideline for new submissions.

Welcome any comments.

RE: [PATCH 0/2] lan78xx: Remove trailing underscores from macros

[Woojung.Huh]

> Joe Perches (2):
>   lan78xx: Remove locally defined trailing underscores from defines and uses
>   microchipphy.h and uses: Remove trailing underscores from defines and
> uses
> 
>  drivers/net/phy/microchip.c  |4 +-
>  drivers/net/usb/lan78xx.c|  368 +++
>  drivers/net/usb/lan78xx.h| 1068 +-
> 
>  include/linux/microchipphy.h |   72 +--
>  4 files changed, 756 insertions(+), 756 deletions(-)

Because there is no specific rule how to name defines, I'm not sure it is worth 
to change 1000+ lines.
It may be better to set guideline for new submissions.

Welcome any comments.

RE: [PATCH] lan78xx: Protect runtime_auto check by #ifdef CONFIG_PM

[Woojung.Huh]

> > > But this leaves open the issue that querying the device too often will
> > > prevent it from going into autosuspend.  It seems to me that the best
> > > way to deal with this is to make sure that the autosuspend timeout is
> > > shorter than the interal between queries, not to make the querying
> > > conditional on !runtime_auto.
> >
> > Short autosuspend timeout can affect performance. For instance our
> experiments showed that
> > shorter than 10sec timeout made Ethernet performance degrade because
> of wakeup delays.
> > So, just putting shorter timeout may have some side effects.
> 
> Sure.  This just means that you need a long statistics interval --
> longer than the autosuspend timeout.  That's why I suggested making the
> interval adjustable.

What do you mean statistics interval?
Interval calling ndo_get_stats64 or another thread/timer or else getting 
statistics?

Thanks.
Woojung

RE: [PATCH] lan78xx: Protect runtime_auto check by #ifdef CONFIG_PM

[Woojung.Huh]

> > > But this leaves open the issue that querying the device too often will
> > > prevent it from going into autosuspend.  It seems to me that the best
> > > way to deal with this is to make sure that the autosuspend timeout is
> > > shorter than the interal between queries, not to make the querying
> > > conditional on !runtime_auto.
> >
> > Short autosuspend timeout can affect performance. For instance our
> experiments showed that
> > shorter than 10sec timeout made Ethernet performance degrade because
> of wakeup delays.
> > So, just putting shorter timeout may have some side effects.
> 
> Sure.  This just means that you need a long statistics interval --
> longer than the autosuspend timeout.  That's why I suggested making the
> interval adjustable.

What do you mean statistics interval?
Interval calling ndo_get_stats64 or another thread/timer or else getting 
statistics?

Thanks.
Woojung

RE: [PATCH] lan78xx: Protect runtime_auto check by #ifdef CONFIG_PM

[Woojung.Huh]

> But this leaves open the issue that querying the device too often will
> prevent it from going into autosuspend.  It seems to me that the best
> way to deal with this is to make sure that the autosuspend timeout is
> shorter than the interal between queries, not to make the querying
> conditional on !runtime_auto.

Short autosuspend timeout can affect performance. For instance our experiments 
showed that
shorter than 10sec timeout made Ethernet performance degrade because of wakeup 
delays.
So, just putting shorter timeout may have some side effects.

Woojung

RE: [PATCH] lan78xx: Protect runtime_auto check by #ifdef CONFIG_PM

[Woojung.Huh]

> But this leaves open the issue that querying the device too often will
> prevent it from going into autosuspend.  It seems to me that the best
> way to deal with this is to make sure that the autosuspend timeout is
> shorter than the interal between queries, not to make the querying
> conditional on !runtime_auto.

Short autosuspend timeout can affect performance. For instance our experiments 
showed that
shorter than 10sec timeout made Ethernet performance degrade because of wakeup 
delays.
So, just putting shorter timeout may have some side effects.

Woojung

RE: [PATCH] lan78xx: Protect runtime_auto check by #ifdef CONFIG_PM

[Woojung.Huh]

> -Original Message-
> From: Guenter Roeck [mailto:li...@roeck-us.net]
> Sent: Sunday, March 20, 2016 6:59 PM
> To: Geert Uytterhoeven; Woojung Huh - C21699; UNGLinuxDriver; David S.
> Miller
> Cc: Rafael J. Wysocki; net...@vger.kernel.org; linux-...@vger.kernel.org;
> linux...@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] lan78xx: Protect runtime_auto check by #ifdef
> CONFIG_PM
>  
> Not that it matters anymore since David reverted the original patch,
> but my reason for not sending a similar patch was that I wasn't sure
> if .runtime_auto should be accessed from drivers in the first place,
> or if there is some logical problem with the code.
> 

Because driver can enable/disable autosuspend by usb_enable_autosuspend() and
usb_disable_autosuspend(), it would be nice to have helper to find out 
autosuspend status.

The code path was to utilize autosuspend power saving, when it is enabled.

Woojung

RE: [PATCH] lan78xx: Protect runtime_auto check by #ifdef CONFIG_PM

[Woojung.Huh]

> -Original Message-
> From: Guenter Roeck [mailto:li...@roeck-us.net]
> Sent: Sunday, March 20, 2016 6:59 PM
> To: Geert Uytterhoeven; Woojung Huh - C21699; UNGLinuxDriver; David S.
> Miller
> Cc: Rafael J. Wysocki; net...@vger.kernel.org; linux-...@vger.kernel.org;
> linux...@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] lan78xx: Protect runtime_auto check by #ifdef
> CONFIG_PM
>  
> Not that it matters anymore since David reverted the original patch,
> but my reason for not sending a similar patch was that I wasn't sure
> if .runtime_auto should be accessed from drivers in the first place,
> or if there is some logical problem with the code.
> 

Because driver can enable/disable autosuspend by usb_enable_autosuspend() and
usb_disable_autosuspend(), it would be nice to have helper to find out 
autosuspend status.

The code path was to utilize autosuspend power saving, when it is enabled.

Woojung

RE: [PATCH] lan78xx: Protect runtime_auto check by #ifdef CONFIG_PM

[Woojung.Huh]

> -Original Message-
> From: Oliver Neukum [mailto:oneu...@suse.com]
> Sent: Monday, March 21, 2016 4:36 AM
> To: Geert Uytterhoeven
> Cc: Woojung Huh - C21699; UNGLinuxDriver; David S. Miller; Guenter Roeck;
> Rafael J. Wysocki; net...@vger.kernel.org; linux-...@vger.kernel.org;
> linux...@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] lan78xx: Protect runtime_auto check by #ifdef
> CONFIG_PM

Thanks for all comments.
Will look into it and submit new patch.

- Woojung

RE: [PATCH] lan78xx: Protect runtime_auto check by #ifdef CONFIG_PM

[Woojung.Huh]

> -Original Message-
> From: Oliver Neukum [mailto:oneu...@suse.com]
> Sent: Monday, March 21, 2016 4:36 AM
> To: Geert Uytterhoeven
> Cc: Woojung Huh - C21699; UNGLinuxDriver; David S. Miller; Guenter Roeck;
> Rafael J. Wysocki; net...@vger.kernel.org; linux-...@vger.kernel.org;
> linux...@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] lan78xx: Protect runtime_auto check by #ifdef
> CONFIG_PM

Thanks for all comments.
Will look into it and submit new patch.

- Woojung