Mostly a copy of pci subsystem support, by replacing pciconf with usbconfig and excluding hardware classes.
Details
Details
Diff Detail
Diff Detail
- Repository
- rG FreeBSD src repository
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Comment Actions
I was pondering this for a while -- also for other devices -- but I highly felt that usbconfig wanted some improvements first to make this a lot easier.
There are tricky bits with USB devices changing "identity" depending on quirks or other state (i.e., first show up as CD-ROM and then become a network device).
But I am all for a good start :) I'll go and look through to see if I can add a few other bits using your logic next week, if that's ok?
| usr.sbin/fwget/usb/usb | ||
|---|---|---|
| 60 | looks like an indent problem here in Phabricator? | |
| usr.sbin/fwget/usb/usb_ralink | ||
| 6 | I do not think there's anything from the Foundation or me in this file. | |
Comment Actions
But I am all for a good start :) I'll go and look through to see if I can add a few other bits using your logic next week, if that's ok?
Perfectly fine with me.
Not made it and likely will not the next 3-4 days. If this needs to go in, please let me know and I'll review the current version and then defer any further changes to the future.
Comment Actions
I think for fwget.8 you can just `%s/default pci/valid types are:
.Dv pci ,
.Dv usb ./"
(But I don't actually know the syntax for using newlines in a replace)
Comment Actions
I see no change in fwget output when testing using git-arc.sh patch -C D48678; cd usr.sbin/fwget; make install; fwget on todays's CURRENT .
I have an Intel AX211, I do not know if it needs the firmware from comms/iwmbt-firmware, but when I install it manually my dmesg does change.
Comment Actions
While we've been waiting, I came up with some additional suggestions for your consideration.
| usr.sbin/fwget/fwget.8 | ||
|---|---|---|
| 26 | I'm sorry for making this kind of super annoying nit, but makewhatis will complain every time the manual database is manually compiled. | |
| 50–55 | Sorry to back track on this, but (while we're waiting) I think I've got something better. In this case, pci or usb are really command modifiers in my mind, and this also makes them render bold. Additionally, I am hoping this shows that can use one, or both separated by a space. | |
| 71–75 | Linking to the driver overview pages here would be very nice, and subsystems should be plural here. | |
| usr.sbin/fwget/usb/usb_ralink | ||
|---|---|---|
| 33 | Srcmgr looked at this a while ago. Using hex like this won't scale well. | |
Comment Actions
The thought was it would be better to have the ids in a file (rather than in code). This would allow masks too. And we could reuse it for pci.
And honestly, this data is in these drivers. We need to find a way to "decorate" that data in the drivers ala devmatch.
Comment Actions
You can use masks here too not just in an extra file. The original structure like this is from @manu who started this for graphics. At the moment I beleive this is the wrong discussion to have here. Please start a thread on hackers or somewhere if you want to rewrite it.
And honestly, this data is in these drivers. We need to find a way to "decorate" that data in the drivers ala devmatch.
And then do what with it? Not even for drm drivers you can do the automated fetch upon first boot based on the firmware request as you normally do not have networking then. But that's also something I think you need to elaborate on elsewhere.
Comment Actions
You need to reply to feedback from the review.
| usr.sbin/fwget/usb/usb_ralink | ||
|---|---|---|
| 33 | You need to at least reply to this. | |
| usr.sbin/fwget/usb/usb_ralink | ||
|---|---|---|
| 33 | To be clear, the problem with this scheme--which is pervasive in fwget--is bugs like this (which have already happened multiple times): case 53*)
addpkg foo ;;
...
case 531)
addpkg bar ;;Here, device 531 silently won't match the second case. The solution is to reimplement all of these case statements using tables where we can easily check for ambiguous matches. flua seems like a good candidate for that. But, that's not a problem here yet and I don't think it should block the patch: no one has prototyped a concrete alternative yet. So I don't think there's anything here that needs to change. | |
| usr.sbin/fwget/usb/usb | ||
|---|---|---|
| 2 | Same, as below, can we use SPDX short form here? | |
| usr.sbin/fwget/usb/usb_ralink | ||
| 3 | Can we use the ISO short form as described in style.9 and https://docs.freebsd.org/en/articles/license-guide/ ? I think it saves loc in the tree, improves visual compatibility with standard or narrow consoles, and makes things really obvious without requiring license expertise. Also, the - at the beginning is no longer required. | |
Comment Actions
No functionel changes intended just SPDX license headers instead of full license text..
| usr.sbin/fwget/usb/usb_ralink | ||
|---|---|---|
| 33 | So can we land this, with a future wish to refactor it with a more robust and efficient implementation that perhaps involves devmatch? | |
| usr.sbin/fwget/usb/usb_ralink | ||
|---|---|---|
| 33 | Yes, you can land this; there are no wildcards here. If fwget gets re-written in flua that's a different story. | |