On Fri, 9 Sep 2011, Felipe Balbi wrote: > > Apart from the hc_driver and various registration calls, the duplicated > > code is all stuff to do with locating and acquiring various resources: > > IRQ vectors and memory-mapped I/O regions. Clearly locating these > > resources will be vary, depending on the bus type. On the other hand, > > acquiring them could be moved into usbcore. (That's where it is > > already for PCI controllers.) > > you mean ioremap() and that sort of thing ? ehci-$arch.c should only > pass the correct resource down to ehci-hcd.c. And ehci-hcd.c should > request_mem_region() and ioremap_nocache() it. Yes, that's what I mean. Except that the request_mem_region() and ioremap_nocache() could be done in usbcore rather than in ehci-hcd.c. > > There's no good reason to dump all this information into more platform > > resources. There already are fields for these things in the hcd > > structure. The code to populate those fields could be shared among all > > the ehci-<arch> drivers. > > my point is that the very existence of those fields in struct hc_driver (You mean struct usb_hcd.) > is redundant where the canonical way to pass resources to drivers (DMA, > IRQ, memory addresses) is via struct resource. Yes, you're right. We probably could remove the fields from usb_hcd. > > This is the same objection as before. When the hc_driver structure is > > no longer duplicated, its members (including bus_suspend and > > bus_resume) won't be duplicated either. > > that's where I wanted to get. That has been my point all along. But you > still want to make struct hc_driver public for, IMHO, no good reason. Well, there _is_ a reason. Whether it's a _good_ reason is a matter of opinion. > > > > No, the runtime PM support _is_ present, but it is in usbcore instead > > > > in the host controller drivers. > > > > > > We can use device hierarchy for that, no ? Instead of adding methods to > > > struct hc_driver, implement things on dev_pm_ops. > > > > I don't understand. We _are_ using the device hierarchy for this. The > > dev_pm_ops methods invoke the callbacks in hc_driver. I don't see any > > why does dev_pm_ops need to directly call hc_diver->bus_suspend() when > you can add another dev_pm_ops to struct ehci-hcd.c ?? I think you're mixing up two different layers of the device model. The bus_suspend and bus_resume routines are used for power management of the root hub, whereas the dev_pm_ops methods are used for power management of the controller device itself. Thus the bus_suspend and bus_resume affect things like the downstream USB ports and the frame counter, whereas (in the PCI driver, for example) the dev_pm_ops methods put the entire controller into D0 or D3. > > way around this, because the dev_pm_ops methods are part of usbcore, > > not duplicated in each host controller driver. > > I think PM needs to be very granular. What I mean by that is that we go > from the very generic implementation to the very HW specific going up > the device hierarchy. > > So, usbcore should care about USB devices and interfaces, ehci-hcd.c > should care about specific bus states and ehci-omap.c should care about > clocks, context save&restore and the OMAP Architecture code > (arch/arm/*omap*) should care about clock domains, power domains, > voltage domains and so on. That's essentially the way it works now. > This is what I wanted to see. Rather than making usbcore call > hc_driver->bus_suspend, let the PM layer handle all the trickery for > you. ehci-hcd's dev_pm_ops::ehci_suspend() will only be called when all > its children are suspended anyway. Likewise, ehci-hcd's > dev_pm_ops::ehci_resume() will be called before resuming any child. > > IMHO, the current implementation is the one going through all hops and > loops, in a sense, reimplementing things which are generic on the PM > framework. It sounds like you don't have a complete picture of how USB power management is implemented. Almost all of the things you're asking for are things we already do. The one exception is something we can't do at all, because root hubs have to use the same dev_pm_ops method pointers as any other hub. > > Because it's better to have one exported symbol than a whole bunch of > > useless platform device structures. > > they're not useless. They will help you a lot with, at least, PM. No; see above. > > You might as well complain about the fact that usb_submit_urb() is > > exported. Somebody could pass it an URB with a bogus function pointer > > for the completion handler. > > ok, granted. On the other hand, usb_submit_urb() is only sending data > over the wire. It also calls the completion handler when the transfer is finished. > So it's not like a bogus data inside an URB could > compromise the current host, right ? It might be a problem for the > attached device, maybe. But who wants to take over a USB Memory Stick ? The bogus data might not make any difference, but when the bogus completion handler runs, it will be able to do whatever it wants on the host. Anyway, this whole thing is silly; if somebody can install a bogus driver module then they already have the ability to take over the entire system. > > So that drivers like ehci-tegra can copy it and then override certain > > fields with their own entry points. > > > > The alternative, which would be less efficient, is to publicize a > > routine that calls usb_create_hcd() with the static ehci_hc_driver as > > an argument. > > another alternative is the pass the differences from ehci-tegra.c via > platform_data to ehci-hcd.c This is inelegant at best, because it requires constant checking for overrides. That's not how other ops tables are used. When one driver wants to override a different layer, it replaces the entire table. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
