Re: [PATCH stable v4.4+] r8152: add Linksys USB3GIGV1 id

[Grant Grundler]

On Thu, Apr 26, 2018 at 12:56 AM, Krzysztof Kozlowski <k...@kernel.org> wrote:
> On Thu, Apr 26, 2018 at 2:40 AM, Grant Grundler <grund...@chromium.org> wrote:
>> On Wed, Apr 25, 2018 at 2:54 AM, Krzysztof Kozlowski <k...@kernel.org>
>> wrote:
>>>
>>> commit 90841047a01b452cc8c3f9b990698b264143334a upstream
>>>
>>> This linksys dongle by default comes up in cdc_ether mode.
>>> This patch allows r8152 to claim the device:
>>>Bus 002 Device 002: ID 13b1:0041 Linksys
>>>
>>> Signed-off-by: Grant Grundler <grund...@chromium.org>
>>> Reviewed-by: Douglas Anderson <diand...@chromium.org>
>>> Signed-off-by: David S. Miller <da...@davemloft.net>
>>> [krzk: Rebase on v4.4]'
>>
>>
>> thanks krzk!
>>
>> FTR, to support RTL8153B (HW ID 0x6010), the follow patch series to bring
>> r8152 v1.09.9 driver from 4.14 kernel.org to 3 (of 5) older Chrome OS
>> kernels:
>>
>> 3.14:
>> https://chromium-review.googlesource.com/q/topic:%22update_r8152-3.14%22+(status:open%20OR%20status:merged)
>> 3.18:
>> https://chromium-review.googlesource.com/q/topic:%2522update-r8152-3.18%2522+(status:open+OR+status:merged)
>> 4.4:
>> https://chromium-review.googlesource.com/q/topic:%2522update_r8152-4.4%2522+(status:open+OR+status:merged)
>>
>> caveat: These series are not suitable directly for kernel.org submission
>> (extraneous stuff in the commit messages, order is different). Using the
>> original SHA1 (in each commit message), this can all be fixed up by
>> hand/simple scripts.
>
> Hi Grant,
>
> These are regular feature/patch backports so they do not fit into
> stable process. Only new quirks and IDs are accepted for stable.

Hi Krzysztof!
Sorry, I wasn't advocating for -stable inclusion. I shared in case
someone has unusually high USB ethernet requirements similar to Chrome
OS test lab which nearly all dongles I've tested can't provide.

Chrome OS test lab needs a USB ethernet dongle that reliably
negotiates a link (e.g. 10k iterations in a row). RTL8153 in general
are good (> 99.99% gets GigE link speed) but RTL8153B is the first
dongle that meets Chrome OS test lab requirements. The patch series
above is required to support RTL8153B.

cheers,
grant

Re: [PATCH stable v4.4+] r8152: add Linksys USB3GIGV1 id

[Grant Grundler]

On Wed, Apr 25, 2018 at 2:54 AM, Krzysztof Kozlowski <k...@kernel.org> wrote:
> commit 90841047a01b452cc8c3f9b990698b264143334a upstream
>
> This linksys dongle by default comes up in cdc_ether mode.
> This patch allows r8152 to claim the device:
>Bus 002 Device 002: ID 13b1:0041 Linksys
>
> Signed-off-by: Grant Grundler <grund...@chromium.org>
> Reviewed-by: Douglas Anderson <diand...@chromium.org>
> Signed-off-by: David S. Miller <da...@davemloft.net>
> [krzk: Rebase on v4.4]
> Signed-off-by: Krzysztof Kozlowski <k...@kernel.org>

thanks krzk!

FTR, to support RTL8153B (HW ID 0x6010), the follow patch series to
bring r8152 v1.09.9 driver from 4.14 kernel.org to 3 (of 5) older
Chrome OS kernels:

3.14: 
https://chromium-review.googlesource.com/q/topic:%22update_r8152-3.14%22+(status:open%20OR%20status:merged)
3.18: 
https://chromium-review.googlesource.com/q/topic:%2522update-r8152-3.18%2522+(status:open+OR+status:merged)
4.4: 
https://chromium-review.googlesource.com/q/topic:%2522update_r8152-4.4%2522+(status:open+OR+status:merged)

caveat: These series are not suitable directly for kernel.org
submission (extraneous stuff in the commit messages, order is
different). Using the original SHA1 (in each commit message), this can
all be fixed up by hand/simple scripts.

cheers,
grant


> ---
>  drivers/net/usb/cdc_ether.c | 10 ++
>  drivers/net/usb/r8152.c |  2 ++
>  2 files changed, 12 insertions(+)
>
> diff --git a/drivers/net/usb/cdc_ether.c b/drivers/net/usb/cdc_ether.c
> index 6578127db847..f71abe50ea6f 100644
> --- a/drivers/net/usb/cdc_ether.c
> +++ b/drivers/net/usb/cdc_ether.c
> @@ -461,6 +461,7 @@ static const struct driver_info wwan_info = {
>  #define REALTEK_VENDOR_ID  0x0bda
>  #define SAMSUNG_VENDOR_ID  0x04e8
>  #define LENOVO_VENDOR_ID   0x17ef
> +#define LINKSYS_VENDOR_ID  0x13b1
>  #define NVIDIA_VENDOR_ID   0x0955
>  #define HP_VENDOR_ID   0x03f0
>
> @@ -650,6 +651,15 @@ static const struct usb_device_id  products[] = {
> .driver_info = 0,
>  },
>
> +#if IS_ENABLED(CONFIG_USB_RTL8152)
> +/* Linksys USB3GIGV1 Ethernet Adapter */
> +{
> +   USB_DEVICE_AND_INTERFACE_INFO(LINKSYS_VENDOR_ID, 0x0041, 
> USB_CLASS_COMM,
> +   USB_CDC_SUBCLASS_ETHERNET, USB_CDC_PROTO_NONE),
> +   .driver_info = 0,
> +},
> +#endif
> +
>  /* Lenovo Thinkpad USB 3.0 Ethernet Adapters (based on Realtek RTL8153) */
>  {
> USB_DEVICE_AND_INTERFACE_INFO(LENOVO_VENDOR_ID, 0x7205, 
> USB_CLASS_COMM,
> diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
> index 89950f5cea71..b2c1a435357f 100644
> --- a/drivers/net/usb/r8152.c
> +++ b/drivers/net/usb/r8152.c
> @@ -506,6 +506,7 @@ enum rtl8152_flags {
>  #define VENDOR_ID_REALTEK  0x0bda
>  #define VENDOR_ID_SAMSUNG  0x04e8
>  #define VENDOR_ID_LENOVO   0x17ef
> +#define VENDOR_ID_LINKSYS  0x13b1
>  #define VENDOR_ID_NVIDIA   0x0955
>
>  #define MCU_TYPE_PLA   0x0100
> @@ -4376,6 +4377,7 @@ static struct usb_device_id rtl8152_table[] = {
> {REALTEK_USB_DEVICE(VENDOR_ID_SAMSUNG, 0xa101)},
> {REALTEK_USB_DEVICE(VENDOR_ID_LENOVO,  0x7205)},
> {REALTEK_USB_DEVICE(VENDOR_ID_LENOVO,  0x304f)},
> +   {REALTEK_USB_DEVICE(VENDOR_ID_LINKSYS, 0x0041)},
> {REALTEK_USB_DEVICE(VENDOR_ID_NVIDIA,  0x09ff)},
> {}
>  };
> --
> 2.7.4
>

Re: [PATCH V4] r8152: add Linksys USB3GIGV1 id

[Grant Grundler]

On Sun, Oct 1, 2017 at 10:39 PM, David Miller <da...@davemloft.net> wrote:
> From: Grant Grundler <grund...@chromium.org>
> Date: Thu, 28 Sep 2017 11:35:00 -0700
>
>> This linksys dongle by default comes up in cdc_ether mode.
>> This patch allows r8152 to claim the device:
>>Bus 002 Device 002: ID 13b1:0041 Linksys
>>
>> Signed-off-by: Grant Grundler <grund...@chromium.org>
>
> Applied, thanks.

thanks David, Doug, Oliver! :)

cheers,
grant

[PATCH V4] r8152: add Linksys USB3GIGV1 id

[Grant Grundler]

This linksys dongle by default comes up in cdc_ether mode.
This patch allows r8152 to claim the device:
   Bus 002 Device 002: ID 13b1:0041 Linksys

Signed-off-by: Grant Grundler <grund...@chromium.org>
---
 drivers/net/usb/cdc_ether.c | 10 ++
 drivers/net/usb/r8152.c |  2 ++
 2 files changed, 12 insertions(+)

V4: use IS_ENABLED() to check CONFIG_USB_RTL8152 is m or y.
(verified by adding #error to the new code and trying to compile
 Thanks Doug for the tip!)
Add LINKSYS vendor #define in same order for both drivers.

V3: for backwards compat, add #ifdef CONFIG_USB_RTL8152 around
the cdc_ether blacklist entry so the cdc_ether driver can
still claim the device if r8152 driver isn't configured.

V2: add LINKSYS_VENDOR_ID to cdc_ether blacklist



diff --git a/drivers/net/usb/cdc_ether.c b/drivers/net/usb/cdc_ether.c
index 8ab281b478f2..677a85360db1 100644
--- a/drivers/net/usb/cdc_ether.c
+++ b/drivers/net/usb/cdc_ether.c
@@ -547,6 +547,7 @@ static const struct driver_info wwan_info = {
 #define REALTEK_VENDOR_ID  0x0bda
 #define SAMSUNG_VENDOR_ID  0x04e8
 #define LENOVO_VENDOR_ID   0x17ef
+#define LINKSYS_VENDOR_ID  0x13b1
 #define NVIDIA_VENDOR_ID   0x0955
 #define HP_VENDOR_ID   0x03f0
 #define MICROSOFT_VENDOR_ID0x045e
@@ -737,6 +738,15 @@ static const struct usb_device_id  products[] = {
.driver_info = 0,
 },
 
+#if IS_ENABLED(CONFIG_USB_RTL8152)
+/* Linksys USB3GIGV1 Ethernet Adapter */
+{
+   USB_DEVICE_AND_INTERFACE_INFO(LINKSYS_VENDOR_ID, 0x0041, USB_CLASS_COMM,
+   USB_CDC_SUBCLASS_ETHERNET, USB_CDC_PROTO_NONE),
+   .driver_info = 0,
+},
+#endif
+
 /* ThinkPad USB-C Dock (based on Realtek RTL8153) */
 {
USB_DEVICE_AND_INTERFACE_INFO(LENOVO_VENDOR_ID, 0x3062, USB_CLASS_COMM,
diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index ceb78e2ea4f0..941ece08ba78 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -613,6 +613,7 @@ enum rtl8152_flags {
 #define VENDOR_ID_MICROSOFT0x045e
 #define VENDOR_ID_SAMSUNG  0x04e8
 #define VENDOR_ID_LENOVO   0x17ef
+#define VENDOR_ID_LINKSYS  0x13b1
 #define VENDOR_ID_NVIDIA   0x0955
 
 #define MCU_TYPE_PLA   0x0100
@@ -5316,6 +5317,7 @@ static const struct usb_device_id rtl8152_table[] = {
{REALTEK_USB_DEVICE(VENDOR_ID_LENOVO,  0x7205)},
{REALTEK_USB_DEVICE(VENDOR_ID_LENOVO,  0x720c)},
{REALTEK_USB_DEVICE(VENDOR_ID_LENOVO,  0x7214)},
+   {REALTEK_USB_DEVICE(VENDOR_ID_LINKSYS, 0x0041)},
{REALTEK_USB_DEVICE(VENDOR_ID_NVIDIA,  0x09ff)},
{}
 };
-- 
2.14.2.822.g60be5d43e6-goog

Re: [PATCH V3] r8152: add Linksys USB3GIGV1 id

[Grant Grundler]

Hi Doug!

On Wed, Sep 27, 2017 at 4:47 PM, Doug Anderson <diand...@chromium.org> wrote:
> Hi,
>
> On Wed, Sep 27, 2017 at 10:28 AM, Grant Grundler <grund...@chromium.org> 
> wrote:
>> This linksys dongle by default comes up in cdc_ether mode.
>> This patch allows r8152 to claim the device:
>>Bus 002 Device 002: ID 13b1:0041 Linksys
>>
>> Signed-off-by: Grant Grundler <grund...@chromium.org>
>> ---
>>  drivers/net/usb/cdc_ether.c | 10 ++
>>  drivers/net/usb/r8152.c |  2 ++
>>  2 files changed, 12 insertions(+)
>>
>> V3: for backwards compat, add #ifdef CONFIG_USB_RTL8152 around
>> the cdc_ether blacklist entry so the cdc_ether driver can
>> still claim the device if r8152 driver isn't configured.
>>
>> V2: add LINKSYS_VENDOR_ID to cdc_ether blacklist
>>
>> diff --git a/drivers/net/usb/cdc_ether.c b/drivers/net/usb/cdc_ether.c
>> index 8ab281b478f2..446dcc0f1f70 100644
>> --- a/drivers/net/usb/cdc_ether.c
>> +++ b/drivers/net/usb/cdc_ether.c
>> @@ -546,6 +546,7 @@ static const struct driver_info wwan_info = {
>>  #define DELL_VENDOR_ID 0x413C
>>  #define REALTEK_VENDOR_ID  0x0bda
>>  #define SAMSUNG_VENDOR_ID  0x04e8
>> +#define LINKSYS_VENDOR_ID  0x13b1
>>  #define LENOVO_VENDOR_ID   0x17ef
>
> Slight nit that "LI" sorts after "LE".  You got it right in the other case...

The list isn't sorted by any rational thing I can see.  I managed to
check my OCD reaction to sort the list numerically. :)

>>  #define NVIDIA_VENDOR_ID   0x0955
>>  #define HP_VENDOR_ID   0x03f0
>> @@ -737,6 +738,15 @@ static const struct usb_device_id  products[] = {
>> .driver_info = 0,
>>  },
>>
>> +#ifdef CONFIG_USB_RTL8152
>> +/* Linksys USB3GIGV1 Ethernet Adapter */
>> +{
>> +   USB_DEVICE_AND_INTERFACE_INFO(LINKSYS_VENDOR_ID, 0x0041, 
>> USB_CLASS_COMM,
>> +   USB_CDC_SUBCLASS_ETHERNET, USB_CDC_PROTO_NONE),
>> +   .driver_info = 0,
>> +},
>> +#endif
>
> I believe you want to use IS_ENABLED(), don't you?

Ah yes - I wasn't aware IS_ENABLED existed.  Will respin V4 with this
if there isn't any other feedback.


> There's still a weird esoteric side case where kernel modules don't
> all need to be included in the filesystem just because they were built
> at the same time.  ...but IMHO that seems like enough of a nit that we
> can probably ignore it unless someone has a better idea.

I think that would require a run time check. I'm perfectly willing to
ignore that case. :)

thanks!
grant

>
>
> -Doug

[PATCH V3] r8152: add Linksys USB3GIGV1 id

[Grant Grundler]

This linksys dongle by default comes up in cdc_ether mode.
This patch allows r8152 to claim the device:
   Bus 002 Device 002: ID 13b1:0041 Linksys

Signed-off-by: Grant Grundler <grund...@chromium.org>
---
 drivers/net/usb/cdc_ether.c | 10 ++
 drivers/net/usb/r8152.c |  2 ++
 2 files changed, 12 insertions(+)

V3: for backwards compat, add #ifdef CONFIG_USB_RTL8152 around
the cdc_ether blacklist entry so the cdc_ether driver can
still claim the device if r8152 driver isn't configured.

V2: add LINKSYS_VENDOR_ID to cdc_ether blacklist

diff --git a/drivers/net/usb/cdc_ether.c b/drivers/net/usb/cdc_ether.c
index 8ab281b478f2..446dcc0f1f70 100644
--- a/drivers/net/usb/cdc_ether.c
+++ b/drivers/net/usb/cdc_ether.c
@@ -546,6 +546,7 @@ static const struct driver_info wwan_info = {
 #define DELL_VENDOR_ID 0x413C
 #define REALTEK_VENDOR_ID  0x0bda
 #define SAMSUNG_VENDOR_ID  0x04e8
+#define LINKSYS_VENDOR_ID  0x13b1
 #define LENOVO_VENDOR_ID   0x17ef
 #define NVIDIA_VENDOR_ID   0x0955
 #define HP_VENDOR_ID   0x03f0
@@ -737,6 +738,15 @@ static const struct usb_device_id  products[] = {
.driver_info = 0,
 },
 
+#ifdef CONFIG_USB_RTL8152
+/* Linksys USB3GIGV1 Ethernet Adapter */
+{
+   USB_DEVICE_AND_INTERFACE_INFO(LINKSYS_VENDOR_ID, 0x0041, USB_CLASS_COMM,
+   USB_CDC_SUBCLASS_ETHERNET, USB_CDC_PROTO_NONE),
+   .driver_info = 0,
+},
+#endif
+
 /* ThinkPad USB-C Dock (based on Realtek RTL8153) */
 {
USB_DEVICE_AND_INTERFACE_INFO(LENOVO_VENDOR_ID, 0x3062, USB_CLASS_COMM,
diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index ceb78e2ea4f0..941ece08ba78 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -613,6 +613,7 @@ enum rtl8152_flags {
 #define VENDOR_ID_MICROSOFT0x045e
 #define VENDOR_ID_SAMSUNG  0x04e8
 #define VENDOR_ID_LENOVO   0x17ef
+#define VENDOR_ID_LINKSYS  0x13b1
 #define VENDOR_ID_NVIDIA   0x0955
 
 #define MCU_TYPE_PLA   0x0100
@@ -5316,6 +5317,7 @@ static const struct usb_device_id rtl8152_table[] = {
{REALTEK_USB_DEVICE(VENDOR_ID_LENOVO,  0x7205)},
{REALTEK_USB_DEVICE(VENDOR_ID_LENOVO,  0x720c)},
{REALTEK_USB_DEVICE(VENDOR_ID_LENOVO,  0x7214)},
+   {REALTEK_USB_DEVICE(VENDOR_ID_LINKSYS, 0x0041)},
{REALTEK_USB_DEVICE(VENDOR_ID_NVIDIA,  0x09ff)},
{}
 };
-- 
2.14.2.822.g60be5d43e6-goog

Re: [PATCH V2] r8152: add Linksys USB3GIGV1 id

[Grant Grundler]

On Wed, Sep 27, 2017 at 12:15 AM, Oliver Neukum  wrote:
> Am Dienstag, den 26.09.2017, 08:19 -0700 schrieb Doug Anderson:
>>
>> I know that for at least some of the adapters in the CDC Ethernet
>> blacklist it was claimed that the CDC Ethernet support in the adapter
>> was kinda broken anyway so the blacklist made sense.  ...but for the
>> Linksys Gigabit adapter the CDC Ethernet driver seems to work OK, it's
>> just not quite as full featured / efficient as the R8152 driver.
>>
>> Is that not a concern?  I guess you could tell people in this
>> situation that they simply need to enable the R8152 driver to get
>> continued support for their Ethernet adapter?
>
> Hi,
>
> yes, it is a valid concern. An #ifdef will be needed.

Good idea - I will post V3 shortly.

I'm assuming you mean to add #ifdef CONFIG_USB_RTL8152 around the
blacklist entry in cdc_ether driver.

cheers,
grant

Re: [PATCH] r8152: add Linksys USB3GIGV1 id

[Grant Grundler]

On Mon, Sep 25, 2017 at 1:17 PM, Grant Grundler <grund...@chromium.org> wrote:
...
> I didn't realize cdc_ether has a blacklist to make sure
> RTL8152|RTL8153 devices are not picked up by cdc_ether. Would you
> prefer I add this device to the blacklist in the same patch?

I've sent a V2 which also updates the blacklist in cdc_ether.

cheers,
grant

[PATCH V2] r8152: add Linksys USB3GIGV1 id

[Grant Grundler]

This linksys dongle by default comes up in cdc_ether mode.
This patch allows r8152 to claim the device:
   Bus 002 Device 002: ID 13b1:0041 Linksys

Signed-off-by: Grant Grundler <grund...@chromium.org>
---
 drivers/net/usb/cdc_ether.c | 8 
 drivers/net/usb/r8152.c | 2 ++
 2 files changed, 10 insertions(+)

V2: add LINKSYS_VENDOR_ID to cdc_ether blacklist

diff --git a/drivers/net/usb/cdc_ether.c b/drivers/net/usb/cdc_ether.c
index 8ab281b478f2..fa5c2e7aff1a 100644
--- a/drivers/net/usb/cdc_ether.c
+++ b/drivers/net/usb/cdc_ether.c
@@ -546,6 +546,7 @@ static const struct driver_info wwan_info = {
 #define DELL_VENDOR_ID 0x413C
 #define REALTEK_VENDOR_ID  0x0bda
 #define SAMSUNG_VENDOR_ID  0x04e8
+#define LINKSYS_VENDOR_ID  0x13b1
 #define LENOVO_VENDOR_ID   0x17ef
 #define NVIDIA_VENDOR_ID   0x0955
 #define HP_VENDOR_ID   0x03f0
@@ -737,6 +738,13 @@ static const struct usb_device_id  products[] = {
.driver_info = 0,
 },
 
+/* Linksys USB3GIGV1 Ethernet Adapter */
+{
+   USB_DEVICE_AND_INTERFACE_INFO(LINKSYS_VENDOR_ID, 0x0041, USB_CLASS_COMM,
+   USB_CDC_SUBCLASS_ETHERNET, USB_CDC_PROTO_NONE),
+   .driver_info = 0,
+},
+
 /* ThinkPad USB-C Dock (based on Realtek RTL8153) */
 {
USB_DEVICE_AND_INTERFACE_INFO(LENOVO_VENDOR_ID, 0x3062, USB_CLASS_COMM,
diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index ceb78e2ea4f0..941ece08ba78 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -613,6 +613,7 @@ enum rtl8152_flags {
 #define VENDOR_ID_MICROSOFT0x045e
 #define VENDOR_ID_SAMSUNG  0x04e8
 #define VENDOR_ID_LENOVO   0x17ef
+#define VENDOR_ID_LINKSYS  0x13b1
 #define VENDOR_ID_NVIDIA   0x0955
 
 #define MCU_TYPE_PLA   0x0100
@@ -5316,6 +5317,7 @@ static const struct usb_device_id rtl8152_table[] = {
{REALTEK_USB_DEVICE(VENDOR_ID_LENOVO,  0x7205)},
{REALTEK_USB_DEVICE(VENDOR_ID_LENOVO,  0x720c)},
{REALTEK_USB_DEVICE(VENDOR_ID_LENOVO,  0x7214)},
+   {REALTEK_USB_DEVICE(VENDOR_ID_LINKSYS, 0x0041)},
{REALTEK_USB_DEVICE(VENDOR_ID_NVIDIA,  0x09ff)},
{}
 };
-- 
2.14.1.821.g8fa685d3b7-goog

Re: [PATCH] r8152: add Linksys USB3GIGV1 id

[Grant Grundler]

[grrhmail...sorry! resending as plain text]

Hallo Oliver!

On Mon, Sep 25, 2017 at 7:51 AM, Oliver Neukum <oneu...@suse.com> wrote:
> Am Freitag, den 22.09.2017, 12:06 -0700 schrieb Grant Grundler:
> > This Linksys dongle by default comes up in cdc_ether mode.
> > This patch allows r8152 to claim the device:
> >Bus 002 Device 002: ID 13b1:0041 Linksys
>
> Hi,
>
> have you tested this in case cdc_ether is for some reason already
> loaded?

I did not consider testing this case since it's not possible on a
normal chromeos system (the entire root file system is signed for
normal users and get's rebooted after an update).  I could test this
in developer mode of course.

Did you expect both driver probe routines to claim the device and
wreak havoc with the device?

> The patch seems to enable r8152 but does not disable cdc_ether.

Correct. r8152 happens to claim the device before cdc_ether does - I
thought because cdc_ether is a class driver and only gets picked up
after vendor specific drivers are probed.  Is that correct?

I didn't realize cdc_ether has a blacklist to make sure
RTL8152|RTL8153 devices are not picked up by cdc_ether. Would you
prefer I add this device to the blacklist in the same patch?

cheers,
grant

>
> Regards
> Oliver
>

[PATCH] r8152: add Linksys USB3GIGV1 id

[Grant Grundler]

This Linksys dongle by default comes up in cdc_ether mode.
This patch allows r8152 to claim the device:
   Bus 002 Device 002: ID 13b1:0041 Linksys

Signed-off-by: Grant Grundler <grund...@chromium.org>
---
 drivers/net/usb/r8152.c | 2 ++
 1 file changed, 2 insertions(+)

This was tested on chromeos-3.14, chromeos-3.18, and chromeos-4.4 kernels
with a mix of ARM/x86-64 systems.

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index ceb78e2ea4f0..941ece08ba78 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -613,6 +613,7 @@ enum rtl8152_flags {
 #define VENDOR_ID_MICROSOFT0x045e
 #define VENDOR_ID_SAMSUNG  0x04e8
 #define VENDOR_ID_LENOVO   0x17ef
+#define VENDOR_ID_LINKSYS  0x13b1
 #define VENDOR_ID_NVIDIA   0x0955
 
 #define MCU_TYPE_PLA   0x0100
@@ -5316,6 +5317,7 @@ static const struct usb_device_id rtl8152_table[] = {
{REALTEK_USB_DEVICE(VENDOR_ID_LENOVO,  0x7205)},
{REALTEK_USB_DEVICE(VENDOR_ID_LENOVO,  0x720c)},
{REALTEK_USB_DEVICE(VENDOR_ID_LENOVO,  0x7214)},
+   {REALTEK_USB_DEVICE(VENDOR_ID_LINKSYS, 0x0041)},
{REALTEK_USB_DEVICE(VENDOR_ID_NVIDIA,  0x09ff)},
{}
 };
-- 
2.14.1.821.g8fa685d3b7-goog

Re: [PATCH v3 5/5] net: asix: autoneg will set WRITE_MEDIUM reg

[Grant Grundler]

On Thu, Sep 1, 2016 at 10:02 AM, Eric Dumazet <eric.duma...@gmail.com> wrote:
> On Thu, 2016-09-01 at 12:47 -0400, Robert Foss wrote:
>
>> I'm not quite sure how the first From line was added, it
>> should not have been.
>> Grant Grundler is most definitely the author.
>>
>> Would you like me to resubmit in v++ and make sure that it has been
>> corrected?
>
> This is too late, patches are already merged in David Miller net-next
> tree.
>
> These kinds of errors can not be fixed, we have to be very careful at
> submission/review time.
>
> I guess Grant does not care, but some contributors, especially new ones
> would like to get proper attribution.

I do not mind. Robert will get email about bugs instead of me. :D

cheers,
grant

>
> Thanks.
>
>

Re: [PATCH 1/3] net: asix: Add in_pm parameter

[Grant Grundler]

On Tue, Jul 26, 2016 at 2:14 PM, Robert Foss  wrote:
...
> Thanks for the feedback (for this patch and the other ones)!
> I'm preparing a v2 and will submit it withing a day or two.

Excellent! very welcome and thanks again for picking this up.

...
>> FTR, current  drivers/net/usb/ax88179_178a.c uses "in_pm" as well.
>
> Ah! Would you like that to be noted in the commit message?

No - I generally do not muck with commit messages that I didn't write.
It's part of the "finger print" of a patch (so one can trace origin
across distro's and kernel versions).

My comment was intended for reviewers and could be included in the
"comments after the commit message". (ie below the "---" marker in the
patch email).

...
>> BTW, I have two additional changes for AX88772x support sitting in my
>> "needs more work" queue (for quite a while already):
>>https://chromium-review.googlesource.com/#/c/229620/
>>"asix: autoneg will set WRITE_MEDIUM reg"
>>
>>https://chromium-review.googlesource.com/#/c/231162/
>>"net: asix: see 802.3 spec for phy reset"
>>
>> I would certainly approve if _anyone_ picked these up, tested them,
>> and then submitted them to netdev.
>
> Unfortunately I am without appropriate hardware at the moment.

That's a risky thing to point out for two reasons:

1) I can send you more HW than you could possibly ever use. :)

2) Reviewers of this patch series are already wondering how you
verified the AX88772x patches work on current kernel.org (or
netdev-next) branches.

Please email me with your shipping address (OFFLIST!) and I'll try to
send you some Asix HW to test with.  I'm located in "Silicon Valley"
(USA) and can easily ship in the US. Other countries is a bit harder
and in some cases not possible.

cheers,
grant

Re: [PATCH 3/3] net: asix: Fix AX88772x resume failures

[Grant Grundler]

Robert,
This patch content LGTM but isn't including the correct "meta data"
regarding it's origin.

On Mon, Jul 25, 2016 at 10:40 AM,  <robert.f...@collabora.com> wrote:
> From: WK Tsai <wk.t...@nvidia.com>

WK Tsai is not the author of this patch.
I believe Allan Chou <al...@asix.com.tw> is the author.

> The change fixes AX88772x resume failure by
> - Restore incorrect AX88772A PHY registers when resetting
> - Need to stop MAC operation when suspending
> - Need to restart MII when restoring PHY
>
> Signed-off-by: WK Tsai <wk.t...@nvidia.com>

This patch was applied to four different chromium.org kernel branches:
   https://chromium-review.googlesource.com/#/q/Fix+AX88772x+resume+failures

And has substantial meta data regarding author and reviewers:
  (summarized from https://chromium-review.googlesource.com/#/c/314520/)

Signed-off-by: Allan Chou <al...@asix.com.tw>
Signed-off-by: WK Tsai <wk.t...@nvidia.com>
Tested-by: Grant Grundler <grund...@chromium.org>
Reviewed-by: Wang-Kai Tsai <gis5...@gmail.com>
Reviewed-by: Mark Kuo <m...@nvidia.com>
Reviewed-by: Grant Grundler <grund...@chromium.org>
Reviewed-by: Allan Chou <allanchou1...@gmail.com>
Reviewed-by: Vincent Palatin <vpala...@chromium.org>

cheers,
grant

> ---
>  drivers/net/usb/asix_devices.c | 47 
> +-
>  1 file changed, 46 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/usb/asix_devices.c b/drivers/net/usb/asix_devices.c
> index ebeb730..083dc2e 100644
> --- a/drivers/net/usb/asix_devices.c
> +++ b/drivers/net/usb/asix_devices.c
> @@ -35,6 +35,15 @@
>
>  #definePHY_MODE_RTL8211CL  0x000C
>
> +#define AX88772A_PHY14H0x14
> +#define AX88772A_PHY14H_DEFAULT 0x442C
> +
> +#define AX88772A_PHY15H0x15
> +#define AX88772A_PHY15H_DEFAULT 0x03C8
> +
> +#define AX88772A_PHY16H0x16
> +#define AX88772A_PHY16H_DEFAULT 0x4044
> +
>  struct ax88172_int_data {
> __le16 res1;
> u8 link;
> @@ -424,7 +433,7 @@ static int ax88772a_hw_reset(struct usbnet *dev, int 
> in_pm)
>  {
> struct asix_data *data = (struct asix_data *)>data;
> int ret, embd_phy;
> -   u16 rx_ctl;
> +   u16 rx_ctl, phy14h, phy15h, phy16h;
> u8 chipcode = 0;
>
> ret = asix_write_gpio(dev, AX_GPIO_RSE, 5, in_pm);
> @@ -482,6 +491,32 @@ static int ax88772a_hw_reset(struct usbnet *dev, int 
> in_pm)
>ret);
> goto out;
> }
> +   } else if ((chipcode & AX_CHIPCODE_MASK) == AX_AX88772A_CHIPCODE) {
> +   /* Check if the PHY registers have default settings */
> +   phy14h = asix_mdio_read_nopm(dev->net, dev->mii.phy_id,
> +AX88772A_PHY14H);
> +   phy15h = asix_mdio_read_nopm(dev->net, dev->mii.phy_id,
> +AX88772A_PHY15H);
> +   phy16h = asix_mdio_read_nopm(dev->net, dev->mii.phy_id,
> +AX88772A_PHY16H);
> +
> +   netdev_dbg(dev->net,
> +  "772a_hw_reset: MR20=0x%x MR21=0x%x MR22=0x%x\n",
> +  phy14h, phy15h, phy16h);
> +
> +   /* Restore PHY registers default setting if not */
> +   if (phy14h != AX88772A_PHY14H_DEFAULT)
> +   asix_mdio_write_nopm(dev->net, dev->mii.phy_id,
> +AX88772A_PHY14H,
> +AX88772A_PHY14H_DEFAULT);
> +   if (phy15h != AX88772A_PHY15H_DEFAULT)
> +   asix_mdio_write_nopm(dev->net, dev->mii.phy_id,
> +AX88772A_PHY15H,
> +AX88772A_PHY15H_DEFAULT);
> +   if (phy16h != AX88772A_PHY16H_DEFAULT)
> +   asix_mdio_write_nopm(dev->net, dev->mii.phy_id,
> +AX88772A_PHY16H,
> +AX88772A_PHY16H_DEFAULT);
> }
>
> ret = asix_write_cmd(dev, AX_CMD_WRITE_IPG0,
> @@ -543,6 +578,15 @@ static const struct net_device_ops ax88772_netdev_ops = {
>  static void ax88772_suspend(struct usbnet *dev)
>  {
> struct asix_common_private *priv = dev->driver_priv;
> +   u16 medium;
> +
> +   /* Stop MAC operation */
> +   medium = asix_read_medium_status(dev, 0);
> +   medium &= ~AX_MEDIUM_RE;
>

Re: [PATCH 2/3] net: asix: Avoid looping when the device is disconnected

[Grant Grundler]

On Mon, Jul 25, 2016 at 10:40 AM,   wrote:
> From: Vincent Palatin 
>
> Check the answers from the USB stack and avoid re-sending multiple times
> the request if the device has disappeared.
>
> Signed-off-by: Vincent Palatin 

The original chromium.org code review:
https://chromium-review.googlesource.com/255931

resulted in some additional commit message meta-data that would be
nice to include in the kernel.org commit message:
Reviewed-by: Julius Werner 
Reviewed-by: Douglas Anderson 
Tested-by: Douglas Anderson 

And yes, patch LGTM too. :)

cheers,
grant

> ---
>  drivers/net/usb/asix_common.c  | 56 
> +-
>  drivers/net/usb/asix_devices.c |  2 ++
>  2 files changed, 46 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/net/usb/asix_common.c b/drivers/net/usb/asix_common.c
> index f0ccf76..0339560 100644
> --- a/drivers/net/usb/asix_common.c
> +++ b/drivers/net/usb/asix_common.c
> @@ -428,13 +428,21 @@ int asix_mdio_read(struct net_device *netdev, int 
> phy_id, int loc)
> __le16 res;
> u8 smsr;
> int i = 0;
> +   int ret;
>
> mutex_lock(>phy_mutex);
> do {
> -   asix_set_sw_mii(dev, 0);
> +   ret = asix_set_sw_mii(dev, 0);
> +   if (ret == -ENODEV)
> +   break;
> usleep_range(1000, 1100);
> -   asix_read_cmd(dev, AX_CMD_STATMNGSTS_REG, 0, 0, 1, , 0);
> -   } while (!(smsr & AX_HOST_EN) && (i++ < 30));
> +   ret = asix_read_cmd(dev, AX_CMD_STATMNGSTS_REG,
> +   0, 0, 1, , 0);
> +   } while (!(smsr & AX_HOST_EN) && (i++ < 30) && (ret != -ENODEV));
> +   if (ret == -ENODEV) {
> +   mutex_unlock(>phy_mutex);
> +   return ret;
> +   }
>
> asix_read_cmd(dev, AX_CMD_READ_MII_REG, phy_id,
> (__u16)loc, 2, , 0);
> @@ -453,16 +461,24 @@ void asix_mdio_write(struct net_device *netdev, int 
> phy_id, int loc, int val)
> __le16 res = cpu_to_le16(val);
> u8 smsr;
> int i = 0;
> +   int ret;
>
> netdev_dbg(dev->net, "asix_mdio_write() phy_id=0x%02x, loc=0x%02x, 
> val=0x%04x\n",
> phy_id, loc, val);
>
> mutex_lock(>phy_mutex);
> do {
> -   asix_set_sw_mii(dev, 0);
> +   ret = asix_set_sw_mii(dev, 0);
> +   if (ret == -ENODEV)
> +   break;
> usleep_range(1000, 1100);
> -   asix_read_cmd(dev, AX_CMD_STATMNGSTS_REG, 0, 0, 1, , 0);
> -   } while (!(smsr & AX_HOST_EN) && (i++ < 30));
> +   ret = asix_read_cmd(dev, AX_CMD_STATMNGSTS_REG,
> +   0, 0, 1, , 0);
> +   } while (!(smsr & AX_HOST_EN) && (i++ < 30) && (ret != -ENODEV));
> +   if (ret == -ENODEV) {
> +   mutex_unlock(>phy_mutex);
> +   return;
> +   }
>
> asix_write_cmd(dev, AX_CMD_WRITE_MII_REG, phy_id,
>(__u16)loc, 2, , 0);
> @@ -476,13 +492,21 @@ int asix_mdio_read_nopm(struct net_device *netdev, int 
> phy_id, int loc)
> __le16 res;
> u8 smsr;
> int i = 0;
> +   int ret;
>
> mutex_lock(>phy_mutex);
> do {
> -   asix_set_sw_mii(dev, 1);
> +   ret = asix_set_sw_mii(dev, 1);
> +   if (ret == -ENODEV)
> +   break;
> usleep_range(1000, 1100);
> -   asix_read_cmd(dev, AX_CMD_STATMNGSTS_REG, 0, 0, 1, , 1);
> -   } while (!(smsr & AX_HOST_EN) && (i++ < 30));
> +   ret = asix_read_cmd(dev, AX_CMD_STATMNGSTS_REG,
> +   0, 0, 1, , 1);
> +   } while (!(smsr & AX_HOST_EN) && (i++ < 30) && (ret != -ENODEV));
> +   if (ret == -ENODEV) {
> +   mutex_unlock(>phy_mutex);
> +   return ret;
> +   }
>
> asix_read_cmd(dev, AX_CMD_READ_MII_REG, phy_id,
>   (__u16)loc, 2, , 1);
> @@ -502,16 +526,24 @@ asix_mdio_write_nopm(struct net_device *netdev, int 
> phy_id, int loc, int val)
> __le16 res = cpu_to_le16(val);
> u8 smsr;
> int i = 0;
> +   int ret;
>
> netdev_dbg(dev->net, "asix_mdio_write() phy_id=0x%02x, loc=0x%02x, 
> val=0x%04x\n",
> phy_id, loc, val);
>
> mutex_lock(>phy_mutex);
> do {
> -   asix_set_sw_mii(dev, 1);
> +   ret = asix_set_sw_mii(dev, 1);
> +   if (ret == -ENODEV)
> +   break;
> usleep_range(1000, 1100);
> -   asix_read_cmd(dev, AX_CMD_STATMNGSTS_REG, 0, 0, 1, , 1);
> -   } while (!(smsr & AX_HOST_EN) && (i++ < 30));
> +   ret = 

Re: [PATCH 1/3] net: asix: Add in_pm parameter

[Grant Grundler]

[as plain text this time...]

Robert,

On Mon, Jul 25, 2016 at 10:40 AM,  <robert.f...@collabora.com> wrote:
> From: Grant Grundler <grund...@chromium.org>

For the record, I believe I am not the author of these patches.

I believe the original author is
Signed-off-by: Freddy Xin <fre...@asix.com.tw>

as recorded in the following code reviews (and testing) that I was
responsible for:

https://chromium-review.googlesource.com/#/q/owner:%22Grant+Grundler%22+status:merged+asix+in_pm

And I would certainly be happy to see this code go upstream and
expected ASIX would submit this change upstream.

> In order to R/W registers in suspend/resume functions, in_pm flags are
> added to some functions to determine whether the nopm version of usb
> functions is called.

FTR, current  drivers/net/usb/ax88179_178a.c uses "in_pm" as well.

> Save BMCR and ANAR PHY registers in suspend function and restore them
> in resume function.
>
> Reset HW in resume function to ensure the PHY works correctly.
>
> Signed-off-by: Grant Grundler <grund...@chromium.org>

BTW, I have two additional changes for AX88772x support sitting in my
"needs more work" queue (for quite a while already):
   https://chromium-review.googlesource.com/#/c/229620/
   "asix: autoneg will set WRITE_MEDIUM reg"

   https://chromium-review.googlesource.com/#/c/231162/
   "net: asix: see 802.3 spec for phy reset"

I would certainly approve if _anyone_ picked these up, tested them,
and then submitted them to netdev.

cheers,
grant

> ---
>  drivers/net/usb/asix.h |  40 +++--
>  drivers/net/usb/asix_common.c  | 180 +++-
>  drivers/net/usb/asix_devices.c | 373 
> -
>  drivers/net/usb/ax88172a.c |  29 ++--
>  4 files changed, 472 insertions(+), 150 deletions(-)
>
> diff --git a/drivers/net/usb/asix.h b/drivers/net/usb/asix.h
> index a2d3ea6..d109242 100644
> --- a/drivers/net/usb/asix.h
> +++ b/drivers/net/usb/asix.h
> @@ -46,6 +46,7 @@
>  #define AX_CMD_SET_SW_MII  0x06
>  #define AX_CMD_READ_MII_REG0x07
>  #define AX_CMD_WRITE_MII_REG   0x08
> +#define AX_CMD_STATMNGSTS_REG  0x09
>  #define AX_CMD_SET_HW_MII  0x0a
>  #define AX_CMD_READ_EEPROM 0x0b
>  #define AX_CMD_WRITE_EEPROM0x0c
> @@ -71,6 +72,17 @@
>  #define AX_CMD_SW_RESET0x20
>  #define AX_CMD_SW_PHY_STATUS   0x21
>  #define AX_CMD_SW_PHY_SELECT   0x22
> +#define AX_QCTCTRL 0x2A
> +
> +#define AX_CHIPCODE_MASK   0x70
> +#define AX_AX88772_CHIPCODE0x00
> +#define AX_AX88772A_CHIPCODE   0x10
> +#define AX_AX88772B_CHIPCODE   0x20
> +#define AX_HOST_EN 0x01
> +
> +#define AX_PHYSEL_PSEL 0x01
> +#define AX_PHYSEL_SSMII0
> +#define AX_PHYSEL_SSEN 0x10
>
>  #define AX_PHY_SELECT_MASK (BIT(3) | BIT(2))
>  #define AX_PHY_SELECT_INTERNAL 0
> @@ -173,6 +185,10 @@ struct asix_rx_fixup_info {
>  };
>
>  struct asix_common_private {
> +   void (*resume)(struct usbnet *dev);
> +   void (*suspend)(struct usbnet *dev);
> +   u16 presvd_phy_advertise;
> +   u16 presvd_phy_bmcr;
> struct asix_rx_fixup_info rx_fixup_info;
>  };
>
> @@ -182,10 +198,10 @@ extern const struct driver_info ax88172a_info;
>  #define FLAG_EEPROM_MAC(1UL << 0)  /* init device MAC from 
> eeprom */
>
>  int asix_read_cmd(struct usbnet *dev, u8 cmd, u16 value, u16 index,
> - u16 size, void *data);
> + u16 size, void *data, int in_pm);
>
>  int asix_write_cmd(struct usbnet *dev, u8 cmd, u16 value, u16 index,
> -  u16 size, void *data);
> +  u16 size, void *data, int in_pm);
>
>  void asix_write_cmd_async(struct usbnet *dev, u8 cmd, u16 value,
>   u16 index, u16 size, void *data);
> @@ -197,27 +213,31 @@ int asix_rx_fixup_common(struct usbnet *dev, struct 
> sk_buff *skb);
>  struct sk_buff *asix_tx_fixup(struct usbnet *dev, struct sk_buff *skb,
>   gfp_t flags);
>
> -int asix_set_sw_mii(struct usbnet *dev);
> -int asix_set_hw_mii(struct usbnet *dev);
> +int asix_set_sw_mii(struct usbnet *dev, int in_pm);
> +int asix_set_hw_mii(struct usbnet *dev, int in_pm);
>
>  int asix_read_phy_addr(struct usbnet *dev, int internal);
>  int asix_get_phy_addr(struct usbnet *dev);
>
> -int asix_sw_reset(struct usbnet *dev, u8 flags);
> +int asix_sw_reset(struct usbnet *dev, u8 flags, int in_pm);
>
> -u16 asix_read_rx_ctl(struct usbne

Re: [PATCH] r8152: add MODULE_VERSION

[Grant Grundler]

On Fri, Jul 15, 2016 at 2:25 PM, David Miller <da...@davemloft.net> wrote:
> From: Grant Grundler <grund...@chromium.org>
> Date: Thu, 14 Jul 2016 11:27:16 -0700
>
>> ethtool -i provides a driver version that is hard coded.
>> Export the same value via "modinfo".
>>
>> Signed-off-by: Grant Grundler <grund...@chromium.org>
>
> Applied.

Excellent - thank you. :)

grant

[PATCH] r8152: add MODULE_VERSION

[Grant Grundler]

ethtool -i provides a driver version that is hard coded.
Export the same value via "modinfo".

Signed-off-by: Grant Grundler <grund...@chromium.org>
---
 drivers/net/usb/r8152.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 0da72d3..1c01ed5 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -4359,3 +4359,4 @@ module_usb_driver(rtl8152_driver);
 MODULE_AUTHOR(DRIVER_AUTHOR);
 MODULE_DESCRIPTION(DRIVER_DESC);
 MODULE_LICENSE("GPL");
+MODULE_VERSION(DRIVER_VERSION);
-- 
2.8.0.rc3.226.g39d4020

Re: [PATCH] net: tulip: turn compile-time warning into dev_warn()

[Grant Grundler]

On Thu, Nov 19, 2015 at 3:50 PM, Francois Romieu <rom...@fr.zoreil.com> wrote:
> Grant Grundler <grantgrund...@gmail.com> :
> [...]
>> Some additional minor refactoring of the code could convert this into
>> a "multi-bus driver" if there is any system that could incorporate
>> both a platform device and a PCI device.
>>
>> I expect the conversion to DMA API to be straight forward as the next
>> patch shows:
>
> You may change your mind once error checking is factored in.

In the absence of your patch, I definitely wouldn't.

The reason is the linux 2.4 PCI-DMA mapping API would panic on failure
and never return an error:
  
http://lists.parisc-linux.org/pipermail/parisc-linux/2000-November/009847.html

The simplistic "conversion" would just panic the system on DMA API
errors and still behave the same as before.

To be clear, I have never advocated for ignoring DMA API errors (just
accepted the linux-2.4 design choice to ignore them since that's what
davem wanted at the time).

> Uncompiled stuff below includes a remaining WTF I am no too sure about
> but it should be closer to what is needed.

Yup - agreed. Over all this LGTM and below I've suggested three ways
to deal with DMA API error handling in set_rx_mode().

cheers,
grant

>
> diff --git a/drivers/net/ethernet/dec/tulip/tulip_core.c 
> b/drivers/net/ethernet/dec/tulip/tulip_core.c
> index ed41559..0e18b5d9 100644
> --- a/drivers/net/ethernet/dec/tulip/tulip_core.c
> +++ b/drivers/net/ethernet/dec/tulip/tulip_core.c
> @@ -263,7 +263,6 @@ static netdev_tx_t tulip_start_xmit(struct sk_buff *skb,
>   struct net_device *dev);
>  static int tulip_open(struct net_device *dev);
>  static int tulip_close(struct net_device *dev);
> -static void tulip_up(struct net_device *dev);
>  static void tulip_down(struct net_device *dev);
>  static struct net_device_stats *tulip_get_stats(struct net_device *dev);
>  static int private_ioctl(struct net_device *dev, struct ifreq *rq, int cmd);
> @@ -291,7 +290,7 @@ static void tulip_set_power_state (struct tulip_private 
> *tp,
>  }
>
>
> -static void tulip_up(struct net_device *dev)
> +static int tulip_up(struct net_device *dev)
>  {
> struct tulip_private *tp = netdev_priv(dev);
> void __iomem *ioaddr = tp->base_addr;
> @@ -299,10 +298,6 @@ static void tulip_up(struct net_device *dev)
> u32 reg;
> int i;
>
> -#ifdef CONFIG_TULIP_NAPI
> -   napi_enable(>napi);
> -#endif
> -
> /* Wake the chip from sleep/snooze mode. */
> tulip_set_power_state (tp, 0, 0);
>
> @@ -353,6 +348,7 @@ static void tulip_up(struct net_device *dev)
> /* This is set_rx_mode(), but without starting the 
> transmitter. */
> u16 *eaddrs = (u16 *)dev->dev_addr;
> u16 *setup_frm = >setup_frame[15*6];
> +   struct device *d = >pdev->dev;
> dma_addr_t mapping;
>
> /* 21140 bug: you must add the broadcast address. */
> @@ -362,9 +358,12 @@ static void tulip_up(struct net_device *dev)
> *setup_frm++ = eaddrs[1]; *setup_frm++ = eaddrs[1];
> *setup_frm++ = eaddrs[2]; *setup_frm++ = eaddrs[2];
>
> -   mapping = pci_map_single(tp->pdev, tp->setup_frame,
> -sizeof(tp->setup_frame),
> -PCI_DMA_TODEVICE);
> +   mapping = dma_map_single(d, tp->setup_frame,
> +sizeof(tp->setup_frame), 
> DMA_TO_DEVICE);
> +   if (unlikely(dma_mapping_error(d, mapping))) {
> +   tulip_set_power_state(tp, 0, 1);
> +   return -EIO;
> +   }
> tp->tx_buffers[tp->cur_tx].skb = NULL;
> tp->tx_buffers[tp->cur_tx].mapping = mapping;
>
> @@ -376,6 +375,10 @@ static void tulip_up(struct net_device *dev)
> tp->cur_tx++;
> }
>
> +#ifdef CONFIG_TULIP_NAPI
> +   napi_enable(>napi);
> +#endif
> +
> tp->saved_if_port = dev->if_port;
> if (dev->if_port == 0)
> dev->if_port = tp->default_port;
> @@ -516,24 +519,30 @@ static int
>  tulip_open(struct net_device *dev)
>  {
> struct tulip_private *tp = netdev_priv(dev);
> -   int retval;
> +   int irq = tp->pdev->irq;
> +   int rc;
>
> -   tulip_init_ring (dev);
> +   rc = tulip_init_ring(dev);
> +   if (rc < 0)
> +   goto out;
>
> -   retval = request_irq(tp->pdev->irq, tuli

[PATCH] net: tulip: update MAINTAINER status to Orphan

[Grant Grundler]

From: Grant Grundler <grund...@parisc-linux.org>

I haven't had any PCI tulip HW for the past ~5 years. I have
been reviewing tulip patches and can continue doing that.

Signed-off-by: Grant Grundler <grund...@parisc-linux.org>
---
I'm also proposing to add linux-parisc to the list since AFAIK, all
parisc systems but the C8000 workstations (PA8800/PA8900 CPU)
use tulip for onboard LAN.

Specific mips and alpha systems also care about tulip driver too.
But I don't know either well enough to suggest respective mailing
lists should see every tulip patch.

 MAINTAINERS | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index ea17512..ec07061 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -10888,9 +10888,9 @@ S:  Maintained
 F: drivers/media/tuners/tua9001*
 
 TULIP NETWORK DRIVERS
-M: Grant Grundler <grund...@parisc-linux.org>
 L: netdev@vger.kernel.org
-S: Maintained
+L: linux-par...@vger.kernel.org
+S: Orphan
 F: drivers/net/ethernet/dec/tulip/
 
 TUN/TAP driver
-- 
2.6.0.rc2.230.g3dd15c0

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] net: tulip: turn compile-time warning into dev_warn()

[Grant Grundler]

On Thu, Nov 19, 2015 at 4:26 AM, Will Deacon <will.dea...@arm.com> wrote:
> On Thu, Nov 19, 2015 at 11:42:26AM +0100, Arnd Bergmann wrote:
>> The tulip driver causes annoying build-time warnings for allmodconfig
>> builds for all recent architectures:
>>
>> dec/tulip/winbond-840.c:910:2: warning: #warning Processor architecture 
>> undefined
>> dec/tulip/tulip_core.c:101:2: warning: #warning Processor architecture 
>> undefined!
>>
>> This is the last remaining warning for arm64, and I'd like to get rid of
>> it. We don't really know the cache line size, architecturally it would
>> be at least 16 bytes, but all implementations I found have 64 or 128
>> bytes. Configuring tulip for 32-byte lines as we do on ARM32 seems to
>> be the safe but slow default, and nobody who cares about performance these
>> days would use a tulip chip anyway, so we can just use that.
>>
>> To save the next person the job of trying to find out what this is for
>> and picking a default for their architecture just to kill off the warning,
>> I'm now removing the preprocessor #warning and turning it into a pr_warn
>> or dev_warn that prints the equivalent information when the driver gets
>> loaded.
>>
>> Signed-off-by: Arnd Bergmann <a...@arndb.de>
>>
>> diff --git a/drivers/net/ethernet/dec/tulip/tulip_core.c 
>> b/drivers/net/ethernet/dec/tulip/tulip_core.c
>> index ed41559bae77..b553409e04ad 100644
>> --- a/drivers/net/ethernet/dec/tulip/tulip_core.c
>> +++ b/drivers/net/ethernet/dec/tulip/tulip_core.c
>> @@ -98,8 +98,7 @@ static int csr0 = 0x01A0 | 0x4800;
>>  #elif defined(__mips__)
>>  static int csr0 = 0x0020 | 0x4000;
>>  #else
>> -#warning Processor architecture undefined!
>> -static int csr0 = 0x00A0 | 0x4800;
>> +static int csr0;
>>  #endif
>>
>>  /* Operational parameters that usually are not changed. */
>> @@ -1982,6 +1981,12 @@ static int __init tulip_init (void)
>>   pr_info("%s", version);
>>  #endif
>>
>> + if (!csr0) {
>> + pr_warn("tulip: unknown CPU architecture, using default 
>> csr0\n");
>> + /* default to 8 longword cache line alignment */
>> + csr0 = 0x00A0 | 0x4800;
>
> Maybe print "defaulting to 8 longword cache line alignment" instead of
> "default csr0"?

This is a good idea.

>> diff --git a/drivers/net/ethernet/dec/tulip/winbond-840.c 
>> b/drivers/net/ethernet/dec/tulip/winbond-840.c
>> index 9beb3d34d4ba..3c0e4d5c5fef 100644
>> --- a/drivers/net/ethernet/dec/tulip/winbond-840.c
>> +++ b/drivers/net/ethernet/dec/tulip/winbond-840.c
>> @@ -907,7 +907,7 @@ static void init_registers(struct net_device *dev)
>>  #elif defined(CONFIG_SPARC) || defined (CONFIG_PARISC) || 
>> defined(CONFIG_ARM)
>>   i |= 0x4800;
>>  #else
>> -#warning Processor architecture undefined
>> + dev_warn(>dev, "unknown CPU architecture, using default csr0 
>> setting\n");
>>   i |= 0x4800;
>
> Then we could print the default csr0 value here.
>
> But, to be honest, this patch fixes a #warning on arm64 for a driver that
> I never expect to be used. So whatever you do to silence it:
>
>   Acked-by: Will Deacon <will.dea...@arm.com>

yeah - same here:
Acked-by: Grant Grundler <grund...@parisc-linux.org>

cheers,
grant

>
> /me waits for on-soc tulip integration.
>
> Will
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] net: tulip: turn compile-time warning into dev_warn()

[Grant Grundler]

On Thu, Nov 19, 2015 at 12:29 PM, Florian Fainelli  wrote:
> On 19/11/15 04:26, Will Deacon wrote:
...
>> /me waits for on-soc tulip integration.
>
> FWIW, this already happened, the ADMtek/Infineon ADM8668 actually
> integrated a Tulip chip. I have not submitted these patches below from
> the OpenWrt tree because the chip is barely used nowadays, and it was
> only mostly popular with the Linksys WRTU54G.
>
> The patches could be made less intrusive if we did convert the pci_dma*
> calls into regular DMA-API calls, which they are nowadays, oh well!

I agree.  IIRC, there was no DMA-API when this driver was written.
James Bottomley added DMA API later and there was no need to convert
since Tulip devices were _only_ PCI at the time.

> https://dev.openwrt.org/browser/trunk/target/linux/adm8668/patches-3.18/004-tulip_pci_split.patch

In general this would be a reasonable patch to submit here with some caveats:
  1) convert to DMA API (first patch)
  2)  add CONFIG_PCI code (second patch) to handle the remaining
discovery and PCI Config space bits.

Some additional minor refactoring of the code could convert this into
a "multi-bus driver" if there is any system that could incorporate
both a platform device and a PCI device.

I expect the conversion to DMA API to be straight forward as the next
patch shows:

> https://dev.openwrt.org/browser/trunk/target/linux/adm8668/patches-3.18/005-tulip_platform.patch

Split this patch into two parts: convert to DMA-API (first patch) and
platform device support (third patch). Should be a "no brainer" to
accept.

Lastly, net/ethernet/dec/tulip driver is up for adoption. I've just
been extremely lazy about updating the MAINTAINERS entry but will
submit that shortly (apologies to Arndt for the bounced email - I know
it's a bit disconcerting to see that.)

I'm happy to continue review tulip code changes anyway.

cheers,
grant

> --
> Florian
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] net: tulip: update MAINTAINER status to Orphan

[Grant Grundler]

On Thu, Nov 19, 2015 at 6:29 PM, Florian Fainelli <f.faine...@gmail.com> wrote:
> On 19/11/15 17:56, Grant Grundler wrote:
>> From: Grant Grundler <grund...@parisc-linux.org>
>>
>> I haven't had any PCI tulip HW for the past ~5 years. I have
>> been reviewing tulip patches and can continue doing that.
>>
>> Signed-off-by: Grant Grundler <grund...@parisc-linux.org>
>> ---
>> I'm also proposing to add linux-parisc to the list since AFAIK, all
>> parisc systems but the C8000 workstations (PA8800/PA8900 CPU)
>> use tulip for onboard LAN.
>>
>> Specific mips and alpha systems also care about tulip driver too.
>> But I don't know either well enough to suggest respective mailing
>> lists should see every tulip patch.
>
> For MIPS, is not Cobalt the primary (and sole) user?

Once upon a time a Mips based router was using tulip as well. I know
they needed to "borrow" some tulip patches that were only in
parisc-linux source tree (for reasons I don't see a need to repeat
here).

> You could add linux-m...@linux-mips.org if that helps.

I wanted to let the mips folks decide if they should be listedand
CC'd Helga (parisc maintainer) in case he objected to added
linux-parisc mailing list.

cheers,
grant

> My Cobalt stayed behind me when
> I moved to the US, so outside of Yoichi, I am not sure who else cares
> these days...
>
>>
>>  MAINTAINERS | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index ea17512..ec07061 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -10888,9 +10888,9 @@ S:Maintained
>>  F:   drivers/media/tuners/tua9001*
>>
>>  TULIP NETWORK DRIVERS
>> -M:   Grant Grundler <grund...@parisc-linux.org>
>>  L:   netdev@vger.kernel.org
>> -S:   Maintained
>> +L:   linux-par...@vger.kernel.org
>> +S:   Orphan
>>  F:   drivers/net/ethernet/dec/tulip/
>>
>>  TUN/TAP driver
>>
>
>
> --
> Florian
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Compex FreedomLine 32 PnP-PCI2 broken with de2104x

[Grant Grundler]

On Mon, Feb 25, 2008 at 02:30:00AM -0500, Jeff Garzik wrote:
 Grant Grundler wrote:
 On Mon, Feb 18, 2008 at 05:40:42PM +0100, Ondrej Zary wrote:
 I think that de2104x driver should be removed (or at least its 
 MODULE_DEVICE_TABLE) and MODULE_DEVICE_TABLE with only 21040 and 21041 
 PCI IDs added to de4x5.

 I can send a patch if this is acceptable.
 It's acceptable to me. Jeff? (jgarzik)

 NAK, sorry, for two reasons:

 1) we don't delete otherwise clean, working drivers

Just to be clear - he's not trying to remove the driver.
He's just interested in making de4x5 the default for this set of boards
by doctoring with the pci device ids each driver will claim.

 simply because of a bug triggered by unplugging a cable.

Ondrej would be happy to test any patches sent. Tracking this sort
of bug down usually isn't trivial as the statement above implies.

 2) de4x5 needs to go away.

Ok. I'd prefer to wait until someone demonstrates de2104x driver is a fully
functional alternative for all the PCI Ids it claims.

thanks,
grant
--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Compex FreedomLine 32 PnP-PCI2 broken with de2104x

[Grant Grundler]

On Mon, Feb 18, 2008 at 05:40:42PM +0100, Ondrej Zary wrote:
 On Monday 18 February 2008 04:21:11 Grant Grundler wrote:
  On Wed, Jan 30, 2008 at 09:23:06PM +0100, Ondrej Zary wrote:
   On Saturday 26 January 2008 21:58:10 Ondrej Zary wrote:
Hello,
I was having problems with these FreedomLine cards with Linux before
but tested it thoroughly today. This card uses DEC 21041 chip and has
TP and BNC connectors:
   
00:12.0 Ethernet controller [0200]: Digital Equipment Corporation
DECchip 21041 [Tulip Pass 3] [1011:0014] (rev 21)
   
   
de2104x driver was loaded automatically by udev and card seemed to
work. Until I disconnected the TP cable and putting it back after a
while. The driver then switched to (non-existing) AUI port and remained
there. I tried to set media to TP using ethtool - and the whole kernel
crashed because of BUG_ON(de_is_running(de));
in de_set_media(). Seems that the driver is unable to stop the DMA in
de_stop_rxtx().
 
  The BUG_ON() is probably fine normally. But the media selection sounds
  broken. It's possible to select the wrong media type with 21040 chip but
  shouldn't be possible with 21041. For 21040 support, see
  de21040_get_media_info(). But de21041_get_srom_info() is expected to
  determine which media
  types are supported from SEPROM media blocks.   My guess is that code
  is broken since it seems to work with de405 driver. If you care to
  work the difference, I'd be happy to make a patch to fix that up.
 
 I don't think that BUG_ON() should be there. It should probably printk a 
 warning but certainly not crash the whole machine.

Ah ok - I agree. I'll see if we can fail more gracfully there.
If you have an idea already, please send me a patch.

Then I found that there's de4x5 driver which supports the same cards as
de2104x (and some other too) - and this one works fine! I can plug and
unplug the cable and even change between TP and BNC ports just by
unplugging one and plugging the other cable in. Unfortunately, this
driver is blacklisted by default - at least in Slackware and Debian.
 
  ISTR there was a time when tulip would compete with de4x5 for devices.
  tulip is the preferred driver. That's clearly no longer the case
  and perhaps both distro's need to revisit this.
 
 de4x5 has no MODULE_DEVICE_TABLE for PCI devices anymore, so no conflicts. 
 That's probably good for cards that work with tulip driver but bad for mine 
 card and also probably for some other cards that (should) work with de2104x.
 
 
The question is: why does de2104x exist? Does it work better with some
hardware?
 
  de2104x is a work in progress.
  That's why it's marked EXPERIMENTAL in the Kconfig file.
 
 Great, it looks to be 6 years old and it's still experimental. Probably 
 because it never worked properly.

Yeah, probably.

 I think that de2104x driver should be removed (or at least its 
 MODULE_DEVICE_TABLE) and MODULE_DEVICE_TABLE with only 21040 and 21041 PCI 
 IDs added to de4x5.
 
 I can send a patch if this is acceptable.

It's acceptable to me. Jeff? (jgarzik)

thanks,
grant
--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] [Bug 5839] uli526x partially recognizing interface

[Grant Grundler]

Patch fixes:
http://bugzilla.kernel.org/show_bug.cgi?id=5839

Init sequence needs to poll phy until phy reset is complete. This is the
same problem that I fixed in 2002 in tulip driver.

Thanks to [EMAIL PROTECTED] for testing this patch.
Thanks to Pozsar Balazs [EMAIL PROTECTED] for posting/testing
a similar patch before:
http://lkml.org/lkml/2006/8/21/45

Signed-off-by: Grant Grundler [EMAIL PROTECTED]


diff --git a/drivers/net/tulip/uli526x.c b/drivers/net/tulip/uli526x.c
index ca2548e..1dd8485 100644
--- a/drivers/net/tulip/uli526x.c
+++ b/drivers/net/tulip/uli526x.c
@@ -484,9 +484,11 @@ static void uli526x_init(struct net_device *dev)
struct uli526x_board_info *db = netdev_priv(dev);
unsigned long ioaddr = db-ioaddr;
u8  phy_tmp;
+   u8  timeout;
u16 phy_value;
u16 phy_reg_reset;
 
+
ULI526X_DBUG(0, uli526x_init(), 0);
 
/* Reset M526x MAC controller */
@@ -511,11 +513,19 @@ static void uli526x_init(struct net_device *dev)
/* Parser SROM and media mode */
db-media_mode = uli526x_media_mode;
 
-   /* Phyxcer capability setting */
+   /* phyxcer capability setting */
phy_reg_reset = phy_read(db-ioaddr, db-phy_addr, 0, db-chip_id);
phy_reg_reset = (phy_reg_reset | 0x8000);
phy_write(db-ioaddr, db-phy_addr, 0, phy_reg_reset, db-chip_id);
+
+   /* See IEEE 802.3-2002.pdf (Section 2, Chapter 22.2.4 Management
+* functions) or phy data sheet for details on phy reset
+*/
udelay(500);
+   timeout = 10;
+   while (timeout-- 
+   phy_read(db-ioaddr, db-phy_addr, 0, db-chip_id)  0x8000)
+   udelay(100);
 
/* Process Phyxcer Media Mode */
uli526x_set_phyxcer(db);

--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] update TULIP MAINTAINERS

[Grant Grundler]

Jeff,
Kyle and I are co-maintaining tulip driver. Normally kyle will review
my patchs and submit them. I'll deal with bugzilla.kernel.org bugs and
try to resolve those bugs.

thanks,
grant

Signed-off-by: Grant Grundler [EMAIL PROTECTED]

--- linux-2.6.24.1/MAINTAINERS-ORIG 2008-02-17 10:33:52.0 -0800
+++ linux-2.6.24.1/MAINTAINERS  2008-02-17 10:47:01.0 -0800
@@ -3776,10 +3776,13 @@
 W: http://www.auk.cx/tms380tr/
 S: Maintained
 
-TULIP NETWORK DRIVER
-L: [EMAIL PROTECTED]
-W: http://sourceforge.net/projects/tulip/
-S: Orphan
+TULIP NETWORK DRIVERS
+P: Grant Grundler
+M: [EMAIL PROTECTED]
+P: Kyle McMartin
+M: [EMAIL PROTECTED]
+L: netdev@vger.kernel.org
+S: Maintained
 
 TUN/TAP driver
 P: Maxim Krasnyansky
--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Compex FreedomLine 32 PnP-PCI2 broken with de2104x

[Grant Grundler]

On Wed, Jan 30, 2008 at 09:23:06PM +0100, Ondrej Zary wrote:
 On Saturday 26 January 2008 21:58:10 Ondrej Zary wrote:
  Hello,
  I was having problems with these FreedomLine cards with Linux before but
  tested it thoroughly today. This card uses DEC 21041 chip and has TP and
  BNC connectors:
 
  00:12.0 Ethernet controller [0200]: Digital Equipment Corporation DECchip
  21041 [Tulip Pass 3] [1011:0014] (rev 21)
 
 
  de2104x driver was loaded automatically by udev and card seemed to work.
  Until I disconnected the TP cable and putting it back after a while. The
  driver then switched to (non-existing) AUI port and remained there. I tried
  to set media to TP using ethtool - and the whole kernel crashed because of
  BUG_ON(de_is_running(de));
  in de_set_media(). Seems that the driver is unable to stop the DMA in
  de_stop_rxtx().

The BUG_ON() is probably fine normally. But the media selection sounds broken.
It's possible to select the wrong media type with 21040 chip but shouldn't
be possible with 21041. For 21040 support, see de21040_get_media_info().
But de21041_get_srom_info() is expected to determine which media
types are supported from SEPROM media blocks.   My guess is that code
is broken since it seems to work with de405 driver. If you care to
work the difference, I'd be happy to make a patch to fix that up.

Also, from code review, DE2104X driver still has a few places with
potential PCI MMIO Write posting issues.  Specifically, I was looking
in de_stop_hw() and de_set_media(). Several other bits of code correctly
flush MMIO writes: e.g. tulip_read_eeprom().


  I commented out AUI detection in the driver - this time it switched to BNC
  after unplugging the cable and remained there. I also attempted to reset
  the chip when de_stop_rxtx failed but failed to do it.

You'd have to basically hardcode only one media type and it's corresponding
parameters.

  Then I found that there's de4x5 driver which supports the same cards as
  de2104x (and some other too) - and this one works fine! I can plug and
  unplug the cable and even change between TP and BNC ports just by
  unplugging one and plugging the other cable in. Unfortunately, this driver
  is blacklisted by default - at least in Slackware and Debian.

ISTR there was a time when tulip would compete with de4x5 for devices.
tulip is the preferred driver. That's clearly no longer the case
and perhaps both distro's need to revisit this.

  The question is: why does de2104x exist? Does it work better with some
  hardware?

de2104x is a work in progress.
That's why it's marked EXPERIMENTAL in the Kconfig file.

  BTW. Found that the problem exist at least since 2003:
  http://oss.sgi.com/archives/netdev/2003-08/msg00951.html
 
 Does the de2104x driver work correctly for anyone?

No idea. I've only used tulip driver.

thanks for the bug report,
grant
--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [patch 06/13] Fix a potential NULL pointer dereference in uli526x_interrupt() in drivers/net/tulip/uli526x.c

[Grant Grundler]

On Tue, Oct 02, 2007 at 02:11:38PM -0700, [EMAIL PROTECTED] wrote:
 From: Micah Gruber [EMAIL PROTECTED]
 
 This patch fixes an apparent potential null dereference bug where we
 dereference dev before a null check.  This patch simply remvoes the
 can't-happen test for a null pointer.
 
 Signed-off-by: Micah Gruber [EMAIL PROTECTED]
 Cc: Grant Grundler [EMAIL PROTECTED]

Acked-by: Grant Grundler [EMAIL PROTECTED]

thanks!
grant

 Acked-by: Jeff Garzik [EMAIL PROTECTED]
 Acked-by: Kyle McMartin [EMAIL PROTECTED]
 Signed-off-by: Andrew Morton [EMAIL PROTECTED]
 ---
 
  drivers/net/tulip/uli526x.c |5 -
  1 file changed, 5 deletions(-)
 
 diff -puN 
 drivers/net/tulip/uli526x.c~fix-a-potential-null-pointer-dereference-in-uli526x_interrupt
  drivers/net/tulip/uli526x.c
 --- 
 a/drivers/net/tulip/uli526x.c~fix-a-potential-null-pointer-dereference-in-uli526x_interrupt
 +++ a/drivers/net/tulip/uli526x.c
 @@ -664,11 +664,6 @@ static irqreturn_t uli526x_interrupt(int
   unsigned long ioaddr = dev-base_addr;
   unsigned long flags;
  
 - if (!dev) {
 - ULI526X_DBUG(1, uli526x_interrupt() without DEVICE arg, 0);
 - return IRQ_NONE;
 - }
 -
   spin_lock_irqsave(db-lock, flags);
   outl(0, ioaddr + DCR7);
  
 _
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Clean up redundant PHY write line for ULi526x Ethernet driver

[Grant Grundler]

On Thu, Sep 27, 2007 at 11:36:58PM -0400, Jeff Garzik wrote:
 Zang Roy-r61911 wrote:
 From: Roy Zang [EMAIL PROTECTED]
 Clean up redundant PHY write line for ULi526x Ethernet
 Driver.
 Signed-off-by: Roy Zang [EMAIL PROTECTED]
 ---
  drivers/net/tulip/uli526x.c |1 -
  1 files changed, 0 insertions(+), 1 deletions(-)
 diff --git a/drivers/net/tulip/uli526x.c b/drivers/net/tulip/uli526x.c
 index ca2548e..53a8e65 100644
 --- a/drivers/net/tulip/uli526x.c
 +++ b/drivers/net/tulip/uli526x.c
 @@ -1512,7 +1512,6 @@ static void uli526x_process_mode(struct 
 uli526x_board_info *db)
  case ULI526X_100MFD: phy_reg = 0x2100; break;
  }
  phy_write(db-ioaddr, db-phy_addr, 0, phy_reg, 
 db-chip_id);
 -phy_write(db-ioaddr, db-phy_addr, 0, phy_reg, 
 db-chip_id);

 Kyle and Grant, I'll queue this up, unless ya'll object...

please do, I've already Ack'd it for akpm's tree when he sent out the
initial cc.

thanks,
grant


   Jeff

-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [TULIP] Need new maintainer

[Grant Grundler]

On Mon, Jul 30, 2007 at 03:31:58PM -0400, Kyle McMartin wrote:
 On Mon, Jul 30, 2007 at 01:04:13PM -0600, Valerie Henson wrote:
  The Tulip network driver needs a new maintainer!  I no longer have
  time to maintain the Tulip network driver and I'm stepping down.  Jeff
  Garzik would be happy to get volunteers.

Val!
I'm sorry to see you have to drop this one...C'est la Vie.

 Since I already take care of a major consumer of these devices (parisc,
 which pretty much all have tulip) I'm willing to take care of this.
 Alternately, Grant is probably willing.

Yeah, I am willing and able to maintain tulip as well.

Either way, parisc and some mips64 folks are stuck with tulip since
that's what is embedded on the motherboard. So it would make sense
for someone from either camp to maintain it.

Thanks to David Lang for the offer of 4-port Dlink Tulip card.
HP has two types of 4-port tulip cards (64-bit/33Mhz and 32-bit/33Mhz)
that I gave to Val...so whoever picks up the maintainership can
probably (eventually) get those from Val.

thanks,
grant
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [patch 06/10] tulip: fix shutdown DMA/irq race

[Grant Grundler]

On Mon, Mar 26, 2007 at 09:47:25PM -0800, [EMAIL PROTECTED] wrote:
 From: Grant Grundler [EMAIL PROTECTED]
 
 IRQs are racing with tulip_down().  DMA can be restarted by the interrupt
 handler _after_ we call tulip_stop_rxtx() and the DMA buffers are unmapped.
  The result is an MCA (hard crash on ia64) because of an IO TLB miss.  The
 long-term fix is to make the interrupt handler shutdown aware.

I don't know why this patch is surfacing again now.
I believe Val has addressed the problem with the long-term fix described.

thanks,
grant

 Signed-off-by: Grant Grundler [EMAIL PROTECTED]
 Acked-by: Valerie Henson [EMAIL PROTECTED]
 Cc: Jeff Garzik [EMAIL PROTECTED]
 Signed-off-by: Andrew Morton [EMAIL PROTECTED]
 ---
 
  drivers/net/tulip/tulip_core.c |   31 +--
  1 file changed, 21 insertions(+), 10 deletions(-)
 
 diff -puN drivers/net/tulip/tulip_core.c~tulip-fix-shutdown-dma-irq-race 
 drivers/net/tulip/tulip_core.c
 --- a/drivers/net/tulip/tulip_core.c~tulip-fix-shutdown-dma-irq-race
 +++ a/drivers/net/tulip/tulip_core.c
 @@ -734,23 +734,36 @@ static void tulip_down (struct net_devic
  #endif
   spin_lock_irqsave (tp-lock, flags);
  
 + /*
 +   FIXME: We should really add a shutdown-in-progress flag and
 +   check it in the interrupt handler to see whether we should
 +   reenable DMA or not.  The preferred ordering here would be:
 +
 +   stop DMA engine
 +   disable interrupts
 +   remove DMA resources
 +   free_irq()
 +
 +   The below works but is non-obvious and doesn't match the
 +   ordering of bring-up. -VAL
 + */
 +
   /* Disable interrupts by clearing the interrupt mask. */
   iowrite32 (0x, ioaddr + CSR7);
 + ioread32 (ioaddr + CSR7);   /* flush posted write */
  
 - /* Stop the Tx and Rx processes. */
 - tulip_stop_rxtx(tp);
 + spin_unlock_irqrestore (tp-lock, flags);
  
 - /* prepare receive buffers */
 - tulip_refill_rx(dev);
 + free_irq (dev-irq, dev);   /* no more races after this */
 + tulip_stop_rxtx(tp);/* Stop DMA */
  
 - /* release any unconsumed transmit buffers */
 - tulip_clean_tx_ring(tp);
 + /* Put driver back into the state we start with */
 + tulip_refill_rx(dev);   /* prepare RX buffers */
 + tulip_clean_tx_ring(tp);/* clean up unsent TX buffers */
  
   if (ioread32 (ioaddr + CSR6) != 0x)
   tp-stats.rx_missed_errors += ioread32 (ioaddr + CSR8)  0x;
  
 - spin_unlock_irqrestore (tp-lock, flags);
 -
   init_timer(tp-timer);
   tp-timer.data = (unsigned long)dev;
   tp-timer.function = tulip_tbl[tp-chip_id].media_timer;
 @@ -776,7 +789,6 @@ static int tulip_close (struct net_devic
   printk (KERN_DEBUG %s: Shutting down ethercard, status was 
 %2.2x.\n,
   dev-name, ioread32 (ioaddr + CSR5));
  
 - free_irq (dev-irq, dev);
  
   /* Free all the skbuffs in the Rx queue. */
   for (i = 0; i  RX_RING_SIZE; i++) {
 @@ -1747,7 +1759,6 @@ static int tulip_suspend (struct pci_dev
   tulip_down(dev);
  
   netif_device_detach(dev);
 - free_irq(dev-irq, dev);
  
   pci_save_state(pdev);
   pci_disable_device(pdev);
 _
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [patch 4/6] [TULIP] Quiet down tulip_stop_rxtx

[Grant Grundler]

On Thu, Mar 15, 2007 at 11:25:10AM -0400, Jeff Garzik wrote:
...
 Here's the problem with this:  this printk is signalling that the DMA 
 engines have not yet stopped, which is an event of which we should be wary.
 
 While it makes sense to do this patch, since the complaining cards 
 appear to work anyway, we also need to take into account the times when 
 this is not a spurious warning.

It's certainly not spurious warning when in the unload driver module path.
This is when the driver unmaps the control data and the particular packet
it's doing DMA to. 

 Thus, I would consider maybe adding a warning somewhere in the 
 DMA-engine-start region of code, that complains if the DMA engines are 
 already active, or somesuch.

Sure. But also in the shutdown path where we unmap any DMA mappings.

thanks,
grant
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/5] fixing errors handling during pci_driver resume stage [ata]

[Grant Grundler]

On Tue, Jan 09, 2007 at 12:01:28PM +0300, Dmitriy Monakhov wrote:
 ata pci drivers have to return correct error code during resume stage in
 case of errors.
...
 @@ -6246,8 +6253,10 @@ int ata_pci_device_suspend(struct pci_de
  int ata_pci_device_resume(struct pci_dev *pdev)
  {
   struct ata_host *host = dev_get_drvdata(pdev-dev);
 + int err;
  
 - ata_pci_device_do_resume(pdev);
 + if ((err = ata_pci_device_do_resume(pdev)))
 + return err;

nit: in every other case I looked at you did:
   err = foo()
   if (err) ...

Can you make that consistent here too?

thanks,
grant
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] [TULIP] Check the return value from pci_set_mwi()

[Grant Grundler]

On Fri, Oct 06, 2006 at 03:59:57PM -0400, Jeff Garzik wrote:
 The unmodified tulip driver checks both MWI and cacheline-size because 
 one of the clones (PNIC or PNIC2) will let you set the MWI bit, but 
 hardwires cacheline size to zero.

Maybe the generic pci_set_mwi() can verify cacheline size is non-zero?
I don't think each driver should need to enforce this.

 If the arches do not behave consistently, we need to keep the check in 
 the tulip driver, to avoid incorrectly programming the csr0 MWI bit.

Why not fix the arches to be consistent?
There's alot more drivers than arches...and we have control
of the arch specific PCI support.

thanks,
grant
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

PATCH: fix cut/paste error in TCPPROBE

[Grant Grundler]

Fix cut/paste error in TCPPROBE help text.

Signed-off-by: Grant Grundler [EMAIL PROTECTED]


--- a/net/Kconfig
+++ b/net/Kconfig
@@ -231,7 +231,7 @@ config NET_TCPPROBE
TCP congestion avoidance modules. If you don't understand
what was just said, you don't need it: say N.
 
-   Documentation on how to use the packet generator can be found
+   Documentation on how to use TCP connection probing can be found
at http://linux-net.osdl.org/index.php/TcpProbe
 
To compile this code as a module, choose M here: the
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/9] [TULIP] Clean tulip.h so it can be used by winbond-840.c

[Grant Grundler]

On Wed, Aug 09, 2006 at 01:33:18AM -0400, Jeff Garzik wrote:
 2) nobody (but parisc folks?) knows what CBMA and CBIO mean.  Just use 
 MMIO and PIO

CBIO is what's in the public documentation. I just want to make it
easy for anyone who bothers to read the documentation to be sure
they are reading about the right register.

thanks,
grant

-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [parisc-linux] [git patches] tulip fixes from parisc-linux

[Grant Grundler]

On Sat, Jul 29, 2006 at 06:54:59PM -0400, Kyle McMartin wrote:
 Hi Val,
 
 Sorry it took so long for me to get around to splitting
 up the changes from the parisc-linux tree. But here
 they finally are.

Kyle,
dude, you rock!
Many thanks for doing that.

I just wanted to warn that some of the changes are already in akpm
's tree (-mm).
Becuase off hand I've forgotten which ones, would it be better to
diff against -mm instead?

thanks,
grant
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [patch 1/2] tulip: fix for 64-bit mips

[Grant Grundler]

On Sun, Jun 25, 2006 at 10:31:08AM +0100, Martin Michlmayr wrote:
 * [EMAIL PROTECTED] [EMAIL PROTECTED] [2006-06-25 01:45]:
  Cc: Valerie Henson [EMAIL PROTECTED]
  
  [akpm: this is a previously-nacked patch, but the problem is real]

ia64 and parisc need the changes to tulip_select_media() too.


 And, for the record, Jeff's suggestion as to how to rework the patch:
 
 | Answer hasn't changed since this was last discussed:  sleep, rather than
 | delay for an extra-long time.  That's the only hurdle for the tulip
 | patches you keep resending.
 
 | Francois Romieu even had an untested patch that attempted this,
 | somewhere.
 
 Now we just need someone to complete this work...

Kyle McMartin has the work queue patch merged into the cvs.parisc-linux.org
CVS tree. It would be easiest to accept this patch and then add the
tulip work queue patch next.

Note that the work queue does NOT solve all of the spin delay with
interrupts blocked problems. tulip_stop_rxtx() polls for nearly as long
and tulip_select_media().

thanks,
grant

 -- 
 Martin Michlmayr
 [EMAIL PROTECTED]
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: PATCHv3 2.6.17-rc5 tulip free_irq() called too late

[Grant Grundler]

[ sorry, for the record I'm not trimming Val's comments - mine are much below]

On Wed, Jun 21, 2006 at 05:43:40PM -0700, Valerie Henson wrote:
...
 Hi folks,
 
 The quick summary of my thoughts on this patch is that it isn't the
 ideal patch, but it works and it's well-tested.  Doing my preferred
 fix (adding a shutdown flag) would be invasive and take many weeks to
 reach the level of stability of Grant's patch.  So I'm taking this
 patch but adding a comment describing my preferred fix.
 
 Here's the long version.  The obvious ordering of bringing up the card
 is:
 
 request_irq()
 setup DMA resources
 enable interrupts
 start DMA engine

The problem here is request_irq() enables interrupts.
SO the third step happens as part of the first step.

And if an IRQ happens to be pending, it will blow up.
But since we reset the chip at init time, that should
never happen.

 
 And the obvious ordering of shutting it down is:
 
 stop DMA engine
 disable interrupts
 remove DMA resources
 free_irq()

This attempts to be symetrical with the init sequence and 
I like that approach where possible. It's not symetrical
in practice because the interrupt code restarts DMA.
(which is what you've noted below)

 The problem with the above shutdown order is that we can receive an
 interrupt in between stopping DMA and disabling interrupts, and the
 tulip irq handler will reenable DMA == Boom!  The solution I prefer
 is to make the irq handler aware of whether we are shutting down or
 not, and not reenable DMA in that case.

While I believe this will work too, I don't advocate this approach
because I don't like to add code to interrupt handlers unless
it's the _only_ way to fix a problem. 

   However, it is a non-trivial
 patch to get right, and I would rather have the bug fixed short-term
 with the ordering Grant uses:
 
 disable interrupts
 free_irq()
 stop rxtx
 remove DMA resources
 
 Make sense to everyone?  I'll redo the patch with the comment and get
 Grant's sign-off.

Yes. I'm ok with anything similar to what you posted above:

Signed-off-by: Grant Grundler [EMAIL PROTECTED]


And Val, thanks for accepting the patch and taking the time to
explain where you want to go next with the code in this case.
I'll be happy to setup remote access to my test environment when
you are ready to test that next step.

thanks,
grant

 
 -VAL
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [openib-general] [PATCH v3 1/7] AMSO1100 Low Level Driver.

[Grant Grundler]

On Wed, Jun 21, 2006 at 11:32:51AM -0500, Steve Wise wrote:
 Um, what's a 'PCI posting flush'?  Can you point me where its
 described/used so I can see if we need it?  Thanx.

I've written this up before:
http://iou.parisc-linux.org/ols_2002/4Posted_vs_Non_Posted.html

grant
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: PATCHv3 2.6.17-rc5 tulip free_irq() called too late

[Grant Grundler]

On Fri, Jun 16, 2006 at 03:32:56AM -0400, Jeff Garzik wrote:
 Correct. Before calling free_irq(), patch V3 masks interrupts on tulip
 and guarantees the tulip will not generate new interrupts before that call.
 
 Incorrect -- it does not guarantee that tulip will not generate new 
 interrupt events.

Are you saying the following sequence doesn't mask tulip interrupts?

/* Disable interrupts by clearing the interrupt mask. */
iowrite32 (0x, ioaddr + CSR7);
ioread32 (ioaddr + CSR7);   /* flush posted write */

Secondly, since you've ignored the two previous times I've asked,
I'll assume you agree tulip driver has to (and does) handle
the case of pending, masked interrupts at initialization because
firmware might leave it in that state.

thanks,
grant
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: PATCHv3 2.6.17-rc5 tulip free_irq() called too late

[Grant Grundler]

[ Jeff, apologies. I hit reply instead of group reply on previous email.
  I've added everyone back on the cc list.]

On Fri, Jun 16, 2006 at 11:30:32AM -0400, Jeff Garzik wrote:
...
 Are you saying this sequence won't mask interrupts on tulip?
 
 /* Disable interrupts by clearing the interrupt mask. */
 iowrite32 (0x, ioaddr + CSR7);
 ioread32 (ioaddr + CSR7);   /* flush posted write */
 
 It does not stop the generation of interrupt events.

This use of interrupt events is misleading.
The CPU does not sees these interrupt events once we mask interrupts.

 The DMA engine is still running, packets are still being received.
 The above code sequence does not change that.

I agree. And I'm asking why does anyone care?
We clean that up after IRQs are stopped from being delivered to the CPU.

...
 Secondly, since you have ignored the two previous times I've asked,
 I'll presume you agree that if firmware leaves it in this state
 (pending, masked interrupts), that the driver has to (and does)
 handle it.
 
 There is no firmware involved here, at any level, after boot.

I agree.  What about at boot time?

 The needed task in the driver has been the same since this thread 
 started:  (1) stop generating new work [stop DMA engine], and (2) 
 quiesce the hardware.  And it must happen in that order.

No it doesn't. I've proven it works in the order I've proposed
on pretty damn anal HW.

 Setting the interrupt mask register to zero doesn't stop new work from 
 appearing.

I agree. It stops the screaming interrupt problem. The fact that we
are in close or down routine means the user already decided
they don't care if new packets do or do not arrive.

Unless you can point to a real user who is affected by
my proposed patch, I ask again patch v3 be accepted.

thanks,
grant
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: PATCHv3 2.6.17-rc5 tulip free_irq() called too late

[Grant Grundler]

On Thu, Jun 15, 2006 at 10:30:17PM +0200, Francois Romieu wrote:
 Afaik free_irq() on a shared irq does not touch the hardware and
 irqs are anything but synchronous. If free_irq() is issued before
 the device is idle and its irq are not acked, it's wrong.

Correct. Before calling free_irq(), patch V3 masks interrupts on tulip
and guarantees the tulip will not generate new interrupts before that call.

grant
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: PATCHv3 2.6.17-rc5 tulip free_irq() called too late

[Grant Grundler]

On Wed, Jun 14, 2006 at 09:05:06AM -0400, Kyle McMartin wrote:
 I think the correct sequence would be:
 
   reset tulip interrupt mask
   flush posted write
 
   synchronize irq /* make sure we got 'em all */

   tulip_stop_rxtx /* turn off dma */
   free irq/* bye bye */
 
 The synchronize irq guarantees we shouldn't see another irq
 generated by the card because it was held up somewhere.

Kyle,
syncronize_irq() only guarantees currently executing interrupt handler
completes before handing control back to the caller.
It does not guarantee IRQ signals still inflight are flushed.
Remember that IRQ lines are a sideband signal and not subject
to PCI data ordering rules.

thanks,
grant
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: PATCHv3 2.6.17-rc5 tulip free_irq() called too late

[Grant Grundler]

On Wed, Jun 14, 2006 at 11:03:48AM -0400, Jeff Garzik wrote:
 Grant Grundler wrote:
 Switching the order to be:
 tulip_stop_rxtx(tp);/* Stop DMA */
 free_irq (dev-irq, dev);   /* no more races after this */
 
 still leaves us open to IRQs being delivered _after_ we've stopped DMA.
 
 Correct.  And that is the preferred, natural, logical, obvious order:
 
 1) Turn things off.
 2) Wait for activity to cease.

Patch v3 does this in two stages:
1) turn off tulip interrupts
2) free_irq() calls syncronize_irq() to handle pending IRQs

then calls tulip_stop_rxtx() which:
1) tells tulip to stop DMA
2) poll until DMA completes

After this we can free remaining resources.

 That in turn allows the interrupt handler to re-enable DMA again.
 
 Then that would be a problem to solve...  Some interrupt handlers will 
 test netif_running() or a driver-specific shutting-down flag, 
 specifically to avoid such behaviors.

I'm not keen on adding more code to tulip_interrupt() routine
for something that rarely happens (compared to IRQs) and is handled
outside the interrupt routine.  I'm pretty sure stopping interrupts
before stopping DMA is sufficient.
Can you show an example where it doesn't work?

This is important since I'm going to propose a new Documentation/pci.txt
based on this experience.

thanks,
grant
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: PATCHv3 2.6.17-rc5 tulip free_irq() called too late

[Grant Grundler]

On Wed, Jun 14, 2006 at 10:47:20PM +0200, Francois Romieu wrote:
 Grant Grundler [EMAIL PROTECTED] :
 [...]
  I'm not keen on adding more code to tulip_interrupt() routine
  for something that rarely happens (compared to IRQs) and is handled
  outside the interrupt routine.  I'm pretty sure stopping interrupts
  before stopping DMA is sufficient.
  Can you show an example where it doesn't work?
 
 Shared irq. 
 
 The device has not quiesced, the kernel stop listening to it and the
 neighbor device receives a late interruption from the network device.

I thought we've worked through that already:
http://www.spinics.net/lists/netdev/msg05902.html

Patch v3 takes care of that problem.
The first step in the sequence is to mask IRQs on the tulip.
The neighbor device sharing the IRQ will not see any interrupts from
the tulip after that.

thanks,
grant
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: PATCHv3 2.6.17-rc5 tulip free_irq() called too late

[Grant Grundler]

On Thu, Jun 08, 2006 at 11:01:20AM -0600, Grant Grundler wrote:
 Here is a new patch that moves free_irq() into tulip_down().
 The resulting code is structured the same as cp_close().

Val,
Two details are wrong in version 2 and are fixed in v3 (appended below):

o we don't need synchronize_irq() before calling free_irq().
  (It should be removed from cp_close() too)
  Thanks to willy for pointing me at kernel/irq/manage.c.

o tulip_stop_rxtx() has to be called _after_ free_irq().
  ie. v2 patch didn't fix the original race condition
  and when under test, dies about as fast as the original code.

Tested on rx4640 (HP IA64) for several hours.
Please apply.

thanks,
grant

Change Log:
IRQs are racing with tulip_down(). DMA can be restarted _after_
we call tulip_stop_rxtx() and the DMA buffers are unmapped.
The result is an MCA (hard crash on ia64) because of an
IO TLB miss.

Signed-off-by: Grant Grundler [EMAIL PROTECTED]

--- a/drivers/net/tulip/tulip_core.c
+++ b/drivers/net/tulip/tulip_core.c
@@ -18,11 +18,11 @@
 
 #define DRV_NAME   tulip
 #ifdef CONFIG_TULIP_NAPI
-#define DRV_VERSION1.1.13-NAPI /* Keep at least for test */
+#define DRV_VERSION1.1.14-NAPI /* Keep at least for test */
 #else
-#define DRV_VERSION1.1.13
+#define DRV_VERSION1.1.14
 #endif
-#define DRV_RELDATEDecember 15, 2004
+#define DRV_RELDATEMay 6, 2006
 
 
 #include linux/module.h
@@ -741,21 +741,20 @@ static void tulip_down (struct net_devic
 
/* Disable interrupts by clearing the interrupt mask. */
iowrite32 (0x, ioaddr + CSR7);
+   ioread32 (ioaddr + CSR7);   /* flush posted write */
 
-   /* Stop the Tx and Rx processes. */
-   tulip_stop_rxtx(tp);
+   spin_unlock_irqrestore (tp-lock, flags);
 
-   /* prepare receive buffers */
-   tulip_refill_rx(dev);
+   free_irq (dev-irq, dev);   /* no more races after this */
+   tulip_stop_rxtx(tp);/* Stop DMA */
 
-   /* release any unconsumed transmit buffers */
-   tulip_clean_tx_ring(tp);
+   /* Put driver back into the state we start with */
+   tulip_refill_rx(dev);   /* prepare RX buffers */
+   tulip_clean_tx_ring(tp);/* clean up unsent TX buffers */
 
if (ioread32 (ioaddr + CSR6) != 0x)
tp-stats.rx_missed_errors += ioread32 (ioaddr + CSR8)  0x;
 
-   spin_unlock_irqrestore (tp-lock, flags);
-
init_timer(tp-timer);
tp-timer.data = (unsigned long)dev;
tp-timer.function = tulip_tbl[tp-chip_id].media_timer;
@@ -774,7 +773,6 @@ static int tulip_close (struct net_devic
printk (KERN_DEBUG %s: Shutting down ethercard, status was 
%2.2x.\n,
dev-name, ioread32 (ioaddr + CSR5));
 
-   free_irq (dev-irq, dev);
 
/* Free all the skbuffs in the Rx queue. */
for (i = 0; i  RX_RING_SIZE; i++) {
@@ -1752,7 +1752,6 @@ static int tulip_suspend (struct pci_dev
tulip_down(dev);
 
netif_device_detach(dev);
-   free_irq(dev-irq, dev);
 
pci_save_state(pdev);
pci_disable_device(pdev);
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: PATCHv3 2.6.17-rc5 tulip free_irq() called too late

[Grant Grundler]

On Tue, Jun 13, 2006 at 08:33:22PM -0400, Jeff Garzik wrote:
 Grant Grundler wrote:
 o tulip_stop_rxtx() has to be called _after_ free_irq().
   ie. v2 patch didn't fix the original race condition
   and when under test, dies about as fast as the original code.
 
 You made the race window smaller, but it's still there.  The chip's DMA 
 engines should be stopped before you unregister the interrupt handler.

Switching the order to be:
tulip_stop_rxtx(tp);/* Stop DMA */
free_irq (dev-irq, dev);   /* no more races after this */

still leaves us open to IRQs being delivered _after_ we've stopped DMA.
That in turn allows the interrupt handler to re-enable DMA again.

Or are you worried about a masked, pending interrupt causing
problems when the driver is re-opened or resumed?
If firmware left the device in a that state at boot time wouldn't
the driver be required to handle it?

thanks,
grant



 
   Jeff
 
 
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: PATCH 2.6.17-rc5 tulip free_irq() called too late

[Grant Grundler]

On Thu, Jun 08, 2006 at 10:43:04AM -0400, Jeff Garzik wrote:
 (CC'ing our newly minted tulip maintainer, Val)

Excellent!
Has MAINTAINERS file been updated? :)

...
 NAK.  This is a band-aid, and one that creates new problems even as it 
 attempts to solve problems.

You failed to demonstrate that below. Any other objections?

 Calling free_irq() while the chip is still active is just a bad idea, 
 because the chip could raise an interrupt, creating a 
 screaming-interrupts situation. Consider especially the case of shared 
 interrupts here, as a concrete example of how this won't work.

The chip IRQ gets turned off in tulip_down().
It won't be screaming for very long.
No one will hear it screaming and no one will care.
The decision to bring down the NIC was already made.

Secondly, if tulip has a problem with shared IRQs, it's already there.
We would have called free_irq() anyway - just a bit later.
In the shared IRQ case, I expect free_irq() to unlink this instance
of the tulip interrupt handler from the CPU vector list. If that
doesn't happen, I could consider that an arch specific bug,
not a tulip driver bug.

 Perhaps cp_close() in 8139cp.c could be an example of a good ordering? 
 It stops the chip, syncs irqs, frees irq, then frees [thus unmapping] 
 the rings.

Sorry, I don't see how it matters if we disable chip IRQ first
or unlink from CPU IRQ list first. Does it?

thanks,
grant
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: PATCH 2.6.17-rc5 tulip free_irq() called too late

[Grant Grundler]

On Thu, Jun 08, 2006 at 09:22:21AM -0600, Grant Grundler wrote:
  Perhaps cp_close() in 8139cp.c could be an example of a good ordering? 
  It stops the chip, syncs irqs, frees irq, then frees [thus unmapping] 
  the rings.
 
 Sorry, I don't see how it matters if we disable chip IRQ first
 or unlink from CPU IRQ list first. Does it?

Ok...I think I understand what you are driving at here.
The case is when CPU vector is enabled and shared but
one device _without_ an interrupt handler is registered
is still yanking on the interrupt line. It will cause
linux to disable the line since the IRQ isn't being handled.

This is only possible in the shared IRQ case.

Can we call free_irq() from tulip_down()?
I have the feeling this is going to cause alot more code movement.

thanks,
grant
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: PATCH 2.6.17-rc5 tulip free_irq() called too late

[Grant Grundler]

On Thu, Jun 08, 2006 at 11:32:39AM -0400, Jeff Garzik wrote:
 The chip IRQ gets turned off in tulip_down().
 It won't be screaming for very long.
 
 Then you admit that you add a race.

Yes - I realized that after I hit send :(

...
 In the shared IRQ case, I expect free_irq() to unlink this instance
 of the tulip interrupt handler from the CPU vector list. If that
 
 Irrelevant -- that doesn't stop the tulip hardware from raising the irq, 
 nor stop the system from attempting to deliver it [in all cases].  In 
 the shared interrupt case, that means that another driver's irq handler 
 will be called for an event tulip hardware raised.

*nod*

thanks,
grant
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: PATCH 2.6.17-rc5 tulip free_irq() called too late

[Grant Grundler]

On Thu, Jun 08, 2006 at 11:38:52AM -0400, Jeff Garzik wrote:
 Can we call free_irq() from tulip_down()?
 
 I'm sure you can answer that yourself.  If it doesn't cause problems 
 elsewhere, yes.  Otherwise, no.  :)

Yeah, well, I was hoping you would Just Know (tm). :)
Research takes time.

 Did you read the example I cited, cp_close() ?

Yes - after I sent the mail. It looks good to me too.

thanks,
grant
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: New tulip maintainer...

[Grant Grundler]

On Thu, Jun 08, 2006 at 11:41:28AM -0400, Jeff Garzik wrote:
 Now that we have a new tulip maintainer, perhaps a resend of the 
 long-outstanding tulip phy patches could be resent?

All the tulip patches I have are archived here:
ftp://ftp.parisc-linux.org/patches/

Anything else tulip I've been involved with is available
via CVS from cvs.parisc-linux.org/linux-2.6/

Thibaut Varene has also committed his changes to that CVS.

thanks,
grant
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: PATCH 2.6.17-rc5 tulip free_irq() called too late

[Grant Grundler]

On Thu, Jun 08, 2006 at 10:43:04AM -0400, Jeff Garzik wrote:
...
 Perhaps cp_close() in 8139cp.c could be an example of a good ordering? 
 It stops the chip, syncs irqs, frees irq, then frees [thus unmapping] 
 the rings.

Here is a new patch that moves free_irq() into tulip_down().
The resulting code is structured the same as cp_close().

While I believe the code is correct, I'm not real happy
that tulip_up() and tulip_down() don't both deal with IRQs.
(ie not symetrical). Oh well, I can submit another patch
for that if Val doesn't want to wrangle that herself.

This is compile tested only.
I'll drop it on ia64/parisc servers later and retest.

thanks,
grant


Index: drivers/net/tulip/tulip_core.c
===
RCS file: /var/cvs/linux-2.6/drivers/net/tulip/tulip_core.c,v
retrieving revision 1.35
diff -u -p -r1.35 tulip_core.c
--- drivers/net/tulip/tulip_core.c  23 Apr 2006 15:18:28 -  1.35
+++ drivers/net/tulip/tulip_core.c  8 Jun 2006 16:25:43 -
@@ -18,11 +18,11 @@
 
 #define DRV_NAME   tulip
 #ifdef CONFIG_TULIP_NAPI
-#define DRV_VERSION1.1.13-NAPI /* Keep at least for test */
+#define DRV_VERSION1.1.14-NAPI /* Keep at least for test */
 #else
-#define DRV_VERSION1.1.13
+#define DRV_VERSION1.1.14
 #endif
-#define DRV_RELDATEDecember 15, 2004
+#define DRV_RELDATEMay 6, 2006
 
 
 #include linux/module.h
@@ -743,6 +745,11 @@ static void tulip_down (struct net_devic
/* Stop the Tx and Rx processes. */
tulip_stop_rxtx(tp);
 
+   spin_unlock_irqrestore (tp-lock, flags);
+
+   synchronize_irq(dev-irq);
+   free_irq (dev-irq, dev);
+
/* prepare receive buffers */
tulip_refill_rx(dev);
 
@@ -752,7 +759,6 @@ static void tulip_down (struct net_devic
if (ioread32 (ioaddr + CSR6) != 0x)
tp-stats.rx_missed_errors += ioread32 (ioaddr + CSR8)  0x;
 
-   spin_unlock_irqrestore (tp-lock, flags);
 
init_timer(tp-timer);
tp-timer.data = (unsigned long)dev;
@@ -774,14 +780,13 @@ static int tulip_close (struct net_devic
int i;
 
netif_stop_queue (dev);
 
tulip_down (dev);
 
if (tulip_debug  1)
printk (KERN_DEBUG %s: Shutting down ethercard, status was 
%2.2x.\n,
dev-name, ioread32 (ioaddr + CSR5));
 
-   free_irq (dev-irq, dev);
 
/* Free all the skbuffs in the Rx queue. */
for (i = 0; i  RX_RING_SIZE; i++) {
@@ -1750,7 +1757,6 @@ static int tulip_suspend (struct pci_dev
tulip_down(dev);
 
netif_device_detach(dev);
-   free_irq(dev-irq, dev);
 
pci_save_state(pdev);
pci_disable_device(pdev);
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

PATCH 2.6.17-rc5 tulip free_irq() called too late

[Grant Grundler]

Jeff,
SLES10 testing exposed an MCA that was confirmed to be a DMA IO TLB miss.
This means tulip device was attempting to DMA to memory that was already
unmapped. The test was crashing in the ifconfig down step when a 4-port
tulip card was under this work load:

while :
do
ifconfig eth24 up
ifconfig eth25 up
ifconfig eth26 up
ifconfig eth27 up
# Pound both interfaces with ethtool
for i in `seq 1000`
do
ethtool eth24 /dev/null
ethtool eth25 /dev/null
ethtool eth26 /dev/null
ethtool eth27 /dev/null
done

# Bring interfaces down
echo ifconfig $nic1 down
ifconfig eth24 down
ifconfig eth25 down
ifconfig eth26 down
ifconfig eth27 down

sleep 5
done


[ And yes, I know tulip doesn't support ethtool. Don't ask.
  It's still a sore point at the moment. Just consider it 
  a delay loop or use sleep 5 instead. ]

The real network load comes from another box(en) running 4 instances
of ping -f -s 1450 192.168.x.y where x.y is the subnet/IP of eth24-27.
The parisc and ia64 machines will crash in minutes.

I believe the problem is a race condition between an interrupt coming
in and the tulip_down() code path. Moving the free_irq() to before
tulip_down() call fixes the problem. I've been able to run the above
test for several hours now.
Please apply.

thanks,
grant


Signed-off-by: Grant Grundler [EMAIL PROTECTED]

--- a/drivers/net/tulip/tulip_core.c
+++ b/drivers/net/tulip/tulip_core.c
@@ -18,11 +18,11 @@
 
 #define DRV_NAME   tulip
 #ifdef CONFIG_TULIP_NAPI
-#define DRV_VERSION1.1.13-NAPI /* Keep at least for test */
+#define DRV_VERSION1.1.14-NAPI /* Keep at least for test */
 #else
-#define DRV_VERSION1.1.13
+#define DRV_VERSION1.1.14
 #endif
-#define DRV_RELDATEDecember 15, 2004
+#define DRV_RELDATEMay 31, 2006
 
 
 #include linux/module.h
@@ -772,14 +774,13 @@ static int tulip_close (struct net_devic
int i;
 
netif_stop_queue (dev);
-
+   free_irq (dev-irq, dev);/* don't let IRQs race w/tulip_down() */
tulip_down (dev);
 
if (tulip_debug  1)
printk (KERN_DEBUG %s: Shutting down ethercard, status was 
%2.2x.\n,
dev-name, ioread32 (ioaddr + CSR5));
 
-   free_irq (dev-irq, dev);
 
/* Free all the skbuffs in the Rx queue. */
for (i = 0; i  RX_RING_SIZE; i++) {
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [openib-general] [PATCH 5/5] [RFC] Infiniband: connection abstraction

[Grant Grundler]

On Wed, Jan 18, 2006 at 10:19:01AM -0800, Sean Hefty wrote:
 Roland Dreier wrote:
   + UCMA_MAX_BACKLOG= 128
 
 Is there any reason that we might want to make this a tunable?  Maybe
 as a module parameter that's writable in sysfs...
 
 There's no reason not to make this tunable.

Yes, there are reasons to NOT make something a tunable:
o increases system complexity (admin)
o increases the amount of documentation (learning curve)
o increases test matrix/cost (devel/support cost)
o generally hurts performance (var vs a constant of the same value)

Any reason to make something a tunable has to compensate
for the above drawbacks. An answer to Roland's question
is a reasonable prerequisite if someone wants add a tunable.

IB doesn't have the much in /sys/class/infiniband* or module parameters
and I think that's a Good Thing.

grant
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [openib-general] RE: [PATCH 2/5] [RFC] Infiniband: connection abstraction

[Grant Grundler]

On Tue, Jan 17, 2006 at 03:24:37PM -0800, Sean Hefty wrote:
 +static void cm_mask_compare_data(u8 *dst, u8 *src, u8 *mask)
 +{
 + int i;
 +
 + for (i = 0; i  IB_CM_PRIVATE_DATA_COMPARE_SIZE; i++)
 + dst[i] = src[i]  mask[i];
 +}

Is this code going to get invoked very often?

If so, can the mask operation use a native size since
IB_CM_PRIVATE_DATA_COMPARE_SIZE is hard coded to 64 byte?

e.g something like:
for (i = 0; i  IB_CM_PRIVATE_DATA_COMPARE_SIZE/sizeof(unsigned long);
i++)
((unsigned long *)dst)[i] = ((unsigned long *)src)[i] 
 ((unsigned long *)mask)[i];

thanks,
grant

 +
 +static int cm_compare_data(struct ib_cm_private_data_compare *src_data,
 +struct ib_cm_private_data_compare *dst_data)
 +{
 + u8 src[IB_CM_PRIVATE_DATA_COMPARE_SIZE];
 + u8 dst[IB_CM_PRIVATE_DATA_COMPARE_SIZE];
 +
 + if (!src_data || !dst_data)
 + return 0;
 + 
 + cm_mask_compare_data(src, src_data-data, dst_data-mask);
 + cm_mask_compare_data(dst, dst_data-data, src_data-mask);
 + return memcmp(src, dst, IB_CM_PRIVATE_DATA_COMPARE_SIZE);
 +}
 +
 +static int cm_compare_private_data(u8 *private_data,
 +struct ib_cm_private_data_compare *dst_data)
 +{
 + u8 src[IB_CM_PRIVATE_DATA_COMPARE_SIZE];
 +
 + if (!dst_data)
 + return 0;
 + 
 + cm_mask_compare_data(src, private_data, dst_data-mask);
 + return memcmp(src, dst_data-data, IB_CM_PRIVATE_DATA_COMPARE_SIZE);
 +}
 +
  static struct cm_id_private * cm_insert_listen(struct cm_id_private 
 *cm_id_priv)
  {
   struct rb_node **link = cm.listen_service_table.rb_node;
 @@ -362,14 +397,18 @@ static struct cm_id_private * cm_insert_
   struct cm_id_private *cur_cm_id_priv;
   __be64 service_id = cm_id_priv-id.service_id;
   __be64 service_mask = cm_id_priv-id.service_mask;
 + int data_cmp;
  
   while (*link) {
   parent = *link;
   cur_cm_id_priv = rb_entry(parent, struct cm_id_private,
 service_node);
 + data_cmp = cm_compare_data(cm_id_priv-compare_data,
 +cur_cm_id_priv-compare_data);
   if ((cur_cm_id_priv-id.service_mask  service_id) ==
   (service_mask  cur_cm_id_priv-id.service_id) 
 - (cm_id_priv-id.device == cur_cm_id_priv-id.device))
 + (cm_id_priv-id.device == cur_cm_id_priv-id.device) 
 + !data_cmp)
   return cur_cm_id_priv;
  
   if (cm_id_priv-id.device  cur_cm_id_priv-id.device)
 @@ -378,6 +417,10 @@ static struct cm_id_private * cm_insert_
   link = (*link)-rb_right;
   else if (service_id  cur_cm_id_priv-id.service_id)
   link = (*link)-rb_left;
 + else if (service_id  cur_cm_id_priv-id.service_id)
 + link = (*link)-rb_right;
 + else if (data_cmp  0)
 + link = (*link)-rb_left;
   else
   link = (*link)-rb_right;
   }
 @@ -387,16 +430,20 @@ static struct cm_id_private * cm_insert_
  }
  
  static struct cm_id_private * cm_find_listen(struct ib_device *device,
 -  __be64 service_id)
 +  __be64 service_id,
 +  u8 *private_data)
  {
   struct rb_node *node = cm.listen_service_table.rb_node;
   struct cm_id_private *cm_id_priv;
 + int data_cmp;
  
   while (node) {
   cm_id_priv = rb_entry(node, struct cm_id_private, service_node);
 + data_cmp = cm_compare_private_data(private_data,
 +cm_id_priv-compare_data);
   if ((cm_id_priv-id.service_mask  service_id) ==
cm_id_priv-id.service_id 
 - (cm_id_priv-id.device == device))
 + (cm_id_priv-id.device == device)  !data_cmp)
   return cm_id_priv;
  
   if (device  cm_id_priv-id.device)
 @@ -405,6 +452,10 @@ static struct cm_id_private * cm_find_li
   node = node-rb_right;
   else if (service_id  cm_id_priv-id.service_id)
   node = node-rb_left;
 + else if (service_id  cm_id_priv-id.service_id)
 + node = node-rb_right;
 + else if (data_cmp  0)
 + node = node-rb_left;
   else
   node = node-rb_right;
   }
 @@ -728,15 +779,14 @@ retest:
   wait_event(cm_id_priv-wait, !atomic_read(cm_id_priv-refcount));
   while ((work = cm_dequeue_work(cm_id_priv)) != NULL)
   cm_free_work(work);
 - 

Re: Resend [PATCH netdev-2.6 2/8] e1000: Performance Enhancements

[Grant Grundler]

On Wed, Dec 07, 2005 at 07:41:29AM -0500, jamal wrote:
  ok - that's fair. I suspect the hyperthreading case is one where
  binding the IRQs to particule CPUs is necessary to reproduce
  the results.
  
 
 
 Note: I didnt bind anything. The p4/xeon starts with routing everything
 to CPU#0 - i just left it like that. I am taking it this is what you
 were asking for earlier?

Yes. In general, one gets better cache utilization when binding
IRQs to a single CPU and often translates into better overall
performance. It's also useful for benchmarking - reproducible results.

   One could draw the conclusion that copybreak is good or prefetch is bad.
  
  Like Robert, I conclude that both helped in this case.
 
 eh? Are we looking at the same results? Robert's conclusion: copybreak
 _bad_, _some_ prefetch good.

Robert commented on *your* results - I only agreed with him.

 Mine so far is inconclusive although one
 could almost say copybreak good - which is the opposite of what Robert
 concluded.

Yes - his results indicated copybreak hurt perf on the AMD box.
Another reason for the e1000 maintainer to enable copybreak. ;)

grant
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Resend [PATCH netdev-2.6 2/8] e1000: Performance Enhancements

[Grant Grundler]

On Wed, Dec 07, 2005 at 02:17:16PM -0500, jamal wrote:
...
 Note, however that as soon as i turn copybreak off, the numbers go
 down ;-
 
 Now for some obtuse theory as to why this happens:
 I think the reason that prefetch + copybreak together have higher
 numbers is because the copybreak code provides a cycle-mask that makes
 the prefetches useful.

I would argue the other way around. copybreak would stall and hurt
small packet routing performance if there was no prefetching.
With agressive prefetching, copybreak takes advantage of data that
is already in flight or in cache. Ideally, the prefetch should be
for whatever number of cachelines the copybreak is going to touch
(bounded by architectural limits and other reasonable criteria).


  i.e if you used X cycles more before using the
 prefetched data, you will end up benefiting from the prefetch. It doesnt
 matter if the code was doing copybreak or just looped killing a few
 cycles. so copybreak is just a useless cycle chewer.

I think that ignores the benefits Dave Miller pointed out with
the socket buffer utilization. Copybreak is cheap once
the data is prefetched.

 And this is why ***prefetching is dangerous***.
 
 To prove this theory i am gonna run tests that add a simple loop which
 counts down from some number X to 0. I will try to increment X
 algorithmically and see where it becomes useful etc.

Please try using udelay(). That will tell you exactly how much
stall in wallclock time the CPU is experiencing anyway because
the packet data isn't in cache/register yet.

I totally agree there is an interaction between copybreak and
prefetching. I'm thinking more prefetching means copybreak be
bumped up higher too. This gets us back to the prefetching
is arch specific optimization I raised near the beginning of
this thread.

thanks,
grant
 
 Hopefully i can do this quickly before i get pulled
  
 cheers,
 jamal
 
 
 
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Resend [PATCH netdev-2.6 2/8] e1000: Performance Enhancements

[Grant Grundler]

On Sat, Dec 03, 2005 at 02:37:59PM -0500, jamal wrote:
 On Sat, 2005-03-12 at 12:00 -0700, Grant Grundler wrote:
  On Sat, Dec 03, 2005 at 09:20:52AM -0500, jamal wrote:
   Ok, so you seem to be saying again that for case #b above, there is no
   harm in issuing the prefetch late since the CPU wont issue a second
   fetch for the address? 
  
  Right.
  
  (When that's not true, add to cost of issueing a prefetch.)
 
 Can we verify if this is the case _always_ or it is the case of some
 architectures? It does seem like an obvious optimization ..

TBH, I'm not interested in a fishing expedition right now.
When someone presents a particular prefetch which is measurably
hurting performance, then we can look at that implementation to
see if the cache controller is issuing multiple requests for the
same cacheline (or not).


 There is also a third case:
 c) prefetch something you will never use. Example in the driver
 prefetching the next descriptor which might never be used because the
 driver exceeds its quota of packets by the time it gets to the packet.

sure - false positives are one of the details of the cost of prefetch.

  I agree with Dave Miller - we should disable the prefetch at compile time
  for the CPU/platform combinations we know don't benefit from it.
  It has to be on a case-by-case basis.
 
 I dont think Dave wants to disable at compile time ;-

Sorry - you are right. He wanted to defer the discussion until
we had a real example(s) where the prefetching hurts performance.
I'll leave it at that too.

thanks,
grant
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Resend [PATCH netdev-2.6 2/8] e1000: Performance Enhancements

[Grant Grundler]

On Thu, Dec 01, 2005 at 09:32:37PM -0500, jamal wrote:
  I think until a counter case is shown, the prefetches should
  remain on unconditionally.  Proof of detriment is the burdon
  of the accusor, especially since the Intel folks aparently
  did a lot of testing :-)

We've already been down this path before. How and where to prefetch
is quite dependent on the CPU implementation and workload.

 So let me take the liberty of first cc-ing all the usual suspects i have
 talked to about this in the past when i attempted to use prefetching on
 e1000.

I'm just an innocent bystander. Really. :)

 Then i will try to make a case and eventually collect some newer
 evidence ;- I will be happy actually if it turns out i am wrong since i
 would like to squueze as much juice out of them processors.
 
 Background:
 --
 
 After mucking with prefetching for some time a while back and observing
 inconsistent behavior, I came across a patch from David Mosberger (BTW,
 not sure if his email address above is correct - maybe someone could
 forward him this discussion).

I've cc'd him: David Mosberger-Tang [EMAIL PROTECTED]

  With this specific patch (attached), in
 all the Xeons i tested on (~2 years back) there was consistent
 improvement of anywhere between 5-10Kpps in the forwarding tests if i
 turned only in the AGGRESSIVE mode. Nothing useful when i used the other
 mode.
 On P3 class hardware prefetching without aggressive mode resulted in
 decreased throughput but also decreased CPU occupancy. No effect with
 aggressive mode. So the overall effect was it was not harmful to have
 aggressive mode on. But i am not sure i even trust that would have been
 the case in every piece of hardware i could find.

At the time you did this, I read the Intel docs on P3 and P4 cache
behaviors. IIRC, the P4 HW prefetches very aggressively. ie the SW
prefetching just becomes noise or burns extra CPU cycles. My guess
was the x86 HW designers had exactly the opposite perspective than
the RISC designers had: x86 SW won't change - ie can't add SW controlled
prefetching to improvement perf - therefore HW has to do it.

I don't know if that's still true or how aggressive the AMD64 CPUs
are at prefetching.

PA-RISC and IA64 processors can execute the SW prefetching in parallel.
ie they essentially only cost backplane/memory controller bandwidth.
There is a trade off between being too smart about when to prefetch
and being too stupid. I mean we shouldn't make complex calculations
to figure out if we need to prefetch something or not unless we know
exactly what the workload is.

In the case of David Mosberger's patch, we *know* 100% of the time we
are going to look at the first cacheline (packet header) after the
DMA completes. IMHO, all RISC architectures should prefetch that
once the DMA is unmapped.

BTW, my interest in all this is because NIC DMA causes the system chipset
to take ownership of the cachelines away from the CPU. The CPU has to
re-acquire a copy of the cacheline when looking at NIC TX/RX descriptor
rings and the payload data buffers. This is a big deal for the small
packet routing/forwarding that Robert Olsen and Jamal are using as a
primary workload/metric.

[BTW2, if the NIC could say I'm done with this cacheline, now give it
to CPU X where X is the target of MSI, I think we see some dramatic
gains too in the control data handling too.]

 Lennert and I as well sweated over testing some simple kernel modules
 with very inconsistent results if he used his laptop or i used mine.
 
 I will test with a newer piece of hardware and one of the older ones i
 have (both Xeons) - perhaps this weekend.
 Robert may have some results perhaps on this driver, Robert?
 It would also be nice for the intel folks to post their full results
 somewhere.

Yeah, I'd like to see that too.

 Having said all the above:
 --
 
 The danger of prefetching is the close dependence on the stride (or
 load-latency as David Mosberger would call it). 

stride != load-latency. stride is the increment used to prefetch the
next cacheline when walking through memory.

Load-latency is how long the CPU needs to acquire a current copy (or
ownership) of the cacheline. The SW prefetching is intended to hide
load latency.  Ideally the stride is equal to the IO cacheline size.
Of course, RX/TX descriptors aren't laid out by cacheline size
and I'm not advocating we want them to be. We might issue a prefetch
for the same cacheline multiple times and depending on architecture,
that might be another source of wasted CPU cycles.

BTW, IO Cacheline size is typically the same as the outermost cache
of the CPU - ie whatever the basic unit of cache coherency is for the
platform.  It is often bigger than the L1 cacheline size.

 If somehow the amount of CPU cycles executed to where the data is really
 used happens to be less than the amount of time it would take to fetch
 the line into cache, then it is useless and maybe detrimental. 

That's not quite 

Re: [openib-general] error compiling kernel...

[Grant Grundler]

On Mon, Nov 07, 2005 at 11:19:41PM -0500, Sayantan Sur wrote:
...
 Has anyone been able to compile gen2 with 2.6.14?

Yes - but I've run into a different problem.
After fixing up include/rdma to point at drivers/infiniband/include/rdma,
I get a symbol missing from from the modules.


iota:/usr/src/linux-2.6.14# make modules_install
...
if [ -r System.map -a -x /sbin/depmod ]; then /sbin/depmod -ae -F System.map  2.
6.14; fi
WARNING: /lib/modules/2.6.14/kernel/drivers/infiniband/ulp/sdp/ib_sdp.ko needs u
nknown symbol ip_dev_find   
WARNING: /lib/modules/2.6.14/kernel/drivers/infiniband/core/ib_at.ko needs unkno
wn symbol ip_dev_find   
WARNING: /lib/modules/2.6.14/kernel/drivers/infiniband/core/ib_addr.ko needs unk
nown symbol ip_dev_find 
iota:/usr/src/linux-2.6.14#

ip_dev_find() is not exported by net/ipv4/fib_frontend.c.

However, drivers/infiniband is the only module that needs this.
CONFIG_IP_MROUTE is another configurable user but cannot be enabled
as a module.

Patch below adds EXPORT_SYMBOL() to fib_frontend.c.
I'm not trying to assert this is the Right Thing.
It's just the first obvious solution to the immediate problem.

thanks,
grant

Signed-off-by: Grant Grundler [EMAIL PROTECTED]

--- linux-2.6.14-ORIG/net/ipv4/fib_frontend.c   2005-10-27 17:02:08.0 
-0700
+++ linux-2.6.14/net/ipv4/fib_frontend.c2005-11-07 21:29:22.0 
-0800
@@ -662,3 +662,4 @@ void __init ip_fib_init(void)
 
 EXPORT_SYMBOL(inet_addr_type);
 EXPORT_SYMBOL(ip_rt_ioctl);
+EXPORT_SYMBOL(ip_dev_find);
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [openib-general] error compiling kernel...

[Grant Grundler]

On Mon, Nov 07, 2005 at 09:59:49PM -0800, Grant Grundler wrote:
 WARNING: /lib/modules/2.6.14/kernel/drivers/infiniband/ulp/sdp/ib_sdp.ko 
 needs unknown symbol ip_dev_find  
  
 Patch below adds EXPORT_SYMBOL() to fib_frontend.c.

...never mind. Johann George just pointed out someone already has
added this diff to the openib.org repository:

https://openib.org/svn/gen2/trunk/src/linux-kernel/patches/linux-2.6.14-fib-frontend.diff

sorry,
grant
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html