Re: [PATCH 2/2] vfio: platform: Add generic DT reset support

[Geert Uytterhoeven]

Hi Eric,

On Wed, Feb 14, 2018 at 10:09 AM, Auger Eric <eric.auger@xxxxxxxxxx> wrote:
> On 13/02/18 17:36, Geert Uytterhoeven wrote:
>> Vfio-platform requires reset support, provided either by ACPI, or, on DT
>> platforms, by a device-specific reset driver matching against the
>> device's compatible value.
>>
>> On many SoCs, devices are connected to an SoC-internal reset controller,
>> and can be reset in a generic way.  Hence add support to reset such
>> devices using the reset controller subsystem, provided the reset
>> hierarchy is described correctly in DT using the "resets" property.
>
> I first acknowledge I am not familiar with what those reset controllers
> do in practice. My fear is that we may rely on generic SW pieces that
> may not be adapted to passthrough constraints. We must guarantee that
> any DMA access attempted by the devices are stopped and any interrupts
> gets stopped. Can we guarantee that the reset controller always induce
> that? Otherwise we may leave the door opened to badly reset assigned
> devices.

An on-SoC reset controller is basically a block controlling signals to the
reset inputs of the individual on-SoC devices.
On Renesas ARM SoCs, this allows to do a full reset of the attached device.

Of course the exact semantics depend on the actual SoC.
If e.g. DMA and interrupts are not stopped for a specific device on a
specific SoC, it still needs a device-specific reset driver, matching against
the appropriate compatible value, cfr. the quoted paragraph below.

You could add a whitelist for of_machine_is_compatible() or
of_device_is_compatible(), but that will grow large fast.

>> Devices that require a more complex reset procedure can still
>> provide a device-specific reset driver, as that takes precedence.

>> @@ -138,6 +152,8 @@ static void vfio_platform_put_reset(struct vfio_platform_device *vdev)
>>
>>       if (vdev->of_reset)
>>               module_put(vdev->reset_module);
>> +
> if (vdev->reset_control) ?
> reset_control_put seems to only check IS_ERR()

    void reset_control_put(struct reset_control *rstc)
    {
            if (IS_ERR_OR_NULL(rstc))
                    return;

So it does handle NULL.

>> +     reset_control_put(vdev->reset_control);

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds