Since we are talking about remove_conflicting_devices() here, a better code
example could be for a platform device instead of a PCI device, like this:
* static const struct platform_driver example_driver = {
* .name = "example",
* ...
* };
*
* static int probe(struct platform_device *pdev)
* {
* struct resource *mem;
* resource_size_t base, size;
*
* mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
* if (!mem)
* return -EINVAL;
* base = mem->start;
* size = resource_size(mem);
*
* ret = remove_conflicting_devices(base, size, false, &example_driver->name);
* if (ret)
* return ret;
*
* // ... and initialize the hardware.
* ...
*
* return 0;
* }
+ * static int probe(struct pci_dev *pdev)
+ * {
+ * int ret;
+ *
+ * // Remove any generic drivers...
+ * ret = remove_conflicting_framebuffers(pdev);
And here we can just use remove_conflicting_pci_devices(pdev) without the
unnecessary level of indirection. It makes the example more clear IMO and
it could be moved as an example for the remove_conflicting_pci_devices().
Another option is to have here an example for platform devices instead of
a PCI device (and move this example when talking about remove
[...]
+ * PCI device drivers can also call remove_conflicting_pci_devices() and let the
+ * function detect the apertures automatically. Device drivers without knowledge of
+ * the framebuffer's location shall call remove_all_conflicting_devices(),
+ * which removes all known devices.
+ *
Can we get all the public aperture functions be in the aperture namespace? i.e:
aperture_remove_conflicting_devices(), aperture_remove_all_conflicting_devices()
and so on. That makes easier to grep, ftrace and also read the drivers' code.
+ * Drivers that are susceptible to being removed by other drivers, such as
+ * generic EFI or VESA drivers, have to register themselves as owners of their
+ * framebuffer apertures. Ownership of the framebuffer memory is achieved
+ * by calling devm_acquire_aperture_of_platform_device(). On success, the driver
AFAICT the aperture infrastructure only allows to remove platform devices, even
when it can check if the requested I/O resource overlaps with a PCI BAR range,
so maybe all functions also should use _platform_device() as suffix instead of
just _device() ? Or maybe the _platform is just verbose but I think that the
functions should be named consistently and only use either _device or _platform.
[...]
+ * static int acquire_framebuffers(struct drm_device *dev, struct platform_device *pdev)
+ * {
+ * resource_size_t base, size;
+ *
This example is missing a struct resource *mem declaration.
+ * The generic driver is now subject to forced removal by other drivers. This
+ * only works for platform drivers that support hot unplugging.
+ * When a driver calls remove_conflicting_devices() et al
+ * for the registered framebuffer range, the aperture helpers call
+ * platform_device_unregister() and the generic driver unloads itself. It
+ * may not access the device's registers, framebuffer memory, ROM, etc
+ * afterwards.
+ */
+
+struct dev_aperture {
+ struct device *dev;
And here we could just use a struct platform_device *pdev since currently we
only support platform devices. It seems to me that this is a DRM-ism that we
are carrying since for DRM drivers made sense to use struct device.
Doing that would also allow get rid of indirections like the need of both a
devm_acquire_aperture_of_platform_device() and devm_aperture_acquire() just
to do a &pdev->dev.
And also some to_platform_device() in drm_aperture_detach_firmware() and
detach_platform_device().
If we ever support non-platform devices then we can refactor it, but I don't
think that is worth to complicate just in case we ever support struct device.
+ resource_size_t base;
+ resource_size_t size;
+ struct list_head lh;
+ void (*detach)(struct device *dev);
Same here, just void (*detach)(struct platform_device *pdev) if you agree with
that I mentioned above.
+};
+
+static LIST_HEAD(apertures);
+static DEFINE_MUTEX(apertures_lock);
+
+static bool overlap(resource_size_t base1, resource_size_t end1,
+ resource_size_t base2, resource_size_t end2)
+{
+ return (base1 < end2) && (end1 > base2);
+}
There's a resource_overlaps() helper in include/linux/ioport.h, I wonder if it
could just be used, maybe declaring and filling a struct resource just to call
that helper. Later as an optimization a resource_range_overlap() or something
could be proposed for include/linux/ioport.h.
Also, I noticed that resource_overlaps() uses <= and >= but this helper uses
< and >. It seems there's an off-by-one error here but maybe I'm wrong on this.
[...]
+static void detach_platform_device(struct device *dev)
+{
+ struct platform_device *pdev = to_platform_device(dev);
+
+ /*
+ * Remove the device from the device hierarchy. This is the right thing
+ * to do for firmware-based DRM drivers, such as EFI, VESA or VGA. After
+ * the new driver takes over the hardware, the firmware device's state
+ * will be lost.
+ *
+ * For non-platform devices, a new callback would be required.
+ *
I wonder if we ever are going to need this. AFAICT the problem only happens for
platform devices. Or do you envision a case when some a bus could need this and
the aperture unregister the device instead of the Linux kernel device model ?
Attachment:
OpenPGP_signature
Description: OpenPGP digital signature