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!
--
Best regards,
Javier Martinez Canillas
Linux Engineering
Red Hat
