Hi Stephen, Thanks for the review ! On Wed, Nov 23, 2011 at 5:27 AM, Stephen Boyd <sboyd@xxxxxxxxxxxxxx> wrote: > How big are the images you're loading? I had to split the memcpy up into > megabyte chunks because I was running out of virtual memory to map in > two huge chunks (one for the firmware and one for the image's resting > place). This may work for now, but I think it will fail for limited > virtual memory environments. Unless CMA solves this problem? It definitely should. The images we specifically load aren't that big (~5MB) but some of the use cases we have do require around a whopping 100MB, and with CMA things just work (we do have to reserve a private CMA pool obviously, but it's a no brainer). > Also, this code assumes the firmware is actually as big as the elf > header says it is. It should be checking to make sure the elf_data is > actually big enough to copy from. Good point, thanks. > This is a bit odd. If I understand correctly, you load the firmware at > least twice. Once to see if the firmware has any virtio devices in the > resource table, and again when the image is actually loaded into RAM. Right; the firmware is loaded once at boot time, and then every time the remote processor is powered up. The former is required since we publish the remote services and their configurations (i.e. virtio devices, features and configurations) in the firmware, as a separate section (i.e. the "resource table"). We could use a separate file for this table, but we chose to couple it with the firmware in order to eliminate potential problems where the wrong resource table is loaded for a given firmware. It really makes sense since the resource table reflects the capabilities/requirements of the firmware. > Is > there any way to avoid loading it twice? I ask because we have something > like a 20Mb image that needs to be loaded and I'd like to avoid loading > it twice just to read a section out of it the first time. Sure, we could think of several approaches to optimize this, e.g. either keep the firmware loaded after boot (possibly to some limited period of time) or just allow platforms to provide their resource table as a separate file. Though frankly I'd just prefer to trust the page cache to eliminate any redundant overhead (at least when the rproc is powered up close enough to boot time), or just ignore this issue completely: since this overhead happens only once on boot, and users tend not to really power off their devices so much, i'm not sure how painful it really is ? > What do you do if dev goes away? Call rproc_unregister(), which blocks if request_firmware_nowait() didn't complete yet. > What do you think about making remoteproc into a bus? That's a really interesting idea, with some nice properties. But I'm not sure if the bus model fits our use cases: buses exclusively couple a single driver to any given hardware instance (i.e. device), and I think that a remote processor is more like an IOMMU: it's a hardware device that is being used by many drivers, and not exclusively driven by a single one. So we really end up having several (completely different) drivers that depend on this hardware block, and need some API to power it up, rather than having a single driver that manipulates it exclusively. > I imagine this function > would allocate a struct device and then rproc_register() would actually > hook it into the device layer. Then we could use the device we allocate > here for the firmware requests and we also have a nice namespace for all > remote processors in the system. Plus all the management code for > refcounting would come for free with the device layer (modulo the > rproc_boot() count). We can probably still do this even without turning remoteproc into a bus (i.e. by creating this device inside rproc_register()). I originally pondered whether to do this or not, as many other kernel frameworks do this too when their ->register() API is invoked, but I couldn't think of any strong advantages in doing so. Sounds like it does worth exploring thought, thanks for the comment. > What is RSC_VIRTIO_CFG? Sorry, that's an undocumented (and unused) resource entry that is going away. It was supposed to specify virtio device features for a given virtio device, but I'm removing it as part of the resource table overhaul I'm doing: once the resource entries will have a variable length, the virtio header will just be an inherent part of the virtio device resource entry. > For MSM I see two main pain points: > > * CMA is new and untested on MSM. I imagine in MSM's situation we would > carve out the memory chunks early on for each processor and then only > remove that memory if the processor is booted? I hope that just works > somehow. For ARM v6+, CMA just works. It's really simple and straight forward to use. > * rproc is tied to virtio fairly closely. Only for firmwares that supports virtio. If yours doesn't, you can probably just ignore the virtio parts (though I really recommend to think of supporting it in the next relevant generation of chipsets of yours. It really is great.) > We probably won't be using > virtio anytime soon on MSM, unless we can do smd over virtio or > something. I'm not particularly knowledgeable about that though. Would > something like that be possible? Virtio today is highly coupled with its vring transport, which defines the shared memory "wire" protocol. Using it as-is pretty much means replacing smd completely (I assume that by smd you mostly care about keeping your firmware images as-is, which pretty much means using the smd's shared memory transport). But there's another way: virtio was originally designed to support several different transports, and actually had pluggable virtqueue ops which allowed several different transport implementations to co-exist. Since virtio_ring was the only transport that anyone ever used for a few years, the virtqueue ops were squashed and today virtio directly calls virtio_vring (see 7c5e9ed0c84e7d70d887878574590638d5572659). By reverting the aforementioned commit, you can potentially do virtio over smd, where smd will plug as another virtqueue_ops implementation. This is probably worth exploring, as you could then use rpmsg and the plethora of virtio drivers without really changing your firmware. > We would also have to keep using the rproc_get_by_name() approach until > we can actually start tying rproc devices to other devices in the kernel. I kept rproc_get_by_name() mainly for you guys :) I'd really want us to scrutinize its use cases though and see if we can get rid of it, as explained in the commentary. Thanks again for your review, Ohad. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
