CHROMIUM: config: enable usb net drivers (dm9601/net1080/rtl... [chromiumos/third

Mike Frysinger (Code Review)

unread,

Nov 2, 2012, 5:27:36 PM11/2/12

to Olof Johansson, Mandeep Singh Baines, Mike Frysinger

Hello Olof Johansson,

I'd like you to do a code review. Please visit

https://gerrit.chromium.org/gerrit/37296

to review the following change.

Change subject: CHROMIUM: config: enable usb net drivers (dm9601/net1080/rtl8150) for all targets
......................................................................

CHROMIUM: config: enable usb net drivers (dm9601/net1080/rtl8150) for all targets

Some configs had these drivers enabled while others did not. Update the
common arch configs to all enable these drivers.

BUG=chromium-os:35943
TEST=`emerge-daisy chromeos-kernel` included these three drivers
TEST=`emerge-stumpy chromeos-kernel` included these three drivers
TEST=`emerge-x86-alex chromeos-kernel` included these three drivers

Change-Id: I9615109a5b17af293855595ba8f06f397db360c4
Signed-off-by: Mike Frysinger <vap...@chromium.org>
---
M chromeos/config/armel/common.config
M chromeos/config/i386/chromeos-pinetrail-i386.flavour.config
M chromeos/config/i386/chromiumos-i386.flavour.config
M chromeos/config/i386/common.config
M chromeos/config/x86_64/common.config
5 files changed, 5 insertions(+), 6 deletions(-)

git pull ssh://gerrit.chromium.org:29418/chromiumos/third_party/kernel refs/changes/96/37296/1
--
To view, visit https://gerrit.chromium.org/gerrit/37296
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I9615109a5b17af293855595ba8f06f397db360c4
Gerrit-PatchSet: 1
Gerrit-Project: chromiumos/third_party/kernel
Gerrit-Branch: chromeos-3.4
Gerrit-Owner: Mike Frysinger <vap...@chromium.org>
Gerrit-Reviewer: Olof Johansson <ol...@chromium.org>

Jon Kliegman (Code Review)

unread,

Nov 8, 2012, 3:48:29 PM11/8/12

to Mike Frysinger, Olof Johansson, Doug Anderson

Jon Kliegman has posted comments on this change.

Change subject: CHROMIUM: config: enable usb net drivers (dm9601/net1080/rtl8150) for all targets
......................................................................

Patch Set 1: Looks good to me, but someone else must approve

LGTM but want to give Olof a chance to look at this. I think our overall policy lately has been to just turn on as modules drivers needed on any platform but not 100%.

--
To view, visit https://gerrit.chromium.org/gerrit/37296
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment

Gerrit-Change-Id: I9615109a5b17af293855595ba8f06f397db360c4
Gerrit-PatchSet: 1
Gerrit-Project: chromiumos/third_party/kernel
Gerrit-Branch: chromeos-3.4
Gerrit-Owner: Mike Frysinger <vap...@chromium.org>

Gerrit-Reviewer: Doug Anderson <dian...@chromium.org>
Gerrit-Reviewer: Jon Kliegman <kli...@chromium.org>
Gerrit-Reviewer: Olof Johansson <ol...@chromium.org>

Doug Anderson (Code Review)

unread,

Nov 9, 2012, 8:48:52 PM11/9/12

to Mike Frysinger, Olof Johansson, Jon Kliegman

Doug Anderson has posted comments on this change.

Change subject: CHROMIUM: config: enable usb net drivers (dm9601/net1080/rtl8150) for all targets
......................................................................

Patch Set 1: I would prefer that you didn't submit this

Please normalize.

./chromeos/scripts/kernelconfig oldconfig

--
To view, visit https://gerrit.chromium.org/gerrit/37296
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment

Gerrit-Change-Id: I9615109a5b17af293855595ba8f06f397db360c4
Gerrit-PatchSet: 1
Gerrit-Project: chromiumos/third_party/kernel
Gerrit-Branch: chromeos-3.4
Gerrit-Owner: Mike Frysinger <vap...@chromium.org>

Mike Frysinger (Code Review)

unread,

Nov 9, 2012, 11:58:45 PM11/9/12

to Mike Frysinger, Olof Johansson, Doug Anderson, Jon Kliegman

Mike Frysinger has posted comments on this change.

Change subject: CHROMIUM: config: enable usb net drivers (dm9601/net1080/rtl8150) for all targets
......................................................................

Patch Set 1:

after i run that, it does introduce changes, but none of them are related to this CL. it produces the same diff with or without my changes. are you sure you want that unrelated noise in here ?

--
To view, visit https://gerrit.chromium.org/gerrit/37296
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment

Gerrit-Change-Id: I9615109a5b17af293855595ba8f06f397db360c4
Gerrit-PatchSet: 1
Gerrit-Project: chromiumos/third_party/kernel
Gerrit-Branch: chromeos-3.4
Gerrit-Owner: Mike Frysinger <vap...@chromium.org>

Gerrit-Reviewer: Doug Anderson <dian...@chromium.org>
Gerrit-Reviewer: Jon Kliegman <kli...@chromium.org>

Gerrit-Reviewer: Mike Frysinger <vap...@chromium.org>
Gerrit-Reviewer: Olof Johansson <ol...@chromium.org>

Doug Anderson (Code Review)

unread,

Nov 10, 2012, 12:05:10 AM11/10/12

to Mike Frysinger, Olof Johansson, Jon Kliegman

Doug Anderson has posted comments on this change.

Change subject: CHROMIUM: config: enable usb net drivers (dm9601/net1080/rtl8150) for all targets
......................................................................

Patch Set 1:

No, I don't want that noise. ...but this CL itself is not normalized.

* Start with m/master
* Normalize
* Commit. Use message like "Normalize splitconfig". Like this: <https://gerrit.chromium.org/gerrit/#/c/20497/>.
* Apply this CL.
* Normalize again.
* "git commit --amend" to normalize this CL.
* Upload

Specifically, if you're making USB net changes the same across all platforms I'd expect it to end up in the base.config.

--
To view, visit https://gerrit.chromium.org/gerrit/37296
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment

Gerrit-Change-Id: I9615109a5b17af293855595ba8f06f397db360c4
Gerrit-PatchSet: 1
Gerrit-Project: chromiumos/third_party/kernel
Gerrit-Branch: chromeos-3.4
Gerrit-Owner: Mike Frysinger <vap...@chromium.org>

Gerrit-Reviewer: Doug Anderson <dian...@chromium.org>
Gerrit-Reviewer: Jon Kliegman <kli...@chromium.org>

Gerrit-Reviewer: Mike Frysinger <vap...@chromium.org>
Gerrit-Reviewer: Olof Johansson <ol...@chromium.org>

Mike Frysinger (Code Review)

unread,

Nov 10, 2012, 1:31:10 AM11/10/12

to Mike Frysinger, Olof Johansson, Doug Anderson, Jon Kliegman, Gerrit

Mike Frysinger has posted comments on this change.

Change subject: CHROMIUM: config: enable usb net drivers (dm9601/net1080/rtl8150) for all targets
......................................................................

Patch Set 3: Verified

i had to run it a few times to get it to settle down. i think this is what you want.

--
To view, visit https://gerrit.chromium.org/gerrit/37296
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I9615109a5b17af293855595ba8f06f397db360c4
Gerrit-PatchSet: 3

Gerrit-Project: chromiumos/third_party/kernel
Gerrit-Branch: chromeos-3.4
Gerrit-Owner: Mike Frysinger <vap...@chromium.org>

Gerrit-Reviewer: Doug Anderson <dian...@chromium.org>
Gerrit-Reviewer: Gerrit <chrom...@google.com>
Gerrit-Reviewer: Jon Kliegman <kli...@chromium.org>
Gerrit-Reviewer: Mike Frysinger <vap...@chromium.org>
Gerrit-Reviewer: Olof Johansson <ol...@chromium.org>

Doug Anderson (Code Review)

unread,

Nov 11, 2012, 5:51:30 PM11/11/12

to Mike Frysinger, Olof Johansson, Jon Kliegman, Gerrit

Doug Anderson has posted comments on this change.

Change subject: CHROMIUM: config: enable usb net drivers (dm9601/net1080/rtl8150) for all targets
......................................................................

Patch Set 3: I would prefer that you didn't submit this

Yup, this is just what I was looking for. Thanks!

* For NET1080 and DM9601: no problem. If we include these modules for x86 then I agree that we should include them for ARM and have no problem giving the +2.
* For RTL8150: this one is a little different because it was only in the Chromium OS config. I believe that we are less picky about what we put in there since that doesn't get used with shipping products. By moving it you're adding a new driver for shipping x86 (and ARM) products. See the CL where it was purposely added only to the Chromium OS config <https://gerrit.chromium.org/gerrit/#/c/2599/>.

Are there common USB ethernet adapters that use the RTL8150 driver? Do we think the driver quality is good enough that we want users to be using it?

Marking -1 until sufficient justification for RTL8150. ...if Olof (or someone else) wants to +2 I have no problem with that, though.

--
To view, visit https://gerrit.chromium.org/gerrit/37296
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I9615109a5b17af293855595ba8f06f397db360c4
Gerrit-PatchSet: 3

Gerrit-Project: chromiumos/third_party/kernel
Gerrit-Branch: chromeos-3.4
Gerrit-Owner: Mike Frysinger <vap...@chromium.org>

Gerrit-Reviewer: Doug Anderson <dian...@chromium.org>
Gerrit-Reviewer: Gerrit <chrom...@google.com>
Gerrit-Reviewer: Jon Kliegman <kli...@chromium.org>

Gerrit-Reviewer: Mike Frysinger <vap...@chromium.org>
Gerrit-Reviewer: Olof Johansson <ol...@chromium.org>

Mike Frysinger (Code Review)

unread,

Nov 11, 2012, 6:30:34 PM11/11/12

to Mike Frysinger, Olof Johansson, Doug Anderson, Jon Kliegman, Gerrit, Grant Grundler

Mike Frysinger has posted comments on this change.

Change subject: CHROMIUM: config: enable usb net drivers (dm9601/net1080/rtl8150) for all targets
......................................................................

Patch Set 3:

when it comes to USB dongles, i'm not sure the barrier should be high. it's not like we're auditing any of them for bugs, nor do we test really any of them ourselves. for users who plug in a device, they just see that either it works or it doesn't. if a driver is semi-stable, working most of the time is a better experience than not at all (because we didn't bother including the driver).

--
To view, visit https://gerrit.chromium.org/gerrit/37296
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I9615109a5b17af293855595ba8f06f397db360c4
Gerrit-PatchSet: 3

Gerrit-Project: chromiumos/third_party/kernel
Gerrit-Branch: chromeos-3.4
Gerrit-Owner: Mike Frysinger <vap...@chromium.org>

Gerrit-Reviewer: Doug Anderson <dian...@chromium.org>
Gerrit-Reviewer: Gerrit <chrom...@google.com>

Gerrit-Reviewer: Grant Grundler <grun...@chromium.org>
Gerrit-Reviewer: Jon Kliegman <kli...@chromium.org>
Gerrit-Reviewer: Mike Frysinger <vap...@chromium.org>
Gerrit-Reviewer: Olof Johansson <ol...@chromium.org>

Doug Anderson (Code Review)

unread,

Nov 11, 2012, 11:17:14 PM11/11/12

to Mike Frysinger, Olof Johansson, Jon Kliegman, Gerrit, Grant Grundler

Doug Anderson has posted comments on this change.

Change subject: CHROMIUM: config: enable usb net drivers (dm9601/net1080/rtl8150) for all targets
......................................................................

Patch Set 3: Looks good to me, approved

Yeah, I know. I really don't have any objection to adding it but just feel a little awkward since it looks like it was purposely put only in Chromium OS and not Chrome OS. ...but in that CL lots of things were changed so that could have been an oversight / nobody thought that this would be useful in Chrome OS too.

I'm not sure I agree that a partly working driver is really better than a non-working driver. A non-working driver means it's just not supported. A partly working driver gives periodic crashes and makes our system look bad. ...but hopefully we'll notice in the crash reports at least.

In any case, I think Olof has had time to look at this if he cared to and there's no real reason to block, so moving to a +2. Maybe wait until tomorrow afternoon to submit just to make sure there are no objections?

Thanks! :)

--
To view, visit https://gerrit.chromium.org/gerrit/37296
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I9615109a5b17af293855595ba8f06f397db360c4
Gerrit-PatchSet: 3

Gerrit-Project: chromiumos/third_party/kernel
Gerrit-Branch: chromeos-3.4
Gerrit-Owner: Mike Frysinger <vap...@chromium.org>

Gerrit-Reviewer: Doug Anderson <dian...@chromium.org>
Gerrit-Reviewer: Gerrit <chrom...@google.com>
Gerrit-Reviewer: Grant Grundler <grun...@chromium.org>
Gerrit-Reviewer: Jon Kliegman <kli...@chromium.org>

Gerrit-Reviewer: Mike Frysinger <vap...@chromium.org>
Gerrit-Reviewer: Olof Johansson <ol...@chromium.org>

Grant Grundler (Code Review)

unread,

Nov 12, 2012, 5:29:34 PM11/12/12

to Mike Frysinger, Olof Johansson, Doug Anderson, Jon Kliegman, Gerrit, Grant Grundler

Grant Grundler has posted comments on this change.

Change subject: CHROMIUM: config: enable usb net drivers (dm9601/net1080/rtl8150) for all targets
......................................................................

Patch Set 3: Looks good to me, approved

I'm all for having USB device drivers enabled consistently across all platforms. Some of the USB devices are soldered down on the motherboards and that might be why the respective driver was only allowed on specific platform config.

Today, AFAIK, for USB ethernet dongles only pegasus and asix drivers are _supported_. We are very close to calling smsc75xx driver supported as well.

For the record, while I'm approving this CL, I'm also not aware of any testing of:
dm9601 (Davicom): "fast ethernet" adapter (100Mb/s)
net1080 (NetChip Tech): USB Host-Host "TurboConnect"
rtl8150: (RealTek) 100Mb/s ethernet

--
To view, visit https://gerrit.chromium.org/gerrit/37296
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I9615109a5b17af293855595ba8f06f397db360c4
Gerrit-PatchSet: 3

Gerrit-Project: chromiumos/third_party/kernel
Gerrit-Branch: chromeos-3.4
Gerrit-Owner: Mike Frysinger <vap...@chromium.org>

Gerrit-Reviewer: Doug Anderson <dian...@chromium.org>
Gerrit-Reviewer: Gerrit <chrom...@google.com>
Gerrit-Reviewer: Grant Grundler <grun...@chromium.org>
Gerrit-Reviewer: Jon Kliegman <kli...@chromium.org>

Gerrit-Reviewer: Mike Frysinger <vap...@chromium.org>
Gerrit-Reviewer: Olof Johansson <ol...@chromium.org>

Mike Frysinger (Code Review)

unread,

Nov 12, 2012, 5:33:48 PM11/12/12

to Mike Frysinger, Olof Johansson, Doug Anderson, Jon Kliegman, Gerrit, Grant Grundler

Mike Frysinger has posted comments on this change.

Change subject: CHROMIUM: config: enable usb net drivers (dm9601/net1080/rtl8150) for all targets
......................................................................

Patch Set 3:

we've got people reporting issues with dm9601 not working on the ARM chromebook (while it does work on other devices like the CR48). that is a USB 1.1 part, so while it can technically get higher than 10mbit/s, calling it 100mbit/s is disingenuous. not that that has stopped many people on amazon/etc... from labeling it as such :).

the others i changed just to be consistent in our configs. i can't see those being arch-specific.

i'll wait a day or two more to see if Olof has an opinion.

--
To view, visit https://gerrit.chromium.org/gerrit/37296
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I9615109a5b17af293855595ba8f06f397db360c4
Gerrit-PatchSet: 3

Gerrit-Project: chromiumos/third_party/kernel
Gerrit-Branch: chromeos-3.4
Gerrit-Owner: Mike Frysinger <vap...@chromium.org>

Gerrit-Reviewer: Doug Anderson <dian...@chromium.org>
Gerrit-Reviewer: Gerrit <chrom...@google.com>
Gerrit-Reviewer: Grant Grundler <grun...@chromium.org>
Gerrit-Reviewer: Jon Kliegman <kli...@chromium.org>

Gerrit-Reviewer: Mike Frysinger <vap...@chromium.org>
Gerrit-Reviewer: Olof Johansson <ol...@chromium.org>

Mike Frysinger (Code Review)

unread,

Nov 15, 2012, 11:12:55 PM11/15/12

to Mike Frysinger, Olof Johansson, Doug Anderson, Jon Kliegman, Gerrit, Grant Grundler

Mike Frysinger has posted comments on this change.

Change subject: CHROMIUM: config: enable usb net drivers (dm9601/net1080/rtl8150) for all targets
......................................................................

Patch Set 3: Ready

--
To view, visit https://gerrit.chromium.org/gerrit/37296
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I9615109a5b17af293855595ba8f06f397db360c4
Gerrit-PatchSet: 3

Gerrit-Project: chromiumos/third_party/kernel
Gerrit-Branch: chromeos-3.4
Gerrit-Owner: Mike Frysinger <vap...@chromium.org>

Gerrit-Reviewer: Doug Anderson <dian...@chromium.org>
Gerrit-Reviewer: Gerrit <chrom...@google.com>
Gerrit-Reviewer: Grant Grundler <grun...@chromium.org>
Gerrit-Reviewer: Jon Kliegman <kli...@chromium.org>

Gerrit-Reviewer: Mike Frysinger <vap...@chromium.org>
Gerrit-Reviewer: Olof Johansson <ol...@chromium.org>

Reply all

Reply to author

Forward

CHROMIUM: config: enable usb net drivers (dm9601/net1080/rtl... [chromiumos/third_party/kernel : chromeos-3.4]

Mike Frysinger (Code Review)

Jon Kliegman (Code Review)

Doug Anderson (Code Review)

Mike Frysinger (Code Review)

Doug Anderson (Code Review)

Mike Frysinger (Code Review)

Doug Anderson (Code Review)

Mike Frysinger (Code Review)

Doug Anderson (Code Review)

Grant Grundler (Code Review)

Mike Frysinger (Code Review)

Mike Frysinger (Code Review)