Re: [PATCH] drivers: firewire: Let several sub-modules depend on HAS_DMA

[Chen Gang]

On 07/13/2014 04:42 PM, Stefan Richter wrote:
> On Jul 13 Chen Gang wrote:
>>  
>>  config FIREWIRE_OHCI
>>  	tristate "OHCI-1394 controllers"
>> -	depends on PCI && FIREWIRE && MMU
>> +	depends on PCI && FIREWIRE && MMU && HAS_DMA
>>  	help
>>  	  Enable this driver if you have a FireWire controller based
>>  	  on the OHCI specification.  For all practical purposes, this
> 
> As far as I understand, architecture configuration files shall already
> ensure that CONFIG_PCI is not enabled if !CONFIG_HAS_DMA.  If so, this
> part can be omitted.  Or am I mistaken?
> 

Yeah, this part can be omitted, what you said is OK to me. FIREWIER_OHCI
need not append additional "depends on HAS_DMA".
 
>> @@ -45,7 +45,7 @@ config FIREWIRE_SBP2
>>  
>>  config FIREWIRE_NET
>>  	tristate "IP networking over 1394"
>> -	depends on FIREWIRE && INET
>> +	depends on FIREWIRE && INET && HAS_DMA
>>  	help
>>  	  This enables IPv4/IPv6 over IEEE 1394, providing IP connectivity
>>  	  with other implementations of RFC 2734/3146 as found on several
> 
> This is not completely necessary:  firewire-net does not use DMA mapping
> APIs directly, it uses them only through firewire-core.  Same with
> sound/firewire/* (a few audio device drivers) and with
> drivers/media/firewire/* (a DVB device driver) which you did not patch.
> 

Yeah, thanks.

> So they could be *compiled* on architectures without HAS_DMA, they could
> just not be *used* (because firewire-core's isochronous DMA mapping
> functions would return errors, but more so because there are no IEEE 1394
> host bus adapters on these platforms in the first place.  Actually Texas
> Instruments used to make a 1394 HBA chip with some sort of GPIO host
> interface instead of PCI interface, but Linux only has a driver for PCI
> HBAs.)
> 
> But see below.
> 

OK, thank you for your details information.

> 
> All in all, I like the following approach better:
> 
> Date: Wed,  9 Jul 2014 21:04:00 +0200
> From: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>
> To: Stefan Richter <stefanr@xxxxxxxxxxxxxxxxx>
> Cc: Jean Delvare <jdelvare@xxxxxxx>, linux1394-devel@xxxxxxxxxxxxxxxxxxxxx, linux-kernel@xxxxxxxxxxxxxxx, Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>
> Subject: [PATCH] firewire: IEEE 1394 (FireWire) support should depend on HAS_DMA
> 
> Commit b3d681a4fc108f9653bbb44e4f4e72db2b8a5734 ("firewire: Use
> COMPILE_TEST for build testing") added COMPILE_TEST as an alternative
> dependency for the purpose of build testing the firewire core.
> However, this bypasses all other implicit dependencies assumed by PCI,
> like HAS_DMA.
> 
> If NO_DMA=y:
> 
>     drivers/built-in.o: In function `fw_iso_buffer_destroy':
>     (.text+0x36a096): undefined reference to `dma_unmap_page'
>     drivers/built-in.o: In function `fw_iso_buffer_map_dma':
>     (.text+0x36a164): undefined reference to `dma_map_page'
>     drivers/built-in.o: In function `fw_iso_buffer_map_dma':
>     (.text+0x36a172): undefined reference to `dma_mapping_error'
>     drivers/built-in.o: In function `sbp2_send_management_orb':
>     sbp2.c:(.text+0x36c6b4): undefined reference to `dma_map_single'
>     sbp2.c:(.text+0x36c6c8): undefined reference to `dma_mapping_error'
>     sbp2.c:(.text+0x36c772): undefined reference to `dma_map_single'
>     sbp2.c:(.text+0x36c786): undefined reference to `dma_mapping_error'
>     sbp2.c:(.text+0x36c854): undefined reference to `dma_unmap_single'
>     sbp2.c:(.text+0x36c872): undefined reference to `dma_unmap_single'
>     drivers/built-in.o: In function `sbp2_map_scatterlist':
>     sbp2.c:(.text+0x36ccbc): undefined reference to `scsi_dma_map'
>     sbp2.c:(.text+0x36cd36): undefined reference to `dma_map_single'
>     sbp2.c:(.text+0x36cd4e): undefined reference to `dma_mapping_error'
>     sbp2.c:(.text+0x36cd84): undefined reference to `scsi_dma_unmap'
>     drivers/built-in.o: In function `sbp2_unmap_scatterlist':
>     sbp2.c:(.text+0x36cda6): undefined reference to `scsi_dma_unmap'
>     sbp2.c:(.text+0x36cdc6): undefined reference to `dma_unmap_single'
>     drivers/built-in.o: In function `complete_command_orb':
>     sbp2.c:(.text+0x36d6ac): undefined reference to `dma_unmap_single'
>     drivers/built-in.o: In function `sbp2_scsi_queuecommand':
>     sbp2.c:(.text+0x36d8e0): undefined reference to `dma_map_single'
>     sbp2.c:(.text+0x36d8f6): undefined reference to `dma_mapping_error'
> 
> Add an explicit dependency on HAS_DMA to fix this.
> 
> Signed-off-by: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>
> Reviewed-by: Jean Delvare <jdelvare@xxxxxxx>
> ---
>  drivers/firewire/Kconfig | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/firewire/Kconfig b/drivers/firewire/Kconfig
> index 4199849e3758..145974f9662b 100644
> --- a/drivers/firewire/Kconfig
> +++ b/drivers/firewire/Kconfig
> @@ -1,4 +1,5 @@
>  menu "IEEE 1394 (FireWire) support"
> +	depends on HAS_DMA
>  	depends on PCI || COMPILE_TEST
>  	# firewire-core does not depend on PCI but is
>  	# not useful without PCI controller driver
> 
> As a downside, this removes the ability to test-build the sound and DVB
> high-level 1394 drivers (and firewire-net) on !HAS_DMA architectures.
> But on the positive side, it is simpler.  If there are no objections,
> I am going to commit Geert's fix.
> 

What Geert has done (patch and comments) also sounds fine to me. Thank
you for all of your work.


Thanks.
-- 
Chen Gang

Open share and attitude like air water and life which God blessed
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/