On Wed, 9 Dec 2020 13:52:03 +0100 Halil Pasic <pasic@xxxxxxxxxxxxx> wrote: > On Tue, 8 Dec 2020 18:30:54 +0100 > Cornelia Huck <cohuck@xxxxxxxxxx> wrote: > > > > > > > But, the more i look at this patch and discuss on this, i think this is > > > not complete. > > > i.e as you know, the main reason for this RFC was the the below thread. > > > https://marc.info/?l=linux-s390&m=158591045732735&w=2 > > > We are still not solving the problem that was mentioned in that RFD. > > > > > > There are couple of things which we needs to consider here. With this > > > patch, the uevents > > > are generated before doing the initialization or before finding the > > > ccw-device > > > connected. Which means, the udev-rules have to manage with a > > > non-initialized setup > > > compared to the previous version (Version without this patch). As you > > > mentioned, the > > > current user-space programs which works with this uevent, especially in > > > case of vfio-ccw > > > will have a problem. > > > > IIUC, we'll get the "normal" ADD uevent when the subchannel device is > > registered (i.e. made visible). For the vfio-ccw case, we want the > > driverctl rule to match in this case, so that the driver override can > > be set before the subchannel device ends up being bound to the I/O > > subchannel driver. So I think that removing the suppression is giving > > us exactly what we want? Modulo any errors in the initialization > > sequence we might currently have in the css bus code, of course. > > > > I believe, I'm the originator of these concerns, yet I find my > concern hard to recognize in the comment of Vineeth, so let me > please try to explain this in a different way. > > AFAIK the uevent handling is asynchronous with regards to matching and > probing, in a sense that there is no synchronization mechanism that > ensures, the userspace has had the ADD event handled (e.g. > driver_override set_ before the kernel proceeds with matching and > probing of the device. Am I wrong about this? > > If I'm, with the suppression gone we end up with race, where userspace > may or may not set driver_override in time. > > The man page of driverctl > (https://manpages.debian.org/testing/driverctl/driverctl.8.en.html) > claims that: "driverctl integrates with udev to support overriding driver > selection for both cold- and hotplugged devices from the moment of discovery, ..." > and "The driver overrides created by driverctl are persistent across > system reboots by default." > > Writing to the driver_override sysfs attribute does not auto-rebind. So > if we can't ensure being in time to set driver_override for the > subchannel before the io_subchannel driver binds, then the userspace > should handle this situation (by unbind and bind) to ensure the > effectiveness of 'driver override'. I couldn't find that code in > driverctl, and I assume if we had that, driver override would work > without this patch. Conny, does that sound about right? > > My argument is purely speculative. I didn't try this out, but trying > stuff out is of limited value with races anyway. Vineeth did you try? > If not, I could check this out myself some time later. Whenever we delegate policy decisions like that to userspace, we'll end up with uncertainty depending on timing. I don't think that there's any way around it. (FWIW, driver_override for pci behaves just the same, unless I misread the code.) What removing the uevent suppression does give us is at least a chance to influence the driver that is being bound and not wait until we have a fully setup ccw device as a child as well. > > > I'm not sure how many rules actually care about events for the > > subchannel device; the ccw device seems like the more helpful device to > > watch out for. > > I tend to agree, but the problem with vfio-ccw is that (currently) we > don't have a ccw device in the host, because we pass-through the > subchannel. When we interrogate the subchannel, we do learn if there > is a device and if, what is its devno. If I were to run a system with > vfio-ccw passthrough, I would want to passthrough the subchannel that > talks to the DASD (identified by devno) I need passed through to my > guest. I think that can be solved by simply adding the devno as a variable to the uevent (valid if it's an I/O subchannel; we don't register the subchannel in the first place if dnv is not set.)
