On Tue, Jan 10, 2017 at 12:52 PM, Will Deacon <will.deacon@xxxxxxx> wrote: > Hi Rob, > > On Fri, Jan 06, 2017 at 11:26:49AM -0500, Rob Clark wrote: >> On Thu, Jan 5, 2017 at 10:49 AM, Will Deacon <will.deacon@xxxxxxx> wrote: >> > On Thu, Jan 05, 2017 at 10:27:27AM -0500, Rob Clark wrote: >> >> I'm not sure if the better solution then would be to have two fault >> >> callbacks, one immediately from the IRQ and a later one from wq. Or >> >> let the driver handle the wq business and give it a way to tell the >> >> IOMMU when to resume. >> >> >> >> I kinda think we should punt on the worker thread for now until we are >> >> ready to resume faulting transactions, because I guess a strong chance >> >> that whatever way we do it now will be wrong ;-) >> > >> > I guess what I'm after is for you to change the interrupt handlers to be >> > threaded, like they are for SMMUv3. I *think* you can do that with a NULL >> > thread_fn for now, and just call report_iommu_fault from the handler. >> > The return value of that could, in theory, be used to queued the paging >> > request and wake the paging thread in future. >> >> If we only pass in the non-threaded irq fxn, I'm not really sure how >> that changes anything.. or maybe I'm not understanding what you mean. >> >> But yeah, I guess we could use request_threaded_irq() to get both IRQ >> context notification and a later thread context notification rather >> than doing the wq thing. Either way the iommu API has to change >> slightly. >> >> >> > I wonder if this should also be predicated on the compatible string, so >> >> > that the "arm,smmu-enable-stall" property is ignored (with a warning) if >> >> > the compatible string isn't specific enough to identify an implementation >> >> > with the required SS behaviour? On the other hand, it feels pretty >> >> > redundant and a single "stalling works" property is all we need. >> >> >> >> We could also drop the property and key the behavior on specific >> >> compat strings I guess. Having both seems a bit odd. Anyways, I'll >> >> defer to DT folks about what the cleaner approach is. >> > >> > As Robin pointed out, we do need to be able to distinguish the integration >> > of the device from the device itself. For example, MMU-9000 might be capable >> > of stalling, but if it's bolted to a PCI RC, it's not safe to do so. >> >> Hmm, well we install the fault handler on the iommu_domain.. perhaps >> maybe a combo of dts property (or deciding based on more specific >> compat string), plus extra param passed in to >> iommu_set_fault_hander(). The dts property or compat string to >> indicate whether the iommu (and how it is wired up) can handle stalls, >> and enable_stall param when fault handler is registered to indicate >> whether the device itself can cope.. if either can't do stalling, then >> don't set CFCFG. > > I thought about this some more, and I think you're right. Having > iommu_set_fault_handler take a flags parameter indicating that, for example, > the fault handler can deal with paging, is all we need to implement the > per-master opt-in functionality for stalling faults. There's no real > requirement to standardise a generic firmware property for that (but > we still need *something* that says stalling is usable on the SMMU -- > perhaps just the compatible string is ok). btw, it occurred to me that maybe it should be flags param to iommu_attach_device() (just in case fault handler not installed?) otoh stalling without a fault handler is silly, but I guess we need it to infer whether stalling can be supported by other devices on same iommu.. tbh I'm on a bit shaky ground when it comes to multiple devices per iommu since the SoC's I'm familiar with do it the other way around. But I guess you have thought more about the multi-device case, so figured I should suggest it.. > Taking this further, there's then no need for the threaded IRQ function > in the SMMUv2 driver after all. Instead, we pass a continuation function > pointer and opaque token from the SMMU driver to the fault handler in > IRQ context (this will be in thread context for SMMUv3, but that should > be fine). The fault handler can then stash these someplace, and signal > a wakeup for its own threaded handler, which ultimately calls the SMMU > continuation function with the opaque token as a parameter when it's done > with the fault. I think that's enough to get things rolling without adding > lots of infrastructure to the SMMU driver initially. If a pattern emerges > amongst users of the interface, then we could consolidate some of the work > handling back into IOMMU core. > > What do you think? It should all be pretty straightforward for what you > want to do. yeah, that makes sense to me.. I can give it a try. BR, -R > Will -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
