[PATCH v1] soc/tegra: fuse: Drop Kconfig dependency on TEGRA20_APB_DMA

[PATCH v1] soc/tegra: fuse: Drop Kconfig dependency on TEGRA20_APB_DMA

[Dmitry Osipenko]

The DMA subsystem could be entirely disabled in Kconfig and then the
TEGRA20_APB_DMA option isn't available too. Hence kernel configuration
fails if DMADEVICES Kconfig option is disabled due to the unsatisfiable
dependency.

The FUSE driver isn't a critical driver and currently it only provides
NVMEM interface to userspace which isn't known to be widely used, and
thus, it's fine if FUSE driver fails to load.

Let's remove the erroneous Kconfig dependency and let the FUSE driver to
fail the probing if DMA is unavailable.

Fixes: 19d41e5e9c68 ("soc/tegra: fuse: Add APB DMA dependency for Tegra20")
Reported-by: Necip Fazil Yildiran <fazilyildiran@gmail.com>
Link: https://bugzilla.kernel.org/show_bug.cgi?id=209301
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/soc/tegra/Kconfig | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/soc/tegra/Kconfig b/drivers/soc/tegra/Kconfig
index 976dee036470..676807c5a215 100644
--- a/drivers/soc/tegra/Kconfig
+++ b/drivers/soc/tegra/Kconfig
@@ -136,7 +136,6 @@ config SOC_TEGRA_FUSE
 	def_bool y
 	depends on ARCH_TEGRA
 	select SOC_BUS
-	select TEGRA20_APB_DMA if ARCH_TEGRA_2x_SOC
 
 config SOC_TEGRA_FLOWCTRL
 	bool
-- 
2.27.0

Re: [PATCH v1] soc/tegra: fuse: Drop Kconfig dependency on TEGRA20_APB_DMA

[Dmitry Osipenko]

23.09.2020 03:34, Dmitry Osipenko пишет:
> The DMA subsystem could be entirely disabled in Kconfig and then the
> TEGRA20_APB_DMA option isn't available too. Hence kernel configuration
> fails if DMADEVICES Kconfig option is disabled due to the unsatisfiable
> dependency.
> 
> The FUSE driver isn't a critical driver and currently it only provides
> NVMEM interface to userspace which isn't known to be widely used, and
> thus, it's fine if FUSE driver fails to load.
> 
> Let's remove the erroneous Kconfig dependency and let the FUSE driver to
> fail the probing if DMA is unavailable.
> 
> Fixes: 19d41e5e9c68 ("soc/tegra: fuse: Add APB DMA dependency for Tegra20")
> Reported-by: Necip Fazil Yildiran <fazilyildiran@gmail.com>
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=209301
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/soc/tegra/Kconfig | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/soc/tegra/Kconfig b/drivers/soc/tegra/Kconfig
> index 976dee036470..676807c5a215 100644
> --- a/drivers/soc/tegra/Kconfig
> +++ b/drivers/soc/tegra/Kconfig
> @@ -136,7 +136,6 @@ config SOC_TEGRA_FUSE
>  	def_bool y
>  	depends on ARCH_TEGRA
>  	select SOC_BUS
> -	select TEGRA20_APB_DMA if ARCH_TEGRA_2x_SOC
>  
>  config SOC_TEGRA_FLOWCTRL
>  	bool
> 

Thierry, a reminder ping.

Re: [PATCH v1] soc/tegra: fuse: Drop Kconfig dependency on TEGRA20_APB_DMA

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1947 bytes --]

On Wed, Sep 23, 2020 at 03:34:21AM +0300, Dmitry Osipenko wrote:
> The DMA subsystem could be entirely disabled in Kconfig and then the
> TEGRA20_APB_DMA option isn't available too. Hence kernel configuration
> fails if DMADEVICES Kconfig option is disabled due to the unsatisfiable
> dependency.
> 
> The FUSE driver isn't a critical driver and currently it only provides
> NVMEM interface to userspace which isn't known to be widely used, and
> thus, it's fine if FUSE driver fails to load.

This isn't entirely accurate. The FUSE driver also provides SKU specific
information via tegra_sku_info and exposes some of the FUSE registers to
other drivers, such as the calibration data for XUSB.

The APB DMA is really only needed to work around an issue on Tegra20, so
perhaps this really is okay. On the other hand, could we not just change
the dependency to something like

	select DMADEVICES if ARCH_TEGRA_2x_SOC
	select TEGRA20_APB_DMA if ARCH_TEGRA_2x_SOC

to fix the Kconfig issue but keep the explicit documentation of this
dependency?

Thierry

> 
> Let's remove the erroneous Kconfig dependency and let the FUSE driver to
> fail the probing if DMA is unavailable.
> 
> Fixes: 19d41e5e9c68 ("soc/tegra: fuse: Add APB DMA dependency for Tegra20")
> Reported-by: Necip Fazil Yildiran <fazilyildiran@gmail.com>
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=209301
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/soc/tegra/Kconfig | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/soc/tegra/Kconfig b/drivers/soc/tegra/Kconfig
> index 976dee036470..676807c5a215 100644
> --- a/drivers/soc/tegra/Kconfig
> +++ b/drivers/soc/tegra/Kconfig
> @@ -136,7 +136,6 @@ config SOC_TEGRA_FUSE
>  	def_bool y
>  	depends on ARCH_TEGRA
>  	select SOC_BUS
> -	select TEGRA20_APB_DMA if ARCH_TEGRA_2x_SOC
>  
>  config SOC_TEGRA_FLOWCTRL
>  	bool
> -- 
> 2.27.0
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v1] soc/tegra: fuse: Drop Kconfig dependency on TEGRA20_APB_DMA

[Dmitry Osipenko]

16.11.2020 16:20, Thierry Reding пишет:
> On Wed, Sep 23, 2020 at 03:34:21AM +0300, Dmitry Osipenko wrote:
>> The DMA subsystem could be entirely disabled in Kconfig and then the
>> TEGRA20_APB_DMA option isn't available too. Hence kernel configuration
>> fails if DMADEVICES Kconfig option is disabled due to the unsatisfiable
>> dependency.
>>
>> The FUSE driver isn't a critical driver and currently it only provides
>> NVMEM interface to userspace which isn't known to be widely used, and
>> thus, it's fine if FUSE driver fails to load.
> 
> This isn't entirely accurate. The FUSE driver also provides SKU specific
> information via tegra_sku_info and exposes some of the FUSE registers to
> other drivers, such as the calibration data for XUSB.

The SKU data is read out only once during early boot and the DMA is not
used for this. The FUSE platform driver exists separately from the early
FUSE code.

> The APB DMA is really only needed to work around an issue on Tegra20, so
> perhaps this really is okay. On the other hand, could we not just change
> the dependency to something like
> 
> 	select DMADEVICES if ARCH_TEGRA_2x_SOC
> 	select TEGRA20_APB_DMA if ARCH_TEGRA_2x_SOC
> 
> to fix the Kconfig issue but keep the explicit documentation of this
> dependency?

On Tegra20, the APB DMA is used only for by NVMEM which is exposed via
sysfs. If DMA is disabled, then NVMEM isn't registered because
tegra20_fuse_probe() returns -EPROBE_DEFER.

Hence there is no real need to enforce the extra dependencies. It's
always better to allow minimizing the build dependencies whenever
possible, IMO, and it's possible to do it in this case.

Re: [PATCH v1] soc/tegra: fuse: Drop Kconfig dependency on TEGRA20_APB_DMA

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 3491 bytes --]

On Mon, Nov 16, 2020 at 04:48:39PM +0300, Dmitry Osipenko wrote:
> 16.11.2020 16:20, Thierry Reding пишет:
> > On Wed, Sep 23, 2020 at 03:34:21AM +0300, Dmitry Osipenko wrote:
> >> The DMA subsystem could be entirely disabled in Kconfig and then the
> >> TEGRA20_APB_DMA option isn't available too. Hence kernel configuration
> >> fails if DMADEVICES Kconfig option is disabled due to the unsatisfiable
> >> dependency.
> >>
> >> The FUSE driver isn't a critical driver and currently it only provides
> >> NVMEM interface to userspace which isn't known to be widely used, and
> >> thus, it's fine if FUSE driver fails to load.
> > 
> > This isn't entirely accurate. The FUSE driver also provides SKU specific
> > information via tegra_sku_info and exposes some of the FUSE registers to
> > other drivers, such as the calibration data for XUSB.
> 
> The SKU data is read out only once during early boot and the DMA is not
> used for this. The FUSE platform driver exists separately from the early
> FUSE code.

True, but the commit message isn't entirely accurate as-is, because on
later Tegra SoCs the driver does a bit more than that. So if you don't
mind I'll reword this to be a little more accurate if I end up deciding
to apply it.

> > The APB DMA is really only needed to work around an issue on Tegra20, so
> > perhaps this really is okay. On the other hand, could we not just change
> > the dependency to something like
> > 
> > 	select DMADEVICES if ARCH_TEGRA_2x_SOC
> > 	select TEGRA20_APB_DMA if ARCH_TEGRA_2x_SOC
> > 
> > to fix the Kconfig issue but keep the explicit documentation of this
> > dependency?
> 
> On Tegra20, the APB DMA is used only for by NVMEM which is exposed via
> sysfs. If DMA is disabled, then NVMEM isn't registered because
> tegra20_fuse_probe() returns -EPROBE_DEFER.

Again, true. But -EPROBE_DEFER is a silent error, so if somebody indeed
has DMADEVICES disabled and TEGRA20_APB_DMA is not available, then they
will not get FUSE support and they are going to struggle to find out why
that's not working.

> Hence there is no real need to enforce the extra dependencies. It's
> always better to allow minimizing the build dependencies whenever
> possible, IMO, and it's possible to do it in this case.

I don't entirely agree with this. Dependencies (and especially selects)
are used to pull in driver and/or features that are generally considered
useful. In this particular case, TEGRA20_APB_DMA is needed for the FUSE
driver to work correctly on Tegra20. Whether FUSE support on Tegra20 is
useful or not isn't quite relevant at this point.

There's also a balance as to what makes sense and what doesn't. APB DMA
is a useful feature in itself and disabling it is very much discouraged
because there are plenty of other drivers that can make use of it. That
is also the reason why we enable it in the default configuration. So I
don't consider a select on a symbol that's enabled by default an extra
dependency. Instead it's more of an explicit statement that you really
do want this enabled if you want that driver to work.

And like I said, if we don't have this and the driver will fail to probe
because of -EPROBE_DEFER, the user is going to have a very difficult
time of finding out why exactly that's happening. We're not even
emitting an error for this, so there's no way of knowing, even if they
enable driver debugging, /why/ the FUSE driver isn't working.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v1] soc/tegra: fuse: Drop Kconfig dependency on TEGRA20_APB_DMA

[Dmitry Osipenko]

16.11.2020 19:49, Thierry Reding пишет:
> On Mon, Nov 16, 2020 at 04:48:39PM +0300, Dmitry Osipenko wrote:
>> 16.11.2020 16:20, Thierry Reding пишет:
>>> On Wed, Sep 23, 2020 at 03:34:21AM +0300, Dmitry Osipenko wrote:
>>>> The DMA subsystem could be entirely disabled in Kconfig and then the
>>>> TEGRA20_APB_DMA option isn't available too. Hence kernel configuration
>>>> fails if DMADEVICES Kconfig option is disabled due to the unsatisfiable
>>>> dependency.
>>>>
>>>> The FUSE driver isn't a critical driver and currently it only provides
>>>> NVMEM interface to userspace which isn't known to be widely used, and
>>>> thus, it's fine if FUSE driver fails to load.
>>>
>>> This isn't entirely accurate. The FUSE driver also provides SKU specific
>>> information via tegra_sku_info and exposes some of the FUSE registers to
>>> other drivers, such as the calibration data for XUSB.
>>
>> The SKU data is read out only once during early boot and the DMA is not
>> used for this. The FUSE platform driver exists separately from the early
>> FUSE code.
> 
> True, but the commit message isn't entirely accurate as-is, because on
> later Tegra SoCs the driver does a bit more than that. So if you don't
> mind I'll reword this to be a little more accurate if I end up deciding
> to apply it.

Please feel free to edit it as yo wish.

>>> The APB DMA is really only needed to work around an issue on Tegra20, so
>>> perhaps this really is okay. On the other hand, could we not just change
>>> the dependency to something like
>>>
>>> 	select DMADEVICES if ARCH_TEGRA_2x_SOC
>>> 	select TEGRA20_APB_DMA if ARCH_TEGRA_2x_SOC
>>>
>>> to fix the Kconfig issue but keep the explicit documentation of this
>>> dependency?
>>
>> On Tegra20, the APB DMA is used only for by NVMEM which is exposed via
>> sysfs. If DMA is disabled, then NVMEM isn't registered because
>> tegra20_fuse_probe() returns -EPROBE_DEFER.
> 
> Again, true. But -EPROBE_DEFER is a silent error, so if somebody indeed
> has DMADEVICES disabled and TEGRA20_APB_DMA is not available, then they
> will not get FUSE support and they are going to struggle to find out why
> that's not working.
> 
>> Hence there is no real need to enforce the extra dependencies. It's
>> always better to allow minimizing the build dependencies whenever
>> possible, IMO, and it's possible to do it in this case.
> 
> I don't entirely agree with this. Dependencies (and especially selects)
> are used to pull in driver and/or features that are generally considered
> useful. In this particular case, TEGRA20_APB_DMA is needed for the FUSE
> driver to work correctly on Tegra20. Whether FUSE support on Tegra20 is
> useful or not isn't quite relevant at this point.
> 
> There's also a balance as to what makes sense and what doesn't. APB DMA
> is a useful feature in itself and disabling it is very much discouraged
> because there are plenty of other drivers that can make use of it. That
> is also the reason why we enable it in the default configuration. So I
> don't consider a select on a symbol that's enabled by default an extra
> dependency. Instead it's more of an explicit statement that you really
> do want this enabled if you want that driver to work.
> 
> And like I said, if we don't have this and the driver will fail to probe
> because of -EPROBE_DEFER, the user is going to have a very difficult
> time of finding out why exactly that's happening. We're not even
> emitting an error for this, so there's no way of knowing, even if they
> enable driver debugging, /why/ the FUSE driver isn't working.

Recent kernels have /sys/kernel/debug/devices_deferred.