Hello Thomas,
On 6/21/22 13:29, Thomas Zimmermann wrote:
[...]
+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.
Bu then we'd have to declare struct resource-es for using an interface. This helper is trivial. If anything, resource_overlaps() should be generalized.
Yes, that works too. Probably then we should just keep as is and then as a follow up we can add another helper to include/linux/ioport.h to avoid having something that's only for the aperture helpers.
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.
struct resource stores the final byte of the resource. In our case 'end' is the byte after that. So the code is correct.
Do we ever have resources that end at the top-most byte of the address space?
I don't know to be honest.
[...]
+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 ?
In the current code, we clearly distinguish between the device and the platform device. The latter is only used in a few places where it's absolutely necessary, because there's no generic equivalent to platform_device_unregister(). (device_unregister() is something else AFAICT.) At some point, I'd like to see the aperture code being handled in a more prominent place within resource management. That would need it to use struct device.
Ok, I was wondering what was the value of the indirection level other than making the code more complex and supporting an hypothetical case of a FW driver that would not bind against a platform device.
But if the goal is to move this at some point to a more generic place (i.e: the Linux device model itself) then I agree that we can just keep it as is.
When you re-spin this patch, feel free to add my R-b since looks good to me.
And thanks again for working on this!