On 7/13/2016 4:55 PM, Alex Williamson wrote:

On Wed, 13 Jul 2016 16:12:35 -0400
Sinan Kaya [off-list ref] wrote:

quoted

On 6/23/2016 2:59 PM, Alex Williamson wrote:

quoted

quoted

quoted

 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.
 

quoted

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.

quoted

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.

quoted

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.

quoted

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.