2005-10-15 18:58:19

by Jesse Barnes

[permalink] [raw]
Subject: ohci1394 unhandled interrupts bug in 2.6.14-rc2

I've been experiencing a bug in the Fedora kernels for awhile involving
the ohci1394 driver. If I include the driver in my initrd (causing it
to be loaded at boot time), the IRQ corresponding to my TI OHCI firewire
controller is disabled (message says "Disabling IRQ #11", I think due to
the kernel irq debug code noticing that it hasn't been handled too many
times). Since this IRQ is shared with my pcmcia, wireless, and usb
devices (don't ask me why it's wired this way, this is a Toshiba
Satellite laptop), nothing important works after the IRQ is disabled.

I've reproduced this problem under 2.6.14-rc2, which includes Al Viro's
latest fix to initialize interrupt handler spinlock (which I was hoping
would fix the problem but didn't).

Is this a known issue? Does the interrupt handler need to special case
initialization somehow and return IRQ_HANDLED even if there's no event
sometimes?

Thanks,
Jesse


2005-10-15 19:40:52

by Stefan Richter

[permalink] [raw]
Subject: Re: ohci1394 unhandled interrupts bug in 2.6.14-rc2

[email protected] wrote:
> I've been experiencing a bug in the Fedora kernels for awhile involving
> the ohci1394 driver. If I include the driver in my initrd (causing it
> to be loaded at boot time), the IRQ corresponding to my TI OHCI firewire
> controller is disabled
...
> this is a Toshiba Satellite laptop)

Somebody mentioned this Linux-on-Toshiba-Satellite page recently on
linux1394-user: http://www.janerob.com/rob/ts5100/index.shtml
The patch available from there was briefly discussed in February:
http://marc.theaimsgroup.com/?l=linux1394-devel&t=110786507900006

Does this patch correct the problem on your machine?
--
Stefan Richter
-=====-=-=-= =-=- -====
http://arcgraph.de/sr/

2005-10-15 20:33:01

by Jesse Barnes

[permalink] [raw]
Subject: Re: ohci1394 unhandled interrupts bug in 2.6.14-rc2

On Sat, Oct 15, 2005 at 09:39:06PM +0200, Stefan Richter wrote:
> Somebody mentioned this Linux-on-Toshiba-Satellite page recently on
> linux1394-user: http://www.janerob.com/rob/ts5100/index.shtml
> The patch available from there was briefly discussed in February:
> http://marc.theaimsgroup.com/?l=linux1394-devel&t=110786507900006
>
> Does this patch correct the problem on your machine?

Yes, it seems to help. If I boot up and modprobe the driver with
toshiba=1, everything looks fine (I have no firewire devices to test
with). If I modprobe it with toshiba=0, the system gets sluggish for a
second then IRQ 11 is disabled. I had to update the patch though, as
shown below.

I'm not sure if the fix is proper though, maybe this should be handled
as a PCI quirk of this Toshiba board instead? Either way, some kind of
fix should make it in soon, ideally to 2.6.14 or 2.6.14.1.

Thanks,
Jesse

diff -X linux-2.6.14-rc2/Documentation/dontdiff -Naur linux-2.6.14-rc2.orig/drivers/ieee1394/ohci1394.c linux-2.6.14-rc2/drivers/ieee1394/ohci1394.c
--- linux-2.6.14-rc2.orig/drivers/ieee1394/ohci1394.c 2005-09-19 20:00:41.000000000 -0700
+++ linux-2.6.14-rc2/drivers/ieee1394/ohci1394.c 2005-10-15 12:55:08.000000000 -0700
@@ -169,6 +169,10 @@
module_param(phys_dma, int, 0644);
MODULE_PARM_DESC(phys_dma, "Enable physical dma (default = 1).");

+static int toshiba __initdata = 0;
+module_param(toshiba, bool, 0);
+MODULE_PARM_DESC(toshiba, "Toshiba Legacy-Free BIOS workaround (default=0).");
+
static void dma_trm_tasklet(unsigned long data);
static void dma_trm_reset(struct dma_trm_ctx *d);

@@ -3222,14 +3226,28 @@
struct hpsb_host *host;
struct ti_ohci *ohci; /* shortcut to currently handled device */
unsigned long ohci_base;
+ u16 toshiba_data;

if (version_printed++ == 0)
PRINT_G(KERN_INFO, "%s", version);

+ if (toshiba) {
+ dev->current_state = 4;
+ pci_read_config_word(dev, PCI_CACHE_LINE_SIZE, &toshiba_data);
+ }
+
if (pci_enable_device(dev))
FAIL(-ENXIO, "Failed to enable OHCI hardware");
pci_set_master(dev);

+ if (toshiba) {
+ mdelay(10);
+ pci_write_config_word(dev, PCI_CACHE_LINE_SIZE, toshiba_data);
+ pci_write_config_word(dev, PCI_INTERRUPT_LINE, dev->irq);
+ pci_write_config_dword(dev, PCI_BASE_ADDRESS_0, pci_resource_start(dev, 0));
+ pci_write_config_dword(dev, PCI_BASE_ADDRESS_1, pci_resource_start(dev, 1));
+ }
+
host = hpsb_alloc_host(&ohci1394_driver, sizeof(struct ti_ohci), &dev->dev);
if (!host) FAIL(-ENOMEM, "Failed to allocate host structure");

2005-10-15 20:43:58

by Jesse Barnes

[permalink] [raw]
Subject: Re: new PCI quirk for Toshiba Satellite?

On Sat, Oct 15, 2005 at 01:29:44PM -0700, Jesse Barnes wrote:
> On Sat, Oct 15, 2005 at 09:39:06PM +0200, Stefan Richter wrote:
> > Somebody mentioned this Linux-on-Toshiba-Satellite page recently on
> > linux1394-user: http://www.janerob.com/rob/ts5100/index.shtml
> > The patch available from there was briefly discussed in February:
> > http://marc.theaimsgroup.com/?l=linux1394-devel&t=110786507900006
> >
> > Does this patch correct the problem on your machine?
>
> Yes, it seems to help. If I boot up and modprobe the driver with
> toshiba=1, everything looks fine (I have no firewire devices to test
> with). If I modprobe it with toshiba=0, the system gets sluggish for a
> second then IRQ 11 is disabled. I had to update the patch though, as
> shown below.
>
> I'm not sure if the fix is proper though, maybe this should be handled
> as a PCI quirk of this Toshiba board instead? Either way, some kind of
> fix should make it in soon, ideally to 2.6.14 or 2.6.14.1.

[Forwarding on to the PCI maintainers.]

It seems that the PCI config space isn't programmed correctly on these
machines for some reason, so the fix below allows my OHCI device to work
if I pass 'toshiba=1'. This seems like something that belongs in the
PCI layer instead though, doesn't it?

Thanks,
Jesse

> diff -X linux-2.6.14-rc2/Documentation/dontdiff -Naur linux-2.6.14-rc2.orig/drivers/ieee1394/ohci1394.c linux-2.6.14-rc2/drivers/ieee1394/ohci1394.c
> --- linux-2.6.14-rc2.orig/drivers/ieee1394/ohci1394.c 2005-09-19 20:00:41.000000000 -0700
> +++ linux-2.6.14-rc2/drivers/ieee1394/ohci1394.c 2005-10-15 12:55:08.000000000 -0700
> @@ -169,6 +169,10 @@
> module_param(phys_dma, int, 0644);
> MODULE_PARM_DESC(phys_dma, "Enable physical dma (default = 1).");
>
> +static int toshiba __initdata = 0;
> +module_param(toshiba, bool, 0);
> +MODULE_PARM_DESC(toshiba, "Toshiba Legacy-Free BIOS workaround (default=0).");
> +
> static void dma_trm_tasklet(unsigned long data);
> static void dma_trm_reset(struct dma_trm_ctx *d);
>
> @@ -3222,14 +3226,28 @@
> struct hpsb_host *host;
> struct ti_ohci *ohci; /* shortcut to currently handled device */
> unsigned long ohci_base;
> + u16 toshiba_data;
>
> if (version_printed++ == 0)
> PRINT_G(KERN_INFO, "%s", version);
>
> + if (toshiba) {
> + dev->current_state = 4;
> + pci_read_config_word(dev, PCI_CACHE_LINE_SIZE, &toshiba_data);
> + }
> +
> if (pci_enable_device(dev))
> FAIL(-ENXIO, "Failed to enable OHCI hardware");
> pci_set_master(dev);
>
> + if (toshiba) {
> + mdelay(10);
> + pci_write_config_word(dev, PCI_CACHE_LINE_SIZE, toshiba_data);
> + pci_write_config_word(dev, PCI_INTERRUPT_LINE, dev->irq);
> + pci_write_config_dword(dev, PCI_BASE_ADDRESS_0, pci_resource_start(dev, 0));
> + pci_write_config_dword(dev, PCI_BASE_ADDRESS_1, pci_resource_start(dev, 1));
> + }
> +
> host = hpsb_alloc_host(&ohci1394_driver, sizeof(struct ti_ohci), &dev->dev);
> if (!host) FAIL(-ENOMEM, "Failed to allocate host structure");
>
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/

2005-10-15 21:04:25

by Stefan Richter

[permalink] [raw]
Subject: Re: ohci1394 unhandled interrupts bug in 2.6.14-rc2

Jesse Barnes wrote:
> On Sat, Oct 15, 2005 at 09:39:06PM +0200, Stefan Richter wrote:
>>Somebody mentioned this Linux-on-Toshiba-Satellite page recently on
>>linux1394-user: http://www.janerob.com/rob/ts5100/index.shtml
>>The patch available from there was briefly discussed in February:
>>http://marc.theaimsgroup.com/?l=linux1394-devel&t=110786507900006
>
> Yes, it seems to help. If I boot up and modprobe the driver with
> toshiba=1, everything looks fine (I have no firewire devices to test
> with). If I modprobe it with toshiba=0, the system gets sluggish for a
> second then IRQ 11 is disabled. I had to update the patch though,

What about the PCI_CACHE_LINE_SIZE read/write?

Jody McIntyre wrote on 2005-02-09:
| Can you try the fix without
| pci_write_config_word(dev,PCI_CACHE_LINE_SIZE,toshiba_pcls);
| or pci_read_config_word(dev,PCI_CACHE_LINE_SIZE,&toshiba_pcls);
| and report if it still works?
|
| If it doesn't work, try leaving those lines out but adding
| pci_clear_mwi(dev);
| after the mdelay(), on the off chance that the device thinks mwi is on.
|
| The correct fix for this, if possible, is actually a pci quirk instead
| of the dmi-based approach, but if reading PCI_CACHE_LINE_SIZE before
| pci_enable_device() really is necessary, this will be rather difficult.
[ http://marc.theaimsgroup.com/?l=linux1394-devel&m=110797909807519 ]
--
Stefan Richter
-=====-=-=-= =-=- -====
http://arcgraph.de/sr/

2005-10-15 22:02:31

by Jesse Barnes

[permalink] [raw]
Subject: Re: ohci1394 unhandled interrupts bug in 2.6.14-rc2

On Sat, Oct 15, 2005 at 11:02:48PM +0200, Stefan Richter wrote:
> What about the PCI_CACHE_LINE_SIZE read/write?
>
> Jody McIntyre wrote on 2005-02-09:
> | Can you try the fix without
> | pci_write_config_word(dev,PCI_CACHE_LINE_SIZE,toshiba_pcls);
> | or pci_read_config_word(dev,PCI_CACHE_LINE_SIZE,&toshiba_pcls);
> | and report if it still works?
> |
> | If it doesn't work, try leaving those lines out but adding
> | pci_clear_mwi(dev);
> | after the mdelay(), on the off chance that the device thinks mwi is on.
> |
> | The correct fix for this, if possible, is actually a pci quirk instead
> | of the dmi-based approach, but if reading PCI_CACHE_LINE_SIZE before
> | pci_enable_device() really is necessary, this will be rather difficult.
> [ http://marc.theaimsgroup.com/?l=linux1394-devel&m=110797909807519 ]

It looks like it is.

I removed the PCI_CACHE_LINE_SIZE read and write, and that didn't work.
I added in a pci_clear_mwi(dev) and that didn't work either. It looks
like the whole patch that I posted earlier is required.

Thanks,
Jesse

2005-10-17 07:55:57

by Andrew Morton

[permalink] [raw]
Subject: Re: ohci1394 unhandled interrupts bug in 2.6.14-rc2

Jesse Barnes <[email protected]> wrote:
>
> diff -X linux-2.6.14-rc2/Documentation/dontdiff -Naur linux-2.6.14-rc2.orig/drivers/ieee1394/ohci1394.c linux-2.6.14-rc2/drivers/ieee1394/ohci1394.c
> --- linux-2.6.14-rc2.orig/drivers/ieee1394/ohci1394.c 2005-09-19 20:00:41.000000000 -0700
> +++ linux-2.6.14-rc2/drivers/ieee1394/ohci1394.c 2005-10-15 12:55:08.000000000 -0700
> @@ -169,6 +169,10 @@
> module_param(phys_dma, int, 0644);
> MODULE_PARM_DESC(phys_dma, "Enable physical dma (default = 1).");
>
> +static int toshiba __initdata = 0;
> +module_param(toshiba, bool, 0);
> +MODULE_PARM_DESC(toshiba, "Toshiba Legacy-Free BIOS workaround (default=0).");
> +
> static void dma_trm_tasklet(unsigned long data);
> static void dma_trm_reset(struct dma_trm_ctx *d);
>
> @@ -3222,14 +3226,28 @@
> struct hpsb_host *host;
> struct ti_ohci *ohci; /* shortcut to currently handled device */
> unsigned long ohci_base;
> + u16 toshiba_data;
>
> if (version_printed++ == 0)
> PRINT_G(KERN_INFO, "%s", version);
>
> + if (toshiba) {
> + dev->current_state = 4;
> + pci_read_config_word(dev, PCI_CACHE_LINE_SIZE, &toshiba_data);
> + }
> +
> if (pci_enable_device(dev))
> FAIL(-ENXIO, "Failed to enable OHCI hardware");
> pci_set_master(dev);
>
> + if (toshiba) {
> + mdelay(10);
> + pci_write_config_word(dev, PCI_CACHE_LINE_SIZE, toshiba_data);
> + pci_write_config_word(dev, PCI_INTERRUPT_LINE, dev->irq);
> + pci_write_config_dword(dev, PCI_BASE_ADDRESS_0, pci_resource_start(dev, 0));
> + pci_write_config_dword(dev, PCI_BASE_ADDRESS_1, pci_resource_start(dev, 1));
> + }
> +
> host = hpsb_alloc_host(&ohci1394_driver, sizeof(struct ti_ohci), &dev->dev);
> if (!host) FAIL(-ENOMEM, "Failed to allocate host structure");

It would be really really preferable if we could find some automatic way of
doing this. Is it possible to use DMI matching, like
arch/i386/kernel/acpi/sleep.c:acpisleep_dmi_table ?

2005-10-17 09:30:47

by Stefan Richter

[permalink] [raw]
Subject: Re: ohci1394 unhandled interrupts bug in 2.6.14-rc2

Andrew Morton wrote:
> Jesse Barnes <[email protected]> wrote:
>
>>diff -X linux-2.6.14-rc2/Documentation/dontdiff -Naur linux-2.6.14-rc2.orig/drivers/ieee1394/ohci1394.c linux-2.6.14-rc2/drivers/ieee1394/ohci1394.c
>>--- linux-2.6.14-rc2.orig/drivers/ieee1394/ohci1394.c 2005-09-19 20:00:41.000000000 -0700
>>+++ linux-2.6.14-rc2/drivers/ieee1394/ohci1394.c 2005-10-15 12:55:08.000000000 -0700
[...]
>>+module_param(toshiba, bool, 0);
>>+MODULE_PARM_DESC(toshiba, "Toshiba Legacy-Free BIOS workaround (default=0).");
[...]
> It would be really really preferable if we could find some automatic way of
> doing this.

I agree.

> Is it possible to use DMI matching, like
> arch/i386/kernel/acpi/sleep.c:acpisleep_dmi_table ?

Earlier forms of the patch do DMI matching:
http://marc.theaimsgroup.com/?l=linux1394-devel&m=110790513206094
http://www.janerob.com/rob/ts5100/tosh-1394.patch
[short-circuited by if (1) at the second URL]

Of course we don't have a complete picture of which models are affected
though.
--
Stefan Richter
-=====-=-=-= =-=- =---=
http://arcgraph.de/sr/

2005-10-17 09:43:11

by Andrew Morton

[permalink] [raw]
Subject: Re: ohci1394 unhandled interrupts bug in 2.6.14-rc2

Stefan Richter <[email protected]> wrote:
>
> Andrew Morton wrote:
> > Jesse Barnes <[email protected]> wrote:
> >
> >>diff -X linux-2.6.14-rc2/Documentation/dontdiff -Naur linux-2.6.14-rc2.orig/drivers/ieee1394/ohci1394.c linux-2.6.14-rc2/drivers/ieee1394/ohci1394.c
> >>--- linux-2.6.14-rc2.orig/drivers/ieee1394/ohci1394.c 2005-09-19 20:00:41.000000000 -0700
> >>+++ linux-2.6.14-rc2/drivers/ieee1394/ohci1394.c 2005-10-15 12:55:08.000000000 -0700
> [...]
> >>+module_param(toshiba, bool, 0);
> >>+MODULE_PARM_DESC(toshiba, "Toshiba Legacy-Free BIOS workaround (default=0).");
> [...]
> > It would be really really preferable if we could find some automatic way of
> > doing this.
>
> I agree.
>
> > Is it possible to use DMI matching, like
> > arch/i386/kernel/acpi/sleep.c:acpisleep_dmi_table ?
>
> Earlier forms of the patch do DMI matching:
> http://marc.theaimsgroup.com/?l=linux1394-devel&m=110790513206094
> http://www.janerob.com/rob/ts5100/tosh-1394.patch
> [short-circuited by if (1) at the second URL]

Rob, can you finish that patch off and send it?

> Of course we don't have a complete picture of which models are affected
> though.

I suppose we could do both. As people are found who need the module
parameter, we grab their DMI strings and add them to the table?

2005-10-17 09:59:16

by Stefan Richter

[permalink] [raw]
Subject: Re: ohci1394 unhandled interrupts bug in 2.6.14-rc2

Andrew Morton wrote:
> Stefan Richter <[email protected]> wrote:
>>Of course we don't have a complete picture of which models are affected
>>though.
>
> I suppose we could do both. As people are found who need the module
> parameter, we grab their DMI strings and add them to the table?

Jesse, what DMI_PRODUCT_NAME matches your laptop?
--
Stefan Richter
-=====-=-=-= =-=- =---=
http://arcgraph.de/sr/

2005-10-17 12:48:47

by rob

[permalink] [raw]
Subject: Re: ohci1394 unhandled interrupts bug in 2.6.14-rc2


On Mon, 17 Oct 2005 02:42:19 -0700, Andrew Morton wrote
> Stefan Richter <[email protected]> wrote:

> > Earlier forms of the patch do DMI matching:
> > http://marc.theaimsgroup.com/?l=linux1394-devel&m=110790513206094
> > http://www.janerob.com/rob/ts5100/tosh-1394.patch
> > [short-circuited by if (1) at the second URL]
>
> Rob, can you finish that patch off and send it?

Sorry, I was advised that this should be correctly handled as a pci-quirk
(Jody McIntyre <[email protected]>), and subsequently my laptop
motherboard died so I have no way of taking it further. The responses I got
indicated that the code works as is for the followiung laptops


> System Vendor: TOSHIBA
> Product Name: S5100-501
> Version: PS510E-00NV7-EN


System Vendor: TOSHIBA
Product Name: S5200-801
Version: PS520E-31P1D-GR

Manufacturer: TOSHIBA
Product Name: Satellite 5200
Version: PS520C-31P0EP

Manufacturer: TOSHIBA
Product Name: Satellite 5205
Version: PS522U-XK00YV

Manufacturer: TOSHIBA
Product Name: S5100-603
Version: PS511E-05328-GR

toshiba satellite 5005-S504

Toshiba Satellite 5105-s607


rob.


--
Open WebMail Project (http://openwebmail.org)



2005-10-17 15:53:51

by Stefan Richter

[permalink] [raw]
Subject: Re: ohci1394 unhandled interrupts bug in 2.6.14-rc2

rob wrote:
> On Mon, 17 Oct 2005 02:42:19 -0700, Andrew Morton wrote
>>Stefan Richter <[email protected]> wrote:
>>>Earlier forms of the patch do DMI matching:
>>>http://marc.theaimsgroup.com/?l=linux1394-devel&m=110790513206094
>>>http://www.janerob.com/rob/ts5100/tosh-1394.patch
>>>[short-circuited by if (1) at the second URL]
>>
>>Rob, can you finish that patch off and send it?
>
> Sorry, I was advised that this should be correctly handled as a pci-quirk
> (Jody McIntyre <[email protected]>),

Since Jesse found that we really need to read & write back the
PCI_CACHE_LINE_SIZE, I gather the workaround has to stay in ohci1394
(and should be triggered by dmi_check_system), doesn't it?

> and subsequently my laptop
> motherboard died so I have no way of taking it further.

We should be able to finish it.

> The responses I got
> indicated that the code works as is for the followiung laptops
>
>>System Vendor: TOSHIBA
>>Product Name: S5100-501
>>Version: PS510E-00NV7-EN
>
> System Vendor: TOSHIBA
> Product Name: S5200-801
> Version: PS520E-31P1D-GR
>
> Manufacturer: TOSHIBA
> Product Name: Satellite 5200
> Version: PS520C-31P0EP
>
> Manufacturer: TOSHIBA
> Product Name: Satellite 5205
> Version: PS522U-XK00YV
>
> Manufacturer: TOSHIBA
> Product Name: S5100-603
> Version: PS511E-05328-GR
>
> toshiba satellite 5005-S504
>
> Toshiba Satellite 5105-s607

Thanks a lot for the survey. Do they all _need_ the patch, or do some of
them need it and the others are just not harmed by the patch?

Does anybody know a DMI_PRODUCT_NAME of a Libretto L1? Something like
PAL1060TNMM or PAL1060TNCM?
--
Stefan Richter
-=====-=-=-= =-=- =---=
http://arcgraph.de/sr/


2005-10-17 16:30:59

by Jesse Barnes

[permalink] [raw]
Subject: Re: ohci1394 unhandled interrupts bug in 2.6.14-rc2

On Monday, October 17, 2005 3:03 am, Stefan Richter wrote:
> Andrew Morton wrote:
> > Stefan Richter <[email protected]> wrote:
> >>Of course we don't have a complete picture of which models are
> >> affected though.
> >
> > I suppose we could do both. As people are found who need the module
> > parameter, we grab their DMI strings and add them to the table?
>
> Jesse, what DMI_PRODUCT_NAME matches your laptop?

I'll have to check when I get home, is the relevant info from the "System
Information" section of the dmidecode output?

Jesse

2005-10-17 18:53:06

by Stefan Richter

[permalink] [raw]
Subject: Re: ohci1394 unhandled interrupts bug in 2.6.14-rc2

Jesse Barnes wrote:
>>Jesse, what DMI_PRODUCT_NAME matches your laptop?
>
> I'll have to check when I get home, is the relevant info from the "System
> Information" section of the dmidecode output?

AFAICT yes. Although I wonder if there is a better suited part of the
DMI table to look at. From what Rob posted, we could simply match
{DMI_SYS_VENDOR, "TOSHIBA"} && {DMI_PRODUCT_VERSION, "PS5"} which
probably catches all Satellite 5xxx's. I hope this isn't too general.

I also wonder how 1394 CardBus cards would react on this patch in
affected Toshibas...

| + toshiba = dmi_check_system(extra_init_dmi_table);
| + if (toshiba) {
| + PRINT_G(KERN_INFO, "Toshiba %s detected, enabling extra
initialization code",
| + dmi_get_system_info(DMI_PRODUCT_NAME));
| + dev->current_state = 4;
| + pci_read_config_word(dev, PCI_CACHE_LINE_SIZE, &toshiba_pcls);
| + }
...
| if (pci_enable_device(dev))
| FAIL(-ENXIO, "Failed to enable OHCI hardware");
...
| + if (toshiba) {
| + mdelay(10);
| + pci_write_config_word(dev, PCI_CACHE_LINE_SIZE, toshiba_pcls);
| + pci_write_config_word(dev, PCI_INTERRUPT_LINE, dev->irq);
| + pci_write_config_dword(dev, PCI_BASE_ADDRESS_0,
pci_resource_start(dev, 0));
| + pci_write_config_dword(dev, PCI_BASE_ADDRESS_1,
pci_resource_start(dev, 1));
| + }
...
| pci_set_master(dev);

--
Stefan Richter
-=====-=-=-= =-=- =---=
http://arcgraph.de/sr/

2005-10-18 05:33:57

by rob

[permalink] [raw]
Subject: Re: ohci1394 unhandled interrupts bug in 2.6.14-rc2

On Mon, 17 Oct 2005 17:58:10 +0200, Stefan Richter wrote
...
> > Toshiba Satellite 5105-s607
>
> Thanks a lot for the survey. Do they all _need_ the patch, or do
> some of them need it and the others are just not harmed by the patch?

they all need the patch to modprobe without errors, not all had firewire
devices to test with.

> Does anybody know a DMI_PRODUCT_NAME of a Libretto L1? Something
> like PAL1060TNMM or PAL1060TNCM?

Might try John Belmonte <john at neggie.net>, he has an L5 and runs a website
for it.

rob.


--
Open WebMail Project (http://openwebmail.org)

2005-10-19 17:54:11

by Jesse Barnes

[permalink] [raw]
Subject: Re: ohci1394 unhandled interrupts bug in 2.6.14-rc2

On Monday, October 17, 2005 11:50 am, Stefan Richter wrote:
> Jesse Barnes wrote:
> >>Jesse, what DMI_PRODUCT_NAME matches your laptop?
> >
> > I'll have to check when I get home, is the relevant info from the
> > "System Information" section of the dmidecode output?
>
> AFAICT yes. Although I wonder if there is a better suited part of the
> DMI table to look at. From what Rob posted, we could simply match
> {DMI_SYS_VENDOR, "TOSHIBA"} && {DMI_PRODUCT_VERSION, "PS5"} which
> probably catches all Satellite 5xxx's. I hope this isn't too general.
>
> I also wonder how 1394 CardBus cards would react on this patch in
> affected Toshibas...

It should be harmless, right? I don't have any way of testing since I
don't have such a card myself. Here's the dmidecode output from my
machine.

Thanks,
Jesse

# dmidecode 2.7
SMBIOS 2.3 present.
45 structures occupying 1419 bytes.
Table at 0x000EB160.

Handle 0x0000, DMI type 0, 20 bytes.
BIOS Information
Vendor: TOSHIBA
Version: Version 1.20
Release Date: 01/24/2005
Address: 0xEB000
Runtime Size: 84 kB
ROM Size: 512 kB
Characteristics:
ISA is supported
PCI is supported
PC Card (PCMCIA) is supported
PNP is supported
APM is supported
BIOS is upgradeable
BIOS shadowing is allowed
Boot from CD is supported
BIOS ROM is socketed
EDD is supported
Japanese floppy for NEC 9800 1.2 MB is supported (int
13h)
3.5"/720 KB floppy services are supported (int 13h)
3.5"/2.88 MB floppy services are supported (int 13h)
Print screen service is supported (int 5h)
8042 keyboard services are supported (int 9h)
Serial services are supported (int 14h)
Printer services are supported (int 17h)
CGA/mono video services are supported (int 10h)
ACPI is supported
USB legacy is supported
LS-120 boot is supported
ATAPI Zip drive boot is supported
Smart battery is supported
Function key-initiated network boot is supported

Handle 0x0001, DMI type 1, 25 bytes.
System Information
Manufacturer: TOSHIBA
Product Name: Satellite M45
Version: PSM40U-01X001
Serial Number: 15106102Q
UUID: 60178840-7169-11D9-A70E-00A0D1BB8852
Wake-up Type: Power Switch

Handle 0x0002, DMI type 2, 17 bytes.
Base Board Information
Manufacturer: TOSHIBA
Product Name: Portable PC
Version: Version A0
Serial Number: 0000000000
Asset Tag: Not Specified
Features: None
Location In Chassis: Not Specified
Chassis Handle: 0x0000
Type: Other
Contained Object Handles: 0

Handle 0x0003, DMI type 3, 17 bytes.
Chassis Information
Manufacturer: TOSHIBA
Type: Portable
Lock: Not Present
Version: Version 1.0
Serial Number: 00000000
Asset Tag: 0000000000
Boot-up State: Safe
Power Supply State: Safe
Thermal State: Safe
Security Status: None
OEM Information: 0x00000000

Handle 0x0004, DMI type 4, 32 bytes.
Processor Information
Socket Designation: mFCPGA
Type: Central Processor
Family: Pentium M
Manufacturer: GenuineIntelation
ID: FF F9 E9 AF 00 00 00 00
Signature: Type 3, Family 4073, Model 159, Stepping 15
Flags: None
Version: Pentium(R) M
Voltage: 2.9 V
External Clock: 133 MHz
Max Speed: 1733 MHz
Current Speed: 1733 MHz
Status: Populated, Enabled
Upgrade: None
L1 Cache Handle: 0x0005
L2 Cache Handle: 0x0006
L3 Cache Handle: Not Provided

Handle 0x0005, DMI type 7, 19 bytes.
Cache Information
Socket Designation: L1 Cache
Configuration: Enabled, Not Socketed, Level 1
Operational Mode: Write Back
Location: Internal
Installed Size: 32 KB
Maximum Size: 32 KB
Supported SRAM Types:
Burst
Pipeline Burst
Asynchronous
Installed SRAM Type: Burst
Speed: Unknown
Error Correction Type: None
System Type: Unknown
Associativity: Unknown

Handle 0x0006, DMI type 7, 19 bytes.
Cache Information
Socket Designation: L2 Cache
Configuration: Enabled, Not Socketed, Level 2
Operational Mode: Write Back
Location: External
Installed Size: 2048 KB
Maximum Size: 2048 KB
Supported SRAM Types:
Burst
Pipeline Burst
Asynchronous
Installed SRAM Type: Burst
Speed: Unknown
Error Correction Type: None
System Type: Unknown
Associativity: Unknown

Handle 0x0007, DMI type 8, 9 bytes.
Port Connector Information
Internal Reference Designator: Serial Port
Internal Connector Type: None
External Reference Designator: J16
External Connector Type: DB-9 male
Port Type: Serial Port 16550A Compatible

Handle 0x0008, DMI type 8, 9 bytes.
Port Connector Information
Internal Reference Designator: Parallel Port
Internal Connector Type: None
External Reference Designator: J20
External Connector Type: DB-25 male
Port Type: Parallel Port ECP/EPP

Handle 0x0009, DMI type 8, 9 bytes.
Port Connector Information
Internal Reference Designator: Kbd Connector
Internal Connector Type: None
External Reference Designator: J8
External Connector Type: PS/2
Port Type: Keyboard Port

Handle 0x000A, DMI type 8, 9 bytes.
Port Connector Information
Internal Reference Designator: Mouse Conn.
Internal Connector Type: None
External Reference Designator: J9
External Connector Type: PS/2
Port Type: Mouse Port

Handle 0x000B, DMI type 8, 9 bytes.
Port Connector Information
Internal Reference Designator: USB Conn.
Internal Connector Type: None
External Reference Designator: J12
External Connector Type: Other
Port Type: USB

Handle 0x000C, DMI type 8, 9 bytes.
Port Connector Information
Internal Reference Designator: Dock Conn.
Internal Connector Type: None
External Reference Designator: J6
External Connector Type: Other
Port Type: Other

Handle 0x000D, DMI type 8, 9 bytes.
Port Connector Information
Internal Reference Designator: VGA Conn.
Internal Connector Type: None
External Reference Designator: J7
External Connector Type: Other
Port Type: Other

Handle 0x000E, DMI type 8, 9 bytes.
Port Connector Information
Internal Reference Designator: DC Input
Internal Connector Type: None
External Reference Designator: J35
External Connector Type: Other
Port Type: Other

Handle 0x000F, DMI type 8, 9 bytes.
Port Connector Information
Internal Reference Designator: IrDA Port
Internal Connector Type: None
External Reference Designator: U1
External Connector Type: Infrared
Port Type: Other

Handle 0x0010, DMI type 8, 9 bytes.
Port Connector Information
Internal Reference Designator: CardBus Conn.
Internal Connector Type: None
External Reference Designator: J26
External Connector Type: Other
Port Type: Cardbus

Handle 0x0011, DMI type 9, 13 bytes.
System Slot Information
Designation: ISA Slot
Type: 16-bit ISA
Current Usage: Available
Length: Long
Characteristics: Unknown

Handle 0x0012, DMI type 9, 13 bytes.
System Slot Information
Designation: PCI Slot
Type: 32-bit PCI
Current Usage: Available
Length: Long
ID: 4
Characteristics: Unknown

Handle 0x0013, DMI type 10, 16 bytes.
On Board Device 1 Information
Type: Other
Status: Disabled
Description: ECP Port
On Board Device 2 Information
Type: Other
Status: Disabled
Description: 16550 UART
On Board Device 3 Information
Type: Other
Status: Disabled
Description: IrDA Port
On Board Device 4 Information
Type: Other
Status: Enabled
Description: CardBus Bridge
On Board Device 5 Information
Type: Other
Status: Enabled
Description: IDE Controller
On Board Device 6 Information
Type: Video
Status: Enabled
Description: Itl

Handle 0x0014, DMI type 11, 5 bytes.
OEM Strings
String 1: PSM40U-01X001,SQ003520,11V

Handle 0x0015, DMI type 12, 5 bytes.
System Configuration Options
Option 1: TOSHIBA

Handle 0x0016, DMI type 13, 22 bytes.
BIOS Language Information
Installable Languages: 1
en|US|iso8859-1
Currently Installed Language: en|US|iso8859-1

Handle 0x0017, DMI type 16, 15 bytes.
Physical Memory Array
Location: System Board Or Motherboard
Use: System Memory
Error Correction Type: None
Maximum Capacity: 2 GB
Error Information Handle: 0x001D
Number Of Devices: 2

Handle 0x0018, DMI type 16, 15 bytes.
Physical Memory Array
Location: System Board Or Motherboard
Use: Video Memory
Error Correction Type: None
Maximum Capacity: 64 MB
Error Information Handle: Not Provided
Number Of Devices: 1

Handle 0x0019, DMI type 16, 15 bytes.
Physical Memory Array
Location: System Board Or Motherboard
Use: Flash Memory
Error Correction Type: None
Maximum Capacity: 512 kB
Error Information Handle: Not Provided
Number Of Devices: 1

Handle 0x001A, DMI type 16, 15 bytes.
Physical Memory Array
Location: System Board Or Motherboard
Use: Cache Memory
Error Correction Type: None
Maximum Capacity: 2 MB
Error Information Handle: Not Provided
Number Of Devices: 1

Handle 0x001B, DMI type 17, 23 bytes.
Memory Device
Array Handle: 0x0017
Error Information Handle: Not Provided
Total Width: 64 bits
Data Width: 64 bits
Size: 512 MB
Form Factor: SODIMM
Set: Unknown
Locator: DRAM Slot 0
Bank Locator: Banks 0/1
Type: DDR
Type Detail: EDO
Speed: 333 MHz (3.0 ns)

Handle 0x001C, DMI type 17, 23 bytes.
Memory Device
Array Handle: 0x0017
Error Information Handle: Not Provided
Total Width: 64 bits
Data Width: 64 bits
Size: No Module Installed
Form Factor: SODIMM
Set: Unknown
Locator: DRAM Slot 1
Bank Locator: Banks 2/3
Type: DDR
Type Detail: EDO
Speed: 333 MHz (3.0 ns)

Handle 0x001D, DMI type 18, 23 bytes.
32-bit Memory Error Information
Type: Unknown
Granularity: Memory Partition Level
Operation: Unknown
Vendor Syndrome: Unknown
Memory Array Address: Unknown
Device Address: Unknown
Resolution: Unknown

Handle 0x001E, DMI type 19, 15 bytes.
Memory Array Mapped Address
Starting Address: 0x00000000000
Ending Address: 0x0001FFFFFFF
Range Size: 512 MB
Physical Array Handle: 0x0017
Partition Width: 0

Handle 0x001F, DMI type 20, 19 bytes.
Memory Device Mapped Address
Starting Address: 0x00000000000
Ending Address: 0x00003FFFFFF
Range Size: 64 MB
Physical Device Handle: 0x001B
Memory Array Mapped Address Handle: 0x0003
Partition Row Position: <OUT OF SPEC>

Handle 0x0020, DMI type 20, 19 bytes.
Memory Device Mapped Address
Starting Address: 0x00000000000
Ending Address: 0x00003FFFFFF
Range Size: 64 MB
Physical Device Handle: 0x001C
Memory Array Mapped Address Handle: 0x0003
Partition Row Position: <OUT OF SPEC>

Handle 0x0021, DMI type 21, 7 bytes.
Built-in Pointing Device
Type: Mouse
Interface: PS/2
Buttons: 2

Handle 0x0022, DMI type 22, 26 bytes.
Portable Battery
Location: Smart Battery Conn J37
Manufacturer: Duracell
Manufacture Date: 01/06/97
Serial Number: 0042
Name: DR36
Design Capacity: Unknown
Design Voltage: 12 mV
SBDS Version: V1.0
Maximum Error: Unknown
SBDS Chemistry: Not Specified
OEM-specific Information: 0x00000000

Handle 0x0023, DMI type 23, 13 bytes.
System Reset
Status: Disabled
Watchdog Timer: Not Present

Handle 0x0024, DMI type 24, 5 bytes.
Hardware Security
Power-On Password Status: Disabled
Keyboard Password Status: Not Implemented
Administrator Password Status: Disabled
Front Panel Reset Status: Not Implemented

Handle 0x0025, DMI type 25, 9 bytes.
System Power Controls
Next Scheduled Power-on: *-* 00:00:00

Handle 0x0026, DMI type 26, 22 bytes.
Voltage Probe
Description: Voltage Probe Description
Location: Unknown
Status: Unknown
Maximum Value: Unknown
Minimum Value: Unknown
Resolution: Unknown
Tolerance: Unknown
Accuracy: Unknown
OEM-specific Information: 0x00000000
Nominal Value: 0.000 V

Handle 0x0027, DMI type 27, 14 bytes.
Cooling Device
Temperature Probe Handle: 0x0028
Type: Unknown
Status: Unknown
OEM-specific Information: 0x00000000
Nominal Speed: 0 rpm

Handle 0x0028, DMI type 28, 22 bytes.
Temperature Probe
Description: Temperature Probe Description
Location: Unknown
Status: Unknown
Maximum Value: Unknown
Minimum Value Unknown
Resolution: Unknown
Tolerance: Unknown
Accuracy: Unknown
OEM-specific Information: 0x00000000
Nominal Value: 0.0 deg C

Handle 0x0029, DMI type 29, 22 bytes.
Electrical Current Probe
Description: Electrical Probe Description
Location: Unknown
Status: Unknown
Maximum Value: Unknown
Minimum Value: Unknown
Resolution: Unknown
Tolerance: Unknown
Accuracy: Unknown
OEM-specific Information: 0x00000000
Nominal Value: 0.000 A

Handle 0x002A, DMI type 30, 6 bytes.
Out-of-band Remote Access
Manufacturer Name: Manufacturer Name
Inbound Connection: Disabled
Outbound Connection: Disabled

Handle 0x002B, DMI type 32, 11 bytes.
System Boot Information
Status: No errors detected

Handle 0x002C, DMI type 127, 4 bytes.
End Of Table

2005-10-20 00:33:43

by Greg KH

[permalink] [raw]
Subject: Re: new PCI quirk for Toshiba Satellite?

On Sat, Oct 15, 2005 at 01:40:40PM -0700, Jesse Barnes wrote:
> On Sat, Oct 15, 2005 at 01:29:44PM -0700, Jesse Barnes wrote:
> > On Sat, Oct 15, 2005 at 09:39:06PM +0200, Stefan Richter wrote:
> > > Somebody mentioned this Linux-on-Toshiba-Satellite page recently on
> > > linux1394-user: http://www.janerob.com/rob/ts5100/index.shtml
> > > The patch available from there was briefly discussed in February:
> > > http://marc.theaimsgroup.com/?l=linux1394-devel&t=110786507900006
> > >
> > > Does this patch correct the problem on your machine?
> >
> > Yes, it seems to help. If I boot up and modprobe the driver with
> > toshiba=1, everything looks fine (I have no firewire devices to test
> > with). If I modprobe it with toshiba=0, the system gets sluggish for a
> > second then IRQ 11 is disabled. I had to update the patch though, as
> > shown below.
> >
> > I'm not sure if the fix is proper though, maybe this should be handled
> > as a PCI quirk of this Toshiba board instead? Either way, some kind of
> > fix should make it in soon, ideally to 2.6.14 or 2.6.14.1.
>
> [Forwarding on to the PCI maintainers.]
>
> It seems that the PCI config space isn't programmed correctly on these
> machines for some reason, so the fix below allows my OHCI device to work
> if I pass 'toshiba=1'. This seems like something that belongs in the
> PCI layer instead though, doesn't it?

Yes, looks like it should be a pci quirk instead.

thanks,

greg k-h

2005-10-20 18:35:45

by Stefan Richter

[permalink] [raw]
Subject: Re: new PCI quirk for Toshiba Satellite?

Greg KH wrote:
> On Sat, Oct 15, 2005 at 01:40:40PM -0700, Jesse Barnes wrote:
>>>On Sat, Oct 15, 2005 at 09:39:06PM +0200, Stefan Richter wrote:
>>>>Somebody mentioned this Linux-on-Toshiba-Satellite page recently on
>>>>linux1394-user: http://www.janerob.com/rob/ts5100/index.shtml
>>>>The patch available from there was briefly discussed in February:
>>>>http://marc.theaimsgroup.com/?l=linux1394-devel&t=110786507900006
...
>>It seems that the PCI config space isn't programmed correctly on these
>>machines for some reason, so the fix below allows my OHCI device to work
>>if I pass 'toshiba=1'. This seems like something that belongs in the
>>PCI layer instead though, doesn't it?
>
> Yes, looks like it should be a pci quirk instead.

I gather from DMI info provided by Rob and Jesse that affected machines
could be determined by dmi_check_system() using these two checks:
- DMI_SYS_VENDOR contains "TOSHIBA" && DMI_PRODUCT_VERSION contains
"PS5", i.e. most of Satellite 5xxx, as well as
- DMI_SYS_VENDOR contains "TOSHIBA" && DMI_PRODUCT_VERSION contains
"PSM4", i.e. all Satellite M4x.
A few more series are probably affected too.

http://marc.theaimsgroup.com/?l=linux-kernel&m=112955338318326
http://marc.theaimsgroup.com/?l=linux-kernel&m=112974457930368
http://linux.toshiba-dme.co.jp/linux/eng/speclist.php3

BTW, the author of the initial incarnation of the patch is this person:
http://nemaru.at.infoseek.co.jp/1394.html
--
Stefan Richter
-=====-=-=-= =-=- =-=--
http://arcgraph.de/sr/

2005-10-21 18:39:12

by Jesse Barnes

[permalink] [raw]
Subject: Re: new PCI quirk for Toshiba Satellite?

On Thursday, October 20, 2005 11:32 am, Stefan Richter wrote:
> Greg KH wrote:
> > On Sat, Oct 15, 2005 at 01:40:40PM -0700, Jesse Barnes wrote:
> >> It seems that the PCI config space isn't programmed correctly on
> >> these machines for some reason, so the fix below allows my OHCI
> >> device to work if I pass 'toshiba=1'. This seems like something
> >> that belongs in the PCI layer instead though, doesn't it?
> >
> > Yes, looks like it should be a pci quirk instead.
>
> I gather from DMI info provided by Rob and Jesse that affected
> machines could be determined by dmi_check_system() using these two
> checks: - DMI_SYS_VENDOR contains "TOSHIBA" && DMI_PRODUCT_VERSION
> contains "PS5", i.e. most of Satellite 5xxx, as well as
> - DMI_SYS_VENDOR contains "TOSHIBA" && DMI_PRODUCT_VERSION contains
> "PSM4", i.e. all Satellite M4x.
> A few more series are probably affected too.
>
> http://marc.theaimsgroup.com/?l=linux-kernel&m=112955338318326
> http://marc.theaimsgroup.com/?l=linux-kernel&m=112974457930368
> http://linux.toshiba-dme.co.jp/linux/eng/speclist.php3
>
> BTW, the author of the initial incarnation of the patch is this
> person: http://nemaru.at.infoseek.co.jp/1394.html

Stefan, is a PCI quirk addition possible or do we have to use
dmi_check_system in the ohci driver itself (since we have to reprogram
the cache line size in addition to the other registers)?

Something like this is what you had in mind (please forgive the word
wrapping)? In ohci1394.c somewhere:

> struct tosh_config_data {
> struct pci_dev *dev;
> u16 cache_line_size;
> };
>
> int ohci1394_toshiba_reprogram_config(struct dmi_system_id *d)
> {
> struct tosh_config_data *data = d->driver_data;
>
> mdelay(10);
> pci_write_config_word(data->dev, PCI_CACHE_LINE_SIZE,
> data->cache_line_size);
> pci_write_config_word(data->dev, PCI_INTERRUPT_LINE, data->dev->irq);
> pci_write_config_dword(data->dev, PCI_BASE_ADDRESS_0,
> pci_resource_start(data->dev, 0));
> pci_write_config_dword(data->dev, PCI_BASE_ADDRESS_1,
> pci_resource_start(data->dev, 1));
> }

And then in ohci1394_pci_probe():

> struct tosh_config_data tosh_data;
> tosh_data.dev = dev;
> pci_read_config_word(dev, PCI_CACHE_LINE_SIZE,
> &tosh_data.cache_line_size);
> static struct dmi_system_id __initdata toshiba_ohci1394_dmi_table[] =
> { { /* Handle lack of config space programming on Toshiba laptops */
> .callback = ohci1394_toshiba_reprogram_config,
> .ident = "Toshiba PS5 based laptop",
> .matches = {
> DMI_MATCH(DMI_SYS_VENDOR, "TOSHIBA"),
> DMI_MATCH(DMI_PRODUCT_VERSION, "PS5"),
> },
> .driver_data = &tosh_data;
> },
> {
> .callback = ohci1394_toshiba_reprogram_config,
> .ident = "Toshiba PSM4 based laptop",
> .matches = {
> DMI_MATCH(DMI_SYS_VENDOR, "TOSHIBA"),
> DMI_MATCH(DMI_PRODUCT_VERSION, "PSM4"),
> },
> .driver_data = &tosh_data;
> },
> }
>
> dmi_check_system(toshiba_ohci1394_dmi_table);

But then what about the dev->current_state = 4? Is that necessary?

And is anyone going to put together a patch for this, or should I do it?
It would be great to have fixed either in 2.6.14 or 2.6.14.1 given that
it breaks my laptop everytime I upgrade my kernel and forget to delete
the ieee1394 devices from my initrd.

Jody or Ben, any comment? I've attached the patch I most recently tested
for reference.

Thanks,
Jesse


Attachments:
(No filename) (3.32 kB)
ohci-toshiba-fix-2.patch (1.61 kB)
Download all attachments

2005-10-21 20:16:06

by Stefan Richter

[permalink] [raw]
Subject: Re: new PCI quirk for Toshiba Satellite?

Jesse Barnes wrote:
> Stefan, is a PCI quirk addition possible or do we have to use
> dmi_check_system in the ohci driver itself (since we have to reprogram
> the cache line size in addition to the other registers)?

I am not familiar with the PCI subsystem, thus cannot advise how to
handle it best nor wanted to post a patch myself (yet).

[...]
>> .callback = ohci1394_toshiba_reprogram_config,
>> .ident = "Toshiba PSM4 based laptop",
>> .matches = {
>> DMI_MATCH(DMI_SYS_VENDOR, "TOSHIBA"),
>> DMI_MATCH(DMI_PRODUCT_VERSION, "PSM4"),
>> },
>> .driver_data = &tosh_data;

It seems to me, using the .callback and .driver_data doesn't make it
cleaner and leaner.

> But then what about the dev->current_state = 4? Is that necessary?

It is necessary; at least if the workaround resides in ohci1394.
Otherwise the controller won't come back after a suspend/ resume cycle.
(See Rob's post from February,
http://marc.theaimsgroup.com/?m=110786495210243 ) Maybe there is another
way to do that if the workaround was moved to pci/quirks.c.

[...]
> + if (toshiba) {
> + dev->current_state = 4;
> + pci_read_config_word(dev, PCI_CACHE_LINE_SIZE, &toshiba_data);
> + }
> +
> if (pci_enable_device(dev))
> FAIL(-ENXIO, "Failed to enable OHCI hardware");
> pci_set_master(dev);
>
> + if (toshiba) {
> + mdelay(10);
> + pci_write_config_word(dev, PCI_CACHE_LINE_SIZE, toshiba_data);
[...]

pci_set_master(dev) can be moved below the second part of the Toshiba
workaround. That means AFAIU, the 2nd part of the Toshiba workaround can
be moved out of ohci1394 into pci_fixup_device() which is called from
pci_enable_device(), to be called as a DECLARE_PCI_FIXUP_ENABLE hook.

The first part of the workaround, i.e. caching the cache line size, for
example by means of a static variable, would have to go into an
_FIXUP_EARLY, _FIXUP_HEADER, or _FIXUP_FINAL hook. I am not sure yet
about which type of hook to use.

Furthermore, everything which belongs to the workaround should IMO be
enclosed by #ifdef SOME_SENSIBLE_MACRO. This avoids kernel bloat for any
target which is surely not a Toshiba laptop. Rob used an #if
defined(__i386__).
--
Stefan Richter
-=====-=-=-= =-=- =-=-=
http://arcgraph.de/sr/

2005-10-24 17:45:30

by Jesse Barnes

[permalink] [raw]
Subject: Re: new PCI quirk for Toshiba Satellite?

On Friday, October 21, 2005 1:13 pm, Stefan Richter wrote:
> Jesse Barnes wrote:
> > Stefan, is a PCI quirk addition possible or do we have to use
> > dmi_check_system in the ohci driver itself (since we have to
> > reprogram the cache line size in addition to the other registers)?
>
> I am not familiar with the PCI subsystem, thus cannot advise how to
> handle it best nor wanted to post a patch myself (yet).

Ok, I'll update Rob's patch to the latest tree and send it on (I hadn't
seen it yet when I wrote my version).

> It seems to me, using the .callback and .driver_data doesn't make it
> cleaner and leaner.

I agree, I thought it was necessary since I hadn't looked at
dmi_check_system and didn't know if it had a return value I could check.
Rob's patch looks much simpler and nicer.

> > But then what about the dev->current_state = 4? Is that necessary?
>
> It is necessary; at least if the workaround resides in ohci1394.
> Otherwise the controller won't come back after a suspend/ resume
> cycle. (See Rob's post from February,
> http://marc.theaimsgroup.com/?m=110786495210243 ) Maybe there is
> another way to do that if the workaround was moved to pci/quirks.c.

Having it all in the driver probably makes the most sense if we have to
have code there anyway. Otherwise future users may have to check both
places if things break again in another configuration.

> Furthermore, everything which belongs to the workaround should IMO be
> enclosed by #ifdef SOME_SENSIBLE_MACRO. This avoids kernel bloat for
> any target which is surely not a Toshiba laptop. Rob used an #if
> defined(__i386__).

Checks against the compiler defined arch are usually wrong since users
could be cross compiling, and I'd like to avoid an ifdef altogether. I
think we can make the code collapse entirely by fixing linux/dmi.h. If
we remove the !defined(CONFIG_X86_64) check around the extern of
dmi_check_system, all other arches will have it defined to simply return
0, causing gcc to remove the dead conditional in ohci1394.c.

Anyway, I'll refresh that patch, test it, and send it on to Andrew,
hopefully tonight.

Thanks,
Jesse

2005-10-24 18:08:09

by Jesse Barnes

[permalink] [raw]
Subject: Re: new PCI quirk for Toshiba Satellite?

On Monday, October 24, 2005 10:45 am, Jesse Barnes wrote:
> Checks against the compiler defined arch are usually wrong since users
> could be cross compiling, and I'd like to avoid an ifdef altogether.
> I think we can make the code collapse entirely by fixing linux/dmi.h.
> If we remove the !defined(CONFIG_X86_64) check around the extern of
> dmi_check_system, all other arches will have it defined to simply
> return 0, causing gcc to remove the dead conditional in ohci1394.c.

Duh, I don't even think we have to do anything to dmi.h, it should work
as described above as it stands now. All we have to do is
unconditionally add dmi.h to ohci1394.c and use the dmi stuff there; on
x86 it'll do something, on other arches it should compile out just fine.

Jesse

2005-10-24 18:22:44

by Stefan Richter

[permalink] [raw]
Subject: Re: new PCI quirk for Toshiba Satellite?

Jesse Barnes wrote:
> On Friday, October 21, 2005 1:13 pm, Stefan Richter wrote:
>>Jesse Barnes wrote:
>>>But then what about the dev->current_state = 4? Is that necessary?
>>
>>It is necessary; at least if the workaround resides in ohci1394.
>>Otherwise the controller won't come back after a suspend/ resume
>>cycle. (See Rob's post from February,
>>http://marc.theaimsgroup.com/?m=110786495210243 ) Maybe there is
>>another way to do that if the workaround was moved to pci/quirks.c.
>
> Having it all in the driver probably makes the most sense if we have to
> have code there anyway. Otherwise future users may have to check both
> places if things break again in another configuration.

No, better move it to pci/quirks.c. I may be wrong (again) but I think
everything of the workaround can be done there.

>>Furthermore, everything which belongs to the workaround should IMO be
>>enclosed by #ifdef SOME_SENSIBLE_MACRO. This avoids kernel bloat for
>>any target which is surely not a Toshiba laptop. Rob used an #if
>>defined(__i386__).
>
> Checks against the compiler defined arch are usually wrong since users
> could be cross compiling, and I'd like to avoid an ifdef altogether.

Once the workaround is in pci/quirks.c, a single #ifdef would suffice
(if it is still of any benefit there).
--
Stefan Richter
-=====-=-=-= =-=- ==---
http://arcgraph.de/sr/

2005-10-24 21:09:15

by Ivan Kokshaysky

[permalink] [raw]
Subject: Re: new PCI quirk for Toshiba Satellite?

On Mon, Oct 24, 2005 at 08:21:06PM +0200, Stefan Richter wrote:
> Once the workaround is in pci/quirks.c, a single #ifdef would suffice
> (if it is still of any benefit there).

Such an obviously i386-specific quirk should go into
arch/i386/pci/fixup.c.

Ivan.