Re: [PATCH resubmit] ax88179_178a: ASIX AX88179_178A USB 3.0/2.0 to gigabit ethernet adapter driver

2013-03-02 Thread David Miller 
From: Freddy Xin 
Date: Sat,  2 Mar 2013 18:41:11 +0800

> From: Freddy Xin 
> 
> This is a resubmission.
> Added kfree() in ax88179_get_eeprom to prevent memory leakage.
> Modified "__le16 rxctl" to "u16 rxctl" in "struct ax88179_data" and removed 
> pointless casts.
> Removed asix_init and asix_exit functions and added 
> "module_usb_driver(ax88179_178a_driver)".
> Fixed endianness issue on big endian systems and verified this driver on 
> iBook G4.
> Removed steps that change net->features in ax88179_set_features function.
> Added "const" to ethtool_ops structure and fixed the coding style of 
> AX88179_BULKIN_SIZE array.
> Fixed the issue that the default MTU is not 1500.
> Added ax88179_change_mtu function and enabled the hardware jumbo frame 
> function to support an
> MTU higher than 1500.
> Fixed indentation and empty line coding style errors.
> The _nopm version usb functions were added to access register in suspend and 
> resume functions.
> Serveral variables allocted dynamically were removed and replaced by stack 
> variables.
> ax88179_get_eeprom were modified from asix_get_eeprom in asix_common.
> 
> This patch adds a driver for ASIX's AX88179 family of USB 3.0/2.0
> to gigabit ethernet adapters. It's based on the AX88xxx driver but
> the usb commands used to access registers for AX88179 are completely 
> different.
> This driver had been verified on x86 system with AX88179/AX88178A and
> Sitcomm LN-032 USB dongles.
> 
> Signed-off-by: Freddy Xin 

Applied.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH resubmit] ax88179_178a: ASIX AX88179_178A USB 3.0/2.0 to gigabit ethernet adapter driver

2013-03-02 Thread David Miller 
From: Freddy Xin fre...@asix.com.tw
Date: Sat,  2 Mar 2013 18:41:11 +0800

 From: Freddy Xin fre...@asix.com.tw
 
 This is a resubmission.
 Added kfree() in ax88179_get_eeprom to prevent memory leakage.
 Modified __le16 rxctl to u16 rxctl in struct ax88179_data and removed 
 pointless casts.
 Removed asix_init and asix_exit functions and added 
 module_usb_driver(ax88179_178a_driver).
 Fixed endianness issue on big endian systems and verified this driver on 
 iBook G4.
 Removed steps that change net-features in ax88179_set_features function.
 Added const to ethtool_ops structure and fixed the coding style of 
 AX88179_BULKIN_SIZE array.
 Fixed the issue that the default MTU is not 1500.
 Added ax88179_change_mtu function and enabled the hardware jumbo frame 
 function to support an
 MTU higher than 1500.
 Fixed indentation and empty line coding style errors.
 The _nopm version usb functions were added to access register in suspend and 
 resume functions.
 Serveral variables allocted dynamically were removed and replaced by stack 
 variables.
 ax88179_get_eeprom were modified from asix_get_eeprom in asix_common.
 
 This patch adds a driver for ASIX's AX88179 family of USB 3.0/2.0
 to gigabit ethernet adapters. It's based on the AX88xxx driver but
 the usb commands used to access registers for AX88179 are completely 
 different.
 This driver had been verified on x86 system with AX88179/AX88178A and
 Sitcomm LN-032 USB dongles.
 
 Signed-off-by: Freddy Xin fre...@asix.com.tw

Applied.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH resubmit] ax88179_178a: ASIX AX88179_178A USB 3.0/2.0 to gigabit ethernet adapter driver

2013-03-01 Thread David Miller 
From: Freddy Xin 
Date: Wed, 27 Feb 2013 20:01:58 +0800

> + eeprom_buff = kmalloc(sizeof(u16) * (last_word - first_word + 1),
> +   GFP_KERNEL);
> + if (!eeprom_buff)
> + return -ENOMEM;
> +
> + /* ax88179/178A returns 2 bytes from eeprom on read */
> + for (i = first_word; i <= last_word; i++) {
> + ret = __ax88179_read_cmd(dev, AX_ACCESS_EEPROM, i, 1, 2,
> +  _buff[i - first_word],
> +  0);
> + if (ret < 0)
> + return -EIO;

In this -EIO error path we will leak eeprom_buff, you must free it in
all possible exit cases of this function after the allocation succeeds.

> + data->rxctl = (__force __le16)(AX_RX_CTL_START | AX_RX_CTL_AB |
> +AX_RX_CTL_IPE);

You're not storying a __le16 in data->rxctl, you're storing the bits
in cpu endianness.  So make it a plain u16 instead and get rid of
these pointless casts.

> +static int __init asix_init(void)
> +{
> + return usb_register(_driver);
> +}
> +module_init(asix_init);
> +
> +static void __exit asix_exit(void)
> +{
> + usb_deregister(_driver);
> +}
> +module_exit(asix_exit);

You can replace all of this with a simple:

module_usb_driver(asix_driver);
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH resubmit] ax88179_178a: ASIX AX88179_178A USB 3.0/2.0 to gigabit ethernet adapter driver

2013-03-01 Thread David Miller 
From: Freddy Xin fre...@asix.com.tw
Date: Wed, 27 Feb 2013 20:01:58 +0800

 + eeprom_buff = kmalloc(sizeof(u16) * (last_word - first_word + 1),
 +   GFP_KERNEL);
 + if (!eeprom_buff)
 + return -ENOMEM;
 +
 + /* ax88179/178A returns 2 bytes from eeprom on read */
 + for (i = first_word; i = last_word; i++) {
 + ret = __ax88179_read_cmd(dev, AX_ACCESS_EEPROM, i, 1, 2,
 +  eeprom_buff[i - first_word],
 +  0);
 + if (ret  0)
 + return -EIO;

In this -EIO error path we will leak eeprom_buff, you must free it in
all possible exit cases of this function after the allocation succeeds.

 + data-rxctl = (__force __le16)(AX_RX_CTL_START | AX_RX_CTL_AB |
 +AX_RX_CTL_IPE);

You're not storying a __le16 in data-rxctl, you're storing the bits
in cpu endianness.  So make it a plain u16 instead and get rid of
these pointless casts.

 +static int __init asix_init(void)
 +{
 + return usb_register(asix_driver);
 +}
 +module_init(asix_init);
 +
 +static void __exit asix_exit(void)
 +{
 + usb_deregister(asix_driver);
 +}
 +module_exit(asix_exit);

You can replace all of this with a simple:

module_usb_driver(asix_driver);
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH, resubmit] ax88179_178a: ASIX AX88179_178A USB 3.0/2.0 to gigabit ethernet adapter driver

2013-02-08 Thread David Miller 
From: "David Laight" 
Date: Fri, 8 Feb 2013 10:23:08 -

> It is much better to define constants for the bit values and
> explicitly mask them as required.

Yes, __be32/__le32 along with bit define macros is the only reasonable
way to do this kind of stuff.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH, resubmit] ax88179_178a: ASIX AX88179_178A USB 3.0/2.0 to gigabit ethernet adapter driver

2013-02-08 Thread David Laight 
> > +struct ax88179_rx_pkt_header {
> > +   u8  l4_csum_err:1,
> > +   l3_csum_err:1,
> > +   l4_type:3,
> > +   l3_type:2,
> > +   ce:1;
> > +
> > +   u8  vlan_ind:3,
> > +   rx_ok:1,
> > +   pri:3,
> > +   bmc:1;
> > +
> > +   u16 len:13,
> > +   crc:1,
> > +   mii:1,
> > +   drop:1;
> > +} __packed;
> 
> This won't work on both big-endian systems (assuming this works on x86).
> You apparently try to convert the structure in-place in
> ax88179_rx_fixup() by calling le32_to_cpus(); that may work if you
> define all the bitfields to be part of a u32 but it won't work with the
> current definition.

Trying to use bit fields to map hardware registers (etc) isn't a
good idea at all. The C standard says absolutely nothing about
the order in which the bits are allocated to the field names.
(There are at least 4 possible orederings.)

It is much better to define constants for the bit values and
explicitly mask them as required.

David

RE: [PATCH, resubmit] ax88179_178a: ASIX AX88179_178A USB 3.0/2.0 to gigabit ethernet adapter driver

2013-02-08 Thread David Laight 
  +struct ax88179_rx_pkt_header {
  +   u8  l4_csum_err:1,
  +   l3_csum_err:1,
  +   l4_type:3,
  +   l3_type:2,
  +   ce:1;
  +
  +   u8  vlan_ind:3,
  +   rx_ok:1,
  +   pri:3,
  +   bmc:1;
  +
  +   u16 len:13,
  +   crc:1,
  +   mii:1,
  +   drop:1;
  +} __packed;
 
 This won't work on both big-endian systems (assuming this works on x86).
 You apparently try to convert the structure in-place in
 ax88179_rx_fixup() by calling le32_to_cpus(); that may work if you
 define all the bitfields to be part of a u32 but it won't work with the
 current definition.

Trying to use bit fields to map hardware registers (etc) isn't a
good idea at all. The C standard says absolutely nothing about
the order in which the bits are allocated to the field names.
(There are at least 4 possible orederings.)

It is much better to define constants for the bit values and
explicitly mask them as required.

David

Re: [PATCH, resubmit] ax88179_178a: ASIX AX88179_178A USB 3.0/2.0 to gigabit ethernet adapter driver

2013-02-08 Thread David Miller 
From: David Laight david.lai...@aculab.com
Date: Fri, 8 Feb 2013 10:23:08 -

 It is much better to define constants for the bit values and
 explicitly mask them as required.

Yes, __be32/__le32 along with bit define macros is the only reasonable
way to do this kind of stuff.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH, resubmit] ax88179_178a: ASIX AX88179_178A USB 3.0/2.0 to gigabit ethernet adapter driver

2013-02-07 Thread Freddy 

On 02/08/2013 04:02 AM, Ben Hutchings wrote:

On Thu, 2013-02-07 at 21:39 +0800, Freddy Xin wrote:

From: Freddy Xin 

This is a resubmission.
Added "const" to ethtool_ops structure and fixed the coding style of 
AX88179_BULKIN_SIZE array.
Fixed the issue that the default MTU is not 1500.
Added ax88179_change_mtu function and enabled the hardware jumbo frame function 
to support an
MTU higher than 1500.
Fixed indentation and empty line coding style errors.
The _nopm version usb functions were added to access register in suspend and 
resume functions.
Serveral variables allocted dynamically were removed and replaced by stack 
variables.
ax88179_get_eeprom were modified from asix_get_eeprom in asix_common.

This patch adds a driver for ASIX's AX88179 family of USB 3.0/2.0
to gigabit ethernet adapters. It's based on the AX88xxx driver but
the usb commands used to access registers for AX88179 are completely different.
This driver had been verified on x86 system with AX88179/AX88178A and
Sitcomm LN-032 USB dongles.

Signed-off-by: Freddy Xin 

[...]

--- /dev/null
+++ b/drivers/net/usb/ax88179_178a.c

[...]

+struct ax88179_data {
+   u16 rxctl;
+};

Should also be declared __packed, as on some architectures it may be
padded to 4 bytes.

rxctl is presumably required to be little-endian (__le16)?


+struct ax88179_int_data {
+   u16 res1;
+   u8 link;
+   u16 res2;
+   u8 status;
+   u16 res3;
+} __packed;

Same here, the u16 fields are presumably little-endian?

[...]

+struct ax88179_rx_pkt_header {
+   u8  l4_csum_err:1,
+   l3_csum_err:1,
+   l4_type:3,
+   l3_type:2,
+   ce:1;
+
+   u8  vlan_ind:3,
+   rx_ok:1,
+   pri:3,
+   bmc:1;
+
+   u16 len:13,
+   crc:1,
+   mii:1,
+   drop:1;
+} __packed;

This won't work on both big-endian systems (assuming this works on x86).
You apparently try to convert the structure in-place in
ax88179_rx_fixup() by calling le32_to_cpus(); that may work if you
define all the bitfields to be part of a u32 but it won't work with the
current definition.

[...]

+static int
+ax88179_set_features(struct net_device *net, netdev_features_t features)
+{
+   u8 tmp;
+   struct usbnet *dev = netdev_priv(net);
+   netdev_features_t changed = net->features ^ features;
+
+   if (changed & NETIF_F_TSO)
+   net->features ^= NETIF_F_TSO;
+
+   if (changed & NETIF_F_SG)
+   net->features ^= NETIF_F_SG;

Don't change net->features; the caller will do that for you.


+   if (changed & NETIF_F_IP_CSUM) {
+   ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_TXCOE_CTL, 1, 1, );
+   tmp ^= AX_TXCOE_TCP | AX_TXCOE_UDP;
+   ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_TXCOE_CTL, 1, 1, );
+
+   net->features ^= NETIF_F_IP_CSUM;
+   }

[...]

Isn't tmp going to be in little-endian byte order, so this doesn't work
correctly on a big-endian system?

There are a lot of reads and writes of 16-bit registers using
ax88179_{read,write}_cmd(); maybe you should add
ax88179_{read,write}_le16() to handle this specific case.

Ben.


Thank you, Ben. I will fix bugs and test it on a big-endian system.

Freddy
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH, resubmit] ax88179_178a: ASIX AX88179_178A USB 3.0/2.0 to gigabit ethernet adapter driver

2013-02-07 Thread David Miller 
From: Ben Hutchings 
Date: Thu, 7 Feb 2013 20:02:55 +

> On Thu, 2013-02-07 at 21:39 +0800, Freddy Xin wrote:
>> From: Freddy Xin 
>> 
>> This is a resubmission.
>> Added "const" to ethtool_ops structure and fixed the coding style of 
>> AX88179_BULKIN_SIZE array.
>> Fixed the issue that the default MTU is not 1500.
>> Added ax88179_change_mtu function and enabled the hardware jumbo frame 
>> function to support an
>> MTU higher than 1500.
>> Fixed indentation and empty line coding style errors.
>> The _nopm version usb functions were added to access register in suspend and 
>> resume functions.
>> Serveral variables allocted dynamically were removed and replaced by stack 
>> variables.
>> ax88179_get_eeprom were modified from asix_get_eeprom in asix_common.
>> 
>> This patch adds a driver for ASIX's AX88179 family of USB 3.0/2.0
>> to gigabit ethernet adapters. It's based on the AX88xxx driver but
>> the usb commands used to access registers for AX88179 are completely 
>> different.
>> This driver had been verified on x86 system with AX88179/AX88178A and
>> Sitcomm LN-032 USB dongles.
>> 
>> Signed-off-by: Freddy Xin 
> [...]
>> --- /dev/null
>> +++ b/drivers/net/usb/ax88179_178a.c
> [...]
>> +struct ax88179_data {
>> +   u16 rxctl;
>> +};
> 
> Should also be declared __packed, as on some architectures it may be
> padded to 4 bytes.

Packed is evil and should be avoid everywhere that it is possible to
do so.  It's going to use byte loads on this u16 on RISC cpus simply
because __packed makes it so that the compiler cannot assume data
alignment any longer.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH, resubmit] ax88179_178a: ASIX AX88179_178A USB 3.0/2.0 to gigabit ethernet adapter driver

2013-02-07 Thread Ben Hutchings 
On Thu, 2013-02-07 at 21:39 +0800, Freddy Xin wrote:
> From: Freddy Xin 
> 
> This is a resubmission.
> Added "const" to ethtool_ops structure and fixed the coding style of 
> AX88179_BULKIN_SIZE array.
> Fixed the issue that the default MTU is not 1500.
> Added ax88179_change_mtu function and enabled the hardware jumbo frame 
> function to support an
> MTU higher than 1500.
> Fixed indentation and empty line coding style errors.
> The _nopm version usb functions were added to access register in suspend and 
> resume functions.
> Serveral variables allocted dynamically were removed and replaced by stack 
> variables.
> ax88179_get_eeprom were modified from asix_get_eeprom in asix_common.
> 
> This patch adds a driver for ASIX's AX88179 family of USB 3.0/2.0
> to gigabit ethernet adapters. It's based on the AX88xxx driver but
> the usb commands used to access registers for AX88179 are completely 
> different.
> This driver had been verified on x86 system with AX88179/AX88178A and
> Sitcomm LN-032 USB dongles.
> 
> Signed-off-by: Freddy Xin 
[...]
> --- /dev/null
> +++ b/drivers/net/usb/ax88179_178a.c
[...]
> +struct ax88179_data {
> +   u16 rxctl;
> +};

Should also be declared __packed, as on some architectures it may be
padded to 4 bytes.

rxctl is presumably required to be little-endian (__le16)?

> +struct ax88179_int_data {
> +   u16 res1;
> +   u8 link;
> +   u16 res2;
> +   u8 status;
> +   u16 res3;
> +} __packed;

Same here, the u16 fields are presumably little-endian?

[...]
> +struct ax88179_rx_pkt_header {
> + u8  l4_csum_err:1,
> + l3_csum_err:1,
> + l4_type:3,
> + l3_type:2,
> + ce:1;
> +
> + u8  vlan_ind:3,
> + rx_ok:1,
> + pri:3,
> + bmc:1;
> +
> + u16 len:13,
> + crc:1,
> + mii:1,
> + drop:1;
> +} __packed;

This won't work on both big-endian systems (assuming this works on x86).
You apparently try to convert the structure in-place in
ax88179_rx_fixup() by calling le32_to_cpus(); that may work if you
define all the bitfields to be part of a u32 but it won't work with the
current definition.

[...]
> +static int
> +ax88179_set_features(struct net_device *net, netdev_features_t features)
> +{
> + u8 tmp;
> + struct usbnet *dev = netdev_priv(net);
> + netdev_features_t changed = net->features ^ features;
> +
> + if (changed & NETIF_F_TSO)
> + net->features ^= NETIF_F_TSO;
> +
> + if (changed & NETIF_F_SG)
> + net->features ^= NETIF_F_SG;

Don't change net->features; the caller will do that for you.

> +   if (changed & NETIF_F_IP_CSUM) {
> +   ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_TXCOE_CTL, 1, 1, 
> );
> +   tmp ^= AX_TXCOE_TCP | AX_TXCOE_UDP;
> +   ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_TXCOE_CTL, 1, 1, 
> );
> +
> +   net->features ^= NETIF_F_IP_CSUM;
> +   }
[...]

Isn't tmp going to be in little-endian byte order, so this doesn't work
correctly on a big-endian system?

There are a lot of reads and writes of 16-bit registers using
ax88179_{read,write}_cmd(); maybe you should add
ax88179_{read,write}_le16() to handle this specific case.

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH, resubmit] ax88179_178a: ASIX AX88179_178A USB 3.0/2.0 to gigabit ethernet adapter driver

2013-02-07 Thread Ben Hutchings 
On Thu, 2013-02-07 at 21:39 +0800, Freddy Xin wrote:
 From: Freddy Xin fre...@asix.com.tw
 
 This is a resubmission.
 Added const to ethtool_ops structure and fixed the coding style of 
 AX88179_BULKIN_SIZE array.
 Fixed the issue that the default MTU is not 1500.
 Added ax88179_change_mtu function and enabled the hardware jumbo frame 
 function to support an
 MTU higher than 1500.
 Fixed indentation and empty line coding style errors.
 The _nopm version usb functions were added to access register in suspend and 
 resume functions.
 Serveral variables allocted dynamically were removed and replaced by stack 
 variables.
 ax88179_get_eeprom were modified from asix_get_eeprom in asix_common.
 
 This patch adds a driver for ASIX's AX88179 family of USB 3.0/2.0
 to gigabit ethernet adapters. It's based on the AX88xxx driver but
 the usb commands used to access registers for AX88179 are completely 
 different.
 This driver had been verified on x86 system with AX88179/AX88178A and
 Sitcomm LN-032 USB dongles.
 
 Signed-off-by: Freddy Xin fre...@asix.com.tw
[...]
 --- /dev/null
 +++ b/drivers/net/usb/ax88179_178a.c
[...]
 +struct ax88179_data {
 +   u16 rxctl;
 +};

Should also be declared __packed, as on some architectures it may be
padded to 4 bytes.

rxctl is presumably required to be little-endian (__le16)?

 +struct ax88179_int_data {
 +   u16 res1;
 +   u8 link;
 +   u16 res2;
 +   u8 status;
 +   u16 res3;
 +} __packed;

Same here, the u16 fields are presumably little-endian?

[...]
 +struct ax88179_rx_pkt_header {
 + u8  l4_csum_err:1,
 + l3_csum_err:1,
 + l4_type:3,
 + l3_type:2,
 + ce:1;
 +
 + u8  vlan_ind:3,
 + rx_ok:1,
 + pri:3,
 + bmc:1;
 +
 + u16 len:13,
 + crc:1,
 + mii:1,
 + drop:1;
 +} __packed;

This won't work on both big-endian systems (assuming this works on x86).
You apparently try to convert the structure in-place in
ax88179_rx_fixup() by calling le32_to_cpus(); that may work if you
define all the bitfields to be part of a u32 but it won't work with the
current definition.

[...]
 +static int
 +ax88179_set_features(struct net_device *net, netdev_features_t features)
 +{
 + u8 tmp;
 + struct usbnet *dev = netdev_priv(net);
 + netdev_features_t changed = net-features ^ features;
 +
 + if (changed  NETIF_F_TSO)
 + net-features ^= NETIF_F_TSO;
 +
 + if (changed  NETIF_F_SG)
 + net-features ^= NETIF_F_SG;

Don't change net-features; the caller will do that for you.

 +   if (changed  NETIF_F_IP_CSUM) {
 +   ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_TXCOE_CTL, 1, 1, 
 tmp);
 +   tmp ^= AX_TXCOE_TCP | AX_TXCOE_UDP;
 +   ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_TXCOE_CTL, 1, 1, 
 tmp);
 +
 +   net-features ^= NETIF_F_IP_CSUM;
 +   }
[...]

Isn't tmp going to be in little-endian byte order, so this doesn't work
correctly on a big-endian system?

There are a lot of reads and writes of 16-bit registers using
ax88179_{read,write}_cmd(); maybe you should add
ax88179_{read,write}_le16() to handle this specific case.

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH, resubmit] ax88179_178a: ASIX AX88179_178A USB 3.0/2.0 to gigabit ethernet adapter driver

2013-02-07 Thread David Miller 
From: Ben Hutchings bhutchi...@solarflare.com
Date: Thu, 7 Feb 2013 20:02:55 +

 On Thu, 2013-02-07 at 21:39 +0800, Freddy Xin wrote:
 From: Freddy Xin fre...@asix.com.tw
 
 This is a resubmission.
 Added const to ethtool_ops structure and fixed the coding style of 
 AX88179_BULKIN_SIZE array.
 Fixed the issue that the default MTU is not 1500.
 Added ax88179_change_mtu function and enabled the hardware jumbo frame 
 function to support an
 MTU higher than 1500.
 Fixed indentation and empty line coding style errors.
 The _nopm version usb functions were added to access register in suspend and 
 resume functions.
 Serveral variables allocted dynamically were removed and replaced by stack 
 variables.
 ax88179_get_eeprom were modified from asix_get_eeprom in asix_common.
 
 This patch adds a driver for ASIX's AX88179 family of USB 3.0/2.0
 to gigabit ethernet adapters. It's based on the AX88xxx driver but
 the usb commands used to access registers for AX88179 are completely 
 different.
 This driver had been verified on x86 system with AX88179/AX88178A and
 Sitcomm LN-032 USB dongles.
 
 Signed-off-by: Freddy Xin fre...@asix.com.tw
 [...]
 --- /dev/null
 +++ b/drivers/net/usb/ax88179_178a.c
 [...]
 +struct ax88179_data {
 +   u16 rxctl;
 +};
 
 Should also be declared __packed, as on some architectures it may be
 padded to 4 bytes.

Packed is evil and should be avoid everywhere that it is possible to
do so.  It's going to use byte loads on this u16 on RISC cpus simply
because __packed makes it so that the compiler cannot assume data
alignment any longer.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH, resubmit] ax88179_178a: ASIX AX88179_178A USB 3.0/2.0 to gigabit ethernet adapter driver

2013-02-07 Thread Freddy 

On 02/08/2013 04:02 AM, Ben Hutchings wrote:

On Thu, 2013-02-07 at 21:39 +0800, Freddy Xin wrote:

From: Freddy Xin fre...@asix.com.tw

This is a resubmission.
Added const to ethtool_ops structure and fixed the coding style of 
AX88179_BULKIN_SIZE array.
Fixed the issue that the default MTU is not 1500.
Added ax88179_change_mtu function and enabled the hardware jumbo frame function 
to support an
MTU higher than 1500.
Fixed indentation and empty line coding style errors.
The _nopm version usb functions were added to access register in suspend and 
resume functions.
Serveral variables allocted dynamically were removed and replaced by stack 
variables.
ax88179_get_eeprom were modified from asix_get_eeprom in asix_common.

This patch adds a driver for ASIX's AX88179 family of USB 3.0/2.0
to gigabit ethernet adapters. It's based on the AX88xxx driver but
the usb commands used to access registers for AX88179 are completely different.
This driver had been verified on x86 system with AX88179/AX88178A and
Sitcomm LN-032 USB dongles.

Signed-off-by: Freddy Xin fre...@asix.com.tw

[...]

--- /dev/null
+++ b/drivers/net/usb/ax88179_178a.c

[...]

+struct ax88179_data {
+   u16 rxctl;
+};

Should also be declared __packed, as on some architectures it may be
padded to 4 bytes.

rxctl is presumably required to be little-endian (__le16)?


+struct ax88179_int_data {
+   u16 res1;
+   u8 link;
+   u16 res2;
+   u8 status;
+   u16 res3;
+} __packed;

Same here, the u16 fields are presumably little-endian?

[...]

+struct ax88179_rx_pkt_header {
+   u8  l4_csum_err:1,
+   l3_csum_err:1,
+   l4_type:3,
+   l3_type:2,
+   ce:1;
+
+   u8  vlan_ind:3,
+   rx_ok:1,
+   pri:3,
+   bmc:1;
+
+   u16 len:13,
+   crc:1,
+   mii:1,
+   drop:1;
+} __packed;

This won't work on both big-endian systems (assuming this works on x86).
You apparently try to convert the structure in-place in
ax88179_rx_fixup() by calling le32_to_cpus(); that may work if you
define all the bitfields to be part of a u32 but it won't work with the
current definition.

[...]

+static int
+ax88179_set_features(struct net_device *net, netdev_features_t features)
+{
+   u8 tmp;
+   struct usbnet *dev = netdev_priv(net);
+   netdev_features_t changed = net-features ^ features;
+
+   if (changed  NETIF_F_TSO)
+   net-features ^= NETIF_F_TSO;
+
+   if (changed  NETIF_F_SG)
+   net-features ^= NETIF_F_SG;

Don't change net-features; the caller will do that for you.


+   if (changed  NETIF_F_IP_CSUM) {
+   ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_TXCOE_CTL, 1, 1, tmp);
+   tmp ^= AX_TXCOE_TCP | AX_TXCOE_UDP;
+   ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_TXCOE_CTL, 1, 1, tmp);
+
+   net-features ^= NETIF_F_IP_CSUM;
+   }

[...]

Isn't tmp going to be in little-endian byte order, so this doesn't work
correctly on a big-endian system?

There are a lot of reads and writes of 16-bit registers using
ax88179_{read,write}_cmd(); maybe you should add
ax88179_{read,write}_le16() to handle this specific case.

Ben.


Thank you, Ben. I will fix bugs and test it on a big-endian system.

Freddy
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH, resubmit] ax88179_178a: ASIX AX88179_178A USB 3.0/2.0 to gigabit ethernet adapter driver

2013-02-06 Thread Freddy 

On 02/07/2013 01:46 PM, Stephen Hemminger wrote:

On Thu,  7 Feb 2013 12:36:34 +0800
Freddy Xin  wrote:


+static struct ethtool_ops ax88179_ethtool_ops = {
+   .get_link   = ethtool_op_get_link,
+   .get_msglevel   = usbnet_get_msglevel,
+   .set_msglevel   = usbnet_set_msglevel,
+   .get_wol= ax88179_get_wol,
+   .set_wol= ax88179_set_wol,
+   .get_eeprom_len = ax88179_get_eeprom_len,
+   .get_eeprom = ax88179_get_eeprom,
+   .get_settings   = ax88179_get_settings,
+   .set_settings   = ax88179_set_settings,
+   .nway_reset = usbnet_nway_reset,
+};
+

ethtool_ops should always be const


Thank you, Stephen. I will fix these errors.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH, resubmit] ax88179_178a: ASIX AX88179_178A USB 3.0/2.0 to gigabit ethernet adapter driver

2013-02-06 Thread Freddy 

On 02/07/2013 01:45 PM, Stephen Hemminger wrote:

On Thu,  7 Feb 2013 12:36:34 +0800
Freddy Xin  wrote:


+struct {unsigned char ctrl, timer_l, timer_h, size, ifg; }
+AX88179_BULKIN_SIZE[] ={
+   {7, 0x4f, 0,0x12, 0xff},
+   {7, 0xf0, 1,0x15, 0xff},
+   {7, 0xae, 7,0x18, 0xff},
+   {7, 0xcc, 0x4c, 0x18, 8},
+};

  Better to make it static, const, and add a couple
  of line breaks.

  static const struct {
   unsigned char ctrl, timer_l, timer_h, size, ifg;
  } AX88179_BULKIN_SIZE[] = {
{7, 0x4f, 0,0x12, 0xff},
{7, 0xf0, 1,0x15, 0xff},
{7, 0xae, 7,0x18, 0xff},
{7, 0xcc, 0x4c, 0x18, 8},
};

Thank you, Stephen. I will fix these errors.




--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH, resubmit] ax88179_178a: ASIX AX88179_178A USB 3.0/2.0 to gigabit ethernet adapter driver

2013-02-06 Thread Stephen Hemminger 
On Thu,  7 Feb 2013 12:36:34 +0800
Freddy Xin  wrote:

> +static struct ethtool_ops ax88179_ethtool_ops = {
> + .get_link   = ethtool_op_get_link,
> + .get_msglevel   = usbnet_get_msglevel,
> + .set_msglevel   = usbnet_set_msglevel,
> + .get_wol= ax88179_get_wol,
> + .set_wol= ax88179_set_wol,
> + .get_eeprom_len = ax88179_get_eeprom_len,
> + .get_eeprom = ax88179_get_eeprom,
> + .get_settings   = ax88179_get_settings,
> + .set_settings   = ax88179_set_settings,
> + .nway_reset = usbnet_nway_reset,
> +};
> +

ethtool_ops should always be const
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH, resubmit] ax88179_178a: ASIX AX88179_178A USB 3.0/2.0 to gigabit ethernet adapter driver

2013-02-06 Thread Stephen Hemminger 
On Thu,  7 Feb 2013 12:36:34 +0800
Freddy Xin  wrote:

> +struct {unsigned char ctrl, timer_l, timer_h, size, ifg; }
> +AX88179_BULKIN_SIZE[] =  {
> + {7, 0x4f, 0,0x12, 0xff},
> + {7, 0xf0, 1,0x15, 0xff},
> + {7, 0xae, 7,0x18, 0xff},
> + {7, 0xcc, 0x4c, 0x18, 8},
> +};

Better to make it static, const, and add a couple
of line breaks.

static const struct {
  unsigned char ctrl, timer_l, timer_h, size, ifg;
} AX88179_BULKIN_SIZE[] =   {
{7, 0x4f, 0,0x12, 0xff},
{7, 0xf0, 1,0x15, 0xff},
{7, 0xae, 7,0x18, 0xff},
{7, 0xcc, 0x4c, 0x18, 8},
};
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH, resubmit] ax88179_178a: ASIX AX88179_178A USB 3.0/2.0 to gigabit ethernet adapter driver

2013-02-06 Thread Stephen Hemminger 
On Thu,  7 Feb 2013 12:36:34 +0800
Freddy Xin fre...@asix.com.tw wrote:

 +struct {unsigned char ctrl, timer_l, timer_h, size, ifg; }
 +AX88179_BULKIN_SIZE[] =  {
 + {7, 0x4f, 0,0x12, 0xff},
 + {7, 0xf0, 1,0x15, 0xff},
 + {7, 0xae, 7,0x18, 0xff},
 + {7, 0xcc, 0x4c, 0x18, 8},
 +};

Better to make it static, const, and add a couple
of line breaks.

static const struct {
  unsigned char ctrl, timer_l, timer_h, size, ifg;
} AX88179_BULKIN_SIZE[] =   {
{7, 0x4f, 0,0x12, 0xff},
{7, 0xf0, 1,0x15, 0xff},
{7, 0xae, 7,0x18, 0xff},
{7, 0xcc, 0x4c, 0x18, 8},
};
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH, resubmit] ax88179_178a: ASIX AX88179_178A USB 3.0/2.0 to gigabit ethernet adapter driver

2013-02-06 Thread Stephen Hemminger 
On Thu,  7 Feb 2013 12:36:34 +0800
Freddy Xin fre...@asix.com.tw wrote:

 +static struct ethtool_ops ax88179_ethtool_ops = {
 + .get_link   = ethtool_op_get_link,
 + .get_msglevel   = usbnet_get_msglevel,
 + .set_msglevel   = usbnet_set_msglevel,
 + .get_wol= ax88179_get_wol,
 + .set_wol= ax88179_set_wol,
 + .get_eeprom_len = ax88179_get_eeprom_len,
 + .get_eeprom = ax88179_get_eeprom,
 + .get_settings   = ax88179_get_settings,
 + .set_settings   = ax88179_set_settings,
 + .nway_reset = usbnet_nway_reset,
 +};
 +

ethtool_ops should always be const
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH, resubmit] ax88179_178a: ASIX AX88179_178A USB 3.0/2.0 to gigabit ethernet adapter driver

2013-02-06 Thread Freddy 

On 02/07/2013 01:45 PM, Stephen Hemminger wrote:

On Thu,  7 Feb 2013 12:36:34 +0800
Freddy Xin fre...@asix.com.tw wrote:


+struct {unsigned char ctrl, timer_l, timer_h, size, ifg; }
+AX88179_BULKIN_SIZE[] ={
+   {7, 0x4f, 0,0x12, 0xff},
+   {7, 0xf0, 1,0x15, 0xff},
+   {7, 0xae, 7,0x18, 0xff},
+   {7, 0xcc, 0x4c, 0x18, 8},
+};

  Better to make it static, const, and add a couple
  of line breaks.

  static const struct {
   unsigned char ctrl, timer_l, timer_h, size, ifg;
  } AX88179_BULKIN_SIZE[] = {
{7, 0x4f, 0,0x12, 0xff},
{7, 0xf0, 1,0x15, 0xff},
{7, 0xae, 7,0x18, 0xff},
{7, 0xcc, 0x4c, 0x18, 8},
};

Thank you, Stephen. I will fix these errors.




--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH, resubmit] ax88179_178a: ASIX AX88179_178A USB 3.0/2.0 to gigabit ethernet adapter driver

2013-02-06 Thread Freddy 

On 02/07/2013 01:46 PM, Stephen Hemminger wrote:

On Thu,  7 Feb 2013 12:36:34 +0800
Freddy Xin fre...@asix.com.tw wrote:


+static struct ethtool_ops ax88179_ethtool_ops = {
+   .get_link   = ethtool_op_get_link,
+   .get_msglevel   = usbnet_get_msglevel,
+   .set_msglevel   = usbnet_set_msglevel,
+   .get_wol= ax88179_get_wol,
+   .set_wol= ax88179_set_wol,
+   .get_eeprom_len = ax88179_get_eeprom_len,
+   .get_eeprom = ax88179_get_eeprom,
+   .get_settings   = ax88179_get_settings,
+   .set_settings   = ax88179_set_settings,
+   .nway_reset = usbnet_nway_reset,
+};
+

ethtool_ops should always be const


Thank you, Stephen. I will fix these errors.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH, resubmit] ax88179_178a: ASIX AX88179_178A USB 3.0/2.0 to gigabit ethernet adapter driver

2013-02-04 Thread Freddy 


-Original Message-
From: Daniel J Blueman [mailto:dan...@quora.org] 
Sent: Sunday, February 03, 2013 10:22 PM
To: Freddy Xin
Cc: Linux Kernel
Subject: Re: [PATCH, resubmit] ax88179_178a: ASIX AX88179_178A USB 3.0/2.0
to gigabit ethernet adapter driver

> Hi Freddy,

> Michael, could you give me more information about how do you test this
driver?
> I have tried to reproduce the issue by using "ifconfig ethX mtu 1500", but
I didn't confront the same issue.
> Thank you in advance for your help.

> I found the same by just starting with 'ifconfig eth0 1500' and testing as
high as 4000; pinging another host with a large payload of size mtu-40
starts failing; after ~30s, I see the transmit time out trace [1].

> Of course, a default MTU size of 1500 is essential to avoid fragmentation
issues, so should be fixed too. Jumbo frames support is essential these days
too.

Hi Daniel,

The default MTU size will be change to 1500 and the HW jumbo frame function
will be enabled in the next submission of the driver.
Thanks for your help.

Freddy

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH, resubmit] ax88179_178a: ASIX AX88179_178A USB 3.0/2.0 to gigabit ethernet adapter driver

2013-02-04 Thread Freddy 


-Original Message-
From: Daniel J Blueman [mailto:dan...@quora.org] 
Sent: Sunday, February 03, 2013 10:22 PM
To: Freddy Xin
Cc: Linux Kernel
Subject: Re: [PATCH, resubmit] ax88179_178a: ASIX AX88179_178A USB 3.0/2.0
to gigabit ethernet adapter driver

 Hi Freddy,

 Michael, could you give me more information about how do you test this
driver?
 I have tried to reproduce the issue by using ifconfig ethX mtu 1500, but
I didn't confront the same issue.
 Thank you in advance for your help.

 I found the same by just starting with 'ifconfig eth0 1500' and testing as
high as 4000; pinging another host with a large payload of size mtu-40
starts failing; after ~30s, I see the transmit time out trace [1].

 Of course, a default MTU size of 1500 is essential to avoid fragmentation
issues, so should be fixed too. Jumbo frames support is essential these days
too.

Hi Daniel,

The default MTU size will be change to 1500 and the HW jumbo frame function
will be enabled in the next submission of the driver.
Thanks for your help.

Freddy

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH, resubmit] ax88179_178a: ASIX AX88179_178A USB 3.0/2.0 to gigabit ethernet adapter driver

2013-02-03 Thread Daniel J Blueman 
Hi Freddy,

> Michael, could you give me more information about how do you test this driver?
> I have tried to reproduce the issue by using "ifconfig ethX mtu 1500", but I 
> didn't confront the same issue.
> Thank you in advance for your help.

I found the same by just starting with 'ifconfig eth0 1500' and
testing as high as 4000; pinging another host with a large payload of
size mtu-40 starts failing; after ~30s, I see the transmit time out
trace [1].

Of course, a default MTU size of 1500 is essential to avoid
fragmentation issues, so should be fixed too. Jumbo frames support is
essential these days too.

Thanks,
  Daniel

--- [1]

usb 4-1: new SuperSpeed USB device number 3 using xhci_hcd
ax88179_178a 4-1:1.0 eth0: register 'ax88179_178a' at
usb-:00:14.0-1, ASIX AX88179 USB 3.0 Gigibit Ethernet,
00:0a:cd:21:46:a7
ax88179_178a 4-1:1.0 eth2: ax88179 - Link status is: 1
[ cut here ]
WARNING: at net/sched/sch_generic.c:254 dev_watchdog+0x26b/0x280()
Hardware name: MacBookPro10,1
NETDEV WATCHDOG: eth2 (ax88179_178a): transmit queue 0 timed out
Modules linked in: fuse snd_hda_codec_hdmi snd_hda_codec_cirrus joydev
hid_apple bcm5974 coretemp kvm_intel kvm ghash_clmulni_intel b43 ssb
ax88179_178a usbnet mii uvcvideo videobuf2_core videobuf2_vmalloc
videobuf2_memops applesmc input_polldev microcode bcma bnep rfcomm
lpc_ich mfd_core snd_hda_intel snd_hda_codec snd_hwdep snd_pcm nouveau
apple_gmux snd_timer i915 ttm drm_kms_helper snd hwmon binfmt_misc
mxm_wmi snd_page_alloc video apple_bl nls_iso8859_1
Pid: 0, comm: swapper/0 Not tainted 3.8.0-rc6-expert+ #2
Call Trace:
  [] ? dev_watchdog+0x250/0x280
 [] warn_slowpath_common+0x7a/0xb0
 [] warn_slowpath_fmt+0x41/0x50
 [] dev_watchdog+0x26b/0x280
 [] ? pfifo_fast_dequeue+0xe0/0xe0
 [] call_timer_fn+0x74/0xf0
 [] ? usleep_range+0x40/0x40
 [] ? pfifo_fast_dequeue+0xe0/0xe0
 [] run_timer_softirq+0x18b/0x220
 [] __do_softirq+0xc2/0x180
 [] ? tick_program_event+0x1f/0x30
 [] ? read_measured_perf_ctrs+0x70/0x70
 [] call_softirq+0x1c/0x30
 [] do_softirq+0x7d/0xb0
 [] irq_exit+0x9e/0xc0
 [] smp_apic_timer_interrupt+0x69/0xa0
 [] apic_timer_interrupt+0x6c/0x80
  [] ? get_next_timer_interrupt+0x1c4/0x290
 [] ? cpuidle_wrap_enter+0x50/0x90
 [] ? cpuidle_wrap_enter+0x4c/0x90
 [] cpuidle_enter_tk+0x10/0x20
 [] cpuidle_idle_call+0x7c/0x110
 [] cpu_idle+0x7a/0xf0
 [] rest_init+0x144/0x150
 [] ? csum_partial_copy_generic+0x170/0x170
 [] ? efi_free_boot_services+0x53/0x58
 [] start_kernel+0x359/0x366
 [] ? repair_env_string+0x5e/0x5e
 [] x86_64_start_reservations+0x131/0x135
 [] ? early_idt_handlers+0x120/0x120
 [] x86_64_start_kernel+0xd3/0xd7
---[ end trace 58c12634a365560a ]---
-- 
Daniel J Blueman
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH, resubmit] ax88179_178a: ASIX AX88179_178A USB 3.0/2.0 to gigabit ethernet adapter driver

2013-02-03 Thread Daniel J Blueman 
Hi Freddy,

 Michael, could you give me more information about how do you test this driver?
 I have tried to reproduce the issue by using ifconfig ethX mtu 1500, but I 
 didn't confront the same issue.
 Thank you in advance for your help.

I found the same by just starting with 'ifconfig eth0 1500' and
testing as high as 4000; pinging another host with a large payload of
size mtu-40 starts failing; after ~30s, I see the transmit time out
trace [1].

Of course, a default MTU size of 1500 is essential to avoid
fragmentation issues, so should be fixed too. Jumbo frames support is
essential these days too.

Thanks,
  Daniel

--- [1]

usb 4-1: new SuperSpeed USB device number 3 using xhci_hcd
ax88179_178a 4-1:1.0 eth0: register 'ax88179_178a' at
usb-:00:14.0-1, ASIX AX88179 USB 3.0 Gigibit Ethernet,
00:0a:cd:21:46:a7
ax88179_178a 4-1:1.0 eth2: ax88179 - Link status is: 1
[ cut here ]
WARNING: at net/sched/sch_generic.c:254 dev_watchdog+0x26b/0x280()
Hardware name: MacBookPro10,1
NETDEV WATCHDOG: eth2 (ax88179_178a): transmit queue 0 timed out
Modules linked in: fuse snd_hda_codec_hdmi snd_hda_codec_cirrus joydev
hid_apple bcm5974 coretemp kvm_intel kvm ghash_clmulni_intel b43 ssb
ax88179_178a usbnet mii uvcvideo videobuf2_core videobuf2_vmalloc
videobuf2_memops applesmc input_polldev microcode bcma bnep rfcomm
lpc_ich mfd_core snd_hda_intel snd_hda_codec snd_hwdep snd_pcm nouveau
apple_gmux snd_timer i915 ttm drm_kms_helper snd hwmon binfmt_misc
mxm_wmi snd_page_alloc video apple_bl nls_iso8859_1
Pid: 0, comm: swapper/0 Not tainted 3.8.0-rc6-expert+ #2
Call Trace:
 IRQ [81408d00] ? dev_watchdog+0x250/0x280
 [8103f8ea] warn_slowpath_common+0x7a/0xb0
 [8103f9c1] warn_slowpath_fmt+0x41/0x50
 [81408d1b] dev_watchdog+0x26b/0x280
 [81408ab0] ? pfifo_fast_dequeue+0xe0/0xe0
 [8104db84] call_timer_fn+0x74/0xf0
 [8104db10] ? usleep_range+0x40/0x40
 [81408ab0] ? pfifo_fast_dequeue+0xe0/0xe0
 [8104df0b] run_timer_softirq+0x18b/0x220
 [81047ad2] __do_softirq+0xc2/0x180
 [8108b3ff] ? tick_program_event+0x1f/0x30
 [813b18d0] ? read_measured_perf_ctrs+0x70/0x70
 [8154647c] call_softirq+0x1c/0x30
 [810042ed] do_softirq+0x7d/0xb0
 [81047d2e] irq_exit+0x9e/0xc0
 [81022779] smp_apic_timer_interrupt+0x69/0xa0
 [81545ddc] apic_timer_interrupt+0x6c/0x80
 EOI [8104e884] ? get_next_timer_interrupt+0x1c4/0x290
 [813b21a0] ? cpuidle_wrap_enter+0x50/0x90
 [813b219c] ? cpuidle_wrap_enter+0x4c/0x90
 [813b1900] cpuidle_enter_tk+0x10/0x20
 [813b1f3c] cpuidle_idle_call+0x7c/0x110
 [8100b7ba] cpu_idle+0x7a/0xf0
 [8152b954] rest_init+0x144/0x150
 [8152b810] ? csum_partial_copy_generic+0x170/0x170
 [81ab49e6] ? efi_free_boot_services+0x53/0x58
 [81aa3ad9] start_kernel+0x359/0x366
 [81aa3580] ? repair_env_string+0x5e/0x5e
 [81aa32a5] x86_64_start_reservations+0x131/0x135
 [81aa3120] ? early_idt_handlers+0x120/0x120
 [81aa337c] x86_64_start_kernel+0xd3/0xd7
---[ end trace 58c12634a365560a ]---
-- 
Daniel J Blueman
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH, resubmit] ax88179_178a: ASIX AX88179_178A USB 3.0/2.0 to gigabit ethernet adapter driver

2013-01-28 Thread Michael Leun 
On Mon, 28 Jan 2013 21:36:20 +0800
"Freddy"  wrote:

> > I would vote to not accept that driver for mainline as long as this 
> > issues are not fixed.
> 
> 
> Michael, could you give me more information about how do you test
> this driver? I have tried to reproduce the issue by using "ifconfig
> ethX mtu 1500", but I didn't confront the same issue. Thank you in
> advance for your help.

I'm very sorry, but this seems to be some mistake on my side. I would
have sworn that I had done this also with mtu 1500, but now it looks
like you have to set the mtu to at least 1519 to see the issue (likely
an typo and I actually had set it to 1600):

jill:/home/ml # ifconfig eth3 192.168.123.2/24
jill:/home/ml # ifconfig eth3
eth3  Link encap:Ethernet  Hardware Adresse 00:24:9B:05:5C:E3  
  inet Adresse:192.168.123.2  Bcast:192.168.123.255  Maske:255.255.255.0
  inet6 Adresse: fe80::224:9bff:fe05:5ce3/64 
Gültigkeitsbereich:Verbindung
  UP BROADCAST RUNNING MULTICAST  MTU:1488  Metric:1
  RX packets:0 errors:0 dropped:0 overruns:0 frame:0
  TX packets:1 errors:0 dropped:0 overruns:0 carrier:0
  collisions:0 Sendewarteschlangenlänge:1000 
  RX bytes:0 (0.0 b)  TX bytes:98 (98.0 b)

jill:/home/ml # ping 192.168.123.1
PING 192.168.123.1 (192.168.123.1) 56(84) bytes of data.
64 bytes from 192.168.123.1: icmp_seq=1 ttl=64 time=1.41 ms
64 bytes from 192.168.123.1: icmp_seq=2 ttl=64 time=0.969 ms
64 bytes from 192.168.123.1: icmp_seq=3 ttl=64 time=1.01 ms
^C
--- 192.168.123.1 ping statistics ---
3 packets transmitted, 3 received, 0% packet loss, time 2002ms
rtt min/avg/max/mdev = 0.969/1.131/1.412/0.199 ms
jill:/home/ml # ping -s 1491 192.168.123.1
PING 192.168.123.1 (192.168.123.1) 1491(1519) bytes of data.
1499 bytes from 192.168.123.1: icmp_seq=1 ttl=64 time=1.30 ms
1499 bytes from 192.168.123.1: icmp_seq=2 ttl=64 time=1.10 ms
1499 bytes from 192.168.123.1: icmp_seq=3 ttl=64 time=1.24 ms
^C
--- 192.168.123.1 ping statistics ---
3 packets transmitted, 3 received, 0% packet loss, time 2002ms
rtt min/avg/max/mdev = 1.109/1.218/1.306/0.086 ms
jill:/home/ml # ifconfig eth3 mtu 1519
jill:/home/ml # ping 192.168.123.1
PING 192.168.123.1 (192.168.123.1) 56(84) bytes of data.
64 bytes from 192.168.123.1: icmp_seq=1 ttl=64 time=1.11 ms
64 bytes from 192.168.123.1: icmp_seq=2 ttl=64 time=0.969 ms
^C
--- 192.168.123.1 ping statistics ---
2 packets transmitted, 2 received, 0% packet loss, time 1001ms
rtt min/avg/max/mdev = 0.969/1.042/1.115/0.073 ms
jill:/home/ml # ping -s 1491 192.168.123.1
PING 192.168.123.1 (192.168.123.1) 1491(1519) bytes of data.
^C
--- 192.168.123.1 ping statistics ---
2 packets transmitted, 0 received, 100% packet loss, time 1000ms

jill:/home/ml # ping 192.168.123.1
PING 192.168.123.1 (192.168.123.1) 56(84) bytes of data.
^C
--- 192.168.123.1 ping statistics ---
6 packets transmitted, 0 received, 100% packet loss, time 4999ms

jill:/home/ml # 

-- 
MfG,

Michael Leun

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH, resubmit] ax88179_178a: ASIX AX88179_178A USB 3.0/2.0 to gigabit ethernet adapter driver

2013-01-28 Thread Bjørn Mork 
"Freddy"  writes:

> Bjørn, I am trying to reproduce the issue mentioned by Michael and I
> have a question about submitting this driver.
>
> Should I merge this driver into asix_devices.c and asix_common.c even
> through the usb command, tx_fixup, and rx_fixup functions are totally
> different?

This is only my personal opinion and does not count as more than that,
but I would have tried to identify as many common parts as possible in
these drivers and reuse as much code as possible instead of creating
slightly different copies.  That does not mean that there shouldn't be
anything different.  If the framing is completely different, then it
does of course not make any sense to share tx_fixup and rx_fixup.  But
my impression from looking briefly on these drivers is that a lof of the
code is very similar. I could of course be wrong...

Please note that modifying asix_common.c in this process is perfectly OK
if that is necessary.  The only requirement is that you don't break
anything that used to work.


Bjørn
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH, resubmit] ax88179_178a: ASIX AX88179_178A USB 3.0/2.0 to gigabit ethernet adapter driver

2013-01-28 Thread Freddy 
> I would vote to not accept that driver for mainline as long as this 
> issues are not fixed.


Michael, could you give me more information about how do you test this driver? 
I have tried to reproduce the issue by using "ifconfig ethX mtu 1500", but I 
didn't confront the same issue.
Thank you in advance for your help.


> The vendor should not be able to claim "hooray, hooray, great device, 
> we even have an driver in linux main line" when it is actually such an 
> useless crap.

> Well, that is fortunately not how these things work. The main goal is getting 
> the devices supported in the kernel. Bugs can be fixed.  If a vendor can get 
> any positive gain out of having a driver in mainline, then that is good for 
> everyone, isn't it?  Of course, we can all agree that the > > > effect of a 
> *working* driver is more positive than a non-working driver...


> For now, the main focus should be fixing the issues which has been noted 
> during review.  Your testing feedback is of course very useful, but you 
> probably need to back them up with actual code change proposals if they are 
> going to be dealt with at this stage.

> Of course I'm offering to help with any information or testing, but 
> unfortunately I do not have the knowhow to fix anything myself.

> I believe this is where you are totally wrong.  You obviuously have the 
> ability to create a few simple test cases for yourself and see if the driver 
> behaves as you expect.  That is very useful.

> And you have a device.  That is also useful.

> Now, the driver source code is available.  And there is another Asix driver 
> in the kernel which already has been cleaned up and can be used as an 
> example. And maybe even partly used for the new devices as well, if the code 
> is duplicated?  I have not looked at this in detail, but I > suspect that 
> much of the problem with the ax88179_178a driver is that it has completely 
> ignored all the work that has gone into the asix driver after it was 
> mainlined.  I find it unlikely that there is no reusable code in the 
> asix_devices.c, asix_common.c and ax88172a.c files.  Trying to > rewrite 
> ax88179_178a to share as much code as possible seems like the best way to 
> clean it up and fix bugs.

Bjørn, I am trying to reproduce the issue mentioned by Michael and I have a 
question about submitting this driver.
Should I merge this driver into asix_devices.c and asix_common.c even through 
the usb command, tx_fixup, and rx_fixup functions are totally different?
Thank you in advance for your reply.

Freddy


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH, resubmit] ax88179_178a: ASIX AX88179_178A USB 3.0/2.0 to gigabit ethernet adapter driver

2013-01-28 Thread Bjørn Mork 
Hello Michael,

Michael Leun  writes:

> I would vote to not accept that driver for mainline as long as this
> issues are not fixed. 
>
> The vendor should not be able to claim "hooray, hooray, great device,
> we even have an driver in linux main line" when it is actually such an
> useless crap.

Well, that is fortunately not how these things work. The main goal is
getting the devices supported in the kernel. Bugs can be fixed.  If a
vendor can get any positive gain out of having a driver in mainline,
then that is good for everyone, isn't it?  Of course, we can all agree
that the effect of a *working* driver is more positive than a
non-working driver...

For now, the main focus should be fixing the issues which has been noted
during review.  Your testing feedback is of course very useful, but you
probably need to back them up with actual code change proposals if they
are going to be dealt with at this stage.

> Of course I'm offering to help with any information or testing, but
> unfortunately I do not have the knowhow to fix anything myself. 

I believe this is where you are totally wrong.  You obviuously have the
ability to create a few simple test cases for yourself and see if the
driver behaves as you expect.  That is very useful.

And you have a device.  That is also useful.

Now, the driver source code is available.  And there is another Asix
driver in the kernel which already has been cleaned up and can be used
as an example. And maybe even partly used for the new devices as well,
if the code is duplicated?  I have not looked at this in detail, but I
suspect that much of the problem with the ax88179_178a driver is that it
has completely ignored all the work that has gone into the asix driver
after it was mainlined.  I find it unlikely that there is no reusable
code in the asix_devices.c, asix_common.c and ax88172a.c files.  Trying
to rewrite ax88179_178a to share as much code as possible seems like the
best way to clean it up and fix bugs.

You have documents like
http://www.kernel.org/doc/Documentation/SubmittingPatches helping a lot
with cleanup jobs like this, because it is mostly about just following
that recipe.  If you do, then the code will be reviewable by the
community because the size will be reduced and the style readable.  And
the driver might even work, as the cleanup will quite possibly reveal a
number of bugs...

Please consider this option.  Reading and fixing up C code is not very
difficult. Getting this driver into the kernel in a good shape is mostly
just monkey work.  But some monkey has to do it :-)


Bjørn
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH, resubmit] ax88179_178a: ASIX AX88179_178A USB 3.0/2.0 to gigabit ethernet adapter driver

2013-01-28 Thread Bjørn Mork 
Hello Michael,

Michael Leun lkml20130...@newton.leun.net writes:

 I would vote to not accept that driver for mainline as long as this
 issues are not fixed. 

 The vendor should not be able to claim hooray, hooray, great device,
 we even have an driver in linux main line when it is actually such an
 useless crap.

Well, that is fortunately not how these things work. The main goal is
getting the devices supported in the kernel. Bugs can be fixed.  If a
vendor can get any positive gain out of having a driver in mainline,
then that is good for everyone, isn't it?  Of course, we can all agree
that the effect of a *working* driver is more positive than a
non-working driver...

For now, the main focus should be fixing the issues which has been noted
during review.  Your testing feedback is of course very useful, but you
probably need to back them up with actual code change proposals if they
are going to be dealt with at this stage.

 Of course I'm offering to help with any information or testing, but
 unfortunately I do not have the knowhow to fix anything myself. 

I believe this is where you are totally wrong.  You obviuously have the
ability to create a few simple test cases for yourself and see if the
driver behaves as you expect.  That is very useful.

And you have a device.  That is also useful.

Now, the driver source code is available.  And there is another Asix
driver in the kernel which already has been cleaned up and can be used
as an example. And maybe even partly used for the new devices as well,
if the code is duplicated?  I have not looked at this in detail, but I
suspect that much of the problem with the ax88179_178a driver is that it
has completely ignored all the work that has gone into the asix driver
after it was mainlined.  I find it unlikely that there is no reusable
code in the asix_devices.c, asix_common.c and ax88172a.c files.  Trying
to rewrite ax88179_178a to share as much code as possible seems like the
best way to clean it up and fix bugs.

You have documents like
http://www.kernel.org/doc/Documentation/SubmittingPatches helping a lot
with cleanup jobs like this, because it is mostly about just following
that recipe.  If you do, then the code will be reviewable by the
community because the size will be reduced and the style readable.  And
the driver might even work, as the cleanup will quite possibly reveal a
number of bugs...

Please consider this option.  Reading and fixing up C code is not very
difficult. Getting this driver into the kernel in a good shape is mostly
just monkey work.  But some monkey has to do it :-)


Bjørn
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH, resubmit] ax88179_178a: ASIX AX88179_178A USB 3.0/2.0 to gigabit ethernet adapter driver

2013-01-28 Thread Freddy 
 I would vote to not accept that driver for mainline as long as this 
 issues are not fixed.


Michael, could you give me more information about how do you test this driver? 
I have tried to reproduce the issue by using ifconfig ethX mtu 1500, but I 
didn't confront the same issue.
Thank you in advance for your help.


 The vendor should not be able to claim hooray, hooray, great device, 
 we even have an driver in linux main line when it is actually such an 
 useless crap.

 Well, that is fortunately not how these things work. The main goal is getting 
 the devices supported in the kernel. Bugs can be fixed.  If a vendor can get 
 any positive gain out of having a driver in mainline, then that is good for 
 everyone, isn't it?  Of course, we can all agree that theeffect of a 
 *working* driver is more positive than a non-working driver...


 For now, the main focus should be fixing the issues which has been noted 
 during review.  Your testing feedback is of course very useful, but you 
 probably need to back them up with actual code change proposals if they are 
 going to be dealt with at this stage.

 Of course I'm offering to help with any information or testing, but 
 unfortunately I do not have the knowhow to fix anything myself.

 I believe this is where you are totally wrong.  You obviuously have the 
 ability to create a few simple test cases for yourself and see if the driver 
 behaves as you expect.  That is very useful.

 And you have a device.  That is also useful.

 Now, the driver source code is available.  And there is another Asix driver 
 in the kernel which already has been cleaned up and can be used as an 
 example. And maybe even partly used for the new devices as well, if the code 
 is duplicated?  I have not looked at this in detail, but I  suspect that 
 much of the problem with the ax88179_178a driver is that it has completely 
 ignored all the work that has gone into the asix driver after it was 
 mainlined.  I find it unlikely that there is no reusable code in the 
 asix_devices.c, asix_common.c and ax88172a.c files.  Trying to  rewrite 
 ax88179_178a to share as much code as possible seems like the best way to 
 clean it up and fix bugs.

Bjørn, I am trying to reproduce the issue mentioned by Michael and I have a 
question about submitting this driver.
Should I merge this driver into asix_devices.c and asix_common.c even through 
the usb command, tx_fixup, and rx_fixup functions are totally different?
Thank you in advance for your reply.

Freddy


--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH, resubmit] ax88179_178a: ASIX AX88179_178A USB 3.0/2.0 to gigabit ethernet adapter driver

2013-01-28 Thread Bjørn Mork 
Freddy fre...@asix.com.tw writes:

 Bjørn, I am trying to reproduce the issue mentioned by Michael and I
 have a question about submitting this driver.

 Should I merge this driver into asix_devices.c and asix_common.c even
 through the usb command, tx_fixup, and rx_fixup functions are totally
 different?

This is only my personal opinion and does not count as more than that,
but I would have tried to identify as many common parts as possible in
these drivers and reuse as much code as possible instead of creating
slightly different copies.  That does not mean that there shouldn't be
anything different.  If the framing is completely different, then it
does of course not make any sense to share tx_fixup and rx_fixup.  But
my impression from looking briefly on these drivers is that a lof of the
code is very similar. I could of course be wrong...

Please note that modifying asix_common.c in this process is perfectly OK
if that is necessary.  The only requirement is that you don't break
anything that used to work.


Bjørn
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH, resubmit] ax88179_178a: ASIX AX88179_178A USB 3.0/2.0 to gigabit ethernet adapter driver

2013-01-28 Thread Michael Leun 
On Mon, 28 Jan 2013 21:36:20 +0800
Freddy fre...@asix.com.tw wrote:

  I would vote to not accept that driver for mainline as long as this 
  issues are not fixed.
 
 
 Michael, could you give me more information about how do you test
 this driver? I have tried to reproduce the issue by using ifconfig
 ethX mtu 1500, but I didn't confront the same issue. Thank you in
 advance for your help.

I'm very sorry, but this seems to be some mistake on my side. I would
have sworn that I had done this also with mtu 1500, but now it looks
like you have to set the mtu to at least 1519 to see the issue (likely
an typo and I actually had set it to 1600):

jill:/home/ml # ifconfig eth3 192.168.123.2/24
jill:/home/ml # ifconfig eth3
eth3  Link encap:Ethernet  Hardware Adresse 00:24:9B:05:5C:E3  
  inet Adresse:192.168.123.2  Bcast:192.168.123.255  Maske:255.255.255.0
  inet6 Adresse: fe80::224:9bff:fe05:5ce3/64 
Gültigkeitsbereich:Verbindung
  UP BROADCAST RUNNING MULTICAST  MTU:1488  Metric:1
  RX packets:0 errors:0 dropped:0 overruns:0 frame:0
  TX packets:1 errors:0 dropped:0 overruns:0 carrier:0
  collisions:0 Sendewarteschlangenlänge:1000 
  RX bytes:0 (0.0 b)  TX bytes:98 (98.0 b)

jill:/home/ml # ping 192.168.123.1
PING 192.168.123.1 (192.168.123.1) 56(84) bytes of data.
64 bytes from 192.168.123.1: icmp_seq=1 ttl=64 time=1.41 ms
64 bytes from 192.168.123.1: icmp_seq=2 ttl=64 time=0.969 ms
64 bytes from 192.168.123.1: icmp_seq=3 ttl=64 time=1.01 ms
^C
--- 192.168.123.1 ping statistics ---
3 packets transmitted, 3 received, 0% packet loss, time 2002ms
rtt min/avg/max/mdev = 0.969/1.131/1.412/0.199 ms
jill:/home/ml # ping -s 1491 192.168.123.1
PING 192.168.123.1 (192.168.123.1) 1491(1519) bytes of data.
1499 bytes from 192.168.123.1: icmp_seq=1 ttl=64 time=1.30 ms
1499 bytes from 192.168.123.1: icmp_seq=2 ttl=64 time=1.10 ms
1499 bytes from 192.168.123.1: icmp_seq=3 ttl=64 time=1.24 ms
^C
--- 192.168.123.1 ping statistics ---
3 packets transmitted, 3 received, 0% packet loss, time 2002ms
rtt min/avg/max/mdev = 1.109/1.218/1.306/0.086 ms
jill:/home/ml # ifconfig eth3 mtu 1519
jill:/home/ml # ping 192.168.123.1
PING 192.168.123.1 (192.168.123.1) 56(84) bytes of data.
64 bytes from 192.168.123.1: icmp_seq=1 ttl=64 time=1.11 ms
64 bytes from 192.168.123.1: icmp_seq=2 ttl=64 time=0.969 ms
^C
--- 192.168.123.1 ping statistics ---
2 packets transmitted, 2 received, 0% packet loss, time 1001ms
rtt min/avg/max/mdev = 0.969/1.042/1.115/0.073 ms
jill:/home/ml # ping -s 1491 192.168.123.1
PING 192.168.123.1 (192.168.123.1) 1491(1519) bytes of data.
^C
--- 192.168.123.1 ping statistics ---
2 packets transmitted, 0 received, 100% packet loss, time 1000ms

jill:/home/ml # ping 192.168.123.1
PING 192.168.123.1 (192.168.123.1) 56(84) bytes of data.
^C
--- 192.168.123.1 ping statistics ---
6 packets transmitted, 0 received, 100% packet loss, time 4999ms

jill:/home/ml # 

-- 
MfG,

Michael Leun

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH, resubmit] ax88179_178a: ASIX AX88179_178A USB 3.0/2.0 to gigabit ethernet adapter driver

2013-01-26 Thread Michael Leun 
On Wed, 23 Jan 2013 10:32:17 +0800
Freddy Xin  wrote:

> This is a resubmission.
> Fixed coding style errors.
[...]
> This patch adds a driver for ASIX's AX88179 family of USB 3.0/2.0
> to gigabit ethernet adapters. It's based on the AX88xxx driver but

I hope, there is some error/mistake on my side, otherwise I fear I
would have to say I find this driver to be total crap.

I tried it with an Digitus DN-3023 identifying itself as idVendor=0b95,
idProduct=1790 (some internet source claims it to be actually based on
asix reference design).

If I plug the device in the interface comes up with an mtu of 1488 -
this would be an very bad joke for any ethernet compliant device, even
more for an gigabit device where you clearly want to see at least a bit
of jumbo frames.

If I set the mtu to 1500 (the driver does not even complain when I set
the mtu to e.g. 2^31-1) and start to send bigger packets in some point
of time the device stops to work - means, after that you cannot even
send smaller packets, after some time you might get an kernel warning
"NETDEV WATCHDOG: eth3 (ax88179_178a): transmit queue 0 timed out" and
you have to unplug / replug the device to get it working again.

Note: With the W$ driver and exactly the same hardware you get at least
jumbo frames up to some 4000+X bytes - not great, but better than
nothing.

I would vote to not accept that driver for mainline as long as this
issues are not fixed. 

The vendor should not be able to claim "hooray, hooray, great device,
we even have an driver in linux main line" when it is actually such an
useless crap.

Of course I'm offering to help with any information or testing, but
unfortunately I do not have the knowhow to fix anything myself. 

-- 
MfG,

Michael Leun

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH, resubmit] ax88179_178a: ASIX AX88179_178A USB 3.0/2.0 to gigabit ethernet adapter driver

2013-01-26 Thread Michael Leun 
On Wed, 23 Jan 2013 10:32:17 +0800
Freddy Xin fre...@asix.com.tw wrote:

 This is a resubmission.
 Fixed coding style errors.
[...]
 This patch adds a driver for ASIX's AX88179 family of USB 3.0/2.0
 to gigabit ethernet adapters. It's based on the AX88xxx driver but

I hope, there is some error/mistake on my side, otherwise I fear I
would have to say I find this driver to be total crap.

I tried it with an Digitus DN-3023 identifying itself as idVendor=0b95,
idProduct=1790 (some internet source claims it to be actually based on
asix reference design).

If I plug the device in the interface comes up with an mtu of 1488 -
this would be an very bad joke for any ethernet compliant device, even
more for an gigabit device where you clearly want to see at least a bit
of jumbo frames.

If I set the mtu to 1500 (the driver does not even complain when I set
the mtu to e.g. 2^31-1) and start to send bigger packets in some point
of time the device stops to work - means, after that you cannot even
send smaller packets, after some time you might get an kernel warning
NETDEV WATCHDOG: eth3 (ax88179_178a): transmit queue 0 timed out and
you have to unplug / replug the device to get it working again.

Note: With the W$ driver and exactly the same hardware you get at least
jumbo frames up to some 4000+X bytes - not great, but better than
nothing.

I would vote to not accept that driver for mainline as long as this
issues are not fixed. 

The vendor should not be able to claim hooray, hooray, great device,
we even have an driver in linux main line when it is actually such an
useless crap.

Of course I'm offering to help with any information or testing, but
unfortunately I do not have the knowhow to fix anything myself. 

-- 
MfG,

Michael Leun

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH, resubmit] ax88179_178a: ASIX AX88179_178A USB 3.0/2.0 to gigabit ethernet adapter driver

2013-01-22 Thread David Miller 
From: Freddy Xin 
Date: Wed, 23 Jan 2013 10:32:17 +0800

> Fixed coding style errors.

There are still many style issues remaining, you have a lot more
work to do, for example:

> + return fn(dev, cmd, USB_DIR_OUT | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
> +value, index, data, size);
> +

This empty line is unnecessary, get rid of it.

Also this function call is not indented properly.  The first character
on the second line of the fn() call should line up with the first
column after the openning parenthesis of the first line of the call.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH, resubmit] ax88179_178a: ASIX AX88179_178A USB 3.0/2.0 to gigabit ethernet adapter driver

2013-01-22 Thread David Miller 
From: Freddy Xin fre...@asix.com.tw
Date: Wed, 23 Jan 2013 10:32:17 +0800

 Fixed coding style errors.

There are still many style issues remaining, you have a lot more
work to do, for example:

 + return fn(dev, cmd, USB_DIR_OUT | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
 +value, index, data, size);
 +

This empty line is unnecessary, get rid of it.

Also this function call is not indented properly.  The first character
on the second line of the fn() call should line up with the first
column after the openning parenthesis of the first line of the call.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH, resubmit] ax88179_178a: ASIX AX88179_178A USB 3.0/2.0 to gigabit ethernet adapter driver

2013-01-18 Thread Alan Stern 
On Fri, 18 Jan 2013, David Miller wrote:

> > +   ret = fn(dev, cmd, USB_DIR_IN | USB_TYPE_VENDOR |
> > +   USB_RECIP_DEVICE, value, index, data, size);
> 
> This is not indented properly.  When a function call takes up
> multiple lines, the text on the second and subsequent lines must
> be left justified to the first column after the openning parenthesis
> of the function call, like this:
> 
>   function(arg1, arg2,
>arg3, arg4);
> 
> You must use the appropriate combination of TAB and space characters
> to achieve this.  If you are trying to only use TAB characters, you
> are doing it wrong.

Documentation/CodingStyle doesn't mention this.  Does the networking 
stack have its own special requirements, not listed in CodingStyle?

Alan Stern

P.S: The standard I have tried to follow for USB code is to indent
continuation lines by two tab stops more than the first line,
regardless of whether the break occurs inside a function call.  I have
seen other people consistently indent continuation lines by 1/2 tab
stop (i.e., four spaces) beyond the first line.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH, resubmit] ax88179_178a: ASIX AX88179_178A USB 3.0/2.0 to gigabit ethernet adapter driver

2013-01-18 Thread David Miller 
From: Freddy Xin 
Date: Thu, 17 Jan 2013 17:32:54 +0800

> +struct ax88179_rx_pkt_header {
> +
> + u8  l4_csum_err:1,

Get rid of such extraneous empty lines.  They do not add clarity,
rather they just take up space.

> + ret = fn(dev, cmd, USB_DIR_IN | USB_TYPE_VENDOR |
> + USB_RECIP_DEVICE, value, index, data, size);

This is not indented properly.  When a function call takes up
multiple lines, the text on the second and subsequent lines must
be left justified to the first column after the openning parenthesis
of the function call, like this:

function(arg1, arg2,
 arg3, arg4);

You must use the appropriate combination of TAB and space characters
to achieve this.  If you are trying to only use TAB characters, you
are doing it wrong.

This code has a lot of other similar coding style errors, please
put some effort into fixing them up before you consider resubmitting
this driver.

All of the coding style errors are probably why nobody reviewed your
driver the first time around, there's already enough properly styled
submissions to review.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH, resubmit] ax88179_178a: ASIX AX88179_178A USB 3.0/2.0 to gigabit ethernet adapter driver

2013-01-18 Thread David Miller 
From: Freddy Xin fre...@asix.com.tw
Date: Thu, 17 Jan 2013 17:32:54 +0800

 +struct ax88179_rx_pkt_header {
 +
 + u8  l4_csum_err:1,

Get rid of such extraneous empty lines.  They do not add clarity,
rather they just take up space.

 + ret = fn(dev, cmd, USB_DIR_IN | USB_TYPE_VENDOR |
 + USB_RECIP_DEVICE, value, index, data, size);

This is not indented properly.  When a function call takes up
multiple lines, the text on the second and subsequent lines must
be left justified to the first column after the openning parenthesis
of the function call, like this:

function(arg1, arg2,
 arg3, arg4);

You must use the appropriate combination of TAB and space characters
to achieve this.  If you are trying to only use TAB characters, you
are doing it wrong.

This code has a lot of other similar coding style errors, please
put some effort into fixing them up before you consider resubmitting
this driver.

All of the coding style errors are probably why nobody reviewed your
driver the first time around, there's already enough properly styled
submissions to review.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH, resubmit] ax88179_178a: ASIX AX88179_178A USB 3.0/2.0 to gigabit ethernet adapter driver

2013-01-18 Thread Alan Stern 
On Fri, 18 Jan 2013, David Miller wrote:

  +   ret = fn(dev, cmd, USB_DIR_IN | USB_TYPE_VENDOR |
  +   USB_RECIP_DEVICE, value, index, data, size);
 
 This is not indented properly.  When a function call takes up
 multiple lines, the text on the second and subsequent lines must
 be left justified to the first column after the openning parenthesis
 of the function call, like this:
 
   function(arg1, arg2,
arg3, arg4);
 
 You must use the appropriate combination of TAB and space characters
 to achieve this.  If you are trying to only use TAB characters, you
 are doing it wrong.

Documentation/CodingStyle doesn't mention this.  Does the networking 
stack have its own special requirements, not listed in CodingStyle?

Alan Stern

P.S: The standard I have tried to follow for USB code is to indent
continuation lines by two tab stops more than the first line,
regardless of whether the break occurs inside a function call.  I have
seen other people consistently indent continuation lines by 1/2 tab
stop (i.e., four spaces) beyond the first line.

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/