Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1442811 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH V8 7/9] vfio, platform: make reset driver a requirement by  default Sinan Kaya <okaya@codeaurora.org> - 2016-07-13 22:20 +0200
    Re: [PATCH V8 7/9] vfio, platform: make reset driver a requirement  by default Alex Williamson <alex.williamson@redhat.com> - 2016-07-13 23:00 +0200
      Re: [PATCH V8 7/9] vfio, platform: make reset driver a requirement by  default Sinan Kaya <okaya@codeaurora.org> - 2016-07-13 23:40 +0200

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

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.

The DT based drivers all have reset functions implemented. They shouldn't be
impacted by the reset_required flag. 

The reset_required flag is again useful for testing purposes when the reset
driver is broken or the ACPI _RST method is missing.

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.

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.

-- 
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.

[toc] | [next] | [standalone]

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

On Wed, 13 Jul 2016 16:12:35 -0400
Sinan Kaya <okaya@codeaurora.org> 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.

> 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,

Alex

[toc] | [prev] | [next] | [standalone]

#1442879

On 7/13/2016 4:55 PM, Alex Williamson wrote:
> On Wed, 13 Jul 2016 16:12:35 -0400
> Sinan Kaya <okaya@codeaurora.org> 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.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web