Re: [PATCH] Drop ISA dependencies from IRDA drivers

Re: [PATCH] Drop ISA dependencies from IRDA drivers

[Jeff Garzik]

Andi Kleen wrote:
> http://bugme.osdl.org/show_bug.cgi?id=3077
> 
> Some IRDA chipsets currently don't work on x86-64, because
> they're dependent on CONFIG_ISA and x86-64 doesn't set this.
> CONFIG_ISA means real ISA slots, and I doubt these chips
> come on real ISA cards, so I just removed the bogus 
> dependency.

Honestly, the issue and patch need more thought, IMO.

Regardless of theory, CONFIG_ISA is currently also used to indicate 
legacy ISA devices that are today integrated into southbridges.

And with legacy ISA devices still around, I don't see a whole lot of 
value in differentiating between "I have ISA slots" and "I have ISA 
devices".

	Jeff

Re: [PATCH] Drop ISA dependencies from IRDA drivers

[Andi Kleen]

On Thu, Jul 15, 2004 at 12:48:07PM -0400, Jeff Garzik wrote:
> Andi Kleen wrote:
> >http://bugme.osdl.org/show_bug.cgi?id=3077
> >
> >Some IRDA chipsets currently don't work on x86-64, because
> >they're dependent on CONFIG_ISA and x86-64 doesn't set this.
> >CONFIG_ISA means real ISA slots, and I doubt these chips
> >come on real ISA cards, so I just removed the bogus 
> >dependency.
> 
> Honestly, the issue and patch need more thought, IMO.
> 
> Regardless of theory, CONFIG_ISA is currently also used to indicate 
> legacy ISA devices that are today integrated into southbridges.

I don't think so. I did most of the original CONFIG_ISA annotations
and I only added it to real ISA devices.

And the LPC devices in southbridges are normally not marked
CONFIG_ISA. 

> 
> And with legacy ISA devices still around, I don't see a whole lot of 
> value in differentiating between "I have ISA slots" and "I have ISA 
> devices".

There is great value. Basically most ISA drivers are not 64bit 
clean (if they even still work on i386 which is also often doubtful
in 2.6) and it is a great way for 64bit archs to get rid of a lot 
of not working code.

-Andi

Re: [PATCH] Drop ISA dependencies from IRDA drivers

[Jeff Garzik]

Andi Kleen wrote:
> On Thu, Jul 15, 2004 at 12:48:07PM -0400, Jeff Garzik wrote:
>>And with legacy ISA devices still around, I don't see a whole lot of 
>>value in differentiating between "I have ISA slots" and "I have ISA 
>>devices".
> 
> 
> There is great value. Basically most ISA drivers are not 64bit 
> clean (if they even still work on i386 which is also often doubtful
> in 2.6) and it is a great way for 64bit archs to get rid of a lot 
> of not working code.

I file that under "hiding bugs", since it will not be immediately 
obvious to a bug hunter or maintainer what the real problem is.

If a driver is broken on 64-bit, please add "&& !64BIT" to the Kconfig.

As you yourself just explained, your wish is to use CONFIG_ISA, but your 
intent is only coincedentally related to ISA.

	Jeff

Re: [PATCH] Drop ISA dependencies from IRDA drivers

[Andi Kleen]

On Thu, Jul 15, 2004 at 05:00:38PM -0400, Jeff Garzik wrote:
> >There is great value. Basically most ISA drivers are not 64bit 
> >clean (if they even still work on i386 which is also often doubtful
> >in 2.6) and it is a great way for 64bit archs to get rid of a lot 
> >of not working code.
> 
> I file that under "hiding bugs", since it will not be immediately 
> obvious to a bug hunter or maintainer what the real problem is.

They should be already aware that most ISA drivers are just 
not maintained anymore and very likely broken. I doubt there is any bug 
hunter or maintainer who is not aware of this fact.

> If a driver is broken on 64-bit, please add "&& !64BIT" to the Kconfig.
> 
> As you yourself just explained, your wish is to use CONFIG_ISA, but your 
> intent is only coincedentally related to ISA.

There are no x86-64 machines with ISA slots. I think it is very related
to ISA to disable drivers that are not used since the hardware has 
no physical mean to support it (yes, I am aware of PCMCIA, but that
is not included in CONFIG_ISA). Same reason to not support
CONFIG_EISA. LPC devices in southbridges are a different thing, but
there doesn't seem to be any reason right now to add a CONFIG_LPC.
If there was one I would have no problems with setting it.

Anyways, this is only tangential to the original reason for the patch.
Can you please drop the bogus ISA dependencies. Jean has clearly stated
that the drivers have nothing to do with ISA itself.

Here's the patch again for your convenience.

-Andi


--------------------------------------------------------------------

Remove wrong ISA dependencies for IRDA drivers.


diff -u linux-2.6.8rc1-amd64/drivers/net/irda/Kconfig-o linux-2.6.8rc1-amd64/drivers/net/irda/Kconfig
--- linux-2.6.8rc1-amd64/drivers/net/irda/Kconfig-o	2004-07-12 06:09:05.000000000 +0200
+++ linux-2.6.8rc1-amd64/drivers/net/irda/Kconfig	2004-07-15 18:33:48.000000000 +0200
@@ -310,7 +310,7 @@
 
 config NSC_FIR
 	tristate "NSC PC87108/PC87338"
-	depends on IRDA && ISA
+	depends on IRDA
 	help
 	  Say Y here if you want to build support for the NSC PC87108 and
 	  PC87338 IrDA chipsets.  This driver supports SIR,
@@ -321,7 +321,7 @@
 
 config WINBOND_FIR
 	tristate "Winbond W83977AF (IR)"
-	depends on IRDA && ISA
+	depends on IRDA
 	help
 	  Say Y here if you want to build IrDA support for the Winbond
 	  W83977AF super-io chipset.  This driver should be used for the IrDA
@@ -347,7 +347,7 @@
 
 config SMC_IRCC_FIR
 	tristate "SMSC IrCC (EXPERIMENTAL)"
-	depends on EXPERIMENTAL && IRDA && ISA
+	depends on EXPERIMENTAL && IRDA
 	help
 	  Say Y here if you want to build support for the SMC Infrared
 	  Communications Controller.  It is used in a wide variety of
@@ -357,7 +357,7 @@
 
 config ALI_FIR
 	tristate "ALi M5123 FIR (EXPERIMENTAL)"
-	depends on EXPERIMENTAL && IRDA && ISA
+	depends on EXPERIMENTAL && IRDA
 	help
 	  Say Y here if you want to build support for the ALi M5123 FIR
 	  Controller.  The ALi M5123 FIR Controller is embedded in ALi M1543C,
@@ -385,7 +385,7 @@
 
 config VIA_FIR
 	tristate "VIA VT8231/VT1211 SIR/MIR/FIR"
-	depends on IRDA && ISA
+	depends on IRDA
 	help
 	  Say Y here if you want to build support for the VIA VT8231
 	  and VIA VT1211 IrDA controllers, found on the motherboards using

Re: [PATCH] Drop ISA dependencies from IRDA drivers

[Martin Diehl]

On 15 Jul 2004, Andi Kleen wrote:

> Remove wrong ISA dependencies for IRDA drivers.
> 
> 
> diff -u linux-2.6.8rc1-amd64/drivers/net/irda/Kconfig-o linux-2.6.8rc1-amd64/drivers/net/irda/Kconfig
> --- linux-2.6.8rc1-amd64/drivers/net/irda/Kconfig-o	2004-07-12 06:09:05.000000000 +0200
> +++ linux-2.6.8rc1-amd64/drivers/net/irda/Kconfig	2004-07-15 18:33:48.000000000 +0200
> @@ -310,7 +310,7 @@
>  
>  config NSC_FIR
>  	tristate "NSC PC87108/PC87338"
> -	depends on IRDA && ISA
> +	depends on IRDA


Admittedly I haven't tried either, but I'm pretty sure this patch will 
break building those drivers because they are calling irda_setup_dma - 
which is CONFIG_ISA. Maybe this can be dropped but I don't see what's 
wrong with !64BIT instead.

Martin

Re: [PATCH] Drop ISA dependencies from IRDA drivers

[Jean Tourrilhes]

On Fri, Jul 16, 2004 at 12:32:44AM +0200, Martin Diehl wrote:
> On 15 Jul 2004, Andi Kleen wrote:
> 
> > Remove wrong ISA dependencies for IRDA drivers.
> > 
> > 
> > diff -u linux-2.6.8rc1-amd64/drivers/net/irda/Kconfig-o linux-2.6.8rc1-amd64/drivers/net/irda/Kconfig
> > --- linux-2.6.8rc1-amd64/drivers/net/irda/Kconfig-o	2004-07-12 06:09:05.000000000 +0200
> > +++ linux-2.6.8rc1-amd64/drivers/net/irda/Kconfig	2004-07-15 18:33:48.000000000 +0200
> > @@ -310,7 +310,7 @@
> >  
> >  config NSC_FIR
> >  	tristate "NSC PC87108/PC87338"
> > -	depends on IRDA && ISA
> > +	depends on IRDA
> 
> 
> Admittedly I haven't tried either, but I'm pretty sure this patch will 
> break building those drivers because they are calling irda_setup_dma - 
> which is CONFIG_ISA. Maybe this can be dropped but I don't see what's 
> wrong with !64BIT instead.

	irda_setup_dma was (supposedly) fixed in 2.6.8-rc1, and no
longer depend on CONFIG_ISA. Those driver are supposed to work on 64
bits.

> Martin

	Jean

Re: [PATCH] Drop ISA dependencies from IRDA drivers

[Martin Diehl]

On Thu, 15 Jul 2004, Jean Tourrilhes wrote:

> 	irda_setup_dma was (supposedly) fixed in 2.6.8-rc1, and no
> longer depend on CONFIG_ISA. Those driver are supposed to work on 64
> bits.

Ok, so maybe the following is missing in addition to Andi's patch?

Martin

-----------

--- linux-2.6.8-rc1/net/irda/irda_device.c	Tue Jul 13 00:31:34 2004
+++ v2.6.8-rc1-md/net/irda/irda_device.c	Fri Jul 16 00:45:08 2004
@@ -529,7 +529,6 @@ int irda_device_set_mode(struct net_devi
 	return ret;
 }
 
-#ifdef CONFIG_ISA
 /*
  * Function setup_dma (idev, buffer, count, mode)
  *
@@ -552,4 +551,3 @@ void irda_setup_dma(int channel, dma_add
 	release_dma_lock(flags);
 }
 EXPORT_SYMBOL(irda_setup_dma);
-#endif

Re: [PATCH] Drop ISA dependencies from IRDA drivers

[Andi Kleen]

On Fri, Jul 16, 2004 at 12:32:44AM +0200, Martin Diehl wrote:
> On 15 Jul 2004, Andi Kleen wrote:
> 
> > Remove wrong ISA dependencies for IRDA drivers.
> > 
> > 
> > diff -u linux-2.6.8rc1-amd64/drivers/net/irda/Kconfig-o linux-2.6.8rc1-amd64/drivers/net/irda/Kconfig
> > --- linux-2.6.8rc1-amd64/drivers/net/irda/Kconfig-o	2004-07-12 06:09:05.000000000 +0200
> > +++ linux-2.6.8rc1-amd64/drivers/net/irda/Kconfig	2004-07-15 18:33:48.000000000 +0200
> > @@ -310,7 +310,7 @@
> >  
> >  config NSC_FIR
> >  	tristate "NSC PC87108/PC87338"
> > -	depends on IRDA && ISA
> > +	depends on IRDA
> 
> 
> Admittedly I haven't tried either, but I'm pretty sure this patch will 
> break building those drivers because they are calling irda_setup_dma - 
> which is CONFIG_ISA. Maybe this can be dropped but I don't see what's 
> wrong with !64BIT instead.

Hmm, good point. 

!64BIT is not needed - apparently they are 64bit clean.

The reason I want to drop the CONFIG_ISA depency is that they *should*
be built on x86-64 too. 

-Andi

Re: [PATCH] Drop ISA dependencies from IRDA drivers

[Martin Diehl]

On 16 Jul 2004, Andi Kleen wrote:

> > Admittedly I haven't tried either, but I'm pretty sure this patch will 
> > break building those drivers because they are calling irda_setup_dma - 
> > which is CONFIG_ISA. Maybe this can be dropped but I don't see what's 
> > wrong with !64BIT instead.
> 
> Hmm, good point. 
> 
> !64BIT is not needed - apparently they are 64bit clean.

I think you are right - however, AFAICS this is not the point in this 
case. These drivers do DMA to legacy devices (call it ISA, LPC, whatever). 
The documented way for those devices without struct pci_dev is to call the 
dma api functions with dev=NULL. For i386 the generic dma functions are 
overwritten so they use GFP_DMA f.e. in this case.

According to include/asm-x86_64/dma-mapping.h there is no such override 
for x86-64. Hence the generic implementation is used which Oopses when 
called with dev=NULL in dma_alloc_coherent because it dereferences dev 
unconditionally.

> The reason I want to drop the CONFIG_ISA depency is that they *should*
> be built on x86-64 too. 

Yes, sure. The point is with current CONFIG_ISA requirement they cannot be 
build on x86-64 - with CONFIG_ISA removed they can, but will Oops. See the 
report Jean was refering to.

I agree !64BIT isn't the clean way to handle this - IMHO x86-64 needs to 
support legacy devices (dev=NULL) in its dma api implementation. If it 
doesn't, I don't see how these drivers might work on this arch.

Martin

Re: [PATCH] Drop ISA dependencies from IRDA drivers

[Andi Kleen]

On Fri, Jul 16, 2004 at 08:19:04AM +0200, Martin Diehl wrote:
> On 16 Jul 2004, Andi Kleen wrote:
> 
> > > Admittedly I haven't tried either, but I'm pretty sure this patch will 
> > > break building those drivers because they are calling irda_setup_dma - 
> > > which is CONFIG_ISA. Maybe this can be dropped but I don't see what's 
> > > wrong with !64BIT instead.
> > 
> > Hmm, good point. 
> > 
> > !64BIT is not needed - apparently they are 64bit clean.
> 
> I think you are right - however, AFAICS this is not the point in this 
> case. These drivers do DMA to legacy devices (call it ISA, LPC, whatever). 
> The documented way for those devices without struct pci_dev is to call the 
> dma api functions with dev=NULL. For i386 the generic dma functions are 
> overwritten so they use GFP_DMA f.e. in this case.


There was at least one user report that at least one driver worked 
with CONFIG_ISA force defined.

> 
> According to include/asm-x86_64/dma-mapping.h there is no such override 
> for x86-64. Hence the generic implementation is used which Oopses when 
> called with dev=NULL in dma_alloc_coherent because it dereferences dev 
> unconditionally.

The old pci_alloc_coherent supported hwdev == NULL under x86-64.
dma_alloc_consistent() should too. I will fix that. 

-Andi

Re: [PATCH] Drop ISA dependencies from IRDA drivers

[Martin Diehl]

On Fri, 16 Jul 2004, Andi Kleen wrote:

> > According to include/asm-x86_64/dma-mapping.h there is no such override 
> > for x86-64. Hence the generic implementation is used which Oopses when 
> > called with dev=NULL in dma_alloc_coherent because it dereferences dev 
> > unconditionally.
> 
> The old pci_alloc_coherent supported hwdev == NULL under x86-64.
> dma_alloc_consistent() should too. I will fix that. 

Ok, thanks. This sounds like the right solution. I think most/all 
functions in include/asm-generic/dma-mapping.h are not prepared to accept 
dev=NULL parameters, so it's a bit more than just dma_alloc_coherent() :-(

Martin

Re: [PATCH] Drop ISA dependencies from IRDA drivers

[Jean Tourrilhes]

On Thu, Jul 15, 2004 at 11:55:52PM +0200, Andi Kleen wrote:
> 
> Anyways, this is only tangential to the original reason for the patch.
> Can you please drop the bogus ISA dependencies. Jean has clearly stated
> that the drivers have nothing to do with ISA itself.

	Andy, I never said that, please quote me accurately. I
personally don't have strong opinions on whether those drivers should
be tagged with CONFIG_ISA or not, but those hardware are definitely
mapped on the ISA bus.

	Also, I just had a report of an user having a problem with the
removal of isa_virt_to_bus on x86-64 :
		http://bugme.osdl.org/show_bug.cgi?id=3073
	Depending on how this bug pans out, we *may* have to revert
the patch and brings back isa_virt_to_bus.

	Regards,

	Jean

Re: [PATCH] Drop ISA dependencies from IRDA drivers

[Andi Kleen]

On Thu, Jul 15, 2004 at 03:35:28PM -0700, Jean Tourrilhes wrote:
> On Thu, Jul 15, 2004 at 11:55:52PM +0200, Andi Kleen wrote:
> > 
> > Anyways, this is only tangential to the original reason for the patch.
> > Can you please drop the bogus ISA dependencies. Jean has clearly stated
> > that the drivers have nothing to do with ISA itself.
> 
> 	Andy, I never said that, please quote me accurately. I
> personally don't have strong opinions on whether those drivers should
> be tagged with CONFIG_ISA or not, but those hardware are definitely
> mapped on the ISA bus.

More likely on LPC interface.  And not on a ISA slot.

Anyways, if you want them to work on x86-64 you will have to drop
that bogus dependency.  I have no plans to ever define CONFIG_ISA
on x86-64.

-Andi