Re: [PATCH V8 7/9] vfio, platform: make reset driver a requirement by default

[Sinan Kaya]

On 7/13/2016 4:55 PM, Alex Williamson wrote:
> On Wed, 13 Jul 2016 16:12:35 -0400
> Sinan Kaya <okaya@xxxxxxxxxxxxxx> wrote:
> 
>> On 6/23/2016 2:59 PM, Alex Williamson wrote:
>>>>>  static struct resource *get_platform_resource(struct vfio_platform_device *vdev,
>>>>> @@ -66,6 +70,7 @@ static int vfio_platform_probe(struct platform_device *pdev)
>>>>>  	vdev->get_resource = get_platform_resource;
>>>>>  	vdev->get_irq = get_platform_irq;
>>>>>  	vdev->parent_module = THIS_MODULE;
>>>>> +	vdev->reset_required = reset_required;  
>>>
>>> Do you see value in making the global reset_required changeable, with
>>> the behavior of any given device dependent on the setting of this
>>> variable at the time of probe?  It seems like a bit of a support issue
>>> to me.  Also, we're breaking existing users if there are any with this
>>> change.  Should we introduce a CONFIG option to set the default?  I
>>> think we can get away with changing the default that way, but I'm not
>>> so sure otherwise.
>>>   
>>
>> We have two groups of existing users.
>>
>> 1. AMBA based drivers
>> 2. DT based drivers
>>
>> and now we are trying to add the ACPI based drivers in this series.
>>
>> The AMBA based drivers do not have reset function implemented. Based on
>> previous conversation with Eric, these devices were mostly used for
>> bringing up the VFIO framework and were not intended for production. 
>> If we want to maintain existing functionality, I can change reset_required to
>> false by default for the AMBA based drivers.
> 
> I think we need to consider them to be in production at this point, so
> probably better to make such a change.
>  
>> The DT based drivers all have reset functions implemented. They shouldn't be
>> impacted by the reset_required flag. 
> 
> Ok, so we're fine there.
> 
>> The reset_required flag is again useful for testing purposes when the reset
>> driver is broken or the ACPI _RST method is missing.
> 
> I don't doubt that, but it doesn't need to be mode 644 for that, which
> allows changing the default dynamically.  We could make it 444 so that
> it can be set at module load time and not modified.  I just don't want
> to try to guess the state of that variable at the time the device was
> probed.

OK. I'll change it to 444.

> 
>> The previously agreed approach was to force the reset required by default
>> for production environment and be able to clear it for testing purposes.
>> When I was implementing HIDMA, I never realized that I needed a reset driver
>> until Arnd told me during the review. We want to avoid this for the long
>> term for DT and ACPI based implementations.
> 
> I agree, but we don't need to make it dynamically changeable for that.
> Also, nothing prevents us from printing a warning when a device is
> probed w/o a reset function, it's just a matter of whether that causes
> a probe failure or a complain and continue.
> 
>> The reset_required command line parameter would be useful if somebody suspects
>> that the ACPI _RST implementation is broken or the DT based reset driver is
>> broken or you quickly want to test the virtualization without having a reset
>> driver ready yet.
>>
>> Let us know which way you want to go. I can also add a Kconfig option and
>> set it by default. But then I have to recompile the kernel when I want to
>> test without the reset stuff.
> 
> Seems like we don't need a Kconfig, but I don't see why the option
> needs to be settable except at module load time and we can complain
> either way to clue in developers and catch such things in testing.
> Thanks,

OK. 

> 
> Alex
> 


-- 
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.