Re: [PATCH] pci-arbiter: Remove embedded pciaccess code
Joan Lledó via Bug reports for the GNU Hurd, le dim. 26 janv. 2020 09:52:12 +0100, a ecrit: > I'm glad to say that our patches for both picutils and libpciaccess are > now upstream. Yay \o/ Samuel
Re: [PATCH] pci-arbiter: Remove embedded pciaccess code
Hello Hurd, I'm glad to say that our patches for both picutils and libpciaccess are now upstream. El 10/11/19 a les 2:44, Samuel Thibault ha escrit: > Joan Lledó, le sam. 09 nov. 2019 17:36:19 +0100, a ecrit: >> I was also wondering if after your changes, libpciaccess would support >> nested arbiters, or if is that something we want at all. > > We'd want this, yes. > >> About pciutils, I've seen our patch doesn't use the RPC get_ndevs(), so >> no need to remove it, but it has the closedir() bug too. So I'll fix it >> and send it upstream as well. > > Possibly indeed. > (it's not my patch, see credits :) ) > > Samuel >
Re: [PATCH] pci-arbiter: Remove embedded pciaccess code
Joan Lledó, le sam. 09 nov. 2019 17:36:19 +0100, a ecrit: > I was also wondering if after your changes, libpciaccess would support > nested arbiters, or if is that something we want at all. We'd want this, yes. > About pciutils, I've seen our patch doesn't use the RPC get_ndevs(), so > no need to remove it, but it has the closedir() bug too. So I'll fix it > and send it upstream as well. Possibly indeed. (it's not my patch, see credits :) ) Samuel
Re: [PATCH] pci-arbiter: Remove embedded pciaccess code
Hi, Thanks for your explanations, Samuel. Then I'll take your changes on the libpciaccess patch, fix the closedir() issue and send it to upstream. I was also wondering if after your changes, libpciaccess would support nested arbiters, or if is that something we want at all. About pciutils, I've seen our patch doesn't use the RPC get_ndevs(), so no need to remove it, but it has the closedir() bug too. So I'll fix it and send it upstream as well.
Re: [PATCH] pci-arbiter: Remove embedded pciaccess code
> That's exactly it, no manual compilation needed. Just make sure to be > getting upgrades from the "unreleased" distrib. Thanks. This week I'll try to test It El sáb., 9 nov. 2019 a las 15:21, Samuel Thibault () escribió: > Almudena Garcia, le sam. 09 nov. 2019 15:15:31 +0100, a ecrit: > > but I don't know if I simply have to "apt full-upgrade", > > That's exactly it, no manual compilation needed. Just make sure to be > getting upgrades from the "unreleased" distrib. > > Samuel > >
Re: [PATCH] pci-arbiter: Remove embedded pciaccess code
Almudena Garcia, le sam. 09 nov. 2019 15:15:31 +0100, a ecrit: > but I don't know if I simply have to "apt full-upgrade", That's exactly it, no manual compilation needed. Just make sure to be getting upgrades from the "unreleased" distrib. Samuel
Re: [PATCH] pci-arbiter: Remove embedded pciaccess code
Then, does Debian repositories includes all new PCI Arbitrer and libpciaccess0 updates? Samuel asked me to test the latest versions of netdde, libpciaccess0 and hurd, in my Thinkpad T60, but I don't know if I simply have to "apt full-upgrade", or I need to do a manual compilation from upstream El sáb., 9 nov. 2019 a las 14:35, Guillem Jover () escribió: > Hi! > > [ BTW it seems like something on the gnu.org side is mandling UTF-8 > characters? :/ ] > > On Sat, 2019-11-09 at 11:54:15 +0100, Samuel Thibault wrote: > > Joan Lledó via Bug reports for the GNU Hurd, le sam. 09 nov. 2019 > 10:50:18 +0100, a ecrit: > > > And, about pciutils, what did you do? > > > > I only added a small fix (see the Debian changelog). AIUI patches > > haven't been submitted to upstream yet (or they haven't been accepted, > > I don't remember, mailing list archives will remember), but they should > > be. > > I've taken these patches, applied them to the pciutils package, and > upload to Debian main. > > Thanks, > Guillem > >
Re: [PATCH] pci-arbiter: Remove embedded pciaccess code
Hi! [ BTW it seems like something on the gnu.org side is mandling UTF-8 characters? :/ ] On Sat, 2019-11-09 at 11:54:15 +0100, Samuel Thibault wrote: > Joan Lledó via Bug reports for the GNU Hurd, le sam. 09 nov. 2019 10:50:18 > +0100, a ecrit: > > And, about pciutils, what did you do? > > I only added a small fix (see the Debian changelog). AIUI patches > haven't been submitted to upstream yet (or they haven't been accepted, > I don't remember, mailing list archives will remember), but they should > be. I've taken these patches, applied them to the pciutils package, and upload to Debian main. Thanks, Guillem
Re: [PATCH] pci-arbiter: Remove embedded pciaccess code
Concerning packages uploaded to the "unreleased" distribution, a repository if there was one would be on https://salsa.debian.org/hurd-team I however don't create one for each and very package I upload to "unreleased". Normally the patchs uploaded there are already submitted upstream or to a Debian bug report, so they will end up in the normal flow anyway so there is no need to create a repository for them. If you want to get the source, you can dget the .dsc, e.g.: dget http://ftp.ports.debian.org/debian-ports/pool-hurd-i386/main/libp/libpciaccess/libpciaccess_0.14-1+hurd.3.dsc Joan Lledó via Bug reports for the GNU Hurd, le sam. 09 nov. 2019 10:48:23 +0100, a ecrit: > - You said libpciaccess upstream code for the Hurd doesn't match the one > in the Debian package. Yes, apparently what ended up submitted upstream was not what I had previously uploaded to "unreleased". > - You attached a patch for libpciaccess, is it for upstream? for Debian? > is already applied? It is for upstream, but like I said it's a bit gross, so we may want to discuss before submitting something to upstream. It can't be "for Debian", Debian is not supposed to keep patches changing upstream source if it's not a Debian-only change. Joan Lledó via Bug reports for the GNU Hurd, le sam. 09 nov. 2019 10:50:18 +0100, a ecrit: > And, about pciutils, what did you do? I only added a small fix (see the Debian changelog). AIUI patches haven't been submitted to upstream yet (or they haven't been accepted, I don't remember, mailing list archives will remember), but they should be. Samuel
Re: [PATCH] pci-arbiter: Remove embedded pciaccess code
El 9/11/19 a les 10:48, Joan Lledó via Bug reports for the GNU Hurd ha escrit: > Now I have some questions: > > - You said libpciaccess upstream code for the Hurd doesn't match the one > in the Debian package. Where is the debian repo for the package? I found > [1] but it doesn't seem to have any patch at any branch. > > - You attached a patch for libpciaccess, is it for upstream? for Debian? > is already applied? > And, about pciutils, what did you do?
Re: [PATCH] pci-arbiter: Remove embedded pciaccess code
Hi, El 3/11/19 a les 21:48, Samuel Thibault ha escrit: > Hello, > > Uploaded fixed netdde, pciutils, libpciaccess, and commited this > pci-arbiter cleanup! > Great! Now I have some questions: - You said libpciaccess upstream code for the Hurd doesn't match the one in the Debian package. Where is the debian repo for the package? I found [1] but it doesn't seem to have any patch at any branch. - You attached a patch for libpciaccess, is it for upstream? for Debian? is already applied? _ [https://salsa.debian.org/xorg-team/lib/libpciaccess]
Re: [PATCH] pci-arbiter: Remove embedded pciaccess code
On 4/11/19 7:48 am, Samuel Thibault wrote: > Hello, > > Uploaded fixed netdde, pciutils, libpciaccess, and commited this > pci-arbiter cleanup! > > Samuel > Thank you for doing all this! wOOt!! I will test it soon. Damien
Re: [PATCH] pci-arbiter: Remove embedded pciaccess code
Hello, Uploaded fixed netdde, pciutils, libpciaccess, and commited this pci-arbiter cleanup! Samuel
Re: [PATCH] pci-arbiter: Remove embedded pciaccess code
Strange, the patches were not attached... I send them again.
Re: [PATCH] pci-arbiter: Remove embedded pciaccess code
Hello, El 27/10/19 a les 16:32, Samuel Thibault ha escrit:> Hello, > Could you try to split your changes in separate patches? I splitted the patch into four patches, the first one is the Damien's original patch adapted to the current head + a changelog. The other three patches are my changes. > I don't remember: can we apply this patch in Debian Hurd already? (i.e. > does the libpciaccess there (possibly in unreleased) work fine enough > for our needs?) I've been taking a look at the big picture and I'd say the problem is the GNU Mach restriction to only one process accessing the cfg io ports. Any release where that is enabled, then libpciaccess must include Damien's patches so all clients are redirected to the arbiter. But for the arbiter, the commits we apply won't change anything I'd say, since the arbiter will be just another client trying to access the io ports, no matter if it does it directly or through a library. > This looks a bit complex, I would say we should rather just cast: > > typedef int (*pci_op_t) (struct pci_device * dev, void *data, >pciaddr_t reg, pciaddr_t width, >pciaddr_t * bytes); > > io_config_file (node->nn->ln->device, offset, len, data, > - pci_sys->write); > + (pci_op_t) pci_sys->write); > > Because it is completely compatible to pass a void* to a function that > takes a const void*. That will simplify the whole thing a lot. Def. I made an unnecessary mess. The attached patch just makes the cast. >>/* Copy the regions info */ >> - rom.base_addr = e->device->rom_base; >> + rom.base_addr = 0; // pci_device_private only >>rom.size = e->device->rom_size; >>memcpy (*data, &rom, size); > > The change is because rom_base is a private field in libpciaccess, we'd > need to get libpciaccess to expose it so we can properly take it into > account. For the time being, putting FIXME here would be enough. Understood! I filled that line in the changelog.
Re: [PATCH] pci-arbiter: Remove embedded pciaccess code
Hello, Thanks for your work? Could you try to split your changes in separate patches? Otherwise the review is harder since the reviewer has to work out which pieces are intended to what part of the changes, and applying the parts that look ok while other parts are getting repolished is harder since the reviewer might miss pieces. I don't remember: can we apply this patch in Debian Hurd already? (i.e. does the libpciaccess there (possibly in unreleased) work fine enough for our needs?) Joan Lledó via Bug reports for the GNU Hurd, le dim. 27 oct. 2019 08:48:19 +0100, a ecrit: > - err = op (dev->bus, dev->dev, dev->func, offset, data, 4); > + if (op.read) > + err = op.io_op.read (dev, data, offset, 4, &actual); > + else > + err = op.io_op.write (dev, data, offset, 4, &actual); This is a bit unfortunate. > these two libpciaccess functions have different prototypes, we cannot > use one single pointer to function like we did until now, so I added > some changes to use always the proper prototype. This looks a bit complex, I would say we should rather just cast: typedef int (*pci_op_t) (struct pci_device * dev, void *data, pciaddr_t reg, pciaddr_t width, pciaddr_t * bytes); io_config_file (node->nn->ln->device, offset, len, data, - pci_sys->write); + (pci_op_t) pci_sys->write); Because it is completely compatible to pass a void* to a function that takes a const void*. That will simplify the whole thing a lot. > except the change in S_pci_get_dev_rom() in pci-ops.c. > I don't know what's this change for, but if we're always setting base_addr > to 0, then probably we don't need that field anymore and can remove it from > struct pci_xrom_bar. > @@ -260,7 +260,7 @@ S_pci_get_dev_rom (struct protid * master, char **data, > size_t * datalen) > } > >/* Copy the regions info */ > - rom.base_addr = e->device->rom_base; > + rom.base_addr = 0; // pci_device_private only >rom.size = e->device->rom_size; >memcpy (*data, &rom, size); The change is because rom_base is a private field in libpciaccess, we'd need to get libpciaccess to expose it so we can properly take it into account. For the time being, putting FIXME here would be enough. The rest looks good, but since I'm not sure which parts can be applied independently of the rest (some buffering issues for instance, I guess), I prefer not to commit anything yet. Samuel
